[U-Boot] [PATCH] w1-eeprom: ds24xxx: fix data abort in ds24xxx_probe()

[U-Boot] [PATCH] w1-eeprom: ds24xxx: fix data abort in ds24xxx_probe()

[Martin Fuzzey]

Data abort was occurring when using "w1 bus" with a DS24B33 present.

The abort occurred in the ds24xxx_probe() because the struct w1_device
pointer was NULL. This is because that structure  is allocated by
the parent device uclass (by .per_child_platdata_auto_alloc_size)
and thus the correct accessor is dev_get_parent_platdata() not
dev_get_platdata()

Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
---
 drivers/w1-eeprom/ds24xxx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/w1-eeprom/ds24xxx.c b/drivers/w1-eeprom/ds24xxx.c
index 56186e5..0967acc 100644
--- a/drivers/w1-eeprom/ds24xxx.c
+++ b/drivers/w1-eeprom/ds24xxx.c
@@ -31,7 +31,7 @@ static int ds24xxx_probe(struct udevice *dev)
 {
 	struct w1_device *w1;
 
-	w1 = dev_get_platdata(dev);
+	w1 = dev_get_parent_platdata(dev);
 	w1->id = 0;
 	return 0;
 }
-- 
1.9.1

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

[Martin Fuzzey]

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) {
+		debug("Cannot find w1 bus %d\n", busnum);
+		ret = -ENODEV;
+	}
+
 	return ret;
 }
 
-- 
1.9.1

[U-Boot] [PATCH] w1-eeprom: ds24xxx: fix data abort in ds24xxx_probe()

[Eugen.Hristev at microchip.com]

On 22.10.2018 19:31, Martin Fuzzey wrote:
> Data abort was occurring when using "w1 bus" with a DS24B33 present.
> 
> The abort occurred in the ds24xxx_probe() because the struct w1_device
> pointer was NULL. This is because that structure  is allocated by
> the parent device uclass (by .per_child_platdata_auto_alloc_size)
> and thus the correct accessor is dev_get_parent_platdata() not
> dev_get_platdata()
> 
> Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>

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

Hi Martin,

At some point during development I switched from holding the data in 
platdata to parent platdata, and this slipped.

Thanks for the fix.
Eugen

> ---
>   drivers/w1-eeprom/ds24xxx.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/w1-eeprom/ds24xxx.c b/drivers/w1-eeprom/ds24xxx.c
> index 56186e5..0967acc 100644
> --- a/drivers/w1-eeprom/ds24xxx.c
> +++ b/drivers/w1-eeprom/ds24xxx.c
> @@ -31,7 +31,7 @@ static int ds24xxx_probe(struct udevice *dev)
>   {
>   	struct w1_device *w1;
>   
> -	w1 = dev_get_platdata(dev);
> +	w1 = dev_get_parent_platdata(dev);
>   	w1->id = 0;
>   	return 0;
>   }
> 

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

[Eugen.Hristev at microchip.com]

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;
>   }
>   
> 

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

[Martin Fuzzey]

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.

> 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.

Regards,

Martin

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

[Eugen.Hristev at 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
> 
> 

[U-Boot] w1-eeprom: ds24xxx: fix data abort in ds24xxx_probe()

[Tom Rini]

On Mon, Oct 22, 2018 at 06:31:07PM +0200, Martin Fuzzey wrote:

> Data abort was occurring when using "w1 bus" with a DS24B33 present.
> 
> The abort occurred in the ds24xxx_probe() because the struct w1_device
> pointer was NULL. This is because that structure  is allocated by
> the parent device uclass (by .per_child_platdata_auto_alloc_size)
> and thus the correct accessor is dev_get_parent_platdata() not
> dev_get_platdata()
> 
> Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
> Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181102/7e1f582b/attachment.sig>

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

[Tom Rini]

On Mon, Oct 22, 2018 at 06:31:08PM +0200, 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>
> Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181102/ef1c74df/attachment.sig>