Re: [PATCH v2 1/3] rust: core abstractions for network PHY drivers

FUJITA Tomonori
2023-10-08T14:28:31Z

On Sun, 8 Oct 2023 02:07:44 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Sat, Oct 7, 2023 at 11:13 AM Andrew Lunn <andrew@lunn.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)?)

Device? I don't think so. If we make Device pub(crate), we need to
make trait Driver pub(crate) too.

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

Ok. As I commented on the RFC reviewing, I don't think that we
need try_from conversion for values from PHYLIB. Implementing bindings
for untrusted stuff doesn't make sense.

https://lore.kernel.org/all/20230926.101928.767176570707357116.fujita.tomonori@gmail.com/

On the other hand, I think that it might worth to use try_from for
set_speed() because its about the bindings and Rust PHY drivers.
However, I leave it alone since likely setting a wrong value doesn't
break anything.