> On Wed, 1 Mar 2017, Alan Stern wrote: >> On Wed, 1 Mar 2017, Ajay Kaher wrote: >>> On Mon, 22 Feb 2017, Ajay Kaher wrote: >>> >>>> >>>>> Alan, as per my understanding I have shifted the lock from >>>>> release_usb_class() to destroy_usb_class() in patch v3. >>>>> If it is not right, please explain in detail which race condition >>>>> I have missed and also share your suggestions. >>>>> >>>> >>>> Have you considered what would happen if destroy_usb_class() ran, but >>>> some other CPU was still holding a reference to usb_class? And what if >>>> the last reference gets dropped later on, while init_usb_class() is >>>> running? >>> >>> Access of usb_class->kref is only from either init_usb_class() >>> or destroy_usb_class(), and both these functions are now protected >>> with Mutex Locking in patch v3, so there is no chance of race condition >>> as per above scenarios. >>> >>>> Maybe that's not possible here, but it is possible in general for >>>> refcounted objects. So yes, this code is probably okay, but it isn't >>>> good form. >>> >>> As per my understanding, I found to be one of the best possible solution >>> for this problem and this solutiuon don't have any side effect. >> >> Alan, I had shared modified Patch v3 as per your inputs to prevent >> the race condition during simultaneously calling of init_usb_class(). >> If you think there is scope to improve the patch, please share your inputs. > > Under the circumstances, your patch is acceptable. > > If you really want to make the point crystal clear, you could replace > usb_class->kref with an ordinary integer counter. Then it would be > obvious that there are no references other than the ones taken by > init_usb_class() and released by destroy_usb_class(). usb_class->kref is not accessible outside the file.c as usb_class is _static_ inside the file.c and pointer of usb_class->kref is not passed anywhere. Hence as you wanted, there are no references of usb_class->kref other than taken by init_usb_class() and released by destroy_usb_class(). thanks, ajay kaher Signed-off-by: Ajay Kaher --- drivers/usb/core/file.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c index 822ced9..a12d184 100644 --- a/drivers/usb/core/file.c +++ b/drivers/usb/core/file.c @@ -27,6 +27,7 @@ #define MAX_USB_MINORS 256 static const struct file_operations *usb_minors[MAX_USB_MINORS]; static DECLARE_RWSEM(minor_rwsem); +static DEFINE_MUTEX(init_usb_class_mutex); static int usb_open(struct inode *inode, struct file *file) { @@ -109,8 +110,10 @@ static void release_usb_class(struct kref *kref) static void destroy_usb_class(void) { + mutex_lock(&init_usb_class_mutex); if (usb_class) kref_put(&usb_class->kref, release_usb_class); + mutex_unlock(&init_usb_class_mutex); } int usb_major_init(void) @@ -171,7 +174,10 @@ int usb_register_dev(struct usb_interface *intf, if (intf->minor >= 0) return -EADDRINUSE; + mutex_lock(&init_usb_class_mutex); retval = init_usb_class(); + mutex_unlock(&init_usb_class_mutex); + if (retval) return retval; Thanks and Regards, Ajay Kaher