[PATCH v4 08/32] cxlflash: Fix to avoid CXL services during EEH

[Matthew R. Ochs]

> On Sep 28, 2015, at 6:05 PM, Daniel Axtens <dja at axtens.net> wrote:
> 
> You have two versions of check_state() below, which is a bit
> confusing. It looks like you've moved the function and also added the
> up/down of the read semaphore. I assume that's all that changed?

Correct.

It was originally moved to meet a dependency due to it being defined statically.

> 
>> 
>> /**
>> + * check_state() - checks and responds to the current adapter state
>> + * @cfg:	Internal structure associated with the host.
>> + *
>> + * This routine can block and should only be used on process context.
>> + * It assumes that the caller is an ioctl thread and holding the ioctl
>> + * read semaphore. This is temporarily let up across the wait to allow
>> + * for draining actively running ioctls. Also note that when waking up
>> + * from waiting in reset, the state is unknown and must be checked again
>> + * before proceeding.
>> + *
>> + * Return: 0 on success, -errno on failure
>> + */
>> +static int check_state(struct cxlflash_cfg *cfg)
>> +{
>> +	struct device *dev = &cfg->dev->dev;
>> +	int rc = 0;
>> +
>> +retry:
>> +	switch (cfg->state) {
>> +	case STATE_LIMBO:
>> +		dev_dbg(dev, "%s: Limbo state, going to wait...\n", __func__);
>> +		up_read(&cfg->ioctl_rwsem);
>> +		rc = wait_event_interruptible(cfg->limbo_waitq,
>> +					      cfg->state != STATE_LIMBO);
>> +		down_read(&cfg->ioctl_rwsem);
>> +		if (unlikely(rc))
>> +			break;
>> +		goto retry;
>> +	case STATE_FAILTERM:
>> +		dev_dbg(dev, "%s: Failed/Terminating!\n", __func__);
>> +		rc = -ENODEV;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return rc;
>> +}
>> +
>> +/**
>>  * cxlflash_disk_attach() - attach a LUN to a context
>>  * @sdev:	SCSI device associated with LUN.
>>  * @attach:	Attach ioctl data structure.
>> @@ -1523,41 +1563,6 @@ err1:
>> }
>> 
>> /**
>> - * check_state() - checks and responds to the current adapter state
>> - * @cfg:	Internal structure associated with the host.
>> - *
>> - * This routine can block and should only be used on process context.
>> - * Note that when waking up from waiting in limbo, the state is unknown
>> - * and must be checked again before proceeding.
>> - *
>> - * Return: 0 on success, -errno on failure
>> - */
>> -static int check_state(struct cxlflash_cfg *cfg)
>> -{
>> -	struct device *dev = &cfg->dev->dev;
>> -	int rc = 0;
>> -
>> -retry:
>> -	switch (cfg->state) {
>> -	case STATE_LIMBO:
>> -		dev_dbg(dev, "%s: Limbo, going to wait...\n", __func__);
>> -		rc = wait_event_interruptible(cfg->limbo_waitq,
>> -					      cfg->state != STATE_LIMBO);
>> -		if (unlikely(rc))
>> -			break;
>> -		goto retry;
>> -	case STATE_FAILTERM:
>> -		dev_dbg(dev, "%s: Failed/Terminating!\n", __func__);
>> -		rc = -ENODEV;
>> -		break;
>> -	default:
>> -		break;
>> -	}
>> -
>> -	return rc;
>> -}
>> -
>> -/**
>>  * cxlflash_afu_recover() - initiates AFU recovery
>>  * @sdev:	SCSI device associated with LUN.
>>  * @recover:	Recover ioctl data structure.
>> @@ -1646,9 +1651,14 @@ retry_recover:
>> 	/* Test if in error state */
>> 	reg = readq_be(&afu->ctrl_map->mbox_r);
>> 	if (reg == -1) {
>> -		dev_dbg(dev, "%s: MMIO read fail! Wait for recovery...\n",
>> -			__func__);
>> -		mutex_unlock(&ctxi->mutex);
>> +		dev_dbg(dev, "%s: MMIO fail, wait for recovery.\n", __func__);
>> +
>> +		/*
>> +		 * Before checking the state, put back the context obtained with
>> +		 * get_context() as it is no longer needed and sleep for a short
>> +		 * period of time (see prolog notes).
>> +		 */
>> +		put_context(ctxi);
> 
> Is this needed for the drain to work? It looks like it fixes a
> refcounting bug in the function, but I'm not sure I understand how it
> interacts with the rest of this patch.

This was simply some "while I'm here" refactoring as the commit originally
included a change here. The main point of this change was to replace the
mutex_unlock() with put_context(), which is a wrapper around the unlocking
of the context's mutex.
> 
> Anyway, the patch overall looks good to me, and makes your driver
> interact with CXL's EEH support in the way I intended when I wrote it.

Thanks for reviewing.