| Message ID | 1490782919-24910-1-git-send-email-shamir.rabinovitch@oracle.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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@oracle.com> > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > > --- > v3->v4: > - Review comment from Mark Bloch <markb@mellanox.com> > 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@kernel.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@mellanox.com> > 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@cisco.com> 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@cisco.com> Other than that, looks good. Reviewed-by: Mark Bloch <markb@mellanox.com> Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@cisco.com> > Date: Mon Nov 7 10:33:11 2005 -0800 I added a Fixes: and Cc: tags for stable, patch applied, thanks.
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: