linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: stmmac: Move debugfs init/exit to ->probe()/->remove()
@ 2018-11-23 12:21 Thierry Reding
  2018-11-23 12:44 ` Jose Abreu
  2018-11-27 13:21 ` [PATCH v2] " Thierry Reding
  0 siblings, 2 replies; 10+ messages in thread
From: Thierry Reding @ 2018-11-23 12:21 UTC (permalink / raw)
  To: David S. Miller
  Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, netdev,
	linux-tegra, linux-kernel

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>
---
This applies on top of net-next.

 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 20 +++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

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);
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] net: stmmac: Move debugfs init/exit to ->probe()/->remove()
  2018-11-23 12:21 [PATCH] net: stmmac: Move debugfs init/exit to ->probe()/->remove() Thierry Reding
@ 2018-11-23 12:44 ` Jose Abreu
  2018-11-23 13:12   ` Thierry Reding
  2018-11-26 15:34   ` Thierry Reding
  2018-11-27 13:21 ` [PATCH v2] " Thierry Reding
  1 sibling, 2 replies; 10+ messages in thread
From: Jose Abreu @ 2018-11-23 12:44 UTC (permalink / raw)
  To: Thierry Reding, David S. Miller
  Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, netdev,
	linux-tegra, linux-kernel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] net: stmmac: Move debugfs init/exit to ->probe()/->remove()
  2018-11-23 12:44 ` Jose Abreu
@ 2018-11-23 13:12   ` Thierry Reding
  2018-11-26 15:34   ` Thierry Reding
  1 sibling, 0 replies; 10+ messages in thread
From: Thierry Reding @ 2018-11-23 13:12 UTC (permalink / raw)
  To: Jose Abreu
  Cc: David S. Miller, Giuseppe Cavallaro, Alexandre Torgue, netdev,
	linux-tegra, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] net: stmmac: Move debugfs init/exit to ->probe()/->remove()
  2018-11-23 12:44 ` Jose Abreu
  2018-11-23 13:12   ` Thierry Reding
@ 2018-11-26 15:34   ` Thierry Reding
  2018-11-27  9:02     ` Jose Abreu
  1 sibling, 1 reply; 10+ messages in thread
From: Thierry Reding @ 2018-11-26 15:34 UTC (permalink / raw)
  To: Jose Abreu
  Cc: David S. Miller, Giuseppe Cavallaro, Alexandre Torgue, netdev,
	linux-tegra, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] net: stmmac: Move debugfs init/exit to ->probe()/->remove()
  2018-11-26 15:34   ` Thierry Reding
@ 2018-11-27  9:02     ` Jose Abreu
  2018-11-27 14:35       ` Thierry Reding
  0 siblings, 1 reply; 10+ messages in thread
From: Jose Abreu @ 2018-11-27  9:02 UTC (permalink / raw)
  To: Thierry Reding, Jose Abreu
  Cc: David S. Miller, Giuseppe Cavallaro, Alexandre Torgue, netdev,
	linux-tegra, linux-kernel

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2] net: stmmac: Move debugfs init/exit to ->probe()/->remove()
  2018-11-23 12:21 [PATCH] net: stmmac: Move debugfs init/exit to ->probe()/->remove() Thierry Reding
  2018-11-23 12:44 ` Jose Abreu
@ 2018-11-27 13:21 ` Thierry Reding
  2018-11-28  9:38   ` Jose Abreu
  2018-11-30 21:16   ` David Miller
  1 sibling, 2 replies; 10+ messages in thread
From: Thierry Reding @ 2018-11-27 13:21 UTC (permalink / raw)
  To: David S. Miller
  Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, netdev,
	linux-tegra, linux-kernel

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.

Note that the ring descriptors cannot be read while the interface is
down, so make sure to return an empty file when the descriptors_status
debugfs file is read.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
This applies on top of net-next.

Changes in v2:
- avoid access to ring descriptors when interface is down

 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 23 +++++++++++--------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 076a8be18d67..5551fead8f66 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;
@@ -3899,6 +3889,9 @@ static int stmmac_sysfs_ring_read(struct seq_file *seq, void *v)
 	u32 tx_count = priv->plat->tx_queues_to_use;
 	u32 queue;
 
+	if ((dev->flags & IFF_UP) == 0)
+		return 0;
+
 	for (queue = 0; queue < rx_count; queue++) {
 		struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
 
@@ -4397,6 +4390,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 +4432,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);
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] net: stmmac: Move debugfs init/exit to ->probe()/->remove()
  2018-11-27  9:02     ` Jose Abreu
@ 2018-11-27 14:35       ` Thierry Reding
  0 siblings, 0 replies; 10+ messages in thread
From: Thierry Reding @ 2018-11-27 14:35 UTC (permalink / raw)
  To: Jose Abreu
  Cc: David S. Miller, Giuseppe Cavallaro, Alexandre Torgue, netdev,
	linux-tegra, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2561 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] net: stmmac: Move debugfs init/exit to ->probe()/->remove()
  2018-11-27 13:21 ` [PATCH v2] " Thierry Reding
@ 2018-11-28  9:38   ` Jose Abreu
  2018-11-28  9:41     ` Thierry Reding
  2018-11-30 21:16   ` David Miller
  1 sibling, 1 reply; 10+ messages in thread
From: Jose Abreu @ 2018-11-28  9:38 UTC (permalink / raw)
  To: Thierry Reding, David S. Miller
  Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, netdev,
	linux-tegra, linux-kernel

On 27-11-2018 13: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.
>
> Note that the ring descriptors cannot be read while the interface is
> down, so make sure to return an empty file when the descriptors_status
> debugfs file is read.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> This applies on top of net-next.
>
> Changes in v2:
> - avoid access to ring descriptors when interface is down
>
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 23 +++++++++++--------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 076a8be18d67..5551fead8f66 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;
> @@ -3899,6 +3889,9 @@ static int stmmac_sysfs_ring_read(struct seq_file *seq, void *v)
>  	u32 tx_count = priv->plat->tx_queues_to_use;
>  	u32 queue;
>  
> +	if ((dev->flags & IFF_UP) == 0)

I tried looking for an helper to check if interface is open but
couldn't find one so I guess this is the right thing to do.

Acked-by: Jose Abreu <joabreu@synopsys.com>

Thanks and Best Regards,
Jose Miguel Abreu

> +		return 0;
> +
>  	for (queue = 0; queue < rx_count; queue++) {
>  		struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
>  
> @@ -4397,6 +4390,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 +4432,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);


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] net: stmmac: Move debugfs init/exit to ->probe()/->remove()
  2018-11-28  9:38   ` Jose Abreu
@ 2018-11-28  9:41     ` Thierry Reding
  0 siblings, 0 replies; 10+ messages in thread
From: Thierry Reding @ 2018-11-28  9:41 UTC (permalink / raw)
  To: Jose Abreu
  Cc: David S. Miller, Giuseppe Cavallaro, Alexandre Torgue, netdev,
	linux-tegra, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2900 bytes --]

On Wed, Nov 28, 2018 at 09:38:32AM +0000, Jose Abreu wrote:
> On 27-11-2018 13: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.
> >
> > Note that the ring descriptors cannot be read while the interface is
> > down, so make sure to return an empty file when the descriptors_status
> > debugfs file is read.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > This applies on top of net-next.
> >
> > Changes in v2:
> > - avoid access to ring descriptors when interface is down
> >
> >  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 23 +++++++++++--------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 076a8be18d67..5551fead8f66 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;
> > @@ -3899,6 +3889,9 @@ static int stmmac_sysfs_ring_read(struct seq_file *seq, void *v)
> >  	u32 tx_count = priv->plat->tx_queues_to_use;
> >  	u32 queue;
> >  
> > +	if ((dev->flags & IFF_UP) == 0)
> 
> I tried looking for an helper to check if interface is open but
> couldn't find one so I guess this is the right thing to do.

Yeah, I had done the same thing and came up empty-handed.

> Acked-by: Jose Abreu <joabreu@synopsys.com>

Thanks!
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] net: stmmac: Move debugfs init/exit to ->probe()/->remove()
  2018-11-27 13:21 ` [PATCH v2] " Thierry Reding
  2018-11-28  9:38   ` Jose Abreu
@ 2018-11-30 21:16   ` David Miller
  1 sibling, 0 replies; 10+ messages in thread
From: David Miller @ 2018-11-30 21:16 UTC (permalink / raw)
  To: thierry.reding
  Cc: peppe.cavallaro, alexandre.torgue, joabreu, netdev, linux-tegra,
	linux-kernel

From: Thierry Reding <thierry.reding@gmail.com>
Date: Tue, 27 Nov 2018 14:21:43 +0100

> 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.
> 
> Note that the ring descriptors cannot be read while the interface is
> down, so make sure to return an empty file when the descriptors_status
> debugfs file is read.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> This applies on top of net-next.
> 
> Changes in v2:
> - avoid access to ring descriptors when interface is down

Applied, thanks.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-11-30 21:16 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-23 12:21 [PATCH] net: stmmac: Move debugfs init/exit to ->probe()/->remove() Thierry Reding
2018-11-23 12:44 ` Jose Abreu
2018-11-23 13:12   ` Thierry Reding
2018-11-26 15:34   ` Thierry Reding
2018-11-27  9:02     ` Jose Abreu
2018-11-27 14:35       ` Thierry Reding
2018-11-27 13:21 ` [PATCH v2] " Thierry Reding
2018-11-28  9:38   ` Jose Abreu
2018-11-28  9:41     ` Thierry Reding
2018-11-30 21:16   ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).