Re: [PATCH] scsi: mpt3sas: clarify mmio pointer types

[Martin K. Petersen]

Arnd,

> The newly added code mixes up phys_addr_t/resource_size_t with dma_addr_t
> and void pointers, as seen from these compiler warning:

Applied to 4.17/scsi-queue. Thanks for fixing this up!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] scsi: libsas: defer ata device eh commands to libata

[Dan Williams]

On Tue, Mar 6, 2018 at 10:04 AM, Martin K. Petersen
 wrote:
>
>> When ata device doing EH, some commands still attached with tasks are not
>> passed to libata when abort failed or recover failed, so libata did not
>> handle these commands. After these commands done, sas task is freed, but
>> ata qc is not freed.
>
> Somebody from the libsas camp, please review!
>
> https://patchwork.kernel.org/patch/10244375/

It's been a while since I've had access to libsas hardware, but this
patch looks like a good idea to me. However, it seems that it could be
narrowed to just the sas_eh_finish_cmd() that need to be converted to
sas_eh_defer_cmd(). Otherwise, if there are no more direct usages of
sas_eh_finish_cmd() then we should consider just renaming
sas_eh_defer_cmd() to sas_eh_finish_cmd() and delete the old
sas_eh_finish_cmd(), i.e. remove the distinction.

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Hi Finn,

I'll leave the unused !write branch in the original condition. I'd
rather keep the address conversion inside the PIO code than
duplicating it in each DMA setup routine though.

You had asked what manual I had used a while ago: it's
http://bitsavers.trailing-edge.com/components/ncr/scsi/53CF94_96-2_Fast_SCSI_Controller_Data_Manual_Apr1993.pdf

Cheers,

  Michael


On Tue, Mar 6, 2018 at 9:37 PM, Finn Thain  wrote:
> On Tue, 6 Mar 2018, Michael Schmitz wrote:
>
>> The whole !write branch will never be executed, and I could just omit it
>> entirely for now, or leave it as it was in the Mac driver.
>>
>
> We could make use of the !write branch in zorro_esp, even if it was only
> to figure out the SELAS/MSG OUT issue for the benefit of mac_esp. But
> let's keep them in sync.
>
> --
>
>> Cheers,
>>
>>   Michael
>>

Re: [PATCH V3 1/8] scsi: hpsa: fix selection of reply queue

[Martin K. Petersen]

Ming,

> Given both Don and Laurence have verified that patch 1 and patch 2
> does fix IO hang, could you consider to merge the two first?

Oh, and I would still need a formal Acked-by: from Don and Tested-by:
from Laurence.

Also, for 4.16/scsi-fixes I would prefer verification to be done with
just patch 1/8 and none of the subsequent changes in place. Just to make
sure we're testing the right thing.

Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v2 00/38] cxlflash: OCXL transport support

[Martin K. Petersen]

> This patch series adds OCXL support to the cxlflash driver. With this
> support, new devices using the OCXL transport will be supported by the
> cxlflash driver along with the existing CXL devices. An effort is made
> to keep this transport specific function independent of the existing
> core driver that communicates with the AFU.

This one has been sitting for a week without any reviews. Andrew, when
will you have time to take a look?

Being a pretty big chunk of code, I would like to merge it sooner rather
than later. Or we can postpone until 4.18, of course.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH V3 1/8] scsi: hpsa: fix selection of reply queue

[Martin K. Petersen]

Hi Ming,

> Given both Don and Laurence have verified that patch 1 and patch 2
> does fix IO hang, could you consider to merge the two first?

I'm not going to merge the MR patch until Kashyap acks it.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] scsi: libsas: defer ata device eh commands to libata

[Martin K. Petersen]

> When ata device doing EH, some commands still attached with tasks are not
> passed to libata when abort failed or recover failed, so libata did not
> handle these commands. After these commands done, sas task is freed, but
> ata qc is not freed.

Somebody from the libsas camp, please review!

https://patchwork.kernel.org/patch/10244375/

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Hi Geert,

On Tue, Mar 6, 2018 at 8:48 PM, Geert Uytterhoeven  wrote:
> BTW, please call the probe/remove functions zorro_esp_probe() resp.
> zorro_esp_remove().

Fair enough.

 +   if (!host) {
 +   pr_err(PFX "No host detected; board configuration 
 problem?\n");
 +   goto out_free;
 +   }
>>
>> here. But I can add the err=-NOMEM here.
>
> After out_free it returns fixed -ENODEV ;-)

Fixed that in my tree already. But I now have six err=-ENOMEM peppered
all over the probe code. Still, if it makes it more readable ...

> Doing "err = -ENOMEM" here, and returning err at the end is better, as
> it propagates meaningful error codes.

Yes, I've belatedly realized that now.

The major obstacle now seems to be dynamic allocation of the driver
private data and storing a pointer to that in a way that it can be
retrieved using just the esp pointer. dev_set_drvdata(esp->dev, zep)
causes the module load to crash ...

Cheers,

  Michael


>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds

Re: [PATCH] Improve ZBC/ZAC error handling

[Martin K. Petersen]

Tejun,

> Except for the nit on the last patch, ata part looks good to me.
> Martin, how do you wanna route the SCSI part?

I want to route it to /dev/null on the grounds of being a BLATANT
LAYERING VIOLATION (cue dramatic sound effect).

scsi_error.c is SPC territory, we really shouldn't wedge any ZBC/SBC
stuff in there. Nor should we call into this file from libata. If
there's a ZAC/ZBC SAT retry deficiency, let's address that instead of
working around it.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] scsi: ipr: Use dma_pool_zalloc()

[Brian King]

On 03/06/2018 02:52 PM, Souptick Joarder wrote:
> Any comment for this patch.
> 
> On Sat, Feb 17, 2018 at 9:56 PM, Souptick Joarder  
> wrote:
>> Use dma_pool_zalloc() instead of dma_pool_alloc + memset
>>
>> Signed-off-by: Souptick Joarder 
>> ---
>>  drivers/scsi/ipr.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
>> index e07dd99..97387be 100644
>> --- a/drivers/scsi/ipr.c
>> +++ b/drivers/scsi/ipr.c
>> @@ -9684,14 +9684,14 @@ static int ipr_alloc_cmd_blks(struct ipr_ioa_cfg 
>> *ioa_cfg)
>> }
>>
>> for (i = 0; i < IPR_NUM_CMD_BLKS; i++) {
>> -   ipr_cmd = dma_pool_alloc(ioa_cfg->ipr_cmd_pool, GFP_KERNEL, 
>> _addr);
>> +   ipr_cmd = dma_pool_zalloc(ioa_cfg->ipr_cmd_pool,
>> +   GFP_KERNEL, _addr);
>>
>> if (!ipr_cmd) {
>> ipr_free_cmd_blks(ioa_cfg);
>> return -ENOMEM;
>> }
>>
>> -   memset(ipr_cmd, 0, sizeof(*ipr_cmd));
>> ioa_cfg->ipr_cmnd_list[i] = ipr_cmd;
>> ioa_cfg->ipr_cmnd_list_dma[i] = dma_addr;
>>
>> --
>> 1.9.1
>>
> 

Thanks!

Acked-by: Brian King 

-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [PATCH V3 1/8] scsi: hpsa: fix selection of reply queue

[Ming Lei]

On Tue, Mar 06, 2018 at 02:24:25PM -0500, Martin K. Petersen wrote:
> 
> Ming,
> 
> > Given both Don and Laurence have verified that patch 1 and patch 2
> > does fix IO hang, could you consider to merge the two first?
> 
> Oh, and I would still need a formal Acked-by: from Don and Tested-by:
> from Laurence.
> 
> Also, for 4.16/scsi-fixes I would prefer verification to be done with
> just patch 1/8 and none of the subsequent changes in place. Just to make
> sure we're testing the right thing.

Hi Martin,

Please consider 2/8 too since it is still a fix.

Thanks,
Ming

Re: [PATCH V2] scsi: qla2xxx: Fix crashes in qla2x00_probe_one on probe failure

[Martin K. Petersen]

Bill,

> Because of the shifting around of code in qla2x00_probe_one recently,
> failures during adapter initialization can lead to problems, i.e. NULL
> pointer crashes and doubly freed data structures which cause eventual
> panics.
>
> This V2 version makes the relevant memory free routines idempotent, so
> repeat calls won't cause any harm. I also removed the problematic
> probe_init_failed exit point as it is not needed.

Applied to 4.16/scsi-fixes. Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

[GIT PULL] SCSI fixes for 4.16-rc4

[James Bottomley]

This patch is mostly fixes for driver specific issues (nine of them)
and the storvsc performance improvement with interrupt handling which
was dropped from the previous fixes pull request.  We also have two
regressions: one is a double call_rcu() in ATA error handling and the
other is a missed conversion to BLK_STS_OK in
__scsi_error_from_host_byte().

The patch is available here:

git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git scsi-fixes

The short changelog is:

Bart Van Assche (1):
  scsi: core: Avoid that ATA error handling can trigger a kernel hang or 
oops

Darren Trapp (1):
  scsi: qla2xxx: Fix FC-NVMe LUN discovery

Hannes Reinecke (4):
  scsi: core: return BLK_STS_OK for DID_OK in __scsi_error_from_host_byte()
  scsi: qla2xxx: ensure async flags are reset correctly
  scsi: qla2xxx: do not check login_state if no loop id is assigned
  scsi: qla2xxx: Fixup locking for session deletion

Manish Rangankar (1):
  scsi: qedi: Fix kernel crash during port toggle

Mauricio Faria de Oliveira (1):
  scsi: mpt3sas: fix oops in error handlers after shutdown/unload

Michael Kelley (EOSG) (1):
  scsi: storvsc: Spread interrupts when picking a channel for I/O requests

Shivasharan S (1):
  scsi: megaraid_sas: Do not use 32-bit atomic request descriptor for 
Ventura controllers

Sreekanth Reddy (1):
  scsi: mpt3sas: wait for and flush running commands on shutdown/unload

himanshu.madh...@cavium.com (1):
  scsi: qla2xxx: Fix NULL pointer crash due to active timer for ABTS

And the diffstat:

 drivers/scsi/hosts.c|  3 --
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 42 
 drivers/scsi/mpt3sas/mpt3sas_base.c |  8 ++---
 drivers/scsi/mpt3sas/mpt3sas_base.h |  3 ++
 drivers/scsi/mpt3sas/mpt3sas_scsih.c| 21 +---
 drivers/scsi/qedi/qedi_fw.c |  5 +++
 drivers/scsi/qla2xxx/qla_def.h  |  5 +--
 drivers/scsi/qla2xxx/qla_gs.c   |  5 +++
 drivers/scsi/qla2xxx/qla_init.c | 51 -
 drivers/scsi/qla2xxx/qla_os.c   | 14 ++--
 drivers/scsi/qla2xxx/qla_target.c   | 17 --
 drivers/scsi/scsi_error.c   |  5 +--
 drivers/scsi/scsi_lib.c |  4 +++
 drivers/scsi/storvsc_drv.c  |  3 +-
 include/scsi/scsi_cmnd.h|  3 ++
 include/scsi/scsi_host.h|  2 --
 16 files changed, 115 insertions(+), 76 deletions(-)

With full diff below.

James

---

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 57bf43e34863..dd9464920456 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -328,8 +328,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/megaraid/megaraid_sas_fusion.c 
b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index 073ced07e662..dc8e850fbfd2 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -216,36 +216,30 @@ inline void megasas_return_cmd_fusion(struct 
megasas_instance *instance,
 /**
  * megasas_fire_cmd_fusion -   Sends command to the FW
  * @instance:  Adapter soft state
- * @req_desc:  32bit or 64bit Request descriptor
+ * @req_desc:  64bit Request descriptor
  *
- * Perform PCI Write. Ventura supports 32 bit Descriptor.
- * Prior to Ventura (12G) MR controller supports 64 bit Descriptor.
+ * Perform PCI Write.
  */
 
 static void
 megasas_fire_cmd_fusion(struct megasas_instance *instance,
union MEGASAS_REQUEST_DESCRIPTOR_UNION *req_desc)
 {
-   if (instance->adapter_type == VENTURA_SERIES)
-   writel(le32_to_cpu(req_desc->u.low),
-   >reg_set->inbound_single_queue_port);
-   else {
 #if defined(writeq) && defined(CONFIG_64BIT)
-   u64 req_data = (((u64)le32_to_cpu(req_desc->u.high) << 32) |
-   le32_to_cpu(req_desc->u.low));
+   u64 req_data = (((u64)le32_to_cpu(req_desc->u.high) << 32) |
+   le32_to_cpu(req_desc->u.low));
 
-   writeq(req_data, >reg_set->inbound_low_queue_port);
+   writeq(req_data, >reg_set->inbound_low_queue_port);
 #else
-   unsigned long flags;
-   spin_lock_irqsave(>hba_lock, flags);
-

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Finn Thain]

On Wed, 7 Mar 2018, Michael Schmitz wrote:

> The major obstacle now seems to be dynamic allocation of the driver 
> private data and storing a pointer to that in a way that it can be 
> retrieved using just the esp pointer. dev_set_drvdata(esp->dev, zep) 
> causes the module load to crash ...

I've just noticed that most esp drivers do this:

static int esp_foo_probe(struct platform_device *dev)
{
...
esp->dev = dev;
...
}

But the esp->dev->dev dereferencing sometimes gets overlooked, resulting 
in a pointer to a struct platform_device being used where a pointer to a 
struct device should be used (i.e. dma_*() calls). I will look into fixing 
this up. sun_esp.c doesn't have this problem, but the other drivers do.

I don't think any of that applies to your zorro_esp code because the 
version you sent does this,

esp->dev = >dev;

which seems fine to me. But it could end up more convenient to use the 
sun_esp approach and set esp->dev = z.

I suspect that the problem with zorro_esp is that sometimes you use the 
esp->dev->driver_data pointer as the struct Scsi_Host pointer, and at 
other times you use it for the struct zorro_driver_data pointer. (I think 
I see now why you put the esp pointer in struct zorro_driver_data.)

If you like, email the current version to me or push it to a repo 
somewhere and I'll take a look at it.

-- 

Re: [PATCH] smartpqi: add in new supported controllers

[Martin K. Petersen]

Don,

Applied to 4.17/scsi-queue. Thank you!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] scsi: aic94xx: Use dma_pool_zalloc()

[Souptick Joarder]

Any comment for this patch.

On Thu, Feb 15, 2018 at 10:09 PM, Souptick Joarder  wrote:
> Use dma_pool_zalloc() instead of dma_pool_alloc + memset
>
> Signed-off-by: Souptick Joarder 
> ---
>  drivers/scsi/aic94xx/aic94xx_hwi.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/aic94xx/aic94xx_hwi.c 
> b/drivers/scsi/aic94xx/aic94xx_hwi.c
> index 2dbc833..1b13133 100644
> --- a/drivers/scsi/aic94xx/aic94xx_hwi.c
> +++ b/drivers/scsi/aic94xx/aic94xx_hwi.c
> @@ -1055,14 +1055,13 @@ static struct asd_ascb *asd_ascb_alloc(struct 
> asd_ha_struct *asd_ha,
>
> if (ascb) {
> ascb->dma_scb.size = sizeof(struct scb);
> -   ascb->dma_scb.vaddr = dma_pool_alloc(asd_ha->scb_pool,
> +   ascb->dma_scb.vaddr = dma_pool_zalloc(asd_ha->scb_pool,
>  gfp_flags,
> 
> >dma_scb.dma_handle);
> if (!ascb->dma_scb.vaddr) {
> kmem_cache_free(asd_ascb_cache, ascb);
> return NULL;
> }
> -   memset(ascb->dma_scb.vaddr, 0, sizeof(struct scb));
> asd_init_ascb(asd_ha, ascb);
>
> spin_lock_irqsave(>tc_index_lock, flags);
> --
> 1.9.1
>

Re: [PATCH v3 0/2] scsi: mpt3sas: prevent oops in the shutdown/unload path

[Martin K. Petersen]

Mauricio Faria,

> Are you OK with / would ack this patchset for stable (v4.14+) ?
>
> I have the backports ready and can submit if you are OK with it.

I am OK.

Sreekanth?

-- 
Martin K. Petersen  Oracle Linux Engineering

[PATCH v2] scsi: sd: Keep disk read-only when re-reading partition

[Jeremy Cline]

If the read-only flag is true on a SCSI disk, re-reading the partition
table sets the flag back to false.

To observe this bug, you can run:

1. blockdev --setro /dev/sda
2. blockdev --rereadpt /dev/sda
3. blockdev --getro /dev/sda

This commit reads the disk's old state and combines it with the device
disk-reported state rather than unconditionally marking it as RW.

Reported-by: Li Ning 
Signed-off-by: Jeremy Cline 
---
Sorry about this, but there's a bug in the first version of this patch.
I'm not sure what the protocol is for sending revised patches when the
earlier version got accepted, but I don't see the first version in
4.16/scsi-fixes yet. If I should make a patch on top of the v1 patch,
just let me know.

Changes in v2:
  - Don't save the value of "get_disk_ro" to "sdkp->write_prot" since,
as Li Ning noticed, that causes a second bug: when you make the disk
writable again with "blockdev --setrw /dev/sda" it isn't possible to
write to the disk until the partition is re-read.

 drivers/scsi/sd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index bff21e636ddd..3541caf3fceb 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2595,6 +2595,7 @@ sd_read_write_protect_flag(struct scsi_disk *sdkp, 
unsigned char *buffer)
int res;
struct scsi_device *sdp = sdkp->device;
struct scsi_mode_data data;
+   int disk_ro = get_disk_ro(sdkp->disk);
int old_wp = sdkp->write_prot;
 
set_disk_ro(sdkp->disk, 0);
@@ -2635,7 +2636,7 @@ sd_read_write_protect_flag(struct scsi_disk *sdkp, 
unsigned char *buffer)
  "Test WP failed, assume Write Enabled\n");
} else {
sdkp->write_prot = ((data.device_specific & 0x80) != 0);
-   set_disk_ro(sdkp->disk, sdkp->write_prot);
+   set_disk_ro(sdkp->disk, sdkp->write_prot || disk_ro);
if (sdkp->first_scan || old_wp != sdkp->write_prot) {
sd_printk(KERN_NOTICE, sdkp, "Write Protect is %s\n",
  sdkp->write_prot ? "on" : "off");
-- 
2.16.2

Re: [PATCH] scsi: ipr: Use dma_pool_zalloc()

[Martin K. Petersen]

Brian,

> Thanks!
>
> Acked-by: Brian King 

Not sure where this patch was sent but it's not in my mailbox, nor the
list archives.

Souptick: Please resubmit to linux-scsi with Brian's Acked-by.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v2 0/2] scsi_io_completion cleanup and fix CONDITION MET handling

[Martin K. Petersen]

Doug,

> Since this patchset seems to have frightened away reviewers, I'll try
> again.

Yeah, I tried and it was a bit too big to digest (for such a critical
piece of code).

> Now I'll send a single patch to do the fix for CONDITION MET. That
> will be followed by a 5 part patchset cleaning up the
> scsi_io_completion() function.

Sounds good! Please make sure the patch descriptions are in each patch
and not in the cover letter.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] scsi: megaraid: Use dma_pool_zalloc()

[Martin K. Petersen]

Souptick,

> Use dma_pool_zalloc() instead of dma_pool_alloc + memset

Applied to 4.17/scsi-queue.

-- 
Martin K. Petersen  Oracle Linux Engineering

[PATCH 4/5] scsi_io_completion: conditional hints on fastpath

[Douglas Gilbert]

ChangeLog:
  - add likely() and unlikely() hints to conditionals on or near the
fastpath
  - fix a comment in scsi_io_completion_action()

Signed-off-by: Douglas Gilbert 
---
 drivers/scsi/scsi_lib.c | 26 --
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8d2d12a69508..23e59281bd02 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -818,7 +818,7 @@ static void scsi_io_completion_action(struct scsi_cmnd 
*cmd, int result)
unsigned long wait_for = (cmd->allowed + 1) * req->timeout;
struct scsi_sense_hdr sshdr;
bool sense_valid_and_current = false;
-   blk_status_t blk_stat;  /* enum, BLK_STS_OK is 0 */
+   blk_status_t blk_stat;  /* u8, BLK_STS_OK is only 0 */
 
/* sense not about current command is termed: deferred */
if (scsi_command_normalize_sense(cmd, ) &&
@@ -1009,7 +1009,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
int good_bytes)
struct request *req = cmd->request;
blk_status_t blk_stat = BLK_STS_OK; /* u8: BLK_STS_OK is only 0 */
 
-   if (result) {
+   if (unlikely(result)) {
blk_stat = scsi_io_completion_nz_result(cmd, result);
if (blk_stat == BLK_STS_OK)
result = 0;
@@ -1017,14 +1017,14 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
int good_bytes)
blk_stat = BLK_STS_OK;
}
 
-   if (blk_rq_is_passthrough(req)) {
+   if (unlikely(blk_rq_is_passthrough(req))) {
/*
 * __scsi_error_from_host_byte may have reset the host_byte
 */
scsi_req(req)->result = cmd->result;
scsi_req(req)->resid_len = scsi_get_resid(cmd);
 
-   if (scsi_bidi_cmnd(cmd)) {
+   if (unlikely(scsi_bidi_cmnd(cmd))) {
/*
 * Bidi commands Must be complete as a whole,
 * both sides at once.
@@ -1037,7 +1037,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
int good_bytes)
}
}
 
-   /* no bidi support for !blk_rq_is_passthrough yet */
+   /* no bidi support yet, other than in pass-through */
BUG_ON(blk_bidi_rq(req));
 
SCSI_LOG_HLCOMPLETE(1, scmd_printk(KERN_INFO, cmd,
@@ -1049,15 +1049,13 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
int good_bytes)
 * handle. Failed, zero length commands always need to drop down
 * to retry code. Fast path should return in this block.
 */
-   if (blk_rq_bytes(req) > 0 || blk_stat == BLK_STS_OK) {
-   if (!scsi_end_request(req, blk_stat, good_bytes, 0))
-   return; /* no bytes remaining */
+   if (likely(blk_rq_bytes(req) > 0 || blk_stat == BLK_STS_OK)) {
+   if (likely(!scsi_end_request(req, blk_stat, good_bytes, 0)))
+   return; /* no bytes remaining */
}
 
-   /*
-* Kill remainder if no retrys.
-*/
-   if (blk_stat && scsi_noretry_cmd(cmd)) {
+   /* Kill remainder if no retrys  */
+   if (unlikely(blk_stat && scsi_noretry_cmd(cmd))) {
if (scsi_end_request(req, blk_stat, blk_rq_bytes(req), 0))
BUG();
return;
@@ -1067,14 +1065,14 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
int good_bytes)
 * If there had been no error, but we have leftover bytes in the
 * requeues just queue the command up again.
 */
-   if (result == 0) {
+   if (likely(result == 0)) {
/*
 * Unprep the request and put it back at the head of the
 * queue. A new command will be prepared and issued.
 * This block is the same as case ACTION_REPREP in
 * scsi_io_completion_action() above.
 */
-   if (q->mq_ops)
+   if (likely(q->mq_ops))
scsi_mq_requeue_cmd(cmd);
else {
scsi_release_buffers(cmd);
-- 
2.14.1

[PATCH 5/5] scsi_io_completion: convert BUGs to WARNs

[Douglas Gilbert]

ChangeLog:
  - replace BUG() and BUG_ON() with WARN variants.
  - try to send sensible reports (without flooding the log)
and continuations that won't make things worse

Signed-off-by: Douglas Gilbert 
---
 drivers/scsi/scsi_lib.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 23e59281bd02..98e80b8a14f4 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1032,13 +1032,20 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
int good_bytes)
scsi_req(req->next_rq)->resid_len = scsi_in(cmd)->resid;
if (scsi_end_request(req, BLK_STS_OK, blk_rq_bytes(req),
blk_rq_bytes(req->next_rq)))
-   BUG();
+   WARN(true, "Bidi command with remaining bytes");
return;
}
}
 
/* no bidi support yet, other than in pass-through */
-   BUG_ON(blk_bidi_rq(req));
+   if (unlikely(blk_bidi_rq(req))) {
+   WARN_ONCE(true, "Only support bidi command in passthrough");
+   scmd_printk(KERN_ERR, cmd, "Killing bidi command\n");
+   if (scsi_end_request(req, BLK_STS_IOERR, blk_rq_bytes(req),
+blk_rq_bytes(req->next_rq)))
+   WARN(true, "Bidi command with remaining bytes");
+   return;
+   }
 
SCSI_LOG_HLCOMPLETE(1, scmd_printk(KERN_INFO, cmd,
"%u sectors total, %d bytes done.\n",
@@ -1057,7 +1064,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
int good_bytes)
/* Kill remainder if no retrys  */
if (unlikely(blk_stat && scsi_noretry_cmd(cmd))) {
if (scsi_end_request(req, blk_stat, blk_rq_bytes(req), 0))
-   BUG();
+   WARN_ONCE(true,
+   "Bytes remaining after failed, no-retry command");
return;
}
 
-- 
2.14.1

Re: [PATCH v2 06/38] cxlflash: Read host function configuration

[Andrew Donnellan]

On 27/02/18 09:20, Uma Krishnan wrote:

Per the OCXL specification, the underlying host can have multiple AFUs
per function with each function supporting its own configuration. The host
function configuration is read on the initialization path to evaluate the
number of functions present and identify the features and configuration of
the functions present. This data is cached for use in later configuration
steps. Note that for the OCXL hardware supported by the cxlflash driver,
only one AFU per function is expected.

Signed-off-by: Uma Krishnan 
Acked-by: Matthew R. Ochs 


Nitpick below

Reviewed-by: Andrew Donnellan 


---
  drivers/scsi/cxlflash/ocxl_hw.c | 41 +
  drivers/scsi/cxlflash/ocxl_hw.h |  2 ++
  2 files changed, 43 insertions(+)

diff --git a/drivers/scsi/cxlflash/ocxl_hw.c b/drivers/scsi/cxlflash/ocxl_hw.c
index e3a0a9b..dc32a73 100644
--- a/drivers/scsi/cxlflash/ocxl_hw.c
+++ b/drivers/scsi/cxlflash/ocxl_hw.c
@@ -32,6 +32,35 @@ static void ocxlflash_destroy_afu(void *afu_cookie)
  }

  /**
+ * ocxlflash_config_fn() - configure the host function
+ * @pdev:  PCI device associated with the host.
+ * @afu:   AFU associated with the host.
+ *
+ * Return: 0 on success, -errno on failure
+ */
+static int ocxlflash_config_fn(struct pci_dev *pdev, struct ocxl_hw_afu *afu)
+{
+   struct ocxl_fn_config *fcfg = >fcfg;
+   struct device *dev = >dev;
+   int rc = 0;
+
+   /* Read DVSEC config of the function */
+   rc = ocxl_config_read_function(pdev, fcfg);
+   if (unlikely(rc)) {
+   dev_err(dev, "%s: ocxl_config_read_function failed rc=%d\n",
+   __func__, rc);
+   goto out;
+   }
+
+   /* Only one AFU per function is supported by ocxlflash */
+   if (fcfg->max_afu_index != 0)
+   dev_warn(dev, "%s: Unexpected AFU index value %d\n",
+__func__, fcfg->max_afu_index);
+out:
+   return rc;
+}
+
+/**
   * ocxlflash_create_afu() - create the AFU for OCXL
   * @pdev: PCI device associated with the host.
   *
@@ -41,6 +70,7 @@ static void *ocxlflash_create_afu(struct pci_dev *pdev)
  {
struct device *dev = >dev;
struct ocxl_hw_afu *afu;
+   int rc;

afu = kzalloc(sizeof(*afu), GFP_KERNEL);
if (unlikely(!afu)) {
@@ -50,8 +80,19 @@ static void *ocxlflash_create_afu(struct pci_dev *pdev)

afu->pdev = pdev;
afu->dev = dev;
+
+   rc = ocxlflash_config_fn(pdev, afu);
+   if (unlikely(rc)) {
+   dev_err(dev, "%s: Function configuration failed rc=%d\n",
+   __func__, rc);
+   goto err1;
+   }
  out:
return afu;
+err1:
+   kfree(afu);
+   afu = NULL;
+   goto out;


I think it would be cleaner to just write return NULL here


  }

  /* Backend ops to ocxlflash services */
diff --git a/drivers/scsi/cxlflash/ocxl_hw.h b/drivers/scsi/cxlflash/ocxl_hw.h
index c7e5c4d..658f420 100644
--- a/drivers/scsi/cxlflash/ocxl_hw.h
+++ b/drivers/scsi/cxlflash/ocxl_hw.h
@@ -16,4 +16,6 @@
  struct ocxl_hw_afu {
struct pci_dev *pdev;   /* PCI device */
struct device *dev; /* Generic device */
+
+   struct ocxl_fn_config fcfg; /* DVSEC config of the function */
  };



--
Andrew Donnellan  OzLabs, ADL Canberra
andrew.donnel...@au1.ibm.com  IBM Australia Limited

Re: [PATCH V4] scsi: core: use blk_mq_requeue_request in __scsi_queue_insert

[Martin K. Petersen]

Jianchao,

> In scsi core, __scsi_queue_insert should just put request back on
> the queue and retry using the same command as before. However, for
> blk-mq, scsi_mq_requeue_cmd is employed here which will unprepare
> the request. To align with the semantics of __scsi_queue_insert,
> use blk_mq_requeue_request with kick_requeue_list == true and put
> the reference of scsi_device.

Applied to 4.17/scsi-queue, thank you!

-- 
Martin K. Petersen  Oracle Linux Engineering

RE: [PATCH v3 0/2] scsi: mpt3sas: prevent oops in the shutdown/unload path

[Sreekanth Reddy]

-Original Message-
From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
Sent: Wednesday, March 7, 2018 6:48 AM
To: Mauricio Faria de Oliveira
Cc: Martin K. Petersen; j...@linux.vnet.ibm.com;
linux-scsi@vger.kernel.org; bart.vanass...@wdc.com;
sreekanth.re...@broadcom.com; dougm...@linux.vnet.ibm.com
Subject: Re: [PATCH v3 0/2] scsi: mpt3sas: prevent oops in the
shutdown/unload path


Mauricio Faria,

> Are you OK with / would ack this patchset for stable (v4.14+) ?
>
> I have the backports ready and can submit if you are OK with it.

I am OK.

Sreekanth?

-- 
Martin K. Petersen  Oracle Linux Engineering

I am also OK for it.

Thanks,
Sreekant

Re: [PATCH] Improve ZBC/ZAC error handling

[Damien Le Moal]

On 2018/03/07 3:49, Martin K. Petersen wrote:
> 
> Tejun,
> 
>> Except for the nit on the last patch, ata part looks good to me.
>> Martin, how do you wanna route the SCSI part?
> 
> I want to route it to /dev/null on the grounds of being a BLATANT
> LAYERING VIOLATION (cue dramatic sound effect).

Got it... Will add some more rework to v2.

> scsi_error.c is SPC territory, we really shouldn't wedge any ZBC/SBC
> stuff in there. Nor should we call into this file from libata. If
> there's a ZAC/ZBC SAT retry deficiency, let's address that instead of
> working around it.

Understood. sd_zbc already handles the retry checks for scsi side, and almost
exactly the same code is necessary from libata (since retry tests are based on
sense asc/ascq and not on ATA status bits). So is it OK to export a function
from sd_zbc.c to call from libata ? Replicating the code is of course trivial
but rather dirty.

Best regards.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH V3 8/8] scsi: megaraid: improve scsi_mq performance via .host_tagset

[Ming Lei]

On Wed, Feb 28, 2018 at 08:28:48PM +0530, Kashyap Desai wrote:
> Ming -
> 
> Quick testing on my setup -  Performance slightly degraded (4-5% drop)for
> megaraid_sas driver with this patch. (From 1610K IOPS it goes to 1544K)
> I confirm that after applying this patch, we have #queue = #numa node.
> 
> ls -l
> /sys/devices/pci:80/:80:02.0/:83:00.0/host10/target10:2:23/10:
> 2:23:0/block/sdy/mq
> total 0
> drwxr-xr-x. 18 root root 0 Feb 28 09:53 0
> drwxr-xr-x. 18 root root 0 Feb 28 09:53 1
> 
> 
> I would suggest to skip megaraid_sas driver changes using shared_tagset
> until and unless there is obvious gain. If overall interface of using
> shared_tagset is commit in kernel tree, we will investigate (megaraid_sas
> driver) in future about real benefit of using it.

Hi Kashyap,

Now I have put patches for removing operating on scsi_host->host_busy
in V4[1], especially which are done in the following 3 patches:

9221638b9bc9 scsi: avoid to hold host_busy for scsi_mq
1ffc8c0ffbe4 scsi: read host_busy via scsi_host_busy()
e453d3983243 scsi: introduce scsi_host_busy()


Could you run your test on V4 and see if IOPS can be improved on
megaraid_sas?


[1] https://github.com/ming1/linux/commits/v4.16-rc-host-tags-v4

Thanks,
Ming

[PATCH] scsi: jazz_esp, sun3x_esp: Pass struct device pointer in dma calls

[Finn Thain]

In jazz_esp and sun3x_esp, the esp_driver_ops methods pass esp->dev
in dma api calls as if it was a pointer to a struct device. But
it actually points to a struct platform_device. Fix this.

Cc: Thomas Bogendoerfer 
Signed-off-by: Finn Thain 
---
 drivers/scsi/jazz_esp.c  | 2 +-
 drivers/scsi/sun3x_esp.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/jazz_esp.c b/drivers/scsi/jazz_esp.c
index 9aaa74e349cc..6eb5ff3e2e61 100644
--- a/drivers/scsi/jazz_esp.c
+++ b/drivers/scsi/jazz_esp.c
@@ -147,7 +147,7 @@ static int esp_jazz_probe(struct platform_device *dev)
esp = shost_priv(host);
 
esp->host = host;
-   esp->dev = dev;
+   esp->dev = >dev;
esp->ops = _esp_ops;
 
res = platform_get_resource(dev, IORESOURCE_MEM, 0);
diff --git a/drivers/scsi/sun3x_esp.c b/drivers/scsi/sun3x_esp.c
index d50c5ed8f428..0b1421cdf8a0 100644
--- a/drivers/scsi/sun3x_esp.c
+++ b/drivers/scsi/sun3x_esp.c
@@ -210,7 +210,7 @@ static int esp_sun3x_probe(struct platform_device *dev)
esp = shost_priv(host);
 
esp->host = host;
-   esp->dev = dev;
+   esp->dev = >dev;
esp->ops = _esp_ops;
 
res = platform_get_resource(dev, IORESOURCE_MEM, 0);
-- 
2.16.1

Re: [PATCH] scsi: libsas: defer ata device eh commands to libata

[Jason Yan]

On 2018/3/7 2:29, Dan Williams wrote:

On Tue, Mar 6, 2018 at 10:04 AM, Martin K. Petersen
 wrote:



When ata device doing EH, some commands still attached with tasks are not
passed to libata when abort failed or recover failed, so libata did not
handle these commands. After these commands done, sas task is freed, but
ata qc is not freed.


Somebody from the libsas camp, please review!

 https://patchwork.kernel.org/patch/10244375/


It's been a while since I've had access to libsas hardware, but this
patch looks like a good idea to me. However, it seems that it could be
narrowed to just the sas_eh_finish_cmd() that need to be converted to
sas_eh_defer_cmd(). Otherwise, if there are no more direct usages of
sas_eh_finish_cmd() then we should consider just renaming
sas_eh_defer_cmd() to sas_eh_finish_cmd() and delete the old
sas_eh_finish_cmd(), i.e. remove the distinction.



Delete the old sas_eh_finish_cmd() seems better. Thanks for the
suggestion. I will respin this patch.


.

Re: [PATCH v2] lpfc: use __raw_writeX on DPP copies

[Martin K. Petersen]

James,

Minor patch formatting nits:

> This patch replaces:
> https://www.spinics.net/lists/linux-scsi/msg117838.html

Patch revision commentary needs to go below the "---" delimiter so it
doesn't end up in the commit description.

> A prior lpfc patch:
> scsi: lpfc: Add push-to-adapter support to sli4
> commitid=1351e69fc6db30e186295f1c9495d03cef6a01a2

This should be formatted as:

Commit 1351e69fc6db ("scsi: lpfc: Add push-to-adapter support to sli4")
fails compilation...

And the following should be added to the tags section:

Fixes: 1351e69fc6db ("scsi: lpfc: Add push-to-adapter support to sli4")


I fixed things up and applied to 4.17/scsi-queue.

Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v2 0/2] scsi_io_completion cleanup and fix CONDITION MET handling

[Douglas Gilbert]

Since this patchset seems to have frightened away reviewers, I'll
try again.

Now I'll send a single patch to do the fix for CONDITION MET.
That will be followed by a 5 part patchset cleaning up the
scsi_io_completion() function.

Douglas Gilbert


On 2018-02-28 01:23 AM, Douglas Gilbert wrote:

This patch started as an attempt to fix the erroneous handling of
CONDITION MET, a relatively rare special case. A solution meant adding
another special case to the already complicated scsi_io_completion()
function. To better understand that function the author found it
useful to refactor the function into one relatively short function
that the fast path passes through and two helper functions. These
helper functions do the bulk of the error and special case handling.

The SCSI PRE-FETCH (10 or 16) command is present both on hard disks
and some SSDs. It is useful when the address of the next block(s) to
be read is known but it is not following the LBA of the current READ
(so read-ahead won't help). It returns two "good" SCSI Status values.
If the requested blocks have fitted (or will most likely fit (when
the IMMED bit is set)) into the disk's cache, it returns CONDITION
MET. If it didn't (or will not) fit then it returns GOOD status.

Future work: if the sd driver was to use the PRE-FETCH command,
decide whether it and/or the block layer needs to know about the
two different "good" statuses. If so a mechanism would be needed
to do that.

ChangeLog for v2:
   - after checkpatch.pl warning about BUG statements and confirmed
 by a reviewer, convert 3 BUG calls in scsi_io_completion()
 into WARN calls

ChangeLog for v1:
   - split scsi_io_completion() into one short function that the
 fast path uses, and two helper functions
   - add conditional hints on the fast path
   - rename some variables to make the code a little clearer
   - add scsi_io_completion_nz_result() helper for the initial
 handling of the non-zero cmd->result case
   - add scsi_io_completion_action() helper for the 'action'
 processing of errors and special cases
   - expand inline function scsi_status_is_good() to check for
 CONDITION MET
   - add a corner case in scsi_io_completion_nz_result() for
 CONDITION MET (and similar "good" SCSI statuses)
   - structure code so extra checks are only on the error
 path (i.e. when cmd->result is non-zero)

This patch is against mkp's 4.17/scsi-queue branch. It also
  applies to lk 4.15.x where it was tested on a SAS SSD.


Douglas Gilbert (2):
   scsi_io_completion cleanup and fix CONDITION MET handling
   scsi_io_completion-change2warn

  drivers/scsi/scsi_lib.c | 304 +---
  include/scsi/scsi.h |   2 +
  2 files changed, 188 insertions(+), 118 deletions(-)

Re: [PATCH v2 05/38] cxlflash: Hardware AFU for OCXL

[Andrew Donnellan]

On 27/02/18 09:20, Uma Krishnan wrote:

When an adapter is initialized, transport specific configuration and MMIO
mapping details need to be saved. For CXL, this data is managed by the
underlying kernel module. To maintain a separation between the cxlflash
core and underlying transports, introduce a new structure to store data
specific to the OCXL AFU.

Initially only the pointers to underlying PCI and generic devices are
added to this new structure - it will be expanded further in future
commits. Services to create and destroy this hardware AFU are added and
integrated in the probe and exit paths of the driver.

Signed-off-by: Uma Krishnan 
Acked-by: Matthew R. Ochs 


One comment below

Reviewed-by: Andrew Donnellan 


---
  drivers/scsi/cxlflash/backend.h |  1 +
  drivers/scsi/cxlflash/cxl_hw.c  |  6 ++
  drivers/scsi/cxlflash/main.c|  9 +++--
  drivers/scsi/cxlflash/ocxl_hw.c | 40 
  drivers/scsi/cxlflash/ocxl_hw.h | 19 +++
  5 files changed, 73 insertions(+), 2 deletions(-)
  create mode 100644 drivers/scsi/cxlflash/ocxl_hw.h

diff --git a/drivers/scsi/cxlflash/backend.h b/drivers/scsi/cxlflash/backend.h
index a60f051..f675bcb 100644
--- a/drivers/scsi/cxlflash/backend.h
+++ b/drivers/scsi/cxlflash/backend.h
@@ -36,6 +36,7 @@ struct cxlflash_backend_ops {
int (*allocate_afu_irqs)(void *ctx_cookie, int num);
void (*free_afu_irqs)(void *ctx_cookie);
void * (*create_afu)(struct pci_dev *dev);
+   void (*destroy_afu)(void *afu_cookie);
struct file * (*get_fd)(void *ctx_cookie, struct file_operations *fops,
int *fd);
void * (*fops_get_context)(struct file *file);
diff --git a/drivers/scsi/cxlflash/cxl_hw.c b/drivers/scsi/cxlflash/cxl_hw.c
index db1cada..a1d6d12 100644
--- a/drivers/scsi/cxlflash/cxl_hw.c
+++ b/drivers/scsi/cxlflash/cxl_hw.c
@@ -110,6 +110,11 @@ static void *cxlflash_create_afu(struct pci_dev *dev)
return cxl_pci_to_afu(dev);
  }

+static void cxlflash_destroy_afu(void *afu)
+{
+   /* Dummy fop for cxl */
+}
+


For ops structs I think it's more common to set the function pointer to 
NULL when not implemented and do a NULL check at the call site.



  static struct file *cxlflash_get_fd(void *ctx_cookie,
struct file_operations *fops, int *fd)
  {
@@ -160,6 +165,7 @@ const struct cxlflash_backend_ops cxlflash_cxl_ops = {
.allocate_afu_irqs  = cxlflash_allocate_afu_irqs,
.free_afu_irqs  = cxlflash_free_afu_irqs,
.create_afu = cxlflash_create_afu,
+   .destroy_afu= cxlflash_destroy_afu,
.get_fd = cxlflash_get_fd,
.fops_get_context   = cxlflash_fops_get_context,
.start_work = cxlflash_start_work,
diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index b83a55a..5d754d1 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -971,6 +971,7 @@ static void cxlflash_remove(struct pci_dev *pdev)
case INIT_STATE_AFU:
term_afu(cfg);
case INIT_STATE_PCI:
+   cfg->ops->destroy_afu(cfg->afu_cookie);
pci_disable_device(pdev);
case INIT_STATE_NONE:
free_mem(cfg);
@@ -3689,8 +3690,6 @@ static int cxlflash_probe(struct pci_dev *pdev,

pci_set_drvdata(pdev, cfg);

-   cfg->afu_cookie = cfg->ops->create_afu(pdev);
-
rc = init_pci(cfg);
if (rc) {
dev_err(dev, "%s: init_pci failed rc=%d\n", __func__, rc);
@@ -3698,6 +3697,12 @@ static int cxlflash_probe(struct pci_dev *pdev,
}
cfg->init_state = INIT_STATE_PCI;

+   cfg->afu_cookie = cfg->ops->create_afu(pdev);
+   if (unlikely(!cfg->afu_cookie)) {
+   dev_err(dev, "%s: create_afu failed\n", __func__);
+   goto out_remove;
+   }
+
rc = init_afu(cfg);
if (rc && !wq_has_sleeper(>reset_waitq)) {
dev_err(dev, "%s: init_afu failed rc=%d\n", __func__, rc);
diff --git a/drivers/scsi/cxlflash/ocxl_hw.c b/drivers/scsi/cxlflash/ocxl_hw.c
index 58a3182..e3a0a9b 100644
--- a/drivers/scsi/cxlflash/ocxl_hw.c
+++ b/drivers/scsi/cxlflash/ocxl_hw.c
@@ -15,8 +15,48 @@
  #include 

  #include "backend.h"
+#include "ocxl_hw.h"
+
+/**
+ * ocxlflash_destroy_afu() - destroy the AFU structure
+ * @afu_cookie:AFU to be freed.
+ */
+static void ocxlflash_destroy_afu(void *afu_cookie)
+{
+   struct ocxl_hw_afu *afu = afu_cookie;
+
+   if (!afu)
+   return; > +
+   kfree(afu);
+}
+
+/**
+ * ocxlflash_create_afu() - create the AFU for OCXL
+ * @pdev:  PCI device associated with the host.
+ *
+ * Return: AFU on success, NULL on failure
+ */
+static void *ocxlflash_create_afu(struct pci_dev *pdev)
+{
+   struct 

Re: [PATCH v2] scsi: sd: Keep disk read-only when re-reading partition

[Martin K. Petersen]

Jeremy,

> Sorry about this, but there's a bug in the first version of this patch.
> I'm not sure what the protocol is for sending revised patches when the
> earlier version got accepted, but I don't see the first version in
> 4.16/scsi-fixes yet.

It's your lucky day! I botched fixing up something the other day and as
a result the tree was never pushed.

v2 added to 4.16/scsi-fixes.

Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: dmesg flooded with "Very big device. Trying to use READ CAPACITY(16)"

[Martin K. Petersen]

Menion,

> Operating big capacity HDD such 8TB with complex filesystems like
> BTRFS in RAID mode endup in dmesg get flooded by this log, due too
> many capacity checks (opaque to the filesystem itself)

What's your definition of flooded? How many do you see?

Also, what kind of controller are these disks attached to? The reason
you see these messages is that to the kernel it looks like a legacy disk
device that predates capacities in the TB range. The warnings are logged
because we're surprised to be going down this path based on what the
device has previously told us.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v2 04/38] cxlflash: Introduce OCXL backend

[Andrew Donnellan]

On 27/02/18 09:20, Uma Krishnan wrote:

Add initial infrastructure to support a new cxlflash transport, OCXL.

Claim a dependency on OCXL and add a new file, ocxl_hw.c, which will host
the backend routines that are specific to OCXL.

Signed-off-by: Uma Krishnan 
Acked-by: Matthew R. Ochs 


Per my response to the first version of this series, at some point we 
might want to make this not depend on OCXL in all cases, but I don't 
feel strongly on that.


Reviewed-by: Andrew Donnellan 


--
Andrew Donnellan  OzLabs, ADL Canberra
andrew.donnel...@au1.ibm.com  IBM Australia Limited

[PATCH 0/5] scsi_io_completion cleanup

[Douglas Gilbert]

This patchset assumes the presence of "[PATCH v4] Make SCSI Status
CONDITION MET equivalent to GOOD". While working on that patch the
author had trouble following the logic in the scsi_io_completion()
function found in scsi_lib.c . That led to a rework of the
scsi_io_completion() which is now much shorter with the bulk of its
code, error processing, moved into 2 helper functions. The fastpath
(i.e. the path the the vast majority of SCSI commands will take)
remains in the shorter scsi_io_completion() and should be easier to
follow with the added conditional hints.

Finally after warnings from checkpatch.pl and the encouragement of
a reviewer, three BUG calls are converted to WARN calls.
Suggestions for alternative recoveries are welcome.

Douglas Gilbert (5):
  scsi_io_completion: rename variables and comments
  scsi_io_completion: add _nz_result helper
  scsi_io_completion: add _action helper function
  scsi_io_completion: conditional hints on fastpath
  scsi_io_completion: convert BUGs to WARNs

 drivers/scsi/scsi_lib.c | 300 
 1 file changed, 177 insertions(+), 123 deletions(-)

-- 
2.14.1

[PATCH 2/5] scsi_io_completion: add _nz_result helper

[Douglas Gilbert]

ChangeLog:
  - break out several interwined paths when cmd->result is non zero
and place them in scsi_io_completion_nz_result helper function
  - after a review comment that proposed reducing the helper's
former 2 return values to a single return value, use
BLK_STS_NOTSUPP as a marker that the helper did not change
the blk_status. This is cleaned up just after its invocation
in scsi_io_completion.


Signed-off-by: Douglas Gilbert 
---
 drivers/scsi/scsi_lib.c | 125 
 1 file changed, 74 insertions(+), 51 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 92eea43cfa6f..e19531b39394 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -739,6 +739,74 @@ static blk_status_t __scsi_error_from_host_byte(struct 
scsi_cmnd *cmd,
}
 }
 
+/*
+ * Helper for scsi_io_completion() when cmd->result is non-zero. Returns
+ * BLK_STS_NOTSUPP if this function does not change blk_status .
+ */
+static blk_status_t scsi_io_completion_nz_result(struct scsi_cmnd *cmd,
+int result)
+{
+   bool sense_valid;
+   bool about_current;
+   /* __scsi_error_from_host_byte() does not return BLK_STS_NOTSUPP */
+   blk_status_t blk_stat = BLK_STS_NOTSUPP;
+   struct request *req = cmd->request;
+   struct scsi_sense_hdr sshdr;
+
+   sense_valid = scsi_command_normalize_sense(cmd, );
+   about_current = sense_valid ? !scsi_sense_is_deferred() : true;
+
+   if (blk_rq_is_passthrough(req)) {
+   if (sense_valid) {
+   /*
+* SG_IO wants current and deferred errors
+*/
+   scsi_req(req)->sense_len =
+   min(8 + cmd->sense_buffer[7],
+   SCSI_SENSE_BUFFERSIZE);
+   }
+   if (about_current)
+   blk_stat = __scsi_error_from_host_byte(cmd, result);
+   } else if (blk_rq_bytes(req) == 0 && about_current) {
+   /*
+* Flush commands do not transfers any data, and thus cannot use
+* good_bytes != blk_rq_bytes(req) as the signal for an error.
+* This sets blk_stat explicitly for the problem case.
+*/
+   blk_stat = __scsi_error_from_host_byte(cmd, result);
+   }
+   /*
+* Recovered errors need reporting, but they're always treated as
+* success, so fiddle the result code here.  For passthrough requests
+* we already took a copy of the original into sreq->result which
+* is what gets returned to the user
+*/
+   if (sense_valid && (sshdr.sense_key == RECOVERED_ERROR)) {
+   /*
+* 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);
+   /* for passthrough, blk_stat may be set */
+   blk_stat = BLK_STS_OK;
+   }
+   /*
+* Another corner case: the SCSI status byte is non-zero but 'good'.
+* Example: PRE-FETCH command returns SAM_STAT_CONDITION_MET when
+* it is able to fit nominated LBs in its cache (and SAM_STAT_GOOD
+* if it can't fit). Treat SAM_STAT_CONDITION_MET and the related
+* intermediate statuses (both obsolete in SAM-4) as good.
+*/
+   if (status_byte(result) && scsi_status_is_good(result))
+   blk_stat = BLK_STS_OK;
+
+   return blk_stat;
+}
+
 /*
  * Function:scsi_io_completion()
  *
@@ -786,22 +854,15 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
int good_bytes)
sense_valid = scsi_command_normalize_sense(cmd, );
if (sense_valid)
sense_current = !scsi_sense_is_deferred();
+   blk_stat = scsi_io_completion_nz_result(cmd, result);
+   if (blk_stat == BLK_STS_OK)
+   result = 0;
+   if (blk_stat == BLK_STS_NOTSUPP)  /* flagging no change */
+   blk_stat = BLK_STS_OK;
+
}
 
if (blk_rq_is_passthrough(req)) {
-   if (result) {
-   if (sense_valid) {
-   /*
-* SG_IO wants current and deferred errors
-*/
-   scsi_req(req)->sense_len =
-   min(8 + cmd->sense_buffer[7],
-   SCSI_SENSE_BUFFERSIZE);
-   }
-   

[PATCH 1/5] scsi_io_completion: rename variables and comments

[Douglas Gilbert]

ChangeLog:
  - add comments to scsi_end_request() noting what its bool return
value means
  - change some variable names in scsi_io_completion()
  - reword some inline comments; move some comments that seemed to be
separated from the code they referred to

Signed-off-by: Douglas Gilbert 
---
 drivers/scsi/scsi_lib.c | 54 ++---
 1 file changed, 29 insertions(+), 25 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 142400476d84..92eea43cfa6f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -650,6 +650,7 @@ static void scsi_release_bidi_buffers(struct scsi_cmnd *cmd)
cmd->request->next_rq->special = NULL;
 }
 
+/* Returns false when no more bytes to process, true if there are more */
 static bool scsi_end_request(struct request *req, blk_status_t error,
unsigned int bytes, unsigned int bidi_bytes)
 {
@@ -772,10 +773,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
int good_bytes)
int result = cmd->result;
struct request_queue *q = cmd->device->request_queue;
struct request *req = cmd->request;
-   blk_status_t error = BLK_STS_OK;
+   blk_status_t blk_stat = BLK_STS_OK; /* u8: BLK_STS_OK is only 0 */
struct scsi_sense_hdr sshdr;
bool sense_valid = false;
-   int sense_deferred = 0, level = 0;
+   bool sense_current = true;  /* false implies "deferred sense" */
+   int level = 0;
enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
  ACTION_DELAYED_RETRY} action;
unsigned long wait_for = (cmd->allowed + 1) * req->timeout;
@@ -783,7 +785,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int 
good_bytes)
if (result) {
sense_valid = scsi_command_normalize_sense(cmd, );
if (sense_valid)
-   sense_deferred = scsi_sense_is_deferred();
+   sense_current = !scsi_sense_is_deferred();
}
 
if (blk_rq_is_passthrough(req)) {
@@ -796,8 +798,9 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int 
good_bytes)
min(8 + cmd->sense_buffer[7],
SCSI_SENSE_BUFFERSIZE);
}
-   if (!sense_deferred)
-   error = __scsi_error_from_host_byte(cmd, 
result);
+   if (sense_current)
+   blk_stat = __scsi_error_from_host_byte(cmd,
+  result);
}
/*
 * __scsi_error_from_host_byte may have reset the host_byte
@@ -816,13 +819,13 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
int good_bytes)
BUG();
return;
}
-   } else if (blk_rq_bytes(req) == 0 && result && !sense_deferred) {
+   } else if (blk_rq_bytes(req) == 0 && result && sense_current) {
/*
 * Flush commands do not transfers any data, and thus cannot use
 * good_bytes != blk_rq_bytes(req) as the signal for an error.
-* This sets the error explicitly for the problem case.
+* This sets blk_stat explicitly for the problem case.
 */
-   error = __scsi_error_from_host_byte(cmd, result);
+   blk_stat = __scsi_error_from_host_byte(cmd, result);
}
 
/* no bidi support for !blk_rq_is_passthrough yet */
@@ -852,8 +855,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int 
good_bytes)
else if (!(req->rq_flags & RQF_QUIET))
scsi_print_sense(cmd);
result = 0;
-   /* for passthrough error may be set */
-   error = BLK_STS_OK;
+   /* for passthrough, blk_stat may be set */
+   blk_stat = BLK_STS_OK;
}
/*
 * Another corner case: the SCSI status byte is non-zero but 'good'.
@@ -864,23 +867,24 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
int good_bytes)
 */
if (status_byte(result) && scsi_status_is_good(result)) {
result = 0;
-   error = BLK_STS_OK;
+   blk_stat = BLK_STS_OK;
}
 
/*
-* special case: failed zero length commands always need to
-* drop down into the retry code. Otherwise, if we finished
-* all bytes in the request we are done now.
+* Next deal with any sectors which we were able to correctly
+* handle. Failed, zero length commands always need to drop down
+* to retry code. Fast path should return in this block.
 */
-   if (!(blk_rq_bytes(req) == 0 && error) &&
-   !scsi_end_request(req, error, 

[PATCH v4] Make SCSI Status CONDITION MET equivalent to GOOD

[Douglas Gilbert]

The SCSI PRE-FETCH (10 or 16) command is present both on hard disks
and some SSDs. It is useful when the address of the next block(s) to
be read is known but it is not following the LBA of the current READ
(so read-ahead won't help). It returns two "good" SCSI Status values.
If the requested blocks have fitted (or will most likely fit (when
the IMMED bit is set)) into the disk's cache, it returns CONDITION
MET. If it didn't (or will not) fit then it returns GOOD status.

The goal of this patch is to stop the SCSI subsystem treating the
CONDITION MET SCSI status as an error. The current state makes the
PRE-FETCH command effectively unusable via pass-throughs.

A cleanup of the scsi_io_completion() function in scsi_lib.c has
been moved out of this patch to its own patchset titled:
"scsi_io_completion cleanup".


ChangeLog to v4 (removing work done in v3 and v2, leaving):
  - expand scsi_status_is_good() to check for CONDITION MET
  - add another corner case in scsi_io_completion() adjacent
to the one for the RECOVERED ERROR sense key case. That
is another "non-error"

Signed-off-by: Douglas Gilbert 
---
 drivers/scsi/scsi_lib.c | 11 +++
 include/scsi/scsi.h |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index aea5a1ae318b..142400476d84 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -855,6 +855,17 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
int good_bytes)
/* 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.
+*/
+   if (status_byte(result) && scsi_status_is_good(result)) {
+   result = 0;
+   error = BLK_STS_OK;
+   }
 
/*
 * special case: failed zero length commands always need to
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index cb85eddb47ea..eb7853c1a23b 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -47,6 +47,8 @@ static inline int scsi_status_is_good(int status)
 */
status &= 0xfe;
return ((status == SAM_STAT_GOOD) ||
+   (status == SAM_STAT_CONDITION_MET) ||
+   /* Next two "intermediate" statuses are obsolete in SAM-4 */
(status == SAM_STAT_INTERMEDIATE) ||
(status == SAM_STAT_INTERMEDIATE_CONDITION_MET) ||
/* FIXME: this is obsolete in SAM-3 */
-- 
2.14.1

Re: [PATCH V3 1/8] scsi: hpsa: fix selection of reply queue

[Martin K. Petersen]

Ming,

> Please consider 2/8 too since it is still a fix.

I still need the driver maintainer to ack the change.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Hi Finn,

Am 07.03.2018 um 13:54 schrieb Finn Thain:
> On Wed, 7 Mar 2018, Michael Schmitz wrote:
> 
>> The major obstacle now seems to be dynamic allocation of the driver 
>> private data and storing a pointer to that in a way that it can be 
>> retrieved using just the esp pointer. dev_set_drvdata(esp->dev, zep) 
>> causes the module load to crash ...
> 
> I've just noticed that most esp drivers do this:
> 
> static int esp_foo_probe(struct platform_device *dev)
> {
>   ...
>   esp->dev = dev;
>   ...
> }
> 
> But the esp->dev->dev dereferencing sometimes gets overlooked, resulting 
> in a pointer to a struct platform_device being used where a pointer to a 
> struct device should be used (i.e. dma_*() calls). I will look into fixing 
> this up. sun_esp.c doesn't have this problem, but the other drivers do.
> 
> I don't think any of that applies to your zorro_esp code because the 
> version you sent does this,
> 
>   esp->dev = >dev;
> 
> which seems fine to me. But it could end up more convenient to use the 
> sun_esp approach and set esp->dev = z.

It just adds another dereferencing step in the dma_map functions which
we only need to do once here.

But sun_esp also does

dev_set_drvdata(>dev, esp);

i.e. not only does it store the struct device pointer in the esp struct
(by indirection through the platform device struct), but also the struct
esp pointer in struct device.

> I suspect that the problem with zorro_esp is that sometimes you use the 
> esp->dev->driver_data pointer as the struct Scsi_Host pointer, and at 
> other times you use it for the struct zorro_driver_data pointer. (I think 
> I see now why you put the esp pointer in struct zorro_driver_data.)

The zorro_esp_priv pointer (zorro_esp_driver_data pointer are only used
to store board specific config data needed for probe), but yes, you've
found my error.

I overlooked that dev_set_drvdata() and zorro_set_drvdata() will store
their payload in the same struct device instance. Using one for struct
zorro_esp_priv and the other for struct Scsi_host is just asking for
trouble. Thanks for jogging my memory ...

Since there is no other place for me to put driver private data, I need
to change the use of that field to point to the current zorro_esp_priv
instance, and retrieve the struct esp pointer from there. I can retrieve
the host pointer from  struct esp so all should be well.

This is a little unusual so I better add a few comments to save the next
maintainer from unnecessary headache.

> If you like, email the current version to me or push it to a repo 
> somewhere and I'll take a look at it.

I'll take you up on that offer another time, but with the use of
dev->driver_data fixed, the driver no longer crashes. I shouldn't
hack kernel code in a rush...

Now on to mangle the rest of the issues raised in the review...

Cheers,

Michael

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Michael Schmitz]

Hi Geert,

OK, in that case I'll need to work out something similar to the test for
optional SCSI function on the Blizzard 1230/1260 to find out what board
I have when dealing with the duplicate Fastlane/Blizzard1230II ID.

Is the board base address as returned by zorro_resource_start() reliable
to distinguish between Zorro II and Zorro III boards?

Cheers,

Michael

Am 06.03.2018 um 20:43 schrieb Geert Uytterhoeven:
> Hi Michael,
> 
> On Tue, Mar 6, 2018 at 2:11 AM, Michael Schmitz  wrote:
>> Index 1 should have been ZORRO_PROD_PHASE5_CYBERSTORM_MK_II, I've
>> corrected that in the meantime.
>>
>> Fastlane / Blizzard 1230_II distinction is something I an not quite
>> sure about - does the probe function get called twice if the device
>> table contains the same ID twice but with different driver_data
>> contents?
> 
> No, the probe function gets called on the first match only.
> Cfr. drivers/zorro/zorro-driver.c:zorro_device_probe().
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds
> 

Re: [PATCH v3 5/8] scsi: hisi_sas: increase timer expire of internal abort task

[Hannes Reinecke]

On 03/05/2018 03:48 PM, John Garry wrote:
> From: Xiaofei Tan 
> 
> The current 110ms expiry time is not long enough for the internal
> abort task.
> 
> The reason is that the internal abort task could be blocked in HW
> if the HW is retrying to set up link. The internal abort task will
> be executed only when the retry process finished.
> 
Hmm. That sounds weird.
I would have expected that a link retrain will force a device reset,
after which no tasks should be active on the target.
Consequently the succeeding abort task will be a no-op.
Care to clarify?

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: [PATCH v3 8/8] scsi: hisi_sas: Code cleanup and minor bug fixes

[Hannes Reinecke]

On 03/05/2018 03:48 PM, John Garry wrote:
> From: Xiang Chen 
> 
> The patch does some code cleanup and fixes some small bugs:
> - Correct return status of phy_up_v3_hw()
> - Add static for function phy_get_max_linkrate_v3_hw()
> - Change exception return status when no reset method
> - Change magic value to ts->stat in slot_complete_vx_hw()
> - Remove unnecessary check for dev_is_sata()
> - Fix some issues of alignment and indents (Authored by
>   Xiaofei Tan in another patch, but added here to be
>   practical)
> 
> Signed-off-by: Xiaofei Tan 
> Signed-off-by: Xiang Chen 
> Signed-off-by: John Garry 
> ---
>  drivers/scsi/hisi_sas/hisi_sas_main.c  | 14 +++---
>  drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |  4 +++-
>  drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 10 ++
>  drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 16 +---
>  4 files changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c 
> b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index dff9723..49c1fa6 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -33,7 +33,7 @@ u8 hisi_sas_get_ata_protocol(struct host_to_dev_fis *fis, 
> int direction)
>   case ATA_CMD_FPDMA_RECV:
>   case ATA_CMD_FPDMA_SEND:
>   case ATA_CMD_NCQ_NON_DATA:
> - return HISI_SAS_SATA_PROTOCOL_FPDMA;
> + return HISI_SAS_SATA_PROTOCOL_FPDMA;
>  
>   case ATA_CMD_DOWNLOAD_MICRO:
>   case ATA_CMD_ID_ATA:
> @@ -45,7 +45,7 @@ u8 hisi_sas_get_ata_protocol(struct host_to_dev_fis *fis, 
> int direction)
>   case ATA_CMD_WRITE_LOG_EXT:
>   case ATA_CMD_PIO_WRITE:
>   case ATA_CMD_PIO_WRITE_EXT:
> - return HISI_SAS_SATA_PROTOCOL_PIO;
> + return HISI_SAS_SATA_PROTOCOL_PIO;
>  
>   case ATA_CMD_DSM:
>   case ATA_CMD_DOWNLOAD_MICRO_DMA:
> @@ -64,7 +64,7 @@ u8 hisi_sas_get_ata_protocol(struct host_to_dev_fis *fis, 
> int direction)
>   case ATA_CMD_WRITE_LOG_DMA_EXT:
>   case ATA_CMD_WRITE_STREAM_DMA_EXT:
>   case ATA_CMD_ZAC_MGMT_IN:
> - return HISI_SAS_SATA_PROTOCOL_DMA;
> + return HISI_SAS_SATA_PROTOCOL_DMA;
>  
>   case ATA_CMD_CHK_POWER:
>   case ATA_CMD_DEV_RESET:
> @@ -77,21 +77,21 @@ u8 hisi_sas_get_ata_protocol(struct host_to_dev_fis *fis, 
> int direction)
>   case ATA_CMD_STANDBY:
>   case ATA_CMD_STANDBYNOW1:
>   case ATA_CMD_ZAC_MGMT_OUT:
> - return HISI_SAS_SATA_PROTOCOL_NONDATA;
> + return HISI_SAS_SATA_PROTOCOL_NONDATA;
>   default:
>   {
>   if (fis->command == ATA_CMD_SET_MAX) {
>   switch (fis->features) {
>   case ATA_SET_MAX_PASSWD:
>   case ATA_SET_MAX_LOCK:
> - return HISI_SAS_SATA_PROTOCOL_PIO;
> + return HISI_SAS_SATA_PROTOCOL_PIO;
>  
>   case ATA_SET_MAX_PASSWD_DMA:
>   case ATA_SET_MAX_UNLOCK_DMA:
> - return HISI_SAS_SATA_PROTOCOL_DMA;
> + return HISI_SAS_SATA_PROTOCOL_DMA;
>  
>   default:
> - return HISI_SAS_SATA_PROTOCOL_NONDATA;
> + return HISI_SAS_SATA_PROTOCOL_NONDATA;
>   }
>   }
>   if (direction == DMA_NONE)
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c 
> b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> index 8dd0e6a6..520ba69 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> @@ -651,8 +651,10 @@ static int reset_hw_v1_hw(struct hisi_hba *hisi_hba)
>   dev_err(dev, "De-reset failed\n");
>   return -EIO;
>   }
> - } else
> + } else {
>   dev_warn(dev, "no reset method\n");
> + return -EIO;
> + }
>  
return -EINVAL?

>   return 0;
>  }
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c 
> b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> index bd1a48a..69c4dd1 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
> @@ -1095,8 +1095,10 @@ static int reset_hw_v2_hw(struct hisi_hba *hisi_hba)
>   dev_err(dev, "SAS de-reset fail.\n");
>   return -EIO;
>   }
> - } else
> - dev_warn(dev, "no reset method\n");
> + } else {
> + dev_err(dev, "no reset method\n");
> + return -EIO;
> + }
>  
>   return 0;
>  }
return -EINVAL?

> @@ -2408,7 +2410,7 @@ static void slot_err_v2_hw(struct hisi_hba *hisi_hba,
>   spin_lock_irqsave(_hba->lock, flags);
>   hisi_sas_slot_task_free(hisi_hba, task, slot);
>   spin_unlock_irqrestore(_hba->lock, flags);
> - return -1;
> + 

[PATCH] scsi: ufs: ufshcd_dump_regs to use memcpy_fromio

[Tomas Winkler]

ufshcd_dump_regs should use memcpy_fromio to read host registers
instead of directly accessing using memcpy.
The same function is utilized in ufs-qcom.

Elminite compilation warning
drivers/scsi/ufs/ufshcd.c:356:9: warning: incorrect type in argument 6 
(different address spaces)
drivers/scsi/ufs/ufshcd.c:356:9:expected void const *buf
drivers/scsi/ufs/ufshcd.c:356:9:got void [noderef] *mmio_base

Signed-off-by: Tomas Winkler 
---
 drivers/scsi/ufs/ufs-qcom.c | 21 ++---
 drivers/scsi/ufs/ufshcd.c   | 35 ---
 drivers/scsi/ufs/ufshcd.h   |  3 +++
 3 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index 2b38db2eeafa..77ac98ea80d4 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -50,19 +50,10 @@ static void ufs_qcom_get_default_testbus_cfg(struct 
ufs_qcom_host *host);
 static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
   u32 clk_cycles);
 
-static void ufs_qcom_dump_regs(struct ufs_hba *hba, int offset, int len,
-   char *prefix)
-{
-   print_hex_dump(KERN_ERR, prefix,
-   len > 4 ? DUMP_PREFIX_OFFSET : DUMP_PREFIX_NONE,
-   16, 4, (void __force *)hba->mmio_base + offset,
-   len * 4, false);
-}
-
 static void ufs_qcom_dump_regs_wrapper(struct ufs_hba *hba, int offset, int 
len,
-   char *prefix, void *priv)
+  const char *prefix, void *priv)
 {
-   ufs_qcom_dump_regs(hba, offset, len, prefix);
+   ufshcd_dump_regs(hba, offset, len * 4, prefix);
 }
 
 static int ufs_qcom_get_connected_tx_lanes(struct ufs_hba *hba, u32 *tx_lanes)
@@ -1431,7 +1422,7 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
 
 static void ufs_qcom_print_hw_debug_reg_all(struct ufs_hba *hba,
void *priv, void (*print_fn)(struct ufs_hba *hba,
-   int offset, int num_regs, char *str, void *priv))
+   int offset, int num_regs, const char *str, void *priv))
 {
u32 reg;
struct ufs_qcom_host *host;
@@ -1613,7 +1604,7 @@ int ufs_qcom_testbus_config(struct ufs_qcom_host *host)
 
 static void ufs_qcom_testbus_read(struct ufs_hba *hba)
 {
-   ufs_qcom_dump_regs(hba, UFS_TEST_BUS, 1, "UFS_TEST_BUS ");
+   ufshcd_dump_regs(hba, UFS_TEST_BUS, 4, "UFS_TEST_BUS ");
 }
 
 static void ufs_qcom_print_unipro_testbus(struct ufs_hba *hba)
@@ -1639,8 +1630,8 @@ static void ufs_qcom_print_unipro_testbus(struct ufs_hba 
*hba)
 
 static void ufs_qcom_dump_dbg_regs(struct ufs_hba *hba)
 {
-   ufs_qcom_dump_regs(hba, REG_UFS_SYS1CLK_1US, 16,
-   "HCI Vendor Specific Registers ");
+   ufshcd_dump_regs(hba, REG_UFS_SYS1CLK_1US, 16 * 4,
+"HCI Vendor Specific Registers ");
 
/* sleep a bit intermittently as we are dumping too much data */
ufs_qcom_print_hw_debug_reg_all(hba, NULL, ufs_qcom_dump_regs_wrapper);
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3abcd31646eb..f2e1da77045c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -98,8 +98,29 @@
_ret;   \
})
 
-#define ufshcd_hex_dump(prefix_str, buf, len) \
-print_hex_dump(KERN_ERR, prefix_str, DUMP_PREFIX_OFFSET, 16, 4, buf, len, 
false)
+#define ufshcd_hex_dump(prefix_str, buf, len) do {   \
+   size_t __len = (len);\
+   print_hex_dump(KERN_ERR, prefix_str, \
+  __len > 4 ? DUMP_PREFIX_OFFSET : DUMP_PREFIX_NONE,\
+  16, 4, buf, __len, false);\
+} while (0)
+
+int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
+const char *prefix)
+{
+   u8 *regs;
+
+   regs = kzalloc(len, GFP_KERNEL);
+   if (!regs)
+   return -ENOMEM;
+
+   memcpy_fromio(regs, hba->mmio_base + offset, len);
+   ufshcd_hex_dump(prefix, regs, len);
+   kfree(regs);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(ufshcd_dump_regs);
 
 enum {
UFSHCD_MAX_CHANNEL  = 0,
@@ -343,15 +364,7 @@ static void ufshcd_print_uic_err_hist(struct ufs_hba *hba,
 
 static void ufshcd_print_host_regs(struct ufs_hba *hba)
 {
-   /*
-* hex_dump reads its data without the readl macro. This might
-* cause inconsistency issues on some platform, as the printed
-* values may be from cache and not the most recent value.
-* To know whether you are looking at an un-cached version verify
-* that IORESOURCE_MEM flag is on when xxx_get_resource() is invoked
-* during platform/pci probe function.
-*/
-   ufshcd_hex_dump("host regs: 

[PATCH v2] scsi: libsas: defer ata device eh commands to libata

[Jason Yan]

When ata device doing EH, some commands still attached with tasks are not
passed to libata when abort failed or recover failed, so libata did not
handle these commands. After these commands done, sas task is freed, but
ata qc is not freed. This will cause ata qc leak and trigger a warning
like below:

WARNING: CPU: 0 PID: 28512 at drivers/ata/libata-eh.c:4037
ata_eh_finish+0xb4/0xcc
CPU: 0 PID: 28512 Comm: kworker/u32:2 Tainted: G W  OE 4.14.0#1
..
Call trace:
[] ata_eh_finish+0xb4/0xcc
[] ata_do_eh+0xc4/0xd8
[] ata_std_error_handler+0x44/0x8c
[] ata_scsi_port_error_handler+0x480/0x694
[] async_sas_ata_eh+0x4c/0x80
[] async_run_entry_fn+0x4c/0x170
[] process_one_work+0x144/0x390
[] worker_thread+0x144/0x418
[] kthread+0x10c/0x138
[] ret_from_fork+0x10/0x18

If ata qc leaked too many, ata tag allocation will fail and io blocked
for ever.

As suggested by Dan Williams, defer ata device commands to libata and
merge sas_eh_finish_cmd() with sas_eh_defer_cmd(). libata will handle
ata qcs correctly after this.

Signed-off-by: Jason Yan 
CC: Xiaofei Tan 
CC: John Garry 
CC: Dan Williams 
---
 drivers/scsi/libsas/sas_scsi_host.c | 30 ++
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/libsas/sas_scsi_host.c 
b/drivers/scsi/libsas/sas_scsi_host.c
index 6de9681ace82..fd76436b289c 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -223,6 +223,7 @@ int sas_queuecommand(struct Scsi_Host *host, struct 
scsi_cmnd *cmd)
 static void sas_eh_finish_cmd(struct scsi_cmnd *cmd)
 {
struct sas_ha_struct *sas_ha = SHOST_TO_SAS_HA(cmd->device->host);
+   struct domain_device *dev = cmd_to_domain_dev(cmd);
struct sas_task *task = TO_SAS_TASK(cmd);
 
/* At this point, we only get called following an actual abort
@@ -231,6 +232,11 @@ static void sas_eh_finish_cmd(struct scsi_cmnd *cmd)
 */
sas_end_task(cmd, task);
 
+   if (dev_is_sata(dev)) {
+   list_move_tail(>eh_entry, _ha->eh_ata_q);
+   return;
+   }
+
/* now finish the command and move it on to the error
 * handler done list, this also takes it off the
 * error handler pending list.
@@ -238,22 +244,6 @@ static void sas_eh_finish_cmd(struct scsi_cmnd *cmd)
scsi_eh_finish_cmd(cmd, _ha->eh_done_q);
 }
 
-static void sas_eh_defer_cmd(struct scsi_cmnd *cmd)
-{
-   struct domain_device *dev = cmd_to_domain_dev(cmd);
-   struct sas_ha_struct *ha = dev->port->ha;
-   struct sas_task *task = TO_SAS_TASK(cmd);
-
-   if (!dev_is_sata(dev)) {
-   sas_eh_finish_cmd(cmd);
-   return;
-   }
-
-   /* report the timeout to libata */
-   sas_end_task(cmd, task);
-   list_move_tail(>eh_entry, >eh_ata_q);
-}
-
 static void sas_scsi_clear_queue_lu(struct list_head *error_q, struct 
scsi_cmnd *my_cmd)
 {
struct scsi_cmnd *cmd, *n;
@@ -261,7 +251,7 @@ static void sas_scsi_clear_queue_lu(struct list_head 
*error_q, struct scsi_cmnd
list_for_each_entry_safe(cmd, n, error_q, eh_entry) {
if (cmd->device->sdev_target == my_cmd->device->sdev_target &&
cmd->device->lun == my_cmd->device->lun)
-   sas_eh_defer_cmd(cmd);
+   sas_eh_finish_cmd(cmd);
}
 }
 
@@ -631,12 +621,12 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host 
*shost, struct list_head *
case TASK_IS_DONE:
SAS_DPRINTK("%s: task 0x%p is done\n", __func__,
task);
-   sas_eh_defer_cmd(cmd);
+   sas_eh_finish_cmd(cmd);
continue;
case TASK_IS_ABORTED:
SAS_DPRINTK("%s: task 0x%p is aborted\n",
__func__, task);
-   sas_eh_defer_cmd(cmd);
+   sas_eh_finish_cmd(cmd);
continue;
case TASK_IS_AT_LU:
SAS_DPRINTK("task 0x%p is at LU: lu recover\n", task);
@@ -647,7 +637,7 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host 
*shost, struct list_head *
"recovered\n",
SAS_ADDR(task->dev),
cmd->device->lun);
-   sas_eh_defer_cmd(cmd);
+   sas_eh_finish_cmd(cmd);
sas_scsi_clear_queue_lu(work_q, cmd);
goto Again;
}
-- 
2.13.6

Re: [PATCH V4] scsi: core: use blk_mq_requeue_request in __scsi_queue_insert

[jianchao.wang]

Hi Martin

Can you take your precious time to review this ?

Thanks in advice.
Jianchao

On 03/03/2018 09:54 AM, Jianchao Wang wrote:
> In scsi core, __scsi_queue_insert should just put request back on
> the queue and retry using the same command as before. However, for
> blk-mq, scsi_mq_requeue_cmd is employed here which will unprepare
> the request. To align with the semantics of __scsi_queue_insert,
> use blk_mq_requeue_request with kick_requeue_list == true and put
> the reference of scsi_device.
> 
> Cc: Christoph Hellwig 
> Signed-off-by: Jianchao Wang 
> Reviewed-by: Bart Van Assche 
> ---
> Changelog:
> V3 -> V4:
>  - modify the comment and make it more clearly
> 
> V2 -> V3:
>  - add comment to explain why we need a put_device in
>__scsi_queue_insert
>  - add reviewed-by
> 
> V1 -> V2:
>  - add put_device on scsi_device->sdev_gendev
>  drivers/scsi/scsi_lib.c | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index a86df9c..6ce33f6 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -191,7 +191,19 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, 
> int reason, bool unbusy)
>*/
>   cmd->result = 0;
>   if (q->mq_ops) {
> - scsi_mq_requeue_cmd(cmd);
> + /*
> +  * Before a SCSI command is dispatched,
> +  * get_device(>sdev_gendev) is called and the host,
> +  * target and device busy counters are increased. Since
> +  * requeuing a request causes these actions to be repeated and
> +  * since scsi_device_unbusy() has already been called,
> +  * put_device(>sdev_gendev) must still be called. Call
> +  * put_device() after blk_mq_requeue_request() to avoid that
> +  * removal of the SCSI device can start before requeueing has
> +  * happened.
> +  */
> + blk_mq_requeue_request(cmd->request, true);
> + put_device(>sdev_gendev);
>   return;
>   }
>   spin_lock_irqsave(q->queue_lock, flags);
> 

dmesg flooded with "Very big device. Trying to use READ CAPACITY(16)"

[Menion]

Hi all
Operating big capacity HDD such 8TB with complex filesystems like
BTRFS in RAID mode endup in dmesg get flooded by this log, due too
many capacity checks (opaque to the filesystem itself)
The logs come from here:

https://elixir.bootlin.com/linux/latest/source/drivers/scsi/sd.c#L2508

The general guideline tells that KERN_NOTICE (which is the default log
level for dmesg in most distribution) should report information for
any user interest
I think that this information is not really of user interest, rather
more of DEBUG interest
So my suggestion is to lower this log to KERN_DEBUG
Do you agree?
Bye

Re: [PATCH] scsi: pmcraid: Use dma_pool_zalloc()

[Souptick Joarder]

Any comment for this patch.

On Thu, Feb 15, 2018 at 8:44 PM, Souptick Joarder  wrote:
> Use dma_pool_zalloc() instead of dma_pool_alloc + memset
>
> Signed-off-by: Souptick Joarder 
> ---
>  drivers/scsi/pmcraid.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
> index 201c8de..7535161 100644
> --- a/drivers/scsi/pmcraid.c
> +++ b/drivers/scsi/pmcraid.c
> @@ -4716,7 +4716,7 @@ static int pmcraid_allocate_control_blocks(struct 
> pmcraid_instance *pinstance)
>
> for (i = 0; i < PMCRAID_MAX_CMD; i++) {
> pinstance->cmd_list[i]->ioa_cb =
> -   dma_pool_alloc(
> +   dma_pool_zalloc(
> pinstance->control_pool,
> GFP_KERNEL,
> &(pinstance->cmd_list[i]->ioa_cb_bus_addr));
> @@ -4725,8 +4725,6 @@ static int pmcraid_allocate_control_blocks(struct 
> pmcraid_instance *pinstance)
> pmcraid_release_control_blocks(pinstance, i);
> return -ENOMEM;
> }
> -   memset(pinstance->cmd_list[i]->ioa_cb, 0,
> -   sizeof(struct pmcraid_control_block));
> }
> return 0;
>  }
> --
> 1.9.1
>

Re: [PATCH] scsi: mvsas: Use dma_pool_zalloc()

[Souptick Joarder]

Any comment for this patch.

On Thu, Feb 15, 2018 at 9:28 PM, Souptick Joarder  wrote:
> Use dma_pool_zalloc() instead of dma_pool_alloc + memset
>
> Signed-off-by: Souptick Joarder 
> ---
>  drivers/scsi/mvsas/mv_sas.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
> index cff43bd..d00d37d 100644
> --- a/drivers/scsi/mvsas/mv_sas.c
> +++ b/drivers/scsi/mvsas/mv_sas.c
> @@ -790,12 +790,11 @@ static int mvs_task_prep(struct sas_task *task, struct 
> mvs_info *mvi, int is_tmf
> slot->n_elem = n_elem;
> slot->slot_tag = tag;
>
> -   slot->buf = dma_pool_alloc(mvi->dma_pool, GFP_ATOMIC, >buf_dma);
> +   slot->buf = dma_pool_zalloc(mvi->dma_pool, GFP_ATOMIC, 
> >buf_dma);
> if (!slot->buf) {
> rc = -ENOMEM;
> goto err_out_tag;
> }
> -   memset(slot->buf, 0, MVS_SLOT_BUF_SZ);
>
> tei.task = task;
> tei.hdr = >slot[tag];
> --
> 1.9.1
>

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

[Finn Thain]

On Tue, 6 Mar 2018, Michael Schmitz wrote:

> The whole !write branch will never be executed, and I could just omit it 
> entirely for now, or leave it as it was in the Mac driver.
> 

We could make use of the !write branch in zorro_esp, even if it was only 
to figure out the SELAS/MSG OUT issue for the benefit of mac_esp. But 
let's keep them in sync.

-- 

> Cheers,
> 
>   Michael
> 

RE: [PATCH] scsi: megaraid: Use dma_pool_zalloc()

[Sumit Saxena]

-Original Message-
From: Souptick Joarder [mailto:jrdr.li...@gmail.com]
Sent: Thursday, February 15, 2018 9:55 PM
To: kashyap.de...@broadcom.com; sumit.sax...@broadcom.com;
shivasharan.srikanteshw...@broadcom.com
Cc: megaraidlinux@broadcom.com; linux-scsi@vger.kernel.org
Subject: [PATCH] scsi: megaraid: Use dma_pool_zalloc()

Use dma_pool_zalloc() instead of dma_pool_alloc + memset

Signed-off-by: Souptick Joarder 
---
 drivers/scsi/megaraid/megaraid_sas_base.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
b/drivers/scsi/megaraid/megaraid_sas_base.c
index a71ee67..905ea36 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -4022,7 +4022,7 @@ static int megasas_create_frame_pool(struct
megasas_instance *instance)

cmd = instance->cmd_list[i];

-   cmd->frame = dma_pool_alloc(instance->frame_dma_pool,
+   cmd->frame = dma_pool_zalloc(instance->frame_dma_pool,
GFP_KERNEL,
>frame_phys_addr);

cmd->sense = dma_pool_alloc(instance->sense_dma_pool,
@@ -4038,7 +4038,6 @@ static int megasas_create_frame_pool(struct
megasas_instance *instance)
return -ENOMEM;
}

-   memset(cmd->frame, 0, instance->mfi_frame_size);
cmd->frame->io.context = cpu_to_le32(cmd->index);
cmd->frame->io.pad_0 = 0;
if ((instance->adapter_type == MFI_SERIES) &&
reset_devices)

Acked-by: Sumit Saxena 

--
1.9.1

Re: [PATCH v3 8/8] scsi: hisi_sas: Code cleanup and minor bug fixes

[John Garry]

Hi Hannes,

Thanks for checking this.


> +  }
>

return -EINVAL?


>return 0;
>  }


[ ... ]


> +  }
>
>return 0;
>  }

return -EINVAL?


> @@ -2408,7 +2410,7 @@ static void slot_err_v2_hw(struct hisi_hba *hisi_hba,
>spin_lock_irqsave(_hba->lock, flags);
>hisi_sas_slot_task_free(hisi_hba, task, slot);
>spin_unlock_irqrestore(_hba->lock, flags);


[ ... ]


>}
> -  } else
> +  } else {
>dev_err(dev, "no reset method!\n");
> +  return -EIO;
> +  }
>
>return 0;
>  }

return -EINVAL?



These 3 changes you suggest are accepted.


> @@ -731,7 +733,7 @@ static void phy_hard_reset_v3_hw(struct hisi_hba 
*hisi_hba, int phy_no)
>start_phy_v3_hw(hisi_hba, phy_no);
>  }
>
> -enum sas_linkrate phy_get_max_linkrate_v3_hw(void)
> +static enum sas_linkrate phy_get_max_linkrate_v3_hw(void)
>  {
>return SAS_LINK_RATE_12_0_GBPS;
>  }
> @@ -1096,7 +1098,7 @@ static int prep_abort_v3_hw(struct hisi_hba *hisi_hba,
>/* dw0 */
>hdr->dw0 = cpu_to_le32((5 << CMD_HDR_CMD_OFF) | /*abort*/
>   (port->id << CMD_HDR_PORT_OFF) |
> - ((dev_is_sata(dev) ? 1:0)
> + (dev_is_sata(dev)
><< CMD_HDR_ABORT_DEVICE_TYPE_OFF) |
>(abort_flag
> << CMD_HDR_ABORT_FLAG_OFF));
> @@ -1114,7 +1116,7 @@ static int prep_abort_v3_hw(struct hisi_hba *hisi_hba,
>
>  static int phy_up_v3_hw(int phy_no, struct hisi_hba *hisi_hba)
>  {
> -  int i, res = 0;
> +  int i, res;
>u32 context, port_id, link_rate;
>struct hisi_sas_phy *phy = _hba->phy[phy_no];
>struct asd_sas_phy *sas_phy = >sas_phy;
> @@ -1186,7 +1188,7 @@ static int phy_up_v3_hw(int phy_no, struct hisi_hba 
*hisi_hba)
>phy->port_id = port_id;
>phy->phy_attached = 1;
>hisi_sas_notify_phy_event(phy, HISI_PHYE_PHY_UP);
> -
> +  res = IRQ_HANDLED;
>  end:
>hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT0,
> CHL_INT0_SL_PHY_ENABLE_MSK);
> @@ -1217,7 +1219,7 @@ static int phy_down_v3_hw(int phy_no, struct hisi_hba 
*hisi_hba)
>hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT0, CHL_INT0_NOT_RDY_MSK);
>hisi_sas_phy_write32(hisi_hba, phy_no, PHYCTRL_NOT_RDY_MSK, 0);
>
> -  return 0;
> +  return IRQ_HANDLED;
>  }
>
>  static void phy_bcast_v3_hw(int phy_no, struct hisi_hba *hisi_hba)

If we're returning IRQ_HANDLED, shouldn't the function have the return
type irqreturn_t ?
But as this isn't an interrupt handler, shouldn't we rather fixup the
caller to check for the correct return values?


Since function phy_bcast_v3_hw() does no checking and would always 
return IRQ_HANDLED, we saw not point in having a return code and 
checking it. However, I did notice that we don't set res = IRQ_HANDLED 
after calling this function:

if (irq_value & CHL_INT0_SL_RX_BCST_ACK_MSK)
/* phy bcast */
phy_bcast_v3_hw(phy_no, hisi_hba);
} else {
if (irq_value & CHL_INT0_NOT_RDY_MSK)
/* phy down */
if (phy_down_v3_hw(phy_no, hisi_hba)
== IRQ_HANDLED)
res = IRQ_HANDLED;
}
}
irq_msk >>= 4;
phy_no++;
}

return res;
}

So this needs to be remedied.




> @@ -1573,7 +1575,7 @@ static irqreturn_t fatal_axi_int_v3_hw(int irq_no, void 
*p)
>spin_lock_irqsave(_hba->lock, flags);
>hisi_sas_slot_task_free(hisi_hba, task, slot);
>spin_unlock_irqrestore(_hba->lock, flags);
> -  return -1;
> +  return ts->stat;
>}
>
>if (unlikely(!sas_dev)) {
>


Thanks,
John


Cheers,

Hannes
-- Dr. Hannes Reinecke Teamlead 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: [PATCH v3 5/8] scsi: hisi_sas: increase timer expire of internal abort task

[John Garry]

On 06/03/2018 11:23, Hannes Reinecke wrote:

On 03/05/2018 03:48 PM, John Garry wrote:

From: Xiaofei Tan 

The current 110ms expiry time is not long enough for the internal
abort task.

The reason is that the internal abort task could be blocked in HW
if the HW is retrying to set up link. The internal abort task will
be executed only when the retry process finished.


Hmm. That sounds weird.
I would have expected that a link retrain will force a device reset,
after which no tasks should be active on the target.
Consequently the succeeding abort task will be a no-op.
Care to clarify?



Hi Hannes,

It sounds like you're talking about TMF task abort, right? This patch is 
related to controller internal abort function.


Our HW supports internal abort, where any pending queued commands in the 
controller can be aborted, so they will not be executed. When a disk is 
removed or a nexus reset are times when we issue such a command.


Thanks very much,
John


Cheers,

Hannes

Re: [PATCH V2] scsi: qla2xxx: Fix crashes in qla2x00_probe_one on probe failure

[Hannes Reinecke]

On 03/05/2018 06:02 AM, Bill Kuzeja wrote:
> Because of the shifting around of code in qla2x00_probe_one recently,
> failures during adapter initialization can lead to problems, i.e. NULL
> pointer crashes and doubly freed data structures which cause eventual
> panics.
> 
> This V2 version makes the relevant memory free routines idempotent, so
> repeat calls won't cause any harm. I also removed the problematic
> probe_init_failed exit point as it is not needed.
> 
> Fixes: d64d6c5671db ("scsi: qla2xxx: Fix NULL pointer crash due to probe 
> failure")
> Signed-off-by: Bill Kuzeja 
> 
> ---
> 
> Some of these issues are due to:
> 
> commit d64d6c5671db scsi: qla2xxx: Fix NULL pointer crash due to probe failure
> 
> where some frees were moved around, as well as the error exit from
> a qla2x00_request_irqs failure.
> 
> This was a fix for:
> 
> commit d74595278f4a scsi: qla2xxx: Add multiple queue pair functionality.
> 
> which caused problems of its own.
> 
> To reproduce these issues, I run a test where I break the card early in
> init, (and also through kprobe fault injection). This way, I've been able
> to hit several different types of crashes, all started by failures of
> various routines called throughout the probe.
> 
> The problematic routines that fail and their exit points are:
> 
> qla2x00_alloc_queues  => probe_init_failed
> initialize_adapter=> probe_failed
> kthread_create=> probe_failed
> scsi_add_host => probe_failed
> 
> Exit points are ordered in this way:
> 
> probe_init_failed:
> qla2x00_free_req_que(ha, req);
> ha->req_q_map[0] = NULL;
> clear_bit(0, ha->req_qid_map);
> qla2x00_free_rsp_que(ha, rsp);
> ha->rsp_q_map[0] = NULL;
> clear_bit(0, ha->rsp_qid_map);
> ha->max_req_queues = ha->max_rsp_queues = 0;
> 
> probe_failed:
> if (base_vha->timer_active)
> qla2x00_stop_timer(base_vha);
> ...
> qla2x00_free_device(base_vha);
> 
> scsi_host_put(base_vha->host);
> 
> probe_hw_failed:
> qla2x00_mem_free(ha);
> qla2x00_free_req_que(ha, req);
> qla2x00_free_rsp_que(ha, rsp);
> qla2x00_clear_drv_active(ha);
> 
> Note that qla2x00_free_device calls qla2x00_mem_free and
> qla2x00_free_rsp_que and qla2x00_free_req_que. So if we exit to
> probe_failed or probe_init_failed, we'll end up calling these
> routines multiple times.
> 
> These routines are not idempotent, I am making them so. This solves
> most of the issues.
> 
> Also probe_init_failed is not needed. In the place that it is called,
> ha->req_q_map[0] and ha->rsp_q_map[0] are not setup. And we now call
> qla2x00_free_rsp_que/qla2x00_free_req_que below in probe_hw_failed.
> I removed this exit point entirely.
> 
> Along the way I found that the return code for qla2x00_alloc_queues
> never really causes us to exit out of the probe routine.
> 
> In order to fail...
> 
> if (!qla2x00_alloc_queues(ha, req, rsp)) {
> 
> ...we must return 0. However, internally, if this routine succeeds it
> returns 1 and if it fails it returns -ENOMEM. So I am modifying
> qla2x00_alloc_queues to fall in line with other return conventions
> where zero is a success (and obviously have changed the probe routine
> accordingly).
> 
> One more issue falls out of this case: when qla2x00_abort_all_cmds
> is invoked from qla2x00_free_device and request queues are not
> allocated (qla2x00_alloc_queues has not succeeded), we crash on a NULL
> pointer (ha->req_q_map[que]). So check for this at the start of
> qla2x00_abort_all_cmds and exit accordingly.
> 
> I've tested out these changes thoroughly failing initialization at
> various times. I've also used kprobes to inject errors to force us
> into various error paths.
> ---
>  drivers/scsi/qla2xxx/qla_os.c | 59 
> +++
>  1 file changed, 37 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index afcb5567..3860bdfc 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -454,7 +454,7 @@ static int qla2x00_alloc_queues(struct qla_hw_data *ha, 
> struct req_que *req,
>   ha->req_q_map[0] = req;
>   set_bit(0, ha->rsp_qid_map);
>   set_bit(0, ha->req_qid_map);
> - return 1;
> + return 0;
>  
>  fail_qpair_map:
>   kfree(ha->base_qpair);
> @@ -471,6 +471,9 @@ static int qla2x00_alloc_queues(struct qla_hw_data *ha, 
> struct req_que *req,
>  
>  static void qla2x00_free_req_que(struct qla_hw_data *ha, struct req_que *req)
>  {
> + if (!ha->req_q_map)
> + return;
> +
>   if (IS_QLAFX00(ha)) {
>   if (req && req->ring_fx00)
>   dma_free_coherent(>pdev->dev,
> @@ -481,14 +484,17 @@ static void qla2x00_free_req_que(struct qla_hw_data 
> *ha, struct req_que *req)
>   (req->length + 1) * sizeof(request_t),
>   req->ring,