[PATCH] pm8001: panics/lockups from asynchronous device removal.

[PATCH] pm8001: panics/lockups from asynchronous device removal.

[Mark Salyzyn]

pm8001_query_task() and pm8001_abort_task() panic kernel when devices asynchronously disappear, a possible scenario since these functions are generally called when errors are mounting. Some of the panics are a direct result of a failure to NULL check some of the structure variables that are in certain states of teardown. One of the lockups was a direct result of returning an unexpected code to libsas' sas_scsi_find_task() function (creating a tight loop of an unexpected code 138 upstream to the scsi layer queue function).

Signed-off-by: mark_salyzyn@xyratex.com
Cc: jack_wang@usish.com
Cc: JBottomley@parallels.com
Cc: crystal_yu@usish.com
Cc: john_gong@usish.com
Cc: lindar_liu <lindar_liu@usish.com>

 drivers/scsi/pm8001/pm8001_sas.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index fb3dc99..1cac756 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -940,6 +940,8 @@ int pm8001_query_task(struct sas_task *task)
 		struct pm8001_hba_info *pm8001_ha =
 			pm8001_find_ha_by_dev(dev);
 
+		if (unlikely(!cmnd || !cmnd->device))
+			return rc;
 		int_to_scsilun(cmnd->device->lun, &lun);
 		rc = pm8001_find_tag(task, &tag);
 		if (rc == 0) {
@@ -947,9 +949,11 @@ int pm8001_query_task(struct sas_task *task)
 			return rc;
 		}
 		PM8001_EH_DBG(pm8001_ha, pm8001_printk("Query:["));
-		for (i = 0; i < 16; i++)
-			printk(KERN_INFO "%02x ", cmnd->cmnd[i]);
-		printk(KERN_INFO "]\n");
+		if (pm8001_ha->logging_level & PM8001_EH_LOGGING) {
+			for (i = 0; i < 16; i++)
+				printk(KERN_LEVEL "%02x ", cmnd->cmnd[i]);
+			printk(KERN_LEVEL "]\n");
+		}
 		tmf_task.tmf = 	TMF_QUERY_TASK;
 		tmf_task.tag_of_task_to_be_managed = tag;
 
@@ -960,6 +964,13 @@ int pm8001_query_task(struct sas_task *task)
 			PM8001_EH_DBG(pm8001_ha,
 				pm8001_printk("The task is still in Lun\n"));
 			break;
+
+		/* libsas can not handle any other error code than this list */
+		default:
+			/* rc is likely something like SAS_OPEN_REJECT etc */
+			pm8001_printk("pm8001_issue_ssp_tmf()=0x%x\n", rc);
+			rc = TMF_RESP_FUNC_FAILED;
+			/* FALLTHRU */
 		/* The task is not in Lun or failed, reset the phy */
 		case TMF_RESP_FUNC_FAILED:
 		case TMF_RESP_FUNC_COMPLETE:
@@ -986,7 +997,8 @@ int pm8001_abort_task(struct sas_task *task)
 	struct pm8001_device *pm8001_dev;
 	struct pm8001_tmf_task tmf_task;
 	int rc = TMF_RESP_FUNC_FAILED;
-	if (unlikely(!task || !task->lldd_task || !task->dev))
+	if (unlikely(!task || !task->lldd_task
+	 || !task->dev || !task->dev->lldd_dev))
 		return rc;
 	spin_lock_irqsave(&task->task_state_lock, flags);
 	if (task->task_state_flags & SAS_TASK_STATE_DONE) {
@@ -1001,6 +1013,8 @@ int pm8001_abort_task(struct sas_task *task)
 		ccb = task->lldd_task;
 		pm8001_dev = dev->lldd_dev;
 		pm8001_ha = pm8001_find_ha_by_dev(dev);
+		if (unlikely(!cmnd || !cmnd->device))
+			return rc;
 		int_to_scsilun(cmnd->device->lun, &lun);
 		rc = pm8001_find_tag(task, &tag);
 		if (rc == 0) {

Sincerely -- Mark Salyzyn

RE: [PATCH] pm8001: panics/lockups from asynchronous device removal.

[Jack Wang]

RE [PATCH] pm8001: panics/lockups from asynchronous device removal.
> 
> pm8001_query_task() and pm8001_abort_task() panic kernel when devices
> asynchronously disappear, a possible scenario since these functions are
> generally called when errors are mounting. Some of the panics are a direct
> result of a failure to NULL check some of the structure variables that are
in
> certain states of teardown. One of the lockups was a direct result of
returning
> an unexpected code to libsas' sas_scsi_find_task() function (creating a
tight
> loop of an unexpected code 138 upstream to the scsi layer queue function).
> 
> Signed-off-by: mark_salyzyn@xyratex.com
> Cc: jack_wang@usish.com
> Cc: JBottomley@parallels.com
> Cc: crystal_yu@usish.com
> Cc: john_gong@usish.com
> Cc: lindar_liu <lindar_liu@usish.com>
> 
>  drivers/scsi/pm8001/pm8001_sas.c |   22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c
> b/drivers/scsi/pm8001/pm8001_sas.c
> index fb3dc99..1cac756 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -940,6 +940,8 @@ int pm8001_query_task(struct sas_task *task)
>  		struct pm8001_hba_info *pm8001_ha =
>  			pm8001_find_ha_by_dev(dev);
> 
> +		if (unlikely(!cmnd || !cmnd->device))
> +			return rc;
>  		int_to_scsilun(cmnd->device->lun, &lun);
>  		rc = pm8001_find_tag(task, &tag);
>  		if (rc == 0) {
> @@ -947,9 +949,11 @@ int pm8001_query_task(struct sas_task *task)
>  			return rc;
>  		}
>  		PM8001_EH_DBG(pm8001_ha, pm8001_printk("Query:["));
> -		for (i = 0; i < 16; i++)
> -			printk(KERN_INFO "%02x ", cmnd->cmnd[i]);
> -		printk(KERN_INFO "]\n");
> +		if (pm8001_ha->logging_level & PM8001_EH_LOGGING) {
> +			for (i = 0; i < 16; i++)
> +				printk(KERN_LEVEL "%02x ", cmnd->cmnd[i]);
> +			printk(KERN_LEVEL "]\n");
[Jack Wang] 

Here KERN_LEVEL is wrong, maybe you means KERN_INFO, others is good, thanks
for fix this.

Re: [PATCH] pm8001: panics/lockups from asynchronous device removal.

[Dan Williams]

On Tue, Jan 17, 2012 at 10:29 AM, Mark Salyzyn <mark_salyzyn@xyratex.com> wrote:
> pm8001_query_task() and pm8001_abort_task() panic kernel when devices asynchronously disappear, a possible scenario since these functions are generally called when errors are mounting. Some of the panics are a direct result of a failure to NULL check some of the structure variables that are in certain states of teardown. One of the lockups was a direct result of returning an unexpected code to libsas' sas_scsi_find_task() function (creating a tight loop of an unexpected code 138 upstream to the scsi layer queue function).
>
> Signed-off-by: mark_salyzyn@xyratex.com
> Cc: jack_wang@usish.com
> Cc: JBottomley@parallels.com
> Cc: crystal_yu@usish.com
> Cc: john_gong@usish.com
> Cc: lindar_liu <lindar_liu@usish.com>

While your in the area of libsas error handling, mind weighing in on
the pending libsas error handling rework backlog?

http://git.kernel.org/?p=linux/kernel/git/djbw/isci.git;a=shortlog;h=refs/heads/libsas-eh-reworks-v4


...more notes below:

> @@ -986,7 +997,8 @@ int pm8001_abort_task(struct sas_task *task)
>        struct pm8001_device *pm8001_dev;
>        struct pm8001_tmf_task tmf_task;
>        int rc = TMF_RESP_FUNC_FAILED;
> -       if (unlikely(!task || !task->lldd_task || !task->dev))
> +       if (unlikely(!task || !task->lldd_task
> +        || !task->dev || !task->dev->lldd_dev))

Hmm some of these are "never can happen" checks, so if you are indeed
seeing !task or !task->dev something is very wrong in libsas. checking
task->lldd_task and task->dev->lldd_dev should be all that is
required.

Also, I think it confuses the situation to return TMF_RESP_FUNC_FAILED
in this case because the truth of the matter, as far as error handling
is concerned, is that the lldd has forgotten the task, but you would
need to check that the lldd does not subsequently call task->task_done
in some other path.

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pm8001: panics/lockups from asynchronous device removal (take 2)

[Mark Salyzyn]

Thanks for the review.

On Jan 17, 2012, at 8:37 PM, Jack Wang wrote:

> Here KERN_LEVEL is wrong, maybe you means KERN_INFO, others is good, thanks for fix this.

Sigh ... checkpatch complained, I edited the patch directly as I thought it was obvious, but reading checkpatch rather than the code. I need to add a required compile-check on a final-patch (or stop editing patches; edit code only, generate patches) before submittal in my process. update follows:


pm8001_query_task and pm8001_abort_task panic kernel when devices asynchronously disappear, a possible scenario since these functions are generally called when errors are mounting. Some of the panics are a direct result of a failure to NULL check some of the structure variables that are in certain states of teardown. One of the lockups was a direct result of returning an unexpected code to libsas' sas_scsi_find_task() function (creating a tight loop of an unexpected code 138 upstream to the scsi layers queue function).

Signed-off-by: mark_salyzyn@xyratex.com
Cc: jack_wang@usish.com
Cc: JBottomley@parallels.com
Cc: crystal_yu@usish.com
Cc: john_gong@usish.com
Cc: lindar_liu <lindar_liu@usish.com>

 drivers/csi/pm8001/pm8001_sas.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index fb3dc99..1cac756 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -940,6 +940,8 @@ int pm8001_query_task(struct sas_task *task)
 		struct pm8001_hba_info *pm8001_ha =
 			pm8001_find_ha_by_dev(dev);
 
+		if (unlikely(!cmnd || !cmnd->device))
+			return rc;
 		int_to_scsilun(cmnd->device->lun, &lun);
 		rc = pm8001_find_tag(task, &tag);
 		if (rc == 0) {
@@ -947,9 +949,11 @@ int pm8001_query_task(struct sas_task *task)
 			return rc;
 		}
 		PM8001_EH_DBG(pm8001_ha, pm8001_printk("Query:["));
-		for (i = 0; i < 16; i++)
-			printk(KERN_INFO "%02x ", cmnd->cmnd[i]);
-		printk(KERN_INFO "]\n");
+		if (pm8001_ha->logging_level & PM8001_EH_LOGGING) {
+			for (i = 0; i < 16; i++)
+				printk(KERN_INFO "%02x ", cmnd->cmnd[i]);
+			printk(KERN_INFO "]\n");
+		}
 		tmf_task.tmf = 	TMF_QUERY_TASK;
 		tmf_task.tag_of_task_to_be_managed = tag;
 
@@ -960,6 +964,13 @@ int pm8001_query_task(struct sas_task *task)
 			PM8001_EH_DBG(pm8001_ha,
 				pm8001_printk("The task is still in Lun\n"));
 			break;
+
+		/* libsas can not handle any other error code than this list */
+		default:
+			/* rc is likely something like SAS_OPEN_REJECT etc */
+			pm8001_printk("pm8001_issue_ssp_tmf()=0x%x\n", rc);
+			rc = TMF_RESP_FUNC_FAILED;
+			/* FALLTHRU */
 		/* The task is not in Lun or failed, reset the phy */
 		case TMF_RESP_FUNC_FAILED:
 		case TMF_RESP_FUNC_COMPLETE:
@@ -986,7 +997,8 @@ int pm8001_abort_task(struct sas_task *task)
 	struct pm8001_device *pm8001_dev;
 	struct pm8001_tmf_task tmf_task;
 	int rc = TMF_RESP_FUNC_FAILED;
-	if (unlikely(!task || !task->lldd_task || !task->dev))
+	if (unlikely(!task || !task->lldd_task
+	 || !task->dev || !task->dev->lldd_dev))
 		return rc;
 	spin_lock_irqsave(&task->task_state_lock, flags);
 	if (task->task_state_flags & SAS_TASK_STATE_DONE) {
@@ -1001,6 +1013,8 @@ int pm8001_abort_task(struct sas_task *task)
 		ccb = task->lldd_task;
 		pm8001_dev = dev->lldd_dev;
 		pm8001_ha = pm8001_find_ha_by_dev(dev);
+		if (unlikely(!cmnd || !cmnd->device))
+			return rc;
 		int_to_scsilun(cmnd->device->lun, &lun);
 		rc = pm8001_find_tag(task, &tag);
 		if (rc == 0) {

Re: [PATCH] pm8001: panics/lockups from asynchronous device removal.

[Mark Salyzyn]

Thanks, Comments inline

On Jan 17, 2012, at 11:04 PM, Dan Williams wrote:

> On Tue, Jan 17, 2012 at 10:29 AM, Mark Salyzyn <mark_salyzyn@xyratex.com> wrote:
>> pm8001_query_task() and pm8001_abort_task() panic kernel when devices asynchronously disappear, a possible scenario since these functions are generally called when errors are mounting. Some of the panics are a direct result of a failure to NULL check some of the structure variables that are in certain states of teardown. One of the lockups was a direct result of returning an unexpected code to libsas' sas_scsi_find_task() function (creating a tight loop of an unexpected code 138 upstream to the scsi layer queue function).
>> 
>> Signed-off-by: mark_salyzyn@xyratex.com
>> Cc: jack_wang@usish.com
>> Cc: JBottomley@parallels.com
>> Cc: crystal_yu@usish.com
>> Cc: john_gong@usish.com
>> Cc: lindar_liu <lindar_liu@usish.com>
> 
> While your in the area of libsas error handling, mind weighing in on the pending libsas error handling rework backlog?
> 
> http://git.kernel.org/?p=linux/kernel/git/djbw/isci.git;a=shortlog;h=refs/heads/libsas-eh-reworks-v4

Xyratex has me focussed on 2.6.32 problems, I will look at these, but will be off the reservation (and with no Q/A testing resource to keep me honest) when doing so ;-}. Pure Code Inspection thus trumps experimentation ...

> ...more notes below:
> 
>> @@ -986,7 +997,8 @@ int pm8001_abort_task(struct sas_task *task)
>>        struct pm8001_device *pm8001_dev;
>>        struct pm8001_tmf_task tmf_task;
>>        int rc = TMF_RESP_FUNC_FAILED;
>> -       if (unlikely(!task || !task->lldd_task || !task->dev))
>> +       if (unlikely(!task || !task->lldd_task
>> +        || !task->dev || !task->dev->lldd_dev))
> 
> Hmm some of these are "never can happen" checks, so if you are indeed
> seeing !task or !task->dev something is very wrong in libsas. checking
> task->lldd_task and task->dev->lldd_dev should be all that is
> required.

OK, point taken, these issues surface with careless abandon in the 2.6.32-vintage kernels; and may, or may not, be present in 3.2.1-vintage kernels.

What this means is that maybe we should be submitting these stabilization/band-aid patches to the stable trees rather than to the top-of-tree?

I'd opt (not that I have the rights) for paranoia and adding these in at top-of-tree, as band-aids against panics (in libsas or pm8001). The checks are not costly IMHO (hopefully less-so as wrapped by unlikely()). My WAG is that this patch should be cancelled here, and placed into the 2.6.32-stable bucket ...

Looking forward to yours and Greg's comments to straighten me out.

> Also, I think it confuses the situation to return TMF_RESP_FUNC_FAILED
> in this case because the truth of the matter, as far as error handling
> is concerned, is that the lldd has forgotten the task, but you would
> need to check that the lldd does not subsequently call task->task_done
> in some other path.

In our private copy of the pm8001 driver, we have some debug code that performs tag tracking and memory-leak detection. In that code we ensure that task_done is not called for a tag that has already been released by another path. In 2.6.32-vintage kernels, when we hit these we try to remove them (those patches are in my back-log).

> Dan

Sincerely -- Mark Salyzyn

Re: [PATCH] pm8001: panics/lockups from asynchronous device removal.

[Greg KH]

On Wed, Jan 18, 2012 at 11:08:27AM -0500, Mark Salyzyn wrote:
> Thanks, Comments inline
> 
> On Jan 17, 2012, at 11:04 PM, Dan Williams wrote:
> 
> > On Tue, Jan 17, 2012 at 10:29 AM, Mark Salyzyn <mark_salyzyn@xyratex.com> wrote:
> >> pm8001_query_task() and pm8001_abort_task() panic kernel when
> >> devices asynchronously disappear, a possible scenario since these
> >> functions are generally called when errors are mounting. Some of
> >> the panics are a direct result of a failure to NULL check some of
> >> the structure variables that are in certain states of teardown. One
> >> of the lockups was a direct result of returning an unexpected code
> >> to libsas' sas_scsi_find_task() function (creating a tight loop of
> >> an unexpected code 138 upstream to the scsi layer queue function).
> >> 
> >> Signed-off-by: mark_salyzyn@xyratex.com
> >> Cc: jack_wang@usish.com
> >> Cc: JBottomley@parallels.com
> >> Cc: crystal_yu@usish.com
> >> Cc: john_gong@usish.com
> >> Cc: lindar_liu <lindar_liu@usish.com>
> > 
> > While your in the area of libsas error handling, mind weighing in on the pending libsas error handling rework backlog?
> > 
> > http://git.kernel.org/?p=linux/kernel/git/djbw/isci.git;a=shortlog;h=refs/heads/libsas-eh-reworks-v4
> 
> Xyratex has me focussed on 2.6.32 problems, I will look at these, but
> will be off the reservation (and with no Q/A testing resource to keep
> me honest) when doing so ;-}. Pure Code Inspection thus trumps
> experimentation ...
> 
> > ...more notes below:
> > 
> >> @@ -986,7 +997,8 @@ int pm8001_abort_task(struct sas_task *task)
> >>        struct pm8001_device *pm8001_dev;
> >>        struct pm8001_tmf_task tmf_task;
> >>        int rc = TMF_RESP_FUNC_FAILED;
> >> -       if (unlikely(!task || !task->lldd_task || !task->dev))
> >> +       if (unlikely(!task || !task->lldd_task
> >> +        || !task->dev || !task->dev->lldd_dev))
> > 
> > Hmm some of these are "never can happen" checks, so if you are indeed
> > seeing !task or !task->dev something is very wrong in libsas. checking
> > task->lldd_task and task->dev->lldd_dev should be all that is
> > required.
> 
> OK, point taken, these issues surface with careless abandon in the
> 2.6.32-vintage kernels; and may, or may not, be present in
> 3.2.1-vintage kernels.
> 
> What this means is that maybe we should be submitting these
> stabilization/band-aid patches to the stable trees rather than to the
> top-of-tree?
> 
> I'd opt (not that I have the rights) for paranoia and adding these in
> at top-of-tree, as band-aids against panics (in libsas or pm8001). The
> checks are not costly IMHO (hopefully less-so as wrapped by
> unlikely()). My WAG is that this patch should be cancelled here, and
> placed into the 2.6.32-stable bucket ...
> 
> Looking forward to yours and Greg's comments to straighten me out.

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>

Re: [PATCH] pm8001: panics/lockups from asynchronous device removal.

[Dan Williams]

On Wed, Jan 18, 2012 at 8:08 AM, Mark Salyzyn <mark_salyzyn@xyratex.com> wrote:
> Thanks, Comments inline
>
> On Jan 17, 2012, at 11:04 PM, Dan Williams wrote:
>
>> On Tue, Jan 17, 2012 at 10:29 AM, Mark Salyzyn <mark_salyzyn@xyratex.com> wrote:
>>> pm8001_query_task() and pm8001_abort_task() panic kernel when devices asynchronously disappear, a possible scenario since these functions are generally called when errors are mounting. Some of the panics are a direct result of a failure to NULL check some of the structure variables that are in certain states of teardown. One of the lockups was a direct result of returning an unexpected code to libsas' sas_scsi_find_task() function (creating a tight loop of an unexpected code 138 upstream to the scsi layer queue function).
>>>
>>> Signed-off-by: mark_salyzyn@xyratex.com
>>> Cc: jack_wang@usish.com
>>> Cc: JBottomley@parallels.com
>>> Cc: crystal_yu@usish.com
>>> Cc: john_gong@usish.com
>>> Cc: lindar_liu <lindar_liu@usish.com>
>>
>> While your in the area of libsas error handling, mind weighing in on the pending libsas error handling rework backlog?
>>
>> http://git.kernel.org/?p=linux/kernel/git/djbw/isci.git;a=shortlog;h=refs/heads/libsas-eh-reworks-v4
>
> Xyratex has me focussed on 2.6.32 problems, I will look at these, but will be off the reservation (and with no Q/A testing resource to keep me honest) when doing so ;-}. Pure Code Inspection thus trumps experimentation ...
>
>> ...more notes below:
>>
>>> @@ -986,7 +997,8 @@ int pm8001_abort_task(struct sas_task *task)
>>>        struct pm8001_device *pm8001_dev;
>>>        struct pm8001_tmf_task tmf_task;
>>>        int rc = TMF_RESP_FUNC_FAILED;
>>> -       if (unlikely(!task || !task->lldd_task || !task->dev))
>>> +       if (unlikely(!task || !task->lldd_task
>>> +        || !task->dev || !task->dev->lldd_dev))
>>
>> Hmm some of these are "never can happen" checks, so if you are indeed
>> seeing !task or !task->dev something is very wrong in libsas. checking
>> task->lldd_task and task->dev->lldd_dev should be all that is
>> required.
>
> OK, point taken, these issues surface with careless abandon in the 2.6.32-vintage kernels; and may, or may not, be present in 3.2.1-vintage kernels.
>
> What this means is that maybe we should be submitting these stabilization/band-aid patches to the stable trees rather than to the top-of-tree?

2.6.32 pre-dates the libsas conversion to "new-style" libata error
handling, so a band aid that works there may now be invalid, like:
http://git.kernel.org/?p=linux/kernel/git/djbw/isci.git;a=commitdiff;h=030253ca

> I'd opt (not that I have the rights) for paranoia and adding these in at top-of-tree, as band-aids against panics (in libsas or pm8001). The checks are not costly IMHO (hopefully less-so as wrapped by unlikely()). My WAG is that this patch should be cancelled here, and placed into the 2.6.32-stable bucket ...

-stable needs the upstream commit as a reference and it is certainly
valid to add the  !task->dev->lldd_dev check to current upstream.  The
!task and !task->dev are paranoia, but they are already in the code,
so removing them is just a clean up to reduce confusion about the
guarantees that libsas makes to lldds.

In other words,
Acked-by: Dan Williams <dan.j.williams@intel.com>

...and if you can reproduce a !task or !task->dev situation on current
upstream I'd like to see it.

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pm8001: panics/lockups from asynchronous device removal.

[Mark Salyzyn]

Thanks for the Ack. I will work on testing that these checks are still relevant for 3.2+

Note, the changes are there for SAS devices and not ATA devices (we have none of the later), so I get the impression that many of the changes in libata/libsas would not touch on the problems we are experiencing.

I guarantee that the unexpected code 138 is 'bad', and the change is relevant for the current crop of kernels.

Sincerely -- Mark Salyzyn

On Jan 18, 2012, at 12:47 PM, Dan Williams wrote:

> On Wed, Jan 18, 2012 at 8:08 AM, Mark Salyzyn <mark_salyzyn@xyratex.com> wrote:
>> Thanks, Comments inline
>> 
>> On Jan 17, 2012, at 11:04 PM, Dan Williams wrote:
>> 
>>> On Tue, Jan 17, 2012 at 10:29 AM, Mark Salyzyn <mark_salyzyn@xyratex.com> wrote:
>>>> pm8001_query_task() and pm8001_abort_task() panic kernel when devices asynchronously disappear, a possible scenario since these functions are generally called when errors are mounting. Some of the panics are a direct result of a failure to NULL check some of the structure variables that are in certain states of teardown. One of the lockups was a direct result of returning an unexpected code to libsas' sas_scsi_find_task() function (creating a tight loop of an unexpected code 138 upstream to the scsi layer queue function).
>>>> 
>>>> Signed-off-by: mark_salyzyn@xyratex.com
>>>> Cc: jack_wang@usish.com
>>>> Cc: JBottomley@parallels.com
>>>> Cc: crystal_yu@usish.com
>>>> Cc: john_gong@usish.com
>>>> Cc: lindar_liu <lindar_liu@usish.com>
>>> 
>>> While your in the area of libsas error handling, mind weighing in on the pending libsas error handling rework backlog?
>>> 
>>> http://git.kernel.org/?p=linux/kernel/git/djbw/isci.git;a=shortlog;h=refs/heads/libsas-eh-reworks-v4
>> 
>> Xyratex has me focussed on 2.6.32 problems, I will look at these, but will be off the reservation (and with no Q/A testing resource to keep me honest) when doing so ;-}. Pure Code Inspection thus trumps experimentation ...
>> 
>>> ...more notes below:
>>> 
>>>> @@ -986,7 +997,8 @@ int pm8001_abort_task(struct sas_task *task)
>>>>        struct pm8001_device *pm8001_dev;
>>>>        struct pm8001_tmf_task tmf_task;
>>>>        int rc = TMF_RESP_FUNC_FAILED;
>>>> -       if (unlikely(!task || !task->lldd_task || !task->dev))
>>>> +       if (unlikely(!task || !task->lldd_task
>>>> +        || !task->dev || !task->dev->lldd_dev))
>>> 
>>> Hmm some of these are "never can happen" checks, so if you are indeed
>>> seeing !task or !task->dev something is very wrong in libsas. checking
>>> task->lldd_task and task->dev->lldd_dev should be all that is
>>> required.
>> 
>> OK, point taken, these issues surface with careless abandon in the 2.6.32-vintage kernels; and may, or may not, be present in 3.2.1-vintage kernels.
>> 
>> What this means is that maybe we should be submitting these stabilization/band-aid patches to the stable trees rather than to the top-of-tree?
> 
> 2.6.32 pre-dates the libsas conversion to "new-style" libata error
> handling, so a band aid that works there may now be invalid, like:
> http://git.kernel.org/?p=linux/kernel/git/djbw/isci.git;a=commitdiff;h=030253ca
> 
>> I'd opt (not that I have the rights) for paranoia and adding these in at top-of-tree, as band-aids against panics (in libsas or pm8001). The checks are not costly IMHO (hopefully less-so as wrapped by unlikely()). My WAG is that this patch should be cancelled here, and placed into the 2.6.32-stable bucket ...
> 
> -stable needs the upstream commit as a reference and it is certainly
> valid to add the  !task->dev->lldd_dev check to current upstream.  The
> !task and !task->dev are paranoia, but they are already in the code,
> so removing them is just a clean up to reduce confusion about the
> guarantees that libsas makes to lldds.
> 
> In other words,
> Acked-by: Dan Williams <dan.j.williams@intel.com>
> 
> ...and if you can reproduce a !task or !task->dev situation on current
> upstream I'd like to see it.
> 
> --
> Dan