Re: [PATCH v6 17/29] fpga: dfl: fme: add partial reconfiguration sub feature support

[Alan Tull]

On Wed, Jun 13, 2018 at 8:07 PM, Moritz Fischer <mdf@xxxxxxxxxx> wrote:

Hellooo,

This probably could use a comment, please see below.

> Hi,
>
> quick question for Alan inline below.
>

>> +static int fme_pr(struct platform_device *pdev, unsigned long arg)
>> +{
>> +     struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
>> +     void __user *argp = (void __user *)arg;
>> +     struct dfl_fpga_fme_port_pr port_pr;
>> +     struct fpga_image_info *info;
>> +     struct fpga_region *region;
>> +     void __iomem *fme_hdr;
>> +     struct dfl_fme *fme;
>> +     unsigned long minsz;
>> +     void *buf = NULL;
>> +     int ret = 0;
>> +     u64 v;
>> +
>> +     minsz = offsetofend(struct dfl_fpga_fme_port_pr, buffer_address);
>> +
>> +     if (copy_from_user(&port_pr, argp, minsz))
>> +             return -EFAULT;
>> +
>> +     if (port_pr.argsz < minsz || port_pr.flags)
>> +             return -EINVAL;
>> +
>> +     if (!IS_ALIGNED(port_pr.buffer_size, 4))
>> +             return -EINVAL;
>> +
>> +     /* get fme header region */
>> +     fme_hdr = dfl_get_feature_ioaddr_by_id(&pdev->dev,
>> +                                            FME_FEATURE_ID_HEADER);
>> +
>> +     /* check port id */
>> +     v = readq(fme_hdr + FME_HDR_CAP);
>> +     if (port_pr.port_id >= FIELD_GET(FME_CAP_NUM_PORTS, v)) {
>> +             dev_dbg(&pdev->dev, "port number more than maximum\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     if (!access_ok(VERIFY_READ,
>> +                    (void __user *)(unsigned long)port_pr.buffer_address,
>> +                    port_pr.buffer_size))
>> +             return -EFAULT;
>> +
>> +     buf = vmalloc(port_pr.buffer_size);
>> +     if (!buf)
>> +             return -ENOMEM;
>> +
>> +     if (copy_from_user(buf,
>> +                        (void __user *)(unsigned long)port_pr.buffer_address,
>> +                        port_pr.buffer_size)) {
>> +             ret = -EFAULT;
>> +             goto free_exit;
>> +     }
>> +
>> +     /* prepare fpga_image_info for PR */
>> +     info = fpga_image_info_alloc(&pdev->dev);
>> +     if (!info) {
>> +             ret = -ENOMEM;
>> +             goto free_exit;
>> +     }
>> +
>> +     info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
>> +
>> +     mutex_lock(&pdata->lock);
>> +     fme = dfl_fpga_pdata_get_private(pdata);
>> +     /* fme device has been unregistered. */
>> +     if (!fme) {
>> +             ret = -EINVAL;
>> +             goto unlock_exit;
>> +     }
>> +
>> +     region = dfl_fme_region_find(fme, port_pr.port_id);
>> +     if (!region) {
>> +             ret = -EINVAL;
>> +             goto unlock_exit;
>> +     }
>> +
>> +     fpga_image_info_free(region->info);
>> +
>> +     info->buf = buf;
>> +     info->count = port_pr.buffer_size;
>> +     info->region_id = port_pr.port_id;
>> +     region->info = info;
>> +
>> +     ret = fpga_region_program_fpga(region);
>> +
>> +     if (region->get_bridges)
>> +             fpga_bridges_put(&region->bridge_list);
>
> I'm wondering if this should not be done as part of
> fpga_region_program_fpga() (It's not at the moment).
>
> Alan, am I missing something obvious? :)

It is up to the caller of  fpga_region_program_fpga to put the bridges
before programming again.  That prevents something other than the
caller from reprogramming a partial region or messing with the bridges
until the caller puts the bridges.

For example, for DT support, applying an overlay calls
fpga_region_program_fpga() which gets the bridges, disables them, does
the programming, and reenables the bridges.  Then the bridges are
held, preventing anything else from disabling the bridges until the
overlay is removed.  This was important in the DT case since the
overlay could include child nodes for hardware in the FPGA.   Allowing
anyone else to play with the bridges while those drivers were
expecting hardware access could be bad.

I can't remember for certain, but I think this code has to put the
bridges here because this patch set allows userspace to reset the PR
region's logic by disabling/reenabling the bridge to clear things out
between acceleration runs (which Hao promises is OK to do, it's
documented elsewhere in the pachset).  It probably could use a comment
right here to explain why the bridges are put here so future
generations know not to break this.

Alan

>> +
>> +     put_device(&region->dev);
>> +unlock_exit:
>> +     mutex_unlock(&pdata->lock);
>> +free_exit:
>> +     vfree(buf);
>> +     if (copy_to_user((void __user *)arg, &port_pr, minsz))
>> +             return -EFAULT;
>> +
>> +     return ret;
>> +}