* [PATCH v3 0/3] thermal: introduce by-name softlink @ 2019-12-05 7:19 Wei Wang 2019-12-05 7:19 ` [PATCH v3 1/3] thermal: prevent cooling device with no type to be registered Wei Wang ` (3 more replies) 0 siblings, 4 replies; 22+ messages in thread From: Wei Wang @ 2019-12-05 7:19 UTC (permalink / raw) Cc: wei.vince.wang, Wei Wang, Zhang Rui, Eduardo Valentin, Daniel Lezcano, Amit Kucheria, linux-pm, linux-kernel The paths thermal_zone%d and cooling_device%d are not intuitive and the numbers are subject to change due to device tree change. This usually leads to tree traversal in userspace code. The patch creates `tz-by-name' and `cdev-by-name' for thermal zone and cooling_device respectively. Changes since v1 [1]: * Split cooling device registration into a seperate patch Changes since v2 [2]: * Split improve error message in thermal zone registration [1]: v1: https://lore.kernel.org/patchwork/patch/1139450/ [2]: v2: https://lkml.org/lkml/2019/12/4/1291 Wei Wang (3): thermal: prevent cooling device with no type to be registered thermal: improve error message in thermal zone registration thermal: create softlink by name for thermal_zone and cooling_device drivers/thermal/thermal_core.c | 55 +++++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 11 deletions(-) -- 2.24.0.393.g34dc348eaf-goog ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 1/3] thermal: prevent cooling device with no type to be registered 2019-12-05 7:19 [PATCH v3 0/3] thermal: introduce by-name softlink Wei Wang @ 2019-12-05 7:19 ` Wei Wang 2019-12-09 7:41 ` Amit Kucheria 2019-12-09 19:36 ` Daniel Lezcano 2019-12-05 7:19 ` [PATCH v3 2/3] thermal: improve error message in thermal zone registration Wei Wang ` (2 subsequent siblings) 3 siblings, 2 replies; 22+ messages in thread From: Wei Wang @ 2019-12-05 7:19 UTC (permalink / raw) Cc: wei.vince.wang, Wei Wang, Zhang Rui, Eduardo Valentin, Daniel Lezcano, Amit Kucheria, linux-pm, linux-kernel commit 54fa38cc2eda ("thermal: core: prevent zones with no types to be registered") added logic to prevent thermal zone with empty type to be registered. Similarly, there are APIs that rely on cdev->type. This patch prevents cooling device without valid type to be registered. Signed-off-by: Wei Wang <wvw@google.com> --- drivers/thermal/thermal_core.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index d4481cc8958f..974e2d91c30b 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -954,12 +954,22 @@ __thermal_cooling_device_register(struct device_node *np, struct thermal_zone_device *pos = NULL; int result; - if (type && strlen(type) >= THERMAL_NAME_LENGTH) + if (!type || !type[0]) { + pr_err("Error: No cooling device type defined\n"); return ERR_PTR(-EINVAL); + } + + if (strlen(type) >= THERMAL_NAME_LENGTH) { + pr_err("Error: Cooling device name over %d chars: %s\n", + THERMAL_NAME_LENGTH, type); + return ERR_PTR(-EINVAL); + } if (!ops || !ops->get_max_state || !ops->get_cur_state || - !ops->set_cur_state) + !ops->set_cur_state) { + pr_err("Error: Cooling device missing callbacks: %s\n", type); return ERR_PTR(-EINVAL); + } cdev = kzalloc(sizeof(*cdev), GFP_KERNEL); if (!cdev) @@ -972,7 +982,7 @@ __thermal_cooling_device_register(struct device_node *np, } cdev->id = result; - strlcpy(cdev->type, type ? : "", sizeof(cdev->type)); + strlcpy(cdev->type, type, sizeof(cdev->type)); mutex_init(&cdev->lock); INIT_LIST_HEAD(&cdev->thermal_instances); cdev->np = np; -- 2.24.0.393.g34dc348eaf-goog ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/3] thermal: prevent cooling device with no type to be registered 2019-12-05 7:19 ` [PATCH v3 1/3] thermal: prevent cooling device with no type to be registered Wei Wang @ 2019-12-09 7:41 ` Amit Kucheria 2019-12-09 17:34 ` Wei Wang 2019-12-09 19:36 ` Daniel Lezcano 1 sibling, 1 reply; 22+ messages in thread From: Amit Kucheria @ 2019-12-09 7:41 UTC (permalink / raw) To: Wei Wang Cc: Wei Wang, Zhang Rui, Eduardo Valentin, Daniel Lezcano, Linux PM list, LKML On Thu, Dec 5, 2019 at 12:50 PM Wei Wang <wvw@google.com> wrote: > > commit 54fa38cc2eda ("thermal: core: prevent zones with no types to be > registered") added logic to prevent thermal zone with empty type to be > registered. Similarly, there are APIs that rely on cdev->type. > This patch prevents cooling device without valid type to be registered. > > Signed-off-by: Wei Wang <wvw@google.com> Looks better now. Thanks. Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org> Tested-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > drivers/thermal/thermal_core.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index d4481cc8958f..974e2d91c30b 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -954,12 +954,22 @@ __thermal_cooling_device_register(struct device_node *np, > struct thermal_zone_device *pos = NULL; > int result; > > - if (type && strlen(type) >= THERMAL_NAME_LENGTH) > + if (!type || !type[0]) { > + pr_err("Error: No cooling device type defined\n"); > return ERR_PTR(-EINVAL); > + } > + > + if (strlen(type) >= THERMAL_NAME_LENGTH) { > + pr_err("Error: Cooling device name over %d chars: %s\n", > + THERMAL_NAME_LENGTH, type); > + return ERR_PTR(-EINVAL); > + } > > if (!ops || !ops->get_max_state || !ops->get_cur_state || > - !ops->set_cur_state) > + !ops->set_cur_state) { > + pr_err("Error: Cooling device missing callbacks: %s\n", type); > return ERR_PTR(-EINVAL); > + } > > cdev = kzalloc(sizeof(*cdev), GFP_KERNEL); > if (!cdev) > @@ -972,7 +982,7 @@ __thermal_cooling_device_register(struct device_node *np, > } > > cdev->id = result; > - strlcpy(cdev->type, type ? : "", sizeof(cdev->type)); > + strlcpy(cdev->type, type, sizeof(cdev->type)); > mutex_init(&cdev->lock); > INIT_LIST_HEAD(&cdev->thermal_instances); > cdev->np = np; > -- > 2.24.0.393.g34dc348eaf-goog > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/3] thermal: prevent cooling device with no type to be registered 2019-12-09 7:41 ` Amit Kucheria @ 2019-12-09 17:34 ` Wei Wang 0 siblings, 0 replies; 22+ messages in thread From: Wei Wang @ 2019-12-09 17:34 UTC (permalink / raw) To: Amit Kucheria Cc: Wei Wang, Zhang Rui, Eduardo Valentin, Daniel Lezcano, Linux PM list, LKML On Sun, Dec 8, 2019 at 11:41 PM Amit Kucheria <amit.kucheria@verdurent.com> wrote: > > On Thu, Dec 5, 2019 at 12:50 PM Wei Wang <wvw@google.com> wrote: > > > > commit 54fa38cc2eda ("thermal: core: prevent zones with no types to be > > registered") added logic to prevent thermal zone with empty type to be > > registered. Similarly, there are APIs that rely on cdev->type. > > This patch prevents cooling device without valid type to be registered. > > > > Signed-off-by: Wei Wang <wvw@google.com> > > Looks better now. Thanks. > Thanks Amit for the review and testing. > Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org> > Tested-by: Amit Kucheria <amit.kucheria@linaro.org> > > > --- > > drivers/thermal/thermal_core.c | 16 +++++++++++++--- > > 1 file changed, 13 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > > index d4481cc8958f..974e2d91c30b 100644 > > --- a/drivers/thermal/thermal_core.c > > +++ b/drivers/thermal/thermal_core.c > > @@ -954,12 +954,22 @@ __thermal_cooling_device_register(struct device_node *np, > > struct thermal_zone_device *pos = NULL; > > int result; > > > > - if (type && strlen(type) >= THERMAL_NAME_LENGTH) > > + if (!type || !type[0]) { > > + pr_err("Error: No cooling device type defined\n"); > > return ERR_PTR(-EINVAL); > > + } > > + > > + if (strlen(type) >= THERMAL_NAME_LENGTH) { > > + pr_err("Error: Cooling device name over %d chars: %s\n", > > + THERMAL_NAME_LENGTH, type); > > + return ERR_PTR(-EINVAL); > > + } > > > > if (!ops || !ops->get_max_state || !ops->get_cur_state || > > - !ops->set_cur_state) > > + !ops->set_cur_state) { > > + pr_err("Error: Cooling device missing callbacks: %s\n", type); > > return ERR_PTR(-EINVAL); > > + } > > > > cdev = kzalloc(sizeof(*cdev), GFP_KERNEL); > > if (!cdev) > > @@ -972,7 +982,7 @@ __thermal_cooling_device_register(struct device_node *np, > > } > > > > cdev->id = result; > > - strlcpy(cdev->type, type ? : "", sizeof(cdev->type)); > > + strlcpy(cdev->type, type, sizeof(cdev->type)); > > mutex_init(&cdev->lock); > > INIT_LIST_HEAD(&cdev->thermal_instances); > > cdev->np = np; > > -- > > 2.24.0.393.g34dc348eaf-goog > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/3] thermal: prevent cooling device with no type to be registered 2019-12-05 7:19 ` [PATCH v3 1/3] thermal: prevent cooling device with no type to be registered Wei Wang 2019-12-09 7:41 ` Amit Kucheria @ 2019-12-09 19:36 ` Daniel Lezcano 2019-12-09 21:13 ` Wei Wang 1 sibling, 1 reply; 22+ messages in thread From: Daniel Lezcano @ 2019-12-09 19:36 UTC (permalink / raw) To: Wei Wang Cc: wei.vince.wang, Zhang Rui, Eduardo Valentin, Amit Kucheria, linux-pm, linux-kernel On 05/12/2019 08:19, Wei Wang wrote: > commit 54fa38cc2eda ("thermal: core: prevent zones with no types to be > registered") added logic to prevent thermal zone with empty type to be > registered. Similarly, there are APIs that rely on cdev->type. > This patch prevents cooling device without valid type to be registered. > > Signed-off-by: Wei Wang <wvw@google.com> > --- > drivers/thermal/thermal_core.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index d4481cc8958f..974e2d91c30b 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -954,12 +954,22 @@ __thermal_cooling_device_register(struct device_node *np, > struct thermal_zone_device *pos = NULL; > int result; > > - if (type && strlen(type) >= THERMAL_NAME_LENGTH) > + if (!type || !type[0]) { Why not use strlen(type) == 0 ? > + pr_err("Error: No cooling device type defined\n"); > return ERR_PTR(-EINVAL); > + } > + > + if (strlen(type) >= THERMAL_NAME_LENGTH) { > + pr_err("Error: Cooling device name over %d chars: %s\n", > + THERMAL_NAME_LENGTH, type); > + return ERR_PTR(-EINVAL); > + } > > if (!ops || !ops->get_max_state || !ops->get_cur_state || > - !ops->set_cur_state) > + !ops->set_cur_state) { > + pr_err("Error: Cooling device missing callbacks: %s\n", type); > return ERR_PTR(-EINVAL); > + } > > cdev = kzalloc(sizeof(*cdev), GFP_KERNEL); > if (!cdev) > @@ -972,7 +982,7 @@ __thermal_cooling_device_register(struct device_node *np, > } > > cdev->id = result; > - strlcpy(cdev->type, type ? : "", sizeof(cdev->type)); > + strlcpy(cdev->type, type, sizeof(cdev->type)); > mutex_init(&cdev->lock); > INIT_LIST_HEAD(&cdev->thermal_instances); > cdev->np = np; > -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/3] thermal: prevent cooling device with no type to be registered 2019-12-09 19:36 ` Daniel Lezcano @ 2019-12-09 21:13 ` Wei Wang 0 siblings, 0 replies; 22+ messages in thread From: Wei Wang @ 2019-12-09 21:13 UTC (permalink / raw) To: Daniel Lezcano Cc: Wei Wang, Zhang Rui, Eduardo Valentin, Amit Kucheria, Linux PM list, LKML On Mon, Dec 9, 2019 at 11:36 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 05/12/2019 08:19, Wei Wang wrote: > > commit 54fa38cc2eda ("thermal: core: prevent zones with no types to be > > registered") added logic to prevent thermal zone with empty type to be > > registered. Similarly, there are APIs that rely on cdev->type. > > This patch prevents cooling device without valid type to be registered. > > > > Signed-off-by: Wei Wang <wvw@google.com> > > --- > > drivers/thermal/thermal_core.c | 16 +++++++++++++--- > > 1 file changed, 13 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > > index d4481cc8958f..974e2d91c30b 100644 > > --- a/drivers/thermal/thermal_core.c > > +++ b/drivers/thermal/thermal_core.c > > @@ -954,12 +954,22 @@ __thermal_cooling_device_register(struct device_node *np, > > struct thermal_zone_device *pos = NULL; > > int result; > > > > - if (type && strlen(type) >= THERMAL_NAME_LENGTH) > > + if (!type || !type[0]) { > > Why not use strlen(type) == 0 ? Checking empty is faster than getting length and this is already a pattern used in this file: https://github.com/torvalds/linux/blob/v5.4/drivers/thermal/thermal_core.c#L63 > > > + pr_err("Error: No cooling device type defined\n"); > > return ERR_PTR(-EINVAL); > > + } > > + > > + if (strlen(type) >= THERMAL_NAME_LENGTH) { > > + pr_err("Error: Cooling device name over %d chars: %s\n", > > + THERMAL_NAME_LENGTH, type); > > + return ERR_PTR(-EINVAL); > > + } > > > > if (!ops || !ops->get_max_state || !ops->get_cur_state || > > - !ops->set_cur_state) > > + !ops->set_cur_state) { > > + pr_err("Error: Cooling device missing callbacks: %s\n", type); > > return ERR_PTR(-EINVAL); > > + } > > > > cdev = kzalloc(sizeof(*cdev), GFP_KERNEL); > > if (!cdev) > > @@ -972,7 +982,7 @@ __thermal_cooling_device_register(struct device_node *np, > > } > > > > cdev->id = result; > > - strlcpy(cdev->type, type ? : "", sizeof(cdev->type)); > > + strlcpy(cdev->type, type, sizeof(cdev->type)); > > mutex_init(&cdev->lock); > > INIT_LIST_HEAD(&cdev->thermal_instances); > > cdev->np = np; > > > > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 2/3] thermal: improve error message in thermal zone registration 2019-12-05 7:19 [PATCH v3 0/3] thermal: introduce by-name softlink Wei Wang 2019-12-05 7:19 ` [PATCH v3 1/3] thermal: prevent cooling device with no type to be registered Wei Wang @ 2019-12-05 7:19 ` Wei Wang 2019-12-09 7:41 ` Amit Kucheria 2019-12-05 7:19 ` [PATCH v3 3/3] thermal: create softlink by name for thermal_zone and cooling_device Wei Wang 2019-12-10 14:36 ` [PATCH v3 0/3] thermal: introduce by-name softlink Daniel Lezcano 3 siblings, 1 reply; 22+ messages in thread From: Wei Wang @ 2019-12-05 7:19 UTC (permalink / raw) Cc: wei.vince.wang, Wei Wang, Zhang Rui, Eduardo Valentin, Daniel Lezcano, Amit Kucheria, linux-pm, linux-kernel Follow up with commit 67eed44b8a8a ("thermal: Add some error messages") to clean up checkpatch warning on line length and also add more message for developers. Signed-off-by: Wei Wang <wvw@google.com> --- drivers/thermal/thermal_core.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index 974e2d91c30b..9db7f72e70f8 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -1255,29 +1255,33 @@ thermal_zone_device_register(const char *type, int trips, int mask, int count; struct thermal_governor *governor; - if (!type || strlen(type) == 0) { + if (!type || !type[0]) { pr_err("Error: No thermal zone type defined\n"); return ERR_PTR(-EINVAL); } - if (type && strlen(type) >= THERMAL_NAME_LENGTH) { - pr_err("Error: Thermal zone name (%s) too long, should be under %d chars\n", - type, THERMAL_NAME_LENGTH); + if (strlen(type) >= THERMAL_NAME_LENGTH) { + pr_err("Error: Thermal zone name over %d chars: %s\n", + THERMAL_NAME_LENGTH, type); return ERR_PTR(-EINVAL); } if (trips > THERMAL_MAX_TRIPS || trips < 0 || mask >> trips) { - pr_err("Error: Incorrect number of thermal trips\n"); + pr_err("Error: Incorrect number of thermal trips: %s\n", type); return ERR_PTR(-EINVAL); } if (!ops) { - pr_err("Error: Thermal zone device ops not defined\n"); + pr_err("Error: Thermal zone device ops not defined: %s\n", + type); return ERR_PTR(-EINVAL); } - if (trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp)) + if (trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp)) { + pr_err("Error: Thermal zone device missing callback: %s\n", + type); return ERR_PTR(-EINVAL); + } tz = kzalloc(sizeof(*tz), GFP_KERNEL); if (!tz) -- 2.24.0.393.g34dc348eaf-goog ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] thermal: improve error message in thermal zone registration 2019-12-05 7:19 ` [PATCH v3 2/3] thermal: improve error message in thermal zone registration Wei Wang @ 2019-12-09 7:41 ` Amit Kucheria 0 siblings, 0 replies; 22+ messages in thread From: Amit Kucheria @ 2019-12-09 7:41 UTC (permalink / raw) To: Wei Wang Cc: Wei Wang, Zhang Rui, Eduardo Valentin, Daniel Lezcano, Linux PM list, LKML On Thu, Dec 5, 2019 at 12:50 PM Wei Wang <wvw@google.com> wrote: > > Follow up with commit 67eed44b8a8a ("thermal: Add some error messages") > to clean up checkpatch warning on line length and also add more message > for developers. > > Signed-off-by: Wei Wang <wvw@google.com> Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > drivers/thermal/thermal_core.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index 974e2d91c30b..9db7f72e70f8 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -1255,29 +1255,33 @@ thermal_zone_device_register(const char *type, int trips, int mask, > int count; > struct thermal_governor *governor; > > - if (!type || strlen(type) == 0) { > + if (!type || !type[0]) { > pr_err("Error: No thermal zone type defined\n"); > return ERR_PTR(-EINVAL); > } > > - if (type && strlen(type) >= THERMAL_NAME_LENGTH) { > - pr_err("Error: Thermal zone name (%s) too long, should be under %d chars\n", > - type, THERMAL_NAME_LENGTH); > + if (strlen(type) >= THERMAL_NAME_LENGTH) { > + pr_err("Error: Thermal zone name over %d chars: %s\n", > + THERMAL_NAME_LENGTH, type); > return ERR_PTR(-EINVAL); > } > > if (trips > THERMAL_MAX_TRIPS || trips < 0 || mask >> trips) { > - pr_err("Error: Incorrect number of thermal trips\n"); > + pr_err("Error: Incorrect number of thermal trips: %s\n", type); > return ERR_PTR(-EINVAL); > } > > if (!ops) { > - pr_err("Error: Thermal zone device ops not defined\n"); > + pr_err("Error: Thermal zone device ops not defined: %s\n", > + type); > return ERR_PTR(-EINVAL); > } > > - if (trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp)) > + if (trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp)) { > + pr_err("Error: Thermal zone device missing callback: %s\n", > + type); > return ERR_PTR(-EINVAL); > + } > > tz = kzalloc(sizeof(*tz), GFP_KERNEL); > if (!tz) > -- > 2.24.0.393.g34dc348eaf-goog > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 3/3] thermal: create softlink by name for thermal_zone and cooling_device 2019-12-05 7:19 [PATCH v3 0/3] thermal: introduce by-name softlink Wei Wang 2019-12-05 7:19 ` [PATCH v3 1/3] thermal: prevent cooling device with no type to be registered Wei Wang 2019-12-05 7:19 ` [PATCH v3 2/3] thermal: improve error message in thermal zone registration Wei Wang @ 2019-12-05 7:19 ` Wei Wang 2019-12-06 19:15 ` Matthias Kaehlcke 2019-12-09 7:42 ` Amit Kucheria 2019-12-10 14:36 ` [PATCH v3 0/3] thermal: introduce by-name softlink Daniel Lezcano 3 siblings, 2 replies; 22+ messages in thread From: Wei Wang @ 2019-12-05 7:19 UTC (permalink / raw) Cc: wei.vince.wang, Wei Wang, Zhang Rui, Eduardo Valentin, Daniel Lezcano, Amit Kucheria, linux-pm, linux-kernel, Amit Kucheria The paths thermal_zone%d and cooling_device%d are not intuitive and the numbers are subject to change due to device tree change. This usually leads to tree traversal in userspace code. The patch creates `tz-by-name' and `cdev-by-name' for thermal zone and cooling_device respectively. Signed-off-by: Wei Wang <wvw@google.com> Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org> --- drivers/thermal/thermal_core.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index 9db7f72e70f8..7872bd527f3f 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -22,6 +22,7 @@ #include <net/netlink.h> #include <net/genetlink.h> #include <linux/suspend.h> +#include <linux/kobject.h> #define CREATE_TRACE_POINTS #include <trace/events/thermal.h> @@ -46,6 +47,8 @@ static DEFINE_MUTEX(poweroff_lock); static atomic_t in_suspend; static bool power_off_triggered; +static struct kobject *cdev_link_kobj; +static struct kobject *tz_link_kobj; static struct thermal_governor *def_governor; @@ -999,9 +1002,15 @@ __thermal_cooling_device_register(struct device_node *np, return ERR_PTR(result); } - /* Add 'this' new cdev to the global cdev list */ + /* Add 'this' new cdev to the global cdev list and create link*/ mutex_lock(&thermal_list_lock); list_add(&cdev->node, &thermal_cdev_list); + if (!cdev_link_kobj) + cdev_link_kobj = kobject_create_and_add("cdev-by-name", + cdev->device.kobj.parent); + if (!cdev_link_kobj || sysfs_create_link(cdev_link_kobj, + &cdev->device.kobj, cdev->type)) + dev_err(&cdev->device, "Failed to create cdev-by-name link\n"); mutex_unlock(&thermal_list_lock); /* Update binding information for 'this' new cdev */ @@ -1167,6 +1176,8 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev) } } } + if (cdev_link_kobj) + sysfs_remove_link(cdev_link_kobj, cdev->type); mutex_unlock(&thermal_list_lock); @@ -1354,6 +1365,12 @@ thermal_zone_device_register(const char *type, int trips, int mask, mutex_lock(&thermal_list_lock); list_add_tail(&tz->node, &thermal_tz_list); + if (!tz_link_kobj) + tz_link_kobj = kobject_create_and_add("tz-by-name", + tz->device.kobj.parent); + if (!tz_link_kobj || sysfs_create_link(tz_link_kobj, + &tz->device.kobj, tz->type)) + dev_err(&tz->device, "Failed to create tz-by-name link\n"); mutex_unlock(&thermal_list_lock); /* Bind cooling devices for this zone */ @@ -1425,6 +1442,8 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz) } } } + if (tz_link_kobj) + sysfs_remove_link(tz_link_kobj, tz->type); mutex_unlock(&thermal_list_lock); -- 2.24.0.393.g34dc348eaf-goog ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/3] thermal: create softlink by name for thermal_zone and cooling_device 2019-12-05 7:19 ` [PATCH v3 3/3] thermal: create softlink by name for thermal_zone and cooling_device Wei Wang @ 2019-12-06 19:15 ` Matthias Kaehlcke 2019-12-09 7:42 ` Amit Kucheria 1 sibling, 0 replies; 22+ messages in thread From: Matthias Kaehlcke @ 2019-12-06 19:15 UTC (permalink / raw) To: Wei Wang Cc: wei.vince.wang, Zhang Rui, Eduardo Valentin, Daniel Lezcano, Amit Kucheria, linux-pm, linux-kernel, Amit Kucheria On Wed, Dec 04, 2019 at 11:19:53PM -0800, Wei Wang wrote: > The paths thermal_zone%d and cooling_device%d are not intuitive and the > numbers are subject to change due to device tree change. This usually > leads to tree traversal in userspace code. > The patch creates `tz-by-name' and `cdev-by-name' for thermal zone and > cooling_device respectively. This is useful, especially for systems with plenty of thermal zones :) > Signed-off-by: Wei Wang <wvw@google.com> > Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > drivers/thermal/thermal_core.c | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index 9db7f72e70f8..7872bd527f3f 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -22,6 +22,7 @@ > #include <net/netlink.h> > #include <net/genetlink.h> > #include <linux/suspend.h> > +#include <linux/kobject.h> > > #define CREATE_TRACE_POINTS > #include <trace/events/thermal.h> > @@ -46,6 +47,8 @@ static DEFINE_MUTEX(poweroff_lock); > > static atomic_t in_suspend; > static bool power_off_triggered; > +static struct kobject *cdev_link_kobj; > +static struct kobject *tz_link_kobj; > > static struct thermal_governor *def_governor; > > @@ -999,9 +1002,15 @@ __thermal_cooling_device_register(struct device_node *np, > return ERR_PTR(result); > } > > - /* Add 'this' new cdev to the global cdev list */ > + /* Add 'this' new cdev to the global cdev list and create link*/ > mutex_lock(&thermal_list_lock); > list_add(&cdev->node, &thermal_cdev_list); > + if (!cdev_link_kobj) > + cdev_link_kobj = kobject_create_and_add("cdev-by-name", > + cdev->device.kobj.parent); > + if (!cdev_link_kobj || sysfs_create_link(cdev_link_kobj, > + &cdev->device.kobj, cdev->type)) > + dev_err(&cdev->device, "Failed to create cdev-by-name link\n"); > mutex_unlock(&thermal_list_lock); > > /* Update binding information for 'this' new cdev */ > @@ -1167,6 +1176,8 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev) > } > } > } > + if (cdev_link_kobj) > + sysfs_remove_link(cdev_link_kobj, cdev->type); > > mutex_unlock(&thermal_list_lock); > > @@ -1354,6 +1365,12 @@ thermal_zone_device_register(const char *type, int trips, int mask, > > mutex_lock(&thermal_list_lock); > list_add_tail(&tz->node, &thermal_tz_list); > + if (!tz_link_kobj) > + tz_link_kobj = kobject_create_and_add("tz-by-name", > + tz->device.kobj.parent); > + if (!tz_link_kobj || sysfs_create_link(tz_link_kobj, > + &tz->device.kobj, tz->type)) > + dev_err(&tz->device, "Failed to create tz-by-name link\n"); > mutex_unlock(&thermal_list_lock); > > /* Bind cooling devices for this zone */ > @@ -1425,6 +1442,8 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz) > } > } > } > + if (tz_link_kobj) > + sysfs_remove_link(tz_link_kobj, tz->type); > > mutex_unlock(&thermal_list_lock); > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> Tested-by: Matthias Kaehlcke <mka@chromium.org> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/3] thermal: create softlink by name for thermal_zone and cooling_device 2019-12-05 7:19 ` [PATCH v3 3/3] thermal: create softlink by name for thermal_zone and cooling_device Wei Wang 2019-12-06 19:15 ` Matthias Kaehlcke @ 2019-12-09 7:42 ` Amit Kucheria 1 sibling, 0 replies; 22+ messages in thread From: Amit Kucheria @ 2019-12-09 7:42 UTC (permalink / raw) To: Wei Wang Cc: Wei Wang, Zhang Rui, Eduardo Valentin, Daniel Lezcano, Linux PM list, LKML On Thu, Dec 5, 2019 at 12:50 PM Wei Wang <wvw@google.com> wrote: > > The paths thermal_zone%d and cooling_device%d are not intuitive and the > numbers are subject to change due to device tree change. This usually > leads to tree traversal in userspace code. > The patch creates `tz-by-name' and `cdev-by-name' for thermal zone and > cooling_device respectively. > > Signed-off-by: Wei Wang <wvw@google.com> > Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org> Tested-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > drivers/thermal/thermal_core.c | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index 9db7f72e70f8..7872bd527f3f 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -22,6 +22,7 @@ > #include <net/netlink.h> > #include <net/genetlink.h> > #include <linux/suspend.h> > +#include <linux/kobject.h> > > #define CREATE_TRACE_POINTS > #include <trace/events/thermal.h> > @@ -46,6 +47,8 @@ static DEFINE_MUTEX(poweroff_lock); > > static atomic_t in_suspend; > static bool power_off_triggered; > +static struct kobject *cdev_link_kobj; > +static struct kobject *tz_link_kobj; > > static struct thermal_governor *def_governor; > > @@ -999,9 +1002,15 @@ __thermal_cooling_device_register(struct device_node *np, > return ERR_PTR(result); > } > > - /* Add 'this' new cdev to the global cdev list */ > + /* Add 'this' new cdev to the global cdev list and create link*/ > mutex_lock(&thermal_list_lock); > list_add(&cdev->node, &thermal_cdev_list); > + if (!cdev_link_kobj) > + cdev_link_kobj = kobject_create_and_add("cdev-by-name", > + cdev->device.kobj.parent); > + if (!cdev_link_kobj || sysfs_create_link(cdev_link_kobj, > + &cdev->device.kobj, cdev->type)) > + dev_err(&cdev->device, "Failed to create cdev-by-name link\n"); > mutex_unlock(&thermal_list_lock); > > /* Update binding information for 'this' new cdev */ > @@ -1167,6 +1176,8 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev) > } > } > } > + if (cdev_link_kobj) > + sysfs_remove_link(cdev_link_kobj, cdev->type); > > mutex_unlock(&thermal_list_lock); > > @@ -1354,6 +1365,12 @@ thermal_zone_device_register(const char *type, int trips, int mask, > > mutex_lock(&thermal_list_lock); > list_add_tail(&tz->node, &thermal_tz_list); > + if (!tz_link_kobj) > + tz_link_kobj = kobject_create_and_add("tz-by-name", > + tz->device.kobj.parent); > + if (!tz_link_kobj || sysfs_create_link(tz_link_kobj, > + &tz->device.kobj, tz->type)) > + dev_err(&tz->device, "Failed to create tz-by-name link\n"); > mutex_unlock(&thermal_list_lock); > > /* Bind cooling devices for this zone */ > @@ -1425,6 +1442,8 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz) > } > } > } > + if (tz_link_kobj) > + sysfs_remove_link(tz_link_kobj, tz->type); > > mutex_unlock(&thermal_list_lock); > > -- > 2.24.0.393.g34dc348eaf-goog > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 0/3] thermal: introduce by-name softlink 2019-12-05 7:19 [PATCH v3 0/3] thermal: introduce by-name softlink Wei Wang ` (2 preceding siblings ...) 2019-12-05 7:19 ` [PATCH v3 3/3] thermal: create softlink by name for thermal_zone and cooling_device Wei Wang @ 2019-12-10 14:36 ` Daniel Lezcano 2019-12-10 20:01 ` Wei Wang 3 siblings, 1 reply; 22+ messages in thread From: Daniel Lezcano @ 2019-12-10 14:36 UTC (permalink / raw) To: Wei Wang Cc: wei.vince.wang, Zhang Rui, Eduardo Valentin, Amit Kucheria, linux-pm, linux-kernel On 05/12/2019 08:19, Wei Wang wrote: > The paths thermal_zone%d and cooling_device%d are not intuitive and the > numbers are subject to change due to device tree change. This usually > leads to tree traversal in userspace code. > The patch creates `tz-by-name' and `cdev-by-name' for thermal zone and > cooling_device respectively. Instead of adding another ABI, I suggest we put the current one deprecated with a warning in the dmesg, update the documentation and change the name the next version. > Changes since v1 [1]: > * Split cooling device registration into a seperate patch > Changes since v2 [2]: > * Split improve error message in thermal zone registration > > [1]: v1: https://lore.kernel.org/patchwork/patch/1139450/ > [2]: v2: https://lkml.org/lkml/2019/12/4/1291 > > Wei Wang (3): > thermal: prevent cooling device with no type to be registered > thermal: improve error message in thermal zone registration > thermal: create softlink by name for thermal_zone and cooling_device > > drivers/thermal/thermal_core.c | 55 +++++++++++++++++++++++++++------- > 1 file changed, 44 insertions(+), 11 deletions(-) > -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 0/3] thermal: introduce by-name softlink 2019-12-10 14:36 ` [PATCH v3 0/3] thermal: introduce by-name softlink Daniel Lezcano @ 2019-12-10 20:01 ` Wei Wang 2019-12-10 20:54 ` Daniel Lezcano 0 siblings, 1 reply; 22+ messages in thread From: Wei Wang @ 2019-12-10 20:01 UTC (permalink / raw) To: Daniel Lezcano Cc: Wei Wang, Zhang Rui, Eduardo Valentin, Amit Kucheria, Linux PM list, LKML On Tue, Dec 10, 2019 at 6:36 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 05/12/2019 08:19, Wei Wang wrote: > > The paths thermal_zone%d and cooling_device%d are not intuitive and the > > numbers are subject to change due to device tree change. This usually > > leads to tree traversal in userspace code. > > The patch creates `tz-by-name' and `cdev-by-name' for thermal zone and > > cooling_device respectively. > > Instead of adding another ABI, I suggest we put the current one > deprecated with a warning in the dmesg, update the documentation and > change the name the next version. > > IMHO, we should keep the existing path which is a common pattern for sysfs interface. There are reasons we need couple thermal zone and cooling device in one class, but might be worth considering split as the latter might be used for other purposes e.g. battery current limit for preventive vdrop prevention. By nature, thermal zone are sensors, and cooling devices are usually components with potential high power use. > > Changes since v1 [1]: > > * Split cooling device registration into a seperate patch > > Changes since v2 [2]: > > * Split improve error message in thermal zone registration > > > > [1]: v1: https://lore.kernel.org/patchwork/patch/1139450/ > > [2]: v2: https://lkml.org/lkml/2019/12/4/1291 > > > > Wei Wang (3): > > thermal: prevent cooling device with no type to be registered > > thermal: improve error message in thermal zone registration > > thermal: create softlink by name for thermal_zone and cooling_device > > > > drivers/thermal/thermal_core.c | 55 +++++++++++++++++++++++++++------- > > 1 file changed, 44 insertions(+), 11 deletions(-) > > > > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 0/3] thermal: introduce by-name softlink 2019-12-10 20:01 ` Wei Wang @ 2019-12-10 20:54 ` Daniel Lezcano 2019-12-11 8:53 ` Greg Kroah-Hartman 2019-12-11 8:54 ` Greg Kroah-Hartman 0 siblings, 2 replies; 22+ messages in thread From: Daniel Lezcano @ 2019-12-10 20:54 UTC (permalink / raw) To: Wei Wang Cc: Wei Wang, Zhang Rui, Eduardo Valentin, Amit Kucheria, Linux PM list, LKML, Rafael J. Wysocki, Greg Kroah-Hartman On 10/12/2019 21:01, Wei Wang wrote: > On Tue, Dec 10, 2019 at 6:36 AM Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> >> On 05/12/2019 08:19, Wei Wang wrote: >>> The paths thermal_zone%d and cooling_device%d are not intuitive and the >>> numbers are subject to change due to device tree change. This usually >>> leads to tree traversal in userspace code. >>> The patch creates `tz-by-name' and `cdev-by-name' for thermal zone and >>> cooling_device respectively. >> >> Instead of adding another ABI, I suggest we put the current one >> deprecated with a warning in the dmesg, update the documentation and >> change the name the next version. >> >> > > IMHO, we should keep the existing path which is a common pattern for > sysfs interface. There are reasons we need couple thermal zone and > cooling device in one class, but might be worth considering split as > the latter might be used for other purposes e.g. battery current limit > for preventive vdrop prevention. By nature, thermal zone are sensors, > and cooling devices are usually components with potential high power > use. [Added Greghk and Rafael in Cc] I understand but I would like to have Greg's and Rafael's opinion on that. The result is: ls -al /sys/devices/virtual/thermal/ total 0 drwxr-xr-x 22 root root 0 Dec 10 12:34 . drwxr-xr-x 15 root root 0 Dec 10 12:34 .. drwxr-xr-x 2 root root 0 Dec 10 13:18 cdev-by-name drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device0 drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device1 drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device10 drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device11 drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device12 drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device13 drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device14 drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device15 drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device2 drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device3 drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device4 drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device5 drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device6 drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device7 drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device8 drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device9 drwxr-xr-x 3 root root 0 Dec 10 12:34 thermal_zone0 drwxr-xr-x 3 root root 0 Dec 10 12:34 thermal_zone1 drwxr-xr-x 2 root root 0 Dec 10 13:18 tz-by-name ls -al /sys/devices/virtual/thermal/cdev-by-name/ total 0 drwxr-xr-x 2 root root 0 Dec 10 13:18 . drwxr-xr-x 22 root root 0 Dec 10 12:34 .. lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-cpufreq-0 -> ../cooling_device0 lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-cpufreq-1 -> ../cooling_device1 lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-idle-0 -> ../cooling_device2 lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-idle-1 -> ../cooling_device3 lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-idle-10 -> ../cooling_device12 lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-idle-11 -> ../cooling_device13 lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-idle-12 -> ../cooling_device14 lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-idle-13 -> ../cooling_device15 lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-idle-2 -> ../cooling_device4 lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-idle-3 -> ../cooling_device5 lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-idle-4 -> ../cooling_device6 lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-idle-5 -> ../cooling_device7 lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-idle-6 -> ../cooling_device8 lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-idle-7 -> ../cooling_device9 lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-idle-8 -> ../cooling_device10 lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-idle-9 -> ../cooling_device11 ls -al /sys/devices/virtual/thermal/tz-by-name/ total 0 drwxr-xr-x 2 root root 0 Dec 10 13:18 . drwxr-xr-x 22 root root 0 Dec 10 12:34 .. lrwxrwxrwx 1 root root 0 Dec 10 20:53 cpu -> ../thermal_zone0 lrwxrwxrwx 1 root root 0 Dec 10 20:53 gpu -> ../thermal_zone1 -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 0/3] thermal: introduce by-name softlink 2019-12-10 20:54 ` Daniel Lezcano @ 2019-12-11 8:53 ` Greg Kroah-Hartman 2019-12-11 8:54 ` Greg Kroah-Hartman 1 sibling, 0 replies; 22+ messages in thread From: Greg Kroah-Hartman @ 2019-12-11 8:53 UTC (permalink / raw) To: Daniel Lezcano Cc: Wei Wang, Wei Wang, Zhang Rui, Eduardo Valentin, Amit Kucheria, Linux PM list, LKML, Rafael J. Wysocki On Tue, Dec 10, 2019 at 09:54:11PM +0100, Daniel Lezcano wrote: > On 10/12/2019 21:01, Wei Wang wrote: > > On Tue, Dec 10, 2019 at 6:36 AM Daniel Lezcano > > <daniel.lezcano@linaro.org> wrote: > >> > >> On 05/12/2019 08:19, Wei Wang wrote: > >>> The paths thermal_zone%d and cooling_device%d are not intuitive and the > >>> numbers are subject to change due to device tree change. This usually > >>> leads to tree traversal in userspace code. > >>> The patch creates `tz-by-name' and `cdev-by-name' for thermal zone and > >>> cooling_device respectively. > >> > >> Instead of adding another ABI, I suggest we put the current one > >> deprecated with a warning in the dmesg, update the documentation and > >> change the name the next version. > >> > >> > > > > IMHO, we should keep the existing path which is a common pattern for > > sysfs interface. There are reasons we need couple thermal zone and > > cooling device in one class, but might be worth considering split as > > the latter might be used for other purposes e.g. battery current limit > > for preventive vdrop prevention. By nature, thermal zone are sensors, > > and cooling devices are usually components with potential high power > > use. > > [Added Greghk and Rafael in Cc] > > I understand but I would like to have Greg's and Rafael's opinion on that. > > The result is: > > ls -al /sys/devices/virtual/thermal/ > total 0 > drwxr-xr-x 22 root root 0 Dec 10 12:34 . > drwxr-xr-x 15 root root 0 Dec 10 12:34 .. > drwxr-xr-x 2 root root 0 Dec 10 13:18 cdev-by-name Ick! > drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device0 > drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device1 > drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device10 > drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device11 > drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device12 > drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device13 > drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device14 > drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device15 > drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device2 > drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device3 > drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device4 > drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device5 > drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device6 > drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device7 > drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device8 > drwxr-xr-x 3 root root 0 Dec 10 12:34 cooling_device9 > drwxr-xr-x 3 root root 0 Dec 10 12:34 thermal_zone0 > drwxr-xr-x 3 root root 0 Dec 10 12:34 thermal_zone1 > drwxr-xr-x 2 root root 0 Dec 10 13:18 tz-by-name > > ls -al /sys/devices/virtual/thermal/cdev-by-name/ > total 0 > drwxr-xr-x 2 root root 0 Dec 10 13:18 . > drwxr-xr-x 22 root root 0 Dec 10 12:34 .. > lrwxrwxrwx 1 root root 0 Dec 10 13:18 thermal-cpufreq-0 -> > ../cooling_device0 Ick ick ick! What is this all for? Where is the Documentation/ABI/ entry trying to explain this mess? Please provide that and we can go from there as I have no idea what this is trying to "help with". thanks, greg k-h ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 0/3] thermal: introduce by-name softlink 2019-12-10 20:54 ` Daniel Lezcano 2019-12-11 8:53 ` Greg Kroah-Hartman @ 2019-12-11 8:54 ` Greg Kroah-Hartman 2019-12-11 20:11 ` Wei Wang 1 sibling, 1 reply; 22+ messages in thread From: Greg Kroah-Hartman @ 2019-12-11 8:54 UTC (permalink / raw) To: Daniel Lezcano Cc: Wei Wang, Wei Wang, Zhang Rui, Eduardo Valentin, Amit Kucheria, Linux PM list, LKML, Rafael J. Wysocki On Tue, Dec 10, 2019 at 09:54:11PM +0100, Daniel Lezcano wrote: > On 10/12/2019 21:01, Wei Wang wrote: > > On Tue, Dec 10, 2019 at 6:36 AM Daniel Lezcano > > <daniel.lezcano@linaro.org> wrote: > >> > >> On 05/12/2019 08:19, Wei Wang wrote: > >>> The paths thermal_zone%d and cooling_device%d are not intuitive and the > >>> numbers are subject to change due to device tree change. This usually > >>> leads to tree traversal in userspace code. tree traversal is supposed to be done in userspace code :) But what userspace code needs to do this, and for what? thanks, greg k-h ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 0/3] thermal: introduce by-name softlink 2019-12-11 8:54 ` Greg Kroah-Hartman @ 2019-12-11 20:11 ` Wei Wang 2019-12-11 21:11 ` Daniel Lezcano 2019-12-12 7:34 ` Greg Kroah-Hartman 0 siblings, 2 replies; 22+ messages in thread From: Wei Wang @ 2019-12-11 20:11 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Daniel Lezcano, Wei Wang, Zhang Rui, Eduardo Valentin, Amit Kucheria, Linux PM list, LKML, Rafael J. Wysocki On Wed, Dec 11, 2019 at 12:54 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, Dec 10, 2019 at 09:54:11PM +0100, Daniel Lezcano wrote: > > On 10/12/2019 21:01, Wei Wang wrote: > > > On Tue, Dec 10, 2019 at 6:36 AM Daniel Lezcano > > > <daniel.lezcano@linaro.org> wrote: > > >> > > >> On 05/12/2019 08:19, Wei Wang wrote: > > >>> The paths thermal_zone%d and cooling_device%d are not intuitive and the > > >>> numbers are subject to change due to device tree change. This usually > > >>> leads to tree traversal in userspace code. > > tree traversal is supposed to be done in userspace code :) > Yes, that can be done in userspace, but given the amount of thermal zones we have in some mobile devices, this will bring a lot of convenience. e.g. this is on Pixel 4 XL: coral:/ # ls /sys/devices/virtual/thermal/ cdev-by-name cooling_device15 cooling_device22 cooling_device3 cooling_device9 thermal_zone15 thermal_zone22 thermal_zone3 thermal_zone37 thermal_zone44 thermal_zone51 thermal_zone59 thermal_zone66 thermal_zone73 thermal_zone80 thermal_zone88 cooling_device0 cooling_device16 cooling_device23 cooling_device30 thermal_zone0 thermal_zone16 thermal_zone23 thermal_zone30 thermal_zone38 thermal_zone45 thermal_zone52 thermal_zone6 thermal_zone67 thermal_zone74 thermal_zone81 thermal_zone9 cooling_device1 cooling_device17 cooling_device24 cooling_device31 thermal_zone1 thermal_zone17 thermal_zone24 thermal_zone31 thermal_zone39 thermal_zone46 thermal_zone53 thermal_zone60 thermal_zone68 thermal_zone75 thermal_zone82 tz-by-name cooling_device10 cooling_device18 cooling_device25 cooling_device4 thermal_zone10 thermal_zone18 thermal_zone25 thermal_zone32 thermal_zone4 thermal_zone47 thermal_zone54 thermal_zone61 thermal_zone69 thermal_zone76 thermal_zone83 cooling_device11 cooling_device19 cooling_device26 cooling_device5 thermal_zone11 thermal_zone19 thermal_zone26 thermal_zone33 thermal_zone40 thermal_zone48 thermal_zone55 thermal_zone62 thermal_zone7 thermal_zone77 thermal_zone84 cooling_device12 cooling_device2 cooling_device27 cooling_device6 thermal_zone12 thermal_zone2 thermal_zone27 thermal_zone34 thermal_zone41 thermal_zone49 thermal_zone56 thermal_zone63 thermal_zone70 thermal_zone78 thermal_zone85 cooling_device13 cooling_device20 cooling_device28 cooling_device7 thermal_zone13 thermal_zone20 thermal_zone28 thermal_zone35 thermal_zone42 thermal_zone5 thermal_zone57 thermal_zone64 thermal_zone71 thermal_zone79 thermal_zone86 cooling_device14 cooling_device21 cooling_device29 cooling_device8 thermal_zone14 thermal_zone21 thermal_zone29 thermal_zone36 thermal_zone43 thermal_zone50 thermal_zone58 thermal_zone65 thermal_zone72 thermal_zone8 thermal_zone87 > But what userspace code needs to do this, and for what? In Android, thermal daemon and thermal HAL as well as some init.rc script would use those thermal paths for managing and monitoring thermal. The daemon/HAL could have logic pipled in, however Android's init.rc script would be really tricky. On a related note, we also create /dev/block/by-name links from userspace. Thanks! -Wei > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 0/3] thermal: introduce by-name softlink 2019-12-11 20:11 ` Wei Wang @ 2019-12-11 21:11 ` Daniel Lezcano 2019-12-11 22:19 ` Wei Wang 2019-12-12 7:34 ` Greg Kroah-Hartman 1 sibling, 1 reply; 22+ messages in thread From: Daniel Lezcano @ 2019-12-11 21:11 UTC (permalink / raw) To: Wei Wang, Greg Kroah-Hartman Cc: Wei Wang, Zhang Rui, Eduardo Valentin, Amit Kucheria, Linux PM list, LKML, Rafael J. Wysocki On 11/12/2019 21:11, Wei Wang wrote: > On Wed, Dec 11, 2019 at 12:54 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: >> >> On Tue, Dec 10, 2019 at 09:54:11PM +0100, Daniel Lezcano wrote: >>> On 10/12/2019 21:01, Wei Wang wrote: >>>> On Tue, Dec 10, 2019 at 6:36 AM Daniel Lezcano >>>> <daniel.lezcano@linaro.org> wrote: >>>>> >>>>> On 05/12/2019 08:19, Wei Wang wrote: >>>>>> The paths thermal_zone%d and cooling_device%d are not intuitive and the >>>>>> numbers are subject to change due to device tree change. This usually >>>>>> leads to tree traversal in userspace code. >> >> tree traversal is supposed to be done in userspace code :) >> > Yes, that can be done in userspace, but given the amount of thermal > zones we have in some mobile devices, this will bring a lot of > convenience. But usually all these thermal zones are fixed and building the name<->tz association is just a question of a few lines of code from userspace, no? What would be the benefit of adding more ABI? If there is a thermal zone change, then a notification can be used, so the userspace code rebuild the name<->tz, no? > e.g. this is on Pixel 4 XL: > coral:/ # ls /sys/devices/virtual/thermal/ > cdev-by-name cooling_device15 cooling_device22 cooling_device3 > cooling_device9 thermal_zone15 thermal_zone22 thermal_zone3 > thermal_zone37 thermal_zone44 thermal_zone51 thermal_zone59 > thermal_zone66 thermal_zone73 thermal_zone80 thermal_zone88 > cooling_device0 cooling_device16 cooling_device23 cooling_device30 > thermal_zone0 thermal_zone16 thermal_zone23 thermal_zone30 > thermal_zone38 thermal_zone45 thermal_zone52 thermal_zone6 > thermal_zone67 thermal_zone74 thermal_zone81 thermal_zone9 > cooling_device1 cooling_device17 cooling_device24 cooling_device31 > thermal_zone1 thermal_zone17 thermal_zone24 thermal_zone31 > thermal_zone39 thermal_zone46 thermal_zone53 thermal_zone60 > thermal_zone68 thermal_zone75 thermal_zone82 tz-by-name > cooling_device10 cooling_device18 cooling_device25 cooling_device4 > thermal_zone10 thermal_zone18 thermal_zone25 thermal_zone32 > thermal_zone4 thermal_zone47 thermal_zone54 thermal_zone61 > thermal_zone69 thermal_zone76 thermal_zone83 > cooling_device11 cooling_device19 cooling_device26 cooling_device5 > thermal_zone11 thermal_zone19 thermal_zone26 thermal_zone33 > thermal_zone40 thermal_zone48 thermal_zone55 thermal_zone62 > thermal_zone7 thermal_zone77 thermal_zone84 > cooling_device12 cooling_device2 cooling_device27 cooling_device6 > thermal_zone12 thermal_zone2 thermal_zone27 thermal_zone34 > thermal_zone41 thermal_zone49 thermal_zone56 thermal_zone63 > thermal_zone70 thermal_zone78 thermal_zone85 > cooling_device13 cooling_device20 cooling_device28 cooling_device7 > thermal_zone13 thermal_zone20 thermal_zone28 thermal_zone35 > thermal_zone42 thermal_zone5 thermal_zone57 thermal_zone64 > thermal_zone71 thermal_zone79 thermal_zone86 > cooling_device14 cooling_device21 cooling_device29 cooling_device8 > thermal_zone14 thermal_zone21 thermal_zone29 thermal_zone36 > thermal_zone43 thermal_zone50 thermal_zone58 thermal_zone65 > thermal_zone72 thermal_zone8 thermal_zone87 > > >> But what userspace code needs to do this, and for what? > In Android, thermal daemon and thermal HAL as well as some init.rc > script would use those thermal paths for managing and monitoring > thermal. The daemon/HAL could have logic pipled in, however Android's > init.rc script would be really tricky. > On a related note, we also create /dev/block/by-name links from userspace. > > Thanks! > -Wei >> >> thanks, >> >> greg k-h -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 0/3] thermal: introduce by-name softlink 2019-12-11 21:11 ` Daniel Lezcano @ 2019-12-11 22:19 ` Wei Wang 2019-12-15 16:34 ` Sandeep Patil 0 siblings, 1 reply; 22+ messages in thread From: Wei Wang @ 2019-12-11 22:19 UTC (permalink / raw) Cc: Linux PM list, LKML On Wed, Dec 11, 2019 at 1:11 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 11/12/2019 21:11, Wei Wang wrote: > > On Wed, Dec 11, 2019 at 12:54 AM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > >> > >> On Tue, Dec 10, 2019 at 09:54:11PM +0100, Daniel Lezcano wrote: > >>> On 10/12/2019 21:01, Wei Wang wrote: > >>>> On Tue, Dec 10, 2019 at 6:36 AM Daniel Lezcano > >>>> <daniel.lezcano@linaro.org> wrote: > >>>>> > >>>>> On 05/12/2019 08:19, Wei Wang wrote: > >>>>>> The paths thermal_zone%d and cooling_device%d are not intuitive and the > >>>>>> numbers are subject to change due to device tree change. This usually > >>>>>> leads to tree traversal in userspace code. > >> > >> tree traversal is supposed to be done in userspace code :) > >> > > Yes, that can be done in userspace, but given the amount of thermal > > zones we have in some mobile devices, this will bring a lot of > > convenience. > > But usually all these thermal zones are fixed and building the name<->tz > association is just a question of a few lines of code from userspace, > no? What would be the benefit of adding more ABI? code reuse as tz mapping is per device tree setup and we try to use the same _type_ in thermal zone in shared client code for Android thermal API. > > If there is a thermal zone change, then a notification can be used, so > the userspace code rebuild the name<->tz, no? Assuming you meant for type<->tz, this mapping won't change in runtime in our case (not unloading things), but it could be changed due to device tree change, e.g. adding/removing thermal zones. > > > e.g. this is on Pixel 4 XL: > > coral:/ # ls /sys/devices/virtual/thermal/ > > cdev-by-name cooling_device15 cooling_device22 cooling_device3 > > cooling_device9 thermal_zone15 thermal_zone22 thermal_zone3 > > thermal_zone37 thermal_zone44 thermal_zone51 thermal_zone59 > > thermal_zone66 thermal_zone73 thermal_zone80 thermal_zone88 > > cooling_device0 cooling_device16 cooling_device23 cooling_device30 > > thermal_zone0 thermal_zone16 thermal_zone23 thermal_zone30 > > thermal_zone38 thermal_zone45 thermal_zone52 thermal_zone6 > > thermal_zone67 thermal_zone74 thermal_zone81 thermal_zone9 > > cooling_device1 cooling_device17 cooling_device24 cooling_device31 > > thermal_zone1 thermal_zone17 thermal_zone24 thermal_zone31 > > thermal_zone39 thermal_zone46 thermal_zone53 thermal_zone60 > > thermal_zone68 thermal_zone75 thermal_zone82 tz-by-name > > cooling_device10 cooling_device18 cooling_device25 cooling_device4 > > thermal_zone10 thermal_zone18 thermal_zone25 thermal_zone32 > > thermal_zone4 thermal_zone47 thermal_zone54 thermal_zone61 > > thermal_zone69 thermal_zone76 thermal_zone83 > > cooling_device11 cooling_device19 cooling_device26 cooling_device5 > > thermal_zone11 thermal_zone19 thermal_zone26 thermal_zone33 > > thermal_zone40 thermal_zone48 thermal_zone55 thermal_zone62 > > thermal_zone7 thermal_zone77 thermal_zone84 > > cooling_device12 cooling_device2 cooling_device27 cooling_device6 > > thermal_zone12 thermal_zone2 thermal_zone27 thermal_zone34 > > thermal_zone41 thermal_zone49 thermal_zone56 thermal_zone63 > > thermal_zone70 thermal_zone78 thermal_zone85 > > cooling_device13 cooling_device20 cooling_device28 cooling_device7 > > thermal_zone13 thermal_zone20 thermal_zone28 thermal_zone35 > > thermal_zone42 thermal_zone5 thermal_zone57 thermal_zone64 > > thermal_zone71 thermal_zone79 thermal_zone86 > > cooling_device14 cooling_device21 cooling_device29 cooling_device8 > > thermal_zone14 thermal_zone21 thermal_zone29 thermal_zone36 > > thermal_zone43 thermal_zone50 thermal_zone58 thermal_zone65 > > thermal_zone72 thermal_zone8 thermal_zone87 > > > > > >> But what userspace code needs to do this, and for what? > > In Android, thermal daemon and thermal HAL as well as some init.rc > > script would use those thermal paths for managing and monitoring > > thermal. The daemon/HAL could have logic pipled in, however Android's > > init.rc script would be really tricky. > > On a related note, we also create /dev/block/by-name links from userspace. > > > > Thanks! > > -Wei > >> > >> thanks, > >> > >> greg k-h > > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 0/3] thermal: introduce by-name softlink 2019-12-11 22:19 ` Wei Wang @ 2019-12-15 16:34 ` Sandeep Patil 2019-12-16 17:37 ` Wei Wang 0 siblings, 1 reply; 22+ messages in thread From: Sandeep Patil @ 2019-12-15 16:34 UTC (permalink / raw) To: Wei Wang Cc: linux-pm, linux-kernel, gregkh, rui.zhang, edubezval, amit.kucheria, rjw, daniel.lezcano Hi Wei, (resending because I accidentally replied only to you. Also adding everyone else back in the thread, not sure what happened before). On Wed, Dec 11, 2019 at 02:19:03PM -0800, Wei Wang wrote: > On Wed, Dec 11, 2019 at 1:11 PM Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: > > > > On 11/12/2019 21:11, Wei Wang wrote: > > > On Wed, Dec 11, 2019 at 12:54 AM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > >> > > >> On Tue, Dec 10, 2019 at 09:54:11PM +0100, Daniel Lezcano wrote: > > >>> On 10/12/2019 21:01, Wei Wang wrote: > > >>>> On Tue, Dec 10, 2019 at 6:36 AM Daniel Lezcano > > >>>> <daniel.lezcano@linaro.org> wrote: > > >>>>> > > >>>>> On 05/12/2019 08:19, Wei Wang wrote: > > >>>>>> The paths thermal_zone%d and cooling_device%d are not intuitive and the > > >>>>>> numbers are subject to change due to device tree change. This usually > > >>>>>> leads to tree traversal in userspace code. > > >> > > >> tree traversal is supposed to be done in userspace code :) > > >> > > > Yes, that can be done in userspace, but given the amount of thermal > > > zones we have in some mobile devices, this will bring a lot of > > > convenience. > > > > But usually all these thermal zones are fixed and building the name<->tz > > association is just a question of a few lines of code from userspace, > > no? What would be the benefit of adding more ABI? > > > code reuse as tz mapping is per device tree setup and we try to use > the same _type_ in thermal zone in shared client code for Android > thermal API. CMIIW, but the tz mapping today can still be done by parsing through the directory the other way? (i.e. tz->type) instead of type->tz? Also, I understand the resistance to add a new ABI since it will have to be kept around forever. What would be worth knowing is if these mappings are static (built once) or you have to go through and parse the directories every time? AFAIU, the parsing (map creation) seems to only happen once during boot, so we (Android) should be ok? > > > > > If there is a thermal zone change, then a notification can be used, so > > the userspace code rebuild the name<->tz, no? > > > Assuming you meant for type<->tz, this mapping won't change in runtime > in our case (not unloading things), but it could be changed due to > device tree change, e.g. adding/removing thermal zones. I actually think we might need this after all. This assumes all tz devices are registered when userspace (thermal hal/daemon) is coming up. However, that may soon not be the case and we have to adjust for devices being added in parallel with userspace? So, may be we need to start monitoring uevents like we do for power supplies too? In that case, the mapping can also be done dynamically. Lastly, I think we can make a case further if there are benefits we can share here other than convenience. Like startup time, handing hardware variations etc? (Also point to the current userspace code that uses this) - ssp > > > > > > > e.g. this is on Pixel 4 XL: > > > coral:/ # ls /sys/devices/virtual/thermal/ > > > cdev-by-name cooling_device15 cooling_device22 cooling_device3 > > > cooling_device9 thermal_zone15 thermal_zone22 thermal_zone3 > > > thermal_zone37 thermal_zone44 thermal_zone51 thermal_zone59 > > > thermal_zone66 thermal_zone73 thermal_zone80 thermal_zone88 > > > cooling_device0 cooling_device16 cooling_device23 cooling_device30 > > > thermal_zone0 thermal_zone16 thermal_zone23 thermal_zone30 > > > thermal_zone38 thermal_zone45 thermal_zone52 thermal_zone6 > > > thermal_zone67 thermal_zone74 thermal_zone81 thermal_zone9 > > > cooling_device1 cooling_device17 cooling_device24 cooling_device31 > > > thermal_zone1 thermal_zone17 thermal_zone24 thermal_zone31 > > > thermal_zone39 thermal_zone46 thermal_zone53 thermal_zone60 > > > thermal_zone68 thermal_zone75 thermal_zone82 tz-by-name > > > cooling_device10 cooling_device18 cooling_device25 cooling_device4 > > > thermal_zone10 thermal_zone18 thermal_zone25 thermal_zone32 > > > thermal_zone4 thermal_zone47 thermal_zone54 thermal_zone61 > > > thermal_zone69 thermal_zone76 thermal_zone83 > > > cooling_device11 cooling_device19 cooling_device26 cooling_device5 > > > thermal_zone11 thermal_zone19 thermal_zone26 thermal_zone33 > > > thermal_zone40 thermal_zone48 thermal_zone55 thermal_zone62 > > > thermal_zone7 thermal_zone77 thermal_zone84 > > > cooling_device12 cooling_device2 cooling_device27 cooling_device6 > > > thermal_zone12 thermal_zone2 thermal_zone27 thermal_zone34 > > > thermal_zone41 thermal_zone49 thermal_zone56 thermal_zone63 > > > thermal_zone70 thermal_zone78 thermal_zone85 > > > cooling_device13 cooling_device20 cooling_device28 cooling_device7 > > > thermal_zone13 thermal_zone20 thermal_zone28 thermal_zone35 > > > thermal_zone42 thermal_zone5 thermal_zone57 thermal_zone64 > > > thermal_zone71 thermal_zone79 thermal_zone86 > > > cooling_device14 cooling_device21 cooling_device29 cooling_device8 > > > thermal_zone14 thermal_zone21 thermal_zone29 thermal_zone36 > > > thermal_zone43 thermal_zone50 thermal_zone58 thermal_zone65 > > > thermal_zone72 thermal_zone8 thermal_zone87 > > > > > > > > >> But what userspace code needs to do this, and for what? > > > In Android, thermal daemon and thermal HAL as well as some init.rc > > > script would use those thermal paths for managing and monitoring > > > thermal. The daemon/HAL could have logic pipled in, however Android's > > > init.rc script would be really tricky. > > > On a related note, we also create /dev/block/by-name links from userspace. > > > > > > Thanks! > > > -Wei > > >> > > >> thanks, > > >> > > >> greg k-h > > > > > > -- > > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > > <http://twitter.com/#!/linaroorg> Twitter | > > <http://www.linaro.org/linaro-blog/> Blog > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 0/3] thermal: introduce by-name softlink 2019-12-15 16:34 ` Sandeep Patil @ 2019-12-16 17:37 ` Wei Wang 0 siblings, 0 replies; 22+ messages in thread From: Wei Wang @ 2019-12-16 17:37 UTC (permalink / raw) To: Sandeep Patil Cc: Linux PM list, LKML, Greg Kroah-Hartman, Zhang Rui, Eduardo Valentin, Amit Kucheria, Rafael J. Wysocki, Daniel Lezcano On Sun, Dec 15, 2019 at 8:34 AM Sandeep Patil <sspatil@android.com> wrote: > > Hi Wei, > > (resending because I accidentally replied only to you. Also adding everyone > else back in the thread, not sure what happened before). > > On Wed, Dec 11, 2019 at 02:19:03PM -0800, Wei Wang wrote: > > On Wed, Dec 11, 2019 at 1:11 PM Daniel Lezcano > > <daniel.lezcano@linaro.org> wrote: > > > > > > On 11/12/2019 21:11, Wei Wang wrote: > > > > On Wed, Dec 11, 2019 at 12:54 AM Greg Kroah-Hartman > > > > <gregkh@linuxfoundation.org> wrote: > > > >> > > > >> On Tue, Dec 10, 2019 at 09:54:11PM +0100, Daniel Lezcano wrote: > > > >>> On 10/12/2019 21:01, Wei Wang wrote: > > > >>>> On Tue, Dec 10, 2019 at 6:36 AM Daniel Lezcano > > > >>>> <daniel.lezcano@linaro.org> wrote: > > > >>>>> > > > >>>>> On 05/12/2019 08:19, Wei Wang wrote: > > > >>>>>> The paths thermal_zone%d and cooling_device%d are not intuitive and the > > > >>>>>> numbers are subject to change due to device tree change. This usually > > > >>>>>> leads to tree traversal in userspace code. > > > >> > > > >> tree traversal is supposed to be done in userspace code :) > > > >> > > > > Yes, that can be done in userspace, but given the amount of thermal > > > > zones we have in some mobile devices, this will bring a lot of > > > > convenience. > > > > > > But usually all these thermal zones are fixed and building the name<->tz > > > association is just a question of a few lines of code from userspace, > > > no? What would be the benefit of adding more ABI? > > > > > > code reuse as tz mapping is per device tree setup and we try to use > > the same _type_ in thermal zone in shared client code for Android > > thermal API. > > CMIIW, but the tz mapping today can still be done by parsing through the > directory the other way? (i.e. tz->type) instead of type->tz? > Parsing can be done in clients, one particular place is for init, but still possible. https://cs.android.com/search?q=tz-by-name%20file:%5C.rc Yeah, as said, we can definitely do pasing in every userspace daemon, but AFAICT, all userspace codes (open source / closed source) are using thermal zones / cooling devices by "name/type". So I am probling the possibility of having this kind of support for convenience. FYI, Chrome also feel this is useful https://lore.kernel.org/lkml/20191206191545.GO228856@google.com/ > Also, I understand the resistance to add a new ABI since it will have to be > kept around forever. What would be worth knowing is if these mappings are > static (built once) or you have to go through and parse the directories every > time? AFAIU, the parsing (map creation) seems to only happen once during > boot, so we (Android) should be ok? > Those mapping are usually static nowadays, but could be changed if modules are unloaded. Right now we only do UEVENT for userspace governor, https://github.com/torvalds/linux/blob/9c7db5004280767566e91a33445bf93aa479ef02/drivers/thermal/user_space.c#L37 Actually, I am about sending another RFC to add some more uevent to userspace. Without the UEVENT, I think we cannot guarantee the mapping is static. > > > > > > > > If there is a thermal zone change, then a notification can be used, so > > > the userspace code rebuild the name<->tz, no? > > > > > > Assuming you meant for type<->tz, this mapping won't change in runtime > > in our case (not unloading things), but it could be changed due to > > device tree change, e.g. adding/removing thermal zones. > > I actually think we might need this after all. This assumes all tz devices > are registered when userspace (thermal hal/daemon) is coming up. However, > that may soon not be the case and we have to adjust for devices being added > in parallel with userspace? > > So, may be we need to start monitoring uevents like we do for power supplies > too? In that case, the mapping can also be done dynamically. > > Lastly, I think we can make a case further if there are benefits we can share > here other than convenience. Like startup time, handing hardware variations > etc? (Also point to the current userspace code that uses this) > I replied above before reading this part, and I guess we had the same thought on this. > - ssp > > > > > > > > > > > e.g. this is on Pixel 4 XL: > > > > coral:/ # ls /sys/devices/virtual/thermal/ > > > > cdev-by-name cooling_device15 cooling_device22 cooling_device3 > > > > cooling_device9 thermal_zone15 thermal_zone22 thermal_zone3 > > > > thermal_zone37 thermal_zone44 thermal_zone51 thermal_zone59 > > > > thermal_zone66 thermal_zone73 thermal_zone80 thermal_zone88 > > > > cooling_device0 cooling_device16 cooling_device23 cooling_device30 > > > > thermal_zone0 thermal_zone16 thermal_zone23 thermal_zone30 > > > > thermal_zone38 thermal_zone45 thermal_zone52 thermal_zone6 > > > > thermal_zone67 thermal_zone74 thermal_zone81 thermal_zone9 > > > > cooling_device1 cooling_device17 cooling_device24 cooling_device31 > > > > thermal_zone1 thermal_zone17 thermal_zone24 thermal_zone31 > > > > thermal_zone39 thermal_zone46 thermal_zone53 thermal_zone60 > > > > thermal_zone68 thermal_zone75 thermal_zone82 tz-by-name > > > > cooling_device10 cooling_device18 cooling_device25 cooling_device4 > > > > thermal_zone10 thermal_zone18 thermal_zone25 thermal_zone32 > > > > thermal_zone4 thermal_zone47 thermal_zone54 thermal_zone61 > > > > thermal_zone69 thermal_zone76 thermal_zone83 > > > > cooling_device11 cooling_device19 cooling_device26 cooling_device5 > > > > thermal_zone11 thermal_zone19 thermal_zone26 thermal_zone33 > > > > thermal_zone40 thermal_zone48 thermal_zone55 thermal_zone62 > > > > thermal_zone7 thermal_zone77 thermal_zone84 > > > > cooling_device12 cooling_device2 cooling_device27 cooling_device6 > > > > thermal_zone12 thermal_zone2 thermal_zone27 thermal_zone34 > > > > thermal_zone41 thermal_zone49 thermal_zone56 thermal_zone63 > > > > thermal_zone70 thermal_zone78 thermal_zone85 > > > > cooling_device13 cooling_device20 cooling_device28 cooling_device7 > > > > thermal_zone13 thermal_zone20 thermal_zone28 thermal_zone35 > > > > thermal_zone42 thermal_zone5 thermal_zone57 thermal_zone64 > > > > thermal_zone71 thermal_zone79 thermal_zone86 > > > > cooling_device14 cooling_device21 cooling_device29 cooling_device8 > > > > thermal_zone14 thermal_zone21 thermal_zone29 thermal_zone36 > > > > thermal_zone43 thermal_zone50 thermal_zone58 thermal_zone65 > > > > thermal_zone72 thermal_zone8 thermal_zone87 > > > > > > > > > > > >> But what userspace code needs to do this, and for what? > > > > In Android, thermal daemon and thermal HAL as well as some init.rc > > > > script would use those thermal paths for managing and monitoring > > > > thermal. The daemon/HAL could have logic pipled in, however Android's > > > > init.rc script would be really tricky. > > > > On a related note, we also create /dev/block/by-name links from userspace. > > > > > > > > Thanks! > > > > -Wei > > > >> > > > >> thanks, > > > >> > > > >> greg k-h > > > > > > > > > -- > > > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > > > > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > > > <http://twitter.com/#!/linaroorg> Twitter | > > > <http://www.linaro.org/linaro-blog/> Blog > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 0/3] thermal: introduce by-name softlink 2019-12-11 20:11 ` Wei Wang 2019-12-11 21:11 ` Daniel Lezcano @ 2019-12-12 7:34 ` Greg Kroah-Hartman 1 sibling, 0 replies; 22+ messages in thread From: Greg Kroah-Hartman @ 2019-12-12 7:34 UTC (permalink / raw) To: Wei Wang Cc: Daniel Lezcano, Wei Wang, Zhang Rui, Eduardo Valentin, Amit Kucheria, Linux PM list, LKML, Rafael J. Wysocki On Wed, Dec 11, 2019 at 12:11:53PM -0800, Wei Wang wrote: > On Wed, Dec 11, 2019 at 12:54 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Tue, Dec 10, 2019 at 09:54:11PM +0100, Daniel Lezcano wrote: > > > On 10/12/2019 21:01, Wei Wang wrote: > > > > On Tue, Dec 10, 2019 at 6:36 AM Daniel Lezcano > > > > <daniel.lezcano@linaro.org> wrote: > > > >> > > > >> On 05/12/2019 08:19, Wei Wang wrote: > > > >>> The paths thermal_zone%d and cooling_device%d are not intuitive and the > > > >>> numbers are subject to change due to device tree change. This usually > > > >>> leads to tree traversal in userspace code. > > > > tree traversal is supposed to be done in userspace code :) > > > Yes, that can be done in userspace, but given the amount of thermal > zones we have in some mobile devices, this will bring a lot of > convenience. Userspace is easier to write than kernel code. Kernel code and apis you have to retain for the next 20+ years, are you willing to do that? > e.g. this is on Pixel 4 XL: > coral:/ # ls /sys/devices/virtual/thermal/ > cdev-by-name cooling_device15 cooling_device22 cooling_device3 > cooling_device9 thermal_zone15 thermal_zone22 thermal_zone3 > thermal_zone37 thermal_zone44 thermal_zone51 thermal_zone59 > thermal_zone66 thermal_zone73 thermal_zone80 thermal_zone88 > cooling_device0 cooling_device16 cooling_device23 cooling_device30 > thermal_zone0 thermal_zone16 thermal_zone23 thermal_zone30 > thermal_zone38 thermal_zone45 thermal_zone52 thermal_zone6 > thermal_zone67 thermal_zone74 thermal_zone81 thermal_zone9 > cooling_device1 cooling_device17 cooling_device24 cooling_device31 > thermal_zone1 thermal_zone17 thermal_zone24 thermal_zone31 > thermal_zone39 thermal_zone46 thermal_zone53 thermal_zone60 > thermal_zone68 thermal_zone75 thermal_zone82 tz-by-name > cooling_device10 cooling_device18 cooling_device25 cooling_device4 > thermal_zone10 thermal_zone18 thermal_zone25 thermal_zone32 > thermal_zone4 thermal_zone47 thermal_zone54 thermal_zone61 > thermal_zone69 thermal_zone76 thermal_zone83 > cooling_device11 cooling_device19 cooling_device26 cooling_device5 > thermal_zone11 thermal_zone19 thermal_zone26 thermal_zone33 > thermal_zone40 thermal_zone48 thermal_zone55 thermal_zone62 > thermal_zone7 thermal_zone77 thermal_zone84 > cooling_device12 cooling_device2 cooling_device27 cooling_device6 > thermal_zone12 thermal_zone2 thermal_zone27 thermal_zone34 > thermal_zone41 thermal_zone49 thermal_zone56 thermal_zone63 > thermal_zone70 thermal_zone78 thermal_zone85 > cooling_device13 cooling_device20 cooling_device28 cooling_device7 > thermal_zone13 thermal_zone20 thermal_zone28 thermal_zone35 > thermal_zone42 thermal_zone5 thermal_zone57 thermal_zone64 > thermal_zone71 thermal_zone79 thermal_zone86 > cooling_device14 cooling_device21 cooling_device29 cooling_device8 > thermal_zone14 thermal_zone21 thermal_zone29 thermal_zone36 > thermal_zone43 thermal_zone50 thermal_zone58 thermal_zone65 > thermal_zone72 thermal_zone8 thermal_zone87 Nice, you have a crazy SoC here, good luck! :) > > But what userspace code needs to do this, and for what? > In Android, thermal daemon and thermal HAL as well as some init.rc > script would use those thermal paths for managing and monitoring > thermal. The daemon/HAL could have logic pipled in, however Android's > init.rc script would be really tricky. Then put that logic in your userspace code. > On a related note, we also create /dev/block/by-name links from userspace. Yes we do, from userspace, not from within the kernel itself, so I think you have answered the question :) thanks, greg k-h ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-12-16 17:38 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-05 7:19 [PATCH v3 0/3] thermal: introduce by-name softlink Wei Wang 2019-12-05 7:19 ` [PATCH v3 1/3] thermal: prevent cooling device with no type to be registered Wei Wang 2019-12-09 7:41 ` Amit Kucheria 2019-12-09 17:34 ` Wei Wang 2019-12-09 19:36 ` Daniel Lezcano 2019-12-09 21:13 ` Wei Wang 2019-12-05 7:19 ` [PATCH v3 2/3] thermal: improve error message in thermal zone registration Wei Wang 2019-12-09 7:41 ` Amit Kucheria 2019-12-05 7:19 ` [PATCH v3 3/3] thermal: create softlink by name for thermal_zone and cooling_device Wei Wang 2019-12-06 19:15 ` Matthias Kaehlcke 2019-12-09 7:42 ` Amit Kucheria 2019-12-10 14:36 ` [PATCH v3 0/3] thermal: introduce by-name softlink Daniel Lezcano 2019-12-10 20:01 ` Wei Wang 2019-12-10 20:54 ` Daniel Lezcano 2019-12-11 8:53 ` Greg Kroah-Hartman 2019-12-11 8:54 ` Greg Kroah-Hartman 2019-12-11 20:11 ` Wei Wang 2019-12-11 21:11 ` Daniel Lezcano 2019-12-11 22:19 ` Wei Wang 2019-12-15 16:34 ` Sandeep Patil 2019-12-16 17:37 ` Wei Wang 2019-12-12 7:34 ` Greg Kroah-Hartman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).