| Message ID | 1465920962-24946-1-git-send-email-jeremy.linton@arm.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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.
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
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
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...
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.
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.
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 >
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
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
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)
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
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...
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
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.
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
> 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
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).
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!
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);
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(+)