This patch is to activate some interrupt sources
that aren't defined in UFSHCI specifications. Those
purpose could be error handling, workaround or whatever.
Kiwoong Kim (2):
scsi: ufs: introduce vendor isr
scsi: ufs: ufs-exynos: implement exynos isr
drivers/scsi/ufs/ufs-exynos.c | 78 +++++++++++++++++++++++++++++------
drivers/scsi/ufs/ufshcd.c | 10 +++++
drivers/scsi/ufs/ufshcd.h | 8 ++++
3 files changed, 84 insertions(+), 12 deletions(-)
--
2.31.1
Based on some events in the real world, I implement
this to block the host's working in some abnormal
conditions using an vendor specific interrupt for
cases that some contexts of a pending request in the
host isn't the same with those of its corresponding UPIUs
if they should have been the same exactly.
Signed-off-by: Kiwoong Kim <[email protected]>
---
drivers/scsi/ufs/ufs-exynos.c | 78 +++++++++++++++++++++++++++++------
1 file changed, 66 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/ufs/ufs-exynos.c b/drivers/scsi/ufs/ufs-exynos.c
index cf46d6f86e0e..2cfe959e05b6 100644
--- a/drivers/scsi/ufs/ufs-exynos.c
+++ b/drivers/scsi/ufs/ufs-exynos.c
@@ -31,6 +31,8 @@
#define HCI_RXPRDT_ENTRY_SIZE 0x04
#define HCI_1US_TO_CNT_VAL 0x0C
#define CNT_VAL_1US_MASK 0x3FF
+#define VENDOR_SPECIFIC_IS 0x38
+#define VENDOR_SPECIFIC_IE 0x3C
#define HCI_UTRL_NEXUS_TYPE 0x40
#define HCI_UTMRL_NEXUS_TYPE 0x44
#define HCI_SW_RST 0x50
@@ -74,6 +76,18 @@
UIC_TRANSPORT_NO_CONNECTION_RX |\
UIC_TRANSPORT_BAD_TC)
+enum exynos_ufs_vs_interrupt {
+ /*
+ * This occurs when information of a pending request isn't
+ * the same with incoming UPIU for the request. For example,
+ * if UFS driver rings with task tag #1, subsequential UPIUs
+ * for this must have one as the value of task tag. But if
+ * it's corrutped until the host receives it or incoming UPIUs
+ * has an unexpected value for task tag, this raises.
+ */
+ RX_UPIU_HIT_ERROR = 1 << 19,
+};
+
enum {
UNIPRO_L1_5 = 0,/* PHY Adapter */
UNIPRO_L2, /* Data Link */
@@ -996,6 +1010,10 @@ static int exynos_ufs_init(struct ufs_hba *hba)
goto out;
exynos_ufs_specify_phy_time_attr(ufs);
exynos_ufs_config_smu(ufs);
+
+ /* enable vendor interrupts */
+ hci_writel(ufs, VENDOR_SPECIFIC_IE, RX_UPIU_HIT_ERROR);
+
return 0;
phy_off:
@@ -1005,26 +1023,33 @@ static int exynos_ufs_init(struct ufs_hba *hba)
return ret;
}
-static int exynos_ufs_host_reset(struct ufs_hba *hba)
+static int exynos_ufs_host_reset(struct exynos_ufs *ufs)
{
- struct exynos_ufs *ufs = ufshcd_get_variant(hba);
unsigned long timeout = jiffies + msecs_to_jiffies(1);
- u32 val;
- int ret = 0;
-
- exynos_ufs_disable_auto_ctrl_hcc_save(ufs, &val);
+ /* host reset */
hci_writel(ufs, UFS_SW_RST_MASK, HCI_SW_RST);
-
do {
if (!(hci_readl(ufs, HCI_SW_RST) & UFS_SW_RST_MASK))
- goto out;
+ return 0;
} while (time_before(jiffies, timeout));
- dev_err(hba->dev, "timeout host sw-reset\n");
- ret = -ETIMEDOUT;
+ pr_err("timeout host sw-reset\n");
+ return -ETIMEDOUT;
+}
+
+static int exynos_ufs_host_init(struct ufs_hba *hba)
+{
+ struct exynos_ufs *ufs = ufshcd_get_variant(hba);
+ u32 val;
+ int ret;
+
+ exynos_ufs_disable_auto_ctrl_hcc_save(ufs, &val);
+
+ ret = exynos_ufs_host_reset(ufs);
+ if (ret)
+ return ret;
-out:
exynos_ufs_auto_ctrl_hcc_restore(ufs, &val);
return ret;
}
@@ -1110,7 +1135,7 @@ static int exynos_ufs_hce_enable_notify(struct ufs_hba *hba,
switch (status) {
case PRE_CHANGE:
- ret = exynos_ufs_host_reset(hba);
+ ret = exynos_ufs_host_init(hba);
if (ret)
return ret;
exynos_ufs_dev_hw_reset(hba);
@@ -1198,6 +1223,34 @@ static int exynos_ufs_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
return 0;
}
+static irqreturn_t exynos_ufs_isr(struct ufs_hba *hba, int *res)
+{
+ struct exynos_ufs *ufs = ufshcd_get_variant(hba);
+ u32 status;
+
+ status = hci_readl(ufs, VENDOR_SPECIFIC_IS);
+ hci_writel(ufs, status, VENDOR_SPECIFIC_IS);
+ /*
+ * If host doesn't guarantee integrity of UTP transmission,
+ * it needs to be reset immediately to make it unable to
+ * proceed requests. Because w/o this, if UTP functions
+ * in host doesn't work properly for such system power margins,
+ * DATA IN from broken devices or whatever in the real world,
+ * some unexpected events could happen, such as tranferring
+ * a broken DATA IN to a device. It could be various types of
+ * problems on the level of file system. In this case, I think
+ * blocking the host's functionality is the best strategy.
+ * Perhaps, if its root cause is temporary, system could recover.
+ */
+ if (status & RX_UPIU_HIT_ERROR) {
+ pr_err("%s: status: 0x%08x\n", __func__, status);
+ exynos_ufs_host_reset(ufs);
+ *res = 1;
+ return IRQ_HANDLED;
+ }
+ return IRQ_NONE;
+}
+
static struct ufs_hba_variant_ops ufs_hba_exynos_ops = {
.name = "exynos_ufs",
.init = exynos_ufs_init,
@@ -1209,6 +1262,7 @@ static struct ufs_hba_variant_ops ufs_hba_exynos_ops = {
.hibern8_notify = exynos_ufs_hibern8_notify,
.suspend = exynos_ufs_suspend,
.resume = exynos_ufs_resume,
+ .intr = exynos_ufs_isr,
};
static int exynos_ufs_probe(struct platform_device *pdev)
--
2.31.1
This patch is to activate some interrupt sources
that aren't defined in UFSHCI specifications. Those
purpose could be error handling, workaround or whatever.
Signed-off-by: Kiwoong Kim <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 10 ++++++++++
drivers/scsi/ufs/ufshcd.h | 8 ++++++++
2 files changed, 18 insertions(+)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 05495c34a2b7..f85a9b335e0b 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6428,6 +6428,16 @@ static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba)
static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
{
irqreturn_t retval = IRQ_NONE;
+ int res = 0;
+ unsigned long flags;
+
+ retval = ufshcd_vops_intr(hba, &res);
+ if (res) {
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ hba->force_reset = true;
+ ufshcd_schedule_eh_work(hba);
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+ }
if (intr_status & UFSHCD_UIC_MASK)
retval |= ufshcd_uic_cmd_compl(hba, intr_status);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 971cfabc4a1e..1ed0a911f864 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -356,6 +356,7 @@ struct ufs_hba_variant_ops {
const union ufs_crypto_cfg_entry *cfg, int slot);
void (*event_notify)(struct ufs_hba *hba,
enum ufs_event_type evt, void *data);
+ irqreturn_t (*intr)(struct ufs_hba *hba, int *res);
};
/* clock gating state */
@@ -1296,6 +1297,13 @@ static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
hba->vops->config_scaling_param(hba, profile, data);
}
+static inline irqreturn_t ufshcd_vops_intr(struct ufs_hba *hba, int *res)
+{
+ if (hba->vops && hba->vops->intr)
+ return hba->vops->intr(hba, res);
+ return IRQ_NONE;
+}
+
extern struct ufs_pm_lvl_states ufs_pm_lvl_states[];
/*
--
2.31.1
On 8/5/21 11:34 PM, Kiwoong Kim wrote:
> This patch is to activate some interrupt sources
> that aren't defined in UFSHCI specifications. Those
> purpose could be error handling, workaround or whatever.
>
> Signed-off-by: Kiwoong Kim <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 10 ++++++++++
> drivers/scsi/ufs/ufshcd.h | 8 ++++++++
> 2 files changed, 18 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 05495c34a2b7..f85a9b335e0b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6428,6 +6428,16 @@ static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba)
> static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
> {
> irqreturn_t retval = IRQ_NONE;
> + int res = 0;
> + unsigned long flags;
> +
> + retval = ufshcd_vops_intr(hba, &res);
> + if (res) {
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + hba->force_reset = true;
> + ufshcd_schedule_eh_work(hba);
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> + }
How can a non-standard extension have error handling code in common
code? Please move the code under if (res) into the vendor code.
> if (intr_status & UFSHCD_UIC_MASK)
> retval |= ufshcd_uic_cmd_compl(hba, intr_status);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 971cfabc4a1e..1ed0a911f864 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -356,6 +356,7 @@ struct ufs_hba_variant_ops {
> const union ufs_crypto_cfg_entry *cfg, int slot);
> void (*event_notify)(struct ufs_hba *hba,
> enum ufs_event_type evt, void *data);
> + irqreturn_t (*intr)(struct ufs_hba *hba, int *res);
> };
>
> /* clock gating state */
> @@ -1296,6 +1297,13 @@ static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
> hba->vops->config_scaling_param(hba, profile, data);
> }
>
> +static inline irqreturn_t ufshcd_vops_intr(struct ufs_hba *hba, int *res)
> +{
> + if (hba->vops && hba->vops->intr)
> + return hba->vops->intr(hba, res);
> + return IRQ_NONE;
> +}
> +
> extern struct ufs_pm_lvl_states ufs_pm_lvl_states[];
So this code adds an indirect function call in the interrupt handler?
This will have a negative impact on performance, especially on a kernel
with security mitigations enabled. See also
https://lwn.net/Articles/774743/.
Thanks,
Bart.
On 8/5/21 11:34 PM, Kiwoong Kim wrote:
> This patch is to activate some interrupt sources
> that aren't defined in UFSHCI specifications. Those
> purpose could be error handling, workaround or whatever.
How about extending the UFS spec instead of adding a non-standard
mechanism in a driver that is otherwise based on a standard?
Thanks,
Bart.
On 8/5/21 11:34 PM, Kiwoong Kim wrote:
> Based on some events in the real world
Which events? Please clarify.
> I implement
> this to block the host's working in some abnormal
> conditions using an vendor specific interrupt for
> cases that some contexts of a pending request in the
> host isn't the same with those of its corresponding UPIUs
> if they should have been the same exactly.
The entire patch description sounds very vague to me. Please make the
description more clear.
> +enum exynos_ufs_vs_interrupt {
> + /*
> + * This occurs when information of a pending request isn't
> + * the same with incoming UPIU for the request. For example,
> + * if UFS driver rings with task tag #1, subsequential UPIUs
> + * for this must have one as the value of task tag. But if
> + * it's corrutped until the host receives it or incoming UPIUs
> + * has an unexpected value for task tag, this raises.
> + */
> + RX_UPIU_HIT_ERROR = 1 << 19,
> +};
The above description needs to be improved. If a request is submitted
with task tag one, only one UPIU can have that task tag instead of all
subsequent UPIUs.
> hci_writel(ufs, UFS_SW_RST_MASK, HCI_SW_RST);
> -
> do {
> if (!(hci_readl(ufs, HCI_SW_RST) & UFS_SW_RST_MASK))
> - goto out;
> + return 0;
> } while (time_before(jiffies, timeout));
Since the above loop is a busy-waiting loop, please insert an msleep()
or cpu_relax() call.
> + * some unexpected events could happen, such as tranferring
^^^^^^^^^^^
Please fix the spelling of this word.
Thanks,
Bart.
>
> On 8/5/21 11:34 PM, Kiwoong Kim wrote:
> > This patch is to activate some interrupt sources
> > that aren't defined in UFSHCI specifications. Those
> > purpose could be error handling, workaround or whatever.
>
> How about extending the UFS spec instead of adding a non-standard
> mechanism in a driver that is otherwise based on a standard?
The variant ops IMO (which he rightfully used), should allow that extra freedom.
Thanks,
Avri
>
> Thanks,
>
> Bart.
> On 8/5/21 11:34 PM, Kiwoong Kim wrote:
> > Based on some events in the real world
>
> Which events? Please clarify.
>
> > I implement
> > this to block the host's working in some abnormal conditions using an
> > vendor specific interrupt for cases that some contexts of a pending
> > request in the host isn't the same with those of its corresponding
> > UPIUs if they should have been the same exactly.
>
> The entire patch description sounds very vague to me. Please make the
> description more clear.
I'll describe a bit clearer in the next version.
>
> > +enum exynos_ufs_vs_interrupt {
> > + /*
> > + * This occurs when information of a pending request isn't
> > + * the same with incoming UPIU for the request. For example,
> > + * if UFS driver rings with task tag #1, subsequential UPIUs
> > + * for this must have one as the value of task tag. But if
> > + * it's corrutped until the host receives it or incoming UPIUs
> > + * has an unexpected value for task tag, this raises.
> > + */
> > + RX_UPIU_HIT_ERROR = 1 << 19,
> > +};
>
> The above description needs to be improved. If a request is submitted with
> task tag one, only one UPIU can have that task tag instead of all
> subsequent UPIUs.
Thank you for your opinion. In an ideal situation where there is no negative impact
from outside the host, yes, you're right, but in the real world, it could be not.
Let me give you one representative example.
There has been some events that a host has one as tag number of a pending request
that should have been originally two, or a device sends a UPIU with two of tag number even
when a host has one as tag number of its corresponding request.
I remember that the first case occurred because of integrity problems
from such as lack of voltage margin of a specific power domain or whatever
and the second case did because of malfunctions of the device.
If those events are temporary, it might not raise some errors.
That means delivering wrong data to file system could be also possible
even if they're rare, and its consequences would be unpredictable, I think.
Speaking the point of view of UFS specifications, yes, those events should
not happen. Now I'm just trying to make the driver fit with the real situations,
especially for cases with abnormal conditions.
In this case, my choice is to block the host's operations and
these situations could be architecture-specific.
>
> > hci_writel(ufs, UFS_SW_RST_MASK, HCI_SW_RST);
> > -
> > do {
> > if (!(hci_readl(ufs, HCI_SW_RST) & UFS_SW_RST_MASK))
> > - goto out;
> > + return 0;
> > } while (time_before(jiffies, timeout));
>
> Since the above loop is a busy-waiting loop, please insert an msleep() or
> cpu_relax() call.
>
> > + * some unexpected events could happen, such as tranferring
> ^^^^^^^^^^^ Please fix the
> spelling of this word.
Got it.
>
> Thanks,
>
> Bart.
> On 8/5/21 11:34 PM, Kiwoong Kim wrote:
> > This patch is to activate some interrupt sources that aren't defined
> > in UFSHCI specifications. Those purpose could be error handling,
> > workaround or whatever.
> >
> > Signed-off-by: Kiwoong Kim <[email protected]>
> > ---
> > drivers/scsi/ufs/ufshcd.c | 10 ++++++++++
> > drivers/scsi/ufs/ufshcd.h | 8 ++++++++
> > 2 files changed, 18 insertions(+)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 05495c34a2b7..f85a9b335e0b 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -6428,6 +6428,16 @@ static irqreturn_t ufshcd_tmc_handler(struct
> ufs_hba *hba)
> > static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
> > {
> > irqreturn_t retval = IRQ_NONE;
> > + int res = 0;
> > + unsigned long flags;
> > +
> > + retval = ufshcd_vops_intr(hba, &res);
> > + if (res) {
> > + spin_lock_irqsave(hba->host->host_lock, flags);
> > + hba->force_reset = true;
> > + ufshcd_schedule_eh_work(hba);
> > + spin_unlock_irqrestore(hba->host->host_lock, flags);
> > + }
>
> How can a non-standard extension have error handling code in common code?
> Please move the code under if (res) into the vendor code.
Got it.
>
> > if (intr_status & UFSHCD_UIC_MASK)
> > retval |= ufshcd_uic_cmd_compl(hba, intr_status); diff --git
> > a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index
> > 971cfabc4a1e..1ed0a911f864 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -356,6 +356,7 @@ struct ufs_hba_variant_ops {
> > const union ufs_crypto_cfg_entry *cfg, int slot);
> > void (*event_notify)(struct ufs_hba *hba,
> > enum ufs_event_type evt, void *data);
> > + irqreturn_t (*intr)(struct ufs_hba *hba, int *res);
> > };
> >
> > /* clock gating state */
> > @@ -1296,6 +1297,13 @@ static inline void
> ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
> > hba->vops->config_scaling_param(hba, profile, data);
> > }
> >
> > +static inline irqreturn_t ufshcd_vops_intr(struct ufs_hba *hba, int
> > +*res) {
> > + if (hba->vops && hba->vops->intr)
> > + return hba->vops->intr(hba, res);
> > + return IRQ_NONE;
> > +}
> > +
> > extern struct ufs_pm_lvl_states ufs_pm_lvl_states[];
>
> So this code adds an indirect function call in the interrupt handler?
> This will have a negative impact on performance, especially on a kernel
> with security mitigations enabled. See also
> https://protect2.fireeye.com/v1/url?k=fe14d1e9-a18fe89c-fe155aa6-
> 0cc47a31ce4e-8200591154f8c42c&q=1&e=7cf22799-390c-4209-8a19-
> 6ad1fa5fa811&u=https%3A%2F%2Flwn.net%2FArticles%2F774743%2F.
Interesting. I'll refer to this. Thanks.
>
> Thanks,
>
> Bart.
> How about extending the UFS spec instead of adding a non-standard
> mechanism in a driver that is otherwise based on a standard?
It seems to be a great approach but I wonder if extending for the events
that all the SoC vendors require in the spec is recommendable.
Because I think there is quite possible that many of those things are
originated for architectural reasons.
On 8/9/21 12:46 AM, Kiwoong Kim wrote:
>> How about extending the UFS spec instead of adding a non-standard
>> mechanism in a driver that is otherwise based on a standard?
>
> It seems to be a great approach but I wonder if extending for the events
> that all the SoC vendors require in the spec is recommendable.
> Because I think there is quite possible that many of those things are
> originated for architectural reasons.
Has the interrupt mechanism supported by this patch series already been
implemented or is it still possible to change the ASIC design? In the
latter case, I propose the following:
* Drop the new interrupt.
* Instead of raising an interrupt if the UFS controller detects an
inconsistency, report this via a check condition code, e.g. LOGICAL UNIT
NOT READY, HARD RESET REQUIRED (there may be a better choice).
The above approach has the advantage that it does not slow down the UFS
interrupt handler.
Thanks,
Bart.
> On 8/9/21 12:46 AM, Kiwoong Kim wrote:
> >> How about extending the UFS spec instead of adding a non-standard
> >> mechanism in a driver that is otherwise based on a standard?
> >
> > It seems to be a great approach but I wonder if extending for the
> > events that all the SoC vendors require in the spec is recommendable.
> > Because I think there is quite possible that many of those things are
> > originated for architectural reasons.
>
> Has the interrupt mechanism supported by this patch series already been
> implemented or is it still possible to change the ASIC design? In the
The former case. It has been included since mass production of the first SoC
supporting UFS for the first time.
> latter case, I propose the following:
> * Drop the new interrupt.
> * Instead of raising an interrupt if the UFS controller detects an
> inconsistency, report this via a check condition code, e.g. LOGICAL UNIT
> NOT READY, HARD RESET REQUIRED (there may be a better choice).
>
> The above approach has the advantage that it does not slow down the UFS
> interrupt handler.
>
> Thanks,
>
> Bart.
>