[RFC] [PATCH] qla4xxx driver resubmission

[RFC] [PATCH] qla4xxx driver resubmission

[David Somayajulu]

This is a resubmission of the qla4xxx driver for QLogic Corporation's
iSCSI HBAs. Changes from the previous submission are:

- fixed driver initialization issues during insmod
- removed all references to iSNS
- changed comments for function to doc style
- removed potential infinite loop in ql4xxx_sem_spinlock()

The complete patch for the driver is at
ftp://ftp.qlogic.com/outgoing/linux/iSCSI/upstream/5.00.05b8-k/0000-Init
ial-commit-of-qla4xxx.txt


For ease of comprehension the above patch has been broken down into the
following smaller patches:
- 0001-ISP4xxx-driver-definitions.diff
- 0002-ISP4xxx-driver-initialization-routines.diff
- 0003-ISP4xxx-driver-OS-files.diff
- 0004-ISP4xxx-driver-ISR-routines.diff
- 0005-ISP4xxx-driver-Mailbox-routines.diff
- 0006-ISP4xxx-driver-support-routines.diff 
- 0007-ISP4xxx-driver-version.diff
These are located at
ftp://ftp.qlogic.com/outgoing/linux/iSCSI/upstream/5.00.05b8-k/

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 mode ie iSNS/SLP.
 - block layer tagging 
 Patches for these would be submitted once the driver is merged
upstream.

Looking forward to comment's/feedback.

Regards,
David Somayajulu
Qlogic Corporation.

Re: [RFC] [PATCH] qla4xxx driver resubmission

[David C Somayajulu]

On Thu, 2006-07-27 at 11:30 -0700, David Somayajulu wrote:
> This is a resubmission of the qla4xxx driver for QLogic Corporation's
> iSCSI HBAs. Changes from the previous submission are:
> 
> - fixed driver initialization issues during insmod
> - removed all references to iSNS
> - changed comments for function to doc style
> - removed potential infinite loop in ql4xxx_sem_spinlock()
> 
> The complete patch for the driver is at
> ftp://ftp.qlogic.com/outgoing/linux/iSCSI/upstream/5.00.05b8-k/0000-Init
> ial-commit-of-qla4xxx.txt
> 
Sorry, my mailer screwed up the above link. Here it is again
ftp://ftp.qlogic.com/outgoing/linux/iSCSI/upstream/5.00.05b8-k/0000-Initial-commit-of-qla4xxx.txt

> 
> For ease of comprehension the above patch has been broken down into the
> following smaller patches:
> - 0001-ISP4xxx-driver-definitions.diff
> - 0002-ISP4xxx-driver-initialization-routines.diff
> - 0003-ISP4xxx-driver-OS-files.diff
> - 0004-ISP4xxx-driver-ISR-routines.diff
> - 0005-ISP4xxx-driver-Mailbox-routines.diff
> - 0006-ISP4xxx-driver-support-routines.diff 
> - 0007-ISP4xxx-driver-version.diff
> These are located at
> ftp://ftp.qlogic.com/outgoing/linux/iSCSI/upstream/5.00.05b8-k/
> 
> 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 mode ie iSNS/SLP.
>  - block layer tagging 
>  Patches for these would be submitted once the driver is merged
> upstream.
> 
> Looking forward to comment's/feedback.

I appologize for the inconvenience.

Cheers
David Somayajulu
qLogic Corporation.

Re: [RFC] [PATCH] qla4xxx driver resubmission

[Doug Maxey]

On Thu, 27 Jul 2006 12:11:23 PDT, David C Somayajulu wrote:
> On Thu, 2006-07-27 at 11:30 -0700, David Somayajulu wrote:
> > This is a resubmission of the qla4xxx driver for QLogic Corporation's
> > iSCSI HBAs. Changes from the previous submission are:
> > 
> > - fixed driver initialization issues during insmod
> > - removed all references to iSNS
> > - changed comments for function to doc style
> > - removed potential infinite loop in ql4xxx_sem_spinlock()
> > 
> > The complete patch for the driver is at
> > ftp://ftp.qlogic.com/outgoing/linux/iSCSI/upstream/5.00.05b8-k/0000-Init
> > ial-commit-of-qla4xxx.txt
> > 
> Sorry, my mailer screwed up the above link. Here it is again
> ftp://ftp.qlogic.com/outgoing/linux/iSCSI/upstream/5.00.05b8-k/0000-Initial-commit-of-qla4xxx.txt

Will take a look this evening.  Too many other rats...

> 
> > 
> > For ease of comprehension the above patch has been broken down into the
> > following smaller patches:
> > - 0001-ISP4xxx-driver-definitions.diff
> > - 0002-ISP4xxx-driver-initialization-routines.diff
> > - 0003-ISP4xxx-driver-OS-files.diff
> > - 0004-ISP4xxx-driver-ISR-routines.diff
> > - 0005-ISP4xxx-driver-Mailbox-routines.diff
> > - 0006-ISP4xxx-driver-support-routines.diff 
> > - 0007-ISP4xxx-driver-version.diff
> > These are located at
> > ftp://ftp.qlogic.com/outgoing/linux/iSCSI/upstream/5.00.05b8-k/
> > 
> > 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 mode ie iSNS/SLP.
> >  - block layer tagging 
> >  Patches for these would be submitted once the driver is merged
> > upstream.
> > 
> > Looking forward to comment's/feedback.
> 
> I appologize for the inconvenience.
> 
> Cheers
> David Somayajulu
> qLogic Corporation.

[RFC] [PATCH] qla4xxx driver resubmission

[David C Somayajulu]

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. 

Re: [RFC] [PATCH] qla4xxx driver resubmission

[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.

-- james s

David C Somayajulu wrote:
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [RFC] [PATCH] qla4xxx driver resubmission

[David Somayajulu]

> 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.
> 
> 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.
> 
> -- james s
> 
> David C Somayajulu wrote:
> > 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-Ini
tial-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/536ce2edf9a6
0c48?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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >

Re: [RFC] [PATCH] qla4xxx driver resubmission

[James Smart]

David Somayajulu wrote:
>> 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.

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.

-- james

Re: [RFC] [PATCH] qla4xxx driver resubmission

[Mike Christie]

James Smart wrote:
> 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.

Re: [RFC] [PATCH] qla4xxx driver resubmission

[Mike Christie]

James Smart wrote:
> 
> 
> David Somayajulu wrote:
>>> 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.
> 
> 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


The tag and its mapping to the request and scsi command have to exist
for the life of the scsi command. Are you thinking we may have cases
where the LLD releases the scsi command (and then scsi-ml releases the
command and request) but the LLD still needs the local command structure
(some driver specific struct) and some mapping to a tag? If so then the
block layer tagging in general will not work. Once we release the
request then the tag number can be reallocated.


> 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.
> 

It was not needed because stex just preallocated a local array of its
command structs and uses the tag to access that struct. qla4xxx uses a
mempool and so it could allocate a array for the mapping but then we
have something very similar to what the block layer and scsi provide in
its tag_map.

If you do queue based tagging and use the tag number and do not
preallocate your commands then there is a lookup function for you do use.

Re: [RFC] [PATCH] qla4xxx driver resubmission

[Jens Axboe]

On Thu, Sep 21 2006, Mike Christie wrote:
> James Smart wrote:
> > 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.

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?

-- 
Jens Axboe

Re: [RFC] [PATCH] qla4xxx driver resubmission

[Mike Christie]

Jens Axboe wrote:
> On Thu, Sep 21 2006, Mike Christie wrote:
>> James Smart wrote:
>>> 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.
> 
> 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?
> 

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

Re: [RFC] [PATCH] qla4xxx driver resubmission

[Jens Axboe]

On Thu, Sep 21 2006, Mike Christie wrote:
> Jens Axboe wrote:
> > On Thu, Sep 21 2006, Mike Christie wrote:
> >> James Smart wrote:
> >>> 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.
> > 
> > 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?
> > 
> 
> 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

-- 
Jens Axboe

Re: [RFC] [PATCH] qla4xxx driver resubmission

[Jens Axboe]

On Thu, Sep 21 2006, Jens Axboe wrote:
> On Thu, Sep 21 2006, Mike Christie wrote:
> > Jens Axboe wrote:
> > > On Thu, Sep 21 2006, Mike Christie wrote:
> > >> James Smart wrote:
> > >>> 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.
> > > 
> > > 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?
> > > 
> > 
> > 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

BTW, please don't CC closed lists on public lists (the iscsi list spams
me on every reply).

-- 
Jens Axboe

RE: [RFC] [PATCH] qla4xxx driver resubmission

[David Somayajulu]

> -----Original Message-----
> From: Jens Axboe [mailto:axboe@kernel.dk]
> Sent: Thursday, September 21, 2006 11:48 AM
> To: Mike Christie
> Cc: James.Smart@Emulex.Com; David Somayajulu; James Bottomley;
linux-scsi@vger.kernel.org; Doug
> Maxey; David Wagner; Ravi Anand; Duane Grigsby
> Subject: Re: [RFC] [PATCH] qla4xxx driver resubmission
> 
> On Thu, Sep 21 2006, Jens Axboe wrote:
> > On Thu, Sep 21 2006, Mike Christie wrote:
> > > Jens Axboe wrote:
> > > > On Thu, Sep 21 2006, Mike Christie wrote:
> > > >> James Smart wrote:
> > > >>> 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.
> > > >
> > > > 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?
> > > >
> > >
> > > 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=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

Re: [RFC] [PATCH] qla4xxx driver resubmission

[Jens Axboe]

On Thu, Sep 21 2006, David Somayajulu wrote:
> 
> 
> > -----Original Message-----
> > From: Jens Axboe [mailto:axboe@kernel.dk]
> > Sent: Thursday, September 21, 2006 11:48 AM
> > To: Mike Christie
> > Cc: James.Smart@Emulex.Com; David Somayajulu; James Bottomley;
> linux-scsi@vger.kernel.org; Doug
> > Maxey; David Wagner; Ravi Anand; Duane Grigsby
> > Subject: Re: [RFC] [PATCH] qla4xxx driver resubmission
> > 
> > On Thu, Sep 21 2006, Jens Axboe wrote:
> > > On Thu, Sep 21 2006, Mike Christie wrote:
> > > > Jens Axboe wrote:
> > > > > On Thu, Sep 21 2006, Mike Christie wrote:
> > > > >> James Smart wrote:
> > > > >>> 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.
> > > > >
> > > > > 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?
> > > > >
> > > >
> > > > 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=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

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).

-- 
Jens Axboe

RE: [RFC] [PATCH] qla4xxx driver resubmission

[David Somayajulu]

> From: Jens Axboe [mailto:axboe@kernel.dk]
> On Thu, Sep 21 2006, David Somayajulu wrote:
> > > From: Jens Axboe [mailto:axboe@kernel.dk]
> > > Sent: Thursday, September 21, 2006 11:48 AM
> > > On Thu, Sep 21 2006, Jens Axboe wrote:
> > > > On Thu, Sep 21 2006, Mike Christie wrote:
> > > > > Jens Axboe wrote:
> > > > > > On Thu, Sep 21 2006, Mike Christie wrote:
> > > > > >> James Smart wrote:
> > > > > >>> 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.
> > > > > >
> > > > > > 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?
> > > > > >
> > > > >
> > > > > 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=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
> 
> 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).
James and Christoph, 
I would you really appreciate if you could let me know where we stand on
the inclusion of qla4xxx driver upstream. Block layer tagging was the
last gating item in the last round and it has been taken care of. 
Thanks
David Somayajulu

RE: [RFC] [PATCH] qla4xxx driver resubmission

[David Somayajulu]

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

RE: [RFC] [PATCH] qla4xxx driver resubmission

[James Bottomley]

On Fri, 2006-09-29 at 17:42 -0700, David Somayajulu wrote:
> Once again, we would really appreciate if you could please let us know
> 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

RE: [RFC] [PATCH] qla4xxx driver resubmission

[David Somayajulu]

> From: James Bottomley [mailto:James.Bottomley@SteelEye.com]
> Sent: Wednesday, October 04, 2006 8:50 AM
> To: David Somayajulu
> Cc: Mike Christie; linux-scsi@vger.kernel.org; Christoph Hellwig;
David Wagner
> Subject: RE: [RFC] [PATCH] qla4xxx driver resubmission
> 
> On Fri, 2006-09-29 at 17:42 -0700, David Somayajulu wrote:
> > Once again, we would really appreciate if you could please let us
know
> > 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.
-david S.

RE: [RFC] [PATCH] qla4xxx driver resubmission

[James Bottomley]

On Wed, 2006-10-04 at 09:30 -0700, David Somayajulu wrote:
> 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.

Quilt and git both warn about this ... which is how I noticed it in the
first place.

James

Re: [RFC] [PATCH] qla4xxx driver resubmission

[Matthew Wilcox]

On Wed, Oct 04, 2006 at 09:30:07AM -0700, David Somayajulu wrote:
> 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.

grep -n ' \t' drivers/scsi/ncr53c8xx.c