re: scsi_debug: support scsi-mq, queues and locks
[ This is not really a new bug, it's just that renaming the function
made it show up as a new bug and I figured maybe you know what's going
on since you are working with related code. -dan ]
Hello Douglas Gilbert,
This is a semi-automatic email about new static checker warnings.
The patch cbf67842c3d9: scsi_debug: support scsi-mq, queues and
locks from Jul 26, 2014, leads to the following Smatch complaint:
drivers/scsi/scsi_debug.c:4153 scsi_debug_queuecommand()
error: we previously assumed 'cmd' could be null (see line 4106)
drivers/scsi/scsi_debug.c
4105 if ((SCSI_DEBUG_OPT_NOISE scsi_debug_opts)
4106 !(SCSI_DEBUG_OPT_NO_CDB_NOISE scsi_debug_opts) cmd) {
^^^
Check.
4107 char b[120];
4108 int n;
4109
4110 len = SCpnt-cmd_len;
4111 if (len 32)
4112 strcpy(b, too long, over 32 bytes);
4113 else {
4114 for (k = 0, n = 0; k len; ++k)
4115 n += scnprintf(b + n, sizeof(b) - n,
%02x ,
4116 (unsigned int)cmd[k]);
4117 }
4118 sdev_printk(KERN_INFO, SCpnt-device, %s: cmd %s\n,
my_name,
4119 b);
4120 }
4121
4122 if ((SCpnt-device-lun = scsi_debug_max_luns)
4123 (SCpnt-device-lun != SAM2_WLUN_REPORT_LUNS))
4124 return schedule_resp(SCpnt, NULL, DID_NO_CONNECT 16,
0);
4125 devip = devInfoReg(SCpnt-device);
4126 if (NULL == devip)
4127 return schedule_resp(SCpnt, NULL, DID_NO_CONNECT 16,
0);
4128
4129 if ((scsi_debug_every_nth != 0)
4130 (atomic_inc_return(sdebug_cmnd_count) =
4131 abs(scsi_debug_every_nth))) {
4132 atomic_set(sdebug_cmnd_count, 0);
4133 if (scsi_debug_every_nth -1)
4134 scsi_debug_every_nth = -1;
4135 if (SCSI_DEBUG_OPT_TIMEOUT scsi_debug_opts)
4136 return 0; /* ignore command causing timeout */
4137 else if (SCSI_DEBUG_OPT_MAC_TIMEOUT scsi_debug_opts
4138 scsi_medium_access_command(SCpnt))
4139 return 0; /* time out reads and writes */
4140 else if (SCSI_DEBUG_OPT_RECOVERED_ERR scsi_debug_opts)
4141 inj_recovered = 1; /* to reads and writes below
*/
4142 else if (SCSI_DEBUG_OPT_TRANSPORT_ERR scsi_debug_opts)
4143 inj_transport = 1; /* to reads and writes below
*/
4144 else if (SCSI_DEBUG_OPT_DIF_ERR scsi_debug_opts)
4145 inj_dif = 1; /* to reads and writes below */
4146 else if (SCSI_DEBUG_OPT_DIX_ERR scsi_debug_opts)
4147 inj_dix = 1; /* to reads and writes below */
4148 else if (SCSI_DEBUG_OPT_SHORT_TRANSFER
scsi_debug_opts)
4149 inj_short = 1;
4150 }
4151
4152 if (devip-wlun) {
4153 switch (*cmd) {
Unchecked dereference.
4154 case INQUIRY:
4155 case REQUEST_SENSE:
regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: scsi_debug: support scsi-mq, queues and locks
On Thu, 2014-07-31 at 12:10 +0300, Dan Carpenter wrote:
[ This is not really a new bug, it's just that renaming the function
made it show up as a new bug and I figured maybe you know what's going
on since you are working with related code. -dan ]
Hello Douglas Gilbert,
This is a semi-automatic email about new static checker warnings.
The patch cbf67842c3d9: scsi_debug: support scsi-mq, queues and
locks from Jul 26, 2014, leads to the following Smatch complaint:
drivers/scsi/scsi_debug.c:4153 scsi_debug_queuecommand()
error: we previously assumed 'cmd' could be null (see line 4106)
drivers/scsi/scsi_debug.c
4105if ((SCSI_DEBUG_OPT_NOISE scsi_debug_opts)
4106!(SCSI_DEBUG_OPT_NO_CDB_NOISE scsi_debug_opts)
cmd) {
^^^
Check.
This check is bogus. cmd comes from
int scsi_debug_queuecommand_lck(struct scsi_cmnd *SCpnt, done_funct_t
done)
{
unsigned char *cmd = (unsigned char *) SCpnt-cmnd;
which can never be NULL (cast is pointless as well, it's already an
unsigned char *).
James
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/3] pm8001: Update MAINTAINERS list
On Wed, Jul 30, 2014 at 5:42 PM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Wed, 2014-07-30 at 17:37 +0530, Suresh Thiagarajan wrote: From: Suresh Thiagarajan suresh.thiagara...@pmcs.com Update pmcs mail list for pm8001 driver support Signed-off-by: Suresh Thiagarajan suresh.thiagara...@pmcs.com --- MAINTAINERS |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 3f2e171..a63259c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6987,6 +6987,7 @@ F: drivers/scsi/pmcraid.* PMC SIERRA PM8001 DRIVER M: xjtu...@gmail.com M: lindar_...@usish.com +L: pmc...@pmcs.com This list doesn't appear to work: This is fixed now. My apologies for the inconvenience. -Suresh pmc...@pmcs.com: host smtp.pmc-sierra.com[216.241.235.148] said: 550 5.1.1 pmc...@pmcs.com: Recipient address rejected: User unknown in local recipient table (in reply to RCPT TO command) James -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: WARNING: CPU: 1 PID: 495 at mm/slab_common.c:69 kmem_cache_create+0x1a9/0x330()
On Wed, 2014-07-30 at 19:59 +0200, Christoph Hellwig wrote: On Wed, Jul 30, 2014 at 12:22:11PM -0500, Mike Christie wrote: Some drivers like qla2xxx do not set proc_name. I think if 2 drivers like that are loaded then you will hit some other warns/bugs in the kmem cache setup code right? Drivers have to opt into using their own caches by setting .cmd_size in the host template. We could enforce they set -proc_name for them, but I'd rather keep the amount of sanity checks low unless there's a real need. OK, so lets send this through normal merge window then we get time to see if there's a problem before the backport to stable happens. James -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] pm8001: Update MAINTAINERS list
If it works, then I'm fine with this, thanks. Please add my ack if needed: Acked-by: Jack Wang xjtu...@gmail.com 2014-07-31 14:53 GMT+02:00 Suresh Thiagarajan suresh.thiagara...@pmcs.com: On Wed, Jul 30, 2014 at 5:42 PM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Wed, 2014-07-30 at 17:37 +0530, Suresh Thiagarajan wrote: From: Suresh Thiagarajan suresh.thiagara...@pmcs.com Update pmcs mail list for pm8001 driver support Signed-off-by: Suresh Thiagarajan suresh.thiagara...@pmcs.com --- MAINTAINERS |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 3f2e171..a63259c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6987,6 +6987,7 @@ F: drivers/scsi/pmcraid.* PMC SIERRA PM8001 DRIVER M: xjtu...@gmail.com M: lindar_...@usish.com +L: pmc...@pmcs.com This list doesn't appear to work: This is fixed now. My apologies for the inconvenience. -Suresh pmc...@pmcs.com: host smtp.pmc-sierra.com[216.241.235.148] said: 550 5.1.1 pmc...@pmcs.com: Recipient address rejected: User unknown in local recipient table (in reply to RCPT TO command) James -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL] SCSI fixes for 3.16-rc7
This is a potential data corruption fix: If we get an error sending down
a barrier, we simply ignore it meaning the barrier semantics get
violated without anyone being any the wiser. If the system crashes at
this point, the filesystem potentially becomes corrupt. Fix is to
report errors on failed barriers.
The patch is available here:
git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git scsi-fixes
The short changelog is:
James Bottomley (1):
scsi: handle flush errors properly
And the diffstat:
drivers/scsi/scsi_lib.c | 8
1 file changed, 8 insertions(+)
Full diff below.
James
---
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f7e3163..3f50dfc 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -733,6 +733,14 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
scsi_next_command(cmd);
return;
}
+ } else if (blk_rq_bytes(req) == 0 result !sense_deferred) {
+ /*
+* Certain non BLOCK_PC requests are commands that don't
+* actually transfer anything (FLUSH), so cannot use
+* good_bytes != blk_rq_bytes(req) as the signal for an error.
+* This sets the error explicitly for the problem case.
+*/
+ error = __scsi_error_from_host_byte(cmd, result);
}
/* no bidi support for !REQ_TYPE_BLOCK_PC yet */
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: hpsa.c: Cleaning up code clarification using strlcpy
On Wed, Jul 30, 2014 at 11:46:52PM +0200, Rickard Strandqvist wrote:
Code clarification using strlcpy instead of strncpy.
And removed unnecessary memset
Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se
---
drivers/scsi/hpsa.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 31184b3..814d64d 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -315,16 +315,15 @@ static ssize_t
host_store_hp_ssd_smart_path_status(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
- int status, len;
+ int status;
struct ctlr_info *h;
struct Scsi_Host *shost = class_to_shost(dev);
char tmpbuf[10];
if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
return -EACCES;
- len = count sizeof(tmpbuf) - 1 ? sizeof(tmpbuf) - 1 : count;
- strncpy(tmpbuf, buf, len);
- tmpbuf[len] = '\0';
+ strlcpy(tmpbuf, buf,
+ count sizeof(tmpbuf) ? sizeof(tmpbuf) : count);
Are we doing the strlcpy thing? Linus and GregKH didn't seem to like this
kind of stuff all that much in some google+ comments I saw recently.
https://plus.google.com/111049168280159033135/posts/1amLbuhWbh5
... strlcpy requires the source to be 0 terminated, even if
its longer than the target size.
which I don't know that we really want to rely on buf being
null terminated here.
Part of Linus's comment was:
If you're actually copying from user space in the kernel, do
ret = strncpy_from_user(buf, userptr, len);
if (ret 0) return ret;
if (ret == len) return -ETOOLONG;
and you're ok (and 'ret' will be the length of the properly
NUL-terminated string).
Don't use strncpy(), don't use strlcpy().
if (sscanf(tmpbuf, %d, status) != 1)
return -EINVAL;
h = shost_to_hba(shost);
@@ -339,16 +338,15 @@ static ssize_t host_store_raid_offload_debug(struct
device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
- int debug_level, len;
+ int debug_level;
struct ctlr_info *h;
struct Scsi_Host *shost = class_to_shost(dev);
char tmpbuf[10];
if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
return -EACCES;
- len = count sizeof(tmpbuf) - 1 ? sizeof(tmpbuf) - 1 : count;
- strncpy(tmpbuf, buf, len);
- tmpbuf[len] = '\0';
+ strlcpy(tmpbuf, buf,
+ count sizeof(tmpbuf) ? sizeof(tmpbuf) : count);
if (sscanf(tmpbuf, %d, debug_level) != 1)
return -EINVAL;
if (debug_level 0)
@@ -5881,8 +5879,8 @@ static int hpsa_controller_hard_reset(struct pci_dev
*pdev,
static void init_driver_version(char *driver_version, int len)
{
- memset(driver_version, 0, len);
strncpy(driver_version, HPSA HPSA_DRIVER_VERSION, len - 1);
+ driver_version[len - 1] = '\0';
So this depends on strncpy actually putting those zeros on the end of that
string so as not to leak a partially uninitialized kernel buffer out into a pci
config space register on a device that could potentially get read later
from user space. (using kzalloc for that particular buffer could alleviate
that dependency easily enough though) Plenty of places that are using strncpy
probably do not care whether those zeroes are padded on, but this one does
care, but that is not signaled to the reader of the code in any way here.
(a comment indicating the zero padding is actually wanted would suffice.)
None of this stuff is in performance critical paths.
-- steve
}
static int write_driver_ver_to_cfgtable(struct CfgTable __iomem *cfgtable)
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] hpsa: work in progress lockless monster patches
Hi Steve, Webb,
let me start with the part most important for me - from my previous mail
And please have a look at [PATCH] hpsa: refine the pci enble/disable handling
I posted in June and add it to your series or review in the list. Thanks.
other comments see below
Hi Tomas,
Thanks for taking a look and for the feedback. I'm actually the one
responsible for the changes you refer to, so I'll try to address your
comments.
On 7/30/14 10:55 AM, Tomas Henzl wrote:
I'm not sure if I got it right,
it seems it uses the same cmd_pool for both alloc function,
from (0 - reserved_cmds) it's cmd_alloc and above that it's handled
by cmd_tagged_alloc.
The logic in both functions seems to be valid for me.
Good. With Christoph's proposed changes, the block layer will select a
tag for I/O requests, and it provides for the driver to reserve part of
the tag space for its own use. Thus, HPSA uses a common pool for both
I/O requests and internally-driven requests (IOCTLs, aborts, etc.), and
we index the pool using the tag. The code is this way mostly to
minimize change from the previous version, but it's also handy in terms
of managing the pool (i.e., there is a single pool to administer,
instead of two, and all the items are initialized the same way, etc.).
The portion of the pool which is reserved to the driver (the
low-numbered tags) continues to be managed through a bitmap, just as it
was prior to this change; the bitmap does cover the whole pool (which is
a historical artifact), but the higher bits are never set, since
allocation of those entries is determined by the tag from the block
layer -- nevertheless, it is convenient to have the map extend over the
whole pool in certain corner cases (such as resets).
In cmd_tagged_alloc Thus, there should never be a collision here between
two requests if this is true you don't need the refcount and just a simple
flag were sufficient for your other needs. (And maybe you get to ~971k
IOPS..)
:-) The code previously had the reference count in there to close
certain race conditions' asynchronous accesses to the command block
(e.g., between an abort and a completing command). We hope that, using
the block layer tags, those races no longer occur, but there didn't seem
to be any point in removing the reference count until we'd had more
experience with the code...and, in a fit of healthy paranoia I added the
check to which you refer. Unfortunately, in some of Rob Elliott's
recent tests, he managed to drive a case where the check triggers. So,
until we reconcile that, I'm inclined to leave the check in place
(hopefully it's not costing a full 1k IOPS :-) ).
Let us assume that the block layer never sends duplicate tags to the driver,
I think there are some weaknesses in the way how it's implemented,
for example here - from fail_all_outstanding_cmds:
refcount = atomic_inc_return(c-refcount);
if (refcount 1) {
...
finish_cmd(c); - this finishes the command
and the tag might be now reused,
when that happens you'll see a race
atomic_dec(h-commands_outstanding);
failcount++;
}
else {what happens when right now comes a call from block
layer?}
cmd_free(h, c);
When references are used it usually means, that when refcount==0 that
a release function frees the resources, in this case it is the tag number.
In your implementation you should ensure that when you signal
to the upper layer that the tag is free, that nothing else holds the reference.
If this is true the conclusion is that you don't need this kind of references
and a simple flag just to debug not yet fixed races is enough.
I'm maybe wrong because I don't understand what you want to protect
in the above example, so that makes me to have some doubts if I understand
the code properly.
cmd_alloc some minor changes are possible
- try to find free entries only to reserved_cmds
(the bitmap might be shorter too)
Excellent catch! I'll make that change. (As I said, we currently rely
on the over-sized bit map, but there is no reason for this code to
search the whole thing...although, with the block layer doing the heavy
lifting, this allocation path is lightly used, and it will be rare that
it runs off the end of the reserved range.)
- next thread should start + 1 from the last result
That's a fair point, but the suggestion is somewhat more complicated
than it seems: if start + 1 is greater than reserved_cmds, then we
want to start at zero, instead. I think it ends up being more code than
it's worth.
In fact, with the partitioned pool, the reserved-to-driver section is
now so small that it's probably not worth the effort to try to resume
the search where the last search left off -- we
Re: [RFC] hpsa: work in progress lockless monster patches
On Thu, Jul 31, 2014 at 03:56:13PM +0200, Tomas Henzl wrote:
Hi Steve, Webb,
let me start with the part most important for me - from my previous mail
And please have a look at [PATCH] hpsa: refine the pci enble/disable
handling
I posted in June and add it to your series or review in the list. Thanks.
Ok. I will try to get to this.
(some customer issues have been eating a lot of my time this week).
When I first saw that patch I knew that Rob Elliott had a lot of patches in the
works that did a lot of cleanup to the initialization code, so I expect some
conflicts.
other comments see below
Hi Tomas,
Thanks for taking a look and for the feedback. I'm actually the one
responsible for the changes you refer to, so I'll try to address your
comments.
On 7/30/14 10:55 AM, Tomas Henzl wrote:
I'm not sure if I got it right,
it seems it uses the same cmd_pool for both alloc function,
from (0 - reserved_cmds) it's cmd_alloc and above that it's handled
by cmd_tagged_alloc.
The logic in both functions seems to be valid for me.
Good. With Christoph's proposed changes, the block layer will select a
tag for I/O requests, and it provides for the driver to reserve part of
the tag space for its own use. Thus, HPSA uses a common pool for both
I/O requests and internally-driven requests (IOCTLs, aborts, etc.), and
we index the pool using the tag. The code is this way mostly to
minimize change from the previous version, but it's also handy in terms
of managing the pool (i.e., there is a single pool to administer,
instead of two, and all the items are initialized the same way, etc.).
The portion of the pool which is reserved to the driver (the
low-numbered tags) continues to be managed through a bitmap, just as it
was prior to this change; the bitmap does cover the whole pool (which is
a historical artifact), but the higher bits are never set, since
allocation of those entries is determined by the tag from the block
layer -- nevertheless, it is convenient to have the map extend over the
whole pool in certain corner cases (such as resets).
In cmd_tagged_alloc Thus, there should never be a collision here between
two requests if this is true you don't need the refcount and just a simple
flag were sufficient for your other needs. (And maybe you get to ~971k
IOPS..)
:-) The code previously had the reference count in there to close
certain race conditions' asynchronous accesses to the command block
(e.g., between an abort and a completing command). We hope that, using
the block layer tags, those races no longer occur, but there didn't seem
to be any point in removing the reference count until we'd had more
experience with the code...and, in a fit of healthy paranoia I added the
check to which you refer. Unfortunately, in some of Rob Elliott's
recent tests, he managed to drive a case where the check triggers. So,
until we reconcile that, I'm inclined to leave the check in place
(hopefully it's not costing a full 1k IOPS :-) ).
Let us assume that the block layer never sends duplicate tags to the driver,
I think there are some weaknesses in the way how it's implemented,
for example here - from fail_all_outstanding_cmds:
refcount = atomic_inc_return(c-refcount);
if (refcount 1) {
...
finish_cmd(c); - this finishes the command
and the tag might be now reused,
when that happens you'll see a race
atomic_dec(h-commands_outstanding);
failcount++;
}
else {what happens when right now comes a call from block
layer?}
cmd_free(h, c);
When references are used it usually means, that when refcount==0 that
a release function frees the resources, in this case it is the tag number.
In your implementation you should ensure that when you signal
to the upper layer that the tag is free, that nothing else holds the
reference.
If this is true the conclusion is that you don't need this kind of references
and a simple flag just to debug not yet fixed races is enough.
Part of the motivation is we want to have this code be workable for distros
that may not have the necessary kernel changes for us to be able to use the
block layer tagging (e.g. tags reserved for driver use). So I am floating
the block layer tag patches, keeping them at the top of my stack of patches
so that all this lockless stuff can still work even without using the block
layer tags. So one reason the reference counting needs to go in is to
accomodate backporting the LLD without touching an older kernel's mid layer
or block layer code, which is important to me for distros, because I can
easily change out the LLD as a module, but scsi mid layer or block layer of
a distro cannot be touched.
Re: [PATCH -next] scsi: fix u14-34f printk format warnings
any comments? Thanks. On 07/24/14 11:07, Randy Dunlap wrote: From: Randy Dunlap rdun...@infradead.org Fix printk format warnings (seen on i386 builds): ../drivers/scsi/u14-34f.c: In function 'port_detect': ../drivers/scsi/u14-34f.c:630:28: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'u64' [-Wformat=] ../drivers/scsi/u14-34f.c: In function 'u14_34f_queuecommand_lck': ../drivers/scsi/u14-34f.c:1290:25: warning: format '%llu' expects argument of type 'long long unsigned int', but argument 6 has type 'int' [-Wformat=] Signed-off-by: Randy Dunlap rdun...@infradead.org Cc: James E.J. Bottomley jbottom...@parallels.com Cc: Christoph Hellwig h...@infradead.org --- drivers/scsi/u14-34f.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Index: linux-next-20140724/drivers/scsi/u14-34f.c === --- linux-next-20140724.orig/drivers/scsi/u14-34f.c +++ linux-next-20140724/drivers/scsi/u14-34f.c @@ -1006,7 +1006,7 @@ static int port_detect \ sh[j]-irq, dma_name, sh[j]-sg_tablesize, sh[j]-can_queue); if (sh[j]-max_id 8 || sh[j]-max_lun 8) - printk(%s: wide SCSI support enabled, max_id %u, max_lun %u.\n, + printk(%s: wide SCSI support enabled, max_id %u, max_lun %llu.\n, BN(j), sh[j]-max_id, sh[j]-max_lun); for (i = 0; i = sh[j]-max_channel; i++) @@ -1285,7 +1285,7 @@ static int u14_34f_queuecommand_lck(stru cpp-cpp_index = i; SCpnt-host_scribble = (unsigned char *) cpp-cpp_index; - if (do_trace) printk(%s: qcomm, mbox %d, target %d.%d:%llu.\n, + if (do_trace) printk(%s: qcomm, mbox %d, target %d.%d:%u.\n, BN(j), i, SCpnt-device-channel, SCpnt-device-id, (u8)SCpnt-device-lun); -- -- ~Randy -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] hpsa: work in progress lockless monster patches
On Sun, Jul 27, 2014 at 05:24:46PM +0200, Hannes Reinecke wrote: On 07/25/2014 09:28 PM, scame...@beardog.cce.hp.com wrote: hpsa: Work In Progress: lockless monster patches To be clear, I am not suggesting that these patches be merged at this time. This patchset is vs. Christoph Hellwig's scsi-mq.4 branch which may be found here: git://git.infradead.org/users/hch/scsi.git We've been working for a long time on a patchset for hpsa to remove all the locks from the main i/o path in pursuit of high IOPS. Some of that work is already upstream, but a lot more of it is not quite yet ready to be merged. However, I think we've gone dark for a bit too long on this, and even though the patches aren't really ready to be merged just yet, I thought I should let other people who might be interested have a look anyway, as things are starting to be at least more stable than unstable. Be warned though, there are still some problems, esp. around error recovery. That being said, with the right hardware (fast SAS SSDs, recent Smart Arrays e.g. P430, with up-to-date firmware, attached to recent disk enclosures) with these patches and the scsi-mq patches, it is possible to get around ~970k IOPS from a single logical drive on a single controller. There are about 150 patches in this set. Rather than bomb the list with that, here is a link to a tarball of the patches in the form of an stgit patch series: https://github.com/smcameron/hpsa-lockless-patches-work-in-progress/blob/master/hpsa-lockless-vs-hch-scsi-mq.4-2014-07-25-1415CDT.tar.bz2?raw=true In some cases, I have probably erred on the side of having too many too small patches, on the theory that it is easier to bake a cake than to unbake a cake. Before these are submitted for reals, there will be some squashing of patches, along with other cleaning up. There are some patches in this set which are already upstream in James's tree which do not happen to be in Christoph's tree (most of which are named *_sent_to_james). There are also quite a few patches which are strictly for debugging and are not ever intended to be merged. Hmm. While you're about to engage on a massive rewrite _and_ we're having 64bit LUN support now, what about getting rid of the weird internal LUN mapping? That way you would get rid of this LUN rescan thingie and the driver would look more sane. Sorry for my slow reply to this. I can sympathize with this desire, the device scanning code in the driver is not my favorite, and it can undoubtedly be improved. However, there are some problems. Plus the REPORT LUN command would actually return the correct data ... No, it wouldn't. Smart Arrays are unfortunately pretty weird. SCSI REPORT LUNS issued to a smart array will report only the logical drives. No physical devices (tape drives, etc.) will be reported. Instead of SCSI REPORT LUNS, the driver uses a couple of proprietary variants of this, CISS_REPORT_LOGICAL and CISS_REPORT_PHYSICAL, which report logical drives and physical devices, and have their own oddities. And it gets weirder. This will require some exposition, so bear with me. What's driving this giant patchball to remove locks from the driver is SSDs. Suddenly, the assumption that disks are dirt slow relative to the host is not quite so true anymore. Smart Array looks something like this: +---+ | host | +-+-+ | | PCI bus | +-|--+ | | | | +--RAID stack |--- smart array |running on | |embedded system | |on PCI board| | | | |-|--| | | | | Back end firmware | || | +|---+ | | SAS | +--+ | physical devices (disks, etc.) | +--+ with the advent of SSDs, it turns out the RAID stack firmware running on the little embedded system on the PCI board becomes a bottleneck and hurts performance. It turns out that with a (significant) firmware change, we can do something like this: +---+ | host | ++--+ | | PCI bus | +|---+ || | | +---/ \--RAID stack |--- smart array | |
Re: [PATCH -next] scsi: fix u14-34f printk format warnings
Thanks, Randy. I'll merge it. I usually want a second review for scsi patches, but I guess this one is trivial enough to waive that requirement. That being said is anyone still using the u14-34f driver? From looking at the commit history it doesn't really look either maintained or used.. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] eata: remove driver_lock
Can I also get a second review?
On Sat, Jul 26, 2014 at 09:25:59AM -0700, Christoph Hellwig wrote:
Can I get a quick review for this one?
On Mon, Jul 14, 2014 at 10:26:33AM +0200, Christoph Hellwig wrote:
port_detect is only called from the module_init routine and thus implicitly
serialized, so remove the driver lock which was held over potentially
sleeping function calls.
Signed-off-by: Christoph Hellwig h...@lst.de
Reported-by: Arthur Marsh arthur.ma...@internode.on.net
Tested-by: Arthur Marsh arthur.ma...@internode.on.net
---
drivers/scsi/eata.c | 9 -
1 file changed, 9 deletions(-)
diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
index 03372cf..980898e 100644
--- a/drivers/scsi/eata.c
+++ b/drivers/scsi/eata.c
@@ -837,7 +837,6 @@ struct hostdata {
static struct Scsi_Host *sh[MAX_BOARDS];
static const char *driver_name = EATA;
static char sha[MAX_BOARDS];
-static DEFINE_SPINLOCK(driver_lock);
/* Initialize num_boards so that ihdlr can work while detect is in
progress */
static unsigned int num_boards = MAX_BOARDS;
@@ -1097,8 +1096,6 @@ static int port_detect(unsigned long port_base,
unsigned int j,
goto fail;
}
- spin_lock_irq(driver_lock);
-
if (do_dma(port_base, 0, READ_CONFIG_PIO)) {
#if defined(DEBUG_DETECT)
printk(%s: detect, do_dma failed at 0x%03lx.\n, name,
@@ -1265,10 +1262,7 @@ static int port_detect(unsigned long port_base,
unsigned int j,
}
#endif
- spin_unlock_irq(driver_lock);
sh[j] = shost = scsi_register(tpnt, sizeof(struct hostdata));
- spin_lock_irq(driver_lock);
-
if (shost == NULL) {
printk(%s: unable to register host, detaching.\n, name);
goto freedma;
@@ -1345,8 +1339,6 @@ static int port_detect(unsigned long port_base,
unsigned int j,
else
sprintf(dma_name, DMA %u, dma_channel);
- spin_unlock_irq(driver_lock);
-
for (i = 0; i shost-can_queue; i++)
ha-cp[i].cp_dma_addr = pci_map_single(ha-pdev,
ha-cp[i],
@@ -1439,7 +1431,6 @@ static int port_detect(unsigned long port_base,
unsigned int j,
freeirq:
free_irq(irq, sha[j]);
freelock:
- spin_unlock_irq(driver_lock);
release_region(port_base, REGION_SIZE);
fail:
return 0;
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
---end quoted text---
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
---end quoted text---
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[scsi/net-next] Pulling in net-next changes
Christoph/James/Mike Is there a timeline for pulling in the cxgb4i changes from net-next, or a specific way to ask for it ? We have more changes in queue for cxgb4i libiscsi, and I would rather have the cxgb4i up to date in the scsi tree before we do that to avoid any potentil issues in the future. Last change to cxgb4i was in net-next tree was e81fbf6cd652 (libcxgbi:cxgb4i Guard ipv6 code with a config check) -Anish -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: 3w-9xxx.c: Cleaning up missing null-terminate in conjunction with strncpy
On Sun, Jul 27, 2014 at 8:11 AM, Rickard Strandqvist rickard_strandqv...@spectrumdigital.se wrote: Replacing strncpy with strlcpy to avoid strings that lacks null terminate. And use the sizeof on the to string rather than strlen on the from string. Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se --- drivers/scsi/3w-9xxx.c |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c index 0a73253..f4d2331 100644 --- a/drivers/scsi/3w-9xxx.c +++ b/drivers/scsi/3w-9xxx.c @@ -621,7 +621,8 @@ static int twa_check_srl(TW_Device_Extension *tw_dev, int *flashed) } /* Load rest of compatibility struct */ - strncpy(tw_dev-tw_compat_info.driver_version, TW_DRIVER_VERSION, strlen(TW_DRIVER_VERSION)); + strlcpy(tw_dev-tw_compat_info.driver_version, TW_DRIVER_VERSION, + sizeof(tw_dev-tw_compat_info.driver_version)); tw_dev-tw_compat_info.driver_srl_high = TW_CURRENT_DRIVER_SRL; tw_dev-tw_compat_info.driver_branch_high = TW_CURRENT_DRIVER_BRANCH; tw_dev-tw_compat_info.driver_build_high = TW_CURRENT_DRIVER_BUILD; -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ Rickard, This patch looks fine. Acked-by: Adam Radford aradf...@gmail.com -Adam -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] iscsi class: fix get_host_stats error handling
On 07/30/2014 07:50 AM, Hannes Reinecke wrote:
On 07/12/2014 10:51 PM, micha...@cs.wisc.edu wrote:
From: Mike Christie micha...@cs.wisc.edu
iscsi_get_host_stats was dropping the error code returned
by drivers like qla4xxx.
Signed-off-by: Mike Christie micha...@cs.wisc.edu
---
drivers/scsi/scsi_transport_iscsi.c |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/scsi/scsi_transport_iscsi.c
b/drivers/scsi/scsi_transport_iscsi.c
index b481e62..14bfa53 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -3467,6 +3467,10 @@ iscsi_get_host_stats(struct iscsi_transport
*transport, struct nlmsghdr *nlh)
memset(buf, 0, host_stats_size);
err = transport-get_host_stats(shost, buf, host_stats_size);
+if (err) {
+kfree(skbhost_stats);
+goto exit_host_stats;
+}
actual_size = nlmsg_total_size(sizeof(*ev) + host_stats_size);
skb_trim(skbhost_stats, NLMSG_ALIGN(actual_size));
What happens with the skbhost_stats allocated earlier? Shouldn't it be
freed here, too?
You mean for this success path right. It is not freed here by the iscsi
code on purpose. For the code path here where we have successfully
called into the driver then a couple lines below we will do
iscsi_multicast_skb() - nlmsg_multicast()
which will pass the skbhost_stats skb to the netlink layer. The
netlink/socket/skb code then frees it when it is done with it.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] lpfc: Avoid to disable pci_dev twice
On 07/17/2014 02:32 PM, Mike Qiu wrote:
Hi, all
How about this patch ?
Any idea ?
In IBM Power servers, when hardware error occurs during probe
state, EEH subsystem will call driver's error_detected interface,
which will call pci_disable_device(). But driver's probe function also
call pci_disable_device() in this situation.
So pci_dev will be disabled twice:
Device lpfc disabling already-disabled device
[ cut here ]
WARNING: at drivers/pci/pci.c:1407
CPU: 0 PID: 8744 Comm: kworker/0:0 Tainted: GW
3.10.42-2002.pkvm2_1_1.6.ppc64 #1
Workqueue: events .work_for_cpu_fn
task: c0274e3f5400 ti: c027d3958000 task.ti: c027d3958000
NIP: c0471b8c LR: c0471b88 CTR: c043ebe0
REGS: c027d395b650 TRAP: 0700 Tainted: GW
(3.10.42-2002.pkvm2_1_1.6.ppc64)
MSR: 900100029032 SF,HV,EE,ME,IR,DR,RI CR: 28b52b44 XER: 2000
CFAR: c0879ab8 SOFTE: 1
...
NIP .pci_disable_device+0xcc/0xe0
LR .pci_disable_device+0xc8/0xe0
Call Trace:
.pci_disable_device+0xc8/0xe0 (unreliable)
.lpfc_disable_pci_dev+0x50/0x80 [lpfc]
.lpfc_pci_probe_one+0x870/0x21a0 [lpfc]
.local_pci_probe+0x68/0xb0
.work_for_cpu_fn+0x38/0x60
.process_one_work+0x1a4/0x4d0
.worker_thread+0x37c/0x490
.kthread+0xf0/0x100
.ret_from_kernel_thread+0x5c/0x80
Signed-off-by: Mike Qiu qiud...@linux.vnet.ibm.com
---
drivers/scsi/lpfc/lpfc.h | 1 +
drivers/scsi/lpfc/lpfc_init.c | 59 +++
2 files changed, 55 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/lpfc/lpfc.h b/drivers/scsi/lpfc/lpfc.h
index 434e903..0c7bad9 100644
--- a/drivers/scsi/lpfc/lpfc.h
+++ b/drivers/scsi/lpfc/lpfc.h
@@ -813,6 +813,7 @@ struct lpfc_hba {
#define VPD_MASK0xf /* mask for any vpd data */
uint8_t soft_wwn_enable;
+ uint8_t probe_done;
struct timer_list fcp_poll_timer;
struct timer_list eratt_poll;
diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index 06f9a5b..c2e67ae 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -9519,6 +9519,9 @@ lpfc_pci_probe_one_s3(struct pci_dev *pdev, const struct
pci_device_id *pid)
}
}
+ /* Set the probe flag */
+ phba-probe_done = 1;
+
/* Perform post initialization setup */
lpfc_post_init_setup(phba);
@@ -9795,6 +9798,9 @@ lpfc_sli_prep_dev_for_recover(struct lpfc_hba *phba)
static void
lpfc_sli_prep_dev_for_reset(struct lpfc_hba *phba)
{
+ if (phba)
+ return;
+
lpfc_printf_log(phba, KERN_ERR, LOG_INIT,
2710 PCI channel disable preparing for reset\n);
@@ -9812,7 +9818,8 @@ lpfc_sli_prep_dev_for_reset(struct lpfc_hba *phba)
/* Disable interrupt and pci device */
lpfc_sli_disable_intr(phba);
- pci_disable_device(phba-pcidev);
+ if (phba-probe_done phba-pcidev)
+ pci_disable_device(phba-pcidev);
}
/**
@@ -10282,6 +10289,9 @@ lpfc_pci_probe_one_s4(struct pci_dev *pdev, const
struct pci_device_id *pid)
goto out_disable_intr;
}
+ /* Set probe_done flag */
+ phba-probe_done = 1;
+
/* Log the current active interrupt mode */
phba-intr_mode = intr_mode;
lpfc_log_intr_mode(phba, intr_mode);
@@ -10544,6 +10554,9 @@ lpfc_sli4_prep_dev_for_recover(struct lpfc_hba *phba)
static void
lpfc_sli4_prep_dev_for_reset(struct lpfc_hba *phba)
{
+ if (!phba)
+ return;
+
lpfc_printf_log(phba, KERN_ERR, LOG_INIT,
2826 PCI channel disable preparing for reset\n);
@@ -10562,7 +10575,9 @@ lpfc_sli4_prep_dev_for_reset(struct lpfc_hba *phba)
/* Disable interrupt and pci device */
lpfc_sli4_disable_intr(phba);
lpfc_sli4_queue_destroy(phba);
- pci_disable_device(phba-pcidev);
+
+ if (phba-probe_done phba-pcidev)
+ pci_disable_device(phba-pcidev);
}
/**
@@ -10893,9 +10908,21 @@ static pci_ers_result_t
lpfc_io_error_detected(struct pci_dev *pdev, pci_channel_state_t state)
{
struct Scsi_Host *shost = pci_get_drvdata(pdev);
- struct lpfc_hba *phba = ((struct lpfc_vport *)shost-hostdata)-phba;
+ struct lpfc_hba *phba;
pci_ers_result_t rc = PCI_ERS_RESULT_DISCONNECT;
+ if (!shost)
+ /* Run here means it may during probe state and
+* Scsi_Host has not been created and We can do nothing
+* in this state so call for hotplug*/
+ return PCI_ERS_RESULT_NONE;
+
+ phba = ((struct lpfc_vport *)shost-hostdata)-phba;
+
+ if (!phba || !phba-probe_done)
+ /* Run here means it may during probe state */
+ return PCI_ERS_RESULT_NONE;
+
switch (phba-pci_dev_grp) {
case LPFC_PCI_DEV_LP:
rc = lpfc_io_error_detected_s3(pdev, state);
@@ -10930,9
Oops in scsi_put_host_cmd_pool
During test of Xen pvSCSI frontend module I found the following issue: When unplugging a passed-through SCSI-device the SCSI Host is removed. When calling the final scsi_host_put() from the driver an Oops is happening: [ 219.816292] (file=drivers/scsi/xen-scsifront.c, line=808) scsifront_remove: device/vscsi/1 removed [ 219.816371] BUG: unable to handle kernel NULL pointer dereference at 0010 [ 219.816380] IP: [805fdcf8] scsi_put_host_cmd_pool+0x38/0xb0 [ 219.816390] PGD 3bd60067 PUD 3d353067 PMD 0 [ 219.816396] Oops: [#1] SMP [ 219.816400] Modules linked in: nls_utf8 sr_mod cdrom xen_scsifront xt_pkttype xt_LOG xt_limit af_packet ip6t_REJECT xt_tcpudp nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_raw ipt_REJECT iptable_raw xt_CT iptable_filter ip6table_mangle nf_conntrack_netbios_ns nf_conntrack_broadcast nf_conntrack_ipv4 nf_defrag_ipv4 ip_tables xt_conntrack nf_conntrack ip6table_filter ip6_tables x_tables x86_pkg_temp_thermal thermal_sys coretemp hwmon crc32_pclmul crc32c_intel ghash_clmulni_intel aesni_intel ablk_helper cryptd lrw gf128mul glue_helper aes_x86_64 microcode pcspkr sg dm_mod autofs4 scsi_dh_alua scsi_dh_emc scsi_dh_rdac scsi_dh_hp_sw scsi_dh xen_blkfront xen_netfront [ 219.816458] CPU: 0 PID: 23 Comm: xenwatch Not tainted 3.16.0-rc6-11-xen+ #66 [ 219.816463] task: 88003da985d0 ti: 88003da9c000 task.ti: 88003da9c000 [ 219.816467] RIP: e030:[805fdcf8] [805fdcf8] scsi_put_host_cmd_pool+0x38/0xb0 [ 219.816474] RSP: e02b:88003da9fc20 EFLAGS: 00010202 [ 219.816477] RAX: a01a50c0 RBX: RCX: 0003 [ 219.816481] RDX: 0240 RSI: 88003d242b80 RDI: 80c708c0 [ 219.816485] RBP: 88003da9fc38 R08: 4f7e974a31ed0290 R09: [ 219.816488] R10: 7ff0 R11: 0001 R12: 8800038f8000 [ 219.816491] R13: a01a50c0 R14: R15: [ 219.816498] FS: 7fe2e2eeb700() GS:88003f80() knlGS: [ 219.816502] CS: e033 DS: ES: CR0: 80050033 [ 219.816505] CR2: 0010 CR3: 3d20c000 CR4: 00042660 [ 219.816509] Stack: [ 219.816511] 8800038f8000 8800038f8030 880003ae3400 88003da9fc58 [ 219.816516] 805fe78b 8800038f8000 88003bb82c40 88003da9fc80 [ 219.816521] 805ff587 8800038f81a0 8800038f8190 880003ae3400 [ 219.816527] Call Trace: [ 219.816533] [805fe78b] scsi_destroy_command_freelist+0x5b/0x60 [ 219.816538] [805ff587] scsi_host_dev_release+0x97/0xe0 [ 219.816543] [805dd71d] device_release+0x2d/0xa0 [ 219.816548] [804dbec7] kobject_cleanup+0x77/0x1b0 [ 219.816553] [804dbd70] kobject_put+0x30/0x60 [ 219.816556] [805dd9e2] put_device+0x12/0x20 [ 219.816560] [805ff490] scsi_host_put+0x10/0x20 [ 219.816565] [a01a2042] scsifront_free+0x42/0x90 [xen_scsifront] [ 219.816569] [a01a20ad] scsifront_remove+0x1d/0x50 [xen_scsifront] [ 219.816576] [805a4ee0] xenbus_dev_remove+0x50/0xa0 [ 219.816580] [805e1a8a] __device_release_driver+0x7a/0xf0 [ 219.816584] [805e1b1e] device_release_driver+0x1e/0x30 [ 219.816588] [805e1440] bus_remove_device+0x100/0x180 [ 219.816593] [805ddef1] device_del+0x121/0x1b0 [ 219.816596] [805ddf99] device_unregister+0x19/0x60 [ 219.816601] [805a56be] xenbus_dev_changed+0x9e/0x1e0 [ 219.816606] [8079d695] ? _raw_spin_unlock_irqrestore+0x25/0x50 [ 219.816611] [805a41d0] ? unregister_xenbus_watch+0x200/0x200 [ 219.816615] [805a7420] frontend_changed+0x20/0x50 [ 219.816619] [805a426f] xenwatch_thread+0x9f/0x160 [ 219.816624] [802a50f0] ? prepare_to_wait_event+0xf0/0xf0 [ 219.816628] [8028485d] kthread+0xcd/0xf0 [ 219.816633] [80284790] ? kthread_create_on_node+0x170/0x170 [ 219.816638] [8079dcbc] ret_from_fork+0x7c/0xb0 [ 219.816642] [80284790] ? kthread_create_on_node+0x170/0x170 [ 219.816645] Code: 8b af c0 00 00 00 48 c7 c7 c0 08 c7 80 e8 d1 e0 19 00 49 8b 84 24 c0 00 00 00 8b 90 48 01 00 00 85 d2 74 2f 48 8b 98 50 01 00 00 8b 43 10 85 c0 74 68 83 e8 01 85 c0 89 43 10 74 37 48 c7 c7 c0 [ 219.816732] RIP [805fdcf8] scsi_put_host_cmd_pool+0x38/0xb0 [ 219.816747] RSP 88003da9fc20 [ 219.816750] CR2: 0010 [ 219.816753] ---[ end trace c6915ea21a3d05f7 ]--- I should mention I've specified .cmd_len in the scsi_host_template. The only other driver doing this seems to be virtio_scsi.c, so I assume the same problem could occur with passed-through SCSI devices under KVM... Juergen -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at
