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

FUJITA Tomonori
2023-11-21T06:19:41Z

On Sun, 19 Nov 2023 17:03:30 +0100
Andrew Lunn <andrew@lunn.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();
}
}