| Message ID | 20170124070639.GA5068@rzhang1-surface (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue 2017-01-24 15:06:39, Zhang Rui wrote: > On Mon, Jan 23, 2017 at 03:49:12PM -0800, Guenter Roeck wrote: > > On Tue, Jan 24, 2017 at 12:26:54AM +0100, Pavel Machek wrote: > > > Hi! > > > > > > > > >I'll try to revert it on the top of v4.10-rc5 now... and yes, it fixes > > > > > >the issue. > > > > > > > > > > > >Any idea what went wrong and how to fix that? > > > > > > > > > > > >Anyway as we are at -rc5 and this is warning fix that caused a > > > > > >regression on different hardware... it should be reverted. > > > > > > > > > > > Agreed. > > > > > > > > > > What exactly does "stopped working" mean ? That might help understanding > > > > > what went wrong. > > > > > > > > /sys files related to battery no longer appear. I beieve this has > > > > something to do with it: > > > > > > > > [ 2.374877] of_get_named_gpiod_flags: parsed 'reset-gpios' property > > > > of node '/ocp@68000000/spi@48098000/tsc2005@0[0]' - status (0) > > > > [ 2.375946] input: TSC2005 touchscreen as > > > > /devices/platform/68000000.ocp/48098000.spi/spi_master/spi1/spi1.0/input/input5 > > > > [ 2.392120] rx51-battery: probe of n900-battery failed with error > > > > -22 > > > > > > Mystery solved: > > > > > > diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c > > > index 3932f92..fe5ec82 100644 > > > --- a/drivers/hwmon/hwmon.c > > > +++ b/drivers/hwmon/hwmon.c > > > @@ -545,8 +545,10 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata, > > > int i, j, err, id; > > > > > > /* Do not accept invalid characters in hwmon name attribute */ > > > - if (name && (!strlen(name) || strpbrk(name, "-* \t\n"))) > > > + if (name && (!strlen(name) || strpbrk(name, "-* \t\n"))) { > > > + printk("hwmon: Invalid character detected: %s\n", name); > > > return ERR_PTR(-EINVAL); > > > + } > > > > > > id = ida_simple_get(&hwmon_ida, 0, 0, GFP_KERNEL); > > > if (id < 0) > > > > > > > > > pavel@n900:~$ dmesg | grep -5 Invalid > > > [ 0.829650] of_get_named_gpiod_flags: parsed 'gpio-reset' property > > > of node '/ocp@68000000/i2c@48072000/tlv320aic3x@19[0]' - status (0) > > > [ 0.833831] tsl2563 2-0029: model 7, rev. 0 > > > [ 0.837768] of_get_named_gpiod_flags: parsed 'enable-gpio' property > > > of node '/ocp@68000000/i2c@48072000/lp5523@32[0]' - status (0) > > > [ 1.921417] omap_i2c 48072000.i2c: controller timed out > > > [ 2.056823] lp5523x 2-0032: lp5523 Programmable led chip found > > > [ 2.064147] hwmon: Invalid character detected: bq27200-0 > > > > So the problem is really that the thermal driver needs to create a valid name. > > > Right. > > Before reverting, can you please try if this patch works or not? Not really. Revert now. Sorry. Are you sure? This does not look equivalent to me at all. "name" file handling moved from drivers to the core, which added some crazy checks what name can contain. Even if this "works", what is the expected effect on the "name" file? Pavel > thanks, > rui > > >From 79320ea3721314ec29ed6cdd6987ff922b11d6f9 Mon Sep 17 00:00:00 2001 > From: Zhang Rui <rui.zhang@intel.com> > Date: Tue, 24 Jan 2017 14:11:03 +0800 > Subject: [PATCH] thermal: fix parameter when registering hwmon > > commit 7611fb68062f ("thermal: thermal_hwmon: Convert to > hwmon_device_register_with_info()") converts thermal core to use > hwmon_device_register_with_info() to register to hwmon instead of > deprecated hwmon_device_register(). > But at the same time, the name of the hwmon device created is changed to > the thermal zone device name in this commit, which may contain > incompatible characters for hwmon. > > Fixes it by using exactly the same parameters as before this commit. > > Fixes: 7611fb68062f ("thermal: thermal_hwmon: Convert to hwmon_devce_register_with_info()") > Reported-by: Pavel Machek <pavel@ucw.cz> > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > drivers/thermal/thermal_hwmon.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c > index c4a508a..0c6789f 100644 > --- a/drivers/thermal/thermal_hwmon.c > +++ b/drivers/thermal/thermal_hwmon.c > @@ -157,8 +157,8 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz) > > INIT_LIST_HEAD(&hwmon->tz_list); > strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH); > - hwmon->device = hwmon_device_register_with_info(NULL, hwmon->type, > - hwmon, NULL, NULL); > + hwmon->device = hwmon_device_register_with_info(NULL, NULL, NULL, > + NULL, NULL); > if (IS_ERR(hwmon->device)) { > result = PTR_ERR(hwmon->device); > goto free_mem;
On 01/23/2017 11:06 PM, Zhang Rui wrote: > On Mon, Jan 23, 2017 at 03:49:12PM -0800, Guenter Roeck wrote: >> On Tue, Jan 24, 2017 at 12:26:54AM +0100, Pavel Machek wrote: >>> Hi! >>> >>>>>> I'll try to revert it on the top of v4.10-rc5 now... and yes, it fixes >>>>>> the issue. >>>>>> >>>>>> Any idea what went wrong and how to fix that? >>>>>> >>>>>> Anyway as we are at -rc5 and this is warning fix that caused a >>>>>> regression on different hardware... it should be reverted. >>>>>> >>>>> Agreed. >>>>> >>>>> What exactly does "stopped working" mean ? That might help understanding >>>>> what went wrong. >>>> >>>> /sys files related to battery no longer appear. I beieve this has >>>> something to do with it: >>>> >>>> [ 2.374877] of_get_named_gpiod_flags: parsed 'reset-gpios' property >>>> of node '/ocp@68000000/spi@48098000/tsc2005@0[0]' - status (0) >>>> [ 2.375946] input: TSC2005 touchscreen as >>>> /devices/platform/68000000.ocp/48098000.spi/spi_master/spi1/spi1.0/input/input5 >>>> [ 2.392120] rx51-battery: probe of n900-battery failed with error >>>> -22 >>> >>> Mystery solved: >>> >>> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c >>> index 3932f92..fe5ec82 100644 >>> --- a/drivers/hwmon/hwmon.c >>> +++ b/drivers/hwmon/hwmon.c >>> @@ -545,8 +545,10 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata, >>> int i, j, err, id; >>> >>> /* Do not accept invalid characters in hwmon name attribute */ >>> - if (name && (!strlen(name) || strpbrk(name, "-* \t\n"))) >>> + if (name && (!strlen(name) || strpbrk(name, "-* \t\n"))) { >>> + printk("hwmon: Invalid character detected: %s\n", name); >>> return ERR_PTR(-EINVAL); >>> + } >>> >>> id = ida_simple_get(&hwmon_ida, 0, 0, GFP_KERNEL); >>> if (id < 0) >>> >>> >>> pavel@n900:~$ dmesg | grep -5 Invalid >>> [ 0.829650] of_get_named_gpiod_flags: parsed 'gpio-reset' property >>> of node '/ocp@68000000/i2c@48072000/tlv320aic3x@19[0]' - status (0) >>> [ 0.833831] tsl2563 2-0029: model 7, rev. 0 >>> [ 0.837768] of_get_named_gpiod_flags: parsed 'enable-gpio' property >>> of node '/ocp@68000000/i2c@48072000/lp5523@32[0]' - status (0) >>> [ 1.921417] omap_i2c 48072000.i2c: controller timed out >>> [ 2.056823] lp5523x 2-0032: lp5523 Programmable led chip found >>> [ 2.064147] hwmon: Invalid character detected: bq27200-0 >> >> So the problem is really that the thermal driver needs to create a valid name. >> > Right. > > Before reverting, can you please try if this patch works or not? > > thanks, > rui > >>From 79320ea3721314ec29ed6cdd6987ff922b11d6f9 Mon Sep 17 00:00:00 2001 > From: Zhang Rui <rui.zhang@intel.com> > Date: Tue, 24 Jan 2017 14:11:03 +0800 > Subject: [PATCH] thermal: fix parameter when registering hwmon > > commit 7611fb68062f ("thermal: thermal_hwmon: Convert to > hwmon_device_register_with_info()") converts thermal core to use > hwmon_device_register_with_info() to register to hwmon instead of > deprecated hwmon_device_register(). > But at the same time, the name of the hwmon device created is changed to > the thermal zone device name in this commit, which may contain > incompatible characters for hwmon. > > Fixes it by using exactly the same parameters as before this commit. > > Fixes: 7611fb68062f ("thermal: thermal_hwmon: Convert to hwmon_devce_register_with_info()") > Reported-by: Pavel Machek <pavel@ucw.cz> > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > drivers/thermal/thermal_hwmon.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c > index c4a508a..0c6789f 100644 > --- a/drivers/thermal/thermal_hwmon.c > +++ b/drivers/thermal/thermal_hwmon.c > @@ -157,8 +157,8 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz) > > INIT_LIST_HEAD(&hwmon->tz_list); > strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH); > - hwmon->device = hwmon_device_register_with_info(NULL, hwmon->type, > - hwmon, NULL, NULL); > + hwmon->device = hwmon_device_register_with_info(NULL, NULL, NULL, > + NULL, NULL); This is wrong. The name should be converted to a valid name. Otherwise you would still have to provide the name attribute yourself, and the resulting name would still be invalid. A simple fix would be to add strreplace(hwmon->type, '-', '_'); after strlcpy(). Guenter > if (IS_ERR(hwmon->device)) { > result = PTR_ERR(hwmon->device); > goto free_mem; >
Hi! > >>>>/sys files related to battery no longer appear. I beieve this has > >>>>something to do with it: > >>>> > >>>>[ 2.374877] of_get_named_gpiod_flags: parsed 'reset-gpios' property > >>>>of node '/ocp@68000000/spi@48098000/tsc2005@0[0]' - status (0) > >>>>[ 2.375946] input: TSC2005 touchscreen as > >>>>/devices/platform/68000000.ocp/48098000.spi/spi_master/spi1/spi1.0/input/input5 > >>>>[ 2.392120] rx51-battery: probe of n900-battery failed with error > >>>>-22 > >>> > >>>Mystery solved: > >>> > >>>diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c > >>>index 3932f92..fe5ec82 100644 > >>>--- a/drivers/hwmon/hwmon.c > >>>+++ b/drivers/hwmon/hwmon.c > >>>@@ -545,8 +545,10 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata, > >>> int i, j, err, id; > >>> > >>> /* Do not accept invalid characters in hwmon name attribute */ > >>>- if (name && (!strlen(name) || strpbrk(name, "-* \t\n"))) > >>>+ if (name && (!strlen(name) || strpbrk(name, "-* \t\n"))) { > >>>+ printk("hwmon: Invalid character detected: %s\n", name); > >>> return ERR_PTR(-EINVAL); > >>>+ } > >>> > >>> id = ida_simple_get(&hwmon_ida, 0, 0, GFP_KERNEL); > >>> if (id < 0) > >>> > >>> > >>>pavel@n900:~$ dmesg | grep -5 Invalid > >>>[ 0.829650] of_get_named_gpiod_flags: parsed 'gpio-reset' property > >>>of node '/ocp@68000000/i2c@48072000/tlv320aic3x@19[0]' - status (0) > >>>[ 0.833831] tsl2563 2-0029: model 7, rev. 0 > >>>[ 0.837768] of_get_named_gpiod_flags: parsed 'enable-gpio' property > >>>of node '/ocp@68000000/i2c@48072000/lp5523@32[0]' - status (0) > >>>[ 1.921417] omap_i2c 48072000.i2c: controller timed out > >>>[ 2.056823] lp5523x 2-0032: lp5523 Programmable led chip found > >>>[ 2.064147] hwmon: Invalid character detected: bq27200-0 > >> > >>So the problem is really that the thermal driver needs to create a valid name. > >> > >Right. > > > >Before reverting, can you please try if this patch works or not? > >diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c > >index c4a508a..0c6789f 100644 > >--- a/drivers/thermal/thermal_hwmon.c > >+++ b/drivers/thermal/thermal_hwmon.c > >@@ -157,8 +157,8 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz) > > > > INIT_LIST_HEAD(&hwmon->tz_list); > > strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH); > >- hwmon->device = hwmon_device_register_with_info(NULL, hwmon->type, > >- hwmon, NULL, NULL); > >+ hwmon->device = hwmon_device_register_with_info(NULL, NULL, NULL, > >+ NULL, NULL); > > This is wrong. The name should be converted to a valid name. Otherwise you would > still have to provide the name attribute yourself, and the resulting name would > still be invalid. A simple fix would be to add > strreplace(hwmon->type, '-', '_'); > after strlcpy(). Simple, yes. But that will change userland interface. Is it okay thing to do? Pavel
On 01/23/2017 11:37 PM, Pavel Machek wrote: > On Tue 2017-01-24 15:06:39, Zhang Rui wrote: >> On Mon, Jan 23, 2017 at 03:49:12PM -0800, Guenter Roeck wrote: >>> On Tue, Jan 24, 2017 at 12:26:54AM +0100, Pavel Machek wrote: >>>> Hi! >>>> >>>>>>> I'll try to revert it on the top of v4.10-rc5 now... and yes, it fixes >>>>>>> the issue. >>>>>>> >>>>>>> Any idea what went wrong and how to fix that? >>>>>>> >>>>>>> Anyway as we are at -rc5 and this is warning fix that caused a >>>>>>> regression on different hardware... it should be reverted. >>>>>>> >>>>>> Agreed. >>>>>> >>>>>> What exactly does "stopped working" mean ? That might help understanding >>>>>> what went wrong. >>>>> >>>>> /sys files related to battery no longer appear. I beieve this has >>>>> something to do with it: >>>>> >>>>> [ 2.374877] of_get_named_gpiod_flags: parsed 'reset-gpios' property >>>>> of node '/ocp@68000000/spi@48098000/tsc2005@0[0]' - status (0) >>>>> [ 2.375946] input: TSC2005 touchscreen as >>>>> /devices/platform/68000000.ocp/48098000.spi/spi_master/spi1/spi1.0/input/input5 >>>>> [ 2.392120] rx51-battery: probe of n900-battery failed with error >>>>> -22 >>>> >>>> Mystery solved: >>>> >>>> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c >>>> index 3932f92..fe5ec82 100644 >>>> --- a/drivers/hwmon/hwmon.c >>>> +++ b/drivers/hwmon/hwmon.c >>>> @@ -545,8 +545,10 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata, >>>> int i, j, err, id; >>>> >>>> /* Do not accept invalid characters in hwmon name attribute */ >>>> - if (name && (!strlen(name) || strpbrk(name, "-* \t\n"))) >>>> + if (name && (!strlen(name) || strpbrk(name, "-* \t\n"))) { >>>> + printk("hwmon: Invalid character detected: %s\n", name); >>>> return ERR_PTR(-EINVAL); >>>> + } >>>> >>>> id = ida_simple_get(&hwmon_ida, 0, 0, GFP_KERNEL); >>>> if (id < 0) >>>> >>>> >>>> pavel@n900:~$ dmesg | grep -5 Invalid >>>> [ 0.829650] of_get_named_gpiod_flags: parsed 'gpio-reset' property >>>> of node '/ocp@68000000/i2c@48072000/tlv320aic3x@19[0]' - status (0) >>>> [ 0.833831] tsl2563 2-0029: model 7, rev. 0 >>>> [ 0.837768] of_get_named_gpiod_flags: parsed 'enable-gpio' property >>>> of node '/ocp@68000000/i2c@48072000/lp5523@32[0]' - status (0) >>>> [ 1.921417] omap_i2c 48072000.i2c: controller timed out >>>> [ 2.056823] lp5523x 2-0032: lp5523 Programmable led chip found >>>> [ 2.064147] hwmon: Invalid character detected: bq27200-0 >>> >>> So the problem is really that the thermal driver needs to create a valid name. >>> >> Right. >> >> Before reverting, can you please try if this patch works or not? > > Not really. Revert now. Sorry. > > Are you sure? This does not look equivalent to me at all. > > "name" file handling moved from drivers to the core, which added some > crazy checks what name can contain. Even if this "works", what is the > expected effect on the "name" file? > The hwmon name attribute must not include '-', as documented in Documentation/hwmon/sysfs-interface. Is enforcing that 'crazy' ? Maybe in your world, but not in mine. Guenter
On 01/24/2017 06:15 AM, Pavel Machek wrote: > Hi! > >>>>>> /sys files related to battery no longer appear. I beieve this has >>>>>> something to do with it: >>>>>> >>>>>> [ 2.374877] of_get_named_gpiod_flags: parsed 'reset-gpios' property >>>>>> of node '/ocp@68000000/spi@48098000/tsc2005@0[0]' - status (0) >>>>>> [ 2.375946] input: TSC2005 touchscreen as >>>>>> /devices/platform/68000000.ocp/48098000.spi/spi_master/spi1/spi1.0/input/input5 >>>>>> [ 2.392120] rx51-battery: probe of n900-battery failed with error >>>>>> -22 >>>>> >>>>> Mystery solved: >>>>> >>>>> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c >>>>> index 3932f92..fe5ec82 100644 >>>>> --- a/drivers/hwmon/hwmon.c >>>>> +++ b/drivers/hwmon/hwmon.c >>>>> @@ -545,8 +545,10 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata, >>>>> int i, j, err, id; >>>>> >>>>> /* Do not accept invalid characters in hwmon name attribute */ >>>>> - if (name && (!strlen(name) || strpbrk(name, "-* \t\n"))) >>>>> + if (name && (!strlen(name) || strpbrk(name, "-* \t\n"))) { >>>>> + printk("hwmon: Invalid character detected: %s\n", name); >>>>> return ERR_PTR(-EINVAL); >>>>> + } >>>>> >>>>> id = ida_simple_get(&hwmon_ida, 0, 0, GFP_KERNEL); >>>>> if (id < 0) >>>>> >>>>> >>>>> pavel@n900:~$ dmesg | grep -5 Invalid >>>>> [ 0.829650] of_get_named_gpiod_flags: parsed 'gpio-reset' property >>>>> of node '/ocp@68000000/i2c@48072000/tlv320aic3x@19[0]' - status (0) >>>>> [ 0.833831] tsl2563 2-0029: model 7, rev. 0 >>>>> [ 0.837768] of_get_named_gpiod_flags: parsed 'enable-gpio' property >>>>> of node '/ocp@68000000/i2c@48072000/lp5523@32[0]' - status (0) >>>>> [ 1.921417] omap_i2c 48072000.i2c: controller timed out >>>>> [ 2.056823] lp5523x 2-0032: lp5523 Programmable led chip found >>>>> [ 2.064147] hwmon: Invalid character detected: bq27200-0 >>>> >>>> So the problem is really that the thermal driver needs to create a valid name. >>>> >>> Right. >>> >>> Before reverting, can you please try if this patch works or not? > >>> diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c >>> index c4a508a..0c6789f 100644 >>> --- a/drivers/thermal/thermal_hwmon.c >>> +++ b/drivers/thermal/thermal_hwmon.c >>> @@ -157,8 +157,8 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz) >>> >>> INIT_LIST_HEAD(&hwmon->tz_list); >>> strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH); >>> - hwmon->device = hwmon_device_register_with_info(NULL, hwmon->type, >>> - hwmon, NULL, NULL); >>> + hwmon->device = hwmon_device_register_with_info(NULL, NULL, NULL, >>> + NULL, NULL); >> >> This is wrong. The name should be converted to a valid name. Otherwise you would >> still have to provide the name attribute yourself, and the resulting name would >> still be invalid. A simple fix would be to add >> strreplace(hwmon->type, '-', '_'); >> after strlcpy(). > > Simple, yes. But that will change userland interface. Is it okay thing > to do? Yes, because the old name is invalid. Note that the new API also changes the sysfs directory path. But, as mentioned separately, if providing valid names is unacceptable for thermal drivers, the thermal subsystem will have to stick with the old API (and I may create a patch to make the name attribute mandatory for the new API to prevent people from defeating the purpose of the conversion by not providing one). Guenter
> >>Right. > >> > >>Before reverting, can you please try if this patch works or not? > > > >Not really. Revert now. Sorry. > > > >Are you sure? This does not look equivalent to me at all. > > > >"name" file handling moved from drivers to the core, which added some > >crazy checks what name can contain. Even if this "works", what is the > >expected effect on the "name" file? > > > The hwmon name attribute must not include '-', as documented in > Documentation/hwmon/sysfs-interface. Is enforcing that 'crazy' ? > Maybe in your world, but not in mine. Well, lets revert the patch and then we can discuss what to do with the "name" problem. Unfortunately, code enforces different rules than documentation says, and it is all visible to userspace. Pavel
On Tue, Jan 24, 2017 at 06:58:01PM +0100, Pavel Machek wrote: > > > >>Right. > > >> > > >>Before reverting, can you please try if this patch works or not? > > > > > >Not really. Revert now. Sorry. > > > > > >Are you sure? This does not look equivalent to me at all. > > > > > >"name" file handling moved from drivers to the core, which added some > > >crazy checks what name can contain. Even if this "works", what is the > > >expected effect on the "name" file? > > > > > The hwmon name attribute must not include '-', as documented in > > Documentation/hwmon/sysfs-interface. Is enforcing that 'crazy' ? > > Maybe in your world, but not in mine. > > Well, lets revert the patch and then we can discuss what to do with > the "name" problem. > Not sure what the problem to be discussed is. Please provide details. > Unfortunately, code enforces different rules than documentation says, Please specify your problem with rules vs. documentation. You mean the additional checks which also block wildcards and spaces/newline in the name ? Are you having a problem with those ? What are those problems ? I'll be more than happy to update the documentation if your problem is with that mismatch. To provide some detail: libsensors gets just as confused with wildcards and whitespace/newline as it does with '-' in the reported name, which is why those are blocked by the new API. > and it is all visible to userspace. > We are talking about a value reported by an attribute, not about an attribute. Maybe the ABI Police thinks differently, but I do not count fixing the value reported by an attribute as an ABI change. Effecively, what this enforcement does is to make userspace actually work. Ultimately that means, per your standard, that fixing values reported by the kernel is not permitted under any circumstances, even if such changes result in userspace actually working as intended as a result of that change. I would argue that such an enforcement goes a little bit too far. In addition, there is actually another problem solved by the patch you want to have reverted: The current thermal code has another problem, which is that it creates the name attribute _after_ registering the hwmon device. The udev event for creating the hwmon device thus occurs before its mandatory name attribute (and any other attribute, for that matter) exists, meaning that any userspace handler acting on it would be completely unpredictable. Now _this_ is a real ABI "change", since it fixes a race condition. I guess you are going to argue that this isn't permitted either ? Guenter
On Tue 2017-01-24 10:45:26, Guenter Roeck wrote: > On Tue, Jan 24, 2017 at 06:58:01PM +0100, Pavel Machek wrote: > > > > > >>Right. > > > >> > > > >>Before reverting, can you please try if this patch works or not? > > > > > > > >Not really. Revert now. Sorry. > > > > > > > >Are you sure? This does not look equivalent to me at all. > > > > > > > >"name" file handling moved from drivers to the core, which added some > > > >crazy checks what name can contain. Even if this "works", what is the > > > >expected effect on the "name" file? > > > > > > > The hwmon name attribute must not include '-', as documented in > > > Documentation/hwmon/sysfs-interface. Is enforcing that 'crazy' ? > > > Maybe in your world, but not in mine. > > > > Well, lets revert the patch and then we can discuss what to do with > > the "name" problem. > > > Not sure what the problem to be discussed is. Please provide > details. See below. > > Unfortunately, code enforces different rules than documentation says, > > Please specify your problem with rules vs. documentation. You mean the > additional checks which also block wildcards and spaces/newline in the > name ? Are you having a problem with those ? What are those problems ? > I'll be more than happy to update the documentation if your problem > is with that mismatch. Yes, it sounds like documentation should be updated. > To provide some detail: libsensors gets just as confused with wildcards > and whitespace/newline as it does with '-' in the reported name, which > is why those are blocked by the new API. > > and it is all visible to userspace. > > > We are talking about a value reported by an attribute, not about an attribute. > Maybe the ABI Police thinks differently, but I do not count fixing the value > reported by an attribute as an ABI change. The rule is not what you think it is. It does not matter if it is attribute or value. It is about breaking userspace. And this is the attribute userland will use to search for sensor it works with, right? So changing this attribute will break userspace if someone uses it. N900 already > 2 temperature sensors on, so someone may actually be using it. > Effecively, what this enforcement does is to make userspace actually work. > Ultimately that means, per your standard, that fixing values reported by the > kernel is not permitted under any circumstances, even if such changes result > in userspace actually working as intended as a result of that change. > I would argue that such an enforcement goes a little bit too far. You can argue whatever you want, but the rule is "no regressions". You may not break userspace app A to fix userspace app B. > In addition, there is actually another problem solved by the patch you want to > have reverted: The current thermal code has another problem, which is that it > creates the name attribute _after_ registering the hwmon device. The udev event > for creating the hwmon device thus occurs before its mandatory name attribute > (and any other attribute, for that matter) exists, meaning that any userspace > handler acting on it would be completely unpredictable. Now _this_ is a real > ABI "change", since it fixes a race condition. I guess you are going to argue > that this isn't permitted either ? You guessed wrong. It would only be problem if somone relied on that race condition. Anyway, I reported a rather serious regression in -rc5. The original patch was obviously not understood enough, and it broke charging on my hardware. I'd argue it had no reason to be in -rc5 in the first place. I believe solution is obvious. Revert the damn patch. I actually wonder why we are discussing it. Maintainer suggested different solution, but you said it was broken. You are suggesting modification of two driver names on N900. You don't know what sideeffects it might have. You don't know if there are more driver names to fix. You did not offer a patch. It is an ugly regression and it is -rc5 time. Actually not your problem, but Fabio Estevam or Zhang Rui has to deal with it soon. Thanks, Pavel
On Tue, Jan 24, 2017 at 8:46 PM, Pavel Machek <pavel@ucw.cz> wrote: > It is an ugly regression and it is -rc5 time. Actually not your > problem, but Fabio Estevam or Zhang Rui has to deal with it soon. I have already sent the revert and it was acked-by you and Guenter.
On Tue, 2017-01-24 at 21:07 -0200, Fabio Estevam wrote: > On Tue, Jan 24, 2017 at 8:46 PM, Pavel Machek <pavel@ucw.cz> wrote: > > > > > It is an ugly regression and it is -rc5 time. Actually not your > > problem, but Fabio Estevam or Zhang Rui has to deal with it soon. > I have already sent the revert and it was acked-by you and Guenter. Revert patch has been applied and queued for next -rc. thanks, rui
On Wed 2017-01-25 13:29:52, Zhang Rui wrote: > On Tue, 2017-01-24 at 21:07 -0200, Fabio Estevam wrote: > > On Tue, Jan 24, 2017 at 8:46 PM, Pavel Machek <pavel@ucw.cz> wrote: > > > > > > > > It is an ugly regression and it is -rc5 time. Actually not your > > > problem, but Fabio Estevam or Zhang Rui has to deal with it soon. > > I have already sent the revert and it was acked-by you and Guenter. > > Revert patch has been applied and queued for next -rc. Thanks! Pavel
Hi! > > > >>Right. > > > >> > > > >>Before reverting, can you please try if this patch works or not? > > > > > > > >Not really. Revert now. Sorry. > > > > > > > >Are you sure? This does not look equivalent to me at all. > > > > > > > >"name" file handling moved from drivers to the core, which added some > > > >crazy checks what name can contain. Even if this "works", what is the > > > >expected effect on the "name" file? > > > > > > > The hwmon name attribute must not include '-', as documented in > > > Documentation/hwmon/sysfs-interface. Is enforcing that 'crazy' ? > > > Maybe in your world, but not in mine. > > > > Well, lets revert the patch and then we can discuss what to do with > > the "name" problem. Ok, so the patch is on the way in. What to do next? pavel@n900:/sys/class/hwmon$ cat hwmon0/name bq27200-0 pavel@n900:/sys/class/hwmon$ cat hwmon1/name rx51-battery > To provide some detail: libsensors gets just as confused with wildcards > and whitespace/newline as it does with '-' in the reported name, which > is why those are blocked by the new API. Ok... Question is "does someone actually use hwmon*/name on N900"? If so, we can't change it, but it is well possible that noone is. Next question is .. are there other drivers affected? Do we want to do '-' -> '_' in the core or somewhere in the drivers? We might want to do the change in early in 4.11 and see what breaks.... Pavel
On 01/25/2017 03:12 AM, Pavel Machek wrote: > Hi! > >>>>>> Right. >>>>>> >>>>>> Before reverting, can you please try if this patch works or not? >>>>> >>>>> Not really. Revert now. Sorry. >>>>> >>>>> Are you sure? This does not look equivalent to me at all. >>>>> >>>>> "name" file handling moved from drivers to the core, which added some >>>>> crazy checks what name can contain. Even if this "works", what is the >>>>> expected effect on the "name" file? >>>>> >>>> The hwmon name attribute must not include '-', as documented in >>>> Documentation/hwmon/sysfs-interface. Is enforcing that 'crazy' ? >>>> Maybe in your world, but not in mine. >>> >>> Well, lets revert the patch and then we can discuss what to do with >>> the "name" problem. > > Ok, so the patch is on the way in. What to do next? > > pavel@n900:/sys/class/hwmon$ cat hwmon0/name > bq27200-0 > pavel@n900:/sys/class/hwmon$ cat hwmon1/name > rx51-battery > The 'name' parameter will be mandatory for hwmon_device_register_with_groups() and hwmon_device_register_with_info() starting with v4.11, and the API documentation will be updated to list all invalid characters. The thermal subsystem doesn't play well when it comes to hwmon userspace interaction (generating and removing sensor attributes dynamically messes up pretty much everything, and generating the name attribute after hwmon registration for sure messes up any udev listener). libsensors gets confused by '-' in the 'name' attribute. libsensors also doesn't expect sensor attributes to be dynamically generated and removed, and may react unpredictably if they are. This all means that any existing users of hwmon devices created by the thermal subsystem will most likely not use libsensors nor udev listeners. At the same time, it seems to be quite important at least to you that the '-' in the name is retained. Given all that, maybe thermal should just stick with the deprecated API and accept the deprecated warning message (or maybe drop hwmon support altogether). Guenter
On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote: > Hi! > > > > > >>Right. > > > > >> > > > > >>Before reverting, can you please try if this patch works or not? > > > > > > > > > >Not really. Revert now. Sorry. > > > > > > > > > >Are you sure? This does not look equivalent to me at all. > > > > > > > > > >"name" file handling moved from drivers to the core, which added some > > > > >crazy checks what name can contain. Even if this "works", what is the > > > > >expected effect on the "name" file? > > > > > > > > > The hwmon name attribute must not include '-', as documented in > > > > Documentation/hwmon/sysfs-interface. Is enforcing that 'crazy' ? > > > > Maybe in your world, but not in mine. > > > > > > Well, lets revert the patch and then we can discuss what to do with > > > the "name" problem. > > Ok, so the patch is on the way in. What to do next? > > pavel@n900:/sys/class/hwmon$ cat hwmon0/name > bq27200-0 > pavel@n900:/sys/class/hwmon$ cat hwmon1/name > rx51-battery > > > To provide some detail: libsensors gets just as confused with wildcards > > and whitespace/newline as it does with '-' in the reported name, which > > is why those are blocked by the new API. > > Ok... Question is "does someone actually use hwmon*/name on N900"? If > so, we can't change it, but it is well possible that noone is. IIRC hwmon is used on Nokia N900. But I have not seen hwmon devices for bq27200 and rx51-battery yet. Those are power supply driver and auto-exporting them also via hwmon is something new, right? If yes, then we can use any name for those new hwmon devices as they cannot break userspace... as there is no userspace application for them. > Next question is .. are there other drivers affected? Do we want to do > '-' -> '_' in the core or somewhere in the drivers? We might want to > do the change in early in 4.11 and see what breaks.... IIRC hwmon core does not accept '-' for a long time (maybe all 4.x versions?).
On Wed, 2017-01-25 at 13:09 +0100, Pali Rohár wrote: > On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote: > > > > Hi! > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Right. > > > > > > > > > > > > > > Before reverting, can you please try if this patch works > > > > > > > or not? > > > > > > Not really. Revert now. Sorry. > > > > > > > > > > > > Are you sure? This does not look equivalent to me at all. > > > > > > > > > > > > "name" file handling moved from drivers to the core, which > > > > > > added some > > > > > > crazy checks what name can contain. Even if this "works", > > > > > > what is the > > > > > > expected effect on the "name" file? > > > > > > > > > > > The hwmon name attribute must not include '-', as documented > > > > > in > > > > > Documentation/hwmon/sysfs-interface. Is enforcing that > > > > > 'crazy' ? > > > > > Maybe in your world, but not in mine. > > > > Well, lets revert the patch and then we can discuss what to do > > > > with > > > > the "name" problem. > > Ok, so the patch is on the way in. What to do next? > > > > pavel@n900:/sys/class/hwmon$ cat hwmon0/name > > bq27200-0 > > pavel@n900:/sys/class/hwmon$ cat hwmon1/name > > rx51-battery > > > > > > > > To provide some detail: libsensors gets just as confused with > > > wildcards > > > and whitespace/newline as it does with '-' in the reported name, > > > which > > > is why those are blocked by the new API. > > Ok... Question is "does someone actually use hwmon*/name on N900"? > > If > > so, we can't change it, but it is well possible that noone is. > IIRC hwmon is used on Nokia N900. > > But I have not seen hwmon devices for bq27200 and rx51-battery yet. > Those are power supply driver and auto-exporting them also via hwmon > is > something new, right? If yes, then we can use any name for those new > hwmon devices as they cannot break userspace... as there is no > userspace > application for them. > If this is the case, you'd better set (struct thermal_zone_params)->no_hwmon when registering the thermal zone device, in which case, the hwmon device will not be created. In fact, I'd prefer to change tzp->no_hwmon to tzp->hwmon to not create hwmon I/F by default, and see if there is anyone using it. If yes, we can set the flag in soc thermal driver, explicitly, at meantime, a hwmon compatible name is required. But one foreseeable result is that we may get bug reports from end user that some sensors (acpitz, etc) are gone in 'sensors' output. And TBH, I'm not quite sure if this can be counted as a regression or not. thanks, rui > > > > Next question is .. are there other drivers affected? Do we want to > > do > > '-' -> '_' in the core or somewhere in the drivers? We might want > > to > > do the change in early in 4.11 and see what breaks.... > IIRC hwmon core does not accept '-' for a long time (maybe all 4.x > versions?). >
On 01/26/2017 05:37 PM, Zhang Rui wrote: > On Wed, 2017-01-25 at 13:09 +0100, Pali Rohár wrote: >> On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote: >>> >>> Hi! >>> >>>> >>>>> >>>>>> >>>>>>> >>>>>>>> >>>>>>>> Right. >>>>>>>> >>>>>>>> Before reverting, can you please try if this patch works >>>>>>>> or not? >>>>>>> Not really. Revert now. Sorry. >>>>>>> >>>>>>> Are you sure? This does not look equivalent to me at all. >>>>>>> >>>>>>> "name" file handling moved from drivers to the core, which >>>>>>> added some >>>>>>> crazy checks what name can contain. Even if this "works", >>>>>>> what is the >>>>>>> expected effect on the "name" file? >>>>>>> >>>>>> The hwmon name attribute must not include '-', as documented >>>>>> in >>>>>> Documentation/hwmon/sysfs-interface. Is enforcing that >>>>>> 'crazy' ? >>>>>> Maybe in your world, but not in mine. >>>>> Well, lets revert the patch and then we can discuss what to do >>>>> with >>>>> the "name" problem. >>> Ok, so the patch is on the way in. What to do next? >>> >>> pavel@n900:/sys/class/hwmon$ cat hwmon0/name >>> bq27200-0 >>> pavel@n900:/sys/class/hwmon$ cat hwmon1/name >>> rx51-battery >>> >>>> >>>> To provide some detail: libsensors gets just as confused with >>>> wildcards >>>> and whitespace/newline as it does with '-' in the reported name, >>>> which >>>> is why those are blocked by the new API. >>> Ok... Question is "does someone actually use hwmon*/name on N900"? >>> If >>> so, we can't change it, but it is well possible that noone is. >> IIRC hwmon is used on Nokia N900. >> >> But I have not seen hwmon devices for bq27200 and rx51-battery yet. >> Those are power supply driver and auto-exporting them also via hwmon >> is >> something new, right? If yes, then we can use any name for those new >> hwmon devices as they cannot break userspace... as there is no >> userspace >> application for them. >> > If this is the case, you'd better set > (struct thermal_zone_params)->no_hwmon when registering the thermal > zone device, in which case, the hwmon device will not be created. > > In fact, I'd prefer to change tzp->no_hwmon to tzp->hwmon to not create > hwmon I/F by default, and see if there is anyone using it. If yes, we > can set the flag in soc thermal driver, explicitly, at meantime, a > hwmon compatible name is required. > > But one foreseeable result is that we may get bug reports from end user > that some sensors (acpitz, etc) are gone in 'sensors' output. And TBH, > I'm not quite sure if this can be counted as a regression or not. > That sounds like fun. Changing bq27200-0 to bq27200_0 is Forbidden by the ABI Police, but taking the entire device away is ok. Anyway, sounds good to me. No one will use something that isn't there, and no one will realize that it could have been there, so I don't expect anyone to complain. Guenter
On Thu, 2017-01-26 at 18:03 -0800, Guenter Roeck wrote: > On 01/26/2017 05:37 PM, Zhang Rui wrote: > > > > On Wed, 2017-01-25 at 13:09 +0100, Pali Rohár wrote: > > > > > > On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote: > > > > > > > > > > > > Hi! > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Right. > > > > > > > > > > > > > > > > > > Before reverting, can you please try if this patch > > > > > > > > > works > > > > > > > > > or not? > > > > > > > > Not really. Revert now. Sorry. > > > > > > > > > > > > > > > > Are you sure? This does not look equivalent to me at > > > > > > > > all. > > > > > > > > > > > > > > > > "name" file handling moved from drivers to the core, > > > > > > > > which > > > > > > > > added some > > > > > > > > crazy checks what name can contain. Even if this > > > > > > > > "works", > > > > > > > > what is the > > > > > > > > expected effect on the "name" file? > > > > > > > > > > > > > > > The hwmon name attribute must not include '-', as > > > > > > > documented > > > > > > > in > > > > > > > Documentation/hwmon/sysfs-interface. Is enforcing that > > > > > > > 'crazy' ? > > > > > > > Maybe in your world, but not in mine. > > > > > > Well, lets revert the patch and then we can discuss what to > > > > > > do > > > > > > with > > > > > > the "name" problem. > > > > Ok, so the patch is on the way in. What to do next? > > > > > > > > pavel@n900:/sys/class/hwmon$ cat hwmon0/name > > > > bq27200-0 > > > > pavel@n900:/sys/class/hwmon$ cat hwmon1/name > > > > rx51-battery > > > > > > > > > > > > > > > > > > > To provide some detail: libsensors gets just as confused with > > > > > wildcards > > > > > and whitespace/newline as it does with '-' in the reported > > > > > name, > > > > > which > > > > > is why those are blocked by the new API. > > > > Ok... Question is "does someone actually use hwmon*/name on > > > > N900"? > > > > If > > > > so, we can't change it, but it is well possible that noone is. > > > IIRC hwmon is used on Nokia N900. > > > > > > But I have not seen hwmon devices for bq27200 and rx51-battery > > > yet. > > > Those are power supply driver and auto-exporting them also via > > > hwmon > > > is > > > something new, right? If yes, then we can use any name for those > > > new > > > hwmon devices as they cannot break userspace... as there is no > > > userspace > > > application for them. > > > > > If this is the case, you'd better set > > (struct thermal_zone_params)->no_hwmon when registering the thermal > > zone device, in which case, the hwmon device will not be created. > > > > In fact, I'd prefer to change tzp->no_hwmon to tzp->hwmon to not > > create > > hwmon I/F by default, and see if there is anyone using it. If yes, > > we > > can set the flag in soc thermal driver, explicitly, at meantime, a > > hwmon compatible name is required. > > > > But one foreseeable result is that we may get bug reports from end > > user > > that some sensors (acpitz, etc) are gone in 'sensors' output. And > > TBH, > > I'm not quite sure if this can be counted as a regression or not. > > > That sounds like fun. Changing bq27200-0 to bq27200_0 is Forbidden by > the ABI Police, but taking the entire device away is ok. > No. IMO, it depends on if the interface is used or not. If hwmon I/F is used, we can not take it away, nor change its name. If thermal zone I/F is used, we can not change it's 'type' name to be compatible with new hwmon API. > Anyway, sounds good to me. No one will use something that isn't > there, > and no one will realize that it could have been there, so I don't > expect > anyone to complain. Yes, I agree. thanks, rui
On 01/26/2017 07:39 PM, Zhang Rui wrote: > On Thu, 2017-01-26 at 18:03 -0800, Guenter Roeck wrote: >> On 01/26/2017 05:37 PM, Zhang Rui wrote: >>> >>> On Wed, 2017-01-25 at 13:09 +0100, Pali Rohár wrote: >>>> >>>> On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote: >>>>> >>>>> >>>>> Hi! >>>>> >>>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Right. >>>>>>>>>> >>>>>>>>>> Before reverting, can you please try if this patch >>>>>>>>>> works >>>>>>>>>> or not? >>>>>>>>> Not really. Revert now. Sorry. >>>>>>>>> >>>>>>>>> Are you sure? This does not look equivalent to me at >>>>>>>>> all. >>>>>>>>> >>>>>>>>> "name" file handling moved from drivers to the core, >>>>>>>>> which >>>>>>>>> added some >>>>>>>>> crazy checks what name can contain. Even if this >>>>>>>>> "works", >>>>>>>>> what is the >>>>>>>>> expected effect on the "name" file? >>>>>>>>> >>>>>>>> The hwmon name attribute must not include '-', as >>>>>>>> documented >>>>>>>> in >>>>>>>> Documentation/hwmon/sysfs-interface. Is enforcing that >>>>>>>> 'crazy' ? >>>>>>>> Maybe in your world, but not in mine. >>>>>>> Well, lets revert the patch and then we can discuss what to >>>>>>> do >>>>>>> with >>>>>>> the "name" problem. >>>>> Ok, so the patch is on the way in. What to do next? >>>>> >>>>> pavel@n900:/sys/class/hwmon$ cat hwmon0/name >>>>> bq27200-0 >>>>> pavel@n900:/sys/class/hwmon$ cat hwmon1/name >>>>> rx51-battery >>>>> >>>>>> >>>>>> >>>>>> To provide some detail: libsensors gets just as confused with >>>>>> wildcards >>>>>> and whitespace/newline as it does with '-' in the reported >>>>>> name, >>>>>> which >>>>>> is why those are blocked by the new API. >>>>> Ok... Question is "does someone actually use hwmon*/name on >>>>> N900"? >>>>> If >>>>> so, we can't change it, but it is well possible that noone is. >>>> IIRC hwmon is used on Nokia N900. >>>> >>>> But I have not seen hwmon devices for bq27200 and rx51-battery >>>> yet. >>>> Those are power supply driver and auto-exporting them also via >>>> hwmon >>>> is >>>> something new, right? If yes, then we can use any name for those >>>> new >>>> hwmon devices as they cannot break userspace... as there is no >>>> userspace >>>> application for them. >>>> >>> If this is the case, you'd better set >>> (struct thermal_zone_params)->no_hwmon when registering the thermal >>> zone device, in which case, the hwmon device will not be created. >>> >>> In fact, I'd prefer to change tzp->no_hwmon to tzp->hwmon to not >>> create >>> hwmon I/F by default, and see if there is anyone using it. If yes, >>> we >>> can set the flag in soc thermal driver, explicitly, at meantime, a >>> hwmon compatible name is required. >>> >>> But one foreseeable result is that we may get bug reports from end >>> user >>> that some sensors (acpitz, etc) are gone in 'sensors' output. And >>> TBH, >>> I'm not quite sure if this can be counted as a regression or not. >>> >> That sounds like fun. Changing bq27200-0 to bq27200_0 is Forbidden by >> the ABI Police, but taking the entire device away is ok. >> > No. IMO, it depends on if the interface is used or not. > If hwmon I/F is used, we can not take it away, nor change its name. Even if the use doesn't depend on that name ? > If thermal zone I/F is used, we can not change it's 'type' name to be > compatible with new hwmon API. > You mean you can not fix the name to be compatible with libsensors. Makes me wonder if there shouldn't be a rule that exploits must not be fixed if already exploited. Guenter >> Anyway, sounds good to me. No one will use something that isn't >> there, >> and no one will realize that it could have been there, so I don't >> expect >> anyone to complain. > > Yes, I agree. > > thanks, > rui >
On Friday 27 January 2017 11:39:42 Zhang Rui wrote: > On Thu, 2017-01-26 at 18:03 -0800, Guenter Roeck wrote: > > On 01/26/2017 05:37 PM, Zhang Rui wrote: > > > > > > On Wed, 2017-01-25 at 13:09 +0100, Pali Rohár wrote: > > > > > > > > On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote: > > > > > > > > > > > > > > > Hi! > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Right. > > > > > > > > > > > > > > > > > > > > Before reverting, can you please try if this patch > > > > > > > > > > works > > > > > > > > > > or not? > > > > > > > > > Not really. Revert now. Sorry. > > > > > > > > > > > > > > > > > > Are you sure? This does not look equivalent to me at > > > > > > > > > all. > > > > > > > > > > > > > > > > > > "name" file handling moved from drivers to the core, > > > > > > > > > which > > > > > > > > > added some > > > > > > > > > crazy checks what name can contain. Even if this > > > > > > > > > "works", > > > > > > > > > what is the > > > > > > > > > expected effect on the "name" file? > > > > > > > > > > > > > > > > > The hwmon name attribute must not include '-', as > > > > > > > > documented > > > > > > > > in > > > > > > > > Documentation/hwmon/sysfs-interface. Is enforcing that > > > > > > > > 'crazy' ? > > > > > > > > Maybe in your world, but not in mine. > > > > > > > Well, lets revert the patch and then we can discuss what to > > > > > > > do > > > > > > > with > > > > > > > the "name" problem. > > > > > Ok, so the patch is on the way in. What to do next? > > > > > > > > > > pavel@n900:/sys/class/hwmon$ cat hwmon0/name > > > > > bq27200-0 > > > > > pavel@n900:/sys/class/hwmon$ cat hwmon1/name > > > > > rx51-battery > > > > > > > > > > > > > > > > > > > > > > > To provide some detail: libsensors gets just as confused with > > > > > > wildcards > > > > > > and whitespace/newline as it does with '-' in the reported > > > > > > name, > > > > > > which > > > > > > is why those are blocked by the new API. > > > > > Ok... Question is "does someone actually use hwmon*/name on > > > > > N900"? > > > > > If > > > > > so, we can't change it, but it is well possible that noone is. > > > > IIRC hwmon is used on Nokia N900. > > > > > > > > But I have not seen hwmon devices for bq27200 and rx51-battery > > > > yet. > > > > Those are power supply driver and auto-exporting them also via > > > > hwmon > > > > is > > > > something new, right? If yes, then we can use any name for those > > > > new > > > > hwmon devices as they cannot break userspace... as there is no > > > > userspace > > > > application for them. > > > > > > > If this is the case, you'd better set > > > (struct thermal_zone_params)->no_hwmon when registering the thermal > > > zone device, in which case, the hwmon device will not be created. > > > > > > In fact, I'd prefer to change tzp->no_hwmon to tzp->hwmon to not > > > create > > > hwmon I/F by default, and see if there is anyone using it. If yes, > > > we > > > can set the flag in soc thermal driver, explicitly, at meantime, a > > > hwmon compatible name is required. > > > > > > But one foreseeable result is that we may get bug reports from end > > > user > > > that some sensors (acpitz, etc) are gone in 'sensors' output. And > > > TBH, > > > I'm not quite sure if this can be counted as a regression or not. > > > > > That sounds like fun. Changing bq27200-0 to bq27200_0 is Forbidden by > > the ABI Police, but taking the entire device away is ok. > > > No. IMO, it depends on if the interface is used or not. > If hwmon I/F is used, we can not take it away, nor change its name. > If thermal zone I/F is used, we can not change it's 'type' name to be > compatible with new hwmon API. And what about allowing driver (rx51_battery.ko) to set hwmon name to be compatible with hwmon policy? Power supply name will be unchanged, but hwmon name could be different (it is new API/ABI -- not used by anyone) to fit into hwmon subsytem. Or instead allowing driver to set name, what about fixing name in hwmon subsystem to be compatible? (By fixing I mean to only fix hwmon name, not power supply). > > Anyway, sounds good to me. No one will use something that isn't > > there, > > and no one will realize that it could have been there, so I don't > > expect > > anyone to complain. > > Yes, I agree. > > thanks, > rui
> >If this is the case, you'd better set > >(struct thermal_zone_params)->no_hwmon when registering the thermal > >zone device, in which case, the hwmon device will not be created. > > > >In fact, I'd prefer to change tzp->no_hwmon to tzp->hwmon to not create > >hwmon I/F by default, and see if there is anyone using it. If yes, we > >can set the flag in soc thermal driver, explicitly, at meantime, a > >hwmon compatible name is required. > > > >But one foreseeable result is that we may get bug reports from end user > >that some sensors (acpitz, etc) are gone in 'sensors' output. And TBH, > >I'm not quite sure if this can be counted as a regression or not. > > > > That sounds like fun. Changing bq27200-0 to bq27200_0 is Forbidden by > the ABI Police, but taking the entire device away is ok. Stop trying to break other people's systems. Thanks you. Pavel
Hi! > >>That sounds like fun. Changing bq27200-0 to bq27200_0 is Forbidden by > >>the ABI Police, but taking the entire device away is ok. Changing bq27200-0 to bq27200_0 is forbidden in -rc6 time. If you believe noone depends on the name, argue your case, and it might be possible to change it in -rc0. If someone uses the name, they care about the device, and you can't take it away. > >No. IMO, it depends on if the interface is used or not. > >If hwmon I/F is used, we can not take it away, nor change its name. > > Even if the use doesn't depend on that name ? If the use doesn't depend on the name, you may get away with changing the name. (But not in -rc6.) > >If thermal zone I/F is used, we can not change it's 'type' name to be > >compatible with new hwmon API. > > You mean you can not fix the name to be compatible with libsensors. > > Makes me wonder if there shouldn't be a rule that exploits must not > be fixed if already exploited. That is not useful argumentation. Pavel
On Wed 2017-01-25 13:09:18, Pali Rohár wrote: > On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote: > > Hi! > > > > > > > >>Right. > > > > > >> > > > > > >>Before reverting, can you please try if this patch works or not? > > > > > > > > > > > >Not really. Revert now. Sorry. > > > > > > > > > > > >Are you sure? This does not look equivalent to me at all. > > > > > > > > > > > >"name" file handling moved from drivers to the core, which added some > > > > > >crazy checks what name can contain. Even if this "works", what is the > > > > > >expected effect on the "name" file? > > > > > > > > > > > The hwmon name attribute must not include '-', as documented in > > > > > Documentation/hwmon/sysfs-interface. Is enforcing that 'crazy' ? > > > > > Maybe in your world, but not in mine. > > > > > > > > Well, lets revert the patch and then we can discuss what to do with > > > > the "name" problem. > > > > Ok, so the patch is on the way in. What to do next? > > > > pavel@n900:/sys/class/hwmon$ cat hwmon0/name > > bq27200-0 > > pavel@n900:/sys/class/hwmon$ cat hwmon1/name > > rx51-battery > > > > > To provide some detail: libsensors gets just as confused with wildcards > > > and whitespace/newline as it does with '-' in the reported name, which > > > is why those are blocked by the new API. > > > > Ok... Question is "does someone actually use hwmon*/name on N900"? If > > so, we can't change it, but it is well possible that noone is. > > IIRC hwmon is used on Nokia N900. > > But I have not seen hwmon devices for bq27200 and rx51-battery yet. > Those are power supply driver and auto-exporting them also via hwmon is > something new, right? If yes, then we can use any name for those new > hwmon devices as they cannot break userspace... as there is no userspace > application for them. So maemo does not use that. Good. Nor does my Debian userspace. Hey, maybe you can get away with changing that. Including batteries in hwmon interface seems useful, and I'd keep that. Pavel
On 01/27/2017 03:16 AM, Pavel Machek wrote: > Hi! > >>>> That sounds like fun. Changing bq27200-0 to bq27200_0 is Forbidden by >>>> the ABI Police, but taking the entire device away is ok. > > Changing bq27200-0 to bq27200_0 is forbidden in -rc6 time. If you > believe noone depends on the name, argue your case, and it might be > possible to change it in -rc0. > > If someone uses the name, they care about the device, and you can't > take it away. > >>> No. IMO, it depends on if the interface is used or not. >>> If hwmon I/F is used, we can not take it away, nor change its name. >> >> Even if the use doesn't depend on that name ? > > If the use doesn't depend on the name, you may get away with changing > the name. (But not in -rc6.) > >>> If thermal zone I/F is used, we can not change it's 'type' name to be >>> compatible with new hwmon API. >> >> You mean you can not fix the name to be compatible with libsensors. >> >> Makes me wonder if there shouldn't be a rule that exploits must not >> be fixed if already exploited. > > That is not useful argumentation. Oh, but it is. Providing a valid name is a bug fix for me. For you it is an ABI change. So the ultimate question is what counts as an ABI change and what counts as a bug fix. Guenter
On 01/27/2017 03:09 AM, Pavel Machek wrote: > >>> If this is the case, you'd better set >>> (struct thermal_zone_params)->no_hwmon when registering the thermal >>> zone device, in which case, the hwmon device will not be created. >>> >>> In fact, I'd prefer to change tzp->no_hwmon to tzp->hwmon to not create >>> hwmon I/F by default, and see if there is anyone using it. If yes, we >>> can set the flag in soc thermal driver, explicitly, at meantime, a >>> hwmon compatible name is required. >>> >>> But one foreseeable result is that we may get bug reports from end user >>> that some sensors (acpitz, etc) are gone in 'sensors' output. And TBH, >>> I'm not quite sure if this can be counted as a regression or not. >>> >> >> That sounds like fun. Changing bq27200-0 to bq27200_0 is Forbidden by >> the ABI Police, but taking the entire device away is ok. > > Stop trying to break other people's systems. Thanks you. > You still have not shown that it does. Actually, I am trying to _fix_ other people's systems (those using libsensors). Guenter
On Thu, 2017-01-26 at 22:27 -0800, Guenter Roeck wrote: > On 01/26/2017 07:39 PM, Zhang Rui wrote: > > > > On Thu, 2017-01-26 at 18:03 -0800, Guenter Roeck wrote: > > > > > > On 01/26/2017 05:37 PM, Zhang Rui wrote: > > > > > > > > > > > > On Wed, 2017-01-25 at 13:09 +0100, Pali Rohár wrote: > > > > > > > > > > > > > > > On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote: > > > > > > > > > > > > > > > > > > > > > > > > Hi! > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Right. > > > > > > > > > > > > > > > > > > > > > > Before reverting, can you please try if this > > > > > > > > > > > patch > > > > > > > > > > > works > > > > > > > > > > > or not? > > > > > > > > > > Not really. Revert now. Sorry. > > > > > > > > > > > > > > > > > > > > Are you sure? This does not look equivalent to me > > > > > > > > > > at > > > > > > > > > > all. > > > > > > > > > > > > > > > > > > > > "name" file handling moved from drivers to the > > > > > > > > > > core, > > > > > > > > > > which > > > > > > > > > > added some > > > > > > > > > > crazy checks what name can contain. Even if this > > > > > > > > > > "works", > > > > > > > > > > what is the > > > > > > > > > > expected effect on the "name" file? > > > > > > > > > > > > > > > > > > > The hwmon name attribute must not include '-', as > > > > > > > > > documented > > > > > > > > > in > > > > > > > > > Documentation/hwmon/sysfs-interface. Is enforcing > > > > > > > > > that > > > > > > > > > 'crazy' ? > > > > > > > > > Maybe in your world, but not in mine. > > > > > > > > Well, lets revert the patch and then we can discuss > > > > > > > > what to > > > > > > > > do > > > > > > > > with > > > > > > > > the "name" problem. > > > > > > Ok, so the patch is on the way in. What to do next? > > > > > > > > > > > > pavel@n900:/sys/class/hwmon$ cat hwmon0/name > > > > > > bq27200-0 > > > > > > pavel@n900:/sys/class/hwmon$ cat hwmon1/name > > > > > > rx51-battery > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > To provide some detail: libsensors gets just as confused > > > > > > > with > > > > > > > wildcards > > > > > > > and whitespace/newline as it does with '-' in the > > > > > > > reported > > > > > > > name, > > > > > > > which > > > > > > > is why those are blocked by the new API. > > > > > > Ok... Question is "does someone actually use hwmon*/name on > > > > > > N900"? > > > > > > If > > > > > > so, we can't change it, but it is well possible that noone > > > > > > is. > > > > > IIRC hwmon is used on Nokia N900. > > > > > > > > > > But I have not seen hwmon devices for bq27200 and rx51- > > > > > battery > > > > > yet. > > > > > Those are power supply driver and auto-exporting them also > > > > > via > > > > > hwmon > > > > > is > > > > > something new, right? If yes, then we can use any name for > > > > > those > > > > > new > > > > > hwmon devices as they cannot break userspace... as there is > > > > > no > > > > > userspace > > > > > application for them. > > > > > > > > > If this is the case, you'd better set > > > > (struct thermal_zone_params)->no_hwmon when registering the > > > > thermal > > > > zone device, in which case, the hwmon device will not be > > > > created. > > > > > > > > In fact, I'd prefer to change tzp->no_hwmon to tzp->hwmon to > > > > not > > > > create > > > > hwmon I/F by default, and see if there is anyone using it. If > > > > yes, > > > > we > > > > can set the flag in soc thermal driver, explicitly, at > > > > meantime, a > > > > hwmon compatible name is required. > > > > > > > > But one foreseeable result is that we may get bug reports from > > > > end > > > > user > > > > that some sensors (acpitz, etc) are gone in 'sensors' output. > > > > And > > > > TBH, > > > > I'm not quite sure if this can be counted as a regression or > > > > not. > > > > > > > That sounds like fun. Changing bq27200-0 to bq27200_0 is > > > Forbidden by > > > the ABI Police, but taking the entire device away is ok. > > > > > No. IMO, it depends on if the interface is used or not. > > If hwmon I/F is used, we can not take it away, nor change its name. > Even if the use doesn't depend on that name ? > when I said "the interface is used", I mean the name string is used. > > > > If thermal zone I/F is used, we can not change it's 'type' name to > > be > > compatible with new hwmon API. > > > You mean you can not fix the name to be compatible with libsensors. > We can try to convert it to a libsensor-compatible string, either for hwmon only, or for both thermal and hwmon. But this is an ABI change, right? And my understanding about the ABI change is that, if no one cares about it, we're okay, or else, this is a regression, and we need to fall back to the previous ABI immediately. In order to change it in a long run, we need to make a note in Documentation/ABI/, and change it sometime in the future (a couple of release cycles or even more). This also happens when I was trying to delete the obsoltete thanks, rui > Makes me wonder if there shouldn't be a rule that exploits must not > be fixed if already exploited. > > Guenter > > > > > > > > > Anyway, sounds good to me. No one will use something that isn't > > > there, > > > and no one will realize that it could have been there, so I don't > > > expect > > > anyone to complain. > > Yes, I agree. > > > > thanks, > > rui > >
On Fri, Jan 27, 2017 at 10:40:33PM +0800, Zhang Rui wrote: > > > > > > If thermal zone I/F is used, we can not change it's 'type' name to > > > be > > > compatible with new hwmon API. > > > > > You mean you can not fix the name to be compatible with libsensors. > > > > We can try to convert it to a libsensor-compatible string, either for > hwmon only, or for both thermal and hwmon. But this is an ABI change, > right? Let's go back to the basics. Fact is that the thermal subsystem registers hwmon devices with 'name' attributes which violate the documented hardware monitoring ABI. I think we can consider this undisputed. The rest is pretty much all opinion. Is a change in a driver to stop violating a documented ABI an ABI change or a bug fix ? In other words, does a driver violating a documented ABI make that ABI violation part of the ABI ? Quite interesting questions. My take is that it is a bug fix, others apparently have the strong opinion that potential users of such an ABI violation have priority, and that a violation of a documented ABI _does_ make this violation part of the ABI. I'll leave it at that. I tried to make my position clear, but it appears that I am quite alone in my opinion. With that, I'll leave it up to you to decide how to proceed. Guenter
Hi! > >>That sounds like fun. Changing bq27200-0 to bq27200_0 is Forbidden by > >>the ABI Police, but taking the entire device away is ok. > > > >Stop trying to break other people's systems. Thanks you. > You still have not shown that it does. Actually, I am trying to _fix_ > other people's systems (those using libsensors). No, I don't know that it does. Please avoid 1) calling me police and 2) claiming that taking away entire devices is okay (when it probably is not). And no, I don't know if anyone uses hwmon ABI on affected N900. [And how many other devices are affected, '-' seems to be quite common character in kernel names.] If I knew about concrete breakage, we'd have very different discussion. Well, so first things first. What about fixing libsensors to allow '-' and '*' in the name? Pavel
On Fri 2017-01-27 06:13:43, Guenter Roeck wrote: > On 01/27/2017 03:16 AM, Pavel Machek wrote: > >Hi! > > > >>>>That sounds like fun. Changing bq27200-0 to bq27200_0 is Forbidden by > >>>>the ABI Police, but taking the entire device away is ok. > > > >Changing bq27200-0 to bq27200_0 is forbidden in -rc6 time. If you > >believe noone depends on the name, argue your case, and it might be > >possible to change it in -rc0. > > > >If someone uses the name, they care about the device, and you can't > >take it away. > > > >>>No. IMO, it depends on if the interface is used or not. > >>>If hwmon I/F is used, we can not take it away, nor change its name. > >> > >>Even if the use doesn't depend on that name ? > > > >If the use doesn't depend on the name, you may get away with changing > >the name. (But not in -rc6.) > > > >>>If thermal zone I/F is used, we can not change it's 'type' name to be > >>>compatible with new hwmon API. > >> > >>You mean you can not fix the name to be compatible with libsensors. > >> > >>Makes me wonder if there shouldn't be a rule that exploits must not > >>be fixed if already exploited. > > > >That is not useful argumentation. > > Oh, but it is. Providing a valid name is a bug fix for me. For you it is > an ABI change. So the ultimate question is what counts as an ABI change > and what counts as a bug fix. You may not fix bugs if it breaks someone's configuration. Now, obviously security is something we can't sacrifice. But if docs don't match the code.. we fix the docs, not the code. Do you want to call me? I'll give you cellphone number. Pavel
On Fri 2017-01-27 09:20:39, Guenter Roeck wrote: > On Fri, Jan 27, 2017 at 10:40:33PM +0800, Zhang Rui wrote: > > > > > > > > If thermal zone I/F is used, we can not change it's 'type' name to > > > > be > > > > compatible with new hwmon API. > > > > > > > You mean you can not fix the name to be compatible with libsensors. > > > > > > > We can try to convert it to a libsensor-compatible string, either for > > hwmon only, or for both thermal and hwmon. But this is an ABI change, > > right? > > Let's go back to the basics. > > Fact is that the thermal subsystem registers hwmon devices with 'name' > attributes which violate the documented hardware monitoring ABI. > I think we can consider this undisputed. > > The rest is pretty much all opinion. > > Is a change in a driver to stop violating a documented ABI an ABI change > or a bug fix ? In other words, does a driver violating a documented ABI > make that ABI violation part of the ABI ? It can be both. But "no regressions" takes precedence over "documentation". > Quite interesting questions. My take is that it is a bug fix, others > apparently have the strong opinion that potential users of such an ABI > violation have priority, and that a violation of a documented ABI _does_ > make this violation part of the ABI. "Potential" users you can work around. "Real" users are problem. And yes, I have strong opinion about release candidates. Outside that, see my next mail. Pavel
Hi! > > > > That sounds like fun. Changing bq27200-0 to bq27200_0 is > > > > Forbidden by > > > > the ABI Police, but taking the entire device away is ok. > > > > > > > No. IMO, it depends on if the interface is used or not. > > > If hwmon I/F is used, we can not take it away, nor change its name. > > Even if the use doesn't depend on that name ? > > > when I said "the interface is used", I mean the name string is used. > > > > > > If thermal zone I/F is used, we can not change it's 'type' name to > > > be > > > compatible with new hwmon API. > > > > > You mean you can not fix the name to be compatible with libsensors. > > > > We can try to convert it to a libsensor-compatible string, either for > hwmon only, or for both thermal and hwmon. But this is an ABI change, > right? > And my understanding about the ABI change is that, if no one cares > about it, we're okay, or else, this is a regression, and we need to > fall back to the previous ABI immediately. In order to change it in a > long run, we need to make a note in Documentation/ABI/, and change it > sometime in the future (a couple of release cycles or even more). If no one cares, you can get away with the change. So I guess that's what we should try. In -rc1. We should _not_ do "this changes in 2 years" dance, as that would increase chance for someone to use the old name. So I guess changing to libsensor-compatible name in v4.11-rc0 (maybe with note in the ABI documenation) is the best plan. If someone complains, we'll have to revert and think about something else. Pavel
diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c index c4a508a..0c6789f 100644 --- a/drivers/thermal/thermal_hwmon.c +++ b/drivers/thermal/thermal_hwmon.c @@ -157,8 +157,8 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz) INIT_LIST_HEAD(&hwmon->tz_list); strlcpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH); - hwmon->device = hwmon_device_register_with_info(NULL, hwmon->type, - hwmon, NULL, NULL); + hwmon->device = hwmon_device_register_with_info(NULL, NULL, NULL, + NULL, NULL); if (IS_ERR(hwmon->device)) { result = PTR_ERR(hwmon->device); goto free_mem;