* [PATCH v1] ufs: introduce UFSHCI_QUIRK_SKIP_INTR_AGGR quirk
@ 2016-11-08 7:59 Kiwoong Kim
2016-11-08 19:56 ` Subhash Jadavani
0 siblings, 1 reply; 5+ messages in thread
From: Kiwoong Kim @ 2016-11-08 7:59 UTC (permalink / raw)
To: James E.J. Bottomley, linux-scsi, Martin K. Petersen,
vinholikatti
Cc: 추헌광
If UFS driver resets interrupt aggregation timer and counter
when there is a pending doorbell, an interrupt of IO completion
of a corresponding task may be missed.
That would cause a command timeout.
If UFS driver resets interrupt aggregation timer and counter
when there is a pending doorbell, a competion interrupt
of a corresponing task may be issued.
That would casue a command timeout.
Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
---
drivers/scsi/ufs/ufshcd.c | 4 +++-
drivers/scsi/ufs/ufshcd.h | 1 +
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c904854..65bbf59 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3730,7 +3730,9 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
* false interrupt if device completes another request after resetting
* aggregation and before reading the DB.
*/
- if (ufshcd_is_intr_aggr_allowed(hba))
+ if ((ufshcd_is_intr_aggr_allowed(hba))
+ && !(hba->quirks & UFSHCI_QUIRK_SKIP_INTR_AGGR))
+ ufshcd_reset_intr_aggr(hba);
ufshcd_reset_intr_aggr(hba);
tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 6a96f24..6a9c6e9 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -497,6 +497,7 @@ struct ufs_hba {
#define UFSHCD_QUIRK_BROKEN_DWORD_UTRD UFS_BIT(7)
#define UFSHCD_QUIRK_BROKEN_REQ_LIST_CLR UFS_BIT(8)
#define UFSHCD_QUIRK_USE_OF_HCE UFS_BIT(9)
+ #define UFSHCI_QUIRK_SKIP_INTR_AGGR UFS_BIT(10)
unsigned int quirks; /* Deviations from standard UFSHCI spec. */
--
2.1.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v1] ufs: introduce UFSHCI_QUIRK_SKIP_INTR_AGGR quirk
2016-11-08 7:59 [PATCH v1] ufs: introduce UFSHCI_QUIRK_SKIP_INTR_AGGR quirk Kiwoong Kim
@ 2016-11-08 19:56 ` Subhash Jadavani
2016-11-09 5:43 ` Kiwoong Kim
2016-11-10 1:28 ` Kiwoong Kim
0 siblings, 2 replies; 5+ messages in thread
From: Subhash Jadavani @ 2016-11-08 19:56 UTC (permalink / raw)
To: Kiwoong Kim
Cc: James E.J. Bottomley, linux-scsi, Martin K. Petersen,
vinholikatti, 추헌광, linux-scsi-owner
On 2016-11-07 23:59, Kiwoong Kim wrote:
> If UFS driver resets interrupt aggregation timer and counter
> when there is a pending doorbell, an interrupt of IO completion
> of a corresponding task may be missed.
> That would cause a command timeout.
>
> If UFS driver resets interrupt aggregation timer and counter
> when there is a pending doorbell, a competion interrupt
> of a corresponing task may be issued.
> That would casue a command timeout.
>
> Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> ---
> drivers/scsi/ufs/ufshcd.c | 4 +++-
> drivers/scsi/ufs/ufshcd.h | 1 +
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c904854..65bbf59 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3730,7 +3730,9 @@ static void ufshcd_transfer_req_compl(struct
> ufs_hba *hba)
> * false interrupt if device completes another request after
> resetting
> * aggregation and before reading the DB.
> */
> - if (ufshcd_is_intr_aggr_allowed(hba))
> + if ((ufshcd_is_intr_aggr_allowed(hba))
> + && !(hba->quirks & UFSHCI_QUIRK_SKIP_INTR_AGGR))
Why do we need this new quirk? If controller has the issue with
interrupt aggregation, you can remove this UFSHCD_CAP_INTR_AGGR to
disable the aggregation altogether.
> + ufshcd_reset_intr_aggr(hba);
> ufshcd_reset_intr_aggr(hba);
>
> tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 6a96f24..6a9c6e9 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -497,6 +497,7 @@ struct ufs_hba {
> #define UFSHCD_QUIRK_BROKEN_DWORD_UTRD UFS_BIT(7)
> #define UFSHCD_QUIRK_BROKEN_REQ_LIST_CLR UFS_BIT(8)
> #define UFSHCD_QUIRK_USE_OF_HCE UFS_BIT(9)
> + #define UFSHCI_QUIRK_SKIP_INTR_AGGR UFS_BIT(10)
>
>
> unsigned int quirks; /* Deviations from standard UFSHCI spec. */
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH v1] ufs: introduce UFSHCI_QUIRK_SKIP_INTR_AGGR quirk
2016-11-08 19:56 ` Subhash Jadavani
@ 2016-11-09 5:43 ` Kiwoong Kim
2016-11-09 18:59 ` Subhash Jadavani
2016-11-10 1:28 ` Kiwoong Kim
1 sibling, 1 reply; 5+ messages in thread
From: Kiwoong Kim @ 2016-11-09 5:43 UTC (permalink / raw)
To: 'Subhash Jadavani'
Cc: 'James E.J. Bottomley', linux-scsi,
'Martin K. Petersen', vinholikatti, '???',
linux-scsi-owner
> > If UFS driver resets interrupt aggregation timer and counter
> > when there is a pending doorbell, an interrupt of IO completion
> > of a corresponding task may be missed.
> > That would cause a command timeout.
> >
> > If UFS driver resets interrupt aggregation timer and counter
> > when there is a pending doorbell, a competion interrupt
> > of a corresponing task may be issued.
> > That would casue a command timeout.
> >
> > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> > ---
> > drivers/scsi/ufs/ufshcd.c | 4 +++-
> > drivers/scsi/ufs/ufshcd.h | 1 +
> > 2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index c904854..65bbf59 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -3730,7 +3730,9 @@ static void ufshcd_transfer_req_compl(struct
> > ufs_hba *hba)
> > * false interrupt if device completes another request after
> > resetting
> > * aggregation and before reading the DB.
> > */
> > - if (ufshcd_is_intr_aggr_allowed(hba))
> > + if ((ufshcd_is_intr_aggr_allowed(hba))
> > + && !(hba->quirks & UFSHCI_QUIRK_SKIP_INTR_AGGR))
>
> Why do we need this new quirk? If controller has the issue with
> interrupt aggregation, you can remove this UFSHCD_CAP_INTR_AGGR to
> disable the aggregation altogether.
This fragment doesn't mean disabling interrupt aggregation.
It just means that interrupt aggregation logic would not be reset in ISR.
But I think that the name of the quirk doesn't represent the function of it
expectedly.
I'll change the name with another one, such as
UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR.
>
> > + ufshcd_reset_intr_aggr(hba);
> > ufshcd_reset_intr_aggr(hba);
> >
> > tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> > index 6a96f24..6a9c6e9 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -497,6 +497,7 @@ struct ufs_hba {
> > #define UFSHCD_QUIRK_BROKEN_DWORD_UTRD UFS_BIT(7)
> > #define UFSHCD_QUIRK_BROKEN_REQ_LIST_CLR UFS_BIT(8)
> > #define UFSHCD_QUIRK_USE_OF_HCE UFS_BIT(9)
> > + #define UFSHCI_QUIRK_SKIP_INTR_AGGR UFS_BIT(10)
> >
> >
> > unsigned int quirks; /* Deviations from standard UFSHCI spec.
> */
>
> --
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> --
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v1] ufs: introduce UFSHCI_QUIRK_SKIP_INTR_AGGR quirk
2016-11-09 5:43 ` Kiwoong Kim
@ 2016-11-09 18:59 ` Subhash Jadavani
0 siblings, 0 replies; 5+ messages in thread
From: Subhash Jadavani @ 2016-11-09 18:59 UTC (permalink / raw)
To: Kiwoong Kim
Cc: 'James E.J. Bottomley', linux-scsi,
'Martin K. Petersen', vinholikatti, '???',
linux-scsi-owner
On 2016-11-08 21:43, Kiwoong Kim wrote:
>> > If UFS driver resets interrupt aggregation timer and counter
>> > when there is a pending doorbell, an interrupt of IO completion
>> > of a corresponding task may be missed.
>> > That would cause a command timeout.
>> >
>> > If UFS driver resets interrupt aggregation timer and counter
>> > when there is a pending doorbell, a competion interrupt
>> > of a corresponing task may be issued.
>> > That would casue a command timeout.
Please add how are you fixing the issue in commit text.
>> >
>> > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
>> > ---
>> > drivers/scsi/ufs/ufshcd.c | 4 +++-
>> > drivers/scsi/ufs/ufshcd.h | 1 +
>> > 2 files changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> > index c904854..65bbf59 100644
>> > --- a/drivers/scsi/ufs/ufshcd.c
>> > +++ b/drivers/scsi/ufs/ufshcd.c
>> > @@ -3730,7 +3730,9 @@ static void ufshcd_transfer_req_compl(struct
>> > ufs_hba *hba)
>> > * false interrupt if device completes another request after
>> > resetting
>> > * aggregation and before reading the DB.
>> > */
>> > - if (ufshcd_is_intr_aggr_allowed(hba))
>> > + if ((ufshcd_is_intr_aggr_allowed(hba))
>> > + && !(hba->quirks & UFSHCI_QUIRK_SKIP_INTR_AGGR))
>>
>> Why do we need this new quirk? If controller has the issue with
>> interrupt aggregation, you can remove this UFSHCD_CAP_INTR_AGGR to
>> disable the aggregation altogether.
>
> This fragment doesn't mean disabling interrupt aggregation.
> It just means that interrupt aggregation logic would not be reset in
> ISR.
ok.
>
> But I think that the name of the quirk doesn't represent the function
> of it
> expectedly.
> I'll change the name with another one, such as
> UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR.
May be UFSHCD_QUIRK_BROKEN_INTR_AGGR_RESET ?
>
>
>>
>> > + ufshcd_reset_intr_aggr(hba);
>> > ufshcd_reset_intr_aggr(hba);
>> >
>> > tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>> > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> > index 6a96f24..6a9c6e9 100644
>> > --- a/drivers/scsi/ufs/ufshcd.h
>> > +++ b/drivers/scsi/ufs/ufshcd.h
>> > @@ -497,6 +497,7 @@ struct ufs_hba {
>> > #define UFSHCD_QUIRK_BROKEN_DWORD_UTRD UFS_BIT(7)
>> > #define UFSHCD_QUIRK_BROKEN_REQ_LIST_CLR UFS_BIT(8)
>> > #define UFSHCD_QUIRK_USE_OF_HCE UFS_BIT(9)
>> > + #define UFSHCI_QUIRK_SKIP_INTR_AGGR UFS_BIT(10)
Please also add code comments explaining what this quirk means and when
should we use it? Please add same for other quirks you introduced in
different patches.
>> >
>> >
>> > unsigned int quirks; /* Deviations from standard UFSHCI spec.
>> */
>>
>> --
>> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>> --
>> 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
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v1] ufs: introduce UFSHCI_QUIRK_SKIP_INTR_AGGR quirk
2016-11-08 19:56 ` Subhash Jadavani
2016-11-09 5:43 ` Kiwoong Kim
@ 2016-11-10 1:28 ` Kiwoong Kim
1 sibling, 0 replies; 5+ messages in thread
From: Kiwoong Kim @ 2016-11-10 1:28 UTC (permalink / raw)
To: 'Subhash Jadavani'
Cc: 'James E.J. Bottomley', linux-scsi,
'Martin K. Petersen', vinholikatti, '???',
linux-scsi-owner, cpgs
Hi, Subhash
I'll update this patch referring what you said (including adding comments of
other quirks)
> > If UFS driver resets interrupt aggregation timer and counter when
> > there is a pending doorbell, an interrupt of IO completion of a
> > corresponding task may be missed.
> > That would cause a command timeout.
> >
> > If UFS driver resets interrupt aggregation timer and counter when
> > there is a pending doorbell, a competion interrupt of a corresponing
> > task may be issued.
> > That would casue a command timeout.
> >
> > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> > ---
> > drivers/scsi/ufs/ufshcd.c | 4 +++-
> > drivers/scsi/ufs/ufshcd.h | 1 +
> > 2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index c904854..65bbf59 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -3730,7 +3730,9 @@ static void ufshcd_transfer_req_compl(struct
> > ufs_hba *hba)
> > * false interrupt if device completes another request after
> > resetting
> > * aggregation and before reading the DB.
> > */
> > - if (ufshcd_is_intr_aggr_allowed(hba))
> > + if ((ufshcd_is_intr_aggr_allowed(hba))
> > + && !(hba->quirks & UFSHCI_QUIRK_SKIP_INTR_AGGR))
>
> Why do we need this new quirk? If controller has the issue with interrupt
> aggregation, you can remove this UFSHCD_CAP_INTR_AGGR to disable the
> aggregation altogether.
>
> > + ufshcd_reset_intr_aggr(hba);
> > ufshcd_reset_intr_aggr(hba);
> >
> > tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> > index 6a96f24..6a9c6e9 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -497,6 +497,7 @@ struct ufs_hba {
> > #define UFSHCD_QUIRK_BROKEN_DWORD_UTRD UFS_BIT(7)
> > #define UFSHCD_QUIRK_BROKEN_REQ_LIST_CLR UFS_BIT(8)
> > #define UFSHCD_QUIRK_USE_OF_HCE UFS_BIT(9)
> > + #define UFSHCI_QUIRK_SKIP_INTR_AGGR UFS_BIT(10)
> >
> >
> > unsigned int quirks; /* Deviations from standard UFSHCI spec.
> */
>
> --
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux Foundation Collaborative Project
> --
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-11-10 1:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-08 7:59 [PATCH v1] ufs: introduce UFSHCI_QUIRK_SKIP_INTR_AGGR quirk Kiwoong Kim
2016-11-08 19:56 ` Subhash Jadavani
2016-11-09 5:43 ` Kiwoong Kim
2016-11-09 18:59 ` Subhash Jadavani
2016-11-10 1:28 ` Kiwoong Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).