| Message ID | 1465594421.2224.58.camel@HansenPartnership.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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