* [PATCH] pci: Fix device_find_first_child() return value handling @ 2023-07-16 15:53 Marek Vasut 2023-07-17 7:42 ` Michal Suchánek 2023-08-15 14:42 ` Tom Rini 0 siblings, 2 replies; 6+ messages in thread From: Marek Vasut @ 2023-07-16 15:53 UTC (permalink / raw) To: u-boot Cc: Marek Vasut, Jonas Karlman, Pali Rohár, Bin Meng, Michal Suchanek, Simon Glass 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)) { unsigned int max_bus; int ret; -- 2.40.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] pci: Fix device_find_first_child() return value handling 2023-07-16 15:53 [PATCH] pci: Fix device_find_first_child() return value handling Marek Vasut @ 2023-07-17 7:42 ` Michal Suchánek 2023-07-17 17:03 ` Marek Vasut 2023-08-15 14:42 ` Tom Rini 1 sibling, 1 reply; 6+ messages in thread From: Michal Suchánek @ 2023-07-17 7:42 UTC (permalink / raw) To: Marek Vasut; +Cc: u-boot, Jonas Karlman, Pali Rohár, Bin Meng, Simon Glass 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pci: Fix device_find_first_child() return value handling 2023-07-17 7:42 ` Michal Suchánek @ 2023-07-17 17:03 ` Marek Vasut 2023-07-27 0:49 ` Simon Glass 0 siblings, 1 reply; 6+ messages in thread From: Marek Vasut @ 2023-07-17 17:03 UTC (permalink / raw) To: Michal Suchánek Cc: u-boot, Jonas Karlman, Pali Rohár, Bin Meng, Simon Glass 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@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. Probably a question for Simon, really. Personally I would be tempted to switch the function to return void. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pci: Fix device_find_first_child() return value handling 2023-07-17 17:03 ` Marek Vasut @ 2023-07-27 0:49 ` Simon Glass 2023-07-27 6:42 ` Michal Suchánek 0 siblings, 1 reply; 6+ messages in thread From: Simon Glass @ 2023-07-27 0:49 UTC (permalink / raw) To: Marek Vasut Cc: Michal Suchánek, u-boot, Jonas Karlman, Pali Rohár, Bin Meng Hi Marek, On Mon, 17 Jul 2023 at 11:03, Marek Vasut <marex@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@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)) { Reviewed-by: Simon Glass <sjg@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pci: Fix device_find_first_child() return value handling 2023-07-27 0:49 ` Simon Glass @ 2023-07-27 6:42 ` Michal Suchánek 0 siblings, 0 replies; 6+ messages in thread From: Michal Suchánek @ 2023-07-27 6:42 UTC (permalink / raw) To: Simon Glass; +Cc: Marek Vasut, u-boot, Jonas Karlman, Pali Rohár, Bin Meng Hello, On Wed, Jul 26, 2023 at 06:49:57PM -0600, Simon Glass wrote: > Hi Marek, > > On Mon, 17 Jul 2023 at 11:03, Marek Vasut <marex@denx.de> wrote: > > > > On 7/17/23 09:42, Michal Suchánek wrote: ... > > > 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. I think when there is one error condition having two ways to report it is one too many. Thanks Michal ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pci: Fix device_find_first_child() return value handling 2023-07-16 15:53 [PATCH] pci: Fix device_find_first_child() return value handling Marek Vasut 2023-07-17 7:42 ` Michal Suchánek @ 2023-08-15 14:42 ` Tom Rini 1 sibling, 0 replies; 6+ messages in thread From: Tom Rini @ 2023-08-15 14:42 UTC (permalink / raw) To: Marek Vasut Cc: u-boot, Jonas Karlman, Pali Rohár, Bin Meng, Michal Suchanek, Simon Glass [-- Attachment #1: Type: text/plain, Size: 438 bytes --] 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> > Reviewed-by: Simon Glass <sjg@chromium.org> Applied to u-boot/next, thanks! -- Tom [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 659 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-08-15 14:43 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-07-16 15:53 [PATCH] pci: Fix device_find_first_child() return value handling Marek Vasut 2023-07-17 7:42 ` Michal Suchánek 2023-07-17 17:03 ` Marek Vasut 2023-07-27 0:49 ` Simon Glass 2023-07-27 6:42 ` Michal Suchánek 2023-08-15 14:42 ` Tom Rini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox