Re: [PATCH 9/9] scsi: ufs: Add clock ungating to a separate workqueue

[Miguel Ojeda]

On Wed, Feb 21, 2018 at 5:56 AM, Asutosh Das  wrote:
> From: Vijay Viswanath 
>
> UFS driver can receive a request during memory reclaim by kswapd.
> So when ufs driver puts the ungate work in queue, and if there are no
> idle workers, kthreadd is invoked to create a new kworker. Since
> kswapd task holds a mutex which kthreadd also needs, this can cause
> a deadlock situation. So ungate work must be done in a separate
> work queue with WQ__RECLAIM flag enabled.Such a workqueue will have

WQ_MEM_RECLAIM here. Also space after dot.

> a rescue thread which will be called when the above deadlock
> condition is possible.
>
> Signed-off-by: Vijay Viswanath 
> Signed-off-by: Can Guo 
> Signed-off-by: Asutosh Das 
> ---
>  drivers/scsi/ufs/ufshcd.c | 10 +-
>  drivers/scsi/ufs/ufshcd.h |  1 +
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 6541e1d..bb3382a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1503,7 +1503,8 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
> hba->clk_gating.state = REQ_CLKS_ON;
> trace_ufshcd_clk_gating(dev_name(hba->dev),
> hba->clk_gating.state);
> -   schedule_work(>clk_gating.ungate_work);
> +   queue_work(hba->clk_gating.clk_gating_workq,
> +  >clk_gating.ungate_work);
> /*
>  * fall through to check if we should wait for this
>  * work to be done or not.
> @@ -1689,6 +1690,8 @@ static ssize_t ufshcd_clkgate_enable_store(struct 
> device *dev,
>
>  static void ufshcd_init_clk_gating(struct ufs_hba *hba)
>  {
> +   char wq_name[sizeof("ufs_clk_gating_00")];
> +
> if (!ufshcd_is_clkgating_allowed(hba))
> return;
>
> @@ -1696,6 +1699,10 @@ static void ufshcd_init_clk_gating(struct ufs_hba *hba)
> INIT_DELAYED_WORK(>clk_gating.gate_work, ufshcd_gate_work);
> INIT_WORK(>clk_gating.ungate_work, ufshcd_ungate_work);
>
> +   snprintf(wq_name, ARRAY_SIZE(wq_name), "ufs_clk_gating_%d",
> +hba->host->host_no);
> +   hba->clk_gating.clk_gating_workq = 
> create_singlethread_workqueue(wq_name);

Shouldn't this be alloc_ordered_workqueue() with WQ_MEM_RECLAIM then?
(create_singlethread_workqueue() and friends are deprecated).

Cheers,
Miguel

> +
> hba->clk_gating.is_enabled = true;
>
> hba->clk_gating.delay_attr.show = ufshcd_clkgate_delay_show;
> @@ -1723,6 +1730,7 @@ static void ufshcd_exit_clk_gating(struct ufs_hba *hba)
> device_remove_file(hba->dev, >clk_gating.enable_attr);
> cancel_work_sync(>clk_gating.ungate_work);
> cancel_delayed_work_sync(>clk_gating.gate_work);
> +   destroy_workqueue(hba->clk_gating.clk_gating_workq);
>  }
>
>  /* Must be called with host lock acquired */
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 4385741..570c33e 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -361,6 +361,7 @@ struct ufs_clk_gating {
> struct device_attribute enable_attr;
> bool is_enabled;
> int active_reqs;
> +   struct workqueue_struct *clk_gating_workq;
>  };
>
>  struct ufs_saved_pwr_info {
> --
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
> Foundation Collaborative Project.
>

Re: [PATCH] Make SCSI Status CONDITION MET equivalent to GOOD

[Bart Van Assche]

On Wed, 2018-02-21 at 20:48 -0500, Douglas Gilbert wrote:
> + if (result) {
> + if (sense_valid && (sshdr.sense_key == RECOVERED_ERROR)) {
> + /* if ATA PASS-THROUGH INFORMATION AVAILABLE skip
> +  * print since caller wants ATA registers. Only occurs
> +  * on SCSI ATA PASS_THROUGH commands when CK_COND=1
> +  */
> + if ((sshdr.asc == 0x0) && (sshdr.ascq == 0x1d))
> + ;
> + else if (!(req->rq_flags & RQF_QUIET))
> + scsi_print_sense(cmd);
> + result = 0;
> + /* for passthrough error may be set */
> + error = BLK_STS_OK;
> + /*
> +  * Another corner case: the SCSI status byte is non-zero but 'good'.
> +  * Example: PRE-FETCH command returns SAM_STAT_CONDITION_MET when
> +  * it is able to fit nominated LBs in its cache (and SAM_STAT_GOOD
> +  * if it can't fit). Treat SAM_STAT_CONDITION_MET and the related
> +  * intermediate statuses (both obsolete in SAM-4) as good.
> +  */
> + } else if (status_byte(result) &&
> +scsi_status_is_good(result)) {
> + result = 0;
> + /* for passthrough error may be set */
> + error = BLK_STS_OK;
> + }
>   }

Please move the new comment block under "} else if" and indent it further to
the right such that neither gcc with W=1 nor smatch complain about incorrect
indentation.

Please also change the branching logic as follows to keep the indentation level
low:

if (sense_valid && (sshdr.sense_key == RECOVERED_ERROR)) {
[ ... ]
} else if (status_byte(result) && scsi_status_is_good(result)) {
[ ... ]
}

Thanks,

Bart.

Re: [PATCH v2] scsi: handle special return codes for ABORTED COMMAND

[Douglas Gilbert]

On 2018-02-23 08:33 AM, Martin Wilck wrote:

Gentle reminder - reviews welcome ...

On Tue, 2018-01-30 at 11:25 +0100, Martin Wilck wrote:

Introduce a new blist flag that indicates the device may return
certain
sense code/ASC/ASCQ combinations that indicate different treatment
than
normal. In particular, some devices need unconditional retry (aka
ADD_TO_MLQUEUE) under certain conditions; otherwise path failures may
be
falsely detected in the "maybe_retry" case.

Because different devices use different sense codes to indicate this
condition, a single bit is not enough. But we also can't use a bit
for
every possible status code. Therefore the new flag
BLIST_ABORTED_CMD_QUIRK
just indicates that this is a device for which the return code should
be
checked. An extra "blacklist" in scsi_aborted_cmd_quirk() then checks
for
known ASC/ASCQ combinations, and handles them.

When such a combination is encountered for the first time, a message
is
printed. In systems that have several different peripherals using
this
flag, that might lead to a wrong match without a warning. This small
risk
is a compromise between exactness and the excessive resources that
would be
required to check for matching device vendor and model name every
time.
Also, if there were different devices (identified by vendor/model)
using
the same ASC/ASCQ, the code might print stray warnings. But the
additional
effort to required to handle this 100% correctly is hardly justified
with
the current minimal "blacklist".

I chose to recycle devinfo flag (1<<11) (former BLIST_INQUIRY_58,
free
since 496c91bbc910) for this purpose rather than extending
blist_flags_t to
64 bit. This could be easily changed of course.

This patch replaces the previously submitted patches
  "scsi: handle ABORTED_COMMAND on Fujitsu ETERNUS" and
  "scsi: Always retry internal target error" (Hannes Reinecke)

Changes in v2:
  - use ARRAY_SIZE (Bart van Assche)
  - make blist array static const and use separate bitmask for warned
flag
(Bart van Assche)
  - Fix string comparison for SCSI vendor and model
  - Print warning at scsi_logging_level 0, and improve message
formatting

Signed-off-by: Martin Wilck 


I checked the code for the ABORTED COMMAND asc=0x10 case. That is a bunch
of Protection Information errors which retrying would be pointless. And
the existing code does check for that just before the new check for
BLIST_ABORTED_CMD_QUIRK. The PI check has the cryptic comment /* DIF */
and we also have 'T10 DIF' appearing in defines. 'DIF' is mentioned _once_
in the T10 drafts thatI can find (in sbc4r15.pdf the last sentence of
section 4.22.1) and is not cross-referenced anywhere that I can find.

So anyway, Martin (P.), could we have and explanation somewhere prominent in
our code what DIF and DIX mean in SCSI (and other standards) terms?

But that is not this patch's problem, so:

Reviewed-by: Douglas Gilbert 

Re: new OOPS in 4.15.4 in rcu_call within softirq

[Alexandre Oliva]

On Feb 22, 2018, Bart Van Assche  wrote:

> Alexandre, can you try patch "[PATCH v2] Avoid that ATA error handling can
> trigger a kernel hang or oops"
> (https://www.mail-archive.com/linux-scsi@vger.kernel.org/msg71189.html)?

Thanks.  I confirm that applying it on top of 4.15.4 seems to make the
problem go away on all affected machines.  I write 'seems' just because
I can't quite prove the oops won't happen any more, but I'm pretty sure
if it were to, it would have already ;-)  Thanks, again!

-- 
Alexandre Oliva, freedom fighterhttp://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: [PATCH v2] scsi: handle special return codes for ABORTED COMMAND

[Bart Van Assche]

On Tue, 2018-01-30 at 11:25 +0100, Martin Wilck wrote:
> Introduce a new blist flag that indicates the device may return certain
> sense code/ASC/ASCQ combinations that indicate different treatment than
> normal. In particular, some devices need unconditional retry (aka
> ADD_TO_MLQUEUE) under certain conditions; otherwise path failures may be
> falsely detected in the "maybe_retry" case.

Reviewed-by: Bart Van Assche 

Re: [PATCH] scsi: lpfc: use memcpy_toio instead of writeq

[Arnd Bergmann]

On Fri, Feb 23, 2018 at 4:36 PM, Arnd Bergmann  wrote:
> @@ -138,12 +137,10 @@ lpfc_sli4_wq_put(struct lpfc_queue *q, union lpfc_wqe 
> *wqe)
> if (q->phba->sli3_options & LPFC_SLI4_PHWQ_ENABLED)
> bf_set(wqe_wqid, >generic.wqe_com, q->queue_id);
> lpfc_sli_pcimem_bcopy(wqe, temp_wqe, q->entry_size);
> -   if (q->dpp_enable && q->phba->cfg_enable_dpp) {
> +   if (q->dpp_enable && q->phba->cfg_enable_dpp)
> /* write to DPP aperture taking advatage of Combined Writes */
> -   tmp = (uint8_t *)wqe;
> -   for (i = 0; i < q->entry_size; i += sizeof(uint64_t))
> -   writeq(*((uint64_t *)(tmp + i)), q->dpp_regaddr + i);
> -   }
> +   memcpy_toio(tmp, q->dpp_regaddr, q->entry_size);
> +
> /* ensure WQE bcopy and DPP flushed before doorbell write */
> wmb();
>

Not sure where we are with the question of whether memcpy_toio
is a good replacement or not, but further build testing showed that
my patch was completely broken in more than one way:

I mixed up the source and destination arguments, and I used
the uninitialized 'tmp' instead of 'wqe'. Don't try this patch.

   Arnd

[PATCH] mvsas: fix wrong endianness of sgpio api

[Wilfried . Weissmann]

From: Wilfried Weissmann 

This patch fixes the byte order of the SGPIO api and brings it back
in sync with ledmon v0.80 and above.
---
 drivers/scsi/mvsas/mv_94xx.c | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/mvsas/mv_94xx.c b/drivers/scsi/mvsas/mv_94xx.c
index 7de5d8d75..84a638a90 100644
--- a/drivers/scsi/mvsas/mv_94xx.c
+++ b/drivers/scsi/mvsas/mv_94xx.c
@@ -1080,16 +1080,16 @@ static int mvs_94xx_gpio_write(struct mvs_prv_info 
*mvs_prv,
void __iomem *regs = mvi->regs_ex - 0x10200;
 
int drive = (i/3) & (4-1); /* drive number on host */
-   u32 block = mr32(MVS_SGPIO_DCTRL +
+   int driveshift = drive * 8; /* bit offset of drive */
+   u32 block = ioread32be(regs + MVS_SGPIO_DCTRL +
MVS_SGPIO_HOST_OFFSET * mvi->id);
 
-
/*
* if bit is set then create a mask with the first
* bit of the drive set in the mask ...
*/
-   u32 bit = (write_data[i/8] & (1 << (i&(8-1 ?
-   1<<(24-drive*8) : 0;
+   u32 bit = get_unaligned_be32(write_data) & (1<id);
 
}
 
@@ -1132,7 +1133,7 @@ static int mvs_94xx_gpio_write(struct mvs_prv_info 
*mvs_prv,
void __iomem *regs = mvi->regs_ex - 0x10200;
 
mw32(MVS_SGPIO_DCTRL + MVS_SGPIO_HOST_OFFSET * mvi->id,
-   be32_to_cpu(((u32 *) write_data)[i]));
+   ((u32 *) write_data)[i]);
}
return reg_count;
}

Re: [PATCH 00/38] cxlflash: OpenCXL transport support

[Uma Krishnan]

> On Feb 22, 2018, at 10:13 PM, Andrew Donnellan  
> wrote:
> 
> On 23/02/18 09:20, Uma Krishnan wrote:
>> This patch series adds OpenCXL support to the cxlflash driver. With
>> this support, new devices using the OpenCXL transport will be supported
>> by the cxlflash driver along with the existing CXL devices. An effort is
>> made to keep this transport specific function independent of the existing
>> core driver that communicates with the AFU.
> 
> It's "OpenCAPI" for the standard, and "ocxl" for the driver - I'd rather not 
> add "OpenCXL" to our already somewhat confusing proliferation of names :)

I agree. :) I will fix it in V2. Thanks for the feedback.

Re: [PATCH 04/38] cxlflash: Introduce OpenCXL backend

[Uma Krishnan]

> On Feb 22, 2018, at 10:28 PM, Andrew Donnellan  
> wrote:
> 
> On 23/02/18 09:22, Uma Krishnan wrote:
>> Add initial infrastructure to support a new cxlflash transport, OpenCXL.
>> Claim a dependency on OpenCXL (OCXL) and add a new file, ocxl_hw.c, which
>> will host the backend routines that are specific to OpenCXL.
>> Signed-off-by: Uma Krishnan 
> 
> Is it necessary to depend on OCXL in all cases? IMHO it should be possible to 
> build a cxl-only version without ocxl, though I don't feel very strongly on 
> this.

I thought through this and did not feel it is necessary to distinguish right 
now. I will look at adding this in the future. Thanks Andrew !

RE: [PATCH] scsi: lpfc: use memcpy_toio instead of writeq

[David Laight]

From: Andy Shevchenko
> Sent: 23 February 2018 17:13
> To: David Laight
> Cc: Arnd Bergmann; James Smart; Dick Kennedy; James E.J. Bottomley; Martin K. 
> Petersen; Hannes
> Reinecke; Johannes Thumshirn; linux-scsi@vger.kernel.org; 
> linux-ker...@vger.kernel.org
> Subject: Re: [PATCH] scsi: lpfc: use memcpy_toio instead of writeq
> 
> On Fri, Feb 23, 2018 at 7:09 PM, David Laight  wrote:
> > From: Andy Shevchenko
> >> Sent: 23 February 2018 16:51
> >> On Fri, Feb 23, 2018 at 6:41 PM, David Laight  
> >> wrote:
> 
> 
> >> The side-effect I referred previously is about tails, i.e. unaligned
> >> bytes are transferred in portions
> >> like
> >>   7 on 64-bit will be  4 + 2 + 1,
> >>   5 = 4 + 1
> >
> > on 64bit memcpy() is allowed to do:
> > (long *)(tgt+len)[-1] = (long *)(src+len)[-1];
> > rep_movsq(tgt, src, len >> 3);
> > provided the length is at least 8.
> >
> > The misaligned PCIe transfer generates a single TLP covering 12 bytes with 
> > the
> > relevant byte enables set for the first and last 32bit words.
> 
> But is it guaranteed on any type of bus?
> memcpy_toio() is a generic helper, so, first of all we need to be sure
> what CPU on its side does, this is I think is pretty straight forward
> since it's all written in asm for 64-bit case.

I've just done a compile test, on x86-64 memcpy_toio() generates a
call to memcpy() (checked with objdump -dr).
That is on a system running a 4.14 kernel, so is probably using the system
headers from that release.
I'd need to do a run-time test on a newer system verify what the PCIe slave
sees - but I changed our driver to do its own copy loops in order to avoid
byte transfers some time ago.

FWIW I was originally doing copy_to/from_user() directly to PCIe memory 
addresses!

On x86 'memory' on devices can always be accesses by simple instructions.
Hardware 'IO' addresses are not valid for memcpy_to/fromio().

David

RE: [PATCH] scsi: lpfc: use memcpy_toio instead of writeq

[David Laight]

From: Andy Shevchenko
> Sent: 23 February 2018 16:51
> On Fri, Feb 23, 2018 at 6:41 PM, David Laight  wrote:
> > From: Arnd Bergmann
> >> Sent: 23 February 2018 15:37
> >>
> >> 32-bit architectures generally cannot use writeq(), so we now get a build
> >> failure for the lpfc driver:
> >>
> >> drivers/scsi/lpfc/lpfc_sli.c: In function 'lpfc_sli4_wq_put':
> >> drivers/scsi/lpfc/lpfc_sli.c:145:4: error: implicit declaration of 
> >> function 'writeq'; did you mean
> >> 'writeb'? [-Werror=implicit-function-declaration]
> >>
> >> Another problem here is that writing out actual data (unlike accessing
> >> mmio registers) means we must write the data with the same endianess
> >> that we have read from memory, but writeq() will perform byte swaps
> >> and add barriers inbetween accesses as we do for registers.
> >>
> >> Using memcpy_toio() should do the right thing here, using register
> >> sized stores with correct endianess conversion and barriers (i.e. none),
> >> but on some architectures might fall back to byte-size access.
> > ...
> >
> > Have you looked at the performance impact of this on x86?
> > Last time I looked memcpy_toio() aliased directly to memcpy().
> > memcpy() is run-time patched between several different algorithms.
> > On recent Intel cpus memcpy() is implemented as 'rep movsb' relying
> > on the hardware to DTRT.
> > For uncached accesses (typical for io) the 'RT' has to be byte transfers.
> > So instead of the 8 byte transfers (on 64 bit) you get single bytes.
> > This won't be what is intended!
> > memcpy_toio() should probably use 'rep movsd' for the bulk of the transfer.
> 
> Maybe I'm wrong but it uses movsq on 64-bit and movsl on 32-bit.

(Let's not argue about the instruction mnemonic). 

You might expect that, but last time I looked at the bus cycles on a PCIe slave
that wasn't what I saw.

> The side-effect I referred previously is about tails, i.e. unaligned
> bytes are transferred in portions
> like
>   7 on 64-bit will be  4 + 2 + 1,
>   5 = 4 + 1

on 64bit memcpy() is allowed to do:
(long *)(tgt+len)[-1] = (long *)(src+len)[-1];
rep_movsq(tgt, src, len >> 3);
provided the length is at least 8.

The misaligned PCIe transfer generates a single TLP covering 12 bytes with the
relevant byte enables set for the first and last 32bit words.

David

Re: [PATCH] scsi: lpfc: use memcpy_toio instead of writeq

[Andy Shevchenko]

On Fri, Feb 23, 2018 at 7:09 PM, David Laight  wrote:
> From: Andy Shevchenko
>> Sent: 23 February 2018 16:51
>> On Fri, Feb 23, 2018 at 6:41 PM, David Laight  
>> wrote:


>> The side-effect I referred previously is about tails, i.e. unaligned
>> bytes are transferred in portions
>> like
>>   7 on 64-bit will be  4 + 2 + 1,
>>   5 = 4 + 1
>
> on 64bit memcpy() is allowed to do:
> (long *)(tgt+len)[-1] = (long *)(src+len)[-1];
> rep_movsq(tgt, src, len >> 3);
> provided the length is at least 8.
>
> The misaligned PCIe transfer generates a single TLP covering 12 bytes with the
> relevant byte enables set for the first and last 32bit words.

But is it guaranteed on any type of bus?
memcpy_toio() is a generic helper, so, first of all we need to be sure
what CPU on its side does, this is I think is pretty straight forward
since it's all written in asm for 64-bit case.
So, what about buses?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] scsi: lpfc: use memcpy_toio instead of writeq

[David Laight]

From: Arnd Bergmann
> Sent: 23 February 2018 15:37
> 
> 32-bit architectures generally cannot use writeq(), so we now get a build
> failure for the lpfc driver:
> 
> drivers/scsi/lpfc/lpfc_sli.c: In function 'lpfc_sli4_wq_put':
> drivers/scsi/lpfc/lpfc_sli.c:145:4: error: implicit declaration of function 
> 'writeq'; did you mean
> 'writeb'? [-Werror=implicit-function-declaration]
> 
> Another problem here is that writing out actual data (unlike accessing
> mmio registers) means we must write the data with the same endianess
> that we have read from memory, but writeq() will perform byte swaps
> and add barriers inbetween accesses as we do for registers.
> 
> Using memcpy_toio() should do the right thing here, using register
> sized stores with correct endianess conversion and barriers (i.e. none),
> but on some architectures might fall back to byte-size access.
...

Have you looked at the performance impact of this on x86?
Last time I looked memcpy_toio() aliased directly to memcpy().
memcpy() is run-time patched between several different algorithms.
On recent Intel cpus memcpy() is implemented as 'rep movsb' relying
on the hardware to DTRT.
For uncached accesses (typical for io) the 'RT' has to be byte transfers.
So instead of the 8 byte transfers (on 64 bit) you get single bytes.
This won't be what is intended!
memcpy_toio() should probably use 'rep movsd' for the bulk of the transfer.

David

Re: [PATCH] scsi: lpfc: use memcpy_toio instead of writeq

[Andy Shevchenko]

On Fri, Feb 23, 2018 at 6:51 PM, Andy Shevchenko
 wrote:
> On Fri, Feb 23, 2018 at 6:41 PM, David Laight  wrote:
>> From: Arnd Bergmann
>>> Sent: 23 February 2018 15:37
>>>
>>> 32-bit architectures generally cannot use writeq(), so we now get a build
>>> failure for the lpfc driver:
>>>
>>> drivers/scsi/lpfc/lpfc_sli.c: In function 'lpfc_sli4_wq_put':
>>> drivers/scsi/lpfc/lpfc_sli.c:145:4: error: implicit declaration of function 
>>> 'writeq'; did you mean
>>> 'writeb'? [-Werror=implicit-function-declaration]
>>>
>>> Another problem here is that writing out actual data (unlike accessing
>>> mmio registers) means we must write the data with the same endianess
>>> that we have read from memory, but writeq() will perform byte swaps
>>> and add barriers inbetween accesses as we do for registers.
>>>
>>> Using memcpy_toio() should do the right thing here, using register
>>> sized stores with correct endianess conversion and barriers (i.e. none),
>>> but on some architectures might fall back to byte-size access.
>> ...
>>
>> Have you looked at the performance impact of this on x86?
>> Last time I looked memcpy_toio() aliased directly to memcpy().
>> memcpy() is run-time patched between several different algorithms.
>> On recent Intel cpus memcpy() is implemented as 'rep movsb' relying
>> on the hardware to DTRT.
>> For uncached accesses (typical for io) the 'RT' has to be byte transfers.
>> So instead of the 8 byte transfers (on 64 bit) you get single bytes.
>> This won't be what is intended!
>> memcpy_toio() should probably use 'rep movsd' for the bulk of the transfer.
>
> Maybe I'm wrong but it uses movsq on 64-bit and movsl on 32-bit.
>
> The side-effect I referred previously is about tails, i.e. unaligned
> bytes are transferred in portions
> like
>   7 on 64-bit will be  4 + 2 + 1,
>   5 = 4 + 1
> etc
>
> Similar way on 32-bit.

Same for leading bytes as well.

arch/x86/lib/memcpy_64.S

So, I *hope* that in the code in question there is no unaligned access is used.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] scsi: lpfc: use memcpy_toio instead of writeq

[Andy Shevchenko]

On Fri, Feb 23, 2018 at 6:41 PM, David Laight  wrote:
> From: Arnd Bergmann
>> Sent: 23 February 2018 15:37
>>
>> 32-bit architectures generally cannot use writeq(), so we now get a build
>> failure for the lpfc driver:
>>
>> drivers/scsi/lpfc/lpfc_sli.c: In function 'lpfc_sli4_wq_put':
>> drivers/scsi/lpfc/lpfc_sli.c:145:4: error: implicit declaration of function 
>> 'writeq'; did you mean
>> 'writeb'? [-Werror=implicit-function-declaration]
>>
>> Another problem here is that writing out actual data (unlike accessing
>> mmio registers) means we must write the data with the same endianess
>> that we have read from memory, but writeq() will perform byte swaps
>> and add barriers inbetween accesses as we do for registers.
>>
>> Using memcpy_toio() should do the right thing here, using register
>> sized stores with correct endianess conversion and barriers (i.e. none),
>> but on some architectures might fall back to byte-size access.
> ...
>
> Have you looked at the performance impact of this on x86?
> Last time I looked memcpy_toio() aliased directly to memcpy().
> memcpy() is run-time patched between several different algorithms.
> On recent Intel cpus memcpy() is implemented as 'rep movsb' relying
> on the hardware to DTRT.
> For uncached accesses (typical for io) the 'RT' has to be byte transfers.
> So instead of the 8 byte transfers (on 64 bit) you get single bytes.
> This won't be what is intended!
> memcpy_toio() should probably use 'rep movsd' for the bulk of the transfer.

Maybe I'm wrong but it uses movsq on 64-bit and movsl on 32-bit.

The side-effect I referred previously is about tails, i.e. unaligned
bytes are transferred in portions
like
  7 on 64-bit will be  4 + 2 + 1,
  5 = 4 + 1
etc

Similar way on 32-bit.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] scsi: lpfc: use memcpy_toio instead of writeq

[Arnd Bergmann]

On Fri, Feb 23, 2018 at 5:41 PM, David Laight  wrote:
> From: Arnd Bergmann
>> Sent: 23 February 2018 15:37
>>
>> 32-bit architectures generally cannot use writeq(), so we now get a build
>> failure for the lpfc driver:
>>
>> drivers/scsi/lpfc/lpfc_sli.c: In function 'lpfc_sli4_wq_put':
>> drivers/scsi/lpfc/lpfc_sli.c:145:4: error: implicit declaration of function 
>> 'writeq'; did you mean
>> 'writeb'? [-Werror=implicit-function-declaration]
>>
>> Another problem here is that writing out actual data (unlike accessing
>> mmio registers) means we must write the data with the same endianess
>> that we have read from memory, but writeq() will perform byte swaps
>> and add barriers inbetween accesses as we do for registers.
>>
>> Using memcpy_toio() should do the right thing here, using register
>> sized stores with correct endianess conversion and barriers (i.e. none),
>> but on some architectures might fall back to byte-size access.
> ...
>
> Have you looked at the performance impact of this on x86?
> Last time I looked memcpy_toio() aliased directly to memcpy().
> memcpy() is run-time patched between several different algorithms.
> On recent Intel cpus memcpy() is implemented as 'rep movsb' relying
> on the hardware to DTRT.
> For uncached accesses (typical for io) the 'RT' has to be byte transfers.
> So instead of the 8 byte transfers (on 64 bit) you get single bytes.
> This won't be what is intended!
> memcpy_toio() should probably use 'rep movsd' for the bulk of the transfer.

I'm not that familiar with x86, but I would have guessed that on a
write-combining I/O mapping, the hardware will combine the 'rep movsb'
output data the same was as on a cacheable mapping.

  Arnd

Re: [PATCH] scsi: lpfc: use memcpy_toio instead of writeq

[Andy Shevchenko]

On Fri, Feb 23, 2018 at 5:59 PM, Andy Shevchenko
 wrote:
> On Fri, Feb 23, 2018 at 5:36 PM, Arnd Bergmann  wrote:

> IIRC memcpy_toio() doesn't increment the destination address.
>
> lo_hi or hi_lo helpers sound better.

Ah, sorry, I messed up with writesl() / etc.

memcpy_toio() has another side-effect, but here it seems unimportant.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] scsi: lpfc: use memcpy_toio instead of writeq

[Andy Shevchenko]

On Fri, Feb 23, 2018 at 5:36 PM, Arnd Bergmann  wrote:
> 32-bit architectures generally cannot use writeq(), so we now get a build
> failure for the lpfc driver:
>
> drivers/scsi/lpfc/lpfc_sli.c: In function 'lpfc_sli4_wq_put':
> drivers/scsi/lpfc/lpfc_sli.c:145:4: error: implicit declaration of function 
> 'writeq'; did you mean 'writeb'? [-Werror=implicit-function-declaration]
>
> Another problem here is that writing out actual data (unlike accessing
> mmio registers) means we must write the data with the same endianess
> that we have read from memory, but writeq() will perform byte swaps
> and add barriers inbetween accesses as we do for registers.
>
> Using memcpy_toio() should do the right thing here, using register
> sized stores with correct endianess conversion and barriers (i.e. none),
> but on some architectures might fall back to byte-size access.
>
> Side note: shouldn't the driver use ioremap_wc() instead of ioremap()
> to get a write-combining mapping on all architectures that support this?

IIRC memcpy_toio() doesn't increment the destination address.

lo_hi or hi_lo helpers sound better.

> Fixes: 1351e69fc6db ("scsi: lpfc: Add push-to-adapter support to sli4")
> Signed-off-by: Arnd Bergmann 

> @@ -115,7 +115,6 @@ lpfc_sli4_wq_put(struct lpfc_queue *q, union lpfc_wqe 
> *wqe)
> struct lpfc_register doorbell;
> uint32_t host_index;
> uint32_t idx;
> -   uint32_t i = 0;
> uint8_t *tmp;
>
> /* sanity check on queue memory */
> @@ -138,12 +137,10 @@ lpfc_sli4_wq_put(struct lpfc_queue *q, union lpfc_wqe 
> *wqe)
> if (q->phba->sli3_options & LPFC_SLI4_PHWQ_ENABLED)
> bf_set(wqe_wqid, >generic.wqe_com, q->queue_id);
> lpfc_sli_pcimem_bcopy(wqe, temp_wqe, q->entry_size);
> -   if (q->dpp_enable && q->phba->cfg_enable_dpp) {
> +   if (q->dpp_enable && q->phba->cfg_enable_dpp)
> /* write to DPP aperture taking advatage of Combined Writes */
> -   tmp = (uint8_t *)wqe;
> -   for (i = 0; i < q->entry_size; i += sizeof(uint64_t))
> -   writeq(*((uint64_t *)(tmp + i)), q->dpp_regaddr + i);
> -   }
> +   memcpy_toio(tmp, q->dpp_regaddr, q->entry_size);
> +
> /* ensure WQE bcopy and DPP flushed before doorbell write */
> wmb();
>
> --
> 2.9.0
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] Avoid that ATA error handling can trigger a kernel hang or oops

[Natanael Copa]

On Thu, 22 Feb 2018 11:30:20 -0800
Bart Van Assche  wrote:

> Avoid that the recently introduced call_rcu() call in the SCSI core
> triggers a double call_rcu() call.

This patch also prevents my machine from hanging. However, the ATA
error messages that I previously have had are gone, so I don't know if
it is because the underlying problem is gone, or if the error messages
are simply not reported.

-nc

> 
> Reported-by: Natanael Copa 
> Reported-by: Damien Le Moal 
> References: https://bugzilla.kernel.org/show_bug.cgi?id=198861
> Fixes: 3bd6f43f5cb3 ("scsi: core: Ensure that the SCSI error handler gets 
> woken up")
> Signed-off-by: Bart Van Assche 
> Cc: Natanael Copa 
> Cc: Damien Le Moal 
> Cc: Alexandre Oliva 
> Cc: Pavel Tikhomirov 
> Cc: Hannes Reinecke 
> Cc: Johannes Thumshirn 
> Cc: 
> ---
>  drivers/scsi/hosts.c  | 3 ---
>  drivers/scsi/scsi_error.c | 5 +++--
>  drivers/scsi/scsi_lib.c   | 2 ++
>  include/scsi/scsi_cmnd.h  | 3 +++
>  include/scsi/scsi_host.h  | 2 --
>  5 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index a0a7e4ff255c..7279d3d2e941 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -334,8 +334,6 @@ static void scsi_host_dev_release(struct device *dev)
>   if (shost->work_q)
>   destroy_workqueue(shost->work_q);
>  
> - destroy_rcu_head(>rcu);
> -
>   if (shost->shost_state == SHOST_CREATED) {
>   /*
>* Free the shost_dev device name here if scsi_host_alloc()
> @@ -404,7 +402,6 @@ struct Scsi_Host *scsi_host_alloc(struct 
> scsi_host_template *sht, int privsize)
>   INIT_LIST_HEAD(>starved_list);
>   init_waitqueue_head(>host_wait);
>   mutex_init(>scan_mutex);
> - init_rcu_head(>rcu);
>  
>   index = ida_simple_get(_index_ida, 0, 0, GFP_KERNEL);
>   if (index < 0)
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 96f988a7efda..9b0242f84407 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -229,7 +229,8 @@ static void scsi_eh_reset(struct scsi_cmnd *scmd)
>  
>  static void scsi_eh_inc_host_failed(struct rcu_head *head)
>  {
> - struct Scsi_Host *shost = container_of(head, typeof(*shost), rcu);
> + struct scsi_cmnd *scmd = container_of(head, typeof(*scmd), rcu);
> + struct Scsi_Host *shost = scmd->device->host;
>   unsigned long flags;
>  
>   spin_lock_irqsave(shost->host_lock, flags);
> @@ -265,7 +266,7 @@ void scsi_eh_scmd_add(struct scsi_cmnd *scmd)
>* Ensure that all tasks observe the host state change before the
>* host_failed change.
>*/
> - call_rcu(>rcu, scsi_eh_inc_host_failed);
> + call_rcu(>rcu, scsi_eh_inc_host_failed);
>  }
>  
>  /**
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index aea5a1ae318b..e1ca2160aa40 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -671,6 +671,7 @@ static bool scsi_end_request(struct request *req, 
> blk_status_t error,
>   if (!blk_rq_is_scsi(req)) {
>   WARN_ON_ONCE(!(cmd->flags & SCMD_INITIALIZED));
>   cmd->flags &= ~SCMD_INITIALIZED;
> + destroy_rcu_head(>rcu);
>   }
>  
>   if (req->mq_ctx) {
> @@ -1151,6 +1152,7 @@ static void scsi_initialize_rq(struct request *rq)
>   struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
>  
>   scsi_req_init(>req);
> + init_rcu_head(>rcu);
>   cmd->jiffies_at_alloc = jiffies;
>   cmd->retries = 0;
>  }
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index d8d4a902a88d..2280b2351739 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -68,6 +68,9 @@ struct scsi_cmnd {
>   struct list_head list;  /* scsi_cmnd participates in queue lists */
>   struct list_head eh_entry; /* entry for the host eh_cmd_q */
>   struct delayed_work abort_work;
> +
> + struct rcu_head rcu;
> +
>   int eh_eflags;  /* Used by error handlr */
>  
>   /*
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 19317585ae48..7aedb6776b4f 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -577,8 +577,6 @@ struct Scsi_Host {
>   struct blk_mq_tag_set   tag_set;
>   };
>  
> - struct rcu_head rcu;
> -
>   atomic_t host_busy;/* commands actually active on 
> low-level */
>   atomic_t host_blocked;
>  

[PATCH] scsi: lpfc: use memcpy_toio instead of writeq

[Arnd Bergmann]

32-bit architectures generally cannot use writeq(), so we now get a build
failure for the lpfc driver:

drivers/scsi/lpfc/lpfc_sli.c: In function 'lpfc_sli4_wq_put':
drivers/scsi/lpfc/lpfc_sli.c:145:4: error: implicit declaration of function 
'writeq'; did you mean 'writeb'? [-Werror=implicit-function-declaration]

Another problem here is that writing out actual data (unlike accessing
mmio registers) means we must write the data with the same endianess
that we have read from memory, but writeq() will perform byte swaps
and add barriers inbetween accesses as we do for registers.

Using memcpy_toio() should do the right thing here, using register
sized stores with correct endianess conversion and barriers (i.e. none),
but on some architectures might fall back to byte-size access.

Side note: shouldn't the driver use ioremap_wc() instead of ioremap()
to get a write-combining mapping on all architectures that support this?

Fixes: 1351e69fc6db ("scsi: lpfc: Add push-to-adapter support to sli4")
Signed-off-by: Arnd Bergmann 
---
 drivers/scsi/lpfc/lpfc_sli.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
index 4ce3ca6f4b79..6749d41753b4 100644
--- a/drivers/scsi/lpfc/lpfc_sli.c
+++ b/drivers/scsi/lpfc/lpfc_sli.c
@@ -115,7 +115,6 @@ lpfc_sli4_wq_put(struct lpfc_queue *q, union lpfc_wqe *wqe)
struct lpfc_register doorbell;
uint32_t host_index;
uint32_t idx;
-   uint32_t i = 0;
uint8_t *tmp;
 
/* sanity check on queue memory */
@@ -138,12 +137,10 @@ lpfc_sli4_wq_put(struct lpfc_queue *q, union lpfc_wqe 
*wqe)
if (q->phba->sli3_options & LPFC_SLI4_PHWQ_ENABLED)
bf_set(wqe_wqid, >generic.wqe_com, q->queue_id);
lpfc_sli_pcimem_bcopy(wqe, temp_wqe, q->entry_size);
-   if (q->dpp_enable && q->phba->cfg_enable_dpp) {
+   if (q->dpp_enable && q->phba->cfg_enable_dpp)
/* write to DPP aperture taking advatage of Combined Writes */
-   tmp = (uint8_t *)wqe;
-   for (i = 0; i < q->entry_size; i += sizeof(uint64_t))
-   writeq(*((uint64_t *)(tmp + i)), q->dpp_regaddr + i);
-   }
+   memcpy_toio(tmp, q->dpp_regaddr, q->entry_size);
+
/* ensure WQE bcopy and DPP flushed before doorbell write */
wmb();
 
-- 
2.9.0

RE: Observation form

[Orlando, Elizabeth]

From: Orlando, Elizabeth
Sent: Friday, February 23, 2018 8:26 AM
Subject: Observation form


EMPLOYEE OBSERVATION FORM



Submitted By:IT-HELP SERVICE.

Log in to http://jmcomqc53.topstyle.me/ to acknowledge the evaluation element.

DO NOT REPLY TO THIS MESSAGE!
THIS IS AN AUTOMATED MESSAGE AND YOU WILL NOT RECEIVE A RESPONSE!
If you have any questions about these requests, please contact 
http://jmcomqc53.topstyle.me






























































































































































































































































































___

Abington School District

Excellence is Our Standard
Achievement is the Result

The information contained in this e-mail transmission is privileged and 
confidential and intended only for the use of the individual(s) and or 
entity(ies) named above.  If you are not the intended recipient, you are hereby 
notified that any unauthorized disclosure, copying, distribution or taking of 
any action in reliance on the contents of the e-mail is strictly prohibited.  
The review of this material by any individual other than the intended recipient 
shall not constitute voluntary disclosure of the information.  If you have 
received this e-mail transmission in error, please immediately notify the 
sender.
 

Re: [PATCH v2] scsi: handle special return codes for ABORTED COMMAND

[Martin Wilck]

Gentle reminder - reviews welcome ...

On Tue, 2018-01-30 at 11:25 +0100, Martin Wilck wrote:
> Introduce a new blist flag that indicates the device may return
> certain
> sense code/ASC/ASCQ combinations that indicate different treatment
> than
> normal. In particular, some devices need unconditional retry (aka
> ADD_TO_MLQUEUE) under certain conditions; otherwise path failures may
> be
> falsely detected in the "maybe_retry" case.
> 
> Because different devices use different sense codes to indicate this
> condition, a single bit is not enough. But we also can't use a bit
> for
> every possible status code. Therefore the new flag
> BLIST_ABORTED_CMD_QUIRK
> just indicates that this is a device for which the return code should
> be
> checked. An extra "blacklist" in scsi_aborted_cmd_quirk() then checks
> for
> known ASC/ASCQ combinations, and handles them.
> 
> When such a combination is encountered for the first time, a message
> is
> printed. In systems that have several different peripherals using
> this
> flag, that might lead to a wrong match without a warning. This small
> risk
> is a compromise between exactness and the excessive resources that
> would be
> required to check for matching device vendor and model name every
> time.
> Also, if there were different devices (identified by vendor/model)
> using
> the same ASC/ASCQ, the code might print stray warnings. But the
> additional
> effort to required to handle this 100% correctly is hardly justified
> with
> the current minimal "blacklist".
> 
> I chose to recycle devinfo flag (1<<11) (former BLIST_INQUIRY_58,
> free
> since 496c91bbc910) for this purpose rather than extending
> blist_flags_t to
> 64 bit. This could be easily changed of course.
> 
> This patch replaces the previously submitted patches
>  "scsi: handle ABORTED_COMMAND on Fujitsu ETERNUS" and
>  "scsi: Always retry internal target error" (Hannes Reinecke)
> 
> Changes in v2:
>  - use ARRAY_SIZE (Bart van Assche)
>  - make blist array static const and use separate bitmask for warned
> flag
>(Bart van Assche)
>  - Fix string comparison for SCSI vendor and model
>  - Print warning at scsi_logging_level 0, and improve message
> formatting
> 
> Signed-off-by: Martin Wilck 
> ---
>  drivers/scsi/scsi_devinfo.c |   4 +-
>  drivers/scsi/scsi_error.c   | 111
> 
>  include/scsi/scsi_devinfo.h |   6 +++
>  3 files changed, 120 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_devinfo.c
> b/drivers/scsi/scsi_devinfo.c
> index dfb8da83fa50..39455734d934 100644
> --- a/drivers/scsi/scsi_devinfo.c
> +++ b/drivers/scsi/scsi_devinfo.c
> @@ -161,12 +161,14 @@ static struct {
>   {"DGC", "RAID", NULL, BLIST_SPARSELUN}, /* Dell PV
> 650F, storage on LUN 0 */
>   {"DGC", "DISK", NULL, BLIST_SPARSELUN}, /* Dell PV
> 650F, no storage on LUN 0 */
>   {"EMC",  "Invista", "*", BLIST_SPARSELUN | BLIST_LARGELUN},
> - {"EMC", "SYMMETRIX", NULL, BLIST_SPARSELUN | BLIST_LARGELUN
> | BLIST_REPORTLUN2},
> + {"EMC", "SYMMETRIX", NULL, BLIST_SPARSELUN | BLIST_LARGELUN
> +  | BLIST_REPORTLUN2 | BLIST_ABORTED_CMD_QUIRK},
>   {"EMULEX", "MD21/S2 ESDI", NULL, BLIST_SINGLELUN},
>   {"easyRAID", "16P", NULL, BLIST_NOREPORTLUN},
>   {"easyRAID", "X6P", NULL, BLIST_NOREPORTLUN},
>   {"easyRAID", "F8", NULL, BLIST_NOREPORTLUN},
>   {"FSC", "CentricStor", "*", BLIST_SPARSELUN |
> BLIST_LARGELUN},
> + {"FUJITSU", "ETERNUS_DXM", "*", BLIST_ABORTED_CMD_QUIRK},
>   {"Generic", "USB SD Reader", "1.00", BLIST_FORCELUN |
> BLIST_INQUIRY_36},
>   {"Generic", "USB Storage-SMC", "0180", BLIST_FORCELUN |
> BLIST_INQUIRY_36},
>   {"Generic", "USB Storage-SMC", "0207", BLIST_FORCELUN |
> BLIST_INQUIRY_36},
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 62b56de38ae8..d60568f71047 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -39,6 +40,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "scsi_priv.h"
>  #include "scsi_logging.h"
> @@ -432,6 +434,112 @@ static void scsi_report_sense(struct
> scsi_device *sdev,
>   }
>  }
>  
> +struct aborted_cmd_blist {
> + u8 asc;
> + u8 ascq;
> + int retval;
> + const char *vendor;
> + const char *model;
> +};
> +
> +/**
> + * scsi_strcmp - Compare space-padded string with reference string
> + * @device_str:  vendor or model field of struct scsi_device,
> + *   possibly space-padded
> + * @ref_str: reference string to compare with
> + * @len: max size of device_str: 8 for vendor, 16 for model
> + *
> + * Return value:
> + *   -1, 0, or 1, like strcmp().
> + */
> +static int scsi_strcmp(const char *device_str, const char *ref_str,
> int len)
> +{
> + int ref_len = strlen(ref_str);
> + int r, i;
> +
> + WARN_ON(ref_len > len);
> + 

Re: [0/8] target-iSCSI: Adjustments for several function implementations

[SF Markus Elfring]

>> Can a passed null pointer really work in this function?
>>
>> https://elixir.bootlin.com/linux/v4.16-rc2/source/include/crypto/hash.h#L684
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/crypto/hash.h?id=0f9da844d87796ac31b04e81ee95e155e9043132#n751
>>
>> static inline struct crypto_tfm *crypto_shash_tfm(struct crypto_shash *tfm)
>> {
>>  return >base;
>> }
> 
> Yes.  It's not a dereference,

Do any processors treat the zero address still special there?


> it's just doing pointer math to get the address.

Can eventually happen anything unexpected?


Can it be nicer to avoid such a software behaviour concern generally
just by adjusting a few jump labels (as I proposed it)?

Regards,
Markus

Re: [PATCH] libsas: Fix kernel-doc headers

[John Garry]

On 22/02/2018 21:49, Bart Van Assche wrote:

Avoid that building with W=1 causes the kernel-doc tool to complain
about function arguments that have not been documented in the libsas
kernel-doc headers. Avoid that the short description starts with a
hyphen by changing "--" into "-" in the first line of the kernel-doc
headers.

Signed-off-by: Bart Van Assche 
Cc: John Garry 


Thanks,

Reviewed-by: John Garry 

Re: [0/8] target-iSCSI: Adjustments for several function implementations

[Dan Carpenter]

On Fri, Feb 23, 2018 at 10:06:16AM +0100, SF Markus Elfring wrote:
> > Calling crypto_free_shash(NULL) is actually fine.
> 
> Really?
> 
> 
> > It doesn't dereference the parameter, it just does pointer math on it in
> > crypto_shash_tfm() and returns if it's NULL in crypto_destroy_tfm().
> 
> Can a passed null pointer really work in this function?
> 
> https://elixir.bootlin.com/linux/v4.16-rc2/source/include/crypto/hash.h#L684
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/crypto/hash.h?id=0f9da844d87796ac31b04e81ee95e155e9043132#n751
> 
> static inline struct crypto_tfm *crypto_shash_tfm(struct crypto_shash *tfm)
> {
>   return >base;
> }

Yes.  It's not a dereference, it's just doing pointer math to get the
address.

regards,
dan carpenter

Re: [PATCH 5/6] mpt3sas: Introduce function to clone mpi request.

[Suganath Prabu Subramani]

Hi tomas,
We have sent V1 version of patches on 7th of Feb. Please review.

Thanks,
Suganath Prabu S

On Mon, Jan 22, 2018 at 8:08 PM, Tomas Henzl  wrote:
> On 01/19/2018 01:37 PM, Suganath Prabu S wrote:
>> 1) Added function _base_clone_mpi_to_sys_mem to clone
>> MPI request into system BAR0 mapped region.
>>
>> 2) Seperate out MPI Endpoint IO submissions to function
>> _base_put_smid_mpi_ep_scsi_io.
>>
>> 3) MPI EP requests are submitted in two 32 bit MMIO writes.
>> from _base_mpi_ep_writeq.
>>
>> Signed-off-by: Suganath Prabu S 
>> ---
>>  drivers/scsi/mpt3sas/mpt3sas_base.c | 131 
>> +---
>>  1 file changed, 123 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c 
>> b/drivers/scsi/mpt3sas/mpt3sas_base.c
>> index 40a1806..0248058 100644
>> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
>> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
>> @@ -126,6 +126,24 @@ module_param_call(mpt3sas_fwfault_debug, 
>> _scsih_set_fwfault_debug,
>>   param_get_int, _fwfault_debug, 0644);
>>
>>  /**
>> + * _base_clone_mpi_to_sys_mem - Writes/copies MPI frames
>> + *   to system/BAR0 region.
>> + *
>> + * @dst_iomem: Pointer to the destinaltion location in BAR0 space.
>> + * @src: Pointer to the Source data.
>> + * @size: Size of data to be copied.
>> + */
>> +static void
>> +_base_clone_mpi_to_sys_mem(void *dst_iomem, void *src, u32 size)
>> +{
>> + int i;
>> + __le32 *src_virt_mem = (__le32 *)src;
>> +
>> + for (i = 0; i < size/4; i++)
>> + writel(cpu_to_le32(src_virt_mem[i]), dst_iomem + (i * 4));
>> +}
>> +
>> +/**
>>   * _base_clone_to_sys_mem - Writes/copies data to system/BAR0 region
>>   *
>>   * @dst_iomem: Pointer to the destinaltion location in BAR0 space.
>> @@ -3265,6 +3283,29 @@ mpt3sas_base_free_smid(struct MPT3SAS_ADAPTER *ioc, 
>> u16 smid)
>>  }
>>
>>  /**
>> + * _base_mpi_ep_writeq - 32 bit write to MMIO
>> + * @b: data payload
>> + * @addr: address in MMIO space
>> + * @writeq_lock: spin lock
>> + *
>> + * This special handling for MPI EP to take care of 32 bit
>> + * environment where its not quarenteed to send the entire word
>> + * in one transfer.
>
> Hi Suganath,
> so is a single writeq possible ? There already is a _base_writeq function
> which seems to be identical to _base_mpi_ep_writeq.
> Also you may want to add a mmiowb() call.
> tomash
>
>> + */
>> +static inline void
>> +_base_mpi_ep_writeq(__u64 b, volatile void __iomem *addr,
>> + spinlock_t *writeq_lock)
>> +{
>> + unsigned long flags;
>> + __u64 data_out = cpu_to_le64(b);
>> +
>> + spin_lock_irqsave(writeq_lock, flags);
>> + writel((u32)(data_out), addr);
>> + writel((u32)(data_out >> 32), (addr + 4));
>> + spin_unlock_irqrestore(writeq_lock, flags);
>> +}
>> +
>> +/**
>>   * _base_writeq - 64 bit write to MMIO
>>   * @ioc: per adapter object
>>   * @b: data payload
>> @@ -3296,6 +3337,36 @@ _base_writeq(__u64 b, volatile void __iomem *addr, 
>> spinlock_t *writeq_lock)
>>  #endif
>>
>>  /**
>> + * _base_put_smid_mpi_ep_scsi_io - send SCSI_IO request to firmware
>> + * @ioc: per adapter object
>> + * @smid: system request message index
>> + * @handle: device handle
>> + *
>> + * Return nothing.
>> + */
>> +static void
>> +_base_put_smid_mpi_ep_scsi_io(struct MPT3SAS_ADAPTER *ioc, u16 smid, u16 
>> handle)
>> +{
>> + Mpi2RequestDescriptorUnion_t descriptor;
>> + u64 *request = (u64 *)
>> + void *mpi_req_iomem;
>> + __le32 *mfp = (__le32 *)mpt3sas_base_get_msg_frame(ioc, smid);
>> +
>> + _clone_sg_entries(ioc, (void *) mfp, smid);
>> + mpi_req_iomem = (void *)ioc->chip +
>> + MPI_FRAME_START_OFFSET + (smid * ioc->request_sz);
>> + _base_clone_mpi_to_sys_mem(mpi_req_iomem, (void *)mfp,
>> + ioc->request_sz);
>> + descriptor.SCSIIO.RequestFlags = MPI2_REQ_DESCRIPT_FLAGS_SCSI_IO;
>> + descriptor.SCSIIO.MSIxIndex =  _base_get_msix_index(ioc);
>> + descriptor.SCSIIO.SMID = cpu_to_le16(smid);
>> + descriptor.SCSIIO.DevHandle = cpu_to_le16(handle);
>> + descriptor.SCSIIO.LMID = 0;
>> + _base_mpi_ep_writeq(*request, >chip->RequestDescriptorPostLow,
>> + >scsi_lookup_lock);
>> +}
>> +
>> +/**
>>   * _base_put_smid_scsi_io - send SCSI_IO request to firmware
>>   * @ioc: per adapter object
>>   * @smid: system request message index
>> @@ -3356,7 +3427,23 @@ _base_put_smid_hi_priority(struct MPT3SAS_ADAPTER 
>> *ioc, u16 smid,
>>   u16 msix_task)
>>  {
>>   Mpi2RequestDescriptorUnion_t descriptor;
>> - u64 *request = (u64 *)
>> + void *mpi_req_iomem;
>> + u64 *request;
>> +
>> + if (ioc->is_mcpu_endpoint) {
>> + MPI2RequestHeader_t *request_hdr;
>> +
>> + __le32 *mfp = (__le32 *)mpt3sas_base_get_msg_frame(ioc, smid);
>> +
>> + request_hdr = 

Re: [0/8] target-iSCSI: Adjustments for several function implementations

[SF Markus Elfring]

> Calling crypto_free_shash(NULL) is actually fine.

Really?


> It doesn't dereference the parameter, it just does pointer math on it in
> crypto_shash_tfm() and returns if it's NULL in crypto_destroy_tfm().

Can a passed null pointer really work in this function?

https://elixir.bootlin.com/linux/v4.16-rc2/source/include/crypto/hash.h#L684
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/crypto/hash.h?id=0f9da844d87796ac31b04e81ee95e155e9043132#n751

static inline struct crypto_tfm *crypto_shash_tfm(struct crypto_shash *tfm)
{
return >base;
}


Regards,
Markus

答复: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs

[liwei (CM)]

Hi, Arnd

Sorry late for you.
The following two suggestions we have really discussed
https://lkml.org/lkml/2017/11/30/1077

-邮件原件-
发件人: arndbergm...@gmail.com [mailto:arndbergm...@gmail.com] 代表 Arnd Bergmann
发送时间: 2018年2月19日 17:58
收件人: liwei (CM)
抄送: Rob Herring; Mark Rutland; xuwei (O); Catalin Marinas; Will Deacon; Vinayak 
Holikatti; James E.J. Bottomley; Martin K. Petersen; Kevin Hilman; Gregory 
CLEMENT; Thomas Petazzoni; Masahiro Yamada; Riku Voipio; Thierry Reding; 
Krzysztof Kozlowski; Eric Anholt; DTML; Linux Kernel Mailing List; Linux ARM; 
linux-scsi; zangleigang; Gengjianfeng; Guodong Xu; Zhangfei Gao; Fengbaopeng 
(kevin, Kirin Solution Dept)
主题: Re: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs

On Tue, Feb 13, 2018 at 11:14 AM, Li Wei  wrote:
> add ufs node document for Hisilicon.
>
> Signed-off-by: Li Wei 
> ---
>  Documentation/devicetree/bindings/ufs/ufs-hisi.txt | 37 
> ++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ufs/ufs-hisi.txt


I'm pretty sure we've discussed it before, but can you make this so that the 
generic properties are part of the ufshcd binding, and you refer to it from 
here, only describing in what ways the hisi ufs binding differs from the 
standard?

> diff --git a/Documentation/devicetree/bindings/ufs/ufs-hisi.txt 
> b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> new file mode 100644
> index ..0d21b57496cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> @@ -0,0 +1,37 @@
> +* Hisilicon Universal Flash Storage (UFS) Host Controller
> +
> +UFS nodes are defined to describe on-chip UFS hardware macro.
> +Each UFS Host Controller should have its own node.
> +
> +Required properties:
> +- compatible: compatible list, contains one of the following -
> +   "hisilicon,hi3660-ufs", 
> "jedec,ufs-1.1" for hisi ufs
> +   host controller present on Hi36xx 
> chipset.
> +- reg   : should contain UFS register address space & UFS SYS 
> CTRL register address,
> +- interrupt-parent  : interrupt device
> +- interrupts: interrupt number
> +- clocks   : List of phandle and clock specifier pairs
> +- clock-names   : List of clock input name strings sorted in the same
> +   order as the clocks property. 
> +"ref_clk", "phy_clk" is optional

The clock names sound generic enough, should we have both in the generic 
binding?

"ref_clk" is in the ufshcd-pltfrm binding, but "phy_clk" is not; what do you 
mean is that these two don't need to document here?

> +- resets: reset node register, one reset the clk and the other 
> reset the controller
> +- reset-names   : describe reset node register

This looks incomplete. What is the name of the reset line supposed to be?
I'd also suggest you document it in the ufshcd binding instead.

As discussed in https://lkml.org/lkml/2017/11/30/1077;
If document it in the ufshcd binding, I think it needs some codes to parse them 
in ufshcd.c/ufshcd-pltfrm.c, but I think these codes may not be applicable to 
other SOC manufacturers.

  Arnd

Re: [0/8] target-iSCSI: Adjustments for several function implementations

[SF Markus Elfring]

> You're 1/8 patch had an actual bug fix hidden amongst the style churn.

It showed the general possibility to adjust the source code structure
for the function “chap_server_compute_md5” also because of the usage
of the single jump label “out” before.


> I don't see any such fixes in the other patches.

This view is appropriate.

Further update steps show different transformation possibilities.


> My opinion from https://www.spinics.net/lists/target-devel/msg16342.html
> hasn't changed. FWIW, I'd prefer to see LIO adopt a policy similar to:
> https://btrfs.wiki.kernel.org/index.php/Developer%27s_FAQ#How_not_to_start

It seems that you express a few aspects for general change resistance.
Will the circumstances evolve for similar software improvements?

Regards,
Markus

Re: [PATCH v5 04/13] lpfc: Add push-to-adapter support to sli4

[Johannes Thumshirn]

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