[BUG] SIOCSIFFLAGS returns -EIO on SMSC LAN911x

[BUG] SIOCSIFFLAGS returns -EIO on SMSC LAN911x

[Javier Martinez Canillas]

Hello,

We have an issue with the SMSC LAN911x driver with today linux-2.6 [1]
executing on an IGEPv2 [2] board using omap2plus_defconfig.

When I try to bring up the network interface, the ioctl set interface
flags command (SIOCSIFFLAGS)  fails returning -EIO:

root@igep00x0:~# ifconfig eth0 up
ifconfig: SIOCSIFFLAGS: Input/output error

But if I execute again it succeeds:

root@igep00x0:~# ifconfig eth0 up
[  832.465423] smsc911x smsc911x.0: eth0: SMSC911x/921x identified at
0xdb086000, IRQ: 336

We found that the problem is on the SMSC LAN911x driver
(drivers/net/ethernet/smsc/smsc911x.c) net_dev ndo_open function
handler smsc911x_open.

When the handler is called, it tries to software reset the device by
writing a bit to a self-clearing configuration register. According to
the data-sheet the hardware clears that bit to notify that the reset
was successfully:

 static int smsc911x_soft_reset(struct smsc911x_data *pdata)
{
       unsigned int timeout;
       unsigned int temp;

       /* Reset the LAN911x */
       smsc911x_reg_write(pdata, HW_CFG, HW_CFG_SRST_);
       timeout = 10;
       do {
               udelay(10);
               temp = smsc911x_reg_read(pdata, HW_CFG);
       } while ((--timeout) && (temp & HW_CFG_SRST_));

       if (unlikely(temp & HW_CFG_SRST_)) {
               SMSC_WARN(pdata, drv, "Failed to complete reset");
               return -EIO;
       }
       return 0;
}

The condition temp & HW_CFG_SRST_ is false so the error code -EIO is
returned and propagated to user-space.

By doing a git bisect we found that this strange behavior exists when
the Kconfig tristate compilation condition SMSC_PHY has the value yes
to be compiled built-in:

SMSC_PHY=y

If this config symbol has the value module (m) or not build at all,
then bringing up the network interface never fails.

Did anyone have the same problem?

The problem is really strange to me, especially since we are sure that
all the requirements to software reset the device are meet in both
cases (with SMSC_PHY compiled built-in and without it)

[1]: HEAD on commit 371de6e4e0042adf4f9b54c414154f57414ddd37 drm/i915:
Disable RC6 on Sandybridge by default
[2]: https://wiki.linaro.org/Boards/Igep

Thank you and best regards,

-- 
Javier Martínez Canillas
(+34) 682 39 81 69
Barcelona, Spain

Re: [BUG] SIOCSIFFLAGS returns -EIO on SMSC LAN911x

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2023 bytes --]

On Fri, 2011-12-30 at 10:44 +0100, Javier Martinez Canillas wrote:
> Hello,
> 
> We have an issue with the SMSC LAN911x driver with today linux-2.6 [1]
> executing on an IGEPv2 [2] board using omap2plus_defconfig.
> 
> When I try to bring up the network interface, the ioctl set interface
> flags command (SIOCSIFFLAGS)  fails returning -EIO:
[...]
> By doing a git bisect we found that this strange behavior exists when
> the Kconfig tristate compilation condition SMSC_PHY has the value yes
> to be compiled built-in:
> 
> SMSC_PHY=y
> 
> If this config symbol has the value module (m) or not build at all,
> then bringing up the network interface never fails.

So the PHY driver is doing something that interferes with soft reset.

The data sheet for this part
<http://www.smsc.com/media/Downloads_Public/Data_Sheets/9221.pdf> says
that soft reset does not work if the PHY is in certain states.  Also, it
doesn't seem to specify a maximum time for soft reset to complete,
though it does say that PHY reset takes 100 us.

> Did anyone have the same problem?
> 
> The problem is really strange to me, especially since we are sure that
> all the requirements to software reset the device are meet in both
> cases (with SMSC_PHY compiled built-in and without it)

I don't know about that.  smsc_phy_config_init() attempts to enable
power-saving on the PHY, but it is writing to a bit that is reserved
according to the data sheet for the combined MAC/PHY chip.  You might
want to try reverting:

commit 698244ace8b63896565022143ab19f141bc48993
Author: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Date:   Wed Jan 6 20:35:14 2010 -0800

    phy: SMSC device Energy Detect power-down mode

Ben.

> [1]: HEAD on commit 371de6e4e0042adf4f9b54c414154f57414ddd37 drm/i915:
> Disable RC6 on Sandybridge by default
> [2]: https://wiki.linaro.org/Boards/Igep
> 
> Thank you and best regards,
> 

-- 
Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [BUG] SIOCSIFFLAGS returns -EIO on SMSC LAN911x

[Javier Martinez Canillas]

On Fri, Dec 30, 2011 at 11:56 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Fri, 2011-12-30 at 10:44 +0100, Javier Martinez Canillas wrote:
>> Hello,
>>
>> We have an issue with the SMSC LAN911x driver with today linux-2.6 [1]
>> executing on an IGEPv2 [2] board using omap2plus_defconfig.
>>
>> When I try to bring up the network interface, the ioctl set interface
>> flags command (SIOCSIFFLAGS)  fails returning -EIO:
> [...]
>> By doing a git bisect we found that this strange behavior exists when
>> the Kconfig tristate compilation condition SMSC_PHY has the value yes
>> to be compiled built-in:
>>
>> SMSC_PHY=y
>>
>> If this config symbol has the value module (m) or not build at all,
>> then bringing up the network interface never fails.
>

Hello Ben, thanks for answering.

> So the PHY driver is doing something that interferes with soft reset.
>
> The data sheet for this part
> <http://www.smsc.com/media/Downloads_Public/Data_Sheets/9221.pdf> says
> that soft reset does not work if the PHY is in certain states.  Also, it
> doesn't seem to specify a maximum time for soft reset to complete,
> though it does say that PHY reset takes 100 us.
>

Yes, it seems that the PHY driver is the responsible of that.

Something I forgot to mention, the ethernet chip we are using is a
SMSC LAN 9221i.

>> Did anyone have the same problem?
>>
>> The problem is really strange to me, especially since we are sure that
>> all the requirements to software reset the device are meet in both
>> cases (with SMSC_PHY compiled built-in and without it)
>
> I don't know about that.  smsc_phy_config_init() attempts to enable
> power-saving on the PHY, but it is writing to a bit that is reserved
> according to the data sheet for the combined MAC/PHY chip.  You might
> want to try reverting:
>
> commit 698244ace8b63896565022143ab19f141bc48993
> Author: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Date:   Wed Jan 6 20:35:14 2010 -0800
>
>    phy: SMSC device Energy Detect power-down mode
>
> Ben.

Thanks a lot, reverting that patch solves the issue.

I've looked at that patch before but since we are using the smsc911x
and that patch made an exception for the lan911x phy_driver I thought
it wasn't the responsible for this:

+static int lan911x_config_init(struct phy_device *phydev)
+{
+       return smsc_phy_ack_interrupt(phydev);
+}

@@ -147,7 +164,7 @@ static struct phy_driver lan911x_int_driver = {
        /* basic functions */
        .config_aneg    = genphy_config_aneg,
        .read_status    = genphy_read_status,
-       .config_init    = smsc_phy_config_init,
+       .config_init    = lan911x_config_init,

But now I realized that the lan911x phy driver is not being used but
the lan8700_driver phy_driver, and the modified smsc_phy_config_init
was executed.

thanks a lot and best regards,

-- 
Javier Martínez Canillas
(+34) 682 39 81 69
Barcelona, Spain

Re: [BUG] SIOCSIFFLAGS returns -EIO on SMSC LAN911x

[Javier Martinez Canillas]

On Fri, Dec 30, 2011 at 11:56 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Fri, 2011-12-30 at 10:44 +0100, Javier Martinez Canillas wrote:
>> Hello,
>>
>> We have an issue with the SMSC LAN911x driver with today linux-2.6 [1]
>> executing on an IGEPv2 [2] board using omap2plus_defconfig.
>>
>> When I try to bring up the network interface, the ioctl set interface
>> flags command (SIOCSIFFLAGS)  fails returning -EIO:
> [...]
>> By doing a git bisect we found that this strange behavior exists when
>> the Kconfig tristate compilation condition SMSC_PHY has the value yes
>> to be compiled built-in:
>>
>> SMSC_PHY=y
>>
>> If this config symbol has the value module (m) or not build at all,
>> then bringing up the network interface never fails.
>
> So the PHY driver is doing something that interferes with soft reset.
>

We have more information about the issue. It seems that the LAN9221i
network device has inside a lan8700 phy chip. When we compile with
SMSC_PHY and the smsc.c driver is included the lan8700 phy_driver
enable the energy detect mode for the transceiver.

> The data sheet for this part
> <http://www.smsc.com/media/Downloads_Public/Data_Sheets/9221.pdf> says
> that soft reset does not work if the PHY is in certain states.  Also, it
> doesn't seem to specify a maximum time for soft reset to complete,
> though it does say that PHY reset takes 100 us.
>

One of the states that prevents software reseting the chip is when the
phy is in low power mode. So the reset fails in the ndo_open handler
(ifconfig ethx up).

>> Did anyone have the same problem?
>>
>> The problem is really strange to me, especially since we are sure that
>> all the requirements to software reset the device are meet in both
>> cases (with SMSC_PHY compiled built-in and without it)
>
> I don't know about that.  smsc_phy_config_init() attempts to enable
> power-saving on the PHY, but it is writing to a bit that is reserved
> according to the data sheet for the combined MAC/PHY chip.  dwein

Since the PHY chip is a lan8700, the power-saving seems to be correct.
But the problem is when latter in the open handler a software reset is
attempt.

Probably I can protect the software reset and only try to reset the
device if the PHY is now in a low power mode. But if we don't reset
the device at interface bringing up, when should we do a the reset?

I'm not that familiarized with the networking layer, if someone can
give me some light I can fix the issue and send a patch.

Thanks a lot and best regards,

-- 
Javier Martínez Canillas
(+34) 682 39 81 69
Barcelona, Spain

Re: [BUG] SIOCSIFFLAGS returns -EIO on SMSC LAN911x

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1816 bytes --]

On Fri, 2011-12-30 at 13:58 +0100, Javier Martinez Canillas wrote:
> On Fri, Dec 30, 2011 at 11:56 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > On Fri, 2011-12-30 at 10:44 +0100, Javier Martinez Canillas wrote:
[...]
> >> Did anyone have the same problem?
> >>
> >> The problem is really strange to me, especially since we are sure that
> >> all the requirements to software reset the device are meet in both
> >> cases (with SMSC_PHY compiled built-in and without it)
> >
> > I don't know about that.  smsc_phy_config_init() attempts to enable
> > power-saving on the PHY, but it is writing to a bit that is reserved
> > according to the data sheet for the combined MAC/PHY chip.  dwein
> 
> Since the PHY chip is a lan8700, the power-saving seems to be correct.

But the feature may not be supported in the combined chip, the 9221i.  I
would tend to believe the more specific data sheet.

> But the problem is when latter in the open handler a software reset is
> attempt.
> 
> Probably I can protect the software reset and only try to reset the
> device if the PHY is now in a low power mode. But if we don't reset
> the device at interface bringing up, when should we do a the reset?

You may have to change the order of initialisation of the PHY and MAC so
that reset works properly.

> I'm not that familiarized with the networking layer, if someone can
> give me some light I can fix the issue and send a patch.

This really doesn't have much to do with general networking, only phylib
and the specifics of the SMSC hardware and drivers.  The smsc911x driver
is responsible for calling phylib functions and so the hardware
initialisation is very much under its control.

Ben.

-- 
Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [BUG] SIOCSIFFLAGS returns -EIO on SMSC LAN911x

[Javier Martinez Canillas]

On Fri, Dec 30, 2011 at 2:15 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Fri, 2011-12-30 at 13:58 +0100, Javier Martinez Canillas wrote:
>> On Fri, Dec 30, 2011 at 11:56 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
>> > On Fri, 2011-12-30 at 10:44 +0100, Javier Martinez Canillas wrote:
> [...]
>> >> Did anyone have the same problem?
>> >>
>> >> The problem is really strange to me, especially since we are sure that
>> >> all the requirements to software reset the device are meet in both
>> >> cases (with SMSC_PHY compiled built-in and without it)
>> >
>> > I don't know about that.  smsc_phy_config_init() attempts to enable
>> > power-saving on the PHY, but it is writing to a bit that is reserved
>> > according to the data sheet for the combined MAC/PHY chip.  dwein
>>
>> Since the PHY chip is a lan8700, the power-saving seems to be correct.
>
> But the feature may not be supported in the combined chip, the 9221i.  I
> would tend to believe the more specific data sheet.
>
>> But the problem is when latter in the open handler a software reset is
>> attempt.
>>
>> Probably I can protect the software reset and only try to reset the
>> device if the PHY is now in a low power mode. But if we don't reset
>> the device at interface bringing up, when should we do a the reset?
>
> You may have to change the order of initialisation of the PHY and MAC so
> that reset works properly.
>
>> I'm not that familiarized with the networking layer, if someone can
>> give me some light I can fix the issue and send a patch.
>
> This really doesn't have much to do with general networking, only phylib
> and the specifics of the SMSC hardware and drivers.  The smsc911x driver
> is responsible for calling phylib functions and so the hardware
> initialisation is very much under its control.
>
> Ben.

Hello Ben,

Thanks for your help.

Finally, what I did was to bring to operational mode the integrated
PHY chip by disabling the energy detect power-down mode before doing a
software reset and re-enabling after it.

Does that solution make sense to you? I've sent the patch-set to the
list for review. The patch-set is composed of the following patches:

Javier Martinez Canillas (2):
      net: phy: smsc: Move SMSC PHY constants to <linux/smscphy.h>
      net/smsc911x: Check if PHY is in operational mode before software reset

Thanks a lot and best regards,

-- 
Javier Martínez Canillas
(+34) 682 39 81 69
Barcelona, Spain

Re: [BUG] SIOCSIFFLAGS returns -EIO on SMSC LAN911x

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

On Mon, 2012-01-02 at 19:37 +0100, Javier Martinez Canillas wrote:
[...]
> Thanks for your help.
> 
> Finally, what I did was to bring to operational mode the integrated
> PHY chip by disabling the energy detect power-down mode before doing a
> software reset and re-enabling after it.
> 
> Does that solution make sense to you? I've sent the patch-set to the
> list for review. The patch-set is composed of the following patches:
[...]

I think that if the data sheet says the power-down bit is reserved on
the 9221i then it is better not to set it at all.  However, given that
you actually have hardware to test on, you are in a better position to
say what actually works.

Ben.

-- 
Ben Hutchings
All the simple programs have been written, and all the good names taken.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [BUG] SIOCSIFFLAGS returns -EIO on SMSC LAN911x

[Javier Martinez Canillas]

On Mon, Jan 2, 2012 at 8:45 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Mon, 2012-01-02 at 19:37 +0100, Javier Martinez Canillas wrote:
> [...]
>> Thanks for your help.
>>
>> Finally, what I did was to bring to operational mode the integrated
>> PHY chip by disabling the energy detect power-down mode before doing a
>> software reset and re-enabling after it.
>>
>> Does that solution make sense to you? I've sent the patch-set to the
>> list for review. The patch-set is composed of the following patches:
> [...]
>
> I think that if the data sheet says the power-down bit is reserved on
> the 9221i then it is better not to set it at all.  However, given that
> you actually have hardware to test on, you are in a better position to
> say what actually works.
>
> Ben.
>

Hello Ben,

I didn't find on the LAN9221/LAN9221i data-sheet where it says that
this bit 0x17.13 is reserved.

In fact if you look at Section 5.5 "PHY Registers" (page 117) you will
see that the PHY register 0x17 "Mode Control/Status Register" (page
122) is a valid PHY register address and can be used to enable and
disable the Energy Detect Power-Down mode by writing to the bit-field
13 (EDPWRDOWN). Also a functional description of this behavior is
explanied in Section "3.10.3.2 Energy Detect Power-Down" (page 47).

Since in drivers/net/phy/smsc.c the phy is initialized with:

#define MII_LAN83C185_CTRL_STATUS 17 /* Mode/Status Register */
#define MII_LAN83C185_EDPWRDOWN (1 << 13) /* EDPWRDOWN */

static int smsc_phy_config_init(struct phy_device *phydev)
{
        int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
        if (rc < 0)
                return rc;

        /* Enable energy detect mode for this SMSC Transceivers */
        rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS,
                       rc | MII_LAN83C185_EDPWRDOWN);
        if (rc < 0)
                return rc;

        return smsc_phy_ack_interrupt (phydev);
}

I think that the enabling of the Energy Detect Power-Down mode is correct.

On section 5.3.9 "HW_CFG—Hardware Configuration Register" it says that:

"Soft Reset Timeout (SRST_TO). If a software reset is attempted when
the PHY is not in the operational state (RX_CLK and TX_CLK running),
the reset will not
complete and the soft reset operation will timeout and this bit will
be set to a ‘1’."

Is exactly the behavior we have when the PHY is in low power mode
because the energy detect power-down mode is enabled and no energy is
detected.

That's why I thought to use the bit-field EDPWRDOWN to disable the
energy detect power-down mode before making the software reset. This
brings the PHY to operational mode so the software reset never fails.
After the software reset was successful we can re-enable the energy
detect power-down mode to take advantage of the automatic energy
detection supported by the device.

I have tested on the hardware and seems to work properly, but I don't
know if is the best approach.

Best regards,

-- 
Javier Martínez Canillas
(+34) 682 39 81 69
Barcelona, Spain