A potential broken at platform driver?

[Richard Gong]

This is a multi-part message in MIME format.
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.

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

2. Using devm_device_add_groups

[    1.191196] *** rsu_status_callback:
[    1.194864] res->a1=2000000
[    1.197660] res->a1=0
[    1.199928] res->a2=0
[    1.202204] res->a3=0
[    1.204479] priv=0xffff80007a427e80
[    1.207968] version=0, state=0, current_image=2000000, fail_image=0, 
error_location=0, error_details=0
[    1.217322] *** stratix10_rsu_probe: priv=0xffff80007a427e80
root@stratix10:/sys/devices/platform/stratix10-rsu.0# cat current_image
[   39.032648] *** current_image_show: priv=0xffff80007a427e80
0x2000000
 ... output all correct data and correct priv pointer

I checked kernel sources and observed that .groups = xx_groups are 
widely used in 
device/misdevice/device_type/device_driver/bus_driver/pci_driver etc, 
but not in platform driver.

A few platform drivers which does utilize groups,
1. driver/s390/char/sclp.c does use .group = xx_groups, but it use the 
global variables for data exchanges between functions.
2. driver/firmware/arm_scpi.c doesn't use .group = xx_groups, instead it 
use devm_device_add_groups().

Regards,
Richard



On 5/29/19 9:55 AM, Richard Gong wrote:
> Hi Greg,
>
> On 5/28/19 6:22 PM, Greg KH wrote:
> > On Tue, May 28, 2019 at 03:20:31PM -0500, richard.gong@xxxxxxxxxxxxxxx 
> > wrote:
> > > +/**
> > > + * rsu_send_msg() - send a message to Intel service layer
> > > + * @priv: pointer to rsu private data
> > > + * @command: RSU status or update command
> > > + * @arg: the request argument, the bitstream address or notify status
> > > + * @callback: function pointer for the callback (status or update)
> > > + *
> > > + * Start an Intel service layer transaction to perform the SMC call 
> > > that
> > > + * is necessary to get RSU boot log or set the address of bitstream to
> > > + * boot after reboot.
> > > + *
> > > + * Returns 0 on success or -ETIMEDOUT on error.
> > > + */
> > > +static int rsu_send_msg(struct stratix10_rsu_priv *priv,
> > > +ÂÂÂÂÂÂÂÂÂÂÂ enum stratix10_svc_command_code command,
> > > +ÂÂÂ unsigned long arg,
> > > +ÂÂÂ void (*callback)(struct stratix10_svc_client *client,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂ struct stratix10_svc_cb_data *data))
> > > +{
> > > +ÂÂÂ struct stratix10_svc_client_msg msg;
> > > +ÂÂÂ int ret;
> > > +
> > > +ÂÂÂ mutex_lock(&priv->lock);
> > > +ÂÂÂ reinit_completion(&priv->completion);
> > > +ÂÂÂ priv->client.receive_cb = callback;
> > > +
> > > +ÂÂÂ msg.command = command;
> > > +ÂÂÂ if (arg)
> > > +ÂÂÂÂÂÂÂ msg.arg[0] = arg;
> > > +
> > > +ÂÂÂ ret = stratix10_svc_send(priv->chan, &msg);
> >
> > meta-question, can you send messages that are on the stack and not in
> > DMA-able memory? Or should this be a dynamicly created variable so you
> > know it can work properly with DMA?
> >
> > And how big is that structure, will it mess with stack sizes?
>
> stratix10_svc_send() is a function from Intel Stratix10 service layer 
> driver, which is used by service clients (RSU and FPGA manager drivers 
> as now) to add a message to the service layer driver's queue for being 
> sent to the secure world via SMC call.
>
> It is not DMA related, we send messages via FIFO API.
>
> The size of FIFO is sizeof(struct stratix10_svc_data) * 
> SVC_NUM_DATA_IN_FIFO, SVC_NUM_DATA_IN_FIFO is defined as 32.
>
> fifo_size = sizeof(struct stratix10_svc_data) *ÂÂÂÂ SVC_NUM_DATA_IN_FIFO;
> ret = kfifo_alloc(&controller->svc_fifo, fifo_size, GFP_KERNEL);
> if (ret) {
>  ÂÂÂÂdev_err(dev, "failed to allocate FIFO\n");
>  ÂÂÂÂÂÂÂ return ret;
> }
> spin_lock_init(&controller->svc_fifo_lock);
>
> It will not mess with stack sizes.
>
> > thanks,
> >
> > greg k-h
>
> Regards,
> Richard