Re: A potential broken at platform driver?

[Richard Gong <richard.gong@linux.intel.com>]

Hi Greg,

On 6/3/19 1:02 PM, Greg KH wrote:
> On Mon, Jun 03, 2019 at 10:57:18AM -0500, Richard Gong wrote:
>>
>> Hi Greg,
>>
>> Following your suggestion, I replaced devm_device_add_groups() with .group =
>> rus_groups in my version #4 submission. But I found out that RSU driver
>> outputs the garbage data if I use .group = rsu_groups.
> 
> What is "garbage"?
I mean incorrect status info.

> 
>> To make RSU driver work properly, I have to revert the change at version #4
>> and use devm_device_add_groups() again. Sorry, I didn't catch this problem
>> early.
>>
>> I did some debug & below are captured log, you can see priv pointer get
>> messed at current_image_show(). I am not sure if something related to
>> platform driver work properly. I attach my debug patch in this mail.
>>
>> 1. Using .groups = rsu_groups,
>>
>> [    1.191115] *** rsu_status_callback:
>> [    1.194782] res->a1=2000000
>> [    1.197588] res->a1=0
>> [    1.199865] res->a2=0
>> [    1.202150] res->a3=0
>> [    1.204433] priv=0xffff80007aa28e80
>> [    1.207933] version=0, state=0, current_image=2000000, fail_image=0,
>> error_location=0, error_details=0
>> [    1.217249] *** stratix10_rsu_probe: priv=0xffff80007aa28e80
>> root@stratix10:/sys/bus/platform/drivers/stratix10-rsu# cat current_image
>> [   38.849341] *** current_image_show: priv=0xffff80007aa28d00
>> ... output garbage data
>> priv pointer got changed
> 
> I don't understand this, sorry.  Are you sure you are actually using the
> correct pointer to your device?
> 
I think so.

The dev pointer at current_image_show() should points to RSU device, but 
it seems point to driver_private if I use .group = rsU_groups. As a 
result I can't get the priv pointer properly at current_image_show().

[    1.190993] *** rsu_status_callback:
[    1.194669] dev=0xffff80007b409410
[    1.198083] priv=0xffff80007a4d4e80
[    1.201582] version=0, state=0, current_image=0x2000000, 
fail_image=0x0, error_location=0x0, error_details=0
[    1.211416] *** stratix10_rsu_probe: priv=0xffff80007a4d4e80
[    1.217063] *** stratix10_rsu_probe: dev=0xffff80007b409410

root@stratix10:/sys/bus/platform/drivers/stratix10-rsu# cat current_image
[   72.101277] *** current_image_show: dev=stratix10_rsu_driver
[   72.136205] *** current_image_show: priv=0xffff80007a4d4d00

If I use devm_device_add_groups(), the dev pointer does point to RSU device,

[    1.191456] *** rsu_status_callback:
[    1.195124] priv=0xffff80007a429280
[    1.198615] version=0, state=0, current_image=0x2000000, 
fail_image=0x0, error_location=0x0, error_details=0
[    1.208458] *** stratix10_rsu_probe: priv=0xffff80007a429280
[    1.214105] *** stratix10_rsu_probe: dev=0xffff80007b409410

root@stratix10:/sys/devices/platform/stratix10-rsu.0# cat current_image
[   31.484131] *** current_image_show: dev=0xffff80007b409410
[   31.489651] *** current_image_show: priv=0xffff80007a429280


>> @@ -394,7 +432,7 @@ static struct platform_driver stratix10_rsu_driver = {
>>   	.remove = stratix10_rsu_remove,
>>   	.driver = {
>>   		.name = "stratix10-rsu",
>> -		.groups = rsu_groups,
>> +//		.groups = rsu_groups,
> 
> Are you sure this is the correct pointer?  I think that might be
> pointing to the driver's attributes, not the device's attributes.
> 
> If platform drivers do not have a way to register groups properly, then
> that really needs to be fixed, as trying to register it by yourself as
> you are doing, is ripe for racing with userspace.
> 
I agree we shouldn't call devm_device_add_groups() directly.

RSU status is only updated after power on or reboot, RSU driver get 
status info at probe() & save them to the private pointer priv via 
platform_set_drvdata().

static struct platform_driver stratix10_rsu_driver = {
         .probe = stratix10_rsu_probe,
         .remove = stratix10_rsu_remove,
         .driver = {
                 .name = "stratix10-rsu",
                 .groups = rsu_groups,
         },
};

The problem is that I don't have a way to properly retrieve the priv 
pointer at xx_show() functions. Global variable is a work around, but I 
don't think it is a good approach.

Any suggestion?

Regards,
Richard

> thanks,
> 
> greg k-h
>