2023-11-26 17:14:21

by Andrew Lunn

Subject: Re: [PATCH 2/2] net: macb: Unregister nedev before MDIO bus in remove

> 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

2023-11-26 17:50:09

by Russell King (Oracle)

Subject: Re: [PATCH 2/2] net: macb: Unregister nedev before MDIO bus in remove

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!

2023-11-27 06:21:22

by claudiu beznea

Subject: Re: [PATCH 2/2] net: macb: Unregister nedev before MDIO bus in remove

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