linux-kernel - Re: [PATCH 4/4] extcon: arizona: Eliminate dead error handling code

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]

Message-id: <5282DFAC.2000003@samsung.com>
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/