On 2022/05/02 18:39, Sean Young wrote: > So this part of the patch address the issue of driver_lock being > unnecessary. This should be in its own patch, so I am going to merge: > > https://patchwork.linuxtv.org/project/linux-media/patch/349f3e34-41ed-f832-3b22-ae10c50e3868@xxxxxxxxxxxxxxxxxxx/ driver_lock is not unnecessary, unless combined with kfree_rcu() approach. >> + /* >> + * We need to wait for RCU grace period in order to allow >> + * display_open() to safely check ->disconnected and increment ->users. >> + */ >> + struct rcu_head rcu; > > Is it possible to modify the users/disconnected fields while holding the > lock mutex in imon_context? This would make it unnecessary to use rcu and > simplify the code. I don't think it is possible, and I don't think it simplifies the code. Unless we revive global driver_lock lock (untested delta diff is shown below), nothing prevents from calling kfree(ictx) before holding ictx->lock or checking ->disconnected or incrementing ->users. As a whole, I think kfree_rcu() is simpler while removing global lock. diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c index 9a4f24e294bc..469a2f869572 100644 --- a/drivers/media/rc/imon.c +++ b/drivers/media/rc/imon.c @@ -166,11 +166,6 @@ struct imon_context { * imon_disconnect() was already called. */ bool disconnected; - /* - * We need to wait for RCU grace period in order to allow - * display_open() to safely check ->disconnected and increment ->users. - */ - struct rcu_head rcu; }; #define TOUCH_TIMEOUT (HZ/30) @@ -457,6 +452,9 @@ static struct usb_driver imon_driver = { .id_table = imon_usb_id_table, }; +/* to prevent races between open() and kfree() */ +static DEFINE_MUTEX(driver_lock); + /* Module bookkeeping bits */ MODULE_AUTHOR(MOD_AUTHOR); MODULE_DESCRIPTION(MOD_DESC); @@ -516,6 +514,8 @@ static int display_open(struct inode *inode, struct file *file) int subminor; int retval = 0; + mutex_lock(&driver_lock); + subminor = iminor(inode); interface = usb_find_interface(&imon_driver, subminor); if (!interface) { @@ -524,15 +524,13 @@ static int display_open(struct inode *inode, struct file *file) goto exit; } - rcu_read_lock(); ictx = usb_get_intfdata(interface); if (!ictx || ictx->disconnected || !refcount_inc_not_zero(&ictx->users)) { - rcu_read_unlock(); pr_err("no context found for minor %d\n", subminor); retval = -ENODEV; goto exit; } - rcu_read_unlock(); + mutex_unlock(&driver_lock); mutex_lock(&ictx->lock); @@ -550,10 +548,15 @@ static int display_open(struct inode *inode, struct file *file) mutex_unlock(&ictx->lock); - if (retval && refcount_dec_and_test(&ictx->users)) - free_imon_context(ictx); - + if (retval) { + mutex_unlock(&driver_lock); + if (refcount_dec_and_test(&ictx->users)) + free_imon_context(ictx); + mutex_unlock(&driver_lock); + } + return retval; exit: + mutex_unlock(&driver_lock); return retval; } @@ -2497,6 +2500,8 @@ static void imon_disconnect(struct usb_interface *interface) struct device *dev; int ifnum; + mutex_lock(&driver_lock); + ictx = usb_get_intfdata(interface); ictx->disconnected = true; dev = ictx->dev; @@ -2542,6 +2547,8 @@ static void imon_disconnect(struct usb_interface *interface) if (refcount_dec_and_test(&ictx->users)) free_imon_context(ictx); + mutex_unlock(&driver_lock); + dev_dbg(dev, "%s: iMON device (intf%d) disconnected\n", __func__, ifnum); }
