Re: [PATCH 06/16] scsi_dh_alua: use local buffer for VPD inquiry

2014-02-14 Thread Bart Van Assche 
On 01/31/14 10:29, Hannes Reinecke wrote:
 - len = (h-buff[2]  8) + h-buff[3] + 4;
 - if (len  h-bufflen) {
 + len = (buff[2]  8) + buff[3] + 4;
 + if (len  bufflen) {

I think this code will become easier to read when using
get_unaligned_be16() for extracting the length from the VPD response.

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 06/16] scsi_dh_alua: use local buffer for VPD inquiry

2014-02-13 Thread Maurizio Lombardi 
Hi,

On 01/31/2014 10:29 AM, Hannes Reinecke wrote:
  static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
  {
 + unsigned char *buff;
 + unsigned char bufflen = 36;
   int len, timeout = ALUA_FAILOVER_TIMEOUT;
[...]
+  len = (buff[2]  8) + buff[3] + 4;
+  if (len  bufflen) {
[...]
+  bufflen = len;

just a nit: is it safe to use char as the type of bufflen? Isn't better
to declare it as int just in case len is  than 255 ?

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: [PATCH 06/16] scsi_dh_alua: use local buffer for VPD inquiry

2014-02-13 Thread Hannes Reinecke 
On 02/13/2014 10:51 AM, Maurizio Lombardi wrote:
 Hi,
 
 On 01/31/2014 10:29 AM, Hannes Reinecke wrote:
  static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data 
 *h)
  {
 +unsigned char *buff;
 +unsigned char bufflen = 36;
  int len, timeout = ALUA_FAILOVER_TIMEOUT;
 [...]
 +len = (buff[2]  8) + buff[3] + 4;
 +if (len  bufflen) {
 [...]
 +bufflen = len;
 
 just a nit: is it safe to use char as the type of bufflen? Isn't better
 to declare it as int just in case len is  than 255 ?
 
Yes, true.

However, this whole section needs to be reworked anyway, as there's
a fair chance we're getting the VPD page 0x83 for free.
(Cf my latest patchset 'Display EVPD pages in sysfs').

And jejb made some positive noises that, so I'll be reworking the
scsi_dh_alua patchset to take advantage of the new fields.

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

[PATCH 06/16] scsi_dh_alua: use local buffer for VPD inquiry

2014-01-31 Thread Hannes Reinecke 
VPD inquiry need to be done only once, so we can be using
a local buffer here.

Signed-off-by: Hannes Reinecke h...@suse.de
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 45 ++
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
b/drivers/scsi/device_handler/scsi_dh_alua.c
index adc77ef..88f98e0 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -322,16 +322,27 @@ static int alua_check_tpgs(struct scsi_device *sdev, 
struct alua_dh_data *h)
  */
 static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 {
+   unsigned char *buff;
+   unsigned char bufflen = 36;
int len, timeout = ALUA_FAILOVER_TIMEOUT;
unsigned char sense[SCSI_SENSE_BUFFERSIZE];
struct scsi_sense_hdr sense_hdr;
unsigned retval;
unsigned char *d;
unsigned long expiry;
+   int err;
 
expiry = round_jiffies_up(jiffies + timeout);
  retry:
-   retval = submit_vpd_inquiry(sdev, h-buff, h-bufflen, sense);
+   buff = kmalloc(bufflen, GFP_KERNEL);
+   if (!buff) {
+   sdev_printk(KERN_WARNING, sdev,
+   %s: kmalloc buffer failed\n,
+   ALUA_DH_NAME);
+   /* Temporary failure, bypass */
+   return SCSI_DH_DEV_TEMP_BUSY;
+   }
+   retval = submit_vpd_inquiry(sdev, buff, bufflen, sense);
if (retval) {
unsigned err;
 
@@ -345,6 +356,7 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, 
struct alua_dh_data *h)
err = SCSI_DH_DEV_TEMP_BUSY;
else
err = SCSI_DH_IO;
+   kfree(buff);
return err;
}
err = alua_check_sense(sdev, sense_hdr);
@@ -362,24 +374,19 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, 
struct alua_dh_data *h)
}
 
/* Check if vpd page exceeds initial buffer */
-   len = (h-buff[2]  8) + h-buff[3] + 4;
-   if (len  h-bufflen) {
+   len = (buff[2]  8) + buff[3] + 4;
+   if (len  bufflen) {
/* Resubmit with the correct length */
-   if (realloc_buffer(h, len)) {
-   sdev_printk(KERN_WARNING, sdev,
-   %s: kmalloc buffer failed\n,
-   ALUA_DH_NAME);
-   /* Temporary failure, bypass */
-   return SCSI_DH_DEV_TEMP_BUSY;
-   }
+   kfree(buff);
+   bufflen = len;
goto retry;
}
 
/*
 * Now look for the correct descriptor.
 */
-   d = h-buff + 4;
-   while (d  h-buff + len) {
+   d = buff + 4;
+   while (d  buff + len) {
switch (d[1]  0xf) {
case 0x4:
/* Relative target port */
@@ -406,13 +413,15 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, 
struct alua_dh_data *h)
ALUA_DH_NAME);
h-state = TPGS_STATE_OPTIMIZED;
h-tpgs = TPGS_MODE_NONE;
-   return SCSI_DH_DEV_UNSUPP;
+   err = SCSI_DH_DEV_UNSUPP;
+   } else {
+   sdev_printk(KERN_INFO, sdev,
+   %s: port group %02x rel port %02x\n,
+   ALUA_DH_NAME, h-group_id, h-rel_port);
+   err = SCSI_DH_OK;
}
-   sdev_printk(KERN_INFO, sdev,
-   %s: port group %02x rel port %02x\n,
-   ALUA_DH_NAME, h-group_id, h-rel_port);
-
-   return SCSI_DH_OK;
+   kfree(buff);
+   return err;
 }
 
 static char print_alua_state(int state)
-- 
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

Re: [PATCH 06/16] scsi_dh_alua: use local buffer for VPD inquiry

2014-01-17 Thread Mike Christie 
On 12/20/2013 06:13 AM, Hannes Reinecke wrote:
 VPD inquiry need to be done only once, so we can be using
 a local buffer here.
 
 Signed-off-by: Hannes Reinecke h...@suse.de
 ---
  drivers/scsi/device_handler/scsi_dh_alua.c | 45 
 ++
  1 file changed, 27 insertions(+), 18 deletions(-)
 
 diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
 b/drivers/scsi/device_handler/scsi_dh_alua.c
 index adc77ef..49952f4 100644
 --- a/drivers/scsi/device_handler/scsi_dh_alua.c
 +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
 @@ -322,16 +322,27 @@ static int alua_check_tpgs(struct scsi_device *sdev, 
 struct alua_dh_data *h)
   */
  static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
  {
 + unsigned char *buff;
 + unsigned char bufflen = 36;
   int len, timeout = ALUA_FAILOVER_TIMEOUT;
   unsigned char sense[SCSI_SENSE_BUFFERSIZE];
   struct scsi_sense_hdr sense_hdr;
   unsigned retval;
   unsigned char *d;
   unsigned long expiry;
 + int err;
  
   expiry = round_jiffies_up(jiffies + timeout);
   retry:
 - retval = submit_vpd_inquiry(sdev, h-buff, h-bufflen, sense);
 + buff = kmalloc(bufflen, GFP_ATOMIC);
 + if (!buff) {
 

Why GFP_ATOMIC? I think it can be less restrictive in this path. If you
need GFP_ATOMIC here, then there are places in this code path you would
want to change from GFP_KERNEL to GFP_ATOMIC.

--
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 06/16] scsi_dh_alua: use local buffer for VPD inquiry

2014-01-17 Thread Hannes Reinecke 
On 01/17/2014 10:04 AM, Mike Christie wrote:
 On 12/20/2013 06:13 AM, Hannes Reinecke wrote:
 VPD inquiry need to be done only once, so we can be using
 a local buffer here.

 Signed-off-by: Hannes Reinecke h...@suse.de
 ---
  drivers/scsi/device_handler/scsi_dh_alua.c | 45 
 ++
  1 file changed, 27 insertions(+), 18 deletions(-)

 diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
 b/drivers/scsi/device_handler/scsi_dh_alua.c
 index adc77ef..49952f4 100644
 --- a/drivers/scsi/device_handler/scsi_dh_alua.c
 +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
 @@ -322,16 +322,27 @@ static int alua_check_tpgs(struct scsi_device *sdev, 
 struct alua_dh_data *h)
   */
  static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data 
 *h)
  {
 +unsigned char *buff;
 +unsigned char bufflen = 36;
  int len, timeout = ALUA_FAILOVER_TIMEOUT;
  unsigned char sense[SCSI_SENSE_BUFFERSIZE];
  struct scsi_sense_hdr sense_hdr;
  unsigned retval;
  unsigned char *d;
  unsigned long expiry;
 +int err;
  
  expiry = round_jiffies_up(jiffies + timeout);
   retry:
 -retval = submit_vpd_inquiry(sdev, h-buff, h-bufflen, sense);
 +buff = kmalloc(bufflen, GFP_ATOMIC);
 +if (!buff) {

 
 Why GFP_ATOMIC? I think it can be less restrictive in this path. If you
 need GFP_ATOMIC here, then there are places in this code path you would
 want to change from GFP_KERNEL to GFP_ATOMIC.
 
Yeah, all right. This is by no means time-critical.

I'll be fixing up the patch.

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

[PATCH 06/16] scsi_dh_alua: use local buffer for VPD inquiry

2013-12-20 Thread Hannes Reinecke 
VPD inquiry need to be done only once, so we can be using
a local buffer here.

Signed-off-by: Hannes Reinecke h...@suse.de
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 45 ++
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c 
b/drivers/scsi/device_handler/scsi_dh_alua.c
index adc77ef..49952f4 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -322,16 +322,27 @@ static int alua_check_tpgs(struct scsi_device *sdev, 
struct alua_dh_data *h)
  */
 static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 {
+   unsigned char *buff;
+   unsigned char bufflen = 36;
int len, timeout = ALUA_FAILOVER_TIMEOUT;
unsigned char sense[SCSI_SENSE_BUFFERSIZE];
struct scsi_sense_hdr sense_hdr;
unsigned retval;
unsigned char *d;
unsigned long expiry;
+   int err;
 
expiry = round_jiffies_up(jiffies + timeout);
  retry:
-   retval = submit_vpd_inquiry(sdev, h-buff, h-bufflen, sense);
+   buff = kmalloc(bufflen, GFP_ATOMIC);
+   if (!buff) {
+   sdev_printk(KERN_WARNING, sdev,
+   %s: kmalloc buffer failed\n,
+   ALUA_DH_NAME);
+   /* Temporary failure, bypass */
+   return SCSI_DH_DEV_TEMP_BUSY;
+   }
+   retval = submit_vpd_inquiry(sdev, buff, bufflen, sense);
if (retval) {
unsigned err;
 
@@ -345,6 +356,7 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, 
struct alua_dh_data *h)
err = SCSI_DH_DEV_TEMP_BUSY;
else
err = SCSI_DH_IO;
+   kfree(buff);
return err;
}
err = alua_check_sense(sdev, sense_hdr);
@@ -362,24 +374,19 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, 
struct alua_dh_data *h)
}
 
/* Check if vpd page exceeds initial buffer */
-   len = (h-buff[2]  8) + h-buff[3] + 4;
-   if (len  h-bufflen) {
+   len = (buff[2]  8) + buff[3] + 4;
+   if (len  bufflen) {
/* Resubmit with the correct length */
-   if (realloc_buffer(h, len)) {
-   sdev_printk(KERN_WARNING, sdev,
-   %s: kmalloc buffer failed\n,
-   ALUA_DH_NAME);
-   /* Temporary failure, bypass */
-   return SCSI_DH_DEV_TEMP_BUSY;
-   }
+   kfree(buff);
+   bufflen = len;
goto retry;
}
 
/*
 * Now look for the correct descriptor.
 */
-   d = h-buff + 4;
-   while (d  h-buff + len) {
+   d = buff + 4;
+   while (d  buff + len) {
switch (d[1]  0xf) {
case 0x4:
/* Relative target port */
@@ -406,13 +413,15 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, 
struct alua_dh_data *h)
ALUA_DH_NAME);
h-state = TPGS_STATE_OPTIMIZED;
h-tpgs = TPGS_MODE_NONE;
-   return SCSI_DH_DEV_UNSUPP;
+   err = SCSI_DH_DEV_UNSUPP;
+   } else {
+   sdev_printk(KERN_INFO, sdev,
+   %s: port group %02x rel port %02x\n,
+   ALUA_DH_NAME, h-group_id, h-rel_port);
+   err = SCSI_DH_OK;
}
-   sdev_printk(KERN_INFO, sdev,
-   %s: port group %02x rel port %02x\n,
-   ALUA_DH_NAME, h-group_id, h-rel_port);
-
-   return SCSI_DH_OK;
+   kfree(buff);
+   return err;
 }
 
 static char print_alua_state(int state)
-- 
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