Re: [PATCH net-next v3 1/3] rust: core abstractions for network PHY drivers

[Boqun Feng <boqun.feng@gmail.com>]

On Thu, Oct 12, 2023 at 02:58:24PM +0900, FUJITA Tomonori wrote:
> On Wed, 11 Oct 2023 11:29:45 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Mon, Oct 09, 2023 at 10:39:10AM +0900, FUJITA Tomonori wrote:
> > [...]
> >> +impl Device {
> >> +    /// Creates a new [`Device`] instance from a raw pointer.
> >> +    ///
> >> +    /// # Safety
> >> +    ///
> >> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> >> +    /// may read or write to the `phy_device` object.
> >> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> >> +        unsafe { &mut *ptr.cast() }
> >> +    }
> >> +
> >> +    /// Gets the id of the PHY.
> >> +    pub fn phy_id(&mut self) -> u32 {
> > 
> > This function doesn't modify the `self`, why does this need to be a
> > `&mut self` function? Ditto for a few functions in this impl block.
> > 
> > It seems you used `&mut self` for all the functions, which looks like
> > more design work is required here.
> 
> Ah, I can drop all the mut here.

It may not be that easy... IIUC, most of the functions in the `impl`
block can only be called correctly with phydev->lock held. In other
words, their usage requires exclusive accesses. We should somehow
express this in the type system, otherwise someone may lose track on
this requirement in the future (for example, calling any function
without the lock held).

A simple type trick comes to me is that

impl Device {
    // rename `from_raw` into `assume_locked`
    pub unsafe fn assume_locked<'a>(ptr: *mut bindings::phy_device) -> &'a LockedDevice {
	...
    }
}

/// LockedDevice is just a new type of Device
pub struct LockedDevice(Device);

impl LockedDevice {
    pub fn phy_id(&self) -> u32 {
        ...
    }
}

Others may have better idea.

Fundamentally, having a mutable method which doesn't modify the object
makes little sense, however we does need a way (other than the `&mut
self`) to express the need of exclusive here..

Regards,
Boqun

>