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

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

[Petr Cvek]

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?

Best regards,
Petr

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

[Petr Cvek]

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?

Best regards,
Petr

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

[Andrew F. Davis]

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.

As for the couple boards hard-coding "ds2760-battery.0", this is a
problem that should be fix regardless of any fix we chose here as IDA is
not guaranteed to return 0, so the battery could just as easily be
called ds2760-battery.7. (I've had this discussion before as the N900
user-space also make this mistake).

Andrew

> Best regards,
> Petr
> 

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

[Andrew F. Davis]

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.

As for the couple boards hard-coding "ds2760-battery.0", this is a
problem that should be fix regardless of any fix we chose here as IDA is
not guaranteed to return 0, so the battery could just as easily be
called ds2760-battery.7. (I've had this discussion before as the N900
user-space also make this mistake).

Andrew

> Best regards,
> Petr
> 

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

[Petr Cvek]

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

> 
> As for the couple boards hard-coding "ds2760-battery.0", this is a
> problem that should be fix regardless of any fix we chose here as IDA is
> not guaranteed to return 0, so the battery could just as easily be
> called ds2760-battery.7. (I've had this discussion before as the N900
> user-space also make this mistake).
> 

I'm not familiar with power supply subsystem, so just a naive idea. Should suffice to change drivers/power/supply/power_supply_core.c __power_supply_is_supplied_by() for only first part of the string? (like "ds2760-battery" subset of "ds2760-battery.7").

Do you have a link to discussion about N900?

Petr

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

[Petr Cvek]

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

> 
> As for the couple boards hard-coding "ds2760-battery.0", this is a
> problem that should be fix regardless of any fix we chose here as IDA is
> not guaranteed to return 0, so the battery could just as easily be
> called ds2760-battery.7. (I've had this discussion before as the N900
> user-space also make this mistake).
> 

I'm not familiar with power supply subsystem, so just a naive idea. Should suffice to change drivers/power/supply/power_supply_core.c __power_supply_is_supplied_by() for only first part of the string? (like "ds2760-battery" subset of "ds2760-battery.7").

Do you have a link to discussion about N900?

Petr

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

[Guenter Roeck]

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.

Anyway, the related change in thermal has been reverted, so that should
no longer be an issue. Also, we'll change the hwmon code in 4.11 to no longer
return an error but to (only) issue a warning if an invalid name is provided.

Guenter

>>
>> As for the couple boards hard-coding "ds2760-battery.0", this is a
>> problem that should be fix regardless of any fix we chose here as IDA is
>> not guaranteed to return 0, so the battery could just as easily be
>> called ds2760-battery.7. (I've had this discussion before as the N900
>> user-space also make this mistake).
>>
>
> I'm not familiar with power supply subsystem, so just a naive idea. Should suffice to change drivers/power/supply/power_supply_core.c __power_supply_is_supplied_by() for only first part of the string? (like "ds2760-battery" subset of "ds2760-battery.7").
>
> Do you have a link to discussion about N900?
>
> Petr
>

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

[Guenter Roeck]

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.

Anyway, the related change in thermal has been reverted, so that should
no longer be an issue. Also, we'll change the hwmon code in 4.11 to no longer
return an error but to (only) issue a warning if an invalid name is provided.

Guenter

>>
>> As for the couple boards hard-coding "ds2760-battery.0", this is a
>> problem that should be fix regardless of any fix we chose here as IDA is
>> not guaranteed to return 0, so the battery could just as easily be
>> called ds2760-battery.7. (I've had this discussion before as the N900
>> user-space also make this mistake).
>>
>
> I'm not familiar with power supply subsystem, so just a naive idea. Should suffice to change drivers/power/supply/power_supply_core.c __power_supply_is_supplied_by() for only first part of the string? (like "ds2760-battery" subset of "ds2760-battery.7").
>
> Do you have a link to discussion about N900?
>
> Petr
>

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

[Petr Cvek]

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?

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

Best regards,
Petr

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

[Petr Cvek]

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?

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

Best regards,
Petr

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

[Guenter Roeck]

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

>
>> 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()"

Guenter

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

[Guenter Roeck]

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

>
>> 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()"

Guenter

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

[Andrew F. Davis]

On 02/02/2017 07: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 ;-) ).
> 
>>
>> As for the couple boards hard-coding "ds2760-battery.0", this is a
>> problem that should be fix regardless of any fix we chose here as IDA is
>> not guaranteed to return 0, so the battery could just as easily be
>> called ds2760-battery.7. (I've had this discussion before as the N900
>> user-space also make this mistake).
>>
> 
> I'm not familiar with power supply subsystem, so just a naive idea. Should suffice to change drivers/power/supply/power_supply_core.c __power_supply_is_supplied_by() for only first part of the string? (like "ds2760-battery" subset of "ds2760-battery.7").
> 
> Do you have a link to discussion about N900?
> 

The thread doesn't look to have happened on the lists, but the commit
that reverted my change is: 9aafabc7fece. I argued against this revert
as relying on sysfs names never changing should be considered ABI abuse
and we should not hold back proper fixes for the sake of keeping a hack
in user-space working.

Andrew

> Petr
> 

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

[Andrew F. Davis]

On 02/02/2017 07: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 ;-) ).
> 
>>
>> As for the couple boards hard-coding "ds2760-battery.0", this is a
>> problem that should be fix regardless of any fix we chose here as IDA is
>> not guaranteed to return 0, so the battery could just as easily be
>> called ds2760-battery.7. (I've had this discussion before as the N900
>> user-space also make this mistake).
>>
> 
> I'm not familiar with power supply subsystem, so just a naive idea. Should suffice to change drivers/power/supply/power_supply_core.c __power_supply_is_supplied_by() for only first part of the string? (like "ds2760-battery" subset of "ds2760-battery.7").
> 
> Do you have a link to discussion about N900?
> 

The thread doesn't look to have happened on the lists, but the commit
that reverted my change is: 9aafabc7fece. I argued against this revert
as relying on sysfs names never changing should be considered ABI abuse
and we should not hold back proper fixes for the sake of keeping a hack
in user-space working.

Andrew

> Petr
> 

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

[Andrew F. Davis]

On 02/02/2017 07: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 ;-) ).
> 
>>
>> As for the couple boards hard-coding "ds2760-battery.0", this is a
>> problem that should be fix regardless of any fix we chose here as IDA is
>> not guaranteed to return 0, so the battery could just as easily be
>> called ds2760-battery.7. (I've had this discussion before as the N900
>> user-space also make this mistake).
>>
> 
> I'm not familiar with power supply subsystem, so just a naive idea. Should suffice to change drivers/power/supply/power_supply_core.c __power_supply_is_supplied_by() for only first part of the string? (like "ds2760-battery" subset of "ds2760-battery.7").
> 
> Do you have a link to discussion about N900?
> 

The thread doesn't look to have happened on the lists, but the commit
that reverted my change is: 9aafabc7fece. I argued against this revert
as relying on sysfs names never changing should be considered ABI abuse
and we should not hold back proper fixes for the sake of keeping a hack
in user-space working.

Andrew

> Petr
> 

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

[Petr Cvek]

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.

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

Petr

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

[Petr Cvek]

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.

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

Petr

Re: [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

[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