[Patch] QLogic qla2x00 driver fixes

[Patch] QLogic qla2x00 driver fixes

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

Don't use cmd->request->nr_hw_segments as it may not be initialized
(SG_IO in particular bypasses anything that initializes this and just
uses scsi_do_req to insert a scsi_request directly on the head of the
queue) and a bogus value here can trip up the checks to make sure that
the number of segments will fit in the queue ring buffer, resulting in
commands that are never completed.

Fix up several issues with PCI DMA mapping and failure to check return
values on the mappings.

Make the check for space in the ring buffer happen after the DMA mapping
is done since any checks done before the mapping has taken place are
bogus.

-- 
Doug Ledford <dledford@redhat.com>
http://people.redhat.com/dledford

[-- Attachment #2: qla.patch --]
[-- Type: text/x-patch, Size: 3816 bytes --]

===== qla_iocb.c 1.15 vs edited =====
--- 1.15/drivers/scsi/qla2xxx/qla_iocb.c	2004-12-09 01:12:03 -05:00
+++ edited/qla_iocb.c	2005-02-24 16:25:55 -05:00
@@ -216,18 +216,7 @@ void qla2x00_build_scsi_iocbs_32(srb_t *
 			cur_seg++;
 		}
 	} else {
-		dma_addr_t	req_dma;
-		struct page	*page;
-		unsigned long	offset;
-
-		page = virt_to_page(cmd->request_buffer);
-		offset = ((unsigned long)cmd->request_buffer & ~PAGE_MASK);
-		req_dma = pci_map_page(ha->pdev, page, offset,
-		    cmd->request_bufflen, cmd->sc_data_direction);
-
-		sp->dma_handle = req_dma;
-
-		*cur_dsd++ = cpu_to_le32(req_dma);
+		*cur_dsd++ = cpu_to_le32(sp->dma_handle);
 		*cur_dsd++ = cpu_to_le32(cmd->request_bufflen);
 	}
 }
@@ -299,19 +288,8 @@ void qla2x00_build_scsi_iocbs_64(srb_t *
 			cur_seg++;
 		}
 	} else {
-		dma_addr_t	req_dma;
-		struct page	*page;
-		unsigned long	offset;
-
-		page = virt_to_page(cmd->request_buffer);
-		offset = ((unsigned long)cmd->request_buffer & ~PAGE_MASK);
-		req_dma = pci_map_page(ha->pdev, page, offset,
-		    cmd->request_bufflen, cmd->sc_data_direction);
-
-		sp->dma_handle = req_dma;
-
-		*cur_dsd++ = cpu_to_le32(LSD(req_dma));
-		*cur_dsd++ = cpu_to_le32(MSD(req_dma));
+		*cur_dsd++ = cpu_to_le32(LSD(sp->dma_handle));
+		*cur_dsd++ = cpu_to_le32(MSD(sp->dma_handle));
 		*cur_dsd++ = cpu_to_le32(cmd->request_bufflen);
 	}
 }
@@ -344,6 +322,8 @@ qla2x00_start_scsi(srb_t *sp)
 
 	/* Setup device pointers. */
 	ret = 0;
+	/* So we know we haven't pci_map'ed anything yet */
+	tot_dsds = 0;
 	fclun = sp->lun_queue->fclun;
 	ha = fclun->fcport->ha;
 	reg = ha->iobase;
@@ -372,8 +352,32 @@ qla2x00_start_scsi(srb_t *sp)
 	if (index == MAX_OUTSTANDING_COMMANDS)
 		goto queuing_error;
 
+	/* Map the sg table so we have an accurate count of sg entries needed */
+	if (cmd->use_sg) {
+		sg = (struct scatterlist *) cmd->request_buffer;
+		tot_dsds = pci_map_sg(ha->pdev, sg, cmd->use_sg,
+		    cmd->sc_data_direction);
+		if (tot_dsds == 0)
+			goto queuing_error;
+	} else if (cmd->request_bufflen) {
+		dma_addr_t	req_dma;
+		struct page	*page;
+		unsigned long	offset;
+
+		page = virt_to_page(cmd->request_buffer);
+		offset = ((unsigned long)cmd->request_buffer & ~PAGE_MASK);
+		req_dma = pci_map_page(ha->pdev, page, offset,
+		    cmd->request_bufflen, cmd->sc_data_direction);
+
+		if (dma_mapping_error(req_dma))
+			goto queuing_error;
+
+		sp->dma_handle = req_dma;
+		tot_dsds = 1;
+	}
+
 	/* Calculate the number of request entries needed. */
-	req_cnt = (ha->calc_request_entries)(cmd->request->nr_hw_segments);
+	req_cnt = (ha->calc_request_entries)(tot_dsds);
 	if (ha->req_q_cnt < (req_cnt + 2)) {
 		cnt = RD_REG_WORD_RELAXED(ISP_REQ_Q_OUT(ha, reg));
 		if (ha->req_ring_index < cnt)
@@ -385,19 +389,6 @@ qla2x00_start_scsi(srb_t *sp)
 	if (ha->req_q_cnt < (req_cnt + 2))
 		goto queuing_error;
 
-	/* Finally, we have enough space, now perform mappings. */
-	tot_dsds = 0;
-	if (cmd->use_sg) {
-		sg = (struct scatterlist *) cmd->request_buffer;
-		tot_dsds = pci_map_sg(ha->pdev, sg, cmd->use_sg,
-		    cmd->sc_data_direction);
-		if (tot_dsds == 0)
-			goto queuing_error;
-	} else if (cmd->request_bufflen) {
-	    tot_dsds++;
-	}
-	req_cnt = (ha->calc_request_entries)(tot_dsds);
-
 	/* Build command packet */
 	ha->current_outstanding_cmd = handle;
 	ha->outstanding_cmds[handle] = sp;
@@ -479,6 +470,14 @@ qla2x00_start_scsi(srb_t *sp)
 	return (QLA_SUCCESS);
 
 queuing_error:
+	if (cmd->use_sg && tot_dsds) {
+		sg = (struct scatterlist *) cmd->request_buffer;
+		pci_unmap_sg(ha->pdev, sg, cmd->use_sg,
+		    cmd->sc_data_direction);
+	} else if (tot_dsds) {
+		pci_unmap_page(ha->pdev, sp->dma_handle, cmd->request_bufflen,
+			       cmd->sc_data_direction);
+	}
 	spin_unlock_irqrestore(&ha->hardware_lock, flags);
 
 	return (QLA_FUNCTION_FAILED);

Re: [Patch] QLogic qla2x00 driver fixes

[Jeff Garzik]

Doug Ledford wrote:
> Don't use cmd->request->nr_hw_segments as it may not be initialized
> (SG_IO in particular bypasses anything that initializes this and just
> uses scsi_do_req to insert a scsi_request directly on the head of the
> queue) and a bogus value here can trip up the checks to make sure that
> the number of segments will fit in the queue ring buffer, resulting in
> commands that are never completed.
> 
> Fix up several issues with PCI DMA mapping and failure to check return
> values on the mappings.
> 
> Make the check for space in the ring buffer happen after the DMA mapping
> is done since any checks done before the mapping has taken place are
> bogus.

For future patches please include a signed-off-by line, per
http://linux.yyz.us/patch-format.html

Darned lawyers...

The patch itself looks OK to me.

	Jeff

Re: [Patch] QLogic qla2x00 driver fixes

[Andrew Vasquez]

On Thu, 24 Feb 2005, Doug Ledford wrote:

> Don't use cmd->request->nr_hw_segments as it may not be initialized
> (SG_IO in particular bypasses anything that initializes this and just
> uses scsi_do_req to insert a scsi_request directly on the head of the
> queue)
>

I opted to begin to use the nr_hw_segments as a guide following the
following thread on linux-scsi:

http://marc.theaimsgroup.com/?l=linux-scsi&m=107940832718154&w=2

and had inquired about its validaty in usage again in September of
last year:

http://marc.theaimsgroup.com/?l=linux-scsi&m=109580376921554&w=2

seems not all callers (properly?) prepare a request, still...

> and a bogus value here can trip up the checks to make sure that
> the number of segments will fit in the queue ring buffer, resulting in
> commands that are never completed.
> 

Yes, this is an unfortunate side-effect.  Not wanting to labor on the
reasoning behind its usage (the first marc referenced-link goes into
some details), the changes queued up in:

http://marc.theaimsgroup.com/?l=linux-scsi&m=110754501726445&w=2

negate any dependencies on nr_hw_segments.

> Fix up several issues with PCI DMA mapping and failure to check return
> values on the mappings.
> 

Yes, thanks for catching this.

> Make the check for space in the ring buffer happen after the DMA mapping
> is done since any checks done before the mapping has taken place are
> bogus.
> 

I'm hoping once the tree opens up after 2.6.11 is released to begin
making further additions to the qla2xxx driver with the experimentaly
patched driver as a base.

I'll queue-up your changes for my next set of patches.

Thanks again,
Andrew Vasquez

Re: [Patch] QLogic qla2x00 driver fixes

[Douglas Gilbert]

Doug Ledford wrote:
> Don't use cmd->request->nr_hw_segments as it may not be initialized
> (SG_IO in particular bypasses anything that initializes this and just
> uses scsi_do_req to insert a scsi_request directly on the head of the
> queue) and a bogus value here can trip up the checks to make sure that
> the number of segments will fit in the queue ring buffer, resulting in
> commands that are never completed.

Doug,
Which SG_IO code path (via the block layer or the sg driver)
does this happen on?
Looking at the sg driver code, it calls scsi_allocate_request()
prior to calling scsi_do_req(). scsi_allocate _request()
allocates a block of memory starting with a scsi_request
instance followed by a struct request instance; then zeroes the
whole block, etc. nr_hw_segments is a member of struct request.
I would expect that is the same struct request instance that
the LLD ends up seeing. Zero may not be an ideal value but
at least it is not random.

It is not as clear to me what is happening in this regard
when the SG_IO ioctl is used in the block layer. If the
problem is in that path then perhaps a "rq->nr_hw_segment = 0;"
could be placed in block/scsi_ioctl.c::sg_io() to make it
consistent (i.e. zero) with the sg driver path.

If zero in request::nr_hw_segment is invalid, the perhaps
scsi_do_req() could adjust it to something sensible
(sg_tablesize ??).

Doug Gilbert

Re: [Patch] QLogic qla2x00 driver fixes

[Arjan van de Ven]

On Thu, 2005-02-24 at 23:21 -0500, Doug Ledford wrote:
> Don't use cmd->request->nr_hw_segments as it may not be initialized
> (SG_IO in particular bypasses anything that initializes this and just
> uses scsi_do_req to insert a scsi_request directly on the head of the
> queue) 

should we fix that in the SG_IO layer ?

Re: [Patch] QLogic qla2x00 driver fixes

[Jeff Garzik]

Arjan van de Ven wrote:
> On Thu, 2005-02-24 at 23:21 -0500, Doug Ledford wrote:
> 
>>Don't use cmd->request->nr_hw_segments as it may not be initialized
>>(SG_IO in particular bypasses anything that initializes this and just
>>uses scsi_do_req to insert a scsi_request directly on the head of the
>>queue) 
> 
> 
> should we fix that in the SG_IO layer ?

Possibly/probably.

For SCSI drivers specifically, using nr_hw_segments is pointless unless 
cmd->use_sg goes away.  At which point tons of SCSI drivers want 
changing anyway.

	n_sg = dma_map_sg(..., cmd->use_sg, ...)

will always do the right thing, when the cmd contains a scatterlist.

Deviate from the norm and pay the price, really...

	Jeff

Re: [Patch] QLogic qla2x00 driver fixes

[Doug Ledford]

On Fri, 2005-02-25 at 03:38 -0500, Jeff Garzik wrote:
> Arjan van de Ven wrote:
> > On Thu, 2005-02-24 at 23:21 -0500, Doug Ledford wrote:
> > 
> >>Don't use cmd->request->nr_hw_segments as it may not be initialized
> >>(SG_IO in particular bypasses anything that initializes this and just
> >>uses scsi_do_req to insert a scsi_request directly on the head of the
> >>queue) 
> > 
> > 
> > should we fix that in the SG_IO layer ?
> 
> Possibly/probably.

I'm not concerned with it personally.  The only reason that the scsi
layer copies the block layer request struct into the scsi
command/request is so that upon completion it has enough information to
mark blocks as either up to date or not while at the same time allowing
the scsi layer to free the original block request at queue time, not at
completion time.  It was never intended to be used by low level drivers.
And since the scsi layer has several ways of bypassing the block layer
to inject commands directly to devices, trying to catch all those
injection points and make them emulate block layer activity when we
already provide everything a low level driver needs in the scsi command
struct is silly.  Just use what's available, reliable, always
initialized, and intended for you to use (cmd->use_sg) instead of trying
to use a roughly duplicate item from a different abstraction layer that
isn't always involved in commands sent to a low level driver (request-
>nr_hw_segments).

> For SCSI drivers specifically, using nr_hw_segments is pointless unless 
> cmd->use_sg goes away.  At which point tons of SCSI drivers want 
> changing anyway.
> 
> 	n_sg = dma_map_sg(..., cmd->use_sg, ...)
> 
> will always do the right thing, when the cmd contains a scatterlist.
> 
> Deviate from the norm and pay the price, really...

Aye, yup.

-- 
Doug Ledford <dledford@redhat.com>
http://people.redhat.com/dledford

Re: [Patch] QLogic qla2x00 driver fixes

[Patrick Mansfield]

On Fri, Feb 25, 2005 at 06:57:39AM -0500, Doug Ledford wrote:
> On Fri, 2005-02-25 at 03:38 -0500, Jeff Garzik wrote:
> > Arjan van de Ven wrote:
> > > On Thu, 2005-02-24 at 23:21 -0500, Doug Ledford wrote:
> > > 
> > >>Don't use cmd->request->nr_hw_segments as it may not be initialized
> > >>(SG_IO in particular bypasses anything that initializes this and just
> > >>uses scsi_do_req to insert a scsi_request directly on the head of the
> > >>queue) 
> > > 
> > > 
> > > should we fix that in the SG_IO layer ?
> > 
> > Possibly/probably.

Doug,

What kernel did you hit this with? 

And same question as Doug G: is it via sg (not the block SG_IO)? sg uses
scsi_do_req(), block SG_IO doesn't.

Jens sent changes last August or so that fixed SG_IO (not sg) to always
set nr_hw_segments, change should be in 2.6.10. It is not obvious that his
change fixed this, I can't find the changeset or log.

> I'm not concerned with it personally.  The only reason that the scsi
> layer copies the block layer request struct into the scsi
> command/request is so that upon completion it has enough information to
> mark blocks as either up to date or not while at the same time allowing
> the scsi layer to free the original block request at queue time, not at
> completion time.  It was never intended to be used by low level drivers.

In 2.6, we no don't copy the request into the command:

struct scsi_cmnd {
	...
	struct request *request
	...
}

-- Patrick Mansfield

Re: [Patch] QLogic qla2x00 driver fixes

[Luben Tuikov]

On 02/25/05 06:57, Doug Ledford wrote:
> struct is silly.  Just use what's available, reliable, always
> initialized, and intended for you to use (cmd->use_sg) instead of trying
> to use a roughly duplicate item from a different abstraction layer that
> isn't always involved in commands sent to a low level driver (request-

Yes, I agree.

	Luben

Re: [Patch] QLogic qla2x00 driver fixes

[Doug Ledford]

On Fri, 2005-02-25 at 09:02 -0800, Patrick Mansfield wrote:
> On Fri, Feb 25, 2005 at 06:57:39AM -0500, Doug Ledford wrote:
> > On Fri, 2005-02-25 at 03:38 -0500, Jeff Garzik wrote:
> > > Arjan van de Ven wrote:
> > > > On Thu, 2005-02-24 at 23:21 -0500, Doug Ledford wrote:
> > > > 
> > > >>Don't use cmd->request->nr_hw_segments as it may not be initialized
> > > >>(SG_IO in particular bypasses anything that initializes this and just
> > > >>uses scsi_do_req to insert a scsi_request directly on the head of the
> > > >>queue) 
> > > > 
> > > > 
> > > > should we fix that in the SG_IO layer ?
> > > 
> > > Possibly/probably.
> 
> Doug,
> 
> What kernel did you hit this with? 

Our RHEL4 kernel (2.6.9 plus bunches-o'-patches)

> And same question as Doug G: is it via sg (not the block SG_IO)? sg uses
> scsi_do_req(), block SG_IO doesn't.

At the moment, I'm sorting that out.  It's a bit of a black box.  We
have two different loops of commands running on my test machine.  There
are 10 instances of:

while true; do scsiinfo -i /dev/sdf; done 

which sends an INQUIRY request to the target device.  I'm running into a
problem getting an strace from this command, so I'm tell you more when I
can.

The other 10 loops are:

while true; do scsi_id -g -s /block/sdf -p 0x80; done

> Jens sent changes last August or so that fixed SG_IO (not sg) to always
> set nr_hw_segments, change should be in 2.6.10. It is not obvious that his
> change fixed this, I can't find the changeset or log.

Finding out if this is fixed is only greatly complicated by the fact
that there exists a drivers/block/scsi_ioctl.c and
drivers/scsi/scsi_ioctl.c and they both are compiled into the kernel, so
knowing which one is actually being used, codepath wise, can be
confusing.  Then on top of that there's drivers/scsi/sg.c with it's
SG_IO path.  It's not necessarily an easy issue to sort out without that
strace and for some reason strace is locking on the test box I'm
running.

(insert delay as we get the strace issue resolved)

OK, the program that's causing the problem, from what I can tell, is
opening /dev/sdf and then issuing a SCSI_IOCTL_SEND_COMMAND request.
That means we go to scsi/sd.c:sd_ioctl to
block/scsi_ioctl.c:scsi_cmd_ioctl to block/scsi_ioctl.c:sg_scsi_ioctl
and it's on that path that we aren't always getting an initialized
nr_hw_segments.

/me votes for consolidating some of this ioctl mess if at all possible.

-- 
Doug Ledford <dledford@redhat.com>
http://people.redhat.com/dledford