lkml.org 
[lkml]   [2021]   [Feb]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH V3 XRT Alveo 04/18] fpga: xrt: xrt-lib platform driver manager
    From
    Date

    On 2/17/21 10:40 PM, Lizhi Hou wrote:
    > xrt-lib kernel module infrastructure code to register and manage all
    > leaf driver modules.
    >
    > Signed-off-by: Sonal Santan <sonal.santan@xilinx.com>
    > Signed-off-by: Max Zhen <max.zhen@xilinx.com>
    > Signed-off-by: Lizhi Hou <lizhih@xilinx.com>
    > ---
    > drivers/fpga/xrt/lib/main.c | 274 ++++++++++++++++++++++++++++++++++++
    > drivers/fpga/xrt/lib/main.h | 17 +++
    > 2 files changed, 291 insertions(+)
    > create mode 100644 drivers/fpga/xrt/lib/main.c
    > create mode 100644 drivers/fpga/xrt/lib/main.h

    Not sure if 'main' is a good base name for something going into a lib.

    >
    > diff --git a/drivers/fpga/xrt/lib/main.c b/drivers/fpga/xrt/lib/main.c
    > new file mode 100644
    > index 000000000000..36fb62710843
    > --- /dev/null
    > +++ b/drivers/fpga/xrt/lib/main.c
    > @@ -0,0 +1,274 @@
    > +// SPDX-License-Identifier: GPL-2.0
    > +/*
    > + * Driver for Xilinx Alveo FPGA Support
    > + *
    > + * Copyright (C) 2020-2021 Xilinx, Inc.
    > + *
    > + * Authors:
    > + * Cheng Zhen <maxz@xilinx.com>
    > + */
    > +
    > +#include <linux/module.h>
    > +#include "xleaf.h"
    > +#include "xroot.h"
    > +#include "main.h"
    > +
    > +#define XRT_IPLIB_MODULE_NAME "xrt-lib"
    > +#define XRT_IPLIB_MODULE_VERSION "4.0.0"
    > +#define XRT_MAX_DEVICE_NODES 128
    > +#define XRT_DRVNAME(drv) ((drv)->driver.name)
    > +
    > +/*
    > + * Subdev driver is known by ID to others. We map the ID to it's
    by it's ID
    > + * struct platform_driver, which contains it's binding name and driver/file ops.
    > + * We also map it to the endpoint name in DTB as well, if it's different
    > + * than the driver's binding name.
    > + */
    > +struct xrt_drv_map {
    > + struct list_head list;
    > + enum xrt_subdev_id id;
    > + struct platform_driver *drv;
    > + struct xrt_subdev_endpoints *eps;
    > + struct ida ida; /* manage driver instance and char dev minor */
    > +};
    > +
    > +static DEFINE_MUTEX(xrt_lib_lock); /* global lock protecting xrt_drv_maps list */
    > +static LIST_HEAD(xrt_drv_maps);
    > +struct class *xrt_class;
    > +
    > +static inline struct xrt_subdev_drvdata *
    > +xrt_drv_map2drvdata(struct xrt_drv_map *map)
    > +{
    > + return (struct xrt_subdev_drvdata *)map->drv->id_table[0].driver_data;
    > +}
    > +
    > +static struct xrt_drv_map *
    > +xrt_drv_find_map_by_id_nolock(enum xrt_subdev_id id)

    name could be by convention

    __xrt_drv_find_map_id

    > +{
    > + const struct list_head *ptr;
    > +
    > + list_for_each(ptr, &xrt_drv_maps) {
    > + struct xrt_drv_map *tmap = list_entry(ptr, struct xrt_drv_map, list);
    > +
    > + if (tmap->id == id)
    > + return tmap;
    > + }
    > + return NULL;
    > +}
    > +
    > +static struct xrt_drv_map *
    > +xrt_drv_find_map_by_id(enum xrt_subdev_id id)
    > +{
    > + struct xrt_drv_map *map;
    > +
    > + mutex_lock(&xrt_lib_lock);
    > + map = xrt_drv_find_map_by_id_nolock(id);
    > + mutex_unlock(&xrt_lib_lock);
    > + /*
    > + * map should remain valid even after lock is dropped since a registered
    even after the lock
    > + * driver should only be unregistered when driver module is being unloaded,
    > + * which means that the driver should not be used by then.
    > + */
    > + return map;
    > +}
    > +
    > +static int xrt_drv_register_driver(struct xrt_drv_map *map)
    > +{
    > + struct xrt_subdev_drvdata *drvdata;
    > + int rc = 0;
    > + const char *drvname = XRT_DRVNAME(map->drv);
    > +
    > + rc = platform_driver_register(map->drv);
    > + if (rc) {
    > + pr_err("register %s platform driver failed\n", drvname);
    > + return rc;
    > + }
    > +
    > + drvdata = xrt_drv_map2drvdata(map);
    > + if (drvdata) {
    > + /* Initialize dev_t for char dev node. */
    > + if (xleaf_devnode_enabled(drvdata)) {
    > + rc = alloc_chrdev_region(&drvdata->xsd_file_ops.xsf_dev_t, 0,
    > + XRT_MAX_DEVICE_NODES, drvname);
    > + if (rc) {
    > + platform_driver_unregister(map->drv);
    > + pr_err("failed to alloc dev minor for %s: %d\n", drvname, rc);
    > + return rc;
    > + }
    > + } else {
    > + drvdata->xsd_file_ops.xsf_dev_t = (dev_t)-1;
    > + }
    > + }
    > +
    > + ida_init(&map->ida);
    > +
    > + pr_info("%s registered successfully\n", drvname);
    > +
    > + return 0;
    > +}
    > +
    > +static void xrt_drv_unregister_driver(struct xrt_drv_map *map)
    > +{
    > + const char *drvname = XRT_DRVNAME(map->drv);
    > + struct xrt_subdev_drvdata *drvdata;
    > +
    > + ida_destroy(&map->ida);
    > +
    > + drvdata = xrt_drv_map2drvdata(map);
    > + if (drvdata && drvdata->xsd_file_ops.xsf_dev_t != (dev_t)-1) {
    > + unregister_chrdev_region(drvdata->xsd_file_ops.xsf_dev_t,
    > + XRT_MAX_DEVICE_NODES);
    > + }
    > +
    > + platform_driver_unregister(map->drv);
    > +
    > + pr_info("%s unregistered successfully\n", drvname);
    > +}
    > +
    > +int xleaf_register_driver(enum xrt_subdev_id id,
    > + struct platform_driver *drv,
    > + struct xrt_subdev_endpoints *eps)
    > +{
    > + struct xrt_drv_map *map;
    > +
    > + mutex_lock(&xrt_lib_lock);

    Trying to minimize length of lock being held.

    Could holding this lock be split or the alloc moved above ?

    > +
    > + map = xrt_drv_find_map_by_id_nolock(id);
    > + if (map) {
    > + mutex_unlock(&xrt_lib_lock);
    > + pr_err("Id %d already has a registered driver, 0x%p\n",
    > + id, map->drv);
    > + return -EEXIST;
    > + }
    > +
    > + map = vzalloc(sizeof(*map));

    general issue

    map is small, so kzalloc

    > + if (!map) {
    > + mutex_unlock(&xrt_lib_lock);
    > + return -ENOMEM;
    > + }
    > + map->id = id;
    > + map->drv = drv;
    > + map->eps = eps;
    > +
    > + xrt_drv_register_driver(map);

    xrt_drv_register_driver failure is unhandled.

    This is the only time xrt_drv_register_driver is called, consider expanding the function here and removing the call.

    > +
    > + list_add(&map->list, &xrt_drv_maps);
    > +
    > + mutex_unlock(&xrt_lib_lock);
    > +
    > + return 0;
    > +}
    > +EXPORT_SYMBOL_GPL(xleaf_register_driver);
    > +
    > +void xleaf_unregister_driver(enum xrt_subdev_id id)
    > +{
    > + struct xrt_drv_map *map;
    > +
    > + mutex_lock(&xrt_lib_lock);
    > +
    > + map = xrt_drv_find_map_by_id_nolock(id);
    > + if (!map) {
    > + mutex_unlock(&xrt_lib_lock);
    > + pr_err("Id %d has no registered driver\n", id);
    > + return;
    > + }
    > +
    > + list_del(&map->list);
    > +
    > + mutex_unlock(&xrt_lib_lock);
    > +
    > + xrt_drv_unregister_driver(map);
    > + vfree(map);
    > +}
    > +EXPORT_SYMBOL_GPL(xleaf_unregister_driver);
    > +
    > +const char *xrt_drv_name(enum xrt_subdev_id id)
    > +{
    > + struct xrt_drv_map *map = xrt_drv_find_map_by_id(id);
    > +
    > + if (map)
    > + return XRT_DRVNAME(map->drv);
    > + return NULL;
    > +}
    > +
    > +int xrt_drv_get_instance(enum xrt_subdev_id id)
    > +{
    > + struct xrt_drv_map *map = xrt_drv_find_map_by_id(id);
    > +
    > + return ida_alloc_range(&map->ida, 0, XRT_MAX_DEVICE_NODES, GFP_KERNEL);
    > +}
    > +
    > +void xrt_drv_put_instance(enum xrt_subdev_id id, int instance)
    > +{
    > + struct xrt_drv_map *map = xrt_drv_find_map_by_id(id);
    > +
    > + ida_free(&map->ida, instance);
    > +}
    > +
    > +struct xrt_subdev_endpoints *xrt_drv_get_endpoints(enum xrt_subdev_id id)
    > +{
    > + struct xrt_drv_map *map = xrt_drv_find_map_by_id(id);
    > + struct xrt_subdev_endpoints *eps;
    > +
    > + eps = map ? map->eps : NULL;
    > + return eps;
    > +}
    > +
    > +/* Leaf driver's module init/fini callbacks. */

    These constructor/destructor calls needs to be more dynamic.

    calls are made even if there are no subdevices to go with the id's.

    Also this list can not grow.  How would a new id be added by a module ?

    > +static void (*leaf_init_fini_cbs[])(bool) = {
    > + group_leaf_init_fini,
    > + vsec_leaf_init_fini,
    > + devctl_leaf_init_fini,
    > + axigate_leaf_init_fini,
    > + icap_leaf_init_fini,
    > + calib_leaf_init_fini,
    > + clkfreq_leaf_init_fini,
    > + clock_leaf_init_fini,
    > + ucs_leaf_init_fini,
    > +};
    > +
    > +static __init int xrt_lib_init(void)
    > +{
    > + int i;
    > +
    > + xrt_class = class_create(THIS_MODULE, XRT_IPLIB_MODULE_NAME);
    > + if (IS_ERR(xrt_class))
    > + return PTR_ERR(xrt_class);
    > +
    > + for (i = 0; i < ARRAY_SIZE(leaf_init_fini_cbs); i++)
    > + leaf_init_fini_cbs[i](true);
    > + return 0;
    > +}
    > +
    > +static __exit void xrt_lib_fini(void)
    > +{
    > + struct xrt_drv_map *map;
    > + int i;
    > +
    > + for (i = 0; i < ARRAY_SIZE(leaf_init_fini_cbs); i++)
    > + leaf_init_fini_cbs[i](false);
    > +
    > + mutex_lock(&xrt_lib_lock);
    > +
    > + while (!list_empty(&xrt_drv_maps)) {
    > + map = list_first_entry_or_null(&xrt_drv_maps, struct xrt_drv_map, list);
    > + pr_err("Unloading module with %s still registered\n", XRT_DRVNAME(map->drv));
    > + list_del(&map->list);
    > + mutex_unlock(&xrt_lib_lock);
    > + xrt_drv_unregister_driver(map);
    > + vfree(map);
    > + mutex_lock(&xrt_lib_lock);
    > + }
    > +
    > + mutex_unlock(&xrt_lib_lock);
    > +
    > + class_destroy(xrt_class);
    > +}
    > +
    > +module_init(xrt_lib_init);
    > +module_exit(xrt_lib_fini);
    > +
    > +MODULE_VERSION(XRT_IPLIB_MODULE_VERSION);
    > +MODULE_AUTHOR("XRT Team <runtime@xilinx.com>");
    > +MODULE_DESCRIPTION("Xilinx Alveo IP Lib driver");
    > +MODULE_LICENSE("GPL v2");
    > diff --git a/drivers/fpga/xrt/lib/main.h b/drivers/fpga/xrt/lib/main.h
    > new file mode 100644
    > index 000000000000..f3bfc87ee614
    > --- /dev/null
    > +++ b/drivers/fpga/xrt/lib/main.h
    > @@ -0,0 +1,17 @@
    > +/* SPDX-License-Identifier: GPL-2.0 */
    > +/*
    > + * Copyright (C) 2020-2021 Xilinx, Inc.
    > + *
    > + * Authors:
    > + * Cheng Zhen <maxz@xilinx.com>
    > + */
    > +
    > +#ifndef _XRT_MAIN_H_
    > +#define _XRT_MAIN_H_
    > +
    > +const char *xrt_drv_name(enum xrt_subdev_id id);

    To be self contained, the header defining enum xrt_subdev_id should be included.

    This is subdev_id.h which comes in with patch 6

    A dependency on a future patch breaks bisectablity.

    It may make sense to collect these small headers into a single large header for the ip infra lib and bring them all in this patch.

    Tom

    > +int xrt_drv_get_instance(enum xrt_subdev_id id);
    > +void xrt_drv_put_instance(enum xrt_subdev_id id, int instance);
    > +struct xrt_subdev_endpoints *xrt_drv_get_endpoints(enum xrt_subdev_id id);
    > +
    > +#endif /* _XRT_MAIN_H_ */

    \
     
     \ /
      Last update: 2021-02-22 16:08    [W:4.426 / U:0.664 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site