[RFC] w1: regression, remove need for ida and use PLATFORM_DEVID_AUTO

[Guenter Roeck]

On 02/03/2017 03:30 PM, Petr Cvek wrote:
> Dne 3.2.2017 v 15:07 Guenter Roeck napsal(a):
>> On 02/03/2017 05:28 AM, Petr Cvek wrote:
>>> Dne 3.2.2017 v 04:31 Guenter Roeck napsal(a):
>>>> On 02/02/2017 05:31 PM, Petr Cvek wrote:
>>>>> Dne 2.2.2017 v 15:23 Andrew F. Davis napsal(a):
>>>>>> On 02/01/2017 07:05 PM, Petr Cvek wrote:
>>>>>>> Patch
>>>>>>>     w1: remove need for ida and use PLATFORM_DEVID_AUTO
>>>>>>>     098f9fb0c962eb2fdba5f9d34f4cf7a938237184
>>>>>>>
>>>>>>> causes a regression in the w1_ds2760 driver. Initialization creates a name "ds2760-battery.0.auto". It seems that name of that size will cause checking code in __thermal_cooling_device_register() from thermal_core.c to fail as it checks for names shorter than 20 chars:
>>>>>>>
>>>>>>>     if (type && strlen(type) >= THERMAL_NAME_LENGTH)
>>>>>>>         return ERR_PTR(-EINVAL);
>>>>>>>
>>>>>>> A second problem seems to be with magician_supplicants list as "ds2760-battery.0" does not match in a test power_supply_am_i_supplied() in ds2760_battery_update_status() from drivers/power/supply/ds2760_battery.c . This causes status flag to be forever in POWER_SUPPLY_STATUS_DISCHARGING state.
>>>>>>>
>>>>>>> Functionality returned after I hotfix changed (drivers/w1/slaves/w1_ds2760.c)
>>>>>>>
>>>>>>>     pdev = platform_device_alloc("ds2760-battery", PLATFORM_DEVID_AUTO);
>>>>>>>
>>>>>>> to
>>>>>>>
>>>>>>>     pdev = platform_device_alloc("ds2760-battery", 0);
>>>>>>>
>>>>>>> Which solution do you advise. Shortening the name let's say just to "ds2760" (with changes to arch files, w1 files and supply files), increasing THERMAL_NAME_LENGTH to let's say 32 (I don't know if this doesn't break something other) or just reverting to the old behavior?
>>>>>>>
>>>>>>
>>>>>> If 20 is no longer the max thermal name then THERMAL_NAME_LENGTH should
>>>>>> be changed.
>>>>>
>>>>> Yes it is that ".auto" addition to "ds2760-battery.0" makes 21 chars long string. With higher number (".10" onwards) it is 22 chars. Load of the module then fails at test in thermal_zone_device_register():
>>>>>
>>>>>     https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/thermal_core.c?id=refs/tags/v4.10-rc6#n1158
>>>>>
>>>>>     if (type && strlen(type) >= THERMAL_NAME_LENGTH)
>>>>>         return ERR_PTR(-EINVAL);
>>>>>
>>>>> I searched in code further and with THERMAL_NAME_LENGTH = 30 (enough space) the module load fails at another place in __hwmon_device_register():
>>>>>
>>>>>     https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/hwmon/hwmon.c?id=refs/tags/v4.10-rc6#n548
>>>>>
>>>>>     if (name && (!strlen(name) || strpbrk(name, "-* \t\n")))
>>>>>         return ERR_PTR(-EINVAL);
>>>>>
>>>>> That's because there is "-" in "ds2760-battery.0.auto" (and that's why shortening to "ds2760.0" works). Is there some reason for disallowing "-" in a name? If not then I'm for removal (in other case we need to change ds2760-battery and others names). Increasing THERMAL_NAME_LENGTH to 22 should be just OK for ds2760 (but maybe not enough for others) I would increased it to 32 (nice number ;-) ).
>>>>>
>>>>
>>>> '-' is not permitted in hwmon name attributes, primarily because libsensors
>>>> interprets it as a delimiter in its configuration file.
>>>>
>>>
>>> OK so this means we need to rename ds2760-battery so libsensors works.
>>>
>>> Hmm is an underscore in "ds2760_battery.0.auto" OK?
>>
>> Theoretically yes, but there was objection to that idea because replacing the '-'
>> with '_' is considered by some to be an ABI change (why the addition of '.auto'
>> doesn't matter escapes me, though).
>>
>
> I read that discussion and both ways are not optimal. Many drivers has "-" in its
> name and even "*" is a valid filename. And changing to underscore is ugly too.
> Too bad libsensors parses "-" that way.
>

FWIW, libsensors also parses '*' as wildcard.

There was a suggestion to change libsensors to be more name-forgiving.
I suspect that may not be as easy as it sounds, but as always everyone
should feel free to submit patches.

>>>
>>>> Anyway, the related change in thermal has been reverted, so that should
>>>
>>> s/thermal/hwmon ? Or I'm confused :-D (there was no change in thermal, code stopped working because of change in w1 driver (".auto" addition))
>>>
>>
>> 3feb479cea37 Revert "thermal: thermal_hwmon: Convert to hwmon_device_register_with_info()"
>
> But after applying revert on my kernel copy there still will be a problem
> in arch init with missing ".auto" match and driver load still fails on a shorter string array.
>

Can't help you there, sorry.

Guenter