Message ID | 20240403144611.65507-1-marpagan@redhat.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [v4] fpga: region: add owner module and take its refcount | expand |
On Wed, Apr 03, 2024 at 04:46:09PM +0200, Marco Pagani wrote: > 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 <gregkh@linuxfoundation.org> > Suggested-by: Xu Yilun <yilun.xu@intel.com> > Reviewed-by: Russ Weight <russ.weight@linux.dev> > Signed-off-by: Marco Pagani <marpagan@redhat.com> > --- > > 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 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..77190a5ef330 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 > > .. kernel-doc:: drivers/fpga/fpga-region.c > - :functions: fpga_region_register > + :functions: __fpga_region_register_full Why you swap the order? You didn't do that for fpga-mgr. > > .. 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..d50ab1509989 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->br_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->br_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->br_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..d175babc3d68 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. > + * @br_owner: module containing the get_bridges function I'm a little confused that you call it br_owner, just because there is only one get_bridge() callback provided by low-level module. If we further have another callback, the name & all the doc would be a problem. And It is really the owner of the region module, not the bridge module. Maybe just name it owner, or ops_owner? 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 *br_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: b1a91ca25f15b6d7b311de4465854a5981dee3d3 > -- > 2.44.0 > >
On 2024-04-09 06:34, Xu Yilun wrote: > On Wed, Apr 03, 2024 at 04:46:09PM +0200, Marco Pagani wrote: >> 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 <gregkh@linuxfoundation.org> >> Suggested-by: Xu Yilun <yilun.xu@intel.com> >> Reviewed-by: Russ Weight <russ.weight@linux.dev> >> Signed-off-by: Marco Pagani <marpagan@redhat.com> >> --- >> >> 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 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..77190a5ef330 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 >> >> .. kernel-doc:: drivers/fpga/fpga-region.c >> - :functions: fpga_region_register >> + :functions: __fpga_region_register_full > > Why you swap the order? You didn't do that for fpga-mgr. Ouch, it's a typo. >> >> .. 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..d50ab1509989 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->br_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->br_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->br_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..d175babc3d68 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. >> + * @br_owner: module containing the get_bridges function > > I'm a little confused that you call it br_owner, just because there is > only one get_bridge() callback provided by low-level module. If we > further have another callback, the name & all the doc would be a problem. > And It is really the owner of the region module, not the bridge module. > > Maybe just name it owner, or ops_owner? Right, it makes sense to me. How about rg_ops_owner for symmetry with the manager and bridge? Thanks, Marco
On Wed, Apr 10, 2024 at 11:49:34AM +0200, Marco Pagani wrote: > > > On 2024-04-09 06:34, Xu Yilun wrote: > > On Wed, Apr 03, 2024 at 04:46:09PM +0200, Marco Pagani wrote: > >> 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 <gregkh@linuxfoundation.org> > >> Suggested-by: Xu Yilun <yilun.xu@intel.com> > >> Reviewed-by: Russ Weight <russ.weight@linux.dev> > >> Signed-off-by: Marco Pagani <marpagan@redhat.com> > >> --- > >> > >> 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 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..77190a5ef330 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 > >> > >> .. kernel-doc:: drivers/fpga/fpga-region.c > >> - :functions: fpga_region_register > >> + :functions: __fpga_region_register_full > > > > Why you swap the order? You didn't do that for fpga-mgr. > > Ouch, it's a typo. > > >> > >> .. 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..d50ab1509989 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->br_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->br_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->br_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..d175babc3d68 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. > >> + * @br_owner: module containing the get_bridges function > > > > I'm a little confused that you call it br_owner, just because there is > > only one get_bridge() callback provided by low-level module. If we > > further have another callback, the name & all the doc would be a problem. > > And It is really the owner of the region module, not the bridge module. > > > > Maybe just name it owner, or ops_owner? > > Right, it makes sense to me. How about rg_ops_owner for symmetry with > the manager and bridge? I think ops_owner is better. First it is a member for region related structure, no need to emphasize the 'region' again. Second I didn't see any 'rg' in fpga core files, I think it is less understandable to everyone. 'br' is more widely used, so it's OK. Thanks, Yilun. > > > Thanks, > Marco > >
diff --git a/Documentation/driver-api/fpga/fpga-region.rst b/Documentation/driver-api/fpga/fpga-region.rst index dc55d60a0b4a..77190a5ef330 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 .. kernel-doc:: drivers/fpga/fpga-region.c - :functions: fpga_region_register + :functions: __fpga_region_register_full .. 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..d50ab1509989 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->br_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->br_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->br_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..d175babc3d68 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. + * @br_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 *br_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 */