On Tue, Jan 03, 2023 at 12:46:20PM +0100, Janusz Krzysztofik wrote: > From: Robin Murphy <robin.murphy@xxxxxxx> > > Coretemp's platform driver is unconventional. All the real work is done > globally by the initcall and CPU hotplug notifiers, while the "driver" > effectively just wraps an allocation and the registration of the hwmon > interface in a long-winded round-trip through the driver core. The whole > logic of dynamically creating and destroying platform devices to bring > the interfaces up and down is error prone, since it assumes > platform_device_add() will synchronously bind the driver and set drvdata > before it returns, thus results in a NULL dereference if drivers_autoprobe > is turned off for the platform bus. Furthermore, the unusual approach of > doing that from within a CPU hotplug notifier, already commented in the > code that it deadlocks suspend, also causes lockdep issues for other > drivers or subsystems which may want to legitimately register a CPU > hotplug notifier from a platform bus notifier. > > All of these issues can be solved by ripping this unusual behaviour out > completely, simply tying the platform devices to the lifetime of the > module itself, and directly managing the hwmon interfaces from the > hotplug notifiers. There is a slight user-visible change in that > /sys/bus/platform/drivers/coretemp will no longer appear, and > /sys/devices/platform/coretemp.n will remain present if package n is > hotplugged off, but hwmon users should really only be looking for the > presence of the hwmon interfaces, whose behaviour remains unchanged. > > v2: describe the problem in neutral terms > > Link: https://lore.kernel.org/lkml/20220922101036.87457-1-janusz.krzysztofik@xxxxxxxxxxxxxxx/ > Link: https://gitlab.freedesktop.org/drm/intel/issues/6641 > Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@xxxxxxxxxxxxxxx> Applied to hwmon-next. Thanks, Guenter > --- > drivers/hwmon/coretemp.c | 128 ++++++++++++++++++--------------------- > 1 file changed, 58 insertions(+), 70 deletions(-) > > diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c > index ca7a9b373bbd6..3e440ebe2508c 100644 > --- a/drivers/hwmon/coretemp.c > +++ b/drivers/hwmon/coretemp.c > @@ -588,66 +588,49 @@ static void coretemp_remove_core(struct platform_data *pdata, int indx) > ida_free(&pdata->ida, indx - BASE_SYSFS_ATTR_NO); > } > > -static int coretemp_probe(struct platform_device *pdev) > +static int coretemp_device_add(int zoneid) > { > - struct device *dev = &pdev->dev; > + struct platform_device *pdev; > struct platform_data *pdata; > + int err; > > /* Initialize the per-zone data structures */ > - pdata = devm_kzalloc(dev, sizeof(struct platform_data), GFP_KERNEL); > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL); > if (!pdata) > return -ENOMEM; > > - pdata->pkg_id = pdev->id; > + pdata->pkg_id = zoneid; > ida_init(&pdata->ida); > - platform_set_drvdata(pdev, pdata); > > - pdata->hwmon_dev = devm_hwmon_device_register_with_groups(dev, DRVNAME, > - pdata, NULL); > - return PTR_ERR_OR_ZERO(pdata->hwmon_dev); > -} > - > -static int coretemp_remove(struct platform_device *pdev) > -{ > - struct platform_data *pdata = platform_get_drvdata(pdev); > - int i; > + pdev = platform_device_alloc(DRVNAME, zoneid); > + if (!pdev) { > + err = -ENOMEM; > + goto err_free_pdata; > + } > > - for (i = MAX_CORE_DATA - 1; i >= 0; --i) > - if (pdata->core_data[i]) > - coretemp_remove_core(pdata, i); > + err = platform_device_add(pdev); > + if (err) > + goto err_put_dev; > > - ida_destroy(&pdata->ida); > + platform_set_drvdata(pdev, pdata); > + zone_devices[zoneid] = pdev; > return 0; > -} > > -static struct platform_driver coretemp_driver = { > - .driver = { > - .name = DRVNAME, > - }, > - .probe = coretemp_probe, > - .remove = coretemp_remove, > -}; > +err_put_dev: > + platform_device_put(pdev); > +err_free_pdata: > + kfree(pdata); > + return err; > +} > > -static struct platform_device *coretemp_device_add(unsigned int cpu) > +static void coretemp_device_remove(int zoneid) > { > - int err, zoneid = topology_logical_die_id(cpu); > - struct platform_device *pdev; > - > - if (zoneid < 0) > - return ERR_PTR(-ENOMEM); > - > - pdev = platform_device_alloc(DRVNAME, zoneid); > - if (!pdev) > - return ERR_PTR(-ENOMEM); > - > - err = platform_device_add(pdev); > - if (err) { > - platform_device_put(pdev); > - return ERR_PTR(err); > - } > + struct platform_device *pdev = zone_devices[zoneid]; > + struct platform_data *pdata = platform_get_drvdata(pdev); > > - zone_devices[zoneid] = pdev; > - return pdev; > + ida_destroy(&pdata->ida); > + kfree(pdata); > + platform_device_unregister(pdev); > } > > static int coretemp_cpu_online(unsigned int cpu) > @@ -671,7 +654,10 @@ static int coretemp_cpu_online(unsigned int cpu) > if (!cpu_has(c, X86_FEATURE_DTHERM)) > return -ENODEV; > > - if (!pdev) { > + pdata = platform_get_drvdata(pdev); > + if (!pdata->hwmon_dev) { > + struct device *hwmon; > + > /* Check the microcode version of the CPU */ > if (chk_ucode_version(cpu)) > return -EINVAL; > @@ -682,9 +668,11 @@ static int coretemp_cpu_online(unsigned int cpu) > * online. So, initialize per-pkg data structures and > * then bring this core online. > */ > - pdev = coretemp_device_add(cpu); > - if (IS_ERR(pdev)) > - return PTR_ERR(pdev); > + hwmon = hwmon_device_register_with_groups(&pdev->dev, DRVNAME, > + pdata, NULL); > + if (IS_ERR(hwmon)) > + return PTR_ERR(hwmon); > + pdata->hwmon_dev = hwmon; > > /* > * Check whether pkgtemp support is available. > @@ -694,7 +682,6 @@ static int coretemp_cpu_online(unsigned int cpu) > coretemp_add_core(pdev, cpu, 1); > } > > - pdata = platform_get_drvdata(pdev); > /* > * Check whether a thread sibling is already online. If not add the > * interface for this CPU core. > @@ -713,18 +700,14 @@ static int coretemp_cpu_offline(unsigned int cpu) > struct temp_data *tdata; > int i, indx = -1, target; > > - /* > - * Don't execute this on suspend as the device remove locks > - * up the machine. > - */ > + /* No need to tear down any interfaces for suspend */ > if (cpuhp_tasks_frozen) > return 0; > > /* If the physical CPU device does not exist, just return */ > - if (!pdev) > - return 0; > - > pd = platform_get_drvdata(pdev); > + if (!pd->hwmon_dev) > + return 0; > > for (i = 0; i < NUM_REAL_CORES; i++) { > if (pd->cpu_map[i] == topology_core_id(cpu)) { > @@ -756,13 +739,14 @@ static int coretemp_cpu_offline(unsigned int cpu) > } > > /* > - * If all cores in this pkg are offline, remove the device. This > - * will invoke the platform driver remove function, which cleans up > - * the rest. > + * If all cores in this pkg are offline, remove the interface. > */ > + tdata = pd->core_data[PKG_SYSFS_ATTR_NO]; > if (cpumask_empty(&pd->cpumask)) { > - zone_devices[topology_logical_die_id(cpu)] = NULL; > - platform_device_unregister(pdev); > + if (tdata) > + coretemp_remove_core(pd, PKG_SYSFS_ATTR_NO); > + hwmon_device_unregister(pd->hwmon_dev); > + pd->hwmon_dev = NULL; > return 0; > } > > @@ -770,7 +754,6 @@ static int coretemp_cpu_offline(unsigned int cpu) > * Check whether this core is the target for the package > * interface. We need to assign it to some other cpu. > */ > - tdata = pd->core_data[PKG_SYSFS_ATTR_NO]; > if (tdata && tdata->cpu == cpu) { > target = cpumask_first(&pd->cpumask); > mutex_lock(&tdata->update_lock); > @@ -789,7 +772,7 @@ static enum cpuhp_state coretemp_hp_online; > > static int __init coretemp_init(void) > { > - int err; > + int i, err; > > /* > * CPUID.06H.EAX[0] indicates whether the CPU has thermal > @@ -805,20 +788,22 @@ static int __init coretemp_init(void) > if (!zone_devices) > return -ENOMEM; > > - err = platform_driver_register(&coretemp_driver); > - if (err) > - goto outzone; > + for (i = 0; i < max_zones; i++) { > + err = coretemp_device_add(i); > + if (err) > + goto outzone; > + } > > err = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hwmon/coretemp:online", > coretemp_cpu_online, coretemp_cpu_offline); > if (err < 0) > - goto outdrv; > + goto outzone; > coretemp_hp_online = err; > return 0; > > -outdrv: > - platform_driver_unregister(&coretemp_driver); > outzone: > + while (i--) > + coretemp_device_remove(i); > kfree(zone_devices); > return err; > } > @@ -826,8 +811,11 @@ module_init(coretemp_init) > > static void __exit coretemp_exit(void) > { > + int i; > + > cpuhp_remove_state(coretemp_hp_online); > - platform_driver_unregister(&coretemp_driver); > + for (i = 0; i < max_zones; i++) > + coretemp_device_remove(i); > kfree(zone_devices); > } > module_exit(coretemp_exit)
