Re: [PATCH v2 3/3] net: phy: add Rust Asix PHY driver

FUJITA Tomonori
2023-10-07T12:07:38Z

On Sat, 7 Oct 2023 03:19:20 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Fri, Oct 6, 2023 at 5:49 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>
>> diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs
>> new file mode 100644
>> index 000000000000..d11c82a9e847
>> --- /dev/null
>> +++ b/drivers/net/phy/ax88796b_rust.rs
>
> Maybe want to link to the C version, just for the crossref?

Sure.

>> + fn read_status(dev: &mut phy::Device) -> Result<u16> {
>> + dev.genphy_update_link()?;
>> + if !dev.get_link() {
>> + return Ok(0);
>> + }
>
> Looking at this usage, I think `get_link()` should be renamed to just
> `link()`. `get_link` makes me think that it is performing an action
> like calling `genphy_update_link`, just `link()` sounds more like a
> static accessor.

Andrew suggested to rename link() to get_link(), I think.

Then we discussed again last week:

https://lore.kernel.org/rust-for-linux/20231004.084644.50784533959398755.fujita.tomonori@gmail.com/

> Or maybe it's worth replacing `get_link` with a `get_updated_link`
> that calls `genphy_update_link` and then returns `link`, the user can
> store it if they need to reuse it. This seems somewhat less accident
> prone than someone calling `.link()`/`.get_link()` repeatedly and
> wondering why their phy isn't coming up.

Once this is merged, I'll think about APIs. I need to add more
bindings anyway.

> In any case, please make the docs clear about what behavior is
> executed and what the preconditions are, it should be clear what's
> going to wait for the bus vs. simple field access.

Sure.

>> + if ret as u32 & uapi::BMCR_SPEED100 != 0 {
>> + dev.set_speed(100);
>> + } else {
>> + dev.set_speed(10);
>> + }
>
> Speed should probably actually be an enum since it has defined values.
> Something like
>
> #[non_exhaustive]
> enum Speed {
> Speed10M,
> Speed100M,
> Speed1000M,
> // 2.5G, 5G, 10G, 25G?
> }
>
> impl Speed {
> fn as_mb(self) -> u32;
> }
>

ethtool.h says:

/* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal.
* Update drivers/net/phy/phy.c:phy_speed_to_str() and
* drivers/net/bonding/bond_3ad.c:__get_link_speed() when adding new values.
*/

I don't know there are drivers that set such values.

>> + let duplex = if ret as u32 & uapi::BMCR_FULLDPLX != 0 {
>> + phy::DuplexMode::Full
>> + } else {
>> + phy::DuplexMode::Half
>> + };
>
> BMCR_x and MII_x are generated as `u32` but that's just a bindgen
> thing. It seems we should reexport them as the correct types so users
> don't need to cast all over:
>
> pub MII_BMCR: u8 = bindings::MII_BMCR as u8;
> pub BMCR_RESV: u16 = bindings::BMCR_RESV as u16; ...
> // (I'd just make a macro for this)
>
> But I'm not sure how to handle that since the uapi crate exposes its
> bindings directly. We're probably going to run into this issue with
> other uapi items at some point, any thoughts Miguel?

reexporting all the BMCR_ values by hand doesn't sound fun. Can we
automaticall generate such?

>> + dev.genphy_read_lpa()?;
>
> Same question as with the `genphy_update_link`
>
>> + 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();
>> + let _ = dev.start_aneg();
>> + }
>> + }
>> +}
>
> Is it worth doing anything with these errors? I know that the C driver doesn't.

I'll check out what other drivers do in the similar situations.

> The overall driver looks correct to me, most of these comments are
> actually about [1/3]

Thanks!