On Thu, Jun 14, 2018 at 11:33:00AM -0500, Alan Tull wrote:
> 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(®ion->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.
Yes, it's correct. I can add some comments here in the next version.
Thanks
Hao
>
> Alan
>
> >> +
> >> + put_device(®ion->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;
> >> +}
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
