A potential broken at platform driver?

From: Richard Gong
Date: Mon Jun 03 2019 - 11:48:50 EST

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
 ... 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().


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;

It will not mess with stack sizes.


greg k-h