From: "Kweh, Hock Leong" <[email protected]>
If kernel module stmmac driver being loaded after OS booted, there is a
race condition between stmmac_open() and stmmac_mdio_register(), which is
invoked inside stmmac_dvr_probe(), and the error is showed in dmesg log as
PHY not found and stmmac_open() failed:
[ 473.919358] stmmaceth 0000:01:00.0 (unnamed net_device) (uninitialized):
stmmac_dvr_probe: warning: cannot get CSR clock
[ 473.919382] stmmaceth 0000:01:00.0: no reset control found
[ 473.919412] stmmac - user ID: 0x10, Synopsys ID: 0x42
[ 473.919429] stmmaceth 0000:01:00.0: DMA HW capability register supported
[ 473.919436] stmmaceth 0000:01:00.0: RX Checksum Offload Engine supported
[ 473.919443] stmmaceth 0000:01:00.0: TX Checksum insertion supported
[ 473.919451] stmmaceth 0000:01:00.0 (unnamed net_device) (uninitialized):
Enable RX Mitigation via HW Watchdog Timer
[ 473.921395] libphy: PHY stmmac-1:00 not found
[ 473.921417] stmmaceth 0000:01:00.0 eth0: Could not attach to PHY
[ 473.921427] stmmaceth 0000:01:00.0 eth0: stmmac_open: Cannot attach to
PHY (error: -19)
[ 473.959710] libphy: stmmac: probed
[ 473.959724] stmmaceth 0000:01:00.0 eth0: PHY ID 01410cc2 at 0 IRQ POLL
(stmmac-1:00) active
[ 473.959728] stmmaceth 0000:01:00.0 eth0: PHY ID 01410cc2 at 1 IRQ POLL
(stmmac-1:01)
[ 473.959731] stmmaceth 0000:01:00.0 eth0: PHY ID 01410cc2 at 2 IRQ POLL
(stmmac-1:02)
[ 473.959734] stmmaceth 0000:01:00.0 eth0: PHY ID 01410cc2 at 3 IRQ POLL
(stmmac-1:03)
The resolution moved the register_netdev() function call to the end of
stmmac_dvr_probe() after stmmac_mdio_register().
Suggested-by: Florian Fainelli <[email protected]>
Signed-off-by: Kweh, Hock Leong <[email protected]>
---
changelog v2:
* Re-implemented the fix base on David & Florian suggestion and tested.
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index bb40382..263ac53 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3339,13 +3339,6 @@ int stmmac_dvr_probe(struct device *device,
spin_lock_init(&priv->lock);
- ret = register_netdev(ndev);
- if (ret) {
- netdev_err(priv->dev, "%s: ERROR %i registering the device\n",
- __func__, ret);
- goto error_netdev_register;
- }
-
/* If a specific clk_csr value is passed from the platform
* this means that the CSR Clock Range selection cannot be
* changed at run-time and it is fixed. Viceversa the driver'll try to
@@ -3372,11 +3365,18 @@ int stmmac_dvr_probe(struct device *device,
}
}
+ ret = register_netdev(ndev);
+ if (ret) {
+ netdev_err(priv->dev, "%s: ERROR %i registering the device\n",
+ __func__, ret);
+ goto error_netdev_register;
+ }
+
return 0;
-error_mdio_register:
- unregister_netdev(ndev);
error_netdev_register:
+ stmmac_mdio_unregister(ndev);
+error_mdio_register:
netif_napi_del(&priv->napi);
error_hw_init:
clk_disable_unprepare(priv->pclk);
--
1.7.9.5
On Tue 2016-12-27 22:42:36, Kweh, Hock Leong wrote:
> From: "Kweh, Hock Leong" <[email protected]>
>
> If kernel module stmmac driver being loaded after OS booted, there is a
> race condition between stmmac_open() and stmmac_mdio_register(), which is
> invoked inside stmmac_dvr_probe(), and the error is showed in dmesg log as
> PHY not found and stmmac_open() failed:
> [ 473.919358] stmmaceth 0000:01:00.0 (unnamed net_device) (uninitialized):
> stmmac_dvr_probe: warning: cannot get CSR clock
> [ 473.919382] stmmaceth 0000:01:00.0: no reset control found
> [ 473.919412] stmmac - user ID: 0x10, Synopsys ID: 0x42
> [ 473.919429] stmmaceth 0000:01:00.0: DMA HW capability register supported
> [ 473.919436] stmmaceth 0000:01:00.0: RX Checksum Offload Engine supported
> [ 473.919443] stmmaceth 0000:01:00.0: TX Checksum insertion supported
> [ 473.919451] stmmaceth 0000:01:00.0 (unnamed net_device) (uninitialized):
> Enable RX Mitigation via HW Watchdog Timer
> [ 473.921395] libphy: PHY stmmac-1:00 not found
> [ 473.921417] stmmaceth 0000:01:00.0 eth0: Could not attach to PHY
> [ 473.921427] stmmaceth 0000:01:00.0 eth0: stmmac_open: Cannot attach to
> PHY (error: -19)
> [ 473.959710] libphy: stmmac: probed
> [ 473.959724] stmmaceth 0000:01:00.0 eth0: PHY ID 01410cc2 at 0 IRQ POLL
> (stmmac-1:00) active
> [ 473.959728] stmmaceth 0000:01:00.0 eth0: PHY ID 01410cc2 at 1 IRQ POLL
> (stmmac-1:01)
> [ 473.959731] stmmaceth 0000:01:00.0 eth0: PHY ID 01410cc2 at 2 IRQ POLL
> (stmmac-1:02)
> [ 473.959734] stmmaceth 0000:01:00.0 eth0: PHY ID 01410cc2 at 3 IRQ POLL
> (stmmac-1:03)
>
> The resolution moved the register_netdev() function call to the end of
> stmmac_dvr_probe() after stmmac_mdio_register().
>
> Suggested-by: Florian Fainelli <[email protected]>
> Signed-off-by: Kweh, Hock Leong <[email protected]>
Acked-by: Pavel Machek <[email protected]>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
From: "Kweh, Hock Leong" <[email protected]>
Date: Tue, 27 Dec 2016 22:42:36 +0800
> From: "Kweh, Hock Leong" <[email protected]>
You are not the author of this change, do not take credit for it.
You have copied Florian's patch character by character, therefore
he is the author.
You also didn't CC: the netdev mailing list properly.
> -----Original Message-----
> From: David Miller [mailto:[email protected]]
> Sent: Wednesday, December 28, 2016 12:34 AM
> To: Kweh, Hock Leong <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Ong, Boon Leong
> <[email protected]>; Voon, Weifeng <[email protected]>;
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH v2] net: stmmac: bug fix to synchronize stmmac_open and
> stmmac_dvr_probe
>
> From: "Kweh, Hock Leong" <[email protected]>
> Date: Tue, 27 Dec 2016 22:42:36 +0800
>
> > From: "Kweh, Hock Leong" <[email protected]>
>
> You are not the author of this change, do not take credit for it.
>
> You have copied Florian's patch character by character, therefore
> he is the author.
>
> You also didn't CC: the netdev mailing list properly.
Noted & Thanks.
Hi Florian, could you submit this fix from your side so that you are the author.
I will help to test out.
Thanks & Regards,
Wilson
There is currently a small window during which the network device registered by
stmmac can be made visible, yet all resources, including and clock and MDIO bus
have not had a chance to be set up, this can lead to the following error to
occur:
[ 473.919358] stmmaceth 0000:01:00.0 (unnamed net_device) (uninitialized):
stmmac_dvr_probe: warning: cannot get CSR clock
[ 473.919382] stmmaceth 0000:01:00.0: no reset control found
[ 473.919412] stmmac - user ID: 0x10, Synopsys ID: 0x42
[ 473.919429] stmmaceth 0000:01:00.0: DMA HW capability register supported
[ 473.919436] stmmaceth 0000:01:00.0: RX Checksum Offload Engine supported
[ 473.919443] stmmaceth 0000:01:00.0: TX Checksum insertion supported
[ 473.919451] stmmaceth 0000:01:00.0 (unnamed net_device) (uninitialized):
Enable RX Mitigation via HW Watchdog Timer
[ 473.921395] libphy: PHY stmmac-1:00 not found
[ 473.921417] stmmaceth 0000:01:00.0 eth0: Could not attach to PHY
[ 473.921427] stmmaceth 0000:01:00.0 eth0: stmmac_open: Cannot attach to
PHY (error: -19)
[ 473.959710] libphy: stmmac: probed
[ 473.959724] stmmaceth 0000:01:00.0 eth0: PHY ID 01410cc2 at 0 IRQ POLL
(stmmac-1:00) active
[ 473.959728] stmmaceth 0000:01:00.0 eth0: PHY ID 01410cc2 at 1 IRQ POLL
(stmmac-1:01)
[ 473.959731] stmmaceth 0000:01:00.0 eth0: PHY ID 01410cc2 at 2 IRQ POLL
(stmmac-1:02)
[ 473.959734] stmmaceth 0000:01:00.0 eth0: PHY ID 01410cc2 at 3 IRQ POLL
(stmmac-1:03)
Fix this by making sure that register_netdev() is the last thing being done,
which guarantees that the clock and the MDIO bus are available.
Fixes: 4bfcbd7abce2 ("stmmac: Move the mdio_register/_unregister in probe/remove")
Reported-by: Kweh, Hock Leong <[email protected]>
Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index bb40382e205d..5910ea51f8f6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3339,13 +3339,6 @@ int stmmac_dvr_probe(struct device *device,
spin_lock_init(&priv->lock);
- ret = register_netdev(ndev);
- if (ret) {
- netdev_err(priv->dev, "%s: ERROR %i registering the device\n",
- __func__, ret);
- goto error_netdev_register;
- }
-
/* If a specific clk_csr value is passed from the platform
* this means that the CSR Clock Range selection cannot be
* changed at run-time and it is fixed. Viceversa the driver'll try to
@@ -3372,11 +3365,14 @@ int stmmac_dvr_probe(struct device *device,
}
}
- return 0;
+ ret = register_netdev(ndev);
+ if (ret)
+ netdev_err(priv->dev, "%s: ERROR %i registering the device\n",
+ __func__, ret);
+
+ return ret;
error_mdio_register:
- unregister_netdev(ndev);
-error_netdev_register:
netif_napi_del(&priv->napi);
error_hw_init:
clk_disable_unprepare(priv->pclk);
--
2.9.3
From: Florian Fainelli <[email protected]>
Date: Tue, 27 Dec 2016 18:23:06 -0800
> There is currently a small window during which the network device registered by
> stmmac can be made visible, yet all resources, including and clock and MDIO bus
> have not had a chance to be set up, this can lead to the following error to
> occur:
...
> Fix this by making sure that register_netdev() is the last thing being done,
> which guarantees that the clock and the MDIO bus are available.
>
> Fixes: 4bfcbd7abce2 ("stmmac: Move the mdio_register/_unregister in probe/remove")
> Reported-by: Kweh, Hock Leong <[email protected]>
> Signed-off-by: Florian Fainelli <[email protected]>
Applied and queued up for -stable, thanks Florian.
> -----Original Message-----
> From: David Miller [mailto:[email protected]]
> Sent: Wednesday, December 28, 2016 12:34 AM
> To: Kweh, Hock Leong <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Ong, Boon Leong
> <[email protected]>; Voon, Weifeng <[email protected]>;
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH v2] net: stmmac: bug fix to synchronize stmmac_open and
> stmmac_dvr_probe
>
> From: "Kweh, Hock Leong" <[email protected]>
> Date: Tue, 27 Dec 2016 22:42:36 +0800
>
> > From: "Kweh, Hock Leong" <[email protected]>
>
> You are not the author of this change, do not take credit for it.
>
> You have copied Florian's patch character by character, therefore
> he is the author.
>
> You also didn't CC: the netdev mailing list properly.
Hi David & Florian,
Just to clarify that I do not copy exactly from Florian.
I have changed it to have proper handling on mdio unregister
while netdev_register() failed as showed below:
return 0;
-error_mdio_register:
- unregister_netdev(ndev);
error_netdev_register:
+ stmmac_mdio_unregister(ndev);
+error_mdio_register:
netif_napi_del(&priv->napi);
Vs
+
+ return ret;
error_mdio_register:
- unregister_netdev(ndev);
-error_netdev_register:
netif_napi_del(&priv->napi);
Just to point it out here, so that Florian could aware if he/she submit
this fix to the mailing list.
Thanks & Regards,
Wilson
On Wed, Dec 28, 2016 at 7:10 AM, Kweh, Hock Leong
<[email protected]> wrote:
>> -----Original Message-----
>> From: David Miller [mailto:[email protected]]
>> Sent: Wednesday, December 28, 2016 12:34 AM
>> To: Kweh, Hock Leong <[email protected]>
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; Ong, Boon Leong
>> <[email protected]>; Voon, Weifeng <[email protected]>;
>> [email protected]; [email protected]; [email protected]
>> Subject: Re: [PATCH v2] net: stmmac: bug fix to synchronize stmmac_open and
>> stmmac_dvr_probe
>>
>> From: "Kweh, Hock Leong" <[email protected]>
>> Date: Tue, 27 Dec 2016 22:42:36 +0800
>>
>> > From: "Kweh, Hock Leong" <[email protected]>
>>
>> You are not the author of this change, do not take credit for it.
>>
>> You have copied Florian's patch character by character, therefore
>> he is the author.
>>
>> You also didn't CC: the netdev mailing list properly.
>
> Noted & Thanks.
>
> Hi Florian, could you submit this fix from your side so that you are the author.
> I will help to test out.
>
> Thanks & Regards,
> Wilson
>
I think you can give *--author* for giving author name. Try git commit
-am "commit message" --author="Author_name <author_email>"
On 12/27/2016 09:49 PM, Kweh, Hock Leong wrote:
>> -----Original Message-----
>> From: David Miller [mailto:[email protected]]
>> Sent: Wednesday, December 28, 2016 12:34 AM
>> To: Kweh, Hock Leong <[email protected]>
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; Ong, Boon Leong
>> <[email protected]>; Voon, Weifeng <[email protected]>;
>> [email protected]; [email protected]; [email protected]
>> Subject: Re: [PATCH v2] net: stmmac: bug fix to synchronize stmmac_open and
>> stmmac_dvr_probe
>>
>> From: "Kweh, Hock Leong" <[email protected]>
>> Date: Tue, 27 Dec 2016 22:42:36 +0800
>>
>>> From: "Kweh, Hock Leong" <[email protected]>
>>
>> You are not the author of this change, do not take credit for it.
>>
>> You have copied Florian's patch character by character, therefore
>> he is the author.
>>
>> You also didn't CC: the netdev mailing list properly.
>
> Hi David & Florian,
>
> Just to clarify that I do not copy exactly from Florian.
> I have changed it to have proper handling on mdio unregister
> while netdev_register() failed as showed below:
>
> return 0;
>
> -error_mdio_register:
> - unregister_netdev(ndev);
> error_netdev_register:
> + stmmac_mdio_unregister(ndev);
Although this is required, we can't be doing it in all circumstances, we
need to mimic what stmmac_drv_remove() does.
Let me submit an incremental fix which takes care of mdio bus
unregistration.
--
Florian
> -----Original Message-----
> From: Kishan Sandeep [mailto:[email protected]]
> Sent: Wednesday, December 28, 2016 7:56 PM
> To: Kweh, Hock Leong <[email protected]>
> Cc: David Miller <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH v2] net: stmmac: bug fix to synchronize stmmac_open and
> stmmac_dvr_probe
>
> On Wed, Dec 28, 2016 at 7:10 AM, Kweh, Hock Leong
> <[email protected]> wrote:
> >> -----Original Message-----
> >> From: David Miller [mailto:[email protected]]
> >> Sent: Wednesday, December 28, 2016 12:34 AM
> >> To: Kweh, Hock Leong <[email protected]>
> >> Cc: [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected]; Ong, Boon
> >> Leong <[email protected]>; Voon, Weifeng
> >> <[email protected]>; [email protected];
> >> [email protected]; [email protected]
> >> Subject: Re: [PATCH v2] net: stmmac: bug fix to synchronize
> >> stmmac_open and stmmac_dvr_probe
> >>
> >> From: "Kweh, Hock Leong" <[email protected]>
> >> Date: Tue, 27 Dec 2016 22:42:36 +0800
> >>
> >> > From: "Kweh, Hock Leong" <[email protected]>
> >>
> >> You are not the author of this change, do not take credit for it.
> >>
> >> You have copied Florian's patch character by character, therefore he
> >> is the author.
> >>
> >> You also didn't CC: the netdev mailing list properly.
> >
> > Noted & Thanks.
> >
> > Hi Florian, could you submit this fix from your side so that you are the author.
> > I will help to test out.
> >
> > Thanks & Regards,
> > Wilson
> >
> I think you can give *--author* for giving author name. Try git commit -am
> "commit message" --author="Author_name <author_email>"
Oh ... I am not aware of that. Thanks for informing.
:-)
Regards,
Wilson
> -----Original Message-----
> From: Florian Fainelli [mailto:[email protected]]
> Sent: Thursday, December 29, 2016 2:43 AM
> To: Kweh, Hock Leong <[email protected]>; David Miller
> <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Ong, Boon Leong <[email protected]>; Voon, Weifeng
> <[email protected]>; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v2] net: stmmac: bug fix to synchronize stmmac_open and
> stmmac_dvr_probe
>
> On 12/27/2016 09:49 PM, Kweh, Hock Leong wrote:
> >> -----Original Message-----
> >> From: David Miller [mailto:[email protected]]
> >> Sent: Wednesday, December 28, 2016 12:34 AM
> >> To: Kweh, Hock Leong <[email protected]>
> >> Cc: [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected]; Ong, Boon
> >> Leong <[email protected]>; Voon, Weifeng
> >> <[email protected]>; [email protected];
> >> [email protected]; [email protected]
> >> Subject: Re: [PATCH v2] net: stmmac: bug fix to synchronize
> >> stmmac_open and stmmac_dvr_probe
> >>
> >> From: "Kweh, Hock Leong" <[email protected]>
> >> Date: Tue, 27 Dec 2016 22:42:36 +0800
> >>
> >>> From: "Kweh, Hock Leong" <[email protected]>
> >>
> >> You are not the author of this change, do not take credit for it.
> >>
> >> You have copied Florian's patch character by character, therefore he
> >> is the author.
> >>
> >> You also didn't CC: the netdev mailing list properly.
> >
> > Hi David & Florian,
> >
> > Just to clarify that I do not copy exactly from Florian.
> > I have changed it to have proper handling on mdio unregister while
> > netdev_register() failed as showed below:
> >
> > return 0;
> >
> > -error_mdio_register:
> > - unregister_netdev(ndev);
> > error_netdev_register:
> > + stmmac_mdio_unregister(ndev);
>
> Although this is required, we can't be doing it in all circumstances, we need to
> mimic what stmmac_drv_remove() does.
>
> Let me submit an incremental fix which takes care of mdio bus unregistration.
> --
> Florian
Noted & Thanks. Will test it out once you submitted.
Thanks & Regards,
Wilson
On 12/28/2016 04:28 PM, Kweh, Hock Leong wrote:
>> Although this is required, we can't be doing it in all circumstances, we need to
>> mimic what stmmac_drv_remove() does.
>>
>> Let me submit an incremental fix which takes care of mdio bus unregistration.
>> --
>> Florian
>
> Noted & Thanks. Will test it out once you submitted.
It's done:
https://www.spinics.net/lists/netdev/msg411934.html
--
Florian