> On Sep 28, 2015, at 6:05 PM, Daniel Axtens <d...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
