Re: tgt infrastructure removal

2014-07-04 Thread Christoph Hellwig
I got an older Ack from Tomo for the removal, and some sort of acks
for the ibmvscsi target removal from Brian, Paolo and Nathan.

Can I get some formal reviews for the code removal patches (1-4) so I
can queue this up for 3.17?

I'll have to redo patch 5 for various changes and will resend it in
time (hopefully after scsi-mq is in).

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


Re: [PATCH 5/5] scsi: remove various exports that were only used by scsi_tgt

2014-07-04 Thread Paolo Bonzini

Il 15/04/2014 12:26, Christoph Hellwig ha scritto:

Signed-off-by: Christoph Hellwig 
---
 drivers/scsi/scsi.c  |   10 +++---
 drivers/scsi/scsi_lib.c  |   10 --
 include/scsi/scsi_cmnd.h |5 -
 include/scsi/scsi_host.h |2 --
 4 files changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 88d46fe..c499172 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -235,7 +235,8 @@ fail:
  * Description: allocate a struct scsi_cmd from host's slab, recycling from the
  *  host's free_list if necessary.
  */
-struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
+static struct scsi_cmnd *
+__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
 {
struct scsi_cmnd *cmd = scsi_host_alloc_command(shost, gfp_mask);

@@ -265,7 +266,6 @@ struct scsi_cmnd *__scsi_get_command(struct Scsi_Host 
*shost, gfp_t gfp_mask)

return cmd;
 }
-EXPORT_SYMBOL_GPL(__scsi_get_command);

 /**
  * scsi_get_command - Allocate and setup a scsi command block
@@ -291,14 +291,13 @@ struct scsi_cmnd *scsi_get_command(struct scsi_device 
*dev, gfp_t gfp_mask)
cmd->jiffies_at_alloc = jiffies;
return cmd;
 }
-EXPORT_SYMBOL(scsi_get_command);

 /**
  * __scsi_put_command - Free a struct scsi_cmnd
  * @shost: dev->host
  * @cmd: Command to free
  */
-void __scsi_put_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
+static void __scsi_put_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
 {
unsigned long flags;

@@ -314,7 +313,6 @@ void __scsi_put_command(struct Scsi_Host *shost, struct 
scsi_cmnd *cmd)
if (likely(cmd != NULL))
scsi_host_free_command(shost, cmd);
 }
-EXPORT_SYMBOL(__scsi_put_command);

 /**
  * scsi_put_command - Free a scsi command block
@@ -338,7 +336,6 @@ void scsi_put_command(struct scsi_cmnd *cmd)

__scsi_put_command(cmd->device->host, cmd);
 }
-EXPORT_SYMBOL(scsi_put_command);

 static struct scsi_host_cmd_pool *
 scsi_find_host_cmd_pool(struct Scsi_Host *shost)
@@ -801,7 +798,6 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
}
scsi_io_completion(cmd, good_bytes);
 }
-EXPORT_SYMBOL(scsi_finish_command);

 /**
  * scsi_adjust_queue_depth - Let low level drivers change a device's queue 
depth
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7fa54fe..609cd61 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -511,6 +511,7 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
scsi_run_queue(sdev->request_queue);
 }

+static void scsi_release_buffers(struct scsi_cmnd *cmd);
 static void __scsi_release_buffers(struct scsi_cmnd *, int);

 /*
@@ -661,11 +662,10 @@ static void __scsi_release_buffers(struct scsi_cmnd *cmd, 
int do_bidi_check)
  * the scatter-gather table, and potentially any bounce
  * buffers.
  */
-void scsi_release_buffers(struct scsi_cmnd *cmd)
+static void scsi_release_buffers(struct scsi_cmnd *cmd)
 {
__scsi_release_buffers(cmd, 1);
 }
-EXPORT_SYMBOL(scsi_release_buffers);

 /**
  * __scsi_error_from_host_byte - translate SCSI error code into errno
@@ -1042,7 +1042,7 @@ static int scsi_init_sgtable(struct request *req, struct 
scsi_data_buffer *sdb,
  * BLKPREP_DEFER if the failure is retryable
  * BLKPREP_KILL if the failure is fatal
  */
-int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
+static int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 {
struct scsi_device *sdev = cmd->device;
struct request *rq = cmd->request;
@@ -1095,7 +1095,6 @@ err_exit:
put_device(&sdev->sdev_gendev);
return error;
 }
-EXPORT_SYMBOL(scsi_init_io);

 static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
struct request *req)
@@ -1649,7 +1648,7 @@ out_delay:
blk_delay_queue(q, SCSI_QUEUE_DELAY);
 }

-u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)
+static u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)
 {
struct device *host_dev;
u64 bounce_limit = 0x;
@@ -1669,7 +1668,6 @@ u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)

return bounce_limit;
 }
-EXPORT_SYMBOL(scsi_calculate_bounce_limit);

 struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
 request_fn_proc *request_fn)
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index dd7c998..a8f3faf 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -140,18 +140,13 @@ static inline struct scsi_driver 
*scsi_cmd_to_driver(struct scsi_cmnd *cmd)
 }

 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
-extern struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *, gfp_t);
 extern void scsi_put_command(struct scsi_cmnd *);
-extern void __scsi_put_command(struct Scsi_Host *, struct scsi_cmnd *);
 e

Re: tgt infrastructure removal

2014-07-04 Thread Paolo Bonzini

Il 04/07/2014 11:36, Christoph Hellwig ha scritto:

I got an older Ack from Tomo for the removal, and some sort of acks
for the ibmvscsi target removal from Brian, Paolo and Nathan.

Can I get some formal reviews for the code removal patches (1-4) so I
can queue this up for 3.17?


Not that there's much to review there. :)

Reviewed-by: Paolo Bonzini 

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


Re: [PATCH] scsi: break from queue depth adjusting loops when device found

2014-07-04 Thread Hannes Reinecke

On 07/03/2014 07:11 PM, Christoph Hellwig wrote:

On Thu, Jul 03, 2014 at 10:05:57AM -0500, Stephen M. Cameron wrote:

From: Stephen M. Cameron 

Don't loop through all the devices even after
finding the one we're looking for


The comments in the code seem to indicate that we want to modify
the queue depth for all LUNs on a given target.

Ccing Mike and Vasu as they wrote this code.


Indeed, that was the idea.
This piece of code tries to keep track of the remote port queue 
depth, which isn't represented at all.
Thing is, each remote target has a target queue depth which can hold 
only so many outstanding SCSI requests. If that is full it'll return 
BUSY for _all_ LUNs served from that port.
And the very _next_ command after the one which filled the target 
queue will get the BUSY status.
So we need to decrease the queue depth on _all_ LUNs here to avoid 
starvation of individual devices.


Hence I guess this is not the correct fix.

Cheers,

Hannes
--
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: tgt infrastructure removal

2014-07-04 Thread Hannes Reinecke

On 07/04/2014 11:52 AM, Paolo Bonzini wrote:

Il 04/07/2014 11:36, Christoph Hellwig ha scritto:

I got an older Ack from Tomo for the removal, and some sort of acks
for the ibmvscsi target removal from Brian, Paolo and Nathan.

Can I get some formal reviews for the code removal patches (1-4) so I
can queue this up for 3.17?


Not that there's much to review there. :)

Reviewed-by: Paolo Bonzini 

And I checked with our partners which might have some interest in 
keeping it. Turns out they haven't.


So:

Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
--
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: how to firing device event when SD card inserting usb card reader

2014-07-04 Thread loody
hi Alan:

2014-06-26 23:57 GMT+08:00 Alan Stern :
> On Thu, 26 Jun 2014, loody wrote:
>
>> hi all:
>> I try below flow:
>> 1. plug in usb card reader
>> 2. wait 2 seconds
>> 3. plug in SD card
>>
>> on ubuntu PC system, the udev can get SD plug in event
>> but on my embedded system, there is no udev plug event when SD plug in.
>>
>> Is this issue related to media change command?
>> if so, should I enable kernel config or do anything in user mode program?
>>
>> thanks for your help in advance,
>
> What does usbmon show?
after we enable CONFIG_SCSI_MULTI_LUN, we can udevent when SD plug in
card reader.

appreciate your kind help,
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 0/2] Fixed for 64bit LUNs

2014-07-04 Thread Hannes Reinecke
Hi Christoph,

here are some more fixes for 64bit LUNs, which should fix the latest
built issues from Fengguang.
Most notably I've introduced a new accessor 'sdev_scsi2lun' for those
devices which are not capable of handling LUNs higher than 255.
And I've removed the custom LUN handling code from ib_srp.

Hannes Reinecke (2):
  Use sdev_scsi2lun for SCSI parallel drivers
  ib_srp: 64bit LUN fixes

 arch/ia64/hp/sim/simscsi.c|  2 +-
 drivers/infiniband/ulp/srp/ib_srp.c   |  9 ++--
 drivers/infiniband/ulp/srpt/ib_srpt.c | 81 +
 drivers/scsi/53c700.c | 12 ++---
 drivers/scsi/BusLogic.c   |  2 +-
 drivers/scsi/NCR5380.c| 32 ++---
 drivers/scsi/NCR53c406a.c |  2 +-
 drivers/scsi/a100u2w.c|  4 +-
 drivers/scsi/advansys.c   | 16 +++
 drivers/scsi/aha152x.c| 13 +++---
 drivers/scsi/aha1542.c|  4 +-
 drivers/scsi/aha1740.c|  2 +-
 drivers/scsi/aic7xxx/aic79xx_osm.c| 19 
 drivers/scsi/aic7xxx/aic79xx_proc.c   |  2 +-
 drivers/scsi/aic7xxx/aic7xxx_osm.c| 23 +-
 drivers/scsi/aic7xxx/aic7xxx_proc.c   |  2 +-
 drivers/scsi/arm/acornscsi.c  | 20 
 drivers/scsi/arm/fas216.c | 15 +++---
 drivers/scsi/arm/queue.c  |  8 ++--
 drivers/scsi/atari_NCR5380.c  | 81 +
 drivers/scsi/atp870u.c|  2 +-
 drivers/scsi/bnx2fc/bnx2fc_io.c   |  4 +-
 drivers/scsi/ch.c | 16 +++
 drivers/scsi/dc395x.c | 73 ++---
 drivers/scsi/eata.c   |  4 +-
 drivers/scsi/eata_pio.c   |  2 +-
 drivers/scsi/esp_scsi.c   |  6 +--
 drivers/scsi/g_NCR5380.c  |  2 +-
 drivers/scsi/gdth.c   |  4 +-
 drivers/scsi/hptiop.c |  6 +--
 drivers/scsi/in2000.c | 36 ---
 drivers/scsi/initio.c |  4 +-
 drivers/scsi/megaraid.c   | 10 ++--
 drivers/scsi/mesh.c   |  6 +--
 drivers/scsi/mvumi.c  |  2 +-
 drivers/scsi/ncr53c8xx.c  | 33 +++---
 drivers/scsi/nsp32.c  | 14 +++---
 drivers/scsi/ps3rom.c |  2 +-
 drivers/scsi/qlogicpti.c  |  6 +--
 drivers/scsi/scsi.c   |  2 +-
 drivers/scsi/stex.c   |  2 +-
 drivers/scsi/sun3_NCR5380.c   | 86 +++
 drivers/scsi/sym53c8xx_2/sym_glue.c   | 19 
 drivers/scsi/sym53c8xx_2/sym_hipd.c   | 10 ++--
 drivers/scsi/tmscsim.c| 12 +++--
 drivers/scsi/u14-34f.c|  6 +--
 drivers/scsi/ufs/ufshcd.c |  4 +-
 drivers/scsi/ultrastor.c  |  2 +-
 drivers/scsi/wd33c93.c| 35 --
 drivers/scsi/wd7000.c |  3 +-
 drivers/staging/rts5208/rtsx.c|  4 +-
 include/scsi/scsi_device.h|  5 ++
 52 files changed, 373 insertions(+), 398 deletions(-)

-- 
1.7.12.4

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


[PATCH 2/2] ib_srp: 64bit LUN fixes

2014-07-04 Thread Hannes Reinecke
SRP is capable of handling 64bit LUNs, so as we now have proper
support for it we can modify the driver to use the standard functions.

Cc: Bart van Assche 
Signed-off-by: Hannes Reinecke 
---
 drivers/infiniband/ulp/srp/ib_srp.c   |  9 ++--
 drivers/infiniband/ulp/srpt/ib_srpt.c | 81 +--
 2 files changed, 7 insertions(+), 83 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c 
b/drivers/infiniband/ulp/srp/ib_srp.c
index e3c2c5b..eb55f25 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1713,7 +1713,8 @@ static void srp_process_aer_req(struct srp_target_port 
*target,
s32 delta = be32_to_cpu(req->req_lim_delta);
 
shost_printk(KERN_ERR, target->scsi_host, PFX
-"ignoring AER for LUN %llu\n", be64_to_cpu(req->lun));
+"ignoring AER for LUN %llu\n",
+scsilun_to_int((struct scsi_lun *)&req->lun));
 
if (srp_response_common(target, delta, &rsp, sizeof rsp))
shost_printk(KERN_ERR, target->scsi_host, PFX
@@ -1887,7 +1888,7 @@ static int srp_queuecommand(struct Scsi_Host *shost, 
struct scsi_cmnd *scmnd)
memset(cmd, 0, sizeof *cmd);
 
cmd->opcode = SRP_CMD;
-   cmd->lun= cpu_to_be64((u64) scmnd->device->lun << 48);
+   int_to_scsilun(scmnd->device->lun, (struct scsi_lun *)&cmd->lun);
cmd->tag= req->index;
memcpy(cmd->cdb, scmnd->cmnd, scmnd->cmd_len);
 
@@ -2305,7 +2306,7 @@ srp_change_queue_depth(struct scsi_device *sdev, int 
qdepth, int reason)
 }
 
 static int srp_send_tsk_mgmt(struct srp_target_port *target,
-u64 req_tag, unsigned int lun, u8 func)
+u64 req_tag, u64 lun, u8 func)
 {
struct srp_rport *rport = target->rport;
struct ib_device *dev = target->srp_host->srp_dev->dev;
@@ -2338,7 +2339,7 @@ static int srp_send_tsk_mgmt(struct srp_target_port 
*target,
memset(tsk_mgmt, 0, sizeof *tsk_mgmt);
 
tsk_mgmt->opcode= SRP_TSK_MGMT;
-   tsk_mgmt->lun   = cpu_to_be64((u64) lun << 48);
+   int_to_scsilun(lun, (struct scsi_lun *)&tsk_mgmt->lun);
tsk_mgmt->tag   = req_tag | SRP_TAG_TSK_MGMT;
tsk_mgmt->tsk_mgmt_func = func;
tsk_mgmt->task_tag  = req_tag;
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c 
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index fe09f27..232c29a 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1601,81 +1601,6 @@ static int srpt_build_tskmgmt_rsp(struct srpt_rdma_ch 
*ch,
return resp_len;
 }
 
-#define NO_SUCH_LUN ((uint64_t)-1LL)
-
-/*
- * SCSI LUN addressing method. See also SAM-2 and the section about
- * eight byte LUNs.
- */
-enum scsi_lun_addr_method {
-   SCSI_LUN_ADDR_METHOD_PERIPHERAL   = 0,
-   SCSI_LUN_ADDR_METHOD_FLAT = 1,
-   SCSI_LUN_ADDR_METHOD_LUN  = 2,
-   SCSI_LUN_ADDR_METHOD_EXTENDED_LUN = 3,
-};
-
-/*
- * srpt_unpack_lun() - Convert from network LUN to linear LUN.
- *
- * Convert an 2-byte, 4-byte, 6-byte or 8-byte LUN structure in network byte
- * order (big endian) to a linear LUN. Supports three LUN addressing methods:
- * peripheral, flat and logical unit. See also SAM-2, section 4.9.4 (page 40).
- */
-static uint64_t srpt_unpack_lun(const uint8_t *lun, int len)
-{
-   uint64_t res = NO_SUCH_LUN;
-   int addressing_method;
-
-   if (unlikely(len < 2)) {
-   printk(KERN_ERR "Illegal LUN length %d, expected 2 bytes or "
-  "more", len);
-   goto out;
-   }
-
-   switch (len) {
-   case 8:
-   if ((*((__be64 *)lun) &
-__constant_cpu_to_be64(0xLL)) != 0)
-   goto out_err;
-   break;
-   case 4:
-   if (*((__be16 *)&lun[2]) != 0)
-   goto out_err;
-   break;
-   case 6:
-   if (*((__be32 *)&lun[2]) != 0)
-   goto out_err;
-   break;
-   case 2:
-   break;
-   default:
-   goto out_err;
-   }
-
-   addressing_method = (*lun) >> 6; /* highest two bits of byte 0 */
-   switch (addressing_method) {
-   case SCSI_LUN_ADDR_METHOD_PERIPHERAL:
-   case SCSI_LUN_ADDR_METHOD_FLAT:
-   case SCSI_LUN_ADDR_METHOD_LUN:
-   res = *(lun + 1) | (((*lun) & 0x3f) << 8);
-   break;
-
-   case SCSI_LUN_ADDR_METHOD_EXTENDED_LUN:
-   default:
-   printk(KERN_ERR "Unimplemented LUN addressing method %u",
-  addressing_method);
-   break;
-   }
-
-out:
-   return res;
-
-out_err:
-   printk(KERN_ERR "Support for multi-level LUNs has not yet been"
-  " implemented");
-   goto out;
-}
-
 static int srpt_check_stop_free(struct se_cmd *cmd)
 {

Re: [PATCH 2/2] ib_srp: 64bit LUN fixes

2014-07-04 Thread Bart Van Assche
On 07/04/14 13:54, Hannes Reinecke wrote:
> SRP is capable of handling 64bit LUNs, so as we now have proper
> support for it we can modify the driver to use the standard functions.
> 
> Cc: Bart van Assche 
> Signed-off-by: Hannes Reinecke 
> ---
>  drivers/infiniband/ulp/srp/ib_srp.c   |  9 ++--
>  drivers/infiniband/ulp/srpt/ib_srpt.c | 81 
> +--
>  2 files changed, 7 insertions(+), 83 deletions(-)

The SRP initiator and target drivers are independent drivers so this
should be two patches instead of one. Furthermore, I do not agree with
introducing a call to int_to_scsilun() in the hot path of the initiator
driver since that causes a small but unnecessary additional overhead.
Please keep in mind that there is no need to repeat the int_to_scsilun()
conversion for every I/O request. Hence my earlier proposal to cache the
int_to_scsilun() result.

Bart.

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


Re: [PATCH 2/2] ib_srp: 64bit LUN fixes

2014-07-04 Thread Hannes Reinecke

On 07/04/2014 02:31 PM, Bart Van Assche wrote:

On 07/04/14 13:54, Hannes Reinecke wrote:

SRP is capable of handling 64bit LUNs, so as we now have proper
support for it we can modify the driver to use the standard functions.

Cc: Bart van Assche 
Signed-off-by: Hannes Reinecke 
---
  drivers/infiniband/ulp/srp/ib_srp.c   |  9 ++--
  drivers/infiniband/ulp/srpt/ib_srpt.c | 81 +--
  2 files changed, 7 insertions(+), 83 deletions(-)


The SRP initiator and target drivers are independent drivers so this
should be two patches instead of one. Furthermore, I do not agree with
introducing a call to int_to_scsilun() in the hot path of the initiator
driver since that causes a small but unnecessary additional overhead.
Please keep in mind that there is no need to repeat the int_to_scsilun()
conversion for every I/O request. Hence my earlier proposal to cache the
int_to_scsilun() result.

What I would like to do is to provide accessor functions for the 
scsi lun; I've started this already for the older SCSI parallel 
drivers (see my earlier patch).
Once everything is moved over to accessors it should be trivial to 
move from the current scsilun_to_int representation in struct 
scsi_device to the 'real' LUN.


Adding another field would be bit of an overkill IMO, seeing that we 
can achieve the very same thing with a bit of code reshuffling.


James, Christoph, what's your opinion on using accessors?
Would solve this issue nicely, and we can also use proper functions 
for HBAs only capable of handling 32bit LUNs.
Drawback is that we would need accessors also for this capable of 
handling full 64bit LUNs, thus potentially incur some overhead there.


Cheers,

Hannes
--
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] Use sdev_scsi2lun for SCSI parallel drivers

2014-07-04 Thread Christoph Hellwig
On Fri, Jul 04, 2014 at 01:54:34PM +0200, Hannes Reinecke wrote:
> SCSI-2 defines only up to 256 LUNs with a flat namespace.
> This patch introduces an accessor 'sdev_scsi2lun' which should be
> used for these drivers to avoid problems when using 64-bit LUNs
> internally.

So what would be set in the upper 48 bits for those drivers?  We receive
the LUN over the wire to start with.

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


Re: [PATCH 2/2] ib_srp: 64bit LUN fixes

2014-07-04 Thread Christoph Hellwig
On Fri, Jul 04, 2014 at 03:01:40PM +0200, Hannes Reinecke wrote:
> What I would like to do is to provide accessor functions for the scsi lun;
> I've started this already for the older SCSI parallel drivers (see my
> earlier patch).
> Once everything is moved over to accessors it should be trivial to move from
> the current scsilun_to_int representation in struct scsi_device to the
> 'real' LUN.
> 
> Adding another field would be bit of an overkill IMO, seeing that we can
> achieve the very same thing with a bit of code reshuffling.
> 
> James, Christoph, what's your opinion on using accessors?
> Would solve this issue nicely, and we can also use proper functions for HBAs
> only capable of handling 32bit LUNs.
> Drawback is that we would need accessors also for this capable of handling
> full 64bit LUNs, thus potentially incur some overhead there.

I think storing the struct scsi_lun in the scsi_device is the right way
to go ahead.  Any "accessors" for 8 or 32-bit LUNs should be simply
enough by just ignoring bits in the array, so there's very little
performance overhead.

If we can get rid of the old scalar LUN field that would be great,
and given that we know the printk format fallout already it doesn't look
like too much work.  Do you want to look into this?

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


Re: [PATCH 1/2] Use sdev_scsi2lun for SCSI parallel drivers

2014-07-04 Thread Hannes Reinecke

On 07/04/2014 03:44 PM, Christoph Hellwig wrote:

On Fri, Jul 04, 2014 at 01:54:34PM +0200, Hannes Reinecke wrote:

SCSI-2 defines only up to 256 LUNs with a flat namespace.
This patch introduces an accessor 'sdev_scsi2lun' which should be
used for these drivers to avoid problems when using 64-bit LUNs
internally.


So what would be set in the upper 48 bits for those drivers?  We receive
the LUN over the wire to start with.

None. All these devices are physically incapable of receiving or 
sending LUNs more that a byte wide.


Cheers,

Hannes
--
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] ib_srp: 64bit LUN fixes

2014-07-04 Thread Hannes Reinecke

On 07/04/2014 03:48 PM, Christoph Hellwig wrote:

On Fri, Jul 04, 2014 at 03:01:40PM +0200, Hannes Reinecke wrote:

What I would like to do is to provide accessor functions for the scsi lun;
I've started this already for the older SCSI parallel drivers (see my
earlier patch).
Once everything is moved over to accessors it should be trivial to move from
the current scsilun_to_int representation in struct scsi_device to the
'real' LUN.

Adding another field would be bit of an overkill IMO, seeing that we can
achieve the very same thing with a bit of code reshuffling.

James, Christoph, what's your opinion on using accessors?
Would solve this issue nicely, and we can also use proper functions for HBAs
only capable of handling 32bit LUNs.
Drawback is that we would need accessors also for this capable of handling
full 64bit LUNs, thus potentially incur some overhead there.


I think storing the struct scsi_lun in the scsi_device is the right way
to go ahead.  Any "accessors" for 8 or 32-bit LUNs should be simply
enough by just ignoring bits in the array, so there's very little
performance overhead.

If we can get rid of the old scalar LUN field that would be great,
and given that we know the printk format fallout already it doesn't look
like too much work.  Do you want to look into this?


Already working on it.

Cheers,

Hannes
--
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


virtio_scsi LUN usage

2014-07-04 Thread Hannes Reinecke

Hi Paolo,

virtio_scsi has this:

static void virtio_scsi_init_hdr(struct virtio_scsi_cmd_req *cmd,
 struct scsi_cmnd *sc)
{
cmd->lun[0] = 1;
cmd->lun[1] = sc->device->id;
cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
cmd->lun[3] = sc->device->lun & 0xff;
cmd->tag = (unsigned long)sc;
cmd->task_attr = VIRTIO_SCSI_S_SIMPLE;
cmd->prio = 0;
cmd->crn = 0;
}

As you might've seen I'm currently working on updating the SCSI 
stack to use 64-bit LUNs.
So while virtio scsi is in principle capable of using 64-bit LUNs, 
this peculiar usage makes it impossible to actually _use_ 64-bit LUNs.
Can't we get rid of this encoding and let the backend provide is 
with correct numbers?


It should work if we were just to implement a REPORT_LUN intercept, 
which sets the correct top two bytes in the command ... with that we 
should be getting only the LUNs for a particular target, and could 
then just use the provided LUN numbers as-is.


Long term I would prefer to have another 'target' field (preferably 
also 64bit wide), which could be used to specify the target 
directly. But that would require a spec change and feature flags 
yadda yadda...


Cheers,

Hannes
--
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] ib_srp: 64bit LUN fixes

2014-07-04 Thread Bart Van Assche
On 07/04/14 16:12, Hannes Reinecke wrote:
> On 07/04/2014 03:48 PM, Christoph Hellwig wrote:
>> I think storing the struct scsi_lun in the scsi_device is the right way
>> to go ahead.  Any "accessors" for 8 or 32-bit LUNs should be simply
>> enough by just ignoring bits in the array, so there's very little
>> performance overhead.
>>
>> If we can get rid of the old scalar LUN field that would be great,
>> and given that we know the printk format fallout already it doesn't look
>> like too much work.  Do you want to look into this?
>
> Already working on it.

That sounds great. Regarding the SRP initiator driver: if the "__be64
lun" declarations in  are changed into "struct scsi_lun lun"
then that allows to encode / decode LUNs inside the SRP initiator
without having to cast the address of these "lun" members into struct
scsi_lun *. In case you would prefer me to have a look into this, please
let me know.

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


Re: [PATCH 2/2] ib_srp: 64bit LUN fixes

2014-07-04 Thread Hannes Reinecke

On 07/04/2014 04:38 PM, Bart Van Assche wrote:

On 07/04/14 16:12, Hannes Reinecke wrote:

On 07/04/2014 03:48 PM, Christoph Hellwig wrote:

I think storing the struct scsi_lun in the scsi_device is the right way
to go ahead.  Any "accessors" for 8 or 32-bit LUNs should be simply
enough by just ignoring bits in the array, so there's very little
performance overhead.

If we can get rid of the old scalar LUN field that would be great,
and given that we know the printk format fallout already it doesn't look
like too much work.  Do you want to look into this?


Already working on it.


That sounds great. Regarding the SRP initiator driver: if the "__be64
lun" declarations in  are changed into "struct scsi_lun lun"
then that allows to encode / decode LUNs inside the SRP initiator
without having to cast the address of these "lun" members into struct
scsi_lun *. In case you would prefer me to have a look into this, please
let me know.


That would be perfect.
Most of the HBAs supporting 64bit LUNs internally are already doing 
this.


Cheers,

Hannes
--
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Some question about usb scsi storage driver

2014-07-04 Thread loody
hi all:
when I trace kernel driver source about usb scsi storage driver, I
have below 2 questions:
1. in sd.c -> static int sd_probe(struct device *dev)
we use below macro to get scsi_device.
   struct scsi_device *sdp = to_scsi_device(dev);

take usb for example, is usb storage driver allocate memory for
scsi_device then calling sd_probe?
I am looking for usb_stor_probe1 and usb_stor_probe2 but not find
where it is.

2. at the end of sd.c -> sd_probe, why we call async_schedule_domain like below
  async_schedule_domain(sd_probe_async, sdkp, &scsi_sd_probe_domain);
to finish scsi device initialization?
Couldn't we put what sd_probe_async directly in sd_probe?
is there any benefit to do so?

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


uas - kernel panic on drive connection

2014-07-04 Thread Jonathan
Beginning with kernel 3.15.1, I am getting hard lockups every time I
connect a drive to my USB 3 HDD dock with ASMedia ASM1051E UASP
compliant chipset. The only way I am able work around this is to set
the quirk to ignore uas for the device. Here are the kernel messages
during connection followed by a backtrace:

[  229.882190] usb 2-1: new SuperSpeed USB device number 3 using
xhci_hcd [  229.908001] usb-storage 2-1:1.0: USB Mass Storage device
detected [  229.908218] usb-storage 2-1:1.0: Quirks match for vid 174c
pid 55aa: 40 [  229.908349] scsi7 : usb-storage 2-1:1.0
[  230.912633] scsi 7:0:0:0: Direct-Access ASMT
2105 0PQ: 0 ANSI: 6 [  230.914818] sd 7:0:0:0: [sdc]
Attached SCSI removable disk [  289.680267] usb 2-1: USB disconnect,
device number 3 [  319.599862] usb 2-1: new SuperSpeed USB device
number 4 using xhci_hcd [  319.626881] scsi8 : uas
[  319.628221] xhci_hcd :02:00.0: ERROR Transfer event TRB DMA ptr
not part of current TD [  320.029323] BUG: unable to handle kernel
paging request at 0103 [  320.029393] IP:
[] kmem_cache_alloc+0x6b/0x170 [  320.029445] PGD
40821a067 PUD 401bbf067 PMD 40411c067 PTE 0 [  320.029505] Oops: 
[#1] PREEMPT SMP [  320.029552] Modules linked in: uas usb_storage
netconsole des_generic ecb md4 nls_utf8 cifs dns_resolver sch_fq_codel
fuse nf_conntrack_ipv4 nf_defrag_ipv4 xt_tcpudp xt_conntrack
nf_conntrack iptable_filter ip_tables x_tables vmnet(O) vsock vmci(O)
vmmon(O) w83627ehf hwmon_vid adm1021 xfs libcrc32c nls_iso8859_1
nls_cp437 vfat fat coretemp hwmon iTCO_wdt intel_rapl
iTCO_vendor_support x86_pkg_temp_thermal intel_powerclamp
crct10dif_pclmul crc32_pclmul ppdev crc32c_intel snd_hda_codec_hdmi
ghash_clmulni_intel aesni_intel snd_hda_codec_realtek
snd_hda_codec_generic aes_x86_64 lrw gf128mul glue_helper ablk_helper
cryptd e1000e hdpvr microcode evdev snd_hda_intel i915 joydev
v4l2_dv_timings ptp mousedev psmouse r8168(O) mac_hid
snd_hda_controller videodev serio_raw pcspkr media snd_hda_codec
i2c_i801 lpc_ich pps_core snd_hwdep drm_kms_helper snd_pcm snd_timer
drm snd battery tpm_tis parport_pc tpm intel_gtt parport i2c_algo_bit
mei_me soundcore mei nuvoton_cir video shpchp button processor nfsd
auth_rpcgss oid_registry nfs_acl nfs lockd fscache sunrpc lirc_zilog(C)
i2c_core lirc_dev rc_core ext4 crc16 mbcache jbd2 hid_generic usbhid
hid sd_mod crc_t10dif crct10dif_common atkbd libps2 ehci_pci xhci_hcd
ehci_hcd firewire_ohci ahci libahci libata firewire_core crc_itu_t
scsi_mod usbcore usb_common i8042 serio [  320.030938] CPU: 0 PID: 782
Comm: mysqld Tainted: G C O  3.15.3-ARCH #1 [  320.030988]
Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z68
Pro3-M, BIOS P2.30 06/29/2012 [  320.031052] task: 8800c9488a30 ti:
8800cebd8000 task.ti: 8800cebd8000 [  320.031103] RIP:
0010:[]  []
kmem_cache_alloc+0x6b/0x170 [  320.031165] RSP: 0018:8800cebdba78
EFLAGS: 00010082 [  320.031203] RAX:  RBX:
a0118140 RCX: 00833300 [  320.031251] RDX:
00833280 RSI: 0020 RDI: a00fd04b
[  320.031298] RBP: 8800cebdbaa8 R08: 00017560 R09:
8803fad57380 [  320.031346] R10: 002f R11:
8803fad5002f R12: 0103 [  320.031394] R13:
0020 R14: 880407af1000 R15: 88040ec01900
[  320.031442] FS:  7f28c2bff700() GS:88041f20()
knlGS: [  320.031496] CS:  0010 DS:  ES:  CR0:
80050033 [  320.031536] CR2: 0103 CR3:
000407474000 CR4: 000407f0 [  320.031584] Stack:
[  320.031602]  a00fd030 a0118140 8803fad57380
0020 [  320.031669]  880407af1000 0020
8800cebdbae0 a00fd04b [  320.031738]  8803c8e93780
880407af1000 880407430968 0084f7e0 [  320.031806] Call
Trace: [  320.031839]  [] ?
scsi_host_alloc_command+0x30/0xd0 [scsi_mod] [  320.031899]
[] scsi_host_alloc_command+0x4b/0xd0 [scsi_mod]
[  320.031956]  [] __scsi_get_command+0x18/0xe0
[scsi_mod] [  320.032009]  []
scsi_get_command+0x1b/0xd0 [scsi_mod] [  320.032064]
[] scsi_get_cmd_from_req+0x74/0xa0 [scsi_mod]
[  320.032121]  [] scsi_setup_fs_cmnd+0x3d/0xb0
[scsi_mod] [  320.032173]  [] sd_prep_fn+0x3b8/0xd30
[sd_mod] [  320.032219]  []
blk_peek_request+0x131/0x280 [  320.032268]  []
scsi_request_fn+0x3e/0x4b0 [scsi_mod] [  320.032315]
[] __blk_run_queue+0x33/0x40 [  320.032356]
[] queue_unplugged+0x2e/0xd0 [  320.032398]
[] blk_flush_plug_list+0x1ed/0x270 [  320.032442]
[] blk_finish_plug+0x14/0x50 [  320.032491]
[] ext4_writepages+0x449/0xd20 [ext4] [  320.032539]
[] do_writepages+0x1e/0x30 [  320.032579]
[] __filemap_fdatawrite_range+0x5d/0x80
[  320.032625]  []
filemap_write_and_wait_range+0x2a/0x70 [  320.032680]
[] ext4_sync_file+0x110/0x370 [ext4] [  320.032726]
[] do_fsync+0x4e/0x80 [  320.032764]
[] SyS_fsync+0x10/0x20 [  320.032804]
[] system_call_fastpath+0x16/0x1b [  320.032846]
Code: 0f 84 c

uas - kernel panic on drive connection

2014-07-04 Thread Jonathan
Beginning with kernel 3.15.1, I am getting hard lockups every time I connect a 
drive to my USB 3 HDD dock with ASMedia ASM1051E UASP compliant chipset. The 
only way I am able work around this is to set the quirk to ignore uas for the 
device. Here are the kernel messages during connection followed by a backtrace:

[  229.882190] usb 2-1: new SuperSpeed USB device number 3 using xhci_hcd
[  229.908001] usb-storage 2-1:1.0: USB Mass Storage device detected
[  229.908218] usb-storage 2-1:1.0: Quirks match for vid 174c pid 55aa: 40
[  229.908349] scsi7 : usb-storage 2-1:1.0
[  230.912633] scsi 7:0:0:0: Direct-Access ASMT 2105 0
PQ: 0 ANSI: 6
[  230.914818] sd 7:0:0:0: [sdc] Attached SCSI removable disk
[  289.680267] usb 2-1: USB disconnect, device number 3
[  319.599862] usb 2-1: new SuperSpeed USB device number 4 using xhci_hcd
[  319.626881] scsi8 : uas
[  319.628221] xhci_hcd :02:00.0: ERROR Transfer event TRB DMA ptr not part 
of current TD
[  320.029323] BUG: unable to handle kernel paging request at 0103
[  320.029393] IP: [] kmem_cache_alloc+0x6b/0x170
[  320.029445] PGD 40821a067 PUD 401bbf067 PMD 40411c067 PTE 0
[  320.029505] Oops:  [#1] PREEMPT SMP 
[  320.029552] Modules linked in: uas usb_storage netconsole des_generic ecb 
md4 nls_utf8 cifs dns_resolver sch_fq_codel fuse nf_conntrack_ipv4 
nf_defrag_ipv4 xt_tcpudp xt_conntrack nf_conntrack iptable_filter ip_tables 
x_tables vmnet(O) vsock vmci(O) vmmon(O) w83627ehf hwmon_vid adm1021 xfs 
libcrc32c nls_iso8859_1 nls_cp437 vfat fat coretemp hwmon iTCO_wdt intel_rapl 
iTCO_vendor_support x86_pkg_temp_thermal intel_powerclamp crct10dif_pclmul 
crc32_pclmul ppdev crc32c_intel snd_hda_codec_hdmi ghash_clmulni_intel 
aesni_intel snd_hda_codec_realtek snd_hda_codec_generic aes_x86_64 lrw gf128mul 
glue_helper ablk_helper cryptd e1000e hdpvr microcode evdev snd_hda_intel i915 
joydev v4l2_dv_timings ptp mousedev psmouse r8168(O) mac_hid snd_hda_controller 
videodev serio_raw pcspkr media snd_hda_codec i2c_i801 lpc_ich pps_core 
snd_hwdep drm_kms_helper snd_pcm snd_timer drm snd battery tpm_tis parport_pc 
tpm intel_gtt parport i2c_algo_bit mei_me soundcore mei nuvoton_cir video 
shpchp button proc
 essor nfsd auth_rpcgss oid_registry nfs_acl nfs lockd fscache sunrpc 
lirc_zilog(C) i2c_core lirc_dev rc_core ext4 crc16 mbcache jbd2 hid_generic 
usbhid hid sd_mod crc_t10dif crct10dif_common atkbd libps2 ehci_pci xhci_hcd 
ehci_hcd firewire_ohci ahci libahci libata firewire_core crc_itu_t scsi_mod 
usbcore usb_common i8042 serio
[  320.030938] CPU: 0 PID: 782 Comm: mysqld Tainted: G C O  3.15.3-ARCH 
#1
[  320.030988] Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./Z68 
Pro3-M, BIOS P2.30 06/29/2012
[  320.031052] task: 8800c9488a30 ti: 8800cebd8000 task.ti: 
8800cebd8000
[  320.031103] RIP: 0010:[]  [] 
kmem_cache_alloc+0x6b/0x170
[  320.031165] RSP: 0018:8800cebdba78  EFLAGS: 00010082
[  320.031203] RAX:  RBX: a0118140 RCX: 00833300
[  320.031251] RDX: 00833280 RSI: 0020 RDI: a00fd04b
[  320.031298] RBP: 8800cebdbaa8 R08: 00017560 R09: 8803fad57380
[  320.031346] R10: 002f R11: 8803fad5002f R12: 0103
[  320.031394] R13: 0020 R14: 880407af1000 R15: 88040ec01900
[  320.031442] FS:  7f28c2bff700() GS:88041f20() 
knlGS:
[  320.031496] CS:  0010 DS:  ES:  CR0: 80050033
[  320.031536] CR2: 0103 CR3: 000407474000 CR4: 000407f0
[  320.031584] Stack:
[  320.031602]  a00fd030 a0118140 8803fad57380 
0020
[  320.031669]  880407af1000 0020 8800cebdbae0 
a00fd04b
[  320.031738]  8803c8e93780 880407af1000 880407430968 
0084f7e0
[  320.031806] Call Trace:
[  320.031839]  [] ? scsi_host_alloc_command+0x30/0xd0 
[scsi_mod]
[  320.031899]  [] scsi_host_alloc_command+0x4b/0xd0 
[scsi_mod]
[  320.031956]  [] __scsi_get_command+0x18/0xe0 [scsi_mod]
[  320.032009]  [] scsi_get_command+0x1b/0xd0 [scsi_mod]
[  320.032064]  [] scsi_get_cmd_from_req+0x74/0xa0 [scsi_mod]
[  320.032121]  [] scsi_setup_fs_cmnd+0x3d/0xb0 [scsi_mod]
[  320.032173]  [] sd_prep_fn+0x3b8/0xd30 [sd_mod]
[  320.032219]  [] blk_peek_request+0x131/0x280
[  320.032268]  [] scsi_request_fn+0x3e/0x4b0 [scsi_mod]
[  320.032315]  [] __blk_run_queue+0x33/0x40
[  320.032356]  [] queue_unplugged+0x2e/0xd0
[  320.032398]  [] blk_flush_plug_list+0x1ed/0x270
[  320.032442]  [] blk_finish_plug+0x14/0x50
[  320.032491]  [] ext4_writepages+0x449/0xd20 [ext4]
[  320.032539]  [] do_writepages+0x1e/0x30
[  320.032579]  [] __filemap_fdatawrite_range+0x5d/0x80
[  320.032625]  [] filemap_write_and_wait_range+0x2a/0x70
[  320.032680]  [] ext4_sync_file+0x110/0x370 [ext4]
[  320.032726]  [] do_fsync+0x4e/0x80
[  320.032764]  [] SyS_fsync+0x10/0x20
[  320.032804]  [] system

Re: Some question about usb scsi storage driver

2014-07-04 Thread Maurizio Lombardi


On 07/04/2014 04:44 PM, loody wrote:
> 
> 2. at the end of sd.c -> sd_probe, why we call async_schedule_domain like 
> below
>   async_schedule_domain(sd_probe_async, sdkp, &scsi_sd_probe_domain);
> to finish scsi device initialization?
> Couldn't we put what sd_probe_async directly in sd_probe?
> is there any benefit to do so?

A possible explanation may be that sd_probe_async() calls sd_spinup_disk(), 
this function spins up the drive and
may block for some seconds, so it is better to do that asynchronously.

Regards,
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Some question about usb scsi storage driver

2014-07-04 Thread Alan Stern
On Fri, 4 Jul 2014, loody wrote:

> hi all:
> when I trace kernel driver source about usb scsi storage driver, I
> have below 2 questions:
> 1. in sd.c -> static int sd_probe(struct device *dev)
> we use below macro to get scsi_device.
>struct scsi_device *sdp = to_scsi_device(dev);
> 
> take usb for example, is usb storage driver allocate memory for
> scsi_device then calling sd_probe?

No.

> I am looking for usb_stor_probe1 and usb_stor_probe2 but not find
> where it is.

Because it's not there.  The memory is allocated by 
drivers/scsi/scsi_scan.c:scsi_alloc_sdev().

> 2. at the end of sd.c -> sd_probe, why we call async_schedule_domain like 
> below
>   async_schedule_domain(sd_probe_async, sdkp, &scsi_sd_probe_domain);
> to finish scsi device initialization?
> Couldn't we put what sd_probe_async directly in sd_probe?
> is there any benefit to do so?

Like Maurizio said, the benefit is that the probe routine will return
quickly so that other devices can be probed while the drive spins up.

Alan Stern

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


Re: [PATCH 2/2] ib_srp: 64bit LUN fixes

2014-07-04 Thread Bart Van Assche
On 07/04/14 16:41, Hannes Reinecke wrote:
> On 07/04/2014 04:38 PM, Bart Van Assche wrote:
>> On 07/04/14 16:12, Hannes Reinecke wrote:
>>> On 07/04/2014 03:48 PM, Christoph Hellwig wrote:
 I think storing the struct scsi_lun in the scsi_device is the right way
 to go ahead.  Any "accessors" for 8 or 32-bit LUNs should be simply
 enough by just ignoring bits in the array, so there's very little
 performance overhead.

 If we can get rid of the old scalar LUN field that would be great,
 and given that we know the printk format fallout already it doesn't
 look
 like too much work.  Do you want to look into this?
>>>
>>> Already working on it.
>>
>> That sounds great. Regarding the SRP initiator driver: if the "__be64
>> lun" declarations in  are changed into "struct scsi_lun lun"
>> then that allows to encode / decode LUNs inside the SRP initiator
>> without having to cast the address of these "lun" members into struct
>> scsi_lun *. In case you would prefer me to have a look into this, please
>> let me know.
>>
> That would be perfect.
> Most of the HBAs supporting 64bit LUNs internally are already doing this.

Here it is (compile tested only on x86 and ppc - will do more testing
once the entire series is available). Please CC all maintainers and
relevant mailing lists for the next version of the 64-bit LUN patch
series. Several IB/SRP users are reading the linux-rdma mailing list
but not the linux-scsi mailing list.


From: Bart Van Assche 
Date: Fri, 4 Jul 2014 17:11:31 +0200
Subject: [PATCH] SRP: Convert lun into struct scsi_lun

Checked the following structure sizes with gdb:
* sizeof(struct srp_cmd) = 48
* sizeof(struct srp_tsk_mgmt) = 48
* sizeof(struct srp_aer_req) = 36

The ibmvscsi changes have been compile tested only (on a PPC system).

Signed-off-by: Bart Van Assche 
---
 drivers/infiniband/ulp/srp/ib_srp.c   |  6 ++--
 drivers/infiniband/ulp/srpt/ib_srpt.c | 68 ++-
 drivers/scsi/ibmvscsi/ibmvscsi.c  |  6 ++--
 drivers/scsi/libsrp.c |  7 ++--
 include/scsi/srp.h|  6 ++--
 5 files changed, 14 insertions(+), 79 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c 
b/drivers/infiniband/ulp/srp/ib_srp.c
index b017a3a..a04f245 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1719,7 +1719,7 @@ static void srp_process_aer_req(struct srp_target_port 
*target,
s32 delta = be32_to_cpu(req->req_lim_delta);
 
shost_printk(KERN_ERR, target->scsi_host, PFX
-"ignoring AER for LUN %llu\n", be64_to_cpu(req->lun));
+"ignoring AER for LUN %u\n", scsilun_to_int(&req->lun));
 
if (srp_response_common(target, delta, &rsp, sizeof rsp))
shost_printk(KERN_ERR, target->scsi_host, PFX
@@ -1914,7 +1914,7 @@ static int srp_queuecommand(struct Scsi_Host *shost, 
struct scsi_cmnd *scmnd)
memset(cmd, 0, sizeof *cmd);
 
cmd->opcode = SRP_CMD;
-   cmd->lun= cpu_to_be64((u64) scmnd->device->lun << 48);
+   int_to_scsilun(scmnd->device->lun, &cmd->lun);
cmd->tag= req->index;
memcpy(cmd->cdb, scmnd->cmnd, scmnd->cmd_len);
 
@@ -2365,7 +2365,7 @@ static int srp_send_tsk_mgmt(struct srp_target_port 
*target,
memset(tsk_mgmt, 0, sizeof *tsk_mgmt);
 
tsk_mgmt->opcode= SRP_TSK_MGMT;
-   tsk_mgmt->lun   = cpu_to_be64((u64) lun << 48);
+   int_to_scsilun(lun, &tsk_mgmt->lun);
tsk_mgmt->tag   = req_tag | SRP_TAG_TSK_MGMT;
tsk_mgmt->tsk_mgmt_func = func;
tsk_mgmt->task_tag  = req_tag;
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c 
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index fe09f27..8a52cec 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1614,68 +1614,6 @@ enum scsi_lun_addr_method {
SCSI_LUN_ADDR_METHOD_EXTENDED_LUN = 3,
 };
 
-/*
- * srpt_unpack_lun() - Convert from network LUN to linear LUN.
- *
- * Convert an 2-byte, 4-byte, 6-byte or 8-byte LUN structure in network byte
- * order (big endian) to a linear LUN. Supports three LUN addressing methods:
- * peripheral, flat and logical unit. See also SAM-2, section 4.9.4 (page 40).
- */
-static uint64_t srpt_unpack_lun(const uint8_t *lun, int len)
-{
-   uint64_t res = NO_SUCH_LUN;
-   int addressing_method;
-
-   if (unlikely(len < 2)) {
-   printk(KERN_ERR "Illegal LUN length %d, expected 2 bytes or "
-  "more", len);
-   goto out;
-   }
-
-   switch (len) {
-   case 8:
-   if ((*((__be64 *)lun) &
-__constant_cpu_to_be64(0xLL)) != 0)
-   goto out_err;
-   break;
-   case 4:
-   if (*((__be16 *)&lun[2]) != 0)
-   goto out_err;
-   break;
-   case 6:
-   if 

[PATCH] qla4xxx: Return -ENOMEM on memory allocation failure

2014-07-04 Thread Himangi Saraogi
In this code, 0 is returned on memory allocation failure, even though
other failures return -ENOMEM or other similar values.

A simplified version of the Coccinelle semantic match that finds this
problem is as follows:

// 
@@
expression ret;
expression x,e1,e2,e3;
identifier alloc;
@@

ret = 0
... when != ret = e1
*x = alloc(...)
... when != ret = e2
if (x == NULL) { ... when != ret = e3
  return ret;
}
// 

Signed-off-by: Himangi Saraogi 
Acked-by: Julia Lawall 
---
 drivers/scsi/qla4xxx/ql4_os.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index c5d9564..72ba671 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -1050,6 +1050,7 @@ static int qla4xxx_get_host_stats(struct Scsi_Host 
*shost, char *buf, int len)
if (!ql_iscsi_stats) {
ql4_printk(KERN_ERR, ha,
   "Unable to allocate memory for iscsi stats\n");
+   ret = -ENOMEM;
goto exit_host_stats;
}
 
-- 
1.9.1

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


Re: [PATCH] scsi: break from queue depth adjusting loops when device found

2014-07-04 Thread Douglas Gilbert

On 14-07-03 01:21 PM, Mike Christie wrote:

On 07/03/2014 12:11 PM, Christoph Hellwig wrote:

On Thu, Jul 03, 2014 at 10:05:57AM -0500, Stephen M. Cameron wrote:

From: Stephen M. Cameron 

Don't loop through all the devices even after
finding the one we're looking for


The comments in the code seem to indicate that we want to modify
the queue depth for all LUNs on a given target.

Ccing Mike and Vasu as they wrote this code.



Ccing James Smart for more details.

We copied this behavior from lpfc's SAM_STAT_TASK_SET_FULL handling
(look at around the 2.6.32 kernel for reference).


Perhaps there should be a knob to control this, maybe within
the scope of a host. I have one RAID controller that has a
JBOD mode in which real disks are presented as LUs in the same
target. Obviously the TSF (queue_type and other) capabilities
of those real disks might differ.

Doug Gilbert


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


[PATCH 1/1] aic79xx: replace kmalloc/memset 0 by kzalloc

2014-07-04 Thread Fabian Frederick
Cc: Hannes Reinecke 
Cc: "James E.J. Bottomley" 
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Fabian Frederick 
---
 drivers/scsi/aic7xxx/aic79xx_core.c | 3 +--
 drivers/scsi/aic7xxx/aic79xx_osm.c  | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/aic7xxx/aic79xx_core.c 
b/drivers/scsi/aic7xxx/aic79xx_core.c
index 0bcacf7..9ce383c 100644
--- a/drivers/scsi/aic7xxx/aic79xx_core.c
+++ b/drivers/scsi/aic7xxx/aic79xx_core.c
@@ -10437,14 +10437,13 @@ ahd_handle_en_lun(struct ahd_softc *ahd, struct 
cam_sim *sim, union ccb *ccb)
return;
}
}
-   lstate = kmalloc(sizeof(*lstate), GFP_ATOMIC);
+   lstate = kzalloc(sizeof(*lstate), GFP_ATOMIC);
if (lstate == NULL) {
xpt_print_path(ccb->ccb_h.path);
printk("Couldn't allocate lstate\n");
ccb->ccb_h.status = CAM_RESRC_UNAVAIL;
return;
}
-   memset(lstate, 0, sizeof(*lstate));
status = xpt_create_path(&lstate->path, /*periph*/NULL,
 xpt_path_path_id(ccb->ccb_h.path),
 xpt_path_target_id(ccb->ccb_h.path),
diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c 
b/drivers/scsi/aic7xxx/aic79xx_osm.c
index 69d5c43..76b5fec 100644
--- a/drivers/scsi/aic7xxx/aic79xx_osm.c
+++ b/drivers/scsi/aic7xxx/aic79xx_osm.c
@@ -1325,10 +1325,9 @@ int
 ahd_platform_alloc(struct ahd_softc *ahd, void *platform_arg)
 {
ahd->platform_data =
-   kmalloc(sizeof(struct ahd_platform_data), GFP_ATOMIC);
+   kzalloc(sizeof(struct ahd_platform_data), GFP_ATOMIC);
if (ahd->platform_data == NULL)
return (ENOMEM);
-   memset(ahd->platform_data, 0, sizeof(struct ahd_platform_data));
ahd->platform_data->irq = AHD_LINUX_NOIRQ;
ahd_lockinit(ahd);
ahd->seltime = (aic79xx_seltime & 0x3) << 4;
-- 
1.9.1

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


RE: [PATCH] scsi: break from queue depth adjusting loops when device found

2014-07-04 Thread Elliott, Robert (Server Storage)


> -Original Message-
> From: Hannes Reinecke [mailto:h...@suse.de]
> Sent: Friday, 04 July, 2014 5:53 AM
> To: Christoph Hellwig; Stephen M. Cameron
> Cc: james.bottom...@parallels.com; Elliott, Robert (Server Storage);
> stephenmcame...@gmail.com; linux-scsi@vger.kernel.org; Vasu Dev; Mike
> Christie
> Subject: Re: [PATCH] scsi: break from queue depth adjusting loops when device
> found
> 
> On 07/03/2014 07:11 PM, Christoph Hellwig wrote:
> > On Thu, Jul 03, 2014 at 10:05:57AM -0500, Stephen M. Cameron wrote:
> >> From: Stephen M. Cameron 
> >>
> >> Don't loop through all the devices even after
> >> finding the one we're looking for
> >
> > The comments in the code seem to indicate that we want to modify
> > the queue depth for all LUNs on a given target.
> >
> > Ccing Mike and Vasu as they wrote this code.
> >
> Indeed, that was the idea.
> This piece of code tries to keep track of the remote port queue
> depth, which isn't represented at all.
> Thing is, each remote target has a target queue depth which can hold
> only so many outstanding SCSI requests. If that is full it'll return
> BUSY for _all_ LUNs served from that port.
> And the very _next_ command after the one which filled the target
> queue will get the BUSY status.
> So we need to decrease the queue depth on _all_ LUNs here to avoid
> starvation of individual devices.
> 
> Hence I guess this is not the correct fix.

Some SCSI target device designs work that way, but others don't. 

In the original SCSI Architecture Model (SAM-1; 1995):
* BUSY meant "the logical unit is busy" 
* TASK SET FULL meant "the logical unit does not have enough 
  resources" 

It didn't say "SCSI target port" or "SCSI target device", so
they were not intended to be affected by other logical units.
That was based on the idealized notion that each I_T_L nexus was 
independent, also assumed by the hierarchical LUN addressing 
scheme and the SCSI Controller Commands (SCC) standard.

With that interpretation, the command identifier/tag (the Q in 
I_T_L_Q nexus) is independent for each I_T_L nexus - two 
logical units behind the same SCSI target port could use the 
same tag value at the same time for different commands.

However, every SCSI transport protocol after parallel SCSI
has chosen to share command tags across all logical units; 
passing the (usually) 8-byte LUN field in every IU/frame is 
not practical.  So, that idealized I_T_L nexus concept has
broken down.  Implementations really have a mix of SCSI target 
device, SCSI target port, and logical unit resources.

When we added the optional status qualifier to SAM-4 (2006), 
we added a SCOPE field indicating if the scope of the BUSY or 
TASK SET FULL is for:
* just the logical unit;
* all logical units in the SCSI target device accessible through 
  the SCSI target port; or
* all logical units in the SCSI target device.

There is no VPD page field defined to report which scope(s) are
implemented or describing the resource limits.  Some designs
are simple, others are complicated.

If TASK SET FULL means a SCSI target port limit has been 
reached, then decrementing the limit on all the logical 
units will over-correct.  If the limit is 256 and there
are 16 LUNs, nominally 16 per LUN, and you send a 257th
command, then the total will drop to 16x15=240, not 256.

mpt3sas assigns separate target numbers for target ports it
discovers, so the SCSI midlayer queue depth logic is more 
correct if those SCSI target devices are implementing a 
SCSI target port scope for BUSY and TASK SET FULL, but 
incorrect if they are implementing a logical unit scope.

[2.711047] scsi 0:0:0:0: Direct-Access HP   EO0400JDVFB  HPD1 
PQ: 0 ANSI: 6
[2.711746] scsi 0:0:1:0: Direct-Access HP   EO0400JDVFB  HPD1 
PQ: 0 ANSI: 6
[2.712423] scsi 0:0:2:0: Direct-Access HP   EO0400JDVFB  HPD1 
PQ: 0 ANSI: 6

hpsa presents multiple LUNs in one target, and a TASK SET FULL 
on one logical unit doesn't mean that you'll get it on the 
others, so the SCSI midlayer queue depth logic is incorrect:
[   10.790885] scsi 2:0:0:0: Direct-Access HP   LOGICAL VOLUME   10.0 
PQ: 0 ANSI: 5
[   10.791095] scsi 2:0:0:1: Direct-Access HP   LOGICAL VOLUME   10.0 
PQ: 0 ANSI: 5
[   10.791300] scsi 2:0:0:2: Direct-Access HP   LOGICAL VOLUME   10.0 
PQ: 0 ANSI: 5

The SCSI host template .cmd_per_lun value, which is used
to set the default initial queue depth for each device 
(i.e., each LUN), implies the logical unit scope. 

Perhaps the SCSI midlayer should keep track of both
SCSI target port and logical unit queue depths, parse
the status qualifier if present, and let the host
template advise on the policy to assume if the
status qualifier is not present.

Short of that, I think it's best to assume logical unit
scope, which always the presumption in SCSI, for BUSY
and TASK SET FULL.

There is some code for a scsi_target structure that I 
don't understand and have just been ignoring: target_bus

Re: [PATCH 1/2] Use sdev_scsi2lun for SCSI parallel drivers

2014-07-04 Thread Douglas Gilbert

On 14-07-04 07:54 AM, Hannes Reinecke wrote:

SCSI-2 defines only up to 256 LUNs with a flat namespace.


My SCSI-2 draft (revision 10b from August 1989) only has
3 bit LUNs placed in byte 1 of the command block, in the top
3 bits. There are also things called LUNTARs and LUNTRNs
at the message level.

8 bit LUNs do ring a distant bell, perhaps after SCSI-2?

Doug Gilbert

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


RE: [PATCH] qla4xxx: Return -ENOMEM on memory allocation failure

2014-07-04 Thread Elliott, Robert (Server Storage)


> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of Himangi Saraogi
> Sent: Friday, 04 July, 2014 1:28 PM
> To: Vikas Chaudhary; iscsi-dri...@qlogic.com; James E.J. Bottomley; linux-
> s...@vger.kernel.org; linux-ker...@vger.kernel.org
> Cc: julia.law...@lip6.fr
> Subject: [PATCH] qla4xxx: Return -ENOMEM on memory allocation failure
> 
> In this code, 0 is returned on memory allocation failure, even though
> other failures return -ENOMEM or other similar values.
> 
> A simplified version of the Coccinelle semantic match that finds this
> problem is as follows:
> 
> // 
> @@
> expression ret;
> expression x,e1,e2,e3;
> identifier alloc;
> @@
> 
> ret = 0
> ... when != ret = e1
> *x = alloc(...)
> ... when != ret = e2
> if (x == NULL) { ... when != ret = e3
>   return ret;
> }
> // 
> 
> Signed-off-by: Himangi Saraogi 
> Acked-by: Julia Lawall 
> ---
>  drivers/scsi/qla4xxx/ql4_os.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
> index c5d9564..72ba671 100644
> --- a/drivers/scsi/qla4xxx/ql4_os.c
> +++ b/drivers/scsi/qla4xxx/ql4_os.c
> @@ -1050,6 +1050,7 @@ static int qla4xxx_get_host_stats(struct Scsi_Host
> *shost, char *buf, int len)
>   if (!ql_iscsi_stats) {
>   ql4_printk(KERN_ERR, ha,
>  "Unable to allocate memory for iscsi stats\n");
> + ret = -ENOMEM;
>   goto exit_host_stats;
>   }
> 

Also, the only caller of this function doesn't use the return 
value - it's overwritten by another function call:

drivers/scsi/scsi_transport_iscsi.c:
err = transport->get_host_stats(shost, buf, host_stats_size);

actual_size = nlmsg_total_size(sizeof(*ev) + host_stats_size);
skb_trim(skbhost_stats, NLMSG_ALIGN(actual_size));
nlhhost_stats->nlmsg_len = actual_size;

err = iscsi_multicast_skb(skbhost_stats, ISCSI_NL_GRP_ISCSID,
  GFP_KERNEL);


---
Rob ElliottHP Server Storage



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


RE: [PATCH 1/2] Use sdev_scsi2lun for SCSI parallel drivers

2014-07-04 Thread Elliott, Robert (Server Storage)


> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of Douglas Gilbert
> Sent: Friday, 04 July, 2014 2:44 PM
> To: Hannes Reinecke; James Bottomley
> Cc: Christoph Hellwig; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 1/2] Use sdev_scsi2lun for SCSI parallel drivers
> 
> On 14-07-04 07:54 AM, Hannes Reinecke wrote:
> > SCSI-2 defines only up to 256 LUNs with a flat namespace.
> 
> My SCSI-2 draft (revision 10b from August 1989) only has
> 3 bit LUNs placed in byte 1 of the command block, in the top
> 3 bits. There are also things called LUNTARs and LUNTRNs
> at the message level.
> 
> 8 bit LUNs do ring a distant bell, perhaps after SCSI-2?
> 
> Doug Gilbert

The hierarchical LUN structures have some limited size
LUN subfields:
* the peripheral device addressing method includes an 8-bit 
  TARGET OR LUN subfield, which specifies an 8-bit LUN at the 
  current level (if the BUS IDENTIFIER field is 0)
* the logical unit addressing method has a 5-bit LUN subfield


---
Rob ElliottHP Server Storage



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


RE: [PATCH] qla4xxx: Return -ENOMEM on memory allocation failure

2014-07-04 Thread Julia Lawall


On Fri, 4 Jul 2014, Elliott, Robert (Server Storage) wrote:

> 
> 
> > -Original Message-
> > From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> > ow...@vger.kernel.org] On Behalf Of Himangi Saraogi
> > Sent: Friday, 04 July, 2014 1:28 PM
> > To: Vikas Chaudhary; iscsi-dri...@qlogic.com; James E.J. Bottomley; linux-
> > s...@vger.kernel.org; linux-ker...@vger.kernel.org
> > Cc: julia.law...@lip6.fr
> > Subject: [PATCH] qla4xxx: Return -ENOMEM on memory allocation failure
> > 
> > In this code, 0 is returned on memory allocation failure, even though
> > other failures return -ENOMEM or other similar values.
> > 
> > A simplified version of the Coccinelle semantic match that finds this
> > problem is as follows:
> > 
> > // 
> > @@
> > expression ret;
> > expression x,e1,e2,e3;
> > identifier alloc;
> > @@
> > 
> > ret = 0
> > ... when != ret = e1
> > *x = alloc(...)
> > ... when != ret = e2
> > if (x == NULL) { ... when != ret = e3
> >   return ret;
> > }
> > // 
> > 
> > Signed-off-by: Himangi Saraogi 
> > Acked-by: Julia Lawall 
> > ---
> >  drivers/scsi/qla4xxx/ql4_os.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
> > index c5d9564..72ba671 100644
> > --- a/drivers/scsi/qla4xxx/ql4_os.c
> > +++ b/drivers/scsi/qla4xxx/ql4_os.c
> > @@ -1050,6 +1050,7 @@ static int qla4xxx_get_host_stats(struct Scsi_Host
> > *shost, char *buf, int len)
> > if (!ql_iscsi_stats) {
> > ql4_printk(KERN_ERR, ha,
> >"Unable to allocate memory for iscsi stats\n");
> > +   ret = -ENOMEM;
> > goto exit_host_stats;
> > }
> > 
> 
> Also, the only caller of this function doesn't use the return 
> value - it's overwritten by another function call:
> 
> drivers/scsi/scsi_transport_iscsi.c:
> err = transport->get_host_stats(shost, buf, host_stats_size);
> 
> actual_size = nlmsg_total_size(sizeof(*ev) + host_stats_size);
> skb_trim(skbhost_stats, NLMSG_ALIGN(actual_size));
> nlhhost_stats->nlmsg_len = actual_size;
> 
> err = iscsi_multicast_skb(skbhost_stats, ISCSI_NL_GRP_ISCSID,
>   GFP_KERNEL);

Maybe that is intentional?  If get_host_stats fails, eg because of a lack 
of memory, the worst that will happen is that a region of the buffer will 
be 0.  If the loop is done again, there seems to be a risk of an infinite 
loop.

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


RE: [PATCH] qla4xxx: Return -ENOMEM on memory allocation failure

2014-07-04 Thread Julia Lawall


On Fri, 4 Jul 2014, Julia Lawall wrote:

> 
> 
> On Fri, 4 Jul 2014, Elliott, Robert (Server Storage) wrote:
> 
> > 
> > 
> > > -Original Message-
> > > From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> > > ow...@vger.kernel.org] On Behalf Of Himangi Saraogi
> > > Sent: Friday, 04 July, 2014 1:28 PM
> > > To: Vikas Chaudhary; iscsi-dri...@qlogic.com; James E.J. Bottomley; linux-
> > > s...@vger.kernel.org; linux-ker...@vger.kernel.org
> > > Cc: julia.law...@lip6.fr
> > > Subject: [PATCH] qla4xxx: Return -ENOMEM on memory allocation failure
> > > 
> > > In this code, 0 is returned on memory allocation failure, even though
> > > other failures return -ENOMEM or other similar values.
> > > 
> > > A simplified version of the Coccinelle semantic match that finds this
> > > problem is as follows:
> > > 
> > > // 
> > > @@
> > > expression ret;
> > > expression x,e1,e2,e3;
> > > identifier alloc;
> > > @@
> > > 
> > > ret = 0
> > > ... when != ret = e1
> > > *x = alloc(...)
> > > ... when != ret = e2
> > > if (x == NULL) { ... when != ret = e3
> > >   return ret;
> > > }
> > > // 
> > > 
> > > Signed-off-by: Himangi Saraogi 
> > > Acked-by: Julia Lawall 
> > > ---
> > >  drivers/scsi/qla4xxx/ql4_os.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
> > > index c5d9564..72ba671 100644
> > > --- a/drivers/scsi/qla4xxx/ql4_os.c
> > > +++ b/drivers/scsi/qla4xxx/ql4_os.c
> > > @@ -1050,6 +1050,7 @@ static int qla4xxx_get_host_stats(struct Scsi_Host
> > > *shost, char *buf, int len)
> > >   if (!ql_iscsi_stats) {
> > >   ql4_printk(KERN_ERR, ha,
> > >  "Unable to allocate memory for iscsi stats\n");
> > > + ret = -ENOMEM;
> > >   goto exit_host_stats;
> > >   }
> > > 
> > 
> > Also, the only caller of this function doesn't use the return 
> > value - it's overwritten by another function call:
> > 
> > drivers/scsi/scsi_transport_iscsi.c:
> > err = transport->get_host_stats(shost, buf, 
> > host_stats_size);
> > 
> > actual_size = nlmsg_total_size(sizeof(*ev) + 
> > host_stats_size);
> > skb_trim(skbhost_stats, NLMSG_ALIGN(actual_size));
> > nlhhost_stats->nlmsg_len = actual_size;
> > 
> > err = iscsi_multicast_skb(skbhost_stats, 
> > ISCSI_NL_GRP_ISCSID,
> >   GFP_KERNEL);
> 
> Maybe that is intentional?  If get_host_stats fails, eg because of a lack 
> of memory, the worst that will happen is that a region of the buffer will 
> be 0.  If the loop is done again, there seems to be a risk of an infinite 
> loop.

Or one could jump out of the function completely, as on failure of 
alloc_skb.

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