From: Moritz Fischer <mdf@kernel.org>
To: Tom Rix <trix@redhat.com>
Cc: Lizhi Hou <lizhi.hou@xilinx.com>,
linux-kernel@vger.kernel.org, Lizhi Hou <lizhih@xilinx.com>,
linux-fpga@vger.kernel.org, maxz@xilinx.com,
sonal.santan@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 V3 XRT Alveo 04/18] fpga: xrt: xrt-lib platform driver manager
Date: Mon, 22 Feb 2021 19:35:39 -0800 [thread overview]
Message-ID: <YDR4CyHu2tum0zIJ@epycbox.lan> (raw)
In-Reply-To: <b93fb3ad-bbde-81db-d448-72fb8049f323@redhat.com>
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
next prev parent reply other threads:[~2021-02-23 3:36 UTC|newest]
Thread overview: 86+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-18 6:40 [PATCH V3 XRT Alveo 00/18] XRT Alveo driver overview Lizhi Hou
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 01/18] Documentation: fpga: Add a document describing XRT Alveo drivers Lizhi Hou
2021-02-19 22:26 ` Tom Rix
2021-03-01 6:48 ` Sonal Santan
2021-03-06 17:19 ` Moritz Fischer
2021-03-08 20:12 ` Sonal Santan
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 02/18] fpga: xrt: driver metadata helper functions Lizhi Hou
2021-02-20 17:07 ` Tom Rix
2021-02-23 6:05 ` Lizhi Hou
2021-02-23 1:23 ` Fernando Pacheco
2021-02-25 20:27 ` Lizhi Hou
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 03/18] fpga: xrt: xclbin file " Lizhi Hou
2021-02-21 17:12 ` Tom Rix
2021-02-21 18:33 ` Moritz Fischer
2021-03-06 1:13 ` Lizhi Hou
2021-02-26 21:23 ` Lizhi Hou
2021-02-28 16:54 ` Tom Rix
2021-03-02 0:25 ` Lizhi Hou
2021-03-02 15:14 ` Moritz Fischer
2021-03-04 18:53 ` Lizhi Hou
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 04/18] fpga: xrt: xrt-lib platform driver manager Lizhi Hou
2021-02-21 20:39 ` Moritz Fischer
2021-03-01 20:34 ` Max Zhen
2021-02-22 15:05 ` Tom Rix
2021-02-23 3:35 ` Moritz Fischer [this message]
2021-03-03 17:20 ` Max Zhen
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 05/18] fpga: xrt: group platform driver Lizhi Hou
2021-02-22 18:50 ` Tom Rix
2021-02-26 21:57 ` Max Zhen
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 06/18] fpga: xrt: platform driver infrastructure Lizhi Hou
2021-02-25 21:59 ` Tom Rix
[not found] ` <13e9a311-2d04-ba65-3ed2-f9f1834c37de@xilinx.com>
2021-03-08 20:36 ` Max Zhen
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 07/18] fpga: xrt: management physical function driver (root) Lizhi Hou
2021-02-26 15:01 ` Tom Rix
2021-02-26 17:56 ` Moritz Fischer
2021-03-16 20:29 ` Max Zhen
2021-03-17 21:08 ` Tom Rix
2021-03-18 0:44 ` Max Zhen
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 08/18] fpga: xrt: main platform driver for management function device Lizhi Hou
2021-02-26 17:22 ` Tom Rix
2021-03-16 21:23 ` Lizhi Hou
2021-03-17 21:12 ` Tom Rix
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 09/18] fpga: xrt: fpga-mgr and region implementation for xclbin download Lizhi Hou
2021-02-28 16:36 ` Tom Rix
2021-03-04 17:50 ` Lizhi Hou
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 10/18] fpga: xrt: VSEC platform driver Lizhi Hou
2021-03-01 19:01 ` Tom Rix
2021-03-05 19:58 ` Lizhi Hou
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 11/18] fpga: xrt: UCS " Lizhi Hou
2021-03-02 16:09 ` Tom Rix
2021-03-10 20:24 ` Lizhi Hou
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 12/18] fpga: xrt: ICAP " Lizhi Hou
2021-02-21 20:24 ` Moritz Fischer
2021-03-02 18:26 ` Lizhi Hou
2021-03-03 15:12 ` Tom Rix
2021-03-17 20:56 ` Lizhi Hou
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 13/18] fpga: xrt: devctl " Lizhi Hou
2021-03-04 13:39 ` Tom Rix
2021-03-16 23:54 ` Lizhi Hou
2021-03-17 21:16 ` Tom Rix
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 14/18] fpga: xrt: clock " Lizhi Hou
2021-03-05 15:23 ` Tom Rix
2021-03-11 0:12 ` Lizhi Hou
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 15/18] fpga: xrt: clock frequence counter " Lizhi Hou
2021-03-06 15:25 ` Tom Rix
2021-03-12 23:43 ` Lizhi Hou
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 16/18] fpga: xrt: DDR calibration " Lizhi Hou
2021-02-21 20:21 ` Moritz Fischer
2021-03-06 15:34 ` Tom Rix
2021-03-13 0:45 ` Lizhi Hou
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 17/18] fpga: xrt: partition isolation " Lizhi Hou
2021-02-21 20:36 ` Moritz Fischer
2021-03-16 20:38 ` Lizhi Hou
2021-03-06 15:54 ` Tom Rix
2021-03-13 6:53 ` Lizhi Hou
2021-02-18 6:40 ` [PATCH V3 XRT Alveo 18/18] fpga: xrt: Kconfig and Makefile updates for XRT drivers Lizhi Hou
2021-02-18 9:02 ` kernel test robot
2021-02-21 14:57 ` Tom Rix
2021-02-21 18:39 ` Moritz Fischer
2021-02-28 20:52 ` Sonal Santan
2021-02-18 13:52 ` [PATCH V3 XRT Alveo 00/18] XRT Alveo driver overview Tom Rix
2021-02-19 5:15 ` Lizhi Hou
2021-02-21 20:43 ` Moritz Fischer
2021-03-01 18:29 ` Lizhi Hou
2021-03-03 6:49 ` Joe Perches
2021-03-03 23:15 ` Moritz Fischer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YDR4CyHu2tum0zIJ@epycbox.lan \
--to=mdf@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=linux-fpga@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lizhi.hou@xilinx.com \
--cc=lizhih@xilinx.com \
--cc=max.zhen@xilinx.com \
--cc=maxz@xilinx.com \
--cc=michal.simek@xilinx.com \
--cc=robh@kernel.org \
--cc=sonal.santan@xilinx.com \
--cc=stefanos@xilinx.com \
--cc=trix@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).