| Message ID | 20220112022521.54669-1-srinivas.pandruvada@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/x86: ISST: Fix possible circular locking dependency detected | expand |
Hi, On 1/12/22 03:25, Srinivas Pandruvada wrote: > As reported: > > [ 256.104522] ====================================================== > [ 256.113783] WARNING: possible circular locking dependency detected > [ 256.120093] 5.16.0-rc6-yocto-standard+ #99 Not tainted > [ 256.125362] ------------------------------------------------------ > [ 256.131673] intel-speed-sel/844 is trying to acquire lock: > [ 256.137290] ffffffffc036f0d0 (punit_misc_dev_lock){+.+.}-{3:3}, at: isst_if_open+0x18/0x90 [isst_if_common] > [ 256.147171] > [ 256.147171] but task is already holding lock: > [ 256.153135] ffffffff8ee7cb50 (misc_mtx){+.+.}-{3:3}, at: misc_open+0x2a/0x170 > [ 256.160407] > [ 256.160407] which lock already depends on the new lock. > [ 256.160407] > [ 256.168712] > [ 256.168712] the existing dependency chain (in reverse order) is: > [ 256.176327] > [ 256.176327] -> #1 (misc_mtx){+.+.}-{3:3}: > [ 256.181946] lock_acquire+0x1e6/0x330 > [ 256.186265] __mutex_lock+0x9b/0x9b0 > [ 256.190497] mutex_lock_nested+0x1b/0x20 > [ 256.195075] misc_register+0x32/0x1a0 > [ 256.199390] isst_if_cdev_register+0x65/0x180 [isst_if_common] > [ 256.205878] isst_if_probe+0x144/0x16e [isst_if_mmio] > ... > [ 256.241976] > [ 256.241976] -> #0 (punit_misc_dev_lock){+.+.}-{3:3}: > [ 256.248552] validate_chain+0xbc6/0x1750 > [ 256.253131] __lock_acquire+0x88c/0xc10 > [ 256.257618] lock_acquire+0x1e6/0x330 > [ 256.261933] __mutex_lock+0x9b/0x9b0 > [ 256.266165] mutex_lock_nested+0x1b/0x20 > [ 256.270739] isst_if_open+0x18/0x90 [isst_if_common] > [ 256.276356] misc_open+0x100/0x170 > [ 256.280409] chrdev_open+0xa5/0x1e0 > ... > > The call sequence suggested that misc_device /dev file can be opened > before misc device is yet to be registered, which is done only once. > > Here punit_misc_dev_lock was used as common lock, to protect the > registration by multiple ISST HW drivers, one time setup, prevent > duplicate registry of misc device and prevent load/unload when device > is open. > > We can split into locks: > - One which just prevent duplicate call to misc_register() and one > time setup. Also never call again if the misc_register() failed or > required one time setup is failed. This lock is not shared with > any misc device callbacks. > > - The other lock protects registry, load and unload of HW drivers. > > Sequence in isst_if_cdev_register() > - Register callbacks under punit_misc_dev_open_lock > - Call isst_misc_reg() which registers misc_device on the first > registry which is under punit_misc_dev_reg_lock, which is not > shared with callbacks. > > Sequence in isst_if_cdev_unregister > Just opposite of isst_if_cdev_register > > Reported-and-tested-by: Liwei Song <liwei.song@windriver.com> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > .../intel/speed_select_if/isst_if_common.c | 97 ++++++++++++------- > 1 file changed, 63 insertions(+), 34 deletions(-) > > diff --git a/drivers/platform/x86/intel/speed_select_if/isst_if_common.c b/drivers/platform/x86/intel/speed_select_if/isst_if_common.c > index c9a85eb2e860..e8424e70d81d 100644 > --- a/drivers/platform/x86/intel/speed_select_if/isst_if_common.c > +++ b/drivers/platform/x86/intel/speed_select_if/isst_if_common.c > @@ -596,7 +596,10 @@ static long isst_if_def_ioctl(struct file *file, unsigned int cmd, > return ret; > } > > -static DEFINE_MUTEX(punit_misc_dev_lock); > +/* Lock to prevent module registration when already opened by user space */ > +static DEFINE_MUTEX(punit_misc_dev_open_lock); > +/* Lock to allow one share misc device for all ISST interace */ > +static DEFINE_MUTEX(punit_misc_dev_reg_lock); > static int misc_usage_count; > static int misc_device_ret; > static int misc_device_open; > @@ -606,7 +609,7 @@ static int isst_if_open(struct inode *inode, struct file *file) > int i, ret = 0; > > /* Fail open, if a module is going away */ > - mutex_lock(&punit_misc_dev_lock); > + mutex_lock(&punit_misc_dev_open_lock); > for (i = 0; i < ISST_IF_DEV_MAX; ++i) { > struct isst_if_cmd_cb *cb = &punit_callbacks[i]; > > @@ -628,7 +631,7 @@ static int isst_if_open(struct inode *inode, struct file *file) > } else { > misc_device_open++; > } > - mutex_unlock(&punit_misc_dev_lock); > + mutex_unlock(&punit_misc_dev_open_lock); > > return ret; > } > @@ -637,7 +640,7 @@ static int isst_if_relase(struct inode *inode, struct file *f) > { > int i; > > - mutex_lock(&punit_misc_dev_lock); > + mutex_lock(&punit_misc_dev_open_lock); > misc_device_open--; > for (i = 0; i < ISST_IF_DEV_MAX; ++i) { > struct isst_if_cmd_cb *cb = &punit_callbacks[i]; > @@ -645,7 +648,7 @@ static int isst_if_relase(struct inode *inode, struct file *f) > if (cb->registered) > module_put(cb->owner); > } > - mutex_unlock(&punit_misc_dev_lock); > + mutex_unlock(&punit_misc_dev_open_lock); > > return 0; > } > @@ -662,6 +665,43 @@ static struct miscdevice isst_if_char_driver = { > .fops = &isst_if_char_driver_ops, > }; > > +static int isst_misc_reg(void) > +{ > + mutex_lock(&punit_misc_dev_reg_lock); > + if (misc_device_ret) > + goto unlock_exit; > + > + if (!misc_usage_count) { > + misc_device_ret = isst_if_cpu_info_init(); > + if (misc_device_ret) > + goto unlock_exit; > + > + misc_device_ret = misc_register(&isst_if_char_driver); > + if (misc_device_ret) { > + isst_if_cpu_info_exit(); > + goto unlock_exit; > + } > + } > + misc_usage_count++; > + > +unlock_exit: > + mutex_unlock(&punit_misc_dev_reg_lock); > + > + return misc_device_ret; > +} > + > +static void isst_misc_unreg(void) > +{ > + mutex_lock(&punit_misc_dev_reg_lock); > + if (misc_usage_count) > + misc_usage_count--; > + if (!misc_usage_count && !misc_device_ret) { > + misc_deregister(&isst_if_char_driver); > + isst_if_cpu_info_exit(); > + } > + mutex_unlock(&punit_misc_dev_reg_lock); > +} > + > /** > * isst_if_cdev_register() - Register callback for IOCTL > * @device_type: The device type this callback handling. > @@ -679,38 +719,31 @@ static struct miscdevice isst_if_char_driver = { > */ > int isst_if_cdev_register(int device_type, struct isst_if_cmd_cb *cb) > { > - if (misc_device_ret) > - return misc_device_ret; > + int ret; > > if (device_type >= ISST_IF_DEV_MAX) > return -EINVAL; > > - mutex_lock(&punit_misc_dev_lock); > + mutex_lock(&punit_misc_dev_open_lock); > + /* Device is already open, we don't want to add new callbacks */ > if (misc_device_open) { > - mutex_unlock(&punit_misc_dev_lock); > + mutex_unlock(&punit_misc_dev_open_lock); > return -EAGAIN; > } > - if (!misc_usage_count) { > - int ret; > - > - misc_device_ret = misc_register(&isst_if_char_driver); > - if (misc_device_ret) > - goto unlock_exit; > - > - ret = isst_if_cpu_info_init(); > - if (ret) { > - misc_deregister(&isst_if_char_driver); > - misc_device_ret = ret; > - goto unlock_exit; > - } > - } > memcpy(&punit_callbacks[device_type], cb, sizeof(*cb)); > punit_callbacks[device_type].registered = 1; > - misc_usage_count++; > -unlock_exit: > - mutex_unlock(&punit_misc_dev_lock); > + mutex_unlock(&punit_misc_dev_open_lock); > > - return misc_device_ret; > + ret = isst_misc_reg(); > + if (ret) { > + /* > + * No need of mutex as the misc device register failed > + * as no one can open device yet. Hence no contention. > + */ > + punit_callbacks[device_type].registered = 0; > + return ret; > + } > + return 0; > } > EXPORT_SYMBOL_GPL(isst_if_cdev_register); > > @@ -725,16 +758,12 @@ EXPORT_SYMBOL_GPL(isst_if_cdev_register); > */ > void isst_if_cdev_unregister(int device_type) > { > - mutex_lock(&punit_misc_dev_lock); > - misc_usage_count--; > + isst_misc_unreg(); > + mutex_lock(&punit_misc_dev_open_lock); > punit_callbacks[device_type].registered = 0; > if (device_type == ISST_IF_DEV_MBOX) > isst_delete_hash(); > - if (!misc_usage_count && !misc_device_ret) { > - misc_deregister(&isst_if_char_driver); > - isst_if_cpu_info_exit(); > - } > - mutex_unlock(&punit_misc_dev_lock); > + mutex_unlock(&punit_misc_dev_open_lock); > } > EXPORT_SYMBOL_GPL(isst_if_cdev_unregister); > >
diff --git a/drivers/platform/x86/intel/speed_select_if/isst_if_common.c b/drivers/platform/x86/intel/speed_select_if/isst_if_common.c index c9a85eb2e860..e8424e70d81d 100644 --- a/drivers/platform/x86/intel/speed_select_if/isst_if_common.c +++ b/drivers/platform/x86/intel/speed_select_if/isst_if_common.c @@ -596,7 +596,10 @@ static long isst_if_def_ioctl(struct file *file, unsigned int cmd, return ret; } -static DEFINE_MUTEX(punit_misc_dev_lock); +/* Lock to prevent module registration when already opened by user space */ +static DEFINE_MUTEX(punit_misc_dev_open_lock); +/* Lock to allow one share misc device for all ISST interace */ +static DEFINE_MUTEX(punit_misc_dev_reg_lock); static int misc_usage_count; static int misc_device_ret; static int misc_device_open; @@ -606,7 +609,7 @@ static int isst_if_open(struct inode *inode, struct file *file) int i, ret = 0; /* Fail open, if a module is going away */ - mutex_lock(&punit_misc_dev_lock); + mutex_lock(&punit_misc_dev_open_lock); for (i = 0; i < ISST_IF_DEV_MAX; ++i) { struct isst_if_cmd_cb *cb = &punit_callbacks[i]; @@ -628,7 +631,7 @@ static int isst_if_open(struct inode *inode, struct file *file) } else { misc_device_open++; } - mutex_unlock(&punit_misc_dev_lock); + mutex_unlock(&punit_misc_dev_open_lock); return ret; } @@ -637,7 +640,7 @@ static int isst_if_relase(struct inode *inode, struct file *f) { int i; - mutex_lock(&punit_misc_dev_lock); + mutex_lock(&punit_misc_dev_open_lock); misc_device_open--; for (i = 0; i < ISST_IF_DEV_MAX; ++i) { struct isst_if_cmd_cb *cb = &punit_callbacks[i]; @@ -645,7 +648,7 @@ static int isst_if_relase(struct inode *inode, struct file *f) if (cb->registered) module_put(cb->owner); } - mutex_unlock(&punit_misc_dev_lock); + mutex_unlock(&punit_misc_dev_open_lock); return 0; } @@ -662,6 +665,43 @@ static struct miscdevice isst_if_char_driver = { .fops = &isst_if_char_driver_ops, }; +static int isst_misc_reg(void) +{ + mutex_lock(&punit_misc_dev_reg_lock); + if (misc_device_ret) + goto unlock_exit; + + if (!misc_usage_count) { + misc_device_ret = isst_if_cpu_info_init(); + if (misc_device_ret) + goto unlock_exit; + + misc_device_ret = misc_register(&isst_if_char_driver); + if (misc_device_ret) { + isst_if_cpu_info_exit(); + goto unlock_exit; + } + } + misc_usage_count++; + +unlock_exit: + mutex_unlock(&punit_misc_dev_reg_lock); + + return misc_device_ret; +} + +static void isst_misc_unreg(void) +{ + mutex_lock(&punit_misc_dev_reg_lock); + if (misc_usage_count) + misc_usage_count--; + if (!misc_usage_count && !misc_device_ret) { + misc_deregister(&isst_if_char_driver); + isst_if_cpu_info_exit(); + } + mutex_unlock(&punit_misc_dev_reg_lock); +} + /** * isst_if_cdev_register() - Register callback for IOCTL * @device_type: The device type this callback handling. @@ -679,38 +719,31 @@ static struct miscdevice isst_if_char_driver = { */ int isst_if_cdev_register(int device_type, struct isst_if_cmd_cb *cb) { - if (misc_device_ret) - return misc_device_ret; + int ret; if (device_type >= ISST_IF_DEV_MAX) return -EINVAL; - mutex_lock(&punit_misc_dev_lock); + mutex_lock(&punit_misc_dev_open_lock); + /* Device is already open, we don't want to add new callbacks */ if (misc_device_open) { - mutex_unlock(&punit_misc_dev_lock); + mutex_unlock(&punit_misc_dev_open_lock); return -EAGAIN; } - if (!misc_usage_count) { - int ret; - - misc_device_ret = misc_register(&isst_if_char_driver); - if (misc_device_ret) - goto unlock_exit; - - ret = isst_if_cpu_info_init(); - if (ret) { - misc_deregister(&isst_if_char_driver); - misc_device_ret = ret; - goto unlock_exit; - } - } memcpy(&punit_callbacks[device_type], cb, sizeof(*cb)); punit_callbacks[device_type].registered = 1; - misc_usage_count++; -unlock_exit: - mutex_unlock(&punit_misc_dev_lock); + mutex_unlock(&punit_misc_dev_open_lock); - return misc_device_ret; + ret = isst_misc_reg(); + if (ret) { + /* + * No need of mutex as the misc device register failed + * as no one can open device yet. Hence no contention. + */ + punit_callbacks[device_type].registered = 0; + return ret; + } + return 0; } EXPORT_SYMBOL_GPL(isst_if_cdev_register); @@ -725,16 +758,12 @@ EXPORT_SYMBOL_GPL(isst_if_cdev_register); */ void isst_if_cdev_unregister(int device_type) { - mutex_lock(&punit_misc_dev_lock); - misc_usage_count--; + isst_misc_unreg(); + mutex_lock(&punit_misc_dev_open_lock); punit_callbacks[device_type].registered = 0; if (device_type == ISST_IF_DEV_MBOX) isst_delete_hash(); - if (!misc_usage_count && !misc_device_ret) { - misc_deregister(&isst_if_char_driver); - isst_if_cpu_info_exit(); - } - mutex_unlock(&punit_misc_dev_lock); + mutex_unlock(&punit_misc_dev_open_lock); } EXPORT_SYMBOL_GPL(isst_if_cdev_unregister);
As reported: [ 256.104522] ====================================================== [ 256.113783] WARNING: possible circular locking dependency detected [ 256.120093] 5.16.0-rc6-yocto-standard+ #99 Not tainted [ 256.125362] ------------------------------------------------------ [ 256.131673] intel-speed-sel/844 is trying to acquire lock: [ 256.137290] ffffffffc036f0d0 (punit_misc_dev_lock){+.+.}-{3:3}, at: isst_if_open+0x18/0x90 [isst_if_common] [ 256.147171] [ 256.147171] but task is already holding lock: [ 256.153135] ffffffff8ee7cb50 (misc_mtx){+.+.}-{3:3}, at: misc_open+0x2a/0x170 [ 256.160407] [ 256.160407] which lock already depends on the new lock. [ 256.160407] [ 256.168712] [ 256.168712] the existing dependency chain (in reverse order) is: [ 256.176327] [ 256.176327] -> #1 (misc_mtx){+.+.}-{3:3}: [ 256.181946] lock_acquire+0x1e6/0x330 [ 256.186265] __mutex_lock+0x9b/0x9b0 [ 256.190497] mutex_lock_nested+0x1b/0x20 [ 256.195075] misc_register+0x32/0x1a0 [ 256.199390] isst_if_cdev_register+0x65/0x180 [isst_if_common] [ 256.205878] isst_if_probe+0x144/0x16e [isst_if_mmio] ... [ 256.241976] [ 256.241976] -> #0 (punit_misc_dev_lock){+.+.}-{3:3}: [ 256.248552] validate_chain+0xbc6/0x1750 [ 256.253131] __lock_acquire+0x88c/0xc10 [ 256.257618] lock_acquire+0x1e6/0x330 [ 256.261933] __mutex_lock+0x9b/0x9b0 [ 256.266165] mutex_lock_nested+0x1b/0x20 [ 256.270739] isst_if_open+0x18/0x90 [isst_if_common] [ 256.276356] misc_open+0x100/0x170 [ 256.280409] chrdev_open+0xa5/0x1e0 ... The call sequence suggested that misc_device /dev file can be opened before misc device is yet to be registered, which is done only once. Here punit_misc_dev_lock was used as common lock, to protect the registration by multiple ISST HW drivers, one time setup, prevent duplicate registry of misc device and prevent load/unload when device is open. We can split into locks: - One which just prevent duplicate call to misc_register() and one time setup. Also never call again if the misc_register() failed or required one time setup is failed. This lock is not shared with any misc device callbacks. - The other lock protects registry, load and unload of HW drivers. Sequence in isst_if_cdev_register() - Register callbacks under punit_misc_dev_open_lock - Call isst_misc_reg() which registers misc_device on the first registry which is under punit_misc_dev_reg_lock, which is not shared with callbacks. Sequence in isst_if_cdev_unregister Just opposite of isst_if_cdev_register Reported-and-tested-by: Liwei Song <liwei.song@windriver.com> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- .../intel/speed_select_if/isst_if_common.c | 97 ++++++++++++------- 1 file changed, 63 insertions(+), 34 deletions(-)