Re: [PATCH v5 3/4] sample: rust_misc_device: Demonstrate additional get/set value functionality

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Dec 06, 2024 at 01:06:30PM +0000, Lee Jones wrote:
> On Fri, 06 Dec 2024, Lee Jones wrote:
> > On Fri, 06 Dec 2024, Greg KH wrote:
> > > On Fri, Dec 06, 2024 at 12:42:14PM +0000, Lee Jones wrote:
> > > > +    fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
> > > > +        let guard = self.inner.lock();
> > > > +        let value = guard.value;
> > > > +
> > > > +        // Refrain from calling write() on a locked resource
> > > > +        drop(guard);
> > > > +
> > > > +        pr_info!("-> Copying data to userspace (value: {})\n", &value);
> > > > +
> > > > +        writer.write::<i32>(&value)?;
> > > > +        Ok(0)
> > > > +    }
> > > 
> > > I don't understand why you have to drop the mutex before calling
> > > pr_info() and write (i.e. copy_to_user())?  It's a mutex, not a
> > > spinlock, so you can hold it over that potentially-sleeping call, right?
> > > Or is there some other reason why here?
> > 
> > This was a request from Alice to demonstrate how to unlock a mutex.
> 
> It's common practice to apply guards only around the protected value.
> 
> Why would this be different?

It isn't, it's just that you are implying that the guard has to be
dropped because of the call to write(), which is confusing.  It's only
"needed" because you want to guard a single cpu instruction that is
guaranteed atomic by the processor :)

As this is an example driver, documentation is essential, so maybe the
comment should be:
	// Drop the mutex as we can now use our local copy
or something like that.

thanks,

greg k-h