[PATCH net-next] net: phy: fix two issues with linkmode bitmaps

[PATCH net-next] net: phy: fix two issues with linkmode bitmaps

[Heiner Kallweit]

I wondered why ethtool suddenly reports that link partner doesn't
support aneg and GBit modes. It turned out that this is caused by two
bugs in conversion to linkmode bitmaps.

1. In genphy_read_status the value of phydev->lp_advertising is
   overwritten, thus GBit modes aren't reported any longer.
2. In mii_lpa_to_linkmode_lpa_t the aneg bit was overwritten by the
   call to mii_adv_to_linkmode_adv_t.

Fixes: c0ec3c273677 ("net: phy: Convert u32 phydev->lp_advertising to linkmode")
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy_device.c | 5 ++++-
 include/linux/mii.h          | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0904002b1..94f60c08b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1696,6 +1696,7 @@ int genphy_read_status(struct phy_device *phydev)
 	int lpagb = 0;
 	int common_adv;
 	int common_adv_gb = 0;
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(lpa_tmp);
 
 	/* Update the link, but return if there was an error */
 	err = genphy_update_link(phydev);
@@ -1734,7 +1735,9 @@ int genphy_read_status(struct phy_device *phydev)
 		if (lpa < 0)
 			return lpa;
 
-		mii_lpa_to_linkmode_lpa_t(phydev->lp_advertising, lpa);
+		mii_lpa_to_linkmode_lpa_t(lpa_tmp, lpa);
+		linkmode_or(phydev->lp_advertising, phydev->lp_advertising,
+			    lpa_tmp);
 
 		adv = phy_read(phydev, MII_ADVERTISE);
 		if (adv < 0)
diff --git a/include/linux/mii.h b/include/linux/mii.h
index fb7ae4ae8..08450609d 100644
--- a/include/linux/mii.h
+++ b/include/linux/mii.h
@@ -413,11 +413,11 @@ static inline void mii_adv_to_linkmode_adv_t(unsigned long *advertising,
 static inline void mii_lpa_to_linkmode_lpa_t(unsigned long *lp_advertising,
 					     u32 lpa)
 {
+	mii_adv_to_linkmode_adv_t(lp_advertising, lpa);
+
 	if (lpa & LPA_LPACK)
 		linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
 				 lp_advertising);
-
-	mii_adv_to_linkmode_adv_t(lp_advertising, lpa);
 }
 
 /**
-- 
2.19.2

Re: [PATCH net-next] net: phy: fix two issues with linkmode bitmaps

[Andrew Lunn]

On Sun, Nov 25, 2018 at 03:23:42PM +0100, Heiner Kallweit wrote:
> I wondered why ethtool suddenly reports that link partner doesn't
> support aneg and GBit modes. It turned out that this is caused by two
> bugs in conversion to linkmode bitmaps.
> 
> 1. In genphy_read_status the value of phydev->lp_advertising is
>    overwritten, thus GBit modes aren't reported any longer.
> 2. In mii_lpa_to_linkmode_lpa_t the aneg bit was overwritten by the
>    call to mii_adv_to_linkmode_adv_t.

Hi Heiner

Thanks for looking into this.

There are more bugs :-(

static inline void mii_lpa_to_linkmode_lpa_t(unsigned long *lp_advertising,
                                             u32 lpa)
{
        if (lpa & LPA_LPACK)
                linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
                                 lp_advertising);

        mii_adv_to_linkmode_adv_t(lp_advertising, lpa);
}

But

static inline void mii_adv_to_linkmode_adv_t(unsigned long *advertising,
                                             u32 adv)
{
        linkmode_zero(advertising);

        if (adv & ADVERTISE_10HALF)
                linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
                                 advertising);
 
So the Autoneg_BIT gets cleared.

I think the better fix is to take the linkmode_zero() out from here.

Then:

        if (adv & ADVERTISE_10HALF)
               linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
                                advertising);
+	else
+              linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
+                                 advertising);

for all the bits mii_adv_to_linkmode_adv_t() looks at.

So mii_adv_to_linkmode_adv_t() only modifies bits it is responsible
for, and leaves the others alone.

    Andrew

Re: [PATCH net-next] net: phy: fix two issues with linkmode bitmaps

[Heiner Kallweit]

On 25.11.2018 17:45, Andrew Lunn wrote:
> On Sun, Nov 25, 2018 at 03:23:42PM +0100, Heiner Kallweit wrote:
>> I wondered why ethtool suddenly reports that link partner doesn't
>> support aneg and GBit modes. It turned out that this is caused by two
>> bugs in conversion to linkmode bitmaps.
>>
>> 1. In genphy_read_status the value of phydev->lp_advertising is
>>    overwritten, thus GBit modes aren't reported any longer.
>> 2. In mii_lpa_to_linkmode_lpa_t the aneg bit was overwritten by the
>>    call to mii_adv_to_linkmode_adv_t.
> 
> Hi Heiner
> 
> Thanks for looking into this.
> 
> There are more bugs :-(
> 
> static inline void mii_lpa_to_linkmode_lpa_t(unsigned long *lp_advertising,
>                                              u32 lpa)
> {
>         if (lpa & LPA_LPACK)
>                 linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
>                                  lp_advertising);
> 
>         mii_adv_to_linkmode_adv_t(lp_advertising, lpa);
> }
> 
> But
> 
> static inline void mii_adv_to_linkmode_adv_t(unsigned long *advertising,
>                                              u32 adv)
> {
>         linkmode_zero(advertising);
> 
>         if (adv & ADVERTISE_10HALF)
>                 linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
>                                  advertising);
>  
> So the Autoneg_BIT gets cleared.
> 
> I think the better fix is to take the linkmode_zero() out from here.
> 
> Then:
> 
>         if (adv & ADVERTISE_10HALF)
>                linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
>                                 advertising);
> +	else
> +              linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
> +                                 advertising);
> 
> for all the bits mii_adv_to_linkmode_adv_t() looks at.
> 
> So mii_adv_to_linkmode_adv_t() only modifies bits it is responsible
> for, and leaves the others alone.
> 
>     Andrew
> 

mii_adv_to_linkmode_adv_t() is used also in phy_mii_ioctl(), and I'm
not sure the proposed change is safe there.

Eventually we'd have three types of mii_xxx_to_linkmode_yyy functions:

1. Function first zeroes the destination linkmode bitmap
2. Function sets bits in the linkmode bitmap but doesn't clear bits
   if condition isn't met
3. Function clears / sets bits it's responsible for

example case 1: mmd_eee_adv_to_linkmode
example case 2: mii_stat1000_to_linkmode_lpa_t
example case 3: what you just proposed as fix for
                mii_adv_to_linkmode_adv_t

Because function naming is the same I'm afraid they easily can be used
incorrectly (the bugs we just discuss are good examples). Maybe it
could be an option to reflect the semantics in the name like this
(better suited proposals welcome):

case 1: mii_xxx_to_linkmode_yyy
case 2: mii_xxx_or_linkmode_yyy
case 3: mii_xxx_mod_linkmode_yyy

Heiner

Re: [PATCH net-next] net: phy: fix two issues with linkmode bitmaps

[Andrew Lunn]

> Eventually we'd have three types of mii_xxx_to_linkmode_yyy functions:
> 
> 1. Function first zeroes the destination linkmode bitmap
> 2. Function sets bits in the linkmode bitmap but doesn't clear bits
>    if condition isn't met
> 3. Function clears / sets bits it's responsible for
> 
> example case 1: mmd_eee_adv_to_linkmode
> example case 2: mii_stat1000_to_linkmode_lpa_t
> example case 3: what you just proposed as fix for
>                 mii_adv_to_linkmode_adv_t
> 
> Because function naming is the same I'm afraid they easily can be used
> incorrectly (the bugs we just discuss are good examples). Maybe it
> could be an option to reflect the semantics in the name like this
> (better suited proposals welcome):
> 
> case 1: mii_xxx_to_linkmode_yyy
> case 2: mii_xxx_or_linkmode_yyy
> case 3: mii_xxx_mod_linkmode_yyy

Hi Heiner

That is a good idea. We should probably do this first, it will help
find the bugs.

     Andrew

Re: [PATCH net-next] net: phy: fix two issues with linkmode bitmaps

[Andrew Lunn]

> Because function naming is the same I'm afraid they easily can be used
> incorrectly (the bugs we just discuss are good examples). Maybe it
> could be an option to reflect the semantics in the name like this
> (better suited proposals welcome):
> 
> case 1: mii_xxx_to_linkmode_yyy
> case 2: mii_xxx_or_linkmode_yyy
> case 3: mii_xxx_mod_linkmode_yyy
 
Hi Heiner

I started a patchset using this idea, and reworks your fix. Lets work
on that, rather than merge this patch.

   Andrew