David C Somayajulu | 1 May 02:36 2008

Re: [PATCH]qla4xxx:Add support for Async Message PDUs [REPOST with fixes]

On Wed, 2008-04-30 at 14:36 -0500, Mike Christie wrote:
> Mike Christie wrote:
> > David C Somayajulu wrote:
> >> +        }
> >> +        if (sshdr.asc == 0x3f && sshdr.ascq == 0x0e) {
> >> +            /* induce rescan */
> >> +            iscsi_block_session(ddb_entry->sess);
> >> +            iscsi_unblock_session(ddb_entry->sess);
> >> +        }
> > 
> > I think I was wrong off list. We could actually just call 
> > iscsi_unblock_session, but we do not know the state of the session do 
> > we? If we got a event that indicated the session was failed could we 
> > have a block queued up and this would reverse it by accident?
> > 
> > I was thinking that because qla4xxx_process_aen is called before this we 
> > could just check the ddb state, but we can call iscsi_block_session from 
> > the interrupt handler to queue up a block so I think that could race 
> > with the unblock call here.
> 
> Maybe to handle the race we do not want to call qla4xxx_mark_device_busy 
> when we get SCS_TIMEOUT. If we get a ddb aen then could we just wait for 
> that and all those events would be serialized through the dpc thread?
> 
> What does SCS_TIMEOUT mean?
I think the driver on receiving the Async Message PDU of type
ISCSI_ASYNC_MSG_SCSI_EVENT, should first retrieve the DDB state. If the
session is still not active, it should block it. If the DDB state
indicates the session is active, then call iscsi_unblock_session() if
rescan is needed. What do you think of this solution ?
(Continue reading)

Pontus Lidman | 1 May 14:54 2008
Picon
Picon
Picon

Hack to not step on vendor commands for some devices

Hi all,

I'm trying to support an usb-storage device that uses vendor-specific
SCSI commands for some functions; I'm using the sg interface from user
space.

The problem is, the linux kernel will replace some bits in the
vendor-specific command with the LUN number, making it unintelligible
to the device. I wrote the included patch that works around this
problem.

cmd->device->scsi_level is SCSI_2 for this device.

Is there any other user-space solution to this problem? If not, what
are the next steps to have a solution appear in the mainline kernel?

Best Regards,

Pontus

--

-- 
Pontus Lidman, pontus <at> lysator.liu.se, Software Engineer

diff --recursive -u linux-2.6.24.5/drivers/scsi/scsi.c linux-2.6.24.5.pl/drivers/scsi/scsi.c
--- linux-2.6.24.5/drivers/scsi/scsi.c	2008-04-19 03:53:39.000000000 +0200
+++ linux-2.6.24.5.pl/drivers/scsi/scsi.c	2008-04-27 17:17:35.000000000 +0200
 <at>  <at>  -501,9 +501,12  <at>  <at> 

 	/* 
 	 * If SCSI-2 or lower, store the LUN value in cmnd.
(Continue reading)

Boaz Harrosh | 1 May 15:47 2008

[PATCH 0/4] Use of new scsi_allocate_command

Submitted are two drivers that should use the new scsi_allocate_command
instead of kmalloc their own.

But first we need some changes. Since the GFP_DMA support is going away
(Submitted next) I would like to change the API a bit before any users
use this code.

Also I would like to snick in a gdth change that is related to this effort.

[PATCH 1/4] scsi_free_command API change - Don't support GFP_DMA
  ISA support is going away and is not needed by any potential clients
  of this code. So change the API now before it has any users.

[PATCH 2/4] isd200: Use new scsi_allocate_command()
  Use scsi_allocate_command in isd200 driver

[PATCH 3/4] gdth: consolidate __gdth_execute && gdth_execute
  Cleanup and optimization. Please see patch for detailed explanations

[PATCH 4/4] gdth: Use scsi_allocate_command for private command allocation

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

Boaz Harrosh | 1 May 15:56 2008

[PATCH 1/4] scsi_free_command API change - Don't support GFP_DMA


Support of GFP_DMA for scsi_cmnd is going away. Don't let new
users get used to it. (None of the potential users need it)
Change that now before it has any users.

Signed-off-by: Boaz Harrosh <bharrosh <at> panasas.com>
---
 drivers/scsi/scsi.c      |   23 +++++++++--------------
 include/scsi/scsi_cmnd.h |    4 ++--
 2 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 749c9c7..a120c04 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
 <at>  <at>  -386,13 +386,13  <at>  <at>  static void scsi_put_host_cmd_pool(gfp_t gfp_mask)

 /**
  * scsi_allocate_command - get a fully allocated SCSI command
- *  <at> gfp_mask:	allocation mask
+ *  <at> gfp_mask:	allocation mask (must not be __GFP_DMA)
  *
  * This function is for use outside of the normal host based pools.
- * It allocates the relevant command and takes an additional reference
- * on the pool it used.  This function *must* be paired with
- * scsi_free_command which also has the identical mask, otherwise the
- * free pool counts will eventually go wrong and you'll trigger a bug.
+ * It allocates a command and takes an additional reference on the
+ * pool. This function *must* be paired with scsi_free_command
+ * otherwise the free pool counts will eventually go wrong and will
(Continue reading)

Boaz Harrosh | 1 May 15:58 2008

[PATCH 2/4] isd200: Use new scsi_allocate_command()


Don't let isd200 struggle with scsi_cmnd allocation and construction.
Use the new scsi_allocate_command()/scsi_free_command() for that.

Signed-off-by: Boaz Harrosh <bharrosh <at> panasas.com>
CC: Alan Stern <stern <at> rowland.harvard.edu>
CC: Matthew Dharm <mdharm-usb <at> one-eyed-alien.net>
---
 drivers/usb/storage/isd200.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index 971d13d..756d30f 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
 <at>  <at>  -292,7 +292,7  <at>  <at>  struct isd200_info {

 	/* maximum number of LUNs supported */
 	unsigned char MaxLUNs;
-	struct scsi_cmnd srb;
+	struct scsi_cmnd *srb;
 	struct scatterlist sg;
 };

 <at>  <at>  -414,7 +414,7  <at>  <at>  static void isd200_build_sense(struct us_data *us, struct scsi_cmnd *srb)
 static void isd200_set_srb(struct isd200_info *info,
 	enum dma_data_direction dir, void* buff, unsigned bufflen)
 {
-	struct scsi_cmnd *srb = &info->srb;
+	struct scsi_cmnd *srb = info->srb;
(Continue reading)

Boaz Harrosh | 1 May 16:02 2008

[PATCH 3/3] gdth: consolidate __gdth_execute && gdth_execute


ioctl into gdth's char device would allocate an host-device at file-open
and would cache that device until the removal of the host, not at file
close. On the other hand a /proc access to gdth, and also flush() call at
host removal, would allocate a new device on every call and destroy it. Even
if one was allocated by file-open.

All this optimization is just a mess. But my primary concern is the construction
of the device just before destruction at flush(), looks extra pointless to me.

So allocate an host-device for every host, and remove it at host removal.
Now the existence of __gdth_execute is pointless and is removed. Also
gdth_execute can operate directly on gdth_ha_str.

Signed-off-by: Boaz Harrosh <bharrosh <at> panasas.com>
---
 drivers/scsi/gdth.c      |   45 +++++++++++++++------------------------------
 drivers/scsi/gdth_proc.c |   16 ++++++++--------
 drivers/scsi/gdth_proc.h |    2 +-
 3 files changed, 24 insertions(+), 39 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index c6d6e7c..35ad3bf 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
 <at>  <at>  -439,10 +439,9  <at>  <at>  static void gdth_scsi_done(struct scsi_cmnd *scp)
 		scp->scsi_done(scp);
 }

-int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
(Continue reading)

Boaz Harrosh | 1 May 16:06 2008

[PATCH 4/4] gdth: Use scsi_allocate_command for private command allocation


In gdth_execute() use scsi_allocate_command for allocation of
a command. To be insulated from future scsi_cmnd construction
considerations.

Signed-off-by: Boaz Harrosh <bharrosh <at> panasas.com>
---
 drivers/scsi/gdth.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index 35ad3bf..7778373 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
 <at>  <at>  -447,16 +447,10  <at>  <at>  int gdth_execute(gdth_ha_str *ha, gdth_cmd_str *gdtcmd, char *cmnd,
     DECLARE_COMPLETION_ONSTACK(wait);
     int rval;

-    scp = kzalloc(sizeof(*scp), GFP_KERNEL);
+    scp = scsi_allocate_command(GFP_KERNEL);
     if (!scp)
         return -ENOMEM;

-    scp->sense_buffer = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
-    if (!scp->sense_buffer) {
-	kfree(scp);
-	return -ENOMEM;
-    }
-
     scp->device = ha->sdev;
(Continue reading)

James Bottomley | 1 May 16:24 2008

Re: [PATCH 0/4] Use of new scsi_allocate_command

On Thu, 2008-05-01 at 16:47 +0300, Boaz Harrosh wrote:
> [PATCH 1/4] scsi_free_command API change - Don't support GFP_DMA
>   ISA support is going away and is not needed by any potential clients
>   of this code. So change the API now before it has any users.

What makes you think this?  We still have lots of ISA installations out
there (and some broken PCI ones requiring ISA DMA masks) that we have to
support.

For gdth you have an ISA board that *you* need to support, so the isa
board has to have this flag.  The reason no-one's run into a bug here is
that the problematic allocation is the sense buffer not the command.
Unfortunately, when the fix was added to the gdth driver for the sense
buffer separation:

commit 1b96f8955aaeeb05f7fb7ff548aa12415fbf3904
Author: Sven Schnelle <svens <at> bitebene.org>
Date:   Mon Mar 10 22:50:04 2008 +0100

    [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference

We actually forgot about this case.  Right at the moment, any ioctl that
uses gdth_execute on an ISA device will probably fail if the executed
command resturns sense and there look to be paths down gdth_execute even
for special commands where the OpCode flips to a regular command (-1)
and so DMAs into the sense buffer.

James

--
(Continue reading)

Boaz Harrosh | 1 May 16:56 2008

Re: [PATCH 0/4] Use of new scsi_allocate_command

On Thu, May 01 2008 at 17:24 +0300, James Bottomley
<James.Bottomley@...> wrote:
> On Thu, 2008-05-01 at 16:47 +0300, Boaz Harrosh wrote:
>> [PATCH 1/4] scsi_free_command API change - Don't support GFP_DMA
>>   ISA support is going away and is not needed by any potential clients
>>   of this code. So change the API now before it has any users.
> 
> What makes you think this?  We still have lots of ISA installations out
> there (and some broken PCI ones requiring ISA DMA masks) that we have to
> support.
> 

Believe me I'm an expert. I'm working on the complete removal of GFP_DMA dma
support, initiated by Andi Kleen. And will submit the complete work on Sunday
once reviewed by Andi and some preliminary test pass.

> For gdth you have an ISA board that *you* need to support, so the isa
> board has to have this flag.  The reason no-one's run into a bug here is
> that the problematic allocation is the sense buffer not the command.
> Unfortunately, when the fix was added to the gdth driver for the sense
> buffer separation:
> 
> commit 1b96f8955aaeeb05f7fb7ff548aa12415fbf3904
> Author: Sven Schnelle <svens@...>
> Date:   Mon Mar 10 22:50:04 2008 +0100
> 
>     [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference
> 
> We actually forgot about this case.  Right at the moment, any ioctl that
> uses gdth_execute on an ISA device will probably fail if the executed
(Continue reading)

Mike Christie | 1 May 16:59 2008
Picon

Re: [PATCH]qla4xxx:Add support for Async Message PDUs [REPOST with fixes]

David C Somayajulu wrote:
> On Wed, 2008-04-30 at 14:36 -0500, Mike Christie wrote:
>> Mike Christie wrote:
>>> David C Somayajulu wrote:
>>>> +        }
>>>> +        if (sshdr.asc == 0x3f && sshdr.ascq == 0x0e) {
>>>> +            /* induce rescan */
>>>> +            iscsi_block_session(ddb_entry->sess);
>>>> +            iscsi_unblock_session(ddb_entry->sess);
>>>> +        }
>>> I think I was wrong off list. We could actually just call 
>>> iscsi_unblock_session, but we do not know the state of the session do 
>>> we? If we got a event that indicated the session was failed could we 
>>> have a block queued up and this would reverse it by accident?
>>>
>>> I was thinking that because qla4xxx_process_aen is called before this we 
>>> could just check the ddb state, but we can call iscsi_block_session from 
>>> the interrupt handler to queue up a block so I think that could race 
>>> with the unblock call here.
>> Maybe to handle the race we do not want to call qla4xxx_mark_device_busy 
>> when we get SCS_TIMEOUT. If we get a ddb aen then could we just wait for 
>> that and all those events would be serialized through the dpc thread?
>>
>> What does SCS_TIMEOUT mean?
> I think the driver on receiving the Async Message PDU of type
> ISCSI_ASYNC_MSG_SCSI_EVENT, should first retrieve the DDB state. If the
> session is still not active, it should block it. If the DDB state

You should not need to block it though. qla4xxx_process_aen will have 
done this already when it set the ddb state, wouldn't it? I am saying if 
(Continue reading)


Gmane