RE: [EXT] [PATCH v1 2/3] scsi: ufs: add error handling of auto-hibern8

[Stanley Chu <stanley.chu@mediatek.com>]

Hi Bean,

On Tue, 2019-05-14 at 11:14 +0000, Bean Huo (beanhuo) wrote:
> Hi, Stanley
> Thanks for reply.
> 
> >
> >On Mon, 2019-05-13 at 18:21 +0000, Bean Huo (beanhuo) wrote:
> >> Hi, Stanley
> >>
> >> >+
> >> >+static inline bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
> >> >+						u32 intr_mask)
> >> >+{
> >> >+	return (ufshcd_is_auto_hibern8_supported(hba) &&
> >> >+		!hba->uic_async_done &&
> >>
> >> Here check if uic_async_done is NULL, no big problem so far, but not safe
> >enough.
> >> How about setting a flag in ufshcd_auto_hibern8_enable(),
> >
> >>
> >> I concern about how to compatible with auto_hibern8 disabled condition.
> >
> >Currently auto-hibern8 disabling method is not implemented in mainstream,
> >so an "enabling" flag may looks redundant unless disabling path is really
> >existed.
> >
> Did you try to update Auto-Hibernate Idle Timer with 0 through '/sys'  (scsi: ufs: Add support for Auto-Hibernate Idle Timer)? 
> I don't know if this will disable your UFS controller Auto-Hibernate.
> If having a look at UFS host Spec, software writes “0” to disable Auto-Hibernate Idle Timer.
> Sorry I cannot verify this on my platform since it doesn't support auto-hibernate.
> 

Sorry I missed this /sys interface for Auto-Hibernate control.

Yes, I have tested "Auto-Hibernate disabled" case, in this case,
UIC_HIBERNATE_ENTER and UIC_HIBERNATE_EXIT interrupts comes only if
Manual-Hibernate is performed and waiting for completion. Both
interrupts will not be identified as Auto-Hibernate errors by checking
hba->uic_async_done.

As for your concerning, I would like to make "Auto-Hibernate error
detection" more precise in next version: Use below conditions instead of
checking hba->uic_async_done:

As-is:

static inline bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
						u32 intr_mask)
{
	return (ufshcd_is_auto_hibern8_supported(hba) &&
		!hba->uic_async_done &&
		(intr_mask & UFSHCD_UIC_AH8_ERROR_MASK));
}

To-be:

static bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
						u32 intr_mask)
{
	if (!ufshcd_is_auto_hibern8_supported(hba))
		return false;

	if (!(intr_mask & UFSHCD_UIC_AH8_ERROR_MASK))
		return false;

	if (hba->active_uic_cmd &&
	    ((hba->active_uic_cmd->command == UIC_CMD_DME_HIBER_ENTER) ||
	    (hba->active_uic_cmd->command == UIC_CMD_DME_HIBER_EXIT)))
		return false;

	return true;
}

What would you think about this change?

> 
> >I agree that checking hba->uic_async_done here does not look so intuitive.
> >However even if auto-hibern8 is disabled, these checks could be safe enough
> >because both "UIC_HIBERNATE_ENTER" and "UIC_HIBERNATE_EXIT" are
> >raised only if "manual-hibernate" is performed, and in this case hba-
> >>uic_async_done shall be true.
> >
> Yes, most of cases ,this is no problem.
> 
> >Anything else or corner case I missed?
> >
> The others are fine. I only concern checking hba->uic_async_done.
> 
> //Bean

Many thanks,
Stanley




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel