Re: [PATCH 2/6] blk-mq: call blk_mq_start_request from ->queue_rq
On Sun, Sep 14, 2014 at 7:40 AM, Christoph Hellwig wrote:
> When we call blk_mq_start_request from the core blk-mq code before calling
> into
> ->queue_rq there is a racy window where the timeout handler can hit before
> we've
> fully set up the driver specific part of the command.
It is quite difficult to happen since the default timeout is 30s, which
is long enough.
Suppose it happens, what is the matter without setting up request's pdu
since the request can only be completed one time thanks to flag of
REQ_ATOM_COMPLETE?
>
> Move the call to blk_mq_start_request into the driver so the driver can start
> the request only once it is fully set up.
The change isn't friendly for drivers since every drivers need to do that and
it should have been done in blk-mq core code.
Also the timeout handler still may see pdu of request not setup because
writing rq in blk_mq_start_request() and writing on pdu may be reordered.
>
> Signed-off-by: Christoph Hellwig
> ---
> block/blk-mq.c| 13 ++---
> drivers/block/mtip32xx/mtip32xx.c | 2 ++
> drivers/block/null_blk.c | 2 ++
> drivers/block/virtio_blk.c| 2 ++
> drivers/scsi/scsi_lib.c | 1 +
> include/linux/blk-mq.h| 1 +
> 6 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 42c94c8..cab 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -380,7 +380,7 @@ void blk_mq_complete_request(struct request *rq)
> }
> EXPORT_SYMBOL(blk_mq_complete_request);
>
> -static void blk_mq_start_request(struct request *rq)
> +void blk_mq_start_request(struct request *rq)
> {
> struct request_queue *q = rq->q;
>
> @@ -412,16 +412,18 @@ static void blk_mq_start_request(struct request *rq)
> rq->nr_phys_segments++;
> }
> }
> +EXPORT_SYMBOL(blk_mq_start_request);
>
> static void __blk_mq_requeue_request(struct request *rq)
> {
> struct request_queue *q = rq->q;
>
> trace_block_rq_requeue(q, rq);
> - clear_bit(REQ_ATOM_STARTED, &rq->atomic_flags);
>
> - if (q->dma_drain_size && blk_rq_bytes(rq))
> - rq->nr_phys_segments--;
> + if (test_and_clear_bit(REQ_ATOM_STARTED, &rq->atomic_flags)) {
> + if (q->dma_drain_size && blk_rq_bytes(rq))
> + rq->nr_phys_segments--;
> + }
> }
>
> void blk_mq_requeue_request(struct request *rq)
> @@ -729,8 +731,6 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx
> *hctx)
> rq = list_first_entry(&rq_list, struct request, queuelist);
> list_del_init(&rq->queuelist);
>
> - blk_mq_start_request(rq);
> -
> ret = q->mq_ops->queue_rq(hctx, rq, list_empty(&rq_list));
> switch (ret) {
> case BLK_MQ_RQ_QUEUE_OK:
> @@ -1177,7 +1177,6 @@ static void blk_mq_make_request(struct request_queue
> *q, struct bio *bio)
> int ret;
>
> blk_mq_bio_to_request(rq, bio);
> - blk_mq_start_request(rq);
>
> /*
> * For OK queue, we are done. For error, kill it. Any other
> diff --git a/drivers/block/mtip32xx/mtip32xx.c
> b/drivers/block/mtip32xx/mtip32xx.c
> index 0e2084f..4042440 100644
> --- a/drivers/block/mtip32xx/mtip32xx.c
> +++ b/drivers/block/mtip32xx/mtip32xx.c
> @@ -3783,6 +3783,8 @@ static int mtip_queue_rq(struct blk_mq_hw_ctx *hctx,
> struct request *rq,
> if (unlikely(mtip_check_unal_depth(hctx, rq)))
> return BLK_MQ_RQ_QUEUE_BUSY;
>
> + blk_mq_start_request(rq);
> +
> ret = mtip_submit_request(hctx, rq);
> if (likely(!ret))
> return BLK_MQ_RQ_QUEUE_OK;
> diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c
> index c5b7315..332ce20 100644
> --- a/drivers/block/null_blk.c
> +++ b/drivers/block/null_blk.c
> @@ -321,6 +321,8 @@ static int null_queue_rq(struct blk_mq_hw_ctx *hctx,
> struct request *rq,
> cmd->rq = rq;
> cmd->nq = hctx->driver_data;
>
> + blk_mq_start_request(rq);
> +
> null_handle_cmd(cmd);
> return BLK_MQ_RQ_QUEUE_OK;
> }
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 13756e0..83816bf 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -205,6 +205,8 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
> struct request *req,
> }
> }
>
> + blk_mq_start_request(req);
> +
> num = blk_rq_map_sg(hctx->queue, vbr->req, vbr->sg);
> if (num) {
> if (rq_data_dir(vbr->req) == WRITE)
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 1ec00ba..b06a355 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1890,6 +1890,7 @@ static int scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
> struct request *req,
> scsi_init_cmd_errh(cmd);
>
[PATCH] xen: make pvscsi frontend dependant on xenbus frontend
The pvscsi frontend driver requires the xenbus frontend driver. Reflect this in Kconfig. Signed-off-by: Juergen Gross --- drivers/scsi/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig index 9130df1..ff62dc1 100644 --- a/drivers/scsi/Kconfig +++ b/drivers/scsi/Kconfig @@ -579,6 +579,7 @@ config VMWARE_PVSCSI config XEN_SCSI_FRONTEND tristate "XEN SCSI frontend driver" depends on SCSI && XEN + select XEN_XENBUS_FRONTEND help The XEN SCSI frontend driver allows the kernel to access SCSI Devices within another guest OS (usually Dom0). -- 1.8.4.5 -- 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 v4 2/2] arcmsr: simplify of updating doneq_index and postq_index
On Fri, 2014-09-12 at 16:05 +0200, Tomas Henzl wrote:
> On 09/12/2014 10:22 AM, Ching Huang wrote:
> > From: Ching Huang
> >
> > This patch is to modify previous patch 16/17 and it is relative to
> > http://git.infradead.org/users/hch/scsi-queue.git/tree/arcmsr-for-3.18:/drivers/scsi/arcmsr
> >
> > change in v4:
> > 1. clean up of duplicate variable declaration in switch.
> > 2. simplify of updating doneq_index and postq_index
> > 3. fix spin_lock area in arcmsr_hbaD_polling_ccbdone
>
> The intention of the doneq_lock is to protect the pmu->doneq_index and the
> associated buffer,
> right? Why is the spinlock not used on other places accessing the doneq_index
> like arcmsr_done4abort_postqueue or arcmsr_hbaD_postqueue_isr ?
In fact, in original code arcmsr_hbaD_postqueue_isr has spinlock with a larger
area.
As to arcmsr_done4abort_postqueue, it should be protected as below.
@@ -1182,35 +1178,25 @@ static void arcmsr_done4abort_postqueue(
break;
case ACB_ADAPTER_TYPE_D: {
struct MessageUnit_D *pmu = acb->pmuD;
- uint32_t ccb_cdb_phy, outbound_write_pointer;
- uint32_t doneq_index, index_stripped, addressLow, residual;
- bool error;
- struct CommandControlBlock *pCCB;
+ uint32_t outbound_write_pointer;
+ uint32_t doneq_index, index_stripped, addressLow, residual,
toggle;
+ unsigned long flags;
- outbound_write_pointer = pmu->done_qbuffer[0].addressLow + 1;
- doneq_index = pmu->doneq_index;
residual = atomic_read(&acb->ccboutstandingcount);
for (i = 0; i < residual; i++) {
- while ((doneq_index & 0xFFF) !=
+ spin_lock_irqsave(&acb->doneq_lock, flags);
+ outbound_write_pointer =
+ pmu->done_qbuffer[0].addressLow + 1;
+ doneq_index = pmu->doneq_index;
+ if ((doneq_index & 0xFFF) !=
(outbound_write_pointer & 0xFFF)) {
- if (doneq_index & 0x4000) {
- index_stripped = doneq_index & 0xFFF;
- index_stripped += 1;
- index_stripped %=
- ARCMSR_MAX_ARC1214_DONEQUEUE;
- pmu->doneq_index = index_stripped ?
- (index_stripped | 0x4000) :
- (index_stripped + 1);
- } else {
- index_stripped = doneq_index;
- index_stripped += 1;
- index_stripped %=
- ARCMSR_MAX_ARC1214_DONEQUEUE;
- pmu->doneq_index = index_stripped ?
- index_stripped :
- ((index_stripped | 0x4000) + 1);
- }
+ toggle = doneq_index & 0x4000;
+ index_stripped = (doneq_index & 0xFFF) + 1;
+ index_stripped %= ARCMSR_MAX_ARC1214_DONEQUEUE;
+ pmu->doneq_index = index_stripped ?
(index_stripped | toggle) :
+ ((toggle ^ 0x4000) + 1);
doneq_index = pmu->doneq_index;
+ spin_unlock_irqrestore(&acb->doneq_lock, flags);
addressLow = pmu->done_qbuffer[doneq_index &
0xFFF].addressLow;
ccb_cdb_phy = (addressLow & 0xFFF0);
@@ -1224,11 +1210,10 @@ static void arcmsr_done4abort_postqueue(
arcmsr_drain_donequeue(acb, pCCB, error);
writel(doneq_index,
pmu->outboundlist_read_pointer);
+ } else {
+ spin_unlock_irqrestore(&acb->doneq_lock, flags);
+ mdelay(10);
}
- mdelay(10);
- outbound_write_pointer =
- pmu->done_qbuffer[0].addressLow + 1;
- doneq_index = pmu->doneq_index;
}
pmu->postq_index = 0;
pmu->doneq_index = 0x40FF;
> And in arcmsr_hbaD_polling_ccbdone the code looks so:
>
> spin_unlock_irqrestore(&acb->doneq_lock, flags);
> doneq_index = pmu->doneq_index;
> flag_ccb = pmu->done_qbuffer[doneq_index & 0xFFF].addressLow;
> you unl
Re: [PATCH v4 1/2] arcmsr: simplify ioctl data read/write
On Fri, 2014-09-12 at 15:34 +0200, Tomas Henzl wrote:
> On 09/12/2014 09:29 AM, Ching Huang wrote:
> > From: Ching Huang
> >
> > This patch is to modify previous patch 13/17 and it is relative to
> > http://git.infradead.org/users/hch/scsi-queue.git/tree/arcmsr-for-3.18:/drivers/scsi/arcmsr
> >
> > change in v4:
> > 1. for readability, rename firstindex to getIndex, rename lastindex to
> > putIndex
> For some reason, the names head+tail areusual for a circular buffer.
> But let us ignore the names, I don't care.
> > 2. define ARCMSR_API_DATA_BUFLEN as 1032
> > 3. simplify ioctl data read by macro CIRC_CNT_TO_END and CIRC_CNT
> It's definitely better when you post renames and other non-functional changes
> in separate
> patches, it's easier for the reviewer.
> >
> > Signed-off-by: Ching Huang
> > ---
> >
> > diff -uprN a/drivers/scsi/arcmsr/arcmsr_attr.c
> > b/drivers/scsi/arcmsr/arcmsr_attr.c
> > --- a/drivers/scsi/arcmsr/arcmsr_attr.c 2014-08-21 12:14:27.0
> > +0800
> > +++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-09-12 15:18:46.659125000
> > +0800
> > @@ -50,6 +50,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -68,7 +69,7 @@ static ssize_t arcmsr_sysfs_iop_message_
> > struct device *dev = container_of(kobj,struct device,kobj);
> > struct Scsi_Host *host = class_to_shost(dev);
> > struct AdapterControlBlock *acb = (struct AdapterControlBlock *)
> > host->hostdata;
> > - uint8_t *pQbuffer,*ptmpQbuffer;
> > + uint8_t *ptmpQbuffer;
> > int32_t allxfer_len = 0;
> > unsigned long flags;
> >
> > @@ -78,57 +79,22 @@ static ssize_t arcmsr_sysfs_iop_message_
> > /* do message unit read. */
> > ptmpQbuffer = (uint8_t *)buf;
> > spin_lock_irqsave(&acb->rqbuffer_lock, flags);
> > - if (acb->rqbuf_firstindex != acb->rqbuf_lastindex) {
> > - pQbuffer = &acb->rqbuffer[acb->rqbuf_firstindex];
> > - if (acb->rqbuf_firstindex > acb->rqbuf_lastindex) {
> > - if ((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex) >=
> > 1032) {
> > - memcpy(ptmpQbuffer, pQbuffer, 1032);
> > - acb->rqbuf_firstindex += 1032;
> > - acb->rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
> > - allxfer_len = 1032;
> > - } else {
> > - if (((ARCMSR_MAX_QBUFFER -
> > acb->rqbuf_firstindex)
> > - + acb->rqbuf_lastindex) > 1032) {
> > - memcpy(ptmpQbuffer, pQbuffer,
> > - ARCMSR_MAX_QBUFFER
> > - - acb->rqbuf_firstindex);
> > - ptmpQbuffer += ARCMSR_MAX_QBUFFER
> > - - acb->rqbuf_firstindex;
> > - memcpy(ptmpQbuffer, acb->rqbuffer, 1032
> > - - (ARCMSR_MAX_QBUFFER -
> > - acb->rqbuf_firstindex));
> > - acb->rqbuf_firstindex = 1032 -
> > - (ARCMSR_MAX_QBUFFER -
> > - acb->rqbuf_firstindex);
> > - allxfer_len = 1032;
> > - } else {
> > - memcpy(ptmpQbuffer, pQbuffer,
> > - ARCMSR_MAX_QBUFFER -
> > - acb->rqbuf_firstindex);
> > - ptmpQbuffer += ARCMSR_MAX_QBUFFER -
> > - acb->rqbuf_firstindex;
> > - memcpy(ptmpQbuffer, acb->rqbuffer,
> > - acb->rqbuf_lastindex);
> > - allxfer_len = ARCMSR_MAX_QBUFFER -
> > - acb->rqbuf_firstindex +
> > - acb->rqbuf_lastindex;
> > - acb->rqbuf_firstindex =
> > - acb->rqbuf_lastindex;
> > - }
> > - }
> > - } else {
> > - if ((acb->rqbuf_lastindex - acb->rqbuf_firstindex) >
> > 1032) {
> > - memcpy(ptmpQbuffer, pQbuffer, 1032);
> > - acb->rqbuf_firstindex += 1032;
> > - allxfer_len = 1032;
> > - } else {
> > - memcpy(ptmpQbuffer, pQbuffer,
> > acb->rqbuf_lastindex
> > - - acb->rqbuf_firstindex);
> > - allxfer_len = acb->rqbuf_lastindex -
> > - acb->rqbuf_firstindex;
> > -
RE: [PATCH] scsi: scsi_devinfo.c: Cleaning up unnecessarily complicated in conjunction with strncpy
> -Original Message- > From: Rickard Strandqvist [mailto:rickard_strandqv...@spectrumdigital.se] > How do you mean? > > strncpy zeroes throughout the remainder of the string "from" until the > length off to_length, or otherwise guaranteed trailing zero characters > and a warning is printed. > > Is not it exactly the functionality that is desired? Ah, I see that in man 3 strcpy: "If the length of src is less than n, strncpy() pads the remainder of dest with null bytes." I agree that should work. --- Rob ElliottHP Server Storage
Re: [PATCH] scsi: scsi_devinfo.c: Cleaning up unnecessarily complicated in conjunction with strncpy
2014-09-14 23:34 GMT+02:00 Elliott, Robert (Server Storage) :
>
>
>> -Original Message-
>> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
>> ow...@vger.kernel.org] On Behalf Of Rickard Strandqvist
> ...
>> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> ...
>> static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
>> char *from, int compatible)
>> {
>> - size_t from_length;
>> -
>> - from_length = strlen(from);
>> - strncpy(to, from, min(to_length, from_length));
>> - if (from_length < to_length) {
>> - if (compatible) {
>> - /*
>> - * NUL terminate the string if it is short.
>> - */
>> - to[from_length] = '\0';
>> - } else {
>> - /*
>> - * space pad the string if it is short.
>> - */
>> - strncpy(&to[from_length], spaces,
>> - to_length - from_length);
>> - }
>> - }
>> - if (from_length > to_length)
>> - printk(KERN_WARNING "%s: %s string '%s' is too long\n",
>> + strncpy(to, from, to_length);
>> + if (to[to_length - 1] != '\0') {
>> + to[to_length - 1] = '\0';
>> + printk(KERN_WARNING "%s: %s string '%s' is too long\n",
>> __func__, name, from);
>> + }
>
> The caller of this function, scsi_dev_info_list_add_keyed, created
> the "to" destination buffer, devinfo, with kmalloc, so it's not
> guaranteed to be full of zeros.
>
> If from_length is shorter than to_length, then this code will
> be inspecting an uninitialized character that strncpy didn't
> touch.
>
> ---
> Rob ElliottHP Server Storage
>
Hi Elliott
How do you mean?
strncpy zeroes throughout the remainder of the string "from" until the
length off to_length, or otherwise guaranteed trailing zero characters
and a warning is printed.
Is not it exactly the functionality that is desired?
Kind regards
Rickard Strandqvist
--
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] scsi: qla2xxx: qla_mr.c: Cleaning up missing null-terminate in conjunction with strncpy
Replacing strncpy with strlcpy to avoid strings that lacks null terminate. Signed-off-by: Rickard Strandqvist --- drivers/scsi/qla2xxx/qla_mr.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c index 4775baa..23bc7e2 100644 --- a/drivers/scsi/qla2xxx/qla_mr.c +++ b/drivers/scsi/qla2xxx/qla_mr.c @@ -1868,21 +1868,21 @@ qlafx00_fx_disc(scsi_qla_host_t *vha, fc_port_t *fcport, uint16_t fx_type) phost_info = &preg_hsi->hsi; memset(preg_hsi, 0, sizeof(struct register_host_info)); phost_info->os_type = OS_TYPE_LINUX; - strncpy(phost_info->sysname, + strlcpy(phost_info->sysname, p_sysid->sysname, SYSNAME_LENGTH); - strncpy(phost_info->nodename, + strlcpy(phost_info->nodename, p_sysid->nodename, NODENAME_LENGTH); if (!strcmp(phost_info->nodename, "(none)")) ha->mr.host_info_resend = true; - strncpy(phost_info->release, + strlcpy(phost_info->release, p_sysid->release, RELEASE_LENGTH); - strncpy(phost_info->version, + strlcpy(phost_info->version, p_sysid->version, VERSION_LENGTH); - strncpy(phost_info->machine, + strlcpy(phost_info->machine, p_sysid->machine, MACHINE_LENGTH); - strncpy(phost_info->domainname, + strlcpy(phost_info->domainname, p_sysid->domainname, DOMNAME_LENGTH); - strncpy(phost_info->hostdriver, + strlcpy(phost_info->hostdriver, QLA2XXX_VERSION, VERSION_LENGTH); do_gettimeofday(&tv); preg_hsi->utc = (uint64_t)tv.tv_sec; -- 1.7.10.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] scsi: qla2xxx: qla_gs.c: Cleaning up missing null-terminate in conjunction with strncpy
Replacing strncpy with strlcpy to avoid strings that lacks null terminate. Signed-off-by: Rickard Strandqvist --- drivers/scsi/qla2xxx/qla_gs.c | 34 +- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c index a0df3b1..03e3276 100644 --- a/drivers/scsi/qla2xxx/qla_gs.c +++ b/drivers/scsi/qla2xxx/qla_gs.c @@ -1300,8 +1300,8 @@ qla2x00_fdmi_rhba(scsi_qla_host_t *vha) /* Manufacturer. */ eiter = (struct ct_fdmi_hba_attr *) (entries + size); eiter->type = __constant_cpu_to_be16(FDMI_HBA_MANUFACTURER); - alen = strlen(QLA2XXX_MANUFACTURER); - strncpy(eiter->a.manufacturer, QLA2XXX_MANUFACTURER, alen + 1); + alen = strlcpy(eiter->a.manufacturer, QLA2XXX_MANUFACTURER, + sizeof(eiter->a.manufacturer)); alen += (alen & 3) ? (4 - (alen & 3)) : 4; eiter->len = cpu_to_be16(4 + alen); size += 4 + alen; @@ -1325,8 +1325,8 @@ qla2x00_fdmi_rhba(scsi_qla_host_t *vha) /* Model name. */ eiter = (struct ct_fdmi_hba_attr *) (entries + size); eiter->type = __constant_cpu_to_be16(FDMI_HBA_MODEL); - strcpy(eiter->a.model, ha->model_number); - alen = strlen(eiter->a.model); + alen = strlcpy(eiter->a.model, ha->model_number, + sizeof(eiter->a.model)); alen += (alen & 3) ? (4 - (alen & 3)) : 4; eiter->len = cpu_to_be16(4 + alen); size += 4 + alen; @@ -1337,8 +1337,8 @@ qla2x00_fdmi_rhba(scsi_qla_host_t *vha) /* Model description. */ eiter = (struct ct_fdmi_hba_attr *) (entries + size); eiter->type = __constant_cpu_to_be16(FDMI_HBA_MODEL_DESCRIPTION); - strncpy(eiter->a.model_desc, ha->model_desc, 80); - alen = strlen(eiter->a.model_desc); + alen = strlcpy(eiter->a.model_desc, ha->model_desc, + sizeof(eiter->a.model_desc)); alen += (alen & 3) ? (4 - (alen & 3)) : 4; eiter->len = cpu_to_be16(4 + alen); size += 4 + alen; @@ -1349,8 +1349,8 @@ qla2x00_fdmi_rhba(scsi_qla_host_t *vha) /* Hardware version. */ eiter = (struct ct_fdmi_hba_attr *) (entries + size); eiter->type = __constant_cpu_to_be16(FDMI_HBA_HARDWARE_VERSION); - strcpy(eiter->a.hw_version, ha->adapter_id); - alen = strlen(eiter->a.hw_version); + alen = strlcpy(eiter->a.hw_version, ha->adapter_id, + sizeof(eiter->a.hw_version)); alen += (alen & 3) ? (4 - (alen & 3)) : 4; eiter->len = cpu_to_be16(4 + alen); size += 4 + alen; @@ -1361,8 +1361,8 @@ qla2x00_fdmi_rhba(scsi_qla_host_t *vha) /* Driver version. */ eiter = (struct ct_fdmi_hba_attr *) (entries + size); eiter->type = __constant_cpu_to_be16(FDMI_HBA_DRIVER_VERSION); - strcpy(eiter->a.driver_version, qla2x00_version_str); - alen = strlen(eiter->a.driver_version); + alen = strlcpy(eiter->a.driver_version, qla2x00_version_str, + sizeof(eiter->a.driver_version)); alen += (alen & 3) ? (4 - (alen & 3)) : 4; eiter->len = cpu_to_be16(4 + alen); size += 4 + alen; @@ -1373,8 +1373,8 @@ qla2x00_fdmi_rhba(scsi_qla_host_t *vha) /* Option ROM version. */ eiter = (struct ct_fdmi_hba_attr *) (entries + size); eiter->type = __constant_cpu_to_be16(FDMI_HBA_OPTION_ROM_VERSION); - strcpy(eiter->a.orom_version, "0.00"); - alen = strlen(eiter->a.orom_version); + alen = strlcpy(eiter->a.orom_version, "0.00", + sizeof(eiter->a.orom_version)); alen += (alen & 3) ? (4 - (alen & 3)) : 4; eiter->len = cpu_to_be16(4 + alen); size += 4 + alen; @@ -1614,8 +1614,8 @@ qla2x00_fdmi_rpa(scsi_qla_host_t *vha) /* OS device name. */ eiter = (struct ct_fdmi_port_attr *) (entries + size); eiter->type = __constant_cpu_to_be16(FDMI_PORT_OS_DEVICE_NAME); - alen = strlen(QLA2XXX_DRIVER_NAME); - strncpy(eiter->a.os_dev_name, QLA2XXX_DRIVER_NAME, alen + 1); + alen = strlcpy(eiter->a.os_dev_name, QLA2XXX_DRIVER_NAME, + sizeof(eiter->a.os_dev_name)); alen += (alen & 3) ? (4 - (alen & 3)) : 4; eiter->len = cpu_to_be16(4 + alen); size += 4 + alen; @@ -1629,9 +1629,9 @@ qla2x00_fdmi_rpa(scsi_qla_host_t *vha) __constant_cpu_to_be32(FDMI_PORT_ATTR_COUNT); eiter = (struct ct_fdmi_port_attr *) (entries + size); eiter->type = __constant_cpu_to_be16(FDMI_PORT_HOST_NAME); - snprintf(eiter->a.host_name, sizeof(eiter->a.host_name), - "%s", fc_host_system_hostname(vha->host)); - alen = strlen(eiter->a.host_name); + alen = strlcpy(eiter->a.host_name, + fc_host_system_hostname(vha-
RE: [PATCH] scsi: scsi_devinfo.c: Cleaning up unnecessarily complicated in conjunction with strncpy
> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of Rickard Strandqvist
...
> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
...
> static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
> char *from, int compatible)
> {
> - size_t from_length;
> -
> - from_length = strlen(from);
> - strncpy(to, from, min(to_length, from_length));
> - if (from_length < to_length) {
> - if (compatible) {
> - /*
> - * NUL terminate the string if it is short.
> - */
> - to[from_length] = '\0';
> - } else {
> - /*
> - * space pad the string if it is short.
> - */
> - strncpy(&to[from_length], spaces,
> - to_length - from_length);
> - }
> - }
> - if (from_length > to_length)
> - printk(KERN_WARNING "%s: %s string '%s' is too long\n",
> + strncpy(to, from, to_length);
> + if (to[to_length - 1] != '\0') {
> + to[to_length - 1] = '\0';
> + printk(KERN_WARNING "%s: %s string '%s' is too long\n",
> __func__, name, from);
> + }
The caller of this function, scsi_dev_info_list_add_keyed, created
the "to" destination buffer, devinfo, with kmalloc, so it's not
guaranteed to be full of zeros.
If from_length is shorter than to_length, then this code will
be inspecting an uninitialized character that strncpy didn't
touch.
---
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
[PATCH] scsi: scsi_devinfo.c: Cleaning up unnecessarily complicated in conjunction with strncpy
I have revamped the code so it becomes both more effective and far more clear.
Signed-off-by: Rickard Strandqvist
---
drivers/scsi/scsi_devinfo.c | 31 +++
1 file changed, 11 insertions(+), 20 deletions(-)
diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 49014a1..f6752cc 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -282,27 +282,18 @@ static struct scsi_dev_info_list_table
*scsi_devinfo_lookup_by_key(int key)
static void scsi_strcpy_devinfo(char *name, char *to, size_t to_length,
char *from, int compatible)
{
- size_t from_length;
-
- from_length = strlen(from);
- strncpy(to, from, min(to_length, from_length));
- if (from_length < to_length) {
- if (compatible) {
- /*
-* NUL terminate the string if it is short.
-*/
- to[from_length] = '\0';
- } else {
- /*
-* space pad the string if it is short.
-*/
- strncpy(&to[from_length], spaces,
- to_length - from_length);
- }
- }
- if (from_length > to_length)
-printk(KERN_WARNING "%s: %s string '%s' is too long\n",
+ strncpy(to, from, to_length);
+ if (to[to_length - 1] != '\0') {
+ to[to_length - 1] = '\0';
+ printk(KERN_WARNING "%s: %s string '%s' is too long\n",
__func__, name, from);
+ }
+ if (!compatible) {
+ /*
+* space pad the string if it is short.
+*/
+ strlcat(to, spaces, to_length);
+ }
}
/**
--
1.7.10.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 20/20] scsi_error: format abort error message
On Sat, Sep 13, 2014 at 01:07:11AM +, Elliott, Robert (Server Storage) wrote: > > I've not seen an answer to my question in reply to the previous version > > of this. Why would you do pretty printing of a variable that can just > > return SUCCESS or FAILURE? > > Is there anything prohibiting the scsi_eh_abort_handler provided by > the LLD from returning any of the other values like NEEDS_RETRY? Right now there is nothing prohibiting them from returning any possible value, but we only handle SUCCESS specially and treat everything else as FAILED. If anyone is motivated enough to make this cleaner we should add a WARN_ON for any other value as it's a driver bug. -- 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 20/20] scsi_error: format abort error message
On 09/13/2014 03:07 AM, Elliott, Robert (Server Storage) wrote:
-Original Message-
From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
ow...@vger.kernel.org] On Behalf Of Christoph Hellwig
Sent: Friday, 05 September, 2014 7:33 PM
Subject: Re: [PATCH 20/20] scsi_error: format abort error message
On Wed, Sep 03, 2014 at 12:06:15PM +0200, Hannes Reinecke wrote:
...
Decode the return value if the command abort failed.
@@ -157,8 +157,8 @@ scmd_eh_abort_handler(struct work_struct *work)
} else {
SCSI_LOG_ERROR_RECOVERY(3,
scmd_printk(KERN_INFO, scmd,
- "scmd %p abort failed, rtn %d\n",
- scmd, rtn));
+ "scmd %p abort failed, rtn %s\n",
+ scmd, scsi_retval_string(rtn)));
...
I've not seen an answer to my question in reply to the previous version
of this. Why would you do pretty printing of a variable that can just
return SUCCESS or FAILURE?
Is there anything prohibiting the scsi_eh_abort_handler provided by
the LLD from returning any of the other values like NEEDS_RETRY?
#define NEEDS_RETRY 0x2001
#define SUCCESS 0x2002
#define FAILED 0x2003
#define QUEUED 0x2004
#define SOFT_ERROR 0x2005
#define ADD_TO_MLQUEUE 0x2006
#define TIMEOUT_ERROR 0x2007
#define SCSI_RETURN_NOT_HANDLED 0x2008
#define FAST_IO_FAIL0x2009
There is not, and hence I've added this patch.
Although the intention here would be that we should be
enforcing it, either by stating it in the description
of the function or programmatically.
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 v2 10/24] uas: zap_pending: data urbs should have completed at this time
On Sun, 2014-09-14 at 12:32 +0200, Hans de Goede wrote:
> Hi,
>
> On 09/14/2014 12:29 PM, James Bottomley wrote:
> > On Sun, 2014-09-14 at 11:26 +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 09/13/2014 09:31 PM, Sergei Shtylyov wrote:
> >>> Hello.
> >>>
> >>> On 9/13/2014 1:26 PM, Hans de Goede wrote:
> >>>
> The data urbs are all killed before calling zap_pending, and their
> completion
> handler should have cleared their inflight flag.
> >>>
> Do not 0 the data inflight flags, and add a check for try_complete
> succeeding,
> as it should always succeed when called from zap_pending.
> >>>
> Signed-off-by: Hans de Goede
> ---
> drivers/usb/storage/uas.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 08edb6b..85bbc1d 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info
> *devinfo, int result)
> struct uas_cmd_info *cmdinfo;
> struct uas_cmd_info *temp;
> unsigned long flags;
> +int err;
> >>>
> >>>Er, I don't see why this variable is necessary.
> >>>
> >>> [...]
> @@ -152,12 +153,11 @@ static void uas_zap_pending(struct uas_dev_info
> *devinfo, int result)
> struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
> SCp);
> uas_log_cmd_state(cmnd, __func__);
> -/* all urbs are killed, clear inflight bits */
> -cmdinfo->state &= ~(COMMAND_INFLIGHT |
> -DATA_IN_URB_INFLIGHT |
> -DATA_OUT_URB_INFLIGHT);
> +/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
> +cmdinfo->state &= ~COMMAND_INFLIGHT;
> cmnd->result = result << 16;
> -uas_try_complete(cmnd, __func__);
> +err = uas_try_complete(cmnd, __func__);
> +WARN_ON(err != 0);
> >>>
> >>>Why not:
> >>>
> >>> WARN_ON(uas_try_complete(cmnd, __func__));
> >>>
> >>
> >> This was discussed already during v1 of this patch-set, WARN_ON may
> >> not have a side-effect, as it may be defined as an empty macro.
> >
> > Must have missed the discussion, but whoever said that loses all their
> > review points. We're very careful to make sure that even in the case
> > where WARN_ON and BUG_ON (and indeed any macros) are compiled out, the
> > side effects are still accounted for. This is the canonical definition
> > of WARN_ON in the compiled out case:
> >
> > #define WARN_ON(condition) ({
> > \
> > int __ret_warn_on = !!(condition); \
> > unlikely(__ret_warn_on);\
> > })
> >
> > So the compiler will eliminate the statement only if there are no side
> > effects.
>
> Ah that is good to know. Still I would like to stick with the new version
> (which adds the err), as I believe that that code is more readable.
>
> AFAIK in general the kernel coding style is to favor:
>
> err = func();
> if (err)
>
> over:
>
> if (func())
>
> And this is sorta the same.
I'm agnostic on that. I was just combatting the impression that you had
to be careful about side effects in known macro statements.
James
--
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 v2 10/24] uas: zap_pending: data urbs should have completed at this time
Hi,
On 09/14/2014 12:29 PM, James Bottomley wrote:
> On Sun, 2014-09-14 at 11:26 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 09/13/2014 09:31 PM, Sergei Shtylyov wrote:
>>> Hello.
>>>
>>> On 9/13/2014 1:26 PM, Hans de Goede wrote:
>>>
The data urbs are all killed before calling zap_pending, and their
completion
handler should have cleared their inflight flag.
>>>
Do not 0 the data inflight flags, and add a check for try_complete
succeeding,
as it should always succeed when called from zap_pending.
>>>
Signed-off-by: Hans de Goede
---
drivers/usb/storage/uas.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 08edb6b..85bbc1d 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info
*devinfo, int result)
struct uas_cmd_info *cmdinfo;
struct uas_cmd_info *temp;
unsigned long flags;
+int err;
>>>
>>>Er, I don't see why this variable is necessary.
>>>
>>> [...]
@@ -152,12 +153,11 @@ static void uas_zap_pending(struct uas_dev_info
*devinfo, int result)
struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
SCp);
uas_log_cmd_state(cmnd, __func__);
-/* all urbs are killed, clear inflight bits */
-cmdinfo->state &= ~(COMMAND_INFLIGHT |
-DATA_IN_URB_INFLIGHT |
-DATA_OUT_URB_INFLIGHT);
+/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
+cmdinfo->state &= ~COMMAND_INFLIGHT;
cmnd->result = result << 16;
-uas_try_complete(cmnd, __func__);
+err = uas_try_complete(cmnd, __func__);
+WARN_ON(err != 0);
>>>
>>>Why not:
>>>
>>> WARN_ON(uas_try_complete(cmnd, __func__));
>>>
>>
>> This was discussed already during v1 of this patch-set, WARN_ON may
>> not have a side-effect, as it may be defined as an empty macro.
>
> Must have missed the discussion, but whoever said that loses all their
> review points. We're very careful to make sure that even in the case
> where WARN_ON and BUG_ON (and indeed any macros) are compiled out, the
> side effects are still accounted for. This is the canonical definition
> of WARN_ON in the compiled out case:
>
> #define WARN_ON(condition) ({ \
> int __ret_warn_on = !!(condition); \
> unlikely(__ret_warn_on);\
> })
>
> So the compiler will eliminate the statement only if there are no side
> effects.
Ah that is good to know. Still I would like to stick with the new version
(which adds the err), as I believe that that code is more readable.
AFAIK in general the kernel coding style is to favor:
err = func();
if (err)
over:
if (func())
And this is sorta the same.
Regards,
Hans
--
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 v2 10/24] uas: zap_pending: data urbs should have completed at this time
On Sun, 2014-09-14 at 11:26 +0200, Hans de Goede wrote:
> Hi,
>
> On 09/13/2014 09:31 PM, Sergei Shtylyov wrote:
> > Hello.
> >
> > On 9/13/2014 1:26 PM, Hans de Goede wrote:
> >
> >> The data urbs are all killed before calling zap_pending, and their
> >> completion
> >> handler should have cleared their inflight flag.
> >
> >> Do not 0 the data inflight flags, and add a check for try_complete
> >> succeeding,
> >> as it should always succeed when called from zap_pending.
> >
> >> Signed-off-by: Hans de Goede
> >> ---
> >> drivers/usb/storage/uas.c | 10 +-
> >> 1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> >> index 08edb6b..85bbc1d 100644
> >> --- a/drivers/usb/storage/uas.c
> >> +++ b/drivers/usb/storage/uas.c
> >> @@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info
> >> *devinfo, int result)
> >> struct uas_cmd_info *cmdinfo;
> >> struct uas_cmd_info *temp;
> >> unsigned long flags;
> >> +int err;
> >
> >Er, I don't see why this variable is necessary.
> >
> > [...]
> >> @@ -152,12 +153,11 @@ static void uas_zap_pending(struct uas_dev_info
> >> *devinfo, int result)
> >> struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
> >> SCp);
> >> uas_log_cmd_state(cmnd, __func__);
> >> -/* all urbs are killed, clear inflight bits */
> >> -cmdinfo->state &= ~(COMMAND_INFLIGHT |
> >> -DATA_IN_URB_INFLIGHT |
> >> -DATA_OUT_URB_INFLIGHT);
> >> +/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
> >> +cmdinfo->state &= ~COMMAND_INFLIGHT;
> >> cmnd->result = result << 16;
> >> -uas_try_complete(cmnd, __func__);
> >> +err = uas_try_complete(cmnd, __func__);
> >> +WARN_ON(err != 0);
> >
> >Why not:
> >
> > WARN_ON(uas_try_complete(cmnd, __func__));
> >
>
> This was discussed already during v1 of this patch-set, WARN_ON may
> not have a side-effect, as it may be defined as an empty macro.
Must have missed the discussion, but whoever said that loses all their
review points. We're very careful to make sure that even in the case
where WARN_ON and BUG_ON (and indeed any macros) are compiled out, the
side effects are still accounted for. This is the canonical definition
of WARN_ON in the compiled out case:
#define WARN_ON(condition) ({ \
int __ret_warn_on = !!(condition); \
unlikely(__ret_warn_on);\
})
So the compiler will eliminate the statement only if there are no side
effects.
James
--
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 fix for 3.17 v3] uas: Add a quirk for rejecting ATA_12 and ATA_16 commands
And set this quirk for the Seagate Expansion Desk (0bc2:2312), as that one
seems to hang upon receiving an ATA_12 or ATA_16 command.
https://bugzilla.kernel.org/show_bug.cgi?id=79511
While at it also add missing documentation for the u value for usb-storage
quirks.
Cc: sta...@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede
--
Changes in v2: Add documentation for new t and u usb-storage.quirks flags
Changes in v3: Fix typo in documentation
---
Documentation/kernel-parameters.txt | 3 +++
drivers/usb/storage/uas.c | 13 +
drivers/usb/storage/unusual_uas.h | 16 ++--
drivers/usb/storage/usb.c | 6 +-
include/linux/usb_usual.h | 2 ++
5 files changed, 29 insertions(+), 11 deletions(-)
diff --git a/Documentation/kernel-parameters.txt
b/Documentation/kernel-parameters.txt
index 5ae8608..ec6b25a 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3541,6 +3541,9 @@ bytes respectively. Such letter suffixes can also be
entirely omitted.
bogus residue values);
s = SINGLE_LUN (the device has only one
Logical Unit);
+ t = NO_ATA_1X (don't allow ATA(12) and ATA(16)
+ commands, uas only);
+ u = IGNORE_UAS (do not try to use uas);
w = NO_WP_DETECT (don't test whether the
medium is write-protected).
Example: quirks=0419:aaf5:rl,0421:0433:rc
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 3f42785..75d2ccd 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -28,6 +28,7 @@
#include
#include "uas-detect.h"
+#include "scsiglue.h"
/*
* The r00-r01c specs define this version of the SENSE IU data structure.
@@ -49,6 +50,7 @@ struct uas_dev_info {
struct usb_anchor cmd_urbs;
struct usb_anchor sense_urbs;
struct usb_anchor data_urbs;
+ unsigned long flags;
int qdepth, resetting;
struct response_iu response;
unsigned cmd_pipe, status_pipe, data_in_pipe, data_out_pipe;
@@ -714,6 +716,15 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
BUILD_BUG_ON(sizeof(struct uas_cmd_info) > sizeof(struct scsi_pointer));
+ if ((devinfo->flags & US_FL_NO_ATA_1X) &&
+ (cmnd->cmnd[0] == ATA_12 || cmnd->cmnd[0] == ATA_16)) {
+ memcpy(cmnd->sense_buffer, usb_stor_sense_invalidCDB,
+ sizeof(usb_stor_sense_invalidCDB));
+ cmnd->result = SAM_STAT_CHECK_CONDITION;
+ cmnd->scsi_done(cmnd);
+ return 0;
+ }
+
spin_lock_irqsave(&devinfo->lock, flags);
if (devinfo->resetting) {
@@ -1080,6 +1091,8 @@ static int uas_probe(struct usb_interface *intf, const
struct usb_device_id *id)
devinfo->resetting = 0;
devinfo->running_task = 0;
devinfo->shutdown = 0;
+ devinfo->flags = id->driver_info;
+ usb_stor_adjust_quirks(udev, &devinfo->flags);
init_usb_anchor(&devinfo->cmd_urbs);
init_usb_anchor(&devinfo->sense_urbs);
init_usb_anchor(&devinfo->data_urbs);
diff --git a/drivers/usb/storage/unusual_uas.h
b/drivers/usb/storage/unusual_uas.h
index 724..52f7012 100644
--- a/drivers/usb/storage/unusual_uas.h
+++ b/drivers/usb/storage/unusual_uas.h
@@ -40,13 +40,9 @@
* and don't forget to CC: the USB development list
*/
-/*
- * This is an example entry for the US_FL_IGNORE_UAS flag. Once we have an
- * actual entry using US_FL_IGNORE_UAS this entry should be removed.
- *
- * UNUSUAL_DEV( 0xabcd, 0x1234, 0x0100, 0x0100,
- * "Example",
- * "Storage with broken UAS",
- * USB_SC_DEVICE, USB_PR_DEVICE, NULL,
- * US_FL_IGNORE_UAS),
- */
+/* https://bugzilla.kernel.org/show_bug.cgi?id=79511 */
+UNUSUAL_DEV(0x0bc2, 0x2312, 0x, 0x,
+ "Seagate",
+ "Expansion Desk",
+ USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+ US_FL_NO_ATA_1X),
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index cedb292..b9d1b93 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -478,7 +478,8 @@ void usb_stor_adjust_quirks(struct usb_device *udev,
unsigned long *fflags)
US_FL_CAPACITY_OK | US_FL_IGNORE_RESIDUE |
US_FL_SINGLE_LUN | US_FL_NO_WP_DETECT |
US_FL_NO_READ_DISC_INFO | US_FL_NO_READ_CAPACITY_16 |
- US_FL_INITIAL_READ10 | US_FL_WRITE_CACHE);
+ US_FL_INITIAL_READ10 | US_FL_WRITE_CACHE |
+ US_FL_NO_ATA_1X);
p = quirks;
while (*p) {
@@ -543,6 +544,9 @@ void usb_stor_adjus
Re: [PATCH fix for 3.17 v2] uas: Add a quirk for rejecting ATA_12 and ATA_16 commands
Hi, On 09/13/2014 10:27 PM, Sergei Shtylyov wrote: > Hello. > > On 9/13/2014 10:21 PM, Thomas Backlund wrote: > >>> And set this quirk for the Seagate Expansion Desk (0bc2:2312), as that one >>> seems to hang upon receiving an ATA_12 or ATA_16 command. > >>> https://bugzilla.kernel.org/show_bug.cgi?id=79511 > >>> While at it also add missing documentation for the u value for usb-storage >>> quirks. > >>> Cc: sta...@vger.kernel.org # 3.16 >>> Signed-off-by: Hans de Goede > >>> -- >>> Changes in v2: Add documentation for new t and u usb-storage.quirks flags >>> --- >>> Documentation/kernel-parameters.txt | 3 +++ >>> drivers/usb/storage/uas.c | 13 + >>> drivers/usb/storage/unusual_uas.h | 16 ++-- >>> drivers/usb/storage/usb.c | 6 +- >>> include/linux/usb_usual.h | 2 ++ >>> 5 files changed, 29 insertions(+), 11 deletions(-) > >>> diff --git a/Documentation/kernel-parameters.txt >>> b/Documentation/kernel-parameters.txt >>> index 5ae8608..7c32053 100644 >>> --- a/Documentation/kernel-parameters.txt >>> +++ b/Documentation/kernel-parameters.txt >>> @@ -3541,6 +3541,9 @@ bytes respectively. Such letter suffixes can also be >>> entirely omitted. >>> bogus residue values); >>> s = SINGLE_LUN (the device has only one >>> Logical Unit); >>> +t = NO_ATA_1X (don't allow ATA12 and ATA12 >>> +commands, uas only); > >> I guess you meant ATA12 and *ATA16* > >... or even ATA(12) and ATA(16). As far as I remember SCSI, it has CDB > length in parens for the command names. Right, ugh. v3 coming up ... Thanks, Hans -- 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: fix regression that accidentally disabled block-based tcq
Hi, On 09/13/2014 07:50 PM, Christoph Hellwig wrote: > On Sat, Sep 13, 2014 at 12:28:41PM +0200, Hans de Goede wrote: >> Yes this one does the trick and fixes things. Note the git tree I used for >> testing also had your previous fix to split up the blk_tcq union in 2 >> separate struct members. Let me know if you want me to re-test without that >> fix. > > I'm pretty sure that isn't needed, so double testing it indeed doesn't > would be helpful. I can confirm that things still work fine with the union left in place. > I'd like to add your Tested-by: tag after that, too. Ack: Tested-by: Hans de Goede Regards, Hans -- 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] ahci_xgene: Fix the error print invalid resource for APM X-Gene SoC AHCI SATA Host Controller driver.
Hello, On Sun, Sep 14, 2014 at 11:36:51AM +0530, Suman Tripathi wrote: > We can maintain same piece (IS_ERR(ctx->csr_mux)), then we can do the > below instead of NULL ?? > > ctx->csr_mux = res ? devm_ioremap_resource(dev, res) : ERR_PTR(-EINVAL); Setting it to NULL on failure would probably make more sense. No need to carry around ERR_PTR() value around. 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: [PATCH v2 10/24] uas: zap_pending: data urbs should have completed at this time
Hi, On 09/13/2014 09:31 PM, Sergei Shtylyov wrote: > Hello. > > On 9/13/2014 1:26 PM, Hans de Goede wrote: > >> The data urbs are all killed before calling zap_pending, and their completion >> handler should have cleared their inflight flag. > >> Do not 0 the data inflight flags, and add a check for try_complete >> succeeding, >> as it should always succeed when called from zap_pending. > >> Signed-off-by: Hans de Goede >> --- >> drivers/usb/storage/uas.c | 10 +- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c >> index 08edb6b..85bbc1d 100644 >> --- a/drivers/usb/storage/uas.c >> +++ b/drivers/usb/storage/uas.c >> @@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info >> *devinfo, int result) >> struct uas_cmd_info *cmdinfo; >> struct uas_cmd_info *temp; >> unsigned long flags; >> +int err; > >Er, I don't see why this variable is necessary. > > [...] >> @@ -152,12 +153,11 @@ static void uas_zap_pending(struct uas_dev_info >> *devinfo, int result) >> struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, >> SCp); >> uas_log_cmd_state(cmnd, __func__); >> -/* all urbs are killed, clear inflight bits */ >> -cmdinfo->state &= ~(COMMAND_INFLIGHT | >> -DATA_IN_URB_INFLIGHT | >> -DATA_OUT_URB_INFLIGHT); >> +/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */ >> +cmdinfo->state &= ~COMMAND_INFLIGHT; >> cmnd->result = result << 16; >> -uas_try_complete(cmnd, __func__); >> +err = uas_try_complete(cmnd, __func__); >> +WARN_ON(err != 0); > >Why not: > > WARN_ON(uas_try_complete(cmnd, __func__)); > This was discussed already during v1 of this patch-set, WARN_ON may not have a side-effect, as it may be defined as an empty macro. Regards, Hans -- 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] scsi_scan: Send TEST UNIT READY to the LUN before scanning
On 09/07/2014 06:24 PM, Christoph Hellwig wrote: Looks like you put a version into the SuSE tree with a blacklist entry just for the Fujitsu array that requires the TEST UNIT READY. Can you send that version upstream as well? I've just had reports that this is required, as it interferes with NetApp arrays. So I'll be reposting a combined series with that. 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
