date:20170509

Re: [PATCH v1] ibmvscsis: Fix cleaning up pointers

2017-05-09 Thread Nicholas A. Bellinger

On Tue, 2017-05-09 at 11:50 -0500, Bryant G. Ly wrote:
> This patch is dependent on:
> 'commit 25e78531268e ("ibmvscsis: Do not send aborted task response")'
> This patch cleans up some pointers after usage.
> 
> Signed-off-by: Bryant G. Ly 
> Reviewed-by: Michael Cyr 
> Cc:  # v4.8+
> ---
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 3 +++
>  1 file changed, 3 insertions(+)

Applied, with a more verbose commit message.

Thanks for splitting this out into an incremental patch.

[PATCH] csiostor: Avoid content leaks and casts

2017-05-09 Thread Kees Cook

When copying attributes, the len argument was padded out and the resulting
memcpy() would copy beyond the end of the source buffer.  Avoid this,
and use size_t for val_len to avoid all the casts. Similarly, avoid source
buffer casts and use void *.

Additionally enforces val_len can be represented by u16 and that
the DMA buffer was not overflowed. Fixes the size of mfa, which is not
FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN (but it will be padded up to 4). This
was noticed by the future CONFIG_FORTIFY_SOURCE checks.

Cc: Daniel Micay 
Signed-off-by: Kees Cook 
---
 drivers/scsi/csiostor/csio_lnode.c | 43 +++---
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/csiostor/csio_lnode.c 
b/drivers/scsi/csiostor/csio_lnode.c
index c00b2ff72b55..be5ee2d37815 100644
--- a/drivers/scsi/csiostor/csio_lnode.c
+++ b/drivers/scsi/csiostor/csio_lnode.c
@@ -238,14 +238,23 @@ csio_osname(uint8_t *buf, size_t buf_len)
 }
 
 static inline void
-csio_append_attrib(uint8_t **ptr, uint16_t type, uint8_t *val, uint16_t len)
+csio_append_attrib(uint8_t **ptr, uint16_t type, void *val, size_t val_len)
 {
+   uint16_t len;
struct fc_fdmi_attr_entry *ae = (struct fc_fdmi_attr_entry *)*ptr;
+
+   if (WARN_ON(val_len > U16_MAX))
+   return;
+
+   len = val_len;
+
ae->type = htons(type);
len += 4;   /* includes attribute type and length */
len = (len + 3) & ~3;   /* should be multiple of 4 bytes */
ae->len = htons(len);
-   memcpy(ae->value, val, len);
+   memcpy(ae->value, val, val_len);
+   if (len > val_len)
+   memset(ae->value + val_len, 0, len - val_len);
*ptr += len;
 }
 
@@ -335,7 +344,7 @@ csio_ln_fdmi_rhba_cbfn(struct csio_hw *hw, struct 
csio_ioreq *fdmi_req)
numattrs++;
val = htonl(FC_PORTSPEED_1GBIT | FC_PORTSPEED_10GBIT);
csio_append_attrib(, FC_FDMI_PORT_ATTR_SUPPORTEDSPEED,
-  (uint8_t *),
+  ,
   FC_FDMI_PORT_ATTR_SUPPORTEDSPEED_LEN);
numattrs++;
 
@@ -346,23 +355,22 @@ csio_ln_fdmi_rhba_cbfn(struct csio_hw *hw, struct 
csio_ioreq *fdmi_req)
else
val = htonl(CSIO_HBA_PORTSPEED_UNKNOWN);
csio_append_attrib(, FC_FDMI_PORT_ATTR_CURRENTPORTSPEED,
-  (uint8_t *),
-  FC_FDMI_PORT_ATTR_CURRENTPORTSPEED_LEN);
+  , FC_FDMI_PORT_ATTR_CURRENTPORTSPEED_LEN);
numattrs++;
 
mfs = ln->ln_sparm.csp.sp_bb_data;
csio_append_attrib(, FC_FDMI_PORT_ATTR_MAXFRAMESIZE,
-  (uint8_t *), FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN);
+  , sizeof(mfs));
numattrs++;
 
strcpy(buf, "csiostor");
csio_append_attrib(, FC_FDMI_PORT_ATTR_OSDEVICENAME, buf,
-  (uint16_t)strlen(buf));
+  strlen(buf));
numattrs++;
 
if (!csio_hostname(buf, sizeof(buf))) {
csio_append_attrib(, FC_FDMI_PORT_ATTR_HOSTNAME,
-  buf, (uint16_t)strlen(buf));
+  buf, strlen(buf));
numattrs++;
}
attrib_blk->numattrs = htonl(numattrs);
@@ -444,33 +452,32 @@ csio_ln_fdmi_dprt_cbfn(struct csio_hw *hw, struct 
csio_ioreq *fdmi_req)
 
strcpy(buf, "Chelsio Communications");
csio_append_attrib(, FC_FDMI_HBA_ATTR_MANUFACTURER, buf,
-  (uint16_t)strlen(buf));
+  strlen(buf));
numattrs++;
csio_append_attrib(, FC_FDMI_HBA_ATTR_SERIALNUMBER,
-  hw->vpd.sn, (uint16_t)sizeof(hw->vpd.sn));
+  hw->vpd.sn, sizeof(hw->vpd.sn));
numattrs++;
csio_append_attrib(, FC_FDMI_HBA_ATTR_MODEL, hw->vpd.id,
-  (uint16_t)sizeof(hw->vpd.id));
+  sizeof(hw->vpd.id));
numattrs++;
csio_append_attrib(, FC_FDMI_HBA_ATTR_MODELDESCRIPTION,
-  hw->model_desc, (uint16_t)strlen(hw->model_desc));
+  hw->model_desc, strlen(hw->model_desc));
numattrs++;
csio_append_attrib(, FC_FDMI_HBA_ATTR_HARDWAREVERSION,
-  hw->hw_ver, (uint16_t)sizeof(hw->hw_ver));
+  hw->hw_ver, sizeof(hw->hw_ver));
numattrs++;
csio_append_attrib(, FC_FDMI_HBA_ATTR_FIRMWAREVERSION,
-  hw->fwrev_str, (uint16_t)strlen(hw->fwrev_str));
+  hw->fwrev_str, strlen(hw->fwrev_str));
numattrs++;
 
if (!csio_osname(buf, sizeof(buf))) {
csio_append_attrib(, FC_FDMI_HBA_ATTR_OSNAMEVERSION,
-  buf, (uint16_t)strlen(buf));
+  buf,

[PATCH] target: remove dead code

2017-05-09 Thread Gustavo A. R. Silva

Local variable _ret_ is assigned to a constant value and it is never
updated again. Remove this variable and the dead code it guards.

Addresses-Coverity-ID: 140761
Signed-off-by: Gustavo A. R. Silva 
---
 drivers/target/target_core_rd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index ddc216c..6e8ef23 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -568,7 +568,7 @@ static ssize_t rd_set_configfs_dev_params(struct se_device 
*dev,
struct rd_dev *rd_dev = RD_DEV(dev);
char *orig, *ptr, *opts;
substring_t args[MAX_OPT_ARGS];
-   int ret = 0, arg, token;
+   int arg, token;
 
opts = kstrdup(page, GFP_KERNEL);
if (!opts)
@@ -603,7 +603,7 @@ static ssize_t rd_set_configfs_dev_params(struct se_device 
*dev,
}
 
kfree(orig);
-   return (!ret) ? count : ret;
+   return count;
 }
 
 static ssize_t rd_show_configfs_dev_params(struct se_device *dev, char *b)
-- 
2.5.0

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

2017-05-09 Thread Martin K. Petersen


Chad,

> To be honest, I'm reluctant to merge these patches on bnx2fc as the
> I/O path on this driver has been stable for quite some time and given
> that it's an older driver I'm not looking to make changes there.

I understand that the driver is in maintenance mode. However, the Linux
kernel and its interfaces are not. We do not offer any guarantees in
that department. So even if you do not intend to add new features to
bnx2fc, you will still need to keep it current wrt. the ongoing changes
in kernel interfaces (for better and for worse).

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v3] sd: Ignore sync cache failures when not supported

2017-05-09 Thread Ewan D. Milne

On Tue, 2017-05-09 at 17:43 +0200, Thierry Escande wrote:
> From: Derek Basehore 
> 
> Some external hard drives don't support the sync command even though the
> hard drive has write cache enabled. In this case, upon suspend request,
> sync cache failures are ignored if the error code in the sense header is
> ILLEGAL_REQUEST. There's not much we can do for these drives, so we
> shouldn't fail to suspend for this error case. The drive may stay
> powered if that's the setup for the port it's plugged into.
> 
> Signed-off-by: Derek Basehore 
> Signed-off-by: Thierry Escande 
> ---
> 
> v3 changes:
> - Pass the sense_hdr structure to sd_sync_cache() instead of the
>   lonely sense_key field
> 
> v2 changes:
> - Change sense_key type to u8 in sd_sync_cache()
> 
>  drivers/scsi/sd.c | 34 +-
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index fcfeddc..ee69fee 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1489,17 +1489,21 @@ static unsigned int sd_check_events(struct gendisk 
> *disk, unsigned int clearing)
>   return retval;
>  }
>  
> -static int sd_sync_cache(struct scsi_disk *sdkp)
> +static int sd_sync_cache(struct scsi_disk *sdkp, struct scsi_sense_hdr 
> *sshdr)
>  {
>   int retries, res;
>   struct scsi_device *sdp = sdkp->device;
>   const int timeout = sdp->request_queue->rq_timeout
>   * SD_FLUSH_TIMEOUT_MULTIPLIER;
> - struct scsi_sense_hdr sshdr;
> + struct scsi_sense_hdr my_sshdr;
>  
>   if (!scsi_device_online(sdp))
>   return -ENODEV;
>  
> + /* caller might not be interested in sense, but we need it */
> + if (!sshdr)
> + sshdr = _sshdr;
> +
>   for (retries = 3; retries > 0; --retries) {
>   unsigned char cmd[10] = { 0 };
>  
> @@ -1508,7 +1512,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
>* Leave the rest of the command zero to indicate
>* flush everything.
>*/
> - res = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, ,
> + res = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, sshdr,
>   timeout, SD_MAX_RETRIES, 0, RQF_PM, NULL);
>   if (res == 0)
>   break;
> @@ -1518,11 +1522,12 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
>   sd_print_result(sdkp, "Synchronize Cache(10) failed", res);
>  
>   if (driver_byte(res) & DRIVER_SENSE)
> - sd_print_sense_hdr(sdkp, );
> + sd_print_sense_hdr(sdkp, sshdr);
> +
>   /* we need to evaluate the error return  */
> - if (scsi_sense_valid() &&
> - (sshdr.asc == 0x3a ||   /* medium not present */
> -  sshdr.asc == 0x20))/* invalid command */
> + if (scsi_sense_valid(sshdr) &&
> + (sshdr->asc == 0x3a ||  /* medium not present */
> +  sshdr->asc == 0x20))   /* invalid command */
>   /* this is no error here */
>   return 0;
>  
> @@ -3323,7 +3328,7 @@ static void sd_shutdown(struct device *dev)
>  
>   if (sdkp->WCE && sdkp->media_present) {
>   sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
> - sd_sync_cache(sdkp);
> + sd_sync_cache(sdkp, NULL);
>   }
>  
>   if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) {
> @@ -3335,6 +3340,7 @@ static void sd_shutdown(struct device *dev)
>  static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
>  {
>   struct scsi_disk *sdkp = dev_get_drvdata(dev);
> + struct scsi_sense_hdr sshdr = { .sense_key = NO_SENSE };
>   int ret = 0;
>  
>   if (!sdkp)  /* E.g.: runtime suspend following sd_remove() */
> @@ -3342,8 +3348,17 @@ static int sd_suspend_common(struct device *dev, bool 
> ignore_stop_errors)
>  
>   if (sdkp->WCE && sdkp->media_present) {
>   sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
> - ret = sd_sync_cache(sdkp);
> + ret = sd_sync_cache(sdkp, );
>   if (ret) {
> + /*
> +  * If this drive doesn't support sync, there's not much
> +  * to do and suspend shouldn't fail.
> +  */
> + if (sshdr.sense_key == ILLEGAL_REQUEST) {
> + ret = 0;
> + goto start_stop;
> + }
> +
>   /* ignore OFFLINE device */
>   if (ret == -ENODEV)
>   ret = 0;

I think you need to check for scsi_sense_valid(sshdr) prior to
examining the .sense_key.  Also, I

[PATCH] scsi: qla2xxx: remove dead code

2017-05-09 Thread Gustavo A. R. Silva

Local variable page_mode is assigned to a constant value and it is never
updated again. Remove this variable and the dead code it guards.

Addresses-Coverity-ID: 200420
Signed-off-by: Gustavo A. R. Silva 
---
 drivers/scsi/qla2xxx/qla_nx.c | 48 ---
 1 file changed, 48 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c
index 0a1723c..bff63b2 100644
--- a/drivers/scsi/qla2xxx/qla_nx.c
+++ b/drivers/scsi/qla2xxx/qla_nx.c
@@ -2676,26 +2676,9 @@ qla82xx_write_flash_data(struct scsi_qla_host *vha, 
uint32_t *dwptr,
int ret;
uint32_t liter;
uint32_t rest_addr;
-   dma_addr_t optrom_dma;
-   void *optrom = NULL;
-   int page_mode = 0;
struct qla_hw_data *ha = vha->hw;
 
ret = -1;
-
-   /* Prepare burst-capable write on supported ISPs. */
-   if (page_mode && !(faddr & 0xfff) &&
-   dwords > OPTROM_BURST_DWORDS) {
-   optrom = dma_alloc_coherent(>pdev->dev, OPTROM_BURST_SIZE,
-   _dma, GFP_KERNEL);
-   if (!optrom) {
-   ql_log(ql_log_warn, vha, 0xb01b,
-   "Unable to allocate memory "
-   "for optrom burst write (%x KB).\n",
-   OPTROM_BURST_SIZE / 1024);
-   }
-   }
-
rest_addr = ha->fdt_block_size - 1;
 
ret = qla82xx_unprotect_flash(ha);
@@ -2718,34 +2701,6 @@ qla82xx_write_flash_data(struct scsi_qla_host *vha, 
uint32_t *dwptr,
}
}
 
-   /* Go with burst-write. */
-   if (optrom && (liter + OPTROM_BURST_DWORDS) <= dwords) {
-   /* Copy data to DMA'ble buffer. */
-   memcpy(optrom, dwptr, OPTROM_BURST_SIZE);
-
-   ret = qla2x00_load_ram(vha, optrom_dma,
-   (ha->flash_data_off | faddr),
-   OPTROM_BURST_DWORDS);
-   if (ret != QLA_SUCCESS) {
-   ql_log(ql_log_warn, vha, 0xb01e,
-   "Unable to burst-write optrom segment "
-   "(%x/%x/%llx).\n", ret,
-   (ha->flash_data_off | faddr),
-   (unsigned long long)optrom_dma);
-   ql_log(ql_log_warn, vha, 0xb01f,
-   "Reverting to slow-write.\n");
-
-   dma_free_coherent(>pdev->dev,
-   OPTROM_BURST_SIZE, optrom, optrom_dma);
-   optrom = NULL;
-   } else {
-   liter += OPTROM_BURST_DWORDS - 1;
-   faddr += OPTROM_BURST_DWORDS - 1;
-   dwptr += OPTROM_BURST_DWORDS - 1;
-   continue;
-   }
-   }
-
ret = qla82xx_write_flash_dword(ha, faddr,
cpu_to_le32(*dwptr));
if (ret) {
@@ -2761,9 +2716,6 @@ qla82xx_write_flash_data(struct scsi_qla_host *vha, 
uint32_t *dwptr,
ql_log(ql_log_warn, vha, 0xb021,
"Unable to protect flash after update.\n");
 write_done:
-   if (optrom)
-   dma_free_coherent(>pdev->dev,
-   OPTROM_BURST_SIZE, optrom, optrom_dma);
return ret;
 }
 
-- 
2.5.0

[PATCH v1] ibmvscsis: Fix cleaning up pointers

2017-05-09 Thread Bryant G. Ly

This patch is dependent on:
'commit 25e78531268e ("ibmvscsis: Do not send aborted task response")'
This patch cleans up some pointers after usage.

Signed-off-by: Bryant G. Ly 
Reviewed-by: Michael Cyr 
Cc:  # v4.8+
---
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c 
b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index d390325..ee64241 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -1170,6 +1170,8 @@ static struct ibmvscsis_cmd 
*ibmvscsis_get_free_cmd(struct scsi_info *vscsi)
cmd = list_first_entry_or_null(>free_cmd,
   struct ibmvscsis_cmd, list);
if (cmd) {
+   if (cmd->abort_cmd)
+   cmd->abort_cmd = NULL;
cmd->flags &= ~(DELAY_SEND);
list_del(>list);
cmd->iue = iue;
@@ -1774,6 +1776,7 @@ static void ibmvscsis_send_messages(struct scsi_info 
*vscsi)
if (cmd->abort_cmd) {
retry = true;
cmd->abort_cmd->flags &= ~(DELAY_SEND);
+   cmd->abort_cmd = NULL;
}
 
/*
-- 
2.5.4 (Apple Git-61)

[PATCH v3] sd: Ignore sync cache failures when not supported

2017-05-09 Thread Thierry Escande

From: Derek Basehore 

Some external hard drives don't support the sync command even though the
hard drive has write cache enabled. In this case, upon suspend request,
sync cache failures are ignored if the error code in the sense header is
ILLEGAL_REQUEST. There's not much we can do for these drives, so we
shouldn't fail to suspend for this error case. The drive may stay
powered if that's the setup for the port it's plugged into.

Signed-off-by: Derek Basehore 
Signed-off-by: Thierry Escande 
---

v3 changes:
- Pass the sense_hdr structure to sd_sync_cache() instead of the
  lonely sense_key field

v2 changes:
- Change sense_key type to u8 in sd_sync_cache()

 drivers/scsi/sd.c | 34 +-
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index fcfeddc..ee69fee 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1489,17 +1489,21 @@ static unsigned int sd_check_events(struct gendisk 
*disk, unsigned int clearing)
return retval;
 }
 
-static int sd_sync_cache(struct scsi_disk *sdkp)
+static int sd_sync_cache(struct scsi_disk *sdkp, struct scsi_sense_hdr *sshdr)
 {
int retries, res;
struct scsi_device *sdp = sdkp->device;
const int timeout = sdp->request_queue->rq_timeout
* SD_FLUSH_TIMEOUT_MULTIPLIER;
-   struct scsi_sense_hdr sshdr;
+   struct scsi_sense_hdr my_sshdr;
 
if (!scsi_device_online(sdp))
return -ENODEV;
 
+   /* caller might not be interested in sense, but we need it */
+   if (!sshdr)
+   sshdr = _sshdr;
+
for (retries = 3; retries > 0; --retries) {
unsigned char cmd[10] = { 0 };
 
@@ -1508,7 +1512,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
 * Leave the rest of the command zero to indicate
 * flush everything.
 */
-   res = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, ,
+   res = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, sshdr,
timeout, SD_MAX_RETRIES, 0, RQF_PM, NULL);
if (res == 0)
break;
@@ -1518,11 +1522,12 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
sd_print_result(sdkp, "Synchronize Cache(10) failed", res);
 
if (driver_byte(res) & DRIVER_SENSE)
-   sd_print_sense_hdr(sdkp, );
+   sd_print_sense_hdr(sdkp, sshdr);
+
/* we need to evaluate the error return  */
-   if (scsi_sense_valid() &&
-   (sshdr.asc == 0x3a ||   /* medium not present */
-sshdr.asc == 0x20))/* invalid command */
+   if (scsi_sense_valid(sshdr) &&
+   (sshdr->asc == 0x3a ||  /* medium not present */
+sshdr->asc == 0x20))   /* invalid command */
/* this is no error here */
return 0;
 
@@ -3323,7 +3328,7 @@ static void sd_shutdown(struct device *dev)
 
if (sdkp->WCE && sdkp->media_present) {
sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
-   sd_sync_cache(sdkp);
+   sd_sync_cache(sdkp, NULL);
}
 
if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) {
@@ -3335,6 +3340,7 @@ static void sd_shutdown(struct device *dev)
 static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
 {
struct scsi_disk *sdkp = dev_get_drvdata(dev);
+   struct scsi_sense_hdr sshdr = { .sense_key = NO_SENSE };
int ret = 0;
 
if (!sdkp)  /* E.g.: runtime suspend following sd_remove() */
@@ -3342,8 +3348,17 @@ static int sd_suspend_common(struct device *dev, bool 
ignore_stop_errors)
 
if (sdkp->WCE && sdkp->media_present) {
sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
-   ret = sd_sync_cache(sdkp);
+   ret = sd_sync_cache(sdkp, );
if (ret) {
+   /*
+* If this drive doesn't support sync, there's not much
+* to do and suspend shouldn't fail.
+*/
+   if (sshdr.sense_key == ILLEGAL_REQUEST) {
+   ret = 0;
+   goto start_stop;
+   }
+
/* ignore OFFLINE device */
if (ret == -ENODEV)
ret = 0;
@@ -3351,6 +3366,7 @@ static int sd_suspend_common(struct device *dev, bool 
ignore_stop_errors)
}
}
 
+start_stop:
if (sdkp->device->manage_start_stop) {
sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
/* an

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

2017-05-09 Thread James Bottomley

On Tue, 2017-05-09 at 10:17 -0400, Chad Dupuis wrote:
> On Mon, 8 May 2017, 10:04pm, Martin K. Petersen wrote:
> 
> > 
> > Sebastian,
> > 
> > > Martin, do you see any chance to get this merged? Chad replied to
> the
> > > list that he is going to test it on 2017-04-10, didn't respond to
> the
> > > ping 10 days later. The series stalled last time in the same way.
> > 
> > I am very reluctant to merge something when a driver has an active
> > maintainer and that person has not acked the change.
> > 
> > That said, Chad: You have been sitting on this for quite a while.
> Please
> > make it a priority. In exchange for veto rights you do have to
> provide
> > timely feedback on anything that touches your driver.
> > 
> > Thanks!
> > 
> 
> We did do some testing and hit a calltrace during device discovery:
> 
> [ 1332.551799] INFO: task scsi_eh_15:1970 blocked for more than 120 
> seconds.
> [ 1332.551804] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
> disables 
> this message.
> [ 1332.551807] scsi_eh_15  D 880823488c14 0  1970  2 
> 0x0080
> [ 1332.551813]  881053a17cb0 0046 88084d693ec0 
> 881053a17fd8
> [ 1332.551817]  881053a17fd8 881053a17fd8 88084d693ec0 
> 880823488d48
> [ 1332.551821]  880823488d50 7fff 88084d693ec0 
> 880823488c14
> [ 1332.551825] Call Trace:
> [ 1332.551838]  [] schedule+0x29/0x70
> [ 1332.551844]  [] schedule_timeout+0x239/0x2d0
> [ 1332.551850]  [] ? console_unlock+0x208/0x400
> [ 1332.551855]  [] ? vprintk_emit+0x3c4/0x510
> [ 1332.551861]  [] ?
> lock_timer_base.isra.33+0x2b/0x50
> [ 1332.551866]  [] wait_for_completion+0x116/0x170
> [ 1332.551874]  [] ? wake_up_state+0x20/0x20
> [ 1332.551885]  [] bnx2fc_abts_cleanup+0x3d/0x62 
> [bnx2fc]
> [ 1332.551892]  [] bnx2fc_eh_abort+0x470/0x580
> [bnx2fc]
> [ 1332.551900]  [] scsi_error_handler+0x59f/0x8b0
> [ 1332.551904]  [] ? scsi_eh_get_sense+0x250/0x250
> [ 1332.551911]  [] kthread+0xcf/0xe0
> [ 1332.551916]  [] ?
> kthread_create_on_node+0x140/0x140
> [ 1332.551923]  [] ret_from_fork+0x58/0x90
> [ 1332.551928]  [] ?
> kthread_create_on_node+0x140/0x140 

Reporting this when you found it would have been helpful ...

That said, it does look like a genuine hang in the workqueues, so it
rather invalidates the current patch set.

> To be honest, I'm reluctant to merge these patches on bnx2fc as the
> I/O path on this driver has been stable for quite some time and given
> that it's an older driver I'm not looking to make changes there.

OK, so find a way to achieve both sets of goals because there's a limit
to how long we allow "stable" drivers to hold up infrastructure changes
within the kernel.  The main goal of the current patch set is to remove
the cpu hotplug calls from the drivers because they want to remove them
from the kernel.  This is rather complex because you're using per cpu
work queues so you currently have to manage starting and stopping them
as the CPUs come up or go down ... getting rid of that for standard
kernel infrastructure will make the driver easier to keep in
maintenance mode for longer.

James

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

2017-05-09 Thread Chad Dupuis


On Mon, 8 May 2017, 10:04pm, Martin K. Petersen wrote:

> 
> Sebastian,
> 
> > Martin, do you see any chance to get this merged? Chad replied to the
> > list that he is going to test it on 2017-04-10, didn't respond to the
> > ping 10 days later. The series stalled last time in the same way.
> 
> I am very reluctant to merge something when a driver has an active
> maintainer and that person has not acked the change.
> 
> That said, Chad: You have been sitting on this for quite a while. Please
> make it a priority. In exchange for veto rights you do have to provide
> timely feedback on anything that touches your driver.
> 
> Thanks!
> 

We did do some testing and hit a calltrace during device discovery:

[ 1332.551799] INFO: task scsi_eh_15:1970 blocked for more than 120 
seconds.
[ 1332.551804] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables 
this message.
[ 1332.551807] scsi_eh_15  D 880823488c14 0  1970  2 
0x0080
[ 1332.551813]  881053a17cb0 0046 88084d693ec0 
881053a17fd8
[ 1332.551817]  881053a17fd8 881053a17fd8 88084d693ec0 
880823488d48
[ 1332.551821]  880823488d50 7fff 88084d693ec0 
880823488c14
[ 1332.551825] Call Trace:
[ 1332.551838]  [] schedule+0x29/0x70
[ 1332.551844]  [] schedule_timeout+0x239/0x2d0
[ 1332.551850]  [] ? console_unlock+0x208/0x400
[ 1332.551855]  [] ? vprintk_emit+0x3c4/0x510
[ 1332.551861]  [] ? lock_timer_base.isra.33+0x2b/0x50
[ 1332.551866]  [] wait_for_completion+0x116/0x170
[ 1332.551874]  [] ? wake_up_state+0x20/0x20
[ 1332.551885]  [] bnx2fc_abts_cleanup+0x3d/0x62 
[bnx2fc]
[ 1332.551892]  [] bnx2fc_eh_abort+0x470/0x580 [bnx2fc]
[ 1332.551900]  [] scsi_error_handler+0x59f/0x8b0
[ 1332.551904]  [] ? scsi_eh_get_sense+0x250/0x250
[ 1332.551911]  [] kthread+0xcf/0xe0
[ 1332.551916]  [] ? kthread_create_on_node+0x140/0x140
[ 1332.551923]  [] ret_from_fork+0x58/0x90
[ 1332.551928]  [] ? kthread_create_on_node+0x140/0x140 

To be honest, I'm reluctant to merge these patches on bnx2fc as the I/O 
path on this driver has been stable for quite some time and given that 
it's an older driver I'm not looking to make changes there.

Re: [PATCH 1/5] scsi: bnx2i: convert to workqueue

2017-05-09 Thread Rangankar, Manish


On 10/04/17 10:42 PM, "Sebastian Andrzej Siewior" 
wrote:

>The driver creates its own per-CPU threads which are updated based on CPU
>hotplug events. It is also possible to use kworkers and remove some of the
>infrastructure get the same job done while saving a few lines of code.
>
>The DECLARE_PER_CPU() definition is moved into the header file where it
>belongs. bnx2i_percpu_io_thread() becomes bnx2i_percpu_io_work() which is
>mostly the same code. The outer loop (kthread_should_stop()) gets removed
>and
>the remaining code is shifted to the left.
>bnx2i_queue_scsi_cmd_resp() is mostly the same. The code checked
>->iothread to
>decide if there is an active per-CPU thread. With the kworkers this is no
>longer possible nor required.
>The allocation of struct bnx2i_work does not happen with ->p_work_lock
>held
>which is not required. I am unsure about the call-stack so I can't say
>if this qualifies it for the allocation with GFP_KERNEL instead of
>GFP_ATOMIC (it is not _bh lock but as I said, I don't know the context).
>The allocation case has been reversed so the inner if case is called on
>!bnx2i_work and is just the invocation one function since the lock is not
>held during allocation. The init of the new bnx2i_work struct is now
>done also without the ->p_work_lock held: it is a new object, nobody
>knows about it yet. It should be enough to hold the lock while adding
>this item to the list. I am unsure about that atomic_inc() so I keep
>things as they were.
>
>The remaining part is the removal CPU hotplug notifier since it is taken
>care by the workqueue code.
>
>This patch was only compile-tested due to -ENODEV.
>
>Cc: qlogic-storage-upstr...@qlogic.com
>Cc: Christoph Hellwig 
>Signed-off-by: Sebastian Andrzej Siewior 
>---

Didn't seen any issue with regression testing. Thanks Sebastian.

Acked-by: Manish Rangankar

A bug in scsi_alloc_target of drivers/scsi/scsi_scan.c

2017-05-09 Thread Dashi DS1 Cao

When debugging a race condition in scsi_remove_target of 3.12, I ran into this 
possible bug within scsi_alloc_target.
When an existing "struct scsi_target" is found and used, the starget just got 
through kzmalloc should be freed, rather than dong a "put_device(dev)".

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 81d4151..96795d4 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -483,7 +483,7 @@ static struct scsi_target *scsi_alloc_target(struct device 
*parent,

spin_unlock_irqrestore(shost->host_lock, flags);
if (ref_got) {
-   put_device(dev);
+   kfree(starget);
return found_target;
}
/*
--

Dashi Cao

Re: [PATCH 25/27] block: remove the discard_zeroes_data flag

2017-05-09 Thread Nicholas A. Bellinger

On Sun, 2017-05-07 at 11:22 +0200, h...@lst.de wrote:
> On Tue, May 02, 2017 at 08:33:15PM -0700, Nicholas A. Bellinger wrote:
> > The larger target/iblock conversion patch looks like post v4.12 material
> > at this point, so to avoid breakage wrt to existing LBPRZ behavior, I'll
> > plan to push the following patch post -rc1.
> 
> I don't think this is safe.  If you want to do the aboe you also
> need to ensure ->execute_unmap always zeroes the data.  For actual
> files in the file backend we should be all fine, but for the block
> device case [1] and iblock we'd need to use blkdev_issue_zeroout
> instead of blkdev_issue_discard when unmap_zeroes_data is set.
> 
> [1] which btw already seems broken as it doesn't invalidate cached
> data when issuing a discard.

Mmm, for [1] that would appear to be true, but after a deeper look at
existing code I don't think this is the case.

The reason being is target backend attributes emulate_tpu and
emulate_tpws are strictly user configurable, and aren't automatically
set based upon the underlying IBLOCK block_device support for either
one.

According to pre v4.12-rc1 code, q->limits.discard_zeroes_data was only
enabled by drivers/scsi/sd.c:sd_config_discard() for
sdkp->provisioning_mode WRITE_SAME with LBPRZ = 1 or explicit ZERO, and
for NVME for devices that supported NVME_QUIRK_DISCARD_ZEROES.  Eg: Only
real DISCARD + ZERO support.

In your changes to v4.12-rc1, this logic to signal real DISCARD + zero
support for SCSI and NVMe via q->limits.max_write_zeroes_sectors has not
changed..

So AFAICT, regardless if the user sets emulate_tpu or emulate_tpws for a
IBLOCK backend, SCSI host code will have to choose sdkp->zeroing_mode
WRITE_SAME with LBPRZ or explicit ZERO, and NVMe host code will have to
chose a ctrl NVME_QUIRK_DEALLOCATE_ZEROES before
q->limits.max_write_zeroes_sectors != 0 is propagated up to target code,
and LBPRZ = 1 is signaled via READ_CAPACITY_16 and EVPD = 0xb2.

That said, simply propagating up q->limits.max_write_zeroes_sectors as
dev_attrib->unmap_zeroes_data following existing code still looks like
the right thing to do.

Re: [PATCH v1] ibmvscsis: Fix cleaning up pointers

[PATCH] csiostor: Avoid content leaks and casts

[PATCH] target: remove dead code

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

Re: [PATCH v3] sd: Ignore sync cache failures when not supported

[PATCH] scsi: qla2xxx: remove dead code

[PATCH v1] ibmvscsis: Fix cleaning up pointers

[PATCH v3] sd: Ignore sync cache failures when not supported

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

Re: [REEEEPOST] bnx2i + bnx2fc: convert to generic workqueue (#3)

Re: [PATCH 1/5] scsi: bnx2i: convert to workqueue

A bug in scsi_alloc_target of drivers/scsi/scsi_scan.c

Re: [PATCH 25/27] block: remove the discard_zeroes_data flag

13 matches

Site Navigation

Mail list logo

Footer information