On 22.10.2018 19:31, Martin Fuzzey wrote:

When the "w1 bus" command is used with no bus master present a data abort may occur.

This is because uclass_first_device() returns zero, but sets the output struct udevice pointer to NULL in the no device found case.

Fix w1_get_bus() to account for this and return an error code as is expected by the callers.

Signed-off-by: Martin Fuzzey martin.fuzzey@flowbird.group

drivers/w1/w1-uclass.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/w1/w1-uclass.c b/drivers/w1/w1-uclass.c index aecf7fe..cb41b68 100644 --- a/drivers/w1/w1-uclass.c +++ b/drivers/w1/w1-uclass.c @@ -115,17 +115,19 @@ int w1_get_bus(int busnum, struct udevice **busp) struct udevice *dev;

for (ret = uclass_first_device(UCLASS_W1, &dev);

•     !ret;
  

•     uclass_next_device(&dev), i++) {
  

• if (ret) {
  

• 	debug("Cannot find w1 bus %d\n", busnum);
  

• 	return ret;
  

• }
  

•     dev && !ret;
  

•     ret = uclass_next_device(&dev), i++) {
  

  if (i == busnum) { *busp = dev; return 0; } }
• if (!ret) {

Hi Martin,

Does this mean that if ret != 0 , we had errors, but we are not printing this debug message ? Perhaps we should print out here the debug error regardless of the ret value ? Since we exited with a return 0 if we did find the proper bus.

May I also ask on which board setup you tested this ? And which defconfig.

Thanks, Eugen

• debug("Cannot find w1 bus %d\n", busnum);
  

• ret = -ENODEV;
  

• }
• return ret; }

On 23.10.2018 11:17, Martin Fuzzey wrote:

Hi Eugen,

On 23/10/18 09:05, Eugen.Hristev@microchip.com wrote:

...

On 22.10.2018 19:31, Martin Fuzzey wrote:

...

When the "w1 bus" command is used with no bus master present a data abort may occur.

This is because uclass_first_device() returns zero, but sets the output struct udevice pointer to NULL in the no device found case.

Fix w1_get_bus() to account for this and return an error code as is expected by the callers.

Signed-off-by: Martin Fuzzey martin.fuzzey@flowbird.group

drivers/w1/w1-uclass.c | 14 ++++++++------    1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/w1/w1-uclass.c b/drivers/w1/w1-uclass.c index aecf7fe..cb41b68 100644 --- a/drivers/w1/w1-uclass.c +++ b/drivers/w1/w1-uclass.c @@ -115,17 +115,19 @@ int w1_get_bus(int busnum, struct udevice **busp)        struct udevice *dev;        for (ret = uclass_first_device(UCLASS_W1, &dev); -         !ret; -         uclass_next_device(&dev), i++) { -        if (ret) { -            debug("Cannot find w1 bus %d\n", busnum); -            return ret; -        } +         dev && !ret; +         ret = uclass_next_device(&dev), i++) {            if (i == busnum) {                *busp = dev;                return 0;            }        }

+    if (!ret) {

Hi Martin,

Does this mean that if ret != 0 , we had errors, but we are not printing this debug message ? Perhaps we should print out here the debug error regardless of the ret value ? Since we exited with a return 0 if we did find the proper bus.

With the current code ret != 0 means that a probe error occurred, not that the bus cannot be found, if no errors occurred but the requested bus did not exist (either because there were no busses at all or the index requested was too big) ret from uclass_first|next_device() == 0

In the error case "Cannot find w1 bus %d" is probably not the correct error message. I would have expected the failing driver to have printed a more explicit error message. So that is why I only print the debug message if ret == 0 but the device was not found.

Good for me.

In cmd/w1.c there is an error message printed regardless of the ret, so, return -ENODEV here if there is no dev is a good way of fixing it.

Reviewed-by: Eugen Hristev eugen.hristev@microchip.com

Thanks again for your fix

...

May I also ask on which board setup you tested this ? And which defconfig.

Tested on an i.MX53 based custom board. I am working on the W1 host controller driver for i.MX53 and plan to submit it soon.

I ran into this problem by accident when I forgot to add the DT entry for my new driver.

Great news, hope that the w1 framework implemented here will help you get on track easily.

Regards,

Martin