[PATCH v5 5/7] scsi_io_completion_reprep helper added
Since the action "reprep" is called from two places, rather than repeat
the code, make a new scsi_io_completion helper with "reprep" as its
suffix.
Signed-off-by: Douglas Gilbert
Reviewed-by: Johannes Thumshirn
---
drivers/scsi/scsi_lib.c | 41 ++---
1 file changed, 18 insertions(+), 23 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 831d1fad38b3..d24bd29cd94a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -751,6 +751,20 @@ static blk_status_t __scsi_error_from_host_byte(struct
scsi_cmnd *cmd,
}
}
+/* Helper for scsi_io_completion() when "reprep" action required. */
+static void scsi_io_completion_reprep(struct scsi_cmnd *cmd,
+ struct request_queue *q)
+{
+ /* A new command will be prepared and issued. */
+ if (q->mq_ops) {
+ scsi_mq_requeue_cmd(cmd);
+ } else {
+ /* Unprep request and put it back at head of the queue. */
+ scsi_release_buffers(cmd);
+ scsi_requeue_command(q, cmd);
+ }
+}
+
/* Helper for scsi_io_completion() when special action required. */
static void scsi_io_completion_action(struct scsi_cmnd *cmd, int result)
{
@@ -892,15 +906,7 @@ static void scsi_io_completion_action(struct scsi_cmnd
*cmd, int result)
return;
/*FALLTHRU*/
case ACTION_REPREP:
- /* Unprep the request and put it back at the head of the queue.
-* A new command will be prepared and issued.
-*/
- if (q->mq_ops) {
- scsi_mq_requeue_cmd(cmd);
- } else {
- scsi_release_buffers(cmd);
- scsi_requeue_command(q, cmd);
- }
+ scsi_io_completion_reprep(cmd, q);
break;
case ACTION_RETRY:
/* Retry the same command immediately */
@@ -1077,20 +1083,9 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
* If there had been no error, but we have leftover bytes in the
* requeues just queue the command up again.
*/
- if (result == 0) {
- /*
-* Unprep the request and put it back at the head of the
-* queue. A new command will be prepared and issued.
-* This block is the same as case ACTION_REPREP in
-* scsi_io_completion_action() above.
-*/
- if (q->mq_ops) {
- scsi_mq_requeue_cmd(cmd);
- } else {
- scsi_release_buffers(cmd);
- scsi_requeue_command(q, cmd);
- }
- } else
+ if (result == 0)
+ scsi_io_completion_reprep(cmd, q);
+ else
scsi_io_completion_action(cmd, result);
}
--
2.14.1
[PATCH v5 7/7] scsi_io_completion convert BUGs to WARNs
The scsi_io_completion function contains three BUG() and BUG_ON() calls.
Replace them with WARN variants.
Signed-off-by: Douglas Gilbert
Reviewed-by: Johannes Thumshirn
---
drivers/scsi/scsi_lib.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 60b57324a244..779c45479ac3 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1046,13 +1046,21 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
scsi_req(req->next_rq)->resid_len = scsi_in(cmd)->resid;
if (scsi_end_request(req, BLK_STS_OK, blk_rq_bytes(req),
blk_rq_bytes(req->next_rq)))
- BUG();
+ WARN_ONCE(true,
+ "Bidi command with remaining bytes");
return;
}
}
/* no bidi support yet, other than in pass-through */
- BUG_ON(blk_bidi_rq(req));
+ if (unlikely(blk_bidi_rq(req))) {
+ WARN_ONCE(true, "Only support bidi command in passthrough");
+ scmd_printk(KERN_ERR, cmd, "Killing bidi command\n");
+ if (scsi_end_request(req, BLK_STS_IOERR, blk_rq_bytes(req),
+blk_rq_bytes(req->next_rq)))
+ WARN_ONCE(true, "Bidi command with remaining bytes");
+ return;
+ }
/*
* Next deal with any sectors which we were able to correctly
@@ -1075,7 +1083,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
/* Kill remainder if no retries. */
if (unlikely(blk_stat && scsi_noretry_cmd(cmd))) {
if (scsi_end_request(req, blk_stat, blk_rq_bytes(req), 0))
- BUG();
+ WARN_ONCE(true,
+ "Bytes remaining after failed, no-retry command");
return;
}
--
2.14.1
[PATCH v5 6/7] scsi_io_completion hints on fastpatch
Add likely() and unlikely() hints to conditionals on or near the fastpath.
Signed-off-by: Douglas Gilbert
Reviewed-by: Johannes Thumshirn
---
A reviewer wanted any performance improvement (or otherwise) quantified.
The improvement was so small, that ftrace ignored it. Inline timing code
suggests the improvement from this whole patchset is around 7 nanoseconds
per invocation (tested on a Lenovo X270 (i5-7200U)). Not exactly huge.
Another win might be the smaller size of scsi_io_completion() after the
refactoring; this should allow more other code to fit in the instruction
cache.
drivers/scsi/scsi_lib.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index d24bd29cd94a..60b57324a244 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1028,17 +1028,17 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
struct request *req = cmd->request;
blk_status_t blk_stat = BLK_STS_OK;
- if (result) /* does not necessarily mean there is an error */
+ if (unlikely(result)) /* a nz result may or may not be an error */
result = scsi_io_completion_nz_result(cmd, result, &blk_stat);
- if (blk_rq_is_passthrough(req)) {
+ if (unlikely(blk_rq_is_passthrough(req))) {
/*
* __scsi_error_from_host_byte may have reset the host_byte
*/
scsi_req(req)->result = cmd->result;
scsi_req(req)->resid_len = scsi_get_resid(cmd);
- if (scsi_bidi_cmnd(cmd)) {
+ if (unlikely(scsi_bidi_cmnd(cmd))) {
/*
* Bidi commands Must be complete as a whole,
* both sides at once.
@@ -1051,7 +1051,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
}
}
- /* no bidi support for !blk_rq_is_passthrough yet */
+ /* no bidi support yet, other than in pass-through */
BUG_ON(blk_bidi_rq(req));
/*
@@ -1067,13 +1067,13 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
* handle. Failed, zero length commands always need to drop down
* to retry code. Fast path should return in this block.
*/
- if (blk_rq_bytes(req) > 0 || blk_stat == BLK_STS_OK) {
- if (!scsi_end_request(req, blk_stat, good_bytes, 0))
+ if (likely(blk_rq_bytes(req) > 0 || blk_stat == BLK_STS_OK)) {
+ if (likely(!scsi_end_request(req, blk_stat, good_bytes, 0)))
return; /* no bytes remaining */
}
-/* Kill remainder if no retries. */
- if (blk_stat && scsi_noretry_cmd(cmd)) {
+ /* Kill remainder if no retries. */
+ if (unlikely(blk_stat && scsi_noretry_cmd(cmd))) {
if (scsi_end_request(req, blk_stat, blk_rq_bytes(req), 0))
BUG();
return;
@@ -1083,7 +1083,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
* If there had been no error, but we have leftover bytes in the
* requeues just queue the command up again.
*/
- if (result == 0)
+ if (likely(result == 0))
scsi_io_completion_reprep(cmd, q);
else
scsi_io_completion_action(cmd, result);
--
2.14.1
[PATCH v5 0/7] scsi_io_completion cleanup
While working of this "[PATCH v4] Make SCSI Status CONDITION MET equivalent to GOOD" the author had trouble finding how to add another corner case check in the scsi_io_completion() function (scsi_lib.c). That function is executed at the completion of every SCSI command but only 20 or so of its hundred of lines of code are executed for the vast majority of invocations (i.e. the fastpath). This patch refactors scsi_io_completion() by taking the bulk of its error processing code into three static helper functions, leaving only the 20 or so code lines that constitute the fastpath. In this process comments were added and tweaked, plus variables renamed. The last two patches in this set are optional: adding conditional hints along the fastpath and converting 3 BUG() calls in scsi_io_completion() to WARNs as requested by a reviewer. There is no attempt in this patchset to change the logic of the original scsi_io_completion() although the last patch attempts to continue from awkward situations rather than crashing the calling thread (and possibly the kernel). Some conditional checks are saved by reducing the number of redundant tests (e.g. multiple checks that the 'result' variable is non-zero). Also De Morgan's laws are applied to some complex conditions to simplify them from the reader's perspective. The fact remains, this is a very complex function. This patch is against Martin Petersen's 4.17/scsi-queue branch. Note: to apply this patchset to the lk 4.15 production kernels (tested on lk 4.15.13), these two lines near the end of scsi_io_completion(): __scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY, 0); and __scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY, 0); need that trailing 0 changed to 'false', prior to applying 4/7 . ChangeLog since v4 - introduce bool do_print to simplify conditionals in the case of a recovered error sense key in 2/7 rather than 3/7 ChangeLog since v3 - make use of bools sense_valid and sense_current consistent in 3/7 and 4/7 Douglas Gilbert (7): scsi_io_completion comment on end_request return scsi_io_completion rename variables scsi_io_completion_nz_result function added scsi_io_completion_action helper added scsi_io_completion_reprep helper added scsi_io_completion hints on fastpatch scsi_io_completion convert BUGs to WARNs drivers/scsi/scsi_lib.c | 378 +++- 1 file changed, 214 insertions(+), 164 deletions(-) -- 2.14.1
[PATCH v5 4/7] scsi_io_completion_action helper added
Place scsi_io_completion()'s complex error processing associated with a
local enumeration into a static helper function. That enumeration's
values start with "ACTION_" so use the suffix "_action" in the helper
function's name.
Signed-off-by: Douglas Gilbert
Reviewed-by: Johannes Thumshirn
drivers/scsi/scsi_lib.c | 321 ++--
1 file changed, 171 insertions(+), 150 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 08c4db34af7b..831d1fad38b3 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -751,6 +751,168 @@ static blk_status_t __scsi_error_from_host_byte(struct
scsi_cmnd *cmd,
}
}
+/* Helper for scsi_io_completion() when special action required. */
+static void scsi_io_completion_action(struct scsi_cmnd *cmd, int result)
+{
+ struct request_queue *q = cmd->device->request_queue;
+ struct request *req = cmd->request;
+ int level = 0;
+ enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
+ ACTION_DELAYED_RETRY} action;
+ unsigned long wait_for = (cmd->allowed + 1) * req->timeout;
+ struct scsi_sense_hdr sshdr;
+ bool sense_valid;
+ bool sense_current = true; /* false implies "deferred sense" */
+ blk_status_t blk_stat;
+
+ sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
+ if (sense_valid)
+ sense_current = !scsi_sense_is_deferred(&sshdr);
+
+ blk_stat = __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_current) {
+ switch (sshdr.sense_key) {
+ case UNIT_ATTENTION:
+ if (cmd->device->removable) {
+ /* Detected disc change. Set a bit
+* and quietly refuse further access.
+*/
+ cmd->device->changed = 1;
+ action = ACTION_FAIL;
+ } else {
+ /* Must have been a power glitch, or a
+* bus reset. Could not have been a
+* media change, so we just retry the
+* command and see what happens.
+*/
+ action = ACTION_RETRY;
+ }
+ break;
+ case ILLEGAL_REQUEST:
+ /* If we had an ILLEGAL REQUEST returned, then
+* we may have performed an unsupported
+* command. The only thing this should be
+* would be a ten byte read where only a six
+* byte read was supported. Also, on a system
+* where READ CAPACITY failed, we may have
+* read past the end of the disk.
+*/
+ if ((cmd->device->use_10_for_rw &&
+ sshdr.asc == 0x20 && sshdr.ascq == 0x00) &&
+ (cmd->cmnd[0] == READ_10 ||
+cmd->cmnd[0] == WRITE_10)) {
+ /* This will issue a new 6-byte command. */
+ cmd->device->use_10_for_rw = 0;
+ action = ACTION_REPREP;
+ } else if (sshdr.asc == 0x10) /* DIX */ {
+ action = ACTION_FAIL;
+ blk_stat = BLK_STS_PROTECTION;
+ /* INVALID COMMAND OPCODE or INVALID FIELD IN CDB */
+ } else if (sshdr.asc == 0x20 || sshdr.asc == 0x24) {
+ action = ACTION_FAIL;
+ blk_stat = BLK_STS_TARGET;
+ } else
+ action = ACTION_FAIL;
+ break;
+ case ABORTED_COMMAND:
+ action = ACTION_FAIL;
+ if (sshdr.asc == 0x10) /* DIF */
+ blk_stat = BLK_STS_PROTECTION;
+ break;
+ case NOT_READY:
+ /* If the device is in the process of becoming
+* ready, or has a temporary blockage, retry.
+*/
+ if (sshdr.asc == 0x04) {
+ switch (sshdr.ascq) {
+ case 0x01: /* becoming ready */
+ case 0x04: /* format in progress */
+ case 0x05: /* rebuild in p
[PATCH v5 2/7] scsi_io_completion rename variables
Change and add some variable names, adjust some associated comments
for clarity. Correct some misleading comments.
Signed-off-by: Douglas Gilbert
---
drivers/scsi/scsi_lib.c | 72 ++---
1 file changed, 39 insertions(+), 33 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index d44ee84df091..d4f2c3fac907 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -777,18 +777,19 @@ static blk_status_t __scsi_error_from_host_byte(struct
scsi_cmnd *cmd,
*be put back on the queue and retried using the same
*command as before, possibly after a delay.
*
- * c) We can call scsi_end_request() with -EIO to fail
- *the remainder of the request.
+ * c) We can call scsi_end_request() with blk_stat other than
+ *BLK_STS_OK, to fail the remainder of the request.
*/
void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
{
int result = cmd->result;
struct request_queue *q = cmd->device->request_queue;
struct request *req = cmd->request;
- blk_status_t error = BLK_STS_OK;
+ blk_status_t blk_stat = BLK_STS_OK;
struct scsi_sense_hdr sshdr;
bool sense_valid = false;
- int sense_deferred = 0, level = 0;
+ bool sense_current = true; /* false implies "deferred sense" */
+ int level = 0;
enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
ACTION_DELAYED_RETRY} action;
unsigned long wait_for = (cmd->allowed + 1) * req->timeout;
@@ -796,7 +797,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int
good_bytes)
if (result) {
sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
if (sense_valid)
- sense_deferred = scsi_sense_is_deferred(&sshdr);
+ sense_current = !scsi_sense_is_deferred(&sshdr);
}
if (blk_rq_is_passthrough(req)) {
@@ -809,8 +810,9 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int
good_bytes)
min(8 + cmd->sense_buffer[7],
SCSI_SENSE_BUFFERSIZE);
}
- if (!sense_deferred)
- error = __scsi_error_from_host_byte(cmd,
result);
+ if (sense_current)
+ blk_stat = __scsi_error_from_host_byte(cmd,
+ result);
}
/*
* __scsi_error_from_host_byte may have reset the host_byte
@@ -829,13 +831,13 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
BUG();
return;
}
- } else if (blk_rq_bytes(req) == 0 && result && !sense_deferred) {
+ } else if (blk_rq_bytes(req) == 0 && result && sense_current) {
/*
* Flush commands do not transfers any data, and thus cannot use
* good_bytes != blk_rq_bytes(req) as the signal for an error.
-* This sets the error explicitly for the problem case.
+* This sets blk_stat explicitly for the problem case.
*/
- error = __scsi_error_from_host_byte(cmd, result);
+ blk_stat = __scsi_error_from_host_byte(cmd, result);
}
/* no bidi support for !blk_rq_is_passthrough yet */
@@ -856,17 +858,22 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
* is what gets returned to the user
*/
if (sense_valid && (sshdr.sense_key == RECOVERED_ERROR)) {
- /* if ATA PASS-THROUGH INFORMATION AVAILABLE skip
+ bool do_print = true;
+
+ /*
+* 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->rq_flags & RQF_QUIET))
+ do_print = false;
+ else if (req->rq_flags & RQF_QUIET)
+ do_print = false;
+ if (do_print)
scsi_print_sense(cmd);
result = 0;
- /* for passthrough error may be set */
- error = BLK_STS_OK;
+ /* for passthrough, blk_stat may be set */
+ blk_stat = BLK_STS_OK;
}
/*
* Another corner case: the SCSI status byte is non-zero but 'good'.
@@ -877,23 +884,22 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
*/
if (status_byte(r
[PATCH v5 3/7] scsi_io_completion_nz_result function added
Break out several intertwined paths when cmd->result is non zero and
place them in the scsi_io_completion_nz_result helper function. The
logic is not changed.
Signed-off-by: Douglas Gilbert
---
A reviewer requested the original helper function's two return values
be reduced to one: the blk_stat variable. This required a hack to
differentiate the default setting of blk_stat (BLK_STS_OK) from the case
when the helper assigns BLK_STS_OK as the return value. The hack was to
return the otherwise unused BLK_STS_NOTSUPP value as an indication that
the helper didn't change anything. That hack was judged by another
reviewer to be worse that the "two return values" ugliness it was
trying to address. So back to the original "two return values" solution.
drivers/scsi/scsi_lib.c | 132 +++-
1 file changed, 75 insertions(+), 57 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index d4f2c3fac907..08c4db34af7b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -751,6 +751,79 @@ static blk_status_t __scsi_error_from_host_byte(struct
scsi_cmnd *cmd,
}
}
+/*
+ * Helper for scsi_io_completion() when cmd->result is non-zero. Returns a
+ * new result that may suppress further error checking. Also modifies
+ * *blk_statp in some cases.
+ */
+static int scsi_io_completion_nz_result(struct scsi_cmnd *cmd, int result,
+ blk_status_t *blk_statp)
+{
+ bool sense_valid;
+ bool sense_current = true; /* false implies "deferred sense" */
+ struct request *req = cmd->request;
+ struct scsi_sense_hdr sshdr;
+
+ sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
+ if (sense_valid)
+ sense_current = !scsi_sense_is_deferred(&sshdr);
+
+ if (blk_rq_is_passthrough(req)) {
+ if (sense_valid) {
+ /*
+* SG_IO wants current and deferred errors
+*/
+ scsi_req(req)->sense_len =
+ min(8 + cmd->sense_buffer[7],
+ SCSI_SENSE_BUFFERSIZE);
+ }
+ if (sense_current)
+ *blk_statp = __scsi_error_from_host_byte(cmd, result);
+ } else if (blk_rq_bytes(req) == 0 && sense_current) {
+ /*
+* Flush commands do not transfers any data, and thus cannot use
+* good_bytes != blk_rq_bytes(req) as the signal for an error.
+* This sets *blk_statp explicitly for the problem case.
+*/
+ *blk_statp = __scsi_error_from_host_byte(cmd, result);
+ }
+ /*
+* Recovered errors need reporting, but they're always treated as
+* success, so fiddle the result code here. For passthrough requests
+* we already took a copy of the original into sreq->result which
+* is what gets returned to the user
+*/
+ if (sense_valid && (sshdr.sense_key == RECOVERED_ERROR)) {
+ bool do_print = true;
+ /*
+* if ATA PASS-THROUGH INFORMATION AVAILABLE [0x0, 0x1d]
+* 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))
+ do_print = false;
+ else if (req->rq_flags & RQF_QUIET)
+ do_print = false;
+ if (do_print)
+ scsi_print_sense(cmd);
+ result = 0;
+ /* for passthrough, *blk_statp may be set */
+ *blk_statp = BLK_STS_OK;
+ }
+ /*
+* Another corner case: the SCSI status byte is non-zero but 'good'.
+* Example: PRE-FETCH command returns SAM_STAT_CONDITION_MET when
+* it is able to fit nominated LBs in its cache (and SAM_STAT_GOOD
+* if it can't fit). Treat SAM_STAT_CONDITION_MET and the related
+* intermediate statuses (both obsolete in SAM-4) as good.
+*/
+ if (status_byte(result) && scsi_status_is_good(result)) {
+ result = 0;
+ *blk_statp = BLK_STS_OK;
+ }
+ return result;
+}
+
/*
* Function:scsi_io_completion()
*
@@ -794,26 +867,14 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
ACTION_DELAYED_RETRY} action;
unsigned long wait_for = (cmd->allowed + 1) * req->timeout;
- if (result) {
+ if (result) { /* does not necessarily mean there is an error */
sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
if (sense_valid)
sense_current = !scsi_sense_is_deferred(&sshdr);
+ result = scsi_io_completion_nz_result(cmd, result, &blk_stat);
[PATCH v5 1/7] scsi_io_completion comment on end_request return
scsi_end_request() is called multiple times from scsi_io_completion()
which branches on its bool returned value. Add comment before the static
definition of scsi_end_request() about the meaning of that return.
Signed-off-by: Douglas Gilbert
Reviewed-by: Johannes Thumshirn
Reviewed-by: Bart Van Assche
---
The reader might think that if scsi_end_request() had actually ended the
request (i.e. no bytes remaining) it would return true. If so, the reader
would be misled.
drivers/scsi/scsi_lib.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 393f9db8f41b..d44ee84df091 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -662,6 +662,7 @@ static void scsi_release_bidi_buffers(struct scsi_cmnd *cmd)
cmd->request->next_rq->special = NULL;
}
+/* Returns false when no more bytes to process, true if there are more */
static bool scsi_end_request(struct request *req, blk_status_t error,
unsigned int bytes, unsigned int bidi_bytes)
{
--
2.14.1
Re: General protection fault with use_blk_mq=1.
> Il giorno 29 mar 2018, alle ore 05:22, Jens Axboe ha > scritto: > > On 3/28/18 9:13 PM, Zephaniah E. Loss-Cutler-Hull wrote: >> On 03/28/2018 06:02 PM, Jens Axboe wrote: >>> On 3/28/18 5:03 PM, Zephaniah E. Loss-Cutler-Hull wrote: I am not subscribed to any of the lists on the To list here, please CC me on any replies. I am encountering a fairly consistent crash anywhere from 15 minutes to 12 hours after boot with scsi_mod.use_blk_mq=1 dm_mod.use_blk_mq=1> The crash looks like: >> Looking through the code, I'd guess that this is dying inside blkg_rwstat_add, which calls percpu_counter_add_batch, which is what RIP is pointing at. >>> >>> Leaving the whole thing here for Paolo - it's crashing off insertion of >>> a request coming out of SG_IO. Don't think we've seen this BFQ failure >>> case before. >>> >>> You can mitigate this by switching the scsi-mq devices to mq-deadline >>> instead. >>> >> >> I'm thinking that I should also be able to mitigate it by disabling >> CONFIG_DEBUG_BLK_CGROUP. >> >> That should remove that entire chunk of code. >> >> Of course, that won't help if this is actually a symptom of a bigger >> problem. > > Yes, it's not a given that it will fully mask the issue at hand. But > turning off BFQ has a much higher chance of working for you. > > This time actually CC'ing Paolo. > Hi Zephaniah, if you are actually interested in the benefits of BFQ (low latency, high responsiveness, fairness, ...) then it may be worth to try what you yourself suggest: disabling CONFIG_DEBUG_BLK_CGROUP. Also because this option activates the heavy computation of debug cgroup statistics, which probably you don't use. In addition, the outcome of your attempt without CONFIG_DEBUG_BLK_CGROUP would give us useful bisection information: - if no failure occurs, then the issue is likely to be confined in that debugging code (which, on the bright side, is likely to be of occasional interest, for only a handful of developers) - if the issue still shows up, then we may have new hints on this odd failure Finally, consider that this issue has been reported to disappear from 4.16 [1], and, as a plus, that the service quality of BFQ had a further boost exactly from 4.16. Looking forward to your feedback, in case you try BFQ without CONFIG_DEBUG_BLK_CGROUP, Paolo [1] https://www.spinics.net/lists/linux-block/msg21422.html > > -- > Jens Axboe
Re: SCSI generic (sg) regression in stable kernels
On Wed, Mar 28, 2018 at 05:37:24PM -0400, Tony Battersby wrote:
> The SCSI generic driver is currently broken in the 3.18, 4.1, 4.4, and
> 4.9 stable kernels. This is the problematic commit:
>
> 109bade9c625 ("scsi: sg: use standard lists for sg_requests")
>
> (all commit ids are from upstream git, not stable.git)
>
> There have already been a number of fixes backported to -stable to fix
> problems introduced by that commit:
>
> bd46fc406b30 ("scsi: sg: off by one in sg_ioctl()")
> 4759df905a47 ("scsi: sg: factor out sg_fill_request_table()")
> 3e0097499839 ("scsi: sg: fixup infoleak when using SG_GET_REQUEST_TABLE")
> 587c3c9f286c ("scsi: sg: Re-fix off by one in sg_fill_request_table()")
>
> I am not sure why 109bade9c625 was added to -stable to begin with. My
> guess is that the "infoleak" fix was flagged as being appropriate for
> -stable, and it depended on the original commit 109bade9c625. But if
> you take a close look, the original commit 109bade9c625 is what
> introduced the infoleak in the first place. Or maybe there was some
> other reason. But I digress.
>
> There is one more fix for 109bade9c625 that has not yet been backported:
>
> 48ae8484e9fc ("scsi: sg: don't return bogus Sg_requests")
>
> Without the fix above, sg oopses as soon as I try to use it. So please
> add 48ae8484e9fc to -stable.
>
> The following -stable kernels already have 48ae8484e9fc:
> 4.14.31
> 4.15.14
>
> The following -stable kernels need 48ae8484e9fc:
> 3.18.102
> 4.1.51
> 4.4.125
> 4.9.91
Now queued up for 4.9.y, 4.4.y, and 3.18.y, thanks!
greg k-h
Re: General protection fault with use_blk_mq=1.
> Il giorno 29 mar 2018, alle ore 03:02, Jens Axboe ha > scritto: > > On 3/28/18 5:03 PM, Zephaniah E. Loss-Cutler-Hull wrote: >> I am not subscribed to any of the lists on the To list here, please CC >> me on any replies. >> >> I am encountering a fairly consistent crash anywhere from 15 minutes to >> 12 hours after boot with scsi_mod.use_blk_mq=1 dm_mod.use_blk_mq=1> >> The crash looks like: >> >> [ 5466.075993] general protection fault: [#1] PREEMPT SMP PTI >> [ 5466.075997] Modules linked in: esp4 xfrm4_mode_tunnel fuse usblp >> uvcvideo pci_stub vboxpci(O) vboxnetadp(O) vboxnetflt(O) vboxdrv(O) >> ip6table_filter ip6_tables xt_tcpudp nf_conntrack_ipv4 nf_defrag_ipv4 >> xt_conntrack nf_conntrack iptable_filter ip_tables x_tables intel_rapl >> joydev serio_raw wmi_bmof iwldvm iwlwifi shpchp kvm_intel kvm irqbypass >> autofs4 algif_skcipher nls_iso8859_1 nls_cp437 crc32_pclmul >> ghash_clmulni_intel >> [ 5466.076022] CPU: 3 PID: 10573 Comm: pool Tainted: G O >> 4.15.13-f1-dirty #148 >> [ 5466.076024] Hardware name: Hewlett-Packard HP EliteBook Folio >> 9470m/18DF, BIOS 68IBD Ver. F.44 05/22/2013 >> [ 5466.076029] RIP: 0010:percpu_counter_add_batch+0x2b/0xb0 >> [ 5466.076031] RSP: 0018:a556c47afb58 EFLAGS: 00010002 >> [ 5466.076033] RAX: 95cda87ce018 RBX: 95cda87cdb68 RCX: >> >> [ 5466.076034] RDX: 3fff RSI: 896495c4 RDI: >> 895b2bed >> [ 5466.076036] RBP: 3fff R08: R09: >> 95cb7d5f8148 >> [ 5466.076037] R10: 0200 R11: R12: >> 0001 >> [ 5466.076038] R13: 95cda87ce088 R14: 95cda6ebd100 R15: >> a556c47afc58 >> [ 5466.076040] FS: 7f25f5305700() GS:95cdbeac() >> knlGS: >> [ 5466.076042] CS: 0010 DS: ES: CR0: 80050033 >> [ 5466.076043] CR2: 7f25e807e0a8 CR3: 0003ed5a6001 CR4: >> 001606e0 >> [ 5466.076044] Call Trace: >> [ 5466.076050] bfqg_stats_update_io_add+0x58/0x100 >> [ 5466.076055] bfq_insert_requests+0xec/0xd80 >> [ 5466.076059] ? blk_rq_append_bio+0x8f/0xa0 >> [ 5466.076061] ? blk_rq_map_user_iov+0xc3/0x1d0 >> [ 5466.076065] blk_mq_sched_insert_request+0xa3/0x130 >> [ 5466.076068] blk_execute_rq+0x3a/0x50 >> [ 5466.076070] sg_io+0x197/0x3e0 >> [ 5466.076073] ? dput+0xca/0x210 >> [ 5466.076077] ? mntput_no_expire+0x11/0x1a0 >> [ 5466.076079] scsi_cmd_ioctl+0x289/0x400 >> [ 5466.076082] ? filename_lookup+0xe1/0x170 >> [ 5466.076085] sd_ioctl+0xc7/0x1a0 >> [ 5466.076088] blkdev_ioctl+0x4d4/0x8c0 >> [ 5466.076091] block_ioctl+0x39/0x40 >> [ 5466.076094] do_vfs_ioctl+0x92/0x5e0 >> [ 5466.076097] ? __fget+0x73/0xc0 >> [ 5466.076099] SyS_ioctl+0x74/0x80 >> [ 5466.076102] do_syscall_64+0x60/0x110 >> [ 5466.076106] entry_SYSCALL_64_after_hwframe+0x3d/0xa2 >> [ 5466.076109] RIP: 0033:0x7f25f75fef47 >> [ 5466.076110] RSP: 002b:7f25f53049a8 EFLAGS: 0246 ORIG_RAX: >> 0010 >> [ 5466.076112] RAX: ffda RBX: 000c RCX: >> 7f25f75fef47 >> [ 5466.076114] RDX: 7f25f53049b0 RSI: 2285 RDI: >> 000c >> [ 5466.076115] RBP: 0010 R08: 7f25e8007818 R09: >> 0200 >> [ 5466.076116] R10: 0001 R11: 0246 R12: >> >> [ 5466.076118] R13: R14: 7f25f8a6b5e0 R15: >> 7f25e80173e0 >> [ 5466.076120] Code: 41 55 49 89 fd bf 01 00 00 00 41 54 49 89 f4 55 89 >> d5 53 e8 18 e1 bb ff 48 c7 c7 c4 95 64 89 e8 dc e9 fb ff 49 8b 45 20 48 >> 63 d5 <65> 8b 18 48 63 db 4c 01 e3 48 39 d3 7d 0a f7 dd 48 63 ed 48 39 >> [ 5466.076147] RIP: percpu_counter_add_batch+0x2b/0xb0 RSP: a556c47afb58 >> [ 5466.076149] ---[ end trace 8d7eb80aafef4494 ]--- >> [ 5466.670153] note: pool[10573] exited with preempt_count 2 >> >> (I only have the one instance right this minute as a result of not >> having remote syslog setup before now.) >> >> This is clearly deep in the blk_mq code, and it goes away when I remove >> the use_blk_mq kernel command line parameters. >> >> My next obvious step is to try and disable the load of the vbox modules. >> >> I can include the full dmesg output if it would be helpful. >> >> The system is an older HP Ultrabook, and the root partition is, sda1 (a >> SSD) -> a LUKS encrypted partition -> LVM -> BTRFS. >> >> The kernel is a stock 4.15.11, however I only recently added the blk_mq >> options, so while I can state that I have seen this on multiple kernels >> in the 4.15.x series, I have not tested earlier kernels in this >> configuration. >> >> Looking through the code, I'd guess that this is dying inside >> blkg_rwstat_add, which calls percpu_counter_add_batch, which is what RIP >> is pointing at. > > Leaving the whole thing here for Paolo - it's crashing off insertion of > a request coming out of SG_IO. Don't think we've seen this BFQ failure > case before. > Actually, we have. Found and reported by Ming ab
Re: PROBLEM: buffer overflow and kernel panic in mmc_ioctl_cdrom_read_data
On 3/28/18 11:07 AM, Piotr Gabriel Kosinski wrote:
> When trying to read raw data from a CD drive using CDROMREADRAW ioctl
> when a CD is not present, the kernel crashes with a stack corruption
> error in mmc_ioctl_cdrom_read_data.
>
> From my (cursory) analysis it looks like the bug is caused by size
> mismatch between:
> - struct request_sense (64 bytes), used inside mmc_ioctl_cdrom_read_data
> - unsigned char[96], expected inside scsi_execute
>
> When the request_sense struct is passed to the cdrom_read_block, which
> then ultimately calls scsi_execute, the struct gets overwritten and
> overrun in drivers/scsi/scsi_lib.c:289:
>
> if (sense && rq->sense_len)
> memcpy(sense, rq->sense, SCSI_SENSE_BUFFERSIZE);
>
> I have recompiled the module with a hacky fix which replaces (in
> mmc_ioctl_cdrom_read_data):
>
> struct request_sense sense;
>
> with
>
> union {
> struct request_sense data;
> unsigned char buf[SCSI_SENSE_BUFFERSIZE];
> } sense;
>
> and that fixes the problem completely. The ioctl returns ENOMEDIUM as
> expected.
Thanks for debugging this. However, the scsi code looks a bit dangerous,
if it assumes that ->sense_len is >= SCSI_SENSE_BUFFERSIZE. I think the
correct fix would be to fix that assumption, and ensure that the path
of sr is correctly setting sense_len.
--
Jens Axboe
Re: General protection fault with use_blk_mq=1.
On 3/28/18 9:13 PM, Zephaniah E. Loss-Cutler-Hull wrote: > On 03/28/2018 06:02 PM, Jens Axboe wrote: >> On 3/28/18 5:03 PM, Zephaniah E. Loss-Cutler-Hull wrote: >>> I am not subscribed to any of the lists on the To list here, please CC >>> me on any replies. >>> >>> I am encountering a fairly consistent crash anywhere from 15 minutes to >>> 12 hours after boot with scsi_mod.use_blk_mq=1 dm_mod.use_blk_mq=1> >>> The crash looks like: >>> > >>> >>> Looking through the code, I'd guess that this is dying inside >>> blkg_rwstat_add, which calls percpu_counter_add_batch, which is what RIP >>> is pointing at. >> >> Leaving the whole thing here for Paolo - it's crashing off insertion of >> a request coming out of SG_IO. Don't think we've seen this BFQ failure >> case before. >> >> You can mitigate this by switching the scsi-mq devices to mq-deadline >> instead. >> > > I'm thinking that I should also be able to mitigate it by disabling > CONFIG_DEBUG_BLK_CGROUP. > > That should remove that entire chunk of code. > > Of course, that won't help if this is actually a symptom of a bigger > problem. Yes, it's not a given that it will fully mask the issue at hand. But turning off BFQ has a much higher chance of working for you. This time actually CC'ing Paolo. -- Jens Axboe
RE: [PATCH] scsi: megaraid_sas: Do not log an error if FW successfully initializes.
Hi Vinson,
> Fixes: 2d2c2331673c ("scsi: megaraid_sas: modified few prints in OCR and
IOC
> INIT path")
> Signed-off-by: Vinson Lee
> ---
> drivers/scsi/megaraid/megaraid_sas_fusion.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
Thanks for the patch.
Acked-by: Shivasharan S
Thanks,
Shivasharan
Re: General protection fault with use_blk_mq=1.
On 03/28/2018 06:02 PM, Jens Axboe wrote: > On 3/28/18 5:03 PM, Zephaniah E. Loss-Cutler-Hull wrote: >> I am not subscribed to any of the lists on the To list here, please CC >> me on any replies. >> >> I am encountering a fairly consistent crash anywhere from 15 minutes to >> 12 hours after boot with scsi_mod.use_blk_mq=1 dm_mod.use_blk_mq=1> >> The crash looks like: >> >> >> Looking through the code, I'd guess that this is dying inside >> blkg_rwstat_add, which calls percpu_counter_add_batch, which is what RIP >> is pointing at. > > Leaving the whole thing here for Paolo - it's crashing off insertion of > a request coming out of SG_IO. Don't think we've seen this BFQ failure > case before. > > You can mitigate this by switching the scsi-mq devices to mq-deadline > instead. > I'm thinking that I should also be able to mitigate it by disabling CONFIG_DEBUG_BLK_CGROUP. That should remove that entire chunk of code. Of course, that won't help if this is actually a symptom of a bigger problem. Regards, Zephaniah E. Loss-Cutler-Hull. signature.asc Description: OpenPGP digital signature
Re: General protection fault with use_blk_mq=1.
On 3/28/18 5:03 PM, Zephaniah E. Loss-Cutler-Hull wrote: > I am not subscribed to any of the lists on the To list here, please CC > me on any replies. > > I am encountering a fairly consistent crash anywhere from 15 minutes to > 12 hours after boot with scsi_mod.use_blk_mq=1 dm_mod.use_blk_mq=1> > The crash looks like: > > [ 5466.075993] general protection fault: [#1] PREEMPT SMP PTI > [ 5466.075997] Modules linked in: esp4 xfrm4_mode_tunnel fuse usblp > uvcvideo pci_stub vboxpci(O) vboxnetadp(O) vboxnetflt(O) vboxdrv(O) > ip6table_filter ip6_tables xt_tcpudp nf_conntrack_ipv4 nf_defrag_ipv4 > xt_conntrack nf_conntrack iptable_filter ip_tables x_tables intel_rapl > joydev serio_raw wmi_bmof iwldvm iwlwifi shpchp kvm_intel kvm irqbypass > autofs4 algif_skcipher nls_iso8859_1 nls_cp437 crc32_pclmul > ghash_clmulni_intel > [ 5466.076022] CPU: 3 PID: 10573 Comm: pool Tainted: G O > 4.15.13-f1-dirty #148 > [ 5466.076024] Hardware name: Hewlett-Packard HP EliteBook Folio > 9470m/18DF, BIOS 68IBD Ver. F.44 05/22/2013 > [ 5466.076029] RIP: 0010:percpu_counter_add_batch+0x2b/0xb0 > [ 5466.076031] RSP: 0018:a556c47afb58 EFLAGS: 00010002 > [ 5466.076033] RAX: 95cda87ce018 RBX: 95cda87cdb68 RCX: > > [ 5466.076034] RDX: 3fff RSI: 896495c4 RDI: > 895b2bed > [ 5466.076036] RBP: 3fff R08: R09: > 95cb7d5f8148 > [ 5466.076037] R10: 0200 R11: R12: > 0001 > [ 5466.076038] R13: 95cda87ce088 R14: 95cda6ebd100 R15: > a556c47afc58 > [ 5466.076040] FS: 7f25f5305700() GS:95cdbeac() > knlGS: > [ 5466.076042] CS: 0010 DS: ES: CR0: 80050033 > [ 5466.076043] CR2: 7f25e807e0a8 CR3: 0003ed5a6001 CR4: > 001606e0 > [ 5466.076044] Call Trace: > [ 5466.076050] bfqg_stats_update_io_add+0x58/0x100 > [ 5466.076055] bfq_insert_requests+0xec/0xd80 > [ 5466.076059] ? blk_rq_append_bio+0x8f/0xa0 > [ 5466.076061] ? blk_rq_map_user_iov+0xc3/0x1d0 > [ 5466.076065] blk_mq_sched_insert_request+0xa3/0x130 > [ 5466.076068] blk_execute_rq+0x3a/0x50 > [ 5466.076070] sg_io+0x197/0x3e0 > [ 5466.076073] ? dput+0xca/0x210 > [ 5466.076077] ? mntput_no_expire+0x11/0x1a0 > [ 5466.076079] scsi_cmd_ioctl+0x289/0x400 > [ 5466.076082] ? filename_lookup+0xe1/0x170 > [ 5466.076085] sd_ioctl+0xc7/0x1a0 > [ 5466.076088] blkdev_ioctl+0x4d4/0x8c0 > [ 5466.076091] block_ioctl+0x39/0x40 > [ 5466.076094] do_vfs_ioctl+0x92/0x5e0 > [ 5466.076097] ? __fget+0x73/0xc0 > [ 5466.076099] SyS_ioctl+0x74/0x80 > [ 5466.076102] do_syscall_64+0x60/0x110 > [ 5466.076106] entry_SYSCALL_64_after_hwframe+0x3d/0xa2 > [ 5466.076109] RIP: 0033:0x7f25f75fef47 > [ 5466.076110] RSP: 002b:7f25f53049a8 EFLAGS: 0246 ORIG_RAX: > 0010 > [ 5466.076112] RAX: ffda RBX: 000c RCX: > 7f25f75fef47 > [ 5466.076114] RDX: 7f25f53049b0 RSI: 2285 RDI: > 000c > [ 5466.076115] RBP: 0010 R08: 7f25e8007818 R09: > 0200 > [ 5466.076116] R10: 0001 R11: 0246 R12: > > [ 5466.076118] R13: R14: 7f25f8a6b5e0 R15: > 7f25e80173e0 > [ 5466.076120] Code: 41 55 49 89 fd bf 01 00 00 00 41 54 49 89 f4 55 89 > d5 53 e8 18 e1 bb ff 48 c7 c7 c4 95 64 89 e8 dc e9 fb ff 49 8b 45 20 48 > 63 d5 <65> 8b 18 48 63 db 4c 01 e3 48 39 d3 7d 0a f7 dd 48 63 ed 48 39 > [ 5466.076147] RIP: percpu_counter_add_batch+0x2b/0xb0 RSP: a556c47afb58 > [ 5466.076149] ---[ end trace 8d7eb80aafef4494 ]--- > [ 5466.670153] note: pool[10573] exited with preempt_count 2 > > (I only have the one instance right this minute as a result of not > having remote syslog setup before now.) > > This is clearly deep in the blk_mq code, and it goes away when I remove > the use_blk_mq kernel command line parameters. > > My next obvious step is to try and disable the load of the vbox modules. > > I can include the full dmesg output if it would be helpful. > > The system is an older HP Ultrabook, and the root partition is, sda1 (a > SSD) -> a LUKS encrypted partition -> LVM -> BTRFS. > > The kernel is a stock 4.15.11, however I only recently added the blk_mq > options, so while I can state that I have seen this on multiple kernels > in the 4.15.x series, I have not tested earlier kernels in this > configuration. > > Looking through the code, I'd guess that this is dying inside > blkg_rwstat_add, which calls percpu_counter_add_batch, which is what RIP > is pointing at. Leaving the whole thing here for Paolo - it's crashing off insertion of a request coming out of SG_IO. Don't think we've seen this BFQ failure case before. You can mitigate this by switching the scsi-mq devices to mq-deadline instead. -- Jens Axboe
答复: 答复: 答复: 答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
Hi, Arnd Thanks for your patiences. -邮件原件- 发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd Bergmann 发送时间: 2018年3月28日 20:50 收件人: liwei (CM) 抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM; linux-scsi; zangleigang; Gengjianfeng; Guodong Xu; Zhangfei Gao; Fengbaopeng (kevin, Kirin Solution Dept); Yaniv Gardi 主题: Re: 答复: 答复: 答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs On Tue, Mar 27, 2018 at 8:15 AM, liwei (CM) wrote: > Hi, Arnd > > At present our ufs module mainly has four clocks from the outside: > hclk_ufs: main clock of ufs controller ,freq is 207.5MHz > cfg_phy_clk: configuration clock of MPHY, freq is 51.875MHz > ref_phy_clk: reference clock of MPHY from PMU, freq is 19.2MHz > ref_io_clk:reference clock for the external interface to the device, freq > is 19.2MHz > > We control two clocks "ref_io_clk" and "cfg_phy_clk" in the driver > because the other two are controlled by main clock module and pmu. I'm not completely sure what you mean with "control" here. Do you mean setting the rate and disabling them during runtime power management? What does it mean for the clock to be controlled by teh "main clock module and pmu"? In the driver we only disable/enable "ref_io_clk" and "cfg_phy_clk" during runtime power management. > for this patch, cfg_phy_clk corresponds to "phy_clk", ref_io_clk corresponds > to "ref_clk". I'm not sure I understand the difference between ref_phy_clk and ref_io_clk, but it sounds like we should give both of those names in the ufs-platform binding. Your hclk_ufs would appear to correspond to what qualcomm calls core_clk, so maybe use that name as well. cfg_phy_clk seems to be something that qcom would not have, but it's also generic enough to list it in the common binding. Ok, let's add a describe for phy_clk in the common binding. > So the clks in the patch you give appear to be unsuitable for describing this > .And the following clks of qcom are internal clock? > We didn't describe or pay attention to the clock inside the ufs module. > > PHY to controller symbol synchronization clocks: > "rx_lane0_sync_clk" - RX Lane 0 > "rx_lane1_sync_clk" - RX Lane 1 > "tx_lane0_sync_clk" - TX Lane 0 > "tx_lane1_sync_clk" - TX Lane 1 Right, let's leave those for the qcom private binding. Arnd
RE: [Resend Patch 1/3] Vmbus: Add function to report available ring buffer to write in total ring size percentage
Maybe best through greg's char-misc tree since it has generic hv code and sometime updates both network and scsi. Alternatively, put common code through one tree, and hold off the network device change till next release. -Original Message- From: Long Li Sent: Wednesday, March 28, 2018 3:43 PM To: Martin K. Petersen ; Long Li Cc: KY Srinivasan ; Haiyang Zhang ; Stephen Hemminger ; James E . J . Bottomley ; de...@linuxdriverproject.org; linux-scsi@vger.kernel.org; linux-ker...@vger.kernel.org; net...@vger.kernel.org Subject: RE: [Resend Patch 1/3] Vmbus: Add function to report available ring buffer to write in total ring size percentage > Subject: Re: [Resend Patch 1/3] Vmbus: Add function to report available ring > buffer to write in total ring size percentage > > > Long, > > > Netvsc has a function to calculate how much ring buffer in percentage > > is available to write. This function is also useful for storvsc and > > other vmbus devices. > > What is the submission strategy for this series? Do you expect it to go > through net or scsi? If the latter, I'll need an ack from davem. Martin, I hope this patch set goes through SCSI, because it's purpose is to improve storvsc. If this strategy is not possible, I can resubmit the 1st two patches to net, and the 3rd patch to scsi after the 1st two are merged. Long > > -- > Martin K. PetersenOracle Linux Engineering
General protection fault with use_blk_mq=1.
I am not subscribed to any of the lists on the To list here, please CC me on any replies. I am encountering a fairly consistent crash anywhere from 15 minutes to 12 hours after boot with scsi_mod.use_blk_mq=1 dm_mod.use_blk_mq=1 The crash looks like: [ 5466.075993] general protection fault: [#1] PREEMPT SMP PTI [ 5466.075997] Modules linked in: esp4 xfrm4_mode_tunnel fuse usblp uvcvideo pci_stub vboxpci(O) vboxnetadp(O) vboxnetflt(O) vboxdrv(O) ip6table_filter ip6_tables xt_tcpudp nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack iptable_filter ip_tables x_tables intel_rapl joydev serio_raw wmi_bmof iwldvm iwlwifi shpchp kvm_intel kvm irqbypass autofs4 algif_skcipher nls_iso8859_1 nls_cp437 crc32_pclmul ghash_clmulni_intel [ 5466.076022] CPU: 3 PID: 10573 Comm: pool Tainted: G O 4.15.13-f1-dirty #148 [ 5466.076024] Hardware name: Hewlett-Packard HP EliteBook Folio 9470m/18DF, BIOS 68IBD Ver. F.44 05/22/2013 [ 5466.076029] RIP: 0010:percpu_counter_add_batch+0x2b/0xb0 [ 5466.076031] RSP: 0018:a556c47afb58 EFLAGS: 00010002 [ 5466.076033] RAX: 95cda87ce018 RBX: 95cda87cdb68 RCX: [ 5466.076034] RDX: 3fff RSI: 896495c4 RDI: 895b2bed [ 5466.076036] RBP: 3fff R08: R09: 95cb7d5f8148 [ 5466.076037] R10: 0200 R11: R12: 0001 [ 5466.076038] R13: 95cda87ce088 R14: 95cda6ebd100 R15: a556c47afc58 [ 5466.076040] FS: 7f25f5305700() GS:95cdbeac() knlGS: [ 5466.076042] CS: 0010 DS: ES: CR0: 80050033 [ 5466.076043] CR2: 7f25e807e0a8 CR3: 0003ed5a6001 CR4: 001606e0 [ 5466.076044] Call Trace: [ 5466.076050] bfqg_stats_update_io_add+0x58/0x100 [ 5466.076055] bfq_insert_requests+0xec/0xd80 [ 5466.076059] ? blk_rq_append_bio+0x8f/0xa0 [ 5466.076061] ? blk_rq_map_user_iov+0xc3/0x1d0 [ 5466.076065] blk_mq_sched_insert_request+0xa3/0x130 [ 5466.076068] blk_execute_rq+0x3a/0x50 [ 5466.076070] sg_io+0x197/0x3e0 [ 5466.076073] ? dput+0xca/0x210 [ 5466.076077] ? mntput_no_expire+0x11/0x1a0 [ 5466.076079] scsi_cmd_ioctl+0x289/0x400 [ 5466.076082] ? filename_lookup+0xe1/0x170 [ 5466.076085] sd_ioctl+0xc7/0x1a0 [ 5466.076088] blkdev_ioctl+0x4d4/0x8c0 [ 5466.076091] block_ioctl+0x39/0x40 [ 5466.076094] do_vfs_ioctl+0x92/0x5e0 [ 5466.076097] ? __fget+0x73/0xc0 [ 5466.076099] SyS_ioctl+0x74/0x80 [ 5466.076102] do_syscall_64+0x60/0x110 [ 5466.076106] entry_SYSCALL_64_after_hwframe+0x3d/0xa2 [ 5466.076109] RIP: 0033:0x7f25f75fef47 [ 5466.076110] RSP: 002b:7f25f53049a8 EFLAGS: 0246 ORIG_RAX: 0010 [ 5466.076112] RAX: ffda RBX: 000c RCX: 7f25f75fef47 [ 5466.076114] RDX: 7f25f53049b0 RSI: 2285 RDI: 000c [ 5466.076115] RBP: 0010 R08: 7f25e8007818 R09: 0200 [ 5466.076116] R10: 0001 R11: 0246 R12: [ 5466.076118] R13: R14: 7f25f8a6b5e0 R15: 7f25e80173e0 [ 5466.076120] Code: 41 55 49 89 fd bf 01 00 00 00 41 54 49 89 f4 55 89 d5 53 e8 18 e1 bb ff 48 c7 c7 c4 95 64 89 e8 dc e9 fb ff 49 8b 45 20 48 63 d5 <65> 8b 18 48 63 db 4c 01 e3 48 39 d3 7d 0a f7 dd 48 63 ed 48 39 [ 5466.076147] RIP: percpu_counter_add_batch+0x2b/0xb0 RSP: a556c47afb58 [ 5466.076149] ---[ end trace 8d7eb80aafef4494 ]--- [ 5466.670153] note: pool[10573] exited with preempt_count 2 (I only have the one instance right this minute as a result of not having remote syslog setup before now.) This is clearly deep in the blk_mq code, and it goes away when I remove the use_blk_mq kernel command line parameters. My next obvious step is to try and disable the load of the vbox modules. I can include the full dmesg output if it would be helpful. The system is an older HP Ultrabook, and the root partition is, sda1 (a SSD) -> a LUKS encrypted partition -> LVM -> BTRFS. The kernel is a stock 4.15.11, however I only recently added the blk_mq options, so while I can state that I have seen this on multiple kernels in the 4.15.x series, I have not tested earlier kernels in this configuration. Looking through the code, I'd guess that this is dying inside blkg_rwstat_add, which calls percpu_counter_add_batch, which is what RIP is pointing at. Regards, Zephaniah E. Loss-Cutler-Hull.
Re: [PATCH v4 3/7] scsi_io_completion_nz_result function added
On Wed, 2018-03-28 at 12:45 -0400, Douglas Gilbert wrote:
> + if (sense_valid && (sshdr.sense_key == RECOVERED_ERROR)) {
> + bool do_print = true;
> + /*
> + * if ATA PASS-THROUGH INFORMATION AVAILABLE [0x0, 0x1d]
> + * 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))
> + do_print = false;
> + else if (req->rq_flags & RQF_QUIET)
> + do_print = false;
> + if (do_print)
> + scsi_print_sense(cmd);
> + /* for passthrough, *blk_statp may be set, so clear */
> + *blk_statp = BLK_STS_OK;
> + result = 0;
> + }
> [ ... ]
> - 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->rq_flags & RQF_QUIET))
> - scsi_print_sense(cmd);
> - result = 0;
> - /* for passthrough, blk_stat may be set */
> - blk_stat = BLK_STS_OK;
> - }
In the new code there is a 'do_print' variable but there is no such variable
in the original code. Please separate code movement from other code changes.
Separating code movement from other code changes is important not only because
it makes reviewing easier but is also important for people who run a bisect.
Thanks,
Bart.
RE: [Resend Patch 1/3] Vmbus: Add function to report available ring buffer to write in total ring size percentage
> Subject: Re: [Resend Patch 1/3] Vmbus: Add function to report available ring > buffer to write in total ring size percentage > > > Long, > > > Netvsc has a function to calculate how much ring buffer in percentage > > is available to write. This function is also useful for storvsc and > > other vmbus devices. > > What is the submission strategy for this series? Do you expect it to go > through net or scsi? If the latter, I'll need an ack from davem. Martin, I hope this patch set goes through SCSI, because it's purpose is to improve storvsc. If this strategy is not possible, I can resubmit the 1st two patches to net, and the 3rd patch to scsi after the 1st two are merged. Long > > -- > Martin K. PetersenOracle Linux Engineering
RE: [PATCH] scsi: devinfo: Add Microsoft iSCSI target to 1024 sector blacklist
> Subject: Re: [PATCH] scsi: devinfo: Add Microsoft iSCSI target to 1024 sector
> blacklist
>
>
> Long, KY: Please confirm.
>
> > The Windows Server 2016 iSCSI target doesn't work with the Linux
> > kernel initiator since the kernel started sending larger requests by
> > default, nor does it implement the block limits VPD page. Apply the
> > sector limit workaround for these targets.
> >
> > Signed-off-by: Ross Lagerwall
> > ---
> > drivers/scsi/scsi_devinfo.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> > index f3b1172..5cb748a 100644
> > --- a/drivers/scsi/scsi_devinfo.c
> > +++ b/drivers/scsi/scsi_devinfo.c
> > @@ -213,7 +213,7 @@ static struct {
> > {"Medion", "Flash XL MMC/SD", "2.6D", BLIST_FORCELUN},
> > {"MegaRAID", "LD", NULL, BLIST_FORCELUN},
> > {"MICROP", "4110", NULL, BLIST_NOTQ},
> > - {"MSFT", "Virtual HD", NULL, BLIST_NO_RSOC},
> > + {"MSFT", "Virtual HD", NULL, BLIST_MAX_1024 | BLIST_NO_RSOC},
Ross,
What about storage_channel_properties.max_transfer_bytes returned from
VSTOR_OPERATION_QUERY_PROPERTIES (in storvsc_channel_init())
Does it return correctly the maximum transfer size for iSCSI?
Long
> > {"MYLEX", "DACARMRB", "*", BLIST_REPORTLUN2},
> > {"nCipher", "Fastness Crypto", NULL, BLIST_FORCELUN},
> > {"NAKAMICH", "MJ-4.8S", NULL, BLIST_FORCELUN |
> BLIST_SINGLELUN},
>
> --
> Martin K. PetersenOracle Linux Engineering
Re: [PATCH] scsi: megaraid_sas: Do not log an error if FW successfully initializes.
Kashyap/Sumit: Please review!
> Fixes: 2d2c2331673c ("scsi: megaraid_sas: modified few prints in OCR and IOC
> INIT path")
> Signed-off-by: Vinson Lee
> ---
> drivers/scsi/megaraid/megaraid_sas_fusion.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> index dc8e850fbfd2..7047d2e25e2c 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> @@ -1124,12 +1124,12 @@ megasas_ioc_init_fusion(struct megasas_instance
> *instance)
> goto fail_fw_init;
> }
>
> - ret = 0;
> + return 0;
>
> fail_fw_init:
> dev_err(&instance->pdev->dev,
> - "Init cmd return status %s for SCSI host %d\n",
> - ret ? "FAILED" : "SUCCESS", instance->host->host_no);
> + "Init cmd return status FAILED for SCSI host %d\n",
> + instance->host->host_no);
>
> return ret;
> }
--
Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: don't look for NULL devices handlers by name
Johannes, > Currently scsi_dh_lookup() doesn't check for NULL as a device > name. This combined with nvme over dm-mapth results in the following > messages emitted by device-mapper: Applied to 4.16/scsi-fixes. Thank you! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH][scsi-next] scsi: core: remove redundant assignment to shost->use_blk_mq
Colin, > The first assignment to shost->use_blk_mq is redundant as it is > overwritten by the following statement. Remove this redundant code. Applied to 4.16/scsi-fixes. Thank you! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: fnic: fix spelling mistake: "abord" -> "abort"
Colin, > Trivial fix to spelling mistake in message text Applied to 4.17/scsi-queue. Thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v3] scsi: ufs: add trace event for ufs upiu
Ohad, > Add UFS Protocol Information Units(upiu) trace events for ufs driver, > used to trace various ufs transaction types- command, task-management > and device management. The trace-point format is generic and can be > easily adapted to trace other upius if needed. Currently tracing ufs > transaction of type 'device management', which this patch introduce, > cannot be obtained from any other trace. Device management > transactions are used for communication with the device such as > reading and writing descriptor or attributes etc. Applied to 4.17/scsi-queue. Thank you! -- Martin K. Petersen Oracle Linux Engineering
Re: [Resend Patch 1/3] Vmbus: Add function to report available ring buffer to write in total ring size percentage
Long, > Netvsc has a function to calculate how much ring buffer in percentage is > available to write. This function is also useful for storvsc and other > vmbus devices. What is the submission strategy for this series? Do you expect it to go through net or scsi? If the latter, I'll need an ack from davem. -- Martin K. Petersen Oracle Linux Engineering
Re: [RFC PATCH] mpt3sas: mpt3sas_scsih_enclosure_find_by_handle can be static
Bart, > Are you aware that if the 0-day test infrastructure suggests an improvement > for a patch that the patch that that improvement applies to gets ignored > unless either the patch is reposted with the improvement applied or that it > is explained why the suggested improvement is inappropriate? Correct. I don't apply anything that causes a 0-day warning. The patch will be closed with "Changes Required" status in patchwork. Always build patch submissions to linux-scsi with: make C=1 CF="-D__CHECK_ENDIAN__" -- Martin K. Petersen Oracle Linux Engineering
[Bug 198081] scsi sg
https://bugzilla.kernel.org/show_bug.cgi?id=198081 Anthony J. Battersby (to...@cybernetics.com) changed: What|Removed |Added CC||to...@cybernetics.com --- Comment #5 from Anthony J. Battersby (to...@cybernetics.com) --- I ran into a similar problem. Applying this fixed it for me: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=48ae8484e9fc324b4968d33c585e54bc98e44d61 See my message to the -stable kernel maintainers for more info: https://marc.info/?l=linux-scsi&m=152227354106242 -- You are receiving this mail because: You are the assignee for the bug.
Re: [PATCH] scsi: devinfo: Add Microsoft iSCSI target to 1024 sector blacklist
Long, KY: Please confirm.
> The Windows Server 2016 iSCSI target doesn't work with the Linux kernel
> initiator since the kernel started sending larger requests by default,
> nor does it implement the block limits VPD page. Apply the sector limit
> workaround for these targets.
>
> Signed-off-by: Ross Lagerwall
> ---
> drivers/scsi/scsi_devinfo.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
> index f3b1172..5cb748a 100644
> --- a/drivers/scsi/scsi_devinfo.c
> +++ b/drivers/scsi/scsi_devinfo.c
> @@ -213,7 +213,7 @@ static struct {
> {"Medion", "Flash XL MMC/SD", "2.6D", BLIST_FORCELUN},
> {"MegaRAID", "LD", NULL, BLIST_FORCELUN},
> {"MICROP", "4110", NULL, BLIST_NOTQ},
> - {"MSFT", "Virtual HD", NULL, BLIST_NO_RSOC},
> + {"MSFT", "Virtual HD", NULL, BLIST_MAX_1024 | BLIST_NO_RSOC},
> {"MYLEX", "DACARMRB", "*", BLIST_REPORTLUN2},
> {"nCipher", "Fastness Crypto", NULL, BLIST_FORCELUN},
> {"NAKAMICH", "MJ-4.8S", NULL, BLIST_FORCELUN | BLIST_SINGLELUN},
--
Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: remove reference to scsi_show_extd_sense()
John,
> In commit 2104551969e8 ("scsi: use per-cpu buffer for formatting
> sense"), function scsi_show_extd_sense() was removed, switching use
> over to scsi_format_extd_sense(). Remove last reference to
> scsi_show_extd_sense() in include/scsi/scsi_dbg.h.
Applied to 4.17/scsi-queue. Thanks!
--
Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: qla2xxx: Fix small memory leak in qla2x00_probe_one on probe failure
> On Mar 28, 2018, at 2:45 PM, Martin K. Petersen
> wrote:
>
>
>> The code that fixes the crashes in the following commit introduced a
>> small memory leak:
>>
>> commit 6a2cf8d3663e ("scsi: qla2xxx: Fix crashes in qla2x00_probe_one on
>> probe failure")
>>
>> Fixing this requires a bit of reworking, which I've explained. Also provide
>> some code cleanup.
>>
>> Fixes: 6a2cf8d3663e ("scsi: qla2xxx: Fix crashes in qla2x00_probe_one on
>> probe failure")
>> Signed-off-by: Bill Kuzeja
>
> Himanshu?
>
Still reviewing this one. Will respond soon
> --
> Martin K. PetersenOracle Linux Engineering
Thanks,
- Himanshu
Re: [PATCH] scsi: qla2xxx: Fix small memory leak in qla2x00_probe_one on probe failure
> The code that fixes the crashes in the following commit introduced a
> small memory leak:
>
> commit 6a2cf8d3663e ("scsi: qla2xxx: Fix crashes in qla2x00_probe_one on
> probe failure")
>
> Fixing this requires a bit of reworking, which I've explained. Also provide
> some code cleanup.
>
> Fixes: 6a2cf8d3663e ("scsi: qla2xxx: Fix crashes in qla2x00_probe_one on
> probe failure")
> Signed-off-by: Bill Kuzeja
Himanshu?
--
Martin K. Petersen Oracle Linux Engineering
SCSI generic (sg) regression in stable kernels
The SCSI generic driver is currently broken in the 3.18, 4.1, 4.4, and
4.9 stable kernels. This is the problematic commit:
109bade9c625 ("scsi: sg: use standard lists for sg_requests")
(all commit ids are from upstream git, not stable.git)
There have already been a number of fixes backported to -stable to fix
problems introduced by that commit:
bd46fc406b30 ("scsi: sg: off by one in sg_ioctl()")
4759df905a47 ("scsi: sg: factor out sg_fill_request_table()")
3e0097499839 ("scsi: sg: fixup infoleak when using SG_GET_REQUEST_TABLE")
587c3c9f286c ("scsi: sg: Re-fix off by one in sg_fill_request_table()")
I am not sure why 109bade9c625 was added to -stable to begin with. My
guess is that the "infoleak" fix was flagged as being appropriate for
-stable, and it depended on the original commit 109bade9c625. But if
you take a close look, the original commit 109bade9c625 is what
introduced the infoleak in the first place. Or maybe there was some
other reason. But I digress.
There is one more fix for 109bade9c625 that has not yet been backported:
48ae8484e9fc ("scsi: sg: don't return bogus Sg_requests")
Without the fix above, sg oopses as soon as I try to use it. So please
add 48ae8484e9fc to -stable.
The following -stable kernels already have 48ae8484e9fc:
4.14.31
4.15.14
The following -stable kernels need 48ae8484e9fc:
3.18.102
4.1.51
4.4.125
4.9.91
The following -stable kernels are not affected because they have not had
the problematic commit backported:
3.2.101
3.16.56
Related bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=198081
Thanks!
Tony Battersby
Cybernetics
Re: [PATCH v3 00/41] cxlflash: OCXL transport support and miscellaneous fixes
Uma, > This patch series adds OCXL support to the cxlflash driver. With this > support, new devices using the OCXL transport will be supported by the > cxlflash driver along with the existing CXL devices. An effort is made > to keep this transport specific function independent of the existing > core driver that communicates with the AFU. > > The first three patches contain a minor fix and staging improvements. > > This series is intended for 4.17 and is bisectable. Something this big really needs to be submitted around rc2/rc3. It's way too late in the cycle to ensure proper zeroday coverage for all these commits. I have started a 4.18/scsi-queue branch to hold this series for now. The 4.18 branch will be rebased once 4.17rc1 is out in a few weeks. Your changes won't show up in for-next until then either. Thanks! -- Martin K. Petersen Oracle Linux Engineering
AACRAID crashes immediately on PPC32
hi AACRAID crashes immediately on PPC32 I have opened this ticket at bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=199223 let me know
Re: [PATCH v2 net-next] qed*: Utilize FW 8.33.11.0
On Wed, Mar 28, 2018 at 11:42:16AM +0300, Michal Kalderon wrote: > This FW contains several fixes and features > > RDMA Features > - SRQ support > - XRC support > - Memory window support > - RDMA low latency queue support > - RDMA bonding support > > RDMA bug fixes > - RDMA remote invalidate during retransmit fix > - iWARP MPA connect interop issue with RTR fix > - iWARP Legacy DPM support > - Fix MPA reject flow > - iWARP error handling > - RQ WQE validation checks > > MISC > - Fix some HSI types endianity > - New Restriction: vlan insertion in core_tx_bd_data can't be set > for LB packets > > ETH > - HW QoS offload support > - Fix vlan, dcb and sriov flow of VF sending a packet with > inband VLAN tag instead of default VLAN > - Allow GRE version 1 offloads in RX flow > - Allow VXLAN steering > > iSCSI / FcoE > - Fix bd availability checking flow > - Support 256th sge proerly in iscsi/fcoe retransmit > - Performance improvement > - Fix handle iSCSI command arrival with AHS and with immediate > - Fix ipv6 traffic class configuration > > DEBUG > - Update debug utilities > > Signed-off-by: Michal Kalderon > Signed-off-by: Tomer Tayar > Signed-off-by: Manish Rangankar > Signed-off-by: Ariel Elior > --- > Changes from v1 > Remove version bump and qedr module version addition > --- > drivers/infiniband/hw/qedr/qedr_hsi_rdma.h |4 +- > drivers/infiniband/hw/qedr/verbs.c |4 +- > drivers/net/ethernet/qlogic/qed/qed_debug.c| 415 +++-- > drivers/net/ethernet/qlogic/qed/qed_dev.c |4 +- > drivers/net/ethernet/qlogic/qed/qed_hsi.h | 1892 > ++-- > .../net/ethernet/qlogic/qed/qed_init_fw_funcs.c| 103 +- > drivers/net/ethernet/qlogic/qed/qed_iwarp.c|7 - > drivers/net/ethernet/qlogic/qed/qed_l2.c |2 +- > drivers/net/ethernet/qlogic/qed/qed_ll2.c | 13 - > include/linux/qed/common_hsi.h |2 +- > include/linux/qed/eth_common.h |2 +- > include/linux/qed/iscsi_common.h |4 +- > include/linux/qed/rdma_common.h|2 + > include/linux/qed/roce_common.h|3 + > 14 files changed, 1310 insertions(+), 1147 deletions(-) For drivers/infiniband: Acked-by: Jason Gunthorpe Jason
Re: [PATCH] scsi: cxgb4i: potential array overflow in t4_uld_rx_handler()
On Wed, Mar 28, 2018 at 09:14:25PM +0530, Varun Prakash wrote:
> On Wed, Mar 21, 2018 at 09:12:00PM -0400, Martin K. Petersen wrote:
> >
> > Varun: Please look at this. Thanks!
> >
> > > What happened to this one?
> > >
> > > regards,
> > > dan carpenter
> > >
> > >
> > > On Wed, Nov 29, 2017 at 02:42:20PM +0300, Dan Carpenter wrote:
> > >> The story is that Smatch marks skb->data as untrusted and so it
> > >> complains about this code:
> > >>
> > >> drivers/scsi/cxgbi/cxgb4i/cxgb4i.c:2111 t4_uld_rx_handler()
> > >> error: buffer overflow 'cxgb4i_cplhandlers' 239 <= 255.
> > >>
> > >> I don't know the code very well, but it looks like a reasonable warning
> > >> message. Let's address it by adding a sanity check to make sure "opc"
> > >> is within bounds.
> > >>
> > >> Fixes: bbc02c7e9d34 ("cxgb4: Add register, message, and FW definitions")
> > >> Signed-off-by: Dan Carpenter
> > >>
> > >> diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> > >> b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> > >> index 266eddf17a99..94b2d5660a07 100644
> > >> --- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> > >> +++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> > >> @@ -2108,12 +2108,12 @@ static int t4_uld_rx_handler(void *handle, const
> > >> __be64 *rsp,
> > >> log_debug(1 << CXGBI_DBG_TOE,
> > >> "cdev %p, opcode 0x%x(0x%x,0x%x), skb %p.\n",
> > >> cdev, opc, rpl->ot.opcode_tid,
> > >> ntohl(rpl->ot.opcode_tid), skb);
> > >> -if (cxgb4i_cplhandlers[opc])
> > >> -cxgb4i_cplhandlers[opc](cdev, skb);
> > >> -else {
> > >> +if (opc >= ARRAY_SIZE(cxgb4i_cplhandlers) ||
> > >> !cxgb4i_cplhandlers[opc]) {
> > >> pr_err("No handler for opcode 0x%x.\n", opc);
> > >> __kfree_skb(skb);
> > >> +return 0;
> > >> }
> > >> +cxgb4i_cplhandlers[opc](cdev, skb);
> > >> return 0;
> > >> nomem:
> > >> log_debug(1 << CXGBI_DBG_TOE, "OOM bailing out.\n");
> > >
> > >
>
> This check is not necessary but we can add it to avoid warning.
Is the reason it's not necessary, because the skb->data comes from the
firmware and we trust it? The v5 declares the array as having 256
elements which also solves this warning. And cxgbit_uld_lro_rx_handler()
has a bounds check. So it seems pretty normal to prevent the array
overflow by force as well as by trust.
> The commit mentioned in "Fixes" is not correct, this code was added in commit
> "7b36b6e [SCSI] cxgb4i v5: iscsi driver"
Yeah. You're right. I can resend with an updated commit message.
regards,
dan carpenter
PROBLEM: buffer overflow and kernel panic in mmc_ioctl_cdrom_read_data
When trying to read raw data from a CD drive using CDROMREADRAW ioctl
when a CD is not present, the kernel crashes with a stack corruption
error in mmc_ioctl_cdrom_read_data.
>From my (cursory) analysis it looks like the bug is caused by size
mismatch between:
- struct request_sense (64 bytes), used inside mmc_ioctl_cdrom_read_data
- unsigned char[96], expected inside scsi_execute
When the request_sense struct is passed to the cdrom_read_block, which
then ultimately calls scsi_execute, the struct gets overwritten and
overrun in drivers/scsi/scsi_lib.c:289:
if (sense && rq->sense_len)
memcpy(sense, rq->sense, SCSI_SENSE_BUFFERSIZE);
I have recompiled the module with a hacky fix which replaces (in
mmc_ioctl_cdrom_read_data):
struct request_sense sense;
with
union {
struct request_sense data;
unsigned char buf[SCSI_SENSE_BUFFERSIZE];
} sense;
and that fixes the problem completely. The ioctl returns ENOMEDIUM as expected.
Regards,
Piotr Kosinski.
Re: [PATCH][scsi-next] scsi: core: remove redundant assignment to shost->use_blk_mq
On Wed, 2018-03-28 at 17:41 +0100, Colin King wrote:
> From: Colin Ian King
>
> The first assignment to shost->use_blk_mq is redundant as it is
> overwritten by the following statement. Remove this redundant code.
>
> Detected by CoverityScan, CID#1466993 ("Unused value")
Reviewed-by: Bart Van Assche
[PATCH v4 3/7] scsi_io_completion_nz_result function added
Break out several intertwined paths when cmd->result is non zero and
place them in the scsi_io_completion_nz_result helper function. The
logic is not changed.
Signed-off-by: Douglas Gilbert
---
A reviewer requested the original helper function's two return values
be reduced to one: the blk_stat variable. This required a hack to
differentiate the default setting of blk_stat (BLK_STS_OK) from the case
when the helper assigns BLK_STS_OK as the return value. The hack is to
return the otherwise unused BLK_STS_NOTSUPP value as an indication that
the helper didn't change anything. That hack was judged by another
reviewer to be worse that the "two return values" ugliness it was
trying to address. So back to the original "two return values" solution.
drivers/scsi/scsi_lib.c | 127
1 file changed, 75 insertions(+), 52 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 598836804745..6a8b9dc7c40e 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -751,6 +751,79 @@ static blk_status_t __scsi_error_from_host_byte(struct
scsi_cmnd *cmd,
}
}
+/*
+ * Helper for scsi_io_completion() when cmd->result is non-zero. Returns a
+ * new result that may suppress further error checking. Also modifies
+ * *blk_statp in some cases.
+ */
+static int scsi_io_completion_nz_result(struct scsi_cmnd *cmd, int result,
+ blk_status_t *blk_statp)
+{
+ bool sense_valid;
+ bool sense_current = true; /* false implies "deferred sense" */
+ struct request *req = cmd->request;
+ struct scsi_sense_hdr sshdr;
+
+ sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
+ if (sense_valid)
+ sense_current = !scsi_sense_is_deferred(&sshdr);
+
+ if (blk_rq_is_passthrough(req)) {
+ if (sense_valid) {
+ /*
+* SG_IO wants current and deferred errors
+*/
+ scsi_req(req)->sense_len =
+ min(8 + cmd->sense_buffer[7],
+ SCSI_SENSE_BUFFERSIZE);
+ }
+ if (sense_current)
+ *blk_statp = __scsi_error_from_host_byte(cmd, result);
+ } else if (blk_rq_bytes(req) == 0 && sense_current) {
+ /*
+* Flush commands do not transfers any data, and thus cannot use
+* good_bytes != blk_rq_bytes(req) as the signal for an error.
+* This sets *blk_statp explicitly for the problem case.
+*/
+ *blk_statp = __scsi_error_from_host_byte(cmd, result);
+ }
+ /*
+* Recovered errors need reporting, but they're always treated as
+* success, so fiddle the result code here. For passthrough requests
+* we already took a copy of the original into sreq->result which
+* is what gets returned to the user
+*/
+ if (sense_valid && (sshdr.sense_key == RECOVERED_ERROR)) {
+ bool do_print = true;
+ /*
+* if ATA PASS-THROUGH INFORMATION AVAILABLE [0x0, 0x1d]
+* 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))
+ do_print = false;
+ else if (req->rq_flags & RQF_QUIET)
+ do_print = false;
+ if (do_print)
+ scsi_print_sense(cmd);
+ /* for passthrough, *blk_statp may be set, so clear */
+ *blk_statp = BLK_STS_OK;
+ result = 0;
+ }
+ /*
+* Another corner case: the SCSI status byte is non-zero but 'good'.
+* Example: PRE-FETCH command returns SAM_STAT_CONDITION_MET when
+* it is able to fit nominated LBs in its cache (and SAM_STAT_GOOD
+* if it can't fit). Treat SAM_STAT_CONDITION_MET and the related
+* intermediate statuses (both obsolete in SAM-4) as good.
+*/
+ if (status_byte(result) && scsi_status_is_good(result)) {
+ *blk_statp = BLK_STS_OK;
+ result = 0;
+ }
+ return result;
+}
+
/*
* Function:scsi_io_completion()
*
@@ -794,26 +867,14 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
ACTION_DELAYED_RETRY} action;
unsigned long wait_for = (cmd->allowed + 1) * req->timeout;
- if (result) {
+ if (result) { /* does not necessarily mean there is an error */
sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
if (sense_valid)
sense_current = !scsi_sense_is_deferred(&sshdr);
+ result = scsi_io_completion_nz_result(cmd, result, &blk_st
[PATCH v4 7/7] scsi_io_completion convert BUGs to WARNs
The scsi_io_completion function contains three BUG() and BUG_ON() calls.
Replace them with WARN variants.
Signed-off-by: Douglas Gilbert
Reviewed-by: Johannes Thumshirn
---
drivers/scsi/scsi_lib.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index a9676d824452..a89ca1ddc01a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1046,13 +1046,21 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
scsi_req(req->next_rq)->resid_len = scsi_in(cmd)->resid;
if (scsi_end_request(req, BLK_STS_OK, blk_rq_bytes(req),
blk_rq_bytes(req->next_rq)))
- BUG();
+ WARN_ONCE(true,
+ "Bidi command with remaining bytes");
return;
}
}
/* no bidi support yet, other than in pass-through */
- BUG_ON(blk_bidi_rq(req));
+ if (unlikely(blk_bidi_rq(req))) {
+ WARN_ONCE(true, "Only support bidi command in passthrough");
+ scmd_printk(KERN_ERR, cmd, "Killing bidi command\n");
+ if (scsi_end_request(req, BLK_STS_IOERR, blk_rq_bytes(req),
+blk_rq_bytes(req->next_rq)))
+ WARN_ONCE(true, "Bidi command with remaining bytes");
+ return;
+ }
/*
* Next deal with any sectors which we were able to correctly
@@ -1075,7 +1083,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
/* Kill remainder if no retries */
if (unlikely(blk_stat && scsi_noretry_cmd(cmd))) {
if (scsi_end_request(req, blk_stat, blk_rq_bytes(req), 0))
- BUG();
+ WARN_ONCE(true,
+ "Bytes remaining after failed, no-retry command");
return;
}
--
2.14.1
[PATCH v4 6/7] scsi_io_completion hints on fastpatch
Add likely() and unlikely() hints to conditionals on or near the fastpath.
Signed-off-by: Douglas Gilbert
Reviewed-by: Johannes Thumshirn
---
A reviewer wanted any performance improvement (or otherwise) quantified.
The improvement was so small, that ftrace ignored it. Inline timing code
suggests the improvement from this whole patchset is around 7 nanoseconds
per invocation (tested on a Lenovo X270 (i5-7200U)). Not exactly huge.
Another win might be the smaller size of scsi_io_completion() after the
refactoring; this should allow more other code to fit in the instruction
cache.
drivers/scsi/scsi_lib.c | 22 ++
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7bf7d3c4b9c2..a9676d824452 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1028,17 +1028,17 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
struct request *req = cmd->request;
blk_status_t blk_stat = BLK_STS_OK;
- if (result) /* does not necessarily mean there is an error */
+ if (unlikely(result)) /* a nz result may or may not be an error */
result = scsi_io_completion_nz_result(cmd, result, &blk_stat);
- if (blk_rq_is_passthrough(req)) {
+ if (unlikely(blk_rq_is_passthrough(req))) {
/*
* __scsi_error_from_host_byte may have reset the host_byte
*/
scsi_req(req)->result = cmd->result;
scsi_req(req)->resid_len = scsi_get_resid(cmd);
- if (scsi_bidi_cmnd(cmd)) {
+ if (unlikely(scsi_bidi_cmnd(cmd))) {
/*
* Bidi commands Must be complete as a whole,
* both sides at once.
@@ -1051,7 +1051,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
}
}
- /* no bidi support for !blk_rq_is_passthrough yet */
+ /* no bidi support yet, other than in pass-through */
BUG_ON(blk_bidi_rq(req));
/*
@@ -1067,15 +1067,13 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
* handle. Failed, zero length commands always need to drop down
* to retry code. Fast path should return in this block.
*/
- if (blk_rq_bytes(req) > 0 || blk_stat == BLK_STS_OK) {
- if (!scsi_end_request(req, blk_stat, good_bytes, 0))
- return; /* no bytes remaining */
+ if (likely(blk_rq_bytes(req) > 0 || blk_stat == BLK_STS_OK)) {
+ if (likely(!scsi_end_request(req, blk_stat, good_bytes, 0)))
+ return; /* no bytes remaining */
}
- /*
-* Kill remainder if no retrys.
-*/
- if (blk_stat && scsi_noretry_cmd(cmd)) {
+ /* Kill remainder if no retries */
+ if (unlikely(blk_stat && scsi_noretry_cmd(cmd))) {
if (scsi_end_request(req, blk_stat, blk_rq_bytes(req), 0))
BUG();
return;
@@ -1085,7 +1083,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
* If there had been no error, but we have leftover bytes in the
* requeues just queue the command up again.
*/
- if (result == 0)
+ if (likely(result == 0))
scsi_io_completion_reprep(cmd, q);
else
scsi_io_completion_action(cmd, result);
--
2.14.1
[PATCH v4 5/7] scsi_io_completion_reprep helper added
Since the action "reprep" is called from two places, rather than repeat
the code, make a new scsi_io_completion helper with "reprep" as its
suffix.
Signed-off-by: Douglas Gilbert
Reviewed-by: Johannes Thumshirn
---
drivers/scsi/scsi_lib.c | 41 ++---
1 file changed, 18 insertions(+), 23 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 048b144de5b0..7bf7d3c4b9c2 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -751,6 +751,20 @@ static blk_status_t __scsi_error_from_host_byte(struct
scsi_cmnd *cmd,
}
}
+/* Helper for scsi_io_completion() when "reprep" action required. */
+static void scsi_io_completion_reprep(struct scsi_cmnd *cmd,
+ struct request_queue *q)
+{
+ /* A new command will be prepared and issued. */
+ if (q->mq_ops) {
+ scsi_mq_requeue_cmd(cmd);
+ } else {
+ /* Unprep request and put it back at head of the queue. */
+ scsi_release_buffers(cmd);
+ scsi_requeue_command(q, cmd);
+ }
+}
+
/* Helper for scsi_io_completion() when special action required. */
static void scsi_io_completion_action(struct scsi_cmnd *cmd, int result)
{
@@ -892,15 +906,7 @@ static void scsi_io_completion_action(struct scsi_cmnd
*cmd, int result)
return;
/*FALLTHRU*/
case ACTION_REPREP:
- /* Unprep the request and put it back at the head of the queue.
-* A new command will be prepared and issued.
-*/
- if (q->mq_ops)
- scsi_mq_requeue_cmd(cmd);
- else {
- scsi_release_buffers(cmd);
- scsi_requeue_command(q, cmd);
- }
+ scsi_io_completion_reprep(cmd, q);
break;
case ACTION_RETRY:
/* Retry the same command immediately */
@@ -1079,20 +1085,9 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
* If there had been no error, but we have leftover bytes in the
* requeues just queue the command up again.
*/
- if (result == 0) {
- /*
-* Unprep the request and put it back at the head of the
-* queue. A new command will be prepared and issued.
-* This block is the same as case ACTION_REPREP in
-* scsi_io_completion_action() above.
-*/
- if (q->mq_ops)
- scsi_mq_requeue_cmd(cmd);
- else {
- scsi_release_buffers(cmd);
- scsi_requeue_command(q, cmd);
- }
- } else
+ if (result == 0)
+ scsi_io_completion_reprep(cmd, q);
+ else
scsi_io_completion_action(cmd, result);
}
--
2.14.1
[PATCH v4 4/7] scsi_io_completion_action helper added
Place scsi_io_completion()'s complex error processing associated with a
local enumeration into a static helper function. That enumeration's
values start with "ACTION_" so use the suffix "_action" in the helper
function's name.
Signed-off-by: Douglas Gilbert
Reviewed-by: Johannes Thumshirn
---
drivers/scsi/scsi_lib.c | 325 ++--
1 file changed, 173 insertions(+), 152 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 6a8b9dc7c40e..048b144de5b0 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -751,6 +751,168 @@ static blk_status_t __scsi_error_from_host_byte(struct
scsi_cmnd *cmd,
}
}
+/* Helper for scsi_io_completion() when special action required. */
+static void scsi_io_completion_action(struct scsi_cmnd *cmd, int result)
+{
+ struct request_queue *q = cmd->device->request_queue;
+ struct request *req = cmd->request;
+ int level = 0;
+ enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
+ ACTION_DELAYED_RETRY} action;
+ unsigned long wait_for = (cmd->allowed + 1) * req->timeout;
+ struct scsi_sense_hdr sshdr;
+ bool sense_valid;
+ bool sense_current = true; /* false implies "deferred sense" */
+ blk_status_t blk_stat;
+
+ sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
+ if (sense_valid)
+ sense_current = !scsi_sense_is_deferred(&sshdr);
+
+ blk_stat = __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_current) {
+ switch (sshdr.sense_key) {
+ case UNIT_ATTENTION:
+ if (cmd->device->removable) {
+ /* Detected disc change. Set a bit
+* and quietly refuse further access.
+*/
+ cmd->device->changed = 1;
+ action = ACTION_FAIL;
+ } else {
+ /* Must have been a power glitch, or a
+* bus reset. Could not have been a
+* media change, so we just retry the
+* command and see what happens.
+*/
+ action = ACTION_RETRY;
+ }
+ break;
+ case ILLEGAL_REQUEST:
+ /* If we had an ILLEGAL REQUEST returned, then
+* we may have performed an unsupported
+* command. The only thing this should be
+* would be a ten byte read where only a six
+* byte read was supported. Also, on a system
+* where READ CAPACITY failed, we may have
+* read past the end of the disk.
+*/
+ if ((cmd->device->use_10_for_rw &&
+ sshdr.asc == 0x20 && sshdr.ascq == 0x00) &&
+ (cmd->cmnd[0] == READ_10 ||
+cmd->cmnd[0] == WRITE_10)) {
+ /* This will issue a new 6-byte command. */
+ cmd->device->use_10_for_rw = 0;
+ action = ACTION_REPREP;
+ } else if (sshdr.asc == 0x10) /* DIX */ {
+ action = ACTION_FAIL;
+ blk_stat = BLK_STS_PROTECTION;
+ /* INVALID COMMAND OPCODE or INVALID FIELD IN CDB */
+ } else if (sshdr.asc == 0x20 || sshdr.asc == 0x24) {
+ action = ACTION_FAIL;
+ blk_stat = BLK_STS_TARGET;
+ } else
+ action = ACTION_FAIL;
+ break;
+ case ABORTED_COMMAND:
+ action = ACTION_FAIL;
+ if (sshdr.asc == 0x10) /* DIF */
+ blk_stat = BLK_STS_PROTECTION;
+ break;
+ case NOT_READY:
+ /* If the device is in the process of becoming
+* ready, or has a temporary blockage, retry.
+*/
+ if (sshdr.asc == 0x04) {
+ switch (sshdr.ascq) {
+ case 0x01: /* becoming ready */
+ case 0x04: /* format in progress */
+ case 0x05: /* rebuild i
[PATCH v4 0/7] scsi_io_completion cleanup
While working of this "[PATCH v4] Make SCSI Status CONDITION MET equivalent to GOOD" the author had trouble finding how to add another corner case check in the scsi_io_completion() function (scsi_lib.c). That function is executed at the completion of every SCSI command but only 20 or so of its hundred of lines of code are executed for the vast majority of invocations (i.e. the fastpath). This patch refactors scsi_io_completion() by taking the bulk of its error processing code into three static helper functions, leaving only the 20 or so code lines that constitute the fastpath. In this process comments were added and tweaked, plus variables renamed. The last two patches in this set are optional: adding conditional hints along the fastpath and converting 3 BUG() calls in scsi_io_completion() to WARNs as requested by a reviewer. There is no attempt in this patchset to change the logic of the original scsi_io_completion(). Some conditional checks are saved by reducing the number of redundant tests (e.g. on the 'result' variable being non-zero). Also De Morgan's laws are applied to some complex conditions to simplify them from the reader's perspective. The fact remains, this is a very complex function. This patch is against Martin Petersen's 4.17/scsi-queue branch. Note: to apply this patchset to the lk 4.15 production kernels (tested on lk 4.15.13), these two lines near the end of scsi_io_completion(): __scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY, 0); and __scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY, 0); need that trailing 0 changed to 'false', prior to applying 4/7 . Douglas Gilbert (7): scsi_io_completion comment on end_request return scsi_io_completion rename variables scsi_io_completion_nz_result function added scsi_io_completion_action helper added scsi_io_completion_reprep helper added scsi_io_completion hints on fastpatch scsi_io_completion convert BUGs to WARNs drivers/scsi/scsi_lib.c | 378 +++- 1 file changed, 214 insertions(+), 164 deletions(-) -- 2.14.1
[PATCH v4 2/7] scsi_io_completion rename variables
Change some variable names and associated comments for clarity. Correct
some misleading comments.
Signed-off-by: Douglas Gilbert
Reviewed-by: Johannes Thumshirn
Reviewed-by: Bart Van Assche
---
drivers/scsi/scsi_lib.c | 57 ++---
1 file changed, 30 insertions(+), 27 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index d44ee84df091..598836804745 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -777,18 +777,19 @@ static blk_status_t __scsi_error_from_host_byte(struct
scsi_cmnd *cmd,
*be put back on the queue and retried using the same
*command as before, possibly after a delay.
*
- * c) We can call scsi_end_request() with -EIO to fail
- *the remainder of the request.
+ * c) We can call scsi_end_request() with blk_stat other than
+ *BLK_STS_OK, to fail the remainder of the request.
*/
void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
{
int result = cmd->result;
struct request_queue *q = cmd->device->request_queue;
struct request *req = cmd->request;
- blk_status_t error = BLK_STS_OK;
+ blk_status_t blk_stat = BLK_STS_OK;
struct scsi_sense_hdr sshdr;
bool sense_valid = false;
- int sense_deferred = 0, level = 0;
+ bool sense_current = true; /* false implies "deferred sense" */
+ int level = 0;
enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
ACTION_DELAYED_RETRY} action;
unsigned long wait_for = (cmd->allowed + 1) * req->timeout;
@@ -796,7 +797,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int
good_bytes)
if (result) {
sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
if (sense_valid)
- sense_deferred = scsi_sense_is_deferred(&sshdr);
+ sense_current = !scsi_sense_is_deferred(&sshdr);
}
if (blk_rq_is_passthrough(req)) {
@@ -809,8 +810,9 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int
good_bytes)
min(8 + cmd->sense_buffer[7],
SCSI_SENSE_BUFFERSIZE);
}
- if (!sense_deferred)
- error = __scsi_error_from_host_byte(cmd,
result);
+ if (sense_current)
+ blk_stat = __scsi_error_from_host_byte(cmd,
+ result);
}
/*
* __scsi_error_from_host_byte may have reset the host_byte
@@ -829,13 +831,13 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
BUG();
return;
}
- } else if (blk_rq_bytes(req) == 0 && result && !sense_deferred) {
+ } else if (blk_rq_bytes(req) == 0 && result && sense_current) {
/*
* Flush commands do not transfers any data, and thus cannot use
* good_bytes != blk_rq_bytes(req) as the signal for an error.
-* This sets the error explicitly for the problem case.
+* This sets blk_stat explicitly for the problem case.
*/
- error = __scsi_error_from_host_byte(cmd, result);
+ blk_stat = __scsi_error_from_host_byte(cmd, result);
}
/* no bidi support for !blk_rq_is_passthrough yet */
@@ -865,8 +867,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int
good_bytes)
else if (!(req->rq_flags & RQF_QUIET))
scsi_print_sense(cmd);
result = 0;
- /* for passthrough error may be set */
- error = BLK_STS_OK;
+ /* for passthrough, blk_stat may be set */
+ blk_stat = BLK_STS_OK;
}
/*
* Another corner case: the SCSI status byte is non-zero but 'good'.
@@ -877,23 +879,24 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
*/
if (status_byte(result) && scsi_status_is_good(result)) {
result = 0;
- error = BLK_STS_OK;
+ blk_stat = BLK_STS_OK;
}
/*
-* special case: failed zero length commands always need to
-* drop down into the retry code. Otherwise, if we finished
-* all bytes in the request we are done now.
+* Next deal with any sectors which we were able to correctly
+* handle. Failed, zero length commands always need to drop down
+* to retry code. Fast path should return in this block.
*/
- if (!(blk_rq_bytes(req) == 0 && error) &&
- !scsi_end_request(req, error, good_bytes, 0))
-
[PATCH v4 1/7] scsi_io_completion comment on end_request return
scsi_end_request() is called multiple times from scsi_io_completion()
which branches on its bool returned value. Add comment before the static
definition of scsi_end_request() about the meaning of that return.
Signed-off-by: Douglas Gilbert
Reviewed-by: Johannes Thumshirn
Reviewed-by: Bart Van Assche
---
The reader might think that if scsi_end_request() had actually ended the
request (i.e. no bytes remaining) it would return true. If so, the reader
would be misled.
drivers/scsi/scsi_lib.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 393f9db8f41b..d44ee84df091 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -662,6 +662,7 @@ static void scsi_release_bidi_buffers(struct scsi_cmnd *cmd)
cmd->request->next_rq->special = NULL;
}
+/* Returns false when no more bytes to process, true if there are more */
static bool scsi_end_request(struct request *req, blk_status_t error,
unsigned int bytes, unsigned int bidi_bytes)
{
--
2.14.1
[PATCH][scsi-next] scsi: core: remove redundant assignment to shost->use_blk_mq
From: Colin Ian King
The first assignment to shost->use_blk_mq is redundant as it is
overwritten by the following statement. Remove this redundant code.
Detected by CoverityScan, CID#1466993 ("Unused value")
Signed-off-by: Colin Ian King
---
drivers/scsi/hosts.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 7649d63a1b8d..3771e59a9fae 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -472,7 +472,6 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template
*sht, int privsize)
else
shost->dma_boundary = 0x;
- shost->use_blk_mq = scsi_use_blk_mq;
shost->use_blk_mq = scsi_use_blk_mq || shost->hostt->force_blk_mq;
device_initialize(&shost->shost_gendev);
--
2.15.1
Re: [PATCH] scsi: cxgb4i: potential array overflow in t4_uld_rx_handler()
On Wed, Mar 21, 2018 at 09:12:00PM -0400, Martin K. Petersen wrote:
>
> Varun: Please look at this. Thanks!
>
> > What happened to this one?
> >
> > regards,
> > dan carpenter
> >
> >
> > On Wed, Nov 29, 2017 at 02:42:20PM +0300, Dan Carpenter wrote:
> >> The story is that Smatch marks skb->data as untrusted and so it
> >> complains about this code:
> >>
> >>drivers/scsi/cxgbi/cxgb4i/cxgb4i.c:2111 t4_uld_rx_handler()
> >>error: buffer overflow 'cxgb4i_cplhandlers' 239 <= 255.
> >>
> >> I don't know the code very well, but it looks like a reasonable warning
> >> message. Let's address it by adding a sanity check to make sure "opc"
> >> is within bounds.
> >>
> >> Fixes: bbc02c7e9d34 ("cxgb4: Add register, message, and FW definitions")
> >> Signed-off-by: Dan Carpenter
> >>
> >> diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> >> b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> >> index 266eddf17a99..94b2d5660a07 100644
> >> --- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> >> +++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> >> @@ -2108,12 +2108,12 @@ static int t4_uld_rx_handler(void *handle, const
> >> __be64 *rsp,
> >>log_debug(1 << CXGBI_DBG_TOE,
> >>"cdev %p, opcode 0x%x(0x%x,0x%x), skb %p.\n",
> >> cdev, opc, rpl->ot.opcode_tid, ntohl(rpl->ot.opcode_tid), skb);
> >> - if (cxgb4i_cplhandlers[opc])
> >> - cxgb4i_cplhandlers[opc](cdev, skb);
> >> - else {
> >> + if (opc >= ARRAY_SIZE(cxgb4i_cplhandlers) || !cxgb4i_cplhandlers[opc]) {
> >>pr_err("No handler for opcode 0x%x.\n", opc);
> >>__kfree_skb(skb);
> >> + return 0;
> >>}
> >> + cxgb4i_cplhandlers[opc](cdev, skb);
> >>return 0;
> >> nomem:
> >>log_debug(1 << CXGBI_DBG_TOE, "OOM bailing out.\n");
> >
> >
This check is not necessary but we can add it to avoid warning.
The commit mentioned in "Fixes" is not correct, this code was added in commit
"7b36b6e [SCSI] cxgb4i v5: iscsi driver"
Acked-by: Varun Prakash
Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks
On 03/28/18 08:14, bige...@linutronix.de wrote: On 2018-03-28 15:05:41 [+], Bart Van Assche wrote: The names of the two functions touched by patch 1/2 start with a double underscore. That by itself is already a hint that these should be called with a lock held (I know that this is not a universal convention in the Linux kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2 with the above comment added. Okay. In that case let me update 1/2. But 2/2 with the comment as Steven suggested is still a no no for you? Although I'm not enthusiast about patch 2/2, if others agree with that patch I'm fine with that patch being sent upstream. Thanks, Bart.
Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks
On 2018-03-28 15:05:41 [+], Bart Van Assche wrote: > The names of the two functions touched by patch 1/2 start with a double > underscore. That by itself is already a hint that these should be called with > a lock held (I know that this is not a universal convention in the Linux > kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2 > with the above comment added. Okay. In that case let me update 1/2. But 2/2 with the comment as Steven suggested is still a no no for you? > Bart. Sebastian
Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks
On Wed, 2018-03-28 at 12:15 +0200, bige...@linutronix.de wrote: > On 2018-03-26 11:13:59 [-0400], Steven Rostedt wrote: > > > diff --git a/drivers/target/target_core_tmr.c > > > b/drivers/target/target_core_tmr.c > > > index 9c7bc1ca341a..3d35dad1de2c 100644 > > > --- a/drivers/target/target_core_tmr.c > > > +++ b/drivers/target/target_core_tmr.c > > > > Can you add a comment above the functions though? > > > > /* Expects to have se_cmd->se_sess->sess_cmd_lock held */ > > I could. I haven't heard from Bart / Nicholas about their opinion. I > know, we do this other places but Bart was against this kind of > annotation in 2/2 of this thread. > So I am waiting to hear from them :) The names of the two functions touched by patch 1/2 start with a double underscore. That by itself is already a hint that these should be called with a lock held (I know that this is not a universal convention in the Linux kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2 with the above comment added. Bart.
Re: [PATCH v3 41/41] cxlflash: Handle spurious interrupts
On Mon, Mar 26, 2018 at 11:35:42AM -0500, Uma Krishnan wrote: > The following Oops can occur when there is heavy I/O traffic and the host > is reset by a tool such as sg_reset. > > [c000200fff3fbc90] c0081690117c process_cmd_doneq+0x104/0x500 >[cxlflash] (unreliable) > [c000200fff3fbd80] c00816901648 cxlflash_rrq_irq+0xd0/0x150 [cxlflash] > [c000200fff3fbde0] c0193130 __handle_irq_event_percpu+0xa0/0x310 > [c000200fff3fbea0] c01933d8 handle_irq_event_percpu+0x38/0x90 > [c000200fff3fbee0] c0193494 handle_irq_event+0x64/0xb0 > [c000200fff3fbf10] c0198ea0 handle_fasteoi_irq+0xc0/0x230 > [c000200fff3fbf40] c019182c generic_handle_irq+0x4c/0x70 > [c000200fff3fbf60] c001794c __do_irq+0x7c/0x1c0 > [c000200fff3fbf90] c002a390 call_do_irq+0x14/0x24 > [c000200e5828fab0] c0017b2c do_IRQ+0x9c/0x130 > [c000200e5828fb00] c0009b04 h_virt_irq_common+0x114/0x120 > > When a context is reset, the pending commands are flushed and the AFU > is notified. Before the AFU handles this request there could be command > completion interrupts queued to PHB which are yet to be delivered to the > context. In this scenario, a context could receive an interrupt for a > command that has been flushed, leading to a possible crash when the memory > for the flushed command is accessed. > > To resolve this problem, a boolean will indicate if the hardware queue is > ready to process interrupts or not. This can be evaluated in the interrupt > handler before proessing an interrupt. > > Signed-off-by: Uma Krishnan Acked-by: Matthew R. Ochs
Re: [PATCH v3 40/41] cxlflash: Remove commmands from pending list on timeout
On Mon, Mar 26, 2018 at 11:35:34AM -0500, Uma Krishnan wrote: > The following Oops can occur if an internal command sent to the AFU does > not complete within the timeout: > > [c00ff101b810] c00816020d94 term_mc+0xfc/0x1b0 [cxlflash] > [c00ff101b8a0] c00816020fb0 term_afu+0x168/0x280 [cxlflash] > [c00ff101b930] c008160232ec cxlflash_pci_error_detected+0x184/0x230 >[cxlflash] > [c00ff101b9e0] c0080d95d468 cxl_vphb_error_detected+0x90/0x150[cxl] > [c00ff101ba20] c0080d95f27c cxl_pci_error_detected+0xa4/0x240 [cxl] > [c00ff101bac0] c003eaf8 eeh_report_error+0xd8/0x1b0 > [c00ff101bb20] c003d0b8 eeh_pe_dev_traverse+0x98/0x170 > [c00ff101bbb0] c003f438 eeh_handle_normal_event+0x198/0x580 > [c00ff101bc60] c003fba4 eeh_handle_event+0x2a4/0x338 > [c00ff101bd10] c00400b8 eeh_event_handler+0x1f8/0x200 > [c00ff101bdc0] c013da48 kthread+0x1a8/0x1b0 > [c00ff101be30] c000b528 ret_from_kernel_thread+0x5c/0xb4 > > When an internal command times out, the command buffer is freed while it > is still in the pending commands list of the context. This corrupts the > list and when the context is cleaned up, a crash is encountered. > > To resolve this issue, when an AFU command or TMF command times out, the > command should be deleted from the hardware queue pending command list > before freeing the buffer. > > Signed-off-by: Uma Krishnan Acked-by: Matthew R. Ochs
RE: [PATCH 1/3] blk-mq: Allow PCI vector offset for mapping queues
> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of Keith Busch
> Sent: Tuesday, March 27, 2018 10:39 AM
> To: Linux NVMe ; Linux Block bl...@vger.kernel.org>
> Cc: Christoph Hellwig ; Sagi Grimberg ;
> Jianchao Wang ; Ming Lei
> ; Jens Axboe ; Keith Busch
> ; Don Brace ; qla2xxx-
> upstr...@qlogic.com; linux-scsi@vger.kernel.org
> Subject: [PATCH 1/3] blk-mq: Allow PCI vector offset for mapping queues
>
> EXTERNAL EMAIL
>
>
> The PCI interrupt vectors intended to be associated with a queue may
> not start at 0; a driver may allocate pre_vectors for special use. This
> patch adds an offset parameter so blk-mq may find the intended affinity
> mask and updates all drivers using this API accordingly.
>
> Cc: Don Brace
> Cc:
> Cc:
> Signed-off-by: Keith Busch
> ---
> v1 -> v2:
>
> Update blk-mq API directly instead of chaining a default parameter to
> a new API, and update all drivers accordingly.
>
> block/blk-mq-pci.c| 6 --
> drivers/nvme/host/pci.c | 2 +-
> drivers/scsi/qla2xxx/qla_os.c | 2 +-
> drivers/scsi/smartpqi/smartpqi_init.c | 2 +-
> include/linux/blk-mq-pci.h| 3 ++-
> 5 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/smartpqi/smartpqi_init.c
> b/drivers/scsi/smartpqi/smartpqi_init.c
> index b2880c7709e6..10c94011c8a8 100644
> --- a/drivers/scsi/smartpqi/smartpqi_init.c
> +++ b/drivers/scsi/smartpqi/smartpqi_init.c
> @@ -5348,7 +5348,7 @@ static int pqi_map_queues(struct Scsi_Host *shost)
> {
> struct pqi_ctrl_info *ctrl_info = shost_to_hba(shost);
>
> - return blk_mq_pci_map_queues(&shost->tag_set, ctrl_info->pci_dev);
> + return blk_mq_pci_map_queues(&shost->tag_set, ctrl_info->pci_dev, 0);
> }
>
Acked-by: Don Brace
Re: [PATCH v3 39/41] cxlflash: Synchronize reset and remove ops
On Mon, Mar 26, 2018 at 11:35:27AM -0500, Uma Krishnan wrote: > The following Oops can be encountered if a device removal or system > shutdown is initiated while an EEH recovery is in process: > > [c00ff2f479c0] c00815256f18 cxlflash_pci_slot_reset+0xa0/0x100 > [cxlflash] > [c00ff2f47a30] c0080dae22e0 cxl_pci_slot_reset+0x168/0x290 [cxl] > [c00ff2f47ae0] c003ef1c eeh_report_reset+0xec/0x170 > [c00ff2f47b20] c003d0b8 eeh_pe_dev_traverse+0x98/0x170 > [c00ff2f47bb0] c003f80c eeh_handle_normal_event+0x56c/0x580 > [c00ff2f47c60] c003fba4 eeh_handle_event+0x2a4/0x338 > [c00ff2f47d10] c00400b8 eeh_event_handler+0x1f8/0x200 > [c00ff2f47dc0] c013da48 kthread+0x1a8/0x1b0 > [c00ff2f47e30] c000b528 ret_from_kernel_thread+0x5c/0xb4 > > The remove handler frees AFU memory while the EEH recovery is in progress, > leading to a race condition. This can result in a crash if the recovery > thread tries to access this memory. > > To resolve this issue, the cxlflash remove handler will evaluate the > device state and yield to any active reset or probing threads. > > Signed-off-by: Uma Krishnan Looks good! Acked-by: Matthew R. Ochs
[Bug 198975] Highpoint 840A RocketRAID Controller and drives are NOT detected by SCSI_HPTIOP kernel module
https://bugzilla.kernel.org/show_bug.cgi?id=198975 --- Comment #4 from lober...@redhat.com --- On Wed, 2018-03-28 at 11:54 +, bugzilla-dae...@bugzilla.kernel.org wrote: > https://bugzilla.kernel.org/show_bug.cgi?id=198975 > > --- Comment #3 from Matthias Lippert (mae.lipp...@gmail.com) --- > Has anybody an idea if this problem is beeing worked on? > Hi Matthias Typically, the vendor (because they have knowledge of the hardware internals) will make an initial start on the upstream driver for this new card and send it to the list. It would then get improved/enhanced/critiqued by folks reading this list. Have you reached out out the Vendor yet. Rehgards Laurence -- You are receiving this mail because: You are watching the assignee of the bug.
Re: 答复: 答复: 答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
On Tue, Mar 27, 2018 at 8:15 AM, liwei (CM) wrote: > Hi, Arnd > > At present our ufs module mainly has four clocks from the outside: > hclk_ufs: main clock of ufs controller ,freq is 207.5MHz > cfg_phy_clk: configuration clock of MPHY, freq is 51.875MHz > ref_phy_clk: reference clock of MPHY from PMU, freq is 19.2MHz > ref_io_clk:reference clock for the external interface to the device, freq > is 19.2MHz > > We control two clocks "ref_io_clk" and "cfg_phy_clk" in the driver because > the other > two are controlled by main clock module and pmu. I'm not completely sure what you mean with "control" here. Do you mean setting the rate and disabling them during runtime power management? What does it mean for the clock to be controlled by teh "main clock module and pmu"? > for this patch, cfg_phy_clk corresponds to "phy_clk", ref_io_clk corresponds > to "ref_clk". I'm not sure I understand the difference between ref_phy_clk and ref_io_clk, but it sounds like we should give both of those names in the ufs-platform binding. Your hclk_ufs would appear to correspond to what qualcomm calls core_clk, so maybe use that name as well. cfg_phy_clk seems to be something that qcom would not have, but it's also generic enough to list it in the common binding. > So the clks in the patch you give appear to be unsuitable for describing this > .And the following clks of qcom are internal clock? > We didn't describe or pay attention to the clock inside the ufs module. > > PHY to controller symbol synchronization clocks: > "rx_lane0_sync_clk" - RX Lane 0 > "rx_lane1_sync_clk" - RX Lane 1 > "tx_lane0_sync_clk" - TX Lane 0 > "tx_lane1_sync_clk" - TX Lane 1 Right, let's leave those for the qcom private binding. Arnd
[Bug 198975] Highpoint 840A RocketRAID Controller and drives are NOT detected by SCSI_HPTIOP kernel module
https://bugzilla.kernel.org/show_bug.cgi?id=198975 --- Comment #3 from Matthias Lippert (mae.lipp...@gmail.com) --- Has anybody an idea if this problem is beeing worked on? -- You are receiving this mail because: You are watching the assignee of the bug.
[PATCH 2/2] scsi: ufs: Add UFS platform driver for Cadence UFS
This patch adds a device tree platform
driver for Cadence UFS Controller.
Signed-off-by: Jan Kotas
---
drivers/scsi/ufs/Kconfig |8 ++
drivers/scsi/ufs/Makefile |1 +
drivers/scsi/ufs/cdns-pltfrm.c | 148
3 files changed, 157 insertions(+), 0 deletions(-)
create mode 100644 drivers/scsi/ufs/cdns-pltfrm.c
diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
index e27b4d4..552f85a 100644
--- a/drivers/scsi/ufs/Kconfig
+++ b/drivers/scsi/ufs/Kconfig
@@ -80,6 +80,14 @@ config SCSI_UFSHCD_PLATFORM
If unsure, say N.
+config SCSI_UFS_CDNS_PLATFORM
+ tristate "Cadence UFS Controller platform driver"
+ depends on SCSI_UFSHCD_PLATFORM
+ ---help---
+ This selects the Cadence specific additions to UFSHCD platform driver.
+
+ If unsure, say N.
+
config SCSI_UFS_DWC_TC_PLATFORM
tristate "DesignWare platform support using a G210 Test Chip"
depends on SCSI_UFSHCD_PLATFORM
diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
index 9310c6c..385062c 100644
--- a/drivers/scsi/ufs/Makefile
+++ b/drivers/scsi/ufs/Makefile
@@ -3,6 +3,7 @@
obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd-dwc.o
tc-dwc-g210.o
obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o
tc-dwc-g210.o
obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
+obj-$(CONFIG_SCSI_UFS_CDNS_PLATFORM) += cdns-pltfrm.o
obj-$(CONFIG_SCSI_UFSHCD) += ufshcd.o
obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
diff --git a/drivers/scsi/ufs/cdns-pltfrm.c b/drivers/scsi/ufs/cdns-pltfrm.c
new file mode 100644
index 000..18db441
--- /dev/null
+++ b/drivers/scsi/ufs/cdns-pltfrm.c
@@ -0,0 +1,148 @@
+/*
+ * Platform UFS Host driver for Cadence controller
+ *
+ * Copyright (C) 2018 Cadence Design Systems, Inc.
+ *
+ * Authors:
+ * Jan Kotas
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include "ufshcd-pltfrm.h"
+
+#define CDNS_UFS_REG_HCLKDIV 0xFC
+
+/**
+ * Sets HCLKDIV register value based on the core_clk
+ * @hba: host controller instance
+ *
+ * Return zero for success and non-zero for failure
+ */
+static int cdns_ufs_set_hclkdiv(struct ufs_hba *hba)
+{
+ struct ufs_clk_info *clki;
+ struct list_head *head = &hba->clk_list_head;
+ unsigned long core_clk_rate = 0;
+ u32 core_clk_div = 0;
+
+ if (list_empty(head))
+ return 0;
+
+ list_for_each_entry(clki, head, list) {
+ if (IS_ERR_OR_NULL(clki->clk))
+ continue;
+ if (!strcmp(clki->name, "core_clk"))
+ core_clk_rate = clk_get_rate(clki->clk);
+ }
+
+ if (!core_clk_rate) {
+ dev_err(hba->dev, "%s: unable to find core_clk rate\n",
+ __func__);
+ return -EINVAL;
+ }
+
+ core_clk_div = core_clk_rate / USEC_PER_SEC;
+
+ ufshcd_writel(hba, core_clk_div, CDNS_UFS_REG_HCLKDIV);
+ /* Make sure the register was updated */
+ mb();
+
+ return 0;
+}
+
+/**
+ * Sets clocks used by the controller
+ * @hba: host controller instance
+ * @on: if true, enable clocks, otherwise disable
+ * @status: notify stage (pre, post change)
+ *
+ * Return zero for success and non-zero for failure
+ */
+static int cdns_ufs_setup_clocks(struct ufs_hba *hba, bool on,
+enum ufs_notify_change_status status)
+{
+ if ((!on) || (status == PRE_CHANGE))
+ return 0;
+
+ return cdns_ufs_set_hclkdiv(hba);
+}
+
+
+static struct ufs_hba_variant_ops cdns_pltfm_hba_vops = {
+ .name = "cdns-ufs-pltfm",
+ .setup_clocks = cdns_ufs_setup_clocks,
+};
+
+/**
+ * cdns_ufs_pltfrm_probe - probe routine of the driver
+ * @pdev: pointer to platform device handle
+ *
+ * Return zero for success and non-zero for failure
+ */
+static int cdns_ufs_pltfrm_probe(struct platform_device *pdev)
+{
+ int err;
+ struct device *dev = &pdev->dev;
+
+ /* Perform generic probe */
+ err = ufshcd_pltfrm_init(pdev, &cdns_pltfm_hba_vops);
+ if (err)
+ dev_err(dev, "ufshcd_pltfrm_init() failed %d\n", err);
+
+ return err;
+}
+
+/**
+ * cdns_ufs_pltfrm_remove - set driver_data of the device to NULL
+ * @pdev: pointer to platform device handle
+ *
+ * Always returns 0
+ */
+static int cdns_ufs_pltfrm_remove(struct platform_device *pdev)
+{
+ struct ufs_hba *hba = platform_get_drvdata(pdev);
+
+ pm_runtime_get_sync(&(pdev)->dev);
+ ufshcd_remove(hba);
+ return 0;
+}
+
+
+static const struct of_device_id cdns_ufs_of_match[] = {
+ { .compatible = "cdns,ufshc"},
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, cdns_ufs_of_match);
+
+static const struct dev_pm_ops cdns_ufs_dev_pm_ops = {
+ .suspend = ufshcd_pltfrm_suspend,
+ .resume = ufshcd_pltfrm_resume,
+
[PATCH 1/2] scsi: ufs: Add UFS platform driver for Cadence UFS
This patch adds a device tree platform
driver description for Cadence UFS Controller.
Signed-off-by: Jan Kotas
---
.../devicetree/bindings/ufs/cdns-ufs-pltfrm.txt| 31
1 files changed, 31 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/ufs/cdns-ufs-pltfrm.txt
diff --git a/Documentation/devicetree/bindings/ufs/cdns-ufs-pltfrm.txt
b/Documentation/devicetree/bindings/ufs/cdns-ufs-pltfrm.txt
new file mode 100644
index 000..d10229d
--- /dev/null
+++ b/Documentation/devicetree/bindings/ufs/cdns-ufs-pltfrm.txt
@@ -0,0 +1,31 @@
+* Cadence Universal Flash Storage (UFS) Controller
+
+UFS nodes are defined to describe on-chip UFS host controllers.
+Each UFS controller instance should have its own node.
+
+Required properties:
+- compatible : compatible list, contains the following controller:
+ "cdns,ufshc"
+ complemented with the JEDEC version:
+ "jedec,ufs-2.0"
+
+- reg : registers mapping
+- interrupts : interrupts mapping
+- clocks : List of phandle and clock specifier pairs.
+- clock-names : List of clock input name strings sorted in the same
+ order as the clocks property. "core_clk" is mandatory.
+- freq-table-hz: Array of operating frequencies stored in
the same
+ order as the clocks property. If this property is not
+ defined or a value in the array is "0" then it is assumed
+ that the frequency is set by the parent clock or a
+ fixed rate clock source.
+
+Example:
+ ufs@fd03 {
+ compatible = "cdns,ufshc", "jedec,ufs-2.0";
+ reg = <0xfd03 0x1>;
+ interrupts = <0 1 0>;
+ freq-table-hz = <0 0>;
+ clocks = <&ufs_core_clk 0>;
+ clock-names = "core_clk";
+ };
--
1.7.1
Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks
On 2018-03-26 11:13:59 [-0400], Steven Rostedt wrote: > > diff --git a/drivers/target/target_core_tmr.c > > b/drivers/target/target_core_tmr.c > > index 9c7bc1ca341a..3d35dad1de2c 100644 > > --- a/drivers/target/target_core_tmr.c > > +++ b/drivers/target/target_core_tmr.c > > Can you add a comment above the functions though? > > /* Expects to have se_cmd->se_sess->sess_cmd_lock held */ I could. I haven't heard from Bart / Nicholas about their opinion. I know, we do this other places but Bart was against this kind of annotation in 2/2 of this thread. So I am waiting to hear from them :) > > -- Steve Sebastian
[PATCH] scsi: fnic: fix spelling mistake: "abord" -> "abort"
From: Colin Ian King Trivial fix to spelling mistake in message text Signed-off-by: Colin Ian King --- drivers/scsi/fnic/fnic_trace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/fnic/fnic_trace.c b/drivers/scsi/fnic/fnic_trace.c index abddde11982b..98597b59c12a 100644 --- a/drivers/scsi/fnic/fnic_trace.c +++ b/drivers/scsi/fnic/fnic_trace.c @@ -296,7 +296,7 @@ int fnic_get_stats_data(struct stats_debug_info *debug, "Number of Abort FW Timeouts: %lld\n" "Number of Abort IO NOT Found: %lld\n" - "Abord issued times: \n" + "Abort issued times: \n" "< 6 sec : %lld\n" " 6 sec - 20 sec : %lld\n" "20 sec - 30 sec : %lld\n" -- 2.15.1
[PATCH v3] scsi: ufs: add trace event for ufs upiu
Add UFS Protocol Information Units(upiu) trace events for ufs driver,
used to trace various ufs transaction types- command, task-management
and device management.
The trace-point format is generic and can be easily adapted to trace
other upius if needed.
Currently tracing ufs transaction of type 'device management', which
this patch introduce, cannot be obtained from any other trace.
Device management transactions are used for communication with the
device such as reading and writing descriptor or attributes etc.
Signed-off-by: Ohad Sharabi
Reviewed-by: Stanislav Nijnikov
Reviewed-by: Bart Van Assche
---
v2->v3:
- modify args 3,4 of trace_ufshcd_upiu to be void *
v1->v2:
- split to transaction specific functions (fix warnings and simplifies
code)
- adding traces when sending query command
drivers/scsi/ufs/ufshcd.c | 40
include/trace/events/ufs.h | 27 +++
2 files changed, 67 insertions(+)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c7da2c1..23ffaed 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -274,6 +274,35 @@ static inline void ufshcd_remove_non_printable(char *val)
*val = ' ';
}
+static void ufshcd_add_cmd_upiu_trace(struct ufs_hba *hba, unsigned int tag,
+ const char *str)
+{
+ struct utp_upiu_req *rq = hba->lrb[tag].ucd_req_ptr;
+
+ trace_ufshcd_upiu(dev_name(hba->dev), str, &rq->header, &rq->sc.cdb);
+}
+
+static void ufshcd_add_query_upiu_trace(struct ufs_hba *hba, unsigned int tag,
+ const char *str)
+{
+ struct utp_upiu_req *rq = hba->lrb[tag].ucd_req_ptr;
+
+ trace_ufshcd_upiu(dev_name(hba->dev), str, &rq->header, &rq->qr);
+}
+
+static void ufshcd_add_tm_upiu_trace(struct ufs_hba *hba, unsigned int tag,
+ const char *str)
+{
+ struct utp_task_req_desc *descp;
+ struct utp_upiu_task_req *task_req;
+ int off = (int)tag - hba->nutrs;
+
+ descp = &hba->utmrdl_base_addr[off];
+ task_req = (struct utp_upiu_task_req *)descp->task_req_upiu;
+ trace_ufshcd_upiu(dev_name(hba->dev), str, &task_req->header,
+ &task_req->input_param1);
+}
+
static void ufshcd_add_command_trace(struct ufs_hba *hba,
unsigned int tag, const char *str)
{
@@ -283,6 +312,9 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba,
struct ufshcd_lrb *lrbp;
int transfer_len = -1;
+ /* trace UPIU also */
+ ufshcd_add_cmd_upiu_trace(hba, tag, str);
+
if (!trace_ufshcd_command_enabled())
return;
@@ -2584,6 +2616,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
hba->dev_cmd.complete = &wait;
+ ufshcd_add_query_upiu_trace(hba, tag, "query_send");
/* Make sure descriptors are ready before ringing the doorbell */
wmb();
spin_lock_irqsave(hba->host->host_lock, flags);
@@ -2593,6 +2626,9 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
+ ufshcd_add_query_upiu_trace(hba, tag,
+ err ? "query_complete_err" : "query_complete");
+
out_put_tag:
ufshcd_put_dev_cmd_tag(hba, tag);
wake_up(&hba->dev_cmd.tag_wq);
@@ -5464,11 +5500,14 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int
lun_id, int task_id,
spin_unlock_irqrestore(host->host_lock, flags);
+ ufshcd_add_tm_upiu_trace(hba, task_tag, "tm_send");
+
/* wait until the task management command is completed */
err = wait_event_timeout(hba->tm_wq,
test_bit(free_slot, &hba->tm_condition),
msecs_to_jiffies(TM_CMD_TIMEOUT));
if (!err) {
+ ufshcd_add_tm_upiu_trace(hba, task_tag, "tm_complete_err");
dev_err(hba->dev, "%s: task management cmd 0x%.2x timed-out\n",
__func__, tm_function);
if (ufshcd_clear_tm_cmd(hba, free_slot))
@@ -5477,6 +5516,7 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int
lun_id, int task_id,
err = -ETIMEDOUT;
} else {
err = ufshcd_task_req_compl(hba, free_slot, tm_response);
+ ufshcd_add_tm_upiu_trace(hba, task_tag, "tm_complete");
}
clear_bit(free_slot, &hba->tm_condition);
diff --git a/include/trace/events/ufs.h b/include/trace/events/ufs.h
index bf6f826..f8260e5 100644
--- a/include/trace/events/ufs.h
+++ b/include/trace/events/ufs.h
@@ -257,6 +257,33 @@
)
);
+TRACE_EVENT(ufshcd_upiu,
+ TP_PROTO(const char *dev_name, const char *str, void *hdr, void *tsf),
+
+ TP_ARGS(dev_name, str, hdr, tsf),
+
+ TP_STRUCT__entry(
+ __string(dev_name, dev_name)
+ __string(str, str)
+ __array(unsigned char, hdr, 12)
+ __array(unsigned c
[PATCH] scsi: Introduce sdev_printk_ratelimited to throttle frequent printk
There would be so many same lines printed by frequent prink if one
disk went wrong, like,
[ 546.185242] sd 0:1:0:0: rejecting I/O to offline device
[ 546.185258] sd 0:1:0:0: rejecting I/O to offline device
[ 546.185280] sd 0:1:0:0: rejecting I/O to offline device
[ 546.185307] sd 0:1:0:0: rejecting I/O to offline device
[ 546.185334] sd 0:1:0:0: rejecting I/O to offline device
[ 546.185364] sd 0:1:0:0: rejecting I/O to offline device
[ 546.185390] sd 0:1:0:0: rejecting I/O to offline device
[ 546.185410] sd 0:1:0:0: rejecting I/O to offline device
For slow serial console, the frequent prink may block the printk, and
if spin_lock was acquired before the printk like in scsi_request_fn,
that could trigger the watchdog.
Relative disscussion can be found here,
https://bugzilla.kernel.org/show_bug.cgi?id=199003
And Petr brought the idea to throttle the frequent printk here, it's
useless to print the same lines frequently after all.
Suggested-by: Petr Mladek
Suggested-by: Sergey Senozhatsky
Signed-off-by: Wen Yang
Signed-off-by: Jiang Biao
Signed-off-by: Tan Hu
CC: BartVanAssche
CC: Petr Mladek
CC: Sergey Senozhatsky
CC: Martin K. Petersen
CC: "James E.J. Bottomley"
CC: Tejun Heo
---
drivers/scsi/scsi_lib.c| 6 +++---
include/scsi/scsi_device.h | 10 ++
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c84f931..f77e801 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1301,7 +1301,7 @@ static int scsi_setup_cmnd(struct scsi_device *sdev,
struct request *req)
* commands. The device must be brought online
* before trying any recovery commands.
*/
- sdev_printk(KERN_ERR, sdev,
+ sdev_printk_ratelimited(KERN_ERR, sdev,
"rejecting I/O to offline device\n");
ret = BLKPREP_KILL;
break;
@@ -1310,7 +1310,7 @@ static int scsi_setup_cmnd(struct scsi_device *sdev,
struct request *req)
* If the device is fully deleted, we refuse to
* process any commands as well.
*/
- sdev_printk(KERN_ERR, sdev,
+ sdev_printk_ratelimited(KERN_ERR, sdev,
"rejecting I/O to dead device\n");
ret = BLKPREP_KILL;
break;
@@ -1802,7 +1802,7 @@ static void scsi_request_fn(struct request_queue *q)
break;
if (unlikely(!scsi_device_online(sdev))) {
- sdev_printk(KERN_ERR, sdev,
+ sdev_printk_ratelimited(KERN_ERR, sdev,
"rejecting I/O to offline device\n");
scsi_kill_request(req, q);
continue;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 7ae177c..378d3f2 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -249,6 +249,16 @@ struct scsi_device {
#define sdev_printk(l, sdev, fmt, a...)\
sdev_prefix_printk(l, sdev, NULL, fmt, ##a)
+#define sdev_printk_ratelimited(l, sdev, fmt, a...)\
+({ \
+ static DEFINE_RATELIMIT_STATE(_rs, \
+ DEFAULT_RATELIMIT_INTERVAL, \
+ DEFAULT_RATELIMIT_BURST); \
+ \
+ if (__ratelimit(&_rs)) \
+ sdev_prefix_printk(l, sdev, NULL, fmt, ##a);\
+})
+
__printf(3, 4) void
scmd_printk(const char *, const struct scsi_cmnd *, const char *, ...);
--
1.8.3.1
RE: [PATCH net-next] qed*: Utilize FW 8.33.11.0
> From: Jason Gunthorpe [mailto:j...@ziepe.ca]
> Sent: Wednesday, March 28, 2018 1:27 AM
>
> On Tue, Mar 27, 2018 at 08:50:24PM +0300, Leon Romanovsky wrote:
> > On Tue, Mar 27, 2018 at 05:41:51PM +, Kalderon, Michal wrote:
> > > > From: Jason Gunthorpe [mailto:j...@ziepe.ca]
> > > > Sent: Tuesday, March 27, 2018 12:18 AM
> > > > > diff --git a/drivers/infiniband/hw/qedr/main.c
> > > > > b/drivers/infiniband/hw/qedr/main.c
> > > > > index db4bf97..7dbbe6d 100644
> > > > > +++ b/drivers/infiniband/hw/qedr/main.c
> > > > > @@ -51,6 +51,7 @@
> > > > > MODULE_DESCRIPTION("QLogic 40G/100G ROCE Driver");
> > > > > MODULE_AUTHOR("QLogic Corporation"); MODULE_LICENSE("Dual
> > > > BSD/GPL");
> > > > > +MODULE_VERSION(QEDR_MODULE_VERSION);
> > > > >
> > > > > #define QEDR_WQ_MULTIPLIER_DFT (3)
> > > > >
> > > > > diff --git a/drivers/infiniband/hw/qedr/qedr.h
> > > > > b/drivers/infiniband/hw/qedr/qedr.h
> > > > > index 86d4511..ab0d411 100644
> > > > > +++ b/drivers/infiniband/hw/qedr/qedr.h
> > > > > @@ -43,6 +43,8 @@
> > > > > #include "qedr_hsi_rdma.h"
> > > > >
> > > > > #define QEDR_NODE_DESC "QLogic 579xx RoCE HCA"
> > > > > +#define QEDR_MODULE_VERSION "8.33.11.20"
> > > > > +
> > > >
> > > > I thought we had a general prohibition against versions like this
> > > > in mainline drivers? And what does this hunk have to do with
> > > > supporting new firmware?
> > > >
> > > I'm assuming you refer only to rdma in regards to version
> > > prohibition right ? as looking at all other vendors (including
> > > Mellanox) all have module versions under net/ why is rdma different
> > > in this way ? I now searched back mails on the topic and found an
> > > email from Leon where he stated: " I am strongly against module
> > > versions. You should rely on official kernel version." But it's not
> > > always the inbox driver that is installed or probed, the kernel
> > > version is not enough. Given different distros, vanilla kernels,
> > > out of box drivers, etc... it is essential for us that based on logs
> > > And modinfo we can determine the qed* drivers that are running.
> >
> > We actually stopped to maintain driver versions, just ensure that
> > inbox, upstream and MLNX_OFED have different names.
> >
> > The discussion thread is here
> > https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-June/
> > 004426.html
> > https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-June/
> > 004441.html
>
> Hmm, Linus pretty clearly said No to MODULE_VERSION and related.
>
> So I can't take this hunk, and you shouldn't do in ethernet either, I guess.
>
> Honestly the idea that this version will somehow have meaning in the distro
> kernels is pretty far fetched. You think distros will backport patches
> changing
> version # in any way that will make some kind of sense?
Leon and Jason, thanks for the references and explanations.
I'll send a V2 without a version bump.
Michal
>
> Jason
Re: [PATCH v3 5/7] scsi_io_completion_reprep helper added
Looks good, Reviewed-by: Johannes Thumshirn -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
