[PATCH 1/1] If device hotplug out, controller status become 0xFFFF_FFFF 1. Controller fatal status become true in nvme_kthread() 2. Ready bit will not be reset to 0 in nvme_wait_ready() for controller disable above two scenarios controller status data is not a valid data, since device/controller is already removed (hotplug)

[Keith Busch]

On Tue, 22 Jul 2014, Mundu wrote:
> Signed-off-by: Mundu <mundu2510 at gmail.com>
> ---
> drivers/block/nvme-core.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
> index 28aec2d..6c6998e 100644
> --- a/drivers/block/nvme-core.c
> +++ b/drivers/block/nvme-core.c
> @@ -1396,10 +1396,14 @@ static int nvme_wait_ready(struct nvme_dev *dev, u64 cap, bool enabled)
> {
> 	unsigned long timeout;
> 	u32 bit = enabled ? NVME_CSTS_RDY : 0;
> +	u32 csts;
>
> 	timeout = ((NVME_CAP_TIMEOUT(cap) + 1) * HZ / 2) + jiffies;
>
> -	while ((readl(&dev->bar->csts) & NVME_CSTS_RDY) != bit) {
> +	while (((csts = readl(&dev->bar->csts)) & NVME_CSTS_RDY) != bit) {
> +		/* If device is hot plugout, csts become 0xFFFFFFFF */
> +		if (csts == -1)
> +			return -ENODEV;

In nvme_dev_map, we already return -ENODEV if reading CSTS returns
-1. That happens before we call nvme_wait_ready. Are you just shorting
the probe in the event someone hot removes a drive the moment after the
driver ioremaps the BAR, but before it enables the device?

> 		msleep(100);
> 		if (fatal_signal_pending(current))
> 			return -EINTR;
> @@ -1905,14 +1909,19 @@ static int nvme_submit_async_req(struct nvme_queue *nvmeq)
> static int nvme_kthread(void *data)
> {
> 	struct nvme_dev *dev, *next;
> +	u32 csts;
>
> 	while (!kthread_should_stop()) {
> 		set_current_state(TASK_INTERRUPTIBLE);
> 		spin_lock(&dev_list_lock);
> 		list_for_each_entry_safe(dev, next, &dev_list, node) {
> 			int i;
> -			if (readl(&dev->bar->csts) & NVME_CSTS_CFS &&
> +			if ((csts = readl(&dev->bar->csts)) & NVME_CSTS_CFS &&
> 							dev->initialized) {
> +				/* If device is hot plugout,
> +				   csts become 0xFFFFFFFF */
> +				if (csts == -1)
> +					continue;

You definitely want to let the reset work get scheduled here instead of
continuing. Some platforms do not support "surprise removal", so this
check guards against that by triggering a reset which will bail on the
device if it appears gone and remove it from pci.

> 				if (work_busy(&dev->reset_work))
> 					continue;
> 				list_del_init(&dev->node);