From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Subject: Re: [RFC] w1: regression, remove need for ida and use PLATFORM_DEVID_AUTO To: "Andrew F. Davis" , zbr@ioremap.net, Sebastian Reichel , philipp.zabel@gmail.com, lost.distance@yahoo.com, daniel@caiaq.de, rui.zhang@intel.com, edubezval@gmail.com, jdelvare@suse.com, linux@roeck-us.net References: <308e185e-26eb-39ee-d82f-a0d210a41743@ti.com> Cc: linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-hwmon@vger.kernel.org From: Petr Cvek Message-ID: Date: Fri, 3 Feb 2017 02:31:44 +0100 MIME-Version: 1.0 In-Reply-To: <308e185e-26eb-39ee-d82f-a0d210a41743@ti.com> Content-Type: text/plain; charset=iso-8859-2 Content-Transfer-Encoding: 7bit List-ID: 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: petr.cvek@tul.cz (Petr Cvek) Date: Fri, 3 Feb 2017 02:31:44 +0100 Subject: [RFC] w1: regression, remove need for ida and use PLATFORM_DEVID_AUTO In-Reply-To: <308e185e-26eb-39ee-d82f-a0d210a41743@ti.com> References: <308e185e-26eb-39ee-d82f-a0d210a41743@ti.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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