| Message ID | 20210218064019.29189-5-lizhih@xilinx.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | XRT Alveo driver overview | expand |
Lizhi, On Wed, Feb 17, 2021 at 10:40:05PM -0800, 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 > > 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 > + * 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) > +{ > + 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); list_for_each_entry, maybe? > + > + 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 > + * 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); Is this not xrt_info() then? > + > + 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); > + > + 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)); > + if (!map) { > + mutex_unlock(&xrt_lib_lock); > + return -ENOMEM; > + } > + map->id = id; > + map->drv = drv; > + map->eps = eps; > + > + xrt_drv_register_driver(map); > + > + 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. */ > +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); > +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_ */ > -- > 2.18.4 > This smells like it should be a bus to me... - Moritz
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_ */
On Mon, Feb 22, 2021 at 07:05:29AM -0800, Tom Rix wrote: > > 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. Please add the headers when you use them, do *not* do header only commits. It's perfectly fine (and desirable) to add things over time to a header as you use them. Note *each* commit must compile and work standing on its own, so yes as Tom pointed out, do not depend on future (later commit) files. > > 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_ */ > - Moritz
Hi Moritz, On 2/21/21 12:39 PM, Moritz Fischer 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. > > > Lizhi, > > On Wed, Feb 17, 2021 at 10:40:05PM -0800, 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 >> >> 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 >> + * 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) >> +{ >> + 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); > list_for_each_entry, maybe? Yes! >> + >> + 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 >> + * 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); > Is this not xrt_info() then? xrt_info() is meant for leaf driver to call where struct platform_device is available. We are initializing the drivers here, so can't call xrt_info(). >> + >> + 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); >> + >> + 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)); >> + if (!map) { >> + mutex_unlock(&xrt_lib_lock); >> + return -ENOMEM; >> + } >> + map->id = id; >> + map->drv = drv; >> + map->eps = eps; >> + >> + xrt_drv_register_driver(map); >> + >> + 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. */ >> +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); >> +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_ */ >> -- >> 2.18.4 >> > This smells like it should be a bus to me... This is discussed in another email thread. I will not add more comment here. Thanks, Max > > - Moritz
On 2/22/21 7:05 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. > > > 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. These files are the main file for xrt-lib.ko. I'm not sure what name you prefer, but I've changed them to lib-drv.[c|h]. Let me know if you don't like them... > >> 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 Sure. >> + * 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 Sure. > >> +{ >> + 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 Sure. >> + * 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 ? We don't want to do memory allocation unless the mapping can't be found. So, finding the mapping and allocating memory for new entry is done as one atomic operation. The code path here is far from being in critical path, so holding a lock here for some longer time should not be a problem. I'd like to keep the code straightforward and easy to follow by holding the lock from start to end. > >> + >> + 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 Sure. >> + 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. Sure. > > This is the only time xrt_drv_register_driver is called, consider expanding the function here and removing the call. Expanding the function here will result in > 3 level of indentation, which is bad. I'd like to keep it as a function. >> + >> + 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 ? We do not support dynamically adding drivers/IDs. All drivers needs to be added statically after thoroughly tested. We do not intend to build an open infrastructure to support random hardware and driver anyway. > >> +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. Yes, we will reconsider where to put these headers in next patch set. Thanks, Max > > 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_ */
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 + * 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) +{ + 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 + * 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); + + 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)); + if (!map) { + mutex_unlock(&xrt_lib_lock); + return -ENOMEM; + } + map->id = id; + map->drv = drv; + map->eps = eps; + + xrt_drv_register_driver(map); + + 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. */ +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); +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_ */