* Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
2016-06-14 20:44 ` Sergei Shtylyov
@ 2016-06-14 20:59 ` Jeremy Linton
2016-06-14 21:24 ` Sergei Shtylyov
2016-06-14 22:26 ` Andrew Lunn
2016-06-14 21:02 ` Jeremy Linton
` (2 subsequent siblings)
3 siblings, 2 replies; 24+ messages in thread
From: Jeremy Linton @ 2016-06-14 20:59 UTC (permalink / raw)
To: Sergei Shtylyov, netdev; +Cc: steve.glendinning
On 06/14/2016 03:44 PM, Sergei Shtylyov wrote:
> On 06/14/2016 07:16 PM, Jeremy Linton wrote:
>
>> If the interrupt configuration isn't set and we are using the
>
> It's never set, judging by the driver code.
>
>> internal phy, then we need to poll the phy to reliably detect
>> phy state changes.
>
> What address your internal PHY is at? Mine is at 1, and things seem
> to work reliably after probing:
>
> SMSC LAN8700 18000000.etherne:01: attached PHY driver [SMSC LAN8700]
> (mii_bus:phy_addr=18000000.etherne:01, irq=-1)
Looks like your phy ends up polling (-1==IRQ_POLL)...
>
> I'm using the device tree on my board.
This was DT as well with a recent fedora/NetworkManager. It actually
seems to be timing related to how fast the device gets configured after
the initial phy probe. There is something like a 1 second window or so
where it will work, but if network manager takes longer than that, the
link state drops and cannot be brought back up unless the cable is
pulled, replugged while the netdevice is being restarted.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
2016-06-14 20:59 ` Jeremy Linton
@ 2016-06-14 21:24 ` Sergei Shtylyov
2016-06-14 22:26 ` Andrew Lunn
1 sibling, 0 replies; 24+ messages in thread
From: Sergei Shtylyov @ 2016-06-14 21:24 UTC (permalink / raw)
To: Jeremy Linton, netdev; +Cc: steve.glendinning
On 06/14/2016 11:59 PM, Jeremy Linton wrote:
>>> If the interrupt configuration isn't set and we are using the
>>
>> It's never set, judging by the driver code.
>>
>>> internal phy, then we need to poll the phy to reliably detect
>>> phy state changes.
>>
>> What address your internal PHY is at? Mine is at 1, and things seem
>> to work reliably after probing:
>>
>> SMSC LAN8700 18000000.etherne:01: attached PHY driver [SMSC LAN8700]
>> (mii_bus:phy_addr=18000000.etherne:01, irq=-1)
>
> Looks like your phy ends up polling (-1==IRQ_POLL)...
Yeah. You didn't answer my question though...
>> I'm using the device tree on my board.
>
> This was DT as well with a recent fedora/NetworkManager. It actually seems
> to be timing related to how fast the device gets configured after the initial
> phy probe. There is something like a 1 second window or so where it will work,
> but if network manager takes longer than that, the link state drops and cannot
> be brought back up unless the cable is pulled, replugged while the netdevice
> is being restarted.
1 second is the PHY poll interval IIRC.
MBR, Sergei
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
2016-06-14 20:59 ` Jeremy Linton
2016-06-14 21:24 ` Sergei Shtylyov
@ 2016-06-14 22:26 ` Andrew Lunn
2016-06-15 15:50 ` Jeremy Linton
1 sibling, 1 reply; 24+ messages in thread
From: Andrew Lunn @ 2016-06-14 22:26 UTC (permalink / raw)
To: Jeremy Linton; +Cc: Sergei Shtylyov, netdev, steve.glendinning
> This was DT as well with a recent fedora/NetworkManager. It
> actually seems to be timing related to how fast the device gets
> configured after the initial phy probe. There is something like a 1
> second window or so where it will work, but if network manager takes
> longer than that, the link state drops and cannot be brought back up
> unless the cable is pulled, replugged while the netdevice is being
> restarted.
Ah!
There is another bug in the driver. The phy is connected to the netdev
after calling register_netdev(). You are supposed to do it before,
because the interface is usable, and can be used, directly after the
register.
Move the call to smsc911x_mii_init() before the register_netdev().
Andrew
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
2016-06-14 22:26 ` Andrew Lunn
@ 2016-06-15 15:50 ` Jeremy Linton
0 siblings, 0 replies; 24+ messages in thread
From: Jeremy Linton @ 2016-06-15 15:50 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Sergei Shtylyov, netdev, steve.glendinning
On 06/14/2016 05:26 PM, Andrew Lunn wrote:
>> This was DT as well with a recent fedora/NetworkManager. It
>> actually seems to be timing related to how fast the device gets
>> configured after the initial phy probe. There is something like a 1
>> second window or so where it will work, but if network manager takes
>> longer than that, the link state drops and cannot be brought back up
>> unless the cable is pulled, replugged while the netdevice is being
>> restarted.
>
> Ah!
>
> There is another bug in the driver. The phy is connected to the netdev
> after calling register_netdev(). You are supposed to do it before,
> because the interface is usable, and can be used, directly after the
> register.
>
> Move the call to smsc911x_mii_init() before the register_netdev().
Yah, I buy that, and will move it an see what happens.
But it doesn't solve the problem of the module use count being bumped
in the probe rather than the ndo_open(). The users of
phy_connect_direct() seem to be split between using it in the probe, and
using it in the ndo_open (pxa168, and ax88796 for two examples of using
it in the open).
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
2016-06-14 20:44 ` Sergei Shtylyov
2016-06-14 20:59 ` Jeremy Linton
@ 2016-06-14 21:02 ` Jeremy Linton
2016-06-14 21:12 ` Jeremy Linton
2016-06-14 21:29 ` Jeremy Linton
3 siblings, 0 replies; 24+ messages in thread
From: Jeremy Linton @ 2016-06-14 21:02 UTC (permalink / raw)
To: Sergei Shtylyov, netdev; +Cc: steve.glendinning
On 06/14/2016 03:44 PM, Sergei Shtylyov wrote:
> On 06/14/2016 07:16 PM, Jeremy Linton wrote:
>
>> If the interrupt configuration isn't set and we are using the
>
> It's never set, judging by the driver code.
>
>> internal phy, then we need to poll the phy to reliably detect
>> phy state changes.
>
> What address your internal PHY is at? Mine is at 1, and things seem
> to work reliably after probing:
>
> SMSC LAN8700 18000000.etherne:01: attached PHY driver [SMSC LAN8700]
> (mii_bus:phy_addr=18000000.etherne:01, irq=-1)
>
BTW, the phy that is causing the problem is the one
labeled "SMSC LAN911x Internal PHY" in phy/smsc.c.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
2016-06-14 20:44 ` Sergei Shtylyov
2016-06-14 20:59 ` Jeremy Linton
2016-06-14 21:02 ` Jeremy Linton
@ 2016-06-14 21:12 ` Jeremy Linton
2016-06-14 21:26 ` Sergei Shtylyov
2016-06-14 21:29 ` Jeremy Linton
3 siblings, 1 reply; 24+ messages in thread
From: Jeremy Linton @ 2016-06-14 21:12 UTC (permalink / raw)
To: Sergei Shtylyov, netdev; +Cc: steve.glendinning
On 06/14/2016 03:44 PM, Sergei Shtylyov wrote:
> On 06/14/2016 07:16 PM, Jeremy Linton wrote:
>
>> If the interrupt configuration isn't set and we are using the
>
> It's never set, judging by the driver code.
AFAIK, I think that its set when the device is configured as a platform
device, or there is an external phy/interrupt setup in DT. I might be
wrong on that..
>
>> internal phy, then we need to poll the phy to reliably detect
>> phy state changes.
>
> What address your internal PHY is at? Mine is at 1, and things seem
> to work reliably after probing:
>
> SMSC LAN8700 18000000.etherne:01: attached PHY driver [SMSC LAN8700]
> (mii_bus:phy_addr=18000000.etherne:01, irq=-1)
>
> I'm using the device tree on my board.
>
> MBR, Sergei
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
2016-06-14 21:12 ` Jeremy Linton
@ 2016-06-14 21:26 ` Sergei Shtylyov
0 siblings, 0 replies; 24+ messages in thread
From: Sergei Shtylyov @ 2016-06-14 21:26 UTC (permalink / raw)
To: Jeremy Linton, netdev; +Cc: steve.glendinning
On 06/15/2016 12:12 AM, Jeremy Linton wrote:
>>> If the interrupt configuration isn't set and we are using the
>>
>> It's never set, judging by the driver code.
>
> AFAIK, I think that its set when the device is configured as a platform
> device, or there is an external phy/interrupt setup in DT. I might be wrong on
> that..
I totally fail to see that, even in net-next. The only place that uses
'phy_irq' is that buggy memcpy()...
MBR, Sergei
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
2016-06-14 20:44 ` Sergei Shtylyov
` (2 preceding siblings ...)
2016-06-14 21:12 ` Jeremy Linton
@ 2016-06-14 21:29 ` Jeremy Linton
2016-06-14 21:34 ` Sergei Shtylyov
3 siblings, 1 reply; 24+ messages in thread
From: Jeremy Linton @ 2016-06-14 21:29 UTC (permalink / raw)
To: Sergei Shtylyov, netdev; +Cc: steve.glendinning
On 06/14/2016 03:44 PM, Sergei Shtylyov wrote:
> On 06/14/2016 07:16 PM, Jeremy Linton wrote:
>
>> If the interrupt configuration isn't set and we are using the
>
> It's never set, judging by the driver code.
>
>> internal phy, then we need to poll the phy to reliably detect
>> phy state changes.
>
> What address your internal PHY is at? Mine is at 1, and things seem
> to work reliably after probing:
>
> SMSC LAN8700 18000000.etherne:01: attached PHY driver [SMSC LAN8700]
> (mii_bus:phy_addr=18000000.etherne:01, irq=-1)
>
> I'm using the device tree on my board.
Ok, I'm back on the machine, this is what mine says without that patch.
SMSC LAN911x Internal PHY 18000000.etherne:01: attached PHY driver [SMSC
LAN911x Internal PHY] (mii_bus:phy_addr=18000000.etherne:01, irq=0)
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
2016-06-14 21:29 ` Jeremy Linton
@ 2016-06-14 21:34 ` Sergei Shtylyov
2016-06-14 21:40 ` Jeremy Linton
0 siblings, 1 reply; 24+ messages in thread
From: Sergei Shtylyov @ 2016-06-14 21:34 UTC (permalink / raw)
To: Jeremy Linton, netdev; +Cc: steve.glendinning
On 06/15/2016 12:29 AM, Jeremy Linton wrote:
>>> If the interrupt configuration isn't set and we are using the
>>
>> It's never set, judging by the driver code.
>>
>>> internal phy, then we need to poll the phy to reliably detect
>>> phy state changes.
>>
>> What address your internal PHY is at? Mine is at 1, and things seem
>> to work reliably after probing:
>>
>> SMSC LAN8700 18000000.etherne:01: attached PHY driver [SMSC LAN8700]
>> (mii_bus:phy_addr=18000000.etherne:01, irq=-1)
>>
>> I'm using the device tree on my board.
>
> Ok, I'm back on the machine, this is what mine says without that patch.
>
>
>
> SMSC LAN911x Internal PHY 18000000.etherne:01: attached PHY driver [SMSC
> LAN911x Internal PHY] (mii_bus:phy_addr=18000000.etherne:01, irq=0)
Hum, that's unexpected... things are probably more complex that I thought.
Do you have extra patches to this driver by changce?
MBR, Sergei
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
2016-06-14 21:34 ` Sergei Shtylyov
@ 2016-06-14 21:40 ` Jeremy Linton
2016-06-14 21:42 ` Sergei Shtylyov
0 siblings, 1 reply; 24+ messages in thread
From: Jeremy Linton @ 2016-06-14 21:40 UTC (permalink / raw)
To: Sergei Shtylyov, netdev; +Cc: steve.glendinning
On 06/14/2016 04:34 PM, Sergei Shtylyov wrote:
> On 06/15/2016 12:29 AM, Jeremy Linton wrote:
>
>>>> If the interrupt configuration isn't set and we are using the
>>>
>>> It's never set, judging by the driver code.
>>>
>>>> internal phy, then we need to poll the phy to reliably detect
>>>> phy state changes.
>>>
>>> What address your internal PHY is at? Mine is at 1, and things seem
>>> to work reliably after probing:
>>>
>>> SMSC LAN8700 18000000.etherne:01: attached PHY driver [SMSC LAN8700]
>>> (mii_bus:phy_addr=18000000.etherne:01, irq=-1)
>>>
>>> I'm using the device tree on my board.
>>
>> Ok, I'm back on the machine, this is what mine says without that patch.
>>
>>
>>
>> SMSC LAN911x Internal PHY 18000000.etherne:01: attached PHY driver [SMSC
>> LAN911x Internal PHY] (mii_bus:phy_addr=18000000.etherne:01, irq=0)
>
> Hum, that's unexpected... things are probably more complex that I
> thought. Do you have extra patches to this driver by changce?
No, the initial kernel where the problem was discovered is
4.5.2-301.fc24.aarch64, but I built a mainline 4.6, and modprobed the
driver with the same effect.
Although, now that I'm looking closer at phy_irq, I'm curious how it
works for anyone else...
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
2016-06-14 21:40 ` Jeremy Linton
@ 2016-06-14 21:42 ` Sergei Shtylyov
2016-06-14 21:53 ` Jeremy Linton
0 siblings, 1 reply; 24+ messages in thread
From: Sergei Shtylyov @ 2016-06-14 21:42 UTC (permalink / raw)
To: Jeremy Linton, netdev; +Cc: steve.glendinning
On 06/15/2016 12:40 AM, Jeremy Linton wrote:
>>>>> If the interrupt configuration isn't set and we are using the
>>>>
>>>> It's never set, judging by the driver code.
>>>>
>>>>> internal phy, then we need to poll the phy to reliably detect
>>>>> phy state changes.
>>>>
>>>> What address your internal PHY is at? Mine is at 1, and things seem
>>>> to work reliably after probing:
>>>>
>>>> SMSC LAN8700 18000000.etherne:01: attached PHY driver [SMSC LAN8700]
>>>> (mii_bus:phy_addr=18000000.etherne:01, irq=-1)
>>>>
>>>> I'm using the device tree on my board.
>>>
>>> Ok, I'm back on the machine, this is what mine says without that patch.
>>>
>>> SMSC LAN911x Internal PHY 18000000.etherne:01: attached PHY driver [SMSC
>>> LAN911x Internal PHY] (mii_bus:phy_addr=18000000.etherne:01, irq=0)
>>
>> Hum, that's unexpected... things are probably more complex that I
>> thought. Do you have extra patches to this driver by changce?
>
> No, the initial kernel where the problem was discovered is
> 4.5.2-301.fc24.aarch64, but I built a mainline 4.6, and modprobed the driver
> with the same effect.
>
>
> Although, now that I'm looking closer at phy_irq, I'm curious how it works for
> anyone else...
Does anything change when you comment out that memcpy()? It shouldn't
probably...
MBR, Sergei
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
2016-06-14 21:42 ` Sergei Shtylyov
@ 2016-06-14 21:53 ` Jeremy Linton
2016-06-14 21:56 ` Sergei Shtylyov
0 siblings, 1 reply; 24+ messages in thread
From: Jeremy Linton @ 2016-06-14 21:53 UTC (permalink / raw)
To: Sergei Shtylyov, netdev; +Cc: steve.glendinning
On 06/14/2016 04:42 PM, Sergei Shtylyov wrote:
> On 06/15/2016 12:40 AM, Jeremy Linton wrote:
>
>>>>>> If the interrupt configuration isn't set and we are using the
>>>>>
>>>>> It's never set, judging by the driver code.
>>>>>
>>>>>> internal phy, then we need to poll the phy to reliably detect
>>>>>> phy state changes.
>>>>>
>>>>> What address your internal PHY is at? Mine is at 1, and things
>>>>> seem
>>>>> to work reliably after probing:
>>>>>
>>>>> SMSC LAN8700 18000000.etherne:01: attached PHY driver [SMSC LAN8700]
>>>>> (mii_bus:phy_addr=18000000.etherne:01, irq=-1)
>>>>>
>>>>> I'm using the device tree on my board.
>>>>
>>>> Ok, I'm back on the machine, this is what mine says without that patch.
>>>>
>>>> SMSC LAN911x Internal PHY 18000000.etherne:01: attached PHY driver
>>>> [SMSC
>>>> LAN911x Internal PHY] (mii_bus:phy_addr=18000000.etherne:01, irq=0)
>>>
>>> Hum, that's unexpected... things are probably more complex that I
>>> thought. Do you have extra patches to this driver by changce?
>>
>> No, the initial kernel where the problem was discovered is
>> 4.5.2-301.fc24.aarch64, but I built a mainline 4.6, and modprobed the
>> driver
>> with the same effect.
>>
>>
>> Although, now that I'm looking closer at phy_irq, I'm curious how it
>> works for
>> anyone else...
>
> Does anything change when you comment out that memcpy()? It
> shouldn't probably...
Well that should change the irq to PHY_POLL by default rather than the
0's in the structure, which may be a better patch.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
2016-06-14 21:53 ` Jeremy Linton
@ 2016-06-14 21:56 ` Sergei Shtylyov
2016-06-15 15:56 ` Jeremy Linton
0 siblings, 1 reply; 24+ messages in thread
From: Sergei Shtylyov @ 2016-06-14 21:56 UTC (permalink / raw)
To: Jeremy Linton, netdev; +Cc: steve.glendinning
On 06/15/2016 12:53 AM, Jeremy Linton wrote:
>>>>>>> If the interrupt configuration isn't set and we are using the
>>>>>>
>>>>>> It's never set, judging by the driver code.
>>>>>>
>>>>>>> internal phy, then we need to poll the phy to reliably detect
>>>>>>> phy state changes.
>>>>>>
>>>>>> What address your internal PHY is at? Mine is at 1, and things
>>>>>> seem
>>>>>> to work reliably after probing:
>>>>>>
>>>>>> SMSC LAN8700 18000000.etherne:01: attached PHY driver [SMSC LAN8700]
>>>>>> (mii_bus:phy_addr=18000000.etherne:01, irq=-1)
>>>>>>
>>>>>> I'm using the device tree on my board.
>>>>>
>>>>> Ok, I'm back on the machine, this is what mine says without that patch.
>>>>>
>>>>> SMSC LAN911x Internal PHY 18000000.etherne:01: attached PHY driver
>>>>> [SMSC
>>>>> LAN911x Internal PHY] (mii_bus:phy_addr=18000000.etherne:01, irq=0)
>>>>
>>>> Hum, that's unexpected... things are probably more complex that I
>>>> thought. Do you have extra patches to this driver by changce?
>>>
>>> No, the initial kernel where the problem was discovered is
>>> 4.5.2-301.fc24.aarch64, but I built a mainline 4.6, and modprobed the
>>> driver
>>> with the same effect.
>>>
>>> Although, now that I'm looking closer at phy_irq, I'm curious how it
>>> works for
>>> anyone else...
>>
>> Does anything change when you comment out that memcpy()? It
>> shouldn't probably...
>
> Well that should change the irq to PHY_POLL by default rather than the 0's
> in the structure, which may be a better patch.
It shouldn't due to the wrong size. It should only overwrite IRQ and index
0, unless I'm mistaken.
MBR, Sergei
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL
2016-06-14 21:56 ` Sergei Shtylyov
@ 2016-06-15 15:56 ` Jeremy Linton
0 siblings, 0 replies; 24+ messages in thread
From: Jeremy Linton @ 2016-06-15 15:56 UTC (permalink / raw)
To: Sergei Shtylyov, netdev; +Cc: steve.glendinning
On 06/14/2016 04:56 PM, Sergei Shtylyov wrote:
> On 06/15/2016 12:53 AM, Jeremy Linton wrote:
>
>>>>>>>> If the interrupt configuration isn't set and we are using the
>>>>>>>
>>>>>>> It's never set, judging by the driver code.
>>>>>>>
>>>>>>>> internal phy, then we need to poll the phy to reliably detect
>>>>>>>> phy state changes.
>>>>>>>
>>>>>>> What address your internal PHY is at? Mine is at 1, and things
>>>>>>> seem
>>>>>>> to work reliably after probing:
>>>>>>>
>>>>>>> SMSC LAN8700 18000000.etherne:01: attached PHY driver [SMSC LAN8700]
>>>>>>> (mii_bus:phy_addr=18000000.etherne:01, irq=-1)
>>>>>>>
>>>>>>> I'm using the device tree on my board.
>>>>>>
>>>>>> Ok, I'm back on the machine, this is what mine says without that
>>>>>> patch.
>>>>>>
>>>>>> SMSC LAN911x Internal PHY 18000000.etherne:01: attached PHY driver
>>>>>> [SMSC
>>>>>> LAN911x Internal PHY] (mii_bus:phy_addr=18000000.etherne:01, irq=0)
>>>>>
>>>>> Hum, that's unexpected... things are probably more complex that I
>>>>> thought. Do you have extra patches to this driver by changce?
>>>>
>>>> No, the initial kernel where the problem was discovered is
>>>> 4.5.2-301.fc24.aarch64, but I built a mainline 4.6, and modprobed the
>>>> driver
>>>> with the same effect.
>>>>
>>>> Although, now that I'm looking closer at phy_irq, I'm curious how it
>>>> works for
>>>> anyone else...
>>>
>>> Does anything change when you comment out that memcpy()? It
>>> shouldn't probably...
>>
>> Well that should change the irq to PHY_POLL by default rather than
>> the 0's
>> in the structure, which may be a better patch.
>
> It shouldn't due to the wrong size. It should only overwrite IRQ and
> index 0, unless I'm mistaken.
Oh, sizeof(pointer)==8 on arm64, yah in the arm32 case you dodge the
bullet.
I think the memcpy removal solves the problem, i'm also going to test
moving the mii_probe and will post an updated patch.
Thanks!
^ permalink raw reply [flat|nested] 24+ messages in thread