[PATCH] pci: Fix device_find_first_child() return value handling
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;
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
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.
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
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
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!
participants (4)
-
Marek Vasut -
Michal Suchánek -
Simon Glass -
Tom Rini