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

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

[Xu Yilun]

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
> 

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

[Marco Pagani]

On 2023-10-30 09:32, Xu Yilun wrote:
> 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.
>

My understanding was that we wanted to remove module reference counting
from the fpga core and ease it from the responsibility of preventing
low-level driver modules from being unloaded. 

If we want to keep reference counting in the fpga core, we could add a
struct module *owner field in the struct fpga_manager_ops (and others
core *_ops) so that the low-level driver can set it to THIS_MODULE.
In this way, we can later use it in fpga_mgr_register() to bump up the
refcount of the low-level driver module by calling
try_module_get(mgr->mops->owner) directly when it registers the manager.
Finally, fpga_mgr_unregister() would call module_put(mgr->mops->owner)
to allow unloading the low-level driver module.

In this way, it would no longer be necessary to call try_module_get()
in fpga_mrg_get() since we could use a kref (included in the struct
fpga_manager) to do refcounting for the in-kernel API users. Only when
the kref reaches zero fpga_mgr_unregister() would succeed and put the
low-level driver module.

I think this approach would be safer since it would avoid the crash
that can currently happen if the low-level driver module is removed
right when executing try_module_get() in fpga_mrg_get(). The possible
caveat is that it would be required to call fpga_mgr_unregister()
before being able to remove the low-level driver module.

>>
>> 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.
>

You are right. I missed that one.
 
> But the code style here is rather misleading, the put_device() should be
> moved out to (of)_fpga_bridge_get().
>

Right, I'll improve the (of)_fpga_bridge_get() style for the next version.

>> 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?
> 

I accidentally misused the word race. My concern was that memory might
be released after the last put_device(), causing mutex_unlock() to be
called on a mutex that does not exist anymore. It should not happen
for the moment since the region does not use devres, but I think it
still makes the code more brittle.

> 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
>>
> 

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

[Xu Yilun]

On Fri, Nov 03, 2023 at 09:31:02PM +0100, Marco Pagani wrote:
> 
> 
> On 2023-10-30 09:32, Xu Yilun wrote:
> > 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.
> >
> 
> My understanding was that we wanted to remove module reference counting
> from the fpga core and ease it from the responsibility of preventing
> low-level driver modules from being unloaded. 

FPGA core needs to prevent low-level driver module unloading sometimes,
e.g. when region reprograming is in progress. That's why we get fpga
region driver modules & bridge modules in fpga_region_program_fpga().

But we try best to get them only necessary. Blindly geting them all the
time results in no way to unload all modules (core & low level modules).

> 
> If we want to keep reference counting in the fpga core, we could add a
> struct module *owner field in the struct fpga_manager_ops (and others
> core *_ops) so that the low-level driver can set it to THIS_MODULE.
> In this way, we can later use it in fpga_mgr_register() to bump up the

Yes, we should pass the module owner in fpga_mgr_register(), but could
not bump up its refcount at once.

> refcount of the low-level driver module by calling
> try_module_get(mgr->mops->owner) directly when it registers the manager.
> Finally, fpga_mgr_unregister() would call module_put(mgr->mops->owner)
> to allow unloading the low-level driver module.

As mentioned above, that makes problem. Most of the low level driver
modules call fpga_mgr_unregister() on module_exit(), but bumping up
their module refcount prevents module_exit() been executed. That came
out to be a dead lock.

> 
> In this way, it would no longer be necessary to call try_module_get()
> in fpga_mrg_get() since we could use a kref (included in the struct
> fpga_manager) to do refcounting for the in-kernel API users. Only when
> the kref reaches zero fpga_mgr_unregister() would succeed and put the
> low-level driver module.
> 
> I think this approach would be safer since it would avoid the crash
> that can currently happen if the low-level driver module is removed
> right when executing try_module_get() in fpga_mrg_get(). The possible
> caveat is that it would be required to call fpga_mgr_unregister()
> before being able to remove the low-level driver module.
> 
> >>
> >> 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.
> >
> 
> You are right. I missed that one.
>  
> > But the code style here is rather misleading, the put_device() should be
> > moved out to (of)_fpga_bridge_get().
> >
> 
> Right, I'll improve the (of)_fpga_bridge_get() style for the next version.
> 
> >> 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?
> > 
> 
> I accidentally misused the word race. My concern was that memory might
> be released after the last put_device(), causing mutex_unlock() to be
> called on a mutex that does not exist anymore. It should not happen
> for the moment since the region does not use devres, but I think it
> still makes the code more brittle.

It makes sense.

But I dislike the mutex itself. The purpose is to exclusively grab the
device, but a mutex is too much heavy for this. The lock/unlock of mutex
scattered in different functions is also an uncommon style. Maybe some
atomic count should be enough.

Thanks,
Yilun


> 
> > 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
> >>
> > 
> 

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

[Greg Kroah-Hartman]

On Wed, Nov 08, 2023 at 11:52:52PM +0800, Xu Yilun wrote:
> > >>
> > >> 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.

Why do you need another reference count with a lock?  You already have
that with the calls to get/put_device().

> > > Could you help elaborate more about the race?
> > > 
> > 
> > I accidentally misused the word race. My concern was that memory might
> > be released after the last put_device(), causing mutex_unlock() to be
> > called on a mutex that does not exist anymore. It should not happen
> > for the moment since the region does not use devres, but I think it
> > still makes the code more brittle.
> 
> It makes sense.
> 
> But I dislike the mutex itself. The purpose is to exclusively grab the
> device, but a mutex is too much heavy for this.

Why "heavy"?  Is this a fast-path?  Have you measured it?

> The lock/unlock of mutex
> scattered in different functions is also an uncommon style. Maybe some
> atomic count should be enough.

Don't make a fake lock with an atomic variable, use real locks if you
need it.

Or don't even care about module unloading at all!  Why does it matter?
It can never happen without explicit intervention and it's something
that a lot of the time, just will cause problems.  Don't do a lot of
extra work for something that doesn't matter.

thanks,

greg k-h

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

[Xu Yilun]

On Wed, Nov 08, 2023 at 05:20:53PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Nov 08, 2023 at 11:52:52PM +0800, Xu Yilun wrote:
> > > >>
> > > >> 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.
> 
> Why do you need another reference count with a lock?  You already have
> that with the calls to get/put_device().

The low-level driver module could still be possibly unloaded at the same
time, if so, when FPGA core run some callbacks provided by low-level driver
module, its referenced page of code is unmapped...

I actually got this idea from this mail thread:

https://lore.kernel.org/all/20231010003746.GN800259@ZenIV/

And I see get/put of "struct file_operations.owner" in many framework
code for this purpose, to ensure no fops->read/write/ioctl() is ongoing
when module unloading.

> 
> > > > Could you help elaborate more about the race?
> > > > 
> > > 
> > > I accidentally misused the word race. My concern was that memory might
> > > be released after the last put_device(), causing mutex_unlock() to be
> > > called on a mutex that does not exist anymore. It should not happen
> > > for the moment since the region does not use devres, but I think it
> > > still makes the code more brittle.
> > 
> > It makes sense.
> > 
> > But I dislike the mutex itself. The purpose is to exclusively grab the
> > device, but a mutex is too much heavy for this.
> 
> Why "heavy"?  Is this a fast-path?  Have you measured it?

It's not a fast-path. But I didn't make it clear, see below...

> 
> > The lock/unlock of mutex
> > scattered in different functions is also an uncommon style. Maybe some
> > atomic count should be enough.
> 
> Don't make a fake lock with an atomic variable, use real locks if you
> need it.

I mean, here it doesn't not looks like a "locking" senario, although it
works.

The purpose here is to declare a device state, which says the device is
exclusively used by a user, no other user is allowed. But usually we use
mutex to protect against critical code blocks, not to represent a long
live device state.

I'm still OK for the existing mutex usage as it doesn't break anything
and if we don't want change much.

> 
> Or don't even care about module unloading at all!  Why does it matter?
> It can never happen without explicit intervention and it's something
> that a lot of the time, just will cause problems.  Don't do a lot of
> extra work for something that doesn't matter.

mm.. as mentioned above some fundamental subsystems do care about
module unloading, and I tend to keep it same way. But mm... I'm OK to
make things easier if you insist.

Thanks,
Yilun

> 
> thanks,
> 
> greg k-h

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

[Greg Kroah-Hartman]

On Thu, Nov 09, 2023 at 01:07:42PM +0800, Xu Yilun wrote:
> On Wed, Nov 08, 2023 at 05:20:53PM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Nov 08, 2023 at 11:52:52PM +0800, Xu Yilun wrote:
> > > > >>
> > > > >> 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.
> > 
> > Why do you need another reference count with a lock?  You already have
> > that with the calls to get/put_device().
> 
> The low-level driver module could still be possibly unloaded at the same
> time, if so, when FPGA core run some callbacks provided by low-level driver
> module, its referenced page of code is unmapped...

Then something is designed wrong here, the unloading of the low-level
driver should remove the access to the device itself.  Perhaps fix that?

> > > The lock/unlock of mutex
> > > scattered in different functions is also an uncommon style. Maybe some
> > > atomic count should be enough.
> > 
> > Don't make a fake lock with an atomic variable, use real locks if you
> > need it.
> 
> I mean, here it doesn't not looks like a "locking" senario, although it
> works.
> 
> The purpose here is to declare a device state, which says the device is
> exclusively used by a user, no other user is allowed. But usually we use
> mutex to protect against critical code blocks, not to represent a long
> live device state.
> 
> I'm still OK for the existing mutex usage as it doesn't break anything
> and if we don't want change much.
> 
> > 
> > Or don't even care about module unloading at all!  Why does it matter?
> > It can never happen without explicit intervention and it's something
> > that a lot of the time, just will cause problems.  Don't do a lot of
> > extra work for something that doesn't matter.
> 
> mm.. as mentioned above some fundamental subsystems do care about
> module unloading, and I tend to keep it same way. But mm... I'm OK to
> make things easier if you insist.

You can care, but my point being that don't make it very complex and
slow just for something that no one will ever do in normal device
operation.

thanks,

greg k-h

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

[Xu Yilun]

On Thu, Nov 09, 2023 at 06:27:24AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Nov 09, 2023 at 01:07:42PM +0800, Xu Yilun wrote:
> > On Wed, Nov 08, 2023 at 05:20:53PM +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Nov 08, 2023 at 11:52:52PM +0800, Xu Yilun wrote:
> > > > > >>
> > > > > >> 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.
> > > 
> > > Why do you need another reference count with a lock?  You already have
> > > that with the calls to get/put_device().
> > 
> > The low-level driver module could still be possibly unloaded at the same
> > time, if so, when FPGA core run some callbacks provided by low-level driver
> > module, its referenced page of code is unmapped...
> 
> Then something is designed wrong here, the unloading of the low-level
> driver should remove the access to the device itself.  Perhaps fix that?

Actually the low-level driver module on its own has no way to garantee its
own code page of callbacks not accessed. It *is* accessing its code page
when it tries (to release) any protection.

Core code must help, and something like file_operations.owner is an
effective way.

Thanks,
Yilun

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

[Greg Kroah-Hartman]

On Thu, Nov 09, 2023 at 03:16:26PM +0800, Xu Yilun wrote:
> On Thu, Nov 09, 2023 at 06:27:24AM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Nov 09, 2023 at 01:07:42PM +0800, Xu Yilun wrote:
> > > On Wed, Nov 08, 2023 at 05:20:53PM +0100, Greg Kroah-Hartman wrote:
> > > > On Wed, Nov 08, 2023 at 11:52:52PM +0800, Xu Yilun wrote:
> > > > > > >>
> > > > > > >> 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.
> > > > 
> > > > Why do you need another reference count with a lock?  You already have
> > > > that with the calls to get/put_device().
> > > 
> > > The low-level driver module could still be possibly unloaded at the same
> > > time, if so, when FPGA core run some callbacks provided by low-level driver
> > > module, its referenced page of code is unmapped...
> > 
> > Then something is designed wrong here, the unloading of the low-level
> > driver should remove the access to the device itself.  Perhaps fix that?
> 
> Actually the low-level driver module on its own has no way to garantee its
> own code page of callbacks not accessed. It *is* accessing its code page
> when it tries (to release) any protection.

It is not up to the low-level driver to do this, it's up to the code
that calls into it (i.e. the fpga core code) to handle the proper
reference counting.

> Core code must help, and something like file_operations.owner is an
> effective way.

Yes, that should be all that you need.

thanks,

greg k-h

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

[Marco Pagani]

On 2023-11-08 17:20, Greg Kroah-Hartman wrote:
> On Wed, Nov 08, 2023 at 11:52:52PM +0800, Xu Yilun wrote:
>>>>>
>>>>> 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.
> 
> Why do you need another reference count with a lock?  You already have
> that with the calls to get/put_device().
>

My understanding is that the lock is there not for reference counting
but to prevent concurrent reprogramming of the region by in-kernel API
consumers.

>>>> Could you help elaborate more about the race?
>>>>
>>>
>>> I accidentally misused the word race. My concern was that memory might
>>> be released after the last put_device(), causing mutex_unlock() to be
>>> called on a mutex that does not exist anymore. It should not happen
>>> for the moment since the region does not use devres, but I think it
>>> still makes the code more brittle.
>>
>> It makes sense.
>>

Thanks,
Marco

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

[Marco Pagani]

On 2023-11-08 16:52, Xu Yilun wrote:
> On Fri, Nov 03, 2023 at 09:31:02PM +0100, Marco Pagani wrote:
>>
>>
>> On 2023-10-30 09:32, Xu Yilun wrote:
>>> 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.
>>>
>>
>> My understanding was that we wanted to remove module reference counting
>> from the fpga core and ease it from the responsibility of preventing
>> low-level driver modules from being unloaded. 
> 
> FPGA core needs to prevent low-level driver module unloading sometimes,
> e.g. when region reprograming is in progress. That's why we get fpga
> region driver modules & bridge modules in fpga_region_program_fpga().
> 
> But we try best to get them only necessary. Blindly geting them all the
> time results in no way to unload all modules (core & low level modules).
> 
>>
>> If we want to keep reference counting in the fpga core, we could add a
>> struct module *owner field in the struct fpga_manager_ops (and others
>> core *_ops) so that the low-level driver can set it to THIS_MODULE.
>> In this way, we can later use it in fpga_mgr_register() to bump up the
> 
> Yes, we should pass the module owner in fpga_mgr_register(), but could
> not bump up its refcount at once.
> 
>> refcount of the low-level driver module by calling
>> try_module_get(mgr->mops->owner) directly when it registers the manager.
>> Finally, fpga_mgr_unregister() would call module_put(mgr->mops->owner)
>> to allow unloading the low-level driver module.
> 
> As mentioned above, that makes problem. Most of the low level driver
> modules call fpga_mgr_unregister() on module_exit(), but bumping up
> their module refcount prevents module_exit() been executed. That came
> out to be a dead lock.
>

Initially, I considered calling try_module_get(mgr->mops->owner)
in fpga_mgr_get(). But then, the new kernel-doc description of
try_module_get() (1) made me question the safety of that approach.
My concern is that the low-level driver could be removed right when
someone is calling fpga_mgr_get() and hasn't yet reached
try_module_get(mgr->mops->owner). In that case, the struct mops
(along with the entire low-level driver module) and the manager dev
would "disappear" under the feet of fpga_mgr_get().

(1) 557aafac1153 ("kernel/module: add documentation for try_module_get()")
 
>>
>> In this way, it would no longer be necessary to call try_module_get()
>> in fpga_mrg_get() since we could use a kref (included in the struct
>> fpga_manager) to do refcounting for the in-kernel API users. Only when
>> the kref reaches zero fpga_mgr_unregister() would succeed and put the
>> low-level driver module.
>>
>> I think this approach would be safer since it would avoid the crash
>> that can currently happen if the low-level driver module is removed
>> right when executing try_module_get() in fpga_mrg_get(). The possible
>> caveat is that it would be required to call fpga_mgr_unregister()
>> before being able to remove the low-level driver module.
>>


[...]

Thanks,
Marco

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

[Greg Kroah-Hartman]

On Fri, Nov 10, 2023 at 11:58:37PM +0100, Marco Pagani wrote:
> 
> 
> On 2023-11-08 16:52, Xu Yilun wrote:
> > On Fri, Nov 03, 2023 at 09:31:02PM +0100, Marco Pagani wrote:
> >>
> >>
> >> On 2023-10-30 09:32, Xu Yilun wrote:
> >>> 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.
> >>>
> >>
> >> My understanding was that we wanted to remove module reference counting
> >> from the fpga core and ease it from the responsibility of preventing
> >> low-level driver modules from being unloaded. 
> > 
> > FPGA core needs to prevent low-level driver module unloading sometimes,
> > e.g. when region reprograming is in progress. That's why we get fpga
> > region driver modules & bridge modules in fpga_region_program_fpga().
> > 
> > But we try best to get them only necessary. Blindly geting them all the
> > time results in no way to unload all modules (core & low level modules).
> > 
> >>
> >> If we want to keep reference counting in the fpga core, we could add a
> >> struct module *owner field in the struct fpga_manager_ops (and others
> >> core *_ops) so that the low-level driver can set it to THIS_MODULE.
> >> In this way, we can later use it in fpga_mgr_register() to bump up the
> > 
> > Yes, we should pass the module owner in fpga_mgr_register(), but could
> > not bump up its refcount at once.
> > 
> >> refcount of the low-level driver module by calling
> >> try_module_get(mgr->mops->owner) directly when it registers the manager.
> >> Finally, fpga_mgr_unregister() would call module_put(mgr->mops->owner)
> >> to allow unloading the low-level driver module.
> > 
> > As mentioned above, that makes problem. Most of the low level driver
> > modules call fpga_mgr_unregister() on module_exit(), but bumping up
> > their module refcount prevents module_exit() been executed. That came
> > out to be a dead lock.
> >
> 
> Initially, I considered calling try_module_get(mgr->mops->owner)
> in fpga_mgr_get(). But then, the new kernel-doc description of
> try_module_get() (1) made me question the safety of that approach.
> My concern is that the low-level driver could be removed right when
> someone is calling fpga_mgr_get() and hasn't yet reached
> try_module_get(mgr->mops->owner).

Can that really happen?  This shouldn't be a real issue, but normally
this should only be needed on an open() like call, don't tie a module
reference to a device reference, those are two different things.

> In that case, the struct mops
> (along with the entire low-level driver module) and the manager dev
> would "disappear" under the feet of fpga_mgr_get().

You should have a lock for handling this anyway, this feels odd that
it's a problem, but I haven't looked at the code in a long time.

try it and see!

greg k-h

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

[Xu Yilun]

On Fri, Nov 10, 2023 at 11:58:37PM +0100, Marco Pagani wrote:
> 
> 
> On 2023-11-08 16:52, Xu Yilun wrote:
> > On Fri, Nov 03, 2023 at 09:31:02PM +0100, Marco Pagani wrote:
> >>
> >>
> >> On 2023-10-30 09:32, Xu Yilun wrote:
> >>> 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.
> >>>
> >>
> >> My understanding was that we wanted to remove module reference counting
> >> from the fpga core and ease it from the responsibility of preventing
> >> low-level driver modules from being unloaded. 
> > 
> > FPGA core needs to prevent low-level driver module unloading sometimes,
> > e.g. when region reprograming is in progress. That's why we get fpga
> > region driver modules & bridge modules in fpga_region_program_fpga().
> > 
> > But we try best to get them only necessary. Blindly geting them all the
> > time results in no way to unload all modules (core & low level modules).
> > 
> >>
> >> If we want to keep reference counting in the fpga core, we could add a
> >> struct module *owner field in the struct fpga_manager_ops (and others
> >> core *_ops) so that the low-level driver can set it to THIS_MODULE.
> >> In this way, we can later use it in fpga_mgr_register() to bump up the
> > 
> > Yes, we should pass the module owner in fpga_mgr_register(), but could
> > not bump up its refcount at once.
> > 
> >> refcount of the low-level driver module by calling
> >> try_module_get(mgr->mops->owner) directly when it registers the manager.
> >> Finally, fpga_mgr_unregister() would call module_put(mgr->mops->owner)
> >> to allow unloading the low-level driver module.
> > 
> > As mentioned above, that makes problem. Most of the low level driver
> > modules call fpga_mgr_unregister() on module_exit(), but bumping up
> > their module refcount prevents module_exit() been executed. That came
> > out to be a dead lock.
> >
> 
> Initially, I considered calling try_module_get(mgr->mops->owner)
> in fpga_mgr_get(). But then, the new kernel-doc description of
> try_module_get() (1) made me question the safety of that approach.
> My concern is that the low-level driver could be removed right when
> someone is calling fpga_mgr_get() and hasn't yet reached
> try_module_get(mgr->mops->owner). In that case, the struct mops
> (along with the entire low-level driver module) and the manager dev
> would "disappear" under the feet of fpga_mgr_get().

I don't get what's the problem. fpga_mgr_get() would first of all
look for mgr_dev via class_find_device(), if low-level module is
unloaded, then you cannot find the mgr_dev and gracefully error out.

If class_find_device() succeed, mgr_dev got a reference and won't
disappear. Finally we may still found module removed when
try_module_get(), but should be another graceful error out.

Am I missing anything?

Thanks,
Yilun

> 
> (1) 557aafac1153 ("kernel/module: add documentation for try_module_get()")
>  

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

[Marco Pagani]

On 2023-11-14 07:53, Xu Yilun wrote:
> On Fri, Nov 10, 2023 at 11:58:37PM +0100, Marco Pagani wrote:
>>
>>
>> On 2023-11-08 16:52, Xu Yilun wrote:
>>> On Fri, Nov 03, 2023 at 09:31:02PM +0100, Marco Pagani wrote:
>>>>
>>>>
>>>> On 2023-10-30 09:32, Xu Yilun wrote:
>>>>> 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.
>>>>>
>>>>
>>>> My understanding was that we wanted to remove module reference counting
>>>> from the fpga core and ease it from the responsibility of preventing
>>>> low-level driver modules from being unloaded. 
>>>
>>> FPGA core needs to prevent low-level driver module unloading sometimes,
>>> e.g. when region reprograming is in progress. That's why we get fpga
>>> region driver modules & bridge modules in fpga_region_program_fpga().
>>>
>>> But we try best to get them only necessary. Blindly geting them all the
>>> time results in no way to unload all modules (core & low level modules).
>>>
>>>>
>>>> If we want to keep reference counting in the fpga core, we could add a
>>>> struct module *owner field in the struct fpga_manager_ops (and others
>>>> core *_ops) so that the low-level driver can set it to THIS_MODULE.
>>>> In this way, we can later use it in fpga_mgr_register() to bump up the
>>>
>>> Yes, we should pass the module owner in fpga_mgr_register(), but could
>>> not bump up its refcount at once.
>>>
>>>> refcount of the low-level driver module by calling
>>>> try_module_get(mgr->mops->owner) directly when it registers the manager.
>>>> Finally, fpga_mgr_unregister() would call module_put(mgr->mops->owner)
>>>> to allow unloading the low-level driver module.
>>>
>>> As mentioned above, that makes problem. Most of the low level driver
>>> modules call fpga_mgr_unregister() on module_exit(), but bumping up
>>> their module refcount prevents module_exit() been executed. That came
>>> out to be a dead lock.
>>>
>>
>> Initially, I considered calling try_module_get(mgr->mops->owner)
>> in fpga_mgr_get(). But then, the new kernel-doc description of
>> try_module_get() (1) made me question the safety of that approach.
>> My concern is that the low-level driver could be removed right when
>> someone is calling fpga_mgr_get() and hasn't yet reached
>> try_module_get(mgr->mops->owner). In that case, the struct mops
>> (along with the entire low-level driver module) and the manager dev
>> would "disappear" under the feet of fpga_mgr_get().
> 
> I don't get what's the problem. fpga_mgr_get() would first of all
> look for mgr_dev via class_find_device(), if low-level module is
> unloaded, then you cannot find the mgr_dev and gracefully error out.
> 
> If class_find_device() succeed, mgr_dev got a reference and won't
> disappear. Finally we may still found module removed when
> try_module_get(), but should be another graceful error out.
> 
> Am I missing anything?
> 

My concern is: suppose that you successfully got the mgr dev from
class_find_device(), and now you are in __fpga_mgr_get(), right before
try_module_get(mgr->mops->owner). At that point, you get descheduled,
and while you are not running, someone unloads the low-level driver
module that ends its life by calling fpga_mgr_unregister(). When you
wake up, you find yourself with a reference to a device that does not
exist anymore, trying to get a module that does not exist anymore
through one of its symbols (module *owner in mops).

Greg suggested checking if this can really happen and eventually
protecting fpga_mgr_get() and fpga_mgr_unregister() with a lock for
mops (if I understood correctly). In that case, considering the same
scenario described above:

fpga_mgr_get() gets the mops lock and the mgr dev but is suspended
before calling try_module_get().

Someone unloads the low-level driver, delete_modules progresses
(the module's recount hasn't yet been incremented) but blocks while
calling fpga_mgr_unregister() since fpga_mgr_get() is holding the lock.

fpga_mgr_get() resumes and tries to get the module through one of its
symbols (mgr->mops->owner). The module's memory hasn't yet been freed
(delete_modules is blocked), and the refcount is zero, so
try_module_get() fails safely, and we can put the mgr dev that is
still present since fpga_mgr_unregister() is blocked.

fpga_mgr_unregister() resumes and unregisters the mgr dev.

I'm still thinking about the possible implications. On the one hand,
it looks safe in this case, but on the other hand, it feels brittle.
In my understanding, the root problem is that there will always be a
critical window (when you have taken a reference to the device but
not yet to the low-level driver module) when unloading the module
could be potentially unsafe depending on the current implementation
and the preemption model.

I still feel that it would be simpler and safer if we could bump
up the refcount during fpga_mgr_register() and maybe have a sysfs
attribute to unlock the low-level driver (if no one has taken the
mgr dev refcount). That way, it would be safer by design since the
refcount will be bumped up right during the module load procedure,
and we could guarantee that the lifetime of the mgr device is
entirely contained in the lifetime of the low-level driver module.

>>
>> (1) 557aafac1153 ("kernel/module: add documentation for try_module_get()")
>>  
 
Thanks,
Marco

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

[Greg Kroah-Hartman]

On Fri, Nov 17, 2023 at 10:58:59PM +0100, Marco Pagani wrote:
> 
> 
> On 2023-11-14 07:53, Xu Yilun wrote:
> > On Fri, Nov 10, 2023 at 11:58:37PM +0100, Marco Pagani wrote:
> >>
> >>
> >> On 2023-11-08 16:52, Xu Yilun wrote:
> >>> On Fri, Nov 03, 2023 at 09:31:02PM +0100, Marco Pagani wrote:
> >>>>
> >>>>
> >>>> On 2023-10-30 09:32, Xu Yilun wrote:
> >>>>> 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.
> >>>>>
> >>>>
> >>>> My understanding was that we wanted to remove module reference counting
> >>>> from the fpga core and ease it from the responsibility of preventing
> >>>> low-level driver modules from being unloaded. 
> >>>
> >>> FPGA core needs to prevent low-level driver module unloading sometimes,
> >>> e.g. when region reprograming is in progress. That's why we get fpga
> >>> region driver modules & bridge modules in fpga_region_program_fpga().
> >>>
> >>> But we try best to get them only necessary. Blindly geting them all the
> >>> time results in no way to unload all modules (core & low level modules).
> >>>
> >>>>
> >>>> If we want to keep reference counting in the fpga core, we could add a
> >>>> struct module *owner field in the struct fpga_manager_ops (and others
> >>>> core *_ops) so that the low-level driver can set it to THIS_MODULE.
> >>>> In this way, we can later use it in fpga_mgr_register() to bump up the
> >>>
> >>> Yes, we should pass the module owner in fpga_mgr_register(), but could
> >>> not bump up its refcount at once.
> >>>
> >>>> refcount of the low-level driver module by calling
> >>>> try_module_get(mgr->mops->owner) directly when it registers the manager.
> >>>> Finally, fpga_mgr_unregister() would call module_put(mgr->mops->owner)
> >>>> to allow unloading the low-level driver module.
> >>>
> >>> As mentioned above, that makes problem. Most of the low level driver
> >>> modules call fpga_mgr_unregister() on module_exit(), but bumping up
> >>> their module refcount prevents module_exit() been executed. That came
> >>> out to be a dead lock.
> >>>
> >>
> >> Initially, I considered calling try_module_get(mgr->mops->owner)
> >> in fpga_mgr_get(). But then, the new kernel-doc description of
> >> try_module_get() (1) made me question the safety of that approach.
> >> My concern is that the low-level driver could be removed right when
> >> someone is calling fpga_mgr_get() and hasn't yet reached
> >> try_module_get(mgr->mops->owner). In that case, the struct mops
> >> (along with the entire low-level driver module) and the manager dev
> >> would "disappear" under the feet of fpga_mgr_get().
> > 
> > I don't get what's the problem. fpga_mgr_get() would first of all
> > look for mgr_dev via class_find_device(), if low-level module is
> > unloaded, then you cannot find the mgr_dev and gracefully error out.
> > 
> > If class_find_device() succeed, mgr_dev got a reference and won't
> > disappear. Finally we may still found module removed when
> > try_module_get(), but should be another graceful error out.
> > 
> > Am I missing anything?
> > 
> 
> My concern is: suppose that you successfully got the mgr dev from
> class_find_device(), and now you are in __fpga_mgr_get(), right before
> try_module_get(mgr->mops->owner). At that point, you get descheduled,
> and while you are not running, someone unloads the low-level driver
> module that ends its life by calling fpga_mgr_unregister(). When you
> wake up, you find yourself with a reference to a device that does not
> exist anymore, trying to get a module that does not exist anymore
> through one of its symbols (module *owner in mops).

Then the user gets to keep the multiple pieces that their kernel is now
in :)

Seriously, as module unload can never happen except by explicit ask,
this should only possibly be an issue that a developer who is working on
the code would hit, so don't work too hard to resolve something that
isn't anything an actual user can hit.

> Greg suggested checking if this can really happen and eventually
> protecting fpga_mgr_get() and fpga_mgr_unregister() with a lock for
> mops (if I understood correctly). In that case, considering the same
> scenario described above:
> 
> fpga_mgr_get() gets the mops lock and the mgr dev but is suspended
> before calling try_module_get().
> 
> Someone unloads the low-level driver, delete_modules progresses
> (the module's recount hasn't yet been incremented) but blocks while
> calling fpga_mgr_unregister() since fpga_mgr_get() is holding the lock.
> 
> fpga_mgr_get() resumes and tries to get the module through one of its
> symbols (mgr->mops->owner). The module's memory hasn't yet been freed
> (delete_modules is blocked), and the refcount is zero, so
> try_module_get() fails safely, and we can put the mgr dev that is
> still present since fpga_mgr_unregister() is blocked.
> 
> fpga_mgr_unregister() resumes and unregisters the mgr dev.

That seems a bit reasonable, try it and see!

> I'm still thinking about the possible implications. On the one hand,
> it looks safe in this case, but on the other hand, it feels brittle.
> In my understanding, the root problem is that there will always be a
> critical window (when you have taken a reference to the device but
> not yet to the low-level driver module) when unloading the module
> could be potentially unsafe depending on the current implementation
> and the preemption model.
> 
> I still feel that it would be simpler and safer if we could bump
> up the refcount during fpga_mgr_register() and maybe have a sysfs
> attribute to unlock the low-level driver (if no one has taken the
> mgr dev refcount).

Ick, no, that shouldn't be needed.

> That way, it would be safer by design since the
> refcount will be bumped up right during the module load procedure,
> and we could guarantee that the lifetime of the mgr device is
> entirely contained in the lifetime of the low-level driver module.

Remember, there are two different things here, code and data.  Trying to
tie one ref count to the other is almost always going to cause problems,
try to keep them independent if at all possible.

Or better yet, just don't use module reference counts at all and
properly drop the device when the specific module is unloaded, like
network drivers do.  That might take more work to restructure things,
which might be useless work given that again, this is something that no
user will ever hit, only developers if at all.

thanks,

greg k-h

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

[Xu Yilun]

On Fri, Nov 17, 2023 at 05:06:16PM -0500, Greg Kroah-Hartman wrote:
> On Fri, Nov 17, 2023 at 10:58:59PM +0100, Marco Pagani wrote:
> > 
> > 
> > On 2023-11-14 07:53, Xu Yilun wrote:
> > > On Fri, Nov 10, 2023 at 11:58:37PM +0100, Marco Pagani wrote:
> > >>
> > >>
> > >> On 2023-11-08 16:52, Xu Yilun wrote:
> > >>> On Fri, Nov 03, 2023 at 09:31:02PM +0100, Marco Pagani wrote:
> > >>>>
> > >>>>
> > >>>> On 2023-10-30 09:32, Xu Yilun wrote:
> > >>>>> 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.
> > >>>>>
> > >>>>
> > >>>> My understanding was that we wanted to remove module reference counting
> > >>>> from the fpga core and ease it from the responsibility of preventing
> > >>>> low-level driver modules from being unloaded. 
> > >>>
> > >>> FPGA core needs to prevent low-level driver module unloading sometimes,
> > >>> e.g. when region reprograming is in progress. That's why we get fpga
> > >>> region driver modules & bridge modules in fpga_region_program_fpga().
> > >>>
> > >>> But we try best to get them only necessary. Blindly geting them all the
> > >>> time results in no way to unload all modules (core & low level modules).
> > >>>
> > >>>>
> > >>>> If we want to keep reference counting in the fpga core, we could add a
> > >>>> struct module *owner field in the struct fpga_manager_ops (and others
> > >>>> core *_ops) so that the low-level driver can set it to THIS_MODULE.
> > >>>> In this way, we can later use it in fpga_mgr_register() to bump up the
> > >>>
> > >>> Yes, we should pass the module owner in fpga_mgr_register(), but could
> > >>> not bump up its refcount at once.
> > >>>
> > >>>> refcount of the low-level driver module by calling
> > >>>> try_module_get(mgr->mops->owner) directly when it registers the manager.
> > >>>> Finally, fpga_mgr_unregister() would call module_put(mgr->mops->owner)
> > >>>> to allow unloading the low-level driver module.
> > >>>
> > >>> As mentioned above, that makes problem. Most of the low level driver
> > >>> modules call fpga_mgr_unregister() on module_exit(), but bumping up
> > >>> their module refcount prevents module_exit() been executed. That came
> > >>> out to be a dead lock.
> > >>>
> > >>
> > >> Initially, I considered calling try_module_get(mgr->mops->owner)
> > >> in fpga_mgr_get(). But then, the new kernel-doc description of
> > >> try_module_get() (1) made me question the safety of that approach.
> > >> My concern is that the low-level driver could be removed right when
> > >> someone is calling fpga_mgr_get() and hasn't yet reached
> > >> try_module_get(mgr->mops->owner). In that case, the struct mops
> > >> (along with the entire low-level driver module) and the manager dev
> > >> would "disappear" under the feet of fpga_mgr_get().
> > > 
> > > I don't get what's the problem. fpga_mgr_get() would first of all
> > > look for mgr_dev via class_find_device(), if low-level module is
> > > unloaded, then you cannot find the mgr_dev and gracefully error out.
> > > 
> > > If class_find_device() succeed, mgr_dev got a reference and won't
> > > disappear. Finally we may still found module removed when
> > > try_module_get(), but should be another graceful error out.
> > > 
> > > Am I missing anything?
> > > 
> > 
> > My concern is: suppose that you successfully got the mgr dev from
> > class_find_device(), and now you are in __fpga_mgr_get(), right before
> > try_module_get(mgr->mops->owner). At that point, you get descheduled,
> > and while you are not running, someone unloads the low-level driver
> > module that ends its life by calling fpga_mgr_unregister(). When you
> > wake up, you find yourself with a reference to a device that does not
> > exist anymore, trying to get a module that does not exist anymore

I may get the problem. The mgr device is still exists, but the module
is removed, so the mgr->mops & owner pointers are invalid..

> > through one of its symbols (module *owner in mops).
> 
> Then the user gets to keep the multiple pieces that their kernel is now
> in :)
> 
> Seriously, as module unload can never happen except by explicit ask,
> this should only possibly be an issue that a developer who is working on
> the code would hit, so don't work too hard to resolve something that
> isn't anything an actual user can hit.
> 
> > Greg suggested checking if this can really happen and eventually
> > protecting fpga_mgr_get() and fpga_mgr_unregister() with a lock for
> > mops (if I understood correctly). In that case, considering the same
> > scenario described above:
> > 
> > fpga_mgr_get() gets the mops lock and the mgr dev but is suspended
> > before calling try_module_get().
> > 
> > Someone unloads the low-level driver, delete_modules progresses
> > (the module's recount hasn't yet been incremented) but blocks while
> > calling fpga_mgr_unregister() since fpga_mgr_get() is holding the lock.
> > 
> > fpga_mgr_get() resumes and tries to get the module through one of its
> > symbols (mgr->mops->owner). The module's memory hasn't yet been freed
> > (delete_modules is blocked), and the refcount is zero, so
> > try_module_get() fails safely, and we can put the mgr dev that is
> > still present since fpga_mgr_unregister() is blocked.
> > 
> > fpga_mgr_unregister() resumes and unregisters the mgr dev.
> 
> That seems a bit reasonable, try it and see!

It also looks good to me.

Thanks,
Yilun

> 
> > I'm still thinking about the possible implications. On the one hand,
> > it looks safe in this case, but on the other hand, it feels brittle.
> > In my understanding, the root problem is that there will always be a
> > critical window (when you have taken a reference to the device but
> > not yet to the low-level driver module) when unloading the module
> > could be potentially unsafe depending on the current implementation
> > and the preemption model.
> > 
> > I still feel that it would be simpler and safer if we could bump
> > up the refcount during fpga_mgr_register() and maybe have a sysfs
> > attribute to unlock the low-level driver (if no one has taken the
> > mgr dev refcount).
> 
> Ick, no, that shouldn't be needed.
> 
> > That way, it would be safer by design since the
> > refcount will be bumped up right during the module load procedure,
> > and we could guarantee that the lifetime of the mgr device is
> > entirely contained in the lifetime of the low-level driver module.
> 
> Remember, there are two different things here, code and data.  Trying to
> tie one ref count to the other is almost always going to cause problems,
> try to keep them independent if at all possible.
> 
> Or better yet, just don't use module reference counts at all and
> properly drop the device when the specific module is unloaded, like
> network drivers do.  That might take more work to restructure things,
> which might be useless work given that again, this is something that no
> user will ever hit, only developers if at all.
> 
> thanks,
> 
> greg k-h