[PATCH v4] IB/IPoIB: ibX: failed to create mcg debug file

[PATCH v4] IB/IPoIB: ibX: failed to create mcg debug file

[Shamir Rabinovitch]

When udev renames the netdev devices, ipoib debugfs entries does not
get renamed. As a result, if subsequent probe of ipoib device reuse the
name then creating a debugfs entry for the new device would fail.

Also, moved ipoib_create_debug_files and ipoib_delete_debug_files as part
of ipoib event handling in order to avoid any race condition between these.

Signed-off-by: Vijay Kumar <vijay.ac.kumar-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

---
v3->v4:
- Review comment from Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
  Fixed duplicate delete of debug files. Debug files no longer 
  deleted in multiple locations. Debug files are only created/
  deleted as result of NETDEV_XXX events. Verified the events
  work well for ipoib child (vlan) interfaces as well. I do see
  that function 'netdev_wait_allrefs' has the potential to send
  the netdev multiple NETDEV_UNREGISTER events. I am not sure
  if this scenario applicable to ipoib but I added protection
  measures and WARN print in 'ipoib_delete_debug_files' to catch
  and handle such (potential) issue gracefully.
- Review comment from Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
  When the above was fixed, I could then try out what Leon suggested
  and figured that the idea to replace 'ipoib_debugfs_rename'
  with 'ipoib_delete_debug_files' and then 'ipoib_create_debug_files'
  is working fine.
v2->v3:
- Move rev change out of commit message
- Fix typo
v1->v2:
- Review comment from Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
  Verified again and NETDEV_UNREGISTER is called correctly. Debug
  files are removed as expected. Thanks.
- Fix compile warning when CONFIG_INFINIBAND_IPOIB_DEBUG is not set
---

---
 drivers/infiniband/ulp/ipoib/ipoib_fs.c   |    3 ++
 drivers/infiniband/ulp/ipoib/ipoib_main.c |   44 +++++++++++++++++++++++++---
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c |    3 --
 3 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_fs.c b/drivers/infiniband/ulp/ipoib/ipoib_fs.c
index 6bd5740..09396bd 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_fs.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_fs.c
@@ -281,8 +281,11 @@ void ipoib_delete_debug_files(struct net_device *dev)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
 
+	WARN_ONCE(!priv->mcg_dentry, "null mcg debug file\n");
+	WARN_ONCE(!priv->path_dentry, "null path debug file\n");
 	debugfs_remove(priv->mcg_dentry);
 	debugfs_remove(priv->path_dentry);
+	priv->mcg_dentry = priv->path_dentry = NULL;
 }
 
 int ipoib_register_debugfs(void)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index b58d9dc..a10a38f 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -108,6 +108,33 @@ struct ipoib_path_iter {
 	.get_net_dev_by_params = ipoib_get_net_dev_by_params,
 };
 
+#ifdef CONFIG_INFINIBAND_IPOIB_DEBUG
+static int ipoib_netdev_event(struct notifier_block *this,
+			      unsigned long event, void *ptr)
+{
+	struct netdev_notifier_info *ni = ptr;
+	struct net_device *dev = ni->dev;
+
+	if (dev->netdev_ops->ndo_open != ipoib_open)
+		return NOTIFY_DONE;
+
+	switch (event) {
+	case NETDEV_REGISTER:
+		ipoib_create_debug_files(dev);
+		break;
+	case NETDEV_CHANGENAME:
+		ipoib_delete_debug_files(dev);
+		ipoib_create_debug_files(dev);
+		break;
+	case NETDEV_UNREGISTER:
+		ipoib_delete_debug_files(dev);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+#endif
+
 int ipoib_open(struct net_device *dev)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
@@ -1653,8 +1680,6 @@ void ipoib_dev_cleanup(struct net_device *dev)
 
 	ASSERT_RTNL();
 
-	ipoib_delete_debug_files(dev);
-
 	/* Delete any child interfaces first */
 	list_for_each_entry_safe(cpriv, tcpriv, &priv->child_intfs, list) {
 		/* Stop GC on child */
@@ -2072,8 +2097,6 @@ int ipoib_set_dev_features(struct ipoib_dev_priv *priv, struct ib_device *hca)
 		goto register_failed;
 	}
 
-	ipoib_create_debug_files(priv->dev);
-
 	if (ipoib_cm_add_mode_attr(priv->dev))
 		goto sysfs_failed;
 	if (ipoib_add_pkey_attr(priv->dev))
@@ -2088,7 +2111,6 @@ int ipoib_set_dev_features(struct ipoib_dev_priv *priv, struct ib_device *hca)
 	return priv->dev;
 
 sysfs_failed:
-	ipoib_delete_debug_files(priv->dev);
 	unregister_netdev(priv->dev);
 
 register_failed:
@@ -2173,6 +2195,12 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)
 	kfree(dev_list);
 }
 
+#ifdef CONFIG_INFINIBAND_IPOIB_DEBUG
+static struct notifier_block ipoib_netdev_notifier = {
+	.notifier_call = ipoib_netdev_event,
+};
+#endif
+
 static int __init ipoib_init_module(void)
 {
 	int ret;
@@ -2225,6 +2253,9 @@ static int __init ipoib_init_module(void)
 	if (ret)
 		goto err_client;
 
+#ifdef CONFIG_INFINIBAND_IPOIB_DEBUG
+	register_netdevice_notifier(&ipoib_netdev_notifier);
+#endif
 	return 0;
 
 err_client:
@@ -2242,6 +2273,9 @@ static int __init ipoib_init_module(void)
 
 static void __exit ipoib_cleanup_module(void)
 {
+#ifdef CONFIG_INFINIBAND_IPOIB_DEBUG
+	unregister_netdevice_notifier(&ipoib_netdev_notifier);
+#endif
 	ipoib_netlink_fini();
 	ib_unregister_client(&ipoib_client);
 	ib_sa_unregister_client(&ipoib_sa_client);
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
index a2f9f29..57eadd2 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
@@ -87,8 +87,6 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
 		goto register_failed;
 	}
 
-	ipoib_create_debug_files(priv->dev);
-
 	/* RTNL childs don't need proprietary sysfs entries */
 	if (type == IPOIB_LEGACY_CHILD) {
 		if (ipoib_cm_add_mode_attr(priv->dev))
@@ -109,7 +107,6 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
 
 sysfs_failed:
 	result = -ENOMEM;
-	ipoib_delete_debug_files(priv->dev);
 	unregister_netdevice(priv->dev);
 
 register_failed:
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] IB/IPoIB: ibX: failed to create mcg debug file

[Mark Bloch]

Hi Shamir,

On 29/03/2017 13:21, Shamir Rabinovitch wrote:
> When udev renames the netdev devices, ipoib debugfs entries does not
> get renamed. As a result, if subsequent probe of ipoib device reuse the
> name then creating a debugfs entry for the new device would fail.
> 
> Also, moved ipoib_create_debug_files and ipoib_delete_debug_files as part
> of ipoib event handling in order to avoid any race condition between these.
> 
> Signed-off-by: Vijay Kumar <vijay.ac.kumar-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> 
> ---
> v3->v4:
> - Review comment from Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>   Fixed duplicate delete of debug files. Debug files no longer 
>   deleted in multiple locations. Debug files are only created/
>   deleted as result of NETDEV_XXX events. Verified the events
>   work well for ipoib child (vlan) interfaces as well. I do see
>   that function 'netdev_wait_allrefs' has the potential to send
>   the netdev multiple NETDEV_UNREGISTER events. I am not sure
>   if this scenario applicable to ipoib but I added protection
>   measures and WARN print in 'ipoib_delete_debug_files' to catch
>   and handle such (potential) issue gracefully.
> - Review comment from Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>   When the above was fixed, I could then try out what Leon suggested
>   and figured that the idea to replace 'ipoib_debugfs_rename'
>   with 'ipoib_delete_debug_files' and then 'ipoib_create_debug_files'
>   is working fine.
> v2->v3:
> - Move rev change out of commit message
> - Fix typo
> v1->v2:
> - Review comment from Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>   Verified again and NETDEV_UNREGISTER is called correctly. Debug
>   files are removed as expected. Thanks.
> - Fix compile warning when CONFIG_INFINIBAND_IPOIB_DEBUG is not set
> ---
> 
> ---
>  drivers/infiniband/ulp/ipoib/ipoib_fs.c   |    3 ++
>  drivers/infiniband/ulp/ipoib/ipoib_main.c |   44 +++++++++++++++++++++++++---
>  drivers/infiniband/ulp/ipoib/ipoib_vlan.c |    3 --
>  3 files changed, 42 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_fs.c b/drivers/infiniband/ulp/ipoib/ipoib_fs.c
> index 6bd5740..09396bd 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_fs.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_fs.c
> @@ -281,8 +281,11 @@ void ipoib_delete_debug_files(struct net_device *dev)
>  {
>  	struct ipoib_dev_priv *priv = netdev_priv(dev);
>  
> +	WARN_ONCE(!priv->mcg_dentry, "null mcg debug file\n");
> +	WARN_ONCE(!priv->path_dentry, "null path debug file\n");
>  	debugfs_remove(priv->mcg_dentry);
>  	debugfs_remove(priv->path_dentry);
> +	priv->mcg_dentry = priv->path_dentry = NULL;
>  }
>  
>  int ipoib_register_debugfs(void)
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index b58d9dc..a10a38f 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -108,6 +108,33 @@ struct ipoib_path_iter {
>  	.get_net_dev_by_params = ipoib_get_net_dev_by_params,
>  };
>  
> +#ifdef CONFIG_INFINIBAND_IPOIB_DEBUG
> +static int ipoib_netdev_event(struct notifier_block *this,
> +			      unsigned long event, void *ptr)
> +{
> +	struct netdev_notifier_info *ni = ptr;
> +	struct net_device *dev = ni->dev;
> +
> +	if (dev->netdev_ops->ndo_open != ipoib_open)
> +		return NOTIFY_DONE;
> +
> +	switch (event) {
> +	case NETDEV_REGISTER:
> +		ipoib_create_debug_files(dev);
> +		break;
> +	case NETDEV_CHANGENAME:
> +		ipoib_delete_debug_files(dev);
> +		ipoib_create_debug_files(dev);
> +		break;
> +	case NETDEV_UNREGISTER:
> +		ipoib_delete_debug_files(dev);
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +#endif
> +
>  int ipoib_open(struct net_device *dev)
>  {
>  	struct ipoib_dev_priv *priv = netdev_priv(dev);
> @@ -1653,8 +1680,6 @@ void ipoib_dev_cleanup(struct net_device *dev)
>  
>  	ASSERT_RTNL();
>  
> -	ipoib_delete_debug_files(dev);
> -
>  	/* Delete any child interfaces first */
>  	list_for_each_entry_safe(cpriv, tcpriv, &priv->child_intfs, list) {
>  		/* Stop GC on child */
> @@ -2072,8 +2097,6 @@ int ipoib_set_dev_features(struct ipoib_dev_priv *priv, struct ib_device *hca)
>  		goto register_failed;
>  	}
>  
> -	ipoib_create_debug_files(priv->dev);
> -
>  	if (ipoib_cm_add_mode_attr(priv->dev))
>  		goto sysfs_failed;
>  	if (ipoib_add_pkey_attr(priv->dev))
> @@ -2088,7 +2111,6 @@ int ipoib_set_dev_features(struct ipoib_dev_priv *priv, struct ib_device *hca)
>  	return priv->dev;
>  
>  sysfs_failed:
> -	ipoib_delete_debug_files(priv->dev);
>  	unregister_netdev(priv->dev);
>  
>  register_failed:
> @@ -2173,6 +2195,12 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)
>  	kfree(dev_list);
>  }
>  
> +#ifdef CONFIG_INFINIBAND_IPOIB_DEBUG
> +static struct notifier_block ipoib_netdev_notifier = {
> +	.notifier_call = ipoib_netdev_event,
> +};
> +#endif
> +
>  static int __init ipoib_init_module(void)
>  {
>  	int ret;
> @@ -2225,6 +2253,9 @@ static int __init ipoib_init_module(void)
>  	if (ret)
>  		goto err_client;
>  
> +#ifdef CONFIG_INFINIBAND_IPOIB_DEBUG
> +	register_netdevice_notifier(&ipoib_netdev_notifier);
> +#endif
>  	return 0;
>  
>  err_client:
> @@ -2242,6 +2273,9 @@ static int __init ipoib_init_module(void)
>  
>  static void __exit ipoib_cleanup_module(void)
>  {
> +#ifdef CONFIG_INFINIBAND_IPOIB_DEBUG
> +	unregister_netdevice_notifier(&ipoib_netdev_notifier);
> +#endif
>  	ipoib_netlink_fini();
>  	ib_unregister_client(&ipoib_client);
>  	ib_sa_unregister_client(&ipoib_sa_client);
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
> index a2f9f29..57eadd2 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
> @@ -87,8 +87,6 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
>  		goto register_failed;
>  	}
>  
> -	ipoib_create_debug_files(priv->dev);
> -
>  	/* RTNL childs don't need proprietary sysfs entries */
>  	if (type == IPOIB_LEGACY_CHILD) {
>  		if (ipoib_cm_add_mode_attr(priv->dev))
> @@ -109,7 +107,6 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
>  
>  sysfs_failed:
>  	result = -ENOMEM;
> -	ipoib_delete_debug_files(priv->dev);
>  	unregister_netdevice(priv->dev);
>  
>  register_failed:
> 
I liked the notation of a rename (and not delete & create), but I 
don't feel strongly about it.

I know the commit that added debugfs is older than the universe, but maybe you can
use the commit below as a fixes line/queue it to stable?

commit 1732b0ef3b3a02e3df328086fb3018741c5476da
Author: Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
Date:   Mon Nov 7 10:33:11 2005 -0800

    [IPoIB] add path record information in debugfs

    Add ibX_path files to debugfs that contain information about the IPoIB
    path cache.  IPoIB ARP only gives GIDs, which the IPoIB driver must
    resolve to real IB paths through the ib_sa module.  For debugging,
    when the ARP table looks OK but traffic isn't flowing, it's useful to
    be able to see if the resolution from GID to path worked.

    Also clean up the formatting of the existing _mcg debugfs files.

    Signed-off-by: Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>


Other than that, looks good.
Reviewed-by: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] IB/IPoIB: ibX: failed to create mcg debug file

[Doug Ledford]

On Wed, 2017-03-29 at 16:20 +0300, Mark Bloch wrote:
> I know the commit that added debugfs is older than the universe, but
> maybe you can
> use the commit below as a fixes line/queue it to stable?
> 
> commit 1732b0ef3b3a02e3df328086fb3018741c5476da
> Author: Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
> Date:   Mon Nov 7 10:33:11 2005 -0800

I added a Fixes: and Cc: tags for stable, patch applied, thanks.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html