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: <20231117093915.2515418-1-aliceryhl@google.com>
Date: Fri, 17 Nov 2023 09:39:15 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: fujita.tomonori@...il.com
Cc: 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

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.

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

> +        // 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:

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

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

Alice