v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900

It seems that battery driver stopped working on N900 between -rc4 and -rc5.

Quoted 2 lines · On Monday 23 January 2017 13:40:58 Pavel Machek wrote:

It seems that battery driver stopped working on N900 between -rc4 and -rc5.

Which one of those 4 drivers stopped working?

It seems that battery driver stopped working on N900 between -rc4 and -rc5.

Hi!

Quoted 2 lines

It seems that battery driver stopped working on N900 between -rc4 and -rc5.

pavel at amd:/data/l/linux-n900$ git bisect log
# bad: [41f283973653dc44af47585fa79fb6da6ffdc2e2] Merge /data//l/clean-cg into mini-v4.10
# good: [4f8d9925eb1de1ec1c4183164f1bc7b01f5ae185] It seems mainline -rc4 did have a fix for the sound issue. Use that one.
git bisect start 'mini-v4.10' '4f8d9925eb1de1ec1c4183164f1bc7b01f5ae185'
# good: [e90665a5d38b17fdbe484a85fbba917a7006522d] Merge tag 'ceph-for-4.10-rc5' of git://github.com/ceph/ceph-client
git bisect good e90665a5d38b17fdbe484a85fbba917a7006522d
# good: [83fd57a740bb19286959b3085eb93532f3e7ef2c] Merge tag 'powerpc-4.10-2' of
git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux
git bisect good 83fd57a740bb19286959b3085eb93532f3e7ef2c
# good: [c497f8d17246720afe680ea1a8fa6e48e75af852] Merge tag 'usb-4.10-rc5' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
git bisect good c497f8d17246720afe680ea1a8fa6e48e75af852
# bad: [bb6c01c2dde67b165cf7c808b0f00677b6f94b96] Merge branch 'for-rc' of git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux
git bisect bad bb6c01c2dde67b165cf7c808b0f00677b6f94b96
# good: [18591add41ec9558ce0e32ef88626c18cc70c686] thermal: rockchip:
handle set_trips without the trip points
git bisect good 18591add41ec9558ce0e32ef88626c18cc70c686
# bad: [f53345e8cf027d03187b9417f1f8883c516e1a5b] thermal: core: move tz->device.groups cleanup to thermal_release
git bisect bad f53345e8cf027d03187b9417f1f8883c516e1a5b
# bad: [7611fb68062f8d7f416f3272894d1edf7bbff29c] thermal:
thermal_hwmon: Convert to hwmon_device_register_with_info() git bisect bad 7611fb68062f8d7f416f3272894d1edf7bbff29c

# first bad commit: [7611fb68062f8d7f416f3272894d1edf7bbff29c]
thermal: thermal_hwmon: Convert to hwmon_device_register_with_info()
pavel at amd:/data/l/linux-n900$

pavel at amd:/data/l/linux-n900$ git bisect log
# bad: [41f283973653dc44af47585fa79fb6da6ffdc2e2] Merge /data//l/clean-cg into mini-v4.10
# good: [4f8d9925eb1de1ec1c4183164f1bc7b01f5ae185] It seems mainline -rc4 did have a fix for the sound issue. Use that one.
git bisect start 'mini-v4.10' '4f8d9925eb1de1ec1c4183164f1bc7b01f5ae185'
# good: [e90665a5d38b17fdbe484a85fbba917a7006522d] Merge tag 'ceph-for-4.10-rc5' of git://github.com/ceph/ceph-client
git bisect good e90665a5d38b17fdbe484a85fbba917a7006522d
# good: [83fd57a740bb19286959b3085eb93532f3e7ef2c] Merge tag 'powerpc-4.10-2' of
git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux
git bisect good 83fd57a740bb19286959b3085eb93532f3e7ef2c
# good: [c497f8d17246720afe680ea1a8fa6e48e75af852] Merge tag 'usb-4.10-rc5' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
git bisect good c497f8d17246720afe680ea1a8fa6e48e75af852
# bad: [bb6c01c2dde67b165cf7c808b0f00677b6f94b96] Merge branch 'for-rc' of git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux
git bisect bad bb6c01c2dde67b165cf7c808b0f00677b6f94b96
# good: [18591add41ec9558ce0e32ef88626c18cc70c686] thermal: rockchip:
handle set_trips without the trip points
git bisect good 18591add41ec9558ce0e32ef88626c18cc70c686
# bad: [f53345e8cf027d03187b9417f1f8883c516e1a5b] thermal: core: move tz->device.groups cleanup to thermal_release
git bisect bad f53345e8cf027d03187b9417f1f8883c516e1a5b
# bad: [7611fb68062f8d7f416f3272894d1edf7bbff29c] thermal:
thermal_hwmon: Convert to hwmon_device_register_with_info() git bisect bad 7611fb68062f8d7f416f3272894d1edf7bbff29c

# first bad commit: [7611fb68062f8d7f416f3272894d1edf7bbff29c]
thermal: thermal_hwmon: Convert to hwmon_device_register_with_info()
pavel at amd:/data/l/linux-n900$

7611fb68062f8d7f416f3272894d1edf7bbff29c is the first bad commit commit 7611fb68062f8d7f416f3272894d1edf7bbff29c

Author: Fabio Estevam <[email protected]>
Date:   Tue Dec 27 15:31:49 2016 -0200

thermal: thermal_hwmon: Convert to
hwmon_device_register_with_info()

Booting Linux on a mx6q based board leads to the following warning:

(NULL device *): hwmon_device_register() is deprecated. Please convert the
driver to use hwmon_device_register_with_info().

,so do as suggested.

Also, this results in the core taking care of creating the 'name'
attribute, so drop the code doing that from the thermal driver.

Suggested-by Guenter Roeck <[email protected]>

Signed-off-by Fabio Estevam <[email protected]>

Signed-off-by Zhang Rui <[email protected]>

:040000 040000 b3f6095ab53677277c2f9d75cb6430afb6825765
156a18e983a22b8ae43c7b312a4e1c63017f0e26 M drivers

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.

Quoted 106 lines · Pavel Machek

Hi!

Quoted 2 lines

It seems that battery driver stopped working on N900 between -rc4 and -rc5.

pavel at amd:/data/l/linux-n900$ git bisect log
# bad: [41f283973653dc44af47585fa79fb6da6ffdc2e2] Merge /data//l/clean-cg into mini-v4.10
# good: [4f8d9925eb1de1ec1c4183164f1bc7b01f5ae185] It seems mainline -rc4 did have a fix for the sound issue. Use that one.
git bisect start 'mini-v4.10' '4f8d9925eb1de1ec1c4183164f1bc7b01f5ae185'
# good: [e90665a5d38b17fdbe484a85fbba917a7006522d] Merge tag 'ceph-for-4.10-rc5' of git://github.com/ceph/ceph-client
git bisect good e90665a5d38b17fdbe484a85fbba917a7006522d
# good: [83fd57a740bb19286959b3085eb93532f3e7ef2c] Merge tag 'powerpc-4.10-2' of
git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux
git bisect good 83fd57a740bb19286959b3085eb93532f3e7ef2c
# good: [c497f8d17246720afe680ea1a8fa6e48e75af852] Merge tag 'usb-4.10-rc5' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
git bisect good c497f8d17246720afe680ea1a8fa6e48e75af852
# bad: [bb6c01c2dde67b165cf7c808b0f00677b6f94b96] Merge branch 'for-rc' of git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux
git bisect bad bb6c01c2dde67b165cf7c808b0f00677b6f94b96
# good: [18591add41ec9558ce0e32ef88626c18cc70c686] thermal: rockchip:
handle set_trips without the trip points
git bisect good 18591add41ec9558ce0e32ef88626c18cc70c686
# bad: [f53345e8cf027d03187b9417f1f8883c516e1a5b] thermal: core: move tz->device.groups cleanup to thermal_release
git bisect bad f53345e8cf027d03187b9417f1f8883c516e1a5b
# bad: [7611fb68062f8d7f416f3272894d1edf7bbff29c] thermal:
thermal_hwmon: Convert to hwmon_device_register_with_info() git bisect bad 7611fb68062f8d7f416f3272894d1edf7bbff29c

# first bad commit: [7611fb68062f8d7f416f3272894d1edf7bbff29c]
thermal: thermal_hwmon: Convert to hwmon_device_register_with_info()
pavel at amd:/data/l/linux-n900$

pavel at amd:/data/l/linux-n900$ git bisect log
# bad: [41f283973653dc44af47585fa79fb6da6ffdc2e2] Merge /data//l/clean-cg into mini-v4.10
# good: [4f8d9925eb1de1ec1c4183164f1bc7b01f5ae185] It seems mainline -rc4 did have a fix for the sound issue. Use that one.
git bisect start 'mini-v4.10' '4f8d9925eb1de1ec1c4183164f1bc7b01f5ae185'
# good: [e90665a5d38b17fdbe484a85fbba917a7006522d] Merge tag 'ceph-for-4.10-rc5' of git://github.com/ceph/ceph-client
git bisect good e90665a5d38b17fdbe484a85fbba917a7006522d
# good: [83fd57a740bb19286959b3085eb93532f3e7ef2c] Merge tag 'powerpc-4.10-2' of
git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux
git bisect good 83fd57a740bb19286959b3085eb93532f3e7ef2c
# good: [c497f8d17246720afe680ea1a8fa6e48e75af852] Merge tag 'usb-4.10-rc5' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
git bisect good c497f8d17246720afe680ea1a8fa6e48e75af852
# bad: [bb6c01c2dde67b165cf7c808b0f00677b6f94b96] Merge branch 'for-rc' of git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux
git bisect bad bb6c01c2dde67b165cf7c808b0f00677b6f94b96
# good: [18591add41ec9558ce0e32ef88626c18cc70c686] thermal: rockchip:
handle set_trips without the trip points
git bisect good 18591add41ec9558ce0e32ef88626c18cc70c686
# bad: [f53345e8cf027d03187b9417f1f8883c516e1a5b] thermal: core: move tz->device.groups cleanup to thermal_release
git bisect bad f53345e8cf027d03187b9417f1f8883c516e1a5b
# bad: [7611fb68062f8d7f416f3272894d1edf7bbff29c] thermal:
thermal_hwmon: Convert to hwmon_device_register_with_info() git bisect bad 7611fb68062f8d7f416f3272894d1edf7bbff29c

# first bad commit: [7611fb68062f8d7f416f3272894d1edf7bbff29c]
thermal: thermal_hwmon: Convert to hwmon_device_register_with_info()
pavel at amd:/data/l/linux-n900$

7611fb68062f8d7f416f3272894d1edf7bbff29c is the first bad commit commit 7611fb68062f8d7f416f3272894d1edf7bbff29c

Author: Fabio Estevam <[email protected]>
Date:   Tue Dec 27 15:31:49 2016 -0200

thermal: thermal_hwmon: Convert to
hwmon_device_register_with_info()

Booting Linux on a mx6q based board leads to the following warning:

(NULL device *): hwmon_device_register() is deprecated. Please convert the
driver to use hwmon_device_register_with_info().

,so do as suggested.

Also, this results in the core taking care of creating the 'name'
attribute, so drop the code doing that from the thermal driver.

Suggested-by Guenter Roeck <[email protected]>

Signed-off-by Fabio Estevam <[email protected]>

Signed-off-by Zhang Rui <[email protected]>

:040000 040000 b3f6095ab53677277c2f9d75cb6430afb6825765
156a18e983a22b8ae43c7b312a4e1c63017f0e26 M drivers

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?

Quoted 3 lines

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.

Quoted 107 lines · Guenter Roeck

Quoted 106 lines · Pavel Machek

Hi!

Quoted 2 lines

It seems that battery driver stopped working on N900 between -rc4 and -rc5.

pavel at amd:/data/l/linux-n900$ git bisect log
# bad: [41f283973653dc44af47585fa79fb6da6ffdc2e2] Merge /data//l/clean-cg into mini-v4.10
# good: [4f8d9925eb1de1ec1c4183164f1bc7b01f5ae185] It seems mainline -rc4 did have a fix for the sound issue. Use that one.
git bisect start 'mini-v4.10' '4f8d9925eb1de1ec1c4183164f1bc7b01f5ae185'
# good: [e90665a5d38b17fdbe484a85fbba917a7006522d] Merge tag 'ceph-for-4.10-rc5' of git://github.com/ceph/ceph-client
git bisect good e90665a5d38b17fdbe484a85fbba917a7006522d
# good: [83fd57a740bb19286959b3085eb93532f3e7ef2c] Merge tag 'powerpc-4.10-2' of
git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux
git bisect good 83fd57a740bb19286959b3085eb93532f3e7ef2c
# good: [c497f8d17246720afe680ea1a8fa6e48e75af852] Merge tag 'usb-4.10-rc5' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
git bisect good c497f8d17246720afe680ea1a8fa6e48e75af852
# bad: [bb6c01c2dde67b165cf7c808b0f00677b6f94b96] Merge branch 'for-rc' of git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux
git bisect bad bb6c01c2dde67b165cf7c808b0f00677b6f94b96
# good: [18591add41ec9558ce0e32ef88626c18cc70c686] thermal: rockchip:
handle set_trips without the trip points
git bisect good 18591add41ec9558ce0e32ef88626c18cc70c686
# bad: [f53345e8cf027d03187b9417f1f8883c516e1a5b] thermal: core: move tz->device.groups cleanup to thermal_release
git bisect bad f53345e8cf027d03187b9417f1f8883c516e1a5b
# bad: [7611fb68062f8d7f416f3272894d1edf7bbff29c] thermal:
thermal_hwmon: Convert to hwmon_device_register_with_info() git bisect bad 7611fb68062f8d7f416f3272894d1edf7bbff29c

# first bad commit: [7611fb68062f8d7f416f3272894d1edf7bbff29c]
thermal: thermal_hwmon: Convert to hwmon_device_register_with_info()
pavel at amd:/data/l/linux-n900$

pavel at amd:/data/l/linux-n900$ git bisect log
# bad: [41f283973653dc44af47585fa79fb6da6ffdc2e2] Merge /data//l/clean-cg into mini-v4.10
# good: [4f8d9925eb1de1ec1c4183164f1bc7b01f5ae185] It seems mainline -rc4 did have a fix for the sound issue. Use that one.
git bisect start 'mini-v4.10' '4f8d9925eb1de1ec1c4183164f1bc7b01f5ae185'
# good: [e90665a5d38b17fdbe484a85fbba917a7006522d] Merge tag 'ceph-for-4.10-rc5' of git://github.com/ceph/ceph-client
git bisect good e90665a5d38b17fdbe484a85fbba917a7006522d
# good: [83fd57a740bb19286959b3085eb93532f3e7ef2c] Merge tag 'powerpc-4.10-2' of
git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux
git bisect good 83fd57a740bb19286959b3085eb93532f3e7ef2c
# good: [c497f8d17246720afe680ea1a8fa6e48e75af852] Merge tag 'usb-4.10-rc5' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
git bisect good c497f8d17246720afe680ea1a8fa6e48e75af852
# bad: [bb6c01c2dde67b165cf7c808b0f00677b6f94b96] Merge branch 'for-rc' of git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux
git bisect bad bb6c01c2dde67b165cf7c808b0f00677b6f94b96
# good: [18591add41ec9558ce0e32ef88626c18cc70c686] thermal: rockchip:
handle set_trips without the trip points
git bisect good 18591add41ec9558ce0e32ef88626c18cc70c686
# bad: [f53345e8cf027d03187b9417f1f8883c516e1a5b] thermal: core: move tz->device.groups cleanup to thermal_release
git bisect bad f53345e8cf027d03187b9417f1f8883c516e1a5b
# bad: [7611fb68062f8d7f416f3272894d1edf7bbff29c] thermal:
thermal_hwmon: Convert to hwmon_device_register_with_info() git bisect bad 7611fb68062f8d7f416f3272894d1edf7bbff29c

# first bad commit: [7611fb68062f8d7f416f3272894d1edf7bbff29c]
thermal: thermal_hwmon: Convert to hwmon_device_register_with_info()
pavel at amd:/data/l/linux-n900$

7611fb68062f8d7f416f3272894d1edf7bbff29c is the first bad commit commit 7611fb68062f8d7f416f3272894d1edf7bbff29c

Author: Fabio Estevam <[email protected]>
Date:   Tue Dec 27 15:31:49 2016 -0200

thermal: thermal_hwmon: Convert to
hwmon_device_register_with_info()

Booting Linux on a mx6q based board leads to the following warning:

(NULL device *): hwmon_device_register() is deprecated. Please convert the
driver to use hwmon_device_register_with_info().

,so do as suggested.

Also, this results in the core taking care of creating the 'name'
attribute, so drop the code doing that from the thermal driver.

Suggested-by Guenter Roeck <[email protected]>

Signed-off-by Fabio Estevam <[email protected]>

Signed-off-by Zhang Rui <[email protected]>

:040000 040000 b3f6095ab53677277c2f9d75cb6430afb6825765
156a18e983a22b8ae43c7b312a4e1c63017f0e26 M drivers

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?

Quoted 7 lines

Quoted 3 lines

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 at 68000000/spi at 48098000/tsc2005 at 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
[    2.399902] omap_hsmmc 4809c000.mmc: GPIO lookup for consumer cd

Quoted 5 lines

Quoted 5 lines

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?

Quoted 7 lines

Quoted 3 lines

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 at 68000000/spi at 48098000/tsc2005 at 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

Hi!

Quoted 16 lines

Quoted 5 lines

Quoted 5 lines

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?

Quoted 7 lines

Quoted 3 lines

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:

Quoted 6 lines

[    2.374877] of_get_named_gpiod_flags: parsed 'reset-gpios' property
of node '/ocp at 68000000/spi at 48098000/tsc2005 at 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:

drivers/hwmon/hwmon.c +3 -1

index 3932f92..fe5ec82 100644
@@ -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 at n900:~$ dmesg | grep -5 Invalid

[    0.829650] of_get_named_gpiod_flags: parsed 'gpio-reset' property
of node '/ocp at 68000000/i2c at 48072000/tlv320aic3x at 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 at 68000000/i2c at 48072000/lp5523 at 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

[    2.064544] bq27xxx-battery 2-0055: failed to register battery
[    2.064605] bq27xxx-battery: probe of 2-0055 failed with error -22
[    2.065368] of_get_named_gpiod_flags: parsed 'power-gpio' property
of node '/ocp at 68000000/i2c at 48072000/tpa6130a2 at 60[0]' - status (0)
[    2.083221] bq2415x-charger 2-006b: automode supported, waiting for
events
[    2.084442] bq2415x-charger 2-006b: driver registered
--
[    2.369842] g_ether gadget: g_ether ready
[    2.377197] tsc2005 spi1.0: GPIO lookup for consumer reset
[    2.377227] tsc2005 spi1.0: using device tree for GPIO lookup
[    2.377288] of_get_named_gpiod_flags: parsed 'reset-gpios' property
of node '/ocp at 68000000/spi at 48098000/tsc2005 at 0[0]' - status (0)
[    2.378936] input: TSC2005 touchscreen as
/devices/platform/68000000.ocp/48098000.spi/spi_master/spi1/spi1.0/input/input5
[    2.395111] hwmon: Invalid character detected: rx51-battery
[    2.402587] rx51-battery: probe of n900-battery failed with error
-22
[    2.410247] omap_hsmmc 4809c000.mmc: GPIO lookup for consumer cd
[    2.410247] omap_hsmmc 4809c000.mmc: using device tree for GPIO
lookup
[    2.410278] of_get_named_gpiod_flags: can't parse 'cd-gpios'
property of node '/ocp at 68000000/mmc at 4809c000[0]'
[    2.410278] of_get_named_gpiod_flags: can't parse 'cd-gpio'

property of node '/ocp at 68000000/mmc at 4809c000[0]'

Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Quoted 45 lines · Pavel Machek

Hi!

Quoted 16 lines

Quoted 5 lines

Quoted 5 lines

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?

Quoted 7 lines

Quoted 3 lines

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:

Quoted 6 lines

[    2.374877] of_get_named_gpiod_flags: parsed 'reset-gpios' property
of node '/ocp at 68000000/spi at 48098000/tsc2005 at 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:

drivers/hwmon/hwmon.c +3 -1

index 3932f92..fe5ec82 100644
@@ -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)

Quoted 9 lines

pavel at n900:~$ dmesg | grep -5 Invalid

[    0.829650] of_get_named_gpiod_flags: parsed 'gpio-reset' property
of node '/ocp at 68000000/i2c at 48072000/tlv320aic3x at 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 at 68000000/i2c at 48072000/lp5523 at 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.

From 79320ea3721314ec29ed6cdd6987ff922b11d6f9 Mon Sep 17 00:00:00 2001

Guenter

Quoted 32 lines

[    2.064544] bq27xxx-battery 2-0055: failed to register battery
[    2.064605] bq27xxx-battery: probe of 2-0055 failed with error -22
[    2.065368] of_get_named_gpiod_flags: parsed 'power-gpio' property
of node '/ocp at 68000000/i2c at 48072000/tpa6130a2 at 60[0]' - status (0)
[    2.083221] bq2415x-charger 2-006b: automode supported, waiting for
events
[    2.084442] bq2415x-charger 2-006b: driver registered
--
[    2.369842] g_ether gadget: g_ether ready
[    2.377197] tsc2005 spi1.0: GPIO lookup for consumer reset
[    2.377227] tsc2005 spi1.0: using device tree for GPIO lookup
[    2.377288] of_get_named_gpiod_flags: parsed 'reset-gpios' property
of node '/ocp at 68000000/spi at 48098000/tsc2005 at 0[0]' - status (0)
[    2.378936] input: TSC2005 touchscreen as
/devices/platform/68000000.ocp/48098000.spi/spi_master/spi1/spi1.0/input/input5
[    2.395111] hwmon: Invalid character detected: rx51-battery
[    2.402587] rx51-battery: probe of n900-battery failed with error
-22
[    2.410247] omap_hsmmc 4809c000.mmc: GPIO lookup for consumer cd
[    2.410247] omap_hsmmc 4809c000.mmc: using device tree for GPIO
lookup
[    2.410278] of_get_named_gpiod_flags: can't parse 'cd-gpios'
property of node '/ocp at 68000000/mmc at 4809c000[0]'
[    2.410278] of_get_named_gpiod_flags: can't parse 'cd-gpio'

property of node '/ocp at 68000000/mmc at 4809c000[0]'

Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Quoted 46 lines · Guenter Roeck

Quoted 45 lines · Pavel Machek

Hi!

Quoted 16 lines

Quoted 5 lines

Quoted 5 lines

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?

Quoted 7 lines

Quoted 3 lines

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:

Quoted 6 lines

[    2.374877] of_get_named_gpiod_flags: parsed 'reset-gpios' property
of node '/ocp at 68000000/spi at 48098000/tsc2005 at 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:

drivers/hwmon/hwmon.c +3 -1

index 3932f92..fe5ec82 100644
@@ -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)

Quoted 12 lines

Quoted 9 lines

pavel at n900:~$ dmesg | grep -5 Invalid

[    0.829650] of_get_named_gpiod_flags: parsed 'gpio-reset' property
of node '/ocp at 68000000/i2c at 48072000/tlv320aic3x at 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 at 68000000/i2c at 48072000/lp5523 at 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

Quoted 1 line

From 79320ea3721314ec29ed6cdd6987ff922b11d6f9 Mon Sep 17 00:00:00 2001

From: Zhang Rui <[email protected]>
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 <[email protected]>

Signed-off-by Zhang Rui <[email protected]>

---
 drivers/thermal/thermal_hwmon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/thermal/thermal_hwmon.c +2 -2

index c4a508a..0c6789f 100644
@@ -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;

Quoted 59 lines · Zhang Rui

Quoted 46 lines · Guenter Roeck

Quoted 45 lines · Pavel Machek

Hi!

Quoted 16 lines

Quoted 5 lines

Quoted 5 lines

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?

Quoted 7 lines

Quoted 3 lines

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:

Quoted 6 lines

[    2.374877] of_get_named_gpiod_flags: parsed 'reset-gpios' property
of node '/ocp at 68000000/spi at 48098000/tsc2005 at 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:

drivers/hwmon/hwmon.c +3 -1

index 3932f92..fe5ec82 100644
@@ -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)

Quoted 12 lines

Quoted 9 lines

pavel at n900:~$ dmesg | grep -5 Invalid

[    0.829650] of_get_named_gpiod_flags: parsed 'gpio-reset' property
of node '/ocp at 68000000/i2c at 48072000/tlv320aic3x at 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 at 68000000/i2c at 48072000/lp5523 at 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.

Quoted 3 lines

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?

Quoted 9 lines

Quoted 4 lines

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.

Quoted 4 lines

"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.

Quoted 13 lines

Quoted 13 lines

Quoted 4 lines

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?

Quoted 3 lines

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.

Quoted 17 lines · Pavel Machek

Quoted 16 lines

Quoted 5 lines

Quoted 4 lines

Right.

Before reverting, can you please try if this patch works or not?

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 1 line

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.

Quoted 2 lines

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 ?

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.

Quoted 3 lines · Pavel Machek

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.

Quoted 6 lines · Fabio Estevam

Quoted 3 lines · Pavel Machek

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.

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 3 lines

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?

Quoted 6 lines

Quoted 3 lines

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.

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.

Hi!

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 3 lines

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?

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

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 at n900:/sys/class/hwmon$ cat hwmon0/name
bq27200-0
pavel at n900:/sys/class/hwmon$ cat hwmon1/name
rx51-battery

Hi!

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 3 lines

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?

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

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 at n900:/sys/class/hwmon$ cat hwmon0/name
bq27200-0
pavel at n900:/sys/class/hwmon$ cat hwmon1/name
rx51-battery

Quoted 3 lines

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....

Quoted 38 lines · On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote:

Hi!

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

Quoted 9 lines

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?

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

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 at n900:/sys/class/hwmon$ cat hwmon0/name
bq27200-0
pavel at n900:/sys/class/hwmon$ cat hwmon1/name
rx51-battery

Quoted 1 line

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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?).

Quoted 49 lines · Pali Roh?r

Quoted 38 lines · On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote:

Hi!

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

Quoted 9 lines

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?

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

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 at n900:/sys/class/hwmon$ cat hwmon0/name
bq27200-0
pavel at n900:/sys/class/hwmon$ cat hwmon1/name
rx51-battery

Quoted 1 line

Quoted 8 lines

Quoted 5 lines

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.

Quoted 8 lines

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.

Quoted 89 lines · Zhang Rui

Quoted 65 lines · Pali Roh?r

Quoted 57 lines · On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote:

Hi!

Quoted 34 lines

Quoted 34 lines

Quoted 34 lines

Quoted 20 lines

Quoted 10 lines

Quoted 5 lines

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.

Quoted 8 lines

"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.

Quoted 4 lines

Quoted 4 lines

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 at n900:/sys/class/hwmon$ cat hwmon0/name
bq27200-0
pavel at n900:/sys/class/hwmon$ cat hwmon1/name
rx51-battery

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 11 lines

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.

Quoted 6 lines

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.

Quoted 96 lines · Guenter Roeck

Quoted 95 lines · Zhang Rui

Quoted 65 lines · Pali Roh?r

Quoted 57 lines · On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote:

Hi!

Quoted 34 lines

Quoted 34 lines

Quoted 34 lines

Quoted 20 lines

Quoted 10 lines

Quoted 5 lines

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.

Quoted 8 lines

"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.

Quoted 4 lines

Quoted 4 lines

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 at n900:/sys/class/hwmon$ cat hwmon0/name
bq27200-0
pavel at n900:/sys/class/hwmon$ cat hwmon1/name
rx51-battery

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 11 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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

Quoted 2 lines

Quoted 2 lines

That sounds like fun. Changing bq27200-0 to bq27200_0 is Forbidden by the ABI Police, but taking the entire device away is ok.

Quoted 2 lines

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 ?

Quoted 2 lines

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.

Hi!

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 2 lines

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.)

Quoted 3 lines

Quoted 2 lines

If thermal zone I/F is used, we can not change it's 'type' name to be compatible with new hwmon API.

Quoted 4 lines

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.

Quoted 25 lines · Pavel Machek

Hi!

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 2 lines

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.)

Quoted 5 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 128 lines · Zhang Rui

Quoted 126 lines · Guenter Roeck

Quoted 124 lines · Zhang Rui

Quoted 81 lines · Pali Roh?r

Quoted 71 lines · On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote:

Hi!

Quoted 43 lines

Quoted 43 lines

Quoted 43 lines

Quoted 27 lines

Quoted 15 lines

Quoted 6 lines

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.

Quoted 9 lines

"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.

Quoted 5 lines

Quoted 5 lines

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 at n900:/sys/class/hwmon$ cat hwmon0/name
bq27200-0
pavel at n900:/sys/class/hwmon$ cat hwmon1/name
rx51-battery

Quoted 11 lines

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.

Quoted 5 lines

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.

Quoted 15 lines

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.

Quoted 6 lines

Quoted 4 lines

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 ?

Quoted 5 lines

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

Quoted 12 lines

Quoted 7 lines

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

Quoted 7 lines

Quoted 5 lines

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?

Quoted 11 lines · Zhang Rui

Quoted 7 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 5 lines

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).

Quoted 96 lines · Guenter Roeck

Quoted 95 lines · Zhang Rui

Quoted 65 lines · Pali Roh?r

Quoted 57 lines · On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote:

Hi!

Quoted 34 lines

Quoted 34 lines

Quoted 34 lines

Quoted 20 lines

Quoted 10 lines

Quoted 5 lines

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.

Quoted 8 lines

"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.

Quoted 4 lines

Quoted 4 lines

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 at n900:/sys/class/hwmon$ cat hwmon0/name
bq27200-0
pavel at n900:/sys/class/hwmon$ cat hwmon1/name
rx51-battery

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 11 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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

Quoted 10 lines

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.

Quoted 4 lines

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.

Quoted 15 lines

Quoted 14 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

Quoted 2 lines

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).

Quoted 28 lines · On Wednesday 25 January 2017 12:12:33 Pavel Machek wrote:

Hi!

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 3 lines

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?

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

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 at n900:/sys/class/hwmon$ cat hwmon0/name
bq27200-0
pavel at n900:/sys/class/hwmon$ cat hwmon1/name
rx51-battery

Quoted 6 lines

Quoted 3 lines

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.

Quoted 46 lines · Guenter Roeck

Quoted 45 lines · Pavel Machek

Hi!

Quoted 16 lines

Quoted 5 lines

Quoted 5 lines

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?

Quoted 7 lines

Quoted 3 lines

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:

Quoted 6 lines

[    2.374877] of_get_named_gpiod_flags: parsed 'reset-gpios' property
of node '/ocp at 68000000/spi at 48098000/tsc2005 at 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:

drivers/hwmon/hwmon.c +3 -1

index 3932f92..fe5ec82 100644
@@ -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)

Quoted 12 lines

Quoted 9 lines

pavel at n900:~$ dmesg | grep -5 Invalid

[    0.829650] of_get_named_gpiod_flags: parsed 'gpio-reset' property
of node '/ocp at 68000000/i2c at 48072000/tlv320aic3x at 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 at 68000000/i2c at 48072000/lp5523 at 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

Quoted 1 line

From 79320ea3721314ec29ed6cdd6987ff922b11d6f9 Mon Sep 17 00:00:00 2001

From: Zhang Rui <[email protected]>
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 <[email protected]>

Signed-off-by Zhang Rui <[email protected]>

---
 drivers/thermal/thermal_hwmon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/thermal/thermal_hwmon.c +0 -0

index c4a508a..0c6789f 100644

drivers/thermal/thermal_hwmon.c +0 -0

 	if (IS_ERR(hwmon->device)) {
 		result = PTR_ERR(hwmon->device);
 		goto free_mem;

Quoted 51 lines

Quoted 30 lines

Quoted 30 lines

Quoted 9 lines

/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 at 68000000/spi at 48098000/tsc2005 at 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:

drivers/hwmon/hwmon.c +3 -1

index 3932f92..fe5ec82 100644
@@ -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)

Quoted 12 lines

Quoted 9 lines

pavel at n900:~$ dmesg | grep -5 Invalid

[    0.829650] of_get_named_gpiod_flags: parsed 'gpio-reset' property
of node '/ocp at 68000000/i2c at 48072000/tlv320aic3x at 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 at 68000000/i2c at 48072000/lp5523 at 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?

drivers/thermal/thermal_hwmon.c +0 -0

index c4a508a..0c6789f 100644

drivers/thermal/thermal_hwmon.c int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz) · line 161

Quoted 7 lines

drivers/thermal/thermal_hwmon.c +2 -2

@@ -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().

Hi!

Quoted 62 lines

Quoted 51 lines

Quoted 30 lines

Quoted 30 lines

Quoted 9 lines

/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 at 68000000/spi at 48098000/tsc2005 at 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:

drivers/hwmon/hwmon.c +3 -1

index 3932f92..fe5ec82 100644
@@ -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)

Quoted 12 lines

Quoted 9 lines

pavel at n900:~$ dmesg | grep -5 Invalid

[    0.829650] of_get_named_gpiod_flags: parsed 'gpio-reset' property
of node '/ocp at 68000000/i2c at 48072000/tlv320aic3x at 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 at 68000000/i2c at 48072000/lp5523 at 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?

drivers/thermal/thermal_hwmon.c +0 -0

index c4a508a..0c6789f 100644

drivers/thermal/thermal_hwmon.c int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz) · line 161

Quoted 7 lines

drivers/thermal/thermal_hwmon.c +2 -2

@@ -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

Quoted 2 lines

	strreplace(hwmon->type, '-', '_');

after strlcpy().

Simple, yes. But that will change userland interface. Is it okay thing to do?

Quoted 43 lines

Quoted 16 lines

Quoted 5 lines

Quoted 5 lines

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?

Quoted 7 lines

Quoted 3 lines

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:

Quoted 6 lines

[    2.374877] of_get_named_gpiod_flags: parsed 'reset-gpios' property
of node '/ocp at 68000000/spi at 48098000/tsc2005 at 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:

drivers/hwmon/hwmon.c +3 -1

index 3932f92..fe5ec82 100644
@@ -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)

Quoted 9 lines

pavel at n900:~$ dmesg | grep -5 Invalid

[    0.829650] of_get_named_gpiod_flags: parsed 'gpio-reset' property
of node '/ocp at 68000000/i2c at 48072000/tlv320aic3x at 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 at 68000000/i2c at 48072000/lp5523 at 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.

Quoted 26 lines

[    2.064544] bq27xxx-battery 2-0055: failed to register battery
[    2.064605] bq27xxx-battery: probe of 2-0055 failed with error -22
[    2.065368] of_get_named_gpiod_flags: parsed 'power-gpio' property
of node '/ocp at 68000000/i2c at 48072000/tpa6130a2 at 60[0]' - status (0)
[    2.083221] bq2415x-charger 2-006b: automode supported, waiting for
events
[    2.084442] bq2415x-charger 2-006b: driver registered
--
[    2.369842] g_ether gadget: g_ether ready
[    2.377197] tsc2005 spi1.0: GPIO lookup for consumer reset
[    2.377227] tsc2005 spi1.0: using device tree for GPIO lookup
[    2.377288] of_get_named_gpiod_flags: parsed 'reset-gpios' property
of node '/ocp at 68000000/spi at 48098000/tsc2005 at 0[0]' - status (0)
[    2.378936] input: TSC2005 touchscreen as
/devices/platform/68000000.ocp/48098000.spi/spi_master/spi1/spi1.0/input/input5
[    2.395111] hwmon: Invalid character detected: rx51-battery
[    2.402587] rx51-battery: probe of n900-battery failed with error
-22
[    2.410247] omap_hsmmc 4809c000.mmc: GPIO lookup for consumer cd
[    2.410247] omap_hsmmc 4809c000.mmc: using device tree for GPIO
lookup
[    2.410278] of_get_named_gpiod_flags: can't parse 'cd-gpios'
property of node '/ocp at 68000000/mmc at 4809c000[0]'
[    2.410278] of_get_named_gpiod_flags: can't parse 'cd-gpio'

property of node '/ocp at 68000000/mmc at 4809c000[0]'

Hi!

Quoted 43 lines

Quoted 43 lines

Quoted 16 lines

Quoted 5 lines

Quoted 5 lines

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?

Quoted 7 lines

Quoted 3 lines

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:

Quoted 6 lines

[    2.374877] of_get_named_gpiod_flags: parsed 'reset-gpios' property
of node '/ocp at 68000000/spi at 48098000/tsc2005 at 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:

drivers/hwmon/hwmon.c +3 -1

index 3932f92..fe5ec82 100644
@@ -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)

Quoted 11 lines

Quoted 9 lines

pavel at n900:~$ dmesg | grep -5 Invalid

[    0.829650] of_get_named_gpiod_flags: parsed 'gpio-reset' property
of node '/ocp at 68000000/i2c at 48072000/tlv320aic3x at 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 at 68000000/i2c at 48072000/lp5523 at 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.

Look down how log continues.

So we really have two problematic names on one device. Plus, those names are exposed to userspace, AFAICT. So "just changing them" might not be good idea.

Maybe we should just relax the check?

Anyway, this just confirms that original patch needs to be reverted, and that there will be some fun issues to solve in v4.11.

Pavel

Quoted 26 lines

Quoted 26 lines

[    2.064544] bq27xxx-battery 2-0055: failed to register battery
[    2.064605] bq27xxx-battery: probe of 2-0055 failed with error -22
[    2.065368] of_get_named_gpiod_flags: parsed 'power-gpio' property
of node '/ocp at 68000000/i2c at 48072000/tpa6130a2 at 60[0]' - status (0)
[    2.083221] bq2415x-charger 2-006b: automode supported, waiting for
events
[    2.084442] bq2415x-charger 2-006b: driver registered
--
[    2.369842] g_ether gadget: g_ether ready
[    2.377197] tsc2005 spi1.0: GPIO lookup for consumer reset
[    2.377227] tsc2005 spi1.0: using device tree for GPIO lookup
[    2.377288] of_get_named_gpiod_flags: parsed 'reset-gpios' property
of node '/ocp at 68000000/spi at 48098000/tsc2005 at 0[0]' - status (0)
[    2.378936] input: TSC2005 touchscreen as
/devices/platform/68000000.ocp/48098000.spi/spi_master/spi1/spi1.0/input/input5
[    2.395111] hwmon: Invalid character detected: rx51-battery
[    2.402587] rx51-battery: probe of n900-battery failed with error
-22
[    2.410247] omap_hsmmc 4809c000.mmc: GPIO lookup for consumer cd
[    2.410247] omap_hsmmc 4809c000.mmc: using device tree for GPIO
lookup
[    2.410278] of_get_named_gpiod_flags: can't parse 'cd-gpios'
property of node '/ocp at 68000000/mmc at 4809c000[0]'
[    2.410278] of_get_named_gpiod_flags: can't parse 'cd-gpio'

property of node '/ocp at 68000000/mmc at 4809c000[0]'

Hi!

Quoted 2 lines

It seems that battery driver stopped working on N900 between -rc4 and -rc5.

pavel at amd:/data/l/linux-n900$ git bisect log
# bad: [41f283973653dc44af47585fa79fb6da6ffdc2e2] Merge /data//l/clean-cg into mini-v4.10
# good: [4f8d9925eb1de1ec1c4183164f1bc7b01f5ae185] It seems mainline -rc4 did have a fix for the sound issue. Use that one.
git bisect start 'mini-v4.10' '4f8d9925eb1de1ec1c4183164f1bc7b01f5ae185'
# good: [e90665a5d38b17fdbe484a85fbba917a7006522d] Merge tag 'ceph-for-4.10-rc5' of git://github.com/ceph/ceph-client
git bisect good e90665a5d38b17fdbe484a85fbba917a7006522d
# good: [83fd57a740bb19286959b3085eb93532f3e7ef2c] Merge tag 'powerpc-4.10-2' of
git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux
git bisect good 83fd57a740bb19286959b3085eb93532f3e7ef2c
# good: [c497f8d17246720afe680ea1a8fa6e48e75af852] Merge tag 'usb-4.10-rc5' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
git bisect good c497f8d17246720afe680ea1a8fa6e48e75af852
# bad: [bb6c01c2dde67b165cf7c808b0f00677b6f94b96] Merge branch 'for-rc' of git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux
git bisect bad bb6c01c2dde67b165cf7c808b0f00677b6f94b96
# good: [18591add41ec9558ce0e32ef88626c18cc70c686] thermal: rockchip:
handle set_trips without the trip points
git bisect good 18591add41ec9558ce0e32ef88626c18cc70c686
# bad: [f53345e8cf027d03187b9417f1f8883c516e1a5b] thermal: core: move tz->device.groups cleanup to thermal_release
git bisect bad f53345e8cf027d03187b9417f1f8883c516e1a5b
# bad: [7611fb68062f8d7f416f3272894d1edf7bbff29c] thermal:
thermal_hwmon: Convert to hwmon_device_register_with_info() git bisect bad 7611fb68062f8d7f416f3272894d1edf7bbff29c

# first bad commit: [7611fb68062f8d7f416f3272894d1edf7bbff29c]
thermal: thermal_hwmon: Convert to hwmon_device_register_with_info()
pavel at amd:/data/l/linux-n900$

pavel at amd:/data/l/linux-n900$ git bisect log
# bad: [41f283973653dc44af47585fa79fb6da6ffdc2e2] Merge /data//l/clean-cg into mini-v4.10
# good: [4f8d9925eb1de1ec1c4183164f1bc7b01f5ae185] It seems mainline -rc4 did have a fix for the sound issue. Use that one.
git bisect start 'mini-v4.10' '4f8d9925eb1de1ec1c4183164f1bc7b01f5ae185'
# good: [e90665a5d38b17fdbe484a85fbba917a7006522d] Merge tag 'ceph-for-4.10-rc5' of git://github.com/ceph/ceph-client
git bisect good e90665a5d38b17fdbe484a85fbba917a7006522d
# good: [83fd57a740bb19286959b3085eb93532f3e7ef2c] Merge tag 'powerpc-4.10-2' of
git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux
git bisect good 83fd57a740bb19286959b3085eb93532f3e7ef2c
# good: [c497f8d17246720afe680ea1a8fa6e48e75af852] Merge tag 'usb-4.10-rc5' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
git bisect good c497f8d17246720afe680ea1a8fa6e48e75af852
# bad: [bb6c01c2dde67b165cf7c808b0f00677b6f94b96] Merge branch 'for-rc' of git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux
git bisect bad bb6c01c2dde67b165cf7c808b0f00677b6f94b96
# good: [18591add41ec9558ce0e32ef88626c18cc70c686] thermal: rockchip:
handle set_trips without the trip points
git bisect good 18591add41ec9558ce0e32ef88626c18cc70c686
# bad: [f53345e8cf027d03187b9417f1f8883c516e1a5b] thermal: core: move tz->device.groups cleanup to thermal_release
git bisect bad f53345e8cf027d03187b9417f1f8883c516e1a5b
# bad: [7611fb68062f8d7f416f3272894d1edf7bbff29c] thermal:
thermal_hwmon: Convert to hwmon_device_register_with_info() git bisect bad 7611fb68062f8d7f416f3272894d1edf7bbff29c

# first bad commit: [7611fb68062f8d7f416f3272894d1edf7bbff29c]
thermal: thermal_hwmon: Convert to hwmon_device_register_with_info()
pavel at amd:/data/l/linux-n900$

7611fb68062f8d7f416f3272894d1edf7bbff29c is the first bad commit commit 7611fb68062f8d7f416f3272894d1edf7bbff29c

Author: Fabio Estevam <[email protected]>
Date:   Tue Dec 27 15:31:49 2016 -0200

thermal: thermal_hwmon: Convert to
hwmon_device_register_with_info()

Booting Linux on a mx6q based board leads to the following warning:

(NULL device *): hwmon_device_register() is deprecated. Please convert the
driver to use hwmon_device_register_with_info().

,so do as suggested.

Also, this results in the core taking care of creating the 'name'
attribute, so drop the code doing that from the thermal driver.

Suggested-by Guenter Roeck <[email protected]>

Signed-off-by Fabio Estevam <[email protected]>

Signed-off-by Zhang Rui <[email protected]>

:040000 040000 b3f6095ab53677277c2f9d75cb6430afb6825765
156a18e983a22b8ae43c7b312a4e1c63017f0e26 M drivers

I'll try to revert it on the top of v4.10-rc5 now.