[PATCH 04/20] hwmon/coretemp: Convert to hotplug state machine

[PATCH 04/20] hwmon/coretemp: Convert to hotplug state machine

[Sebastian Andrzej Siewior]

Install the callbacks via the state machine. Setup and teardown are handled
by the hotplug core.

Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/hwmon/coretemp.c | 74 +++++++++++++++---------------------------------
 1 file changed, 23 insertions(+), 51 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 6a27eb2fed17..390038884f9e 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -676,7 +676,7 @@ static bool is_any_core_online(struct platform_data *pdata)
 	return false;
 }
 
-static void get_core_online(unsigned int cpu)
+static int coretemp_cpu_online(unsigned int cpu)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	struct platform_device *pdev = coretemp_get_pdev(cpu);
@@ -688,12 +688,12 @@ static void get_core_online(unsigned int cpu)
 	 * without thermal sensors will be filtered out.
 	 */
 	if (!cpu_has(c, X86_FEATURE_DTHERM))
-		return;
+		return 0;
 
 	if (!pdev) {
 		/* Check the microcode version of the CPU */
 		if (chk_ucode_version(cpu))
-			return;
+			return 0;
 
 		/*
 		 * Alright, we have DTS support.
@@ -703,7 +703,7 @@ static void get_core_online(unsigned int cpu)
 		 */
 		err = coretemp_device_add(cpu);
 		if (err)
-			return;
+			return 0;
 		/*
 		 * Check whether pkgtemp support is available.
 		 * If so, add interfaces for pkgtemp.
@@ -716,9 +716,10 @@ static void get_core_online(unsigned int cpu)
 	 * So, just add interfaces for this core.
 	 */
 	coretemp_add_core(cpu, 0);
+	return 0;
 }
 
-static void put_core_offline(unsigned int cpu)
+static int coretemp_cpu_offline(unsigned int cpu)
 {
 	int i, indx;
 	struct platform_data *pdata;
@@ -726,7 +727,7 @@ static void put_core_offline(unsigned int cpu)
 
 	/* If the physical CPU device does not exist, just return */
 	if (!pdev)
-		return;
+		return 0;
 
 	pdata = platform_get_drvdata(pdev);
 
@@ -734,7 +735,7 @@ static void put_core_offline(unsigned int cpu)
 
 	/* The core id is too big, just return */
 	if (indx > MAX_CORE_DATA - 1)
-		return;
+		return 0;
 
 	if (pdata->core_data[indx] && pdata->core_data[indx]->cpu == cpu)
 		coretemp_remove_core(pdata, indx);
@@ -747,7 +748,7 @@ static void put_core_offline(unsigned int cpu)
 	 */
 	for_each_sibling(i, cpu) {
 		if (i != cpu) {
-			get_core_online(i);
+			coretemp_cpu_online(i);
 			/*
 			 * Display temperature sensor data for one HT sibling
 			 * per core only, so abort the loop after one such
@@ -764,38 +765,20 @@ static void put_core_offline(unsigned int cpu)
 	 */
 	if (!is_any_core_online(pdata))
 		coretemp_device_remove(cpu);
+	return 0;
 }
 
-static int coretemp_cpu_callback(struct notifier_block *nfb,
-				 unsigned long action, void *hcpu)
-{
-	unsigned int cpu = (unsigned long) hcpu;
-
-	switch (action) {
-	case CPU_ONLINE:
-	case CPU_DOWN_FAILED:
-		get_core_online(cpu);
-		break;
-	case CPU_DOWN_PREPARE:
-		put_core_offline(cpu);
-		break;
-	}
-	return NOTIFY_OK;
-}
-
-static struct notifier_block coretemp_cpu_notifier __refdata = {
-	.notifier_call = coretemp_cpu_callback,
-};
-
 static const struct x86_cpu_id __initconst coretemp_ids[] = {
 	{ X86_VENDOR_INTEL, X86_FAMILY_ANY, X86_MODEL_ANY, X86_FEATURE_DTHERM },
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, coretemp_ids);
 
+static enum cpuhp_state coretemp_hp_online;
+
 static int __init coretemp_init(void)
 {
-	int i, err;
+	int err;
 
 	/*
 	 * CPUID.06H.EAX[0] indicates whether the CPU has thermal
@@ -809,44 +792,33 @@ static int __init coretemp_init(void)
 	if (err)
 		goto exit;
 
-	cpu_notifier_register_begin();
-	for_each_online_cpu(i)
-		get_core_online(i);
+	err = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hwmon/coretemp:online",
+				coretemp_cpu_online, coretemp_cpu_offline);
+	if (err < 0)
+		goto exit_driver_unreg;
+	coretemp_hp_online = err;
 
 #ifndef CONFIG_HOTPLUG_CPU
 	if (list_empty(&pdev_list)) {
-		cpu_notifier_register_done();
 		err = -ENODEV;
-		goto exit_driver_unreg;
+		goto exit_hp_unreg;
 	}
 #endif
-
-	__register_hotcpu_notifier(&coretemp_cpu_notifier);
-	cpu_notifier_register_done();
 	return 0;
 
 #ifndef CONFIG_HOTPLUG_CPU
+exit_hp_unreg:
+	cpuhp_remove_state(coretemp_hp_online);
+#endif
 exit_driver_unreg:
 	platform_driver_unregister(&coretemp_driver);
-#endif
 exit:
 	return err;
 }
 
 static void __exit coretemp_exit(void)
 {
-	struct pdev_entry *p, *n;
-
-	cpu_notifier_register_begin();
-	__unregister_hotcpu_notifier(&coretemp_cpu_notifier);
-	mutex_lock(&pdev_list_mutex);
-	list_for_each_entry_safe(p, n, &pdev_list, list) {
-		platform_device_unregister(p->pdev);
-		list_del(&p->list);
-		kfree(p);
-	}
-	mutex_unlock(&pdev_list_mutex);
-	cpu_notifier_register_done();
+	cpuhp_remove_state(coretemp_hp_online);
 	platform_driver_unregister(&coretemp_driver);
 }
 
-- 
2.10.2

Re: [04/20] hwmon/coretemp: Convert to hotplug state machine

[Guenter Roeck]

On Thu, Nov 17, 2016 at 07:35:25PM +0100, Sebastian Andrzej Siewior wrote:
> Install the callbacks via the state machine. Setup and teardown are handled
> by the hotplug core.
> 
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-hwmon@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Install, modprobe, modprobe -r, modprobe,

[24078.179118] ------------[ cut here ]------------
[24078.179124] WARNING: CPU: 0 PID: 14 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x6e/0x80
[24078.179125] sysfs: cannot create duplicate filename
'/devices/platform/coretemp.0'

and many more of those.

I can not test the other hwmon drivers in this series, but I would assume
that they have the same problem.

Guenter

> ---
>  drivers/hwmon/coretemp.c | 74 +++++++++++++++---------------------------------
>  1 file changed, 23 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 6a27eb2fed17..390038884f9e 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -676,7 +676,7 @@ static bool is_any_core_online(struct platform_data *pdata)
>  	return false;
>  }
>  
> -static void get_core_online(unsigned int cpu)
> +static int coretemp_cpu_online(unsigned int cpu)
>  {
>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
>  	struct platform_device *pdev = coretemp_get_pdev(cpu);
> @@ -688,12 +688,12 @@ static void get_core_online(unsigned int cpu)
>  	 * without thermal sensors will be filtered out.
>  	 */
>  	if (!cpu_has(c, X86_FEATURE_DTHERM))
> -		return;
> +		return 0;
>  
>  	if (!pdev) {
>  		/* Check the microcode version of the CPU */
>  		if (chk_ucode_version(cpu))
> -			return;
> +			return 0;
>  
>  		/*
>  		 * Alright, we have DTS support.
> @@ -703,7 +703,7 @@ static void get_core_online(unsigned int cpu)
>  		 */
>  		err = coretemp_device_add(cpu);
>  		if (err)
> -			return;
> +			return 0;
>  		/*
>  		 * Check whether pkgtemp support is available.
>  		 * If so, add interfaces for pkgtemp.
> @@ -716,9 +716,10 @@ static void get_core_online(unsigned int cpu)
>  	 * So, just add interfaces for this core.
>  	 */
>  	coretemp_add_core(cpu, 0);
> +	return 0;
>  }
>  
> -static void put_core_offline(unsigned int cpu)
> +static int coretemp_cpu_offline(unsigned int cpu)
>  {
>  	int i, indx;
>  	struct platform_data *pdata;
> @@ -726,7 +727,7 @@ static void put_core_offline(unsigned int cpu)
>  
>  	/* If the physical CPU device does not exist, just return */
>  	if (!pdev)
> -		return;
> +		return 0;
>  
>  	pdata = platform_get_drvdata(pdev);
>  
> @@ -734,7 +735,7 @@ static void put_core_offline(unsigned int cpu)
>  
>  	/* The core id is too big, just return */
>  	if (indx > MAX_CORE_DATA - 1)
> -		return;
> +		return 0;
>  
>  	if (pdata->core_data[indx] && pdata->core_data[indx]->cpu == cpu)
>  		coretemp_remove_core(pdata, indx);
> @@ -747,7 +748,7 @@ static void put_core_offline(unsigned int cpu)
>  	 */
>  	for_each_sibling(i, cpu) {
>  		if (i != cpu) {
> -			get_core_online(i);
> +			coretemp_cpu_online(i);
>  			/*
>  			 * Display temperature sensor data for one HT sibling
>  			 * per core only, so abort the loop after one such
> @@ -764,38 +765,20 @@ static void put_core_offline(unsigned int cpu)
>  	 */
>  	if (!is_any_core_online(pdata))
>  		coretemp_device_remove(cpu);
> +	return 0;
>  }
>  
> -static int coretemp_cpu_callback(struct notifier_block *nfb,
> -				 unsigned long action, void *hcpu)
> -{
> -	unsigned int cpu = (unsigned long) hcpu;
> -
> -	switch (action) {
> -	case CPU_ONLINE:
> -	case CPU_DOWN_FAILED:
> -		get_core_online(cpu);
> -		break;
> -	case CPU_DOWN_PREPARE:
> -		put_core_offline(cpu);
> -		break;
> -	}
> -	return NOTIFY_OK;
> -}
> -
> -static struct notifier_block coretemp_cpu_notifier __refdata = {
> -	.notifier_call = coretemp_cpu_callback,
> -};
> -
>  static const struct x86_cpu_id __initconst coretemp_ids[] = {
>  	{ X86_VENDOR_INTEL, X86_FAMILY_ANY, X86_MODEL_ANY, X86_FEATURE_DTHERM },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(x86cpu, coretemp_ids);
>  
> +static enum cpuhp_state coretemp_hp_online;
> +
>  static int __init coretemp_init(void)
>  {
> -	int i, err;
> +	int err;
>  
>  	/*
>  	 * CPUID.06H.EAX[0] indicates whether the CPU has thermal
> @@ -809,44 +792,33 @@ static int __init coretemp_init(void)
>  	if (err)
>  		goto exit;
>  
> -	cpu_notifier_register_begin();
> -	for_each_online_cpu(i)
> -		get_core_online(i);
> +	err = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hwmon/coretemp:online",
> +				coretemp_cpu_online, coretemp_cpu_offline);
> +	if (err < 0)
> +		goto exit_driver_unreg;
> +	coretemp_hp_online = err;
>  
>  #ifndef CONFIG_HOTPLUG_CPU
>  	if (list_empty(&pdev_list)) {
> -		cpu_notifier_register_done();
>  		err = -ENODEV;
> -		goto exit_driver_unreg;
> +		goto exit_hp_unreg;
>  	}
>  #endif
> -
> -	__register_hotcpu_notifier(&coretemp_cpu_notifier);
> -	cpu_notifier_register_done();
>  	return 0;
>  
>  #ifndef CONFIG_HOTPLUG_CPU
> +exit_hp_unreg:
> +	cpuhp_remove_state(coretemp_hp_online);
> +#endif
>  exit_driver_unreg:
>  	platform_driver_unregister(&coretemp_driver);
> -#endif
>  exit:
>  	return err;
>  }
>  
>  static void __exit coretemp_exit(void)
>  {
> -	struct pdev_entry *p, *n;
> -
> -	cpu_notifier_register_begin();
> -	__unregister_hotcpu_notifier(&coretemp_cpu_notifier);
> -	mutex_lock(&pdev_list_mutex);
> -	list_for_each_entry_safe(p, n, &pdev_list, list) {
> -		platform_device_unregister(p->pdev);
> -		list_del(&p->list);
> -		kfree(p);
> -	}
> -	mutex_unlock(&pdev_list_mutex);
> -	cpu_notifier_register_done();
> +	cpuhp_remove_state(coretemp_hp_online);
>  	platform_driver_unregister(&coretemp_driver);
>  }
>  

Re: [04/20] hwmon/coretemp: Convert to hotplug state machine

[Sebastian Andrzej Siewior]

On 2016-11-20 14:30:55 [-0800], Guenter Roeck wrote:
> Install, modprobe, modprobe -r, modprobe,
> 
> [24078.179118] ------------[ cut here ]------------
> [24078.179124] WARNING: CPU: 0 PID: 14 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x6e/0x80
> [24078.179125] sysfs: cannot create duplicate filename
> '/devices/platform/coretemp.0'
> 
> and many more of those.
> 
> I can not test the other hwmon drivers in this series, but I would assume
> that they have the same problem.

I managed to trigger this. I will take a look.

> Guenter

Sebastian

[PATCH 05/20] hwmon/via-cputemp: Remove pointless CPU check on each CPU

[Sebastian Andrzej Siewior]

From: Thomas Gleixner <tglx@linutronix.de>

The check loop for the cpu type is pointless as we already have a cpu model
match before that. The only thing which is not covered by that check would
be a smp system with two different cores. Not likely to happen.

Cc: Jean Delvare <jdelvare@suse.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/hwmon/via-cputemp.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c
index ac91c07e3f90..5b9866b1b437 100644
--- a/drivers/hwmon/via-cputemp.c
+++ b/drivers/hwmon/via-cputemp.c
@@ -319,22 +319,8 @@ static int __init via_cputemp_init(void)
 		goto exit;
 
 	cpu_notifier_register_begin();
-	for_each_online_cpu(i) {
-		struct cpuinfo_x86 *c = &cpu_data(i);
-
-		if (c->x86 != 6)
-			continue;
-
-		if (c->x86_model < 0x0a)
-			continue;
-
-		if (c->x86_model > 0x0f) {
-			pr_warn("Unknown CPU model 0x%x\n", c->x86_model);
-			continue;
-		}
-
+	for_each_online_cpu(i)
 		via_cputemp_device_add(i);
-	}
 
 #ifndef CONFIG_HOTPLUG_CPU
 	if (list_empty(&pdev_list)) {
-- 
2.10.2

Re: [05/20] hwmon/via-cputemp: Remove pointless CPU check on each CPU

[Guenter Roeck]

On Thu, Nov 17, 2016 at 07:35:26PM +0100, Sebastian Andrzej Siewior wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The check loop for the cpu type is pointless as we already have a cpu model
> match before that. The only thing which is not covered by that check would
> be a smp system with two different cores. Not likely to happen.
> 
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-hwmon@vger.kernel.org
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Applied to -next.

Thanks,
Guenter

Re: [05/20] hwmon/via-cputemp: Remove pointless CPU check on each CPU

[Sebastian Andrzej Siewior]

On 2016-11-19 09:23:31 [-0800], Guenter Roeck wrote:
> Applied to -next.

Thanks. Since you took that one, could you also please consider to apply
|[PATCH 06/20 v2] hwmon/via-cputemp: Convert to hotplug state machine
? It depends on the 5th patch from the series which applied.

> Thanks,
> Guenter

Sebastian

Re: [05/20] hwmon/via-cputemp: Remove pointless CPU check on each CPU

[Guenter Roeck]

On 11/19/2016 02:53 PM, Sebastian Andrzej Siewior wrote:
> On 2016-11-19 09:23:31 [-0800], Guenter Roeck wrote:
>> Applied to -next.
>
> Thanks. Since you took that one, could you also please consider to apply
> |[PATCH 06/20 v2] hwmon/via-cputemp: Convert to hotplug state machine
> ? It depends on the 5th patch from the series which applied.
>

Problem is that I have no idea if any of the patches in this series really work.
I wasn't copied on all patches, meaning I don't have the infrastructure, meaning
I'll have to dig them up from patchwork for testing, and/or figure out if the
required infrastructure is already in the kernel, and that all takes time.
Just Acking or applying the the patches w/o testing doesn't seem appropriate,
given their level of intrusiveness. I did spend most of today working through
my backlog of hwmon patches, but that series simply was too much. I hope I'll
get to it tomorrow, but no promises.

Guenter

Re: [05/20] hwmon/via-cputemp: Remove pointless CPU check on each CPU

[Sebastian Andrzej Siewior]

On 2016-11-19 19:53:21 [-0800], Guenter Roeck wrote:
> Problem is that I have no idea if any of the patches in this series really work.
> I wasn't copied on all patches, meaning I don't have the infrastructure, meaning
> I'll have to dig them up from patchwork for testing, and/or figure out if the
> required infrastructure is already in the kernel, and that all takes time.
> Just Acking or applying the the patches w/o testing doesn't seem appropriate,
> given their level of intrusiveness. I did spend most of today working through
> my backlog of hwmon patches, but that series simply was too much. I hope I'll
> get to it tomorrow, but no promises.

You don't have to dig all the patches from the series. Patch four and five are
via-cputemp related and independent of the others and you were on Cc: on
both of them. The other 18 patches were converting other drivers for
instance:
  [PATCH 01/20] x86/mce/therm_throt: Convert to hotplug state machine                      
  [PATCH 02/20] x86/cpuid: Convert to hotplug state machine                                
  [PATCH 03/20] x86/msr: Convert to hotplug state machine     

Infrastructure. The DYN allocation is available since 5b7aa87e0482
("cpu/hotplug: Implement setup/removal interface") which is part of part
of v4.6-rc1. So testing them on v4.8 should be enough :)

> Guenter

Sebastian

[PATCH 06/20] hwmon/via-cputemp: Convert to hotplug state machine

[Sebastian Andrzej Siewior]

Install the callbacks via the state machine and let the core invoke the
callbacks on the already online CPUs. When the hotplug state is
unregistered the cleanup function is called for each cpu. So both cpu loops
in init() and exit() are not longer required.

Cc: Jean Delvare <jdelvare@suse.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/hwmon/via-cputemp.c | 61 ++++++++++++---------------------------------
 1 file changed, 16 insertions(+), 45 deletions(-)

diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c
index 5b9866b1b437..ee5377ecd7a9 100644
--- a/drivers/hwmon/via-cputemp.c
+++ b/drivers/hwmon/via-cputemp.c
@@ -220,7 +220,7 @@ struct pdev_entry {
 static LIST_HEAD(pdev_list);
 static DEFINE_MUTEX(pdev_list_mutex);
 
-static int via_cputemp_device_add(unsigned int cpu)
+static int via_cputemp_online(unsigned int cpu)
 {
 	int err;
 	struct platform_device *pdev;
@@ -261,7 +261,7 @@ static int via_cputemp_device_add(unsigned int cpu)
 	return err;
 }
 
-static void via_cputemp_device_remove(unsigned int cpu)
+static int via_cputemp_down_prep(unsigned int cpu)
 {
 	struct pdev_entry *p;
 
@@ -272,33 +272,13 @@ static void via_cputemp_device_remove(unsigned int cpu)
 			list_del(&p->list);
 			mutex_unlock(&pdev_list_mutex);
 			kfree(p);
-			return;
+			return 0;
 		}
 	}
 	mutex_unlock(&pdev_list_mutex);
+	return 0;
 }
 
-static int via_cputemp_cpu_callback(struct notifier_block *nfb,
-				    unsigned long action, void *hcpu)
-{
-	unsigned int cpu = (unsigned long) hcpu;
-
-	switch (action) {
-	case CPU_ONLINE:
-	case CPU_DOWN_FAILED:
-		via_cputemp_device_add(cpu);
-		break;
-	case CPU_DOWN_PREPARE:
-		via_cputemp_device_remove(cpu);
-		break;
-	}
-	return NOTIFY_OK;
-}
-
-static struct notifier_block via_cputemp_cpu_notifier __refdata = {
-	.notifier_call = via_cputemp_cpu_callback,
-};
-
 static const struct x86_cpu_id __initconst cputemp_ids[] = {
 	{ X86_VENDOR_CENTAUR, 6, 0xa, }, /* C7 A */
 	{ X86_VENDOR_CENTAUR, 6, 0xd, }, /* C7 D */
@@ -307,6 +287,8 @@ static const struct x86_cpu_id __initconst cputemp_ids[] = {
 };
 MODULE_DEVICE_TABLE(x86cpu, cputemp_ids);
 
+static enum cpuhp_state via_temp_online;
+
 static int __init via_cputemp_init(void)
 {
 	int i, err;
@@ -318,44 +300,33 @@ static int __init via_cputemp_init(void)
 	if (err)
 		goto exit;
 
-	cpu_notifier_register_begin();
-	for_each_online_cpu(i)
-		via_cputemp_device_add(i);
+	err = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hwmon/via:online",
+				via_cputemp_online, via_cputemp_down_prep);
+	if (err < 0)
+		goto exit_driver_unreg;
+	via_temp_online = err;
 
 #ifndef CONFIG_HOTPLUG_CPU
 	if (list_empty(&pdev_list)) {
-		cpu_notifier_register_done();
 		err = -ENODEV;
-		goto exit_driver_unreg;
+		goto exit_hp_unreg;
 	}
 #endif
-
-	__register_hotcpu_notifier(&via_cputemp_cpu_notifier);
-	cpu_notifier_register_done();
 	return 0;
 
 #ifndef CONFIG_HOTPLUG_CPU
+exit_hp_unreg:
+	cpuhp_remove_state_nocalls(via_temp_online);
+#endif
 exit_driver_unreg:
 	platform_driver_unregister(&via_cputemp_driver);
-#endif
 exit:
 	return err;
 }
 
 static void __exit via_cputemp_exit(void)
 {
-	struct pdev_entry *p, *n;
-
-	cpu_notifier_register_begin();
-	__unregister_hotcpu_notifier(&via_cputemp_cpu_notifier);
-	mutex_lock(&pdev_list_mutex);
-	list_for_each_entry_safe(p, n, &pdev_list, list) {
-		platform_device_unregister(p->pdev);
-		list_del(&p->list);
-		kfree(p);
-	}
-	mutex_unlock(&pdev_list_mutex);
-	cpu_notifier_register_done();
+	cpuhp_remove_state(via_temp_online);
 	platform_driver_unregister(&via_cputemp_driver);
 }
 
-- 
2.10.2

[PATCH 06/20 v2] hwmon/via-cputemp: Convert to hotplug state machine

[Sebastian Andrzej Siewior]

Install the callbacks via the state machine and let the core invoke the
callbacks on the already online CPUs. When the hotplug state is
unregistered the cleanup function is called for each cpu. So both cpu loops
in init() and exit() are not longer required.

Cc: Jean Delvare <jdelvare@suse.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: drop `i' in via_cputemp_init() because it is unused, reported by
       kbuild test robot <fengguang.wu@intel.com> 

 drivers/hwmon/via-cputemp.c | 63 ++++++++++++---------------------------------
 1 file changed, 17 insertions(+), 46 deletions(-)

diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c
index 5b9866b1b437..d1f209a5feac 100644
--- a/drivers/hwmon/via-cputemp.c
+++ b/drivers/hwmon/via-cputemp.c
@@ -220,7 +220,7 @@ struct pdev_entry {
 static LIST_HEAD(pdev_list);
 static DEFINE_MUTEX(pdev_list_mutex);
 
-static int via_cputemp_device_add(unsigned int cpu)
+static int via_cputemp_online(unsigned int cpu)
 {
 	int err;
 	struct platform_device *pdev;
@@ -261,7 +261,7 @@ static int via_cputemp_device_add(unsigned int cpu)
 	return err;
 }
 
-static void via_cputemp_device_remove(unsigned int cpu)
+static int via_cputemp_down_prep(unsigned int cpu)
 {
 	struct pdev_entry *p;
 
@@ -272,33 +272,13 @@ static void via_cputemp_device_remove(unsigned int cpu)
 			list_del(&p->list);
 			mutex_unlock(&pdev_list_mutex);
 			kfree(p);
-			return;
+			return 0;
 		}
 	}
 	mutex_unlock(&pdev_list_mutex);
+	return 0;
 }
 
-static int via_cputemp_cpu_callback(struct notifier_block *nfb,
-				    unsigned long action, void *hcpu)
-{
-	unsigned int cpu = (unsigned long) hcpu;
-
-	switch (action) {
-	case CPU_ONLINE:
-	case CPU_DOWN_FAILED:
-		via_cputemp_device_add(cpu);
-		break;
-	case CPU_DOWN_PREPARE:
-		via_cputemp_device_remove(cpu);
-		break;
-	}
-	return NOTIFY_OK;
-}
-
-static struct notifier_block via_cputemp_cpu_notifier __refdata = {
-	.notifier_call = via_cputemp_cpu_callback,
-};
-
 static const struct x86_cpu_id __initconst cputemp_ids[] = {
 	{ X86_VENDOR_CENTAUR, 6, 0xa, }, /* C7 A */
 	{ X86_VENDOR_CENTAUR, 6, 0xd, }, /* C7 D */
@@ -307,9 +287,11 @@ static const struct x86_cpu_id __initconst cputemp_ids[] = {
 };
 MODULE_DEVICE_TABLE(x86cpu, cputemp_ids);
 
+static enum cpuhp_state via_temp_online;
+
 static int __init via_cputemp_init(void)
 {
-	int i, err;
+	int err;
 
 	if (!x86_match_cpu(cputemp_ids))
 		return -ENODEV;
@@ -318,44 +300,33 @@ static int __init via_cputemp_init(void)
 	if (err)
 		goto exit;
 
-	cpu_notifier_register_begin();
-	for_each_online_cpu(i)
-		via_cputemp_device_add(i);
+	err = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hwmon/via:online",
+				via_cputemp_online, via_cputemp_down_prep);
+	if (err < 0)
+		goto exit_driver_unreg;
+	via_temp_online = err;
 
 #ifndef CONFIG_HOTPLUG_CPU
 	if (list_empty(&pdev_list)) {
-		cpu_notifier_register_done();
 		err = -ENODEV;
-		goto exit_driver_unreg;
+		goto exit_hp_unreg;
 	}
 #endif
-
-	__register_hotcpu_notifier(&via_cputemp_cpu_notifier);
-	cpu_notifier_register_done();
 	return 0;
 
 #ifndef CONFIG_HOTPLUG_CPU
+exit_hp_unreg:
+	cpuhp_remove_state_nocalls(via_temp_online);
+#endif
 exit_driver_unreg:
 	platform_driver_unregister(&via_cputemp_driver);
-#endif
 exit:
 	return err;
 }
 
 static void __exit via_cputemp_exit(void)
 {
-	struct pdev_entry *p, *n;
-
-	cpu_notifier_register_begin();
-	__unregister_hotcpu_notifier(&via_cputemp_cpu_notifier);
-	mutex_lock(&pdev_list_mutex);
-	list_for_each_entry_safe(p, n, &pdev_list, list) {
-		platform_device_unregister(p->pdev);
-		list_del(&p->list);
-		kfree(p);
-	}
-	mutex_unlock(&pdev_list_mutex);
-	cpu_notifier_register_done();
+	cpuhp_remove_state(via_temp_online);
 	platform_driver_unregister(&via_cputemp_driver);
 }
 
-- 
2.10.2

Re: [PATCH 06/20] hwmon/via-cputemp: Convert to hotplug state machine

[Guenter Roeck]

On 11/17/2016 10:35 AM, Sebastian Andrzej Siewior wrote:
> Install the callbacks via the state machine and let the core invoke the
> callbacks on the already online CPUs. When the hotplug state is
> unregistered the cleanup function is called for each cpu. So both cpu loops
> in init() and exit() are not longer required.
>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-hwmon@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

I would like to apply this patch, but I can not test it, and I want to make sure
it survives multiple removal/insertion sequences. Can someone give me a Tested-by: ?

Thanks,
Guenter

> ---
>  drivers/hwmon/via-cputemp.c | 61 ++++++++++++---------------------------------
>  1 file changed, 16 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/hwmon/via-cputemp.c b/drivers/hwmon/via-cputemp.c
> index 5b9866b1b437..ee5377ecd7a9 100644
> --- a/drivers/hwmon/via-cputemp.c
> +++ b/drivers/hwmon/via-cputemp.c
> @@ -220,7 +220,7 @@ struct pdev_entry {
>  static LIST_HEAD(pdev_list);
>  static DEFINE_MUTEX(pdev_list_mutex);
>
> -static int via_cputemp_device_add(unsigned int cpu)
> +static int via_cputemp_online(unsigned int cpu)
>  {
>  	int err;
>  	struct platform_device *pdev;
> @@ -261,7 +261,7 @@ static int via_cputemp_device_add(unsigned int cpu)
>  	return err;
>  }
>
> -static void via_cputemp_device_remove(unsigned int cpu)
> +static int via_cputemp_down_prep(unsigned int cpu)
>  {
>  	struct pdev_entry *p;
>
> @@ -272,33 +272,13 @@ static void via_cputemp_device_remove(unsigned int cpu)
>  			list_del(&p->list);
>  			mutex_unlock(&pdev_list_mutex);
>  			kfree(p);
> -			return;
> +			return 0;
>  		}
>  	}
>  	mutex_unlock(&pdev_list_mutex);
> +	return 0;
>  }
>
> -static int via_cputemp_cpu_callback(struct notifier_block *nfb,
> -				    unsigned long action, void *hcpu)
> -{
> -	unsigned int cpu = (unsigned long) hcpu;
> -
> -	switch (action) {
> -	case CPU_ONLINE:
> -	case CPU_DOWN_FAILED:
> -		via_cputemp_device_add(cpu);
> -		break;
> -	case CPU_DOWN_PREPARE:
> -		via_cputemp_device_remove(cpu);
> -		break;
> -	}
> -	return NOTIFY_OK;
> -}
> -
> -static struct notifier_block via_cputemp_cpu_notifier __refdata = {
> -	.notifier_call = via_cputemp_cpu_callback,
> -};
> -
>  static const struct x86_cpu_id __initconst cputemp_ids[] = {
>  	{ X86_VENDOR_CENTAUR, 6, 0xa, }, /* C7 A */
>  	{ X86_VENDOR_CENTAUR, 6, 0xd, }, /* C7 D */
> @@ -307,6 +287,8 @@ static const struct x86_cpu_id __initconst cputemp_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(x86cpu, cputemp_ids);
>
> +static enum cpuhp_state via_temp_online;
> +
>  static int __init via_cputemp_init(void)
>  {
>  	int i, err;
> @@ -318,44 +300,33 @@ static int __init via_cputemp_init(void)
>  	if (err)
>  		goto exit;
>
> -	cpu_notifier_register_begin();
> -	for_each_online_cpu(i)
> -		via_cputemp_device_add(i);
> +	err = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hwmon/via:online",
> +				via_cputemp_online, via_cputemp_down_prep);
> +	if (err < 0)
> +		goto exit_driver_unreg;
> +	via_temp_online = err;
>
>  #ifndef CONFIG_HOTPLUG_CPU
>  	if (list_empty(&pdev_list)) {
> -		cpu_notifier_register_done();
>  		err = -ENODEV;
> -		goto exit_driver_unreg;
> +		goto exit_hp_unreg;
>  	}
>  #endif
> -
> -	__register_hotcpu_notifier(&via_cputemp_cpu_notifier);
> -	cpu_notifier_register_done();
>  	return 0;
>
>  #ifndef CONFIG_HOTPLUG_CPU
> +exit_hp_unreg:
> +	cpuhp_remove_state_nocalls(via_temp_online);
> +#endif
>  exit_driver_unreg:
>  	platform_driver_unregister(&via_cputemp_driver);
> -#endif
>  exit:
>  	return err;
>  }
>
>  static void __exit via_cputemp_exit(void)
>  {
> -	struct pdev_entry *p, *n;
> -
> -	cpu_notifier_register_begin();
> -	__unregister_hotcpu_notifier(&via_cputemp_cpu_notifier);
> -	mutex_lock(&pdev_list_mutex);
> -	list_for_each_entry_safe(p, n, &pdev_list, list) {
> -		platform_device_unregister(p->pdev);
> -		list_del(&p->list);
> -		kfree(p);
> -	}
> -	mutex_unlock(&pdev_list_mutex);
> -	cpu_notifier_register_done();
> +	cpuhp_remove_state(via_temp_online);
>  	platform_driver_unregister(&via_cputemp_driver);
>  }
>
>

Re: [PATCH 06/20] hwmon/via-cputemp: Convert to hotplug state machine

[Thomas Gleixner]

Guenter,

On Thu, 17 Nov 2016, Sebastian Andrzej Siewior wrote:

> Install the callbacks via the state machine and let the core invoke the
> callbacks on the already online CPUs. When the hotplug state is
> unregistered the cleanup function is called for each cpu. So both cpu loops
> in init() and exit() are not longer required.

Can we please get those two VIA patches merged for 4.10? They are blocking
the final removal of the CPU hotplug notifier crap.

The first one which removes that loop is really harmless as there are no
multisocket VIAs. Heterogenous cores in a single die would be surprising
and the loop check would be the least of our worries in that case. IOW, it
would never get so far...

The one converting the notifier is not changing any of the functionality.

I cannot test on all VIA SMP variants either, but at least on the one I
have access to it just works.

Thanks,

	tglx

Re: [PATCH 06/20] hwmon/via-cputemp: Convert to hotplug state machine

[Guenter Roeck]

On Fri, Dec 09, 2016 at 12:53:30PM +0100, Thomas Gleixner wrote:
> Guenter,
> 
> On Thu, 17 Nov 2016, Sebastian Andrzej Siewior wrote:
> 
> > Install the callbacks via the state machine and let the core invoke the
> > callbacks on the already online CPUs. When the hotplug state is
> > unregistered the cleanup function is called for each cpu. So both cpu loops
> > in init() and exit() are not longer required.
> 
> Can we please get those two VIA patches merged for 4.10? They are blocking
> the final removal of the CPU hotplug notifier crap.
> 
> The first one which removes that loop is really harmless as there are no
> multisocket VIAs. Heterogenous cores in a single die would be surprising
> and the loop check would be the least of our worries in that case. IOW, it
> would never get so far...
> 
I had queued that one already.

> The one converting the notifier is not changing any of the functionality.
> 
> I cannot test on all VIA SMP variants either, but at least on the one I
> have access to it just works.
> 
I queued up the second patch as well. Hope it does not blow up on us.
Sorry, I got a bit nervous after the coretemp experience.

Thanks,
Guenter

Re: [PATCH 06/20] hwmon/via-cputemp: Convert to hotplug state machine

[Thomas Gleixner]

On Fri, 9 Dec 2016, Guenter Roeck wrote:
> I queued up the second patch as well. Hope it does not blow up on us.
> Sorry, I got a bit nervous after the coretemp experience.

Sorry for that, but we are watching out for blow ups and are ready to fix
any fallout.

Thanks

	tglx