diff mbox

kernel BUG in drivers/scsi/53c700.c:1129

Message ID 1465594421.2224.58.camel@HansenPartnership.com (mailing list archive)
State Superseded
Headers show

Commit Message

James Bottomley June 10, 2016, 9:33 p.m. UTC 
On Fri, 2016-06-10 at 14:01 -0700, James Bottomley wrote:
> On Fri, 2016-06-10 at 16:58 -0400, Ewan D. Milne wrote:
> > I'm not sure if this is the problem, but the tagging changes to
> > scsi_tcq.h may have altered the 53c700 driver's assumptions.
> > In one case it sets sdev->current_cmnd and then some of the
> > tagging calls would return it if the tag was SCSI_NO_TAG.
> > 
> > NCR_700_queuecommand_lck() does:
> > 
> >         if ((hostdata->tag_negotiated & (1<<scmd_id(SCp))) &&
> >             SCp->device->simple_tags) {
> >                 slot->tag = SCp->request->tag;
> >                 CDEBUG(KERN_DEBUG, SCp, "sending out tag %d, slot
> > %p\n",
> >                        slot->tag, slot);
> >         } else {
> >                 slot->tag = SCSI_NO_TAG;
> >                 /* must populate current_cmnd for
> > scsi_host_find_tag
> > to
> > work */
> >                 SCp->device->current_cmnd = SCp;
> >         }
> 
> Thanks ... I was just about to look for something this.  I'd got to
> interpreting the script as reselected with tag information present 
> and then trying to look the command up with no tag present, hence the
> BUG().

Yes, you're right, it's

commit 64d513ac31bd02a3c9b69ef04444f36c196f9a9d
Author: Christoph Hellwig <hch@lst.de>
Date:   Thu Oct 8 09:28:04 2015 +0100

    scsi: use host wide tags by default

Again.  This time because it's transformation of the handling of
SCSI_NO_TAG is wrong.  You can't replace the return sdev->current_cmnd
original in scsi_find_tag with the NULL return in scsi_find_host_tag.

I think this changesets wins the prize for the greatest number of
generated faults.

Does this fix 53c700.c?

I suppose we'd better look for other places where no tag fell through
...

James

---

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

Comments

James Bottomley June 10, 2016, 9:43 p.m. UTC | #1 
On Fri, 2016-06-10 at 14:33 -0700, James Bottomley wrote:
> On Fri, 2016-06-10 at 14:01 -0700, James Bottomley wrote:
> > On Fri, 2016-06-10 at 16:58 -0400, Ewan D. Milne wrote:
> > > I'm not sure if this is the problem, but the tagging changes to
> > > scsi_tcq.h may have altered the 53c700 driver's assumptions.
> > > In one case it sets sdev->current_cmnd and then some of the
> > > tagging calls would return it if the tag was SCSI_NO_TAG.
> > > 
> > > NCR_700_queuecommand_lck() does:
> > > 
> > >         if ((hostdata->tag_negotiated & (1<<scmd_id(SCp))) &&
> > >             SCp->device->simple_tags) {
> > >                 slot->tag = SCp->request->tag;
> > >                 CDEBUG(KERN_DEBUG, SCp, "sending out tag %d, slot
> > > %p\n",
> > >                        slot->tag, slot);
> > >         } else {
> > >                 slot->tag = SCSI_NO_TAG;
> > >                 /* must populate current_cmnd for
> > > scsi_host_find_tag
> > > to
> > > work */
> > >                 SCp->device->current_cmnd = SCp;
> > >         }
> > 
> > Thanks ... I was just about to look for something this.  I'd got to
> > interpreting the script as reselected with tag information present 
> > and then trying to look the command up with no tag present, hence
> > the
> > BUG().
> 
> Yes, you're right, it's
> 
> commit 64d513ac31bd02a3c9b69ef04444f36c196f9a9d
> Author: Christoph Hellwig <hch@lst.de>
> Date:   Thu Oct 8 09:28:04 2015 +0100
> 
>     scsi: use host wide tags by default
> 
> Again.  This time because it's transformation of the handling of
> SCSI_NO_TAG is wrong.  You can't replace the return sdev
> ->current_cmnd
> original in scsi_find_tag with the NULL return in scsi_find_host_tag.
> 
> I think this changesets wins the prize for the greatest number of
> generated faults.
> 
> Does this fix 53c700.c?
> 
> I suppose we'd better look for other places where no tag fell through
> ...

OK, I checked: snic and fnic use SCSI_NO_TAG but they don't save
anything in current_cmnd, so they can't rely on the original behaviour.
 I think we'll be safe with a local change in 53c700.c

James


--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Helge Deller June 10, 2016, 9:46 p.m. UTC | #2 
On 10.06.2016 23:33, James Bottomley wrote:
> On Fri, 2016-06-10 at 14:01 -0700, James Bottomley wrote:
>> On Fri, 2016-06-10 at 16:58 -0400, Ewan D. Milne wrote:
>>> I'm not sure if this is the problem, but the tagging changes to
>>> scsi_tcq.h may have altered the 53c700 driver's assumptions.
>>> In one case it sets sdev->current_cmnd and then some of the
>>> tagging calls would return it if the tag was SCSI_NO_TAG.
>>>
>>> NCR_700_queuecommand_lck() does:
>>>
>>>         if ((hostdata->tag_negotiated & (1<<scmd_id(SCp))) &&
>>>             SCp->device->simple_tags) {
>>>                 slot->tag = SCp->request->tag;
>>>                 CDEBUG(KERN_DEBUG, SCp, "sending out tag %d, slot
>>> %p\n",
>>>                        slot->tag, slot);
>>>         } else {
>>>                 slot->tag = SCSI_NO_TAG;
>>>                 /* must populate current_cmnd for
>>> scsi_host_find_tag
>>> to
>>> work */
>>>                 SCp->device->current_cmnd = SCp;
>>>         }
>>
>> Thanks ... I was just about to look for something this.  I'd got to
>> interpreting the script as reselected with tag information present 
>> and then trying to look the command up with no tag present, hence the
>> BUG().
> 
> Yes, you're right, it's
> 
> commit 64d513ac31bd02a3c9b69ef04444f36c196f9a9d
> Author: Christoph Hellwig <hch@lst.de>
> Date:   Thu Oct 8 09:28:04 2015 +0100
> 
>     scsi: use host wide tags by default
> 
> Again.  This time because it's transformation of the handling of
> SCSI_NO_TAG is wrong.  You can't replace the return sdev->current_cmnd
> original in scsi_find_tag with the NULL return in scsi_find_host_tag.
> 
> I think this changesets wins the prize for the greatest number of
> generated faults.
> 
> Does this fix 53c700.c?

Yes, the patch below fixes the boot problems for 53c700.c:

[    9.869236] 53c700: Version 2.8 By James.Bottomley@HansenPartnership.com
[    9.949797] scsi0: 53c710 rev 2 
[   10.994453] scsi host0: LASI SCSI 53c700
[   13.008861] scsi 0:0:6:0: Direct-Access     QUANTUM  FIREBALL_TM3200S 300X PQ: 0 ANSI: 2
[   13.106740] scsi target0:0:6: Beginning Domain Validation
[   13.171889] scsi 0:0:6:0: tag#0 Enabling Tag Command Queuing
[   13.239996] scsi target0:0:6: asynchronous
[   13.305374] scsi target0:0:6: FAST-10 SCSI 10.0 MB/s ST (100 ns, offset 8)
[   13.397482] scsi target0:0:6: Domain Validation skipping write tests
[   13.473831] scsi target0:0:6: Ending Domain Validation
[   13.557116] scsi 0:0:6:1: tag#0 Disabling Tag Command Queuing
[   13.637379] st: Version 20160209, fixed bufsize 32768, s/g segs 256
[   13.746803] sd 0:0:6:0: Attached scsi generic sg0 type 0

THANKS!
(please queue the patch up for 4.6-stable at least)
Helge

 
> I suppose we'd better look for other places where no tag fell through
> ...
> 
> James
> 
> ---
> 
> diff --git a/drivers/scsi/53c700.c b/drivers/scsi/53c700.c
> index d4c2856..3ddc85e 100644
> --- a/drivers/scsi/53c700.c
> +++ b/drivers/scsi/53c700.c
> @@ -1122,7 +1122,7 @@ process_script_interrupt(__u32 dsps, __u32 dsp, struct scsi_cmnd *SCp,
>  		} else {
>  			struct scsi_cmnd *SCp;
>  
> -			SCp = scsi_host_find_tag(SDp->host, SCSI_NO_TAG);
> +			SCp = SDp->current_cmnd;
>  			if(unlikely(SCp == NULL)) {
>  				sdev_printk(KERN_ERR, SDp,
>  					"no saved request for untagged cmd\n");
> @@ -1826,7 +1826,7 @@ NCR_700_queuecommand_lck(struct scsi_cmnd *SCp, void (*done)(struct scsi_cmnd *)
>  		       slot->tag, slot);
>  	} else {
>  		slot->tag = SCSI_NO_TAG;
> -		/* must populate current_cmnd for scsi_host_find_tag to work */
> +		/* save current command for reselection */
>  		SCp->device->current_cmnd = SCp;
>  	}
>  	/* sanity check: some of the commands generated by the mid-layer
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig June 13, 2016, 8:09 a.m. UTC | #3 
On Fri, Jun 10, 2016 at 02:43:52PM -0700, James Bottomley wrote:
> OK, I checked: snic and fnic use SCSI_NO_TAG but they don't save
> anything in current_cmnd, so they can't rely on the original behaviour.
>  I think we'll be safe with a local change in 53c700.c

Please move the current_cmnd field in struct scsi_device into the 53c700
driver while you're at it, so that others don't accidentally rely on it.
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/scsi/53c700.c b/drivers/scsi/53c700.c
index d4c2856..3ddc85e 100644
--- a/drivers/scsi/53c700.c
+++ b/drivers/scsi/53c700.c
@@ -1122,7 +1122,7 @@  process_script_interrupt(__u32 dsps, __u32 dsp, struct scsi_cmnd *SCp,
 		} else {
 			struct scsi_cmnd *SCp;
 
-			SCp = scsi_host_find_tag(SDp->host, SCSI_NO_TAG);
+			SCp = SDp->current_cmnd;
 			if(unlikely(SCp == NULL)) {
 				sdev_printk(KERN_ERR, SDp,
 					"no saved request for untagged cmd\n");
@@ -1826,7 +1826,7 @@  NCR_700_queuecommand_lck(struct scsi_cmnd *SCp, void (*done)(struct scsi_cmnd *)
 		       slot->tag, slot);
 	} else {
 		slot->tag = SCSI_NO_TAG;
-		/* must populate current_cmnd for scsi_host_find_tag to work */
+		/* save current command for reselection */
 		SCp->device->current_cmnd = SCp;
 	}
 	/* sanity check: some of the commands generated by the mid-layer