Re: [PATCH v1] ufs: introduce UFSHCI_QUIRK_SKIP_INTR_AGGR quirk

[Subhash Jadavani <subhashj@codeaurora.org>]

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