Re: [PATCH net-next 4/5] treewide: replace dev->trans_start update with helper
On 05/03/2016 04:33 PM, Florian Westphal wrote:
> Replace all trans_start updates with netif_trans_update helper.
> change was done via spatch:
>
> struct net_device *d;
> @@
> - d->trans_start = jiffies
> + netif_trans_update(d)
>
> Compile tested only.
>
> Cc: user-mode-linux-de...@lists.sourceforge.net
> Cc: linux-xte...@linux-xtensa.org
> Cc: linux1394-de...@lists.sourceforge.net
> Cc: linux-r...@vger.kernel.org
> Cc: net...@vger.kernel.org
> Cc: mpt-fusionlinux@broadcom.com
> Cc: linux-scsi@vger.kernel.org
> Cc: linux-...@vger.kernel.org
> Cc: linux-par...@vger.kernel.org
> Cc: linux-o...@vger.kernel.org
> Cc: linux-h...@vger.kernel.org
> Cc: linux-...@vger.kernel.org
> Cc: linux-wirel...@vger.kernel.org
> Cc: linux-s...@vger.kernel.org
> Cc: de...@driverdev.osuosl.org
> Cc: b.a.t.m@lists.open-mesh.org
> Cc: linux-blueto...@vger.kernel.org
> Signed-off-by: Florian Westphal
> ---
> Checkpatch complains about whitespace damage, but
> this extra whitespace already exists before this patch.
>
> drivers/net/can/mscan/mscan.c | 4 ++--
> drivers/net/can/usb/ems_usb.c | 4 ++--
> drivers/net/can/usb/esd_usb2.c | 4 ++--
> drivers/net/can/usb/peak_usb/pcan_usb_core.c | 4 ++--
> diff --git a/drivers/net/can/mscan/mscan.c b/drivers/net/can/mscan/mscan.c
> index e36b740..acb708f 100644
> --- a/drivers/net/can/mscan/mscan.c
> +++ b/drivers/net/can/mscan/mscan.c
> @@ -276,7 +276,7 @@ static netdev_tx_t mscan_start_xmit(struct sk_buff *skb,
> struct net_device *dev)
> out_8(®s->cantflg, 1 << buf_id);
>
> if (!test_bit(F_TX_PROGRESS, &priv->flags))
> - dev->trans_start = jiffies;
> + netif_trans_update(dev);
>
> list_add_tail(&priv->tx_queue[buf_id].list, &priv->tx_head);
>
> @@ -469,7 +469,7 @@ static irqreturn_t mscan_isr(int irq, void *dev_id)
> clear_bit(F_TX_PROGRESS, &priv->flags);
> priv->cur_pri = 0;
> } else {
> - dev->trans_start = jiffies;
> + netif_trans_update(dev);
> }
>
> if (!test_bit(F_TX_WAIT_ALL, &priv->flags))
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index 3400fd1..71f0e79 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -521,7 +521,7 @@ static void ems_usb_write_bulk_callback(struct urb *urb)
> if (urb->status)
> netdev_info(netdev, "Tx URB aborted (%d)\n", urb->status);
>
> - netdev->trans_start = jiffies;
> + netif_trans_update(netdev);
>
> /* transmission complete interrupt */
> netdev->stats.tx_packets++;
> @@ -835,7 +835,7 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff
> *skb, struct net_device *ne
> stats->tx_dropped++;
> }
> } else {
> - netdev->trans_start = jiffies;
> + netif_trans_update(netdev);
>
> /* Slow down tx path */
> if (atomic_read(&dev->active_tx_urbs) >= MAX_TX_URBS ||
> diff --git a/drivers/net/can/usb/esd_usb2.c b/drivers/net/can/usb/esd_usb2.c
> index 113e64f..784a900 100644
> --- a/drivers/net/can/usb/esd_usb2.c
> +++ b/drivers/net/can/usb/esd_usb2.c
> @@ -480,7 +480,7 @@ static void esd_usb2_write_bulk_callback(struct urb *urb)
> if (urb->status)
> netdev_info(netdev, "Tx URB aborted (%d)\n", urb->status);
>
> - netdev->trans_start = jiffies;
> + netif_trans_update(netdev);
> }
>
> static ssize_t show_firmware(struct device *d,
> @@ -820,7 +820,7 @@ static netdev_tx_t esd_usb2_start_xmit(struct sk_buff
> *skb,
> goto releasebuf;
> }
>
> - netdev->trans_start = jiffies;
> + netif_trans_update(netdev);
>
> /*
>* Release our reference to this URB, the USB core will eventually free
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> index 5a2e341..bfb91d8 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> @@ -274,7 +274,7 @@ static void peak_usb_write_bulk_callback(struct urb *urb)
> netdev->stats.tx_bytes += context->data_len;
>
> /* prevent tx timeout */
> - netdev->trans_start = jiffies;
> + netif_trans_update(netdev);
> break;
>
> default:
> @@ -373,7 +373,7 @@ static netdev_tx_t peak_usb_ndo_start_xmit(struct sk_buff
> *skb,
> stats->tx_dropped++;
> }
> } else {
> - netdev->trans_start = jiffies;
> + netif_trans_update(netdev);
>
> /* slow down tx path */
> if (atomic_read(&dev->active_tx_urbs) >= PCAN_USB_MAX_TX_URBS)
For the drivers/can part:
Acked-by: Marc Kleine-Budde
regards,
Marc
--
Pengutro
[PATCHv2] scsi_lib: Decode T-10 vendor IDs
Some arrays / HBAs will only present T-10 vendor IDs, so we
should be decoding them, too.
Suggested-by: Paul Mackerras
Tested-by: Paul Mackerras
Signed-off-by: Hannes Reinecke
---
drivers/scsi/scsi_lib.c | 16
1 file changed, 16 insertions(+)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b920c5d..3082de2 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3064,6 +3064,7 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id,
size_t id_len)
* - EUI-64 based 12-byte
* - NAA IEEE Registered
* - NAA IEEE Extended
+* - T10 Vendor ID
* as longer descriptors reduce the likelyhood
* of identification clashes.
*/
@@ -3082,6 +3083,21 @@ int scsi_vpd_lun_id(struct scsi_device *sdev, char *id,
size_t id_len)
goto next_desig;
switch (d[1] & 0xf) {
+ case 0x1:
+ /* T-10 Vendor ID */
+ if (cur_id_size > d[3])
+ break;
+ /* Prefer anything */
+ if (cur_id_type > 0x01 && cur_id_type != 0xff)
+ break;
+ cur_id_size = d[3];
+ if (cur_id_size + 4 > id_len)
+ cur_id_size = id_len - 4;
+ cur_id_str = d + 4;
+ cur_id_type = d[1] & 0xf;
+ id_size = snprintf(id, id_len, "t10.%*pE",
+ cur_id_size, cur_id_str);
+ break;
case 0x2:
/* EUI-64 */
if (cur_id_size > d[3])
--
1.8.5.6
--
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] megaraid: add scsi_cmnd NULL check before use
Smatch doesn't quite catch it because we check "cmd_fusion->scmd" for NULL then assign "scmd_local = cmd_fusion->scmd;" and dereference scmd_local unconditionally... It does catch part of the bug if you have cross function analysis: drivers/scsi/megaraid/megaraid_sas_fusion.c:2318 complete_cmd_fusion() error: we previously assumed 'cmd_fusion->scmd' could be null (see line 2281) But that code was from 2010 so I never reported it to the original author or the list. regards, dan carpenter -- 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] megaraid: add scsi_cmnd NULL check before use
I've updated Smatch to catch these and I'm testing it now. We'll see how it goes. If my quick and dirty method doesn't has too many false positives then it's will take some months before I get around to doing it properly... Thanks for notifying me on this. regards, dan carpenter -- 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] megaraid: add scsi_cmnd NULL check before use
> -Original Message- > From: Dan Carpenter [mailto:dan.carpen...@oracle.com] > Sent: Monday, May 09, 2016 1:36 PM > To: Finn Thain > Cc: Petros Koutoupis; kashyap.de...@avagotech.com; > sumit.sax...@avagotech.com; uday.ling...@avagotech.com; > megaraidlinux@avagotech.com; linux-scsi@vger.kernel.org > Subject: Re: [PATCH] megaraid: add scsi_cmnd NULL check before use > > Smatch doesn't quite catch it because we check "cmd_fusion->scmd" for NULL > then assign "scmd_local = cmd_fusion->scmd;" and dereference scmd_local > unconditionally... > > It does catch part of the bug if you have cross function analysis: > > drivers/scsi/megaraid/megaraid_sas_fusion.c:2318 complete_cmd_fusion() > error: we previously assumed 'cmd_fusion->scmd' could be null (see line 2281) > > But that code was from 2010 so I never reported it to the original author or the > list. "cmd_fusion->scmd" should not be NULL if scsi_io_req->Function is set to MPI2_FUNCTION_SCSI_IO_REQUEST (OR) MEGASAS_MPI2_FUNCTION_LD_IO_REQUES (inside these two cases only, cmd_fusion->scmd will be dereferenced). If cmd_fusion->scmd is NULL for these "scsi_io_req->Function", that will a BUG and should not continue with other commands processing in that case. > > regards, > dan carpenter -- 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] libfc: replace 'rp_mutex' with 'rp_lock'
On Mon, Apr 25, 2016 at 10:01:33AM +0200, Hannes Reinecke wrote: > We cannot use an embedded mutex in a structure with reference > counting, as mutex unlock might be delayed, and the waiters > might then access an already freed memory area. > So convert it to a spinlock. > > For details cf https://lkml.org/lkml/2015/2/11/245 > > Signed-off-by: Hannes Reinecke Reviewed-by: Johannes Thumshirn -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 -- 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] Fix a bdi reregistration race, v3
On 05/05/2016 04:40 PM, Joe Lawrence wrote: > On 05/05/2016 03:58 PM, Bart Van Assche wrote: >> On 03/28/2016 02:29 PM, Bart Van Assche wrote: >>> Avoid that the sd driver registers a BDI device with a name that >>> is still in use. This patch avoids that the following warning gets >>> triggered: >>> >>> [ ... ] >> >> (replying to my own e-mail) >> >> If anyone could review this patch that would be very welcome. > > Hi Bart, > > I *think* I may be hitting this same problem running some tests here at > Stratus > ... snip... Hi Bart, Good news = With your v3 patch, I didn't see the "sysfs: cannot create duplicate filename '/devices/virtual/bdi/65:0'" warning during my weekend testing (573 surprise disk HBA removals). Bad news = I still crashed in add_disk > sysfs_create_link > sysfs_do_create_link_sd on a NULL target_kobj->sd ... unfortunately I don't have kdump working, so all I have is a serial console output to work with for now. Regards, -- Joe -- 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] tcm_qla2xxx Add SCSI command jammer/discard capability to the tcm_qla2xxx module
- Original Message -
> From: "Laurence Oberman"
> To: "Nicholas A. Bellinger"
> Cc: "Himanshu Madhani" , "Bart Van Assche"
> , "linux-scsi"
> , "target-devel" ,
> "Quinn Tran"
> Sent: Monday, April 4, 2016 6:50:03 PM
> Subject: Re: [PATCH] tcm_qla2xxx Add SCSI command jammer/discard capability
> to the tcm_qla2xxx module
>
> Hello Nicholas
>
> Its fixed now.
> Many Thanks.
>
> $ scripts/checkpatch.pl
> 0001-tcm_qla2xxx-Add-SCSI-command-jammer-discard-capabili.patch
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #12:
> new file mode 100644
>
> total: 0 errors, 1 warnings, 91 lines checked
>
> 0001-tcm_qla2xxx-Add-SCSI-command-jammer-discard-capabili.patch has style
> problems, please review.
>
> NOTE: If any of the errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>
>
>
> Tested by: Laurence Oberman
> Signed-off-by: Laurence Oberman
> ---
> Documentation/scsi/tcm_qla2xxx.txt | 22 ++
> drivers/scsi/qla2xxx/Kconfig |9 +
> drivers/scsi/qla2xxx/tcm_qla2xxx.c | 20
> drivers/scsi/qla2xxx/tcm_qla2xxx.h |1 +
> 4 files changed, 52 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/scsi/tcm_qla2xxx.txt
>
> diff --git a/Documentation/scsi/tcm_qla2xxx.txt
> b/Documentation/scsi/tcm_qla2xxx.txt
> new file mode 100644
> index 000..c3a670a
> --- /dev/null
> +++ b/Documentation/scsi/tcm_qla2xxx.txt
> @@ -0,0 +1,22 @@
> +tcm_qla2xxx jam_host attribute
> +--
> +There is now a new module endpoint atribute called jam_host
> +attribute: jam_host: boolean=0/1
> +This attribute and accompanying code is only included if the
> +Kconfig parameter TCM_QLA2XXX_DEBUG is set to Y
> +By default this jammer code and functionality is disabled
> +
> +Use this attribute to control the discarding of SCSI commands to a
> +selected host.
> +This may be useful for testing error handling and simulating slow drain
> +and other fabric issues.
> +
> +Setting a boolean of 1 for the jam_host attribute for a particular host
> + will discard the commands for that host.
> +Reset back to 0 to stop the jamming.
> +
> +Enable host 4 to be jammed
> +echo 1 >
> /sys/kernel/config/target/qla2xxx/21:00:00:24:ff:27:8f:ae/tpgt_1/attrib/jam_host
> +
> +Disable jamming on host 4
> +echo 0 >
> /sys/kernel/config/target/qla2xxx/21:00:00:24:ff:27:8f:ae/tpgt_1/attrib/jam_host
> diff --git a/drivers/scsi/qla2xxx/Kconfig b/drivers/scsi/qla2xxx/Kconfig
> index 10aa18b..67c0d5a 100644
> --- a/drivers/scsi/qla2xxx/Kconfig
> +++ b/drivers/scsi/qla2xxx/Kconfig
> @@ -36,3 +36,12 @@ config TCM_QLA2XXX
> default n
> ---help---
> Say Y here to enable the TCM_QLA2XXX fabric module for QLogic 24xx+
> series
> target mode HBAs
> +
> +if TCM_QLA2XXX
> +config TCM_QLA2XXX_DEBUG
> + bool "TCM_QLA2XXX fabric module DEBUG mode for QLogic 24xx+ series
> target
> mode HBAs"
> + default n
> + ---help---
> + Say Y here to enable the TCM_QLA2XXX fabric module DEBUG for QLogic
> 24xx+
> series target mode HBAs
> + This will include code to enable the SCSI command jammer
> +endif
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index 1808a01..948224e 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -457,6 +457,10 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha,
> struct qla_tgt_cmd *cmd,
> struct se_cmd *se_cmd = &cmd->se_cmd;
> struct se_session *se_sess;
> struct qla_tgt_sess *sess;
> +#ifdef CONFIG_TCM_QLA2XXX_DEBUG
> + struct se_portal_group *se_tpg;
> + struct tcm_qla2xxx_tpg *tpg;
> +#endif
> int flags = TARGET_SCF_ACK_KREF;
>
> if (bidi)
> @@ -477,6 +481,15 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha,
> struct qla_tgt_cmd *cmd,
> return -EINVAL;
> }
>
> +#ifdef CONFIG_TCM_QLA2XXX_DEBUG
> + se_tpg = se_sess->se_tpg;
> + tpg = container_of(se_tpg, struct tcm_qla2xxx_tpg, se_tpg);
> + if (unlikely(tpg->tpg_attrib.jam_host)) {
> + /* return, and dont run target_submit_cmd,discarding command */
> + return 0;
> + }
> +#endif
> +
> cmd->vha->tgt_counters.qla_core_sbt_cmd++;
> return target_submit_cmd(se_cmd, se_sess, cdb, &cmd->sense_buffer[0],
> cmd->unpacked_lun, data_length, fcp_task_attr,
> @@ -844,6 +857,9 @@ DEF_QLA_TPG_ATTRIB(cache_dynamic_acls);
> DEF_QLA_TPG_ATTRIB(demo_mode_write_protect);
> DEF_QLA_TPG_ATTRIB(prod_mode_write_protect);
> DEF_QLA_TPG_ATTRIB(demo_mode_login_only);
> +#ifdef CONFIG_TCM_QLA2XXX_DEBUG
> +DEF_QLA_TPG_ATTRIB(jam_host);
> +#endif
>
> static struct configfs_attribute *tcm_qla2xxx_tpg_attrib_attrs[] = {
> &tcm_qla2xxx_tpg_attrib_attr_generate_node_acls,
> @@ -851,6 +867,9 @@ static struct configfs_attribute
> *tcm_qla2x
Re: [PATCH v3 3/6] scsi_debug: add multiple queue support
On 05/05/2016 09:40 PM, Douglas Gilbert wrote:
static bool stop_queued_cmnd(struct scsi_cmnd *cmnd)
{
unsigned long iflags;
- int k, qmax, r_qmax;
+ int j, k, qmax, r_qmax;
+ struct sdebug_queue *sqp;
struct sdebug_queued_cmd *sqcp;
struct sdebug_dev_info *devip;
struct sdebug_defer *sd_dp;
- [ ... ]
+ for (j = 0, sqp = sdebug_q_arr; j < submit_queues; ++j, ++sqp) {
+ spin_lock_irqsave(&sqp->qc_lock, iflags);
+ qmax = sdebug_max_queue;
+ r_qmax = atomic_read(&retired_max_queue);
+ if (r_qmax > qmax)
+ qmax = r_qmax;
+ for (k = 0; k < qmax; ++k) {
+ if (test_bit(k, sqp->in_use_bm)) {
+ sqcp = &sqp->qc_arr[k];
+ if (cmnd != sqcp->a_cmnd)
+ continue;
+ /* found */
+ [ ... ]
It is not clear to me why a for-loop is used in this function to look up
the sqp pointer instead of calling get_queue() or using sqa_idx?
static const char * scsi_debug_info(struct Scsi_Host * shp)
{
- sprintf(sdebug_info,
- "scsi_debug, version %s [%s], dev_size_mb=%d, opts=0x%x",
- SDEBUG_VERSION, sdebug_version_date, sdebug_dev_size_mb,
- sdebug_opts);
+ int k;
+
+ k = scnprintf(sdebug_info, sizeof(sdebug_info),
+ "%s: version %s [%s], dev_size_mb=%d, opts=0x%x\n",
+ my_name, SDEBUG_VERSION, sdebug_version_date,
+ sdebug_dev_size_mb, sdebug_opts);
+ if (k >= (sizeof(sdebug_info) - 1))
+ return sdebug_info;
+ scnprintf(sdebug_info + k, sizeof(sdebug_info) - k,
+ "%s: submit_queues=%d, statistics=%d\n", my_name,
+ submit_queues, (int)sdebug_statistics);
return sdebug_info;
}
From the comment above the definition of scnprintf(): "The return value
is the number of characters written into @buf not including the trailing
'\0'". Maybe you need snprintf() instead of scnprintf()?
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: [RFC PATCH 0/4] dm mpath: vastly improve blk-mq IO performance
Mike Snitzer redhat.com> writes: > I developed these changes some weeks ago but have since focused on > regression and performance testing on larger NUMA systems. Hello Mike, Sorry that I do yet have any performance numbers available for the SRP protocol for this patch series. But I want to let you know that since I started testing this patch series three weeks ago that I haven't seen any regressions caused by this patch series. 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: [PATCHv3 00/14] libata: ZAC support
On Fri, May 06, 2016 at 01:05:36PM +0200, Hannes Reinecke wrote: > On 04/25/2016 10:16 PM, Tejun Heo wrote: > > Hello, > > > > On Mon, Apr 25, 2016 at 12:45:42PM +0200, Hannes Reinecke wrote: > >> here's a patchset implementing ZAC support for libata. > >> > >> This is the second part of a larger patchset for ZAC/ZBC support; > >> it requires the scsi trace fixes queued for in mkp/4.7/scsi-queue and > >> the patchset 'libata: SATL update' queued in tj/for-4.7-zac. > > > > The patches look good from libata side. If others are okay with it, I > > can pull mkp/4.7/scsi-queue into tj/for-4.7-zac and apply this series > > on top of it. > > > Ping? > > I can easily split off the libsas bits into a different patchset if > this turns out to be an issue. > But it would be really good if this could make it 4.7, it will help > further integration a _lot_. Pulled in mkp/4.7/scsi-queue into libata/for-4.7-zac and applied the patches on top. Thanks. -- tejun -- 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: [PATCHv2]sd: Don't treat succeeded SYNC as error
On Wed, May 4, 2016 at 7:02 PM, Jinpu Wang wrote: > On Mon, May 2, 2016 at 3:44 PM, James Bottomley > wrote: >> On Mon, 2016-05-02 at 12:05 +0200, Hannes Reinecke wrote: >>> On 04/29/2016 02:49 PM, Jinpu Wang wrote: >>> > Hi, all >>> > >>> > We hit IO error on fsync, it turns out was because sd treat >>> > succeeded >>> > SYNC as error. From what I checked in SBC spec there is no >>> > indication >>> > we should fail IO in this case, so we create this patch. >>> > >>> > >>> > Best Regards, >>> > >>> > Jack Wang >>> > >>> > v2: >>> > No change on patch itself, only resend in body as suggested by >>> > Bart, >>> > still keep the attachment in case mail client break the format. >>> > >>> > From 5d1f72d9643ce61cd9f3d312377378c43f171d0c Mon Sep 17 00:00:00 >>> > 2001 >>> > From: Jack Wang >>> > Date: Mon, 25 Apr 2016 12:05:22 +0200 >>> > Subject: [PATCH] sd: Don't treat succeeded SYNC as error >>> > >>> > We hit IO error in our production on multipath devices during >>> > resize >>> > device on target side, the problem turns out sd driver passes up as >>> > IO >>> > error when sense data is UNIT_ATTENTION and ASC && ASCQ indicate >>> > Capacity data has changed, even storage side sync the data >>> > properly. >>> > >>> > In order to fix this check in sd_done, report success if condition >>> > matches. >>> > >>> > Sebastian Parschauer report/analyze the bug here: >>> > https://sourceforge.net/p/scst/mailman/message/34953416/ >>> > >>> > Signed-off-by: Sebastian Parschauer >>> > Signed-off-by: Jack Wang >>> > --- >>> > drivers/scsi/sd.c | 13 + >>> > 1 file changed, 13 insertions(+) >>> > >>> Well. >>> Is there anything which guarantees us that 'capacity data has >>> changed' will be the only sense code which we'll be seeing as a >>> response to SYNCHRONIZE CACHE? >>> I sincerely doubt so. >>> So why don't you fall back to the default action (ie retry the >>> command) whenever you hit an UNIT ATTENTION? >>> This way we would cove any resulting sense code, _and_ would get rid >>> of the rather ugly special case here. >> >> Actually, why are we getting here at all? should we be eating this >> unit attention once we've reported it in scsi_check_sense()? >> >> I also don't quite understand why the normal retry mechanism in >> scsi_io_completion() (called after drv->done()) isn't handling this. >> We set retries on a flush command and we give sd_sync_cache three >> goes. Any one of those should also cause the CC/UA to be ignored. >> >> James >> >> > > Sorry for delay, I agree safer to retry this command. > I checked the code path, in scsi_io_completion, we call > __scsi_error_from_host_byte for FLUSH request, > and we set error to EIO by default, somehow the code report error > directly to user space without retry. > [ 647.206270] sd 1:0:0:0: [sdb] tag#0 Send: scmd 0x8800b6558480 > [ 647.206422] sd 1:0:0:0: [sdb] tag#0 CDB: Synchronize Cache(10) 35 > 00 00 00 00 00 00 00 00 00 > [ 647.209748] sd 1:0:0:0: Capacity data has changed > [ 647.209896] sd 1:0:0:0: [sdb] tag#0 Done: SUCCESS Result: > hostbyte=DID_OK driverbyte=DRIVER_OK > [ 647.210157] sd 1:0:0:0: [sdb] tag#0 CDB: Synchronize Cache(10) 35 > 00 00 00 00 00 00 00 00 00 > [ 647.210419] sd 1:0:0:0: [sdb] tag#0 Sense Key : Unit Attention [current] > [ 647.210567] sd 1:0:0:0: [sdb] tag#0 Add. Sense: Capacity data has changed > [ 647.210741] sd 1:0:0:0: [sdb] tag#0 scsi host busy 1 failed 0 > [ 647.210888] sd 1:0:0:0: Notifying upper driver of completion (result > 802) > [ 647.211035] sd 1:0:0:0: [sdb] tag#0 sd_done: completed 0 of 0 bytes > [ 647.211182] sd 1:0:0:0: [sdb] tag#0 0 sectors total, 0 bytes done, error -5 > [ 647.211330] blk_update_request: I/O error, dev sdb, sector 0 > > Will figure out why retry doesn't work. > > Thanks James and Hannes for all your input. > > Regards, > Jack Hi James, Hannes and all, I find out it's code below which report error directly back to user space without any retry. 913 /* 914 * If we finished all bytes in the request we are done now. 915 */ 916 if (!scsi_end_request(req, error, good_bytes, 0)) 917 return; But not sure, what's the best way to fix the behavior to let it retry, maybe add condition with sense key && asc && ascq direct go to requeue before line 913? Thanks Jack -- 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] tcm_qla2xxx Add SCSI command jammer/discard capability to the tcm_qla2xxx module
On 5/9/16, 7:56 AM, "Laurence Oberman" wrote:
>
>
>- Original Message -
>> From: "Laurence Oberman"
>> To: "Nicholas A. Bellinger"
>> Cc: "Himanshu Madhani" , "Bart Van Assche"
>> , "linux-scsi"
>> , "target-devel" ,
>> "Quinn Tran"
>> Sent: Monday, April 4, 2016 6:50:03 PM
>> Subject: Re: [PATCH] tcm_qla2xxx Add SCSI command jammer/discard capability
>> to the tcm_qla2xxx module
>>
>> Hello Nicholas
>>
>> Its fixed now.
>> Many Thanks.
>>
>> $ scripts/checkpatch.pl
>> 0001-tcm_qla2xxx-Add-SCSI-command-jammer-discard-capabili.patch
>> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
>> #12:
>> new file mode 100644
>>
>> total: 0 errors, 1 warnings, 91 lines checked
>>
>> 0001-tcm_qla2xxx-Add-SCSI-command-jammer-discard-capabili.patch has style
>> problems, please review.
>>
>> NOTE: If any of the errors are false positives, please report
>> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>>
>>
>>
>> Tested by: Laurence Oberman
>> Signed-off-by: Laurence Oberman
>> ---
>> Documentation/scsi/tcm_qla2xxx.txt | 22 ++
>> drivers/scsi/qla2xxx/Kconfig |9 +
>> drivers/scsi/qla2xxx/tcm_qla2xxx.c | 20
>> drivers/scsi/qla2xxx/tcm_qla2xxx.h |1 +
>> 4 files changed, 52 insertions(+), 0 deletions(-)
>> create mode 100644 Documentation/scsi/tcm_qla2xxx.txt
>>
>> diff --git a/Documentation/scsi/tcm_qla2xxx.txt
>> b/Documentation/scsi/tcm_qla2xxx.txt
>> new file mode 100644
>> index 000..c3a670a
>> --- /dev/null
>> +++ b/Documentation/scsi/tcm_qla2xxx.txt
>> @@ -0,0 +1,22 @@
>> +tcm_qla2xxx jam_host attribute
>> +--
>> +There is now a new module endpoint atribute called jam_host
>> +attribute: jam_host: boolean=0/1
>> +This attribute and accompanying code is only included if the
>> +Kconfig parameter TCM_QLA2XXX_DEBUG is set to Y
>> +By default this jammer code and functionality is disabled
>> +
>> +Use this attribute to control the discarding of SCSI commands to a
>> +selected host.
>> +This may be useful for testing error handling and simulating slow drain
>> +and other fabric issues.
>> +
>> +Setting a boolean of 1 for the jam_host attribute for a particular host
>> + will discard the commands for that host.
>> +Reset back to 0 to stop the jamming.
>> +
>> +Enable host 4 to be jammed
>> +echo 1 >
>> /sys/kernel/config/target/qla2xxx/21:00:00:24:ff:27:8f:ae/tpgt_1/attrib/jam_host
>> +
>> +Disable jamming on host 4
>> +echo 0 >
>> /sys/kernel/config/target/qla2xxx/21:00:00:24:ff:27:8f:ae/tpgt_1/attrib/jam_host
>> diff --git a/drivers/scsi/qla2xxx/Kconfig b/drivers/scsi/qla2xxx/Kconfig
>> index 10aa18b..67c0d5a 100644
>> --- a/drivers/scsi/qla2xxx/Kconfig
>> +++ b/drivers/scsi/qla2xxx/Kconfig
>> @@ -36,3 +36,12 @@ config TCM_QLA2XXX
>> default n
>> ---help---
>> Say Y here to enable the TCM_QLA2XXX fabric module for QLogic 24xx+
>> series
>> target mode HBAs
>> +
>> +if TCM_QLA2XXX
>> +config TCM_QLA2XXX_DEBUG
>> +bool "TCM_QLA2XXX fabric module DEBUG mode for QLogic 24xx+ series
>> target
>> mode HBAs"
>> +default n
>> +---help---
>> +Say Y here to enable the TCM_QLA2XXX fabric module DEBUG for QLogic
>> 24xx+
>> series target mode HBAs
>> +This will include code to enable the SCSI command jammer
>> +endif
>> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
>> b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
>> index 1808a01..948224e 100644
>> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
>> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
>> @@ -457,6 +457,10 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha,
>> struct qla_tgt_cmd *cmd,
>> struct se_cmd *se_cmd = &cmd->se_cmd;
>> struct se_session *se_sess;
>> struct qla_tgt_sess *sess;
>> +#ifdef CONFIG_TCM_QLA2XXX_DEBUG
>> +struct se_portal_group *se_tpg;
>> +struct tcm_qla2xxx_tpg *tpg;
>> +#endif
>> int flags = TARGET_SCF_ACK_KREF;
>>
>> if (bidi)
>> @@ -477,6 +481,15 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha,
>> struct qla_tgt_cmd *cmd,
>> return -EINVAL;
>> }
>>
>> +#ifdef CONFIG_TCM_QLA2XXX_DEBUG
>> +se_tpg = se_sess->se_tpg;
>> +tpg = container_of(se_tpg, struct tcm_qla2xxx_tpg, se_tpg);
>> +if (unlikely(tpg->tpg_attrib.jam_host)) {
>> +/* return, and dont run target_submit_cmd,discarding command */
>> +return 0;
>> +}
>> +#endif
>> +
>> cmd->vha->tgt_counters.qla_core_sbt_cmd++;
>> return target_submit_cmd(se_cmd, se_sess, cdb, &cmd->sense_buffer[0],
>> cmd->unpacked_lun, data_length, fcp_task_attr,
>> @@ -844,6 +857,9 @@ DEF_QLA_TPG_ATTRIB(cache_dynamic_acls);
>> DEF_QLA_TPG_ATTRIB(demo_mode_write_protect);
>> DEF_QLA_TPG_ATTRIB(prod_mode_write_protect);
>> DEF_QLA_TPG_ATTRIB(demo_mode_login_only);
>> +#ifdef CONFIG_TCM_QLA2XXX_DEBUG
>> +DEF_QLA_TPG_ATTRIB(jam_host);
>> +#endif
>>
>> static struct configfs_attrib
[PATCH] hpsa: Fix type ZBC conditional checks
The device ID obtained from the inquiry can only be of a single type.
The original code places a check for TYPE_ZBC right after the check
for TYPE_DISK. Logically, if the first if statement sees a device of
a TYPE_DISK and moves on to the second statement checking if not
TYPE_ZBC, it will always hit the continue.
Signed-off-by: Petros Koutoupis
Acked-by: Don Brace
--- linux/drivers/scsi/hpsa.c.orig 2016-04-27 21:43:44.463140419 -0500
+++ linux/drivers/scsi/hpsa.c 2016-04-27 22:45:31.015140419 -0500
@@ -1637,9 +1637,8 @@ static void hpsa_figure_phys_disk_ptrs(s
for (j = 0; j < ndevices; j++) {
if (dev[j] == NULL)
continue;
- if (dev[j]->devtype != TYPE_DISK)
- continue;
- if (dev[j]->devtype != TYPE_ZBC)
+ if ((dev[j]->devtype != TYPE_DISK) &&
+ (dev[j]->devtype != TYPE_ZBC))
continue;
if (is_logical_device(dev[j]))
continue;
@@ -1684,9 +1683,8 @@ static void hpsa_update_log_drive_phys_d
for (i = 0; i < ndevices; i++) {
if (dev[i] == NULL)
continue;
- if (dev[i]->devtype != TYPE_DISK)
- continue;
- if (dev[i]->devtype != TYPE_ZBC)
+ if ((dev[i]->devtype != TYPE_DISK) &&
+ (dev[i]->devtype != TYPE_ZBC))
continue;
if (!is_logical_device(dev[i]))
continue;
--
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] megaraid: add scsi_cmnd NULL check before use
I'm confused what you are saying. According to Petros, these bugs are causing failures in real life, I was only added to the CC list because Smatch should have warned about them. regards, dan carpenter -- 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] megaraid: add scsi_cmnd NULL check before use
On Mon, 2016-05-09 at 15:18 +0530, Sumit Saxena wrote: > > > > -Original Message- > > From: Dan Carpenter [mailto:dan.carpen...@oracle.com] > > Sent: Monday, May 09, 2016 1:36 PM > > To: Finn Thain > > Cc: Petros Koutoupis; kashyap.de...@avagotech.com; > > sumit.sax...@avagotech.com; uday.ling...@avagotech.com; > > megaraidlinux@avagotech.com; linux-scsi@vger.kernel.org > > Subject: Re: [PATCH] megaraid: add scsi_cmnd NULL check before use > > > > Smatch doesn't quite catch it because we check "cmd_fusion->scmd" for > NULL > > > > then assign "scmd_local = cmd_fusion->scmd;" and dereference scmd_local > > unconditionally... > > > > It does catch part of the bug if you have cross function analysis: > > > > drivers/scsi/megaraid/megaraid_sas_fusion.c:2318 complete_cmd_fusion() > > error: we previously assumed 'cmd_fusion->scmd' could be null (see > line 2281) > > > > > > But that code was from 2010 so I never reported it to the original > author or the > > > > list. > "cmd_fusion->scmd" should not be NULL if scsi_io_req->Function is set to > MPI2_FUNCTION_SCSI_IO_REQUEST (OR) MEGASAS_MPI2_FUNCTION_LD_IO_REQUES > (inside these two cases only, cmd_fusion->scmd will be dereferenced). If > cmd_fusion->scmd is NULL for these "scsi_io_req->Function", that will a > BUG and > should not continue with other commands processing in that case. > Sumit, To clarify, a detection of cmd_fusion->scmd being NULL with scsi_io_req->Function set to MPI2_FUNCTION_SCSI_IO_REQUEST or MEGASAS_MPI2_FUNCTION_LD_IO_REQUEST should instead trigger a BUG() and not attempt to iterate to the next command in the list. Thank you. -- Petros -- 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 v3 3/6] scsi_debug: add multiple queue support
On 2016-05-09 06:12 PM, Bart Van Assche wrote:
On 05/05/2016 09:40 PM, Douglas Gilbert wrote:
static bool stop_queued_cmnd(struct scsi_cmnd *cmnd)
{
unsigned long iflags;
-int k, qmax, r_qmax;
+int j, k, qmax, r_qmax;
+struct sdebug_queue *sqp;
struct sdebug_queued_cmd *sqcp;
struct sdebug_dev_info *devip;
struct sdebug_defer *sd_dp;
-[ ... ]
+for (j = 0, sqp = sdebug_q_arr; j < submit_queues; ++j, ++sqp) {
+spin_lock_irqsave(&sqp->qc_lock, iflags);
+qmax = sdebug_max_queue;
+r_qmax = atomic_read(&retired_max_queue);
+if (r_qmax > qmax)
+qmax = r_qmax;
+for (k = 0; k < qmax; ++k) {
+if (test_bit(k, sqp->in_use_bm)) {
+sqcp = &sqp->qc_arr[k];
+if (cmnd != sqcp->a_cmnd)
+continue;
+/* found */
+[ ... ]
It is not clear to me why a for-loop is used in this function to look up the sqp
pointer instead of calling get_queue() or using sqa_idx?
Yes get_queue() could be used to remove the outer loop.
sqa_idx is found in the sdebug_defer structure and instances of it
exist only for scsi commands that are currently deferred (on or work
item or a hr timer). There is a pointer from scsi_defer to the
corresponding scsi_cmnd but not vice versa.
Using the every_nth parameter, scsi commands can be ignored, meaning
they are neither deferred (queued) nor completed (by calling scsi_done()).
In this case the driver just returns on the command delivery thread
(without error) and forgets about that scsi command instance. It is
simulating an unreported transport or LLD failure. So there is an
extremely high chance the mid-level will attempt to abort that command.
In that case there is no related sqa_idx and that scsi command will not
be found by stop_queued_cmnd(). That is fine since there are no related
resources to free up.
So IMO the code is correct as it stands but its efficiency could be
improved by using get_queue() to replace the outer loop.
static const char * scsi_debug_info(struct Scsi_Host * shp)
{
-sprintf(sdebug_info,
-"scsi_debug, version %s [%s], dev_size_mb=%d, opts=0x%x",
-SDEBUG_VERSION, sdebug_version_date, sdebug_dev_size_mb,
-sdebug_opts);
+int k;
+
+k = scnprintf(sdebug_info, sizeof(sdebug_info),
+ "%s: version %s [%s], dev_size_mb=%d, opts=0x%x\n",
+ my_name, SDEBUG_VERSION, sdebug_version_date,
+ sdebug_dev_size_mb, sdebug_opts);
+if (k >= (sizeof(sdebug_info) - 1))
+return sdebug_info;
+scnprintf(sdebug_info + k, sizeof(sdebug_info) - k,
+ "%s: submit_queues=%d, statistics=%d\n", my_name,
+ submit_queues, (int)sdebug_statistics);
return sdebug_info;
}
From the comment above the definition of scnprintf(): "The return value is the
number of characters written into @buf not including the trailing '\0'". Maybe
you need snprintf() instead of scnprintf()?
The maximum return value from the first scnprintf(sdebug_info ...)
is (sizeof(sdebug_info) - 1)). So strictly speaking the early return
comparison should be "==" for scnprintf and ">=" for snprintf. Given
that snprintf is more dangerous then scnprintf (e.g. with repeated
"k += snprintf(buff + k, buff_len - k, ...)" statements) then I would
prefer to keep scnprintf and yes, ">=" is overkill but will work.
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 v3 3/6] scsi_debug: add multiple queue support
On 05/09/2016 03:30 PM, Douglas Gilbert wrote:> The maximum return value from the first scnprintf(sdebug_info ...) is (sizeof(sdebug_info) - 1)). So strictly speaking the early return comparison should be "==" for scnprintf and ">=" for snprintf. Given that snprintf is more dangerous then scnprintf (e.g. with repeated "k += snprintf(buff + k, buff_len - k, ...)" statements) then I would prefer to keep scnprintf and yes, ">=" is overkill but will work. Thanks for the feedback. I'm fine with the other changes in this patch, hence: Reviewed-by: Bart Van Assche -- 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 resend v2] [SCSI] bfa: fix bfa_fcb_itnim_alloc() error handling
> "Anil" == Anil Gurumurthy writes: Anil> Apologies for the delay. Patch looks good. Acked by: Anil Anil> Gurumurthy Applied to 4.7/scsi-queue. -- Martin K. Petersen Oracle Linux Engineering -- 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
