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 (3):
scsi: ufs: introduce vendor isr
scsi: ufs: introduce force requeue
scsi: ufs: ufs-exynos: implement exynos isr
drivers/scsi/ufs/ufs-exynos.c | 84 ++++++++++++++++++++++++++++++++++++-------
drivers/scsi/ufs/ufshcd.c | 22 ++++++++++--
drivers/scsi/ufs/ufshcd.h | 2 ++
3 files changed, 93 insertions(+), 15 deletions(-)
--
2.7.4
This patch is to raise recovery in some abnormal
conditions using an vendor specific interrupt
for some cases, such as a situation 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 representative case is shown like below.
In the case, a broken UTRD entry, for internal
coherent problem or whatever, that had smaller value
of PRDT length than expected was transferred to the host.
So, the host raised an interrupt of transfer complete
even if device didn't finish its data transfer because
the host sees a fetched version of UTRD to determine
if data tranfer is over or not. Then the application level
seemed to recogize this as a sort of corruption and this
symptom led to boot failure.
[5: init: 1] init: Failed to copy /avb into /metadata/gsi/dsu/avb/: No such file or directory
[5: init: 1] init: [libfs_mgr]Created logical partition system on device /dev/block/dm-0
[4: init: 1] init: [libfs_mgr]Created logical partition vendor on device /dev/block/dm-1
[4: init: 1] init: [libfs_avb]total vbmeta size mismatch: 13504 (expected: 15616)
[4: init: 1] init: [libfs_avb]VerifyVbmetaImages failed
[4: init: 1] init: Failed to open AvbHandle: No such file or directory
If the issued command would be write, it could be a
disaster because some parts of its entire data chunk
may be not written or more than the chunk may be written.
Using the VENDOR_SPECIFIC_IS.RX_UPIU_HIT_ERROR, the host
can detect the abnormal condition right when an
DATA IN for offset bigger than expected for the broken value
(i.e. PRDT length in this case) is received, do something for
recovery and system will keep going safe.
Signed-off-by: Kiwoong Kim <[email protected]>
---
drivers/scsi/ufs/ufs-exynos.c | 84 ++++++++++++++++++++++++++++++++++++-------
1 file changed, 72 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/ufs/ufs-exynos.c b/drivers/scsi/ufs/ufs-exynos.c
index a14dd8c..b0c8843 100644
--- a/drivers/scsi/ufs/ufs-exynos.c
+++ b/drivers/scsi/ufs/ufs-exynos.c
@@ -21,6 +21,7 @@
#include "unipro.h"
#include "ufs-exynos.h"
+#include "../scsi_transport_api.h"
/*
* Exynos's Vendor specific registers for UFSHCI
@@ -31,6 +32,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 +77,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 +1011,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 +1024,34 @@ 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;
+ cpu_relax();
} 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 +1137,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 +1225,38 @@ 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)
+{
+ struct exynos_ufs *ufs = ufshcd_get_variant(hba);
+ u32 status;
+ unsigned long flags;
+
+ if (!hba->priv) return IRQ_HANDLED;
+ 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 transferring
+ * 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);
+ hba->force_reset = true;
+ hba->force_requeue = true;
+ scsi_schedule_eh(hba->host);
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+ 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 +1268,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.7.4
On 9/13/21 12:55 AM, 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.
>
> Kiwoong Kim (3):
> scsi: ufs: introduce vendor isr
> scsi: ufs: introduce force requeue
> scsi: ufs: ufs-exynos: implement exynos isr
>
> drivers/scsi/ufs/ufs-exynos.c | 84 ++++++++++++++++++++++++++++++++++++-------
> drivers/scsi/ufs/ufshcd.c | 22 ++++++++++--
> drivers/scsi/ufs/ufshcd.h | 2 ++
> 3 files changed, 93 insertions(+), 15 deletions(-)
The UFS protocol is standardized. Your employer has a representative in the
UFS standardization committee. Please work with that representative to
standardize this feature instead of adding non-standard extensions to the UFS
driver.
Thanks,
Bart.
Hi Bart,
On Mon, Sep 13, 2021 at 9:42 PM Bart Van Assche <[email protected]> wrote:
>
> On 9/13/21 12:55 AM, 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.
> >
> > Kiwoong Kim (3):
> > scsi: ufs: introduce vendor isr
> > scsi: ufs: introduce force requeue
> > scsi: ufs: ufs-exynos: implement exynos isr
> >
> > drivers/scsi/ufs/ufs-exynos.c | 84 ++++++++++++++++++++++++++++++++++++-------
> > drivers/scsi/ufs/ufshcd.c | 22 ++++++++++--
> > drivers/scsi/ufs/ufshcd.h | 2 ++
> > 3 files changed, 93 insertions(+), 15 deletions(-)
>
> The UFS protocol is standardized. Your employer has a representative in the
> UFS standardization committee. Please work with that representative to
> standardize this feature instead of adding non-standard extensions to the UFS
> driver.
>
Thanks for your input. Completely agree with you, in fact your suggestions
make sense to me. As a driver developer, surely we can take these concerns
to the IP designers and see how far we can get in terms of standardization.
That, however, is not something that can be accomplished overnight. My main
concern is, what about millions of devices which are already in the market?
UFS subsystem does support _vops_ to handle vendor specific hooks/modifications.
I am not saying we should always follow this path, but surely until
these deviations
are either fixed or become part of UFS standard itself, IMO.
Thanks!
> Thanks,
>
> Bart.
>
>
--
Regards,
Alim
On 9/13/21 12:55 AM, Kiwoong Kim wrote:
> This patch is to raise recovery in some abnormal
> conditions using an vendor specific interrupt
> for some cases, such as a situation 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 representative case is shown like below.
> In the case, a broken UTRD entry, for internal
> coherent problem or whatever, that had smaller value
> of PRDT length than expected was transferred to the host.
> So, the host raised an interrupt of transfer complete
> even if device didn't finish its data transfer because
> the host sees a fetched version of UTRD to determine
> if data tranfer is over or not. Then the application level
> seemed to recogize this as a sort of corruption and this
> symptom led to boot failure.
How can a UTRD entry be broken? Does that perhaps indicate memory
corruption at the host side? Working around host-side memory
corruption in a driver seems wrong to me. I think the root cause
of the memory corruption should be fixed.
> +static irqreturn_t exynos_ufs_isr(struct ufs_hba *hba)
> +{
> + struct exynos_ufs *ufs = ufshcd_get_variant(hba);
> + u32 status;
> + unsigned long flags;
> +
> + if (!hba->priv) return IRQ_HANDLED;
Please verify patches with checkpatch before posting these on the
linux-scsi mailing list. The above if-statement does not follow the
Linux kernel coding style.
> + if (status & RX_UPIU_HIT_ERROR) {
> + pr_err("%s: status: 0x%08x\n", __func__, status);
> + hba->force_reset = true;
> + hba->force_requeue = true;
> + scsi_schedule_eh(hba->host);
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> + return IRQ_HANDLED;
> + }
> + return IRQ_NONE;
> +}
So the above code unlocks the host_lock depending on whether or not
status & RX_UPIU_HIT_ERROR is true? Yikes ...
Additionally, in the above code I found the following pattern:
unsigned long flags;
[ ... ]
spin_unlock_irqrestore(hba->host->host_lock, flags);
Such code is ALWAYS wrong. The value of the 'flags' argument passed to
spin_unlock_irqrestore() must come from spin_lock_irqsave().
Bart.
On 9/13/21 10:26, Alim Akhtar wrote:
> Thanks for your input. Completely agree with you, in fact your
> suggestions make sense to me. As a driver developer, surely we can
> take these concerns to the IP designers and see how far we can get in
> terms of standardization. That, however, is not something that can be
> accomplished overnight. My main concern is, what about millions of
> devices which are already in the market? UFS subsystem does support
> _vops_ to handle vendor specific hooks/modifications. I am not saying
> we should always follow this path, but surely until these deviations
> are either fixed or become part of UFS standard itself, IMO.
Hi Alim,
If there are already millions of devices in the market that support this
feature then that's an argument to proceed with this patch series.
Thanks,
Bart.
> On 9/13/21 12:55 AM, Kiwoong Kim wrote:
> > This patch is to raise recovery in some abnormal conditions using an
> > vendor specific interrupt for some cases, such as a situation 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 representative case is shown like below.
> > In the case, a broken UTRD entry, for internal coherent problem or
> > whatever, that had smaller value of PRDT length than expected was
> > transferred to the host.
> > So, the host raised an interrupt of transfer complete even if device
> > didn't finish its data transfer because the host sees a fetched
> > version of UTRD to determine if data tranfer is over or not. Then the
> > application level seemed to recogize this as a sort of corruption and
> > this symptom led to boot failure.
>
> How can a UTRD entry be broken? Does that perhaps indicate memory
> corruption at the host side? Working around host-side memory corruption in
> a driver seems wrong to me. I think the root cause of the memory
> corruption should be fixed.
For SoC internal problems, yes, of course, they should be fixed.
But I don't think the causes always come from inside the system.
They could be outside the system or a device, such as sending DATA IN
with a tag that a host has ever submitted a command with because of
some bugs of the device. You might think putting this sort of code doesn't
make sense but there could be various events that can't be understood in the
point of view of the spec. And chips that is already fab-outed can't be fixed.
That's why I put the details into Exynos. I'm not talking about the spec.
I think Exynos isn't required to contain only things related with the spec
and it can have realistic part.
>
> > +static irqreturn_t exynos_ufs_isr(struct ufs_hba *hba) {
> > + struct exynos_ufs *ufs = ufshcd_get_variant(hba);
> > + u32 status;
> > + unsigned long flags;
> > +
> > + if (!hba->priv) return IRQ_HANDLED;
>
> Please verify patches with checkpatch before posting these on the linux-
> scsi mailing list. The above if-statement does not follow the Linux kernel
> coding style.
>
> > + if (status & RX_UPIU_HIT_ERROR) {
> > + pr_err("%s: status: 0x%08x\n", __func__, status);
> > + hba->force_reset = true;
> > + hba->force_requeue = true;
> > + scsi_schedule_eh(hba->host);
> > + spin_unlock_irqrestore(hba->host->host_lock, flags);
> > + return IRQ_HANDLED;
> > + }
> > + return IRQ_NONE;
> > +}
>
> So the above code unlocks the host_lock depending on whether or not status
> & RX_UPIU_HIT_ERROR is true? Yikes ...
>
> Additionally, in the above code I found the following pattern:
>
> unsigned long flags;
> [ ... ]
> spin_unlock_irqrestore(hba->host->host_lock, flags);
>
> Such code is ALWAYS wrong. The value of the 'flags' argument passed to
> spin_unlock_irqrestore() must come from spin_lock_irqsave().
>
> Bart.
I missed for two things. Thanks, I'll look more carefully.
Hi,
> +static irqreturn_t exynos_ufs_isr(struct ufs_hba *hba) {
> + struct exynos_ufs *ufs = ufshcd_get_variant(hba);
> + u32 status;
> + unsigned long flags;
> +
> + if (!hba->priv) return IRQ_HANDLED;
> + 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 transferring
> + * 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);
> + hba->force_reset = true;
> + hba->force_requeue = true;
If force_reset is true, isn't force_requeue redundant?
Thanks,
Avri
> + scsi_schedule_eh(hba->host);
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> + 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 +1268,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.7.4