linux-mips
[Top] [All Lists]

Re: [PATCH] MIPS: ath79: Avoid using unitialized 'reg' variable

To: Markos Chandras <markos.chandras@gmail.com>
Subject: Re: [PATCH] MIPS: ath79: Avoid using unitialized 'reg' variable
From: Gabor Juhos <juhosg@openwrt.org>
Date: Fri, 16 Aug 2013 15:57:29 +0200
Cc: Jonas Gorski <jogo@openwrt.org>, Markos Chandras <markos.chandras@imgtec.com>, linux-mips@linux-mips.org
In-reply-to: <CAG2jQ8gkQgGYcsz4x7wgnhq18EzyK5qe64CLR3+iefqb8hGEvQ@mail.gmail.com>
List-archive: <http://www.linux-mips.org/archives/linux-mips/>
List-help: <mailto:ecartis@linux-mips.org?Subject=help>
List-id: linux-mips <linux-mips.eddie.linux-mips.org>
List-owner: <mailto:ralf@linux-mips.org>
List-post: <mailto:linux-mips@linux-mips.org>
List-software: Ecartis version 1.0.0
List-subscribe: <mailto:ecartis@linux-mips.org?subject=subscribe%20linux-mips>
List-unsubscribe: <mailto:ecartis@linux-mips.org?subject=unsubscribe%20linux-mips>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <1376384478-27424-1-git-send-email-markos.chandras@imgtec.com> <CAOiHx==9E9m5Ds0trutySyaxM0VLJfh1+LKcxYfWFWFt-8dx1A@mail.gmail.com> <CAG2jQ8iUWU83xWqfYF=ev9Gfxsx6ocBK-0wLaQs8QE=d2+5NmA@mail.gmail.com> <CAG2jQ8gkQgGYcsz4x7wgnhq18EzyK5qe64CLR3+iefqb8hGEvQ@mail.gmail.com>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20130509 Thunderbird/17.0.6
Hi Markos,
> On 15 August 2013 14:42, Markos Chandras <markos.chandras@gmail.com> wrote:
>> On 14 August 2013 12:12, Jonas Gorski <jogo@openwrt.org> wrote:
>>> Hi,
>>>
>>> On Tue, Aug 13, 2013 at 11:01 AM, Markos Chandras
>>> <markos.chandras@imgtec.com> wrote:
>>>> Fixes the following build error:
>>>> arch/mips/include/asm/mach-ath79/ath79.h:139:20: error: 'reg' may be used
>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>> arch/mips/ath79/common.c:62:6: note: 'reg' was declared here
>>>> In file included from arch/mips/ath79/common.c:20:0:
>>>> arch/mips/ath79/common.c: In function 'ath79_device_reset_clear':
>>>> arch/mips/include/asm/mach-ath79/ath79.h:139:20:
>>>> error: 'reg' may be used uninitialized in this function
>>>> [-Werror=maybe-uninitialized]
>>>> arch/mips/ath79/common.c:90:6: note: 'reg' was declared here
>>>>
>>>> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
>>>> ---

<snip>

>>>> +       } else {
>>>>                 BUG();
>>>> +               panic("Unknown SOC!");
>>>
>>> Both BUG() and panic() seems to be a bit overkill, especially since
>>> the panic won't be reached unless BUG is disabled - just the panic()
>>> should be enough.
>>>
>>> Also the panic message isn't very helpful, maybe print the raw id of
>>> the SoC here?
>>>
>>>
>>
>> Hi Jonas,
>>
>> Thank you for the review. I will submit a new patch.
>>
>> --
>> Regards,
>> Markos Chandras
> 
> Hi Jonas,
> 
> I had a look at the code again and it seems that reporting the 'id' is
> not needed since an unknown SOC will cause a panic
> earlier in the boot process. Look at arch/mips/ath79/setup.c, in the
> plat_mem_setup function.
> This one calls ath79_detect_sys_type which causes the following panic
> if an unknown SOC is detected.
> 
> panic("ath79: unknown SoC, id:0x%08x", id);
> 
> This makes me think that ath79_device_reset_set and
> ath79_device_reset_clear should not care about an unknown SOC at all.
> 

The BUG() call helps to ensure that the ath79_device_reset{clear,set} functions
will be modified when someone adds support for a new SoC. So I prefer to have a
panic() or at least a WARN()+return here. However, instead of the 'Unknown SoC!'
message, a 'reset register is not defined for the SoC' text would be more
meaningful given the actual context.

-Gabor


<Prev in Thread] Current Thread [Next in Thread>