Skip to contentSkip to conversation

qla2xxx tcq question/bug

DISCUSSION Mike Christie 5 participants 8 messages Jul 9, 2004 – Jul 11, 2004 SCSI

lore
Share & tools
lore

Mike Christie

author
#0 lore raw

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,

Signature · 2 lines
Mike Christie
[email protected]
#1 lore raw
Quoted 17 lines · Mike Christie

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;
	}
}
Quoted 9 lines

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.

Quoted 2 lines

Is this usage from 2.4?

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

Quoted 2 lines

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.

Quoted 3 lines

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

#2 lore raw
Quoted 9 lines · Andrew Vasquez
Quoted 2 lines · Mike Christie

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

author
#3 lore raw
Quoted 9 lines · Andrew Vasquez
Quoted 6 lines · Andrew Vasquez
Quoted 2 lines · Mike Christie

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

Quoted 8 lines
Quoted 4 lines

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.

Signature · 2 lines
Mike Christie
[email protected]
#4 lore raw
In reply to Andrew Vasquez
Quoted 2 lines · Andrew Vasquez

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

#5 lore raw
Quoted 10 lines · James Bottomley
Quoted 2 lines · Andrew Vasquez

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

author
#6 lore raw
In reply to Andrew Vasquez
Quoted 3 lines · Andrew Vasquez

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,

Signature · 2 lines
Mike Christie
[email protected]
#7 lore raw
Quoted 10 lines · Mike Christie
Quoted 3 lines · Andrew Vasquez

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.

Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center