Re: [PATCH v2 1/1] bsg-lib: fix kernel panic resulting from missing allocation of reply-buffer
> /** > - * bsg_destroy_job - routine to teardown/delete a bsg job > + * bsg_teardown_job - routine to teardown a bsg job > * @job: bsg_job that is to be torn down > */ > -static void bsg_destroy_job(struct kref *kref) > +static void bsg_teardown_job(struct kref *kref) Why this rename? The destroy name seems to be one of the most common patterns for the kref_put callbacks. Otherwise this looks fine: Reviewed-by: Christoph Hellwig
Re: [PATCH] scsi_transport_sas: Fix error handling in sas_smp_request()
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 01/19] Remove an obsolete function declaration
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 02/19] Avoid sign extension of scsi_device.type
Looks fine, Reviewed-by: Christoph Hellwig
Re: [PATCH 03/19] Suppress gcc 7 fall-through warnings reported with W=1
Looks fine, Reviewed-by: Christoph Hellwig
Re: [PATCH 04/19] Convert a strncmp() call into a strcmp() call
Looks fine, Reviewed-by: Christoph Hellwig
Re: [PATCH 05/19] scsi_setup_fs_cmnd(): Call scsi_req_init() instead of open-coding it
I don't really see the point of doing more work in exactly the same amount of code here, especially given that this is the fast path. Do you need this for any future patches?
Re: [PATCH 06/19] Document which queue type a function is intended for
On Wed, Aug 23, 2017 at 02:39:56PM -0700, Bart Van Assche wrote: > Document which queue type a function is intended for if this is not > easy to derive from the function name. How about adding mq and legacy to the function names instead?
Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]
On Wed, Aug 23, 2017 at 02:39:57PM -0700, Bart Van Assche wrote: > Only annotate pointers that are shared across threads with __rcu. > Use rcu_dereference() when dereferencing an RCU pointer. Protect > also the RCU pointer dereferences when freeing RCU pointers. This > patch suppresses about twenty sparse complaints about the vpd_pg8[03] > pointers. Shouldn't the kfrees be kfree_rcu? or where else is the rcu protection for them?
Re: [PATCH 08/19] Use blk_mq_rq_to_pdu() to convert a request to a SCSI command pointer
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 09/19] sd, sr: Convert two assignments into warning statements
On Wed, Aug 23, 2017 at 02:39:59PM -0700, Bart Van Assche wrote: > Before scsi_prep_fn() calls the ULP .init_command() callback > function it stores the SCSI command pointer in request.special. > This means that the SCpnt = rq->special assignments in the sd > and sr drivers assign a pointer to itself. Hence convert these > two assignment statements into warning statements. Looks good. Reviewed-by: Christoph Hellwig Btw, in the longer run we should aim to kill the special pointer in struct request entirely.
Re: [PATCH 10/19] sd: Fix indentation
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 13/19] libiscsi: Fix indentation
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 12/19] sg: Fix type of last blk_trace_setup() argument
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 15/19] libsas: Annotate fall-through in a switch statement
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 14/19] libsas: Remove a set-but-not-used variable
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 16/19] scsi_transport_sas, sas_tlr_supported(): Check kzalloc() return value
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 17/19] scsi_transport_srp: Suppress a W=1 compiler warning
On Wed, Aug 23, 2017 at 02:40:07PM -0700, Bart Van Assche wrote: > Avoid that the following compiler warning is reported when building > with W=1: > > drivers/scsi/scsi_transport_srp.c:92:19: warning: comparison is always false > due to limited range of data type [-Wtype-limits] > > Signed-off-by: Bart Van Assche > Cc: Christoph Hellwig > Cc: Hannes Reinecke > Cc: Johannes Thumshirn > --- > drivers/scsi/scsi_transport_srp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/scsi_transport_srp.c > b/drivers/scsi/scsi_transport_srp.c > index 698cc4681706..b8f5e4c47579 100644 > --- a/drivers/scsi/scsi_transport_srp.c > +++ b/drivers/scsi/scsi_transport_srp.c > @@ -89,7 +89,7 @@ int srp_tmo_valid(int reconnect_delay, int > fast_io_fail_tmo, int dev_loss_tmo) > if (fast_io_fail_tmo < 0 && > dev_loss_tmo > SCSI_DEVICE_BLOCK_MAX_TIMEOUT) > return -EINVAL; > - if (dev_loss_tmo >= LONG_MAX / HZ) > + if (dev_loss_tmo + 0UL >= LONG_MAX / HZ) > return -EINVAL; That's a weird change.. If we can to promote dev_loss_tmo to long we should just cast it. And of course we should always ask us why we got this warning. If long is 64-bit and int is 32-bit the warning makes sense, as for every reasonable comparism the 32-bit timeout can't be larger than LONG_MAX divided by 100 or 100. But for 32-bit longs it can, so we should keep it, maybe with a comment.
Re: [PATCH 18/19] scsi_debug: Remove a set-but-not-used variable
Looks fine, Reviewed-by: Christoph Hellwig
Re: [PATCH 19/19] iscsi_tcp: Remove a set-but-not-used variable
Looks fine, Reviewed-by: Christoph Hellwig
Re: [PATCH v2] scsi: lpfc: avoid false positive gcc-8 warning
On Thu, Aug 24, 2017 at 2:34 AM, James Smart wrote:
> Arnd Bergmann, testing gcc-8, encountered the following:
>
>> This is an interesting regression with gcc-8, showing a harmless
>> warning for correct code:
>>
>>In file included from include/linux/kernel.h:13:0,
>>...
>>from drivers/scsi/lpfc/lpfc_debugfs.c:23:
>>include/linux/printk.h:301:2: error: 'eq' may be used
>>uninitialized in this function [-Werror=maybe-uninitialized]
>> printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
>> ^~
>>In file included from drivers/scsi/lpfc/lpfc_debugfs.c:58:0:
>>drivers/scsi/lpfc/lpfc_debugfs.h:451:31: note: 'eq' was
>>declared here
>
> The code is fine: a for loop which if there's at least 1 itteration,
> will assign eq a value. Followed by an if test that checks for no
> itterations and assigns eq a default value. But the checker doesn't
> see the relationship between the two so assumes eq may not a have a
> value.
>
> I believe, simply initializing with a NULL will solve the issue.
>
> Signed-off-by: James Smart
That's probably good enough here, as the warning is rather obscure
(only one instance in the entire kernel in 1000 randconfig builds),
with an unreleased compiler.
Anyway, I have successfully reduced a test case and reported
a gcc bug for it, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81958
The compiler test case is
__attribute__ ((__cold__)) int printk();
struct lpfc_queue {
int queue_id;
struct lpfc_queue *hba_eq;
} *cq_phba;
void lpfc_debug_dump_all_queues(unsigned maxidx)
{
struct lpfc_queue *eq;
unsigned eqidx;
printk();
for (eqidx = 0; eqidx < maxidx; eqidx++) {
eq = &cq_phba->hba_eq[eqidx];
if (eq->queue_id)
break;
}
if (eqidx == maxidx)
eq = &cq_phba->hba_eq[0];
printk(eq);
}
and I see no reason why the compiler should get this wrong.
I have also come up with a different workaround of my own
(sorry for the broken formatting here) and tested it successfully
over night. I have definitely spent more time on it than it was
worth now. Let me know if you prefer that version over yours,
then I'll submit that as a proper patch with your Ack.
Signed-off-by: Arnd Bergmann
--- a/drivers/scsi/lpfc/lpfc_debugfs.h
+++ b/drivers/scsi/lpfc/lpfc_debugfs.h
@@ -450,7 +450,7 @@ lpfc_debug_dump_cq(struct lpfc_hba *phba, int
qtype, int wqidx)
{
struct lpfc_queue *wq, *cq, *eq;
char *qtypestr;
- unsigned int eqidx;
+ int eqidx;
/* fcp/nvme wq and cq are 1:1, thus same indexes */
@@ -478,16 +478,16 @@ lpfc_debug_dump_cq(struct lpfc_hba *phba, int
qtype, int wqidx)
return;
for (eqidx = 0; eqidx < phba->io_channel_irqs; eqidx++) {
- eq = phba->sli4_hba.hba_eq[eqidx];
- if (cq->assoc_qid == eq->queue_id)
+ if (cq->assoc_qid == phba->sli4_hba.hba_eq[eqidx]->queue_id)
break;
}
if (eqidx == phba->io_channel_irqs) {
pr_err("Couldn't find EQ for CQ. Using EQ[0]\n");
eqidx = 0;
- eq = phba->sli4_hba.hba_eq[0];
}
+ eq = phba->sli4_hba.hba_eq[eqidx];
+
if (qtype == DUMP_FCP || qtype == DUMP_NVME)
pr_err("%s CQ: WQ[Idx:%d|Qid%d]->CQ[Idx%d|Qid%d]"
"->EQ[Idx:%d|Qid:%d]:\n",
Arnd
Re: [PATCH] scsi: sd: remove duplicated setting of gd->minors
On Fri, Aug 18, 2017 at 02:49:30PM +0800, weiping zhang wrote: > gd->minors has been set when call alloc_disk() in sd_probe. > > Signed-off-by: weiping zhang > --- > drivers/scsi/sd.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index bea36ad..c18ca7a 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -3223,7 +3223,6 @@ static void sd_probe_async(void *data, async_cookie_t > cookie) > > gd->major = sd_major((index & 0xf0) >> 4); > gd->first_minor = ((index & 0xf) << 4) | (index & 0xfff00); > - gd->minors = SD_MINORS; > > gd->fops = &sd_fops; > gd->private_data = &sdkp->driver; > -- > 2.9.4 > Hi Martin, would you please look into this misc patch ? Thanks
Re: [PATCH] Improve requeuing behavior
On Wed, Aug 23, 2017 at 02:05:35PM -0700, Bart Van Assche wrote:
> Requests are unprepared and reprepared when being requeued. Avoid
> that requeuing resets .jiffies_at_alloc and .retries by initializing
> these two member variables from inside blk_get_request() and by
> preserving both member variables when preparing a request. This patch
> affects the requeuing behavior of scsi-sq and scsi-mq.
>
> Reported-by: Brian King
> References: https://lkml.org/lkml/2017/8/18/923 ("Re: [BUG][bisected 270065e]
> linux-next fails to boot on powerpc")
> Signed-off-by: Bart Van Assche
> Cc: Brian King
> Cc: Hannes Reinecke
> Cc: Christoph Hellwig
> Cc: Johannes Thumshirn
> ---
> drivers/scsi/scsi_lib.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index ebc5c713ee37..8d1ec1e7b0e2 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1122,6 +1122,8 @@ void scsi_initialize_rq(struct request *rq)
> struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
>
> scsi_req_init(&cmd->req);
> + cmd->jiffies_at_alloc = jiffies;
> + cmd->retries = 0;
> }
> EXPORT_SYMBOL(scsi_initialize_rq);
How is this working for non-passthrough commands where we don't
call scsi_initialize_rq?
Re: [PATCH] scsi_transport_sas: Fix error handling in sas_smp_request()
On 08/23/2017 08:25 PM, Bart Van Assche wrote:
> sas_function_template.smp_handler implementations either return
> 0 or a Unix error code. Convert that error code into a SCSI
> result. This patch is what I came up with after having analyzed
> the following sparse warnings:
>
> drivers/scsi/scsi_transport_sas.c:187:21: warning: incorrect type in
> assignment (different base types)
> drivers/scsi/scsi_transport_sas.c:187:21:expected restricted blk_status_t
> [usertype] ret
> drivers/scsi/scsi_transport_sas.c:187:21:got int
> drivers/scsi/scsi_transport_sas.c:188:39: warning: incorrect type in
> assignment (different base types)
> drivers/scsi/scsi_transport_sas.c:188:39:expected int [signed] result
> drivers/scsi/scsi_transport_sas.c:188:39:got restricted blk_status_t
> [usertype] ret
>
> Fixes: commit 17d5363b83f8 ("scsi: introduce a result field in struct
> scsi_request")
> Signed-off-by: Bart Van Assche
> Cc: Christoph Hellwig
> Cc: Hannes Reinecke
> Cc: Johannes Thumshirn
> Cc:
> ---
> drivers/scsi/scsi_transport_sas.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_sas.c
> b/drivers/scsi/scsi_transport_sas.c
> index 5006a656e16a..a318c46db7cc 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -173,7 +173,7 @@ static void sas_smp_request(struct request_queue *q,
> struct Scsi_Host *shost,
> struct sas_rphy *rphy)
> {
> struct request *req;
> - blk_status_t ret;
> + int ret;
> int (*handler)(struct Scsi_Host *, struct sas_rphy *, struct request *);
>
> while ((req = blk_fetch_request(q)) != NULL) {
> @@ -185,7 +185,9 @@ static void sas_smp_request(struct request_queue *q,
> struct Scsi_Host *shost,
> blk_rq_bytes(req->next_rq);
> handler = to_sas_internal(shost->transportt)->f->smp_handler;
> ret = handler(shost, rphy, req);
> - scsi_req(req)->result = ret;
> + WARN_ONCE(ret != 0 && !IS_ERR_VALUE(ret + 0UL),
> + "%s: ret = %d\n", __func__, ret);
> + scsi_req(req)->result = ret ? DID_ERROR << 16 : 0;
>
> blk_end_request_all(req, 0);
>
>
Weelll ... I'd rather audit the handler so as to ensure that the correct
value is returned.
And this 'ret + 0UL' construct is decidedly ugly ...
Cheers,
Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
[PATCH] mptsas: Fixup device hotplug for VMWare ESXi
VMWare ESXi emulates an mptsas HBA, but exposes all drives as
direct-attached SAS drives.
This it not how the driver originally envisioned things; SAS drives
were supposed to be connected via an expander, and only SATA drives
would be direct attached.
As such any hotplug event for direct-attach SAS drives was silently
ignored, and the guest failed to detect new drives from within a
VMWare ESXi environment.
Bugzilla: https://bugzilla.suse.com/show_bug.cgi?id=1030850
Signed-off-by: Hannes Reinecke
---
drivers/message/fusion/mptsas.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index f6308ad..b9bd6aa 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -4352,11 +4352,10 @@ static void mptsas_expander_delete(MPT_ADAPTER *ioc,
return;
phy_info = mptsas_refreshing_device_handles(ioc, &sas_device);
- /* Only For SATA Device ADD */
- if (!phy_info && (sas_device.device_info &
- MPI_SAS_DEVICE_INFO_SATA_DEVICE)) {
+ /* Device hostplug */
+ if (!phy_info) {
devtprintk(ioc, printk(MYIOC_s_DEBUG_FMT
- "%s %d SATA HOT PLUG: "
+ "%s %d HOT PLUG: "
"parent handle of device %x\n", ioc->name,
__func__, __LINE__, sas_device.handle_parent));
port_info = mptsas_find_portinfo_by_handle(ioc,
--
1.8.5.6
[PATCH] ses: Fix racy cleanup of /sys in remove_dev()
From: Calvin Owens Currently we free the resources backing the enclosure device before we call device_unregister(). This is racy: during rmmod of low-level SCSI drivers that hook into enclosure, we end up with a small window of time during which writing to /sys can OOPS. Example trace with mpt3sas: general protection fault: [#1] SMP KASAN Modules linked in: mpt3sas(-) <...> RIP: [] ses_get_page2_descriptor.isra.6+0x38/0x220 [ses] Call Trace: [] ses_set_fault+0xf4/0x400 [ses] [] set_component_fault+0xa9/0xf0 [enclosure] [] dev_attr_store+0x3c/0x70 [] sysfs_kf_write+0x115/0x180 [] kernfs_fop_write+0x275/0x3a0 [] __vfs_write+0xe0/0x3e0 [] vfs_write+0x13f/0x4a0 [] SyS_write+0x111/0x230 [] entry_SYSCALL_64_fastpath+0x13/0x94 Fortunately the solution is extremely simple: call device_unregister() before we free the resources, and the race no longer exists. The driver core holds a reference over ->remove_dev(), so AFAICT this is safe. Bugzilla: https://bugzilla.suse.com/show_bug.cgi?id=989094 Signed-off-by: Calvin Owens Acked-by: Hannes Reinecke --- drivers/scsi/ses.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index a3f9350..ea7066c 100644 --- a/drivers/scsi/ses.c +++ b/drivers/scsi/ses.c @@ -818,8 +818,6 @@ static void ses_intf_remove_enclosure(struct scsi_device *sdev) if (!edev) return; - enclosure_unregister(edev); - ses_dev = edev->scratch; edev->scratch = NULL; @@ -831,6 +829,7 @@ static void ses_intf_remove_enclosure(struct scsi_device *sdev) kfree(edev->component[0].scratch); put_device(&edev->edev); + enclosure_unregister(edev); } static void ses_intf_remove(struct device *cdev, -- 1.8.5.6
Re: [PATCH v2 1/1] bsg-lib: fix kernel panic resulting from missing allocation of reply-buffer
On Thu, Aug 24, 2017 at 10:45:56AM +0200, Christoph Hellwig wrote: > > /** > > - * bsg_destroy_job - routine to teardown/delete a bsg job > > + * bsg_teardown_job - routine to teardown a bsg job > > * @job: bsg_job that is to be torn down > > */ > > -static void bsg_destroy_job(struct kref *kref) > > +static void bsg_teardown_job(struct kref *kref) > > Why this rename? The destroy name seems to be one of the most > common patterns for the kref_put callbacks. > Hmm, I did it mostly so it is symmetric with bsg_prepare_job() and it doesn't really itself destroy the job-struct anymore. If there are other thing amiss I can change that along with them, if it bothers poeple. Beste Grüße / Best regards, - Benjamin Block > > Otherwise this looks fine: > > Reviewed-by: Christoph Hellwig > -- Linux on z Systems Development / IBM Systems & Technology Group IBM Deutschland Research & Development GmbH Vorsitz. AufsR.: Martina Koederitz /Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294
Re: [PATCH v2 0/1] bsg: fix regression resulting in panics when sending commands via BSG
On 08/23/2017 05:57 PM, Benjamin Block wrote: > Hello all, > > This is the second try for fixing the regression in the BSG-interface that > exists since v4.11 (for more infos see the first series). > > I separated my other changes from the bug-fix so that it is easier to apply > if judged good. I will rebase my cleanups I sent in v1 and send them when I > get a bit more time. But the regression-fix is more important, so here's > that. > > I did some more tests on it than on v1, including some heavy parallel I/O > on the same blk-queue using both BSG and the normal SCSI-stack at the same > time (throwing some intentional bad commands in it too). That seemed to > work all well enough - i.e. it didn't crash and got the expected results. I > haven't done any external error-inject, but IMO that would be beyond the > scope right now. > > The fix is based on Christoph's idea, I discussed this with him off-list > already. > > I rebased the series on Jens' for-next. Added for 4.13, thanks. -- Jens Axboe
Re: [PATCH] scsi_transport_sas: Fix error handling in sas_smp_request()
On Thu, 2017-08-24 at 14:21 +0200, Hannes Reinecke wrote:
> On 08/23/2017 08:25 PM, Bart Van Assche wrote:
> > sas_function_template.smp_handler implementations either return
> > 0 or a Unix error code. Convert that error code into a SCSI
> > result. This patch is what I came up with after having analyzed
> > the following sparse warnings:
> >
> > drivers/scsi/scsi_transport_sas.c:187:21: warning: incorrect type in
> > assignment (different base types)
> > drivers/scsi/scsi_transport_sas.c:187:21:expected restricted
> > blk_status_t [usertype] ret
> > drivers/scsi/scsi_transport_sas.c:187:21:got int
> > drivers/scsi/scsi_transport_sas.c:188:39: warning: incorrect type in
> > assignment (different base types)
> > drivers/scsi/scsi_transport_sas.c:188:39:expected int [signed] result
> > drivers/scsi/scsi_transport_sas.c:188:39:got restricted blk_status_t
> > [usertype] ret
> >
> > Fixes: commit 17d5363b83f8 ("scsi: introduce a result field in struct
> > scsi_request")
> > Signed-off-by: Bart Van Assche
> > Cc: Christoph Hellwig
> > Cc: Hannes Reinecke
> > Cc: Johannes Thumshirn
> > Cc:
> > ---
> > drivers/scsi/scsi_transport_sas.c | 6 --
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/scsi_transport_sas.c
> > b/drivers/scsi/scsi_transport_sas.c
> > index 5006a656e16a..a318c46db7cc 100644
> > --- a/drivers/scsi/scsi_transport_sas.c
> > +++ b/drivers/scsi/scsi_transport_sas.c
> > @@ -173,7 +173,7 @@ static void sas_smp_request(struct request_queue *q,
> > struct Scsi_Host *shost,
> > struct sas_rphy *rphy)
> > {
> > struct request *req;
> > - blk_status_t ret;
> > + int ret;
> > int (*handler)(struct Scsi_Host *, struct sas_rphy *, struct request *);
> >
> > while ((req = blk_fetch_request(q)) != NULL) {
> > @@ -185,7 +185,9 @@ static void sas_smp_request(struct request_queue *q,
> > struct Scsi_Host *shost,
> > blk_rq_bytes(req->next_rq);
> > handler = to_sas_internal(shost->transportt)->f->smp_handler;
> > ret = handler(shost, rphy, req);
> > - scsi_req(req)->result = ret;
> > + WARN_ONCE(ret != 0 && !IS_ERR_VALUE(ret + 0UL),
> > + "%s: ret = %d\n", __func__, ret);
> > + scsi_req(req)->result = ret ? DID_ERROR << 16 : 0;
> >
> > blk_end_request_all(req, 0);
> >
> >
>
> Weelll ... I'd rather audit the handler so as to ensure that the correct
> value is returned.
> And this 'ret + 0UL' construct is decidedly ugly ...
Hello Hannes,
Changing "+ 0UL" into an explicit (unsigned long) cast is easy. But I would
prefer to leave the conversion of the smp_handler functions to someone who
has the hardware available to test such a conversion. These are the smp_handler
implementations I am aware of:
$ git grep -nH '\.smp_handler[[:blank:]]*='
drivers/message/fusion/mptsas.c:2356: .smp_handler=
mptsas_smp_handler,
drivers/scsi/hpsa.c:9463: .smp_handler = hpsa_sas_smp_handler,
drivers/scsi/libsas/sas_init.c:548: .smp_handler = sas_smp_handler,
drivers/scsi/mpt3sas/mpt3sas_transport.c:2129: .smp_handler=
_transport_smp_handler,
drivers/scsi/smartpqi/smartpqi_sas_transport.c:349: .smp_handler =
pqi_sas_smp_handler,
Bart.
Re: [PATCH] Improve requeuing behavior
On Thu, 2017-08-24 at 14:09 +0200, Christoph Hellwig wrote:
> On Wed, Aug 23, 2017 at 02:05:35PM -0700, Bart Van Assche wrote:
> > Requests are unprepared and reprepared when being requeued. Avoid
> > that requeuing resets .jiffies_at_alloc and .retries by initializing
> > these two member variables from inside blk_get_request() and by
> > preserving both member variables when preparing a request. This patch
> > affects the requeuing behavior of scsi-sq and scsi-mq.
> >
> > Reported-by: Brian King
> > References: https://lkml.org/lkml/2017/8/18/923 ("Re: [BUG][bisected
> > 270065e] linux-next fails to boot on powerpc")
> > Signed-off-by: Bart Van Assche
> > Cc: Brian King
> > Cc: Hannes Reinecke
> > Cc: Christoph Hellwig
> > Cc: Johannes Thumshirn
> > ---
> > drivers/scsi/scsi_lib.c | 7 ++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index ebc5c713ee37..8d1ec1e7b0e2 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -1122,6 +1122,8 @@ void scsi_initialize_rq(struct request *rq)
> > struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
> >
> > scsi_req_init(&cmd->req);
> > + cmd->jiffies_at_alloc = jiffies;
> > + cmd->retries = 0;
> > }
> > EXPORT_SYMBOL(scsi_initialize_rq);
>
> How is this working for non-passthrough commands where we don't
> call scsi_initialize_rq?
Hello Christoph,
Commit ca18d6f769d2 ("block: Make most scsi_req_init() calls implicit")
introduced the scsi_initialize_rq() function in such a way that it is not only
called for pass-through requests but also for FS requests. Because that commit
sets .initialize_rq_fn for both blk-sq and blk-mq blk_get_request() now calls
scsi_initialize_rq() for all SCSI requests. Or did I perhaps overlook something?
Bart.
Re: [PATCH] scsi: lpfc: remove useless code in lpfc_sli4_bsg_link_diag_test
Hi Martin, On 08/23/2017 09:45 PM, Martin K. Petersen wrote: Gustavo, Remove variable assignments. The value stored in local variable _rc_ is overwritten at line 2448:rc = lpfc_sli4_bsg_set_link_diag_state(phba, 0); before it can be used. Applied to 4.14/scsi-queue. Thank you! Glad to help. :) -- Gustavo A. R. Silva
Re: [PATCH 05/19] scsi_setup_fs_cmnd(): Call scsi_req_init() instead of open-coding it
On Thu, 2017-08-24 at 11:05 +0200, Christoph Hellwig wrote: > I don't really see the point of doing more work in exactly the same > amount of code here, especially given that this is the fast path. > > Do you need this for any future patches? Hello Christoph, I will drop this patch. Bart.
Re: [PATCH 08/15] PCI: make device_type const
On Sat, Aug 19, 2017 at 01:52:19PM +0530, Bhumika Goyal wrote:
> Make this const as it is only stored in the type field of a device
> structure, which is const.
> Done using Coccinelle.
>
> Signed-off-by: Bhumika Goyal
Applied to pci/misc for v4.14, thanks!
> ---
> drivers/pci/endpoint/pci-epf-core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/endpoint/pci-epf-core.c
> b/drivers/pci/endpoint/pci-epf-core.c
> index 6877d6a..9d0de12 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -27,7 +27,7 @@
> #include
>
> static struct bus_type pci_epf_bus_type;
> -static struct device_type pci_epf_type;
> +static const struct device_type pci_epf_type;
>
> /**
> * pci_epf_linkup() - Notify the function driver that EPC device has
> @@ -275,7 +275,7 @@ static void pci_epf_dev_release(struct device *dev)
> kfree(epf);
> }
>
> -static struct device_type pci_epf_type = {
> +static const struct device_type pci_epf_type = {
> .release= pci_epf_dev_release,
> };
>
> --
> 1.9.1
>
Re: [PATCH v2] scsi: lpfc: avoid false positive gcc-8 warning
On 8/24/2017 4:01 AM, Arnd Bergmann wrote: I have also come up with a different workaround of my own (sorry for the broken formatting here) and tested it successfully over night. I have definitely spent more time on it than it was worth now. Let me know if you prefer that version over yours, then I'll submit that as a proper patch with your Ack. Signed-off-by: Arnd Bergmann --- a/drivers/scsi/lpfc/lpfc_debugfs.h I'm ok with either solution. I prefer less change but this is a trivial thing. Signed-off-by: James Smart -- james
Re: [PATCH 17/19] scsi_transport_srp: Suppress a W=1 compiler warning
On Thu, 2017-08-24 at 11:11 +0200, Christoph Hellwig wrote: > On Wed, Aug 23, 2017 at 02:40:07PM -0700, Bart Van Assche wrote: > > Avoid that the following compiler warning is reported when building > > with W=1: > > > > drivers/scsi/scsi_transport_srp.c:92:19: warning: comparison is always > > false due to limited range of data type [-Wtype-limits] > > > > Signed-off-by: Bart Van Assche > > Cc: Christoph Hellwig > > Cc: Hannes Reinecke > > Cc: Johannes Thumshirn > > --- > > drivers/scsi/scsi_transport_srp.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/scsi/scsi_transport_srp.c > > b/drivers/scsi/scsi_transport_srp.c > > index 698cc4681706..b8f5e4c47579 100644 > > --- a/drivers/scsi/scsi_transport_srp.c > > +++ b/drivers/scsi/scsi_transport_srp.c > > @@ -89,7 +89,7 @@ int srp_tmo_valid(int reconnect_delay, int > > fast_io_fail_tmo, int dev_loss_tmo) > > if (fast_io_fail_tmo < 0 && > > dev_loss_tmo > SCSI_DEVICE_BLOCK_MAX_TIMEOUT) > > return -EINVAL; > > - if (dev_loss_tmo >= LONG_MAX / HZ) > > + if (dev_loss_tmo + 0UL >= LONG_MAX / HZ) > > return -EINVAL; > > That's a weird change.. If we can to promote dev_loss_tmo to > long we should just cast it. And of course we should always ask > us why we got this warning. If long is 64-bit and int is 32-bit > the warning makes sense, as for every reasonable comparism > the 32-bit timeout can't be larger than LONG_MAX divided by 100 or 100. > > But for 32-bit longs it can, so we should keep it, maybe with a comment. Hello Christoph, The purpose of that check is to avoid that dev_loss_tmo * HZ can overflow. That check is only needed on 32-bit systems since only on these systems sizeof(long) == sizeof(int). How about changing the type of the dev_loss_tmo argument from int to long such that no explicit cast is needed? Thanks, Bart.
Re: [PATCH 03/15] [media] i2c: make device_type const
On Sat, 19 Aug 2017, Bhumika Goyal wrote:
> Make this const as it is only stored in the type field of a device
> structure, which is const.
> Done using Coccinelle.
>
> Signed-off-by: Bhumika Goyal
Acked-by: Guennadi Liakhovetski
Thanks
Guennadi
> ---
> drivers/media/i2c/soc_camera/mt9t031.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/soc_camera/mt9t031.c
> b/drivers/media/i2c/soc_camera/mt9t031.c
> index 714fb35..4802d30 100644
> --- a/drivers/media/i2c/soc_camera/mt9t031.c
> +++ b/drivers/media/i2c/soc_camera/mt9t031.c
> @@ -592,7 +592,7 @@ static int mt9t031_runtime_resume(struct device *dev)
> .runtime_resume = mt9t031_runtime_resume,
> };
>
> -static struct device_type mt9t031_dev_type = {
> +static const struct device_type mt9t031_dev_type = {
> .name = "MT9T031",
> .pm = &mt9t031_dev_pm_ops,
> };
> --
> 1.9.1
>
Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]
On Thu, 2017-08-24 at 11:07 +0200, Christoph Hellwig wrote: > On Wed, Aug 23, 2017 at 02:39:57PM -0700, Bart Van Assche wrote: > > Only annotate pointers that are shared across threads with __rcu. > > Use rcu_dereference() when dereferencing an RCU pointer. Protect > > also the RCU pointer dereferences when freeing RCU pointers. This > > patch suppresses about twenty sparse complaints about the vpd_pg8[03] > > pointers. > > Shouldn't the kfrees be kfree_rcu? or where else is the rcu protection > for them? Hello Christoph, My understanding of the SCSI VPD code is as follows: * rcu_read_lock() / rcu_read_unlock() is used to prevent that another thread updates a VPD buffer while it is being read. * All code that either updates or reads a VPD buffer holds a reference on the SCSI device that buffer is associated with. That is why I think it is not needed to use kfree_rcu() in scsi_device_dev_release_usercontext(). Bart.
Re: [PATCH] Improve requeuing behavior
On Thu, Aug 24, 2017 at 04:14:22PM +, Bart Van Assche wrote:
> Commit ca18d6f769d2 ("block: Make most scsi_req_init() calls implicit")
> introduced the scsi_initialize_rq() function in such a way that it is not only
> called for pass-through requests but also for FS requests. Because that commit
> sets .initialize_rq_fn for both blk-sq and blk-mq blk_get_request() now calls
> scsi_initialize_rq() for all SCSI requests. Or did I perhaps overlook
> something?
initialize_rq_fn is only called from blk_get_request, which is not
called for normal file system read/write/flush/discard request.
Also unless my memory fails me that's something that I and Jens requested
to be changed from your original version.
Re: [PATCH 06/19] Document which queue type a function is intended for
On Thu, Aug 24, 2017 at 04:57:03PM +, Bart Van Assche wrote: > Is something like the patch below perhaps what you had in mind? Yes. Except that I really don't like the sq naming - blk-mq can also be used for single queues, so it's a rather confusing name Something like legacy or old seems to be more suitable.
Re: [PATCH 06/19] Document which queue type a function is intended for
On Thu, 2017-08-24 at 11:05 +0200, Christoph Hellwig wrote:
> On Wed, Aug 23, 2017 at 02:39:56PM -0700, Bart Van Assche wrote:
> > Document which queue type a function is intended for if this is not
> > easy to derive from the function name.
>
> How about adding mq and legacy to the function names instead?
Hello Christoph,
Is something like the patch below perhaps what you had in mind?
[PATCH] Document which queue type a function is intended for
Rename several functions to make it easy to see which queue type a
function is intended for.
Signed-off-by: Bart Van Assche
Cc: Christoph Hellwig
Cc: Hannes Reinecke
Cc: Johannes Thumshirn
---
drivers/scsi/scsi_lib.c | 23 ---
drivers/scsi/scsi_priv.h | 2 +-
drivers/scsi/scsi_scan.c | 2 +-
3 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index efdcd9e79404..f1589a0488ec 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2006,8 +2006,8 @@ static enum blk_eh_timer_return scsi_timeout(struct
request *req,
return scsi_times_out(req);
}
-static int scsi_init_request(struct blk_mq_tag_set *set, struct request *rq,
- unsigned int hctx_idx, unsigned int numa_node)
+static int scsi_mq_init_request(struct blk_mq_tag_set *set, struct request *rq,
+ unsigned int hctx_idx, unsigned int numa_node)
{
struct Scsi_Host *shost = set->driver_data;
const bool unchecked_isa_dma = shost->unchecked_isa_dma;
@@ -2031,8 +2031,8 @@ static int scsi_init_request(struct blk_mq_tag_set *set,
struct request *rq,
return 0;
}
-static void scsi_exit_request(struct blk_mq_tag_set *set, struct request *rq,
- unsigned int hctx_idx)
+static void scsi_mq_exit_request(struct blk_mq_tag_set *set, struct request
*rq,
+unsigned int hctx_idx)
{
struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
@@ -2109,7 +2109,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct
request_queue *q)
}
EXPORT_SYMBOL_GPL(__scsi_init_queue);
-static int scsi_init_rq(struct request_queue *q, struct request *rq, gfp_t gfp)
+static int scsi_sq_init_rq(struct request_queue *q, struct request *rq,
+ gfp_t gfp)
{
struct Scsi_Host *shost = q->rq_alloc_data;
const bool unchecked_isa_dma = shost->unchecked_isa_dma;
@@ -2139,7 +2140,7 @@ static int scsi_init_rq(struct request_queue *q, struct
request *rq, gfp_t gfp)
return -ENOMEM;
}
-static void scsi_exit_rq(struct request_queue *q, struct request *rq)
+static void scsi_sq_exit_rq(struct request_queue *q, struct request *rq)
{
struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
@@ -2149,7 +2150,7 @@ static void scsi_exit_rq(struct request_queue *q, struct
request *rq)
cmd->sense_buffer);
}
-struct request_queue *scsi_alloc_queue(struct scsi_device *sdev)
+struct request_queue *scsi_sq_alloc_queue(struct scsi_device *sdev)
{
struct Scsi_Host *shost = sdev->host;
struct request_queue *q;
@@ -2160,8 +2161,8 @@ struct request_queue *scsi_alloc_queue(struct scsi_device
*sdev)
q->cmd_size = sizeof(struct scsi_cmnd) + shost->hostt->cmd_size;
q->rq_alloc_data = shost;
q->request_fn = scsi_request_fn;
- q->init_rq_fn = scsi_init_rq;
- q->exit_rq_fn = scsi_exit_rq;
+ q->init_rq_fn = scsi_sq_init_rq;
+ q->exit_rq_fn = scsi_sq_exit_rq;
q->initialize_rq_fn = scsi_initialize_rq;
if (blk_init_allocated_queue(q) < 0) {
@@ -2185,8 +2186,8 @@ static const struct blk_mq_ops scsi_mq_ops = {
#ifdef CONFIG_BLK_DEBUG_FS
.show_rq= scsi_show_rq,
#endif
- .init_request = scsi_init_request,
- .exit_request = scsi_exit_request,
+ .init_request = scsi_mq_init_request,
+ .exit_request = scsi_mq_exit_request,
.initialize_rq_fn = scsi_initialize_rq,
.map_queues = scsi_map_queues,
};
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index c11c1f9c912c..cdf166634b6f 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -88,7 +88,7 @@ extern void scsi_queue_insert(struct scsi_cmnd *cmd, int
reason);
extern void scsi_io_completion(struct scsi_cmnd *, unsigned int);
extern void scsi_run_host_queues(struct Scsi_Host *shost);
extern void scsi_requeue_run_queue(struct work_struct *work);
-extern struct request_queue *scsi_alloc_queue(struct scsi_device *sdev);
+extern struct request_queue *scsi_sq_alloc_queue(struct scsi_device *sdev);
extern struct request_queue *scsi_mq_alloc_queue(struct scsi_device *sdev);
extern void scsi_start_queue(struct scsi_device *sdev);
extern int scsi_mq_setup_tags(struct Scsi_Host *shost);
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index fd88dabd599d..b331726a6eb4 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi
Re: [PATCH] Improve requeuing behavior
On Thu, 2017-08-24 at 18:57 +0200, h...@lst.de wrote:
> On Thu, Aug 24, 2017 at 04:14:22PM +, Bart Van Assche wrote:
> > Commit ca18d6f769d2 ("block: Make most scsi_req_init() calls implicit")
> > introduced the scsi_initialize_rq() function in such a way that it is not
> > only
> > called for pass-through requests but also for FS requests. Because that
> > commit
> > sets .initialize_rq_fn for both blk-sq and blk-mq blk_get_request() now
> > calls
> > scsi_initialize_rq() for all SCSI requests. Or did I perhaps overlook
> > something?
>
> initialize_rq_fn is only called from blk_get_request, which is not
> called for normal file system read/write/flush/discard request.
>
> Also unless my memory fails me that's something that I and Jens requested
> to be changed from your original version.
Hello Christoph,
Is one of the following two approaches perhaps what you had in mind?
* Adding an .initialize_rq_fn() call to multiple blk_rq_init() and
blk_mq_rq_ctx_init() callers, e.g. blk_mq_make_request(), __get_request(),
blk_kick_flush() and scsi_ioctl_reset().
* Moving the .initialize_rq_fn() call from blk_get_request() into blk_rq_init()
and blk_mq_rq_ctx_init() (see also
https://www.spinics.net/lists/linux-scsi/msg108934.html).
Thanks,
Bart.
Re: [PATCH 06/19] Document which queue type a function is intended for
On Thu, 2017-08-24 at 18:58 +0200, h...@lst.de wrote: > On Thu, Aug 24, 2017 at 04:57:03PM +, Bart Van Assche wrote: > > Is something like the patch below perhaps what you had in mind? > > Yes. Except that I really don't like the sq naming - blk-mq can > also be used for single queues, so it's a rather confusing name > Something like legacy or old seems to be more suitable. Hello Christoph, Unless anyone prefers otherwise I will use "old" because the same word is already used in the device mapper to refer to legacy request processing code. Bart.
Re: [PATCH 11/15] remoteproc: make device_type const
On Sat 19 Aug 01:22 PDT 2017, Bhumika Goyal wrote:
> Make this const as it is only stored in the type field of a device
> structure, which is const.
> Done using Coccinelle.
>
> Signed-off-by: Bhumika Goyal
Applied, thanks.
Regards,
Bjorn
> ---
> drivers/remoteproc/remoteproc_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index 364ef28..48b2c5d 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1360,7 +1360,7 @@ static void rproc_type_release(struct device *dev)
> kfree(rproc);
> }
>
> -static struct device_type rproc_type = {
> +static const struct device_type rproc_type = {
> .name = "remoteproc",
> .release= rproc_type_release,
> };
> --
> 1.9.1
>
Re: [Patch v2 1/2] libiscsi: Fix use-after-free race during iscsi_session_teardown
On Thu, Jul 13, 2017 at 9:11 AM, Khazhismel Kumykov wrote: Ping in case this was missed smime.p7s Description: S/MIME Cryptographic Signature
[bug report] scsi: lpfc: Fix handling of FCP and NVME FC4 types in Pt2Pt topology
Hello Dick Kennedy, This is a semi-automatic email about new static checker warnings. The patch 8fd5f79a9cf4: "scsi: lpfc: Fix handling of FCP and NVME FC4 types in Pt2Pt topology" from Aug 23, 2017, leads to the following Smatch complaint: drivers/scsi/lpfc/lpfc_els.c:2010 lpfc_issue_els_plogi() error: we previously assumed 'ndlp' could be null (see line 1998) drivers/scsi/lpfc/lpfc_els.c 1997 ndlp = lpfc_findnode_did(vport, did); 1998 if (ndlp && !NLP_CHK_NODE_ACT(ndlp)) 1999 ndlp = NULL; ^^^ ndlp can be NULL. 2000 2001 /* If ndlp is not NULL, we will bump the reference count on it */ 2002 cmdsize = (sizeof(uint32_t) + sizeof(struct serv_parm)); 2003 elsiocb = lpfc_prep_els_iocb(vport, 1, cmdsize, retry, ndlp, did, 2004 ELS_CMD_PLOGI); 2005 if (!elsiocb) 2006 return 1; 2007 2008 shost = lpfc_shost_from_vport(vport); 2009 spin_lock_irq(shost->host_lock); 2010 ndlp->nlp_flag &= ~NLP_FCP_PRLI_RJT; ^^ We added a new unchecked dereference. 2011 spin_unlock_irq(shost->host_lock); 2012 regards, dan carpenter
[bug report] scsi: qla2xxx: Cleanup NPIV host in target mode during config teardown
Hello Quinn Tran,
This is a semi-automatic email about new static checker warnings.
The patch 821c9f4cab5e: "scsi: qla2xxx: Cleanup NPIV host in target
mode during config teardown" from Aug 23, 2017, leads to the
following Smatch complaint:
drivers/scsi/qla2xxx/qla_target.c:1553 qlt_release()
error: we previously assumed 'vha->vha_tgt.qla_tgt' could be null (see line
1533)
drivers/scsi/qla2xxx/qla_target.c
1532
1533 if ((vha->vha_tgt.qla_tgt != NULL) && !tgt->tgt_stop &&
1534 !tgt->tgt_stopped)
1535 qlt_stop_phase1(tgt);
1536
1537 if ((vha->vha_tgt.qla_tgt != NULL) && !tgt->tgt_stopped)
^^
Existing code assumes this can be NULL.
1538 qlt_stop_phase2(tgt);
1539
1540 for (i = 0; i < vha->hw->max_qpairs + 1; i++) {
1541 unsigned long flags;
1542
1543 h = &tgt->qphints[i];
1544 if (h->qpair) {
1545 spin_lock_irqsave(h->qpair->qp_lock_ptr, flags);
1546 list_del(&h->hint_elem);
1547 spin_unlock_irqrestore(h->qpair->qp_lock_ptr,
flags);
1548 h->qpair = NULL;
1549 }
1550 }
1551 kfree(tgt->qphints);
1552 mutex_lock(&qla_tgt_mutex);
1553 list_del(&vha->vha_tgt.qla_tgt->tgt_list_entry);\
The patch adds a new dereference.
1554 mutex_unlock(&qla_tgt_mutex);
1555
regards,
dan carpenter
[PATCH] scsi: Fix the kerneldoc for scsi_initialize_rq()
The kerneldoc comment for scsi_initialize_rq() neglected to document the "rq" parameter, leading to this docs build warning: ./drivers/scsi/scsi_lib.c:1116: warning: No description found for parameter 'rq' Document the parameter and make the build slightly quieter. Signed-off-by: Jonathan Corbet --- 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 f6097b89d5d3..a76515814ee7 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1108,6 +1108,7 @@ EXPORT_SYMBOL(scsi_init_io); /** * scsi_initialize_rq - initialize struct scsi_cmnd.req + * @rq: Request used to locate the scsi_cmnd structure. * * Called from inside blk_get_request(). */ -- 2.13.4
[PATCH v3 2/2] scsi: lpfc: avoid false-positive gcc-8 warning
This is an interesting regression with gcc-8, showing a harmless
warning for correct code:
In file included from include/linux/kernel.h:13:0,
...
from drivers/scsi/lpfc/lpfc_debugfs.c:23:
include/linux/printk.h:301:2: error: 'eq' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
^~
In file included from drivers/scsi/lpfc/lpfc_debugfs.c:58:0:
drivers/scsi/lpfc/lpfc_debugfs.h:451:31: note: 'eq' was declared here
I managed to reduce the warning into a small test case for
gcc-8 that I reported in the gcc bugzilla[1].
As a workaround, this changes the logic to move the two assignments
of 'eq' out of the conditions and instead make the index conditional.
This works for all configurations I tried and avoids adding a bogus
initialization.
Acked-by: James Smart
Link: [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81958
Signed-off-by: Arnd Bergmann
---
drivers/scsi/lpfc/lpfc_debugfs.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/lpfc/lpfc_debugfs.h b/drivers/scsi/lpfc/lpfc_debugfs.h
index 7b7d314af0e0..c4edd87bfc65 100644
--- a/drivers/scsi/lpfc/lpfc_debugfs.h
+++ b/drivers/scsi/lpfc/lpfc_debugfs.h
@@ -478,16 +478,16 @@ lpfc_debug_dump_cq(struct lpfc_hba *phba, int qtype, int
wqidx)
return;
for (eqidx = 0; eqidx < phba->io_channel_irqs; eqidx++) {
- eq = phba->sli4_hba.hba_eq[eqidx];
- if (cq->assoc_qid == eq->queue_id)
+ if (cq->assoc_qid == phba->sli4_hba.hba_eq[eqidx]->queue_id)
break;
}
if (eqidx == phba->io_channel_irqs) {
pr_err("Couldn't find EQ for CQ. Using EQ[0]\n");
eqidx = 0;
- eq = phba->sli4_hba.hba_eq[0];
}
+ eq = phba->sli4_hba.hba_eq[eqidx];
+
if (qtype == DUMP_FCP || qtype == DUMP_NVME)
pr_err("%s CQ: WQ[Idx:%d|Qid%d]->CQ[Idx%d|Qid%d]"
"->EQ[Idx:%d|Qid:%d]:\n",
--
2.9.0
[PATCH 1/2] scsi: lpfc: avoid an unused function warning
The only reference to lpfc_nvmet_replenish_context() is inside of
an #ifdef, leading to a harmless warning when CONFIG_NVME_TARGET_FC
is disabled:
drivers/scsi/lpfc/lpfc_nvmet.c:1457:1: error: 'lpfc_nvmet_replenish_context'
defined but not used [-Werror=unused-function]
This replaces the preprocessor conditional with a C condition,
so the compiler can see that the function is intentionally unused.
Fixes: 9a38e4f1c82f ("scsi: lpfc: Fix MRQ > 1 context list handling")
Signed-off-by: Arnd Bergmann
---
drivers/scsi/lpfc/lpfc_nvmet.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/lpfc/lpfc_nvmet.c b/drivers/scsi/lpfc/lpfc_nvmet.c
index 48a1c0c27a3f..0b7c1a49e203 100644
--- a/drivers/scsi/lpfc/lpfc_nvmet.c
+++ b/drivers/scsi/lpfc/lpfc_nvmet.c
@@ -1527,7 +1527,6 @@ lpfc_nvmet_unsol_fcp_buffer(struct lpfc_hba *phba,
struct rqb_dmabuf *nvmebuf,
uint64_t isr_timestamp)
{
-#if (IS_ENABLED(CONFIG_NVME_TARGET_FC))
struct lpfc_nvmet_rcv_ctx *ctxp;
struct lpfc_nvmet_tgtport *tgtp;
struct fc_frame_header *fc_hdr;
@@ -1541,6 +1540,9 @@ lpfc_nvmet_unsol_fcp_buffer(struct lpfc_hba *phba,
uint32_t id;
#endif
+ if (!IS_ENABLED(CONFIG_NVME_TARGET_FC))
+ return;
+
ctx_buf = NULL;
if (!nvmebuf || !phba->targetport) {
lpfc_printf_log(phba, KERN_ERR, LOG_NVME_IOERR,
@@ -1695,7 +1697,6 @@ lpfc_nvmet_unsol_fcp_buffer(struct lpfc_hba *phba,
if (nvmebuf)
lpfc_rq_buf_free(phba, &nvmebuf->hbuf); /* repost */
-#endif
}
/**
--
2.9.0
Re: [PATCH] scsi: Fix the kerneldoc for scsi_initialize_rq()
On Thu, 2017-08-24 at 16:11 -0600, Jonathan Corbet wrote: > /** > * scsi_initialize_rq - initialize struct scsi_cmnd.req > + * @rq: Request used to locate the scsi_cmnd structure. > * > * Called from inside blk_get_request(). > */ Since the request structure and the SCSI command structure are adjacent, how about the following: @rq: Request associated with the SCSI command to be initialized. Thanks, Bart.
RE: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03]
> -Original Message- > From: Bart Van Assche [mailto:bart.vanass...@wdc.com] > Sent: Friday, August 25, 2017 2:54 AM > To: h...@lst.de > Cc: j...@linux.vnet.ibm.com; linux-scsi@vger.kernel.org; h...@suse.de; > jthumsh...@suse.de; martin.peter...@oracle.com; Seymour, Shane M > > Subject: Re: [PATCH 07/19] Fix RCU handling of scsi_device.vpd_pg8[03] > > On Thu, 2017-08-24 at 11:07 +0200, Christoph Hellwig wrote: > > On Wed, Aug 23, 2017 at 02:39:57PM -0700, Bart Van Assche wrote: > > > Only annotate pointers that are shared across threads with __rcu. > > > Use rcu_dereference() when dereferencing an RCU pointer. Protect > > > also the RCU pointer dereferences when freeing RCU pointers. This > > > patch suppresses about twenty sparse complaints about the > > > vpd_pg8[03] pointers. > > > > Shouldn't the kfrees be kfree_rcu? or where else is the rcu > > protection for them? > > Hello Christoph, > Hi Bart, > My understanding of the SCSI VPD code is as follows: > * rcu_read_lock() / rcu_read_unlock() is used to prevent that another thread > updates a VPD buffer while it is being read. My understanding is that it doesn't do that - you can update an RCU pointer with rcu_assign_pointer() after someone has called rcu_read_lock() and before they call rcu_read_unlock(). What rcu_read_lock() / rcu_read_unlock() do is mark a read-side critical section when accessing an RCU data item. If you have 2 CPUs in a read-side critical section and a 3rd CPU replacing the pointer using rcu_assign_pointer() one CPU could potentially end up with the old pointer and the other one with the new pointer or both old or both new (the only guarantee you have is that the pointer won't be partially updated with bits of old and the new pointer). To free the old pointer directly you have to call synchronize_rcu() after which you can call kfree() or if you don't call synchronize_rcu() you have to use a delayed freeing mechanism like kfree_rcu() so you can guarantee that the old pointer is still valid while used in a read-side critical section. Using something like kfree_rcu() means that you also don’t have to wait like I believe you can do if you call synchronize_rcu() since you could be forced to wait for a RCU grace period to end before you can call kfree(). So what they really do is ensure that someone updating the pointer can't free the old pointer (in case someone is using it) until everyone has left their read-side critical section (if the read-side critical section started before synchronize_rcu() was called). You'll find a good example here that uses synchronize_rcu(): https://www.kernel.org/doc/Documentation/RCU/whatisRCU.txt Search for " WHAT ARE SOME EXAMPLE USES OF CORE RCU API?" - it has a lot of other information about RCU. > * All code that either updates or reads a VPD buffer holds a reference on > the SCSI device that buffer is associated with. That is why I think it is > not needed to use kfree_rcu() in scsi_device_dev_release_usercontext(). The only counter argument I'd put up into why it shouldn't be done the way you want to is that if someone else sees that code and doesn't understand the context and can't guarantee similar to this situation where all references to the structure should have already been dropped and think that it's ok to directly kfree something returned from rcu_dereference() when it's something that they really shouldn't do. The only real difference is that kfree_rcu will return the memory for reuse when RCU processing gets done in softirq and what you're doing will do it immediately. It's easier to use kfree_rcu() from what I can see. It is possible I may be being too picky (it's a personal failing sometimes) but is it really that a large overhead to free the RCU pointers in a way that RCU pointers are expected to work even if the pointers shouldn't be accessible to anything? Shane > > Bart. >
