Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
From:   "hoyoung seo" <hy50.seo@samsung.com>
To:     "'Bart Van Assche'" <bvanassche@acm.org>,
        <linux-scsi@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <alim.akhtar@samsung.com>, <avri.altman@wdc.com>,
        <jejb@linux.ibm.com>, <martin.petersen@oracle.com>,
        <beanhuo@micron.com>, <kwangwon.min@samsung.com>,
        <kwmad.kim@samsung.com>, <sh425.lee@samsung.com>,
        <sc.suh@samsung.com>, <quic_nguyenb@quicinc.com>,
        <cpgs@samsung.com>
In-Reply-To: <633b3970-ddd4-4dc6-b586-96ecc5b86456@acm.org>
Subject: RE: [PATCH v1] scsi: ufs: core: Process abort completed command in
 MCQ mode
Date:   Mon, 6 Nov 2023 13:56:54 +0900
Message-ID: <028001da106d$a9ac9330$fd05b990$@samsung.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQI/NadRec19V/jm3RVDFrtg0RqXhwIoHnBGAuFKLQkCpak+jQLnFFpSr0kiZdA=
Content-Language: ko
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 102P
DLP-Filter: Pass
References: <CGME20231101084246epcas2p32ae15219878d1c31e7d8a14c22489519@epcas2p3.samsung.com>
        <20231101084504.79087-1-hy50.seo@samsung.com>
        <e1ed8776-d8ad-49f2-bf8f-2759256e33e9@acm.org>
        <000001da0d42$1a1c8410$4e558c30$@samsung.com>
        <633b3970-ddd4-4dc6-b586-96ecc5b86456@acm.org>
Precedence: bulk

> -----Original Message-----
> From: Bart Van Assche <bvanassche=40acm.org>
> Sent: Friday, November 3, 2023 4:37 AM
> To: hoyoung seo <hy50.seo=40samsung.com>; linux-scsi=40vger.kernel.org; l=
inux-
> kernel=40vger.kernel.org; alim.akhtar=40samsung.com; avri.altman=40wdc.co=
m;
> jejb=40linux.ibm.com; martin.petersen=40oracle.com; beanhuo=40micron.com;
> kwangwon.min=40samsung.com; kwmad.kim=40samsung.com; sh425.lee=40samsung.=
com;
> sc.suh=40samsung.com; quic_nguyenb=40quicinc.com; cpgs=40samsung.com
> Subject: Re: =5BPATCH v1=5D scsi: ufs: core: Process abort completed comm=
and
> in MCQ mode
>=20
>=20
> On 11/1/23 21:07, hoyoung seo wrote:
> > when the ufs host receives any error, the ufs driver executes the error=
-
> hander.
> > If the error-hendler attempts re-init, it must abort and organize
> unprocessed
> >   requests.
> > The above operation is the same for both MCQ/legacy mode.
> > However, in the MCQ mode, if b or c is included in the following
> > specs, the OCS is updated to aborted, which is different from the legac=
y
> mode.
> >
> > B. If the command is in the Submission Queue and not issued to the
> > device yet, the host controller will mark the command to be skipped in
> the Submission Queue.
> > The host controller will post to the Completion Queue to update the
> > OCS field with =E2=80=98ABORTED=E2=80=99.=0D=0A>=20>=20C.=20If=20the=20=
command=20is=20issued=20to=20the=20device=20already=20but=20there=20is=20no=
=0D=0A>=20>=20response=20yet=20from=20the=20device,=20the=20host=20software=
=20driver=20issue=20the=20Abort=0D=0A>=20task=20management=20function=0D=0A=
>=20>=20=20=20to=20the=20device=20for=20that=20command.=0D=0A>=20>=20Then=
=20the=20host=20driver=20set=20SQRTCy.ICU=20as=20=E2=80=981=E2=80=99=20to=
=20initiate=20the=20clean=20up=0D=0A>=20>=20the=20hardware=20resources.=20T=
he=20host=20controller=20will=20post=20to=20the=20Completion=0D=0A>=20Queue=
=20to=20update=20the=20OCS=0D=0A>=20>=20=20=20field=20with=20=E2=80=98ABORT=
ED=E2=80=99.=0D=0A>=20>=0D=0A>=20>=20Unlike=20legacy=20mode,=20this=20pheno=
menon=20causes=20unintended=20behavior.=20(As=0D=0A>=20>=20shown=20in=20the=
=20log=20below)=0D=0A>=20>=0D=0A>=20>=20=5B1:=20=20kworker/u20:2:23157=5D=
=20ufshcd_try_to_abort_task:=20cmd=20pending=20in=20the=0D=0A>=20>=20device=
.=20tag=20=3D=209=0D=0A>=20>=20=5B3:=20=20kworker/u20:2:23157=5D=20Aborting=
=20tag=209=20/=20CDB=200x2a=20succeeded=0D=0A>=20>=20=5B4:=20=20=20=20=20=
=20swapper/4:=20=20=20=200=5D=20sd=200:0:0:0:=20=5Bsda=5D=20tag=239=20UNKNO=
WN(0x2003)=20Result:=0D=0A>=20hostbyte=3D0x05=20driverbyte=3DDRIVER_OK=20cm=
d_age=3D0s=20//=20DID_ABORT=0D=0A>=20>=20=5B4:=20=20=20=20=20=20swapper/4:=
=20=20=20=200=5D=20sd=200:0:0:0:=20=5Bsda=5D=20tag=239=20CDB:=20opcode=3D0x=
2a=202a=2000=0D=0A>=2000=20d3=2002=2000=2000=2001=2000=2000=0D=0A>=20>=20=
=5B4:=20=20=20=20=20=20swapper/4:=20=20=20=200=5D=20I/O=20error,=20dev=20sd=
a,=20sector=20110628864=20op=0D=0A>=200x1:(WRITE)=20flags=200x800=20phys_se=
g=20256=20prio=20class=202=0D=0A>=20>=0D=0A>=20>=0D=0A>=20>=20For=20command=
s=20that=20have=20completed=20the=20abort=20operation=20in=20MCQ=20mode,=0D=
=0A>=20>=20since=20OCS=20has=20been=20updated=20to=20aborted,=20it=20seems=
=20that=20it=20will=20be=0D=0A>=20retransmitted=20only=0D=0A>=20>=20=20=20w=
hen=20it=20is=20made=20to=20REQUEUE.=0D=0A>=20=0D=0A>=20Hi=20Hoyoung,=0D=0A=
>=20=0D=0A>=20Thank=20you=20for=20having=20provided=20this=20clarification=
=20-=20this=20really=20helps.=0D=0A>=20=0D=0A>=20Regarding=20(B):=20I=20wou=
ld=20appreciate=20it=20if=20this=20patch=20would=20be=20reworked=20such=0D=
=0A>=20that=20no=20new=20'if=20(is_mcq_enabled(hba))'=20statements=20are=20=
introduced.=0D=0A>=20Has=20it=20been=20considered=20to=20modify=20ufshcd_mc=
q_sqe_search()=20such=20that=20it=20sets=0D=0A>=20the=20SCSI=20result=20to=
=20DID_REQUEUE=20<<=2016=20instead=20of=20modifying=0D=0A>=20ufshcd_transfe=
r_rsp_status()?=0D=0A>=20=0D=0A>=20Regarding=20(C):=20SQRTCy.ICU=20is=20onl=
y=20set=20by=20ufshcd_mcq_sq_cleanup()=20and=20the=0D=0A>=20only=20caller=
=20of=20that=20function=20is=20ufshcd_clear_cmd().=0D=0A>=20There=20is=20on=
ly=20one=20function=20that=20calls=20ufshcd_clear_cmd()=20for=20SCSI=20comm=
ands,=0D=0A>=20namely=20ufshcd_eh_device_reset_handler().=20The=20latter=20=
function=20should=20not=0D=0A>=20set=20the=20SCSI=20result=20code.=20All=20=
it=20should=20do=20is=20to=20abort=20all=20pending=0D=0A>=20commands.=20The=
=20SCSI=20error=20handler=20will=20resubmit=20all=20aborted=20commands.=0D=
=0A>=20=0D=0A>=20Thanks,=0D=0A>=20=0D=0A>=20Bart.=0D=0A=0D=0AHi,=0D=0A=0D=
=0AI=20do=20not=20understand=20what=20do=20you=20want.=0D=0A=0D=0ARegarding=
=20(B)->=20In=20the=20ufshcd_mcq_sqe_search(),=20just=20find=20desc_addr=20=
of=20the=20tag=20=0D=0Afor=20setting=20nullify=20in=20SQD.=20=0D=0AAfter=20=
that=20the=20host=20h/w=20checks=20the=20nullify=20during=20processes=20the=
=20SQD,=20=20=0D=0Aand=20sets=20the=20OCS=20to=20'aborted'.=0D=0A=0D=0ARega=
rding=20(C)->=20Also,=20when=20the=20ufs=20driver=20sets=20the=20SQRTCy.ICU=
=20to=201,=20=0D=0Athe=20host=20h/w=20updates=20the=20OCS=20to=20the=20Abor=
ted.=0D=0A=0D=0ASo=20the=20ufs=20driver=20s/w=20can't=20do=20anything=20unt=
il=20the=20OCS=20update.=0D=0Athe=20host=20h/w=20must=20update=20the=20OCS=
=20and=20then=20process=20the=20Aborted.=0D=0A=0D=0AWhat=20you=20want=20is=
=20not=20to=20arbitrarily=20modify=20the=20results=20for=20OCS?=0D=0ATheref=
ore,=20if=20we=20use=20the=20current=20MCQ,=20we=20will=20do=20something=20=
that=20=0D=0Awe=20do=20not=20intend=20to=20do.=0D=0ATherefore,=20the=20OCS=
=20processing=20code=20for=20mcq=20and=20legacy=20mode=20needs=20=0D=0Ato=
=20be=20disadvantageous.=0D=0A=0D=0AIf=20you=20do=20not=20want=20to=20use=
=20'if=20(is_mcq_enabled=20(hba)',=20=0D=0AI=20will=20upload=20the=20patch=
=20again=0D=0A=0D=0AThanks.=0D=0A=0D=0ABRs=20SEO.=0D=0A=0D=0A