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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <20231119.185736.1872000177070369059.fujita.tomonori@gmail.com>
Date: Sun, 19 Nov 2023 18:57:36 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: aliceryhl@...gle.com
Cc: fujita.tomonori@...il.com, andrew@...n.ch, 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 Fri, 17 Nov 2023 09:39:15 +0000
Alice Ryhl <aliceryhl@...gle.com> wrote:

> FUJITA Tomonori <fujita.tomonori@...il.com> writes:
>> This is the Rust implementation of drivers/net/phy/ax88796b.c. The
>> features are equivalent. You can choose C or Rust versionon kernel
>> configuration.
>> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
>> Reviewed-by: Trevor Gross <tmgross@...ch.edu>
>> Reviewed-by: Benno Lossin <benno.lossin@...ton.me>
> 
> Overall looks reasonable. I found various nits below:
> 
> There's a typo in your commit message: versionon.

Oops, will fix.

> 
>> +use kernel::c_str;
>> +use kernel::net::phy::{self, DeviceId, Driver};
>> +use kernel::prelude::*;
>> +use kernel::uapi;
> 
> You used the other import style in other patches.

use kernel::{
    c_str,
    net::phy::{self, DeviceId, Driver},
    prelude::*,
    uapi,
};

?

>> +        // If MII_LPA is 0, phy_resolve_aneg_linkmode() will fail to resolve
>> +        // linkmode so use MII_BMCR as default values.
>> +        let ret = dev.read(uapi::MII_BMCR as u16)?;
>> +
>> +        if ret as u32 & uapi::BMCR_SPEED100 != 0 {
> 
> The `ret as u32` and `uapi::MII_BMCR as u16` casts make me think that
> these constants are defined as the wrong type?
> 
> It's probably difficult to get bindgen to change the type, but you could
> do this at the top of the function or file:

Yes, if we could specfy a type with bindgen, it's easier.

> 	const MII_BMCR: u16 = uapi::MII_BMCR as u16;
> 	const BMCR_SPEED100: u16 = uapi::BMCR_SPEED100 as u16;

I'll.


>> +            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.


Thanks!