Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Kiwoong Kim" <kwmad.kim@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>, <adrian.hunter@intel.com>,
        <sc.suh@samsung.com>, <hy50.seo@samsung.com>,
        <sh425.lee@samsung.com>, <bhoon95.kim@samsung.com>,
        <kwangwon.min@samsung.com>, <junwoo80.lee@samsung.com>
In-Reply-To: <60542301-e77f-ad26-e249-29d42f446d74@acm.org>
Subject: RE: [PATCH v2] ufs: poll pmc until another pa request is completed
Date:   Thu, 11 May 2023 09:26:24 +0900
Message-ID: <039b01d9839f$37cef8d0$a76cea70$@samsung.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Language: ko
Thread-Index: AQN1t83238AtSis63Xat9rXUxX7L4wHvqgOrAXsXcCusAGySUA==
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 102P
DLP-Filter: Pass
References: <CGME20230425012857epcas2p45eaa3f5f87de424bbf951b22653b3e70@epcas2p4.samsung.com>
        <1682385635-43601-1-git-send-email-kwmad.kim@samsung.com>
        <60542301-e77f-ad26-e249-29d42f446d74@acm.org>
Precedence: bulk

> > =40=40 -4138,6 +4141,61 =40=40 int ufshcd_dme_get_attr(struct ufs_hba *=
hba, u32
> attr_sel,
> >   =7D
> >   EXPORT_SYMBOL_GPL(ufshcd_dme_get_attr);
> >
> > +static int __ufshcd_poll_uic_pwr(struct ufs_hba *hba, struct
> uic_command *cmd,
> > +		struct completion *cnf)
>=20
> What does the name =22cnf=22 mean? To me it seems to be a weird name for =
a
> completion function pointer.

'cnf' is a term used in Unipro spec and I thought it's good to use terms in=
 the spec, especially in this file.
ufshcd.c is an implementation of UFS and its related specifications.

It's a notification meaning that UFS host's Unipro HW receives a UIC reques=
t from the host.
I guess maybe 'cnf' stands for 'confirm' but I thought 'confirm' look a lit=
tle bit abstract.

If you have an better idea of naming it, please let me know.

>=20
> > +=7B
> > +	unsigned long flags;
> > +	int ret;
> > +	ktime_t timeout;
> > +	u32 mode =3D cmd->argument3;
>=20
> Is my understanding correct that __ufshcd_send_uic_cmd() does not modify
> cmd->argument3? If so, why does this function copy cmd->argument3 and
> re-assign cmd->argument3?

This is for the case when unipro responds w/ busy(09h).
When IS.UCCS is enabled and is raised, UFS driver updates cmd->argumen3.
With this patch, it will go through the loop again w/ an unexpected value o=
f cmd->argumen3.

>=20
> > +	timeout =3D ktime_add_ms(ktime_get(), UIC_PA_RDY_TIMEOUT);
>=20
> =22deadline=22 is probably a better name for this variable than =22timeou=
t=22.
> Additionally, please consider using jiffies since I think that the
> accuracy of the jiffies counter is sufficient in this context.
>=20
> > +	do =7B
> > +		spin_lock_irqsave(hba->host->host_lock, flags);
> > +		hba->active_uic_cmd =3D NULL;
>=20
> Is my understanding correct that it is guaranteed that
> hba->active_uic_cmd is NULL here? If so, what is the purpose of the
> above statement?

Yeah, putting 'hba->active_uic_cmd =3D NULL' after wait_for_completion_time=
out looks natural.
But you can see there is one goto case w/ a UIC command not issued for UIC =
not ready, i.e. =21ufshcd_ready_for_uic_cmd.
To cover it together, 'hba->active_uic_cmd =3D NULL' has to be also put at =
the end of this function
and even wrapped w/ the spin lock. I wanted to reduce LOC and found a perio=
d already wrapped by the spin lock.
That is, it has the same result, I thought.

>=20
> > +		ret =3D __ufshcd_send_uic_cmd(hba, cmd, true);
> > +		spin_unlock_irqrestore(hba->host->host_lock, flags);
> > +		if (ret) =7B
> > +			dev_err(hba->dev,
> > +				=22pwr ctrl cmd 0x%x with mode 0x%x uic
> error %d=5Cn=22,
> > +				cmd->command, cmd->argument3, ret);
> > +			goto out;
> > +		=7D
> > +
> > +		/* This value is heuristic */
> > +		if (=21wait_for_completion_timeout(&cmd->done,
> > +		    msecs_to_jiffies(5))) =7B
>=20
> Please align msecs_to_jiffies(5) with the first argument (=22&cmd->done=
=22).
>=20
> > +			ret =3D -ETIMEDOUT;
> > +			dev_err(hba->dev,
> > +				=22pwr ctrl cmd 0x%x with mode 0x%x timeout=5Cn=22,
> > +				cmd->command, cmd->argument3);
> > +			if (cmd->cmd_active)
> > +				goto out;
> > +
> > +			dev_info(hba->dev, =22%s: pwr ctrl cmd has already been
> completed=5Cn=22, __func__);
> > +		=7D
> > +
> > +		/* retry for only busy cases */
>=20
> Please fix the word order in the above comment (for only -> only for)
>=20
> Thanks,
>=20
> Bart.=20

For others, let me change it.

Thanks.
Kiwoong Kim