Re: [RFC PATCH] fpga: remove module reference counting from core components

Xu Yilun
2023-10-30T08:33:45Z

On Fri, Oct 27, 2023 at 05:29:27PM +0200, Marco Pagani wrote:
> Remove unnecessary module reference counting from the core components
> of the subsystem. Low-level driver modules cannot be removed before
> core modules since they use their exported symbols.

Could you help show the code for this conclusion?

This is different from what I remember, a module cannot be removed when
its exported symbols are being used by other modules. IOW, the core
modules cannot be removed when there exist related low-level driver
modules. But the low-level driver modules could be removed freely
without other protecting mechanism.

>
> For more context, refer to this thread:
> https://lore.kernel.org/linux-fpga/ZS6hhlvjUcqyv8zL@yilunxu-OptiPlex-7050
>
> Other changes:
>
> In __fpga_bridge_get(): do a (missing ?) get_device() and bind the

I think get_device() is in (of)_fpga_bridge_get() -> class_find_device()
and put_device() correspond to it.

But the code style here is rather misleading, the put_device() should be
moved out to (of)_fpga_bridge_get().

> image to the bridge only after the mutex has been acquired.

This is good to me.

>
> In __fpga_mgr_get(): do a get_device(). Currently, get_device() is
> called when allocating an image in fpga_image_info_alloc().
> However, since there are still two (of_)fpga_mgr_get() functions
> exposed by the core, I think they should behave as expected.

Same as fpga bridge.

>
> In fpga_region_get() / fpga_region_put(): call get_device() before
> acquiring the mutex and put_device() after having released the mutex
> to avoid races.

Could you help elaborate more about the race?

Thanks,
Yilun

>
> Fixes: 654ba4cc0f3e ("fpga manager: ensure lifetime with of_fpga_mgr_get")
> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> ---
> drivers/fpga/fpga-bridge.c | 24 +++++++-----------------
> drivers/fpga/fpga-mgr.c | 8 +-------
> drivers/fpga/fpga-region.c | 14 ++++----------
> 3 files changed, 12 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
> index a024be2b84e2..3bcc9c9849c5 100644
> --- a/drivers/fpga/fpga-bridge.c
> +++ b/drivers/fpga/fpga-bridge.c
> @@ -58,30 +58,21 @@ EXPORT_SYMBOL_GPL(fpga_bridge_disable);
> static struct fpga_bridge *__fpga_bridge_get(struct device *dev,
> struct fpga_image_info *info)
> {
> - struct fpga_bridge *bridge;
> - int ret = -ENODEV;
> -
> - bridge = to_fpga_bridge(dev);
> + struct fpga_bridge *bridge = to_fpga_bridge(dev);
>
> - bridge->info = info;
> + get_device(dev);
>
> if (!mutex_trylock(&bridge->mutex)) {
> - ret = -EBUSY;
> - goto err_dev;
> + dev_dbg(dev, "%s: FPGA Bridge already in use\n", __func__);
> + put_device(dev);
> + return ERR_PTR(-EBUSY);
> }
>
> - if (!try_module_get(dev->parent->driver->owner))
> - goto err_ll_mod;
> + bridge->info = info;
>
> dev_dbg(&bridge->dev, "get\n");
>
> return bridge;
> -
> -err_ll_mod:
> - mutex_unlock(&bridge->mutex);
> -err_dev:
> - put_device(dev);
> - return ERR_PTR(ret);
> }
>
> /**
> @@ -93,7 +84,7 @@ static struct fpga_bridge *__fpga_bridge_get(struct device *dev,
> * Return:
> * * fpga_bridge struct pointer if successful.
> * * -EBUSY if someone already has a reference to the bridge.
> - * * -ENODEV if @np is not an FPGA Bridge or can't take parent driver refcount.
> + * * -ENODEV if @np is not an FPGA Bridge.
> */
> struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
> struct fpga_image_info *info)
> @@ -146,7 +137,6 @@ void fpga_bridge_put(struct fpga_bridge *bridge)
> dev_dbg(&bridge->dev, "put\n");
>
> bridge->info = NULL;
> - module_put(bridge->dev.parent->driver->owner);
> mutex_unlock(&bridge->mutex);
> put_device(&bridge->dev);
> }
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 06651389c592..6c355eafd18f 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -670,14 +670,9 @@ static struct fpga_manager *__fpga_mgr_get(struct device *dev)
>
> mgr = to_fpga_manager(dev);
>
> - if (!try_module_get(dev->parent->driver->owner))
> - goto err_dev;
> + get_device(&mgr->dev);
>
> return mgr;
> -
> -err_dev:
> - put_device(dev);
> - return ERR_PTR(-ENODEV);
> }
>
> static int fpga_mgr_dev_match(struct device *dev, const void *data)
> @@ -727,7 +722,6 @@ EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
> */
> void fpga_mgr_put(struct fpga_manager *mgr)
> {
> - module_put(mgr->dev.parent->driver->owner);
> put_device(&mgr->dev);
> }
> EXPORT_SYMBOL_GPL(fpga_mgr_put);
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index b364a929425c..c299956cafdc 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -41,22 +41,17 @@ EXPORT_SYMBOL_GPL(fpga_region_class_find);
> * Return:
> * * fpga_region struct if successful.
> * * -EBUSY if someone already has a reference to the region.
> - * * -ENODEV if can't take parent driver module refcount.
> */
> static struct fpga_region *fpga_region_get(struct fpga_region *region)
> {
> struct device *dev = &region->dev;
>
> + get_device(dev);
> +
> if (!mutex_trylock(&region->mutex)) {
> dev_dbg(dev, "%s: FPGA Region already in use\n", __func__);
> - return ERR_PTR(-EBUSY);
> - }
> -
> - get_device(dev);
> - if (!try_module_get(dev->parent->driver->owner)) {
> put_device(dev);
> - mutex_unlock(&region->mutex);
> - return ERR_PTR(-ENODEV);
> + return ERR_PTR(-EBUSY);
> }
>
> dev_dbg(dev, "get\n");
> @@ -75,9 +70,8 @@ static void fpga_region_put(struct fpga_region *region)
>
> dev_dbg(dev, "put\n");
>
> - module_put(dev->parent->driver->owner);
> - put_device(dev);
> mutex_unlock(&region->mutex);
> + put_device(dev);
> }
>
> /**
> --
> 2.41.0
>