Re: [PATCH 03/14] nvme: return BLK_EH_DONE from ->timeout

[Ming Lei]

On Wed, May 23, 2018 at 02:19:30PM +0200, Christoph Hellwig wrote:
> NVMe always completes the request before returning from ->timeout, either
> by polling for it, or by disabling the controller.  Return BLK_EH_DONE so
> that the block layer doesn't even try to complete it again.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  drivers/nvme/host/pci.c| 14 +-
>  drivers/nvme/host/rdma.c   |  2 +-
>  drivers/nvme/target/loop.c |  2 +-
>  3 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 917e1714f7d9..31525324b79f 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1205,7 +1205,7 @@ static enum blk_eh_timer_return nvme_timeout(struct 
> request *req, bool reserved)
>   nvme_warn_reset(dev, csts);
>   nvme_dev_disable(dev, false);
>   nvme_reset_ctrl(>ctrl);
> - return BLK_EH_HANDLED;
> + return BLK_EH_DONE;
>   }
>  
>   /*
> @@ -1215,14 +1215,14 @@ static enum blk_eh_timer_return nvme_timeout(struct 
> request *req, bool reserved)
>   dev_warn(dev->ctrl.device,
>"I/O %d QID %d timeout, completion polled\n",
>req->tag, nvmeq->qid);
> - return BLK_EH_HANDLED;
> + return BLK_EH_DONE;
>   }
>  
>   /*
>* Shutdown immediately if controller times out while starting. The
>* reset work will see the pci device disabled when it gets the forced
>* cancellation error. All outstanding requests are completed on
> -  * shutdown, so we return BLK_EH_HANDLED.
> +  * shutdown, so we return BLK_EH_DONE.
>*/
>   switch (dev->ctrl.state) {
>   case NVME_CTRL_CONNECTING:
> @@ -1232,7 +1232,7 @@ static enum blk_eh_timer_return nvme_timeout(struct 
> request *req, bool reserved)
>req->tag, nvmeq->qid);
>   nvme_dev_disable(dev, false);
>   nvme_req(req)->flags |= NVME_REQ_CANCELLED;
> - return BLK_EH_HANDLED;
> + return BLK_EH_DONE;
>   default:
>   break;
>   }
> @@ -1249,12 +1249,8 @@ static enum blk_eh_timer_return nvme_timeout(struct 
> request *req, bool reserved)
>   nvme_dev_disable(dev, false);
>   nvme_reset_ctrl(>ctrl);
>  
> - /*
> -  * Mark the request as handled, since the inline shutdown
> -  * forces all outstanding requests to complete.
> -  */
>   nvme_req(req)->flags |= NVME_REQ_CANCELLED;
> - return BLK_EH_HANDLED;
> + return BLK_EH_DONE;
>   }
>  
>   if (atomic_dec_return(>ctrl.abort_limit) < 0) {
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 1eb4438a8763..ac7462cd7f0f 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1598,7 +1598,7 @@ nvme_rdma_timeout(struct request *rq, bool reserved)
>   /* fail with DNR on cmd timeout */
>   nvme_req(rq)->status = NVME_SC_ABORT_REQ | NVME_SC_DNR;
>  
> - return BLK_EH_HANDLED;
> + return BLK_EH_DONE;
>  }
>  
>  static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
> diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
> index 27a8561c0cb9..22e3627bf16b 100644
> --- a/drivers/nvme/target/loop.c
> +++ b/drivers/nvme/target/loop.c
> @@ -146,7 +146,7 @@ nvme_loop_timeout(struct request *rq, bool reserved)
>   /* fail with DNR on admin cmd timeout */
>   nvme_req(rq)->status = NVME_SC_ABORT_REQ | NVME_SC_DNR;
>  
> - return BLK_EH_HANDLED;
> + return BLK_EH_DONE;
>  }
>  
>  static blk_status_t nvme_loop_queue_rq(struct blk_mq_hw_ctx *hctx,
> -- 
> 2.17.0
> 

This change should have been done after '[PATCH 13/14] blk-mq: Remove
generation seqeunce', otherwise the timed-out request won't be completed
by nvme_cancel_request() at all because we always marked this request as
'COMPLETE' before calling .timeout().

Thanks,
Ming

Re: [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Jason Yan]

On 2018/5/23 17:24, Sebastian Andrzej Siewior wrote:

On 2018-05-23 09:46:30 [+0100], John Garry wrote:

On 22/05/2018 18:31, Sebastian Andrzej Siewior wrote:

On 2018-05-18 14:31:27 [+0100], John Garry wrote:

On 04/05/2018 15:50, Sebastian Andrzej Siewior wrote:

Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
management duties from lldds") the sas_ata_qc_issue() function unlocks
the ata_port.lock and disables interrupts before doing so.
That lock is always taken with disabled interrupts so at this point, the
interrupts are already disabled. There is no need to disable the
interrupts before the unlock operation because they are already
disabled.


Is the only caller ata_qc_issue(), which has spin_lock_irqsave(host lock)
enabled?


I don't understand the question.


It seems to me that ap->lock can be taken in interrupt context, so then we
know it should be locked with interrupts disabled always.


yes, the comment above the function says so, too.


I was just really asking how you know interrupts are disabled already? Maybe
it's obvious.


The lock has to be taken with interrupts disabled. If the interrupts
were enabled at the time sas_ata_qc_issue() was invoked then the lock
was already taken in a bad way and disabling interrupts before the
unlock does not make it any better.
To verify that the locking is okay you can build the kernel with lockdep
enabled and CONFIG_PROVE_LOCKING=y set. That way lockdep will inform you
as soon as it notices the lock taken in interrupt context and in
process-context with enabled interrupts.



For libsas, ata_qc_issue() now have three callers:
1. sas_queuecommand(), the normal io patch, have taken ap->lock with
irq disabled
2. ata_exec_internal_sg(), from EH context, have taken ap->lock with
irq disabled too
3. __ata_qc_complete(), from io completion path, have taken ap->lock
with irq disabled too(this may be in interrupts context)

I think it's fine to delete this irq save code. As for the "TODO"
comment, I think we can add:

BUG_ON(!irqs_disabled());

or

WARN_ON_ONCE(!irqs_disabled());

and then delete the "TODO" comment.


cheers,


Sebastian

.

Re: BUG: scsi/qla2xxx: BUG_ON(blk_queued_rq(req) is triggered in blk_finish_request

[jianchao.wang]

Would anyone please take a look at this ?

Thanks in advance
Jianchao

On 05/23/2018 11:55 AM, jianchao.wang wrote:
> 
> 
> Hi all
> 
> Our customer met a panic triggered by BUG_ON in blk_finish_request.
>>From the dmesg log, the BUG_ON was triggered after command abort occurred 
>>many times.
> There is a race condition in the following scenario.
> 
> cpu A   cpu B
> kworker interrupt
>  
> scmd_eh_abort_handler()   
>   -> scsi_try_to_abort_cmd()
> -> qla2xxx_eh_abort()
>   -> qla2x00_eh_wait_on_command()   qla2x00_status_entry()   
>   -> qla2x00_sp_compl()
> -> qla2x00_sp_free_dma()
>   -> scsi_queue_insert()  
> -> __scsi_queue_insert() 
>   -> blk_requeue_request()
> -> blk_clear_rq_complete()
> -> scsi_done
>   -> blk_complete_request
> -> blk_mark_rq_complete   
>   -> elv_requeue_request()  -> __blk_complete_request()
> -> __elv_add_request()
> // req will be queued here
>  
> BLK_SOFTIRQ
> scsi_softirq_done()
>   -> scsi_finish_command()
> -> 
> scsi_io_completion()
>   -> 
> scsi_end_request()
> -> 
> blk_finish_request()  // BUG_ON(blk_queued_rq(req)) !!!
> 
> The issue will not be triggered most of time, because the request is marked 
> as complete by timeout path.
> So the scsi_done from qla2x00_sp_compl does nothing.
> But as the scenario above, if the complete state has been cleaned by 
> blk_requeue_request, we will get
> the request both requeued and completed, and then BUG_ON(blk_queued_rq(req)) 
> in blk_finish_request comes up.
> 
> Is there any solution for this in qla2xxx driver side ?
> 
> Thanks
> Jianchao
> 
> 

Re: [PATCH V2] PCI/portdrv: do not disable device on reboot/shutdown

[Sinan Kaya]

On 5/23/2018 5:32 PM, Bjorn Helgaas wrote:
> 
> The crash seems to indicate that the hpsa device attempted a DMA after
> we cleared the Root Port's PCI_COMMAND_MASTER, which means
> hpsa_shutdown() didn't stop DMA from the device (it looks like *most*
> shutdown methods don't disable device DMA, so it's in good company).

All drivers are expected to shutdown DMA and interrupts in their shutdown()
routines. They can skip removing threads, data structures etc. but DMA and
interrupt disabling are required. This is the difference between shutdown()
and remove() callbacks.

If you see that this is not being done in HPSA, then that is where the
bugfix should be.

Counter argument is that if shutdown() is not implemented, at least remove()
should be called. Expecting all drivers to implement shutdown() callbacks
is just bad by design in my opinion. 

Code should have fallen back to remove() if shutdown() doesn't exist.
I can propose a patch for this but this is yet another story to chase.

> 
>> This has been found to cause crashes on HP DL360 Gen9 machines during
>> reboot. Besides, kexec is already clearing the bus master bit in
>> pci_device_shutdown() after all PCI drivers are removed.
> 
> The original path was:
> 
>   pci_device_shutdown(hpsa)
> drv->shutdown
>   hpsa_shutdown # hpsa_pci_driver.shutdown
>   ...
>   pci_device_shutdown(RP)   # root port
> drv->shutdown
>   pcie_portdrv_remove   # pcie_portdriver.shutdown
> pcie_port_device_remove
>   pci_disable_device
> do_pci_disable_device
>   # clear RP PCI_COMMAND_MASTER
> if (kexec)
>   pci_clear_master(RP)
> # clear RP PCI_COMMAND_MASTER
> 
> If I understand correctly, the new path after this patch is:
> 
>   pci_device_shutdown(hpsa)
> drv->shutdown
>   hpsa_shutdown # hpsa_pci_driver.shutdown
>   ...
>   pci_device_shutdown(RP)   # root port
> drv->shutdown
>   pcie_portdrv_shutdown # pcie_portdriver.shutdown
> __pcie_portdrv_remove(RP, false)
>   pcie_port_device_remove(RP, false)
> # do NOT clear RP PCI_COMMAND_MASTER

yup

> if (kexec)
>   pci_clear_master(RP)
> # clear RP PCI_COMMAND_MASTER
> 
> I guess this patch avoids the panic during reboot because we're not in
> the kexec path, so we never clear PCI_COMMAND_MASTER for the Root
> Port, so the hpsa device can DMA happily until the lights go out.
> 
> But DMA continuing for some random amount of time before the reboot or
> shutdown happens makes me a little queasy.  That doesn't sound safe.
> The more I think about this, the more confused I get.  What am I
> missing?  

see above.

> 
>> Just remove the extra clear in shutdown path by seperating the remove and
>> shutdown APIs in the PORTDRV.
>>
>>  static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev,
>> @@ -218,7 +228,7 @@ static struct pci_driver pcie_portdriver = {
>>  
>>  .probe  = pcie_portdrv_probe,
>>  .remove = pcie_portdrv_remove,
>> -.shutdown   = pcie_portdrv_remove,
>> +.shutdown   = pcie_portdrv_shutdown,
> 
> What are the circumstances when we call .remove() vs .shutdown()?
> 
> I guess the main (maybe only) way to call .remove() is to hot-remove
> the port?  And .shutdown() is basically used in the reboot and kexec
> paths?

Correct. shutdown() is only called during reboot/shutdown calls. If you echo
1 into the remove file, remove() gets called. Handy for hotplug use cases.
It needs to be the exact opposite of the probe. It needs to clean up resources
etc. and have the HW in a state where it can be reinitialized via probe again.

> 
>>  .err_handler= _portdrv_err_handler,
>>  
>> -- 
>> 2.7.4
>>
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

Re: [PATCH V2] PCI/portdrv: do not disable device on reboot/shutdown

[Bjorn Helgaas]

[-cc Gabriele (invalid email address)]
[+cc Don, esc.storagedev, linux-scsi since hpsa is involved]

Background for newcomers:

  Ryan reported a panic on shutdown/reboot [1] on DL360 Gen9.  I think
  the problem is that the shutdown path clears PCI_COMMAND_MASTER on
  the Root Port leading to an hpsa device, the hpsa device attempts a
  DMA, the Root Port treats the DMA as an Unsupported Request (a fatal
  error), and that leads to a panic.

  This patch avoids the problem by changing the Root Port shutdown
  path so it doesn't clear PCI_COMMAND_MASTER.

  [1] https://bugzilla.kernel.org/show_bug.cgi?id=199779

On Tue, May 22, 2018 at 10:44:46PM -0400, Sinan Kaya wrote:
> 'Commit cc27b735ad3a ("PCI/portdrv: Turn off PCIe services during
> shutdown")' has been added to kernel to shutdown pending PCIe port
> service interrupts during reboot so that a newly started kexec kernel
> wouldn't observe pending interrupts.
> 
> pcie_port_device_remove() is disabling the root port and switches by
> calling pci_disable_device() after all PCIe service drivers are shutdown.

It's interesting and annoying that pci_enable_device() and
pci_disable_device() sound like they should be inverses, or at least
related, but they really aren't.

pci_enable_device() basically turns on PCI_COMMAND_MEMORY and/or
PCI_COMMAND_IO so the device will respond to programmed I/O.  But
pci_disable_device() leaves programmed I/O enabled and turns off
PCI_COMMAND_MASTER, which keeps the device from performing DMA or
forwarding transactions upstream (in the case of bridges).

Not an issue for this patch, just an observation :)

> pci_disable_device() has a much wider impact then port service itself and
> it prevents all inbound transactions to reach to the system and impacts
> the entire PCI traffic behind the bridge.
> 
> Issue is that pcie_port_device_remove() doesn't maintain any coordination
> with the rest of the PCI device drivers in the system before clearing the
> bus master bit.

I am interested in this part because if there really isn't any
coordination, that's a good argument for getting rid of portdrv, which
I want to do anyway.  But I haven't dug into this enough to verify it.

The portdrv does register as a regular PCI driver (pcie_portdriver),
and I'm pretty sure the driver core is smart enough to call the driver
entry points in a postorder traversal (children before parents), which
means the hpsa .shutdown() should be called before the portdrv
.shutdown().

The crash seems to indicate that the hpsa device attempted a DMA after
we cleared the Root Port's PCI_COMMAND_MASTER, which means
hpsa_shutdown() didn't stop DMA from the device (it looks like *most*
shutdown methods don't disable device DMA, so it's in good company).

> This has been found to cause crashes on HP DL360 Gen9 machines during
> reboot. Besides, kexec is already clearing the bus master bit in
> pci_device_shutdown() after all PCI drivers are removed.

The original path was:

  pci_device_shutdown(hpsa)
drv->shutdown
  hpsa_shutdown # hpsa_pci_driver.shutdown
  ...
  pci_device_shutdown(RP)   # root port
drv->shutdown
  pcie_portdrv_remove   # pcie_portdriver.shutdown
pcie_port_device_remove
  pci_disable_device
do_pci_disable_device
  # clear RP PCI_COMMAND_MASTER
if (kexec)
  pci_clear_master(RP)
# clear RP PCI_COMMAND_MASTER

If I understand correctly, the new path after this patch is:

  pci_device_shutdown(hpsa)
drv->shutdown
  hpsa_shutdown # hpsa_pci_driver.shutdown
  ...
  pci_device_shutdown(RP)   # root port
drv->shutdown
  pcie_portdrv_shutdown # pcie_portdriver.shutdown
__pcie_portdrv_remove(RP, false)
  pcie_port_device_remove(RP, false)
# do NOT clear RP PCI_COMMAND_MASTER
if (kexec)
  pci_clear_master(RP)
# clear RP PCI_COMMAND_MASTER

I guess this patch avoids the panic during reboot because we're not in
the kexec path, so we never clear PCI_COMMAND_MASTER for the Root
Port, so the hpsa device can DMA happily until the lights go out.

But DMA continuing for some random amount of time before the reboot or
shutdown happens makes me a little queasy.  That doesn't sound safe.
The more I think about this, the more confused I get.  What am I
missing?  

> Just remove the extra clear in shutdown path by seperating the remove and
> shutdown APIs in the PORTDRV.
> 
> Signed-off-by: Sinan Kaya 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=199779
> Fixes: cc27b735ad3a ("PCI/portdrv: Turn off PCIe services during shutdown")
> Cc: sta...@vger.kernel.org
> Reported-by: Ryan Finnie 
> ---
>  drivers/pci/pcie/portdrv.h  |  2 +-
>  drivers/pci/pcie/portdrv_core.c |  5 +++--
>  drivers/pci/pcie/portdrv_pci.c  | 16 +---
>  3 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git 

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Randy Dunlap]

On 05/23/2018 02:22 PM, Jens Axboe wrote:
> On 5/23/18 3:20 PM, Randy Dunlap wrote:
>> On 05/23/2018 02:14 PM, Jens Axboe wrote:
>>> On 5/23/18 2:52 PM, Kees Cook wrote:
 On Wed, May 23, 2018 at 7:31 AM, Jens Axboe  wrote:
> On 5/23/18 8:25 AM, Christoph Hellwig wrote:
>> On Wed, May 23, 2018 at 08:13:56AM -0600, Jens Axboe wrote:
 Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
 drivers/scsi/Makefile as:

 obj-$(CONFIG_BLK_SCSI_REQUEST)+= scsi_sense.o

 Every place I want to use the code is already covered by
 CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
 put the .c file. :P
>>>
>>> I think this is so much saner than a SCSI select or dependency, so I'll
>>> have to disagree with Martin and Christoph. Just put it in drivers/scsi,
>>> if it's the location they care about.
>>
>> I actually plan to remove CONFIG_BLK_SCSI_REQUEST in a merge window
>> or two.  The only users are scsi and the ide layer, (virtio_blk
>> support has already been accidentally disabled for a while), and getting
>> rid of it allows to to shrink and simply the scsi data structures.
>>
>> But if you want this for now lets keep scsi_sense.c in drivers/scsi
>> but depend on CONFIG_BLK_SCSI_REQUEST, that is easy enough to fix up.
>
> It could be a stand-alone dependency, doesn't have to be BLK_SCSI_REQUEST.
> BLA_SCSI_SENSE or something would do. I don't care too much about that,
> mostly getting rid of the entire stack dependency.

 Aaand, I can't do this and leave it in drivers/scsi because of 
 drivers/Makefile:

 obj-$(CONFIG_SCSI)  += scsi/

 So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's
 scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll
 still need to move the code from drivers/scsi/ to block/. Is this
 okay?
>>>
>>> Ugh, so that would necessitate a change there too. As I said before,
>>> I don't really care where it lives. I know the SCSI folks seem bothered
>>> by moving it, but in reality, it's not like this stuff will likely ever
>>> really change. Of the two choices (select entire SCSI stack, or just move
>>> this little bit), I know what I would consider the saner option...
>>>
>>
>> or option 3:
>>
>> obj-y   += scsi/
>>
>> so that make descends into drivers/scsi/ and then builds whatever is needed,
>> depending on individual kconfig options.
> 
> Right, that was the initial option, the later two are the other options.
> 

Sorry, I'm late to the party.

-- 
~Randy

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Jens Axboe]

On 5/23/18 3:20 PM, Randy Dunlap wrote:
> On 05/23/2018 02:14 PM, Jens Axboe wrote:
>> On 5/23/18 2:52 PM, Kees Cook wrote:
>>> On Wed, May 23, 2018 at 7:31 AM, Jens Axboe  wrote:
 On 5/23/18 8:25 AM, Christoph Hellwig wrote:
> On Wed, May 23, 2018 at 08:13:56AM -0600, Jens Axboe wrote:
>>> Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
>>> drivers/scsi/Makefile as:
>>>
>>> obj-$(CONFIG_BLK_SCSI_REQUEST)+= scsi_sense.o
>>>
>>> Every place I want to use the code is already covered by
>>> CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
>>> put the .c file. :P
>>
>> I think this is so much saner than a SCSI select or dependency, so I'll
>> have to disagree with Martin and Christoph. Just put it in drivers/scsi,
>> if it's the location they care about.
>
> I actually plan to remove CONFIG_BLK_SCSI_REQUEST in a merge window
> or two.  The only users are scsi and the ide layer, (virtio_blk
> support has already been accidentally disabled for a while), and getting
> rid of it allows to to shrink and simply the scsi data structures.
>
> But if you want this for now lets keep scsi_sense.c in drivers/scsi
> but depend on CONFIG_BLK_SCSI_REQUEST, that is easy enough to fix up.

 It could be a stand-alone dependency, doesn't have to be BLK_SCSI_REQUEST.
 BLA_SCSI_SENSE or something would do. I don't care too much about that,
 mostly getting rid of the entire stack dependency.
>>>
>>> Aaand, I can't do this and leave it in drivers/scsi because of 
>>> drivers/Makefile:
>>>
>>> obj-$(CONFIG_SCSI)  += scsi/
>>>
>>> So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's
>>> scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll
>>> still need to move the code from drivers/scsi/ to block/. Is this
>>> okay?
>>
>> Ugh, so that would necessitate a change there too. As I said before,
>> I don't really care where it lives. I know the SCSI folks seem bothered
>> by moving it, but in reality, it's not like this stuff will likely ever
>> really change. Of the two choices (select entire SCSI stack, or just move
>> this little bit), I know what I would consider the saner option...
>>
> 
> or option 3:
> 
> obj-y   += scsi/
> 
> so that make descends into drivers/scsi/ and then builds whatever is needed,
> depending on individual kconfig options.

Right, that was the initial option, the later two are the other options.

-- 
Jens Axboe

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Randy Dunlap]

On 05/23/2018 02:14 PM, Jens Axboe wrote:
> On 5/23/18 2:52 PM, Kees Cook wrote:
>> On Wed, May 23, 2018 at 7:31 AM, Jens Axboe  wrote:
>>> On 5/23/18 8:25 AM, Christoph Hellwig wrote:
 On Wed, May 23, 2018 at 08:13:56AM -0600, Jens Axboe wrote:
>> Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
>> drivers/scsi/Makefile as:
>>
>> obj-$(CONFIG_BLK_SCSI_REQUEST)+= scsi_sense.o
>>
>> Every place I want to use the code is already covered by
>> CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
>> put the .c file. :P
>
> I think this is so much saner than a SCSI select or dependency, so I'll
> have to disagree with Martin and Christoph. Just put it in drivers/scsi,
> if it's the location they care about.

 I actually plan to remove CONFIG_BLK_SCSI_REQUEST in a merge window
 or two.  The only users are scsi and the ide layer, (virtio_blk
 support has already been accidentally disabled for a while), and getting
 rid of it allows to to shrink and simply the scsi data structures.

 But if you want this for now lets keep scsi_sense.c in drivers/scsi
 but depend on CONFIG_BLK_SCSI_REQUEST, that is easy enough to fix up.
>>>
>>> It could be a stand-alone dependency, doesn't have to be BLK_SCSI_REQUEST.
>>> BLA_SCSI_SENSE or something would do. I don't care too much about that,
>>> mostly getting rid of the entire stack dependency.
>>
>> Aaand, I can't do this and leave it in drivers/scsi because of 
>> drivers/Makefile:
>>
>> obj-$(CONFIG_SCSI)  += scsi/
>>
>> So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's
>> scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll
>> still need to move the code from drivers/scsi/ to block/. Is this
>> okay?
> 
> Ugh, so that would necessitate a change there too. As I said before,
> I don't really care where it lives. I know the SCSI folks seem bothered
> by moving it, but in reality, it's not like this stuff will likely ever
> really change. Of the two choices (select entire SCSI stack, or just move
> this little bit), I know what I would consider the saner option...
> 

or option 3:

obj-y   += scsi/

so that make descends into drivers/scsi/ and then builds whatever is needed,
depending on individual kconfig options.

-- 
~Randy

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

On Wed, May 23, 2018 at 2:06 PM, Martin K. Petersen
 wrote:
>
> Kees,
>
>> obj-$(CONFIG_SCSI)  += scsi/
>>
>> So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's
>> scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll
>> still need to move the code from drivers/scsi/ to block/. Is this
>> okay?
>
> The reason this sucks is that scsi_normalize_sense() is an inherent core
> feature in the SCSI layer. Dealing with sense data for ioctls is just a
> fringe use case.

True, though I'm finding other robustness issues in the CDROM code.
They're probably all insane corner cases, but it seems like it'd be
nice to just fix them:

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 3522d2cae1b6..7726c8618c30 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -,9 +2223,12 @@ static int cdrom_read_cdda_bpc(struct
cdrom_device_info *cdi, __u8 __user *ubuf,

blk_execute_rq(q, cdi->disk, rq, 0);
if (scsi_req(rq)->result) {
-   struct request_sense *s = req->sense;
+   struct scsi_sense_hdr sshdr;
+
ret = -EIO;
-   cdi->last_sense = s->sense_key;
+   scsi_normalize_sense(req->sense, req->sense_len,
+);
+   cdi->last_sense = sshdr.sense_key;
}

if (blk_rq_unmap_user(bio))

This code wasn't checking req->sense_len, for example. It'll just get
stale data at worst case, but it's still ugly, especially since we
have a solution to do it right.

> I don't want to get too hung up on what goes where. But architecturally
> it really irks me to move a core piece of SCSI state machine
> functionality out of the subsystem to accommodate ioctl handling.

It looks like there is more in block/scsi_ioctl.c than just ioctl
handling, which is why I put the function in there originally.
Honestly, it's almost so small I could make it a static inline. :P

> I'm traveling today so I probably won't get a chance to look closely
> until tomorrow morning.

No worries; thanks for looking at it!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Jens Axboe]

On 5/23/18 2:52 PM, Kees Cook wrote:
> On Wed, May 23, 2018 at 7:31 AM, Jens Axboe  wrote:
>> On 5/23/18 8:25 AM, Christoph Hellwig wrote:
>>> On Wed, May 23, 2018 at 08:13:56AM -0600, Jens Axboe wrote:
> Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
> drivers/scsi/Makefile as:
>
> obj-$(CONFIG_BLK_SCSI_REQUEST)+= scsi_sense.o
>
> Every place I want to use the code is already covered by
> CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
> put the .c file. :P

 I think this is so much saner than a SCSI select or dependency, so I'll
 have to disagree with Martin and Christoph. Just put it in drivers/scsi,
 if it's the location they care about.
>>>
>>> I actually plan to remove CONFIG_BLK_SCSI_REQUEST in a merge window
>>> or two.  The only users are scsi and the ide layer, (virtio_blk
>>> support has already been accidentally disabled for a while), and getting
>>> rid of it allows to to shrink and simply the scsi data structures.
>>>
>>> But if you want this for now lets keep scsi_sense.c in drivers/scsi
>>> but depend on CONFIG_BLK_SCSI_REQUEST, that is easy enough to fix up.
>>
>> It could be a stand-alone dependency, doesn't have to be BLK_SCSI_REQUEST.
>> BLA_SCSI_SENSE or something would do. I don't care too much about that,
>> mostly getting rid of the entire stack dependency.
> 
> Aaand, I can't do this and leave it in drivers/scsi because of 
> drivers/Makefile:
> 
> obj-$(CONFIG_SCSI)  += scsi/
> 
> So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's
> scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll
> still need to move the code from drivers/scsi/ to block/. Is this
> okay?

Ugh, so that would necessitate a change there too. As I said before,
I don't really care where it lives. I know the SCSI folks seem bothered
by moving it, but in reality, it's not like this stuff will likely ever
really change. Of the two choices (select entire SCSI stack, or just move
this little bit), I know what I would consider the saner option...

-- 
Jens Axboe

Re: [PATCH 6/6] scsi: Check sense buffer size at build time

[Kees Cook]

On Wed, May 23, 2018 at 1:25 AM, Sergei Shtylyov
 wrote:
> Hello!
>
> On 5/22/2018 9:15 PM, Kees Cook wrote:
>
>> To avoid introducing problems like those fixed in commit f7068114d45e
>> ("sr: pass down correctly sized SCSI sense buffer"), this creates a macro
>> wrapper for scsi_execute() that verifies the size of the sense buffer
>> similar to what was done for command string sizes in commit 3756f6401c30
>> ("exec: avoid gcc-8 warning for get_task_comm").
>>
>> Another solution could be to add another argument to scsi_execute(),
>> but this function already takes a lot of arguments and Jens was not fond
>> of that approach. As there was only a pair of dynamically allocated sense
>> buffers, this also moves those 96 bytes onto the stack to avoid triggering
>> the sizeof() check.
>>
>> Signed-off-by: Kees Cook 
>> ---
>>   drivers/scsi/scsi_lib.c|  6 +++---
>>   include/scsi/scsi_device.h | 12 +++-
>>   2 files changed, 14 insertions(+), 4 deletions(-)
>>
> [...]
>>
>> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
>> index 7ae177c8e399..1bb87b6c0ad2 100644
>> --- a/include/scsi/scsi_device.h
>> +++ b/include/scsi/scsi_device.h
>> @@ -426,11 +426,21 @@ extern const char *scsi_device_state_name(enum
>> scsi_device_state);
>>   extern int scsi_is_sdev_device(const struct device *);
>>   extern int scsi_is_target_device(const struct device *);
>>   extern void scsi_sanitize_inquiry_string(unsigned char *s, int len);
>> -extern int scsi_execute(struct scsi_device *sdev, const unsigned char
>> *cmd,
>> +extern int __scsi_execute(struct scsi_device *sdev, const unsigned char
>> *cmd,
>> int data_direction, void *buffer, unsigned
>> bufflen,
>> unsigned char *sense, struct scsi_sense_hdr
>> *sshdr,
>> int timeout, int retries, u64 flags,
>> req_flags_t rq_flags, int *resid);
>> +/* Make sure any sense buffer is the correct size. */
>> +#define scsi_execute(sdev, cmd, data_direction, buffer, bufflen, sense,
>> \
>> +sshdr, timeout, retries, flags, rq_flags, resid)   \
>> +({ \
>> +   BUILD_BUG_ON((sense) != NULL && \
>> +sizeof(sense) != SCSI_SENSE_BUFFERSIZE);   \
>
>
>This would only check the size of the 'sense' pointer, no?

Correct. Passing in a variable that was declared as:

char sense[SCSI_SENSE_BUFFERSIZE];

would pass this check. Dynamically allocated would not (and we don't
have any cases of that left after the prior patch in this series), and
it should cast improper casts.

If people don't like this bit of robustness vs complexity/limit, I'm
happy to leave it off the series.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Martin K. Petersen]

Kees,

> obj-$(CONFIG_SCSI)  += scsi/
>
> So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's
> scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll
> still need to move the code from drivers/scsi/ to block/. Is this
> okay?

The reason this sucks is that scsi_normalize_sense() is an inherent core
feature in the SCSI layer. Dealing with sense data for ioctls is just a
fringe use case.

I don't want to get too hung up on what goes where. But architecturally
it really irks me to move a core piece of SCSI state machine
functionality out of the subsystem to accommodate ioctl handling.

I'm traveling today so I probably won't get a chance to look closely
until tomorrow morning.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

On Wed, May 23, 2018 at 7:31 AM, Jens Axboe  wrote:
> On 5/23/18 8:25 AM, Christoph Hellwig wrote:
>> On Wed, May 23, 2018 at 08:13:56AM -0600, Jens Axboe wrote:
 Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
 drivers/scsi/Makefile as:

 obj-$(CONFIG_BLK_SCSI_REQUEST)+= scsi_sense.o

 Every place I want to use the code is already covered by
 CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
 put the .c file. :P
>>>
>>> I think this is so much saner than a SCSI select or dependency, so I'll
>>> have to disagree with Martin and Christoph. Just put it in drivers/scsi,
>>> if it's the location they care about.
>>
>> I actually plan to remove CONFIG_BLK_SCSI_REQUEST in a merge window
>> or two.  The only users are scsi and the ide layer, (virtio_blk
>> support has already been accidentally disabled for a while), and getting
>> rid of it allows to to shrink and simply the scsi data structures.
>>
>> But if you want this for now lets keep scsi_sense.c in drivers/scsi
>> but depend on CONFIG_BLK_SCSI_REQUEST, that is easy enough to fix up.
>
> It could be a stand-alone dependency, doesn't have to be BLK_SCSI_REQUEST.
> BLA_SCSI_SENSE or something would do. I don't care too much about that,
> mostly getting rid of the entire stack dependency.

Aaand, I can't do this and leave it in drivers/scsi because of drivers/Makefile:

obj-$(CONFIG_SCSI)  += scsi/

So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's
scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll
still need to move the code from drivers/scsi/ to block/. Is this
okay?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH net-next 1/2] cxgb4: change the port capability bits definition

[David Miller]

From: Ganesh Goudar 
Date: Wed, 23 May 2018 20:02:58 +0530

> MDI Port Capabilities bit definitions were inconsistent with
> regard to the MDI enum values. 2 bits used to define MDI in
> the port capabilities are not really separable, it's a 2-bit
> field with 4 different values. Change the port capability bit
> definitions to be "AUTO" and "STRAIGHT" in order to get them
> to line up with the enum's.
> 
> Signed-off-by: Casey Leedom 
> Signed-off-by: Ganesh Goudar 

Applied.

Re: [PATCH net-next 2/2] cxgb4: do L1 config when module is inserted

[David Miller]

From: Ganesh Goudar 
Date: Wed, 23 May 2018 20:03:33 +0530

> trigger an L1 configure operation when a transceiver module
> is inserted in order to cause current "sticky" options like
> Requested Forward Error Correction to be reapplied.
> 
> Signed-off-by: Casey Leedom 
> Signed-off-by: Ganesh Goudar 

Applied.

Re: [PATCH RFC 1/3] scsi: ufs: set the device reference clock setting

[Rob Herring]

On Tue, May 22, 2018 at 09:51:38AM +0530, Sayali Lokhande wrote:
> From: Subhash Jadavani 
> 
> UFS host supplies the reference clock to UFS device and UFS device
> specification allows host to provide one of the 4 frequencies (19.2 MHz,
> 26 MHz, 38.4 MHz, 52 MHz) for reference clock. Host should set the
> device reference clock frequency setting in the device based on what
> frequency it is supplying to UFS device.
> 
> Signed-off-by: Subhash Jadavani 
> [c...@codeaurora.org: Resolved trivial merge conflicts]
> Signed-off-by: Can Guo 
> Signed-off-by: Sayali Lokhande 
> ---
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  |  8 +++
>  drivers/scsi/ufs/ufs.h |  9 
>  drivers/scsi/ufs/ufshcd-pltfrm.c   | 20 
>  drivers/scsi/ufs/ufshcd.c  | 60 
> ++
>  drivers/scsi/ufs/ufshcd.h  |  2 +
>  5 files changed, 99 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt 
> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> index c39dfef..ac94220 100644
> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> @@ -41,6 +41,12 @@ Optional properties:
>  -lanes-per-direction : number of lanes available per direction - either 1 or 
> 2.
> Note that it is assume same number of lanes is used 
> both
> directions at once. If not specified, default is 2 
> lanes per direction.
> +- dev-ref-clk-freq   : Specify the device reference clock frequency, must be 
> one of the following:
> +   0: 19.2 MHz
> +   1: 26 MHz
> +   2: 38.4 MHz
> +   3: 52 MHz
> +   Defaults to 26 MHz if not specified.

You already have "ref_clk", can't you just read its frequency?

Rob

Re: [PATCH RFC] sr: mark the device as changed when burning a CD

[Maurizio Lombardi]

Dne 23.5.2018 v 16:42 Jens Axboe napsal(a):
> On 5/23/18 3:19 AM, Maurizio Lombardi wrote:
>>
>>
>> Dne 22.5.2018 v 16:47 Jens Axboe napsal(a):
>>> It's been many years, but back in the day the program writing the cd
>>> would eject the disc once done. This of course forces a reload of
>>> the toc and clearing of the flag. What program is this? Seems like
>>> it should probably eject when it's done.
>>
>> They are using wodim to burn the CDs on their servers.
>> The problem is that they do not want the CD to be ejected because their 
>> drives
>> lack a motorized tray, thus requiring manual intervention which they would 
>> like to avoid.
> 
> I took a quick look at it, man that sr driver needs a bit of love :-)
> 
> Anyway, I wonder if something like the below would work. Check for
> a close track command in the sr completion handler, and flag the media
> as changed if we see one. Totally untested...
> 
> 
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 3f3cb72e0c0c..48f0d7a096db 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -328,6 +328,9 @@ static int sr_done(struct scsi_cmnd *SCpnt)
>   scmd_printk(KERN_INFO, SCpnt, "done: %x\n", result);
>  #endif
>  
> + if (SCpnt->cmnd[0] == GPCMD_CLOSE_TRACK)
> + cd->device->changed = 1;
> +
>   /*
>* Handle MEDIUM ERRORs or VOLUME OVERFLOWs that indicate partial
>* success.  Since this is a relatively rare error condition, no
> 

Nice! I will test it ASAP and I'll let you know.

Thanks,
Maurizio

Re: [PATCH RFC] sr: mark the device as changed when burning a CD

[Jens Axboe]

On 5/23/18 3:19 AM, Maurizio Lombardi wrote:
> 
> 
> Dne 22.5.2018 v 16:47 Jens Axboe napsal(a):
>> It's been many years, but back in the day the program writing the cd
>> would eject the disc once done. This of course forces a reload of
>> the toc and clearing of the flag. What program is this? Seems like
>> it should probably eject when it's done.
> 
> They are using wodim to burn the CDs on their servers.
> The problem is that they do not want the CD to be ejected because their drives
> lack a motorized tray, thus requiring manual intervention which they would 
> like to avoid.

I took a quick look at it, man that sr driver needs a bit of love :-)

Anyway, I wonder if something like the below would work. Check for
a close track command in the sr completion handler, and flag the media
as changed if we see one. Totally untested...


diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 3f3cb72e0c0c..48f0d7a096db 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -328,6 +328,9 @@ static int sr_done(struct scsi_cmnd *SCpnt)
scmd_printk(KERN_INFO, SCpnt, "done: %x\n", result);
 #endif
 
+   if (SCpnt->cmnd[0] == GPCMD_CLOSE_TRACK)
+   cd->device->changed = 1;
+
/*
 * Handle MEDIUM ERRORs or VOLUME OVERFLOWs that indicate partial
 * success.  Since this is a relatively rare error condition, no

-- 
Jens Axboe

[PATCH net-next 2/2] cxgb4: do L1 config when module is inserted

[Ganesh Goudar]

trigger an L1 configure operation when a transceiver module
is inserted in order to cause current "sticky" options like
Requested Forward Error Correction to be reapplied.

Signed-off-by: Casey Leedom 
Signed-off-by: Ganesh Goudar 
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4.h  | 26 ++--
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 11 +--
 drivers/net/ethernet/chelsio/cxgb4/t4_hw.c  | 40 +
 3 files changed, 65 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h 
b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
index 211086b..0f305d9 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
@@ -491,6 +491,9 @@ struct link_config {
 
unsigned char  link_ok;  /* link up? */
unsigned char  link_down_rc; /* link down reason */
+
+   bool new_module; /* ->OS Transceiver Module inserted */
+   bool redo_l1cfg; /* ->CC redo current "sticky" L1 CFG */
 };
 
 #define FW_LEN16(fw_struct) FW_CMD_LEN16_V(sizeof(fw_struct) / 16)
@@ -1324,7 +1327,7 @@ static inline unsigned int qtimer_val(const struct 
adapter *adap,
 extern char cxgb4_driver_name[];
 extern const char cxgb4_driver_version[];
 
-void t4_os_portmod_changed(const struct adapter *adap, int port_id);
+void t4_os_portmod_changed(struct adapter *adap, int port_id);
 void t4_os_link_changed(struct adapter *adap, int port_id, int link_stat);
 
 void t4_free_sge_resources(struct adapter *adap);
@@ -1505,8 +1508,25 @@ void t4_intr_disable(struct adapter *adapter);
 int t4_slow_intr_handler(struct adapter *adapter);
 
 int t4_wait_dev_ready(void __iomem *regs);
-int t4_link_l1cfg(struct adapter *adap, unsigned int mbox, unsigned int port,
- struct link_config *lc);
+
+int t4_link_l1cfg_core(struct adapter *adap, unsigned int mbox,
+  unsigned int port, struct link_config *lc,
+  bool sleep_ok, int timeout);
+
+static inline int t4_link_l1cfg(struct adapter *adapter, unsigned int mbox,
+   unsigned int port, struct link_config *lc)
+{
+   return t4_link_l1cfg_core(adapter, mbox, port, lc,
+ true, FW_CMD_MAX_TIMEOUT);
+}
+
+static inline int t4_link_l1cfg_ns(struct adapter *adapter, unsigned int mbox,
+  unsigned int port, struct link_config *lc)
+{
+   return t4_link_l1cfg_core(adapter, mbox, port, lc,
+ false, FW_CMD_MAX_TIMEOUT);
+}
+
 int t4_restart_aneg(struct adapter *adap, unsigned int mbox, unsigned int 
port);
 
 u32 t4_read_pcie_cfg4(struct adapter *adap, int reg);
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c 
b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 130d1ee..513e1d3 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -301,14 +301,14 @@ void t4_os_link_changed(struct adapter *adapter, int 
port_id, int link_stat)
}
 }
 
-void t4_os_portmod_changed(const struct adapter *adap, int port_id)
+void t4_os_portmod_changed(struct adapter *adap, int port_id)
 {
static const char *mod_str[] = {
NULL, "LR", "SR", "ER", "passive DA", "active DA", "LRM"
};
 
-   const struct net_device *dev = adap->port[port_id];
-   const struct port_info *pi = netdev_priv(dev);
+   struct net_device *dev = adap->port[port_id];
+   struct port_info *pi = netdev_priv(dev);
 
if (pi->mod_type == FW_PORT_MOD_TYPE_NONE)
netdev_info(dev, "port module unplugged\n");
@@ -325,6 +325,11 @@ void t4_os_portmod_changed(const struct adapter *adap, int 
port_id)
else
netdev_info(dev, "%s: unknown module type %d inserted\n",
dev->name, pi->mod_type);
+
+   /* If the interface is running, then we'll need any "sticky" Link
+* Parameters redone with a new Transceiver Module.
+*/
+   pi->link_cfg.redo_l1cfg = netif_running(dev);
 }
 
 int dbfifo_int_thresh = 10; /* 10 == 640 entry threshold */
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c 
b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
index 537ed07..704f696 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
@@ -4058,14 +4058,16 @@ static inline fw_port_cap32_t cc_to_fwcap_fec(enum 
cc_fec cc_fec)
  * - If auto-negotiation is off set the MAC to the proper speed/duplex/FC,
  *   otherwise do it later based on the outcome of auto-negotiation.
  */
-int t4_link_l1cfg(struct adapter *adapter, unsigned int mbox,
- unsigned int port, struct link_config *lc)
+int t4_link_l1cfg_core(struct adapter *adapter, unsigned int mbox,
+  unsigned int port, struct link_config *lc,
+ 

[PATCH net-next 1/2] cxgb4: change the port capability bits definition

[Ganesh Goudar]

MDI Port Capabilities bit definitions were inconsistent with
regard to the MDI enum values. 2 bits used to define MDI in
the port capabilities are not really separable, it's a 2-bit
field with 4 different values. Change the port capability bit
definitions to be "AUTO" and "STRAIGHT" in order to get them
to line up with the enum's.

Signed-off-by: Casey Leedom 
Signed-off-by: Ganesh Goudar 
---
 drivers/net/ethernet/chelsio/cxgb4/t4_hw.c | 4 ++--
 drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h  | 8 
 drivers/net/ethernet/chelsio/cxgb4vf/t4vf_hw.c | 2 +-
 drivers/scsi/csiostor/csio_hw.c| 2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c 
b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
index df5e7c7..537ed07 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
@@ -3941,8 +3941,8 @@ static fw_port_cap32_t fwcaps16_to_caps32(fw_port_cap16_t 
caps16)
CAP16_TO_CAP32(FC_RX);
CAP16_TO_CAP32(FC_TX);
CAP16_TO_CAP32(ANEG);
-   CAP16_TO_CAP32(MDIX);
CAP16_TO_CAP32(MDIAUTO);
+   CAP16_TO_CAP32(MDISTRAIGHT);
CAP16_TO_CAP32(FEC_RS);
CAP16_TO_CAP32(FEC_BASER_RS);
CAP16_TO_CAP32(802_3_PAUSE);
@@ -3982,8 +3982,8 @@ static fw_port_cap16_t fwcaps32_to_caps16(fw_port_cap32_t 
caps32)
CAP32_TO_CAP16(802_3_PAUSE);
CAP32_TO_CAP16(802_3_ASM_DIR);
CAP32_TO_CAP16(ANEG);
-   CAP32_TO_CAP16(MDIX);
CAP32_TO_CAP16(MDIAUTO);
+   CAP32_TO_CAP16(MDISTRAIGHT);
CAP32_TO_CAP16(FEC_RS);
CAP32_TO_CAP16(FEC_BASER_RS);
 
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h 
b/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h
index e6b2e95..2d91480 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4fw_api.h
@@ -2471,8 +2471,8 @@ enum fw_port_cap {
FW_PORT_CAP_FC_RX   = 0x0040,
FW_PORT_CAP_FC_TX   = 0x0080,
FW_PORT_CAP_ANEG= 0x0100,
-   FW_PORT_CAP_MDIX= 0x0200,
-   FW_PORT_CAP_MDIAUTO = 0x0400,
+   FW_PORT_CAP_MDIAUTO = 0x0200,
+   FW_PORT_CAP_MDISTRAIGHT = 0x0400,
FW_PORT_CAP_FEC_RS  = 0x0800,
FW_PORT_CAP_FEC_BASER_RS= 0x1000,
FW_PORT_CAP_FEC_RESERVED= 0x2000,
@@ -2515,8 +2515,8 @@ enum fw_port_mdi {
 #defineFW_PORT_CAP32_802_3_PAUSE   0x0004UL
 #defineFW_PORT_CAP32_802_3_ASM_DIR 0x0008UL
 #defineFW_PORT_CAP32_ANEG  0x0010UL
-#defineFW_PORT_CAP32_MDIX  0x0020UL
-#defineFW_PORT_CAP32_MDIAUTO   0x0040UL
+#defineFW_PORT_CAP32_MDIAUTO   0x0020UL
+#defineFW_PORT_CAP32_MDISTRAIGHT   0x0040UL
 #defineFW_PORT_CAP32_FEC_RS0x0080UL
 #defineFW_PORT_CAP32_FEC_BASER_RS  0x0100UL
 #defineFW_PORT_CAP32_FEC_RESERVED1 0x0200UL
diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/t4vf_hw.c 
b/drivers/net/ethernet/chelsio/cxgb4vf/t4vf_hw.c
index 798695b..3017f78 100644
--- a/drivers/net/ethernet/chelsio/cxgb4vf/t4vf_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb4vf/t4vf_hw.c
@@ -341,8 +341,8 @@ static fw_port_cap32_t fwcaps16_to_caps32(fw_port_cap16_t 
caps16)
CAP16_TO_CAP32(FC_RX);
CAP16_TO_CAP32(FC_TX);
CAP16_TO_CAP32(ANEG);
-   CAP16_TO_CAP32(MDIX);
CAP16_TO_CAP32(MDIAUTO);
+   CAP16_TO_CAP32(MDISTRAIGHT);
CAP16_TO_CAP32(FEC_RS);
CAP16_TO_CAP32(FEC_BASER_RS);
CAP16_TO_CAP32(802_3_PAUSE);
diff --git a/drivers/scsi/csiostor/csio_hw.c b/drivers/scsi/csiostor/csio_hw.c
index 96bbb82..a10cf25 100644
--- a/drivers/scsi/csiostor/csio_hw.c
+++ b/drivers/scsi/csiostor/csio_hw.c
@@ -1500,8 +1500,8 @@ fw_port_cap32_t fwcaps16_to_caps32(fw_port_cap16_t caps16)
CAP16_TO_CAP32(FC_RX);
CAP16_TO_CAP32(FC_TX);
CAP16_TO_CAP32(ANEG);
-   CAP16_TO_CAP32(MDIX);
CAP16_TO_CAP32(MDIAUTO);
+   CAP16_TO_CAP32(MDISTRAIGHT);
CAP16_TO_CAP32(FEC_RS);
CAP16_TO_CAP32(FEC_BASER_RS);
CAP16_TO_CAP32(802_3_PAUSE);
-- 
2.1.0

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Jens Axboe]

On 5/23/18 8:25 AM, Christoph Hellwig wrote:
> On Wed, May 23, 2018 at 08:13:56AM -0600, Jens Axboe wrote:
>>> Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
>>> drivers/scsi/Makefile as:
>>>
>>> obj-$(CONFIG_BLK_SCSI_REQUEST)+= scsi_sense.o
>>>
>>> Every place I want to use the code is already covered by
>>> CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
>>> put the .c file. :P
>>
>> I think this is so much saner than a SCSI select or dependency, so I'll
>> have to disagree with Martin and Christoph. Just put it in drivers/scsi,
>> if it's the location they care about.
> 
> I actually plan to remove CONFIG_BLK_SCSI_REQUEST in a merge window
> or two.  The only users are scsi and the ide layer, (virtio_blk
> support has already been accidentally disabled for a while), and getting
> rid of it allows to to shrink and simply the scsi data structures.
> 
> But if you want this for now lets keep scsi_sense.c in drivers/scsi
> but depend on CONFIG_BLK_SCSI_REQUEST, that is easy enough to fix up.

It could be a stand-alone dependency, doesn't have to be BLK_SCSI_REQUEST.
BLA_SCSI_SENSE or something would do. I don't care too much about that,
mostly getting rid of the entire stack dependency.

-- 
Jens Axboe

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Christoph Hellwig]

On Wed, May 23, 2018 at 08:13:56AM -0600, Jens Axboe wrote:
> > Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
> > drivers/scsi/Makefile as:
> > 
> > obj-$(CONFIG_BLK_SCSI_REQUEST)+= scsi_sense.o
> > 
> > Every place I want to use the code is already covered by
> > CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
> > put the .c file. :P
> 
> I think this is so much saner than a SCSI select or dependency, so I'll
> have to disagree with Martin and Christoph. Just put it in drivers/scsi,
> if it's the location they care about.

I actually plan to remove CONFIG_BLK_SCSI_REQUEST in a merge window
or two.  The only users are scsi and the ide layer, (virtio_blk
support has already been accidentally disabled for a while), and getting
rid of it allows to to shrink and simply the scsi data structures.

But if you want this for now lets keep scsi_sense.c in drivers/scsi
but depend on CONFIG_BLK_SCSI_REQUEST, that is easy enough to fix up.

Re: [PATCH 13/14] blk-mq: Remove generation seqeunce

[Keith Busch]

On Wed, May 23, 2018 at 02:19:40PM +0200, Christoph Hellwig wrote:
> -static void hctx_unlock(struct blk_mq_hw_ctx *hctx, int srcu_idx)
> - __releases(hctx->srcu)
> -{
> - if (!(hctx->flags & BLK_MQ_F_BLOCKING))
> - rcu_read_unlock();
> - else
> - srcu_read_unlock(hctx->srcu, srcu_idx);
> -}
> -
> -static void hctx_lock(struct blk_mq_hw_ctx *hctx, int *srcu_idx)
> - __acquires(hctx->srcu)
> -{
> - if (!(hctx->flags & BLK_MQ_F_BLOCKING)) {
> - /* shut up gcc false positive */
> - *srcu_idx = 0;
> - rcu_read_lock();
> - } else
> - *srcu_idx = srcu_read_lock(hctx->srcu);
> -}

I may have too ambitious on removing hctx_lock. That's actually nothing
to do with the completion issues, but it is necessary for quiesce to
work reliably. Otherwise, I still think there is some promise to the
reset of the approach.

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Jens Axboe]

On 5/22/18 5:49 PM, Kees Cook wrote:
> On Tue, May 22, 2018 at 4:42 PM, Jens Axboe  wrote:
>> On May 22, 2018, at 5:31 PM, Kees Cook  wrote:
>>>
 On Tue, May 22, 2018 at 12:16 PM, Jens Axboe  wrote:
> On 5/22/18 1:13 PM, Christoph Hellwig wrote:
>> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
>> I think Martin and Christoph are objecting to moving the code to
>> block/scsi_ioctl.h. I don't care too much about where the code is, but
>> think it would be nice to have the definitions in a separate header. But
>> if they prefer just pulling in all of SCSI for it, well then I guess
>> it's pointless to move the header bits. Seems very heavy handed to me,
>> though.
>
> It might be heavy handed for the 3 remaining users of drivers/ide,

 Brutal :-)
>>>
>>> Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c
>>> too. Is this okay under the same considerations?
>>
>> This is going from somewhat crazy to pretty nuts, imho. I guess in practical 
>> terms it doesn’t matter that much, since most folks would be using sr. I 
>> still think a split would be vastly superior. Just keep the scsi code in 
>> drivers/scsi/ and make it independently selectable.
> 
> I had originally tied it to BLK_SCSI_REQUEST. Logically speaking,
> sense buffers are part of the request, and the CONFIG work is already
> done. This is roughly what I tried to do before, since scsi_ioctl.c is
> the only code pulled in for CONFIG_BLK_SCSI_REQUEST:
> 
> $ git grep BLK_SCSI_REQUEST | grep Makefile
> block/Makefile:obj-$(CONFIG_BLK_SCSI_REQUEST)   += scsi_ioctl.o
> 
> Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
> drivers/scsi/Makefile as:
> 
> obj-$(CONFIG_BLK_SCSI_REQUEST)+= scsi_sense.o
> 
> Every place I want to use the code is already covered by
> CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
> put the .c file. :P

I think this is so much saner than a SCSI select or dependency, so I'll
have to disagree with Martin and Christoph. Just put it in drivers/scsi,
if it's the location they care about.

-- 
Jens Axboe

Re: [PATCH 12/14] blk-mq: Fix timeout and state order

[Hannes Reinecke]

On 05/23/2018 02:19 PM, Christoph Hellwig wrote:

From: Keith Busch 

The block layer had been setting the state to in-flight prior to updating
the timer. This is the wrong order since the timeout handler could observe
the in-flight state with the older timeout, believing the request had
expired when in fact it is just getting started.

Signed-off-by: Keith Busch 
---
  block/blk-mq.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 22ab2a148a9b..614cb03732ed 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -697,8 +697,8 @@ void blk_mq_start_request(struct request *rq)
preempt_disable();
write_seqcount_begin(>gstate_seq);
  
-	blk_mq_rq_update_state(rq, MQ_RQ_IN_FLIGHT);

blk_add_timer(rq);
+   blk_mq_rq_update_state(rq, MQ_RQ_IN_FLIGHT);
  
  	write_seqcount_end(>gstate_seq);

preempt_enable();


Reviewed-by: Hannes Reinecke 

Cheers,

Hannes

Re: [PATCH 10/14] block: remove BLK_EH_HANDLED

[Hannes Reinecke]

On 05/23/2018 02:19 PM, Christoph Hellwig wrote:

Signed-off-by: Christoph Hellwig 
---
  Documentation/scsi/scsi_eh.txt | 11 ---
  block/blk-mq.c |  3 ---
  block/blk-timeout.c|  3 ---
  include/linux/blkdev.h |  1 -
  4 files changed, 18 deletions(-)


Reviewed-by: Hannes Reinecke 

Cheers,

Hannes

Re: [PATCH 09/14] libiscsi: don't try to bypass SCSI EH

[Hannes Reinecke]

On 05/23/2018 02:19 PM, Christoph Hellwig wrote:

libiscsi is the only SCSI code that return BLK_EH_HANDLED, thus trying to
bypass the normal SCSI EH code.  We are going to remove this return value
at the block layer, and at least from a quick look it doesn't look too
harmful to try to send an abort for these cases, especially as the first
one should not actually be possible.  If this doesn't work out iscsi
will probably need its own eh_strategy_handler instead to just do the
right thing.

Signed-off-by: Christoph Hellwig 
---
  drivers/scsi/libiscsi.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index eee43ba83a60..71bdc0b52cf9 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1982,7 +1982,7 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct 
scsi_cmnd *sc)
 * Raced with completion. Blk layer has taken ownership
 * so let timeout code complete it now.
 */
-   rc = BLK_EH_HANDLED;
+   rc = BLK_EH_DONE;
goto done;
}
  
@@ -1997,7 +1997,7 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)

if (unlikely(system_state != SYSTEM_RUNNING)) {
sc->result = DID_NO_CONNECT << 16;
ISCSI_DBG_EH(session, "sc on shutdown, handled\n");
-   rc = BLK_EH_HANDLED;
+   rc = BLK_EH_DONE;
goto done;
}
/*

That should be okay, as these two returns are the pathological case 
where the command has already completed before the timeout handler got 
invoked.

IE the very same issue which triggered this patchset in the first place.

Reviewed-by: Hannes Reinecke 

Cheers,

Hannes

Re: [PATCH 11/14] block: document the blk_eh_timer_return values

[Hannes Reinecke]

On 05/23/2018 02:19 PM, Christoph Hellwig wrote:

Signed-off-by: Christoph Hellwig 
---
  include/linux/blkdev.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 3d9d4da4dedd..3815d9dcfbe0 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -337,8 +337,8 @@ typedef int (init_rq_fn)(struct request_queue *, struct 
request *, gfp_t);
  typedef void (exit_rq_fn)(struct request_queue *, struct request *);
  
  enum blk_eh_timer_return {

-   BLK_EH_DONE,
-   BLK_EH_RESET_TIMER,
+   BLK_EH_DONE,/* drivers has completed the command */
+   BLK_EH_RESET_TIMER, /* reset timer and try again */
  };
  
  typedef enum blk_eh_timer_return (rq_timed_out_fn)(struct request *);



Reviewed-by: Hannes Reinecke 

Cheers,

Hannes

Re: [PATCH 08/14] mmc: complete requests from ->timeout

[Hannes Reinecke]

On 05/23/2018 02:19 PM, Christoph Hellwig wrote:

By completing the request entirely in the driver we can remove the
BLK_EH_HANDLED return value and thus the split responsibility between the
driver and the block layer that has been causing trouble.

[While this keeps existing behavior it seems to mismatch the comment,
  maintainers please chime in!]

Signed-off-by: Christoph Hellwig 
---
  drivers/mmc/core/queue.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index 56e9a803db21..648eb6743ed5 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -111,8 +111,9 @@ static enum blk_eh_timer_return mmc_cqe_timed_out(struct 
request *req)
__mmc_cqe_recovery_notifier(mq);
return BLK_EH_RESET_TIMER;
}
-   /* No timeout */
-   return BLK_EH_HANDLED;
+   /* No timeout (XXX: huh? comment doesn't make much sense) */
+   blk_mq_complete_request(req);
+   return BLK_EH_DONE;
default:
/* Timeout is handled by mmc core */
return BLK_EH_RESET_TIMER;


Reviewed-by: Hannes Reinecke 

Cheers,

Hannes

Re: [PATCH 07/14] scsi_transport_fc: complete requests from ->timeout

[Hannes Reinecke]

On 05/23/2018 02:19 PM, Christoph Hellwig wrote:

By completing the request entirely in the driver we can remove the
BLK_EH_HANDLED return value and thus the split responsibility between the
driver and the block layer that has been causing trouble.

Signed-off-by: Christoph Hellwig 
---
  drivers/scsi/scsi_transport_fc.c | 7 +++
  1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 90075a0ddcfe..7a9a65588a1b 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -3591,10 +3591,9 @@ fc_bsg_job_timeout(struct request *req)
}
  
  	/* the blk_end_sync_io() doesn't check the error */

-   if (!inflight)
-   return BLK_EH_DONE;
-   else
-   return BLK_EH_HANDLED;
+   if (inflight)
+   blk_mq_complete_request(req);
+   return BLK_EH_DONE;
  }
  
  /**



Reviewed-by: Hannes Reinecke 

Cheers,

Hannes

Re: [PATCH 06/14] null_blk: complete requests from ->timeout

[Hannes Reinecke]

On 05/23/2018 02:19 PM, Christoph Hellwig wrote:

By completing the request entirely in the driver we can remove the
BLK_EH_HANDLED return value and thus the split responsibility between the
driver and the block layer that has been causing trouble.

Signed-off-by: Christoph Hellwig 
---
  drivers/block/null_blk.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)


Reviewed-by: Hannes Reinecke 

Cheers,

Hannes

Re: [PATCH 05/14] mtip32xx: complete requests from ->timeout

[Hannes Reinecke]

On 05/23/2018 02:19 PM, Christoph Hellwig wrote:

By completing the request entirely in the driver we can remove the
BLK_EH_HANDLED return value and thus the split responsibility between the
driver and the block layer that has been causing trouble.

Signed-off-by: Christoph Hellwig 
---
  drivers/block/mtip32xx/mtip32xx.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c 
b/drivers/block/mtip32xx/mtip32xx.c
index 6df5b0b1517a..beace13effe4 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -3720,7 +3720,8 @@ static enum blk_eh_timer_return mtip_cmd_timeout(struct 
request *req,
struct mtip_cmd *cmd = blk_mq_rq_to_pdu(req);
  
  		cmd->status = BLK_STS_TIMEOUT;

-   return BLK_EH_HANDLED;
+   blk_mq_complete_request(req);
+   return BLK_EH_DONE;
}
  
  	if (test_bit(req->tag, dd->port->cmds_to_issue))



Reviewed-by: Hannes Reinecke 

Cheers,

Hannes

Re: [PATCH 04/14] nbd: complete requests from ->timeout

[Hannes Reinecke]

On 05/23/2018 02:19 PM, Christoph Hellwig wrote:

By completing the request entirely in the driver we can remove the
BLK_EH_HANDLED return value and thus the split responsibility between the
driver and the block layer that has been causing trouble.

Signed-off-by: Christoph Hellwig 
---
  drivers/block/nbd.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 09d9959287fe..30a851b8a99c 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -300,7 +300,7 @@ static enum blk_eh_timer_return nbd_xmit_timeout(struct 
request *req,
  
  	if (!refcount_inc_not_zero(>config_refs)) {

cmd->status = BLK_STS_TIMEOUT;
-   return BLK_EH_HANDLED;
+   goto done;
}
config = nbd->config;
  
@@ -338,8 +338,9 @@ static enum blk_eh_timer_return nbd_xmit_timeout(struct request *req,

cmd->status = BLK_STS_IOERR;
sock_shutdown(nbd);
nbd_config_put(nbd);
-
-   return BLK_EH_HANDLED;
+done:
+   blk_mq_complete_request(req);
+   return BLK_EH_DONE;
  }
  
  /*



Again, some testcase would be nice ...

Reviewed-by: Hannes Reinecke 

Cheers,

Hannes

Re: [PATCH 03/14] nvme: return BLK_EH_DONE from ->timeout

[Hannes Reinecke]

On 05/23/2018 02:19 PM, Christoph Hellwig wrote:

NVMe always completes the request before returning from ->timeout, either
by polling for it, or by disabling the controller.  Return BLK_EH_DONE so
that the block layer doesn't even try to complete it again.

Signed-off-by: Christoph Hellwig 
---
  drivers/nvme/host/pci.c| 14 +-
  drivers/nvme/host/rdma.c , |  2 +-
  drivers/nvme/target/loop.c |  2 +-
  3 files changed, 7 insertions(+), 11 deletions(-)


Is there a way of _testing_ this patch?
It looks pretty dodgy, just replacing BLK_EH_HANDLED with 
BLK_EH_NOT_HANDLED.
And, as nothing else has changed, would imply that we can do it on 
older/stable versions, too.

Which means that the original code was buggy to start with.
Hence a test here would be really beneficial.

But then, assuming you did some tests here:

Reviewed-by: Hannes Reinecke 

Cheers,

Hannes

Re: [PATCH 02/14] block: rename BLK_EH_NOT_HANDLED to BLK_EH_DONE

[Hannes Reinecke]

On 05/23/2018 02:19 PM, Christoph Hellwig wrote:

The BLK_EH_NOT_HANDLED implies nothing happen, but very often that
is not what is happening - instead the driver already completed the
command.  Fix the symbolic name to reflect that a little better.

Signed-off-by: Christoph Hellwig 
---
  Documentation/scsi/scsi_eh.txt| 4 ++--
  block/blk-mq.c| 2 +-
  block/blk-timeout.c   | 2 +-
  drivers/block/nbd.c   | 2 +-
  drivers/message/fusion/mptsas.c   | 2 +-
  drivers/s390/block/dasd.c | 6 +++---
  drivers/scsi/gdth.c   | 2 +-
  drivers/scsi/libiscsi.c   | 2 +-
  drivers/scsi/megaraid/megaraid_sas_base.c | 2 +-
  drivers/scsi/mvumi.c  | 2 +-
  drivers/scsi/qla4xxx/ql4_os.c | 2 +-
  drivers/scsi/scsi_error.c | 4 ++--
  drivers/scsi/scsi_transport_fc.c  | 4 ++--
  drivers/scsi/scsi_transport_srp.c | 4 ++--
  drivers/scsi/ufs/ufshcd.c | 6 +++---
  include/linux/blkdev.h| 2 +-
  include/scsi/scsi_host.h  | 2 +-
  17 files changed, 25 insertions(+), 25 deletions(-)


Reviewed-by: Hannes Reinecke 

Cheers,

Hannes

Re: [PATCH 01/14] libata: remove ata_scsi_timed_out

[Hannes Reinecke]

On 05/23/2018 02:19 PM, Christoph Hellwig wrote:

As far as I can tell this function can't even be called any more, given
that ATA implements its own eh_strategy_handler with ata_scsi_error, which
never calls ->eh_timed_out.

Signed-off-by: Christoph Hellwig 
---
  drivers/ata/libata-eh.c | 51 -
  include/linux/libata.h  |  2 --
  2 files changed, 53 deletions(-)


Reviewed-by: Hannes Reinecke 

Cheers,

Hannes

Re: BLK_EH_HANDLED

[Hannes Reinecke]

On 05/23/2018 02:19 PM, Christoph Hellwig wrote:

Hi all,

this series removes the BLK_EH_HANDLED return value, and instead places
responsibility of timed out commands entirely on the drivers.  Except
for some odd layering vilations in libiscsi this actually is
surprisingly simple.  The last two pathes contain a respin of Keith'
series to refcount struct request in the eye of timeouts.  Once the
BLK_EH_HANDLED special case is gone this actually seems the EH fixing
approach that seems the most sensible to me.


YES!

Cheers,

Hannes

Re: [PATCH 07/14] scsi_transport_fc: complete requests from ->timeout

[Johannes Thumshirn]

Looks good,
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

Re: [PATCH 06/14] null_blk: complete requests from ->timeout

[Johannes Thumshirn]

Looks good,
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

Re: [PATCH 05/14] mtip32xx: complete requests from ->timeout

[Johannes Thumshirn]

Looks good,
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

Re: [PATCH 03/14] nvme: return BLK_EH_DONE from ->timeout

[Johannes Thumshirn]

Looks good,
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

Re: [PATCH 02/14] block: rename BLK_EH_NOT_HANDLED to BLK_EH_DONE

[Johannes Thumshirn]

Looks good,
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

[PATCH 13/14] blk-mq: Remove generation seqeunce

[Christoph Hellwig]

From: Keith Busch 

This patch simplifies the timeout handling by relying on the request
reference counting to ensure the iterator is operating on an inflight
and truly timed out request. Since the reference counting prevents the
tag from being reallocated, the block layer no longer needs to prevent
drivers from completing their requests while the timeout handler is
operating on it: a driver completing a request is allowed to proceed to
the next state without additional syncronization with the block layer.

This also removes any need for generation sequence numbers since the
request lifetime is prevented from being reallocated as a new sequence
while timeout handling is operating on it.

To enables this a refcount is added to struct request so that request
users can be sure they're operating on the same request without it
changing while they're processing it.  The request's tag won't be
released for reuse until both the timeout handler and the completion
are done with it.

[hch: slight cleanups]

Signed-off-by: Keith Busch 
Signed-off-by: Christoph Hellwig 
---
 block/blk-core.c   |   6 -
 block/blk-mq-debugfs.c |   1 -
 block/blk-mq.c | 318 +++--
 block/blk-mq.h |  42 +-
 block/blk-timeout.c|   1 -
 include/linux/blkdev.h |  35 ++---
 6 files changed, 98 insertions(+), 305 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 43370faee935..cee03cad99f2 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -198,12 +198,6 @@ void blk_rq_init(struct request_queue *q, struct request 
*rq)
rq->internal_tag = -1;
rq->start_time_ns = ktime_get_ns();
rq->part = NULL;
-   seqcount_init(>gstate_seq);
-   u64_stats_init(>aborted_gstate_sync);
-   /*
-* See comment of blk_mq_init_request
-*/
-   WRITE_ONCE(rq->gstate, MQ_RQ_GEN_INC);
 }
 EXPORT_SYMBOL(blk_rq_init);
 
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 3080e18cb859..ffa622366922 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -344,7 +344,6 @@ static const char *const rqf_name[] = {
RQF_NAME(STATS),
RQF_NAME(SPECIAL_PAYLOAD),
RQF_NAME(ZONE_WRITE_LOCKED),
-   RQF_NAME(MQ_TIMEOUT_EXPIRED),
RQF_NAME(MQ_POLL_SLEPT),
 };
 #undef RQF_NAME
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 614cb03732ed..8c7b1803b7e9 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -332,6 +332,7 @@ static struct request *blk_mq_rq_ctx_init(struct 
blk_mq_alloc_data *data,
 #endif
 
data->ctx->rq_dispatched[op_is_sync(op)]++;
+   refcount_set(>ref, 1);
return rq;
 }
 
@@ -465,13 +466,27 @@ struct request *blk_mq_alloc_request_hctx(struct 
request_queue *q,
 }
 EXPORT_SYMBOL_GPL(blk_mq_alloc_request_hctx);
 
+static void __blk_mq_free_request(struct request *rq)
+{
+   struct request_queue *q = rq->q;
+   struct blk_mq_ctx *ctx = rq->mq_ctx;
+   struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, ctx->cpu);
+   const int sched_tag = rq->internal_tag;
+
+   if (rq->tag != -1)
+   blk_mq_put_tag(hctx, hctx->tags, ctx, rq->tag);
+   if (sched_tag != -1)
+   blk_mq_put_tag(hctx, hctx->sched_tags, ctx, sched_tag);
+   blk_mq_sched_restart(hctx);
+   blk_queue_exit(q);
+}
+
 void blk_mq_free_request(struct request *rq)
 {
struct request_queue *q = rq->q;
struct elevator_queue *e = q->elevator;
struct blk_mq_ctx *ctx = rq->mq_ctx;
struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, ctx->cpu);
-   const int sched_tag = rq->internal_tag;
 
if (rq->rq_flags & RQF_ELVPRIV) {
if (e && e->type->ops.mq.finish_request)
@@ -494,13 +509,9 @@ void blk_mq_free_request(struct request *rq)
if (blk_rq_rl(rq))
blk_put_rl(blk_rq_rl(rq));
 
-   blk_mq_rq_update_state(rq, MQ_RQ_IDLE);
-   if (rq->tag != -1)
-   blk_mq_put_tag(hctx, hctx->tags, ctx, rq->tag);
-   if (sched_tag != -1)
-   blk_mq_put_tag(hctx, hctx->sched_tags, ctx, sched_tag);
-   blk_mq_sched_restart(hctx);
-   blk_queue_exit(q);
+   WRITE_ONCE(rq->state, MQ_RQ_IDLE);
+   if (refcount_dec_and_test(>ref))
+   __blk_mq_free_request(rq);
 }
 EXPORT_SYMBOL_GPL(blk_mq_free_request);
 
@@ -541,14 +552,27 @@ static void __blk_mq_complete_request_remote(void *data)
rq->q->softirq_done_fn(rq);
 }
 
-static void __blk_mq_complete_request(struct request *rq)
+
+/**
+ * blk_mq_complete_request - end I/O on a request
+ * @rq:the request being processed
+ *
+ * Description:
+ * Ends all I/O on a request. It does not handle partial completions.
+ * The actual completion happens out-of-order, through a IPI handler.
+ **/
+void blk_mq_complete_request(struct request *rq)
 {
+   struct request_queue *q = rq->q;
struct blk_mq_ctx *ctx 

[PATCH 14/14] blk-mq: simplify blk_mq_rq_timed_out

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig 
---
 block/blk-mq.c | 22 --
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 8c7b1803b7e9..592bab689f8e 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -747,22 +747,16 @@ EXPORT_SYMBOL(blk_mq_tag_to_rq);
 
 static void blk_mq_rq_timed_out(struct request *req, bool reserved)
 {
-   const struct blk_mq_ops *ops = req->q->mq_ops;
-   enum blk_eh_timer_return ret = BLK_EH_RESET_TIMER;
+   if (req->q->mq_ops->timeout) {
+   enum blk_eh_timer_return ret;
 
-   if (ops->timeout)
-   ret = ops->timeout(req, reserved);
-
-   switch (ret) {
-   case BLK_EH_RESET_TIMER:
-   blk_add_timer(req);
-   break;
-   case BLK_EH_DONE:
-   break;
-   default:
-   printk(KERN_ERR "block: bad eh return: %d\n", ret);
-   break;
+   ret = req->q->mq_ops->timeout(req, reserved);
+   if (ret == BLK_EH_DONE)
+   return;
+   WARN_ON_ONCE(ret != BLK_EH_RESET_TIMER);
}
+
+   blk_add_timer(req);
 }
 
 static bool blk_mq_req_expired(struct request *rq, unsigned long *next)
-- 
2.17.0

[PATCH 10/14] block: remove BLK_EH_HANDLED

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig 
---
 Documentation/scsi/scsi_eh.txt | 11 ---
 block/blk-mq.c |  3 ---
 block/blk-timeout.c|  3 ---
 include/linux/blkdev.h |  1 -
 4 files changed, 18 deletions(-)

diff --git a/Documentation/scsi/scsi_eh.txt b/Documentation/scsi/scsi_eh.txt
index 3ae8419e72cf..1b7436932a2b 100644
--- a/Documentation/scsi/scsi_eh.txt
+++ b/Documentation/scsi/scsi_eh.txt
@@ -82,17 +82,6 @@ function
  1. invokes optional hostt->eh_timed_out() callback.  Return value can
 be one of
 
-- BLK_EH_HANDLED
-   This indicates that eh_timed_out() dealt with the timeout.
-   The command is passed back to the block layer and completed
-   via __blk_complete_requests().
-
-   *NOTE* After returning BLK_EH_HANDLED the SCSI layer is
-   assumed to be finished with the command, and no other
-   functions from the SCSI layer will be called. So this
-   should typically only be returned if the eh_timed_out()
-   handler raced with normal completion.
-
 - BLK_EH_RESET_TIMER
This indicates that more time is required to finish the
command.  Timer is restarted.  This action is counted as a
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 546acc8cb201..22ab2a148a9b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -844,9 +844,6 @@ static void blk_mq_rq_timed_out(struct request *req, bool 
reserved)
ret = ops->timeout(req, reserved);
 
switch (ret) {
-   case BLK_EH_HANDLED:
-   __blk_mq_complete_request(req);
-   break;
case BLK_EH_RESET_TIMER:
/*
 * As nothing prevents from completion happening while
diff --git a/block/blk-timeout.c b/block/blk-timeout.c
index 3c622335811f..aae8f233341b 100644
--- a/block/blk-timeout.c
+++ b/block/blk-timeout.c
@@ -86,9 +86,6 @@ static void blk_rq_timed_out(struct request *req)
if (q->rq_timed_out_fn)
ret = q->rq_timed_out_fn(req);
switch (ret) {
-   case BLK_EH_HANDLED:
-   __blk_complete_request(req);
-   break;
case BLK_EH_RESET_TIMER:
blk_add_timer(req);
blk_clear_rq_complete(req);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index ba0844c176f2..3d9d4da4dedd 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -338,7 +338,6 @@ typedef void (exit_rq_fn)(struct request_queue *, struct 
request *);
 
 enum blk_eh_timer_return {
BLK_EH_DONE,
-   BLK_EH_HANDLED,
BLK_EH_RESET_TIMER,
 };
 
-- 
2.17.0

[PATCH 08/14] mmc: complete requests from ->timeout

[Christoph Hellwig]

By completing the request entirely in the driver we can remove the
BLK_EH_HANDLED return value and thus the split responsibility between the
driver and the block layer that has been causing trouble.

[While this keeps existing behavior it seems to mismatch the comment,
 maintainers please chime in!]

Signed-off-by: Christoph Hellwig 
---
 drivers/mmc/core/queue.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index 56e9a803db21..648eb6743ed5 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -111,8 +111,9 @@ static enum blk_eh_timer_return mmc_cqe_timed_out(struct 
request *req)
__mmc_cqe_recovery_notifier(mq);
return BLK_EH_RESET_TIMER;
}
-   /* No timeout */
-   return BLK_EH_HANDLED;
+   /* No timeout (XXX: huh? comment doesn't make much sense) */
+   blk_mq_complete_request(req);
+   return BLK_EH_DONE;
default:
/* Timeout is handled by mmc core */
return BLK_EH_RESET_TIMER;
-- 
2.17.0

[PATCH 11/14] block: document the blk_eh_timer_return values

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig 
---
 include/linux/blkdev.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 3d9d4da4dedd..3815d9dcfbe0 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -337,8 +337,8 @@ typedef int (init_rq_fn)(struct request_queue *, struct 
request *, gfp_t);
 typedef void (exit_rq_fn)(struct request_queue *, struct request *);
 
 enum blk_eh_timer_return {
-   BLK_EH_DONE,
-   BLK_EH_RESET_TIMER,
+   BLK_EH_DONE,/* drivers has completed the command */
+   BLK_EH_RESET_TIMER, /* reset timer and try again */
 };
 
 typedef enum blk_eh_timer_return (rq_timed_out_fn)(struct request *);
-- 
2.17.0

[PATCH 09/14] libiscsi: don't try to bypass SCSI EH

[Christoph Hellwig]

libiscsi is the only SCSI code that return BLK_EH_HANDLED, thus trying to
bypass the normal SCSI EH code.  We are going to remove this return value
at the block layer, and at least from a quick look it doesn't look too
harmful to try to send an abort for these cases, especially as the first
one should not actually be possible.  If this doesn't work out iscsi
will probably need its own eh_strategy_handler instead to just do the
right thing.

Signed-off-by: Christoph Hellwig 
---
 drivers/scsi/libiscsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index eee43ba83a60..71bdc0b52cf9 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1982,7 +1982,7 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct 
scsi_cmnd *sc)
 * Raced with completion. Blk layer has taken ownership
 * so let timeout code complete it now.
 */
-   rc = BLK_EH_HANDLED;
+   rc = BLK_EH_DONE;
goto done;
}
 
@@ -1997,7 +1997,7 @@ enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct 
scsi_cmnd *sc)
if (unlikely(system_state != SYSTEM_RUNNING)) {
sc->result = DID_NO_CONNECT << 16;
ISCSI_DBG_EH(session, "sc on shutdown, handled\n");
-   rc = BLK_EH_HANDLED;
+   rc = BLK_EH_DONE;
goto done;
}
/*
-- 
2.17.0

[PATCH 12/14] blk-mq: Fix timeout and state order

[Christoph Hellwig]

From: Keith Busch 

The block layer had been setting the state to in-flight prior to updating
the timer. This is the wrong order since the timeout handler could observe
the in-flight state with the older timeout, believing the request had
expired when in fact it is just getting started.

Signed-off-by: Keith Busch 
---
 block/blk-mq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 22ab2a148a9b..614cb03732ed 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -697,8 +697,8 @@ void blk_mq_start_request(struct request *rq)
preempt_disable();
write_seqcount_begin(>gstate_seq);
 
-   blk_mq_rq_update_state(rq, MQ_RQ_IN_FLIGHT);
blk_add_timer(rq);
+   blk_mq_rq_update_state(rq, MQ_RQ_IN_FLIGHT);
 
write_seqcount_end(>gstate_seq);
preempt_enable();
-- 
2.17.0

[PATCH 07/14] scsi_transport_fc: complete requests from ->timeout

[Christoph Hellwig]

By completing the request entirely in the driver we can remove the
BLK_EH_HANDLED return value and thus the split responsibility between the
driver and the block layer that has been causing trouble.

Signed-off-by: Christoph Hellwig 
---
 drivers/scsi/scsi_transport_fc.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 90075a0ddcfe..7a9a65588a1b 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -3591,10 +3591,9 @@ fc_bsg_job_timeout(struct request *req)
}
 
/* the blk_end_sync_io() doesn't check the error */
-   if (!inflight)
-   return BLK_EH_DONE;
-   else
-   return BLK_EH_HANDLED;
+   if (inflight)
+   blk_mq_complete_request(req);
+   return BLK_EH_DONE;
 }
 
 /**
-- 
2.17.0

[PATCH 05/14] mtip32xx: complete requests from ->timeout

[Christoph Hellwig]

By completing the request entirely in the driver we can remove the
BLK_EH_HANDLED return value and thus the split responsibility between the
driver and the block layer that has been causing trouble.

Signed-off-by: Christoph Hellwig 
---
 drivers/block/mtip32xx/mtip32xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c 
b/drivers/block/mtip32xx/mtip32xx.c
index 6df5b0b1517a..beace13effe4 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -3720,7 +3720,8 @@ static enum blk_eh_timer_return mtip_cmd_timeout(struct 
request *req,
struct mtip_cmd *cmd = blk_mq_rq_to_pdu(req);
 
cmd->status = BLK_STS_TIMEOUT;
-   return BLK_EH_HANDLED;
+   blk_mq_complete_request(req);
+   return BLK_EH_DONE;
}
 
if (test_bit(req->tag, dd->port->cmds_to_issue))
-- 
2.17.0

[PATCH 06/14] null_blk: complete requests from ->timeout

[Christoph Hellwig]

By completing the request entirely in the driver we can remove the
BLK_EH_HANDLED return value and thus the split responsibility between the
driver and the block layer that has been causing trouble.

Signed-off-by: Christoph Hellwig 
---
 drivers/block/null_blk.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c
index a76553293a31..f86e7af1dc8e 100644
--- a/drivers/block/null_blk.c
+++ b/drivers/block/null_blk.c
@@ -1365,7 +1365,8 @@ static blk_qc_t null_queue_bio(struct request_queue *q, 
struct bio *bio)
 static enum blk_eh_timer_return null_rq_timed_out_fn(struct request *rq)
 {
pr_info("null: rq %p timed out\n", rq);
-   return BLK_EH_HANDLED;
+   blk_mq_complete_request(rq);
+   return BLK_EH_DONE;
 }
 
 static int null_rq_prep_fn(struct request_queue *q, struct request *req)
@@ -1427,7 +1428,8 @@ static void null_request_fn(struct request_queue *q)
 static enum blk_eh_timer_return null_timeout_rq(struct request *rq, bool res)
 {
pr_info("null: rq %p timed out\n", rq);
-   return BLK_EH_HANDLED;
+   blk_mq_complete_request(rq);
+   return BLK_EH_DONE;
 }
 
 static blk_status_t null_queue_rq(struct blk_mq_hw_ctx *hctx,
-- 
2.17.0

BLK_EH_HANDLED

[Christoph Hellwig]

Hi all,

this series removes the BLK_EH_HANDLED return value, and instead places
responsibility of timed out commands entirely on the drivers.  Except
for some odd layering vilations in libiscsi this actually is
surprisingly simple.  The last two pathes contain a respin of Keith'
series to refcount struct request in the eye of timeouts.  Once the
BLK_EH_HANDLED special case is gone this actually seems the EH fixing
approach that seems the most sensible to me.

[PATCH 01/14] libata: remove ata_scsi_timed_out

[Christoph Hellwig]

As far as I can tell this function can't even be called any more, given
that ATA implements its own eh_strategy_handler with ata_scsi_error, which
never calls ->eh_timed_out.

Signed-off-by: Christoph Hellwig 
---
 drivers/ata/libata-eh.c | 51 -
 include/linux/libata.h  |  2 --
 2 files changed, 53 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index c016829a38fd..e87785dec151 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -500,57 +500,6 @@ void ata_eh_release(struct ata_port *ap)
mutex_unlock(>host->eh_mutex);
 }
 
-/**
- * ata_scsi_timed_out - SCSI layer time out callback
- * @cmd: timed out SCSI command
- *
- * Handles SCSI layer timeout.  We race with normal completion of
- * the qc for @cmd.  If the qc is already gone, we lose and let
- * the scsi command finish (EH_HANDLED).  Otherwise, the qc has
- * timed out and EH should be invoked.  Prevent ata_qc_complete()
- * from finishing it by setting EH_SCHEDULED and return
- * EH_NOT_HANDLED.
- *
- * TODO: kill this function once old EH is gone.
- *
- * LOCKING:
- * Called from timer context
- *
- * RETURNS:
- * EH_HANDLED or EH_NOT_HANDLED
- */
-enum blk_eh_timer_return ata_scsi_timed_out(struct scsi_cmnd *cmd)
-{
-   struct Scsi_Host *host = cmd->device->host;
-   struct ata_port *ap = ata_shost_to_port(host);
-   unsigned long flags;
-   struct ata_queued_cmd *qc;
-   enum blk_eh_timer_return ret;
-
-   DPRINTK("ENTER\n");
-
-   if (ap->ops->error_handler) {
-   ret = BLK_EH_NOT_HANDLED;
-   goto out;
-   }
-
-   ret = BLK_EH_HANDLED;
-   spin_lock_irqsave(ap->lock, flags);
-   qc = ata_qc_from_tag(ap, ap->link.active_tag);
-   if (qc) {
-   WARN_ON(qc->scsicmd != cmd);
-   qc->flags |= ATA_QCFLAG_EH_SCHEDULED;
-   qc->err_mask |= AC_ERR_TIMEOUT;
-   ret = BLK_EH_NOT_HANDLED;
-   }
-   spin_unlock_irqrestore(ap->lock, flags);
-
- out:
-   DPRINTK("EXIT, ret=%d\n", ret);
-   return ret;
-}
-EXPORT_SYMBOL(ata_scsi_timed_out);
-
 static void ata_eh_unload(struct ata_port *ap)
 {
struct ata_link *link;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 1795fecdea17..1c113134c98f 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1133,7 +1133,6 @@ extern int ata_sas_port_start(struct ata_port *ap);
 extern void ata_sas_port_stop(struct ata_port *ap);
 extern int ata_sas_slave_configure(struct scsi_device *, struct ata_port *);
 extern int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap);
-extern enum blk_eh_timer_return ata_scsi_timed_out(struct scsi_cmnd *cmd);
 extern int sata_scr_valid(struct ata_link *link);
 extern int sata_scr_read(struct ata_link *link, int reg, u32 *val);
 extern int sata_scr_write(struct ata_link *link, int reg, u32 val);
@@ -1359,7 +1358,6 @@ extern struct device_attribute *ata_common_sdev_attrs[];
.proc_name  = drv_name, \
.slave_configure= ata_scsi_slave_config,\
.slave_destroy  = ata_scsi_slave_destroy,   \
-   .eh_timed_out   = ata_scsi_timed_out,   \
.bios_param = ata_std_bios_param,   \
.unlock_native_capacity = ata_scsi_unlock_native_capacity, \
.sdev_attrs = ata_common_sdev_attrs
-- 
2.17.0

[PATCH 02/14] block: rename BLK_EH_NOT_HANDLED to BLK_EH_DONE

[Christoph Hellwig]

The BLK_EH_NOT_HANDLED implies nothing happen, but very often that
is not what is happening - instead the driver already completed the
command.  Fix the symbolic name to reflect that a little better.

Signed-off-by: Christoph Hellwig 
---
 Documentation/scsi/scsi_eh.txt| 4 ++--
 block/blk-mq.c| 2 +-
 block/blk-timeout.c   | 2 +-
 drivers/block/nbd.c   | 2 +-
 drivers/message/fusion/mptsas.c   | 2 +-
 drivers/s390/block/dasd.c | 6 +++---
 drivers/scsi/gdth.c   | 2 +-
 drivers/scsi/libiscsi.c   | 2 +-
 drivers/scsi/megaraid/megaraid_sas_base.c | 2 +-
 drivers/scsi/mvumi.c  | 2 +-
 drivers/scsi/qla4xxx/ql4_os.c | 2 +-
 drivers/scsi/scsi_error.c | 4 ++--
 drivers/scsi/scsi_transport_fc.c  | 4 ++--
 drivers/scsi/scsi_transport_srp.c | 4 ++--
 drivers/scsi/ufs/ufshcd.c | 6 +++---
 include/linux/blkdev.h| 2 +-
 include/scsi/scsi_host.h  | 2 +-
 17 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/Documentation/scsi/scsi_eh.txt b/Documentation/scsi/scsi_eh.txt
index 11e447bdb3a5..3ae8419e72cf 100644
--- a/Documentation/scsi/scsi_eh.txt
+++ b/Documentation/scsi/scsi_eh.txt
@@ -97,9 +97,9 @@ function
This indicates that more time is required to finish the
command.  Timer is restarted.  This action is counted as a
retry and only allowed scmd->allowed + 1(!) times.  Once the
-   limit is reached, action for BLK_EH_NOT_HANDLED is taken instead.
+   limit is reached, action for BLK_EH_DONE is taken instead.
 
-- BLK_EH_NOT_HANDLED
+- BLK_EH_DONE
 eh_timed_out() callback did not handle the command.
Step #2 is taken.
 
diff --git a/block/blk-mq.c b/block/blk-mq.c
index df928200b17e..546acc8cb201 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -856,7 +856,7 @@ static void blk_mq_rq_timed_out(struct request *req, bool 
reserved)
blk_mq_rq_update_aborted_gstate(req, 0);
blk_add_timer(req);
break;
-   case BLK_EH_NOT_HANDLED:
+   case BLK_EH_DONE:
break;
default:
printk(KERN_ERR "block: bad eh return: %d\n", ret);
diff --git a/block/blk-timeout.c b/block/blk-timeout.c
index 652d4d4d3e97..3c622335811f 100644
--- a/block/blk-timeout.c
+++ b/block/blk-timeout.c
@@ -93,7 +93,7 @@ static void blk_rq_timed_out(struct request *req)
blk_add_timer(req);
blk_clear_rq_complete(req);
break;
-   case BLK_EH_NOT_HANDLED:
+   case BLK_EH_DONE:
/*
 * LLD handles this for now but in the future
 * we can send a request msg to abort the command
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 1147e4680c80..09d9959287fe 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -328,7 +328,7 @@ static enum blk_eh_timer_return nbd_xmit_timeout(struct 
request *req,
}
blk_mq_requeue_request(req, true);
nbd_config_put(nbd);
-   return BLK_EH_NOT_HANDLED;
+   return BLK_EH_DONE;
}
} else {
dev_err_ratelimited(nbd_to_dev(nbd),
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 86503f60468f..19a5aa70ecda 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -1929,7 +1929,7 @@ static enum blk_eh_timer_return 
mptsas_eh_timed_out(struct scsi_cmnd *sc)
MPT_SCSI_HOST *hd;
MPT_ADAPTER   *ioc;
VirtDevice*vdevice;
-   enum blk_eh_timer_return rc = BLK_EH_NOT_HANDLED;
+   enum blk_eh_timer_return rc = BLK_EH_DONE;
 
hd = shost_priv(sc->device->host);
if (hd == NULL) {
diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 04143c08bd6e..b0e89ca48a3c 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -3053,7 +3053,7 @@ static blk_status_t do_dasd_request(struct blk_mq_hw_ctx 
*hctx,
  *
  * Return values:
  * BLK_EH_RESET_TIMER if the request should be left running
- * BLK_EH_NOT_HANDLED if the request is handled or terminated
+ * BLK_EH_DONE if the request is handled or terminated
  *   by the driver.
  */
 enum blk_eh_timer_return dasd_times_out(struct request *req, bool reserved)
@@ -3065,7 +3065,7 @@ enum blk_eh_timer_return dasd_times_out(struct request 
*req, bool reserved)
int rc = 0;
 
if (!cqr)
-   return BLK_EH_NOT_HANDLED;
+   return BLK_EH_DONE;
 
spin_lock_irqsave(>dq->lock, flags);
device = cqr->startdev ? cqr->startdev : block->base;
@@ -3124,7 +3124,7 @@ enum blk_eh_timer_return dasd_times_out(struct request 
*req, bool 

[PATCH 03/14] nvme: return BLK_EH_DONE from ->timeout

[Christoph Hellwig]

NVMe always completes the request before returning from ->timeout, either
by polling for it, or by disabling the controller.  Return BLK_EH_DONE so
that the block layer doesn't even try to complete it again.

Signed-off-by: Christoph Hellwig 
---
 drivers/nvme/host/pci.c| 14 +-
 drivers/nvme/host/rdma.c   |  2 +-
 drivers/nvme/target/loop.c |  2 +-
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 917e1714f7d9..31525324b79f 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1205,7 +1205,7 @@ static enum blk_eh_timer_return nvme_timeout(struct 
request *req, bool reserved)
nvme_warn_reset(dev, csts);
nvme_dev_disable(dev, false);
nvme_reset_ctrl(>ctrl);
-   return BLK_EH_HANDLED;
+   return BLK_EH_DONE;
}
 
/*
@@ -1215,14 +1215,14 @@ static enum blk_eh_timer_return nvme_timeout(struct 
request *req, bool reserved)
dev_warn(dev->ctrl.device,
 "I/O %d QID %d timeout, completion polled\n",
 req->tag, nvmeq->qid);
-   return BLK_EH_HANDLED;
+   return BLK_EH_DONE;
}
 
/*
 * Shutdown immediately if controller times out while starting. The
 * reset work will see the pci device disabled when it gets the forced
 * cancellation error. All outstanding requests are completed on
-* shutdown, so we return BLK_EH_HANDLED.
+* shutdown, so we return BLK_EH_DONE.
 */
switch (dev->ctrl.state) {
case NVME_CTRL_CONNECTING:
@@ -1232,7 +1232,7 @@ static enum blk_eh_timer_return nvme_timeout(struct 
request *req, bool reserved)
 req->tag, nvmeq->qid);
nvme_dev_disable(dev, false);
nvme_req(req)->flags |= NVME_REQ_CANCELLED;
-   return BLK_EH_HANDLED;
+   return BLK_EH_DONE;
default:
break;
}
@@ -1249,12 +1249,8 @@ static enum blk_eh_timer_return nvme_timeout(struct 
request *req, bool reserved)
nvme_dev_disable(dev, false);
nvme_reset_ctrl(>ctrl);
 
-   /*
-* Mark the request as handled, since the inline shutdown
-* forces all outstanding requests to complete.
-*/
nvme_req(req)->flags |= NVME_REQ_CANCELLED;
-   return BLK_EH_HANDLED;
+   return BLK_EH_DONE;
}
 
if (atomic_dec_return(>ctrl.abort_limit) < 0) {
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 1eb4438a8763..ac7462cd7f0f 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1598,7 +1598,7 @@ nvme_rdma_timeout(struct request *rq, bool reserved)
/* fail with DNR on cmd timeout */
nvme_req(rq)->status = NVME_SC_ABORT_REQ | NVME_SC_DNR;
 
-   return BLK_EH_HANDLED;
+   return BLK_EH_DONE;
 }
 
 static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 27a8561c0cb9..22e3627bf16b 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -146,7 +146,7 @@ nvme_loop_timeout(struct request *rq, bool reserved)
/* fail with DNR on admin cmd timeout */
nvme_req(rq)->status = NVME_SC_ABORT_REQ | NVME_SC_DNR;
 
-   return BLK_EH_HANDLED;
+   return BLK_EH_DONE;
 }
 
 static blk_status_t nvme_loop_queue_rq(struct blk_mq_hw_ctx *hctx,
-- 
2.17.0

[PATCH 04/14] nbd: complete requests from ->timeout

[Christoph Hellwig]

By completing the request entirely in the driver we can remove the
BLK_EH_HANDLED return value and thus the split responsibility between the
driver and the block layer that has been causing trouble.

Signed-off-by: Christoph Hellwig 
---
 drivers/block/nbd.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 09d9959287fe..30a851b8a99c 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -300,7 +300,7 @@ static enum blk_eh_timer_return nbd_xmit_timeout(struct 
request *req,
 
if (!refcount_inc_not_zero(>config_refs)) {
cmd->status = BLK_STS_TIMEOUT;
-   return BLK_EH_HANDLED;
+   goto done;
}
config = nbd->config;
 
@@ -338,8 +338,9 @@ static enum blk_eh_timer_return nbd_xmit_timeout(struct 
request *req,
cmd->status = BLK_STS_IOERR;
sock_shutdown(nbd);
nbd_config_put(nbd);
-
-   return BLK_EH_HANDLED;
+done:
+   blk_mq_complete_request(req);
+   return BLK_EH_DONE;
 }
 
 /*
-- 
2.17.0

Re: [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

On 2018-05-23 09:46:30 [+0100], John Garry wrote:
> On 22/05/2018 18:31, Sebastian Andrzej Siewior wrote:
> > On 2018-05-18 14:31:27 [+0100], John Garry wrote:
> > > On 04/05/2018 15:50, Sebastian Andrzej Siewior wrote:
> > > > Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
> > > > management duties from lldds") the sas_ata_qc_issue() function unlocks
> > > > the ata_port.lock and disables interrupts before doing so.
> > > > That lock is always taken with disabled interrupts so at this point, the
> > > > interrupts are already disabled. There is no need to disable the
> > > > interrupts before the unlock operation because they are already
> > > > disabled.
> > > 
> > > Is the only caller ata_qc_issue(), which has spin_lock_irqsave(host lock)
> > > enabled?
> > 
> > I don't understand the question.
> 
> It seems to me that ap->lock can be taken in interrupt context, so then we
> know it should be locked with interrupts disabled always.

yes, the comment above the function says so, too.

> I was just really asking how you know interrupts are disabled already? Maybe
> it's obvious.

The lock has to be taken with interrupts disabled. If the interrupts
were enabled at the time sas_ata_qc_issue() was invoked then the lock
was already taken in a bad way and disabling interrupts before the
unlock does not make it any better.
To verify that the locking is okay you can build the kernel with lockdep
enabled and CONFIG_PROVE_LOCKING=y set. That way lockdep will inform you
as soon as it notices the lock taken in interrupt context and in
process-context with enabled interrupts.

> cheers,

Sebastian

Re: [PATCH RFC] sr: mark the device as changed when burning a CD

[Maurizio Lombardi]

Dne 22.5.2018 v 16:47 Jens Axboe napsal(a):
> It's been many years, but back in the day the program writing the cd
> would eject the disc once done. This of course forces a reload of
> the toc and clearing of the flag. What program is this? Seems like
> it should probably eject when it's done.

They are using wodim to burn the CDs on their servers.
The problem is that they do not want the CD to be ejected because their drives
lack a motorized tray, thus requiring manual intervention which they would like 
to avoid.


> 
> There are many commands that will indicate writing to the device, but
> not cause anything that should force a reload. A mode select comes to
> mind.
> 

Ah, I understand... so I have to find a way to distinguish an ongoing cd 
burning process
from, say, a mode select command.

Thanks,
Maurizio

Donation Grant !!!

[Adrian Gillian Bayford]

£1.5 Million Has Been Granted To You As A Donation Visit 
www.bbc.co.uk/news/uk-england-19254228 Sendname Address Phone for more info

Re: [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[John Garry]

On 22/05/2018 18:31, Sebastian Andrzej Siewior wrote:

On 2018-05-18 14:31:27 [+0100], John Garry wrote:

On 04/05/2018 15:50, Sebastian Andrzej Siewior wrote:

Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
management duties from lldds") the sas_ata_qc_issue() function unlocks
the ata_port.lock and disables interrupts before doing so.
That lock is always taken with disabled interrupts so at this point, the
interrupts are already disabled. There is no need to disable the
interrupts before the unlock operation because they are already
disabled.


Is the only caller ata_qc_issue(), which has spin_lock_irqsave(host lock)
enabled?


I don't understand the question.


It seems to me that ap->lock can be taken in interrupt context, so then 
we know it should be locked with interrupts disabled always.


I was just really asking how you know interrupts are disabled already? 
Maybe it's obvious.


cheers,




Restoring the interrupt state later does not change anything because
they were disabled and remain disabled. Therefore remove the operations
which do not change the behaviour.

Signed-off-by: Sebastian Andrzej Siewior 
---
 drivers/scsi/libsas/sas_ata.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 0cc1567eacc1..bc5d917ff643 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -176,7 +176,6 @@ static void sas_ata_task_done(struct sas_task *task)

 static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
 {
-   unsigned long flags;
struct sas_task *task;
struct scatterlist *sg;
int ret = AC_ERR_SYSTEM;
@@ -190,7 +189,6 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd 
*qc)
/* TODO: audit callers to ensure they are ready for qc_issue to
 * unconditionally re-enable interrupts
 */


Is the "TODO" comment still relevant?


can't tell. The interrupts are never enabled during the unlock.


cheers,


-   local_irq_save(flags);
spin_unlock(ap->lock);

/* If the device fell off, no sense in issuing commands */
@@ -252,7 +250,6 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd 
*qc)

  out:
spin_lock(ap->lock);
-   local_irq_restore(flags);
return ret;
 }


Sebastian

.

Re: [PATCH 6/6] scsi: Check sense buffer size at build time

[Sergei Shtylyov]

Hello!

On 5/22/2018 9:15 PM, Kees Cook wrote:


To avoid introducing problems like those fixed in commit f7068114d45e
("sr: pass down correctly sized SCSI sense buffer"), this creates a macro
wrapper for scsi_execute() that verifies the size of the sense buffer
similar to what was done for command string sizes in commit 3756f6401c30
("exec: avoid gcc-8 warning for get_task_comm").

Another solution could be to add another argument to scsi_execute(),
but this function already takes a lot of arguments and Jens was not fond
of that approach. As there was only a pair of dynamically allocated sense
buffers, this also moves those 96 bytes onto the stack to avoid triggering
the sizeof() check.

Signed-off-by: Kees Cook 
---
  drivers/scsi/scsi_lib.c|  6 +++---
  include/scsi/scsi_device.h | 12 +++-
  2 files changed, 14 insertions(+), 4 deletions(-)


[...]

diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 7ae177c8e399..1bb87b6c0ad2 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -426,11 +426,21 @@ extern const char *scsi_device_state_name(enum 
scsi_device_state);
  extern int scsi_is_sdev_device(const struct device *);
  extern int scsi_is_target_device(const struct device *);
  extern void scsi_sanitize_inquiry_string(unsigned char *s, int len);
-extern int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
+extern int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
int data_direction, void *buffer, unsigned bufflen,
unsigned char *sense, struct scsi_sense_hdr *sshdr,
int timeout, int retries, u64 flags,
req_flags_t rq_flags, int *resid);
+/* Make sure any sense buffer is the correct size. */
+#define scsi_execute(sdev, cmd, data_direction, buffer, bufflen, sense,
\
+sshdr, timeout, retries, flags, rq_flags, resid)   \
+({ \
+   BUILD_BUG_ON((sense) != NULL && \
+sizeof(sense) != SCSI_SENSE_BUFFERSIZE);   \


   This would only check the size of the 'sense' pointer, no?


+   __scsi_execute(sdev, cmd, data_direction, buffer, bufflen,  \
+  sense, sshdr, timeout, retries, flags, rq_flags, \
+  resid);  \
+})
  static inline int scsi_execute_req(struct scsi_device *sdev,
const unsigned char *cmd, int data_direction, void *buffer,
unsigned bufflen, struct scsi_sense_hdr *sshdr, int timeout,


MBR, Sergei