qla4xxx driver resubmission

This is another resubmission of qla4xxx driver for QLogic Corporation's iSCSI HBAs.

The main changes from the previous submission are
- use block layer tagging
- clean up code and remove unused function for firmware pass thru

Please not that this patch is dependant on the previously posted patch titled
[RFC] [PATCH] helper function for retrieving scsi_cmd given host based block layer tag ( http://marc.theaimsgroup.com/?l=linux-scsi&m=115871027627964&w=2 )

The complete mega patch for the driver is at ftp://ftp.qlogic.com/outgoing/linux/iSCSI/upstream/5.00.05b10-k/0000-Initial-Commit-qla4xxx.txt

For ease of comprehension the above patch has been broken down into the following smaller patches: (located at ftp://ftp.qlogic.com/outgoing/linux/iSCSI/upstream/5.00.05b10-k/ )

0001-qla4xxx-driver-defs.diff
0002-qla4xxx-driver-init.diff
0003-qla4xxx-driver-os.diff
0004-qla4xxx-driver-isr.diff
0005-qla4xxx-driver-mbx.diff
0006-qla4xxx-driver-support.diff
0007-qla4xxx-driver-version.diff

All patches are against scsi-misc-2.6.git

Other items which needs to be done are
- Add support for sysfs attributes.
- Userspace support for session creation/deletion for iscsi HBAs.
- Add support for other discovery modes ie iSNS/SLP.

Here are the links to the previous two submissions
http://marc.theaimsgroup.com/?l=linux-scsi&m=115532714515853&w=2

http://groups.google.com/group/open-iscsi/browse_frm/thread/536ce2edf9a60c48?hl=en

I believe block layer tagging was the gating item in the last submission. I would really appreciate if it can be merged upstream.

Regards, David Somayajulu Qlogic Corporation.

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Why does your LLD need to reach up into the block layer to find an i/o

Even if you are using tags as an index into something... I would think

having to go up to the blk layer to retrieve a something that should

already been known lower in the driver leaves room for race

-- james s

Quoted 1 line · David C Somayajulu

This is another resubmission of qla4xxx driver for QLogic

Quoted 6 lines

The main changes from the previous submission are
- use block layer tagging
- clean up code and remove unused function for firmware pass thru

Please not that this patch is dependant on the previously posted

Quoted 1 line

[RFC] [PATCH] helper function for retrieving scsi_cmd given host

Quoted 5 lines

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

The complete mega patch for the driver is at

Quoted 5 lines

For ease of comprehension the above patch has been broken down into the following smaller patches:
(located at

Quoted 16 lines

0001-qla4xxx-driver-defs.diff
0002-qla4xxx-driver-init.diff
0003-qla4xxx-driver-os.diff
0004-qla4xxx-driver-isr.diff
0005-qla4xxx-driver-mbx.diff
0006-qla4xxx-driver-support.diff
0007-qla4xxx-driver-version.diff

All patches are against scsi-misc-2.6.git

Other items which needs to be done are
- Add support for sysfs attributes.
- Userspace support for session creation/deletion for iscsi HBAs.
- Add support for other discovery modes ie iSNS/SLP.

Quoted 4 lines

Here are the links to the previous two submissions
http://marc.theaimsgroup.com/?l=linux-scsi&m=115532714515853&w=2

Quoted 2 lines

I believe block layer tagging was the gating item in the last

Quoted 10 lines

I would really appreciate if it can be merged upstream.

Regards,
David Somayajulu
Qlogic Corporation.

-
To unsubscribe from this list: send the line "unsubscribe

Quoted 3 lines

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 1 line

Why does your LLD need to reach up into the block layer to find an i/o

?
Block layer tagging was the gating item in the previous submission. Going over the conversation on the linux-scsi reflector on STEX driver in this aspect, led us to believe that there should not be any driver level book-keeping of outstanding commands to the HBA. This was the reason for creating scsi_host_find_tag() in scsi_tcq.h (in the patch titled [RFC] [PATCH] helper function for retrieving scsi_cmd given hostbased block layer tag)
http://marc.theaimsgroup.com/?l=linux-scsi&m=115871027627964&w=2

James, would you mind letting us know if block-layer tagging in your opinion simply meant using the scsi_cmd->request->tag and let the driver do the book-keeping of out-standing commands to HBA (like stex driver) or is it along the lines we have implemented.

Quoted 11 lines · David Somayajulu

Quoted 1 line

Why does your LLD need to reach up into the block layer to find an i/o

?
Block layer tagging was the gating item in the previous submission. Going over the conversation on the linux-scsi reflector on STEX driver in this aspect, led us to believe that there should not be any driver level book-keeping of outstanding commands to the HBA. This was the reason for creating scsi_host_find_tag() in scsi_tcq.h (in the patch titled [RFC] [PATCH] helper function for retrieving scsi_cmd given hostbased block layer tag)
http://marc.theaimsgroup.com/?l=linux-scsi&m=115871027627964&w=2

Quoted 4 lines

James, would you mind letting us know if block-layer tagging in your opinion simply meant using the scsi_cmd->request->tag and let the driver do the book-keeping of out-standing commands to HBA (like stex driver) or is it along the lines we have implemented.

Well, in general, I've been working on stuff that has little use for the tags, so they aren't that meaningful. I can certainly see some implementations
that can benefit.

But, on a newish transport like iSCSI, I'm surprised. There's lots of house-keeping that has to be done with resets and task management that makes
me believe it's better/easier with local structures. I also know that I like

to double-check my hardware (based on local structure) rather than blindly trusting job handles. I also believe there will be locking issues or data structures in flight issues between the block layer and LLDD that will be ugly.

I wanted some justification. The new block layer function hasn't been needed
in the past and I thought it a rather odd thing to add.

Why does your LLD need to reach up into the block layer to find an i/o ?

Even if you are using tags as an index into something... I would think that having to go up to the blk layer to retrieve a something that should have already been known lower in the driver leaves room for race conditions.

Quoted 6 lines · James Smart

Why does your LLD need to reach up into the block layer to find an i/o ?

Even if you are using tags as an index into something... I would think that having to go up to the blk layer to retrieve a something that should have already been known lower in the driver leaves room for race conditions.

This is how the blk and scsi tag api work for queue based tagging. We have the scsi_find_tag() function which takes a scsi device and than calls blk_queue_find_tag to get the request. It then does all the magic sdev to request_queue and request to scsi command work and pass the LLD the scsi command for the tag. So we can either add a driver array and do some tag to scsi command or driver stucture mapping or we can use the array already created in the scsi host block queue tag. And as you see Dave's patch did the latter in the spirit of not duplicating what scsi-ml or the block layer already do.

I think there are some basic races though. For example, scsi_request_fn calls blk_queue_start_tag with only the queue lock held and so if the request_fn was called for two devices on the same host at the same time they both could call find_first_zero_bit on the shared bqt->tag_map and end up getting the same tag.

Quoted 17 lines · Mike Christie

Quoted 6 lines · James Smart

Why does your LLD need to reach up into the block layer to find an i/o ?

Even if you are using tags as an index into something... I would think that having to go up to the blk layer to retrieve a something that should have already been known lower in the driver leaves room for race conditions.

This is how the blk and scsi tag api work for queue based tagging. We have the scsi_find_tag() function which takes a scsi device and than calls blk_queue_find_tag to get the request. It then does all the magic sdev to request_queue and request to scsi command work and pass the LLD the scsi command for the tag. So we can either add a driver array and do some tag to scsi command or driver stucture mapping or we can use the array already created in the scsi host block queue tag. And as you see Dave's patch did the latter in the spirit of not duplicating what scsi-ml or the block layer already do.

Quoted 5 lines

I think there are some basic races though. For example, scsi_request_fn calls blk_queue_start_tag with only the queue lock held and so if the request_fn was called for two devices on the same host at the same time they both could call find_first_zero_bit on the shared bqt->tag_map and end up getting the same tag.

Hrmpf good point, I suspect that would be easy enough to fix with just using a

        do {
                tag = ffz_bit(..);
        } while (test_and_set_bit(tag, map);

construct. Agree?

Quoted 27 lines · Jens Axboe

Quoted 17 lines · Mike Christie

Quoted 6 lines · James Smart

Why does your LLD need to reach up into the block layer to find an i/o ?

Even if you are using tags as an index into something... I would think that having to go up to the blk layer to retrieve a something that should have already been known lower in the driver leaves room for race conditions.

This is how the blk and scsi tag api work for queue based tagging. We have the scsi_find_tag() function which takes a scsi device and than calls blk_queue_find_tag to get the request. It then does all the magic sdev to request_queue and request to scsi command work and pass the LLD the scsi command for the tag. So we can either add a driver array and do some tag to scsi command or driver stucture mapping or we can use the array already created in the scsi host block queue tag. And as you see Dave's patch did the latter in the spirit of not duplicating what scsi-ml or the block layer already do.

Quoted 5 lines

I think there are some basic races though. For example, scsi_request_fn calls blk_queue_start_tag with only the queue lock held and so if the request_fn was called for two devices on the same host at the same time they both could call find_first_zero_bit on the shared bqt->tag_map and end up getting the same tag.

Hrmpf good point, I suspect that would be easy enough to fix with just using a

Quoted 6 lines

        do {
                tag = ffz_bit(..);
        } while (test_and_set_bit(tag, map);

construct. Agree?

I think so. I think this is similar to what Dave was going to do too.

Quoted 32 lines · Mike Christie

Quoted 31 lines · Jens Axboe

Quoted 17 lines · Mike Christie

Quoted 6 lines · James Smart

Why does your LLD need to reach up into the block layer to find an i/o ?

Even if you are using tags as an index into something... I would think that having to go up to the blk layer to retrieve a something that should have already been known lower in the driver leaves room for race conditions.

This is how the blk and scsi tag api work for queue based tagging. We have the scsi_find_tag() function which takes a scsi device and than calls blk_queue_find_tag to get the request. It then does all the magic sdev to request_queue and request to scsi command work and pass the LLD the scsi command for the tag. So we can either add a driver array and do some tag to scsi command or driver stucture mapping or we can use the array already created in the scsi host block queue tag. And as you see Dave's patch did the latter in the spirit of not duplicating what scsi-ml or the block layer already do.

Quoted 5 lines

I think there are some basic races though. For example, scsi_request_fn calls blk_queue_start_tag with only the queue lock held and so if the request_fn was called for two devices on the same host at the same time they both could call find_first_zero_bit on the shared bqt->tag_map and end up getting the same tag.

Hrmpf good point, I suspect that would be easy enough to fix with just using a

        do {
                tag = ffz_bit(..);
        } while (test_and_set_bit(tag, map);

Quoted 4 lines

Quoted 2 lines

construct. Agree?

I think so. I think this is similar to what Dave was going to do too.

Already checked in such a fix:

http://git.kernel.dk/?p=linux-2.6-block.git;a=commit;h=7ddbe6863ac4b7a5588aa4a70f262c098f3c469a

-----Original Message-----
From: Jens Axboe [mailto:[email protected]]
Sent: Thursday, September 21, 2006 11:48 AM
To: Mike Christie
Cc: [email protected]; David Somayajulu; James Bottomley;

Maxey; David Wagner; Ravi Anand; Duane Grigsby
Subject: Re: [RFC] [PATCH] qla4xxx driver resubmission

Quoted 5 lines · Jens Axboe

Quoted 4 lines · Mike Christie

Quoted 3 lines · Jens Axboe

Quoted 2 lines · Mike Christie

Quoted 1 line · James Smart

Why does your LLD need to reach up into the block layer to

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Even if you are using tags as an index into something... I

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

having to go up to the blk layer to retrieve a something that

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

already been known lower in the driver leaves room for race

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

This is how the blk and scsi tag api work for queue based

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

have the scsi_find_tag() function which takes a scsi device and

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

calls blk_queue_find_tag to get the request. It then does all

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

sdev to request_queue and request to scsi command work and pass

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

the scsi command for the tag. So we can either add a driver

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

some tag to scsi command or driver stucture mapping or we can

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

array already created in the scsi host block queue tag. And as

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Dave's patch did the latter in the spirit of not duplicating

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

scsi-ml or the block layer already do.

I think there are some basic races though. For example,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

calls blk_queue_start_tag with only the queue lock held and so

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

request_fn was called for two devices on the same host at the

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

they both could call find_first_zero_bit on the shared

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

end up getting the same tag.

Hrmpf good point, I suspect that would be easy enough to fix

Quoted 10 lines

Quoted 10 lines

Quoted 8 lines

using a

        do {
                tag = ffz_bit(..);
        } while (test_and_set_bit(tag, map);

construct. Agree?

I think so. I think this is similar to what Dave was going to do

Quoted 4 lines

Already checked in such a fix:

Quoted 7 lines

-----Original Message-----
From: Jens Axboe [mailto:[email protected]]
Sent: Thursday, September 21, 2006 11:48 AM
To: Mike Christie
Cc: [email protected]; David Somayajulu; James Bottomley;

[email protected]; Doug

Quoted 9 lines

Maxey; David Wagner; Ravi Anand; Duane Grigsby
Subject: Re: [RFC] [PATCH] qla4xxx driver resubmission

Quoted 5 lines · Jens Axboe

Quoted 4 lines · Mike Christie

Quoted 3 lines · Jens Axboe

Quoted 2 lines · Mike Christie

Quoted 1 line · James Smart

Why does your LLD need to reach up into the block layer to

find an i/o ?

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Even if you are using tags as an index into something... I

would think that

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

having to go up to the blk layer to retrieve a something that

should have

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

already been known lower in the driver leaves room for race

conditions.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

This is how the blk and scsi tag api work for queue based

tagging. We

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

have the scsi_find_tag() function which takes a scsi device and

than

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

calls blk_queue_find_tag to get the request. It then does all

the magic

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

sdev to request_queue and request to scsi command work and pass

the LLD

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

the scsi command for the tag. So we can either add a driver

array and do

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

some tag to scsi command or driver stucture mapping or we can

use the

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

array already created in the scsi host block queue tag. And as

you see

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Dave's patch did the latter in the spirit of not duplicating

what

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

scsi-ml or the block layer already do.

I think there are some basic races though. For example,

scsi_request_fn

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

calls blk_queue_start_tag with only the queue lock held and so

if the

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

request_fn was called for two devices on the same host at the

same time

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

they both could call find_first_zero_bit on the shared

bqt->tag_map and

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

end up getting the same tag.

Hrmpf good point, I suspect that would be easy enough to fix

with just

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 8 lines

using a

        do {
                tag = ffz_bit(..);
        } while (test_and_set_bit(tag, map);

construct. Agree?

I think so. I think this is similar to what Dave was going to do

too.

Quoted 4 lines

Quoted 4 lines

Already checked in such a fix:

http://git.kernel.dk/?p=linux-2.6-block.git;a=commit;h=7ddbe6863ac4b7a55
88aa4a70f262c098f3c469a

Thanks. This a lot elegant than the one I came up with using a lock. Jens, any comments on this patch
[RFC] [PATCH] helper function for retrieving scsi_cmd given host based block layer tag
http://marc.theaimsgroup.com/?l=linux-scsi&m=115871027627964&w=2

From: Jens Axboe [mailto:[email protected]]

Quoted 13 lines · David Somayajulu

Quoted 8 lines

From: Jens Axboe [mailto:[email protected]]
Sent: Thursday, September 21, 2006 11:48 AM

Quoted 5 lines · Jens Axboe

Quoted 4 lines · Mike Christie

Quoted 3 lines · Jens Axboe

Quoted 2 lines · Mike Christie

Quoted 1 line · James Smart

Why does your LLD need to reach up into the block layer to

find an i/o ?

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Even if you are using tags as an index into something... I

would think that

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

having to go up to the blk layer to retrieve a something

Quoted 2 lines

should have

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

already been known lower in the driver leaves room for

Quoted 5 lines

conditions.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

This is how the blk and scsi tag api work for queue based

tagging. We

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

have the scsi_find_tag() function which takes a scsi device

Quoted 2 lines

than

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

calls blk_queue_find_tag to get the request. It then does

Quoted 2 lines

the magic

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

sdev to request_queue and request to scsi command work and

Quoted 4 lines

the LLD

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

the scsi command for the tag. So we can either add a driver

array and do

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

some tag to scsi command or driver stucture mapping or we

Quoted 2 lines

use the

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

array already created in the scsi host block queue tag. And

Quoted 2 lines

you see

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Dave's patch did the latter in the spirit of not

Quoted 6 lines

what

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

scsi-ml or the block layer already do.

I think there are some basic races though. For example,

scsi_request_fn

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

calls blk_queue_start_tag with only the queue lock held and

Quoted 2 lines

if the

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

request_fn was called for two devices on the same host at

Quoted 9 lines

same time

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

they both could call find_first_zero_bit on the shared

bqt->tag_map and

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

end up getting the same tag.

Hrmpf good point, I suspect that would be easy enough to fix

with just

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

using a

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

        do {
                tag = ffz_bit(..);
        } while (test_and_set_bit(tag, map);

construct. Agree?

I think so. I think this is similar to what Dave was going to

Quoted 6 lines

too.

Quoted 4 lines

Quoted 4 lines

Already checked in such a fix:

Quoted 5 lines

88aa4a70f262c098f3c469a

Thanks. This a lot elegant than the one I came up with using a lock. Jens, any comments on this patch
[RFC] [PATCH] helper function for retrieving scsi_cmd given host

Quoted 2 lines

block layer tag
http://marc.theaimsgroup.com/?l=linux-scsi&m=115871027627964&w=2

Patch looks very nice to me, I'll apply it and let James scream if he dislikes it for some reason (I don't see why, though).

Once again, we would really appreciate if you could please let us know the status of qla4xxx driver regarding its inclusion upstream.

From: James Bottomley [mailto:[email protected]]
Sent: Wednesday, October 04, 2006 8:50 AM
To: David Somayajulu
Cc: Mike Christie; [email protected]; Christoph Hellwig;

Subject: RE: [RFC] [PATCH] qla4xxx driver resubmission

Quoted 1 line · David Somayajulu

Once again, we would really appreciate if you could please let us

Quoted 1 line

the status of qla4xxx driver regarding its inclusion upstream.

OK, I have this (it won't go to scsi-misc until Jens pushes the block changes on which it depends).

However, there was a tonne of trailing whitespace I had to remove:

Warning: trailing whitespace in line 6 of drivers/scsi/qla4xxx/Kconfig
Warning: trailing whitespace in lines 173,543,544,801,802 of drivers/scsi/qla4xxx/ql4_mbx.c
Warning: trailing whitespace in lines 587,712 of drivers/scsi/qla4xxx/ql4_isr.c
Warning: trailing whitespace in lines 491,896,1147,1176,1439 of drivers/scsi/qla4xxx/ql4_os.c
Warning: trailing whitespace in lines 44,363,481,527,1135 of drivers/scsi/qla4xxx/ql4_init.c

James

Thanks a lot James. My apologies for the inconvenience pertaining to whitespaces. For future, is there any tool out there which one run, to sanitize the code before submission.

Thanks a lot James. My apologies for the inconvenience pertaining to whitespaces. For future, is there any tool out there which one run, to sanitize the code before submission.