linux-kernel - RE: [RFC PATCH v1 2/2] scsi: ufs: ufs-exynos: implement exynos isr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <000001d78cf0$9eb0de30$dc129a90$@samsung.com>
Date:   Mon, 9 Aug 2021 16:31:50 +0900
From:   "Kiwoong Kim" <kwmad.kim@...sung.com>
To:     "'Bart Van Assche'" <bvanassche@....org>,
        <linux-scsi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <alim.akhtar@...sung.com>, <avri.altman@....com>,
        <jejb@...ux.ibm.com>, <martin.petersen@...cle.com>,
        <beanhuo@...ron.com>, <cang@...eaurora.org>,
        <adrian.hunter@...el.com>, <sc.suh@...sung.com>,
        <hy50.seo@...sung.com>, <sh425.lee@...sung.com>,
        <bhoon95.kim@...sung.com>
Subject: RE: [RFC PATCH v1 2/2] scsi: ufs: ufs-exynos: implement exynos isr

> 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.