| Message ID | cover.1631519695.git.kwmad.kim@samsung.com |
|---|---|
| Headers | show |
| Series | scsi: ufs: introduce vendor isr | expand |
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 <bvanassche@acm.org> 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. > >
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 00:55, Kiwoong Kim wrote: > +static inline irqreturn_t > +ufshcd_vendor_isr_def(struct ufs_hba *hba) > +{ > + return IRQ_NONE; > +} Since "static inline irqreturn_t ufshcd_vendor_isr_def(struct ufs_hba *hba)" occupies less than 80 columns please use a single line for the declaration of this function. Additionally, please leave out the "inline" keyword since modern compilers are good at deciding when to inline a function and when not. 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.
> Since "static inline irqreturn_t ufshcd_vendor_isr_def(struct ufs_hba > *hba)" occupies less than 80 columns please use a single line for the > declaration of this function. Additionally, please leave out the "inline" > keyword since modern compilers are good at deciding when to inline a > function and when not. Got it. Thanks.
> On 9/13/21 00:55, Kiwoong Kim wrote: > > +static inline irqreturn_t > > +ufshcd_vendor_isr_def(struct ufs_hba *hba) > > +{ > > + return IRQ_NONE; > > +} > > Since "static inline irqreturn_t ufshcd_vendor_isr_def(struct ufs_hba > *hba)" occupies less than 80 columns please use a single line for the > declaration of this function. btw, It is 100 now. > > Thanks, > > Bart.
On 9/14/21 4:53 AM, Avri Altman wrote: >> Since "static inline irqreturn_t ufshcd_vendor_isr_def(struct ufs_hba >> *hba)" occupies less than 80 columns please use a single line for the >> declaration of this function. > > btw, It is 100 now. Are you sure? In Documentation/process/coding-style.rst I found the following: The preferred limit on the length of a single line is 80 columns. From the commit message of bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning"): Yes, staying withing 80 columns is certainly still _preferred_. But it's not the hard limit that the checkpatch warnings imply, and other concerns can most certainly dominate. Increase the default limit to 100 characters. Not because 100 characters is some hard limit either, but that's certainly a "what are you doing" kind of value and less likely to be about the occasional slightly longer lines. Bart.
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