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

[Guenter Roeck <linux@roeck-us.net>]

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
>