netdev - Re: [PATCH net-next v1 2/2] net: phy: qt2025: wait until PHY becomes ready

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: <c8ba40d3-0a18-4fb4-9ca3-d6cee6872712@lunn.ch>
Date: Tue, 1 Oct 2024 14:48:06 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org,
	rust-for-linux@...r.kernel.org, hkallweit1@...il.com,
	tmgross@...ch.edu, ojeda@...nel.org, alex.gaynor@...il.com,
	gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	a.hindborg@...sung.com
Subject: Re: [PATCH net-next v1 2/2] net: phy: qt2025: wait until PHY becomes
 ready

On Tue, Oct 01, 2024 at 01:36:41PM +0200, Alice Ryhl wrote:
> On Tue, Oct 1, 2024 at 1:27 PM FUJITA Tomonori
> <fujita.tomonori@...il.com> wrote:
> >
> > Wait until a PHY becomes ready in the probe callback by using a sleep
> > function.
> >
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
> > ---
> >  drivers/net/phy/qt2025.rs | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs
> > index 28d8981f410b..3a8ef9f73642 100644
> > --- a/drivers/net/phy/qt2025.rs
> > +++ b/drivers/net/phy/qt2025.rs
> > @@ -93,8 +93,15 @@ fn probe(dev: &mut phy::Device) -> Result<()> {
> >          // The micro-controller will start running from SRAM.
> >          dev.write(C45::new(Mmd::PCS, 0xe854), 0x0040)?;
> >
> > -        // TODO: sleep here until the hw becomes ready.
> > -        Ok(())
> > +        // sleep here until the hw becomes ready.
> > +        for _ in 0..60 {
> > +            kernel::delay::sleep(core::time::Duration::from_millis(50));
> > +            let val = dev.read(C45::new(Mmd::PCS, 0xd7fd))?;
> > +            if val != 0x00 && val != 0x10 {
> > +                return Ok(());
> > +            }
> 
> Why not place the sleep after this check? That way, we don't need to
> sleep if the check succeeds immediately.

Nice, you just made my point :-)

I generally point developers at iopoll.h, because developers nearly
always get this sort of polling for something to happen wrong. 

The kernel sleep functions guarantee the minimum sleep time. They say
nothing about the maximum sleep time. You can ask it to sleep for 1ms,
and in reality, due to something stealing the CPU and not being RT
friendly, it actually sleeps for 10ms. This extra long sleep time
blows straight past your timeout, if you have a time based timeout.
What most developers do is after the sleep() returns they check to see
if the timeout has been reached and then exit with -ETIMEDOUT. They
don't check the state of the hardware, which given its had a long time
to do its thing, probably is now in a good state. But the function
returns -ETIMEDOUT.

There should always be a check of the hardware state after the sleep,
in order to determine ETIMEDOUT vs 0.

As i said, most C developers get this wrong. So i don't really see why
Rust developers also will not get this wrong. So i like to discourage
this sort of code, and have Rust implementations of iopoll.h.

	Andrew