[U-Boot-Users] [PATCH] PCI_READ_VIA_DWORD_OP: initialize val parameter on fail

[Wolfgang Denk]

In message <4665122B.6010707 at necel.com> you wrote:
> 
> pci_hose_read_config_{byte,word}_via_dword uses a temporary read
> buffer `val32', so if read_config_dword returns -1 then val32 also
> should be initialized.

This is actually misleading, since you  don't  initialize  the  local
variable  "val32"  (which would not ake sense as it goes out of scope
anyway when you return from  the  function).  Instead,  you  use  the
variable  (more exatly, the pointer), the name of which was passed as
a macro argument.

> Without this fix we'll go on scanning bus with vendor or header_
> type uninitialized. This brings many unnecessary config trials.

Agreed. The purpose of the patch is OK, but I have to  admit  that  I
don't like the implementation.

> compiled and tested with our MIPS board.
> 
> Thanks,
> Shinya

Your Signed-off-by: line is missing here.

> @@ -83,7 +83,10 @@ int pci_hose_read_config_##size##_via_dw
>  	u32 val32;							\
>  									\
>  	if (pci_hose_read_config_dword(hose, dev, offset & 0xfc, &val32) < 0)\
> +	{								\

Bad brace style here.

> +		error_code;						\
...
> +PCI_READ_VIA_DWORD_OP(byte, u8 *, 0x03, *val = 0xff)
> +PCI_READ_VIA_DWORD_OP(word, u16 *, 0x02, *val = 0xffff)

I know that this is not your invention, and you  just  copy  existing
code,  but  I  think  injecting  C statements like this through macro
arguments is bad style and should be avoided. 

I have to admit that I don't get wht you need all this "error_code"
trickery instead of simpley writing

	*val = -1;


Can you please change your patch like that, and while you are at it
please fix the other uses of this ugly construct as well?

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
So we follow our wandering paths, and the very darkness acts  as  our
guide and our doubts serve to reassure us. - Jean-Pierre de Caussade,
eighteenth-century Jesuit priest