| Message ID | 20240731184256.1852840-1-srinivas.pandruvada@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/x86: ISST: Simplify isst_misc_reg() and isst_misc_unreg() | expand |
On Wed, 31 Jul 2024, Srinivas Pandruvada wrote: > After commit '1630dc626c87 ("platform/x86: ISST: Add model specific > loading for common module")' isst_misc_reg() and isst_misc_unreg() can be > simplified. Since these functions are only called during module_init() > and module_exit() respectively, there is no contention while calling > misc_register()/misc_deregister or isst_if_cpu_info_init()/ > isst_if_cpu_info_exit(). > > Hence remove mutex and reference counting. > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > --- > .../intel/speed_select_if/isst_if_common.c | 42 +++++-------------- > 1 file changed, 11 insertions(+), 31 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 3065f149e721..febfd5eeceb4 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 > @@ -651,10 +651,6 @@ static long isst_if_def_ioctl(struct file *file, unsigned int cmd, > > /* Lock to prevent module registration when already opened by user space */ > static DEFINE_MUTEX(punit_misc_dev_open_lock); > -/* Lock to allow one shared misc device for all ISST interfaces */ > -static DEFINE_MUTEX(punit_misc_dev_reg_lock); > -static int misc_usage_count; > -static int misc_device_ret; > static int misc_device_open; > > static int isst_if_open(struct inode *inode, struct file *file) > @@ -720,39 +716,23 @@ static struct miscdevice isst_if_char_driver = { > > 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++; > + int ret; > > -unlock_exit: > - mutex_unlock(&punit_misc_dev_reg_lock); > + ret = isst_if_cpu_info_init(); > + if (ret) > + return ret; > > - return misc_device_ret; > + ret = misc_register(&isst_if_char_driver); > + if (ret) > + isst_if_cpu_info_exit(); > + > + return 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); > + misc_deregister(&isst_if_char_driver); > + isst_if_cpu_info_exit(); > } > > /** > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Hi, On 7/31/24 8:42 PM, Srinivas Pandruvada wrote: > After commit '1630dc626c87 ("platform/x86: ISST: Add model specific > loading for common module")' isst_misc_reg() and isst_misc_unreg() can be > simplified. Since these functions are only called during module_init() > and module_exit() respectively, there is no contention while calling > misc_register()/misc_deregister or isst_if_cpu_info_init()/ > isst_if_cpu_info_exit(). > > Hence remove mutex and reference counting. > > 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 | 42 +++++-------------- > 1 file changed, 11 insertions(+), 31 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 3065f149e721..febfd5eeceb4 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 > @@ -651,10 +651,6 @@ static long isst_if_def_ioctl(struct file *file, unsigned int cmd, > > /* Lock to prevent module registration when already opened by user space */ > static DEFINE_MUTEX(punit_misc_dev_open_lock); > -/* Lock to allow one shared misc device for all ISST interfaces */ > -static DEFINE_MUTEX(punit_misc_dev_reg_lock); > -static int misc_usage_count; > -static int misc_device_ret; > static int misc_device_open; > > static int isst_if_open(struct inode *inode, struct file *file) > @@ -720,39 +716,23 @@ static struct miscdevice isst_if_char_driver = { > > 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++; > + int ret; > > -unlock_exit: > - mutex_unlock(&punit_misc_dev_reg_lock); > + ret = isst_if_cpu_info_init(); > + if (ret) > + return ret; > > - return misc_device_ret; > + ret = misc_register(&isst_if_char_driver); > + if (ret) > + isst_if_cpu_info_exit(); > + > + return 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); > + misc_deregister(&isst_if_char_driver); > + isst_if_cpu_info_exit(); > } > > /**
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 3065f149e721..febfd5eeceb4 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 @@ -651,10 +651,6 @@ static long isst_if_def_ioctl(struct file *file, unsigned int cmd, /* Lock to prevent module registration when already opened by user space */ static DEFINE_MUTEX(punit_misc_dev_open_lock); -/* Lock to allow one shared misc device for all ISST interfaces */ -static DEFINE_MUTEX(punit_misc_dev_reg_lock); -static int misc_usage_count; -static int misc_device_ret; static int misc_device_open; static int isst_if_open(struct inode *inode, struct file *file) @@ -720,39 +716,23 @@ static struct miscdevice isst_if_char_driver = { 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++; + int ret; -unlock_exit: - mutex_unlock(&punit_misc_dev_reg_lock); + ret = isst_if_cpu_info_init(); + if (ret) + return ret; - return misc_device_ret; + ret = misc_register(&isst_if_char_driver); + if (ret) + isst_if_cpu_info_exit(); + + return 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); + misc_deregister(&isst_if_char_driver); + isst_if_cpu_info_exit(); } /**
After commit '1630dc626c87 ("platform/x86: ISST: Add model specific loading for common module")' isst_misc_reg() and isst_misc_unreg() can be simplified. Since these functions are only called during module_init() and module_exit() respectively, there is no contention while calling misc_register()/misc_deregister or isst_if_cpu_info_init()/ isst_if_cpu_info_exit(). Hence remove mutex and reference counting. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- .../intel/speed_select_if/isst_if_common.c | 42 +++++-------------- 1 file changed, 11 insertions(+), 31 deletions(-)