Re: [RFC PATCH] fpga: remove module reference counting from core components

[Xu Yilun <yilun.xu@linux.intel.com>]

On Wed, Nov 08, 2023 at 05:20:53PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Nov 08, 2023 at 11:52:52PM +0800, Xu Yilun wrote:
> > > >>
> > > >> In fpga_region_get() / fpga_region_put(): call get_device() before
> > > >> acquiring the mutex and put_device() after having released the mutex
> > > >> to avoid races.
> 
> Why do you need another reference count with a lock?  You already have
> that with the calls to get/put_device().

The low-level driver module could still be possibly unloaded at the same
time, if so, when FPGA core run some callbacks provided by low-level driver
module, its referenced page of code is unmapped...

I actually got this idea from this mail thread:

https://lore.kernel.org/all/20231010003746.GN800259@ZenIV/

And I see get/put of "struct file_operations.owner" in many framework
code for this purpose, to ensure no fops->read/write/ioctl() is ongoing
when module unloading.

> 
> > > > Could you help elaborate more about the race?
> > > > 
> > > 
> > > I accidentally misused the word race. My concern was that memory might
> > > be released after the last put_device(), causing mutex_unlock() to be
> > > called on a mutex that does not exist anymore. It should not happen
> > > for the moment since the region does not use devres, but I think it
> > > still makes the code more brittle.
> > 
> > It makes sense.
> > 
> > But I dislike the mutex itself. The purpose is to exclusively grab the
> > device, but a mutex is too much heavy for this.
> 
> Why "heavy"?  Is this a fast-path?  Have you measured it?

It's not a fast-path. But I didn't make it clear, see below...

> 
> > The lock/unlock of mutex
> > scattered in different functions is also an uncommon style. Maybe some
> > atomic count should be enough.
> 
> Don't make a fake lock with an atomic variable, use real locks if you
> need it.

I mean, here it doesn't not looks like a "locking" senario, although it
works.

The purpose here is to declare a device state, which says the device is
exclusively used by a user, no other user is allowed. But usually we use
mutex to protect against critical code blocks, not to represent a long
live device state.

I'm still OK for the existing mutex usage as it doesn't break anything
and if we don't want change much.

> 
> Or don't even care about module unloading at all!  Why does it matter?
> It can never happen without explicit intervention and it's something
> that a lot of the time, just will cause problems.  Don't do a lot of
> extra work for something that doesn't matter.

mm.. as mentioned above some fundamental subsystems do care about
module unloading, and I tend to keep it same way. But mm... I'm OK to
make things easier if you insist.

Thanks,
Yilun

> 
> thanks,
> 
> greg k-h