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: <878qsg8rrc.fsf@waldekranz.com>
Date: Mon, 16 Dec 2024 00:12:55 +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 sön, dec 08, 2024 at 22:23, Tobias Waldekranz <tobias@...dekranz.com> wrote:
> 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.

Andrew, have you had a chance to mull this over?

If you want to go with a global timeout then let's do that, but either
way I would really like to move this series forward.