Re: [RFC PATCH v4 1/1] fpga: add an owner and use it to take the low-level module's refcount

[Xu Yilun]

On Tue, Jan 09, 2024 at 12:53:14PM +0100, Marco Pagani wrote:
> 
> 
> On 09/01/24 05:40, Xu Yilun wrote:
> > On Mon, Jan 08, 2024 at 06:24:55PM +0100, Marco Pagani wrote:
> >>
> >>
> >> On 2024-01-08 10:07, Xu Yilun wrote:
> >>> On Sat, Jan 06, 2024 at 12:15:26AM +0100, Marco Pagani wrote:
> >>>> Add a module owner field to the fpga_manager struct to take the
> >>>> low-level control module refcount instead of assuming that the parent
> >>>> device has a driver and using its owner pointer. The owner is now
> >>>> passed as an additional argument at registration time. To this end,
> >>>> the functions for registration have been modified to take an additional
> >>>> owner parameter and renamed to avoid conflicts. The old function names
> >>>> are now used for helper macros that automatically set the module that
> >>>> registers the fpga manager as the owner. This ensures compatibility
> >>>> with existing low-level control modules and reduces the chances of
> >>>> registering a manager without setting the owner.
> >>>>
> >>>> To detect when the owner module pointer becomes stale, set the mops
> >>>> pointer to null during fpga_mgr_unregister() and test it before taking
> >>>> the module's refcount. Use a mutex to protect against a crash that can
> >>>> happen if __fpga_mgr_get() gets suspended between testing the mops
> >>>> pointer and taking the refcount while the low-level module is being
> >>>> unloaded.
> >>>>
> >>>> Other changes: opportunistically move put_device() from __fpga_mgr_get()
> >>>> to fpga_mgr_get() and of_fpga_mgr_get() to improve code clarity since
> >>>> the device refcount in taken in these functions.
> >>>>
> >>>> Fixes: 654ba4cc0f3e ("fpga manager: ensure lifetime with of_fpga_mgr_get")
> >>>> Suggested-by: Xu Yilun <yilun.xu@xxxxxxxxx>
> >>>> Signed-off-by: Marco Pagani <marpagan@xxxxxxxxxx>
> >>>> ---
> >>>>  drivers/fpga/fpga-mgr.c       | 93 ++++++++++++++++++++++-------------
> >>>>  include/linux/fpga/fpga-mgr.h | 80 +++++++++++++++++++++++++++---
> >>>>  2 files changed, 134 insertions(+), 39 deletions(-)
> >>>>
> >>>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> >>>> index 06651389c592..d7bfbdfdf2fc 100644
> >>>> --- a/drivers/fpga/fpga-mgr.c
> >>>> +++ b/drivers/fpga/fpga-mgr.c
> >>>> @@ -664,20 +664,20 @@ static struct attribute *fpga_mgr_attrs[] = {
> >>>>  };
> >>>>  ATTRIBUTE_GROUPS(fpga_mgr);
> >>>>  
> >>>> -static struct fpga_manager *__fpga_mgr_get(struct device *dev)
> >>>> +static struct fpga_manager *__fpga_mgr_get(struct device *mgr_dev)
> >>>>  {
> >>>>  	struct fpga_manager *mgr;
> >>>>  
> >>>> -	mgr = to_fpga_manager(dev);
> >>>> +	mgr = to_fpga_manager(mgr_dev);
> >>>>  
> >>>> -	if (!try_module_get(dev->parent->driver->owner))
> >>>> -		goto err_dev;
> >>>> +	mutex_lock(&mgr->mops_mutex);
> >>>>  
> >>>> -	return mgr;
> >>>> +	if (!mgr->mops || !try_module_get(mgr->mops_owner))
> >>>
> >>> Why move the owner out of struct fpga_manager_ops? The owner within the
> >>> ops struct makes more sense to me, it better illustrates what the mutex
> >>> is protecting.
> >>>
> >>
> >> I think having the owner in fpga_manager_ops made sense when it was
> >> meant to be set while initializing the struct in the low-level module.
> >> However, since the owner is now passed as an argument and set at
> >> registration time, keeping it in the FPGA manager context seems more
> >> straightforward to me.
> > 
> > That's OK. But then why not directly check mops_owner here?
> >
> 
> We can do that, but it would impose a precondition since it would not
> allow registering a manager with a NULL ops owner. In that case, I think

No we should allow ops module owner = NULL, we should allow built-in
drivers.

I'm OK with the current implementation now.

> we need to make the precondition explicit and add a check in
> fpga_mgr_register_*() that prevents registering a manager with a NULL ops
> owner. Otherwise, the programmer could register a manager that cannot be
> taken.
>  

[...]

> >>>> +#define fpga_mgr_register_full(parent, info) \
> >>>> +	__fpga_mgr_register_full(parent, info, THIS_MODULE)
> >>>> +
> >>>
> >>> Delete the line, and ...
> >>>
> >>>>  struct fpga_manager *
> >>>> -fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *info);
> >>>> +__fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *info,
> >>>> +			 struct module *owner);
> >>>
> >>> Add a line here, to make the related functions packed.
> >>> Same for the rest of code.
> >>
> >> Do you prefer having the macro just above the function prototype? Like:
> >>
> >> #define devm_fpga_mgr_register(parent, name, mops, priv) \
> >> 	__devm_fpga_mgr_register(parent, name, mops, priv, THIS_MODULE)
> >> struct fpga_manager *
> >> __devm_fpga_mgr_register(struct device *parent, const char *name,
> >> 			 const struct fpga_manager_ops *mops, void *priv,
> >> 			 struct module *owner);
> > 
> > Yes, that's it.
> 
> My only concern is that if we keep the kernel-doc comments only for the
> __fpga_mgr_register* functions in fpga-mgr.c, we would not get the docs
> for the helper macros that are the most likely to be used.

That's not a problem, people should be smart enough to find the doc and
know what the MACRO is doing. And usually function doc should be placed
near function source code rather than declaration.

Thanks,
Yilun

> 
> 
> >> [...]
> 
> 
> Thanks,
> Marco
>