Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1345264 > unrolled thread

[PATCH v5 00/15] add fixes, device quirks, error recovery,

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 00/15] add fixes, device quirks, error recovery, Yaniv Gardi <ygardi@codeaurora.org> - 2016-02-28 14:40 +0100
    [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler Yaniv Gardi <ygardi@codeaurora.org> - 2016-02-28 14:40 +0100
      Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler Hannes Reinecke <hare@suse.de> - 2016-03-01 08:40 +0100
        Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler ygardi@codeaurora.org - 2016-03-01 14:30 +0100
          Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler Hannes Reinecke <hare@suse.de> - 2016-03-03 08:30 +0100
            Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler ygardi@codeaurora.org - 2016-03-03 10:20 +0100
              Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler Hannes Reinecke <hare@suse.de> - 2016-03-03 14:00 +0100
    [PATCH v5 08/15] scsi: ufs: make error handling bit faster Yaniv Gardi <ygardi@codeaurora.org> - 2016-02-28 14:40 +0100
      Re: [PATCH v5 08/15] scsi: ufs: make error handling bit faster Hannes Reinecke <hare@suse.de> - 2016-03-01 09:00 +0100
        Re: [PATCH v5 08/15] scsi: ufs: make error handling bit faster ygardi@codeaurora.org - 2016-03-01 11:00 +0100
          Re: [PATCH v5 08/15] scsi: ufs: make error handling bit faster Hannes Reinecke <hare@suse.de> - 2016-03-01 11:10 +0100
    [PATCH v5 11/15] scsi: ufs: handle non spec compliant bkops behaviour by device Yaniv Gardi <ygardi@codeaurora.org> - 2016-02-28 14:40 +0100
    [PATCH v5 02/15] scsi: ufs: avoid spurious UFS host controller interrupts Yaniv Gardi <ygardi@codeaurora.org> - 2016-02-28 14:40 +0100
      Re: [PATCH v5 02/15] scsi: ufs: avoid spurious UFS host controller  interrupts Hannes Reinecke <hare@suse.de> - 2016-03-01 06:20 +0100

#1345264 — [PATCH v5 00/15] add fixes, device quirks, error recovery,

V5:
as per comment, patch 05/14 in V4, was divided into 2 patches in V5

V4:
fixed DOS EOL symbols in 2 new files

V3:
Due to comments in V2
Removed change 02/17 of V2 
Removed change 03/17 of V2
Removed change 17/17 of V2

V2:
This series should be pushed on top of 15 patches series:
"Big fixes, retries, handle a race condition"
fixed and few comments.

V1:
This series should be pushed on top of 15 patches series:
"Big fixes, retries, handle a race condition"


Yaniv Gardi (15):
  scsi: ufs-qcom: add number of lanes per direction
  scsi: ufs: avoid spurious UFS host controller interrupts
  scsi: ufs: implement scsi host timeout handler
  scsi: ufs: verify hba controller hce reg value
  scsi: ufs: add support to read device and string descriptors
  scsi: ufs: separate device and host quirks
  scsi: ufs: disable vccq if it's not needed by UFS device
  scsi: ufs: make error handling bit faster
  scsi: ufs: add error recovery after DL NAC error
  scsi: ufs: add retry for query descriptors
  scsi: ufs: handle non spec compliant bkops behaviour by device
  scsi: ufs: tune UniPro parameters to optimize hibern8 exit time
  scsi: ufs: fix leakage during link off state
  scsi: ufs: add device quirk delay before putting UFS rails in LPM
  scsi: ufs-qcom: set PA_Local_TX_LCC_Enable before link startup

 .../devicetree/bindings/ufs/ufshcd-pltfrm.txt      |   3 +
 drivers/scsi/ufs/Makefile                          |   2 +-
 drivers/scsi/ufs/ufs-qcom.c                        | 155 ++++-
 drivers/scsi/ufs/ufs-qcom.h                        |   9 +
 drivers/scsi/ufs/ufs.h                             |  33 +
 drivers/scsi/ufs/ufs_quirks.c                      | 104 +++
 drivers/scsi/ufs/ufs_quirks.h                      | 133 ++++
 drivers/scsi/ufs/ufshcd-pltfrm.c                   |  19 +
 drivers/scsi/ufs/ufshcd.c                          | 749 ++++++++++++++++++---
 drivers/scsi/ufs/ufshcd.h                          |  40 +-
 drivers/scsi/ufs/ufshci.h                          |   4 +
 drivers/scsi/ufs/unipro.h                          |  22 +
 12 files changed, 1145 insertions(+), 128 deletions(-)
 create mode 100644 drivers/scsi/ufs/ufs_quirks.c
 create mode 100644 drivers/scsi/ufs/ufs_quirks.h

-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[toc] | [next] | [standalone]

#1345265 — [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler

A race condition exists between request requeueing and scsi layer
error handling:
When UFS driver queuecommand returns a busy status for a request,
it will be requeued and its tag will be freed and set to -1.
At the same time it is possible that the request will timeout and
scsi layer will start error handling for it. The scsi layer reuses
the request and its tag to send error related commands to the device,
however its tag is no longer valid.
As this request was never really sent to the device, there is no
point to start error handling with the device.
Implement the scsi error handling timeout callback and bypass SCSI
error handling for request that were not actually sent to the device.
For such requests simply reset the block layer timer. Otherwise, let
SCSI layer perform the usual error handling.

Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index de7280c..3400ceb 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4568,6 +4568,41 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 	ufshcd_probe_hba(hba);
 }
 
+static enum blk_eh_timer_return ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
+{
+	unsigned long flags;
+	struct Scsi_Host *host;
+	struct ufs_hba *hba;
+	int index;
+	bool found = false;
+
+	if (!scmd || !scmd->device || !scmd->device->host)
+		return BLK_EH_NOT_HANDLED;
+
+	host = scmd->device->host;
+	hba = shost_priv(host);
+	if (!hba)
+		return BLK_EH_NOT_HANDLED;
+
+	spin_lock_irqsave(host->host_lock, flags);
+
+	for_each_set_bit(index, &hba->outstanding_reqs, hba->nutrs) {
+		if (hba->lrb[index].cmd == scmd) {
+			found = true;
+			break;
+		}
+	}
+
+	spin_unlock_irqrestore(host->host_lock, flags);
+
+	/*
+	 * Bypass SCSI error handling and reset the block layer timer if this
+	 * SCSI command was not actually dispatched to UFS driver, otherwise
+	 * let SCSI layer handle the error as usual.
+	 */
+	return found ? BLK_EH_NOT_HANDLED : BLK_EH_RESET_TIMER;
+}
+
 static struct scsi_host_template ufshcd_driver_template = {
 	.module			= THIS_MODULE,
 	.name			= UFSHCD,
@@ -4580,6 +4615,7 @@ static struct scsi_host_template ufshcd_driver_template = {
 	.eh_abort_handler	= ufshcd_abort,
 	.eh_device_reset_handler = ufshcd_eh_device_reset_handler,
 	.eh_host_reset_handler   = ufshcd_eh_host_reset_handler,
+	.eh_timed_out		= ufshcd_eh_timed_out,
 	.this_id		= -1,
 	.sg_tablesize		= SG_ALL,
 	.cmd_per_lun		= UFSHCD_CMD_PER_LUN,
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[toc] | [prev] | [next] | [standalone]

#1346426 — Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler

On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
> A race condition exists between request requeueing and scsi layer
> error handling:
> When UFS driver queuecommand returns a busy status for a request,
> it will be requeued and its tag will be freed and set to -1.
> At the same time it is possible that the request will timeout and
> scsi layer will start error handling for it. The scsi layer reuses
> the request and its tag to send error related commands to the device,
> however its tag is no longer valid.
Hmm. How can the host return a 'busy' status for a request?
From my understanding we have three possibilities:

1) queuecommand returns busy; however, that means that the command has
never been send and this issue shouldn't occur
2) The command returns with BUSY status. But in this case it has already
been returned, so there cannot be any timeout coming in.
3) The host receives a command with a tag which is already in-use.
However, that should have been prevented by the block-layer, which
really should ensure that this situation never happens.

So either way I look at it, it really looks like a bug and adding a
timeout handler will just paper over it.
(Not that a timeout handler is a bad idea, in fact I'm convinced that
you need one. Just not for this purpose.)

So can you elaborate how this 'busy' status comes about?
Is the command sent to the device?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1346615 — Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler

> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
>> A race condition exists between request requeueing and scsi layer
>> error handling:
>> When UFS driver queuecommand returns a busy status for a request,
>> it will be requeued and its tag will be freed and set to -1.
>> At the same time it is possible that the request will timeout and
>> scsi layer will start error handling for it. The scsi layer reuses
>> the request and its tag to send error related commands to the device,
>> however its tag is no longer valid.
> Hmm. How can the host return a 'busy' status for a request?
> From my understanding we have three possibilities:
>
> 1) queuecommand returns busy; however, that means that the command has
> never been send and this issue shouldn't occur
> 2) The command returns with BUSY status. But in this case it has already
> been returned, so there cannot be any timeout coming in.
> 3) The host receives a command with a tag which is already in-use.
> However, that should have been prevented by the block-layer, which
> really should ensure that this situation never happens.
>
> So either way I look at it, it really looks like a bug and adding a
> timeout handler will just paper over it.
> (Not that a timeout handler is a bad idea, in fact I'm convinced that
> you need one. Just not for this purpose.)
>
> So can you elaborate how this 'busy' status comes about?
> Is the command sent to the device?
>
> Cheers,
>
> Hannes


Hi Hannes,

it's going to be a bit long :)
I think you are missing the point.
I will describe a race condition happened to us a while ago, that was
quite difficult to understand and fix.
So, this patch is not about the "busy" returning to the scsi dispatch
routine. it's about the abort triggered after 30 seconds.

imagine a request being queued and sent to the scsi, and then to the ufs.
a timer, initialized to 30 seconds start ticking.
but the request is never sent to the ufs device, as queuecommand() returns
with "SCSI_MLQUEUE_HOST_BUSY"
by looking at the code, this could happen, for example:
	err = ufshcd_hold(hba, true);
	if (err) {
		err = SCSI_MLQUEUE_HOST_BUSY;
		goto out;
	}

so, now, the request should be re-queued, and its timer should be reset.
(REMEMBER THIS POINT, let's call it "POINT A")
BUT, a context switch happens before it's actually re-queued, and CPU is
moving to other tasks, doing other things for 30 seconds. yes, sounds
crazy, but it did happen.

NOW, the timeout_handler invoked, and the scsi_abort() routine start
executing, (since 30 seconds passed with no completion).
so far, so good.
but hey, another context switch happens, right at the beginning of
scsi_abort() routine, before anything useful happens. (this is "POINT B")
so, now, context is going back "POINT A", to the blk_requeue_request()
routine, that is calling:
blk_delete_timer(rq); (which does nothing cause the timer already expired)
and then it calls:
blk_queue_end_tag()
which place "-1" in the tag field of the request, marking the request, as
"not tagged yet".

however, a context switch happens again, and we are back in scsi_abort()
routine ("POINT B"), that now needs to abort this very request, but hey,
in the "tag" field, what it sees is tag "-1" which is obviously wrong.

this patch fixes this very rare race condition:
1. upon timeout, blk_rq_timed_out() is called
2. then it calls rq_timed_out_fn() which eventually call
the new callback presented in this patch: "ufshcd_eh_timed_out()"
3. this routine returns with the right flag:
BLK_EH_NOT_HANDLED or BLK_EH_RESET_TIMER.
4. blk_rq_timed_out() checks the returned value:
in case of BLK_EH_HANDLED, it handles normally, meaning, calling scsi_abort()
in case of BLK_EH_RESET_TIMER it starts a new timer, and scsi_abort()
never called.

hope that helps.
regards,
Yaniv

> --
> Dr. Hannes Reinecke		      zSeries & Storage
> hare@suse.de			      +49 911 74053 688
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
>

[toc] | [prev] | [next] | [standalone]

#1348826 — Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler

On 03/01/2016 09:25 PM, ygardi@codeaurora.org wrote:
>> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
>>> A race condition exists between request requeueing and scsi layer
>>> error handling:
>>> When UFS driver queuecommand returns a busy status for a request,
>>> it will be requeued and its tag will be freed and set to -1.
>>> At the same time it is possible that the request will timeout and
>>> scsi layer will start error handling for it. The scsi layer reuses
>>> the request and its tag to send error related commands to the device,
>>> however its tag is no longer valid.
>> Hmm. How can the host return a 'busy' status for a request?
>> From my understanding we have three possibilities:
>>
>> 1) queuecommand returns busy; however, that means that the command has
>> never been send and this issue shouldn't occur
>> 2) The command returns with BUSY status. But in this case it has already
>> been returned, so there cannot be any timeout coming in.
>> 3) The host receives a command with a tag which is already in-use.
>> However, that should have been prevented by the block-layer, which
>> really should ensure that this situation never happens.
>>
>> So either way I look at it, it really looks like a bug and adding a
>> timeout handler will just paper over it.
>> (Not that a timeout handler is a bad idea, in fact I'm convinced that
>> you need one. Just not for this purpose.)
>>
>> So can you elaborate how this 'busy' status comes about?
>> Is the command sent to the device?
>>
>> Cheers,
>>
>> Hannes
> 
> 
> Hi Hannes,
> 
> it's going to be a bit long :)
> I think you are missing the point.
> I will describe a race condition happened to us a while ago, that was
> quite difficult to understand and fix.
> So, this patch is not about the "busy" returning to the scsi dispatch
> routine. it's about the abort triggered after 30 seconds.
> 
> imagine a request being queued and sent to the scsi, and then to the ufs.
> a timer, initialized to 30 seconds start ticking.
> but the request is never sent to the ufs device, as queuecommand() returns
> with "SCSI_MLQUEUE_HOST_BUSY"
> by looking at the code, this could happen, for example:
> 	err = ufshcd_hold(hba, true);
> 	if (err) {
> 		err = SCSI_MLQUEUE_HOST_BUSY;
> 		goto out;
> 	}
> 
Uuhhh.
You probably should not have pointed me to that piece of code ...
open-coding loops in ufshcd_hold() ... shudder.
(Did I ever review that one? Must've ...)
_Anyway_: sleeping in queuecommand is always a bad idea, as then
precisely those issues you've just described will happen.

Couldn't you just call
ufshcd_hold(hba, false)
instead of
ufshcd_hold(hba, true)
?
The request will be requeued more-or-less immediately, avoiding the
issue with timeout handler kicking in.
And the queue will remain blocked until the ungate work item returns, at
which point I/O submission will continue.
As the request will be requeued to the head of the queue there won't be
other I/O competing with tags, so it shouldn't have any adverse effects.

Wouldn't that work?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1348926 — Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler

> On 03/01/2016 09:25 PM, ygardi@codeaurora.org wrote:
>>> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
>>>> A race condition exists between request requeueing and scsi layer
>>>> error handling:
>>>> When UFS driver queuecommand returns a busy status for a request,
>>>> it will be requeued and its tag will be freed and set to -1.
>>>> At the same time it is possible that the request will timeout and
>>>> scsi layer will start error handling for it. The scsi layer reuses
>>>> the request and its tag to send error related commands to the device,
>>>> however its tag is no longer valid.
>>> Hmm. How can the host return a 'busy' status for a request?
>>> From my understanding we have three possibilities:
>>>
>>> 1) queuecommand returns busy; however, that means that the command has
>>> never been send and this issue shouldn't occur
>>> 2) The command returns with BUSY status. But in this case it has
>>> already
>>> been returned, so there cannot be any timeout coming in.
>>> 3) The host receives a command with a tag which is already in-use.
>>> However, that should have been prevented by the block-layer, which
>>> really should ensure that this situation never happens.
>>>
>>> So either way I look at it, it really looks like a bug and adding a
>>> timeout handler will just paper over it.
>>> (Not that a timeout handler is a bad idea, in fact I'm convinced that
>>> you need one. Just not for this purpose.)
>>>
>>> So can you elaborate how this 'busy' status comes about?
>>> Is the command sent to the device?
>>>
>>> Cheers,
>>>
>>> Hannes
>>
>>
>> Hi Hannes,
>>
>> it's going to be a bit long :)
>> I think you are missing the point.
>> I will describe a race condition happened to us a while ago, that was
>> quite difficult to understand and fix.
>> So, this patch is not about the "busy" returning to the scsi dispatch
>> routine. it's about the abort triggered after 30 seconds.
>>
>> imagine a request being queued and sent to the scsi, and then to the
>> ufs.
>> a timer, initialized to 30 seconds start ticking.
>> but the request is never sent to the ufs device, as queuecommand()
>> returns
>> with "SCSI_MLQUEUE_HOST_BUSY"
>> by looking at the code, this could happen, for example:
>> 	err = ufshcd_hold(hba, true);
>> 	if (err) {
>> 		err = SCSI_MLQUEUE_HOST_BUSY;
>> 		goto out;
>> 	}
>>
> Uuhhh.
> You probably should not have pointed me to that piece of code ...
> open-coding loops in ufshcd_hold() ... shudder.
> (Did I ever review that one? Must've ...)
> _Anyway_: sleeping in queuecommand is always a bad idea, as then
> precisely those issues you've just described will happen.
>
> Couldn't you just call
> ufshcd_hold(hba, false)
> instead of
> ufshcd_hold(hba, true)
> ?
> The request will be requeued more-or-less immediately, avoiding the
> issue with timeout handler kicking in.
> And the queue will remain blocked until the ungate work item returns, at
> which point I/O submission will continue.
> As the request will be requeued to the head of the queue there won't be
> other I/O competing with tags, so it shouldn't have any adverse effects.
>
> Wouldn't that work?
>
> Cheers,
>
> Hannes

Hi Hannes

This is a bug, and it should be fixed.
if you choose to bypass it, by calling ufshcd_hold(hba, false), not only
the race condition is still there, and can pop-out at any other point in
the future, but also, not sure what are the consequences of
ufshcd_hold(hba, false) unstead of "true".
so, changing the already tested and working code, (not to return BUSY from
queuecommand) is not a fix.
I strongly recommend we upstream this race-condition fix.

thanks,
Yaniv



> --
> Dr. Hannes Reinecke		      zSeries & Storage
> hare@suse.de			      +49 911 74053 688
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [next] | [standalone]

#1349126 — Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler

On 03/03/2016 05:10 PM, ygardi@codeaurora.org wrote:
>> On 03/01/2016 09:25 PM, ygardi@codeaurora.org wrote:
>>>> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
>>>>> A race condition exists between request requeueing and scsi layer
>>>>> error handling:
>>>>> When UFS driver queuecommand returns a busy status for a request,
>>>>> it will be requeued and its tag will be freed and set to -1.
>>>>> At the same time it is possible that the request will timeout and
>>>>> scsi layer will start error handling for it. The scsi layer reuses
>>>>> the request and its tag to send error related commands to the device,
>>>>> however its tag is no longer valid.
>>>> Hmm. How can the host return a 'busy' status for a request?
>>>> From my understanding we have three possibilities:
>>>>
>>>> 1) queuecommand returns busy; however, that means that the command has
>>>> never been send and this issue shouldn't occur
>>>> 2) The command returns with BUSY status. But in this case it has
>>>> already
>>>> been returned, so there cannot be any timeout coming in.
>>>> 3) The host receives a command with a tag which is already in-use.
>>>> However, that should have been prevented by the block-layer, which
>>>> really should ensure that this situation never happens.
>>>>
>>>> So either way I look at it, it really looks like a bug and adding a
>>>> timeout handler will just paper over it.
>>>> (Not that a timeout handler is a bad idea, in fact I'm convinced that
>>>> you need one. Just not for this purpose.)
>>>>
>>>> So can you elaborate how this 'busy' status comes about?
>>>> Is the command sent to the device?
>>>>
>>>> Cheers,
>>>>
>>>> Hannes
>>>
>>>
>>> Hi Hannes,
>>>
>>> it's going to be a bit long :)
>>> I think you are missing the point.
>>> I will describe a race condition happened to us a while ago, that was
>>> quite difficult to understand and fix.
>>> So, this patch is not about the "busy" returning to the scsi dispatch
>>> routine. it's about the abort triggered after 30 seconds.
>>>
>>> imagine a request being queued and sent to the scsi, and then to the
>>> ufs.
>>> a timer, initialized to 30 seconds start ticking.
>>> but the request is never sent to the ufs device, as queuecommand()
>>> returns
>>> with "SCSI_MLQUEUE_HOST_BUSY"
>>> by looking at the code, this could happen, for example:
>>> 	err = ufshcd_hold(hba, true);
>>> 	if (err) {
>>> 		err = SCSI_MLQUEUE_HOST_BUSY;
>>> 		goto out;
>>> 	}
>>>
>> Uuhhh.
>> You probably should not have pointed me to that piece of code ...
>> open-coding loops in ufshcd_hold() ... shudder.
>> (Did I ever review that one? Must've ...)
>> _Anyway_: sleeping in queuecommand is always a bad idea, as then
>> precisely those issues you've just described will happen.
>>
>> Couldn't you just call
>> ufshcd_hold(hba, false)
>> instead of
>> ufshcd_hold(hba, true)
>> ?
>> The request will be requeued more-or-less immediately, avoiding the
>> issue with timeout handler kicking in.
>> And the queue will remain blocked until the ungate work item returns, at
>> which point I/O submission will continue.
>> As the request will be requeued to the head of the queue there won't be
>> other I/O competing with tags, so it shouldn't have any adverse effects.
>>
>> Wouldn't that work?
>>
>> Cheers,
>>
>> Hannes
> 
> Hi Hannes
> 
> This is a bug, and it should be fixed.
Oh, definitely agreed. The question is _where_.


> if you choose to bypass it, by calling ufshcd_hold(hba, false), not only
> the race condition is still there, and can pop-out at any other point in
> the future, but also, not sure what are the consequences of
> ufshcd_hold(hba, false) unstead of "true".
Well ... seeing it's your driver, I would've thought _you_ should know ...

> so, changing the already tested and working code, (not to return BUSY from
> queuecommand) is not a fix.
Hey, I did _not_ suggest not to retury BUSY from queuecommand.

I was suggesting this patch:

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9c1b94b..b9295ad 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1388,7 +1388,7 @@ static int ufshcd_queuecommand(struct Scsi_Host
*host, struct scsi_cmnd *cmd)
                goto out;
        }

-       err = ufshcd_hold(hba, true);
+       err = ufshcd_hold(hba, false);
        if (err) {
                err = SCSI_MLQUEUE_HOST_BUSY;
                clear_bit_unlock(tag, &hba->lrb_in_use);

which, by reading the code, should be avoiding this issue.
I was just asking you if you could give this patch a spin and see if it
works. If not (for whatever reason) I'm happy to accept your patch.
But first I would like to have an explanation why the above would _not_
work.

Unfortunately I don't have the hardware otherwise I'd be running the
tests myself.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1345266 — [PATCH v5 08/15] scsi: ufs: make error handling bit faster

UFS driver's error handler forcefully tries to clear all the pending
requests. For each pending request in the queue, it waits 1 sec for it
to get cleared. If we have multiple requests in the queue then it's
possible that we might end up waiting for those many seconds before
resetting the host. But note that resetting host would any way clear
all the pending requests from the hardware. Hence this change skips
the forceful clear of the pending requests if we are anyway going to
reset the host (for fatal errors).

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 155 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 112 insertions(+), 43 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 987cf27..dc096f1 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -133,9 +133,11 @@ enum {
 /* UFSHCD UIC layer error flags */
 enum {
 	UFSHCD_UIC_DL_PA_INIT_ERROR = (1 << 0), /* Data link layer error */
-	UFSHCD_UIC_NL_ERROR = (1 << 1), /* Network layer error */
-	UFSHCD_UIC_TL_ERROR = (1 << 2), /* Transport Layer error */
-	UFSHCD_UIC_DME_ERROR = (1 << 3), /* DME error */
+	UFSHCD_UIC_DL_NAC_RECEIVED_ERROR = (1 << 1), /* Data link layer error */
+	UFSHCD_UIC_DL_TCx_REPLAY_ERROR = (1 << 2), /* Data link layer error */
+	UFSHCD_UIC_NL_ERROR = (1 << 3), /* Network layer error */
+	UFSHCD_UIC_TL_ERROR = (1 << 4), /* Transport Layer error */
+	UFSHCD_UIC_DME_ERROR = (1 << 5), /* DME error */
 };
 
 /* Interrupt configuration options */
@@ -3465,31 +3467,18 @@ static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 }
 
 /**
- * ufshcd_transfer_req_compl - handle SCSI and query command completion
+ * __ufshcd_transfer_req_compl - handle SCSI and query command completion
  * @hba: per adapter instance
+ * @completed_reqs: requests to complete
  */
-static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
+static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
+					unsigned long completed_reqs)
 {
 	struct ufshcd_lrb *lrbp;
 	struct scsi_cmnd *cmd;
-	unsigned long completed_reqs;
-	u32 tr_doorbell;
 	int result;
 	int index;
 
-	/* Resetting interrupt aggregation counters first and reading the
-	 * DOOR_BELL afterward allows us to handle all the completed requests.
-	 * In order to prevent other interrupts starvation the DB is read once
-	 * after reset. The down side of this solution is the possibility of
-	 * false interrupt if device completes another request after resetting
-	 * aggregation and before reading the DB.
-	 */
-	if (ufshcd_is_intr_aggr_allowed(hba))
-		ufshcd_reset_intr_aggr(hba);
-
-	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
-	completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
-
 	for_each_set_bit(index, &completed_reqs, hba->nutrs) {
 		lrbp = &hba->lrb[index];
 		cmd = lrbp->cmd;
@@ -3519,6 +3508,31 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
 }
 
 /**
+ * ufshcd_transfer_req_compl - handle SCSI and query command completion
+ * @hba: per adapter instance
+ */
+static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
+{
+	unsigned long completed_reqs;
+	u32 tr_doorbell;
+
+	/* Resetting interrupt aggregation counters first and reading the
+	 * DOOR_BELL afterward allows us to handle all the completed requests.
+	 * In order to prevent other interrupts starvation the DB is read once
+	 * after reset. The down side of this solution is the possibility of
+	 * false interrupt if device completes another request after resetting
+	 * aggregation and before reading the DB.
+	 */
+	if (ufshcd_is_intr_aggr_allowed(hba))
+		ufshcd_reset_intr_aggr(hba);
+
+	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
+	completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
+
+	__ufshcd_transfer_req_compl(hba, completed_reqs);
+}
+
+/**
  * ufshcd_disable_ee - disable exception event
  * @hba: per-adapter instance
  * @mask: exception event to disable
@@ -3773,6 +3787,13 @@ out:
 	return;
 }
 
+/* Complete requests that have door-bell cleared */
+static void ufshcd_complete_requests(struct ufs_hba *hba)
+{
+	ufshcd_transfer_req_compl(hba);
+	ufshcd_tmc_handler(hba);
+}
+
 /**
  * ufshcd_err_handler - handle UFS errors that require s/w attention
  * @work: pointer to work structure
@@ -3785,6 +3806,7 @@ static void ufshcd_err_handler(struct work_struct *work)
 	u32 err_tm = 0;
 	int err = 0;
 	int tag;
+	bool needs_reset = false;
 
 	hba = container_of(work, struct ufs_hba, eh_work);
 
@@ -3792,40 +3814,75 @@ static void ufshcd_err_handler(struct work_struct *work)
 	ufshcd_hold(hba, false);
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
-	if (hba->ufshcd_state == UFSHCD_STATE_RESET) {
-		spin_unlock_irqrestore(hba->host->host_lock, flags);
+	if (hba->ufshcd_state == UFSHCD_STATE_RESET)
 		goto out;
-	}
 
 	hba->ufshcd_state = UFSHCD_STATE_RESET;
 	ufshcd_set_eh_in_progress(hba);
 
 	/* Complete requests that have door-bell cleared by h/w */
-	ufshcd_transfer_req_compl(hba);
-	ufshcd_tmc_handler(hba);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
+	ufshcd_complete_requests(hba);
+	if ((hba->saved_err & INT_FATAL_ERRORS) ||
+	    ((hba->saved_err & UIC_ERROR) &&
+	    (hba->saved_uic_err & (UFSHCD_UIC_DL_PA_INIT_ERROR |
+				   UFSHCD_UIC_DL_NAC_RECEIVED_ERROR |
+				   UFSHCD_UIC_DL_TCx_REPLAY_ERROR))))
+		needs_reset = true;
 
+	/*
+	 * if host reset is required then skip clearing the pending
+	 * transfers forcefully because they will automatically get
+	 * cleared after link startup.
+	 */
+	if (needs_reset)
+		goto skip_pending_xfer_clear;
+
+	/* release lock as clear command might sleep */
+	spin_unlock_irqrestore(hba->host->host_lock, flags);
 	/* Clear pending transfer requests */
-	for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs)
-		if (ufshcd_clear_cmd(hba, tag))
-			err_xfer |= 1 << tag;
+	for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs) {
+		if (ufshcd_clear_cmd(hba, tag)) {
+			err_xfer = true;
+			goto lock_skip_pending_xfer_clear;
+		}
+	}
 
 	/* Clear pending task management requests */
-	for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs)
-		if (ufshcd_clear_tm_cmd(hba, tag))
-			err_tm |= 1 << tag;
+	for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs) {
+		if (ufshcd_clear_tm_cmd(hba, tag)) {
+			err_tm = true;
+			goto lock_skip_pending_xfer_clear;
+		}
+	}
 
-	/* Complete the requests that are cleared by s/w */
+lock_skip_pending_xfer_clear:
 	spin_lock_irqsave(hba->host->host_lock, flags);
-	ufshcd_transfer_req_compl(hba);
-	ufshcd_tmc_handler(hba);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
+	/* Complete the requests that are cleared by s/w */
+	ufshcd_complete_requests(hba);
+
+	if (err_xfer || err_tm)
+		needs_reset = true;
+
+skip_pending_xfer_clear:
 	/* Fatal errors need reset */
-	if (err_xfer || err_tm || (hba->saved_err & INT_FATAL_ERRORS) ||
-			((hba->saved_err & UIC_ERROR) &&
-			 (hba->saved_uic_err & UFSHCD_UIC_DL_PA_INIT_ERROR))) {
+	if (needs_reset) {
+		unsigned long max_doorbells = (1UL << hba->nutrs) - 1;
+
+		/*
+		 * ufshcd_reset_and_restore() does the link reinitialization
+		 * which will need atleast one empty doorbell slot to send the
+		 * device management commands (NOP and query commands).
+		 * If there is no slot empty at this moment then free up last
+		 * slot forcefully.
+		 */
+		if (hba->outstanding_reqs == max_doorbells)
+			__ufshcd_transfer_req_compl(hba,
+						    (1UL << (hba->nutrs - 1)));
+
+		spin_unlock_irqrestore(hba->host->host_lock, flags);
 		err = ufshcd_reset_and_restore(hba);
+		spin_lock_irqsave(hba->host->host_lock, flags);
 		if (err) {
 			dev_err(hba->dev, "%s: reset and restore failed\n",
 					__func__);
@@ -3839,9 +3896,18 @@ static void ufshcd_err_handler(struct work_struct *work)
 		hba->saved_err = 0;
 		hba->saved_uic_err = 0;
 	}
+
+	if (!needs_reset) {
+		hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
+		if (hba->saved_err || hba->saved_uic_err)
+			dev_err_ratelimited(hba->dev, "%s: exit: saved_err 0x%x saved_uic_err 0x%x",
+			    __func__, hba->saved_err, hba->saved_uic_err);
+	}
+
 	ufshcd_clear_eh_in_progress(hba);
 
 out:
+	spin_unlock_irqrestore(hba->host->host_lock, flags);
 	scsi_unblock_requests(hba->host);
 	ufshcd_release(hba);
 	pm_runtime_put_sync(hba->dev);
@@ -3896,15 +3962,18 @@ static void ufshcd_check_errors(struct ufs_hba *hba)
 	}
 
 	if (queue_eh_work) {
+		/*
+		 * update the transfer error masks to sticky bits, let's do this
+		 * irrespective of current ufshcd_state.
+		 */
+		hba->saved_err |= hba->errors;
+		hba->saved_uic_err |= hba->uic_error;
+
 		/* handle fatal errors only when link is functional */
 		if (hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
 			/* block commands from scsi mid-layer */
 			scsi_block_requests(hba->host);
 
-			/* transfer error masks to sticky bits */
-			hba->saved_err |= hba->errors;
-			hba->saved_uic_err |= hba->uic_error;
-
 			hba->ufshcd_state = UFSHCD_STATE_ERROR;
 			schedule_work(&hba->eh_work);
 		}
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[toc] | [prev] | [next] | [standalone]

#1346437 — Re: [PATCH v5 08/15] scsi: ufs: make error handling bit faster

On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
> UFS driver's error handler forcefully tries to clear all the pending
> requests. For each pending request in the queue, it waits 1 sec for it
> to get cleared. If we have multiple requests in the queue then it's
> possible that we might end up waiting for those many seconds before
> resetting the host. But note that resetting host would any way clear
> all the pending requests from the hardware. Hence this change skips
> the forceful clear of the pending requests if we are anyway going to
> reset the host (for fatal errors).
> 
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
> 
> ---
>  drivers/scsi/ufs/ufshcd.c | 155 +++++++++++++++++++++++++++++++++-------------
>  1 file changed, 112 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 987cf27..dc096f1 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -133,9 +133,11 @@ enum {
>  /* UFSHCD UIC layer error flags */
>  enum {
>  	UFSHCD_UIC_DL_PA_INIT_ERROR = (1 << 0), /* Data link layer error */
> -	UFSHCD_UIC_NL_ERROR = (1 << 1), /* Network layer error */
> -	UFSHCD_UIC_TL_ERROR = (1 << 2), /* Transport Layer error */
> -	UFSHCD_UIC_DME_ERROR = (1 << 3), /* DME error */
> +	UFSHCD_UIC_DL_NAC_RECEIVED_ERROR = (1 << 1), /* Data link layer error */
> +	UFSHCD_UIC_DL_TCx_REPLAY_ERROR = (1 << 2), /* Data link layer error */
> +	UFSHCD_UIC_NL_ERROR = (1 << 3), /* Network layer error */
> +	UFSHCD_UIC_TL_ERROR = (1 << 4), /* Transport Layer error */
> +	UFSHCD_UIC_DME_ERROR = (1 << 5), /* DME error */
>  };
>  
>  /* Interrupt configuration options */
> @@ -3465,31 +3467,18 @@ static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
>  }
>  
>  /**
> - * ufshcd_transfer_req_compl - handle SCSI and query command completion
> + * __ufshcd_transfer_req_compl - handle SCSI and query command completion
>   * @hba: per adapter instance
> + * @completed_reqs: requests to complete
>   */
> -static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
> +static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
> +					unsigned long completed_reqs)
>  {
>  	struct ufshcd_lrb *lrbp;
>  	struct scsi_cmnd *cmd;
> -	unsigned long completed_reqs;
> -	u32 tr_doorbell;
>  	int result;
>  	int index;
>  
> -	/* Resetting interrupt aggregation counters first and reading the
> -	 * DOOR_BELL afterward allows us to handle all the completed requests.
> -	 * In order to prevent other interrupts starvation the DB is read once
> -	 * after reset. The down side of this solution is the possibility of
> -	 * false interrupt if device completes another request after resetting
> -	 * aggregation and before reading the DB.
> -	 */
> -	if (ufshcd_is_intr_aggr_allowed(hba))
> -		ufshcd_reset_intr_aggr(hba);
> -
> -	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> -	completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
> -
>  	for_each_set_bit(index, &completed_reqs, hba->nutrs) {
>  		lrbp = &hba->lrb[index];
>  		cmd = lrbp->cmd;
> @@ -3519,6 +3508,31 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
>  }
>  
>  /**
> + * ufshcd_transfer_req_compl - handle SCSI and query command completion
> + * @hba: per adapter instance
> + */
> +static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
> +{
> +	unsigned long completed_reqs;
> +	u32 tr_doorbell;
> +
> +	/* Resetting interrupt aggregation counters first and reading the
> +	 * DOOR_BELL afterward allows us to handle all the completed requests.
> +	 * In order to prevent other interrupts starvation the DB is read once
> +	 * after reset. The down side of this solution is the possibility of
> +	 * false interrupt if device completes another request after resetting
> +	 * aggregation and before reading the DB.
> +	 */
> +	if (ufshcd_is_intr_aggr_allowed(hba))
> +		ufshcd_reset_intr_aggr(hba);
> +
> +	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> +	completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
> +
> +	__ufshcd_transfer_req_compl(hba, completed_reqs);
> +}
> +
> +/**
>   * ufshcd_disable_ee - disable exception event
>   * @hba: per-adapter instance
>   * @mask: exception event to disable
> @@ -3773,6 +3787,13 @@ out:
>  	return;
>  }
>  
> +/* Complete requests that have door-bell cleared */
> +static void ufshcd_complete_requests(struct ufs_hba *hba)
> +{
> +	ufshcd_transfer_req_compl(hba);
> +	ufshcd_tmc_handler(hba);
> +}
> +
>  /**
>   * ufshcd_err_handler - handle UFS errors that require s/w attention
>   * @work: pointer to work structure
> @@ -3785,6 +3806,7 @@ static void ufshcd_err_handler(struct work_struct *work)
>  	u32 err_tm = 0;
>  	int err = 0;
>  	int tag;
> +	bool needs_reset = false;
>  
>  	hba = container_of(work, struct ufs_hba, eh_work);
>  
> @@ -3792,40 +3814,75 @@ static void ufshcd_err_handler(struct work_struct *work)
>  	ufshcd_hold(hba, false);
>  
>  	spin_lock_irqsave(hba->host->host_lock, flags);
> -	if (hba->ufshcd_state == UFSHCD_STATE_RESET) {
> -		spin_unlock_irqrestore(hba->host->host_lock, flags);
> +	if (hba->ufshcd_state == UFSHCD_STATE_RESET)
>  		goto out;
> -	}
>  
>  	hba->ufshcd_state = UFSHCD_STATE_RESET;
>  	ufshcd_set_eh_in_progress(hba);
>  
>  	/* Complete requests that have door-bell cleared by h/w */
> -	ufshcd_transfer_req_compl(hba);
> -	ufshcd_tmc_handler(hba);
> -	spin_unlock_irqrestore(hba->host->host_lock, flags);
> +	ufshcd_complete_requests(hba);
> +	if ((hba->saved_err & INT_FATAL_ERRORS) ||
> +	    ((hba->saved_err & UIC_ERROR) &&
> +	    (hba->saved_uic_err & (UFSHCD_UIC_DL_PA_INIT_ERROR |
> +				   UFSHCD_UIC_DL_NAC_RECEIVED_ERROR |
> +				   UFSHCD_UIC_DL_TCx_REPLAY_ERROR))))
> +		needs_reset = true;
>  
> +	/*
> +	 * if host reset is required then skip clearing the pending
> +	 * transfers forcefully because they will automatically get
> +	 * cleared after link startup.
> +	 */
> +	if (needs_reset)
> +		goto skip_pending_xfer_clear;
> +
> +	/* release lock as clear command might sleep */
> +	spin_unlock_irqrestore(hba->host->host_lock, flags);
>  	/* Clear pending transfer requests */
> -	for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs)
> -		if (ufshcd_clear_cmd(hba, tag))
> -			err_xfer |= 1 << tag;
> +	for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs) {
> +		if (ufshcd_clear_cmd(hba, tag)) {
> +			err_xfer = true;
> +			goto lock_skip_pending_xfer_clear;
> +		}
> +	}
>  
>  	/* Clear pending task management requests */
> -	for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs)
> -		if (ufshcd_clear_tm_cmd(hba, tag))
> -			err_tm |= 1 << tag;
> +	for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs) {
> +		if (ufshcd_clear_tm_cmd(hba, tag)) {
> +			err_tm = true;
> +			goto lock_skip_pending_xfer_clear;
> +		}
> +	}
>  
> -	/* Complete the requests that are cleared by s/w */
> +lock_skip_pending_xfer_clear:
>  	spin_lock_irqsave(hba->host->host_lock, flags);
> -	ufshcd_transfer_req_compl(hba);
> -	ufshcd_tmc_handler(hba);
> -	spin_unlock_irqrestore(hba->host->host_lock, flags);
>  
> +	/* Complete the requests that are cleared by s/w */
> +	ufshcd_complete_requests(hba);
> +
> +	if (err_xfer || err_tm)
> +		needs_reset = true;
> +
> +skip_pending_xfer_clear:
>  	/* Fatal errors need reset */
> -	if (err_xfer || err_tm || (hba->saved_err & INT_FATAL_ERRORS) ||
> -			((hba->saved_err & UIC_ERROR) &&
> -			 (hba->saved_uic_err & UFSHCD_UIC_DL_PA_INIT_ERROR))) {
> +	if (needs_reset) {
> +		unsigned long max_doorbells = (1UL << hba->nutrs) - 1;
> +
> +		/*
> +		 * ufshcd_reset_and_restore() does the link reinitialization
> +		 * which will need atleast one empty doorbell slot to send the
> +		 * device management commands (NOP and query commands).
> +		 * If there is no slot empty at this moment then free up last
> +		 * slot forcefully.
> +		 */
> +		if (hba->outstanding_reqs == max_doorbells)
> +			__ufshcd_transfer_req_compl(hba,
> +						    (1UL << (hba->nutrs - 1)));
> +
> +		spin_unlock_irqrestore(hba->host->host_lock, flags);
>  		err = ufshcd_reset_and_restore(hba);
> +		spin_lock_irqsave(hba->host->host_lock, flags);
>  		if (err) {
>  			dev_err(hba->dev, "%s: reset and restore failed\n",
>  					__func__);
Why don't you reserve a command slot for this case (ie reduce the number
of tags by one)?
That way you would always have at least one slot free, wouldn't you?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1346495 — Re: [PATCH v5 08/15] scsi: ufs: make error handling bit faster

> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
>> UFS driver's error handler forcefully tries to clear all the pending
>> requests. For each pending request in the queue, it waits 1 sec for it
>> to get cleared. If we have multiple requests in the queue then it's
>> possible that we might end up waiting for those many seconds before
>> resetting the host. But note that resetting host would any way clear
>> all the pending requests from the hardware. Hence this change skips
>> the forceful clear of the pending requests if we are anyway going to
>> reset the host (for fatal errors).
>>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 155
>> +++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 112 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 987cf27..dc096f1 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -133,9 +133,11 @@ enum {
>>  /* UFSHCD UIC layer error flags */
>>  enum {
>>  	UFSHCD_UIC_DL_PA_INIT_ERROR = (1 << 0), /* Data link layer error */
>> -	UFSHCD_UIC_NL_ERROR = (1 << 1), /* Network layer error */
>> -	UFSHCD_UIC_TL_ERROR = (1 << 2), /* Transport Layer error */
>> -	UFSHCD_UIC_DME_ERROR = (1 << 3), /* DME error */
>> +	UFSHCD_UIC_DL_NAC_RECEIVED_ERROR = (1 << 1), /* Data link layer error
>> */
>> +	UFSHCD_UIC_DL_TCx_REPLAY_ERROR = (1 << 2), /* Data link layer error */
>> +	UFSHCD_UIC_NL_ERROR = (1 << 3), /* Network layer error */
>> +	UFSHCD_UIC_TL_ERROR = (1 << 4), /* Transport Layer error */
>> +	UFSHCD_UIC_DME_ERROR = (1 << 5), /* DME error */
>>  };
>>
>>  /* Interrupt configuration options */
>> @@ -3465,31 +3467,18 @@ static void ufshcd_uic_cmd_compl(struct ufs_hba
>> *hba, u32 intr_status)
>>  }
>>
>>  /**
>> - * ufshcd_transfer_req_compl - handle SCSI and query command completion
>> + * __ufshcd_transfer_req_compl - handle SCSI and query command
>> completion
>>   * @hba: per adapter instance
>> + * @completed_reqs: requests to complete
>>   */
>> -static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
>> +static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
>> +					unsigned long completed_reqs)
>>  {
>>  	struct ufshcd_lrb *lrbp;
>>  	struct scsi_cmnd *cmd;
>> -	unsigned long completed_reqs;
>> -	u32 tr_doorbell;
>>  	int result;
>>  	int index;
>>
>> -	/* Resetting interrupt aggregation counters first and reading the
>> -	 * DOOR_BELL afterward allows us to handle all the completed requests.
>> -	 * In order to prevent other interrupts starvation the DB is read once
>> -	 * after reset. The down side of this solution is the possibility of
>> -	 * false interrupt if device completes another request after resetting
>> -	 * aggregation and before reading the DB.
>> -	 */
>> -	if (ufshcd_is_intr_aggr_allowed(hba))
>> -		ufshcd_reset_intr_aggr(hba);
>> -
>> -	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>> -	completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
>> -
>>  	for_each_set_bit(index, &completed_reqs, hba->nutrs) {
>>  		lrbp = &hba->lrb[index];
>>  		cmd = lrbp->cmd;
>> @@ -3519,6 +3508,31 @@ static void ufshcd_transfer_req_compl(struct
>> ufs_hba *hba)
>>  }
>>
>>  /**
>> + * ufshcd_transfer_req_compl - handle SCSI and query command completion
>> + * @hba: per adapter instance
>> + */
>> +static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
>> +{
>> +	unsigned long completed_reqs;
>> +	u32 tr_doorbell;
>> +
>> +	/* Resetting interrupt aggregation counters first and reading the
>> +	 * DOOR_BELL afterward allows us to handle all the completed requests.
>> +	 * In order to prevent other interrupts starvation the DB is read once
>> +	 * after reset. The down side of this solution is the possibility of
>> +	 * false interrupt if device completes another request after resetting
>> +	 * aggregation and before reading the DB.
>> +	 */
>> +	if (ufshcd_is_intr_aggr_allowed(hba))
>> +		ufshcd_reset_intr_aggr(hba);
>> +
>> +	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>> +	completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
>> +
>> +	__ufshcd_transfer_req_compl(hba, completed_reqs);
>> +}
>> +
>> +/**
>>   * ufshcd_disable_ee - disable exception event
>>   * @hba: per-adapter instance
>>   * @mask: exception event to disable
>> @@ -3773,6 +3787,13 @@ out:
>>  	return;
>>  }
>>
>> +/* Complete requests that have door-bell cleared */
>> +static void ufshcd_complete_requests(struct ufs_hba *hba)
>> +{
>> +	ufshcd_transfer_req_compl(hba);
>> +	ufshcd_tmc_handler(hba);
>> +}
>> +
>>  /**
>>   * ufshcd_err_handler - handle UFS errors that require s/w attention
>>   * @work: pointer to work structure
>> @@ -3785,6 +3806,7 @@ static void ufshcd_err_handler(struct work_struct
>> *work)
>>  	u32 err_tm = 0;
>>  	int err = 0;
>>  	int tag;
>> +	bool needs_reset = false;
>>
>>  	hba = container_of(work, struct ufs_hba, eh_work);
>>
>> @@ -3792,40 +3814,75 @@ static void ufshcd_err_handler(struct
>> work_struct *work)
>>  	ufshcd_hold(hba, false);
>>
>>  	spin_lock_irqsave(hba->host->host_lock, flags);
>> -	if (hba->ufshcd_state == UFSHCD_STATE_RESET) {
>> -		spin_unlock_irqrestore(hba->host->host_lock, flags);
>> +	if (hba->ufshcd_state == UFSHCD_STATE_RESET)
>>  		goto out;
>> -	}
>>
>>  	hba->ufshcd_state = UFSHCD_STATE_RESET;
>>  	ufshcd_set_eh_in_progress(hba);
>>
>>  	/* Complete requests that have door-bell cleared by h/w */
>> -	ufshcd_transfer_req_compl(hba);
>> -	ufshcd_tmc_handler(hba);
>> -	spin_unlock_irqrestore(hba->host->host_lock, flags);
>> +	ufshcd_complete_requests(hba);
>> +	if ((hba->saved_err & INT_FATAL_ERRORS) ||
>> +	    ((hba->saved_err & UIC_ERROR) &&
>> +	    (hba->saved_uic_err & (UFSHCD_UIC_DL_PA_INIT_ERROR |
>> +				   UFSHCD_UIC_DL_NAC_RECEIVED_ERROR |
>> +				   UFSHCD_UIC_DL_TCx_REPLAY_ERROR))))
>> +		needs_reset = true;
>>
>> +	/*
>> +	 * if host reset is required then skip clearing the pending
>> +	 * transfers forcefully because they will automatically get
>> +	 * cleared after link startup.
>> +	 */
>> +	if (needs_reset)
>> +		goto skip_pending_xfer_clear;
>> +
>> +	/* release lock as clear command might sleep */
>> +	spin_unlock_irqrestore(hba->host->host_lock, flags);
>>  	/* Clear pending transfer requests */
>> -	for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs)
>> -		if (ufshcd_clear_cmd(hba, tag))
>> -			err_xfer |= 1 << tag;
>> +	for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs) {
>> +		if (ufshcd_clear_cmd(hba, tag)) {
>> +			err_xfer = true;
>> +			goto lock_skip_pending_xfer_clear;
>> +		}
>> +	}
>>
>>  	/* Clear pending task management requests */
>> -	for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs)
>> -		if (ufshcd_clear_tm_cmd(hba, tag))
>> -			err_tm |= 1 << tag;
>> +	for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs) {
>> +		if (ufshcd_clear_tm_cmd(hba, tag)) {
>> +			err_tm = true;
>> +			goto lock_skip_pending_xfer_clear;
>> +		}
>> +	}
>>
>> -	/* Complete the requests that are cleared by s/w */
>> +lock_skip_pending_xfer_clear:
>>  	spin_lock_irqsave(hba->host->host_lock, flags);
>> -	ufshcd_transfer_req_compl(hba);
>> -	ufshcd_tmc_handler(hba);
>> -	spin_unlock_irqrestore(hba->host->host_lock, flags);
>>
>> +	/* Complete the requests that are cleared by s/w */
>> +	ufshcd_complete_requests(hba);
>> +
>> +	if (err_xfer || err_tm)
>> +		needs_reset = true;
>> +
>> +skip_pending_xfer_clear:
>>  	/* Fatal errors need reset */
>> -	if (err_xfer || err_tm || (hba->saved_err & INT_FATAL_ERRORS) ||
>> -			((hba->saved_err & UIC_ERROR) &&
>> -			 (hba->saved_uic_err & UFSHCD_UIC_DL_PA_INIT_ERROR))) {
>> +	if (needs_reset) {
>> +		unsigned long max_doorbells = (1UL << hba->nutrs) - 1;
>> +
>> +		/*
>> +		 * ufshcd_reset_and_restore() does the link reinitialization
>> +		 * which will need atleast one empty doorbell slot to send the
>> +		 * device management commands (NOP and query commands).
>> +		 * If there is no slot empty at this moment then free up last
>> +		 * slot forcefully.
>> +		 */
>> +		if (hba->outstanding_reqs == max_doorbells)
>> +			__ufshcd_transfer_req_compl(hba,
>> +						    (1UL << (hba->nutrs - 1)));
>> +
>> +		spin_unlock_irqrestore(hba->host->host_lock, flags);
>>  		err = ufshcd_reset_and_restore(hba);
>> +		spin_lock_irqsave(hba->host->host_lock, flags);
>>  		if (err) {
>>  			dev_err(hba->dev, "%s: reset and restore failed\n",
>>  					__func__);
> Why don't you reserve a command slot for this case (ie reduce the number
> of tags by one)?
> That way you would always have at least one slot free, wouldn't you?
>

Hello Hannes,

We are discussing here, a very-very rare scenario where 2 conditions must
co-exist:
1. a fatal error that requires the controller to be reset.
2. all slots are taken.

This 2 conditions, very rarely should happen together.
At that point, it would be better to free the last slot, than to save one
slot for this scenario.
Also, we should remember that reducing the queue-depth for the entire
usual operation, might be a performance hit at some point, where, for
example, a LUN has only 8 slots, will now have 7, which is a hit of 12.5%
of the potential parallelism. So, i would recommend to stick with the
above proposal that not only makes more sense when you estimate the
probability of the conditions to co-exist, but also was tested and proven
to be safe with no performance hit.

Regards,
Yaniv




> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke		      zSeries & Storage
> hare@suse.de			      +49 911 74053 688
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [next] | [standalone]

#1346509 — Re: [PATCH v5 08/15] scsi: ufs: make error handling bit faster

On 03/01/2016 05:56 PM, ygardi@codeaurora.org wrote:
>> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
>>> UFS driver's error handler forcefully tries to clear all the pending
>>> requests. For each pending request in the queue, it waits 1 sec for it
>>> to get cleared. If we have multiple requests in the queue then it's
>>> possible that we might end up waiting for those many seconds before
>>> resetting the host. But note that resetting host would any way clear
>>> all the pending requests from the hardware. Hence this change skips
>>> the forceful clear of the pending requests if we are anyway going to
>>> reset the host (for fatal errors).
>>>
>>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>>>
>>> ---
>>>  drivers/scsi/ufs/ufshcd.c | 155
>>> +++++++++++++++++++++++++++++++++-------------
>>>  1 file changed, 112 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index 987cf27..dc096f1 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -133,9 +133,11 @@ enum {
>>>  /* UFSHCD UIC layer error flags */
>>>  enum {
>>>  	UFSHCD_UIC_DL_PA_INIT_ERROR = (1 << 0), /* Data link layer error */
>>> -	UFSHCD_UIC_NL_ERROR = (1 << 1), /* Network layer error */
>>> -	UFSHCD_UIC_TL_ERROR = (1 << 2), /* Transport Layer error */
>>> -	UFSHCD_UIC_DME_ERROR = (1 << 3), /* DME error */
>>> +	UFSHCD_UIC_DL_NAC_RECEIVED_ERROR = (1 << 1), /* Data link layer error
>>> */
>>> +	UFSHCD_UIC_DL_TCx_REPLAY_ERROR = (1 << 2), /* Data link layer error */
>>> +	UFSHCD_UIC_NL_ERROR = (1 << 3), /* Network layer error */
>>> +	UFSHCD_UIC_TL_ERROR = (1 << 4), /* Transport Layer error */
>>> +	UFSHCD_UIC_DME_ERROR = (1 << 5), /* DME error */
>>>  };
>>>
>>>  /* Interrupt configuration options */
>>> @@ -3465,31 +3467,18 @@ static void ufshcd_uic_cmd_compl(struct ufs_hba
>>> *hba, u32 intr_status)
>>>  }
>>>
>>>  /**
>>> - * ufshcd_transfer_req_compl - handle SCSI and query command completion
>>> + * __ufshcd_transfer_req_compl - handle SCSI and query command
>>> completion
>>>   * @hba: per adapter instance
>>> + * @completed_reqs: requests to complete
>>>   */
>>> -static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
>>> +static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
>>> +					unsigned long completed_reqs)
>>>  {
>>>  	struct ufshcd_lrb *lrbp;
>>>  	struct scsi_cmnd *cmd;
>>> -	unsigned long completed_reqs;
>>> -	u32 tr_doorbell;
>>>  	int result;
>>>  	int index;
>>>
>>> -	/* Resetting interrupt aggregation counters first and reading the
>>> -	 * DOOR_BELL afterward allows us to handle all the completed requests.
>>> -	 * In order to prevent other interrupts starvation the DB is read once
>>> -	 * after reset. The down side of this solution is the possibility of
>>> -	 * false interrupt if device completes another request after resetting
>>> -	 * aggregation and before reading the DB.
>>> -	 */
>>> -	if (ufshcd_is_intr_aggr_allowed(hba))
>>> -		ufshcd_reset_intr_aggr(hba);
>>> -
>>> -	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>>> -	completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
>>> -
>>>  	for_each_set_bit(index, &completed_reqs, hba->nutrs) {
>>>  		lrbp = &hba->lrb[index];
>>>  		cmd = lrbp->cmd;
>>> @@ -3519,6 +3508,31 @@ static void ufshcd_transfer_req_compl(struct
>>> ufs_hba *hba)
>>>  }
>>>
>>>  /**
>>> + * ufshcd_transfer_req_compl - handle SCSI and query command completion
>>> + * @hba: per adapter instance
>>> + */
>>> +static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
>>> +{
>>> +	unsigned long completed_reqs;
>>> +	u32 tr_doorbell;
>>> +
>>> +	/* Resetting interrupt aggregation counters first and reading the
>>> +	 * DOOR_BELL afterward allows us to handle all the completed requests.
>>> +	 * In order to prevent other interrupts starvation the DB is read once
>>> +	 * after reset. The down side of this solution is the possibility of
>>> +	 * false interrupt if device completes another request after resetting
>>> +	 * aggregation and before reading the DB.
>>> +	 */
>>> +	if (ufshcd_is_intr_aggr_allowed(hba))
>>> +		ufshcd_reset_intr_aggr(hba);
>>> +
>>> +	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>>> +	completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
>>> +
>>> +	__ufshcd_transfer_req_compl(hba, completed_reqs);
>>> +}
>>> +
>>> +/**
>>>   * ufshcd_disable_ee - disable exception event
>>>   * @hba: per-adapter instance
>>>   * @mask: exception event to disable
>>> @@ -3773,6 +3787,13 @@ out:
>>>  	return;
>>>  }
>>>
>>> +/* Complete requests that have door-bell cleared */
>>> +static void ufshcd_complete_requests(struct ufs_hba *hba)
>>> +{
>>> +	ufshcd_transfer_req_compl(hba);
>>> +	ufshcd_tmc_handler(hba);
>>> +}
>>> +
>>>  /**
>>>   * ufshcd_err_handler - handle UFS errors that require s/w attention
>>>   * @work: pointer to work structure
>>> @@ -3785,6 +3806,7 @@ static void ufshcd_err_handler(struct work_struct
>>> *work)
>>>  	u32 err_tm = 0;
>>>  	int err = 0;
>>>  	int tag;
>>> +	bool needs_reset = false;
>>>
>>>  	hba = container_of(work, struct ufs_hba, eh_work);
>>>
>>> @@ -3792,40 +3814,75 @@ static void ufshcd_err_handler(struct
>>> work_struct *work)
>>>  	ufshcd_hold(hba, false);
>>>
>>>  	spin_lock_irqsave(hba->host->host_lock, flags);
>>> -	if (hba->ufshcd_state == UFSHCD_STATE_RESET) {
>>> -		spin_unlock_irqrestore(hba->host->host_lock, flags);
>>> +	if (hba->ufshcd_state == UFSHCD_STATE_RESET)
>>>  		goto out;
>>> -	}
>>>
>>>  	hba->ufshcd_state = UFSHCD_STATE_RESET;
>>>  	ufshcd_set_eh_in_progress(hba);
>>>
>>>  	/* Complete requests that have door-bell cleared by h/w */
>>> -	ufshcd_transfer_req_compl(hba);
>>> -	ufshcd_tmc_handler(hba);
>>> -	spin_unlock_irqrestore(hba->host->host_lock, flags);
>>> +	ufshcd_complete_requests(hba);
>>> +	if ((hba->saved_err & INT_FATAL_ERRORS) ||
>>> +	    ((hba->saved_err & UIC_ERROR) &&
>>> +	    (hba->saved_uic_err & (UFSHCD_UIC_DL_PA_INIT_ERROR |
>>> +				   UFSHCD_UIC_DL_NAC_RECEIVED_ERROR |
>>> +				   UFSHCD_UIC_DL_TCx_REPLAY_ERROR))))
>>> +		needs_reset = true;
>>>
>>> +	/*
>>> +	 * if host reset is required then skip clearing the pending
>>> +	 * transfers forcefully because they will automatically get
>>> +	 * cleared after link startup.
>>> +	 */
>>> +	if (needs_reset)
>>> +		goto skip_pending_xfer_clear;
>>> +
>>> +	/* release lock as clear command might sleep */
>>> +	spin_unlock_irqrestore(hba->host->host_lock, flags);
>>>  	/* Clear pending transfer requests */
>>> -	for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs)
>>> -		if (ufshcd_clear_cmd(hba, tag))
>>> -			err_xfer |= 1 << tag;
>>> +	for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs) {
>>> +		if (ufshcd_clear_cmd(hba, tag)) {
>>> +			err_xfer = true;
>>> +			goto lock_skip_pending_xfer_clear;
>>> +		}
>>> +	}
>>>
>>>  	/* Clear pending task management requests */
>>> -	for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs)
>>> -		if (ufshcd_clear_tm_cmd(hba, tag))
>>> -			err_tm |= 1 << tag;
>>> +	for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs) {
>>> +		if (ufshcd_clear_tm_cmd(hba, tag)) {
>>> +			err_tm = true;
>>> +			goto lock_skip_pending_xfer_clear;
>>> +		}
>>> +	}
>>>
>>> -	/* Complete the requests that are cleared by s/w */
>>> +lock_skip_pending_xfer_clear:
>>>  	spin_lock_irqsave(hba->host->host_lock, flags);
>>> -	ufshcd_transfer_req_compl(hba);
>>> -	ufshcd_tmc_handler(hba);
>>> -	spin_unlock_irqrestore(hba->host->host_lock, flags);
>>>
>>> +	/* Complete the requests that are cleared by s/w */
>>> +	ufshcd_complete_requests(hba);
>>> +
>>> +	if (err_xfer || err_tm)
>>> +		needs_reset = true;
>>> +
>>> +skip_pending_xfer_clear:
>>>  	/* Fatal errors need reset */
>>> -	if (err_xfer || err_tm || (hba->saved_err & INT_FATAL_ERRORS) ||
>>> -			((hba->saved_err & UIC_ERROR) &&
>>> -			 (hba->saved_uic_err & UFSHCD_UIC_DL_PA_INIT_ERROR))) {
>>> +	if (needs_reset) {
>>> +		unsigned long max_doorbells = (1UL << hba->nutrs) - 1;
>>> +
>>> +		/*
>>> +		 * ufshcd_reset_and_restore() does the link reinitialization
>>> +		 * which will need atleast one empty doorbell slot to send the
>>> +		 * device management commands (NOP and query commands).
>>> +		 * If there is no slot empty at this moment then free up last
>>> +		 * slot forcefully.
>>> +		 */
>>> +		if (hba->outstanding_reqs == max_doorbells)
>>> +			__ufshcd_transfer_req_compl(hba,
>>> +						    (1UL << (hba->nutrs - 1)));
>>> +
>>> +		spin_unlock_irqrestore(hba->host->host_lock, flags);
>>>  		err = ufshcd_reset_and_restore(hba);
>>> +		spin_lock_irqsave(hba->host->host_lock, flags);
>>>  		if (err) {
>>>  			dev_err(hba->dev, "%s: reset and restore failed\n",
>>>  					__func__);
>> Why don't you reserve a command slot for this case (ie reduce the number
>> of tags by one)?
>> That way you would always have at least one slot free, wouldn't you?
>>
> 
> Hello Hannes,
> 
> We are discussing here, a very-very rare scenario where 2 conditions must
> co-exist:
> 1. a fatal error that requires the controller to be reset.
> 2. all slots are taken.
> 
> This 2 conditions, very rarely should happen together.
> At that point, it would be better to free the last slot, than to save one
> slot for this scenario.
> Also, we should remember that reducing the queue-depth for the entire
> usual operation, might be a performance hit at some point, where, for
> example, a LUN has only 8 slots, will now have 7, which is a hit of 12.5%
> of the potential parallelism. So, i would recommend to stick with the
> above proposal that not only makes more sense when you estimate the
> probability of the conditions to co-exist, but also was tested and proven
> to be safe with no performance hit.
> 
Okay, good point. I didn't know how many slots there are.
But if you only got 8 you surely don't want to waste one.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1345267 — [PATCH v5 11/15] scsi: ufs: handle non spec compliant bkops behaviour by device

We are seeing that some devices are raising the urgent bkops exception
events even when BKOPS status doesn't indicate performace impacted or
critical. Handle these device by determining their urgent bkops status
at runtime.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 53 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/scsi/ufs/ufshcd.h |  6 ++++++
 2 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 394054e..d28c629 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3763,7 +3763,7 @@ out:
  */
 static int ufshcd_urgent_bkops(struct ufs_hba *hba)
 {
-	return ufshcd_bkops_ctrl(hba, BKOPS_STATUS_PERF_IMPACT);
+	return ufshcd_bkops_ctrl(hba, hba->urgent_bkops_lvl);
 }
 
 static inline int ufshcd_get_ee_status(struct ufs_hba *hba, u32 *status)
@@ -3772,6 +3772,43 @@ static inline int ufshcd_get_ee_status(struct ufs_hba *hba, u32 *status)
 			QUERY_ATTR_IDN_EE_STATUS, 0, 0, status);
 }
 
+static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
+{
+	int err;
+	u32 curr_status = 0;
+
+	if (hba->is_urgent_bkops_lvl_checked)
+		goto enable_auto_bkops;
+
+	err = ufshcd_get_bkops_status(hba, &curr_status);
+	if (err) {
+		dev_err(hba->dev, "%s: failed to get BKOPS status %d\n",
+				__func__, err);
+		goto out;
+	}
+
+	/*
+	 * We are seeing that some devices are raising the urgent bkops
+	 * exception events even when BKOPS status doesn't indicate performace
+	 * impacted or critical. Handle these device by determining their urgent
+	 * bkops status at runtime.
+	 */
+	if (curr_status < BKOPS_STATUS_PERF_IMPACT) {
+		dev_err(hba->dev, "%s: device raised urgent BKOPS exception for bkops status %d\n",
+				__func__, curr_status);
+		/* update the current status as the urgent bkops level */
+		hba->urgent_bkops_lvl = curr_status;
+		hba->is_urgent_bkops_lvl_checked = true;
+	}
+
+enable_auto_bkops:
+	err = ufshcd_enable_auto_bkops(hba);
+out:
+	if (err < 0)
+		dev_err(hba->dev, "%s: failed to handle urgent bkops %d\n",
+				__func__, err);
+}
+
 /**
  * ufshcd_exception_event_handler - handle exceptions raised by device
  * @work: pointer to work data
@@ -3795,12 +3832,10 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
 	}
 
 	status &= hba->ee_ctrl_mask;
-	if (status & MASK_EE_URGENT_BKOPS) {
-		err = ufshcd_urgent_bkops(hba);
-		if (err < 0)
-			dev_err(hba->dev, "%s: failed to handle urgent bkops %d\n",
-					__func__, err);
-	}
+
+	if (status & MASK_EE_URGENT_BKOPS)
+		ufshcd_bkops_exception_event_handler(hba);
+
 out:
 	pm_runtime_put_sync(hba->dev);
 	return;
@@ -4763,6 +4798,10 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
 
 	ufshcd_init_pwr_info(hba);
 
+	/* set the default level for urgent bkops */
+	hba->urgent_bkops_lvl = BKOPS_STATUS_PERF_IMPACT;
+	hba->is_urgent_bkops_lvl_checked = false;
+
 	/* UniPro link is active now */
 	ufshcd_set_link_active(hba);
 
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 77e79c0..65f29aa 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -383,6 +383,9 @@ struct ufs_init_prefetch {
  * @clk_list_head: UFS host controller clocks list node head
  * @pwr_info: holds current power mode
  * @max_pwr_info: keeps the device max valid pwm
+ * @urgent_bkops_lvl: keeps track of urgent bkops level for device
+ * @is_urgent_bkops_lvl_checked: keeps track if the urgent bkops level for
+ *  device is known or not.
  */
 struct ufs_hba {
 	void __iomem *mmio_base;
@@ -538,6 +541,9 @@ struct ufs_hba {
 	struct devfreq *devfreq;
 	struct ufs_clk_scaling clk_scaling;
 	bool is_sys_suspended;
+
+	enum bkops_status urgent_bkops_lvl;
+	bool is_urgent_bkops_lvl_checked;
 };
 
 /* Returns true if clocks can be gated. Otherwise false */
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[toc] | [prev] | [next] | [standalone]

#1345269 — [PATCH v5 02/15] scsi: ufs: avoid spurious UFS host controller interrupts

When control reaches to Linux UFS driver during UFS boot mode, UFS host
controller interrupt status/enable registers may have left over
settings.
In order to avoid any spurious interrupts due to these left overs,
it's important to clear these interrupt status/enable registers before
enabling UFS interrupt handling.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufshcd.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a8e42df..de7280c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5837,6 +5837,21 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	init_waitqueue_head(&hba->dev_cmd.tag_wq);
 
 	ufshcd_init_clk_gating(hba);
+
+	/*
+	 * In order to avoid any spurious interrupt immediately after
+	 * registering UFS controller interrupt handler, clear any pending UFS
+	 * interrupt status and disable all the UFS interrupts.
+	 */
+	ufshcd_writel(hba, ufshcd_readl(hba, REG_INTERRUPT_STATUS),
+		      REG_INTERRUPT_STATUS);
+	ufshcd_writel(hba, 0, REG_INTERRUPT_ENABLE);
+	/*
+	 * Make sure that UFS interrupts are disabled and any pending interrupt
+	 * status is cleared before registering UFS interrupt handler.
+	 */
+	mb();
+
 	/* IRQ registration */
 	err = devm_request_irq(dev, irq, ufshcd_intr, IRQF_SHARED, UFSHCD, hba);
 	if (err) {
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[toc] | [prev] | [next] | [standalone]

#1346371 — Re: [PATCH v5 02/15] scsi: ufs: avoid spurious UFS host controller interrupts

On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
> When control reaches to Linux UFS driver during UFS boot mode, UFS host
> controller interrupt status/enable registers may have left over
> settings.
> In order to avoid any spurious interrupts due to these left overs,
> it's important to clear these interrupt status/enable registers before
> enabling UFS interrupt handling.
> 
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
> 
> ---
>  drivers/scsi/ufs/ufshcd.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web