netdev - Re: [PATCH] lan78xx: relocate mdix setting to phy driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c894a7bd-4062-cb63-6b70-02bb86c874c1@gmail.com>
Date:   Tue, 15 Nov 2016 15:47:25 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Woojung.Huh@...rochip.com, davem@...emloft.net
Cc:     andrew@...n.ch, netdev@...r.kernel.org,
        UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH] lan78xx: relocate mdix setting to phy driver

On 11/15/2016 01:57 PM, Woojung.Huh@...rochip.com wrote:
> From: Woojung Huh <woojung.huh@...rochip.com>
> 
> Relocate mdix code to phy driver to be called at config_init().
> 
> Signed-off-by: Woojung Huh <woojung.huh@...rochip.com>
> ---
>  drivers/net/phy/microchip.c | 43 +++++++++++++++++++++++++-
>  drivers/net/usb/lan78xx.c   | 73 ++-------------------------------------------
>  2 files changed, 45 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/net/phy/microchip.c b/drivers/net/phy/microchip.c
> index 7c00e50..13553df 100644
> --- a/drivers/net/phy/microchip.c
> +++ b/drivers/net/phy/microchip.c
> @@ -106,6 +106,47 @@ static int lan88xx_set_wol(struct phy_device *phydev,
>  	return 0;
>  }
>  
> +static void lan88xx_set_mdix(struct phy_device *phydev)
> +{
> +	int buf;
> +
> +	if (phydev->mdix == ETH_TP_MDI) {
> +		phy_write(phydev, LAN88XX_EXT_PAGE_ACCESS,
> +			  LAN88XX_EXT_PAGE_SPACE_1);

Should you take this write out of the if/else

> +		buf = phy_read(phydev, LAN88XX_EXT_MODE_CTRL);

And this one too

> +		buf &= ~LAN88XX_EXT_MODE_CTRL_MDIX_MASK_;
> +		phy_write(phydev, LAN88XX_EXT_MODE_CTRL,
> +			  buf | LAN88XX_EXT_MODE_CTRL_MDI_);
> +		phy_write(phydev, LAN88XX_EXT_PAGE_ACCESS,
> +			  LAN88XX_EXT_PAGE_SPACE_0);

And this one too as well

> +	} else if (phydev->mdix == ETH_TP_MDI_X) {

switch/case statement would be more appropriate and it would be easier
to add support for future possible modes.

And once you take the write/read/write out of the switch case, it's just
a matter of translating phydev->mdix into the appropriate mask value to
write.

So essentially, this comes down to:

static void lan88xx_set_mdix(struct phy_device *phydev)
{
	int buf;
	int mask_val;

	switch (phydev->mdix) {
	case ETH_TP_MDI:
		mask_val = LAN88XX_EXT_MODE_CTRL_MDI_;
		break;
	case ETH_TP_MDI_X:
		mask_val = LAN88XX_EXT_MODE_CTRL_MDI_X_;
		break;
	case ETH_TP_MDI_AUTO:
		mask_val = LAN88XX_EXT_MODE_CTRL_AUTO_MDIX_:
		break:
	default:
		return;
	}

	phy_write(phydev, LAN88XX_EXT_PAGE_ACCESS,
		  LAN88XX_EXT_PAGE_SPACE_1);
	buf = phy_read(phydev, LAN88XX_EXT_MODE_CTRL);
	buf &= ~LAN88XX_EXT_MODE_CTRL_MDIX_MASK_;
	buf |= mask_val;
	phy_write(phydev, LAN88XX_EXT_MODE_CTRL, buf);
	phy_write(phydev, LAN88XX_EXT_PAGE_ACCESS,
		  LAN88XX_EXT_PAGE_SPACE_0);
}

I leave it up to you whether you need to do this now, or as a subsequent
clean up patch.
-- 
Florian