qla2xxx tcq question/bug

qla2xxx tcq question/bug

[Mike Christie]

Hi Andrew,

For the qla2xxx driver in 2.6.7 there is this code:

/* Update tagged queuing modifier */
cmd_pkt->control_flags = __constant_cpu_to_le16(CF_SIMPLE_TAG);
if (cmd->device->tagged_supported) {
	switch (cmd->tag) {
		case HEAD_OF_QUEUE_TAG:
			cmd_pkt->control_flags =
			__constant_cpu_to_le16(CF_HEAD_TAG);
			break;
		case ORDERED_QUEUE_TAG:
			cmd_pkt->control_flags =
			__constant_cpu_to_le16(CF_ORDERED_TAG);
			break;
	}
}

but do you know where/how cmd->tag gets set? It looks like it gets
copied from req->tag in the scsi_prep_fn, but for drivers that do not
call scsi_actiavte_tcq that value never gets touched by the block
layer. Even for drivers that do call scsi_activate_tcq, the req->tag
value is just the tag nr, and those drivers will use scsi_populate_msg
to get the task attribute. req->tag gets copied in the prep_fn and
does not get set until after the request_fn has prepared the request so
really cmd->tag looks like it is junk for every case.

Is this usage from 2.4? Should the qla2xx driver be calling
scsi_activate_tcq/scsi_populate_msg for devices that support tags,
or is that a waste becuase you never use the tag number and all you
really want is the attribute?


Thanks,


-- 
Mike Christie
mikenc@us.ibm.com

Re: qla2xxx tcq question/bug

[Andrew Vasquez]

On Fri, 09 Jul 2004, Mike Christie wrote:

> For the qla2xxx driver in 2.6.7 there is this code:
> 
> /* Update tagged queuing modifier */
> cmd_pkt->control_flags = __constant_cpu_to_le16(CF_SIMPLE_TAG);
> if (cmd->device->tagged_supported) {
> 	switch (cmd->tag) {
> 		case HEAD_OF_QUEUE_TAG:
> 			cmd_pkt->control_flags =
> 			__constant_cpu_to_le16(CF_HEAD_TAG);
> 			break;
> 		case ORDERED_QUEUE_TAG:
> 			cmd_pkt->control_flags =
> 			__constant_cpu_to_le16(CF_ORDERED_TAG);
> 			break;
> 	}
> }
> 
> but do you know where/how cmd->tag gets set? It looks like it gets
> copied from req->tag in the scsi_prep_fn, but for drivers that do not
> call scsi_actiavte_tcq that value never gets touched by the block
> layer. Even for drivers that do call scsi_activate_tcq, the req->tag
> value is just the tag nr, and those drivers will use scsi_populate_msg
> to get the task attribute. req->tag gets copied in the prep_fn and
> does not get set until after the request_fn has prepared the request so
> really cmd->tag looks like it is junk for every case.
> 

The driver's slave_configure() call back will issue a
scsi_adjust_queue_depth() call for all sdev->tagged_supported devices,
relevant code:

	...
	if (sdev->tagged_supported) {
		if (ql2xmaxqdepth != 0 && ql2xmaxqdepth <= 0xffffU)
			queue_depth = ql2xmaxqdepth;

		ql2xmaxqdepth = queue_depth;

		scsi_adjust_queue_depth(sdev, MSG_ORDERED_TAG,
		   queue_depth);
		...

Which is similar to the implementation of scsi_activate_tcq():

	if (sdev->tagged_supported) {
		if (!blk_queue_tagged(sdev->request_queue))
			blk_queue_init_tags(sdev->request_queue,
			   depth, NULL);
		scsi_adjust_queue_depth(sdev, MSG_ORDERED_TAG, depth);
	}

What's missing of course is the blk_queue_init_tags() call.

> Is this usage from 2.4? 
>

Yes, the code can be traced back to our 4.x driver from 2+ years ago.

> Should the qla2xx driver be calling
> scsi_activate_tcq/scsi_populate_msg for devices that support tags,

Could I get a definitive answer on this from the group?  It certainly
does appear that we are missing the block-layer allocation of the tag
map.

> or is that a waste becuase you never use the tag number and all you
> really want is the attribute?
> 

I've taken a look through the ipr code (the only visible caller of
scsi_populate_tag_msg()) -- looks like the qla driver has some
problems here.

--
Andrew Vasquez

Re: qla2xxx tcq question/bug

[Andrew Vasquez]

On Fri, 09 Jul 2004, Andrew Vasquez wrote:

> On Fri, 09 Jul 2004, Mike Christie wrote:
> 
> > Should the qla2xx driver be calling
> > scsi_activate_tcq/scsi_populate_msg for devices that support tags,
> 
> I've taken a look through the ipr code (the only visible caller of
> scsi_populate_tag_msg()) -- looks like the qla driver has some
> problems here.
> 

BTW: 53c700.c appears to be the only scsi driver which calls
scsi_activate_tcq()???

--
Andrew Vasquez

Re: qla2xxx tcq question/bug

[Mike Christie]

Andrew Vasquez wrote:
> I've taken a look through the ipr code (the only visible caller of
> scsi_populate_tag_msg()) -- looks like the qla driver has some
> problems here.

53c700.c uses it too.

The ipr driver actaully has a bug, but Brian was fixing it.

Thanks,

-- 
Mike Christie
mikenc@us.ibm.com

Re: qla2xxx tcq question/bug

[Mike Christie]

Andrew Vasquez wrote:
> On Fri, 09 Jul 2004, Andrew Vasquez wrote:
> 
> 
>>On Fri, 09 Jul 2004, Mike Christie wrote:
>>
>>
>>>Should the qla2xx driver be calling
>>>scsi_activate_tcq/scsi_populate_msg for devices that support tags,
>>
>>I've taken a look through the ipr code (the only visible caller of
>>scsi_populate_tag_msg()) -- looks like the qla driver has some
>>problems here.
>>
> 
> 
> BTW: 53c700.c appears to be the only scsi driver which calls
> scsi_activate_tcq()???

And several other drivers like lpfc and qlogicfc do it your way.


-- 
Mike Christie
mikenc@us.ibm.com

Re: qla2xxx tcq question/bug

[Brian King]

Mike Christie wrote:
> Andrew Vasquez wrote:
> 
>> I've taken a look through the ipr code (the only visible caller of
>> scsi_populate_tag_msg()) -- looks like the qla driver has some
>> problems here.
> 
> 
> 53c700.c uses it too.
> 
> The ipr driver actaully has a bug, but Brian was fixing it.

I'm in the process of adding the scsi_activate_tcq/scsi_deactivate_tcq 
calls to the ipr driver, as Mike mentioned. I figured I would also 
update the API doc to clarify the usage, using the 53c000.c driver as 
the model.


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: qla2xxx tcq question/bug

[James Bottomley]

On Fri, 2004-07-09 at 18:54, Andrew Vasquez wrote:
> BTW: 53c700.c appears to be the only scsi driver which calls
> scsi_activate_tcq()???

That would be because two years ago the scsi maintainer tried to make it
an example to other driver writers of how to use the block layer tag
command queueing.

You could say that was one of the less successful instances of leading
by example...

James

Re: qla2xxx tcq question/bug

[Guennadi Liakhovetski]

On Sat, 9 Jul 2004, James Bottomley wrote:

> On Fri, 2004-07-09 at 18:54, Andrew Vasquez wrote:
>> BTW: 53c700.c appears to be the only scsi driver which calls
>> scsi_activate_tcq()???
>
> That would be because two years ago the scsi maintainer tried to make it
> an example to other driver writers of how to use the block layer tag
> command queueing.
>
> You could say that was one of the less successful instances of leading
> by example...

Wow! I was all this time wondering why the tagged-command queuing was done 
in LLD, and was going to ask if it shouldn't be indeed done 
generically:-)) I know now!:-)

Thanks
Guennadi
---
Guennadi Liakhovetski