Using scsi_cmd->request->nr_hw_segments within a LLDD...

Using scsi_cmd->request->nr_hw_segments within a LLDD...

[Andrew Vasquez]

All,

Back in March the qla2xxx driver began to use the value in
cmd->request->nr_hw_segments as a guestimate during the calucation of
the number of driver-request-queue entries that would be needed to
fullfill a given command-request.  Here's an URL to thread which
prompted the change:

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

Recently we've had a number of problem reports which appear to
indicate that the value specified within the variable is
un-initialized and/or set to some bogus value.

Here's the relevant code the driver uses in qla2x00_start_scsi():

	/* Calculate the number of request entries needed. */
	req_cnt = (ha->calc_request_entries)(cmd->request->nr_hw_segments);
	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)
			ha->req_q_cnt = cnt - ha->req_ring_index;
		else
			ha->req_q_cnt = ha->request_q_length -
			    (ha->req_ring_index - cnt);
	}
	if (ha->req_q_cnt < (req_cnt + 2))
		goto queuing_error;

The 'req_cnt' derived value in several instances is greater than 4000.

So, can an LLDD depend on the value being set to a proper value
during all I/O submission paths -- BIO as well as IOCTL?

If not, then what should a device driver writer use as an alternative?

Regards,
Andrew Vasquez

Re: Using scsi_cmd->request->nr_hw_segments within a LLDD...

[Jens Axboe]

On Tue, Sep 21 2004, Andrew Vasquez wrote:
> 
> All,
> 
> Back in March the qla2xxx driver began to use the value in
> cmd->request->nr_hw_segments as a guestimate during the calucation of
> the number of driver-request-queue entries that would be needed to
> fullfill a given command-request.  Here's an URL to thread which
> prompted the change:
> 
> 	http://marc.theaimsgroup.com/?l=linux-scsi&m=107940832718154&w=2
> 
> Recently we've had a number of problem reports which appear to
> indicate that the value specified within the variable is
> un-initialized and/or set to some bogus value.
> 
> Here's the relevant code the driver uses in qla2x00_start_scsi():
> 
> 	/* Calculate the number of request entries needed. */
> 	req_cnt = (ha->calc_request_entries)(cmd->request->nr_hw_segments);
> 	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)
> 			ha->req_q_cnt = cnt - ha->req_ring_index;
> 		else
> 			ha->req_q_cnt = ha->request_q_length -
> 			    (ha->req_ring_index - cnt);
> 	}
> 	if (ha->req_q_cnt < (req_cnt + 2))
> 		goto queuing_error;
> 
> The 'req_cnt' derived value in several instances is greater than 4000.
> 
> So, can an LLDD depend on the value being set to a proper value
> during all I/O submission paths -- BIO as well as IOCTL?
> 
> If not, then what should a device driver writer use as an alternative?

Would this by any chance be from request initiated with SG_IO? This
problem should be fixed in 2.6.9-rcX, are you still seeing it?

->nr_hw_segments was not initialized for some paths earlier, so it
contained random crap.

-- 
Jens Axboe

Re: Using scsi_cmd->request->nr_hw_segments within a LLDD...

[Patrick Mansfield]

On Wed, Sep 22, 2004 at 08:57:27AM +0200, Jens Axboe wrote:
> On Tue, Sep 21 2004, Andrew Vasquez wrote:

> Would this by any chance be from request initiated with SG_IO? This
> problem should be fixed in 2.6.9-rcX, are you still seeing it?
> 
> ->nr_hw_segments was not initialized for some paths earlier, so it
> contained random crap.

Jens, 

What code change is it? I thought it was already in 2.6.8.

-- Patrick Mansfield

Re: Using scsi_cmd->request->nr_hw_segments within a LLDD...

[Jens Axboe]

On Wed, Sep 22 2004, Patrick Mansfield wrote:
> On Wed, Sep 22, 2004 at 08:57:27AM +0200, Jens Axboe wrote:
> > On Tue, Sep 21 2004, Andrew Vasquez wrote:
> 
> > Would this by any chance be from request initiated with SG_IO? This
> > problem should be fixed in 2.6.9-rcX, are you still seeing it?
> > 
> > ->nr_hw_segments was not initialized for some paths earlier, so it
> > contained random crap.
> 
> Jens, 
> 
> What code change is it? I thought it was already in 2.6.8.

Yes it was already in 2.6.8, I just wasn't sure. It's the
bio_copy_user() stuff that replaces the kmalloc() approach for unaligned
user data.

-- 
Jens Axboe