netdev - Re: [PATCH net] net: phy: marvell10g: limit soft reset to 88x3310

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <87tv1db0mz.fsf@tarshish>
Date:   Tue, 21 Apr 2020 13:20:52 +0300
From:   Baruch Siach <baruch@...s.co.il>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [PATCH net] net: phy: marvell10g: limit soft reset to 88x3310

Hi Russell,

On Tue, Apr 21 2020, Russell King - ARM Linux admin wrote:
> On Tue, Apr 21, 2020 at 12:04:46PM +0300, Baruch Siach wrote:
>> The MV_V2_PORT_CTRL_SWRST bit in MV_V2_PORT_CTRL is reserved on 88E2110.
>> Setting SWRST on 88E2110 breaks packets transfer after interface down/up
>> cycle.
>>
>> Fixes: 8f48c2ac85ed ("net: marvell10g: soft-reset the PHY when coming out of low power")
>> Signed-off-by: Baruch Siach <baruch@...s.co.il>
>
> Okay, the presence of 88E2110 combined with 88X3310 support is going to
> be a constant source of pain in terms of maintanence, since I know
> nothing about this PHY, nor do I have any way to test my changes there.
>
> I think we need to think about how to deal with that - do we split the
> code, so that 88X3310 can be maintained separately from 88E2110 (even
> though most of the code may be the same), or can someone send me a board
> that has the 88E2110 on (I can't purchase as I have no funds to do so.)

I'll contact you in private about hardware availability.

> So, I guess splitting the code is likely to be the only solution.

This situation is no different than other drivers that support many
variants of the same basic hardware. FEC and mvneta driver come to mind
as examples. Hardware availability limitation is always a challenge.

I don't think this justifies splitting the code. But that's your call.

Thanks for reviewing,
baruch

>> ---
>>  drivers/net/phy/marvell10g.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
>> index d3cb88651ad2..601686f64341 100644
>> --- a/drivers/net/phy/marvell10g.c
>> +++ b/drivers/net/phy/marvell10g.c
>> @@ -263,7 +263,8 @@ static int mv3310_power_up(struct phy_device *phydev)
>>  	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
>>  				 MV_V2_PORT_CTRL_PWRDOWN);
>>
>> -	if (priv->firmware_ver < 0x00030000)
>> +	if (phydev->drv->phy_id != MARVELL_PHY_ID_88X3310 ||
>> +	    priv->firmware_ver < 0x00030000)
>>  		return ret;
>>
>>  	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,

--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@...s.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -