| Message ID | 1628862553-179450-3-git-send-email-john.garry@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | Remove scsi_cmnd.tag | expand |
On 8/13/21 3:49 PM, John Garry wrote: > It is never read. Setting it and the request tag seems dodgy > anyway. > > Signed-off-by: John Garry <john.garry@huawei.com> > --- > drivers/scsi/fnic/fnic_scsi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
On 8/13/21 6:49 AM, John Garry wrote: > It is never read. Setting it and the request tag seems dodgy > anyway. This is done because there is code in the SCSI error handler that may allocate a SCSI command without allocating a tag. See also scsi_ioctl_reset(). > --- > drivers/scsi/fnic/fnic_scsi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c > index 0f9cedf78872..f8afbfb468dc 100644 > --- a/drivers/scsi/fnic/fnic_scsi.c > +++ b/drivers/scsi/fnic/fnic_scsi.c > @@ -2213,7 +2213,7 @@ fnic_scsi_host_start_tag(struct fnic *fnic, struct scsi_cmnd *sc) > if (IS_ERR(dummy)) > return SCSI_NO_TAG; > > - sc->tag = rq->tag = dummy->tag; > + rq->tag = dummy->tag; > sc->host_scribble = (unsigned char *)dummy; Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On Fri, Aug 13, 2021 at 08:17:45PM -0700, Bart Van Assche wrote: > On 8/13/21 6:49 AM, John Garry wrote: > > It is never read. Setting it and the request tag seems dodgy > > anyway. > > This is done because there is code in the SCSI error handler that may > allocate a SCSI command without allocating a tag. See also > scsi_ioctl_reset(). Yes. Hannes had a great series to stop passing the pointless scsi_cmnd to the reset methods. Hannes, any chance you coul look into resurrecting that?
On 8/14/21 9:39 AM, Christoph Hellwig wrote: > On Fri, Aug 13, 2021 at 08:17:45PM -0700, Bart Van Assche wrote: >> On 8/13/21 6:49 AM, John Garry wrote: >>> It is never read. Setting it and the request tag seems dodgy >>> anyway. >> >> This is done because there is code in the SCSI error handler that may >> allocate a SCSI command without allocating a tag. See also >> scsi_ioctl_reset(). > > Yes. Hannes had a great series to stop passing the pointless scsi_cmnd > to the reset methods. Hannes, any chance you coul look into > resurrecting that? > Sure. Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
On 14/08/2021 13:35, Hannes Reinecke wrote: > On 8/14/21 9:39 AM, Christoph Hellwig wrote: >> On Fri, Aug 13, 2021 at 08:17:45PM -0700, Bart Van Assche wrote: >>> On 8/13/21 6:49 AM, John Garry wrote: >>>> It is never read. Setting it and the request tag seems dodgy >>>> anyway. >>> >>> This is done because there is code in the SCSI error handler that may >>> allocate a SCSI command without allocating a tag. See also >>> scsi_ioctl_reset(). Right, so we just get a loan of the tag of a real request. fnic driver comment: "Really should fix the midlayer to pass in a proper request for ioctls..." >> >> Yes. Hannes had a great series to stop passing the pointless scsi_cmnd >> to the reset methods. Hannes, any chance you coul look into >> resurrecting that? >> > Sure. The latest iteration of that series - at v7 - still passed that fake SCSI command to the reset method, and the reset method allocated the internal command. So will we change change scsi_ioctl_reset() to allocate an internal command, rather than the LLDD? Thanks, John
On 8/16/21 12:00 PM, John Garry wrote: > On 14/08/2021 13:35, Hannes Reinecke wrote: >> On 8/14/21 9:39 AM, Christoph Hellwig wrote: >>> On Fri, Aug 13, 2021 at 08:17:45PM -0700, Bart Van Assche wrote: >>>> On 8/13/21 6:49 AM, John Garry wrote: >>>>> It is never read. Setting it and the request tag seems dodgy >>>>> anyway. >>>> >>>> This is done because there is code in the SCSI error handler that may >>>> allocate a SCSI command without allocating a tag. See also >>>> scsi_ioctl_reset(). > > Right, so we just get a loan of the tag of a real request. fnic driver > comment: > > "Really should fix the midlayer to pass in a proper request for ioctls..." > >>> >>> Yes. Hannes had a great series to stop passing the pointless scsi_cmnd >>> to the reset methods. Hannes, any chance you coul look into >>> resurrecting that? >>> >> Sure. > > The latest iteration of that series - at v7 - still passed that fake > SCSI command to the reset method, and the reset method allocated the > internal command. > > So will we change change scsi_ioctl_reset() to allocate an internal > command, rather than the LLDD? > Nah, Christoph was talking about my patch series to revamp the SCSI error handler. With that one we'll be passing the respective objects to the SCSI EH functions (ie struct scsi_device for eh_device_reset()), doing away with the need to allocate an internal command for ioctl reset. Currently revamping the patchset, should be ready later this week. Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c index 0f9cedf78872..f8afbfb468dc 100644 --- a/drivers/scsi/fnic/fnic_scsi.c +++ b/drivers/scsi/fnic/fnic_scsi.c @@ -2213,7 +2213,7 @@ fnic_scsi_host_start_tag(struct fnic *fnic, struct scsi_cmnd *sc) if (IS_ERR(dummy)) return SCSI_NO_TAG; - sc->tag = rq->tag = dummy->tag; + rq->tag = dummy->tag; sc->host_scribble = (unsigned char *)dummy; return dummy->tag;
It is never read. Setting it and the request tag seems dodgy anyway. Signed-off-by: John Garry <john.garry@huawei.com> --- drivers/scsi/fnic/fnic_scsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.26.2