netdev - Re: [PATCH net-next v3 3/3] 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: <803e270b-7123-0ebd-439a-6eb0cece1373@proton.me>
Date: Mon, 09 Oct 2023 13:45:49 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Andrew Lunn <andrew@...n.ch>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, miguel.ojeda.sandonis@...il.com, greg@...ah.com, tmgross@...ch.edu
Subject: Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver

On 09.10.23 15:15, Andrew Lunn wrote:
>>> +        if ret as u32 & uapi::BMCR_SPEED100 != 0 {
>>> +            dev.set_speed(uapi::SPEED_100);
>>> +        } else {
>>> +            dev.set_speed(uapi::SPEED_10);
>>> +        }
>>
>> Maybe refactor to only have one `dev.set_speed` call?
> 
> This is a common pattern in the C code. This is basically a
> re-implementation of
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L2432
> 
> because this PHY is broken. Being one of the maintainers of the PHY
> subsystem, it helps me review this code if it happens to look like the
> existing code it is adding a workaround to.
> 
> Is there a Rust reason to only have one call?

My reason was consistency, since the call to `set_duplex`
below that was changed to only have one call:

+        let duplex = if ret as u32 & uapi::BMCR_FULLDPLX != 0 {
+            phy::DuplexMode::Full
+        } else {
+            phy::DuplexMode::Half
+        };
+        dev.set_duplex(duplex);

I think it should be consistent, I chose to reduce the number of
function calls, since it is immediately obvious that only the argument
is depending on the condition. But if you think it should mirror the C
side, then maybe change the duplex back to calling twice?

-- 
Cheers,
Benno