Re: [PATCH v2 00/15] qla2xxx: Add Target Multiqueue support

[Nicholas A. Bellinger]

Hey guys,

Apologies for missing this earlier.  Comments below.

On Tue, 2017-06-13 at 20:47 -0700, Himanshu Madhani wrote:
> Hi Martin, Nic,
> 
> This patch series adds support for multiqueue for qla2xxx target mode driver.
> 
> This series depends on the seris applied to Martin's scsi/for-next branch
> (https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/log/?h=for-next)
> 
> I've also added followig patch ("qla2xxx: Include Exchange offload/Extended
> Login into FW dump") which was dropped from earlier series for rework.
> 
> I am planning to send series for FC-NVME support as well which depends on this
> series, so I would like this series to be applied to scsi tree instead of
> target-pending for only this submission, as both series touches common code
> in qla2xxx driver. 
> 
> Please apply this series to for-next for inclusion in 4.13 merge window.
> 
> Note: Comment from Bart for patch#1 will be addresed as new patch after our 
>   FC-NVME series is posted. 
> 
> Changes from v1 --> v2
> 
> o Fixed 0-day kernel compile failure
> 
> Thanks,
> Himanshu 
> 
> Himanshu Madhani (1):
>   qla2xxx: Update driver version to 9.01.00.00-k
> 
> Quinn Tran (13):
>   qla2xxx: Combine Active command arrays.
>   qla2xxx: Preparation for Target MQ.
>   qla2xxx: Enable Target Multi Queue
>   qla2xxx: Add debug knob for user control workload
>   qla2xxx: Add fw_started flags to qpair
>   qla2xxx: move fields from qla_hw_data to qla_qpair
>   qla2xxx: use shadow register for ISP27XX
>   qla2xxx: Add function call to qpair for door bell
>   qla2xxx: Add debug logging routine for qpair
>   qla2xxx: Remove unused tgt_enable_64bit_addr flag
>   qla2xxx: Remove datasegs_per_cmd and datasegs_per_cont field
>   qla2xxx: Move target stat counters from vha to qpair.
>   qla2xxx: Include Exchange offload/Extended Login into FW dump
> 
> Sawan Chandak (1):
>   qla2xxx: Fix mailbox failure while deleting Queue pairs
> 
>  drivers/scsi/qla2xxx/qla_attr.c|4 +-
>  drivers/scsi/qla2xxx/qla_dbg.c |  150 ++
>  drivers/scsi/qla2xxx/qla_dbg.h |   17 +
>  drivers/scsi/qla2xxx/qla_def.h |  119 -
>  drivers/scsi/qla2xxx/qla_dfs.c |  137 -
>  drivers/scsi/qla2xxx/qla_gbl.h |   19 +-
>  drivers/scsi/qla2xxx/qla_init.c|   53 +-
>  drivers/scsi/qla2xxx/qla_inline.h  |   44 ++
>  drivers/scsi/qla2xxx/qla_iocb.c|   32 +-
>  drivers/scsi/qla2xxx/qla_isr.c |   93 +++-
>  drivers/scsi/qla2xxx/qla_mid.c |   44 +-
>  drivers/scsi/qla2xxx/qla_os.c  |  156 --
>  drivers/scsi/qla2xxx/qla_target.c  | 1028 
> +---
>  drivers/scsi/qla2xxx/qla_target.h  |   50 +-
>  drivers/scsi/qla2xxx/qla_version.h |4 +-
>  drivers/scsi/qla2xxx/tcm_qla2xxx.c |8 +-
>  16 files changed, 1348 insertions(+), 610 deletions(-)
> 

If it hasn't already been merged to MKP's tree, then:

Acked-by: Nicholas Bellinger 

Btw, I don't think my missing ACK should hold up qla2xxx patches from
being merged in the future, especially ones that are exposing new
qla2xxx specific functionality.

The only ones that would really need my ACK are ones specific to
interaction with target-core.

[PATCH] iscsi-target: Add login_keys_workaround attribute for non RFC initiators

[Nicholas A. Bellinger]

From: Nicholas Bellinger 

This patch re-introduces part of a long standing login workaround that
was recently dropped by:

  commit 1c99de981f30b3e7868b8d20ce5479fa1c0fea46
  Author: Nicholas Bellinger 
  Date:   Sun Apr 2 13:36:44 2017 -0700

  iscsi-target: Drop work-around for legacy GlobalSAN initiator

Namely, the workaround for FirstBurstLength ended up being required by
Mellanox Flexboot PXE boot ROMs as reported by Robert.

So this patch re-adds the work-around for FirstBurstLength within
iscsi_check_proposer_for_optional_reply(), and makes the key optional
to respond when the initiator does not propose, nor respond to it.

Also as requested by Arun, this patch introduces a new TPG attribute
named 'login_keys_workaround' that controls the use of both the
FirstBurstLength workaround, as well as the two other existing
workarounds for gPXE iSCSI boot client.

By default, the workaround is enabled with login_keys_workaround=1,
since Mellanox FlexBoot requires it, and Arun has verified the Qlogic
MSFT initiator already proposes FirstBurstLength, so it's uneffected
by this re-adding this part of the original work-around.

Reported-by: Robert LeBlanc 
Cc: Robert LeBlanc 
Cc: Arun Easi 
Signed-off-by: Nicholas Bellinger 
---
 drivers/target/iscsi/iscsi_target_configfs.c   |  2 ++
 drivers/target/iscsi/iscsi_target_nego.c   |  6 ++--
 drivers/target/iscsi/iscsi_target_parameters.c | 41 ++
 drivers/target/iscsi/iscsi_target_parameters.h |  2 +-
 drivers/target/iscsi/iscsi_target_tpg.c| 19 
 drivers/target/iscsi/iscsi_target_tpg.h|  1 +
 include/target/iscsi/iscsi_target_core.h   |  9 ++
 7 files changed, 64 insertions(+), 16 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_configfs.c 
b/drivers/target/iscsi/iscsi_target_configfs.c
index 535a8e0..0dd4c45 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -781,6 +781,7 @@ static int lio_target_init_nodeacl(struct se_node_acl 
*se_nacl,
 DEF_TPG_ATTRIB(t10_pi);
 DEF_TPG_ATTRIB(fabric_prot_type);
 DEF_TPG_ATTRIB(tpg_enabled_sendtargets);
+DEF_TPG_ATTRIB(login_keys_workaround);
 
 static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
_tpg_attrib_attr_authentication,
@@ -796,6 +797,7 @@ static int lio_target_init_nodeacl(struct se_node_acl 
*se_nacl,
_tpg_attrib_attr_t10_pi,
_tpg_attrib_attr_fabric_prot_type,
_tpg_attrib_attr_tpg_enabled_sendtargets,
+   _tpg_attrib_attr_login_keys_workaround,
NULL,
 };
 
diff --git a/drivers/target/iscsi/iscsi_target_nego.c 
b/drivers/target/iscsi/iscsi_target_nego.c
index 96df63f..7a6751f 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -864,7 +864,8 @@ static int iscsi_target_handle_csg_zero(
SENDER_TARGET,
login->rsp_buf,
>rsp_length,
-   conn->param_list);
+   conn->param_list,
+   conn->tpg->tpg_attrib.login_keys_workaround);
if (ret < 0)
return -1;
 
@@ -934,7 +935,8 @@ static int iscsi_target_handle_csg_one(struct iscsi_conn 
*conn, struct iscsi_log
SENDER_TARGET,
login->rsp_buf,
>rsp_length,
-   conn->param_list);
+   conn->param_list,
+   conn->tpg->tpg_attrib.login_keys_workaround);
if (ret < 0) {
iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_INITIATOR_ERR,
ISCSI_LOGIN_STATUS_INIT_ERR);
diff --git a/drivers/target/iscsi/iscsi_target_parameters.c 
b/drivers/target/iscsi/iscsi_target_parameters.c
index fce6276..caab104 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -765,7 +765,8 @@ static int iscsi_check_for_auth_key(char *key)
return 0;
 }
 
-static void iscsi_check_proposer_for_optional_reply(struct iscsi_param *param)
+static void iscsi_check_proposer_for_optional_reply(struct iscsi_param *param,
+   bool keys_workaround)
 {
if (IS_TYPE_BOOL_AND(param)) {
if (!strcmp(param->value, NO))
@@ -773,19 +774,31 @@ static void 
iscsi_check_proposer_for_optional_reply(struct iscsi_param *param)
} else if (IS_TYPE_BOOL_OR(param)) {
if (!strcmp(param->value, YES))
SET_PSTATE_REPLY_OPTIONAL(param);
-/*
- * Required for gPXE iSCSI boot client
- */
-   if (!strcmp(param->name, IMMEDIATEDATA))
-   SET_PSTATE_REPLY_OPTIONAL(param);
+
+  

RE: device support in hpasa, was: Re: OOPS from cciss_ioctl in 4.12+git

[Don Brace]

> -Original Message-
> From: Hannes Reinecke [mailto:h...@suse.de]
> Sent: Friday, July 07, 2017 11:05 AM
> To: Jens Axboe ; Christoph Hellwig
> ; Meelis Roos 
> Cc: Linux Kernel list ; linux-
> bl...@vger.kernel.org; Don Brace ; Scott
> Benesh ; Scott Teel
> ; Kevin Barnett
> ; linux-scsi@vger.kernel.org
> Subject: Re: device support in hpasa, was: Re: OOPS from cciss_ioctl in
> 4.12+git
> 
> EXTERNAL EMAIL
> 
> 
> On 07/07/2017 05:03 PM, Jens Axboe wrote:
> > On 07/07/2017 09:00 AM, Christoph Hellwig wrote:
> >> On Thu, Jul 06, 2017 at 12:55:04PM +0300, Meelis Roos wrote:
>  Also we're trying to move people away from the cciss driver, can you
>  check if the hpsa SCSI driver works for you as well?
> >>>
> >>> I have older adapter:
> >>>
> >>> Compaq Computer Corporation Smart Array 64xx [0e11:0046] (rev 01)
> >>>
> >>> That does not seem to be supported by hpsa AFAICS.
> >>
> >> Looks like.  Although hpsa has support for various SA5 controllers
> >> it seems like it decided to skip all Compaq branded controllers.
> >>
> >> As far as I can tell we could simply add support for those to
> >> hpsa.  Ccing hpsa folks to figure out if that's the case.
> >
> > Pretty sure Hannes had a patch he tested for that, he talked about
> > that back at LSFMM earlier this year. Hannes?
> >
> Oh, I do.
> hpsa is working happily on SLES for _all_ SmartArray controllers.
> You need to enable 'hpsa_allow_any=1', though.
> But I'm perfectly happy with making that the default and drop cciss
> completely.
> 
> 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)

The 6400 controllers are quite old and are no longer tested. 
So, it would be nice to deprecate the cciss driver, but we would not support 
those formerly cciss devices with hpsa.
I do not recall seeing a cciss_ioctl issue, what was the issue?

Thanks,
Don Brace
ESC - Smart Storage
Microsemi Corporation

Re: device support in hpsa, was: Re: OOPS from cciss_ioctl in 4.12+git

[Laurence Oberman]

On 07/07/2017 02:08 PM, Christoph Hellwig wrote:

On Fri, Jul 07, 2017 at 11:42:38AM -0400, Laurence Oberman wrote:

What happens when  hpsa_allow_any=1 with the Smart Array 64xx
It should probe.


But only if it has a HP vendor ID as far as I can tell.  We'd
still need to add the compaq ids so that these controllers get
probed.  But maybe it's time to add them and flip the hpsa_allow_any
default (maybe conditionally on a config option?) and mark cciss
deprecated.


Agreed, I vote yes.

Re: device support in hpsa, was: Re: OOPS from cciss_ioctl in 4.12+git

[Christoph Hellwig]

On Fri, Jul 07, 2017 at 11:42:38AM -0400, Laurence Oberman wrote:
> What happens when  hpsa_allow_any=1 with the Smart Array 64xx
> It should probe.

But only if it has a HP vendor ID as far as I can tell.  We'd
still need to add the compaq ids so that these controllers get
probed.  But maybe it's time to add them and flip the hpsa_allow_any
default (maybe conditionally on a config option?) and mark cciss
deprecated.

Re: [PATCH v2] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[Chris Clayton]

On 07/07/17 09:56, Johannes Thumshirn wrote:
> SG_DXFER_FROM_DEV transfers do not necessarily have a dxferp as we set
> it to NULL for the old sg_io read/write interface, but must have a length
> bigger than 0. This fixes a regression introduced by commit 28676d869bbb
> ("scsi: sg: check for valid direction before starting the request")
> 

I've tested this new patch and the Nero applications can still find the optical 
drives on my laptop.

Tested-by: Chris Clayton 

> Signed-off-by: Johannes Thumshirn 
> Fixes: 28676d869bbb ("scsi: sg: check for valid direction before starting the 
> request")
> Reported-by: Chris Clayton 
> Tested-by: Chris Clayton 
> Cc: Douglas Gilbert 
> Reviewed-by: Hannes Reinecke 
> ---
> Changes to v1:
> * Fix breakage of the sg_io v3 interface, verified using sg_inq
> 
>  drivers/scsi/sg.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 21225d62b0c1..1e82d4128a84 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -758,8 +758,11 @@ static bool sg_is_valid_dxfer(sg_io_hdr_t *hp)
>   if (hp->dxferp || hp->dxfer_len > 0)
>   return false;
>   return true;
> - case SG_DXFER_TO_DEV:
>   case SG_DXFER_FROM_DEV:
> + if (hp->dxfer_len < 0)
> + return false;
> + return true;
> + case SG_DXFER_TO_DEV:
>   case SG_DXFER_TO_FROM_DEV:
>   if (!hp->dxferp || hp->dxfer_len == 0)
>   return false;
> 

Re: [PATCH 4/5] target: user: Fix sense data handling

[Mike Christie]

On 07/06/2017 11:50 PM, Nicholas A. Bellinger wrote:
> Hey MNC & Co,
> 
> On Wed, 2017-06-28 at 12:44 -0500, Mike Christie wrote:
>> On 06/28/2017 12:58 AM, Damien Le Moal wrote:
>>> If the user request handler completed the request with a CHECK CONDITION
>>> status, tcmu_handle_completion() copies the command entry sense data
>>> into the session request structure sense data. However, the sense data
>>> length indicated by the field scsi_sense_length is not set and equal to
>>> 0, resulting in the copy being a no-op and failure to propagate the
>>> sense data back to the initiator. To fix this, properly set the session
>>> command sense data length and also set the session command
>>> SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
>>> sense data.
>>>
>>> Signed-off-by: Damien Le Moal 
>>> ---
>>>  drivers/target/target_core_user.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/target/target_core_user.c 
>>> b/drivers/target/target_core_user.c
>>> index beb5f09..7426b4c 100644
>>> --- a/drivers/target/target_core_user.c
>>> +++ b/drivers/target/target_core_user.c
>>> @@ -831,7 +831,9 @@ static void tcmu_handle_completion(struct tcmu_cmd 
>>> *cmd, struct tcmu_cmd_entry *
>>> entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
>>> } else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
>>> memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
>>> -  se_cmd->scsi_sense_length);
>>> +  TRANSPORT_SENSE_BUFFER);
>>> +   se_cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
>>> +   se_cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
>>> } else if (se_cmd->se_cmd_flags & SCF_BIDI) {
>>> /* Get Data-In buffer before clean up */
>>> gather_data_area(udev, cmd, true);
>>>
>>
>> I have a patch similar to this and 5/5 in my set:
>>
>> https://www.spinics.net/lists/target-devel/msg15430.html
>>
>> If yours gets merged first then I will build my set over them, so patch
>> looks ok to me.
>>
>> Reviewed-by: Mike Christie 
> 
> The RFC patches above from May 31st weren't merged because I thought you
> where going to send out a second series..

I am/was. Sorry for the confusion. Above, I meant if Daemien's patches
gets merged before I can repost. I have been having troubles testing the
usb patch and was not sure how long it would take me.

I should have just broken out 1 - 6 like you just did for me below.

> 
> https://www.spinics.net/lists/target-devel/msg15505.html
> 
> Since that hasn't been the case, I'll go ahead and merge the bugfixes in
> patches #1-6 for v4.13 now.  :)

Thanks.

> 
> Please resend patches #7-13 as post v4.13 items at your earliest
> convenience.
> 
> Apologies for the confusion.
> 

It was my fault :)

Re: device support in hpasa, was: Re: OOPS from cciss_ioctl in 4.12+git

[Hannes Reinecke]

On 07/07/2017 05:03 PM, Jens Axboe wrote:
> On 07/07/2017 09:00 AM, Christoph Hellwig wrote:
>> On Thu, Jul 06, 2017 at 12:55:04PM +0300, Meelis Roos wrote:
 Also we're trying to move people away from the cciss driver, can you
 check if the hpsa SCSI driver works for you as well?
>>>
>>> I have older adapter:
>>>
>>> Compaq Computer Corporation Smart Array 64xx [0e11:0046] (rev 01)
>>>
>>> That does not seem to be supported by hpsa AFAICS.
>>
>> Looks like.  Although hpsa has support for various SA5 controllers
>> it seems like it decided to skip all Compaq branded controllers.
>>
>> As far as I can tell we could simply add support for those to
>> hpsa.  Ccing hpsa folks to figure out if that's the case.
> 
> Pretty sure Hannes had a patch he tested for that, he talked about
> that back at LSFMM earlier this year. Hannes?
> 
Oh, I do.
hpsa is working happily on SLES for _all_ SmartArray controllers.
You need to enable 'hpsa_allow_any=1', though.
But I'm perfectly happy with making that the default and drop cciss
completely.

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)

Re: device support in hpasa, was: Re: OOPS from cciss_ioctl in 4.12+git

[Laurence Oberman]

On 07/07/2017 11:03 AM, Jens Axboe wrote:

On 07/07/2017 09:00 AM, Christoph Hellwig wrote:

On Thu, Jul 06, 2017 at 12:55:04PM +0300, Meelis Roos wrote:

Also we're trying to move people away from the cciss driver, can you
check if the hpsa SCSI driver works for you as well?


I have older adapter:

Compaq Computer Corporation Smart Array 64xx [0e11:0046] (rev 01)

That does not seem to be supported by hpsa AFAICS.


Looks like.  Although hpsa has support for various SA5 controllers
it seems like it decided to skip all Compaq branded controllers.

As far as I can tell we could simply add support for those to
hpsa.  Ccing hpsa folks to figure out if that's the case.


Pretty sure Hannes had a patch he tested for that, he talked about
that back at LSFMM earlier this year. Hannes?



What happens when  hpsa_allow_any=1 with the Smart Array 64xx
It should probe.

Re: device support in hpasa, was: Re: OOPS from cciss_ioctl in 4.12+git

[Jens Axboe]

On 07/07/2017 09:00 AM, Christoph Hellwig wrote:
> On Thu, Jul 06, 2017 at 12:55:04PM +0300, Meelis Roos wrote:
>>> Also we're trying to move people away from the cciss driver, can you
>>> check if the hpsa SCSI driver works for you as well?
>>
>> I have older adapter:
>>
>> Compaq Computer Corporation Smart Array 64xx [0e11:0046] (rev 01)
>>
>> That does not seem to be supported by hpsa AFAICS.
> 
> Looks like.  Although hpsa has support for various SA5 controllers
> it seems like it decided to skip all Compaq branded controllers.
> 
> As far as I can tell we could simply add support for those to
> hpsa.  Ccing hpsa folks to figure out if that's the case.

Pretty sure Hannes had a patch he tested for that, he talked about
that back at LSFMM earlier this year. Hannes?

-- 
Jens Axboe

device support in hpasa, was: Re: OOPS from cciss_ioctl in 4.12+git

[Christoph Hellwig]

On Thu, Jul 06, 2017 at 12:55:04PM +0300, Meelis Roos wrote:
> > Also we're trying to move people away from the cciss driver, can you
> > check if the hpsa SCSI driver works for you as well?
> 
> I have older adapter:
> 
> Compaq Computer Corporation Smart Array 64xx [0e11:0046] (rev 01)
> 
> That does not seem to be supported by hpsa AFAICS.

Looks like.  Although hpsa has support for various SA5 controllers
it seems like it decided to skip all Compaq branded controllers.

As far as I can tell we could simply add support for those to
hpsa.  Ccing hpsa folks to figure out if that's the case.

Re: [PATCH 1/5] scsi: bnx2i: convert to workqueue

[Sebastian Andrzej Siewior]

On 2017-07-07 09:20:02 [-0400], Chad Dupuis wrote:
> What was the question?  My observation is that the patch I proposed fixed 
> the issue we saw on testing the patch set.  With that small change 
> (essentially modulo by the number of active CPUs vs. the total number) 
> your patch set worked ok.

That mail at the bottom of this mail where I said why I think your patch
is a nop in this context.

Sebastian

On 2017-05-17 17:07:34 [+0200], To Chad Dupuis wrote:
> > > Sebastian, can you add this change to your patch set?
> >
> > Are sure that you can reliably reproduce the issue and fix it with the
> > patch above? Because this patch:
>
> oh. Okay. Now it clicked. It can fix the issue but it is still possible,
> that CPU0 goes down between your check for it and schedule_work_on()
> returning. Let my think of something…

Oh wait. I already thought about this: it may take bnx2fc_percpu from
CPU7 and run the worker on CPU3. The job isn't lost, because the worker
does:

| static void bnx2fc_percpu_io_work(struct work_struct *work_s)
| {
| struct bnx2fc_percpu_s *p;
 …
| p = container_of(work_s, struct bnx2fc_percpu_s, work);
|
| spin_lock_bh(>fp_work_lock);

and so will access bnx2fc_percpu of CPU7 running on CPU3. So I *think*
that your patch should make no difference and there should be no leak if
schedule_work_on() is invoked on an offline CPU.

Re: [PATCH 1/5] scsi: bnx2i: convert to workqueue

[Chad Dupuis]

On Fri, 7 Jul 2017, 9:14am, Sebastian Andrzej Siewior wrote:

> On 2017-06-29 15:57:56 [+0200], Johannes Thumshirn wrote:
> > So here we are again,
> > Tested-by: Johannes Thumshirn 
> > 
> > FCoE will follow as soon as my setup can speak FCoE again.
> 
> So it all looks good, doesn't it? Chad never responded to my question
> on his patch. I still doubt that it fixes the problem he observed.
> 
> Sebastian
> 

What was the question?  My observation is that the patch I proposed fixed 
the issue we saw on testing the patch set.  With that small change 
(essentially modulo by the number of active CPUs vs. the total number) 
your patch set worked ok.

Re: [PATCH 1/5] scsi: bnx2i: convert to workqueue

[Sebastian Andrzej Siewior]

On 2017-06-29 15:57:56 [+0200], Johannes Thumshirn wrote:
> So here we are again,
> Tested-by: Johannes Thumshirn 
> 
> FCoE will follow as soon as my setup can speak FCoE again.

So it all looks good, doesn't it? Chad never responded to my question
on his patch. I still doubt that it fixes the problem he observed.

Sebastian

Re: [patch] scsi: qedi: silence sprintf() overflow warning

[Dan Carpenter]

On Tue, Feb 07, 2017 at 02:27:09PM +0100, walter harms wrote:
> 
> 
> Am 07.02.2017 14:01, schrieb Dan Carpenter:
> > The problem here is this:
> > 
> > sprintf(host_buf, "qedi_ofld%d", qedi->shost->host_no);
> > 
> > host_buf is 16 character so we only have 6 characters left for
> > ->host_no.  But ->host_no is set in scsi_host_alloc():
> > 
> > index = ida_simple_get(_index_ida, 0, 0, GFP_KERNEL);
> > 
> > It could theoretically go up to 0x800 so we need space for 10
> > digits.
> > 
> > Signed-off-by: Dan Carpenter 
> > 
> > diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> > index 5eda21d903e9..0dcf3b08230c 100644
> > --- a/drivers/scsi/qedi/qedi_main.c
> > +++ b/drivers/scsi/qedi/qedi_main.c
> > @@ -1735,7 +1735,7 @@ static int __qedi_probe(struct pci_dev *pdev, int 
> > mode)
> > u32 dp_module = 0;
> > u8 dp_level = 0;
> > bool is_vf = false;
> > -   char host_buf[16];
> > +   char host_buf[20];
> > struct qed_link_params link_params;
> > struct qed_slowpath_params sp_params;
> > struct qed_probe_params qed_params;
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" 
> > in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> any chance to use snprintf here ?
>  sprintf(host_buf, "qedi_ofld%d", qedi->shost->host_no);
> 
> or something like asprint() :)
> 
> if ever anyone change the type to very_long_type in the future it would 
> simply break
> but not hurt.

No, I don't think that's required.  There are infinite possible futures
and the future you're describing is not likely.  We'd just end up making
the code more complicated for no reason.

regards,
dan carpenter

[PATCH v2] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[Johannes Thumshirn]

SG_DXFER_FROM_DEV transfers do not necessarily have a dxferp as we set
it to NULL for the old sg_io read/write interface, but must have a length
bigger than 0. This fixes a regression introduced by commit 28676d869bbb
("scsi: sg: check for valid direction before starting the request")

Signed-off-by: Johannes Thumshirn 
Fixes: 28676d869bbb ("scsi: sg: check for valid direction before starting the 
request")
Reported-by: Chris Clayton 
Tested-by: Chris Clayton 
Cc: Douglas Gilbert 
Reviewed-by: Hannes Reinecke 
---
Changes to v1:
* Fix breakage of the sg_io v3 interface, verified using sg_inq

 drivers/scsi/sg.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 21225d62b0c1..1e82d4128a84 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -758,8 +758,11 @@ static bool sg_is_valid_dxfer(sg_io_hdr_t *hp)
if (hp->dxferp || hp->dxfer_len > 0)
return false;
return true;
-   case SG_DXFER_TO_DEV:
case SG_DXFER_FROM_DEV:
+   if (hp->dxfer_len < 0)
+   return false;
+   return true;
+   case SG_DXFER_TO_DEV:
case SG_DXFER_TO_FROM_DEV:
if (!hp->dxferp || hp->dxfer_len == 0)
return false;
-- 
2.12.3

Re: [PATCH] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[Johannes Thumshirn]

On Thu, Jul 06, 2017 at 02:47:22PM -0400, Douglas Gilbert wrote:
> Can you check your patch with one of the utilities from sg3_utils
> such as sg_inq which will use SG_DXFER_FROM_DEV with the newer
> interface?

Correct, this patch broke sg_inq. I'll send a corrected v2.

> BTW I'm not sure why dxferp is set to NULL for SG_DXFER_FROM_DEV
> transfers; perhaps some magic done by the block layer. Maybe a
> comment in the code (e.g. on line 654) would help.

This is due to:

commit fad7f01e61bf737fe8a3740d803f000db57ecac6
Author: FUJITA Tomonori 
Date:   Tue Sep 2 16:20:20 2008 +0900

sg: set dxferp to NULL for READ with the older SG interface

With the older SG interface, we don't know a user-space address to
trasfer data when executing a SCSI command. So we can't pass a
user-space address to blk_rq_map_user.

This patch fixes sg to pass a NULL user-space address to
blk_rq_map_user so that it just sets up a request and bios with page
frames propely without data transfer.

Signed-off-by: FUJITA Tomonori 
Signed-off-by: Jens Axboe 

> 
> Also sg_is_valid_dxfer() is only called once and is more complex
> than it looks; so perhaps it could be inlined back in
> sg_common_write().

The compiler will inline it anyways (at least the one I checked with) and
inlining it into sg_common_write() won't make the code more readable IMHO. But
ultimately it's your driver so if you insist I'll do.

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 4/5] target: user: Fix sense data handling

[Damien Le Moal]

On 7/7/17 15:06, Nicholas A. Bellinger wrote:
>>> My apologies. I have been busy with other things and could not get to that.
>>>
> 
> Oh btw, I was talking to MNC wrt to his original patch series, and not
> yours.  :)

Yes, I realized that after sending. Tired Friday afternoon here in
Japan. My brain is slowing down for the weekend :)

Cheers.

-- 
Damien Le Moal,
Western Digital

Re: [PATCH 4/5] target: user: Fix sense data handling

[Nicholas A. Bellinger]

On Thu, 2017-07-06 at 23:05 -0700, Nicholas A. Bellinger wrote:
> On Fri, 2017-07-07 at 14:14 +0900, Damien Le Moal wrote:
> > Nicholas,
> > 
> > On 7/7/17 13:50, Nicholas A. Bellinger wrote:
> > > Hey MNC & Co,
> > > 
> > > On Wed, 2017-06-28 at 12:44 -0500, Mike Christie wrote:
> > >> On 06/28/2017 12:58 AM, Damien Le Moal wrote:
> > >>> If the user request handler completed the request with a CHECK CONDITION
> > >>> status, tcmu_handle_completion() copies the command entry sense data
> > >>> into the session request structure sense data. However, the sense data
> > >>> length indicated by the field scsi_sense_length is not set and equal to
> > >>> 0, resulting in the copy being a no-op and failure to propagate the
> > >>> sense data back to the initiator. To fix this, properly set the session
> > >>> command sense data length and also set the session command
> > >>> SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
> > >>> sense data.
> > >>>
> > >>> Signed-off-by: Damien Le Moal 
> > >>> ---
> > >>>  drivers/target/target_core_user.c | 4 +++-
> > >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/drivers/target/target_core_user.c 
> > >>> b/drivers/target/target_core_user.c
> > >>> index beb5f09..7426b4c 100644
> > >>> --- a/drivers/target/target_core_user.c
> > >>> +++ b/drivers/target/target_core_user.c
> > >>> @@ -831,7 +831,9 @@ static void tcmu_handle_completion(struct tcmu_cmd 
> > >>> *cmd, struct tcmu_cmd_entry *
> > >>> entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
> > >>> } else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
> > >>> memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
> > >>> -  se_cmd->scsi_sense_length);
> > >>> +  TRANSPORT_SENSE_BUFFER);
> > >>> +   se_cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
> > >>> +   se_cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
> > >>> } else if (se_cmd->se_cmd_flags & SCF_BIDI) {
> > >>> /* Get Data-In buffer before clean up */
> > >>> gather_data_area(udev, cmd, true);
> > >>>
> > >>
> > >> I have a patch similar to this and 5/5 in my set:
> > >>
> > >> https://www.spinics.net/lists/target-devel/msg15430.html
> > >>
> > >> If yours gets merged first then I will build my set over them, so patch
> > >> looks ok to me.
> > >>
> > >> Reviewed-by: Mike Christie 
> > > 
> > > The RFC patches above from May 31st weren't merged because I thought you
> > > where going to send out a second series..
> > 
> > My apologies. I have been busy with other things and could not get to that.
> > 

Oh btw, I was talking to MNC wrt to his original patch series, and not
yours.  :)

Re: [PATCH 4/5] target: user: Fix sense data handling

[Nicholas A. Bellinger]

On Fri, 2017-07-07 at 14:14 +0900, Damien Le Moal wrote:
> Nicholas,
> 
> On 7/7/17 13:50, Nicholas A. Bellinger wrote:
> > Hey MNC & Co,
> > 
> > On Wed, 2017-06-28 at 12:44 -0500, Mike Christie wrote:
> >> On 06/28/2017 12:58 AM, Damien Le Moal wrote:
> >>> If the user request handler completed the request with a CHECK CONDITION
> >>> status, tcmu_handle_completion() copies the command entry sense data
> >>> into the session request structure sense data. However, the sense data
> >>> length indicated by the field scsi_sense_length is not set and equal to
> >>> 0, resulting in the copy being a no-op and failure to propagate the
> >>> sense data back to the initiator. To fix this, properly set the session
> >>> command sense data length and also set the session command
> >>> SCF_TRANSPORT_TASK_SENSE flag to indicate that the command has valid
> >>> sense data.
> >>>
> >>> Signed-off-by: Damien Le Moal 
> >>> ---
> >>>  drivers/target/target_core_user.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/target/target_core_user.c 
> >>> b/drivers/target/target_core_user.c
> >>> index beb5f09..7426b4c 100644
> >>> --- a/drivers/target/target_core_user.c
> >>> +++ b/drivers/target/target_core_user.c
> >>> @@ -831,7 +831,9 @@ static void tcmu_handle_completion(struct tcmu_cmd 
> >>> *cmd, struct tcmu_cmd_entry *
> >>>   entry->rsp.scsi_status = SAM_STAT_CHECK_CONDITION;
> >>>   } else if (entry->rsp.scsi_status == SAM_STAT_CHECK_CONDITION) {
> >>>   memcpy(se_cmd->sense_buffer, entry->rsp.sense_buffer,
> >>> -se_cmd->scsi_sense_length);
> >>> +TRANSPORT_SENSE_BUFFER);
> >>> + se_cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
> >>> + se_cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
> >>>   } else if (se_cmd->se_cmd_flags & SCF_BIDI) {
> >>>   /* Get Data-In buffer before clean up */
> >>>   gather_data_area(udev, cmd, true);
> >>>
> >>
> >> I have a patch similar to this and 5/5 in my set:
> >>
> >> https://www.spinics.net/lists/target-devel/msg15430.html
> >>
> >> If yours gets merged first then I will build my set over them, so patch
> >> looks ok to me.
> >>
> >> Reviewed-by: Mike Christie 
> > 
> > The RFC patches above from May 31st weren't merged because I thought you
> > where going to send out a second series..
> 
> My apologies. I have been busy with other things and could not get to that.
> 
> > 
> > https://www.spinics.net/lists/target-devel/msg15505.html
> > 
> > Since that hasn't been the case, I'll go ahead and merge the bugfixes in
> > patches #1-6 for v4.13 now.  :)
> > 
> > Please resend patches #7-13 as post v4.13 items at your earliest
> > convenience.
> 
> I will retest first thing Monday the merge with Mikes series and send
> incremental fixes if any is needed.
> 

Great, thanks.

Everything including MNC's #1-6 and your #1-2 be pushed to
target-pending/for-next shortly.

Please use this as your base for testing.  :)