The current implementation of the fpga region assumes that the low-level
module registers a driver for the parent device and uses its owner pointer
to take the module's refcount. This approach is problematic since it can
lead to a null pointer dereference while attempting to get the region
during programming if the parent device does not have a driver.
To address this problem, add a module owner pointer to the fpga_region
struct and use it to take the module's refcount. Modify the functions for
registering a region to take an additional owner module parameter and
rename them to avoid conflicts. Use the old function names for helper
macros that automatically set the module that registers the region as the
owner. This ensures compatibility with existing low-level control modules
and reduces the chances of registering a region without setting the owner.
Also, update the documentation to keep it consistent with the new interface
for registering an fpga region.
Fixes: 0fa20cdfcc1f ("fpga: fpga-region: device tree control for FPGA")
Suggested-by: Greg Kroah-Hartman <[email protected]>
Suggested-by: Xu Yilun <[email protected]>
Reviewed-by: Russ Weight <[email protected]>
Signed-off-by: Marco Pagani <[email protected]>
---
v5:
- Reverted function names swap in the documentation
- Renamed owner module pointer br_owner -> ops_owner
v4:
- Split out the swap between put_device() and mutex_unlock() while
releasing the region to avoid potential use after release issues
v3:
- Add reviewed-by Russ Weight
v2:
- Fixed typo in the documentation sets -> set
- Renamed owner module pointer get_br_owner -> br_owner
---
Documentation/driver-api/fpga/fpga-region.rst | 13 ++++++----
drivers/fpga/fpga-region.c | 24 +++++++++++--------
include/linux/fpga/fpga-region.h | 13 +++++++---
3 files changed, 32 insertions(+), 18 deletions(-)
diff --git a/Documentation/driver-api/fpga/fpga-region.rst b/Documentation/driver-api/fpga/fpga-region.rst
index dc55d60a0b4a..2d03b5fb7657 100644
--- a/Documentation/driver-api/fpga/fpga-region.rst
+++ b/Documentation/driver-api/fpga/fpga-region.rst
@@ -46,13 +46,16 @@ API to add a new FPGA region
----------------------------
* struct fpga_region - The FPGA region struct
-* struct fpga_region_info - Parameter structure for fpga_region_register_full()
-* fpga_region_register_full() - Create and register an FPGA region using the
+* struct fpga_region_info - Parameter structure for __fpga_region_register_full()
+* __fpga_region_register_full() - Create and register an FPGA region using the
fpga_region_info structure to provide the full flexibility of options
-* fpga_region_register() - Create and register an FPGA region using standard
+* __fpga_region_register() - Create and register an FPGA region using standard
arguments
* fpga_region_unregister() - Unregister an FPGA region
+Helper macros ``fpga_region_register()`` and ``fpga_region_register_full()``
+automatically set the module that registers the FPGA region as the owner.
+
The FPGA region's probe function will need to get a reference to the FPGA
Manager it will be using to do the programming. This usually would happen
during the region's probe function.
@@ -82,10 +85,10 @@ following APIs to handle building or tearing down that list.
:functions: fpga_region_info
.. kernel-doc:: drivers/fpga/fpga-region.c
- :functions: fpga_region_register_full
+ :functions: __fpga_region_register_full
.. kernel-doc:: drivers/fpga/fpga-region.c
- :functions: fpga_region_register
+ :functions: __fpga_region_register
.. kernel-doc:: drivers/fpga/fpga-region.c
:functions: fpga_region_unregister
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index b364a929425c..cb446dc68fca 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -53,7 +53,7 @@ static struct fpga_region *fpga_region_get(struct fpga_region *region)
}
get_device(dev);
- if (!try_module_get(dev->parent->driver->owner)) {
+ if (!try_module_get(region->ops_owner)) {
put_device(dev);
mutex_unlock(®ion->mutex);
return ERR_PTR(-ENODEV);
@@ -75,7 +75,7 @@ static void fpga_region_put(struct fpga_region *region)
dev_dbg(dev, "put\n");
- module_put(dev->parent->driver->owner);
+ module_put(region->ops_owner);
put_device(dev);
mutex_unlock(®ion->mutex);
}
@@ -181,14 +181,16 @@ static struct attribute *fpga_region_attrs[] = {
ATTRIBUTE_GROUPS(fpga_region);
/**
- * fpga_region_register_full - create and register an FPGA Region device
+ * __fpga_region_register_full - create and register an FPGA Region device
* @parent: device parent
* @info: parameters for FPGA Region
+ * @owner: owner module containing the get_bridges function
*
* Return: struct fpga_region or ERR_PTR()
*/
struct fpga_region *
-fpga_region_register_full(struct device *parent, const struct fpga_region_info *info)
+__fpga_region_register_full(struct device *parent, const struct fpga_region_info *info,
+ struct module *owner)
{
struct fpga_region *region;
int id, ret = 0;
@@ -213,6 +215,7 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
region->compat_id = info->compat_id;
region->priv = info->priv;
region->get_bridges = info->get_bridges;
+ region->ops_owner = owner;
mutex_init(®ion->mutex);
INIT_LIST_HEAD(®ion->bridge_list);
@@ -241,13 +244,14 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
return ERR_PTR(ret);
}
-EXPORT_SYMBOL_GPL(fpga_region_register_full);
+EXPORT_SYMBOL_GPL(__fpga_region_register_full);
/**
- * fpga_region_register - create and register an FPGA Region device
+ * __fpga_region_register - create and register an FPGA Region device
* @parent: device parent
* @mgr: manager that programs this region
* @get_bridges: optional function to get bridges to a list
+ * @owner: owner module containing get_bridges function
*
* This simple version of the register function should be sufficient for most users.
* The fpga_region_register_full() function is available for users that need to
@@ -256,17 +260,17 @@ EXPORT_SYMBOL_GPL(fpga_region_register_full);
* Return: struct fpga_region or ERR_PTR()
*/
struct fpga_region *
-fpga_region_register(struct device *parent, struct fpga_manager *mgr,
- int (*get_bridges)(struct fpga_region *))
+__fpga_region_register(struct device *parent, struct fpga_manager *mgr,
+ int (*get_bridges)(struct fpga_region *), struct module *owner)
{
struct fpga_region_info info = { 0 };
info.mgr = mgr;
info.get_bridges = get_bridges;
- return fpga_region_register_full(parent, &info);
+ return __fpga_region_register_full(parent, &info, owner);
}
-EXPORT_SYMBOL_GPL(fpga_region_register);
+EXPORT_SYMBOL_GPL(__fpga_region_register);
/**
* fpga_region_unregister - unregister an FPGA region
diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
index 9d4d32909340..5fbc05fe70a6 100644
--- a/include/linux/fpga/fpga-region.h
+++ b/include/linux/fpga/fpga-region.h
@@ -36,6 +36,7 @@ struct fpga_region_info {
* @mgr: FPGA manager
* @info: FPGA image info
* @compat_id: FPGA region id for compatibility check.
+ * @ops_owner: module containing the get_bridges function
* @priv: private data
* @get_bridges: optional function to get bridges to a list
*/
@@ -46,6 +47,7 @@ struct fpga_region {
struct fpga_manager *mgr;
struct fpga_image_info *info;
struct fpga_compat_id *compat_id;
+ struct module *ops_owner;
void *priv;
int (*get_bridges)(struct fpga_region *region);
};
@@ -58,12 +60,17 @@ fpga_region_class_find(struct device *start, const void *data,
int fpga_region_program_fpga(struct fpga_region *region);
+#define fpga_region_register_full(parent, info) \
+ __fpga_region_register_full(parent, info, THIS_MODULE)
struct fpga_region *
-fpga_region_register_full(struct device *parent, const struct fpga_region_info *info);
+__fpga_region_register_full(struct device *parent, const struct fpga_region_info *info,
+ struct module *owner);
+#define fpga_region_register(parent, mgr, get_bridges) \
+ __fpga_region_register(parent, mgr, get_bridges, THIS_MODULE)
struct fpga_region *
-fpga_region_register(struct device *parent, struct fpga_manager *mgr,
- int (*get_bridges)(struct fpga_region *));
+__fpga_region_register(struct device *parent, struct fpga_manager *mgr,
+ int (*get_bridges)(struct fpga_region *), struct module *owner);
void fpga_region_unregister(struct fpga_region *region);
#endif /* _FPGA_REGION_H */
base-commit: 5d04660b29fb31e88e945c8b3eb658976824d0fa
--
2.44.0
> /**
> - * fpga_region_register_full - create and register an FPGA Region device
> + * __fpga_region_register_full - create and register an FPGA Region device
> * @parent: device parent
> * @info: parameters for FPGA Region
> + * @owner: owner module containing the get_bridges function
This is too specific and easily get unaligned if we add another
callback. How about "module containing the region ops"?
> *
> * Return: struct fpga_region or ERR_PTR()
> */
> struct fpga_region *
> -fpga_region_register_full(struct device *parent, const struct fpga_region_info *info)
> +__fpga_region_register_full(struct device *parent, const struct fpga_region_info *info,
> + struct module *owner)
> {
> struct fpga_region *region;
> int id, ret = 0;
> @@ -213,6 +215,7 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
> region->compat_id = info->compat_id;
> region->priv = info->priv;
> region->get_bridges = info->get_bridges;
> + region->ops_owner = owner;
>
> mutex_init(®ion->mutex);
> INIT_LIST_HEAD(®ion->bridge_list);
> @@ -241,13 +244,14 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
>
> return ERR_PTR(ret);
> }
> -EXPORT_SYMBOL_GPL(fpga_region_register_full);
> +EXPORT_SYMBOL_GPL(__fpga_region_register_full);
>
> /**
> - * fpga_region_register - create and register an FPGA Region device
> + * __fpga_region_register - create and register an FPGA Region device
> * @parent: device parent
> * @mgr: manager that programs this region
> * @get_bridges: optional function to get bridges to a list
> + * @owner: owner module containing get_bridges function
ditto
> *
> * This simple version of the register function should be sufficient for most users.
> * The fpga_region_register_full() function is available for users that need to
> @@ -256,17 +260,17 @@ EXPORT_SYMBOL_GPL(fpga_region_register_full);
> * Return: struct fpga_region or ERR_PTR()
> */
> struct fpga_region *
> -fpga_region_register(struct device *parent, struct fpga_manager *mgr,
> - int (*get_bridges)(struct fpga_region *))
> +__fpga_region_register(struct device *parent, struct fpga_manager *mgr,
> + int (*get_bridges)(struct fpga_region *), struct module *owner)
> {
> struct fpga_region_info info = { 0 };
>
> info.mgr = mgr;
> info.get_bridges = get_bridges;
>
> - return fpga_region_register_full(parent, &info);
> + return __fpga_region_register_full(parent, &info, owner);
> }
> -EXPORT_SYMBOL_GPL(fpga_region_register);
> +EXPORT_SYMBOL_GPL(__fpga_region_register);
>
> /**
> * fpga_region_unregister - unregister an FPGA region
> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
> index 9d4d32909340..5fbc05fe70a6 100644
> --- a/include/linux/fpga/fpga-region.h
> +++ b/include/linux/fpga/fpga-region.h
> @@ -36,6 +36,7 @@ struct fpga_region_info {
> * @mgr: FPGA manager
> * @info: FPGA image info
> * @compat_id: FPGA region id for compatibility check.
> + * @ops_owner: module containing the get_bridges function
ditto
Thanks,
Yilun
> * @priv: private data
> * @get_bridges: optional function to get bridges to a list
> */
> @@ -46,6 +47,7 @@ struct fpga_region {
> struct fpga_manager *mgr;
> struct fpga_image_info *info;
> struct fpga_compat_id *compat_id;
> + struct module *ops_owner;
> void *priv;
> int (*get_bridges)(struct fpga_region *region);
> };
On 2024-04-13 04:35, Xu Yilun wrote:
>> /**
>> - * fpga_region_register_full - create and register an FPGA Region device
>> + * __fpga_region_register_full - create and register an FPGA Region device
>> * @parent: device parent
>> * @info: parameters for FPGA Region
>> + * @owner: owner module containing the get_bridges function
>
> This is too specific and easily get unaligned if we add another
> callback. How about "module containing the region ops"?
I had some concerns about using the name "region ops" in the kernel-doc
comment since it was not supported by a struct definition nor referenced
in the documentation. However, since the name is now referred to in the
ops_owner pointer, making the change makes sense to me.
Thanks,
Marco
>
>> *
>> * Return: struct fpga_region or ERR_PTR()
>> */
>> struct fpga_region *
>> -fpga_region_register_full(struct device *parent, const struct fpga_region_info *info)
>> +__fpga_region_register_full(struct device *parent, const struct fpga_region_info *info,
>> + struct module *owner)
>> {
>> struct fpga_region *region;
>> int id, ret = 0;
>> @@ -213,6 +215,7 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
>> region->compat_id = info->compat_id;
>> region->priv = info->priv;
>> region->get_bridges = info->get_bridges;
>> + region->ops_owner = owner;
>>
>> mutex_init(®ion->mutex);
>> INIT_LIST_HEAD(®ion->bridge_list);
>> @@ -241,13 +244,14 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
>>
>> return ERR_PTR(ret);
>> }
>> -EXPORT_SYMBOL_GPL(fpga_region_register_full);
>> +EXPORT_SYMBOL_GPL(__fpga_region_register_full);
>>
>> /**
>> - * fpga_region_register - create and register an FPGA Region device
>> + * __fpga_region_register - create and register an FPGA Region device
>> * @parent: device parent
>> * @mgr: manager that programs this region
>> * @get_bridges: optional function to get bridges to a list
>> + * @owner: owner module containing get_bridges function
>
> ditto
>
>> *
>> * This simple version of the register function should be sufficient for most users.
>> * The fpga_region_register_full() function is available for users that need to
>> @@ -256,17 +260,17 @@ EXPORT_SYMBOL_GPL(fpga_region_register_full);
>> * Return: struct fpga_region or ERR_PTR()
>> */
>> struct fpga_region *
>> -fpga_region_register(struct device *parent, struct fpga_manager *mgr,
>> - int (*get_bridges)(struct fpga_region *))
>> +__fpga_region_register(struct device *parent, struct fpga_manager *mgr,
>> + int (*get_bridges)(struct fpga_region *), struct module *owner)
>> {
>> struct fpga_region_info info = { 0 };
>>
>> info.mgr = mgr;
>> info.get_bridges = get_bridges;
>>
>> - return fpga_region_register_full(parent, &info);
>> + return __fpga_region_register_full(parent, &info, owner);
>> }
>> -EXPORT_SYMBOL_GPL(fpga_region_register);
>> +EXPORT_SYMBOL_GPL(__fpga_region_register);
>>
>> /**
>> * fpga_region_unregister - unregister an FPGA region
>> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
>> index 9d4d32909340..5fbc05fe70a6 100644
>> --- a/include/linux/fpga/fpga-region.h
>> +++ b/include/linux/fpga/fpga-region.h
>> @@ -36,6 +36,7 @@ struct fpga_region_info {
>> * @mgr: FPGA manager
>> * @info: FPGA image info
>> * @compat_id: FPGA region id for compatibility check.
>> + * @ops_owner: module containing the get_bridges function
>
> ditto
>
> Thanks,
> Yilun
>
>> * @priv: private data
>> * @get_bridges: optional function to get bridges to a list
>> */
>> @@ -46,6 +47,7 @@ struct fpga_region {
>> struct fpga_manager *mgr;
>> struct fpga_image_info *info;
>> struct fpga_compat_id *compat_id;
>> + struct module *ops_owner;
>> void *priv;
>> int (*get_bridges)(struct fpga_region *region);
>> };
>
On 2024-04-15 14:19, Marco Pagani wrote:
>
>
> On 2024-04-13 04:35, Xu Yilun wrote:
>>> /**
>>> - * fpga_region_register_full - create and register an FPGA Region device
>>> + * __fpga_region_register_full - create and register an FPGA Region device
>>> * @parent: device parent
>>> * @info: parameters for FPGA Region
>>> + * @owner: owner module containing the get_bridges function
>>
>> This is too specific and easily get unaligned if we add another
>> callback. How about "module containing the region ops"?
>
> I had some concerns about using the name "region ops" in the kernel-doc
> comment since it was not supported by a struct definition nor referenced
> in the documentation. However, since the name is now referred to in the
> ops_owner pointer, making the change makes sense to me.
>
On second thought, I think it would be better to leave the @owner
description to "module containing the get_bridges function" for the
moment. Otherwise, it could confuse the user by blurring the connection
between the @owner and @get_bridges parameters.
* __fpga_region_register - create and register an FPGA Region device
* [...]
* @get_bridges: optional function to get bridges to a list
* @owner: owner module containing get_bridges function
We can always modify the @owner description later, together with all the
necessary changes to add a new op, like grouping all ops in a structure
and changing the registration function signature.
Thanks,
Marco
>
>>
>>> *
>>> * Return: struct fpga_region or ERR_PTR()
>>> */
>>> struct fpga_region *
>>> -fpga_region_register_full(struct device *parent, const struct fpga_region_info *info)
>>> +__fpga_region_register_full(struct device *parent, const struct fpga_region_info *info,
>>> + struct module *owner)
>>> {
>>> struct fpga_region *region;
>>> int id, ret = 0;
>>> @@ -213,6 +215,7 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
>>> region->compat_id = info->compat_id;
>>> region->priv = info->priv;
>>> region->get_bridges = info->get_bridges;
>>> + region->ops_owner = owner;
>>>
>>> mutex_init(®ion->mutex);
>>> INIT_LIST_HEAD(®ion->bridge_list);
>>> @@ -241,13 +244,14 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
>>>
>>> return ERR_PTR(ret);
>>> }
>>> -EXPORT_SYMBOL_GPL(fpga_region_register_full);
>>> +EXPORT_SYMBOL_GPL(__fpga_region_register_full);
>>>
>>> /**
>>> - * fpga_region_register - create and register an FPGA Region device
>>> + * __fpga_region_register - create and register an FPGA Region device
>>> * @parent: device parent
>>> * @mgr: manager that programs this region
>>> * @get_bridges: optional function to get bridges to a list
>>> + * @owner: owner module containing get_bridges function
>>
>> ditto
>>
>>> *
>>> * This simple version of the register function should be sufficient for most users.
>>> * The fpga_region_register_full() function is available for users that need to
>>> @@ -256,17 +260,17 @@ EXPORT_SYMBOL_GPL(fpga_region_register_full);
>>> * Return: struct fpga_region or ERR_PTR()
>>> */
>>> struct fpga_region *
>>> -fpga_region_register(struct device *parent, struct fpga_manager *mgr,
>>> - int (*get_bridges)(struct fpga_region *))
>>> +__fpga_region_register(struct device *parent, struct fpga_manager *mgr,
>>> + int (*get_bridges)(struct fpga_region *), struct module *owner)
>>> {
>>> struct fpga_region_info info = { 0 };
>>>
>>> info.mgr = mgr;
>>> info.get_bridges = get_bridges;
>>>
>>> - return fpga_region_register_full(parent, &info);
>>> + return __fpga_region_register_full(parent, &info, owner);
>>> }
>>> -EXPORT_SYMBOL_GPL(fpga_region_register);
>>> +EXPORT_SYMBOL_GPL(__fpga_region_register);
>>>
>>> /**
>>> * fpga_region_unregister - unregister an FPGA region
>>> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
>>> index 9d4d32909340..5fbc05fe70a6 100644
>>> --- a/include/linux/fpga/fpga-region.h
>>> +++ b/include/linux/fpga/fpga-region.h
>>> @@ -36,6 +36,7 @@ struct fpga_region_info {
>>> * @mgr: FPGA manager
>>> * @info: FPGA image info
>>> * @compat_id: FPGA region id for compatibility check.
>>> + * @ops_owner: module containing the get_bridges function
>>
>> ditto
>>
>> Thanks,
>> Yilun
>>
>>> * @priv: private data
>>> * @get_bridges: optional function to get bridges to a list
>>> */
>>> @@ -46,6 +47,7 @@ struct fpga_region {
>>> struct fpga_manager *mgr;
>>> struct fpga_image_info *info;
>>> struct fpga_compat_id *compat_id;
>>> + struct module *ops_owner;
>>> void *priv;
>>> int (*get_bridges)(struct fpga_region *region);
>>> };
>>
On 2024-04-19 09:32, Xu Yilun wrote:
> On Mon, Apr 15, 2024 at 07:11:03PM +0200, Marco Pagani wrote:
>> On 2024-04-15 14:19, Marco Pagani wrote:
>>>
>>>
>>> On 2024-04-13 04:35, Xu Yilun wrote:
>>>>> /**
>>>>> - * fpga_region_register_full - create and register an FPGA Region device
>>>>> + * __fpga_region_register_full - create and register an FPGA Region device
>>>>> * @parent: device parent
>>>>> * @info: parameters for FPGA Region
>>>>> + * @owner: owner module containing the get_bridges function
>>>>
>>>> This is too specific and easily get unaligned if we add another
>>>> callback. How about "module containing the region ops"?
>>>
>>> I had some concerns about using the name "region ops" in the kernel-doc
>>> comment since it was not supported by a struct definition nor referenced
>>> in the documentation. However, since the name is now referred to in the
>>> ops_owner pointer, making the change makes sense to me.
>>>
>>
>> On second thought, I think it would be better to leave the @owner
>> description to "module containing the get_bridges function" for the
>> moment. Otherwise, it could confuse the user by blurring the connection
>> between the @owner and @get_bridges parameters.
>>
>> * __fpga_region_register - create and register an FPGA Region device
>> * [...]
>> * @get_bridges: optional function to get bridges to a list
>> * @owner: owner module containing get_bridges function
>>
>> We can always modify the @owner description later, together with all the
>> necessary changes to add a new op, like grouping all ops in a structure
>> and changing the registration function signature.
>
> OK, it's good to me. I'll apply this patch to for-next.
>
> Acked-by: Xu Yilun <[email protected]>
Thanks, I'll quickly send v6 to fix a minor typo in the kernel-doc:
- @owner: owner module containing get_bridges function
+ @owner: module containing the get_bridges function
Marco
>>>>> *
>>>>> * Return: struct fpga_region or ERR_PTR()
>>>>> */
>>>>> struct fpga_region *
>>>>> -fpga_region_register_full(struct device *parent, const struct fpga_region_info *info)
>>>>> +__fpga_region_register_full(struct device *parent, const struct fpga_region_info *info,
>>>>> + struct module *owner)
>>>>> {
>>>>> struct fpga_region *region;
>>>>> int id, ret = 0;
>>>>> @@ -213,6 +215,7 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
>>>>> region->compat_id = info->compat_id;
>>>>> region->priv = info->priv;
>>>>> region->get_bridges = info->get_bridges;
>>>>> + region->ops_owner = owner;
>>>>>
>>>>> mutex_init(®ion->mutex);
>>>>> INIT_LIST_HEAD(®ion->bridge_list);
>>>>> @@ -241,13 +244,14 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
>>>>>
>>>>> return ERR_PTR(ret);
>>>>> }
>>>>> -EXPORT_SYMBOL_GPL(fpga_region_register_full);
>>>>> +EXPORT_SYMBOL_GPL(__fpga_region_register_full);
>>>>>
>>>>> /**
>>>>> - * fpga_region_register - create and register an FPGA Region device
>>>>> + * __fpga_region_register - create and register an FPGA Region device
>>>>> * @parent: device parent
>>>>> * @mgr: manager that programs this region
>>>>> * @get_bridges: optional function to get bridges to a list
>>>>> + * @owner: owner module containing get_bridges function
>>>>
>>>> ditto
>>>>
>>>>> *
>>>>> * This simple version of the register function should be sufficient for most users.
>>>>> * The fpga_region_register_full() function is available for users that need to
>>>>> @@ -256,17 +260,17 @@ EXPORT_SYMBOL_GPL(fpga_region_register_full);
>>>>> * Return: struct fpga_region or ERR_PTR()
>>>>> */
>>>>> struct fpga_region *
>>>>> -fpga_region_register(struct device *parent, struct fpga_manager *mgr,
>>>>> - int (*get_bridges)(struct fpga_region *))
>>>>> +__fpga_region_register(struct device *parent, struct fpga_manager *mgr,
>>>>> + int (*get_bridges)(struct fpga_region *), struct module *owner)
>>>>> {
>>>>> struct fpga_region_info info = { 0 };
>>>>>
>>>>> info.mgr = mgr;
>>>>> info.get_bridges = get_bridges;
>>>>>
>>>>> - return fpga_region_register_full(parent, &info);
>>>>> + return __fpga_region_register_full(parent, &info, owner);
>>>>> }
>>>>> -EXPORT_SYMBOL_GPL(fpga_region_register);
>>>>> +EXPORT_SYMBOL_GPL(__fpga_region_register);
>>>>>
>>>>> /**
>>>>> * fpga_region_unregister - unregister an FPGA region
>>>>> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
>>>>> index 9d4d32909340..5fbc05fe70a6 100644
>>>>> --- a/include/linux/fpga/fpga-region.h
>>>>> +++ b/include/linux/fpga/fpga-region.h
>>>>> @@ -36,6 +36,7 @@ struct fpga_region_info {
>>>>> * @mgr: FPGA manager
>>>>> * @info: FPGA image info
>>>>> * @compat_id: FPGA region id for compatibility check.
>>>>> + * @ops_owner: module containing the get_bridges function
>>>>
>>>> ditto
>>>>
>>>> Thanks,
>>>> Yilun
>>>>
>>>>> * @priv: private data
>>>>> * @get_bridges: optional function to get bridges to a list
>>>>> */
>>>>> @@ -46,6 +47,7 @@ struct fpga_region {
>>>>> struct fpga_manager *mgr;
>>>>> struct fpga_image_info *info;
>>>>> struct fpga_compat_id *compat_id;
>>>>> + struct module *ops_owner;
>>>>> void *priv;
>>>>> int (*get_bridges)(struct fpga_region *region);
>>>>> };
>>>>
>>
>
On Mon, Apr 15, 2024 at 07:11:03PM +0200, Marco Pagani wrote:
> On 2024-04-15 14:19, Marco Pagani wrote:
> >
> >
> > On 2024-04-13 04:35, Xu Yilun wrote:
> >>> /**
> >>> - * fpga_region_register_full - create and register an FPGA Region device
> >>> + * __fpga_region_register_full - create and register an FPGA Region device
> >>> * @parent: device parent
> >>> * @info: parameters for FPGA Region
> >>> + * @owner: owner module containing the get_bridges function
> >>
> >> This is too specific and easily get unaligned if we add another
> >> callback. How about "module containing the region ops"?
> >
> > I had some concerns about using the name "region ops" in the kernel-doc
> > comment since it was not supported by a struct definition nor referenced
> > in the documentation. However, since the name is now referred to in the
> > ops_owner pointer, making the change makes sense to me.
> >
>
> On second thought, I think it would be better to leave the @owner
> description to "module containing the get_bridges function" for the
> moment. Otherwise, it could confuse the user by blurring the connection
> between the @owner and @get_bridges parameters.
>
> * __fpga_region_register - create and register an FPGA Region device
> * [...]
> * @get_bridges: optional function to get bridges to a list
> * @owner: owner module containing get_bridges function
>
> We can always modify the @owner description later, together with all the
> necessary changes to add a new op, like grouping all ops in a structure
> and changing the registration function signature.
OK, it's good to me. I'll apply this patch to for-next.
Acked-by: Xu Yilun <[email protected]>
>
> Thanks,
> Marco
>
> >
> >>
> >>> *
> >>> * Return: struct fpga_region or ERR_PTR()
> >>> */
> >>> struct fpga_region *
> >>> -fpga_region_register_full(struct device *parent, const struct fpga_region_info *info)
> >>> +__fpga_region_register_full(struct device *parent, const struct fpga_region_info *info,
> >>> + struct module *owner)
> >>> {
> >>> struct fpga_region *region;
> >>> int id, ret = 0;
> >>> @@ -213,6 +215,7 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
> >>> region->compat_id = info->compat_id;
> >>> region->priv = info->priv;
> >>> region->get_bridges = info->get_bridges;
> >>> + region->ops_owner = owner;
> >>>
> >>> mutex_init(®ion->mutex);
> >>> INIT_LIST_HEAD(®ion->bridge_list);
> >>> @@ -241,13 +244,14 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
> >>>
> >>> return ERR_PTR(ret);
> >>> }
> >>> -EXPORT_SYMBOL_GPL(fpga_region_register_full);
> >>> +EXPORT_SYMBOL_GPL(__fpga_region_register_full);
> >>>
> >>> /**
> >>> - * fpga_region_register - create and register an FPGA Region device
> >>> + * __fpga_region_register - create and register an FPGA Region device
> >>> * @parent: device parent
> >>> * @mgr: manager that programs this region
> >>> * @get_bridges: optional function to get bridges to a list
> >>> + * @owner: owner module containing get_bridges function
> >>
> >> ditto
> >>
> >>> *
> >>> * This simple version of the register function should be sufficient for most users.
> >>> * The fpga_region_register_full() function is available for users that need to
> >>> @@ -256,17 +260,17 @@ EXPORT_SYMBOL_GPL(fpga_region_register_full);
> >>> * Return: struct fpga_region or ERR_PTR()
> >>> */
> >>> struct fpga_region *
> >>> -fpga_region_register(struct device *parent, struct fpga_manager *mgr,
> >>> - int (*get_bridges)(struct fpga_region *))
> >>> +__fpga_region_register(struct device *parent, struct fpga_manager *mgr,
> >>> + int (*get_bridges)(struct fpga_region *), struct module *owner)
> >>> {
> >>> struct fpga_region_info info = { 0 };
> >>>
> >>> info.mgr = mgr;
> >>> info.get_bridges = get_bridges;
> >>>
> >>> - return fpga_region_register_full(parent, &info);
> >>> + return __fpga_region_register_full(parent, &info, owner);
> >>> }
> >>> -EXPORT_SYMBOL_GPL(fpga_region_register);
> >>> +EXPORT_SYMBOL_GPL(__fpga_region_register);
> >>>
> >>> /**
> >>> * fpga_region_unregister - unregister an FPGA region
> >>> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
> >>> index 9d4d32909340..5fbc05fe70a6 100644
> >>> --- a/include/linux/fpga/fpga-region.h
> >>> +++ b/include/linux/fpga/fpga-region.h
> >>> @@ -36,6 +36,7 @@ struct fpga_region_info {
> >>> * @mgr: FPGA manager
> >>> * @info: FPGA image info
> >>> * @compat_id: FPGA region id for compatibility check.
> >>> + * @ops_owner: module containing the get_bridges function
> >>
> >> ditto
> >>
> >> Thanks,
> >> Yilun
> >>
> >>> * @priv: private data
> >>> * @get_bridges: optional function to get bridges to a list
> >>> */
> >>> @@ -46,6 +47,7 @@ struct fpga_region {
> >>> struct fpga_manager *mgr;
> >>> struct fpga_image_info *info;
> >>> struct fpga_compat_id *compat_id;
> >>> + struct module *ops_owner;
> >>> void *priv;
> >>> int (*get_bridges)(struct fpga_region *region);
> >>> };
> >>
>