[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CALNs47ujBcwHG+sgeH3m7gvkW6JKWtD0ZS66ujmswLODuExJhg@mail.gmail.com>
Date: Sun, 8 Oct 2023 02:07:44 -0400
From: Trevor Gross <tmgross@...ch.edu>
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
Subject: Re: [PATCH v2 1/3] rust: core abstractions for network PHY drivers
On Sat, Oct 7, 2023 at 11:13 AM Andrew Lunn <andrew@...n.ch> wrote:
>
> > The safety comment here still needs something like
> >
> > with the exception of fields that are synchronized via the `lock` mutex
>
> I'm not sure that really adds much useful information. Which values
> are protected by the lock? More importantly, which are not protected
> by the lock?
>
> As a general rule of thumb, driver writers don't understand
> locking. Yes, there are some which do, but many don't. So the
> workaround to that is make it so they don't need to understand
> locking. All the locking happens in the core.
>
> The exception is suspend and resume, which are called without the
> lock. So if i was to add a comment about locking, i would only put a
> comment on those two.
This doesn't get used by driver implementations, it's only used within
the abstractions here. I think anyone who needs the details can refer
to the C side, I just suggested to note the locking caveat based on
your second comment at
https://lore.kernel.org/rust-for-linux/ec6d8479-f893-4a3f-bf3e-aa0c81c4adad@lunn.ch/
Fujita - since this doesn't get exposed, could this be pub(crate)?)
> > Andrew, are there any restrictions about calling phy_init_hw more than
> > once? Or are there certain things that you are not allowed to do until
> > you call that function?
>
> phy_init_hw can be called multiple times. It used by drivers as a work
> around to broken hardware/firmware to get the device back into a good
> state. It is also used during resume, since often the PHY looses its
> settings when suspended.
Great, thank you for the clarification
> > > + unsafe extern "C" fn read_mmd_callback(
> > > + phydev: *mut bindings::phy_device,
> > > + devnum: i32,
> > > + regnum: u16,
> > > + ) -> i32 {
> > > + from_result(|| {
> > > + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> > > + let dev = unsafe { Device::from_raw(phydev) };
> > > + let ret = T::read_mmd(dev, devnum as u8, regnum)?;
> > > + Ok(ret.into())
> > > + })
> > > + }
> >
> > Since your're reading a bus, it probably doesn't hurt to do a quick
> > check when converting
> >
> > let devnum_u8 = u8::try_from(devnum).(|_| {
> > warn_once!("devnum {devnum} exceeds u8 limits");
> > code::EINVAL
> > })?
>
> I would actually say this is the wrong place to do that. Such checks
> should happen in the core, so it checks all drivers, not just the
> current one Rust driver. Feel free to submit a C patch adding this.
>
> Andrew
I guess it does that already:
https://elixir.bootlin.com/linux/v6.6-rc4/source/drivers/net/phy/phy-core.c#L556
Fujita, I think we started doing comments when we know that
lossy/bitwise `as` casts are correct. Maybe just leave the code as-is
but add
// CAST: the C side verifies devnum < 32
?
Powered by blists - more mailing lists