| Message ID | 20191204215618.125826-2-wvw@google.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Zhang Rui |
| Headers | show |
| Series | thermal: introduce by-name softlink | expand |
Hi Wei, On Thu, Dec 5, 2019 at 3:26 AM Wei Wang <wvw@google.com> wrote: > > Make cooling device registration behavior consistent with Consistent how? Please add details. > thermal zone. This patch also cleans up a unnecessary > nullptr check. > > Signed-off-by: Wei Wang <wvw@google.com> > --- > drivers/thermal/thermal_core.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index d4481cc8958f..64fbb59c2f44 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -954,8 +954,16 @@ __thermal_cooling_device_register(struct device_node *np, > struct thermal_zone_device *pos = NULL; > int result; > > - if (type && strlen(type) >= THERMAL_NAME_LENGTH) > - return ERR_PTR(-EINVAL); > + 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 (%s) too long, " > + "should be under %d chars\n", type, THERMAL_NAME_LENGTH); Consider fitting into a single greppable string as "Error: Cooling device name over %d chars: %s\n" > + return ERR_PTR(-EINVAL); > + } > > if (!ops || !ops->get_max_state || !ops->get_cur_state || > !ops->set_cur_state) > @@ -972,7 +980,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; > @@ -1250,7 +1258,7 @@ thermal_zone_device_register(const char *type, int trips, int mask, > return ERR_PTR(-EINVAL); > } > > - if (type && strlen(type) >= THERMAL_NAME_LENGTH) { > + if (strlen(type) >= THERMAL_NAME_LENGTH) { > pr_err("Error: Thermal zone name (%s) too long, should be under %d chars\n", > type, THERMAL_NAME_LENGTH); > return ERR_PTR(-EINVAL); > -- > 2.24.0.393.g34dc348eaf-goog >
On Wed, Dec 4, 2019 at 8:13 PM Amit Kucheria <amit.kucheria@verdurent.com> wrote: > > Hi Wei, > > On Thu, Dec 5, 2019 at 3:26 AM Wei Wang <wvw@google.com> wrote: > > > > Make cooling device registration behavior consistent with > > Consistent how? Please add details. > Consistent with https://lore.kernel.org/linux-pm/1478581767-7009-2-git-send-email-edubezval@gmail.com/ will include aboce in next version. > > thermal zone. This patch also cleans up a unnecessary > > nullptr check. > > > > Signed-off-by: Wei Wang <wvw@google.com> > > --- > > drivers/thermal/thermal_core.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > > index d4481cc8958f..64fbb59c2f44 100644 > > --- a/drivers/thermal/thermal_core.c > > +++ b/drivers/thermal/thermal_core.c > > @@ -954,8 +954,16 @@ __thermal_cooling_device_register(struct device_node *np, > > struct thermal_zone_device *pos = NULL; > > int result; > > > > - if (type && strlen(type) >= THERMAL_NAME_LENGTH) > > - return ERR_PTR(-EINVAL); > > + 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 (%s) too long, " > > + "should be under %d chars\n", type, THERMAL_NAME_LENGTH); > > Consider fitting into a single greppable string as "Error: Cooling > device name over %d chars: %s\n" > Was intentionally keep it the same as this https://lore.kernel.org/linux-pm/31a29628894a14e716fff113fd9ce945fe649c05.1562876950.git.amit.kucheria@linaro.org/ Will make it shorter in both places next verion > > + return ERR_PTR(-EINVAL); > > + } > > > > if (!ops || !ops->get_max_state || !ops->get_cur_state || > > !ops->set_cur_state) > > @@ -972,7 +980,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; > > @@ -1250,7 +1258,7 @@ thermal_zone_device_register(const char *type, int trips, int mask, > > return ERR_PTR(-EINVAL); > > } > > > > - if (type && strlen(type) >= THERMAL_NAME_LENGTH) { > > + if (strlen(type) >= THERMAL_NAME_LENGTH) { > > pr_err("Error: Thermal zone name (%s) too long, should be under %d chars\n", > > type, THERMAL_NAME_LENGTH); > > return ERR_PTR(-EINVAL); > > -- > > 2.24.0.393.g34dc348eaf-goog > >
On Thu, Dec 5, 2019 at 11:44 AM Wei Wang <wvw@google.com> wrote: > > On Wed, Dec 4, 2019 at 8:13 PM Amit Kucheria > <amit.kucheria@verdurent.com> wrote: > > > > Hi Wei, > > > > On Thu, Dec 5, 2019 at 3:26 AM Wei Wang <wvw@google.com> wrote: > > > > > > Make cooling device registration behavior consistent with > > > > Consistent how? Please add details. > > > Consistent with > https://lore.kernel.org/linux-pm/1478581767-7009-2-git-send-email-edubezval@gmail.com/ > > will include aboce in next version. Thanks. > > > > thermal zone. This patch also cleans up a unnecessary > > > nullptr check. > > > > > > Signed-off-by: Wei Wang <wvw@google.com> > > > --- > > > drivers/thermal/thermal_core.c | 16 ++++++++++++---- > > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > > > index d4481cc8958f..64fbb59c2f44 100644 > > > --- a/drivers/thermal/thermal_core.c > > > +++ b/drivers/thermal/thermal_core.c > > > @@ -954,8 +954,16 @@ __thermal_cooling_device_register(struct device_node *np, > > > struct thermal_zone_device *pos = NULL; > > > int result; > > > > > > - if (type && strlen(type) >= THERMAL_NAME_LENGTH) > > > - return ERR_PTR(-EINVAL); > > > + 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 (%s) too long, " > > > + "should be under %d chars\n", type, THERMAL_NAME_LENGTH); > > > > Consider fitting into a single greppable string as "Error: Cooling > > device name over %d chars: %s\n" > > > Was intentionally keep it the same as this > https://lore.kernel.org/linux-pm/31a29628894a14e716fff113fd9ce945fe649c05.1562876950.git.amit.kucheria@linaro.org/ > Will make it shorter in both places next verion Yes please, make it a separate patch. We didn't catch it during review. > > > > + return ERR_PTR(-EINVAL); > > > + } > > > > > > if (!ops || !ops->get_max_state || !ops->get_cur_state || > > > !ops->set_cur_state) > > > @@ -972,7 +980,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; > > > @@ -1250,7 +1258,7 @@ thermal_zone_device_register(const char *type, int trips, int mask, > > > return ERR_PTR(-EINVAL); > > > } > > > > > > - if (type && strlen(type) >= THERMAL_NAME_LENGTH) { > > > + if (strlen(type) >= THERMAL_NAME_LENGTH) { > > > pr_err("Error: Thermal zone name (%s) too long, should be under %d chars\n", > > > type, THERMAL_NAME_LENGTH); > > > return ERR_PTR(-EINVAL); > > > -- > > > 2.24.0.393.g34dc348eaf-goog > > >
On Thu, Dec 5, 2019 at 11:56 AM Amit Kucheria <amit.kucheria@verdurent.com> wrote: > > On Thu, Dec 5, 2019 at 11:44 AM Wei Wang <wvw@google.com> wrote: > > > > On Wed, Dec 4, 2019 at 8:13 PM Amit Kucheria > > <amit.kucheria@verdurent.com> wrote: > > > > > > Hi Wei, > > > > > > On Thu, Dec 5, 2019 at 3:26 AM Wei Wang <wvw@google.com> wrote: > > > > > > > > Make cooling device registration behavior consistent with > > > > > > Consistent how? Please add details. > > > > > Consistent with > > https://lore.kernel.org/linux-pm/1478581767-7009-2-git-send-email-edubezval@gmail.com/ Studying this a bit more, git blame pointed to this SHA[1] that fixed it so that NULL value for 'type' is allowed, we just check for it. However, none of the users of thermal_cooling_device_register() seem to pass NULL. Rui, any insight into the history of why we would NOT want to create a sysfs attribute by passing NULL? Do we still need to allow for NULL values or should we cleanup the API to prevent NULL values? [1] 204dd1d39c32f39a95 > > > > will include aboce in next version. > > Thanks. > > > > > > > thermal zone. This patch also cleans up a unnecessary > > > > nullptr check. > > > > > > > > Signed-off-by: Wei Wang <wvw@google.com> > > > > --- > > > > drivers/thermal/thermal_core.c | 16 ++++++++++++---- > > > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > > > > index d4481cc8958f..64fbb59c2f44 100644 > > > > --- a/drivers/thermal/thermal_core.c > > > > +++ b/drivers/thermal/thermal_core.c > > > > @@ -954,8 +954,16 @@ __thermal_cooling_device_register(struct device_node *np, > > > > struct thermal_zone_device *pos = NULL; > > > > int result; > > > > > > > > - if (type && strlen(type) >= THERMAL_NAME_LENGTH) > > > > - return ERR_PTR(-EINVAL); > > > > + 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 (%s) too long, " > > > > + "should be under %d chars\n", type, THERMAL_NAME_LENGTH); > > > > > > Consider fitting into a single greppable string as "Error: Cooling > > > device name over %d chars: %s\n" > > > > > Was intentionally keep it the same as this > > https://lore.kernel.org/linux-pm/31a29628894a14e716fff113fd9ce945fe649c05.1562876950.git.amit.kucheria@linaro.org/ > > Will make it shorter in both places next verion > > Yes please, make it a separate patch. We didn't catch it during review. > > > > > > > + return ERR_PTR(-EINVAL); > > > > + } > > > > > > > > if (!ops || !ops->get_max_state || !ops->get_cur_state || > > > > !ops->set_cur_state) > > > > @@ -972,7 +980,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; > > > > @@ -1250,7 +1258,7 @@ thermal_zone_device_register(const char *type, int trips, int mask, > > > > return ERR_PTR(-EINVAL); > > > > } > > > > > > > > - if (type && strlen(type) >= THERMAL_NAME_LENGTH) { > > > > + if (strlen(type) >= THERMAL_NAME_LENGTH) { > > > > pr_err("Error: Thermal zone name (%s) too long, should be under %d chars\n", > > > > type, THERMAL_NAME_LENGTH); > > > > return ERR_PTR(-EINVAL); > > > > -- > > > > 2.24.0.393.g34dc348eaf-goog > > > >
On Thu, 2019-12-05 at 12:36 +0530, Amit Kucheria wrote: > On Thu, Dec 5, 2019 at 11:56 AM Amit Kucheria > <amit.kucheria@verdurent.com> wrote: > > > > On Thu, Dec 5, 2019 at 11:44 AM Wei Wang <wvw@google.com> wrote: > > > > > > On Wed, Dec 4, 2019 at 8:13 PM Amit Kucheria > > > <amit.kucheria@verdurent.com> wrote: > > > > > > > > Hi Wei, > > > > > > > > On Thu, Dec 5, 2019 at 3:26 AM Wei Wang <wvw@google.com> wrote: > > > > > > > > > > Make cooling device registration behavior consistent with > > > > > > > > Consistent how? Please add details. > > > > > > > > > > Consistent with > > > https://lore.kernel.org/linux-pm/1478581767-7009-2-git-send-email-edubezval@gmail.com/ > > Studying this a bit more, git blame pointed to this SHA[1] that fixed > it so that NULL value for 'type' is allowed, we just check for it. > However, none of the users of thermal_cooling_device_register() seem > to pass NULL. > > Rui, any insight into the history of why we would NOT want to create > a > sysfs attribute by passing NULL? Actually, I don't recall there is any requirement that wants to register a cooling_device without "type". > Do we still need to allow for NULL > values or should we cleanup the API to prevent NULL values? > well, my suggestion is to make this (do NULL check) a separate patch and see if we have any complains, if yes, we can revert it easily. thanks, rui > [1] 204dd1d39c32f39a95 > > > > > > > > will include aboce in next version. > > > > Thanks. > > > > > > > > > > thermal zone. This patch also cleans up a unnecessary > > > > > nullptr check. > > > > > > > > > > Signed-off-by: Wei Wang <wvw@google.com> > > > > > --- > > > > > drivers/thermal/thermal_core.c | 16 ++++++++++++---- > > > > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/thermal/thermal_core.c > > > > > b/drivers/thermal/thermal_core.c > > > > > index d4481cc8958f..64fbb59c2f44 100644 > > > > > --- a/drivers/thermal/thermal_core.c > > > > > +++ b/drivers/thermal/thermal_core.c > > > > > @@ -954,8 +954,16 @@ __thermal_cooling_device_register(struct > > > > > device_node *np, > > > > > struct thermal_zone_device *pos = NULL; > > > > > int result; > > > > > > > > > > - if (type && strlen(type) >= THERMAL_NAME_LENGTH) > > > > > - return ERR_PTR(-EINVAL); > > > > > + 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 (%s) too long, > > > > > " > > > > > + "should be under %d chars\n", type, > > > > > THERMAL_NAME_LENGTH); > > > > > > > > Consider fitting into a single greppable string as "Error: > > > > Cooling > > > > device name over %d chars: %s\n" > > > > > > > > > > Was intentionally keep it the same as this > > > https://lore.kernel.org/linux-pm/31a29628894a14e716fff113fd9ce945fe649c05.1562876950.git.amit.kucheria@linaro.org/ > > > Will make it shorter in both places next verion > > > > Yes please, make it a separate patch. We didn't catch it during > > review. > > > > > > > > > > + return ERR_PTR(-EINVAL); > > > > > + } > > > > > > > > > > if (!ops || !ops->get_max_state || !ops- > > > > > >get_cur_state || > > > > > !ops->set_cur_state) > > > > > @@ -972,7 +980,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; > > > > > @@ -1250,7 +1258,7 @@ thermal_zone_device_register(const char > > > > > *type, int trips, int mask, > > > > > return ERR_PTR(-EINVAL); > > > > > } > > > > > > > > > > - if (type && strlen(type) >= THERMAL_NAME_LENGTH) { > > > > > + if (strlen(type) >= THERMAL_NAME_LENGTH) { > > > > > pr_err("Error: Thermal zone name (%s) too > > > > > long, should be under %d chars\n", > > > > > type, THERMAL_NAME_LENGTH); > > > > > return ERR_PTR(-EINVAL); > > > > > -- > > > > > 2.24.0.393.g34dc348eaf-goog > > > > >
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index d4481cc8958f..64fbb59c2f44 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -954,8 +954,16 @@ __thermal_cooling_device_register(struct device_node *np, struct thermal_zone_device *pos = NULL; int result; - if (type && strlen(type) >= THERMAL_NAME_LENGTH) - return ERR_PTR(-EINVAL); + 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 (%s) too long, " + "should be under %d chars\n", type, THERMAL_NAME_LENGTH); + return ERR_PTR(-EINVAL); + } if (!ops || !ops->get_max_state || !ops->get_cur_state || !ops->set_cur_state) @@ -972,7 +980,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; @@ -1250,7 +1258,7 @@ thermal_zone_device_register(const char *type, int trips, int mask, return ERR_PTR(-EINVAL); } - if (type && strlen(type) >= THERMAL_NAME_LENGTH) { + if (strlen(type) >= THERMAL_NAME_LENGTH) { pr_err("Error: Thermal zone name (%s) too long, should be under %d chars\n", type, THERMAL_NAME_LENGTH); return ERR_PTR(-EINVAL);
Make cooling device registration behavior consistent with thermal zone. This patch also cleans up a unnecessary nullptr check. Signed-off-by: Wei Wang <wvw@google.com> --- drivers/thermal/thermal_core.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)