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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 257 bytes --]

Hi!

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Pali Rohár]

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?

-- 
Pali Rohár
pali.rohar@gmail.com

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

On Mon 2017-01-23 13:42:06, Pali Rohár wrote:
> 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?

n900-battery.

									Pavel

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 5082 bytes --]

Hi!

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

pavel@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@amd:/data/l/linux-n900$

pavel@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@amd:/data/l/linux-n900$

7611fb68062f8d7f416f3272894d1edf7bbff29c is the first bad commit
commit 7611fb68062f8d7f416f3272894d1edf7bbff29c
Author: Fabio Estevam <fabio.estevam@nxp.com>
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 <linux@roeck-us.net>
 Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
 Signed-off-by: Zhang Rui <rui.zhang@intel.com>

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

Best regards,

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Guenter Roeck]

On 01/23/2017 06:19 AM, Pavel Machek wrote:
> Hi!
>
>> It seems that battery driver stopped working on N900 between -rc4 and
>> -rc5.
>
> pavel@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@amd:/data/l/linux-n900$
>
> pavel@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@amd:/data/l/linux-n900$
>
> 7611fb68062f8d7f416f3272894d1edf7bbff29c is the first bad commit
> commit 7611fb68062f8d7f416f3272894d1edf7bbff29c
> Author: Fabio Estevam <fabio.estevam@nxp.com>
> 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 <linux@roeck-us.net>
>  Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
>  Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>
> :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.
>
Agreed.

What exactly does "stopped working" mean ? That might help understanding
what went wrong.

Thanks,
Guenter

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 6105 bytes --]

On Mon 2017-01-23 06:31:31, Guenter Roeck wrote:
> On 01/23/2017 06:19 AM, Pavel Machek wrote:
> >Hi!
> >
> >>It seems that battery driver stopped working on N900 between -rc4 and
> >>-rc5.
> >
> >pavel@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@amd:/data/l/linux-n900$
> >
> >pavel@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@amd:/data/l/linux-n900$
> >
> >7611fb68062f8d7f416f3272894d1edf7bbff29c is the first bad commit
> >commit 7611fb68062f8d7f416f3272894d1edf7bbff29c
> >Author: Fabio Estevam <fabio.estevam@nxp.com>
> >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 <linux@roeck-us.net>
> > Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> >
> >: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.
> >
> 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
[    2.399902] omap_hsmmc 4809c000.mmc: GPIO lookup for consumer cd

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Guenter Roeck]

On Mon, Jan 23, 2017 at 03:40:31PM +0100, Pavel Machek wrote:
> On Mon 2017-01-23 06:31:31, Guenter Roeck wrote:
> > On 01/23/2017 06:19 AM, Pavel Machek wrote:
> > >Hi!
> > >
> > >>It seems that battery driver stopped working on N900 between -rc4 and
> > >>-rc5.
> > >
> > >pavel@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@amd:/data/l/linux-n900$
> > >
> > >pavel@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@amd:/data/l/linux-n900$
> > >
> > >7611fb68062f8d7f416f3272894d1edf7bbff29c is the first bad commit
> > >commit 7611fb68062f8d7f416f3272894d1edf7bbff29c
> > >Author: Fabio Estevam <fabio.estevam@nxp.com>
> > >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 <linux@roeck-us.net>
> > > Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > >
> > >: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.
> > >
> > 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
> [    2.399902] omap_hsmmc 4809c000.mmc: GPIO lookup for consumer cd
> 

Only problem I can imagine is that power_supply_register() fails registering
the thermal zone, which in turn would suggest that the offending patch might
not work at all, at least not under some circumstances. So it should
definitely be reverted until we figure out the problem.

Guenter

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3616 bytes --]

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
[    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@68000000/i2c@48072000/tpa6130a2@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@68000000/spi@48098000/tsc2005@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@68000000/mmc@4809c000[0]'
[    2.410278] of_get_named_gpiod_flags: can't parse 'cd-gpio'
property of node '/ocp@68000000/mmc@4809c000[0]'

									Pavel


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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Guenter Roeck]

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.

Guenter

> [    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@68000000/i2c@48072000/tpa6130a2@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@68000000/spi@48098000/tsc2005@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@68000000/mmc@4809c000[0]'
> [    2.410278] of_get_named_gpiod_flags: can't parse 'cd-gpio'
> property of node '/ocp@68000000/mmc@4809c000[0]'
> 
> 									Pavel
> 
> 
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Zhang Rui]

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);
 	if (IS_ERR(hwmon->device)) {
 		result = PTR_ERR(hwmon->device);
 		goto free_mem;
-- 
2.7.4

 
> Guenter
> 
> > [    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@68000000/i2c@48072000/tpa6130a2@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@68000000/spi@48098000/tsc2005@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@68000000/mmc@4809c000[0]'
> > [    2.410278] of_get_named_gpiod_flags: can't parse 'cd-gpio'
> > property of node '/ocp@68000000/mmc@4809c000[0]'
> > 
> > 									Pavel
> > 
> > 
> > -- 
> > (english) http://www.livejournal.com/~pavelmachek
> > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 
> 

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 4917 bytes --]

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;

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Guenter Roeck]

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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]


> >>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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Guenter Roeck]

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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 4314 bytes --]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Fabio Estevam]

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.

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

[Zhang Rui]

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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 637 bytes --]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1603 bytes --]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Guenter Roeck]

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

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

[Pali Rohár]

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

-- 
Pali Rohár
pali.rohar@gmail.com

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

[Zhang Rui]

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

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

[Guenter Roeck]

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

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

[Zhang Rui]

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

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

[Guenter Roeck]

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
>

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Guenter Roeck]

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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1788 bytes --]

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


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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Zhang Rui]

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

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

[Guenter Roeck]

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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1643 bytes --]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Pali Rohár]

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

-- 
Pali Rohár
pali.rohar@gmail.com

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]


> >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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Guenter Roeck]

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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2202 bytes --]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Guenter Roeck]

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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3136 bytes --]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Guenter Roeck]

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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 4381 bytes --]

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.

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


> > [    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@68000000/i2c@48072000/tpa6130a2@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@68000000/spi@48098000/tsc2005@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@68000000/mmc@4809c000[0]'
> > [    2.410278] of_get_named_gpiod_flags: can't parse 'cd-gpio'
> > property of node '/ocp@68000000/mmc@4809c000[0]'
> > 

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Guenter Roeck]

On 01/23/2017 11:34 PM, 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.
>
> 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?
>

So that invalid names are reported to user space ? No. If you insist reporting
bad names to user space just because bad names have been reported to user
space before, messing up libsensors, stick with the old API that lets you do
that, but don't mess up the new API.

Guenter

> 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
>
>
>>> [    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@68000000/i2c@48072000/tpa6130a2@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@68000000/spi@48098000/tsc2005@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@68000000/mmc@4809c000[0]'
>>> [    2.410278] of_get_named_gpiod_flags: can't parse 'cd-gpio'
>>> property of node '/ocp@68000000/mmc@4809c000[0]'
>>>
>

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 5358 bytes --]

On Mon 2017-01-23 15:13:31, Pavel Machek wrote:
> Hi!
> 
> > It seems that battery driver stopped working on N900 between -rc4 and
> > -rc5.
> 
> pavel@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@amd:/data/l/linux-n900$
> 
> pavel@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@amd:/data/l/linux-n900$
> 
> 7611fb68062f8d7f416f3272894d1edf7bbff29c is the first bad commit
> commit 7611fb68062f8d7f416f3272894d1edf7bbff29c
> Author: Fabio Estevam <fabio.estevam@nxp.com>
> 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 <linux@roeck-us.net>
>  Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
>  Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> 
> :040000 040000 b3f6095ab53677277c2f9d75cb6430afb6825765
> 156a18e983a22b8ae43c7b312a4e1c63017f0e26 M	drivers
> 
> I'll try to revert it on the top of v4.10-rc5 now.

As I mentioned, works ok with the revert.

rx51_battery.c calls power_supply_register()
power_supply_core.c calls thermal_zone_device_register() in psy_register_thermal()
thermal_core.c calls thermal_add_hwmon_sysfs()

Best regards,
									Pavel



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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]