Re: [PATCH V4 XRT Alveo 06/20] fpga: xrt: char dev node helper functions

From: Max Zhen <max.zhen@xilinx.com>
To: Tom Rix <trix@redhat.com>, Lizhi Hou <lizhi.hou@xilinx.com>,
	<linux-kernel@vger.kernel.org>
Cc: <linux-fpga@vger.kernel.org>, <sonal.santan@xilinx.com>,
	<yliu@xilinx.com>, <michal.simek@xilinx.com>,
	<stefanos@xilinx.com>, <devicetree@vger.kernel.org>,
	<mdf@kernel.org>, <robh@kernel.org>,
	Max Zhen <max.zhen@xilinx.com>
Subject: Re: [PATCH V4 XRT Alveo 06/20] fpga: xrt: char dev node helper functions
Date: Tue, 6 Apr 2021 09:29:44 -0700	[thread overview]
Message-ID: <d557363e-6ef8-2f7b-12d9-be16874ae27a@xilinx.com> (raw)
In-Reply-To: <755d73b9-1e23-c2e9-1502-15df14b9ed35@redhat.com>

On 3/30/21 6:45 AM, Tom Rix wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> It is unclear from the changelog if this new patch was split from an existing patch or new content.
>
> the file ops seem to come from mgmnt/main.c, which call what could be file ops here.  why is this complicated redirection needed ?

This is part of infra code which is split from subdev.c, not from 
mgmt/main.c. Sorry about the confusion. We further split infra code to 
avoid having one huge patch for review.

>
> On 3/23/21 10:29 PM, Lizhi Hou wrote:
>> Helper functions for char device node creation / removal for platform
>> drivers. This is part of platform driver infrastructure.
>>
>> Signed-off-by: Sonal Santan <sonal.santan@xilinx.com>
>> Signed-off-by: Max Zhen <max.zhen@xilinx.com>
>> Signed-off-by: Lizhi Hou <lizhi.hou@xilinx.com>
>> ---
>>   drivers/fpga/xrt/lib/cdev.c | 232 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 232 insertions(+)
>>   create mode 100644 drivers/fpga/xrt/lib/cdev.c
>>
>> diff --git a/drivers/fpga/xrt/lib/cdev.c b/drivers/fpga/xrt/lib/cdev.c
>> new file mode 100644
>> index 000000000000..38efd24b6e10
>> --- /dev/null
>> +++ b/drivers/fpga/xrt/lib/cdev.c
>> @@ -0,0 +1,232 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Xilinx Alveo FPGA device node helper functions.
>> + *
>> + * Copyright (C) 2020-2021 Xilinx, Inc.
>> + *
>> + * Authors:
>> + *   Cheng Zhen <maxz@xilinx.com>
>> + */
>> +
>> +#include "xleaf.h"
>> +
>> +extern struct class *xrt_class;
>> +
>> +#define XRT_CDEV_DIR         "xfpga"
> maybe "xrt_fpga" or just "xrt"

Will change it to just "xrt", yes.

>> +#define INODE2PDATA(inode)   \
>> +     container_of((inode)->i_cdev, struct xrt_subdev_platdata, xsp_cdev)
>> +#define INODE2PDEV(inode)    \
>> +     to_platform_device(kobj_to_dev((inode)->i_cdev->kobj.parent))
>> +#define CDEV_NAME(sysdev)    (strchr((sysdev)->kobj.name, '!') + 1)
>> +
>> +/* Allow it to be accessed from cdev. */
>> +static void xleaf_devnode_allowed(struct platform_device *pdev)
>> +{
>> +     struct xrt_subdev_platdata *pdata = DEV_PDATA(pdev);
>> +
>> +     /* Allow new opens. */
>> +     mutex_lock(&pdata->xsp_devnode_lock);
>> +     pdata->xsp_devnode_online = true;
>> +     mutex_unlock(&pdata->xsp_devnode_lock);
>> +}
>> +
>> +/* Turn off access from cdev and wait for all existing user to go away. */
>> +static int xleaf_devnode_disallowed(struct platform_device *pdev)
>> +{
>> +     int ret = 0;
>> +     struct xrt_subdev_platdata *pdata = DEV_PDATA(pdev);
>> +
>> +     mutex_lock(&pdata->xsp_devnode_lock);
>> +
>> +     /* Prevent new opens. */
>> +     pdata->xsp_devnode_online = false;
>> +     /* Wait for existing user to close. */
>> +     while (!ret && pdata->xsp_devnode_ref) {
>> +             int rc;
>> +
>> +             mutex_unlock(&pdata->xsp_devnode_lock);
>> +             rc = wait_for_completion_killable(&pdata->xsp_devnode_comp);
>> +             mutex_lock(&pdata->xsp_devnode_lock);
>> +
>> +             if (rc == -ERESTARTSYS) {
>> +                     /* Restore online state. */
>> +                     pdata->xsp_devnode_online = true;
>> +                     xrt_err(pdev, "%s is in use, ref=%d",
>> +                             CDEV_NAME(pdata->xsp_sysdev),
>> +                             pdata->xsp_devnode_ref);
>> +                     ret = -EBUSY;
>> +             }
>> +     }
>> +
>> +     mutex_unlock(&pdata->xsp_devnode_lock);
>> +
>> +     return ret;
>> +}
>> +
>> +static struct platform_device *
>> +__xleaf_devnode_open(struct inode *inode, bool excl)
>> +{
>> +     struct xrt_subdev_platdata *pdata = INODE2PDATA(inode);
>> +     struct platform_device *pdev = INODE2PDEV(inode);
>> +     bool opened = false;
>> +
>> +     mutex_lock(&pdata->xsp_devnode_lock);
>> +
>> +     if (pdata->xsp_devnode_online) {
>> +             if (excl && pdata->xsp_devnode_ref) {
>> +                     xrt_err(pdev, "%s has already been opened exclusively",
>> +                             CDEV_NAME(pdata->xsp_sysdev));
>> +             } else if (!excl && pdata->xsp_devnode_excl) {
>> +                     xrt_err(pdev, "%s has been opened exclusively",
>> +                             CDEV_NAME(pdata->xsp_sysdev));
>> +             } else {
>> +                     pdata->xsp_devnode_ref++;
>> +                     pdata->xsp_devnode_excl = excl;
>> +                     opened = true;
>> +                     xrt_info(pdev, "opened %s, ref=%d",
>> +                              CDEV_NAME(pdata->xsp_sysdev),
>> +                              pdata->xsp_devnode_ref);
>> +             }
>> +     } else {
>> +             xrt_err(pdev, "%s is offline", CDEV_NAME(pdata->xsp_sysdev));
>> +     }
>> +
>> +     mutex_unlock(&pdata->xsp_devnode_lock);
>> +
>> +     pdev = opened ? pdev : NULL;
>> +     return pdev;
>> +}
>> +
>> +struct platform_device *
>> +xleaf_devnode_open_excl(struct inode *inode)
>> +{
>> +     return __xleaf_devnode_open(inode, true);
>> +}
> This function is unused, remove.

This is part of infra's API for leaf driver to call. The caller has been 
removed from this initial patch set to reduce the size of the patch. You 
will see it in the next follow up patch once we finish reviewing current 
one.

>> +
>> +struct platform_device *
>> +xleaf_devnode_open(struct inode *inode)
>> +{
>> +     return __xleaf_devnode_open(inode, false);
>> +}
>> +EXPORT_SYMBOL_GPL(xleaf_devnode_open);
> does this really need to be exported ?

Yes, this is part of infra's API in xrt-lib.ko. One of the caller is in 
xrt-mgmt.ko.

>> +
>> +void xleaf_devnode_close(struct inode *inode)
>> +{
>> +     struct xrt_subdev_platdata *pdata = INODE2PDATA(inode);
>> +     struct platform_device *pdev = INODE2PDEV(inode);
>> +     bool notify = false;
>> +
>> +     mutex_lock(&pdata->xsp_devnode_lock);
>> +
>> +     WARN_ON(pdata->xsp_devnode_ref == 0);
>> +     pdata->xsp_devnode_ref--;
>> +     if (pdata->xsp_devnode_ref == 0) {
>> +             pdata->xsp_devnode_excl = false;
>> +             notify = true;
>> +     }
>> +     if (notify) {
>> +             xrt_info(pdev, "closed %s, ref=%d",
>> +                      CDEV_NAME(pdata->xsp_sysdev), pdata->xsp_devnode_ref);
> xsp_devnode_ref will always be 0, so no need to report it.

Will remove.

>> +     } else {
>> +             xrt_info(pdev, "closed %s, notifying waiter",
>> +                      CDEV_NAME(pdata->xsp_sysdev));
>> +     }
>> +
>> +     mutex_unlock(&pdata->xsp_devnode_lock);
>> +
>> +     if (notify)
>> +             complete(&pdata->xsp_devnode_comp);
>> +}
>> +EXPORT_SYMBOL_GPL(xleaf_devnode_close);
>> +
>> +static inline enum xrt_subdev_file_mode
>> +devnode_mode(struct xrt_subdev_drvdata *drvdata)
>> +{
>> +     return drvdata->xsd_file_ops.xsf_mode;
>> +}
>> +
>> +int xleaf_devnode_create(struct platform_device *pdev, const char *file_name,
>> +                      const char *inst_name)
>> +{
>> +     struct xrt_subdev_drvdata *drvdata = DEV_DRVDATA(pdev);
>> +     struct xrt_subdev_file_ops *fops = &drvdata->xsd_file_ops;
>> +     struct xrt_subdev_platdata *pdata = DEV_PDATA(pdev);
>> +     struct cdev *cdevp;
>> +     struct device *sysdev;
>> +     int ret = 0;
>> +     char fname[256];
>> +
>> +     mutex_init(&pdata->xsp_devnode_lock);
>> +     init_completion(&pdata->xsp_devnode_comp);
>> +
>> +     cdevp = &DEV_PDATA(pdev)->xsp_cdev;
>> +     cdev_init(cdevp, &fops->xsf_ops);
>> +     cdevp->owner = fops->xsf_ops.owner;
>> +     cdevp->dev = MKDEV(MAJOR(fops->xsf_dev_t), pdev->id);
>> +
>> +     /*
>> +      * Set pdev as parent of cdev so that when pdev (and its platform
>> +      * data) will not be freed when cdev is not freed.
>> +      */
>> +     cdev_set_parent(cdevp, &DEV(pdev)->kobj);
>> +
>> +     ret = cdev_add(cdevp, cdevp->dev, 1);
>> +     if (ret) {
>> +             xrt_err(pdev, "failed to add cdev: %d", ret);
>> +             goto failed;
>> +     }
>> +     if (!file_name)
>> +             file_name = pdev->name;
>> +     if (!inst_name) {
>> +             if (devnode_mode(drvdata) == XRT_SUBDEV_FILE_MULTI_INST) {
>> +                     snprintf(fname, sizeof(fname), "%s/%s/%s.%u",
>> +                              XRT_CDEV_DIR, DEV_PDATA(pdev)->xsp_root_name,
>> +                              file_name, pdev->id);
>> +             } else {
>> +                     snprintf(fname, sizeof(fname), "%s/%s/%s",
>> +                              XRT_CDEV_DIR, DEV_PDATA(pdev)->xsp_root_name,
>> +                              file_name);
>> +             }
>> +     } else {
>> +             snprintf(fname, sizeof(fname), "%s/%s/%s.%s", XRT_CDEV_DIR,
>> +                      DEV_PDATA(pdev)->xsp_root_name, file_name, inst_name);
>> +     }
>> +     sysdev = device_create(xrt_class, NULL, cdevp->dev, NULL, "%s", fname);
>> +     if (IS_ERR(sysdev)) {
>> +             ret = PTR_ERR(sysdev);
>> +             xrt_err(pdev, "failed to create device node: %d", ret);
>> +             goto failed_cdev_add;
>> +     }
>> +     pdata->xsp_sysdev = sysdev;
>> +
>> +     xleaf_devnode_allowed(pdev);
>> +
>> +     xrt_info(pdev, "created (%d, %d): /dev/%s",
>> +              MAJOR(cdevp->dev), pdev->id, fname);
>> +     return 0;
>> +
>> +failed_cdev_add:
>> +     cdev_del(cdevp);
>> +failed:
>> +     cdevp->owner = NULL;
>> +     return ret;
>> +}
>> +
>> +int xleaf_devnode_destroy(struct platform_device *pdev)
>> +{
>> +     struct xrt_subdev_platdata *pdata = DEV_PDATA(pdev);
>> +     struct cdev *cdevp = &pdata->xsp_cdev;
>> +     dev_t dev = cdevp->dev;
>> +     int rc;
>> +
>> +     rc = xleaf_devnode_disallowed(pdev);
>> +     if (rc)
>> +             return rc;
> Failure of one leaf to be destroyed is not handled well.
>
> could a able to destroy check be done over the whole group ?

Yes, this is not handled properly. Handling this type of error during 
cleaning up code path is not very clean. I think it might make more 
sense to just change the code so that xleaf_devnode_disallowed() will 
not fail. It will instead just waiting for existing user to quit. This 
is just like the remove callback of a platform device. It does not 
return error.

Or maybe there is a better way to handle the error like this?

Thanks,

Max

>
> Tom
>
>> +
>> +     xrt_info(pdev, "removed (%d, %d): /dev/%s/%s", MAJOR(dev), MINOR(dev),
>> +              XRT_CDEV_DIR, CDEV_NAME(pdata->xsp_sysdev));
>> +     device_destroy(xrt_class, cdevp->dev);
>> +     pdata->xsp_sysdev = NULL;
>> +     cdev_del(cdevp);
>> +     return 0;
>> +}