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

[Gabor Juhos <juhosg@openwrt.org>]

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