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@xxxxxxxxxx> > --- > 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 = ®ion->dev; > > + get_device(dev); > + > if (!mutex_trylock(®ion->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(®ion->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(®ion->mutex); > + put_device(dev); > } > > /** > -- > 2.41.0 >
