On Thu, Nov 09, 2023 at 01:07:42PM +0800, Xu Yilun wrote: > 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... Then something is designed wrong here, the unloading of the low-level driver should remove the access to the device itself. Perhaps fix that? > > > 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. You can care, but my point being that don't make it very complex and slow just for something that no one will ever do in normal device operation. thanks, greg k-h
