Re: [PATCH] dmaengine: idxd: add command status to idxd sysfs attribute

[Dave Jiang <dave.jiang@intel.com>]

On 8/28/2020 3:42 AM, Vinod Koul wrote:
> On 27-08-20, 14:24, Dave Jiang wrote:
>> Export admin command status to sysfs attribute in order to allow user to
>> retrieve configuration error. Allows user tooling to retrieve the command
>> error and provide more user friendly error messages.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/dma/idxd/device.c |    6 +++++-
>>   drivers/dma/idxd/idxd.h   |    1 +
>>   drivers/dma/idxd/sysfs.c  |   10 ++++++++++
>>   3 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
>> index 00dab1465ca3..bdca1bc55cfa 100644
>> --- a/drivers/dma/idxd/device.c
>> +++ b/drivers/dma/idxd/device.c
>> @@ -368,6 +368,7 @@ static void idxd_cmd_exec(struct idxd_device *idxd, int cmd_code, u32 operand,
>>   	dev_dbg(&idxd->pdev->dev, "%s: sending cmd: %#x op: %#x\n",
>>   		__func__, cmd_code, operand);
>>   
>> +	idxd->cmd_status = 0;
>>   	__set_bit(IDXD_FLAG_CMD_RUNNING, &idxd->flags);
>>   	idxd->cmd_done = &done;
>>   	iowrite32(cmd.bits, idxd->reg_base + IDXD_CMD_OFFSET);
>> @@ -379,8 +380,11 @@ static void idxd_cmd_exec(struct idxd_device *idxd, int cmd_code, u32 operand,
>>   	spin_unlock_irqrestore(&idxd->dev_lock, flags);
>>   	wait_for_completion(&done);
>>   	spin_lock_irqsave(&idxd->dev_lock, flags);
>> -	if (status)
>> +	if (status) {
>>   		*status = ioread32(idxd->reg_base + IDXD_CMDSTS_OFFSET);
>> +		idxd->cmd_status = *status & 0xff;
> 
> define the magic 0xff and use GENMASK to define that!

Ok will do.

> 
>> +	}
>> +
>>   	__clear_bit(IDXD_FLAG_CMD_RUNNING, &idxd->flags);
>>   	/* Wake up other pending commands */
>>   	wake_up(&idxd->cmd_waitq);
>> diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
>> index e8bec6eb9f7e..c64df197e724 100644
>> --- a/drivers/dma/idxd/idxd.h
>> +++ b/drivers/dma/idxd/idxd.h
>> @@ -156,6 +156,7 @@ struct idxd_device {
>>   	unsigned long flags;
>>   	int id;
>>   	int major;
>> +	u8 cmd_status;
>>   
>>   	struct pci_dev *pdev;
>>   	void __iomem *reg_base;
>> diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
>> index c5f19802cb9e..1db8d021f5ae 100644
>> --- a/drivers/dma/idxd/sysfs.c
>> +++ b/drivers/dma/idxd/sysfs.c
>> @@ -1395,6 +1395,15 @@ static ssize_t cdev_major_show(struct device *dev,
>>   }
>>   static DEVICE_ATTR_RO(cdev_major);
>>   
>> +static ssize_t cmd_status_show(struct device *dev,
>> +			       struct device_attribute *attr, char *buf)
>> +{
>> +	struct idxd_device *idxd = container_of(dev, struct idxd_device, conf_dev);
>> +
>> +	return sprintf(buf, "%#x\n", idxd->cmd_status);
>> +}
>> +static DEVICE_ATTR_RO(cmd_status);
> 
> Update ABI docs for this?
> 

Yes

>> +
>>   static struct attribute *idxd_device_attributes[] = {
>>   	&dev_attr_version.attr,
>>   	&dev_attr_max_groups.attr,
>> @@ -1413,6 +1422,7 @@ static struct attribute *idxd_device_attributes[] = {
>>   	&dev_attr_max_tokens.attr,
>>   	&dev_attr_token_limit.attr,
>>   	&dev_attr_cdev_major.attr,
>> +	&dev_attr_cmd_status.attr,
>>   	NULL,
>>   };
>>   
>