Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1564957 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-23 13:50 +0100
    Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pali Rohár <pali.rohar@gmail.com> - 2017-01-23 13:50 +0100
      Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-23 15:30 +0100
    Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-23 15:20 +0100
      Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Guenter Roeck <linux@roeck-us.net> - 2017-01-23 15:40 +0100
        Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-23 15:50 +0100
          Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Guenter Roeck <linux@roeck-us.net> - 2017-01-23 18:20 +0100
          Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-24 00:30 +0100
            Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Guenter Roeck <linux@roeck-us.net> - 2017-01-24 00:50 +0100
              Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Zhang Rui <rui.zhang@intel.com> - 2017-01-24 08:10 +0100
                Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-24 08:40 +0100
                  Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Guenter Roeck <linux@roeck-us.net> - 2017-01-24 15:20 +0100
                    Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-24 19:00 +0100
                      Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Guenter Roeck <linux@roeck-us.net> - 2017-01-24 19:50 +0100
                        Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-24 23:50 +0100
                          Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Fabio Estevam <festevam@gmail.com> - 2017-01-25 00:10 +0100
                            Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Zhang Rui <rui.zhang@intel.com> - 2017-01-25 06:40 +0100
                              Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-25 11:20 +0100
                        Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-25 12:20 +0100
                          Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Guenter Roeck <linux@roeck-us.net> - 2017-01-25 13:00 +0100
                          Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pali Rohár <pali.rohar@gmail.com> - 2017-01-25 13:10 +0100
                            Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Zhang Rui <rui.zhang@intel.com> - 2017-01-27 02:40 +0100
                              Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Guenter Roeck <linux@roeck-us.net> - 2017-01-27 03:10 +0100
                                Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Zhang Rui <rui.zhang@intel.com> - 2017-01-27 04:50 +0100
                                  Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Guenter Roeck <linux@roeck-us.net> - 2017-01-27 08:20 +0100
                                    Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-27 12:20 +0100
                                      Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Guenter Roeck <linux@roeck-us.net> - 2017-01-27 15:20 +0100
                                        Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-27 20:10 +0100
                                    Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Zhang Rui <rui.zhang@intel.com> - 2017-01-27 15:50 +0100
                                      Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Guenter Roeck <linux@roeck-us.net> - 2017-01-27 19:30 +0100
                                        Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-27 20:20 +0100
                                      Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-27 20:40 +0100
                                  Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pali Rohár <pali.rohar@gmail.com> - 2017-01-27 09:40 +0100
                                Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-27 12:30 +0100
                                  Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Guenter Roeck <linux@roeck-us.net> - 2017-01-27 15:20 +0100
                                    Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-27 20:20 +0100
                            Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-27 12:40 +0100
                Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Guenter Roeck <linux@roeck-us.net> - 2017-01-24 15:20 +0100
                  Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-24 15:20 +0100
                    Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Guenter Roeck <linux@roeck-us.net> - 2017-01-24 15:40 +0100
              Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-24 08:40 +0100
                Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Guenter Roeck <linux@roeck-us.net> - 2017-01-24 15:20 +0100
      Re: v4.10-rc4 to v4.10-rc5: battery regression on Nokia N900 Pavel Machek <pavel@ucw.cz> - 2017-01-23 21:30 +0100

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1566543

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

[toc] | [prev] | [next] | [standalone]

#1567859

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

[toc] | [prev] | [next] | [standalone]

#1567867

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

[toc] | [prev] | [next] | [standalone]

#1567884

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

[toc] | [prev] | [next] | [standalone]

#1567913

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
>

[toc] | [prev] | [next] | [standalone]

#1568123

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1568399

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

[toc] | [prev] | [next] | [standalone]

#1568626

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1568408

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

[toc] | [prev] | [next] | [standalone]

#1568597

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

[toc] | [prev] | [next] | [standalone]

#1568631

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1568638

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1567951

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

[toc] | [prev] | [next] | [standalone]

#1568157

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1568397

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

[toc] | [prev] | [next] | [standalone]

#1568628

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1568194

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1565886

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

[toc] | [prev] | [next] | [standalone]

#1565890

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1565907

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

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web