[U-Boot] [PATCH] w1: fix data abort if no one wire bus master present

[Eugen.Hristev at microchip.com <Eugen.Hristev@microchip.com>]

On 23.10.2018 11:17, Martin Fuzzey wrote:
> Hi Eugen,
> 
> On 23/10/18 09:05, Eugen.Hristev at 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
> 
>