Re: A potential broken at platform driver?

From: Richard Gong
Date: Mon Jun 03 2019 - 19:00:18 EST

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

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?



greg k-h