| Message ID | 20181123122122.18957-1-thierry.reding@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: stmmac: Move debugfs init/exit to ->probe()/->remove() | expand |
On 23-11-2018 12:21, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > Setting up and tearing down debugfs is current unbalanced, as seen by > this error during resume from suspend: > > [ 752.134067] dwc-eth-dwmac 2490000.ethernet eth0: ERROR failed to create debugfs directory > [ 752.134347] dwc-eth-dwmac 2490000.ethernet eth0: stmmac_hw_setup: failed debugFS registration > > The imbalance happens because the driver creates the debugfs hierarchy > when the device is opened and tears it down when the device is closed. > There's little gain in that, and it could be argued that it is even > surprising because it's not usually done for other devices. Fix the > imbalance by moving the debugfs creation and teardown to the driver's > ->probe() and ->remove() implementations instead. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > Did you test trying to dump "descriptors_status" file when interface is not open ? I think that's the main reason why this is not in probe ... Thanks and Best Regards, Jose Miguel Abreu
On Fri, Nov 23, 2018 at 12:44:02PM +0000, Jose Abreu wrote: > On 23-11-2018 12:21, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > Setting up and tearing down debugfs is current unbalanced, as seen by > > this error during resume from suspend: > > > > [ 752.134067] dwc-eth-dwmac 2490000.ethernet eth0: ERROR failed to create debugfs directory > > [ 752.134347] dwc-eth-dwmac 2490000.ethernet eth0: stmmac_hw_setup: failed debugFS registration > > > > The imbalance happens because the driver creates the debugfs hierarchy > > when the device is opened and tears it down when the device is closed. > > There's little gain in that, and it could be argued that it is even > > surprising because it's not usually done for other devices. Fix the > > imbalance by moving the debugfs creation and teardown to the driver's > > ->probe() and ->remove() implementations instead. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > > > Did you test trying to dump "descriptors_status" file when > interface is not open ? I think that's the main reason why this > is not in probe ... I can't say that I did. I'll run a couple more tests and see if I can fix both of the issues. Thierry
On Fri, Nov 23, 2018 at 12:44:02PM +0000, Jose Abreu wrote: > On 23-11-2018 12:21, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > Setting up and tearing down debugfs is current unbalanced, as seen by > > this error during resume from suspend: > > > > [ 752.134067] dwc-eth-dwmac 2490000.ethernet eth0: ERROR failed to create debugfs directory > > [ 752.134347] dwc-eth-dwmac 2490000.ethernet eth0: stmmac_hw_setup: failed debugFS registration > > > > The imbalance happens because the driver creates the debugfs hierarchy > > when the device is opened and tears it down when the device is closed. > > There's little gain in that, and it could be argued that it is even > > surprising because it's not usually done for other devices. Fix the > > imbalance by moving the debugfs creation and teardown to the driver's > > ->probe() and ->remove() implementations instead. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > > > Did you test trying to dump "descriptors_status" file when > interface is not open ? I think that's the main reason why this > is not in probe ... Indeed, that seems to cause a hang. Still, it doesn't sound like the right things to repeatedly create and remove debugfs files just because we can't provide the contents when the device is down. How about we return an empty file or an error code instead when the interface is down? Thierry
On 26-11-2018 15:34, Thierry Reding wrote: > On Fri, Nov 23, 2018 at 12:44:02PM +0000, Jose Abreu wrote: >> On 23-11-2018 12:21, Thierry Reding wrote: >>> From: Thierry Reding <treding@nvidia.com> >>> >>> Setting up and tearing down debugfs is current unbalanced, as seen by >>> this error during resume from suspend: >>> >>> [ 752.134067] dwc-eth-dwmac 2490000.ethernet eth0: ERROR failed to create debugfs directory >>> [ 752.134347] dwc-eth-dwmac 2490000.ethernet eth0: stmmac_hw_setup: failed debugFS registration >>> >>> The imbalance happens because the driver creates the debugfs hierarchy >>> when the device is opened and tears it down when the device is closed. >>> There's little gain in that, and it could be argued that it is even >>> surprising because it's not usually done for other devices. Fix the >>> imbalance by moving the debugfs creation and teardown to the driver's >>> ->probe() and ->remove() implementations instead. >>> >>> Signed-off-by: Thierry Reding <treding@nvidia.com> >>> --- >>> >> Did you test trying to dump "descriptors_status" file when >> interface is not open ? I think that's the main reason why this >> is not in probe ... > Indeed, that seems to cause a hang. Still, it doesn't sound like the > right things to repeatedly create and remove debugfs files just because > we can't provide the contents when the device is down. Agree. > > How about we return an empty file or an error code instead when the > interface is down? I think an error code would be more suitable (ENODEV/EBUSY ?). Can you submit v2 ? Thanks and Best Regards, Jose Miguel Abreu > > Thierry
On Tue, Nov 27, 2018 at 09:02:51AM +0000, Jose Abreu wrote: > On 26-11-2018 15:34, Thierry Reding wrote: > > On Fri, Nov 23, 2018 at 12:44:02PM +0000, Jose Abreu wrote: > >> On 23-11-2018 12:21, Thierry Reding wrote: > >>> From: Thierry Reding <treding@nvidia.com> > >>> > >>> Setting up and tearing down debugfs is current unbalanced, as seen by > >>> this error during resume from suspend: > >>> > >>> [ 752.134067] dwc-eth-dwmac 2490000.ethernet eth0: ERROR failed to create debugfs directory > >>> [ 752.134347] dwc-eth-dwmac 2490000.ethernet eth0: stmmac_hw_setup: failed debugFS registration > >>> > >>> The imbalance happens because the driver creates the debugfs hierarchy > >>> when the device is opened and tears it down when the device is closed. > >>> There's little gain in that, and it could be argued that it is even > >>> surprising because it's not usually done for other devices. Fix the > >>> imbalance by moving the debugfs creation and teardown to the driver's > >>> ->probe() and ->remove() implementations instead. > >>> > >>> Signed-off-by: Thierry Reding <treding@nvidia.com> > >>> --- > >>> > >> Did you test trying to dump "descriptors_status" file when > >> interface is not open ? I think that's the main reason why this > >> is not in probe ... > > Indeed, that seems to cause a hang. Still, it doesn't sound like the > > right things to repeatedly create and remove debugfs files just because > > we can't provide the contents when the device is down. > > Agree. > > > > > How about we return an empty file or an error code instead when the > > interface is down? > > I think an error code would be more suitable (ENODEV/EBUSY ?). > Can you submit v2 ? I submitted a v2 earlier but wanted to elaborate why I ended up going with an empty file instead of an error code. None of the error codes seem like a good fit. For example ENODEV might lead people to think that somehow the device was removed, whereas EBUSY could be misinterpreted as the device being busy and therefore being unable to dump the status of the rings. I was leaning towards EPERM in the end, but then thought that it also was ambiguous because it could be interpreted as meaning the user lacked the permissions to query the rings status. So in the end, it occurred to me that reading the rings status for an interface that was down wasn't really an error. It's just that there's no ring structures to dump, so the most logical way to return that was with success but an empty file. Thierry
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 076a8be18d67..3ced86ac2ab1 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -2550,12 +2550,6 @@ static int stmmac_hw_setup(struct net_device *dev, bool init_ptp) netdev_warn(priv->dev, "PTP init failed\n"); } -#ifdef CONFIG_DEBUG_FS - ret = stmmac_init_fs(dev); - if (ret < 0) - netdev_warn(priv->dev, "%s: failed debugFS registration\n", - __func__); -#endif priv->tx_lpi_timer = STMMAC_DEFAULT_TWT_LS; if (priv->use_riwt) { @@ -2756,10 +2750,6 @@ static int stmmac_release(struct net_device *dev) netif_carrier_off(dev); -#ifdef CONFIG_DEBUG_FS - stmmac_exit_fs(dev); -#endif - stmmac_release_ptp(priv); return 0; @@ -4397,6 +4387,13 @@ int stmmac_dvr_probe(struct device *device, goto error_netdev_register; } +#ifdef CONFIG_DEBUG_FS + ret = stmmac_init_fs(ndev); + if (ret < 0) + netdev_warn(priv->dev, "%s: failed debugFS registration\n", + __func__); +#endif + return ret; error_netdev_register: @@ -4432,6 +4429,9 @@ int stmmac_dvr_remove(struct device *dev) netdev_info(priv->dev, "%s: removing driver", __func__); +#ifdef CONFIG_DEBUG_FS + stmmac_exit_fs(ndev); +#endif stmmac_stop_all_dma(priv); stmmac_mac_set(priv, priv->ioaddr, false);