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: <87ikrt98d2.fsf@waldekranz.com>
Date: Sun, 08 Dec 2024 22:23:53 +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 lör, dec 07, 2024 at 16:38, Andrew Lunn <andrew@...n.ch> wrote:
> On Fri, Dec 06, 2024 at 02:39:25PM +0100, Tobias Waldekranz wrote:
>> 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?
>
> Ah, the previous patch.
>
> I wounder if the simpler fix is just to increase the timeout? I don't

It would certainly be simpler. To me, it just felt a bit dangerous to
have a static 1s timeout buried that deep in the stack.

> think we have any code specifically wanting a timeout, so changing the
> timeout should have no real effect.

I imagine some teardown scenario, in which we typically ignore return
values. In that case, if we're trying to remove lots of objects from
hardware that require waiting on busy bits (ATU/VTU), we could end up
blocking for minutes rather than seconds.

But it is definitely more of a gut feeling - I don't have a concrete
example.