diff mbox

[net-next] net: phy: marvell: Show complete link partner advertising

Message ID 20170612133237.E20945085BC@solo.franken.de
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Thomas Bogendoerfer June 12, 2017, 12:54 p.m. UTC
From: Thomas Bogendoerfer <tbogendoerfer@suse.de>

Give back all modes advertised by the link partner. This change brings
the marvell phy driver in line with all other phy drivers.

Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
---
 drivers/net/phy/marvell.c | 7 -------
 1 file changed, 7 deletions(-)

Comments

Florian Fainelli June 12, 2017, 4:05 p.m. UTC | #1
On 06/12/2017 05:54 AM, Thomas Bogendoerfer wrote:
> From: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> 
> Give back all modes advertised by the link partner. This change brings
> the marvell phy driver in line with all other phy drivers.
> 
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>

I thought Russell had a similar patch but I can't find it applied in
net-next, so:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

drivers/net/phy/lxt.c has a similar pattern that would be worth fixing too.

> ---
>  drivers/net/phy/marvell.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 4c5246fed69b..8400403b3f62 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -1139,7 +1139,6 @@ static int marvell_read_status_page_an(struct phy_device *phydev,
>  	int status;
>  	int lpa;
>  	int lpagb;
> -	int adv;
>  
>  	status = phy_read(phydev, MII_M1011_PHY_STATUS);
>  	if (status < 0)
> @@ -1153,12 +1152,6 @@ static int marvell_read_status_page_an(struct phy_device *phydev,
>  	if (lpagb < 0)
>  		return lpagb;
>  
> -	adv = phy_read(phydev, MII_ADVERTISE);
> -	if (adv < 0)
> -		return adv;
> -
> -	lpa &= adv;
> -
>  	if (status & MII_M1011_PHY_STATUS_FULLDUPLEX)
>  		phydev->duplex = DUPLEX_FULL;
>  	else
>
David Miller June 12, 2017, 4:08 p.m. UTC | #2
From: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Date: Mon, 12 Jun 2017 14:54:57 +0200

> From: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> 
> Give back all modes advertised by the link partner. This change brings
> the marvell phy driver in line with all other phy drivers.
> 
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>

Applied, thanks.
Russell King (Oracle) June 12, 2017, 4:10 p.m. UTC | #3
On Mon, Jun 12, 2017 at 09:05:04AM -0700, Florian Fainelli wrote:
> On 06/12/2017 05:54 AM, Thomas Bogendoerfer wrote:
> > From: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> > 
> > Give back all modes advertised by the link partner. This change brings
> > the marvell phy driver in line with all other phy drivers.
> > 
> > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> 
> I thought Russell had a similar patch but I can't find it applied in
> net-next, so:

I do, it has a subject line of "net: phy: fix marvell phy status reading"
and it's already been sent (first link).  DaveM said he applied it
(second link):

https://www.mail-archive.com/netdev@vger.kernel.org/msg170743.html
https://www.mail-archive.com/netdev@vger.kernel.org/msg171036.html

However, Thomas' patch removes slightly more code (I didn't spot that
"adv" is no longer used and we don't need to read the MII_ADVERTISE
register anymore.)
Thomas Bogendoerfer June 13, 2017, 8:32 a.m. UTC | #4
On Mon, Jun 12, 2017 at 09:05:04AM -0700, Florian Fainelli wrote:
> On 06/12/2017 05:54 AM, Thomas Bogendoerfer wrote:
> > From: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> > 
> > Give back all modes advertised by the link partner. This change brings
> > the marvell phy driver in line with all other phy drivers.
> > 
> > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> 
> I thought Russell had a similar patch but I can't find it applied in
> net-next, so:
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> drivers/net/phy/lxt.c has a similar pattern that would be worth fixing too.

that's different and correct. The lpa value is not exported as lp_advertising,
but only used internal. Well the bug here is IMHO, that it doesn't
export lpa to lp_advertising at all as it's done in genphy_read_status().
And from a quick grep there are more phy drivers doing that...
I'll have a look later.

Thomas.
diff mbox

Patch

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 4c5246fed69b..8400403b3f62 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -1139,7 +1139,6 @@  static int marvell_read_status_page_an(struct phy_device *phydev,
 	int status;
 	int lpa;
 	int lpagb;
-	int adv;
 
 	status = phy_read(phydev, MII_M1011_PHY_STATUS);
 	if (status < 0)
@@ -1153,12 +1152,6 @@  static int marvell_read_status_page_an(struct phy_device *phydev,
 	if (lpagb < 0)
 		return lpagb;
 
-	adv = phy_read(phydev, MII_ADVERTISE);
-	if (adv < 0)
-		return adv;
-
-	lpa &= adv;
-
 	if (status & MII_M1011_PHY_STATUS_FULLDUPLEX)
 		phydev->duplex = DUPLEX_FULL;
 	else