linux-kernel - Re: [PATCH] hid: Initialize battery_no to -1 & correct its format string

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 9 Jan 2015 18:03:26 +0530
From:	Aniroop Mathur <aniroop.mathur@...il.com>
To:	Benjamin Tissoires <benjamin.tissoires@...il.com>
Cc:	Ping Cheng <pinglinux@...il.com>,
	Jason Gerecke <killertofu@...il.com>,
	Ping Cheng <pingc@...om.com>, Jiri Kosina <jkosina@...e.cz>,
	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Aniroop Mathur <a.mathur@...sung.com>
Subject: Re: [PATCH] hid: Initialize battery_no to -1 & correct its format string

Hello Mr. Benjamin,

On Wed, Jan 7, 2015 at 7:20 AM, Benjamin Tissoires
<benjamin.tissoires@...il.com> wrote:
> On Tue, Jan 6, 2015 at 7:50 PM, Ping Cheng <pinglinux@...il.com> wrote:
>> Hi Benjamin,
>>
>> The relevant code was introduced by
>> d70420b914c98a3758674c6e9858810e0ab4ea30. Can you take a look and let
>> us know if Aniroop's patch fits your original thought or not?
>
> Hehe, indeed, that's my code.
>
> I would agree with Jiri, I really do not care. This code is called
> only once at initialization and I am pretty sure it will not change
> the time to initialize the device (I am confident the compiler will do
> something better than blindly following the code).
>
> So either way it is good. But I have a slight preference to initialize
> the variable to 0 and keep the unsigned in the name.
>
> Cheers,
> Benjamin
>

Atleast, there is a need to fix format specifier of "n" variable.
"n" is declared as unsigned long so %ld should be changed to %lu.

Thanks !!

>>
>> Thanks,
>>
>> Ping
>>
>> On Tue, Jan 6, 2015 at 6:32 AM, Aniroop Mathur <aniroop.mathur@...il.com> wrote:
>>> Dear Mr. Jason and Mr. Ping,
>>>
>>> Please update about below patch.
>>> Except avoiding subtraction, there is also a need to avoid negative
>>> battery name which may come due to %ld, so need to change to %lu.
>>>
>>> Thanks,
>>> Aniroop
>>>
>>> On Mon, Dec 29, 2014 at 5:33 PM, Jiri Kosina <jkosina@...e.cz> wrote:
>>>> On Sun, 28 Dec 2014, Aniroop Mathur wrote:
>>>>
>>>>> From: Aniroop Mathur <a.mathur@...sung.com>
>>>>>
>>>>> This patch initializes battery_no to -1 to avoid extra subtraction
>>>>> operation performed everytime wacom battery is initialized
>>>>
>>>> This is so femto-optimization, that I don't really care deeply. Adding
>>>> Jason and Ping to CC. If they want this, I'll apply the patch.
>>>>
>>>>> and correct format string of unsigned long from %ld to %lu.
>>>>>
>>>>> Signed-off-by: Aniroop Mathur <a.mathur@...sung.com>
>>>>> ---
>>>>>  drivers/hid/wacom_sys.c | 8 ++++----
>>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>>>>> index 129fd33..4b5ff84 100644
>>>>> --- a/drivers/hid/wacom_sys.c
>>>>> +++ b/drivers/hid/wacom_sys.c
>>>>> @@ -919,17 +919,17 @@ static int wacom_ac_get_property(struct power_supply *psy,
>>>>>
>>>>>  static int wacom_initialize_battery(struct wacom *wacom)
>>>>>  {
>>>>> -     static atomic_t battery_no = ATOMIC_INIT(0);
>>>>> +     static atomic_t battery_no = ATOMIC_INIT(-1);
>>>>>       int error;
>>>>>       unsigned long n;
>>>>>
>>>>>       if (wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) {
>>>>> -             n = atomic_inc_return(&battery_no) - 1;
>>>>> +             n = atomic_inc_return(&battery_no);
>>>>>
>>>>>               wacom->battery.properties = wacom_battery_props;
>>>>>               wacom->battery.num_properties = ARRAY_SIZE(wacom_battery_props);
>>>>>               wacom->battery.get_property = wacom_battery_get_property;
>>>>> -             sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%ld", n);
>>>>> +             sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%lu", n);
>>>>>               wacom->battery.name = wacom->wacom_wac.bat_name;
>>>>>               wacom->battery.type = POWER_SUPPLY_TYPE_BATTERY;
>>>>>               wacom->battery.use_for_apm = 0;
>>>>> @@ -937,7 +937,7 @@ static int wacom_initialize_battery(struct wacom *wacom)
>>>>>               wacom->ac.properties = wacom_ac_props;
>>>>>               wacom->ac.num_properties = ARRAY_SIZE(wacom_ac_props);
>>>>>               wacom->ac.get_property = wacom_ac_get_property;
>>>>> -             sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%ld", n);
>>>>> +             sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%lu", n);
>>>>>               wacom->ac.name = wacom->wacom_wac.ac_name;
>>>>>               wacom->ac.type = POWER_SUPPLY_TYPE_MAINS;
>>>>>               wacom->ac.use_for_apm = 0;
>>>>> --
>>>>> 1.9.1
>>>>>
>>>>
>>>> --
>>>> Jiri Kosina
>>>> SUSE Labs
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>>> the body of a message to majordomo@...r.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/