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

From: Marco Pagani
Date: Tue Jan 09 2024 - 06:53:33 EST

Next message: Dietmar Eggemann: "Re: [PATCH v3 4/5] sched: Rename arch_update_thermal_pressure into arch_update_hw_pressure"
Previous message: Oleksandr Natalenko: "Flood of logitech-hidpp-device messages in v6.7"
In reply to: Xu Yilun: "Re: [RFC PATCH v4 1/1] fpga: add an owner and use it to take the low-level module's refcount"
Next in thread: Xu Yilun: "Re: [RFC PATCH v4 1/1] fpga: add an owner and use it to take the low-level module's refcount"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

>>
>>
>>>> + mgr = ERR_PTR(-ENODEV);
>>>>
>>>> -err_dev:
>>>> - put_device(dev);
>>>> - return ERR_PTR(-ENODEV);
>>>> + mutex_unlock(&mgr->mops_mutex);
>>>> +
>>>> + return mgr;
>>>> }
>>>>
>>>> static int fpga_mgr_dev_match(struct device *dev, const void *data)
>>>> @@ -693,12 +693,18 @@ static int fpga_mgr_dev_match(struct device *dev, const void *data)
>>>> */
>>>> struct fpga_manager *fpga_mgr_get(struct device *dev)
>>>> {
>>>> - struct device *mgr_dev = class_find_device(&fpga_mgr_class, NULL, dev,
>>>> - fpga_mgr_dev_match);
>>>> + struct fpga_manager *mgr;
>>>> + struct device *mgr_dev;
>>>> +
>>>> + mgr_dev = class_find_device(&fpga_mgr_class, NULL, dev, fpga_mgr_dev_match);
>>>> if (!mgr_dev)
>>>> return ERR_PTR(-ENODEV);
>>>>
>>>> - return __fpga_mgr_get(mgr_dev);
>>>> + mgr = __fpga_mgr_get(mgr_dev);
>>>> + if (IS_ERR(mgr))
>>>> + put_device(mgr_dev);
>>>> +
>>>> + return mgr;
>>>> }
>>>> EXPORT_SYMBOL_GPL(fpga_mgr_get);
>>>>
>>>> @@ -711,13 +717,18 @@ EXPORT_SYMBOL_GPL(fpga_mgr_get);
>>>> */
>>>> struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
>>>> {
>>>> - struct device *dev;
>>>> + struct fpga_manager *mgr;
>>>> + struct device *mgr_dev;
>>>>
>>>> - dev = class_find_device_by_of_node(&fpga_mgr_class, node);
>>>> - if (!dev)
>>>> + mgr_dev = class_find_device_by_of_node(&fpga_mgr_class, node);
>>>> + if (!mgr_dev)
>>>> return ERR_PTR(-ENODEV);
>>>>
>>>> - return __fpga_mgr_get(dev);
>>>> + mgr = __fpga_mgr_get(mgr_dev);
>>>> + if (IS_ERR(mgr))
>>>> + put_device(mgr_dev);
>>>> +
>>>> + return mgr;
>>>> }
>>>> EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
>>>>
>>>> @@ -727,7 +738,7 @@ EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
>>>> */
>>>> void fpga_mgr_put(struct fpga_manager *mgr)
>>>> {
>>>> - module_put(mgr->dev.parent->driver->owner);
>>>> + module_put(mgr->mops_owner);
>>>> put_device(&mgr->dev);
>>>> }
>>>> EXPORT_SYMBOL_GPL(fpga_mgr_put);
>>>> @@ -766,9 +777,10 @@ void fpga_mgr_unlock(struct fpga_manager *mgr)
>>>> EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
>>>>
>>>> /**
>>>> - * fpga_mgr_register_full - create and register an FPGA Manager device
>>>> + * __fpga_mgr_register_full - create and register an FPGA Manager device
>>>> * @parent: fpga manager device from pdev
>>>> * @info: parameters for fpga manager
>>>> + * @owner: owner module containing the ops
>>>> *
>>>> * The caller of this function is responsible for calling fpga_mgr_unregister().
>>>> * Using devm_fpga_mgr_register_full() instead is recommended.
>>>> @@ -776,7 +788,8 @@ EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
>>>> * Return: pointer to struct fpga_manager pointer or ERR_PTR()
>>>> */
>>>> 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)
>>>> {
>>>> const struct fpga_manager_ops *mops = info->mops;
>>>> struct fpga_manager *mgr;
>>>> @@ -803,6 +816,9 @@ fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *in
>>>> }
>>>>
>>>> mutex_init(&mgr->ref_mutex);
>>>> + mutex_init(&mgr->mops_mutex);
>>>> +
>>>> + mgr->mops_owner = owner;
>>>>
>>>> mgr->name = info->name;
>>>> mgr->mops = info->mops;
>>>> @@ -841,14 +857,15 @@ fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *in
>>>>
>>>> return ERR_PTR(ret);
>>>> }
>>>> -EXPORT_SYMBOL_GPL(fpga_mgr_register_full);
>>>> +EXPORT_SYMBOL_GPL(__fpga_mgr_register_full);
>>>>
>>>> /**
>>>> - * fpga_mgr_register - create and register an FPGA Manager device
>>>> + * __fpga_mgr_register - create and register an FPGA Manager device
>>>> * @parent: fpga manager device from pdev
>>>> * @name: fpga manager name
>>>> * @mops: pointer to structure of fpga manager ops
>>>> * @priv: fpga manager private data
>>>> + * @owner: owner module containing the ops
>>>> *
>>>> * The caller of this function is responsible for calling fpga_mgr_unregister().
>>>> * Using devm_fpga_mgr_register() instead is recommended. This simple
>>>> @@ -859,8 +876,8 @@ EXPORT_SYMBOL_GPL(fpga_mgr_register_full);
>>>> * Return: pointer to struct fpga_manager pointer or ERR_PTR()
>>>> */
>>>> struct fpga_manager *
>>>> -fpga_mgr_register(struct device *parent, const char *name,
>>>> - const struct fpga_manager_ops *mops, void *priv)
>>>> +__fpga_mgr_register(struct device *parent, const char *name,
>>>> + const struct fpga_manager_ops *mops, void *priv, struct module *owner)
>>>> {
>>>> struct fpga_manager_info info = { 0 };
>>>>
>>>> @@ -868,9 +885,9 @@ fpga_mgr_register(struct device *parent, const char *name,
>>>> info.mops = mops;
>>>> info.priv = priv;
>>>>
>>>> - return fpga_mgr_register_full(parent, &info);
>>>> + return __fpga_mgr_register_full(parent, &info, owner);
>>>> }
>>>> -EXPORT_SYMBOL_GPL(fpga_mgr_register);
>>>> +EXPORT_SYMBOL_GPL(__fpga_mgr_register);
>>>>
>>>> /**
>>>> * fpga_mgr_unregister - unregister an FPGA manager
>>>> @@ -888,6 +905,12 @@ void fpga_mgr_unregister(struct fpga_manager *mgr)
>>>> */
>>>> fpga_mgr_fpga_remove(mgr);
>>>>
>>>> + mutex_lock(&mgr->mops_mutex);
>>>> +
>>>> + mgr->mops = NULL;
>
> Same here, is it better set mgr->mops_owner = NULL?
>
>>>> +
>>>> + mutex_unlock(&mgr->mops_mutex);
>>>> +
>>>> device_unregister(&mgr->dev);
>>>> }
>>>> EXPORT_SYMBOL_GPL(fpga_mgr_unregister);
>>>> @@ -900,9 +923,10 @@ static void devm_fpga_mgr_unregister(struct device *dev, void *res)
>>>> }
>>>>
>>>> /**
>>>> - * devm_fpga_mgr_register_full - resource managed variant of fpga_mgr_register()
>>>> + * __devm_fpga_mgr_register_full - resource managed variant of fpga_mgr_register()
>>>> * @parent: fpga manager device from pdev
>>>> * @info: parameters for fpga manager
>>>> + * @owner: owner module containing the ops
>>>> *
>>>> * Return: fpga manager pointer on success, negative error code otherwise.
>>>> *
>>>> @@ -910,7 +934,8 @@ static void devm_fpga_mgr_unregister(struct device *dev, void *res)
>>>> * function will be called automatically when the managing device is detached.
>>>> */
>>>> struct fpga_manager *
>>>> -devm_fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *info)
>>>> +__devm_fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *info,
>>>> + struct module *owner)
>>>> {
>>>> struct fpga_mgr_devres *dr;
>>>> struct fpga_manager *mgr;
>>>> @@ -919,7 +944,7 @@ devm_fpga_mgr_register_full(struct device *parent, const struct fpga_manager_inf
>>>> if (!dr)
>>>> return ERR_PTR(-ENOMEM);
>>>>
>>>> - mgr = fpga_mgr_register_full(parent, info);
>>>> + mgr = __fpga_mgr_register_full(parent, info, owner);
>>>> if (IS_ERR(mgr)) {
>>>> devres_free(dr);
>>>> return mgr;
>>>> @@ -930,14 +955,15 @@ devm_fpga_mgr_register_full(struct device *parent, const struct fpga_manager_inf
>>>>
>>>> return mgr;
>>>> }
>>>> -EXPORT_SYMBOL_GPL(devm_fpga_mgr_register_full);
>>>> +EXPORT_SYMBOL_GPL(__devm_fpga_mgr_register_full);
>>>>
>>>> /**
>>>> - * devm_fpga_mgr_register - resource managed variant of fpga_mgr_register()
>>>> + * __devm_fpga_mgr_register - resource managed variant of fpga_mgr_register()
>>>> * @parent: fpga manager device from pdev
>>>> * @name: fpga manager name
>>>> * @mops: pointer to structure of fpga manager ops
>>>> * @priv: fpga manager private data
>>>> + * @owner: owner module containing the ops
>>>> *
>>>> * Return: fpga manager pointer on success, negative error code otherwise.
>>>> *
>>>> @@ -946,8 +972,9 @@ EXPORT_SYMBOL_GPL(devm_fpga_mgr_register_full);
>>>> * device is detached.
>>>> */
>>>> struct fpga_manager *
>>>> -devm_fpga_mgr_register(struct device *parent, const char *name,
>>>> - const struct fpga_manager_ops *mops, void *priv)
>>>> +__devm_fpga_mgr_register(struct device *parent, const char *name,
>>>> + const struct fpga_manager_ops *mops, void *priv,
>>>> + struct module *owner)
>>>> {
>>>> struct fpga_manager_info info = { 0 };
>>>>
>>>> @@ -955,9 +982,9 @@ devm_fpga_mgr_register(struct device *parent, const char *name,
>>>> info.mops = mops;
>>>> info.priv = priv;
>>>>
>>>> - return devm_fpga_mgr_register_full(parent, &info);
>>>> + return __devm_fpga_mgr_register_full(parent, &info, owner);
>>>> }
>>>> -EXPORT_SYMBOL_GPL(devm_fpga_mgr_register);
>>>> +EXPORT_SYMBOL_GPL(__devm_fpga_mgr_register);
>>>>
>>>> static void fpga_mgr_dev_release(struct device *dev)
>>>> {
>>>> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
>>>> index 54f63459efd6..967540311462 100644
>>>> --- a/include/linux/fpga/fpga-mgr.h
>>>> +++ b/include/linux/fpga/fpga-mgr.h
>>>> @@ -201,6 +201,8 @@ struct fpga_manager_ops {
>>>> * @state: state of fpga manager
>>>> * @compat_id: FPGA manager id for compatibility check.
>>>> * @mops: pointer to struct of fpga manager ops
>>>> + * @mops_mutex: protects mops from low-level module removal
>
> Same here, change the doc if needed.
>
>>>> + * @mops_owner: module containing the mops
>>>> * @priv: low level driver private date
>>>> */
>>>> struct fpga_manager {
>>>> @@ -210,6 +212,8 @@ struct fpga_manager {
>>>> enum fpga_mgr_states state;
>>>> struct fpga_compat_id *compat_id;
>>>> const struct fpga_manager_ops *mops;
>>>> + struct mutex mops_mutex;
>>>> + struct module *mops_owner;
>>>> void *priv;
>>>> };
>>>>
>>>> @@ -222,6 +226,7 @@ void fpga_image_info_free(struct fpga_image_info *info);
>>>> int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info);
>>>>
>>>> int fpga_mgr_lock(struct fpga_manager *mgr);
>>>> +
>>>
>>> Why adding a line?
>>>
>>
>> I'll remove the line.
>>
>>>> void fpga_mgr_unlock(struct fpga_manager *mgr);
>>>>
>>>> struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
>>>> @@ -230,18 +235,81 @@ struct fpga_manager *fpga_mgr_get(struct device *dev);
>>>>
>>>> void fpga_mgr_put(struct fpga_manager *mgr);
>>>>
>>>> +/**
>>>> + * fpga_mgr_register_full - create and register an FPGA Manager device
>>>> + * @parent: fpga manager device from pdev
>>>> + * @info: parameters for fpga manager
>>>> + *
>>>> + * The caller of this function is responsible for calling fpga_mgr_unregister().
>>>> + * Using devm_fpga_mgr_register_full() instead is recommended.
>>>> + *
>>>> + * Return: pointer to struct fpga_manager pointer or ERR_PTR()
>>>> + */
>>>
>>> No need to duplicate the doc, just remove it.
>>> Same for the rest of code.
>>
>> Do you mean keeping the kernel-doc comments only for the __fpga_mgr_*
>> functions in fpga-mgr.c?
>>
>>>
>>>> +#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.

>> [...]

Thanks,
Marco

Next message: Dietmar Eggemann: "Re: [PATCH v3 4/5] sched: Rename arch_update_thermal_pressure into arch_update_hw_pressure"
Previous message: Oleksandr Natalenko: "Flood of logitech-hidpp-device messages in v6.7"
In reply to: Xu Yilun: "Re: [RFC PATCH v4 1/1] fpga: add an owner and use it to take the low-level module's refcount"
Next in thread: Xu Yilun: "Re: [RFC PATCH v4 1/1] fpga: add an owner and use it to take the low-level module's refcount"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]