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
