Re: A potential broken at platform driver?

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux