Message-Id: <20231008.232827.1538387095628135783.fujita.tomonori@gmail.com>
Date: Sun, 08 Oct 2023 23:28:27 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: tmgross@...ch.edu
Cc: andrew@...n.ch, 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 Sun, 8 Oct 2023 02:07:44 -0400
Trevor Gross <tmgross@...ch.edu> wrote:

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

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.

Powered by blists - more mailing lists