unregister_netdev() calls the struct net_device_ops::ndo_stop function,
which in the case of the macb driver is macb_close(). macb_close() calls,
in turn, PHY-specific APIs (e.g., phy_detach()). The call trace is as
follows:

macb_close() ->
  phylink_disconnect_phy() ->
    phy_disconnect() ->
      phy_detach()

phy_detach() will remove associated sysfs files by calling
kernfs_remove_by_name_ns(), which will hit
"kernfs: cannot remove 'attached_dev', no directory" WARN(), which will
throw a stack trace too.

To avoid this, call unregiser_netdev() before mdiobus_unregister() and
mdiobus_free().

Fixes: 7897b071ac3b ("net: macb: convert to phylink")
Signed-off-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>
---
 drivers/net/ethernet/cadence/macb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index cebae0f418f2..73d041af3de1 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -5165,11 +5165,11 @@ static void macb_remove(struct platform_device *pdev)
 
 	if (dev) {
 		bp = netdev_priv(dev);
+		unregister_netdev(dev);
 		phy_exit(bp->sgmii_phy);
 		mdiobus_unregister(bp->mii_bus);
 		mdiobus_free(bp->mii_bus);
 
-		unregister_netdev(dev);
 		tasklet_kill(&bp->hresp_err_tasklet);
 		pm_runtime_disable(&pdev->dev);
 		pm_runtime_dont_use_autosuspend(&pdev->dev);
-- 
2.39.2

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index cebae0f418f2..73d041af3de1 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -5165,11 +5165,11 @@ static void macb_remove(struct platform_device *pdev)
>  
>  	if (dev) {
>  		bp = netdev_priv(dev);
> +		unregister_netdev(dev);
>  		phy_exit(bp->sgmii_phy);
>  		mdiobus_unregister(bp->mii_bus);
>  		mdiobus_free(bp->mii_bus);
>  
> -		unregister_netdev(dev);
>  		tasklet_kill(&bp->hresp_err_tasklet);


I don't know this driver...

What does this tasklet do? Is it safe for it to run after the netdev
is unregistered, and the PHY and the mdio bus is gone? Maybe this
tasklet_kill should be after the interrupt is disabled, but before the
netdev is unregistered?

If you have one bug here, there might be others.

	Andrew

On 26.11.2023 19:13, Andrew Lunn wrote:
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index cebae0f418f2..73d041af3de1 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -5165,11 +5165,11 @@ static void macb_remove(struct platform_device *pdev)
>>  
>>  	if (dev) {
>>  		bp = netdev_priv(dev);
>> +		unregister_netdev(dev);
>>  		phy_exit(bp->sgmii_phy);
>>  		mdiobus_unregister(bp->mii_bus);
>>  		mdiobus_free(bp->mii_bus);
>>  
>> -		unregister_netdev(dev);
>>  		tasklet_kill(&bp->hresp_err_tasklet);
> 
> 
> I don't know this driver...
> 
> What does this tasklet do? 

It handles bus errors that my happens while DMA fetches data from system
memory. It re-initializes the TX/RX, DMA buffers, clear interrupts,
stop/start all tx queues.

> Is it safe for it to run after the netdev
> is unregistered, and the PHY and the mdio bus is gone? 

Not really, as it accesses netdev specific data.

> Maybe this
> tasklet_kill should be after the interrupt is disabled, but before the
> netdev is unregistered?

That would be a better place, indeed.

Thank you,
Claudiu Beznea

> 
> If you have one bug here, there might be others.
> 
> 	Andrew

On Sun, Nov 26, 2023 at 06:13:55PM +0100, Andrew Lunn wrote:
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index cebae0f418f2..73d041af3de1 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -5165,11 +5165,11 @@ static void macb_remove(struct platform_device *pdev)
> >  
> >  	if (dev) {
> >  		bp = netdev_priv(dev);
> > +		unregister_netdev(dev);
> >  		phy_exit(bp->sgmii_phy);
> >  		mdiobus_unregister(bp->mii_bus);
> >  		mdiobus_free(bp->mii_bus);
> >  
> > -		unregister_netdev(dev);
> >  		tasklet_kill(&bp->hresp_err_tasklet);
> 
> 
> I don't know this driver...
> 
> What does this tasklet do? Is it safe for it to run after the netdev
> is unregistered, and the PHY and the mdio bus is gone? Maybe this
> tasklet_kill should be after the interrupt is disabled, but before the
> netdev is unregistered?

.. and while evaluating Andrew's comment, check whether the tasklet
is necessary for the device to successfully close or not - remembering
that unregister_netdev() will close down an open netdev.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

On Sun, Nov 26, 2023 at 04:10:46PM +0200, Claudiu Beznea wrote:
> unregister_netdev() calls the struct net_device_ops::ndo_stop function,
> which in the case of the macb driver is macb_close(). macb_close() calls,
> in turn, PHY-specific APIs (e.g., phy_detach()). The call trace is as
> follows:
> 
> macb_close() ->
>   phylink_disconnect_phy() ->
>     phy_disconnect() ->
>       phy_detach()
> 
> phy_detach() will remove associated sysfs files by calling
> kernfs_remove_by_name_ns(), which will hit
> "kernfs: cannot remove 'attached_dev', no directory" WARN(), which will
> throw a stack trace too.
> 
> To avoid this, call unregiser_netdev() before mdiobus_unregister() and
> mdiobus_free().

Definitely the correct fix, also to fix the issue in patch 1.
> 
> Fixes: 7897b071ac3b ("net: macb: convert to phylink")
> Signed-off-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!