Hi Marek,
On Mon, 17 Jul 2023 at 11:03, Marek Vasut <ma...@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 <jo...@kwiboo.se>
> >> Signed-off-by: Marek Vasut <ma...@denx.de>
> >> ---
> >> Cc: "Pali Rohár" <p...@kernel.org>
> >> Cc: Bin Meng <bmeng...@gmail.com>
> >> Cc: Marek Vasut <ma...@denx.de>
> >> Cc: Michal Suchanek <msucha...@suse.de>
> >> Cc: Simon Glass <s...@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 <s...@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
