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

[Simon Glass]

Hi Marek,

On Mon, 17 Jul 2023 at 11:03, Marek Vasut <marex at denx.de> wrote:
>
> On 7/17/23 09:42, Michal Suchánek wrote:
> > 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 at kwiboo.se>
> >> Signed-off-by: Marek Vasut <marex at denx.de>
> >> ---
> >> Cc: "Pali Rohár" <pali at kernel.org>
> >> Cc: Bin Meng <bmeng.cn at gmail.com>
> >> Cc: Marek Vasut <marex at denx.de>
> >> Cc: Michal Suchanek <msuchanek at suse.de>
> >> Cc: Simon Glass <sjg at 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)) {

Reviewed-by: Simon Glass <sjg at chromium.org>

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

Yes, please remove the 'ret' at the top of the function.

> >
> > 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.
>
> Probably a question for Simon, really. Personally I would be tempted to
> switch the function to return void.

So long as the function has its meaning documented, I think it is OK.
As a separate patch, I am OK with changing
device_find_first/next_child() to void, or alternatively having them
return 0 on success and -ENODEV if nothing was found.

Regards,
Simon