Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1679558 > unrolled thread

Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral Geert Uytterhoeven <geert@linux-m68k.org> - 2017-07-02 22:40 +0200
    Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral Florian Fainelli <f.fainelli@gmail.com> - 2017-07-09 18:50 +0200
      Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral Andrew Lunn <andrew@lunn.ch> - 2017-07-09 19:30 +0200
        Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral Geert Uytterhoeven <geert@linux-m68k.org> - 2017-07-09 21:40 +0200

#1679558 — Re: [PATCH] of_mdio: Fix broken PHY IRQ in case of probe deferral

On Tue, Jun 6, 2017 at 11:43 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, May 23, 2017 at 11:36 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Fri, May 19, 2017 at 12:21 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 05/18/2017 01:36 PM, Geert Uytterhoeven wrote:
>>>> On Thu, May 18, 2017 at 9:34 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>>>>>> This most certainly works fine in the simple case where you have one PHY
>>>>>>> hanging off the MDIO bus, now what happens if you have several?
>>>>>>>
>>>>>>> Presumably, the first PHY that returns EPROBE_DEFER will make the entire
>>>>>>> bus registration return EPROB_DEFER as well, and so on, and so forth,
>>>>>>> but I am not sure if we will be properly unwinding the successful
>>>>>>> registration of PHYs that either don't have an interrupt, or did not
>>>>>>> return EPROBE_DEFER.
>>>>>>>
>>>>>>> It should be possible to mimic this behavior by using the fixed PHY, and
>>>>>>> possibly the dsa_loop.c driver which would create 4 ports, expecting 4
>>>>>>> fixed PHYs to be present.
>>>>>>
>>>>>> mdiobus_unregister(), called from of_mdiobus_register() on failure,
>>>>>> should do the unwinding, right?
>>>>>>
>>>>>> And when the driver is reprobed, all PHYs are reprobed, until they all
>>>>>> succeed.
>>>>>
>>>>> That is the theory. I looked at that while reviewing the patch. But
>>>>> this has probably not been tested in anger. It would be good to test
>>>>> this properly, with not just the first PHY returning -EPROBE_DEFER, to
>>>>> really test the unwind.
>>>>
>>>> Unfortunately I don't have a board with multiple PHYs, so I cannot test
>>>> that case.
>>
>> I tried adding a few dummy PHYs in DT, but that didn't work.
>>
>> So how can we proceed?
>>
>> I think the only way my patch can cause issues is because some systems
>> may rely on EPROBE_DEFER errors being ignored.
>>
>>>> Does unbinding/rebinding a network driver with multiple PHYs currently
>>>> work? Or module unload/reload?
>>>
>>> Usually there is a strict 1:1 mapping between a network device (not
>>> driver) and a PHY device, switch drivers however, would have multiple
>>> PHYs (one per port, aka net_deice).
>>>
>>> NB: binding and unbinding of PHYs is pretty broken at the moment though,
>>> because there is a complete disconnect between what the Ethernet MAC
>>> expects, and the state in which the PHY is. I had some patches to fix
>>> that, but this turned out to be playing whack-a-mole which I typically
>>> suck at.
>>
>> I didn't mean unbinding the PHY, but the network device.
>> Don't you have the same issue with the state of PHYs as left by the bootloader?
>
> Anyone who can test the behavior on an Ethernet device with multiple PHYs,
> e.g. by faking an -EPROBE_DEFER somewhere in the middle?
>
> I'd like to get this issue fixed in v4.13, to avoid a regression when migrating
> several systems to a new and better clock driver in v4.14, which will trigger
> EPROBE_DEFER.

Ping?

This patch fixes a real issue.

Thanks!

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

[toc] | [next] | [standalone]

#1683766

On 07/02/2017 01:37 PM, Geert Uytterhoeven wrote:
> On Tue, Jun 6, 2017 at 11:43 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Tue, May 23, 2017 at 11:36 AM, Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>>> On Fri, May 19, 2017 at 12:21 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>> On 05/18/2017 01:36 PM, Geert Uytterhoeven wrote:
>>>>> On Thu, May 18, 2017 at 9:34 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>>>>>>> This most certainly works fine in the simple case where you have one PHY
>>>>>>>> hanging off the MDIO bus, now what happens if you have several?
>>>>>>>>
>>>>>>>> Presumably, the first PHY that returns EPROBE_DEFER will make the entire
>>>>>>>> bus registration return EPROB_DEFER as well, and so on, and so forth,
>>>>>>>> but I am not sure if we will be properly unwinding the successful
>>>>>>>> registration of PHYs that either don't have an interrupt, or did not
>>>>>>>> return EPROBE_DEFER.
>>>>>>>>
>>>>>>>> It should be possible to mimic this behavior by using the fixed PHY, and
>>>>>>>> possibly the dsa_loop.c driver which would create 4 ports, expecting 4
>>>>>>>> fixed PHYs to be present.
>>>>>>>
>>>>>>> mdiobus_unregister(), called from of_mdiobus_register() on failure,
>>>>>>> should do the unwinding, right?
>>>>>>>
>>>>>>> And when the driver is reprobed, all PHYs are reprobed, until they all
>>>>>>> succeed.
>>>>>>
>>>>>> That is the theory. I looked at that while reviewing the patch. But
>>>>>> this has probably not been tested in anger. It would be good to test
>>>>>> this properly, with not just the first PHY returning -EPROBE_DEFER, to
>>>>>> really test the unwind.
>>>>>
>>>>> Unfortunately I don't have a board with multiple PHYs, so I cannot test
>>>>> that case.
>>>
>>> I tried adding a few dummy PHYs in DT, but that didn't work.
>>>
>>> So how can we proceed?
>>>
>>> I think the only way my patch can cause issues is because some systems
>>> may rely on EPROBE_DEFER errors being ignored.
>>>
>>>>> Does unbinding/rebinding a network driver with multiple PHYs currently
>>>>> work? Or module unload/reload?
>>>>
>>>> Usually there is a strict 1:1 mapping between a network device (not
>>>> driver) and a PHY device, switch drivers however, would have multiple
>>>> PHYs (one per port, aka net_deice).
>>>>
>>>> NB: binding and unbinding of PHYs is pretty broken at the moment though,
>>>> because there is a complete disconnect between what the Ethernet MAC
>>>> expects, and the state in which the PHY is. I had some patches to fix
>>>> that, but this turned out to be playing whack-a-mole which I typically
>>>> suck at.
>>>
>>> I didn't mean unbinding the PHY, but the network device.
>>> Don't you have the same issue with the state of PHYs as left by the bootloader?
>>
>> Anyone who can test the behavior on an Ethernet device with multiple PHYs,
>> e.g. by faking an -EPROBE_DEFER somewhere in the middle?
>>
>> I'd like to get this issue fixed in v4.13, to avoid a regression when migrating
>> several systems to a new and better clock driver in v4.14, which will trigger
>> EPROBE_DEFER.
> 
> Ping?
> 
> This patch fixes a real issue.

It sure does fix a real issue, but I am really concerned about the
inability to test this patch in a configuration where we have multiple
PHY(s) or MDIO device(s) hanging off the same MDIO bus and one of those
requesting an EPROBE_DEFER.

I currently don't have a setup where I could exercise this, Andrew, do you?
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1683773

> It sure does fix a real issue, but I am really concerned about the
> inability to test this patch in a configuration where we have multiple
> PHY(s) or MDIO device(s) hanging off the same MDIO bus and one of those
> requesting an EPROBE_DEFER.
> 
> I currently don't have a setup where I could exercise this, Andrew, do you?

Hi Florian

What i do have, is a switch with some built in copper Marvell PHYs and
external SFF modules which use fixed link. I can probably hack the
fixed-link driver to return EPROBE_DEFER a few times.

	   Andrew

[toc] | [prev] | [next] | [standalone]

#1683827

Hi Florian, Andrew,

On Sun, Jul 9, 2017 at 7:28 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> It sure does fix a real issue, but I am really concerned about the
>> inability to test this patch in a configuration where we have multiple
>> PHY(s) or MDIO device(s) hanging off the same MDIO bus and one of those
>> requesting an EPROBE_DEFER.

If that case happens now, it silently falls back to polling, hiding the
problem.

Actually it means there may be users that rely on this broken behavior,
that start to fail when their interrupts are suddenly handled :-(

>> I currently don't have a setup where I could exercise this, Andrew, do you?
>
> What i do have, is a switch with some built in copper Marvell PHYs and
> external SFF modules which use fixed link. I can probably hack the
> fixed-link driver to return EPROBE_DEFER a few times.

That would be great, thanks!

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web