[PATCH] scsi: ratelimit error messages

[Wei Fang]

If we continue to send IOs when the scsi device is offline/deleting,
a lot of error messages will be printked, and may appear on serial
console. It will extremly slow down the system.

Fix this by using printk_ratelimit().

I noticed that printk_ratelimit() is warnned not to use, but using
printk_ratelimited() will lose the messages printked by sdev_printk(),
maybe we should remove all printk_ratelimit() in scsi layer next time
by rewriting sdev_printk().

Signed-off-by: Wei Fang <fangw...@huawei.com>
---
 drivers/scsi/scsi_lib.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e9e1e14..faebf74 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1222,8 +1222,9 @@ scsi_prep_state_check(struct scsi_device *sdev, struct 
request *req)
 * commands.  The device must be brought online
 * before trying any recovery commands.
 */
-   sdev_printk(KERN_ERR, sdev,
-   "rejecting I/O to offline device\n");
+   if (printk_ratelimit())
+   sdev_printk(KERN_ERR, sdev,
+   "rejecting I/O to offline 
device\n");
ret = BLKPREP_KILL;
break;
case SDEV_DEL:
@@ -1231,8 +1232,9 @@ scsi_prep_state_check(struct scsi_device *sdev, struct 
request *req)
 * If the device is fully deleted, we refuse to
 * process any commands as well.
 */
-   sdev_printk(KERN_ERR, sdev,
-   "rejecting I/O to dead device\n");
+   if (printk_ratelimit())
+   sdev_printk(KERN_ERR, sdev,
+   "rejecting I/O to dead device\n");
ret = BLKPREP_KILL;
break;
case SDEV_BLOCK:
@@ -1711,8 +1713,9 @@ static void scsi_request_fn(struct request_queue *q)
break;
 
if (unlikely(!scsi_device_online(sdev))) {
-   sdev_printk(KERN_ERR, sdev,
-   "rejecting I/O to offline device\n");
+   if (printk_ratelimit())
+   sdev_printk(KERN_ERR, sdev,
+   "rejecting I/O to offline 
device\n");
scsi_kill_request(req, q);
continue;
}
-- 
2.4.11

--
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

[PATCH v4] scsi: avoid a permanent stop of the scsi device's request queue

[Wei Fang]

A race between scanning and fc_remote_port_delete() may result in a
permanent stop if the device gets blocked before scsi_sysfs_add_sdev()
and unblocked after.  The reason is that blocking a device sets both
the SDEV_BLOCKED state and the QUEUE_FLAG_STOPPED.  However,
scsi_sysfs_add_sdev() unconditionally sets SDEV_RUNNING which causes
the device to be ignored by scsi_target_unblock() and thus never have
its QUEUE_FLAG_STOPPED cleared leading to a device which is apparently
running but has a stopped queue.

We actually have two places where SDEV_RUNNING is set: once in
scsi_add_lun() which respects the blocked flag and once in
scsi_sysfs_add_sdev() which doesn't.  Since the second set is entirely
spurious, simply remove it to fix the problem.

Reported-by: Zengxi Chen <chenzen...@huawei.com>
Signed-off-by: Wei Fang <fangw...@huawei.com>
Reviewed-by: Ewan D. Milne <emi...@redhat.com>
---
Changes v1->v2:
- don't modify scsi_internal_device_unblock(), just remove changing
  state to SDEV_RUNNING in scsi_sysfs_add_sdev(), suggested by
  James Bottomley and Ewan D. Milne.
Changes v2->v3
- Use a clearer description of this problem
Changes v3->v4
- Remove useless change of comment about SDEV_RUNNING

 drivers/scsi/scsi_sysfs.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 0734927..82dfe07 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1204,10 +1204,6 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
struct request_queue *rq = sdev->request_queue;
struct scsi_target *starget = sdev->sdev_target;
 
-   error = scsi_device_set_state(sdev, SDEV_RUNNING);
-   if (error)
-   return error;
-
error = scsi_target_add(starget);
if (error)
return error;
-- 
2.4.11

--
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

Re: [PATCH v3] scsi: avoid a permanent stop of the scsi device's request queue

[Wei Fang]

Hi, James, Ewan,

On 2016/12/13 0:43, James Bottomley wrote:
> On Mon, 2016-12-12 at 11:23 -0500, Ewan D. Milne wrote:
>> On Mon, 2016-12-12 at 10:20 +0800, Wei Fang wrote:
>>> A race between scanning and fc_remote_port_delete() may result in a
>>> permanent stop if the device gets blocked before
>>> scsi_sysfs_add_sdev()
>>> and unblocked after.  The reason is that blocking a device sets
>>> both
>>> the SDEV_BLOCKED state and the QUEUE_FLAG_STOPPED.  However,
>>> scsi_sysfs_add_sdev() unconditionally sets SDEV_RUNNING which
>>> causes
>>> the device to be ignored by scsi_target_unblock() and thus never
>>> have
>>> its QUEUE_FLAG_STOPPED cleared leading to a device which is
>>> apparently
>>> running but has a stopped queue.
>>>
>>> We actually have two places where SDEV_RUNNING is set: once in
>>> scsi_add_lun() which respects the blocked flag and once in
>>> scsi_sysfs_add_sdev() which doesn't.  Since the second set is
>>> entirely
>>> spurious, simply remove it to fix the problem.
>>>
>>> Reported-by: Zengxi Chen <chenzen...@huawei.com>
>>> Signed-off-by: Wei Fang <fangw...@huawei.com>
>>> ---
>>> Changes v1->v2:
>>> - don't modify scsi_internal_device_unblock(), just remove changing
>>>   state to SDEV_RUNNING in scsi_sysfs_add_sdev(), suggested by
>>>   James Bottomley and Ewan D. Milne.
>>> Changes v2->v3
>>> - Use a clearer description of this problem
>>>
>>>  drivers/scsi/scsi_sysfs.c  | 4 
>>>  include/scsi/scsi_device.h | 2 +-
>>>  2 files changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>>> index 0734927..82dfe07 100644
>>> --- a/drivers/scsi/scsi_sysfs.c
>>> +++ b/drivers/scsi/scsi_sysfs.c
>>> @@ -1204,10 +1204,6 @@ int scsi_sysfs_add_sdev(struct scsi_device
>>> *sdev)
>>> struct request_queue *rq = sdev->request_queue;
>>> struct scsi_target *starget = sdev->sdev_target;
>>>  
>>> -   error = scsi_device_set_state(sdev, SDEV_RUNNING);
>>> -   if (error)
>>> -   return error;
>>> -
>>> error = scsi_target_add(starget);
>>> if (error)
>>> return error;
>>> diff --git a/include/scsi/scsi_device.h
>>> b/include/scsi/scsi_device.h
>>> index 8990e58..8bfb37f 100644
>>> --- a/include/scsi/scsi_device.h
>>> +++ b/include/scsi/scsi_device.h
>>> @@ -31,7 +31,7 @@ struct scsi_mode_data {
>>>  enum scsi_device_state {
>>> SDEV_CREATED = 1,   /* device created but not added
>>> to sysfs
>>>  * Only internal commands allowed
>>> (for inq) */
>>> -   SDEV_RUNNING,   /* device properly configured
>>> +   SDEV_RUNNING,   /* device properly configured
>>> and not blocked
>>>  * All commands allowed */
>>> SDEV_CANCEL,/* beginning to delete device
>>>  * Only error handler commands
>>> allowed */
>>
>> Well, James said not to bother with the comment, but OK.
> 
> The comment change still adds no value: the states are exclusive so
> every state that's not SDEV_BLOCKED or SDEV_CREATED_BLOCKED means that
> state and not blocked; that makes the proposed addition a tautology.
> 
> The reason I don't want the change to the comment is because there's
> nothing to fix in the comments, so that hunk shouldn't be part of a
> backport to stable.  The way we work in linux is to backport whole
> upstream commits, because it makes the stable process so much easier. 
>  If you really want to change the comment, it should be a separate
> patch ... but it better add value otherwise it won't get applied.

Sorry, I mistook what you means. This hunk will be removed.

>> I take it this has passed your testing.
> 
> Yes, I'd like this confirmation, too, please.

This patch have been tested on Zengxi Chen's machine over 48 hours,
and everything goes well. Without this patch, this problem will be
encountered in half an hour.

Thanks,
Wei

> James
> 
> 
>>   I have not heard back yet from the site that reported this problem 
>> to me on their reproducer. The change looks good to me.
>>
>> Reviewed-by: Ewan D. Milne <emi...@redhat.com>
>>
>>
>> --
>> 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
>>
> 
> --
> 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
> 
> .
> 

--
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

[PATCH v3] scsi: avoid a permanent stop of the scsi device's request queue

[Wei Fang]

A race between scanning and fc_remote_port_delete() may result in a
permanent stop if the device gets blocked before scsi_sysfs_add_sdev()
and unblocked after.  The reason is that blocking a device sets both
the SDEV_BLOCKED state and the QUEUE_FLAG_STOPPED.  However,
scsi_sysfs_add_sdev() unconditionally sets SDEV_RUNNING which causes
the device to be ignored by scsi_target_unblock() and thus never have
its QUEUE_FLAG_STOPPED cleared leading to a device which is apparently
running but has a stopped queue.

We actually have two places where SDEV_RUNNING is set: once in
scsi_add_lun() which respects the blocked flag and once in
scsi_sysfs_add_sdev() which doesn't.  Since the second set is entirely
spurious, simply remove it to fix the problem.

Reported-by: Zengxi Chen <chenzen...@huawei.com>
Signed-off-by: Wei Fang <fangw...@huawei.com>
---
Changes v1->v2:
- don't modify scsi_internal_device_unblock(), just remove changing
  state to SDEV_RUNNING in scsi_sysfs_add_sdev(), suggested by
  James Bottomley and Ewan D. Milne.
Changes v2->v3
- Use a clearer description of this problem

 drivers/scsi/scsi_sysfs.c  | 4 
 include/scsi/scsi_device.h | 2 +-
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 0734927..82dfe07 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1204,10 +1204,6 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
struct request_queue *rq = sdev->request_queue;
struct scsi_target *starget = sdev->sdev_target;
 
-   error = scsi_device_set_state(sdev, SDEV_RUNNING);
-   if (error)
-   return error;
-
error = scsi_target_add(starget);
if (error)
return error;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 8990e58..8bfb37f 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -31,7 +31,7 @@ struct scsi_mode_data {
 enum scsi_device_state {
SDEV_CREATED = 1,   /* device created but not added to sysfs
 * Only internal commands allowed (for inq) */
-   SDEV_RUNNING,   /* device properly configured
+   SDEV_RUNNING,   /* device properly configured and not blocked
 * All commands allowed */
SDEV_CANCEL,/* beginning to delete device
 * Only error handler commands allowed */
-- 
2.4.11

--
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

Re: [PATCH v2] scsi: avoid a permanent stop of the scsi device's request queue

[Wei Fang]

Hi, James,

On 2016/12/10 0:02, James Bottomley wrote:
> On Fri, 2016-12-09 at 17:35 +0800, Wei Fang wrote:
>> A scan work can run simultaneously with fc_remote_port_delete().
>> If a scsi device is added to the ->__devices list in the scan work,
>> it can be touched and will be blocked in scsi_target_block(), which
>> will be called in fc_remote_port_delete(), and QUEUE_FLAG_STOPPED
>> flag will be setted to the scsi device's request queue.
>>  
>> The scsi device is being setted to the SDEV_RUNNING state in
>> scsi_sysfs_add_sdev() at the end of the scan work. When the remote
>> port reappears, scsi_target_unblock() will be called, but the
>> QUEUE_FLAG_STOPPED flag will not be cleared, since
>> scsi_internal_device_unblock() ignores SCSI devices in SDEV_RUNNING
>> state. It results in a permanent stop of the scsi device's request
>> queue. Every requests sended to it will be blocked.
> 
> This is a bit unclear as a description of the problem
> 
>> Since the scsi device shouldn't be unblocked in this case, fix
>> it by removing scsi_device_set_state() in scsi_sysfs_add_sdev().
> 
> So is this as a description of the solution, because the reader doesn't
> know there's a prior place where SDEV_RUNNING was previously set.
> 
> How about
> 
> ---
> A race between scanning and fc_remote_port_delete() may result in a
> permanent stop if the device gets blocked before scsi_sysfs_add_lun()
> and unblocked after.  The reason is that blocking a device sets both
> the SDEV_BLOCKED state and the QUEUE_FLAG_STOPPED.  However,
> scsi_sysfs_add_sdev() unconditionally sets SDEV_RUNNING which causes
> the device to be ignored by scsi_target_unblock() and thus never have
> its QUEUE_FLAG_STOPPED cleared leading to a device which is apparently
> running but has a stopped queue.
> 
> We actually have two places where SDEV_RUNNING is set: once in
> scsi_add_lun() which respects the blocked flag and once in
> scsi_sysfs_add_sdev() which doesn't.  Since the second set is entirely
> spurious, simply remove it to fix the problem.

That's much clearer, thanks. I'll use this in patch v3.

Thanks,
Wei

--
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

[PATCH v2] scsi: avoid a permanent stop of the scsi device's request queue

[Wei Fang]

A scan work can run simultaneously with fc_remote_port_delete().
If a scsi device is added to the ->__devices list in the scan work,
it can be touched and will be blocked in scsi_target_block(), which
will be called in fc_remote_port_delete(), and QUEUE_FLAG_STOPPED
flag will be setted to the scsi device's request queue.

The scsi device is being setted to the SDEV_RUNNING state in
scsi_sysfs_add_sdev() at the end of the scan work. When the remote
port reappears, scsi_target_unblock() will be called, but the
QUEUE_FLAG_STOPPED flag will not be cleared, since
scsi_internal_device_unblock() ignores SCSI devices in SDEV_RUNNING
state. It results in a permanent stop of the scsi device's request
queue. Every requests sended to it will be blocked.

Since the scsi device shouldn't be unblocked in this case, fix
it by removing scsi_device_set_state() in scsi_sysfs_add_sdev().

Reported-and-tested-by: Zengxi Chen <chenzen...@huawei.com>
Signed-off-by: Wei Fang <fangw...@huawei.com>
---
Changes v1->v2:
- don't modify scsi_internal_device_unblock(), just remove changing
  state to SDEV_RUNNING in scsi_sysfs_add_sdev(), suggested by
  James Bottomley and Ewan D. Milne.

 drivers/scsi/scsi_sysfs.c  | 4 
 include/scsi/scsi_device.h | 2 +-
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 0734927..82dfe07 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1204,10 +1204,6 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
struct request_queue *rq = sdev->request_queue;
struct scsi_target *starget = sdev->sdev_target;
 
-   error = scsi_device_set_state(sdev, SDEV_RUNNING);
-   if (error)
-   return error;
-
error = scsi_target_add(starget);
if (error)
return error;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 8990e58..5c53cf5 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -31,7 +31,7 @@ struct scsi_mode_data {
 enum scsi_device_state {
SDEV_CREATED = 1,   /* device created but not added to sysfs
 * Only internal commands allowed (for inq) */
-   SDEV_RUNNING,   /* device properly configured
+   SDEV_RUNNING,   /* device properly initialized
 * All commands allowed */
SDEV_CANCEL,/* beginning to delete device
 * Only error handler commands allowed */
-- 
2.4.11

--
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

Re: [PATCH] scsi: avoid a permanent stop of the scsi device's request queue

[Wei Fang]

On 2016/12/8 23:39, James Bottomley wrote:
> On Thu, 2016-12-08 at 11:22 +0800, Wei Fang wrote:
>> Hi, James, Ewan,
>>
>> On 2016/12/8 10:33, James Bottomley wrote:
>>> On Thu, 2016-12-08 at 10:28 +0800, Wei Fang wrote:
>>>> Hi, James, Ewan,
>>>>
>>>> On 2016/12/8 7:43, James Bottomley wrote:
>>>>> On Wed, 2016-12-07 at 15:30 -0500, Ewan D. Milne wrote:
>>>>>> On Wed, 2016-12-07 at 12:09 -0800, James Bottomley wrote:
>>>>>>> Hm, it looks like the state set in scsi_sysfs_add_sdev() is
>>>>>>> bogus. 
>>>>>>>  We expect the state to have been properly set before that
>>>>>>> (in
>>>>>>> scsi_add_lun), so can we not simply remove it?
>>>>>>>
>>>>>>> James
>>>>>>>
>>>>>>
>>>>>> I was considering that, but...
>>>>>>
>>>>>> enum scsi_device_state {
>>>>>> SDEV_CREATED = 1,   /* device created but not
>>>>>> added
>>>>>> to
>>>>>> sysfs
>>>>>> 
>>>>>> 
>>>>>>  
>>>>>> 
>>>>>> 
>>>>>>   
>>>>>>  * Only internal commands
>>>>>> allowed
>>>>>> (for inq) */
>>>>>>
>>>>>> So it seems the intent was for the state to not change until
>>>>>> then.
>>>>>
>>>>> I think this is historical.  There was a change somewhere that
>>>>> moved
>>>>> the sysfs state handling out of the sdev stat to is_visible, so
>>>>> the
>>>>> sdev state no-longer reflects  it.
>>>>>
>>>>>> The call to set the SDEV_RUNNING state earlier in
>>>>>> scsi_add_lun()
>>>>>> was added with:
>>>>>>
>>>>>> commit 6f4267e3bd1211b3d09130e626b0b3d885077610
>>>>>> Author: James Bottomley <
>>>>>> james.bottom...@hansenpartnership.com>
>>>>>> Date:   Fri Aug 22 16:53:31 2008 -0500
>>>>>>
>>>>>> [SCSI] Update the SCSI state model to allow blocking in
>>>>>> the
>>>>>> created state
>>>>>>
>>>>>> Which allows the device to go into ->BLOCK (which is needed,
>>>>>> since it
>>>>>> actually happens).
>>>>>>
>>>>>> Should we remove the call from scsi_sysfs_add_sdev() and
>>>>>> change
>>>>>> the
>>>>>> comment in scsi_device.h to reflect the intent?
>>>>
>>>> This sounds reasonable.
>>>>
>>>>> Assuming someone with the problem actually tests it, yes.
>>>>
>>>> This problem can be stably reproduced on Zengxi Chen's machine,
>>>> who
>>>> reported the bug. We can test it on this machine.
>>>>
>>>> The patch is as below, just for sure:
>>>>
>>>> diff --git a/drivers/scsi/scsi_sysfs.c
>>>> b/drivers/scsi/scsi_sysfs.c
>>>> index 0734927..82dfe07 100644
>>>> --- a/drivers/scsi/scsi_sysfs.c
>>>> +++ b/drivers/scsi/scsi_sysfs.c
>>>> @@ -1204,10 +1204,6 @@ int scsi_sysfs_add_sdev(struct scsi_device
>>>> *sdev)
>>>> struct request_queue *rq = sdev->request_queue;
>>>> struct scsi_target *starget = sdev->sdev_target;
>>>>
>>>> -   error = scsi_device_set_state(sdev, SDEV_RUNNING);
>>>> -   if (error)
>>>> -   return error;
>>>> -
>>
>> I looked through those code and found that if we fix this bug
>> by removing setting the state in scsi_sysfs_add_sdev(), it
>> can't be fixed completely:
>>
>> scsi_device_set_state(sdev, SDEV_RUNNING) in scsi_add_lun() and
>> scsi_device_set_state(sdev, SDEV_CREATED_BLOCK) in
>> scsi_internal_device_block()
>> can be called simultaneously. Because there is no synchronization
>> between scsi_device_set_state(), those calls may both return
>> success, and the state may be SDEV_RUNNING after that, and the
>> device queue is stopped.
> 
> As Bart said, we've had this problem for a while, but the theoretical
> issue never really shows.  Unless it's suddenly exposed for you, lets
> go with the simple fix (if you confirm it works) and defer the far more
> complex issue of concurrent state changes.

OK, I'll test it and send patch v2.

Thanks,
Wei

>>> That's it, although not the second hunk: CREATED still means device
>>> not
>>> added to sysfs.  It's just that RUNNING now doesn't mean it is.
>>>
>>> James


--
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

Re: [PATCH] scsi: avoid a permanent stop of the scsi device's request queue

[Wei Fang]

Hi, James, Ewan, Bart,

On 2016/12/8 11:22, Wei Fang wrote:
> I looked through those code and found that if we fix this bug
> by removing setting the state in scsi_sysfs_add_sdev(), it
> can't be fixed completely:
> 
> scsi_device_set_state(sdev, SDEV_RUNNING) in scsi_add_lun() and
> scsi_device_set_state(sdev, SDEV_CREATED_BLOCK) in 
> scsi_internal_device_block()
> can be called simultaneously. Because there is no synchronization
> between scsi_device_set_state(), those calls may both return
> success, and the state may be SDEV_RUNNING after that, and the
> device queue is stopped.

Can we fix it in this way:

Add a state lock to make sure the result of simultaneously calling
of scsi_device_set_state() is not unpredictable.

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 253ee74..80cb493 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2457,10 +2457,16 @@ EXPORT_SYMBOL(scsi_test_unit_ready);
 int
 scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
 {
-   enum scsi_device_state oldstate = sdev->sdev_state;
+   enum scsi_device_state oldstate;
+   unsigned long flags;
+
+   spin_lock_irqsave(>state_lock, flags);
+   oldstate = sdev->sdev_state;

-   if (state == oldstate)
+   if (state == oldstate) {
+   spin_unlock_irqrestore(>state_lock, flags);
return 0;
+   }

switch (state) {
case SDEV_CREATED:
@@ -2558,9 +2564,11 @@ scsi_device_set_state(struct scsi_device *sdev, enum 
scsi_device_state state)

}
sdev->sdev_state = state;
+   spin_unlock_irqrestore(>state_lock, flags);
return 0;

  illegal:
+   spin_unlock_irqrestore(>state_lock, flags);
SCSI_LOG_ERROR_RECOVERY(1,
sdev_printk(KERN_ERR, sdev,
"Illegal state transition %s->%s",
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 6f7128f..ba2f38f 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -238,6 +238,7 @@ static struct scsi_device *scsi_alloc_sdev(struct 
scsi_target *starget,
INIT_LIST_HEAD(>starved_entry);
INIT_LIST_HEAD(>event_list);
spin_lock_init(>list_lock);
+   spin_lock_init(>state_lock);
mutex_init(>inquiry_mutex);
INIT_WORK(>event_work, scsi_evt_thread);
INIT_WORK(>requeue_work, scsi_requeue_run_queue);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 0734927..82dfe07 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1204,10 +1204,6 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
struct request_queue *rq = sdev->request_queue;
struct scsi_target *starget = sdev->sdev_target;

-   error = scsi_device_set_state(sdev, SDEV_RUNNING);
-   if (error)
-   return error;
-
error = scsi_target_add(starget);
if (error)
return error;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 8990e58..e00764e 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -31,7 +31,7 @@ struct scsi_mode_data {
 enum scsi_device_state {
SDEV_CREATED = 1,   /* device created but not added to sysfs
 * Only internal commands allowed (for inq) */
-   SDEV_RUNNING,   /* device properly configured
+   SDEV_RUNNING,   /* device properly initialized
 * All commands allowed */
SDEV_CANCEL,/* beginning to delete device
 * Only error handler commands allowed */
@@ -207,6 +207,7 @@ struct scsi_device {
void*handler_data;

unsigned char   access_state;
+   spinlock_t  state_lock;
enum scsi_device_state sdev_state;
unsigned long   sdev_data[0];
 } __attribute__((aligned(sizeof(unsigned long;

Haven't tested yet. Sending this for your opinion.

Thanks,
Wei

--
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

Re: [PATCH] scsi: avoid a permanent stop of the scsi device's request queue

[Wei Fang]

Hi, James, Ewan,

On 2016/12/8 10:33, James Bottomley wrote:
> On Thu, 2016-12-08 at 10:28 +0800, Wei Fang wrote:
>> Hi, James, Ewan,
>>
>> On 2016/12/8 7:43, James Bottomley wrote:
>>> On Wed, 2016-12-07 at 15:30 -0500, Ewan D. Milne wrote:
>>>> On Wed, 2016-12-07 at 12:09 -0800, James Bottomley wrote:
>>>>> Hm, it looks like the state set in scsi_sysfs_add_sdev() is
>>>>> bogus. 
>>>>>  We expect the state to have been properly set before that (in
>>>>> scsi_add_lun), so can we not simply remove it?
>>>>>
>>>>> James
>>>>>
>>>>
>>>> I was considering that, but...
>>>>
>>>> enum scsi_device_state {
>>>> SDEV_CREATED = 1,   /* device created but not added
>>>> to
>>>> sysfs
>>>> 
>>>>  
>>>> 
>>>>   
>>>>  * Only internal commands allowed
>>>> (for inq) */
>>>>
>>>> So it seems the intent was for the state to not change until
>>>> then.
>>>
>>> I think this is historical.  There was a change somewhere that
>>> moved
>>> the sysfs state handling out of the sdev stat to is_visible, so the
>>> sdev state no-longer reflects  it.
>>>
>>>> The call to set the SDEV_RUNNING state earlier in scsi_add_lun()
>>>> was added with:
>>>>
>>>> commit 6f4267e3bd1211b3d09130e626b0b3d885077610
>>>> Author: James Bottomley <james.bottom...@hansenpartnership.com>
>>>> Date:   Fri Aug 22 16:53:31 2008 -0500
>>>>
>>>> [SCSI] Update the SCSI state model to allow blocking in the
>>>> created state
>>>>
>>>> Which allows the device to go into ->BLOCK (which is needed,
>>>> since it
>>>> actually happens).
>>>>
>>>> Should we remove the call from scsi_sysfs_add_sdev() and change
>>>> the
>>>> comment in scsi_device.h to reflect the intent?
>>
>> This sounds reasonable.
>>
>>> Assuming someone with the problem actually tests it, yes.
>>
>> This problem can be stably reproduced on Zengxi Chen's machine, who
>> reported the bug. We can test it on this machine.
>>
>> The patch is as below, just for sure:
>>
>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>> index 0734927..82dfe07 100644
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -1204,10 +1204,6 @@ int scsi_sysfs_add_sdev(struct scsi_device
>> *sdev)
>> struct request_queue *rq = sdev->request_queue;
>> struct scsi_target *starget = sdev->sdev_target;
>>
>> -   error = scsi_device_set_state(sdev, SDEV_RUNNING);
>> -   if (error)
>> -   return error;
>> -

I looked through those code and found that if we fix this bug
by removing setting the state in scsi_sysfs_add_sdev(), it
can't be fixed completely:

scsi_device_set_state(sdev, SDEV_RUNNING) in scsi_add_lun() and
scsi_device_set_state(sdev, SDEV_CREATED_BLOCK) in scsi_internal_device_block()
can be called simultaneously. Because there is no synchronization
between scsi_device_set_state(), those calls may both return
success, and the state may be SDEV_RUNNING after that, and the
device queue is stopped.

Thanks,
Wei

> That's it, although not the second hunk: CREATED still means device not
> added to sysfs.  It's just that RUNNING now doesn't mean it is.
> 
> James
> 
> 
> 
> .
> 

--
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

Re: [PATCH] scsi: avoid a permanent stop of the scsi device's request queue

[Wei Fang]

Hi, James, Ewan,

On 2016/12/8 7:43, James Bottomley wrote:
> On Wed, 2016-12-07 at 15:30 -0500, Ewan D. Milne wrote:
>> On Wed, 2016-12-07 at 12:09 -0800, James Bottomley wrote:
>>> Hm, it looks like the state set in scsi_sysfs_add_sdev() is bogus. 
>>>  We expect the state to have been properly set before that (in
>>> scsi_add_lun), so can we not simply remove it?
>>>
>>> James
>>>
>>
>> I was considering that, but...
>>
>> enum scsi_device_state {
>> SDEV_CREATED = 1,   /* device created but not added to
>> sysfs
>>  
>>   
>>  * Only internal commands allowed
>> (for inq) */
>>
>> So it seems the intent was for the state to not change until then.
> 
> I think this is historical.  There was a change somewhere that moved
> the sysfs state handling out of the sdev stat to is_visible, so the
> sdev state no-longer reflects  it.
> 
>> The call to set the SDEV_RUNNING state earlier in scsi_add_lun()
>> was added with:
>>
>> commit 6f4267e3bd1211b3d09130e626b0b3d885077610
>> Author: James Bottomley 
>> Date:   Fri Aug 22 16:53:31 2008 -0500
>>
>> [SCSI] Update the SCSI state model to allow blocking in the
>> created state
>>
>> Which allows the device to go into ->BLOCK (which is needed, since it
>> actually happens).
>>
>> Should we remove the call from scsi_sysfs_add_sdev() and change the
>> comment in scsi_device.h to reflect the intent?

This sounds reasonable.

> Assuming someone with the problem actually tests it, yes.

This problem can be stably reproduced on Zengxi Chen's machine, who
reported the bug. We can test it on this machine.

The patch is as below, just for sure:

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 0734927..82dfe07 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1204,10 +1204,6 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
struct request_queue *rq = sdev->request_queue;
struct scsi_target *starget = sdev->sdev_target;

-   error = scsi_device_set_state(sdev, SDEV_RUNNING);
-   if (error)
-   return error;
-
error = scsi_target_add(starget);
if (error)
return error;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 8990e58..08636ea 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -29,7 +29,7 @@ struct scsi_mode_data {
  * scsi_lib:scsi_device_set_state().
  */
 enum scsi_device_state {
-   SDEV_CREATED = 1,   /* device created but not added to sysfs
+   SDEV_CREATED = 1,   /* device created
 * Only internal commands allowed (for inq) */
SDEV_RUNNING,   /* device properly configured
 * All commands allowed */

Thanks,
Wei

>> I have not verified the async vs. non-async scan path yet but it 
>> looks like it would be OK.
> 
> I did.  The async device addition occurs after scsi_add_lun(), so it
> rules the state change in both cases.
> 
> James
> 
> 
>> -Ewan
>>
>>
>> --
>> 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
>>
> 
> 
> 

--
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

Re: [PATCH] scsi: avoid a permanent stop of the scsi device's request queue

[Wei Fang]

Hi, Bart,

On 2016/12/7 12:40, Bart Van Assche wrote:
> I am aware that commit 5c10e63c943b caused the behavior change. But that 
> doesn't mean that a fix has to undo the changes introduced by that 
> commit. We do not only want to make sure that the SCSI core works as 
> intended but also that the SCSI core code is as easy to comprehend as 
> reasonably possible. Adding "&& sdev->sdev_state != SDEV_RUNNING" in 
> scsi_internal_device_unblock() would require a long comment to explain 
> why that code has been added. I think modifying scsi_sysfs_add_sdev() 
> such that it does not unblock devices will result in code that is easier 
> to understand.

Agree that we should make the code easier to comprehend if possible :)

If we modify scsi_sysfs_add_sdev() as below:
...
if (scsi_device_created(sdev))
error = scsi_device_set_state(sdev, SDEV_RUNNING);
if (error)
error = scsi_device_set_state(sdev, SDEV_BLOCK);
...
there's a chance that the state will be changed to SDEV_RUNNING.

If a SCSI device is blocked after the check of the device's creating
and before being changed to SDEV_RUNNING state, the state will still
become SDEV_RUNNING. If we fix this problem in this way, we need
introduce a way to synchronize those code.

Actually I don't know quite well about the synchronization of
scsi_device_set_state(). There are so many cases it can be called
simultaneously, will the state become a unpredictable value, or this
is tolerated?

Thanks,
Wei

--
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

Re: [PATCH] scsi: avoid a permanent stop of the scsi device's request queue

[Wei Fang]

Hi, Bart,

On 2016/12/7 10:45, Bart Van Assche wrote:
> On 12/06/16 17:21, Wei Fang wrote:
>> The state of the scsi device first is changed to SDEV_BLOCK in
>> scsi_add_lun() as you mentioned, then it will be changed to SDEV_RUNNING
>> in scsi_sysfs_add_sdev().
> 
> Hello Wei,
> 
> The purpose of the scsi_device_set_state(sdev, SDEV_RUNNING) call in 
> scsi_sysfs_add_sdev() is to change the device state from SDEV_CREATED 
> into SDEV_RUNNING. Have you tried to modify scsi_sysfs_add_sdev() such 
> that it only changes the device state into SDEV_RUNNING if the current 
> state is SDEV_CREATED and also such that it changes SDEV_CREATED_BLOCK 
> into SDEV_BLOCK?

Does those code in scsi_add_lun() have done this thing?
...
ret = scsi_device_set_state(sdev, SDEV_RUNNING);
if (ret) {
ret = scsi_device_set_state(sdev, SDEV_BLOCK);
...
}
...
You mean we shouldn't change the device state from SDEV_BLOCK
into SDEV_RUNNING in scsi_sysfs_add_sdev()?

I thought it doesn't matter that the state is changed from SDEV_BLOCK
to SDEV_RUNNING in scsi_sysfs_add_sdev(), if the queue can be unblocked
in scsi_internal_device_unblock() in SDEV_RUNNING state. But it
was broken since commit 5c10e63c943b.

Thanks,
Wei

--
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

Re: [PATCH] scsi: avoid a permanent stop of the scsi device's request queue

[Wei Fang]

Hi, Bart,

On 2016/12/6 23:51, Bart Van Assche wrote:
> On 12/06/16 01:12, Wei Fang wrote:
>> The scsi device is being setted to the SDEV_RUNNING state at the end of
>> the scan work. When the remote port reappears, scsi_target_unblock()
>> will be called, but the QUEUE_FLAG_STOPPED flag will not be cleared,
>> since scsi_internal_device_unblock() ignores SCSI devices in SDEV_RUNNING
>> state. It results in a permanent stop of the scsi device's request
>> queue. Every requests sended to it will be blocked.
> 
> Hello Wei,
> 
> scsi_device_set_state() does not allow the transition from 
> SDEV_CREATED_BLOCK to SDEV_RUNNING. If a SCSI device is blocked after it 
> has been added to the __devices list and before scsi_add_lun() finishes 
> then I think the scan code will change its state into SDEV_BLOCK. Are 
> you sure what you described above is what happened?

Yes, we already encountered this case on out machine:

The state of the scsi device first is changed to SDEV_BLOCK in
scsi_add_lun() as you metioned, then it will be changed to SDEV_RUNNING
in scsi_sysfs_add_sdev().

Thanks,
Wei

--
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

[PATCH] scsi: avoid a permanent stop of the scsi device's request queue

[Wei Fang]

A scan work can run simultaneously with fc_remote_port_delete().
If a scsi device is added to the ->__devices list in the scan work,
it can be touched and will be blocked in scsi_target_block(), which
will be called in fc_remote_port_delete(), and QUEUE_FLAG_STOPPED
flag will be setted to the scsi device's request queue.

The scsi device is being setted to the SDEV_RUNNING state at the end of
the scan work. When the remote port reappears, scsi_target_unblock()
will be called, but the QUEUE_FLAG_STOPPED flag will not be cleared,
since scsi_internal_device_unblock() ignores SCSI devices in SDEV_RUNNING
state. It results in a permanent stop of the scsi device's request
queue. Every requests sended to it will be blocked.

It looks like it's a regression caused by:
commit 5c10e63c943b
("[SCSI] limit state transitions in scsi_internal_device_unblock")

Fix this by restarting the device queue if the state is SDEV_RUNNING.

Reported-by: Zengxi Chen <chenzen...@huawei.com>
Signed-off-by: Wei Fang <fangw...@huawei.com>
---
 drivers/scsi/scsi_lib.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9ca1f17..253ee74 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2942,7 +2942,8 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
else
sdev->sdev_state = SDEV_CREATED;
} else if (sdev->sdev_state != SDEV_CANCEL &&
-sdev->sdev_state != SDEV_OFFLINE)
+sdev->sdev_state != SDEV_OFFLINE &&
+sdev->sdev_state != SDEV_RUNNING)
return -EINVAL;
 
if (q->mq_ops) {
-- 
2.4.11

--
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

[RFC PATCH] scsi:fc: avoid a permanent stop of the scsi device's request queue

[Wei Fang]

A scan work can run simultaneously with fc_remote_port_delete().
If a scsi device is added to the ->__devices list in the scan work,
it can be touched and will be blocked in scsi_target_block(), and
QUEUE_FLAG_STOPPED will be setted to the scsi device's request queue.
But unfortunately, this flag will not be cleared when the scsi device
is being setted to the SDEV_RUNNING state in the end of the scan work.

After that, all the requests sending to the device will be blocked.

Fix this by flushing the scan work before blocking the device.

Reported-by: Zengxi Chen <chenzen...@huawei.com>
Signed-off-by: Wei Fang <fangw...@huawei.com>
---
 drivers/scsi/scsi_transport_fc.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index e05c07f..0d476df 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -2978,14 +2978,6 @@ fc_remote_port_delete(struct fc_rport  *rport)
unsigned long timeout = rport->dev_loss_tmo;
unsigned long flags;
 
-   /*
-* No need to flush the fc_host work_q's, as all adds are synchronous.
-*
-* We do need to reclaim the rport scan work element, so eventually
-* (in fc_rport_final_delete()) we'll flush the scsi host work_q if
-* there's still a scan pending.
-*/
-
spin_lock_irqsave(shost->host_lock, flags);
 
if (rport->port_state != FC_PORTSTATE_ONLINE) {
@@ -3012,6 +3004,14 @@ fc_remote_port_delete(struct fc_rport  *rport)
 
spin_unlock_irqrestore(shost->host_lock, flags);
 
+   /*
+* make sure no scan is pending before blocking it, otherwise
+* simultaneously scan may cause a permanent QUEUE_FLAG_STOPPED
+* flag set of the device's request queue.
+*/
+   if (rport->flags & FC_RPORT_SCAN_PENDING)
+   scsi_flush_work(shost);
+
scsi_target_block(>dev);
 
/* see if we need to kill io faster than waiting for device loss */
-- 
2.4.11

--
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

[RESEND PATCH] scsi: fix ata_port_wait_eh() hang caused by missing to wake up eh thread

[Wei Fang]

->host_eh_schedule and ->shost_state may be accessed simultaneously as
below:

1.In ata port probe path:
ata_std_sched_eh()
scsi_schedule_eh()
...
spin_lock_irqsave(shost->host_lock, flags);
if (scsi_host_set_state(shost, SHOST_RECOVERY) == 0 ||
scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY) == 0) {
shost->host_eh_scheduled++;
scsi_eh_wakeup();
}
spin_unlock_irqrestore(shost->host_lock, flags);
...

2.In IO complete path:
scsi_device_unbusy()
...
atomic_dec(>host_busy)
if (unlikely(scsi_host_in_recovery(shost) &&
(shost->host_failed || shost->host_eh_scheduled))) {
spin_lock_irqsave(shost->host_lock, flags);
scsi_eh_wakeup(shost);
spin_unlock_irqrestore(shost->host_lock, flags);
}
...

In the IO complete path, ->host_eh_schedule and ->shost_state are
accessed without ->host_lock, and obsoleted data may be got after
scsi_eh_wakeup() has been called in scsi_schedule_eh().

It's the case that causes the problem:
* One IO hasn't finished when scsi_schedule_eh() has been called
  in ata port probe path
* scsi_eh_wakeup() has been called in scsi_schedule_eh() without
  waking up eh thread because of ->host_busy != ->host_failed
* this IO completes and scsi_device_unbusy() is called
* obsoleted ->host_eh_schedule and ->shost_state are got, and eh
  thread can't be woken up in this path too

Eh thread won't be woken up ever in this case, and a hung task
will happen:

INFO: task kworker/u64:0:6 blocked for more than 120 seconds.
...
Call trace:
[] __switch_to+0x78/0x90
[] __schedule+0x244/0x79c
[] schedule+0x40/0x98
[] ata_port_wait_eh+0x8c/0x110
[] sas_ata_wait_eh+0x3c/0x44
[] sas_probe_sata_device+0x74/0xf8
[] sas_add_device+0xac/0x1ac
[] process_one_work+0x164/0x428
[] worker_thread+0x144/0x4a8
[] kthread+0xfc/0x110

After that, the host is in recovery state, and any IOs to this
host will be blocked.

Fix this by accessing ->host_eh_schedule and ->shost_state in
->host_lock. We have tested the IOPS throughput by fio and found
no performance degradation.

Signed-off-by: Wei Fang <fangw...@huawei.com>
---
 drivers/scsi/scsi_lib.c | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 2cca9cf..5a72e1d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -272,23 +272,27 @@ static void scsi_init_cmd_errh(struct scsi_cmnd *cmd)
cmd->cmd_len = scsi_command_size(cmd->cmnd);
 }
 
+static void __scsi_eh_wakeup(struct Scsi_Host *shost)
+{
+   unsigned long flags;
+
+   spin_lock_irqsave(shost->host_lock, flags);
+   if (unlikely(scsi_host_in_recovery(shost) &&
+(shost->host_failed || shost->host_eh_scheduled)))
+   scsi_eh_wakeup(shost);
+   spin_unlock_irqrestore(shost->host_lock, flags);
+}
+
 void scsi_device_unbusy(struct scsi_device *sdev)
 {
struct Scsi_Host *shost = sdev->host;
struct scsi_target *starget = scsi_target(sdev);
-   unsigned long flags;
 
atomic_dec(>host_busy);
if (starget->can_queue > 0)
atomic_dec(>target_busy);
 
-   if (unlikely(scsi_host_in_recovery(shost) &&
-(shost->host_failed || shost->host_eh_scheduled))) {
-   spin_lock_irqsave(shost->host_lock, flags);
-   scsi_eh_wakeup(shost);
-   spin_unlock_irqrestore(shost->host_lock, flags);
-   }
-
+   __scsi_eh_wakeup(shost);
atomic_dec(>device_busy);
 }
 
@@ -1457,6 +1461,7 @@ static inline int scsi_host_queue_ready(struct 
request_queue *q,
spin_unlock_irq(shost->host_lock);
 out_dec:
atomic_dec(>host_busy);
+   __scsi_eh_wakeup(shost);
return 0;
 }
 
@@ -1931,6 +1936,7 @@ static int scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 
 out_dec_host_busy:
atomic_dec(>host_busy);
+   __scsi_eh_wakeup(shost);
 out_dec_target_busy:
if (scsi_target(sdev)->can_queue > 0)
atomic_dec(_target(sdev)->target_busy);
-- 
1.9.3

--
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

[PATCH] scsi: fix ata_port_wait_eh() hang caused by missing to wake up eh thread

[Wei Fang]

->host_eh_schedule and ->shost_state may be accessed simultaneously as
bellow:

1.In ata port probe path:
ata_std_sched_eh()
scsi_schedule_eh()
...
spin_lock_irqsave(shost->host_lock, flags);
if (scsi_host_set_state(shost, SHOST_RECOVERY) == 0 ||
scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY) == 0) {
shost->host_eh_scheduled++
scsi_eh_wakeup()
}
spin_unlock_irqrestore(shost->host_lock, flags);
...

2.In IO complete path:
scsi_device_unbusy()
...
atomic_dec(>host_busy)
if (unlikely(scsi_host_in_recovery(shost) &&
(shost->host_failed || shost->host_eh_scheduled))) {
spin_lock_irqsave(shost->host_lock, flags);
scsi_eh_wakeup(shost);
spin_unlock_irqrestore(shost->host_lock, flags);
}
...

In the IO complete path, ->host_eh_schedule and ->shost_state are
accessed without ->host_lock, and obsoleted data may be got after
scsi_eh_wakeup() has been called in scsi_schedule_eh().

It's the case that causes the problem:
* One IO hasn't finished when scsi_schedule_eh() has been called
  in ata port probe path
* scsi_eh_wakeup() has been called in scsi_schedule_eh() without
  waking up eh thread
* this IO completes and scsi_device_unbusy() is called
* obsoleted ->host_eh_schedule and ->shost_state are got, and eh
  thread can't been woken up in this path

Eh thread won't been woken up ever in this case, and a hung task
will happen:

INFO: task kworker/u64:0:6 blocked for more than 120 seconds.
...
Call trace:
[] __switch_to+0x78/0x90
[] __schedule+0x244/0x79c
[] schedule+0x40/0x98
[] ata_port_wait_eh+0x8c/0x110
[] sas_ata_wait_eh+0x3c/0x44
[] sas_probe_sata_device+0x74/0xf8
[] sas_add_device+0xac/0x1ac
[] process_one_work+0x164/0x428
[] worker_thread+0x144/0x4a8
[] kthread+0xfc/0x110

After that, the host is in recovery state, and any IOs to this
host will be blocked.

Fix this by accessing ->host_eh_schedule and ->shost_state in
->host_lock.

Signed-off-by: Wei Fang <fangw...@huawei.com>
---
 drivers/scsi/scsi_lib.c | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 2cca9cf..5a72e1d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -272,23 +272,27 @@ static void scsi_init_cmd_errh(struct scsi_cmnd *cmd)
cmd->cmd_len = scsi_command_size(cmd->cmnd);
 }
 
+static void __scsi_eh_wakeup(struct Scsi_Host *shost)
+{
+   unsigned long flags;
+
+   spin_lock_irqsave(shost->host_lock, flags);
+   if (unlikely(scsi_host_in_recovery(shost) &&
+(shost->host_failed || shost->host_eh_scheduled)))
+   scsi_eh_wakeup(shost);
+   spin_unlock_irqrestore(shost->host_lock, flags);
+}
+
 void scsi_device_unbusy(struct scsi_device *sdev)
 {
struct Scsi_Host *shost = sdev->host;
struct scsi_target *starget = scsi_target(sdev);
-   unsigned long flags;
 
atomic_dec(>host_busy);
if (starget->can_queue > 0)
atomic_dec(>target_busy);
 
-   if (unlikely(scsi_host_in_recovery(shost) &&
-(shost->host_failed || shost->host_eh_scheduled))) {
-   spin_lock_irqsave(shost->host_lock, flags);
-   scsi_eh_wakeup(shost);
-   spin_unlock_irqrestore(shost->host_lock, flags);
-   }
-
+   __scsi_eh_wakeup(shost);
atomic_dec(>device_busy);
 }
 
@@ -1457,6 +1461,7 @@ static inline int scsi_host_queue_ready(struct 
request_queue *q,
spin_unlock_irq(shost->host_lock);
 out_dec:
atomic_dec(>host_busy);
+   __scsi_eh_wakeup(shost);
return 0;
 }
 
@@ -1931,6 +1936,7 @@ static int scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 
 out_dec_host_busy:
atomic_dec(>host_busy);
+   __scsi_eh_wakeup(shost);
 out_dec_target_busy:
if (scsi_target(sdev)->can_queue > 0)
atomic_dec(_target(sdev)->target_busy);
-- 
1.9.3

--
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

[PATCH v2] scsi:libsas: fix oops caused by assigning a freed task to ->lldd_task

[Wei Fang]

A freed task has been assigned to ->lldd_task when lldd_execute_task()
failed in sas_ata_qc_issue(), and access of ->lldd_task will cause
an oops:

Call trace:
[] sas_ata_post_internal+0x6c/0x150
[] ata_exec_internal_sg+0x32c/0x588
[] ata_exec_internal+0x88/0xe8
[] ata_dev_read_id+0x204/0x5e0
[] ata_dev_reread_id+0x60/0xc8
[] ata_dev_revalidate+0x88/0x1e0
[] ata_eh_recover+0xcf8/0x13a8
[] ata_do_eh+0x5c/0xe0
[] ata_std_error_handler+0x48/0x98
[] ata_scsi_port_error_handler+0x474/0x658
[] async_sas_ata_eh+0x50/0x80
[] async_run_entry_fn+0x64/0x180
[] process_one_work+0x164/0x438
[] worker_thread+0x144/0x4b0
[] kthread+0xfc/0x110

Fix this by reassigning NULL to ->lldd_task in error path.

Signed-off-by: Wei Fang <fangw...@huawei.com>
---
 drivers/scsi/libsas/sas_ata.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 935c430..596a545 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -253,6 +253,7 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd 
*qc)
if (qc->scsicmd)
ASSIGN_SAS_TASK(qc->scsicmd, NULL);
sas_free_task(task);
+   qc->lldd_task = NULL;
ret = AC_ERR_SYSTEM;
}
 
-- 
1.7.1

--
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

Re: [PATCH] scsi:libsas: fix oops caused by assigning a freed task to ->lldd_task

[Wei Fang]

Hi, Hannes,

> This is most definitely wrong.
> Sure you mean
> 
> qc->lldd_task = NULL;
> 
> in that line?

My mistake. Thanks for pointing me out. Will resend soon.
Please ignore this patch.

Thanks,
Wei

--
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

[PATCH] scsi:libsas: fix oops caused by assigning a freed task to ->lldd_task

[Wei Fang]

A freed task has been assigned to ->lldd_task when lldd_execute_task()
failed in sas_ata_qc_issue(), and access of ->lldd_task will cause
an oops:

Call trace:
[] sas_ata_post_internal+0x6c/0x150
[] ata_exec_internal_sg+0x32c/0x588
[] ata_exec_internal+0x88/0xe8
[] ata_dev_read_id+0x204/0x5e0
[] ata_dev_reread_id+0x60/0xc8
[] ata_dev_revalidate+0x88/0x1e0
[] ata_eh_recover+0xcf8/0x13a8
[] ata_do_eh+0x5c/0xe0
[] ata_std_error_handler+0x48/0x98
[] ata_scsi_port_error_handler+0x474/0x658
[] async_sas_ata_eh+0x50/0x80
[] async_run_entry_fn+0x64/0x180
[] process_one_work+0x164/0x438
[] worker_thread+0x144/0x4b0
[] kthread+0xfc/0x110

Fix this by reassigning NULL to ->lldd_task in error path.

Signed-off-by: Wei Fang <fangw...@huawei.com>
---
 drivers/scsi/libsas/sas_ata.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 935c430..33c7c66 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -253,6 +253,7 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd 
*qc)
if (qc->scsicmd)
ASSIGN_SAS_TASK(qc->scsicmd, NULL);
sas_free_task(task);
+   qc->lldd_task = task;
ret = AC_ERR_SYSTEM;
}
 
-- 
1.7.1

--
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

[PATCH v4 2/2] Documentation/scsi: update scsi_eh.txt about ->host_failed

[Wei Fang]

Update the new concurrency rules of ->host_failed.

Signed-off-by: Wei Fang <fangw...@huawei.com>
---
 Documentation/scsi/scsi_eh.txt |8 ++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/scsi/scsi_eh.txt b/Documentation/scsi/scsi_eh.txt
index 8638f61..37eca00 100644
--- a/Documentation/scsi/scsi_eh.txt
+++ b/Documentation/scsi/scsi_eh.txt
@@ -263,19 +263,23 @@ scmd->allowed.
 
  3. scmd recovered
 ACTION: scsi_eh_finish_cmd() is invoked to EH-finish scmd
-   - shost->host_failed--
- clear scmd->eh_eflags
- scsi_setup_cmd_retry()
- move from local eh_work_q to local eh_done_q
 LOCKING: none
+CONCURRENCY: at most one thread per separate eh_work_q to
+keep queue manipulation lockless
 
  4. EH completes
 ACTION: scsi_eh_flush_done_q() retries scmds or notifies upper
-   layer of failure.
+   layer of failure. May be called concurrently but must have
+   a no more than one thread per separate eh_work_q to
+   manipulate the queue locklessly
- scmd is removed from eh_done_q and scmd->eh_entry is cleared
- if retry is necessary, scmd is requeued using
   scsi_queue_insert()
- otherwise, scsi_finish_command() is invoked for scmd
+   - zero shost->host_failed
 LOCKING: queue or finish function performs appropriate locking
 
 
-- 
1.7.1

--
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

[PATCH v4 1/2] scsi: fix race between simultaneous decrements of ->host_failed

[Wei Fang]

sas_ata_strategy_handler() adds the works of the ata error handler
to system_unbound_wq. This workqueue asynchronously runs work items,
so the ata error handler will be performed concurrently on different
CPUs. In this case, ->host_failed will be decreased simultaneously in
scsi_eh_finish_cmd() on different CPUs, and become abnormal.

It will lead to permanently inequal between ->host_failed and
 ->host_busy, and scsi error handler thread won't become running.
IO errors after that won't be handled forever.

Since all scmds must have been handled in the strategy handle, just
remove the decrement in scsi_eh_finish_cmd() and zero ->host_busy
after the strategy handle to fix this race.

This fixes the problem introduced in
commit 50824d6c5657 ("[SCSI] libsas: async ata-eh").

Reviewed-by: James Bottomley <j...@linux.vnet.ibm.com>
Signed-off-by: Wei Fang <fangw...@huawei.com>
---
Changes v1->v2:
- update Documentation/scsi/scsi_eh.txt about ->host_failed
Changes v2->v3:
- don't use atomic type, just zero ->host_failed after the strategy handle
Changes v3->v4:
- add new concurrency rules of ->host_failed in scsi_eh.txt

 drivers/ata/libata-eh.c   |2 +-
 drivers/scsi/scsi_error.c |4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 961acc7..91a9e6a 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -606,7 +606,7 @@ void ata_scsi_error(struct Scsi_Host *host)
ata_scsi_port_error_handler(host, ap);
 
/* finish or retry handled scmd's and clean up */
-   WARN_ON(host->host_failed || !list_empty(_work_q));
+   WARN_ON(!list_empty(_work_q));
 
DPRINTK("EXIT\n");
 }
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 984ddcb..1b9c049 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1127,7 +1127,6 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
  */
 void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
 {
-   scmd->device->host->host_failed--;
scmd->eh_eflags = 0;
list_move_tail(>eh_entry, done_q);
 }
@@ -2226,6 +2225,9 @@ int scsi_error_handler(void *data)
else
scsi_unjam_host(shost);
 
+   /* All scmds have been handled */
+   shost->host_failed = 0;
+
/*
 * Note - if the above fails completely, the action is to take
 * individual devices offline and flush the queue of any
-- 
1.7.1

--
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

Re: [PATCH v3 2/2] Documentation/scsi: update scsi_eh.txt about ->host_failed

[Wei Fang]

On 2016/6/7 11:22, Martin K. Petersen wrote:
>>>>>> "Wei" == Wei Fang <fangw...@huawei.com> writes:
> 
> Wei,
> 
> Wei> Update the new rules of ->host_failed.
> 
> Are you going to incorporate the amendment from James?
> 

Yes, I'll send the patch soon.

Thanks,
Wei

--
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

[PATCH v3 2/2] Documentation/scsi: update scsi_eh.txt about ->host_failed

[Wei Fang]

Update the new rules of ->host_failed.

Signed-off-by: Wei Fang <fangw...@huawei.com>
---
 Documentation/scsi/scsi_eh.txt |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Documentation/scsi/scsi_eh.txt b/Documentation/scsi/scsi_eh.txt
index 8638f61..9702c78 100644
--- a/Documentation/scsi/scsi_eh.txt
+++ b/Documentation/scsi/scsi_eh.txt
@@ -263,7 +263,6 @@ scmd->allowed.
 
  3. scmd recovered
 ACTION: scsi_eh_finish_cmd() is invoked to EH-finish scmd
-   - shost->host_failed--
- clear scmd->eh_eflags
- scsi_setup_cmd_retry()
- move from local eh_work_q to local eh_done_q
@@ -276,6 +275,7 @@ scmd->allowed.
- if retry is necessary, scmd is requeued using
   scsi_queue_insert()
- otherwise, scsi_finish_command() is invoked for scmd
+   - zero shost->host_failed
 LOCKING: queue or finish function performs appropriate locking
 
 
-- 
1.7.1

--
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

[PATCH v3 1/2] scsi: fix race between simultaneous decrements of ->host_failed

[Wei Fang]

sas_ata_strategy_handler() adds the works of the ata error handler
to system_unbound_wq. This workqueue asynchronously runs work items,
so the ata error handler will be performed concurrently on different
CPUs. In this case, ->host_failed will be decreased simultaneously in
scsi_eh_finish_cmd() on different CPUs, and become abnormal.

It will lead to permanently inequal between ->host_failed and
 ->host_busy, and scsi error handler thread won't become running.
IO errors after that won't be handled forever.

Since all scmds must have been handled in the strategy handle, just
remove the decrement in scsi_eh_finish_cmd() and zero ->host_busy
after the strategy handle to fix this race.

This fixes the problem introduced in
commit 50824d6c5657 ("[SCSI] libsas: async ata-eh").

Signed-off-by: Wei Fang <fangw...@huawei.com>
---
Changes v1->v2:
- update Documentation/scsi/scsi_eh.txt about ->host_failed
Changes v2->v3:
- don't use atomic type, just zero ->host_failed after the strategy handle

 drivers/ata/libata-eh.c   |2 +-
 drivers/scsi/scsi_error.c |4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 961acc7..91a9e6a 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -606,7 +606,7 @@ void ata_scsi_error(struct Scsi_Host *host)
ata_scsi_port_error_handler(host, ap);
 
/* finish or retry handled scmd's and clean up */
-   WARN_ON(host->host_failed || !list_empty(_work_q));
+   WARN_ON(!list_empty(_work_q));
 
DPRINTK("EXIT\n");
 }
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 984ddcb..1b9c049 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1127,7 +1127,6 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
  */
 void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
 {
-   scmd->device->host->host_failed--;
scmd->eh_eflags = 0;
list_move_tail(>eh_entry, done_q);
 }
@@ -2226,6 +2225,9 @@ int scsi_error_handler(void *data)
else
scsi_unjam_host(shost);
 
+   /* All scmds have been handled */
+   shost->host_failed = 0;
+
/*
 * Note - if the above fails completely, the action is to take
 * individual devices offline and flush the queue of any
-- 
1.7.1

--
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

Re: [PATCH 1/2] scsi: fix race between simultaneous decrements of ->host_failed

[Wei Fang]

Forgot to add changes, please ignore it.

On 2016/6/2 15:54, Wei Fang wrote:
> sas_ata_strategy_handler() adds the works of the ata error handler
> to system_unbound_wq. This workqueue asynchronously runs work items,
> so the ata error handler will be performed concurrently on different
> CPUs. In this case, ->host_failed will be decreased simultaneously in
> scsi_eh_finish_cmd() on different CPUs, and become abnormal.
> 
> It will lead to permanently inequal between ->host_failed and
>  ->host_busy, and scsi error handler thread won't become running.
> IO errors after that won't be handled forever.
> 
> Since all scmds must have been handled in the strategy handle, just
> remove the decrement in scsi_eh_finish_cmd() and zero ->host_busy
> after the strategy handle to fix this race.
> 
> This fixes the problem introduced in
> commit 50824d6c5657 ("[SCSI] libsas: async ata-eh").
> 
> Signed-off-by: Wei Fang <fangw...@huawei.com>
> ---
>  drivers/ata/libata-eh.c   |2 +-
>  drivers/scsi/scsi_error.c |4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 961acc7..91a9e6a 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -606,7 +606,7 @@ void ata_scsi_error(struct Scsi_Host *host)
>   ata_scsi_port_error_handler(host, ap);
>  
>   /* finish or retry handled scmd's and clean up */
> - WARN_ON(host->host_failed || !list_empty(_work_q));
> + WARN_ON(!list_empty(_work_q));
>  
>   DPRINTK("EXIT\n");
>  }
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 984ddcb..1b9c049 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1127,7 +1127,6 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int 
> rtn)
>   */
>  void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
>  {
> - scmd->device->host->host_failed--;
>   scmd->eh_eflags = 0;
>   list_move_tail(>eh_entry, done_q);
>  }
> @@ -2226,6 +2225,9 @@ int scsi_error_handler(void *data)
>   else
>   scsi_unjam_host(shost);
>  
> + /* All scmds have been handled */
> + shost->host_failed = 0;
> +
>   /*
>* Note - if the above fails completely, the action is to take
>* individual devices offline and flush the queue of any
> 

--
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

[PATCH 1/2] scsi: fix race between simultaneous decrements of ->host_failed

[Wei Fang]

sas_ata_strategy_handler() adds the works of the ata error handler
to system_unbound_wq. This workqueue asynchronously runs work items,
so the ata error handler will be performed concurrently on different
CPUs. In this case, ->host_failed will be decreased simultaneously in
scsi_eh_finish_cmd() on different CPUs, and become abnormal.

It will lead to permanently inequal between ->host_failed and
 ->host_busy, and scsi error handler thread won't become running.
IO errors after that won't be handled forever.

Since all scmds must have been handled in the strategy handle, just
remove the decrement in scsi_eh_finish_cmd() and zero ->host_busy
after the strategy handle to fix this race.

This fixes the problem introduced in
commit 50824d6c5657 ("[SCSI] libsas: async ata-eh").

Signed-off-by: Wei Fang <fangw...@huawei.com>
---
 drivers/ata/libata-eh.c   |2 +-
 drivers/scsi/scsi_error.c |4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 961acc7..91a9e6a 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -606,7 +606,7 @@ void ata_scsi_error(struct Scsi_Host *host)
ata_scsi_port_error_handler(host, ap);
 
/* finish or retry handled scmd's and clean up */
-   WARN_ON(host->host_failed || !list_empty(_work_q));
+   WARN_ON(!list_empty(_work_q));
 
DPRINTK("EXIT\n");
 }
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 984ddcb..1b9c049 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1127,7 +1127,6 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
  */
 void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
 {
-   scmd->device->host->host_failed--;
scmd->eh_eflags = 0;
list_move_tail(>eh_entry, done_q);
 }
@@ -2226,6 +2225,9 @@ int scsi_error_handler(void *data)
else
scsi_unjam_host(shost);
 
+   /* All scmds have been handled */
+   shost->host_failed = 0;
+
/*
 * Note - if the above fails completely, the action is to take
 * individual devices offline and flush the queue of any
-- 
1.7.1

--
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

[PATCH 2/2] Documentation/scsi: update scsi_eh.txt about ->host_failed

[Wei Fang]

Update the new rules of ->host_failed.

Signed-off-by: Wei Fang <fangw...@huawei.com>
---
 Documentation/scsi/scsi_eh.txt |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Documentation/scsi/scsi_eh.txt b/Documentation/scsi/scsi_eh.txt
index 8638f61..9702c78 100644
--- a/Documentation/scsi/scsi_eh.txt
+++ b/Documentation/scsi/scsi_eh.txt
@@ -263,7 +263,6 @@ scmd->allowed.
 
  3. scmd recovered
 ACTION: scsi_eh_finish_cmd() is invoked to EH-finish scmd
-   - shost->host_failed--
- clear scmd->eh_eflags
- scsi_setup_cmd_retry()
- move from local eh_work_q to local eh_done_q
@@ -276,6 +275,7 @@ scmd->allowed.
- if retry is necessary, scmd is requeued using
   scsi_queue_insert()
- otherwise, scsi_finish_command() is invoked for scmd
+   - zero shost->host_failed
 LOCKING: queue or finish function performs appropriate locking
 
 
-- 
1.7.1

--
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

Re: [PATCH v2 1/2] scsi: fix race between simultaneous decrements of ->host_failed

[Wei Fang]

On 2016/6/2 10:37, Wei Fang wrote:
> Hi, Kevin,
> 
> On 2016/6/1 22:36, Kevin Groeneveld wrote:
>>> Subject: [PATCH v2 1/2] scsi: fix race between simultaneous decrements of 
>>> ->host_failed
>>
>> I wonder if this could be related to 
>> http://www.spinics.net/lists/linux-scsi/msg86808.html?
>>
>> I never did get to the bottom of that.  If I have time I hope to retest my 
>> scsi hang issue with this patch.
>>
> 
> The concurrently decrements of host_failed only lead to abnormal
> of host_failed, host_busy will be zero after error handler, and
> the result may be host_failed > host_busy forever. But in your
> case, host_busy > host_failed, so I think it's not the same
> case. I'm afraid that this patch can't fix your scsi hang issue.

Something wrong in my words. host_busy may not be zero after error
handler, but the result is true that missing decrement of host_failed
may lead to host_failed > host_busy.

Thanks,
Wei

--
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

Re: [PATCH v2 1/2] scsi: fix race between simultaneous decrements of ->host_failed

[Wei Fang]

Hi, Kevin,

On 2016/6/1 22:36, Kevin Groeneveld wrote:
>> Subject: [PATCH v2 1/2] scsi: fix race between simultaneous decrements of 
>> ->host_failed
> 
> I wonder if this could be related to 
> http://www.spinics.net/lists/linux-scsi/msg86808.html?
> 
> I never did get to the bottom of that.  If I have time I hope to retest my 
> scsi hang issue with this patch.
> 

The concurrently decrements of host_failed only lead to abnormal
of host_failed, host_busy will be zero after error handler, and
the result may be host_failed > host_busy forever. But in your
case, host_busy > host_failed, so I think it's not the same
case. I'm afraid that this patch can't fix your scsi hang issue.

Thanks,
Wei

--
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

Re: [PATCH v2 1/2] scsi: fix race between simultaneous decrements of ->host_failed

[Wei Fang]

Hi, James,

On 2016/6/1 22:06, James Bottomley wrote:
> On Tue, 2016-05-31 at 16:38 +0800, Wei Fang wrote:
>> sas_ata_strategy_handler() adds the works of the ata error handler
>> to system_unbound_wq. This workqueue asynchronously runs work items,
>> so the ata error handler will be performed concurrently on different
>> CPUs. In this case, ->host_failed will be decreased simultaneously in
>> scsi_eh_finish_cmd() on different CPUs, and become abnormal.
>>
>> It will lead to permanently inequal between ->host_failed and
>>  ->host_busy, and scsi error handler thread won't become running.
>> IO errors after that won't be handled forever.
>>
>> Use atomic type for ->host_failed to fix this race.
> 
> As I said previously, you don't need atomics to do this, could you just
> remove the decrement in scsi_eh_finish_command() and zero the counter
> after the strategy handler completes.
> 

OK, I'll send v3 later.

Thanks,
Wei

--
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

Re: [PATCH v2 1/2] scsi: fix race between simultaneous decrements of ->host_failed

[Wei Fang]

Hi, Tejun,

On 2016/5/31 22:33, Tejun Heo wrote:
> On Tue, May 31, 2016 at 04:38:17PM +0800, Wei Fang wrote:
>> sas_ata_strategy_handler() adds the works of the ata error handler
>> to system_unbound_wq. This workqueue asynchronously runs work items,
> 
> Are there more than one error handling work items per host?

The ata error handler here means async_sas_ata_eh(), every port will
execute it's own async_sas_ata_eh() in sas_ata_strategy_handler().

Thanks,
Wei

> 
> Thanks.
> 

--
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

[PATCH v2 1/2] scsi: fix race between simultaneous decrements of ->host_failed

[Wei Fang]

sas_ata_strategy_handler() adds the works of the ata error handler
to system_unbound_wq. This workqueue asynchronously runs work items,
so the ata error handler will be performed concurrently on different
CPUs. In this case, ->host_failed will be decreased simultaneously in
scsi_eh_finish_cmd() on different CPUs, and become abnormal.

It will lead to permanently inequal between ->host_failed and
 ->host_busy, and scsi error handler thread won't become running.
IO errors after that won't be handled forever.

Use atomic type for ->host_failed to fix this race.

This fixes the problem introduced in
commit 50824d6c5657 ("[SCSI] libsas: async ata-eh").

Reviewed-by: Christoph Hellwig <h...@infradead.org>
Signed-off-by: Wei Fang <fangw...@huawei.com>
---
 drivers/ata/libata-eh.c |2 +-
 drivers/scsi/libsas/sas_scsi_host.c |5 +++--
 drivers/scsi/scsi.c |2 +-
 drivers/scsi/scsi_error.c   |   15 +--
 drivers/scsi/scsi_lib.c |3 ++-
 include/scsi/scsi_host.h|2 +-
 6 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 961acc7..a0e7612 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -606,7 +606,7 @@ void ata_scsi_error(struct Scsi_Host *host)
ata_scsi_port_error_handler(host, ap);
 
/* finish or retry handled scmd's and clean up */
-   WARN_ON(host->host_failed || !list_empty(_work_q));
+   WARN_ON(atomic_read(>host_failed) || !list_empty(_work_q));
 
DPRINTK("EXIT\n");
 }
diff --git a/drivers/scsi/libsas/sas_scsi_host.c 
b/drivers/scsi/libsas/sas_scsi_host.c
index 519dac4..8d74003 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -757,7 +757,8 @@ retry:
spin_unlock_irq(shost->host_lock);
 
SAS_DPRINTK("Enter %s busy: %d failed: %d\n",
-   __func__, atomic_read(>host_busy), 
shost->host_failed);
+   __func__, atomic_read(>host_busy),
+   atomic_read(>host_failed));
/*
 * Deal with commands that still have SAS tasks (i.e. they didn't
 * complete via the normal sas_task completion mechanism),
@@ -800,7 +801,7 @@ out:
 
SAS_DPRINTK("--- Exit %s: busy: %d failed: %d tries: %d\n",
__func__, atomic_read(>host_busy),
-   shost->host_failed, tries);
+   atomic_read(>host_failed), tries);
 }
 
 enum blk_eh_timer_return sas_scsi_timed_out(struct scsi_cmnd *cmd)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 1deb6ad..7840915 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -527,7 +527,7 @@ void scsi_log_completion(struct scsi_cmnd *cmd, int 
disposition)
scmd_printk(KERN_INFO, cmd,
"scsi host busy %d failed %d\n",

atomic_read(>device->host->host_busy),
-   cmd->device->host->host_failed);
+   
atomic_read(>device->host->host_failed));
}
}
 }
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 984ddcb..f37666f 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -62,7 +62,8 @@ static int scsi_try_to_abort_cmd(struct scsi_host_template *,
 /* called with shost->host_lock held */
 void scsi_eh_wakeup(struct Scsi_Host *shost)
 {
-   if (atomic_read(>host_busy) == shost->host_failed) {
+   if (atomic_read(>host_busy) ==
+   atomic_read(>host_failed)) {
trace_scsi_eh_wakeup(shost);
wake_up_process(shost->ehandler);
SCSI_LOG_ERROR_RECOVERY(5, shost_printk(KERN_INFO, shost,
@@ -250,7 +251,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
eh_flag &= ~SCSI_EH_CANCEL_CMD;
scmd->eh_eflags |= eh_flag;
list_add_tail(>eh_entry, >eh_cmd_q);
-   shost->host_failed++;
+   atomic_inc(>host_failed);
scsi_eh_wakeup(shost);
  out_unlock:
spin_unlock_irqrestore(shost->host_lock, flags);
@@ -1127,7 +1128,7 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
  */
 void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
 {
-   scmd->device->host->host_failed--;
+   atomic_dec(>device->host->host_failed);
scmd->eh_eflags = 0;
list_move_tail(>eh_entry, done_q);
 }
@@ -2190,8 +2191,10 @@ int scsi_error_handler(void *data)
if (kthread_should_stop())
break;
 
-   if ((shost->host_failed == 0 && shost->host_eh_scheduled == 0) 
||
- 

[PATCH v2 2/2] Documentation/scsi: update scsi_eh.txt about ->host_failed

[Wei Fang]

Update the new concurrency rules of ->host_failed.

Signed-off-by: Wei Fang <fangw...@huawei.com>
---
 Documentation/scsi/scsi_eh.txt |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/scsi/scsi_eh.txt b/Documentation/scsi/scsi_eh.txt
index 8638f61..e6d0de2 100644
--- a/Documentation/scsi/scsi_eh.txt
+++ b/Documentation/scsi/scsi_eh.txt
@@ -252,7 +252,7 @@ scmd->allowed.
- set scmd->eh_eflags
- add scmd to shost->eh_cmd_q
- set SHOST_RECOVERY
-   - shost->host_failed++
+   - atomic_inc(>host_failed)
 LOCKING: shost->host_lock
 
  2. EH starts
@@ -263,7 +263,7 @@ scmd->allowed.
 
  3. scmd recovered
 ACTION: scsi_eh_finish_cmd() is invoked to EH-finish scmd
-   - shost->host_failed--
+   - atomic_dec(>host_failed)
- clear scmd->eh_eflags
- scsi_setup_cmd_retry()
- move from local eh_work_q to local eh_done_q
-- 
1.7.1

--
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

Re: [PATCH] scsi: fix race between simultaneous decrements of ->host_failed

[Wei Fang]

Hi, Christoph,

On 2016/5/29 14:54, Christoph Hellwig wrote:
> On Sat, May 28, 2016 at 11:51:11AM +0800, Wei Fang wrote:
>> async_sas_ata_eh(), which will call scsi_eh_finish_cmd() in some case,
>> would be performed simultaneously in sas_ata_strategy_handler(). In this
>> case, ->host_failed may be decreased simultaneously in
>> scsi_eh_finish_cmd() on different CPUs, and become abnormal.
>>
>> It will lead to permanently inequal between ->host_failed and
>>  ->host_busy. Then SCSI error handler thread won't become running,
>> SCSI errors after that won't be handled forever.
>>
>> Use atomic type for ->host_failed to fix this race.
> 
> Looks fine,
> 
> Reviewed-by: Christoph Hellwig <h...@lst.de>
> 
> But please also update Documentation/scsi/scsi_eh.txt for this
> change.

Thanks for reviewing the patch.
I looked around the file, and didn't find the part should be updated.
Would you point me out?

Thanks,
Wei

> 
> .
> 

--
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

Re: [PATCH] scsi: fix race between simultaneous decrements of ->host_failed

[Wei Fang]

Hi James, Christoph,

On 2016/5/29 23:41, James Bottomley wrote:
> On Sat, 2016-05-28 at 23:54 -0700, Christoph Hellwig wrote:
>> On Sat, May 28, 2016 at 11:51:11AM +0800, Wei Fang wrote:
>>> async_sas_ata_eh(), which will call scsi_eh_finish_cmd() in some 
>>> case, would be performed simultaneously in 
>>> sas_ata_strategy_handler(). In this case, ->host_failed may be 
>>> decreased simultaneously in scsi_eh_finish_cmd() on different CPUs,
>>> and become abnormal.
>>>
>>> It will lead to permanently inequal between ->host_failed and
>>>  ->host_busy. Then SCSI error handler thread won't become running,
>>> SCSI errors after that won't be handled forever.
>>>
>>> Use atomic type for ->host_failed to fix this race.
>>
>> Looks fine,
> 
> Actually, it doesn't look fine at all.  The same mechanism that's
> supposed to protect the host_failed decrement is also supposed to
> protect the list_move_tail().  If there's a problem with the former
> then we're also in danger of corrupting the list.

Scmd is moved to local eh_done_q list here, and I checked that the
list won't be touched concurrently.

> Can we go back to the theory of what the problem is, since it's not
> spelled out very clearly in the change log.  Our usual reason for not
> requiring locking in eh routines is that the eh is single threaded on
> the eh thread per host, so any host manipulations can't have
> concurrency problems.  In this case, the sas_ata routines are trying to
> be clever and use asynchronous workqueues for the port error handler
> and you theorise that these can execute concurrently on two CPUs, thus
> causing the problem?

Yes, it's the case. The works of the port error handler are added to
system_unbound_wq, and will be performed concurrently on different CPUs.
We have already met that problem on our machine.

Thanks,
Wei

> James
> 
> 
> 
> .
> 

--
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

[PATCH] scsi: fix race between simultaneous decrements of ->host_failed

[Wei Fang]

async_sas_ata_eh(), which will call scsi_eh_finish_cmd() in some case,
would be performed simultaneously in sas_ata_strategy_handler(). In this
case, ->host_failed may be decreased simultaneously in
scsi_eh_finish_cmd() on different CPUs, and become abnormal.

It will lead to permanently inequal between ->host_failed and
 ->host_busy. Then SCSI error handler thread won't become running,
SCSI errors after that won't be handled forever.

Use atomic type for ->host_failed to fix this race.

Signed-off-by: Wei Fang <fangw...@huawei.com>
---
 drivers/ata/libata-eh.c |2 +-
 drivers/scsi/libsas/sas_scsi_host.c |5 +++--
 drivers/scsi/scsi.c |2 +-
 drivers/scsi/scsi_error.c   |   15 +--
 drivers/scsi/scsi_lib.c |3 ++-
 include/scsi/scsi_host.h|2 +-
 6 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 961acc7..a0e7612 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -606,7 +606,7 @@ void ata_scsi_error(struct Scsi_Host *host)
ata_scsi_port_error_handler(host, ap);
 
/* finish or retry handled scmd's and clean up */
-   WARN_ON(host->host_failed || !list_empty(_work_q));
+   WARN_ON(atomic_read(>host_failed) || !list_empty(_work_q));
 
DPRINTK("EXIT\n");
 }
diff --git a/drivers/scsi/libsas/sas_scsi_host.c 
b/drivers/scsi/libsas/sas_scsi_host.c
index 519dac4..8d74003 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -757,7 +757,8 @@ retry:
spin_unlock_irq(shost->host_lock);
 
SAS_DPRINTK("Enter %s busy: %d failed: %d\n",
-   __func__, atomic_read(>host_busy), 
shost->host_failed);
+   __func__, atomic_read(>host_busy),
+   atomic_read(>host_failed));
/*
 * Deal with commands that still have SAS tasks (i.e. they didn't
 * complete via the normal sas_task completion mechanism),
@@ -800,7 +801,7 @@ out:
 
SAS_DPRINTK("--- Exit %s: busy: %d failed: %d tries: %d\n",
__func__, atomic_read(>host_busy),
-   shost->host_failed, tries);
+   atomic_read(>host_failed), tries);
 }
 
 enum blk_eh_timer_return sas_scsi_timed_out(struct scsi_cmnd *cmd)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 1deb6ad..7840915 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -527,7 +527,7 @@ void scsi_log_completion(struct scsi_cmnd *cmd, int 
disposition)
scmd_printk(KERN_INFO, cmd,
"scsi host busy %d failed %d\n",

atomic_read(>device->host->host_busy),
-   cmd->device->host->host_failed);
+   
atomic_read(>device->host->host_failed));
}
}
 }
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 984ddcb..f37666f 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -62,7 +62,8 @@ static int scsi_try_to_abort_cmd(struct scsi_host_template *,
 /* called with shost->host_lock held */
 void scsi_eh_wakeup(struct Scsi_Host *shost)
 {
-   if (atomic_read(>host_busy) == shost->host_failed) {
+   if (atomic_read(>host_busy) ==
+   atomic_read(>host_failed)) {
trace_scsi_eh_wakeup(shost);
wake_up_process(shost->ehandler);
SCSI_LOG_ERROR_RECOVERY(5, shost_printk(KERN_INFO, shost,
@@ -250,7 +251,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
eh_flag &= ~SCSI_EH_CANCEL_CMD;
scmd->eh_eflags |= eh_flag;
list_add_tail(>eh_entry, >eh_cmd_q);
-   shost->host_failed++;
+   atomic_inc(>host_failed);
scsi_eh_wakeup(shost);
  out_unlock:
spin_unlock_irqrestore(shost->host_lock, flags);
@@ -1127,7 +1128,7 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
  */
 void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
 {
-   scmd->device->host->host_failed--;
+   atomic_dec(>device->host->host_failed);
scmd->eh_eflags = 0;
list_move_tail(>eh_entry, done_q);
 }
@@ -2190,8 +2191,10 @@ int scsi_error_handler(void *data)
if (kthread_should_stop())
break;
 
-   if ((shost->host_failed == 0 && shost->host_eh_scheduled == 0) 
||
-   shost->host_failed != atomic_read(>host_busy)) {
+   if ((atomic_read(>host_failed) == 0 &&
+shost->host_eh_scheduled == 0) |

Re: [PATCH v3 08/32] scsi: hisi_sas: add hisi_sas_remove

[Wei Fang]

Hi John,

On 2015/11/10 0:32, John Garry wrote:
> This patch also includes relevant memory/pool
> free'ing and sas/scsi host removal
> 
> Signed-off-by: John Garry 
> ---
>  drivers/scsi/hisi_sas/hisi_sas_main.c | 71 
> ++-
>  1 file changed, 70 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c 
> b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 97f5368..7f81000 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -126,6 +126,59 @@ err_out:
>   return -ENOMEM;
>  }
>  
> +static void hisi_sas_free(struct hisi_hba *hisi_hba)
> +{
> + struct device *dev = _hba->pdev->dev;
> + int i, s;
> +
> + for (i = 0; i < hisi_hba->queue_count; i++) {
> + s = sizeof(struct hisi_sas_cmd_hdr) * HISI_SAS_QUEUE_SLOTS;
> + if (hisi_hba->cmd_hdr[i])
> + dma_free_coherent(dev, s,
> +   hisi_hba->cmd_hdr[i],
> +   hisi_hba->cmd_hdr_dma[i]);
> +
> + s = hisi_hba->hw->complete_hdr_size * HISI_SAS_QUEUE_SLOTS;
> + if (hisi_hba->complete_hdr[i])
> + dma_free_coherent(dev, s,
> +   hisi_hba->complete_hdr[i],
> +   hisi_hba->complete_hdr_dma[i]);
> + }
> +
> + dma_pool_destroy(hisi_hba->status_buffer_pool);
> + dma_pool_destroy(hisi_hba->command_table_pool);
> + dma_pool_destroy(hisi_hba->sge_page_pool);
> +
> + s = HISI_SAS_MAX_ITCT_ENTRIES * sizeof(struct hisi_sas_itct);
> + if (hisi_hba->itct)
> + dma_free_coherent(dev, s,
> +   hisi_hba->itct, hisi_hba->itct_dma);
> +
> + s = HISI_SAS_COMMAND_ENTRIES * sizeof(struct hisi_sas_iost);
> + if (hisi_hba->iost)
> + dma_free_coherent(dev, s,
> +   hisi_hba->iost, hisi_hba->iost_dma);
> +
> + s = HISI_SAS_COMMAND_ENTRIES * sizeof(struct hisi_sas_breakpoint);
> + if (hisi_hba->breakpoint)
> + dma_free_coherent(dev, s,
> +   hisi_hba->breakpoint,
> +   hisi_hba->breakpoint_dma);
> +
> +
> + s = sizeof(struct hisi_sas_initial_fis) * HISI_SAS_MAX_PHYS;
> + if (hisi_hba->initial_fis)
> + dma_free_coherent(dev, s,
> +   hisi_hba->initial_fis,
> +   hisi_hba->initial_fis_dma);
> +
> + s = HISI_SAS_COMMAND_ENTRIES * sizeof(struct hisi_sas_breakpoint) * 2;
> + if (hisi_hba->sata_breakpoint)
> + dma_free_coherent(dev, s,
> +   hisi_hba->sata_breakpoint,
> +   hisi_hba->sata_breakpoint_dma);
> +
> +}
>  
>  static struct Scsi_Host *hisi_sas_shost_alloc(struct platform_device *pdev,
> const struct hisi_sas_hw *hw)
> @@ -188,8 +241,10 @@ static struct Scsi_Host *hisi_sas_shost_alloc(struct 
> platform_device *pdev,
>   if (IS_ERR(hisi_hba->ctrl))
>   goto err_out;
>  
> - if (hisi_sas_alloc(hisi_hba, shost))
> + if (hisi_sas_alloc(hisi_hba, shost)) {
> + hisi_sas_free(hisi_hba);
>   goto err_out;
> + }
>  
>   return shost;
>  err_out:
> @@ -270,6 +325,20 @@ err_out_ha:
>  }
>  EXPORT_SYMBOL_GPL(hisi_sas_probe);
>  
> +int hisi_sas_remove(struct platform_device *pdev)
> +{
> + struct sas_ha_struct *sha = platform_get_drvdata(pdev);
> + struct hisi_hba *hisi_hba = sha->lldd_ha;
> +
> + sas_unregister_ha(sha);
> + sas_remove_host(sha->core.shost);
> + scsi_remove_host(sha->core.shost);

scsi host should be removed before detaching SAS transport.

See more information:
http://www.spinics.net/lists/linux-scsi/msg90088.html

Thanks,
Wei

> + hisi_sas_free(hisi_hba);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(hisi_sas_remove);
> +
>  static __init int hisi_sas_init(void)
>  {
>   pr_info("hisi_sas: driver version %s\n", DRV_VERSION);
> 

--
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