netdev - Re: [PATCH net-next v7 5/5] net: phy: add Rust Asix PHY driver

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20231121.151939.1903605088782465261.fujita.tomonori@gmail.com>
Date: Tue, 21 Nov 2023 15:19:39 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: andrew@...n.ch
Cc: fujita.tomonori@...il.com, aliceryhl@...gle.com,
 benno.lossin@...ton.me, miguel.ojeda.sandonis@...il.com,
 netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, tmgross@...ch.edu,
 wedsonaf@...il.com
Subject: Re: [PATCH net-next v7 5/5] net: phy: add Rust Asix PHY driver

On Sun, 19 Nov 2023 17:03:30 +0100
Andrew Lunn <andrew@...n.ch> wrote:

>> >> +            let _ = dev.init_hw();
>> >> +            let _ = dev.start_aneg();
>> > 
>> > Just to confirm: You want to call `start_aneg` even if `init_hw` returns
>> > failure? And you want to ignore both errors?
>> 
>> Yeah, I tried to implement the exact same behavior in the original C driver.
> 
> You probably could check the return values, and it would not make a
> difference. Also, link_change_notify() is a void function, so you
> cannot return the error anyway.
> 
> These low level functions basically only fail if the hardware is
> `dead`. You get an -EIO or maybe -TIMEDOUT back. And there is no real
> recovery. You tend to get such errors during probe and fail the
> probe. Or maybe if power management is wrong and it has turned a
> critical clock off. But that is unlikely in this case, we are calling
> link_change_notify because the PHY has told us something changed
> recently, so it probably is alive.
> 
> I would say part of not checking the return code is also that C does
> not have the nice feature that Rust has of making very simple to check
> the return code. That combined with it being mostly pointless for PHY
> drivers.

Understood. I'll check the first return value if you prefer. I might
add WARN_ON_ONCE after Rust supports it.

diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs
index ce6640ff523f..7522070a6acd 100644
--- a/drivers/net/phy/ax88796b_rust.rs
+++ b/drivers/net/phy/ax88796b_rust.rs
@@ -95,7 +95,9 @@ fn link_change_notify(dev: &mut phy::Device) {
         // Reset PHY, otherwise MII_LPA will provide outdated information.
         // This issue is reproducible only with some link partner PHYs.
         if dev.state() == phy::DeviceState::NoLink {
-            let _ = dev.init_hw();
+            if let Err(_) = dev.init_hw() {
+                return;
+            }
             let _ = dev.start_aneg();
         }
     }