[PATCH/RFC 0/5] Fix the sh_eth race between open and MDIO bus registration

[PATCH/RFC 0/5] Fix the sh_eth race between open and MDIO bus registration

[Laurent Pinchart]

Hello,

This patch series attempts to fix the race condition that exists in the sh_eth
driver between network device open and MDIO bus registration. The actual fix
is in patch 4/5, with previous patches preparing the driver and patch 5/5
cleaning up an unrelated issue.

I've based the idea on Sergei's attempt to fix the problem and can successfully
boot the Koelsch board over NFS with this series. I might have missed other
issues though, hence the RFC status.

The patches are based on top of Simon's latest devel branch plus the following
two patches

	sh_eth: add device tree support
	sh_eth: update OF PHY registeration

Sergei, assuming this series gets accepted, should I send it to the netdev
mailing list or would you like to take it in your tree to avoid conflicts ?

Laurent Pinchart (5):
  sh_eth: Use the platform device for memory allocation
  sh_eth: Use the platform device as the MDIO bus parent
  sh_eth: Simplify MDIO bus initialization and release
  sh_eth: Register MDIO bus before registering the network device
  sh_eth: Remove goto statements that jump straight to a return

 drivers/net/ethernet/renesas/sh_eth.c | 82 +++++++++++++----------------------
 1 file changed, 29 insertions(+), 53 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 0/5] Fix the sh_eth race between open and MDIO bus registration

[Geert Uytterhoeven]

Hi Laurent,

On Wed, Mar 19, 2014 at 12:25 AM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> Laurent Pinchart (5):
>   sh_eth: Use the platform device for memory allocation
>   sh_eth: Use the platform device as the MDIO bus parent
>   sh_eth: Simplify MDIO bus initialization and release
>   sh_eth: Register MDIO bus before registering the network device
>   sh_eth: Remove goto statements that jump straight to a return

Thanks, the changes look fine to me, so

Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Still have to do a test run on Koelsch, though.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC 0/5] Fix the sh_eth race between open and MDIO bus registration

[Ben Dooks]

On 19/03/14 08:41, Geert Uytterhoeven wrote:
> Hi Laurent,
>
> On Wed, Mar 19, 2014 at 12:25 AM, Laurent Pinchart
> <laurent.pinchart+renesas@ideasonboard.com> wrote:
>> Laurent Pinchart (5):
>>    sh_eth: Use the platform device for memory allocation
>>    sh_eth: Use the platform device as the MDIO bus parent
>>    sh_eth: Simplify MDIO bus initialization and release
>>    sh_eth: Register MDIO bus before registering the network device
>>    sh_eth: Remove goto statements that jump straight to a return
>
> Thanks, the changes look fine to me, so

I think the only issue I have is the re-parent of the MDIO device.

My view also is that the probe should explicitly get a reference
if it is going to be created sub-devices.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH/RFC 0/5] Fix the sh_eth race between open and MDIO bus registration

[Laurent Pinchart]

Hi Ben,

On Wednesday 19 March 2014 10:14:53 Ben Dooks wrote:
> On 19/03/14 08:41, Geert Uytterhoeven wrote:
> > On Wed, Mar 19, 2014 at 12:25 AM, Laurent Pinchart wrote:
> >>
> >> Laurent Pinchart (5):
> >>    sh_eth: Use the platform device for memory allocation
> >>    sh_eth: Use the platform device as the MDIO bus parent
> >>    sh_eth: Simplify MDIO bus initialization and release
> >>    sh_eth: Register MDIO bus before registering the network device
> >>    sh_eth: Remove goto statements that jump straight to a return
> > 
> > Thanks, the changes look fine to me, so
> 
> I think the only issue I have is the re-parent of the MDIO device. 
> 
> My view also is that the probe should explicitly get a reference if it is
> going to be created sub-devices.

I'm not sure to follow you, could you please elaborate on that ? What should 
take a reference on what ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 0/5] Fix the sh_eth race between open and MDIO bus registration

[Geert Uytterhoeven]

Hi Laurent,

On Wed, Mar 19, 2014 at 11:07 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wednesday 19 March 2014 10:14:53 Ben Dooks wrote:
>> On 19/03/14 08:41, Geert Uytterhoeven wrote:
>> > On Wed, Mar 19, 2014 at 12:25 AM, Laurent Pinchart wrote:
>> >>
>> >> Laurent Pinchart (5):
>> >>    sh_eth: Use the platform device for memory allocation
>> >>    sh_eth: Use the platform device as the MDIO bus parent
>> >>    sh_eth: Simplify MDIO bus initialization and release
>> >>    sh_eth: Register MDIO bus before registering the network device
>> >>    sh_eth: Remove goto statements that jump straight to a return
>> >
>> > Thanks, the changes look fine to me, so
>>
>> I think the only issue I have is the re-parent of the MDIO device.
>>
>> My view also is that the probe should explicitly get a reference if it is
>> going to be created sub-devices.
>
> I'm not sure to follow you, could you please elaborate on that ? What should
> take a reference on what ?

I think he means a runtime pm reference, and he is right.

I gave your series a try on Koelsch. Now the clock is:
  1. enabled in sh_eth_drv_probe(),
  2. disabled from the worker thread,
  3. enabled and disabled in sh_eth_get_stats(),
  4. enabled in sh_eth_open() for nfsroot,
  5. disabled in sh_eth_close() on shutdown,
  6. enabled and disabled in sh_eth_get_stats().

I wondered whether 2 could happen too soon, so I added msleep(2000)
to sh_eth_drv_probe(), just after the call to pm_runtime_resume().
Then it fails to obtain the MAC address:

    sh-eth ee700000.ethernet: no valid MAC address supplied, using a random one.

Due to 4, the network hardware works, and it manages to receive an IP address
from my DHCP server. But as the MAC address is wrong, the IP address is also
wrong, and it hangs when trying to mount NFS.

Applying Ben's "PATCH] sh_eth: ensure pm_runtime cannot suspend the
device during init" fixes this.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH/RFC 0/5] Fix the sh_eth race between open and MDIO bus registration

[Laurent Pinchart]

Hi Geert,

On Wednesday 19 March 2014 14:35:59 Geert Uytterhoeven wrote:
> On Wed, Mar 19, 2014 at 11:07 AM, Laurent Pinchart wrote:
> > On Wednesday 19 March 2014 10:14:53 Ben Dooks wrote:
> >> On 19/03/14 08:41, Geert Uytterhoeven wrote:
> >> > On Wed, Mar 19, 2014 at 12:25 AM, Laurent Pinchart wrote:
> >> >> Laurent Pinchart (5):
> >> >>    sh_eth: Use the platform device for memory allocation
> >> >>    sh_eth: Use the platform device as the MDIO bus parent
> >> >>    sh_eth: Simplify MDIO bus initialization and release
> >> >>    sh_eth: Register MDIO bus before registering the network device
> >> >>    sh_eth: Remove goto statements that jump straight to a return
> >> > 
> >> > Thanks, the changes look fine to me, so
> >> 
> >> I think the only issue I have is the re-parent of the MDIO device.
> >> 
> >> My view also is that the probe should explicitly get a reference if it is
> >> going to be created sub-devices.
> > 
> > I'm not sure to follow you, could you please elaborate on that ? What
> > should take a reference on what ?
> 
> I think he means a runtime pm reference, and he is right.
> 
> I gave your series a try on Koelsch. Now the clock is:
>   1. enabled in sh_eth_drv_probe(),
>   2. disabled from the worker thread,
>   3. enabled and disabled in sh_eth_get_stats(),
>   4. enabled in sh_eth_open() for nfsroot,
>   5. disabled in sh_eth_close() on shutdown,
>   6. enabled and disabled in sh_eth_get_stats().
> 
> I wondered whether 2 could happen too soon, so I added msleep(2000) to
> sh_eth_drv_probe(), just after the call to pm_runtime_resume(). Then it
> fails to obtain the MAC address:
> 
> sh-eth ee700000.ethernet: no valid MAC address supplied, using a random one.
> 
> Due to 4, the network hardware works, and it manages to receive an IP
> address from my DHCP server. But as the MAC address is wrong, the IP address
> is also wrong, and it hangs when trying to mount NFS.
> 
> Applying Ben's "PATCH] sh_eth: ensure pm_runtime cannot suspend the device
> during init" fixes this.

I've investigated the issue. The pm_runtime_resume() call from the sh_eth 
probe function ends up calling rpm_resume() synchronously. The function 
resumes the device, and right before returning calls rpm_idle(dev, RPM_ASYNC). 
This queues a RPM_REQ_IDLE request, resulting in the device being suspended 
the next time the work queue is run.

pm_runtime_resume() seem to be unsafe at probe time if the PM workqueue can 
run before the probe function is done with the device, which means pretty much 
everywhere as probe() usually calls functions that can sleep.

I thus agree that a pm_runtime_get_sync() call is needed. The 
pm_runtime_put_sync() call at the end of the probe function could be replaced 
by a pm_runtime_put() call though. The PM runtime documentation should also be 
updated.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 0/5] Fix the sh_eth race between open and MDIO bus registration

[Ben Dooks]

On 19/03/14 16:06, Laurent Pinchart wrote:
> Hi Geert,
>
> On Wednesday 19 March 2014 14:35:59 Geert Uytterhoeven wrote:
>> On Wed, Mar 19, 2014 at 11:07 AM, Laurent Pinchart wrote:
>>> On Wednesday 19 March 2014 10:14:53 Ben Dooks wrote:
>>>> On 19/03/14 08:41, Geert Uytterhoeven wrote:
>>>>> On Wed, Mar 19, 2014 at 12:25 AM, Laurent Pinchart wrote:
>>>>>> Laurent Pinchart (5):
>>>>>>     sh_eth: Use the platform device for memory allocation
>>>>>>     sh_eth: Use the platform device as the MDIO bus parent
>>>>>>     sh_eth: Simplify MDIO bus initialization and release
>>>>>>     sh_eth: Register MDIO bus before registering the network device
>>>>>>     sh_eth: Remove goto statements that jump straight to a return
>>>>>
>>>>> Thanks, the changes look fine to me, so
>>>>
>>>> I think the only issue I have is the re-parent of the MDIO device.
>>>>
>>>> My view also is that the probe should explicitly get a reference if it is
>>>> going to be created sub-devices.
>>>
>>> I'm not sure to follow you, could you please elaborate on that ? What
>>> should take a reference on what ?
>>
>> I think he means a runtime pm reference, and he is right.
>>
>> I gave your series a try on Koelsch. Now the clock is:
>>    1. enabled in sh_eth_drv_probe(),
>>    2. disabled from the worker thread,
>>    3. enabled and disabled in sh_eth_get_stats(),
>>    4. enabled in sh_eth_open() for nfsroot,
>>    5. disabled in sh_eth_close() on shutdown,
>>    6. enabled and disabled in sh_eth_get_stats().
>>
>> I wondered whether 2 could happen too soon, so I added msleep(2000) to
>> sh_eth_drv_probe(), just after the call to pm_runtime_resume(). Then it
>> fails to obtain the MAC address:
>>
>> sh-eth ee700000.ethernet: no valid MAC address supplied, using a random one.
>>
>> Due to 4, the network hardware works, and it manages to receive an IP
>> address from my DHCP server. But as the MAC address is wrong, the IP address
>> is also wrong, and it hangs when trying to mount NFS.
>>
>> Applying Ben's "PATCH] sh_eth: ensure pm_runtime cannot suspend the device
>> during init" fixes this.
>
> I've investigated the issue. The pm_runtime_resume() call from the sh_eth
> probe function ends up calling rpm_resume() synchronously. The function
> resumes the device, and right before returning calls rpm_idle(dev, RPM_ASYNC).
> This queues a RPM_REQ_IDLE request, resulting in the device being suspended
> the next time the work queue is run.
>
> pm_runtime_resume() seem to be unsafe at probe time if the PM workqueue can
> run before the probe function is done with the device, which means pretty much
> everywhere as probe() usually calls functions that can sleep.
>
> I thus agree that a pm_runtime_get_sync() call is needed. The
> pm_runtime_put_sync() call at the end of the probe function could be replaced
> by a pm_runtime_put() call though. The PM runtime documentation should also be
> updated.

Thanks, that validates what I saw but did not get time to fully
trace.

I agree that pm_runtime_put() is probably a better option as we do
not need to ensure that the device is shut down immediately. I will
re-do the patch and submit it tonight.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH/RFC 0/5] Fix the sh_eth race between open and MDIO bus registration

[Laurent Pinchart]

Hi Ben,

On Wednesday 19 March 2014 16:45:01 Ben Dooks wrote:
> On 19/03/14 16:06, Laurent Pinchart wrote:
> > On Wednesday 19 March 2014 14:35:59 Geert Uytterhoeven wrote:
> >> On Wed, Mar 19, 2014 at 11:07 AM, Laurent Pinchart wrote:
> >>> On Wednesday 19 March 2014 10:14:53 Ben Dooks wrote:
> >>>> On 19/03/14 08:41, Geert Uytterhoeven wrote:
> >>>>> On Wed, Mar 19, 2014 at 12:25 AM, Laurent Pinchart wrote:
> >>>>>> Laurent Pinchart (5):
> >>>>>>     sh_eth: Use the platform device for memory allocation
> >>>>>>     sh_eth: Use the platform device as the MDIO bus parent
> >>>>>>     sh_eth: Simplify MDIO bus initialization and release
> >>>>>>     sh_eth: Register MDIO bus before registering the network device
> >>>>>>     sh_eth: Remove goto statements that jump straight to a return
> >>>>> 
> >>>>> Thanks, the changes look fine to me, so
> >>>> 
> >>>> I think the only issue I have is the re-parent of the MDIO device.
> >>>> 
> >>>> My view also is that the probe should explicitly get a reference if it
> >>>> is going to be created sub-devices.
> >>> 
> >>> I'm not sure to follow you, could you please elaborate on that ? What
> >>> should take a reference on what ?
> >> 
> >> I think he means a runtime pm reference, and he is right.
> >> 
> >> I gave your series a try on Koelsch. Now the clock is:
> >>    1. enabled in sh_eth_drv_probe(),
> >>    2. disabled from the worker thread,
> >>    3. enabled and disabled in sh_eth_get_stats(),
> >>    4. enabled in sh_eth_open() for nfsroot,
> >>    5. disabled in sh_eth_close() on shutdown,
> >>    6. enabled and disabled in sh_eth_get_stats().
> >> 
> >> I wondered whether 2 could happen too soon, so I added msleep(2000) to
> >> sh_eth_drv_probe(), just after the call to pm_runtime_resume(). Then it
> >> fails to obtain the MAC address:
> >> 
> >> sh-eth ee700000.ethernet: no valid MAC address supplied, using a random
> >> one.
> >> 
> >> Due to 4, the network hardware works, and it manages to receive an IP
> >> address from my DHCP server. But as the MAC address is wrong, the IP
> >> address is also wrong, and it hangs when trying to mount NFS.
> >> 
> >> Applying Ben's "PATCH] sh_eth: ensure pm_runtime cannot suspend the
> >> device during init" fixes this.
> > 
> > I've investigated the issue. The pm_runtime_resume() call from the sh_eth
> > probe function ends up calling rpm_resume() synchronously. The function
> > resumes the device, and right before returning calls rpm_idle(dev,
> > RPM_ASYNC). This queues a RPM_REQ_IDLE request, resulting in the device
> > being suspended the next time the work queue is run.
> > 
> > pm_runtime_resume() seem to be unsafe at probe time if the PM workqueue
> > can run before the probe function is done with the device, which means
> > pretty much everywhere as probe() usually calls functions that can sleep.
> > 
> > I thus agree that a pm_runtime_get_sync() call is needed. The
> > pm_runtime_put_sync() call at the end of the probe function could be
> > replaced by a pm_runtime_put() call though. The PM runtime documentation
> > should also be updated.
> 
> Thanks, that validates what I saw but did not get time to fully
> trace.
> 
> I agree that pm_runtime_put() is probably a better option as we do
> not need to ensure that the device is shut down immediately. I will
> re-do the patch and submit it tonight.

Thank you. Could you please also submit a patch that fixes the runtime PM 
documentation ? It doesn't have to be long, but driver writers should be 
warned of the potential pm_runtime_resume() issues. This should also help 
getting a reply from the runtime PM developers.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 0/5] Fix the sh_eth race between open and MDIO bus registration

[Ben Dooks]

On 19/03/14 16:48, Laurent Pinchart wrote:
> Hi Ben,
>
> On Wednesday 19 March 2014 16:45:01 Ben Dooks wrote:
>> On 19/03/14 16:06, Laurent Pinchart wrote:
>>> On Wednesday 19 March 2014 14:35:59 Geert Uytterhoeven wrote:
>>>> On Wed, Mar 19, 2014 at 11:07 AM, Laurent Pinchart wrote:
>>>>> On Wednesday 19 March 2014 10:14:53 Ben Dooks wrote:
>>>>>> On 19/03/14 08:41, Geert Uytterhoeven wrote:
>>>>>>> On Wed, Mar 19, 2014 at 12:25 AM, Laurent Pinchart wrote:
>>>>>>>> Laurent Pinchart (5):
>>>>>>>>      sh_eth: Use the platform device for memory allocation
>>>>>>>>      sh_eth: Use the platform device as the MDIO bus parent
>>>>>>>>      sh_eth: Simplify MDIO bus initialization and release
>>>>>>>>      sh_eth: Register MDIO bus before registering the network device
>>>>>>>>      sh_eth: Remove goto statements that jump straight to a return
>>>>>>>
>>>>>>> Thanks, the changes look fine to me, so
>>>>>>
>>>>>> I think the only issue I have is the re-parent of the MDIO device.
>>>>>>
>>>>>> My view also is that the probe should explicitly get a reference if it
>>>>>> is going to be created sub-devices.
>>>>>
>>>>> I'm not sure to follow you, could you please elaborate on that ? What
>>>>> should take a reference on what ?
>>>>
>>>> I think he means a runtime pm reference, and he is right.
>>>>
>>>> I gave your series a try on Koelsch. Now the clock is:
>>>>     1. enabled in sh_eth_drv_probe(),
>>>>     2. disabled from the worker thread,
>>>>     3. enabled and disabled in sh_eth_get_stats(),
>>>>     4. enabled in sh_eth_open() for nfsroot,
>>>>     5. disabled in sh_eth_close() on shutdown,
>>>>     6. enabled and disabled in sh_eth_get_stats().
>>>>
>>>> I wondered whether 2 could happen too soon, so I added msleep(2000) to
>>>> sh_eth_drv_probe(), just after the call to pm_runtime_resume(). Then it
>>>> fails to obtain the MAC address:
>>>>
>>>> sh-eth ee700000.ethernet: no valid MAC address supplied, using a random
>>>> one.
>>>>
>>>> Due to 4, the network hardware works, and it manages to receive an IP
>>>> address from my DHCP server. But as the MAC address is wrong, the IP
>>>> address is also wrong, and it hangs when trying to mount NFS.
>>>>
>>>> Applying Ben's "PATCH] sh_eth: ensure pm_runtime cannot suspend the
>>>> device during init" fixes this.
>>>
>>> I've investigated the issue. The pm_runtime_resume() call from the sh_eth
>>> probe function ends up calling rpm_resume() synchronously. The function
>>> resumes the device, and right before returning calls rpm_idle(dev,
>>> RPM_ASYNC). This queues a RPM_REQ_IDLE request, resulting in the device
>>> being suspended the next time the work queue is run.
>>>
>>> pm_runtime_resume() seem to be unsafe at probe time if the PM workqueue
>>> can run before the probe function is done with the device, which means
>>> pretty much everywhere as probe() usually calls functions that can sleep.
>>>
>>> I thus agree that a pm_runtime_get_sync() call is needed. The
>>> pm_runtime_put_sync() call at the end of the probe function could be
>>> replaced by a pm_runtime_put() call though. The PM runtime documentation
>>> should also be updated.
>>
>> Thanks, that validates what I saw but did not get time to fully
>> trace.
>>
>> I agree that pm_runtime_put() is probably a better option as we do
>> not need to ensure that the device is shut down immediately. I will
>> re-do the patch and submit it tonight.
>
> Thank you. Could you please also submit a patch that fixes the runtime PM
> documentation ? It doesn't have to be long, but driver writers should be
> warned of the potential pm_runtime_resume() issues. This should also help
> getting a reply from the runtime PM developers.

Yes, good idea.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH/RFC 0/5] Fix the sh_eth race between open and MDIO bus registration

[Sergei Shtylyov]

Hello.

On 03/19/2014 02:25 AM, Laurent Pinchart wrote:

> This patch series attempts to fix the race condition that exists in the sh_eth
> driver between network device open and MDIO bus registration. The actual fix
> is in patch 4/5, with previous patches preparing the driver and patch 5/5
> cleaning up an unrelated issue.

> I've based the idea on Sergei's attempt to fix the problem and can successfully
> boot the Koelsch board over NFS with this series. I might have missed other
> issues though, hence the RFC status.

> The patches are based on top of Simon's latest devel branch plus the following
> two patches

> 	sh_eth: add device tree support
> 	sh_eth: update OF PHY registeration

    Oops, I have forgotten to mention these two patches when specifying the 
dependencies of my patch. :-/

> Sergei, assuming this series gets accepted, should I send it to the netdev
> mailing list or would you like to take it in your tree to avoid conflicts ?

    Send them to netdev, I don't have pending patches to 'sh_eth' against 
'net-next.git', so you would just need to rebase against this repo where my 
recent 4 patches have been already merged.

WBR, Sergei