netdev - Re: [PATCH net-next v3] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X

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]

Date: Wed, 26 Jul 2023 11:50:40 +0200
From: Ante Knezic <ante.knezic@...mholz.de>
To: <olteanv@...il.com>
CC: <andrew@...n.ch>, <ante.knezic@...mholz.de>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <f.fainelli@...il.com>, <kuba@...nel.org>,
	<linux-kernel@...r.kernel.org>, <linux@...linux.org.uk>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>
Subject: Re: [PATCH net-next v3] net: dsa: mv88e6xxx: Add erratum 3.14 for 88E6390X and 88E6190X

On Tue, 25 Jul 2023 20:23:43 +0300 Vladimir Oltean wrote:
> I'm not sure which way is preferred by PHY maintainers, but it seems to
> be a useless complication to simulate that you have a struct mdio_device
> for the other lanes when you don't. It appears more appropriate to just
> use mdiobus_c45_write(mpcs->mdio.bus, lanes[i]).
> 

Agreed.

> There's also the locking question (with the big caveat that we don't
> know what the register writes do!). There's locking at the bus level,
> but the MDIO device isn't locked. So phylink on those other PCSes can
> still do stuff, even in-between the first and the second write to
> undocumented register 0xf054.
> 
> I can speculate that writing 0x400c -> 0x4000 is something like: set
> RX_RESET | TX_RESET followed by clear RX_RESET | TX_RESET. Is it ok if
> stuff happens in between these writes - will it stick, or does this
> logically interact with anything else in any other way? I guess we won't
> know. I might be a bit closer to being okay with it if you could confirm
> that some other (unrelated) register write to the PCS does make it
> through (and can be read back) in between the 2 erratum writes.

I was able to confirm this by successfully reading and writing to the 
SGMII_BMCR register between erratum writes. This did not affect the issue
that erratum fixes. Unfortunatelly, there is no info about what the
actuall writing to magic registers does.

>>  static int mv88e639x_sgmii_pcs_post_config(struct phylink_pcs *pcs,
>>  	                                   phy_interface_t interface)
>>  {
>>  	struct mv88e639x_pcs *mpcs = sgmii_pcs_to_mv88e639x_pcs(pcs);
>> +	struct mv88e6xxx_chip *chip = mpcs->chip;
>>  
>>  	mv88e639x_sgmii_pcs_control_pwr(mpcs, true);
>>  
>> +	if (chip->info->prod_num == MV88E6XXX_PORT_SWITCH_ID_PROD_6190X ||
>> +	    chip->info->prod_num == MV88E6XXX_PORT_SWITCH_ID_PROD_6390X)
>> +	        mv88e6390_erratum_3_14(mpcs);
>
>You could at least print an error if a write failure occurred, so that
>it doesn't go completely unnoticed.

Ok, I was simply following the above notion (we don't check or print 
errors when powering on the serdes lane) but I agree with your point and 
will adapt the patch for the next version.