[PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

If the interrupt configuration isn't set and we are using the
internal phy, then we need to poll the phy to reliably detect
phy state changes.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index 8af2556..369dc7d 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1020,6 +1020,9 @@ static int smsc911x_mii_probe(struct net_device *dev)
 		return -ENODEV;
 	}
 
+	if ((!phydev->irq) && (!pdata->using_extphy))
+		phydev->irq = PHY_POLL;
+
 	SMSC_TRACE(pdata, probe, "PHY: addr %d, phy_id 0x%08X",
 		   phydev->mdio.addr, phydev->phy_id);
 
-- 
2.5.5

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Sergei Shtylyov]

On 06/14/2016 07:16 PM, Jeremy Linton wrote:

> If the interrupt configuration isn't set and we are using the
> internal phy, then we need to poll the phy to reliably detect
> phy state changes.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  drivers/net/ethernet/smsc/smsc911x.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
> index 8af2556..369dc7d 100644
> --- a/drivers/net/ethernet/smsc/smsc911x.c
> +++ b/drivers/net/ethernet/smsc/smsc911x.c
> @@ -1020,6 +1020,9 @@ static int smsc911x_mii_probe(struct net_device *dev)
>  		return -ENODEV;
>  	}
>
> +	if ((!phydev->irq) && (!pdata->using_extphy))

    Inner parens aren't needed at all.

[...]

MBR, Sergei

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Andrew Lunn]

On Tue, Jun 14, 2016 at 11:16:02AM -0500, Jeremy Linton wrote:
> If the interrupt configuration isn't set and we are using the
> internal phy, then we need to poll the phy to reliably detect
> phy state changes.

Hi Jeremy

Why does the external phy not have the exact same problem?

    Andrew

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Sergei Shtylyov]

On 06/14/2016 09:30 PM, Sergei Shtylyov wrote:

>> If the interrupt configuration isn't set and we are using the
>> internal phy, then we need to poll the phy to reliably detect
>> phy state changes.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>  drivers/net/ethernet/smsc/smsc911x.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/smsc/smsc911x.c
>> b/drivers/net/ethernet/smsc/smsc911x.c
>> index 8af2556..369dc7d 100644
>> --- a/drivers/net/ethernet/smsc/smsc911x.c
>> +++ b/drivers/net/ethernet/smsc/smsc911x.c
>> @@ -1020,6 +1020,9 @@ static int smsc911x_mii_probe(struct net_device *dev)
>>          return -ENODEV;
>>      }
>>
>> +    if ((!phydev->irq) && (!pdata->using_extphy))
>
>    Inner parens aren't needed at all.

    Hm, 'phydev->irq' shouldn't be 0 in the first place. It seems to me we 
should correctly initialize 'pdata->phy_irq[]' in smsc911x_mii_init()...

MBR, Sergei

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Sergei Shtylyov]

On 06/14/2016 10:27 PM, Sergei Shtylyov wrote:

>>> If the interrupt configuration isn't set and we are using the
>>> internal phy, then we need to poll the phy to reliably detect
>>> phy state changes.
>>>
>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>> ---
>>>  drivers/net/ethernet/smsc/smsc911x.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/smsc/smsc911x.c
>>> b/drivers/net/ethernet/smsc/smsc911x.c
>>> index 8af2556..369dc7d 100644
>>> --- a/drivers/net/ethernet/smsc/smsc911x.c
>>> +++ b/drivers/net/ethernet/smsc/smsc911x.c
>>> @@ -1020,6 +1020,9 @@ static int smsc911x_mii_probe(struct net_device *dev)
>>>          return -ENODEV;
>>>      }
>>>
>>> +    if ((!phydev->irq) && (!pdata->using_extphy))
>>
>>    Inner parens aren't needed at all.
>
>    Hm, 'phydev->irq' shouldn't be 0 in the first place. It seems to me we
> should correctly initialize 'pdata->phy_irq[]' in smsc911x_mii_init()...

    And looking at that array, I doubt it's really useful for anything... And 
the memcpy() there seems buggy as well -- it copies just 4 bytes of this array 
to 'pdata->mii_bus->irq'. I do care about this driver, so might be a good idea 
to clean it up a bit...

MBR, Sergei

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Andrew Lunn]

On Tue, Jun 14, 2016 at 10:49:20PM +0300, Sergei Shtylyov wrote:
> On 06/14/2016 10:27 PM, Sergei Shtylyov wrote:
> 
> >>>If the interrupt configuration isn't set and we are using the
> >>>internal phy, then we need to poll the phy to reliably detect
> >>>phy state changes.
> >>>
> >>>Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> >>>---
> >>> drivers/net/ethernet/smsc/smsc911x.c | 3 +++
> >>> 1 file changed, 3 insertions(+)
> >>>
> >>>diff --git a/drivers/net/ethernet/smsc/smsc911x.c
> >>>b/drivers/net/ethernet/smsc/smsc911x.c
> >>>index 8af2556..369dc7d 100644
> >>>--- a/drivers/net/ethernet/smsc/smsc911x.c
> >>>+++ b/drivers/net/ethernet/smsc/smsc911x.c
> >>>@@ -1020,6 +1020,9 @@ static int smsc911x_mii_probe(struct net_device *dev)
> >>>         return -ENODEV;
> >>>     }
> >>>
> >>>+    if ((!phydev->irq) && (!pdata->using_extphy))
> >>
> >>   Inner parens aren't needed at all.
> >
> >   Hm, 'phydev->irq' shouldn't be 0 in the first place. It seems to me we
> >should correctly initialize 'pdata->phy_irq[]' in smsc911x_mii_init()...

Hi Sergei

The mdio layer, when it allocates the mdiobus structure, will
initialise all the phy interrupts to polling.

>    And looking at that array, I doubt it's really useful for
> anything... And the memcpy() there seems buggy as well -- it copies
> just 4 bytes of this array to 'pdata->mii_bus->irq'.

0 is not a valid interrupt. So it should probably loop over the array
and copy any which are not 0 into pdata->mii_bus->irq[].

    Andrew

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

On 06/14/2016 02:49 PM, Sergei Shtylyov wrote:
> On 06/14/2016 10:27 PM, Sergei Shtylyov wrote:
>
>>>> If the interrupt configuration isn't set and we are using the
>>>> internal phy, then we need to poll the phy to reliably detect
>>>> phy state changes.
>>>>
>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>>> ---
>>>>  drivers/net/ethernet/smsc/smsc911x.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/smsc/smsc911x.c
>>>> b/drivers/net/ethernet/smsc/smsc911x.c
>>>> index 8af2556..369dc7d 100644
>>>> --- a/drivers/net/ethernet/smsc/smsc911x.c
>>>> +++ b/drivers/net/ethernet/smsc/smsc911x.c
>>>> @@ -1020,6 +1020,9 @@ static int smsc911x_mii_probe(struct
>>>> net_device *dev)
>>>>          return -ENODEV;
>>>>      }
>>>>
>>>> +    if ((!phydev->irq) && (!pdata->using_extphy))
>>>
>>>    Inner parens aren't needed at all.
>>
>>    Hm, 'phydev->irq' shouldn't be 0 in the first place. It seems to me we
>> should correctly initialize 'pdata->phy_irq[]' in smsc911x_mii_init()...
>
>     And looking at that array, I doubt it's really useful for
> anything... And the memcpy() there seems buggy as well -- it copies just
> 4 bytes of this array to 'pdata->mii_bus->irq'. I do care about this
> driver, so might be a good idea to clean it up a bit...

	The use of phy_connect_direct() in the driver probe is incorrect, and 
keeps the driver from being unloaded.

	Also, some portion of smsc's can deliver mii state change interrupts 
via the smsc interrupt, but that code is no longer in this driver.

	I suspect a portion of the problem, besides all the strange hardware 
configurations this driver supports are the emulated hardware like QEMU 
that also uses it.

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Sergei Shtylyov]

On 06/14/2016 11:12 PM, Andrew Lunn wrote:
> On Tue, Jun 14, 2016 at 10:49:20PM +0300, Sergei Shtylyov wrote:
>> On 06/14/2016 10:27 PM, Sergei Shtylyov wrote:
>>
>>>>> If the interrupt configuration isn't set and we are using the
>>>>> internal phy, then we need to poll the phy to reliably detect
>>>>> phy state changes.
>>>>>
>>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>>>> ---
>>>>> drivers/net/ethernet/smsc/smsc911x.c | 3 +++
>>>>> 1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/smsc/smsc911x.c
>>>>> b/drivers/net/ethernet/smsc/smsc911x.c
>>>>> index 8af2556..369dc7d 100644
>>>>> --- a/drivers/net/ethernet/smsc/smsc911x.c
>>>>> +++ b/drivers/net/ethernet/smsc/smsc911x.c
>>>>> @@ -1020,6 +1020,9 @@ static int smsc911x_mii_probe(struct net_device *dev)
>>>>>         return -ENODEV;
>>>>>     }
>>>>>
>>>>> +    if ((!phydev->irq) && (!pdata->using_extphy))
>>>>
>>>>   Inner parens aren't needed at all.
>>>
>>>   Hm, 'phydev->irq' shouldn't be 0 in the first place. It seems to me we
>>> should correctly initialize 'pdata->phy_irq[]' in smsc911x_mii_init()...
>
> Hi Sergei
>
> The mdio layer, when it allocates the mdiobus structure, will
> initialise all the phy interrupts to polling.

    And this whole array would get overwritten with zeros by memcpy() iff that 
one was correct (it really overwrites only index 0).

>>    And looking at that array, I doubt it's really useful for
>> anything... And the memcpy() there seems buggy as well -- it copies
>> just 4 bytes of this array to 'pdata->mii_bus->irq'.
>
> 0 is not a valid interrupt.

    Or at least Linus says so... :-)

> So it should probably loop over the array
> and copy any which are not 0 into pdata->mii_bus->irq[].

    I don't see where that array is explicitly initialized in the first place. 
So it seems actively harmful...

>     Andrew

MBR, Sergei

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Sergei Shtylyov]

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.

MBR, Sergei

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

On 06/14/2016 01:42 PM, Andrew Lunn wrote:
> On Tue, Jun 14, 2016 at 11:16:02AM -0500, Jeremy Linton wrote:
>> If the interrupt configuration isn't set and we are using the
>> internal phy, then we need to poll the phy to reliably detect
>> phy state changes.
>
> Hi Jeremy
>  Why does the external phy not have the exact same problem?

Hello,

It may... but I've only got a fairly limited hardware testing setup, and 
this driver is used across a pretty broad set of hardware configured in 
various ways. So, I'm trying to limit the scope of things I might break. 
Initially I thought it was added to avoid a QEMU issue, but now I don't 
think that is the case.

So, if 0 can't be an interrupt why not update phy_interrupt_is_valid() 
to check for it? That would fix the problem too...

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

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.

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

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.

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

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
>

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Sergei Shtylyov]

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

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Sergei Shtylyov]

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

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

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)

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Sergei Shtylyov]

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

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

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...

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Sergei Shtylyov]

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

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

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.

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Sergei Shtylyov]

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

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Andrew Lunn]

> 	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

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

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).
	

Re: [PATCH] net: smsc911x: If PHY doesn't have an interrupt then POLL

[Jeremy Linton]

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!