RE: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout
> -Original Message-
> From: Mike Christie [mailto:micha...@cs.wisc.edu]
> Sent: Thursday, June 5, 2014 6:33 PM
> To: KY Srinivasan
> Cc: James Bottomley; linux-ker...@vger.kernel.org; a...@canonical.com;
> de...@linuxdriverproject.org; h...@infradead.org; linux-
> s...@vger.kernel.org; oher...@suse.com; gre...@linuxfoundation.org;
> jasow...@redhat.com
> Subject: Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT
> from the basic I/O timeout
>
> On 06/04/2014 12:15 PM, KY Srinivasan wrote:
> >
> >
> >> -Original Message-
> >> From: James Bottomley [mailto:jbottom...@parallels.com]
> >> Sent: Wednesday, June 4, 2014 10:02 AM
> >> To: KY Srinivasan
> >> Cc: linux-ker...@vger.kernel.org; a...@canonical.com;
> >> de...@linuxdriverproject.org; h...@infradead.org; linux-
> >> s...@vger.kernel.org; oher...@suse.com; gre...@linuxfoundation.org;
> >> jasow...@redhat.com
> >> Subject: Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the
> >> FLUSH_TIMEOUT from the basic I/O timeout
> >>
> >> On Wed, 2014-06-04 at 09:33 -0700, K. Y. Srinivasan wrote:
> >>> Commit ID: 7e660100d85af860e7ad763202fff717adcdaacd added code to
> >>> derive the FLUSH_TIMEOUT from the basic I/O timeout. However, this
> >>> patch did not use the basic I/O timeout of the device. Fix this bug.
> >>>
> >>> Signed-off-by: K. Y. Srinivasan
> >>> ---
> >>> drivers/scsi/sd.c |4 +++-
> >>> 1 files changed, 3 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
> >>> e9689d5..54150b1 100644
> >>> --- a/drivers/scsi/sd.c
> >>> +++ b/drivers/scsi/sd.c
> >>> @@ -832,7 +832,9 @@ static int sd_setup_write_same_cmnd(struct
> >>> scsi_device *sdp, struct request *rq)
> >>>
> >>> static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct
> >>> request *rq) {
> >>> - rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
> >>> + int timeout = sdp->request_queue->rq_timeout;
> >>> +
> >>> + rq->timeout = (timeout * SD_FLUSH_TIMEOUT_MULTIPLIER);
> >>
> >> Could you share where you found this to be a problem? It looks like
> >> a bug in block because all inbound requests being prepared should
> >> have a timeout set, so block would be the place to fix it.
> >
> > Perhaps; what I found was that the value in rq->timeout was 0 coming
> > into this function and thus multiplying obviously has no effect.
> >
>
> I think you are right. We hit this problem because we are doing:
>
> scsi_request_fn -> blk_peek_request -> sd_prep_fn ->
> scsi_setup_flush_cmnd.
>
> At this time request->timeout is zero so the multiplication does nothing. See
> how sd_setup_write_same_cmnd will set the request->timeout at this time.
>
> Then in scsi_request_fn we do:
>
> scsi_request_fn -> blk_start_request -> blk_add_timer.
>
> At this time it will set the request->timeout if something like req block pc
> users (like scsi_execute() or block/scsi_ioctl.c) or the write same code
> mentioned above have not set the timeout.
I don't think this is a recent change. Prior to this commit, we were setting
the timeout
value in this function; it just happened to be a different constant unrelated
to the I/O
timeout.
K. Y
>
>
--
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/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout
On 06/04/2014 12:15 PM, KY Srinivasan wrote:
>
>
>> -Original Message-
>> From: James Bottomley [mailto:jbottom...@parallels.com]
>> Sent: Wednesday, June 4, 2014 10:02 AM
>> To: KY Srinivasan
>> Cc: linux-ker...@vger.kernel.org; a...@canonical.com;
>> de...@linuxdriverproject.org; h...@infradead.org; linux-
>> s...@vger.kernel.org; oher...@suse.com; gre...@linuxfoundation.org;
>> jasow...@redhat.com
>> Subject: Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT
>> from the basic I/O timeout
>>
>> On Wed, 2014-06-04 at 09:33 -0700, K. Y. Srinivasan wrote:
>>> Commit ID: 7e660100d85af860e7ad763202fff717adcdaacd added code to
>>> derive the FLUSH_TIMEOUT from the basic I/O timeout. However, this
>>> patch did not use the basic I/O timeout of the device. Fix this bug.
>>>
>>> Signed-off-by: K. Y. Srinivasan
>>> ---
>>> drivers/scsi/sd.c |4 +++-
>>> 1 files changed, 3 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
>>> e9689d5..54150b1 100644
>>> --- a/drivers/scsi/sd.c
>>> +++ b/drivers/scsi/sd.c
>>> @@ -832,7 +832,9 @@ static int sd_setup_write_same_cmnd(struct
>>> scsi_device *sdp, struct request *rq)
>>>
>>> static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct
>>> request *rq) {
>>> - rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
>>> + int timeout = sdp->request_queue->rq_timeout;
>>> +
>>> + rq->timeout = (timeout * SD_FLUSH_TIMEOUT_MULTIPLIER);
>>
>> Could you share where you found this to be a problem? It looks like a bug in
>> block because all inbound requests being prepared should have a timeout
>> set, so block would be the place to fix it.
>
> Perhaps; what I found was that the value in rq->timeout was 0 coming into
> this function and thus multiplying obviously has no effect.
>
I think you are right. We hit this problem because we are doing:
scsi_request_fn -> blk_peek_request -> sd_prep_fn -> scsi_setup_flush_cmnd.
At this time request->timeout is zero so the multiplication does
nothing. See how sd_setup_write_same_cmnd will set the request->timeout
at this time.
Then in scsi_request_fn we do:
scsi_request_fn -> blk_start_request -> blk_add_timer.
At this time it will set the request->timeout if something like req
block pc users (like scsi_execute() or block/scsi_ioctl.c) or the write
same code mentioned above have not set the timeout.
--
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] mpt3sas: delay scsi_add_host call to work with scsi-mq
In _scsih_probe, delay the call to scsi_add_host until
the host template has been completely filled in.
Otherwise, the default .can_queue value of 1 causes
scsi-mq to set the block layer request queue size
to its minimum size, resulting in awful performance.
In _scsih_probe error handling, call mpt3sas_base_detach
rather than scsi_remove_host to properly clean up
in reverse order.
In _scsih_remove, call scsi_remove_host earlier
to clean up in reverse order.
Signed-off-by: Robert Elliott
---
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 20 ++--
1 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 18e713d..529b5fd 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -7431,9 +7431,9 @@ static void _scsih_remove(struct pci_dev *pdev)
}
sas_remove_host(shost);
+ scsi_remove_host(shost);
mpt3sas_base_detach(ioc);
list_del(&ioc->list);
- scsi_remove_host(shost);
scsi_host_put(shost);
}
@@ -7801,13 +7801,6 @@ _scsih_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
}
}
- if ((scsi_add_host(shost, &pdev->dev))) {
- pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n",
- ioc->name, __FILE__, __LINE__, __func__);
- list_del(&ioc->list);
- goto out_add_shost_fail;
- }
-
/* register EEDP capabilities with SCSI layer */
if (prot_mask > 0)
scsi_host_set_prot(shost, prot_mask);
@@ -7835,15 +7828,22 @@ _scsih_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
ioc->name, __FILE__, __LINE__, __func__);
goto out_attach_fail;
}
+
+ if ((scsi_add_host(shost, &pdev->dev))) {
+ pr_err(MPT3SAS_FMT "failure at %s:%d/%s()!\n",
+ ioc->name, __FILE__, __LINE__, __func__);
+ goto out_add_shost_fail;
+ }
+
scsi_scan_host(shost);
return 0;
+ out_add_shost_fail:
+ mpt3sas_base_detach(ioc);
out_attach_fail:
destroy_workqueue(ioc->firmware_event_thread);
out_thread_fail:
list_del(&ioc->list);
- scsi_remove_host(shost);
- out_add_shost_fail:
scsi_host_put(shost);
return -ENODEV;
}
---
Rob ElliottHP Server Storage
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
[PATCH 1/2] tcm_fc: Generate TASK_SET_FULL status for DataIN failures
From: Nicholas Bellinger
This patch changes ft_queue_data_in() to set SAM_STAT_TASK_SET_FULL
status upon a lport->tt.seq_send() failure, where it will now stop
sending subsequent DataIN, and immediately attempt to send the
response with exception status.
Sending a response with SAM_STAT_TASK_SET_FULL status is useful in
order to signal the initiator that it should try to reduce it's
current queue_depth, to lower the number of outstanding I/Os on
the wire.
Also, add a check to skip sending DataIN if TASK_SET_FULL status
has already been set due to a response lport->tt.seq_send()
failure, that has asked target-core to requeue a response.
Reported-by: Vasu Dev
Cc: Vasu Dev
Cc: Jun Wu
Signed-off-by: Nicholas Bellinger
---
drivers/target/tcm_fc/tfc_io.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/target/tcm_fc/tfc_io.c b/drivers/target/tcm_fc/tfc_io.c
index e415af3..140659f 100644
--- a/drivers/target/tcm_fc/tfc_io.c
+++ b/drivers/target/tcm_fc/tfc_io.c
@@ -82,6 +82,10 @@ int ft_queue_data_in(struct se_cmd *se_cmd)
if (cmd->aborted)
return 0;
+
+ if (se_cmd->scsi_status == SAM_STAT_TASK_SET_FULL)
+ goto queue_status;
+
ep = fc_seq_exch(cmd->seq);
lport = ep->lp;
cmd->seq = lport->tt.seq_start_next(cmd->seq);
@@ -178,14 +182,22 @@ int ft_queue_data_in(struct se_cmd *se_cmd)
FC_TYPE_FCP, f_ctl, fh_off);
error = lport->tt.seq_send(lport, seq, fp);
if (error) {
- /* XXX For now, initiator will retry */
pr_err_ratelimited("%s: Failed to send frame %p, "
"xid <0x%x>, remaining %zu, "
"lso_max <0x%x>\n",
__func__, fp, ep->xid,
remaining, lport->lso_max);
+ /*
+* Generate a TASK_SET_FULL status to notify the
+* initiator to reduce it's queue_depth, ignoring
+* the rest of the data-in and immediately attempt
+* to send the response.
+*/
+ se_cmd->scsi_status = SAM_STAT_TASK_SET_FULL;
+ break;
}
}
+queue_status:
return ft_queue_status(se_cmd);
}
--
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 0/2] tcm_fc: Generate TASK_SET_FULL for DataIN + response failures
From: Nicholas Bellinger Hi Vasu, This series generates SAM_STAT_TASK_SET_FULL status for lport->tt.seq_send() failures in DataIN + response status codepaths, which is done in order to get the initiator to reduce it's current queue_depth thus reducing the number of outstanding I/Os permitted in flight on the wire. For the DataIN case, once a lport->tt.seq_send() failure occurs it will stop attempting to send subsequent DataIN payload data, and immediately attempt to send a response packet with SAM_STAT_TASK_SET_FULL status. For the response case, once a lport->tt.seq_send() failure occurs it will set SAM_STAT_TASK_SET_FULL status and return -ENOMEM to the target, forcing the response to be requeued by target-core Note this series has been compile tested only, so please review + test. Thank you, --nab Nicholas Bellinger (2): tcm_fc: Generate TASK_SET_FULL status for DataIN failures tcm_fc: Generate TASK_SET_FULL status for response failures drivers/target/tcm_fc/tfc_cmd.c | 19 --- drivers/target/tcm_fc/tfc_io.c | 14 +- 2 files changed, 29 insertions(+), 4 deletions(-) -- 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 2/2] tcm_fc: Generate TASK_SET_FULL status for response failures
From: Nicholas Bellinger
This patch changes ft_queue_status() to set SAM_STAT_TASK_SET_FULL
status upon lport->tt.seq_send( failure, and return -EAGAIN to notify
target-core to attempt to requeue the response.
It also does the same for a fc_frame_alloc() failures, in order to
signal the initiator that it should try to reduce it's current
queue_depth, to lower the number of outstanding I/Os on the wire.
Reported-by: Vasu Dev
Cc: Vasu Dev
Cc: Jun Wu
Signed-off-by: Nicholas Bellinger
---
drivers/target/tcm_fc/tfc_cmd.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index f5fd515..5585038 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -128,6 +128,7 @@ int ft_queue_status(struct se_cmd *se_cmd)
struct fc_lport *lport;
struct fc_exch *ep;
size_t len;
+ int rc;
if (cmd->aborted)
return 0;
@@ -137,9 +138,10 @@ int ft_queue_status(struct se_cmd *se_cmd)
len = sizeof(*fcp) + se_cmd->scsi_sense_length;
fp = fc_frame_alloc(lport, len);
if (!fp) {
- /* XXX shouldn't just drop it - requeue and retry? */
- return 0;
+ se_cmd->scsi_status = SAM_STAT_TASK_SET_FULL;
+ return -ENOMEM;
}
+
fcp = fc_frame_payload_get(fp, len);
memset(fcp, 0, len);
fcp->resp.fr_status = se_cmd->scsi_status;
@@ -170,7 +172,18 @@ int ft_queue_status(struct se_cmd *se_cmd)
fc_fill_fc_hdr(fp, FC_RCTL_DD_CMD_STATUS, ep->did, ep->sid, FC_TYPE_FCP,
FC_FC_EX_CTX | FC_FC_LAST_SEQ | FC_FC_END_SEQ, 0);
- lport->tt.seq_send(lport, cmd->seq, fp);
+ rc = lport->tt.seq_send(lport, cmd->seq, fp);
+ if (rc) {
+ pr_err_ratelimited("%s: Failed to send response frame %p, "
+ "xid <0x%x>\n", __func__, fp, ep->xid);
+ /*
+* Generate a TASK_SET_FULL status to notify the initiator
+* to reduce it's queue_depth after the se_cmd response has
+* been re-queued by target-core.
+*/
+ se_cmd->scsi_status = SAM_STAT_TASK_SET_FULL;
+ return -ENOMEM;
+ }
lport->tt.exch_done(cmd->seq);
return 0;
}
--
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
SCSI eats error from flush failure during hot plug
Hello,
I am testing ATA device durability during hot unplug. I have a power
fault test suite that has turned up issues with the fsync->SCSI->ATA
codepath. If a device is unplugged while an fsync is in progress, ATA
returns a flush error to the SCSI driver but scsi_io_completion()
seems to disregard it. fsync() returns no error, which should mean
that the write was durably flushed to disk. I expect fsync() to return
EIO or something similar when the flush isn't acked by the device.
When the failure occurs, the SCSI sense key is set to ABORTED_COMMAND.
However, scsi_end_command() is called without any of the sense context
and scsi_io_completion() returns early without checking sense at all.
This commit may be related:
d6b0c53723753fc0cfda63f56735b225c43e1e9a
(http://git.opencores.org/?a=commitdiff&p=linux&h=d6b0c53723753fc0cfda63f56735b225c43e1e9a)
The following patch fixes the issue, but it's a hack. I only have a
vague understanding of Linux drivers, so I'm looking for advice on how
to make this fix better and get it upstream.
Thanks!
Steven Haber
Qumulo, Inc.
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9db097a..789af39 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -822,40 +822,44 @@ void scsi_io_completion(struct scsi_cmnd *cmd,
unsigned int good_bytes)
/*
* Recovered errors need reporting, but they're always treated
* as success, so fiddle the result code here. For BLOCK_PC
* we already took a copy of the original into rq->errors which
* is what gets returned to the user
*/
if (sense_valid && (sshdr.sense_key == RECOVERED_ERROR)) {
/* if ATA PASS-THROUGH INFORMATION AVAILABLE skip
* print since caller wants ATA registers. Only occurs on
* SCSI ATA PASS_THROUGH commands when CK_COND=1
*/
if ((sshdr.asc == 0x0) && (sshdr.ascq == 0x1d))
;
else if (!(req->cmd_flags & REQ_QUIET))
scsi_print_sense("", cmd);
result = 0;
/* BLOCK_PC may have set error */
error = 0;
}
+ if (sense_valid && (sshdr.sense_key == ABORTED_COMMAND) &&
+good_bytes == 0)
+ error = (sshdr.asc == 0x10) ? -EILSEQ : -EIO;
+
/*
* A number of bytes were successfully read. If there
* are leftovers and there is some kind of error
* (result != 0), retry the rest.
*/
if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
return;
error = __scsi_error_from_host_byte(cmd, result);
if (host_byte(result) == DID_RESET) {
/* Third party bus reset or reset for error recovery
* reasons. Just retry the command and see what
* happens.
*/
action = ACTION_RETRY;
} else if (sense_valid && !sense_deferred) {
switch (sshdr.sense_key) {
case UNIT_ATTENTION:
if (cmd->device->removable) {
--
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 v5] sg: relax 16 byte cdb restriction
On 14-06-05 11:40 AM, Boaz Harrosh wrote:
On 06/03/2014 08:18 PM, Douglas Gilbert wrote:
v4 of this patch was sent 20131201.
ChangeLog:
- remove the 16 byte CDB (SCSI command) length limit
from the sg driver by handling longer CDBs the same
way as the bsg driver. Remove comment from sg.h
public interface about the cmd_len field being
limited to 16 bytes.
- remove some dead code caused by this change
- cleanup comment block at the top of sg.h, fix urls
Signed-off-by: Douglas Gilbert
sg_cdb_dg5.patch
<>
+/* SG_MAX_CDB_SIZE should be 260 (spc4r37 section 3.1.30) however the type
+ * of sg_io_hdr::cmd_len can only represent 255
+ */
+#define SG_MAX_CDB_SIZE 255
+
Just a nit on above comment (code is all good). CDB bigger then 16 is 8 bytes
aligned
So the maximum for sg is 252 not 255 as above.
(As you said theoretical max is 260 but since it would not fit then the
next allowed size is 252)
Yes, "a multiple of four" so 252 would be better.
Doug Gilbert
/*
* Suppose you want to calculate the formula muldiv(x,m,d)=int(x * m / d)
* Then when using 32 bit integers x * m may overflow during the calculation.
@@ -161,7 +164,7 @@ typedef struct sg_fd { /* holds the state of a
file descriptor */
char low_dma; /* as in parent but possibly overridden to 1 */
char force_packid; /* 1 -> pack_id input to read(), 0 -> ignored */
char cmd_q; /* 1 -> allow command queuing, 0 -> don't */
- char next_cmd_len; /* 0 -> automatic (def), >0 -> use on next
write() */
+ unsigned char next_cmd_len; /* 0: automatic, >0: use on next write() */
char keep_orphan; /* 0 -> drop orphan (def), 1 -> keep for read()
*/
char mmap_called; /* 0 -> mmap() never called on this fd */
struct kref f_ref;
@@ -566,7 +569,7 @@ sg_write(struct file *filp, const char __user *buf, size_t
count, loff_t * ppos)
Sg_request *srp;
struct sg_header old_hdr;
sg_io_hdr_t *hp;
- unsigned char cmnd[MAX_COMMAND_SIZE];
+ unsigned char cmnd[SG_MAX_CDB_SIZE];
if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
return -ENXIO;
@@ -598,12 +601,6 @@ sg_write(struct file *filp, const char __user *buf, size_t
count, loff_t * ppos)
buf += SZ_SG_HEADER;
__get_user(opcode, buf);
if (sfp->next_cmd_len > 0) {
- if (sfp->next_cmd_len > MAX_COMMAND_SIZE) {
- SCSI_LOG_TIMEOUT(1, printk("sg_write: command length too
long\n"));
- sfp->next_cmd_len = 0;
- sg_remove_request(sfp, srp);
- return -EIO;
- }
cmd_size = sfp->next_cmd_len;
sfp->next_cmd_len = 0; /* reset so only this write()
effected */
} else {
@@ -675,7 +672,7 @@ sg_new_write(Sg_fd *sfp, struct file *file, const char
__user *buf,
int k;
Sg_request *srp;
sg_io_hdr_t *hp;
- unsigned char cmnd[MAX_COMMAND_SIZE];
+ unsigned char cmnd[SG_MAX_CDB_SIZE];
int timeout;
unsigned long ul_timeout;
@@ -1645,14 +1642,24 @@ static int sg_start_req(Sg_request *srp, unsigned char
*cmd)
struct request_queue *q = sfp->parentdp->device->request_queue;
struct rq_map_data *md, map_data;
int rw = hp->dxfer_direction == SG_DXFER_TO_DEV ? WRITE : READ;
+ unsigned char *long_cmdp = NULL;
SCSI_LOG_TIMEOUT(4, printk(KERN_INFO "sg_start_req: dxfer_len=%d\n",
dxfer_len));
+ if (hp->cmd_len > BLK_MAX_CDB) {
+ long_cmdp = kzalloc(hp->cmd_len, GFP_KERNEL);
+ if (!long_cmdp)
+ return -ENOMEM;
+ }
rq = blk_get_request(q, rw, GFP_ATOMIC);
- if (!rq)
+ if (!rq) {
+ kfree(long_cmdp);
return -ENOMEM;
+ }
+ if (hp->cmd_len > BLK_MAX_CDB)
+ rq->cmd = long_cmdp;
memcpy(rq->cmd, cmd, hp->cmd_len);
rq->cmd_len = hp->cmd_len;
@@ -1739,6 +1746,8 @@ static int sg_finish_rem_req(Sg_request * srp)
if (srp->bio)
ret = blk_rq_unmap_user(srp->bio);
+ if (srp->rq->cmd != srp->rq->__cmd)
+ kfree(srp->rq->cmd);
blk_put_request(srp->rq);
}
diff --git a/include/scsi/sg.h b/include/scsi/sg.h
index a9f3c6f..d8c0c43 100644
--- a/include/scsi/sg.h
+++ b/include/scsi/sg.h
@@ -4,77 +4,34 @@
#include
/*
- History:
-Started: Aug 9 by Lawrence Foard (entr...@world.std.com), to allow user
- process control of SCSI devices.
-Development Sponsored by Killy Corp. NY NY
-Original driver (sg.h):
-* Copyright (C) 1992 Lawrence Foard
-Version 2 and 3 extensions to driver:
-* Copyright (C) 1998 - 200
Re: [PATCH v2] sg: add SG_FLAG_Q_AT_TAIL flag
On 14-06-05 11:27 AM, Boaz Harrosh wrote: On 06/04/2014 05:58 PM, Douglas Gilbert wrote: When the SG_IO ioctl was copied into the block layer and later into the bsg driver, subtle differences emerged. One difference is the way injected commands are queued through the block layer (i.e. this is not SCSI device queueing nor SATA NCQ). Summarizing: - SG_IO in the block layer: blk_exec*(at_head=false) - sg SG_IO: at_head=true - bsg SG_IO: at_head=true Some time ago Boaz Harrosh introduced a sg v4 flag called BSG_FLAG_Q_AT_TAIL to override the bsg driver default. This patch does the equivalent for the sg driver. Yep ChangeLog: Introduce SG_FLAG_Q_AT_TAIL flag to cause commands to be injected into the block layer with at_head=false. Changes since v1: Make guard condition (only take sg v3 interface or later invocations) clearer. Signed-off-by: Douglas Gilbert Douglas Hi Please I'm just curious? up until now all application users used "SG_FLAG_Q_AT_HEAD". Therefor I deduce that you guys came across a new application that can make use of the new SG_FLAG_Q_AT_TAIL facility. sg_dd and more recently ddpt request the equivalent of SG_FLAG_Q_AT_TAIL on SCSI READs and WRITEs. So that is the default with a Linux block device, implemented with a bsg device, and ignored with a sg device. When you think of dd with POSIX threading (e.g. sgp_dd) then SG_FLAG_Q_AT_HEAD seems rather counter-productive. Also WRITE_ATOMIC with SG_FLAG_Q_AT_HEAD seems to be asking for trouble (unless power failure was imminent). OTOH an INQUIRY with SG_FLAG_Q_AT_TAIL is a bit strange as well (as it is implicit "head of queue" through SCSI device queues). What was the application's writer considerations for using the old sg interface and not use the newer bsg interface that already has this support. For me I can see 2 main areas that I find bsg missing. 1. aio "scatter_gather" type io. (ie multiple pointers multiple length buffers that are written / read from same linear range on device) [The async aspect of aio can be implemented via bsg with the write+read system calls] 2. mmap of direct device range to user vm memory Which of these, or other, considerations where cardinal in using of the old interface in new code? (For me personally both above shortcomings are very much missed] Not a subject that I wanted to bring up by since you ask ... The bsg driver first appeared in the kernel in lk 2.6.23 which was released in October 2007. It probably took a few releases to be usable, lets say it has been in place for 6 years. In that time there have been no new features added to the sg driver while several new features have been added to bsg (e.g. async support) but in the last year or so, it has lost its active maintainer: Fujita Tomonori (aka Tomo). Various bugs have been reported against the bsg driver (actually some against the sg driver and tests showed the bsg driver had the same problem of worse). No fixes have been presented for any of bsg's recently reported problems as far as I am aware. Consider me a part-time kernel driver maintainer so the sg and scsi_debug drivers are more than enough for me. I have no desire to pick up the bsg driver. Volunteers can contact Jens. As a tool maker I get another view of various SCSI (and related) pass-throughs across several OSes. sg3_utils has been ambivalent about which Linux SCSI pass-through it uses since version 1.27 [20090411], about 5 years. Judging from my email, Linux users demonstrate problems and suggestions using the these devices, ordered by frequency: - block devices (e.g. /dev/sdc) - sg devices - bsg devices I could be wrong about the order of the first two, but bsg devices are a long way last (e.g. a handful in the last two years). At a low level, bsg is not helped by only supporting the v4 interface while block devices and the sg devices only support the v3 interface. To most sg3_utils users this is almost invisible since its library chooses between the two interfaces dynamically. Only issuing something like VERIFY(32) via sg_verify to a Linux sg device would demonstrate a difference (i.e. it would work with a bsg device, fail with a sg device). I also maintain the SCSI side of smartmontools and adding bsg support is on my ToDo list. The most active maintainer has responded: why bother? And that seems to be a common response from those familiar with these issues. So the bsg driver, IMO, has become a niche player, with some important niches. OSD users need bi-directional, variable length commands. Several transports use the bsg driver as a transport layer pass-through. For example my smp_utils package uses the bsg driver in that fashion, thus avoiding several proprietary interfaces. The O_EXCL issue is instructive. Vaughan Cao brought this up last year (or earlier ?). Basically the sg driver's handling of the O_EXCL flag was flaky, not obviously, but if you threw enough threads at it, bad thing
[PATCH 3/4] [SCSI] qlogicfas: don't call free_dma()
The qlogicfas scsi driver does not use DMA, and the call to free_dma() in its exit function seems to have been copied incorrectly from another driver but never caused trouble. One case where it gets in the way is randconfig builds on ARM, which depending on the configuration does not provide a free_dma() function, causing this build error: drivers/scsi/qlogicfas.c: In function 'qlogicfas_release': drivers/scsi/qlogicfas.c:175:3: error: implicit declaration of function 'free_dma' [-Werror=implicit-function-declaration] free_dma(shost->dma_channel); ^ Removing the incorrect function calls should be the obvious fix for this, with no downsides. Signed-off-by: Arnd Bergmann --- drivers/scsi/qlogicfas.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/scsi/qlogicfas.c b/drivers/scsi/qlogicfas.c index 13d628b..a22bb1b 100644 --- a/drivers/scsi/qlogicfas.c +++ b/drivers/scsi/qlogicfas.c @@ -171,8 +171,6 @@ static int qlogicfas_release(struct Scsi_Host *shost) qlogicfas408_disable_ints(priv); free_irq(shost->irq, shost); } - if (shost->dma_channel != 0xff) - free_dma(shost->dma_channel); if (shost->io_port && shost->n_io_port) release_region(shost->io_port, shost->n_io_port); scsi_host_put(shost); -- 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] [SCSI] pas16: don't call free_dma()
The pas16 scsi driver does not use DMA, and the call to free_dma() in its exit function seems to have been copied incorrectly from another driver but never caused trouble. One case where it gets in the way is randconfig builds on ARM, which depending on the configuration does not provide a free_dma() function, causing this build error: drivers/scsi/pas16.c: In function 'pas16_release': drivers/scsi/pas16.c:611:3: error: implicit declaration of function 'free_dma' [-Werror=implicit-function-declaration] free_dma(shost->dma_channel); Removing the incorrect function calls should be the obvious fix for this, with no downsides. Signed-off-by: Arnd Bergmann Cc: Finn Thain Cc: Michael Schmitz --- drivers/scsi/pas16.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/scsi/pas16.c b/drivers/scsi/pas16.c index 0d78a4d..80bacb5 100644 --- a/drivers/scsi/pas16.c +++ b/drivers/scsi/pas16.c @@ -607,8 +607,6 @@ static int pas16_release(struct Scsi_Host *shost) if (shost->irq) free_irq(shost->irq, shost); NCR5380_exit(shost); - if (shost->dma_channel != 0xff) - free_dma(shost->dma_channel); if (shost->io_port && shost->n_io_port) release_region(shost->io_port, shost->n_io_port); scsi_unregister(shost); -- 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4] ARM randconfig fixes for SCSI
Hi James, These are some fixes for ancient randconfig build bugs I ran into on ARM. Clearly none of these are urgent, but it would be nice to have them merged for 3.17 if they look good to you. Arnd Bergmann (4): [SCSI] Don't build AdvanSys on ARM [SCSI] pas16: don't call free_dma() [SCSI] qlogicfas: don't call free_dma() [SCSI] NCR53c406a: don't call free_dma() by default drivers/scsi/Kconfig | 2 +- drivers/scsi/NCR53c406a.c | 2 +- drivers/scsi/pas16.c | 2 -- drivers/scsi/qlogicfas.c | 2 -- 4 files changed, 2 insertions(+), 6 deletions(-) -- 1.8.3.2 Cc: Matthew Wilcox Cc: Finn Thain Cc: Michael Schmitz -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] [SCSI] Don't build AdvanSys on ARM
The advansys SCSI driver uses the dma_cache_sync function, which is not available on the ARM architecture, and cannot be implemented correctly, so we always get this build error: drivers/scsi/advansys.c: In function 'advansys_get_sense_buffer_dma': drivers/scsi/advansys.c:7882:2: error: implicit declaration of function 'dma_cache_sync' [-Werror=implicit-function-declaration] dma_cache_sync(board->dev, scp->sense_buffer, ^ It seems nobody has missed this driver so far, so let's just disable it for ARM to help randconfig builds. Signed-off-by: Arnd Bergmann Cc: Matthew Wilcox --- drivers/scsi/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig index baca589..8368e00 100644 --- a/drivers/scsi/Kconfig +++ b/drivers/scsi/Kconfig @@ -528,7 +528,7 @@ config SCSI_DPT_I2O config SCSI_ADVANSYS tristate "AdvanSys SCSI support" - depends on SCSI && VIRT_TO_BUS + depends on SCSI && VIRT_TO_BUS && !ARM depends on ISA || EISA || PCI help This is a driver for all SCSI host adapters manufactured by -- 1.8.3.2 -- 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 4/4] [SCSI] NCR53c406a: don't call free_dma() by default
The NCR53c406a scsi driver normally does not use DMA, unless
the USE_PIO macro is disabled by modifying the source code.
The call to free_dma() for some reason uses #ifdef USE_DMA,
which does not do the right thing, since USE_DMA is defined
as a boolean that is either 0 or 1, but always present.
One case where it gets in the way is randconfig builds on ARM,
which depending on the configuration does not provide a free_dma()
function, causing this build error:
drivers/scsi/NCR53c406a.c: In function 'NCR53c406a_release':
drivers/scsi/NCR53c406a.c:600:3: error: implicit declaration of function
'free_dma' [-Werror=implicit-function-declaration]
free_dma(shost->dma_channel);
^
This changes the code to use #if USE_DMA, to match the
rest of the file, which seems to be what the author intended.
Signed-off-by: Arnd Bergmann
---
drivers/scsi/NCR53c406a.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/NCR53c406a.c b/drivers/scsi/NCR53c406a.c
index c91888a..2c78785 100644
--- a/drivers/scsi/NCR53c406a.c
+++ b/drivers/scsi/NCR53c406a.c
@@ -595,7 +595,7 @@ static int NCR53c406a_release(struct Scsi_Host *shost)
{
if (shost->irq)
free_irq(shost->irq, NULL);
-#ifdef USE_DMA
+#if USE_DMA
if (shost->dma_channel != 0xff)
free_dma(shost->dma_channel);
#endif
--
1.8.3.2
--
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] libiscsi, iser: Adjust data_length to include protection information
On 6/4/2014 1:18 AM, Martin K. Petersen wrote:
"Mike" == Mike Christie writes:
Mike> On 06/01/2014 11:19 AM, Sagi Grimberg wrote:
+/*
+ * data integrity helpers
+ */
+static inline unsigned +iscsi_prot_len(unsigned data_len, unsigned
sector_size) +{
+ switch (sector_size) {
+ case 512:
+ return (data_len >> 9) * 8;
+ case 1024:
+ return (data_len >> 10) * 8;
+ case 2048:
+ return (data_len >> 11) * 8;
+ case 4096:
+ return (data_len >> 12) * 8;
+ default:
+ return (data_len >> ilog2(sector_size)) * 8;
+ }
+}
#endif
Mike> I do not think this should not be in the iscsi code.
In the data integrity update I posted there's a flag saying "transfer PI
on the wire". That was meant to be the thing driver's should key off of
to adjust transfer length. But I'm also happy to provide a unsigned int
scsi_transfer_length(struct scsi_cmnd *) thingy that returns the right
byte count. Just bear in mind that the host-facing DIX transfer length
may be different.
OK, let me prepare v1 moving this logic to a scsi helper and we'll have
another round of comments.
Thanks,
Sagi.
--
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] libiscsi, iser: Adjust data_length to include protection information
On 6/3/2014 7:11 PM, Mike Christie wrote: On 06/01/2014 11:19 AM, Sagi Grimberg wrote: /** + * iscsi_adjust_dl - Adjust SCSI data length to include PI + * @sc: scsi command. + * @data_length: command data length. + * + * Adjust the data length to account for how much data + * is actually on the wire. + * + * returns the adjusted data length + **/ +static unsigned +iscsi_adjust_dl(struct scsi_cmnd *sc, unsigned data_len) Hey, one other comment. Could you rename this to iscsi_adjust_data_len or iscsi_adjust_dlength? It is more common in the iscsi code to use those names for data length. No need - I moved this logic to a scsi helper anyway (as MKP suggested)... Thanks! Sagi. -- 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 0/2] Include protection information in iscsi header
On 6/3/2014 9:16 AM, Roland Dreier wrote: On Sun, Jun 1, 2014 at 9:19 AM, Sagi Grimberg wrote: Although these patches involve 3 subsystems with different maintainers (scsi, iser, target) I would prefer seeing these patches included together. Why? Because they break wire compatibility? I hate to say it but even if they're merged at the same time, you can't guarantee that targets and initiators will be updated together. Yes that's true, but still I would like to avoid a kernel release that the target and initiator can't talk to one another... Sagi. -- 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] sg: add SG_FLAG_Q_AT_TAIL flag
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 6/5/2014 10:27 AM, Boaz Harrosh wrote: > 1. aio "scatter_gather" type io. (ie multiple pointers multiple length > buffers that are written / read from same linear range on device) [The > async aspect of aio can be implemented via bsg with the write+read system > calls] 2. mmap of direct device range to user vm memory I suspect that belies a bit of a gap in the understanding of the kinds of applications that use pass-through (vs just using sd, or using it for a guest OS). These use cases don't tend to be useful for things like SCSI changers, tape devices, or SES devices. What is useful is the ability to reset devices, or maybe some of the other "edge" features provided by SG that never managed to make it into bsg. Nor are they useful for the monitoring type applications that use pass-through to pull some vendor specific statistic or device state. Furthermore, i've see a fair number of cases where people slap together shell scripts using the /dev/sg* handles instead of the /dev/bsg/* ones probably because its simply more convenient. -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.10 (MingW32) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBAgAGBQJTkKYLAAoJEL5i86xrzcy78ZEIAK9s8hcgtX3bloYbW+09OHWu M12ySzk6hEOvJcGZwoBobkG5q9cHPk1ehaCtzaTE5MlBaSOSfg+AAHVUusr3PUZR REmwS+eBZu6wRghXPE6c0oLuBulQ1FeJXkDsfuRhkaoBfZxfc/BiTEb67CCbHPm4 gT34VCiVRB0G0Sp5rnu9S9f1LvRmF2DoMCK+CmCBNh0q/dD3EskQJOh5c9sAKHKJ 0TO1LyuRj5jUILgOma/gHX3LHa7JN9EE+DKK5mm8s75vMKwv8FpWMc6B9LeOfcIn XDDMM5tdrtbXMvZ6M5jp+bhbnoydxhRHgXBpiTMe3ze4VZXXLdmSBX/am9oVhKA= =TdvH -END PGP SIGNATURE- -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire
On 6/2/2014 7:54 PM, Martin K. Petersen wrote:
"Sagi" == Sagi Grimberg writes:
Sagi,
Sagi> In various areas of the code, it is assumed that
Sagi> se_cmd-> data_length describes pure data. In case
Sagi> that protection information exists over the wire (protect bits is
Sagi> are on) the target core decrease the protection length from the
Sagi> data length (instead of each transport peeking in the cdb).
I completely agree with the notion of including PI in the transport byte
count.
Why do you open code the transfer length adjustment below?
Can't say I have a good reason for that.
I'll move this logic to scsi_cmnd.h.
+ /**
+* Adjust data_length to include
+* protection information
+**/
+ switch (sc->device->sector_size) {
+ case 512:
+ data_len += (data_len >> 9) * 8;
+ break;
+ case 1024:
+ data_len += (data_len >> 10) * 8;
+ break;
+ case 2048:
+ data_len += (data_len >> 11) * 8;
+ break;
+ case 4096:
+ data_len += (data_len >> 12) * 8;
+ break;
+ default:
+ data_len +=
+ (data_len >> ilog2(sc->device->sector_size)) *
8;
+ }
--
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 v5] sg: relax 16 byte cdb restriction
On 06/05/2014 06:40 PM, Boaz Harrosh wrote: > On 06/03/2014 08:18 PM, Douglas Gilbert wrote: >> v4 of this patch was sent 20131201. >> >> ChangeLog: >> - remove the 16 byte CDB (SCSI command) length limit >>from the sg driver by handling longer CDBs the same >>way as the bsg driver. Remove comment from sg.h >>public interface about the cmd_len field being >>limited to 16 bytes. >> - remove some dead code caused by this change >> - cleanup comment block at the top of sg.h, fix urls >> >> Signed-off-by: Douglas Gilbert >> >> >> sg_cdb_dg5.patch >> >> > <> >> >> +/* SG_MAX_CDB_SIZE should be 260 (spc4r37 section 3.1.30) however the type >> + * of sg_io_hdr::cmd_len can only represent 255 >> + */ >> +#define SG_MAX_CDB_SIZE 255 >> + <> BTW here too. Why new code is using the old interface? I thought that the all point for having bsg at all is that new stuff are implemented there in a cleaner interface for example large CDBs. Otherwise what was the point for bsg in the first place? (It was before my time, It would be nice to know?) What is then left unique at bsg after this patch? only bidi? Thanks Boaz -- 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 v5] sg: relax 16 byte cdb restriction
On 06/03/2014 08:18 PM, Douglas Gilbert wrote:
> v4 of this patch was sent 20131201.
>
> ChangeLog:
> - remove the 16 byte CDB (SCSI command) length limit
>from the sg driver by handling longer CDBs the same
>way as the bsg driver. Remove comment from sg.h
>public interface about the cmd_len field being
>limited to 16 bytes.
> - remove some dead code caused by this change
> - cleanup comment block at the top of sg.h, fix urls
>
> Signed-off-by: Douglas Gilbert
>
>
> sg_cdb_dg5.patch
>
>
<>
>
> +/* SG_MAX_CDB_SIZE should be 260 (spc4r37 section 3.1.30) however the type
> + * of sg_io_hdr::cmd_len can only represent 255
> + */
> +#define SG_MAX_CDB_SIZE 255
> +
Just a nit on above comment (code is all good). CDB bigger then 16 is 8 bytes
aligned
So the maximum for sg is 252 not 255 as above.
(As you said theoretical max is 260 but since it would not fit then the
next allowed size is 252)
> /*
> * Suppose you want to calculate the formula muldiv(x,m,d)=int(x * m / d)
> * Then when using 32 bit integers x * m may overflow during the calculation.
> @@ -161,7 +164,7 @@ typedef struct sg_fd {/* holds the state of a
> file descriptor */
> char low_dma; /* as in parent but possibly overridden to 1 */
> char force_packid; /* 1 -> pack_id input to read(), 0 -> ignored */
> char cmd_q; /* 1 -> allow command queuing, 0 -> don't */
> - char next_cmd_len; /* 0 -> automatic (def), >0 -> use on next
> write() */
> + unsigned char next_cmd_len; /* 0: automatic, >0: use on next write() */
> char keep_orphan; /* 0 -> drop orphan (def), 1 -> keep for read()
> */
> char mmap_called; /* 0 -> mmap() never called on this fd */
> struct kref f_ref;
> @@ -566,7 +569,7 @@ sg_write(struct file *filp, const char __user *buf,
> size_t count, loff_t * ppos)
> Sg_request *srp;
> struct sg_header old_hdr;
> sg_io_hdr_t *hp;
> - unsigned char cmnd[MAX_COMMAND_SIZE];
> + unsigned char cmnd[SG_MAX_CDB_SIZE];
>
> if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
> return -ENXIO;
> @@ -598,12 +601,6 @@ sg_write(struct file *filp, const char __user *buf,
> size_t count, loff_t * ppos)
> buf += SZ_SG_HEADER;
> __get_user(opcode, buf);
> if (sfp->next_cmd_len > 0) {
> - if (sfp->next_cmd_len > MAX_COMMAND_SIZE) {
> - SCSI_LOG_TIMEOUT(1, printk("sg_write: command length
> too long\n"));
> - sfp->next_cmd_len = 0;
> - sg_remove_request(sfp, srp);
> - return -EIO;
> - }
> cmd_size = sfp->next_cmd_len;
> sfp->next_cmd_len = 0; /* reset so only this write() effected
> */
> } else {
> @@ -675,7 +672,7 @@ sg_new_write(Sg_fd *sfp, struct file *file, const char
> __user *buf,
> int k;
> Sg_request *srp;
> sg_io_hdr_t *hp;
> - unsigned char cmnd[MAX_COMMAND_SIZE];
> + unsigned char cmnd[SG_MAX_CDB_SIZE];
> int timeout;
> unsigned long ul_timeout;
>
> @@ -1645,14 +1642,24 @@ static int sg_start_req(Sg_request *srp, unsigned
> char *cmd)
> struct request_queue *q = sfp->parentdp->device->request_queue;
> struct rq_map_data *md, map_data;
> int rw = hp->dxfer_direction == SG_DXFER_TO_DEV ? WRITE : READ;
> + unsigned char *long_cmdp = NULL;
>
> SCSI_LOG_TIMEOUT(4, printk(KERN_INFO "sg_start_req: dxfer_len=%d\n",
> dxfer_len));
> + if (hp->cmd_len > BLK_MAX_CDB) {
> + long_cmdp = kzalloc(hp->cmd_len, GFP_KERNEL);
> + if (!long_cmdp)
> + return -ENOMEM;
> + }
>
> rq = blk_get_request(q, rw, GFP_ATOMIC);
> - if (!rq)
> + if (!rq) {
> + kfree(long_cmdp);
> return -ENOMEM;
> + }
>
> + if (hp->cmd_len > BLK_MAX_CDB)
> + rq->cmd = long_cmdp;
> memcpy(rq->cmd, cmd, hp->cmd_len);
>
> rq->cmd_len = hp->cmd_len;
> @@ -1739,6 +1746,8 @@ static int sg_finish_rem_req(Sg_request * srp)
> if (srp->bio)
> ret = blk_rq_unmap_user(srp->bio);
>
> + if (srp->rq->cmd != srp->rq->__cmd)
> + kfree(srp->rq->cmd);
> blk_put_request(srp->rq);
> }
>
> diff --git a/include/scsi/sg.h b/include/scsi/sg.h
> index a9f3c6f..d8c0c43 100644
> --- a/include/scsi/sg.h
> +++ b/include/scsi/sg.h
> @@ -4,77 +4,34 @@
> #include
>
> /*
> - History:
> -Started: Aug 9 by Lawrence Foard (entr...@world.std.com), to allow user
> - process control of SCSI devices.
> -Development Sponsored by Killy Corp. NY NY
> -Original driver (sg.h):
> -* Copyright (C) 1992 Lawrence Foard
> -Version 2 and 3 extensions to driver:
> -* Copyright (C) 1998
Re: [PATCH v2] sg: add SG_FLAG_Q_AT_TAIL flag
On 06/04/2014 05:58 PM, Douglas Gilbert wrote: > When the SG_IO ioctl was copied into the block layer and > later into the bsg driver, subtle differences emerged. > > One difference is the way injected commands are queued through > the block layer (i.e. this is not SCSI device queueing nor SATA > NCQ). Summarizing: >- SG_IO in the block layer: blk_exec*(at_head=false) >- sg SG_IO: at_head=true >- bsg SG_IO: at_head=true > > Some time ago Boaz Harrosh introduced a sg v4 flag called > BSG_FLAG_Q_AT_TAIL to override the bsg driver default. > This patch does the equivalent for the sg driver. > Yep > > ChangeLog: > Introduce SG_FLAG_Q_AT_TAIL flag to cause commands > to be injected into the block layer with > at_head=false. > > Changes since v1: > Make guard condition (only take sg v3 interface or later > invocations) clearer. > > Signed-off-by: Douglas Gilbert > Douglas Hi Please I'm just curious? up until now all application users used "SG_FLAG_Q_AT_HEAD". Therefor I deduce that you guys came across a new application that can make use of the new SG_FLAG_Q_AT_TAIL facility. What was the application's writer considerations for using the old sg interface and not use the newer bsg interface that already has this support. For me I can see 2 main areas that I find bsg missing. 1. aio "scatter_gather" type io. (ie multiple pointers multiple length buffers that are written / read from same linear range on device) [The async aspect of aio can be implemented via bsg with the write+read system calls] 2. mmap of direct device range to user vm memory Which of these, or other, considerations where cardinal in using of the old interface in new code? (For me personally both above shortcomings are very much missed] Thanks Boaz -- 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] block SG_IO: add SG_FLAG_Q_AT_HEAD flag
This would be something for Jens to pick up.
Looks good to me,
Reviewed-by: Christoph Hellwig
Next step would be to switch to the same default for all
implementations..
On Thu, Jun 05, 2014 at 10:02:22AM -0400, Douglas Gilbert wrote:
> After the SG_IO ioctl was copied into the block layer and
> later into the bsg driver, subtle differences emerged.
>
> One difference is the way injected commands are queued through
> the block layer (i.e. this is not SCSI device queueing nor SATA
> NCQ). Summarizing:
> - SG_IO on block layer device: blk_exec*(at_head=false)
> - sg device SG_IO: at_head=true
> - bsg device SG_IO: at_head=true
>
> Some time ago Boaz Harrosh introduced a sg v4 flag called
> BSG_FLAG_Q_AT_TAIL to override the bsg driver default. A
> recent patch titled: "sg: add SG_FLAG_Q_AT_TAIL flag"
> allowed the sg driver default to be overridden. This patch
> allows a SG_IO ioctl sent to a block layer device to have
> its default overridden.
>
> ChangeLog:
> - introduce SG_FLAG_Q_AT_HEAD flag in sg.h to cause
> commands that are injected via a block layer
> device SG_IO ioctl to set at_head=true
> - make comments clearer about queueing in sg.h since the
> header is used both by the sg device and block layer
> device implementations of the SG_IO ioctl.
> - introduce BSG_FLAG_Q_AT_HEAD in bsg.h for compatibility
> (it does nothing) and update comments.
>
>
> Signed-off-by: Douglas Gilbert
> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
> index 2648797..e49b7ef 100644
> --- a/block/scsi_ioctl.c
> +++ b/block/scsi_ioctl.c
> @@ -288,6 +288,7 @@ static int sg_io(struct request_queue *q, struct gendisk
> *bd_disk,
> unsigned long start_time;
> ssize_t ret = 0;
> int writing = 0;
> + int at_head = 0;
> struct request *rq;
> char sense[SCSI_SENSE_BUFFERSIZE];
> struct bio *bio;
> @@ -311,6 +312,8 @@ static int sg_io(struct request_queue *q, struct gendisk
> *bd_disk,
> case SG_DXFER_FROM_DEV:
> break;
> }
> + if (hdr->flags & SG_FLAG_Q_AT_HEAD)
> + at_head = 1;
>
> rq = blk_get_request(q, writing ? WRITE : READ, GFP_KERNEL);
> if (!rq)
> @@ -366,7 +369,7 @@ static int sg_io(struct request_queue *q, struct gendisk
> *bd_disk,
>* (if he doesn't check that is his problem).
>* N.B. a non-zero SCSI status is _not_ necessarily an error.
>*/
> - blk_execute_rq(q, bd_disk, rq, 0);
> + blk_execute_rq(q, bd_disk, rq, at_head);
>
> hdr->duration = jiffies_to_msecs(jiffies - start_time);
>
> diff --git a/include/scsi/sg.h b/include/scsi/sg.h
> index 9859355..750e5db 100644
> --- a/include/scsi/sg.h
> +++ b/include/scsi/sg.h
> @@ -86,7 +86,9 @@ typedef struct sg_io_hdr
> #define SG_FLAG_MMAP_IO 4 /* request memory mapped IO */
> #define SG_FLAG_NO_DXFER 0x1 /* no transfer of kernel buffers to/from */
> /* user space (debug indirect IO) */
> -#define SG_FLAG_Q_AT_TAIL 0x10 /* default is Q_AT_HEAD */
> +/* defaults:: for sg driver: Q_AT_HEAD; for block layer: Q_AT_TAIL */
> +#define SG_FLAG_Q_AT_TAIL 0x10
> +#define SG_FLAG_Q_AT_HEAD 0x20
>
> /* following 'info' values are "or"-ed together */
> #define SG_INFO_OK_MASK 0x1
> diff --git a/include/uapi/linux/bsg.h b/include/uapi/linux/bsg.h
> index 7a12e1c..02986cf 100644
> --- a/include/uapi/linux/bsg.h
> +++ b/include/uapi/linux/bsg.h
> @@ -10,12 +10,13 @@
> #define BSG_SUB_PROTOCOL_SCSI_TRANSPORT 2
>
> /*
> - * For flags member below
> - * sg.h sg_io_hdr also has bits defined for it's flags member. However
> - * none of these bits are implemented/used by bsg. The bits below are
> - * allocated to not conflict with sg.h ones anyway.
> + * For flag constants below:
> + * sg.h sg_io_hdr also has bits defined for it's flags member. These
> + * two flag values (0x10 and 0x20) have the same meaning in sg.h . For
> + * bsg the BSG_FLAG_Q_AT_HEAD flag is ignored since it is the deafult.
> */
> -#define BSG_FLAG_Q_AT_TAIL 0x10 /* default, == 0 at this bit, is Q_AT_HEAD */
> +#define BSG_FLAG_Q_AT_TAIL 0x10 /* default is Q_AT_HEAD */
> +#define BSG_FLAG_Q_AT_HEAD 0x20
>
> struct sg_io_v4 {
> __s32 guard;/* [i] 'Q' to differentiate from v3 */
---end quoted text---
--
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] block SG_IO: add SG_FLAG_Q_AT_HEAD flag
After the SG_IO ioctl was copied into the block layer and
later into the bsg driver, subtle differences emerged.
One difference is the way injected commands are queued through
the block layer (i.e. this is not SCSI device queueing nor SATA
NCQ). Summarizing:
- SG_IO on block layer device: blk_exec*(at_head=false)
- sg device SG_IO: at_head=true
- bsg device SG_IO: at_head=true
Some time ago Boaz Harrosh introduced a sg v4 flag called
BSG_FLAG_Q_AT_TAIL to override the bsg driver default. A
recent patch titled: "sg: add SG_FLAG_Q_AT_TAIL flag"
allowed the sg driver default to be overridden. This patch
allows a SG_IO ioctl sent to a block layer device to have
its default overridden.
ChangeLog:
- introduce SG_FLAG_Q_AT_HEAD flag in sg.h to cause
commands that are injected via a block layer
device SG_IO ioctl to set at_head=true
- make comments clearer about queueing in sg.h since the
header is used both by the sg device and block layer
device implementations of the SG_IO ioctl.
- introduce BSG_FLAG_Q_AT_HEAD in bsg.h for compatibility
(it does nothing) and update comments.
Signed-off-by: Douglas Gilbert
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 2648797..e49b7ef 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -288,6 +288,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
unsigned long start_time;
ssize_t ret = 0;
int writing = 0;
+ int at_head = 0;
struct request *rq;
char sense[SCSI_SENSE_BUFFERSIZE];
struct bio *bio;
@@ -311,6 +312,8 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
case SG_DXFER_FROM_DEV:
break;
}
+ if (hdr->flags & SG_FLAG_Q_AT_HEAD)
+ at_head = 1;
rq = blk_get_request(q, writing ? WRITE : READ, GFP_KERNEL);
if (!rq)
@@ -366,7 +369,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
* (if he doesn't check that is his problem).
* N.B. a non-zero SCSI status is _not_ necessarily an error.
*/
- blk_execute_rq(q, bd_disk, rq, 0);
+ blk_execute_rq(q, bd_disk, rq, at_head);
hdr->duration = jiffies_to_msecs(jiffies - start_time);
diff --git a/include/scsi/sg.h b/include/scsi/sg.h
index 9859355..750e5db 100644
--- a/include/scsi/sg.h
+++ b/include/scsi/sg.h
@@ -86,7 +86,9 @@ typedef struct sg_io_hdr
#define SG_FLAG_MMAP_IO 4 /* request memory mapped IO */
#define SG_FLAG_NO_DXFER 0x1 /* no transfer of kernel buffers to/from */
/* user space (debug indirect IO) */
-#define SG_FLAG_Q_AT_TAIL 0x10 /* default is Q_AT_HEAD */
+/* defaults:: for sg driver: Q_AT_HEAD; for block layer: Q_AT_TAIL */
+#define SG_FLAG_Q_AT_TAIL 0x10
+#define SG_FLAG_Q_AT_HEAD 0x20
/* following 'info' values are "or"-ed together */
#define SG_INFO_OK_MASK 0x1
diff --git a/include/uapi/linux/bsg.h b/include/uapi/linux/bsg.h
index 7a12e1c..02986cf 100644
--- a/include/uapi/linux/bsg.h
+++ b/include/uapi/linux/bsg.h
@@ -10,12 +10,13 @@
#define BSG_SUB_PROTOCOL_SCSI_TRANSPORT 2
/*
- * For flags member below
- * sg.h sg_io_hdr also has bits defined for it's flags member. However
- * none of these bits are implemented/used by bsg. The bits below are
- * allocated to not conflict with sg.h ones anyway.
+ * For flag constants below:
+ * sg.h sg_io_hdr also has bits defined for it's flags member. These
+ * two flag values (0x10 and 0x20) have the same meaning in sg.h . For
+ * bsg the BSG_FLAG_Q_AT_HEAD flag is ignored since it is the deafult.
*/
-#define BSG_FLAG_Q_AT_TAIL 0x10 /* default, == 0 at this bit, is Q_AT_HEAD */
+#define BSG_FLAG_Q_AT_TAIL 0x10 /* default is Q_AT_HEAD */
+#define BSG_FLAG_Q_AT_HEAD 0x20
struct sg_io_v4 {
__s32 guard; /* [i] 'Q' to differentiate from v3 */
Re: [PATCH v5] sg: relax 16 byte cdb restriction
Looks good, Reviewed-by: Christoph Hellwig -- 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
[Resend PATCH V1 4/4] scsi: ufs: Fix queue depth handling for best effort cases
From: Sujit Reddy Thumma
Some UFS devices may expose bLUQueueDepth field as zero indicating
that the queue depth depends on the number of resources available
for LUN at a particular instant to handle the outstanding transfer
requests. Currently, when response for SCSI command is TASK_FULL
the LLD decrements the queue depth but fails to increment when the
resources are available. The scsi mid-layer handles the change in
queue depth heuristically and offers simple interface with
->change_queue_depth.
Signed-off-by: Sujit Reddy Thumma
Signed-off-by: Dolev Raviv
---
drivers/scsi/ufs/ufshcd.c | 97 ---
1 file changed, 42 insertions(+), 55 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b301ed8..b103e95 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1991,27 +1991,55 @@ static int ufshcd_slave_alloc(struct scsi_device *sdev)
/* allow SCSI layer to restart the device in case of errors */
sdev->allow_restart = 1;
+
lun_qdepth = ufshcd_read_sdev_qdepth(hba, sdev);
- if (lun_qdepth == 0 || lun_qdepth > hba->nutrs) {
- dev_info(hba->dev, "%s, lun %d queue depth is %d\n", __func__,
- sdev->lun, lun_qdepth);
+ if (lun_qdepth <= 0)
+ /* eventually, we can figure out the real queue depth */
lun_qdepth = hba->nutrs;
- } else if (lun_qdepth < 0) {
- lun_qdepth = 1;
- }
+ else
+ lun_qdepth = min_t(int, lun_qdepth, hba->nutrs);
- /*
-* Inform SCSI Midlayer that the LUN queue depth is same as the
-* controller queue depth. If a LUN queue depth is less than the
-* controller queue depth and if the LUN reports
-* SAM_STAT_TASK_SET_FULL, the LUN queue depth will be adjusted
-* with scsi_adjust_queue_depth.
-*/
+ dev_dbg(hba->dev, "%s: activate tcq with queue depth %d\n",
+ __func__, lun_qdepth);
scsi_activate_tcq(sdev, lun_qdepth);
+
return 0;
}
/**
+ * ufshcd_change_queue_depth - change queue depth
+ * @sdev: pointer to SCSI device
+ * @depth: required depth to set
+ * @reason: reason for changing the depth
+ *
+ * Change queue depth according to the reason and make sure
+ * the max. limits are not crossed.
+ */
+int ufshcd_change_queue_depth(struct scsi_device *sdev, int depth, int reason)
+{
+ struct ufs_hba *hba = shost_priv(sdev->host);
+
+ if (depth > hba->nutrs)
+ depth = hba->nutrs;
+
+ switch (reason) {
+ case SCSI_QDEPTH_DEFAULT:
+ case SCSI_QDEPTH_RAMP_UP:
+ if (!sdev->tagged_supported)
+ depth = 1;
+ scsi_adjust_queue_depth(sdev, scsi_get_tag_type(sdev), depth);
+ break;
+ case SCSI_QDEPTH_QFULL:
+ scsi_track_queue_full(sdev, depth);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return depth;
+}
+
+/**
* ufshcd_slave_destroy - remove SCSI device configurations
* @sdev: pointer to SCSI device
*/
@@ -2064,42 +2092,6 @@ static int ufshcd_task_req_compl(struct ufs_hba *hba,
u32 index, u8 *resp)
}
/**
- * ufshcd_adjust_lun_qdepth - Update LUN queue depth if device responds with
- * SAM_STAT_TASK_SET_FULL SCSI command status.
- * @cmd: pointer to SCSI command
- */
-static void ufshcd_adjust_lun_qdepth(struct scsi_cmnd *cmd)
-{
- struct ufs_hba *hba;
- int i;
- int lun_qdepth = 0;
-
- hba = shost_priv(cmd->device->host);
-
- /*
-* LUN queue depth can be obtained by counting outstanding commands
-* on the LUN.
-*/
- for (i = 0; i < hba->nutrs; i++) {
- if (test_bit(i, &hba->outstanding_reqs)) {
-
- /*
-* Check if the outstanding command belongs
-* to the LUN which reported SAM_STAT_TASK_SET_FULL.
-*/
- if (cmd->device->lun == hba->lrb[i].lun)
- lun_qdepth++;
- }
- }
-
- /*
-* LUN queue depth will be total outstanding commands, except the
-* command for which the LUN reported SAM_STAT_TASK_SET_FULL.
-*/
- scsi_adjust_queue_depth(cmd->device, MSG_SIMPLE_TAG, lun_qdepth - 1);
-}
-
-/**
* ufshcd_scsi_cmd_status - Update SCSI command result based on SCSI status
* @lrb: pointer to local reference block of completed command
* @scsi_status: SCSI command status
@@ -2120,12 +2112,6 @@ ufshcd_scsi_cmd_status(struct ufshcd_lrb *lrbp, int
scsi_status)
scsi_status;
break;
case SAM_STAT_TASK_SET_FULL:
- /*
-* If a LUN reports SAM_STAT_TASK_SET_FULL, then the LUN queue
-* depth needs to
[PATCH V1 0/4] LU queue depth manament
Resending patch 4, to fix the author. Some UFS devices don't support a LUN queue depth same as the device queue depth. This requires reading the unit descriptor for extracting the LU's queue depth. Dolev Raviv (1): scsi: ufs: query descriptor API scsi: ufs: device query status and size check scsi: ufs: Logical Unit (LU) command queue depth Sujit Reddy Thumma (1): scsi: ufs: Fix queue depth handling for best effort cases drivers/scsi/ufs/ufs.h| 38 +- drivers/scsi/ufs/ufshcd.c | 318 -- 2 files changed, 261 insertions(+), 95 deletions(-) -- 1.8.5.2 -- QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- 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] virtio-scsi: avoid cancelling uninitialized work items
On Wed, Jun 04, 2014 at 01:34:56PM +0200, Paolo Bonzini wrote: > Calling the workqueue interface on uninitialized work items isn't a > good idea even if they're zeroed. It's not failing catastrophically only > through happy accidents. > > Signed-off-by: Paolo Bonzini Looks good, Reviewed-by: Christoph Hellwig -- 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 4/6] scsi_error: fix invalid setting of host byte
On Wed, Jun 04, 2014 at 01:34:57PM +0200, Paolo Bonzini wrote: > From: Ulrich Obergfell > > After scsi_try_to_abort_cmd returns, the eh_abort_handler may have > already found that the command has completed in the device, causing > the host_byte to be nonzero (e.g. it could be DID_ABORT). When > this happens, ORing DID_TIME_OUT into the host byte will corrupt > the result field and initiate an unwanted command retry. > > Fix this by using set_host_byte instead, following the model of > commit 2082ebc45af9c9c648383b8cde0dc1948eadbf31. > > Cc: sta...@vger.kernel.org > Signed-off-by: Ulrich Obergfell > [Fix all instances according to review comments. - Paolo] > Signed-off-by: Paolo Bonzini Looks good, Reviewed-by: Christoph Hellwig -- 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 1/6] virtio_scsi: remove ACCESS_ONCE() and smp_read_barrier_depends()
On Wed, Jun 04, 2014 at 01:34:54PM +0200, Paolo Bonzini wrote: > From: Ming Lei > > Access to tgt->req_vq is strictly serialized by spin_lock > of tgt->tgt_lock, so the ACCESS_ONCE() isn't necessary. > > smp_read_barrier_depends() in virtscsi_req_done was introduced > to order reading req_vq and decreasing tgt->reqs, but it isn't > needed now because req_vq is read from > scsi->req_vqs[vq->index - VIRTIO_SCSI_VQ_BASE] instead of > tgt->req_vq, so remove the unnecessary barrier. > > Also remove related comment about the barrier. Looks good, Reviewed-by: Christoph Hellwig -- 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 7/7] mptfusion: tweak null pointer checks
> JL: Comments on the above warnings, in order: Can you move these comments into the actual commit message instead of the part that gets discarded? > > No-brainer, the enclosing switch statement dereferences 'reply', > so we can't get here unless 'reply' is valid. ok. > "Retry target reset" needs to know the target ID and channel, so > enclose that entire block inside a if (pScsiTmReply) block. Reading the code in mptsas_taskmgmt_complete it's pretty obvious that it can't do anything useful if mr/pScsiTmReply are NULL, so I suspect it would be best to just return at the beginning of the function. I'd love to understand if it actually could ever be zero, which I doubt. Maybe the LSI people can shed some light on that? > The code before the loop checks for 'port_info->phy_info', but not > many other places in the driver bother. Not sure what to do here. It's pretty obvious from reading mptsas_sas_io_unit_pg0 that we never register a port_info with a NULL phy_info in the lists, so all NULL checks on it could be deleted. > 'hostdata' is checked and then immediately dereferenced after > that block. How about a default return string of "N/A" to indicate > one isn't available? shost_priv can't return NULL, so the if (h) should be removed. > Not sure what to do here. 'vdevice' is needed to "set up the > message frame" target ID and channel, so it should probably return > an error in in this case. vdevice can't ever be NULL here, it's allocated in ->slave_alloc and thus guaranteed to be around when ->queuecommand is called. -- 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 6/7] mptfusion: combine fw_event_work and its event_data
> > - sz = offsetof(struct fw_event_work, event_data) + > - sizeof(MpiEventDataSasDeviceStatusChange_t); > + sz = sizeof(*fw_event) + > + sizeof(MpiEventDataSasDeviceStatusChange_t); > fw_event = kzalloc(sz, GFP_ATOMIC); Seems like there is no point in keeping the sz variable here and at the other occurances. Not that it really matters, but if we make a pass over this code we might as well fix that up, too. > - sz = offsetof(struct fw_event_work, event_data); > + sz = sizeof(*fw_event); Eww, using offsetoff for an allocation size. Good riddance that this gets sorted out. Looks good, Reviewed-by: Christoph Hellwig -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] mptfusion: make adapter prod_name[] a pointer
On Wed, Jun 04, 2014 at 12:51:55PM -0400, Joe Lawrence wrote: > The struct _MPT_ADAPTER doesn't need a full copy of the product string, > so prod_name can point to the string literal storage that the driver > already provides. > > Avoids the following sparse warning: > > drivers/message/fusion/mptbase.c:2858 MptDisplayIocCapabilities() > warn: this array is probably non-NULL. 'ioc->prod_name' Looks good, Reviewed-by: Christoph Hellwig -- 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 4/7] mptfusion: zero kernel-space source of copy_to_user
On Wed, Jun 04, 2014 at 12:58:36PM -0400, Joe Lawrence wrote:
> Hi Dan,
>
> kzalloc silenced that smatch warning, but the code looks like:
>
> (calculate data_size)
> ...
> karg = kmalloc(data_size, GFP_KERNEL);
> ...
> if (copy_from_user(karg, uarg, data_size)) {
> ...
> if (copy_to_user((char __user *)arg, karg, data_size)) {
>
> where 'data_size' once calculated, is unchanged. Since the size
> allocated is the same copied from the user and the same copied back out
> to the user, would this really be considered an info leak?
I think the stastic checker is wrong here. But the code would still
benefit from switching to memdup_user, which should shut up the checker
in addition to simplifying the code.
--
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 3/7] mptfusion: initChainBuffers should return errno
On Wed, Jun 04, 2014 at 12:49:45PM -0400, Joe Lawrence wrote: > The lone caller of initChainBuffers checks the return code for < 0, so > it is safe to appease smatch and return the proper errno value. I don't think this is useful on it's own as the whole callchain around it uses the same -1 for error convention. Returning proper errnos seems useful to me, but without converting the whole chain it would just introduce more inconsistencies. -- 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/7] mptfusion: mark file-private functions as static
Looks good, Reviewed-by: Christoph Hellwig -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] mptfusion: remove redundant kfree checks
Looks good, Reviewed-by: Christoph Hellwig -- 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] sg: add SG_FLAG_Q_AT_TAIL flag
On Wed, Jun 04, 2014 at 10:58:30AM -0400, Douglas Gilbert wrote: > When the SG_IO ioctl was copied into the block layer and > later into the bsg driver, subtle differences emerged. > > One difference is the way injected commands are queued through > the block layer (i.e. this is not SCSI device queueing nor SATA > NCQ). Summarizing: > - SG_IO in the block layer: blk_exec*(at_head=false) > - sg SG_IO: at_head=true > - bsg SG_IO: at_head=true > > Some time ago Boaz Harrosh introduced a sg v4 flag called > BSG_FLAG_Q_AT_TAIL to override the bsg driver default. > This patch does the equivalent for the sg driver. Looks good, Reviewed-by: Christoph Hellwig Any chance to get a patch for the block-layer SG_IO code, too? -- 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: ibmvscsi: protect abort handler from done-scmd in flight
Il 05/06/2014 08:16, Liu Ping Fan ha scritto: Take the following scene in guest: seqA: scsi_done() -> gapX (before taking REQ_ATOM_COMPLETE) seqB: scmd_eh_abort_handler()-> ...-> ibmvscsi_eh_abort_handler()-> ...->scsi_put_command(scmd) If seqA is scheduled at gapX, and seqB reclaims scmd. Then when seqA comes back, it tries to access the scmd when is turned back to mempool. This patch fixes the race by ensuring when ibmvscsi_eh_abort_handler() returns, no scsi_done is in flight Signed-off-by: Liu Ping Fan --- When trying to figure the scsi_cmnd in flight issue, I learned from Paolo (thanks). He showed me the way how virtscsi resolves the race between abort-handler and scsi_done in flight. And I think that this method is also needed by ibmvscsi. --- drivers/scsi/ibmvscsi/ibmvscsi.c | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c index fa76440..325cef6 100644 --- a/drivers/scsi/ibmvscsi/ibmvscsi.c +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c @@ -1828,16 +1828,19 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq, if ((crq->status != VIOSRP_OK && crq->status != VIOSRP_OK2) && evt_struct->cmnd) evt_struct->cmnd->result = DID_ERROR << 16; - if (evt_struct->done) - evt_struct->done(evt_struct); - else - dev_err(hostdata->dev, "returned done() is NULL; not running it!\n"); /* * Lock the host_lock before messing with these structures, since we * are running in a task context +* Also, this lock helps ibmvscsi_eh_abort_handler() to shield the +* scsi_done() in flight. */ spin_lock_irqsave(evt_struct->hostdata->host->host_lock, flags); + if (evt_struct->done) + evt_struct->done(evt_struct); + else + dev_err(hostdata->dev, "returned done() is NULL; not running it!\n"); + list_del(&evt_struct->list); free_event_struct(&evt_struct->hostdata->pool, evt_struct); spin_unlock_irqrestore(evt_struct->hostdata->host->host_lock, flags); I think this is not necessary because ibmvscsi places TMFs and commands in the same queue; virtio-scsi instead uses two different queues. So ibmvscsi_handle_crq processes all completed requests, which naturally serializes the processing of the TMF and the command. Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] scanning fixes
Hi all, here are two fixes for the scanning logic which resolve two long-standing issues: 1) We need to send a 'TEST UNIT READY' to the LUN before scanning. The INQUIRY command does _not_ clear any unit attentions, so if there are any outstanding unit attention conditions they'll be attached to the first command after INQUIRY. Which typically wouldn't be too bad, as most of the commands are now equipped with at least rudimentary error checking. However, the problem arises when we're sending a REPORT LUN command to that LUN. As per spec the REPORT LUN command will _always_ return something, but the list might not be fully populated if the firmware is still starting up (see SPC for details here). This will cause us to miss some devices during startup. Added to that we're not handling the unit attention 'REPORT LUN DATA HAS CHANGED', so the kernel will never be able to rescan all disks. To fix this we should be issuing a TEST UNIT READY after INQUIRY, and wait until any pending unit attention goes away. 2) Power-on/Reset handling While we're not sending out uevents for various unit attention conditions, we fail to observe the status precedence as per SAM. This might cause any 29/XX sense code to effectively overwrite any preceding unit attentions, causing us to miss those events. Hence as a minimal fix we need to report the power-on reset event via uevents, too. Hannes Reinecke (2): scsi_scan: Send TEST UNIT READY to the LUN before scanning scsi: Handle power-on reset unit attention drivers/scsi/scsi_error.c | 6 drivers/scsi/scsi_lib.c| 4 +++ drivers/scsi/scsi_scan.c | 86 +++--- include/scsi/scsi_device.h | 3 +- 4 files changed, 85 insertions(+), 14 deletions(-) -- 1.7.12.4 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] scsi: Handle power-on reset unit attention
As per SAM there is a status precedence, with any sense code 29/XX
taking second place just after an ACA ACTIVE status.
Additionally, each target might prefer to not queue any unit
attention conditions but just report one.
Due to the above this will be that one with the highest precedence.
This results in the sense code 29/XX effectively overwriting any
other unit attention.
Hence we should report the power-on reset to userland so that
it can take appropriate action.
Signed-off-by: Hannes Reinecke
---
drivers/scsi/scsi_error.c | 6 ++
drivers/scsi/scsi_lib.c| 4
include/scsi/scsi_device.h | 3 ++-
3 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 47a1ffc..65ed333 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -420,6 +420,12 @@ static void scsi_report_sense(struct scsi_device *sdev,
"threshold.\n");
}
+ if (sshdr->asc == 0x29) {
+ evt_type = SDEV_EVT_POWER_ON_RESET_OCCURRED;
+ sdev_printk(KERN_WARNING, sdev,
+ "Power-on or device reset occurred\n");
+ }
+
if (sshdr->asc == 0x2a && sshdr->ascq == 0x01) {
evt_type = SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED;
sdev_printk(KERN_WARNING, sdev,
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9f841df..ee158c1 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2183,6 +2183,9 @@ static void scsi_evt_emit(struct scsi_device *sdev,
struct scsi_event *evt)
case SDEV_EVT_LUN_CHANGE_REPORTED:
envp[idx++] = "SDEV_UA=REPORTED_LUNS_DATA_HAS_CHANGED";
break;
+ case SDEV_EVT_POWER_ON_RESET_OCCURRED:
+ envp[idx++] = "SDEV_UA=POWER_ON_RESET_OCCURRED";
+ break;
default:
/* do nothing */
break;
@@ -2286,6 +2289,7 @@ struct scsi_event *sdev_evt_alloc(enum scsi_device_event
evt_type,
case SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED:
case SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED:
case SDEV_EVT_LUN_CHANGE_REPORTED:
+ case SDEV_EVT_POWER_ON_RESET_OCCURRED:
default:
/* do nothing */
break;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 5853c91..7b9a886 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -57,9 +57,10 @@ enum scsi_device_event {
SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED, /* 38 07 UA reported */
SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED,/* 2A 01 UA reported */
SDEV_EVT_LUN_CHANGE_REPORTED, /* 3F 0E UA reported */
+ SDEV_EVT_POWER_ON_RESET_OCCURRED, /* 29 00 UA reported */
SDEV_EVT_FIRST = SDEV_EVT_MEDIA_CHANGE,
- SDEV_EVT_LAST = SDEV_EVT_LUN_CHANGE_REPORTED,
+ SDEV_EVT_LAST = SDEV_EVT_POWER_ON_RESET_OCCURRED,
SDEV_EVT_MAXBITS= SDEV_EVT_LAST + 1
};
--
1.7.12.4
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] scsi_scan: Send TEST UNIT READY to the LUN before scanning
REPORT_LUN_SCAN does not report any outstanding unit attention
condition as per SAM. However, the target might not be fully
initialized at that time, so we might end up getting a
default entry (or even a partially filled one).
But as we're not able to process the REPORT LUN DATA HAS CHANGED
unit attention correctly we'll be missing out some LUNs during
startup.
So it's better to send a TEST UNIT READY for modern implementations
and wait until the unit attention condition goes away.
Signed-off-by: Hannes Reinecke
---
drivers/scsi/scsi_scan.c | 86
1 file changed, 73 insertions(+), 13 deletions(-)
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index e02b3aa..a8e59c3 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -123,6 +123,13 @@ MODULE_PARM_DESC(inq_timeout,
"Timeout (in seconds) waiting for devices to answer INQUIRY."
" Default is 20. Some devices may need more; most need less.");
+static unsigned int scsi_scan_timeout = SCSI_TIMEOUT/HZ + 58;
+
+module_param_named(scan_timeout, scsi_scan_timeout, uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(scan_timeout,
+"Timeout (in seconds) waiting for devices to become ready"
+" after INQUIRY. Default is 60.");
+
/* This lock protects only this list */
static DEFINE_SPINLOCK(async_scan_lock);
static LIST_HEAD(scanning_hosts);
@@ -712,19 +719,6 @@ static int scsi_probe_lun(struct scsi_device *sdev,
unsigned char *inq_result,
}
/*
-* Related to the above issue:
-*
-* XXX Devices (disk or all?) should be sent a TEST UNIT READY,
-* and if not ready, sent a START_STOP to start (maybe spin up) and
-* then send the INQUIRY again, since the INQUIRY can change after
-* a device is initialized.
-*
-* Ideally, start a device if explicitly asked to do so. This
-* assumes that a device is spun up on power on, spun down on
-* request, and then spun up on request.
-*/
-
- /*
* The scanning code needs to know the scsi_level, even if no
* device is attached at LUN 0 (SCSI_SCAN_TARGET_PRESENT) so
* non-zero LUNs can be scanned.
@@ -739,6 +733,65 @@ static int scsi_probe_lun(struct scsi_device *sdev,
unsigned char *inq_result,
}
/**
+ * scsi_test_lun - waiting for a LUN to become ready
+ * @sdev: scsi_device to test
+ *
+ * Description:
+ * Wait for the lun associated with @sdev to become ready
+ *
+ * Send a TEST UNIT READY to detect any unit attention conditions.
+ * Retry TEST UNIT READY for up to @scsi_scan_timeout if the
+ * returned sense key is 02/04/01 (Not ready, Logical Unit is
+ * in process of becoming ready)
+ **/
+static int
+scsi_test_lun(struct scsi_device *sdev)
+{
+ struct scsi_sense_hdr sshdr;
+ int res = SCSI_SCAN_TARGET_PRESENT;
+ int tur_result;
+ unsigned long tur_timeout = jiffies + scsi_scan_timeout * HZ;
+
+ /* Skip for older devices */
+ if (sdev->scsi_level <= SCSI_3)
+ return SCSI_SCAN_LUN_PRESENT;
+
+ /*
+* Wait for the device to become ready.
+*
+* Some targets take some time before the firmware is
+* fully initialized, during which time they might not
+* be able to fill out any REPORT_LUN command correctly.
+* And as we're not capable of handling the
+* INQUIRY DATA CHANGED unit attention correctly we'd
+* rather wait here.
+*/
+ do {
+ tur_result = scsi_test_unit_ready(sdev, SCSI_TIMEOUT,
+ 3, &sshdr);
+ if (!tur_result) {
+ res = SCSI_SCAN_LUN_PRESENT;
+ break;
+ }
+ if ((driver_byte(tur_result) & DRIVER_SENSE) &&
+ scsi_sense_valid(&sshdr)) {
+ SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
+ "scsi_scan: tur returned %02x/%02x/%02x\n",
+ sshdr.sense_key, sshdr.asc, sshdr.ascq));
+ if (sshdr.sense_key == NOT_READY &&
+ sshdr.asc == 0x04 && sshdr.ascq == 0x01) {
+ /* Logical Unit is in process
+* of becoming ready */
+ msleep(100);
+ continue;
+ }
+ }
+ res = SCSI_SCAN_LUN_PRESENT;
+ } while (time_before_eq(jiffies, tur_timeout));
+ return res;
+}
+
+/**
* scsi_add_lun - allocate and fully initialze a scsi_device
* @sdev: holds information to be stored in the new scsi_device
* @inq_result:holds the result of a previous INQUIRY to the LUN
@@ -1142,6 +1195,13 @@ static int scsi_probe_and_add_lun(s
Re: [PATCH] scsi: bfa: bfad_attr.c: Optimization of the Code
On 06/05/14 08:55, Bart Van Assche wrote: > On 06/04/14 20:08, Rickard Strandqvist wrote: > This is ugly. Please use sprintf(buf, "%.*s\n", PAGE_SIZE - 1, str) > instead of strncpy() + strlen(). (replying to my own e-mail) The above should of course have read "sprintf(buf, "%.*s\n", PAGE_SIZE - 2, str)" to avoid that the terminating '\0' triggers a buffer overflow. 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
