Hello Greg,
I'm going to follow Alan's advice, and add a mutex instead of juggling
with the existing one. So I'll prepare a new patch with a completely
different solution. I've answered your questions below, even though I'm
not sure any of that is still relevant.
On 26/10/2022 15:07, Greg KH wrote:
int xillybus_find_inode(struct inode *inode,
+ struct mutex **to_unlock,
To unlock when? Who unlocks it? What is the lifespan here?
xillybus_find_inode() finds the structure that represents a Xillybus
(PCIe / OF) or XillyUSB (USB) device. The idea was to delay the unlock
of the mutex until other means have been taken to prevent that structure
from being freed. Which was virtually immediately after
xillybus_find_inode() returns, but gave XillyUSB's driver a chance to
acquire another mutex before releasing this one.
XillyUSB's driver can't prevent the release of this structure before it
knows which structure it is (that's xillybus_find_inode()'s job to find
out). But because xillybus_find_inode() unlocks its mutex before
returning, there is a short period of time where the structure is
unprotected. So I thought, let's extend the life of the first mutex just
a little bit to keep the whole thing watertight.
Why can't it just be part of the structure?
The problem is that this structure is either a struct xilly_endpoint
(for PCIe / OF) or a struct xillyusb_dev (for USB), and these have
virtually nothing in common. xillybus_find_inode() is used by two
completely different drivers that are grouped into a single class.
*private_data = unit->private_data;
*index = minor - unit->lowest_minor;
+ *to_unlock = &unit_mutex;
Why are you wanting the caller to unlock this? It's a global mutex (for
the whole file), this feels really odd.
As mentioned above, this gives the caller (i.e. XillyUSB's driver) a
chance to ensure that the structure isn't freed as a result of the
device's disconnection.
What is this function supposed to be doing? You only return an int, and
you have some odd opaque void pointer being set. That feels wrong and
is not a normal design at all.
xillybus_find_inode() finds the device's structure, based upon the
inode's major and minor.
The opaque void pointer is the structure that represents the device,
either a PCIe / OF device or a XillyUSB device. Because of the
difference between a driver for PCIe / OF and a USB driver, these are
different structs, and hence represented by a void pointer. The int just
says which of the device files, that are covered by the struct, has been
opened.
Once again, all this is history, as I'm preparing a new patch, which is
going to add a separate mutex.
Regards,
Eli
