Re: A potential broken at platform driver?

[Richard Gong]

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