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@linux.intel.com 
>> 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