netdev - Re: [PATCH net 2/4] net: dsa: mv88e6xxx: Give chips more time to activate their PPUs

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: <87ldwt7wxe.fsf@waldekranz.com>
Date: Fri, 06 Dec 2024 14:39:25 +0100
From: Tobias Waldekranz <tobias@...dekranz.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: davem@...emloft.net, kuba@...nel.org, f.fainelli@...il.com,
 olteanv@...il.com, netdev@...r.kernel.org, linux@...linux.org.uk,
 chris.packham@...iedtelesis.co.nz
Subject: Re: [PATCH net 2/4] net: dsa: mv88e6xxx: Give chips more time to
 activate their PPUs

On fre, dec 06, 2024 at 14:18, Andrew Lunn <andrew@...n.ch> wrote:
> On Fri, Dec 06, 2024 at 02:07:34PM +0100, Tobias Waldekranz wrote:
>> In a daisy-chain of three 6393X devices, delays of up to 750ms are
>> sometimes observed before completion of PPU initialization (Global 1,
>> register 0, bit 15) is signaled. Therefore, allow chips more time
>> before giving up.
>>  static int mv88e6352_g1_wait_ppu_polling(struct mv88e6xxx_chip *chip)
>>  {
>>  	int bit = __bf_shf(MV88E6352_G1_STS_PPU_STATE);
>> +	int err, i;
>>  
>> -	return mv88e6xxx_g1_wait_bit(chip, MV88E6XXX_G1_STS, bit, 1);
>> +	for (i = 0; i < 20; i++) {
>> +		err = _mv88e6xxx_wait_bit(chip, chip->info->global1_addr,
>> +					  MV88E6XXX_G1_STS, bit, 1, NULL);
>> +		if (err != -ETIMEDOUT)
>> +			break;
>> +	}
>
> The commit message does not indicate why it is necessary to swap to
> _mv88e6xxx_wait_bit().

It is not strictly necessary, I just wanted to avoid flooding the logs
with spurious timeout errors. Do you want me to update the message?

>> +
>> +	if (err) {
>> +		dev_err(chip->dev, "PPU did not come online: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	if (i)
>> +		dev_warn(chip->dev,
>> +			 "PPU was slow to come online, retried %d times\n", i);
>
> dev_dbg()? Does the user care if it took longer than one loop
> iteration?

My resoning was: While it does seem fine that the device takes this long
to initialize, if it turns out that this is an indication of some bigger
issue, it might be good to have it recorded in the log.