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:	Wed, 13 Nov 2013 11:10:52 +0900
From:	Chanwoo Choi <cw00.choi@...sung.com>
To:	Charles Keepax <ckeepax@...nsource.wolfsonmicro.com>
Cc:	myungjoo.ham@...sung.com, sameo@...ux.intel.com,
	lee.jones@...aro.org, patches@...nsource.wolfsonmicro.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] extcon: arizona: Eliminate dead error handling code

On 11/12/2013 11:30 PM, Charles Keepax wrote:
> On Tue, Nov 12, 2013 at 09:15:29AM +0900, Chanwoo Choi wrote:
>> Hi CHarles,
>>
>> On 11/08/2013 10:19 PM, Charles Keepax wrote:
>>> As a small disclaimer I would personally prefer to not merge this patch.
>>> I have added it based on previous code review of the other patches in
>>> this chain.
>>>
>>> arizona_hpdet_do_id currently can only return 0 or -EAGAIN making the
>>> else if clause handling error codes redundant, this patch removes this
>>> clause.
>>>
>>> Whilst this clause is not currently hit removing it makes the code
>>> fragile. It will not be obvious whilst editing arizona_hpdet_do_id that
>>> you shouldn't add a return value other than 0 or -EAGAIN.
>>>
>>> Signed-off-by: Charles Keepax <ckeepax@...nsource.wolfsonmicro.com>
>>> ---
>>>  drivers/extcon/extcon-arizona.c |    5 +++--
>>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
>>> index 0d70bf6..2313b1e 100644
>>> --- a/drivers/extcon/extcon-arizona.c
>>> +++ b/drivers/extcon/extcon-arizona.c
>>> @@ -476,6 +476,9 @@ static int arizona_hpdet_read(struct arizona_extcon_info *info)
>>>  	return val;
>>>  }
>>>  
>>> +/* This function should only return 0 or -EAGAIN, if other return values are
>>> + * added additional handling should be added in arizona_hpdet_irq.
>>> + */
>>
>> As Lee Jones commented, you should modify this comment of arizona_hpdet_do_id() and add
>> the description of return value. Because arizoa_hpdet_do_id() has different meaning
>> between -EAGAIN and other minus value.
> 
> I take it you are still very keen on applying a patch for this
> dead code elimination? I really do feel it would be better to
> leave this part of the code as it currently is, the extra safety
> clearly outweights the cost of a redundant else if.

In that case, I think that you could modify arizona_hpdet_read() to add exception handling
for minus return value instead of elimination.

I knew about your opinion. But I think it isn't proper method remaining dead code on mainline tree.
People who don't know the history about this dead code would think that "Is this code necessary?".

Thanks,
Chanwoo Choi


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

Powered by blists - more mailing lists