Re: [PATCH] pci: Fix device_find_first_child() return value handling

["Michal Suchánek" <msuchanek@suse.de>]

Hello,

On Sun, Jul 16, 2023 at 05:53:24PM +0200, Marek Vasut wrote:
> This function only ever returns 0, but may not assign the second
> parameter. Same thing for device_find_next_child(). Do not assign
> ret to stop proliferation of this misuse.
> 
> Reported-by: Jonas Karlman <jonas@kwiboo.se>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: "Pali Rohár" <pali@kernel.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Michal Suchanek <msuchanek@suse.de>
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  drivers/pci/pci-uclass.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> index 8d27e40338c..6421eda7721 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -545,9 +545,9 @@ int pci_auto_config_devices(struct udevice *bus)
>  	sub_bus = dev_seq(bus);
>  	debug("%s: start\n", __func__);
>  	pciauto_config_init(hose);
> -	for (ret = device_find_first_child(bus, &dev);
> -	     !ret && dev;
> -	     ret = device_find_next_child(&dev)) {
> +	for (device_find_first_child(bus, &dev);
> +	     dev;
> +	     device_find_next_child(&dev)) {

Sounds like you will need to remove the declaration of the now unused ret
variable as well.

More generally, what is the overall vision for these functions returning
always zero?

Should the return value be kept in case the underlying implementation
changes and errors can happen in the future, and consequently checked?

Should the return value be removed when meaningless making these
useless assignments and checks an error?

I already elimimnated a return value where using it lead to incorrect
behavior but here using it or not is equally correct with the current
implementation.

Thanks

Michal