* [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