[RFC PATCH v4 0/1] fpga: improve protection against low-level control module unloading

[RFC PATCH v4 0/1] fpga: improve protection against low-level control module unloading

[Marco Pagani]

This RFC proposes a solution to keep protecting the fpga manager against
the unloading of the low-level control modules while addressing the
limitations of the current implementation. Currently, the code assumes
that the low-level module registers a driver for the parent device that
is later used to take the module's refcount. This proposal removes this
limitation by adding a module owner field to the fpga_manager struct
that can be set while registering the manager.

For more context, please refer to these threads:
https://lore.kernel.org/linux-fpga/ZS6hhlvjUcqyv8zL@yilunxu-OptiPlex-7050
https://lore.kernel.org/linux-fpga/ZT9qENE9fE3Z0KCW@yilunxu-OptiPlex-7050

v4:
- Use helper macros to set the owner module
v3:
- Improved locking
v2:
- Fixed protection against races during module removal

Marco Pagani (1):
  fpga: add an owner and use it to take the low-level module's refcount

 drivers/fpga/fpga-mgr.c       | 93 ++++++++++++++++++++++-------------
 include/linux/fpga/fpga-mgr.h | 80 +++++++++++++++++++++++++++---
 2 files changed, 134 insertions(+), 39 deletions(-)


base-commit: 610a9b8f49fbcf1100716370d3b5f6f884a2835a
-- 
2.43.0

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

[Marco Pagani]

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@intel.com>
Signed-off-by: Marco Pagani <marpagan@redhat.com>
---
 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))
+		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;
+
+	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
+ * @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);
+
 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()
+ */
+#define fpga_mgr_register_full(parent, info) \
+	__fpga_mgr_register_full(parent, info, THIS_MODULE)
+
 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);
+/**
+ * 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
+ *
+ * The caller of this function is responsible for calling fpga_mgr_unregister().
+ * Using devm_fpga_mgr_register() instead is recommended. This simple
+ * version of the register function should be sufficient for most users. The
+ * fpga_mgr_register_full() function is available for users that need to pass
+ * additional, optional parameters.
+ *
+ * Return: pointer to struct fpga_manager pointer or ERR_PTR()
+ */
+#define fpga_mgr_register(parent, name, mops, priv) \
+	__fpga_mgr_register(parent, name, mops, priv, THIS_MODULE)
 
 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);
+
 void fpga_mgr_unregister(struct fpga_manager *mgr);
 
+/**
+ * devm_fpga_mgr_register_full - resource managed variant of fpga_mgr_register()
+ * @parent:	fpga manager device from pdev
+ * @info:	parameters for fpga manager
+ *
+ * Return:  fpga manager pointer on success, negative error code otherwise.
+ *
+ * This is the devres variant of fpga_mgr_register_full() for which the unregister
+ * function will be called automatically when the managing device is detached.
+ */
+#define devm_fpga_mgr_register_full(parent, info) \
+	__devm_fpga_mgr_register_full(parent, info, THIS_MODULE)
+
 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);
+/**
+ * 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
+ *
+ * Return:  fpga manager pointer on success, negative error code otherwise.
+ *
+ * This is the devres variant of fpga_mgr_register() for which the
+ * unregister function will be called automatically when the managing
+ * device is detached.
+ */
+#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);
+__devm_fpga_mgr_register(struct device *parent, const char *name,
+			 const struct fpga_manager_ops *mops, void *priv,
+			 struct module *owner);
 
 #endif /*_LINUX_FPGA_MGR_H */
-- 
2.43.0

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

[Xu Yilun]

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@intel.com>
> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> ---
>  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.

> +		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;
> +
> +	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
> + * @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?

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

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

> +/**
> + * 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
> + *
> + * The caller of this function is responsible for calling fpga_mgr_unregister().
> + * Using devm_fpga_mgr_register() instead is recommended. This simple
> + * version of the register function should be sufficient for most users. The
> + * fpga_mgr_register_full() function is available for users that need to pass
> + * additional, optional parameters.
> + *
> + * Return: pointer to struct fpga_manager pointer or ERR_PTR()
> + */
> +#define fpga_mgr_register(parent, name, mops, priv) \
> +	__fpga_mgr_register(parent, name, mops, priv, THIS_MODULE)
>  
>  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);
> +
>  void fpga_mgr_unregister(struct fpga_manager *mgr);
>  
> +/**
> + * devm_fpga_mgr_register_full - resource managed variant of fpga_mgr_register()
> + * @parent:	fpga manager device from pdev
> + * @info:	parameters for fpga manager
> + *
> + * Return:  fpga manager pointer on success, negative error code otherwise.
> + *
> + * This is the devres variant of fpga_mgr_register_full() for which the unregister
> + * function will be called automatically when the managing device is detached.
> + */
> +#define devm_fpga_mgr_register_full(parent, info) \
> +	__devm_fpga_mgr_register_full(parent, info, THIS_MODULE)
> +
>  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);
> +/**
> + * 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
> + *
> + * Return:  fpga manager pointer on success, negative error code otherwise.
> + *
> + * This is the devres variant of fpga_mgr_register() for which the
> + * unregister function will be called automatically when the managing
> + * device is detached.
> + */
> +#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);
> +__devm_fpga_mgr_register(struct device *parent, const char *name,
> +			 const struct fpga_manager_ops *mops, void *priv,
> +			 struct module *owner);
>  
>  #endif /*_LINUX_FPGA_MGR_H */
> -- 
> 2.43.0
> 
> 

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

[Marco Pagani]

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@intel.com>
>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
>> ---
>>  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.

 
>> +		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;
>> +
>> +	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
>> + * @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);

> 
>> +/**
>> + * 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
>> + *
>> + * The caller of this function is responsible for calling fpga_mgr_unregister().
>> + * Using devm_fpga_mgr_register() instead is recommended. This simple
>> + * version of the register function should be sufficient for most users. The
>> + * fpga_mgr_register_full() function is available for users that need to pass
>> + * additional, optional parameters.
>> + *
>> + * Return: pointer to struct fpga_manager pointer or ERR_PTR()
>> + */
>> +#define fpga_mgr_register(parent, name, mops, priv) \
>> +	__fpga_mgr_register(parent, name, mops, priv, THIS_MODULE)
>>  
>>  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);
>> +
>>  void fpga_mgr_unregister(struct fpga_manager *mgr);
>>  
>> +/**
>> + * devm_fpga_mgr_register_full - resource managed variant of fpga_mgr_register()
>> + * @parent:	fpga manager device from pdev
>> + * @info:	parameters for fpga manager
>> + *
>> + * Return:  fpga manager pointer on success, negative error code otherwise.
>> + *
>> + * This is the devres variant of fpga_mgr_register_full() for which the unregister
>> + * function will be called automatically when the managing device is detached.
>> + */
>> +#define devm_fpga_mgr_register_full(parent, info) \
>> +	__devm_fpga_mgr_register_full(parent, info, THIS_MODULE)
>> +
>>  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);
>> +/**
>> + * 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
>> + *
>> + * Return:  fpga manager pointer on success, negative error code otherwise.
>> + *
>> + * This is the devres variant of fpga_mgr_register() for which the
>> + * unregister function will be called automatically when the managing
>> + * device is detached.
>> + */
>> +#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);
>> +__devm_fpga_mgr_register(struct device *parent, const char *name,
>> +			 const struct fpga_manager_ops *mops, void *priv,
>> +			 struct module *owner);
>>  
>>  #endif /*_LINUX_FPGA_MGR_H */
>> -- 
>> 2.43.0
>>
>>
> 

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

[Xu Yilun]

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@intel.com>
> >> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> >> ---
> >>  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?

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

Thanks,
Yilun

> 
> > 
> >> +/**
> >> + * 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
> >> + *
> >> + * The caller of this function is responsible for calling fpga_mgr_unregister().
> >> + * Using devm_fpga_mgr_register() instead is recommended. This simple
> >> + * version of the register function should be sufficient for most users. The
> >> + * fpga_mgr_register_full() function is available for users that need to pass
> >> + * additional, optional parameters.
> >> + *
> >> + * Return: pointer to struct fpga_manager pointer or ERR_PTR()
> >> + */
> >> +#define fpga_mgr_register(parent, name, mops, priv) \
> >> +	__fpga_mgr_register(parent, name, mops, priv, THIS_MODULE)
> >>  
> >>  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);
> >> +
> >>  void fpga_mgr_unregister(struct fpga_manager *mgr);
> >>  
> >> +/**
> >> + * devm_fpga_mgr_register_full - resource managed variant of fpga_mgr_register()
> >> + * @parent:	fpga manager device from pdev
> >> + * @info:	parameters for fpga manager
> >> + *
> >> + * Return:  fpga manager pointer on success, negative error code otherwise.
> >> + *
> >> + * This is the devres variant of fpga_mgr_register_full() for which the unregister
> >> + * function will be called automatically when the managing device is detached.
> >> + */
> >> +#define devm_fpga_mgr_register_full(parent, info) \
> >> +	__devm_fpga_mgr_register_full(parent, info, THIS_MODULE)
> >> +
> >>  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);
> >> +/**
> >> + * 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
> >> + *
> >> + * Return:  fpga manager pointer on success, negative error code otherwise.
> >> + *
> >> + * This is the devres variant of fpga_mgr_register() for which the
> >> + * unregister function will be called automatically when the managing
> >> + * device is detached.
> >> + */
> >> +#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);
> >> +__devm_fpga_mgr_register(struct device *parent, const char *name,
> >> +			 const struct fpga_manager_ops *mops, void *priv,
> >> +			 struct module *owner);
> >>  
> >>  #endif /*_LINUX_FPGA_MGR_H */
> >> -- 
> >> 2.43.0
> >>
> >>
> > 
> 

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

[Marco Pagani]

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@intel.com>
>>>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
>>>> ---
>>>>  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

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@intel.com>
> >>>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> >>>> ---
> >>>>  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
>