From mboxrd@z Thu Jan  1 00:00:00 1970
From: wyung@micron.com (Winson Yung (wyung))
Date: Mon, 26 May 2014 22:15:13 -0700
Subject: [PATCH] NVMe: Add support to receive NVMe asynchronous events
In-Reply-To: <20140526235156.GS6121@linux.intel.com>
References: <53839E67.1010006@micron.com>
 <20140526235156.GS6121@linux.intel.com>
Message-ID: <53841F61.4000500@micron.com>

On 5/26/2014 4:51 PM, Matthew Wilcox wrote:
> On Mon, May 26, 2014@01:04:55PM -0700, Winson Yung (wyung) wrote:
>> As a NVMe mandatory admin command, this driver should be setup so
>> that it can receive drive critical asynchronous notification for
>> issue such as device reliability, temperature above threshold, or
>> available spare space fallen below threshould. This patch enables
>> very basic mechanism to log the asynchronous events in kernel log.
>
> Umm ... did you test this patch?

I didn't have the time to test the patch yet, perhaps I should mark this 
patch as RFC instead. But yes, I will try to verify when I have a system 
setup.

>
>> Signed-off-by: Winson Yung <wyung at micron.com>
>> ---
>>   drivers/block/nvme-core.c |   57 +++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 57 insertions(+)
>>
>> diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
>> index cd8a8bc7..4cd9f8e 100644
>> --- a/drivers/block/nvme-core.c
>> +++ b/drivers/block/nvme-core.c
>> @@ -198,6 +198,7 @@ static int alloc_cmdid_killable(struct nvme_queue *nvmeq, void *ctx,
>>   #define CMD_CTX_COMPLETED	(0x310 + CMD_CTX_BASE)
>>   #define CMD_CTX_INVALID		(0x314 + CMD_CTX_BASE)
>>   #define CMD_CTX_ABORT		(0x318 + CMD_CTX_BASE)
>> +#define CMD_CTX_ASYNC_EVENT	(0x319 + CMD_CTX_BASE)
>
> Please only use multiples of four; that is, 0x31c, not 0x319.  Also, these
> are for use by 'special_completion', not 'async_completion'.
>

Understood about the multiple of four thingy, I will fix it. I will 
change the name to avoid confusion if I can still use async_completion.

>> +static int nvme_enable_async_events(struct nvme_dev *dev)
>> +{
>> +	int status, cmdid;
>> +	u32 result, async_events;
>> +	struct nvme_queue *adminq;
>> +
>> +	async_events = NVME_SMART_CRIT_SPARE |
>> +			NVME_SMART_CRIT_TEMPERATURE |
>> +			NVME_SMART_CRIT_RELIABILITY |
>> +			NVME_SMART_CRIT_MEDIA |
>> +			NVME_SMART_CRIT_VOLATILE_MEMORY;
>> +
>> +	status = nvme_set_features(dev, NVME_FEAT_ASYNC_EVENT,
>> +					async_events, 0, &result);
>> +
>> +	if (status < 0)
>> +		return status;
>> +
>> +	if (status > 0) {
>> +		dev_err(&dev->pci_dev->dev, "Could not enable async event (%d)\n",
>> +									status);
>> +		return -EBUSY;
>> +	}
>> +
>> +	adminq = rcu_dereference(dev->queues[0]);
>> +
>> +	/* Allocate a cmdid entry in preparation of next incoming async event */
>> +	cmdid = alloc_cmdid(adminq, CMD_CTX_ASYNC_EVENT, async_completion, 0);
>> +	return cmdid;
>> +}
>
> How is this supposed to work?  You've allocated a command ID, but you
> haven't submitted an asynchronous event request command.  So how does
> the controller know to use this command ID to report asynchronous events?
>

Please see my reply to Keith's email, perhaps my assumption was wrong.

> You should probably take a look at Keith's work last year to allow userspace
> to send async event requests:
>
> http://lists.infradead.org/pipermail/linux-nvme/2013-August/000350.html
>

Thanks for the link, I wasn't aware of that. I will take a look.