linux-kernel - RE: [PATCH v2] ufs: poll pmc until another pa request is completed

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] [day] [month] [year] [list]

Message-ID: <039b01d9839f$37cef8d0$a76cea70$@samsung.com>
Date:   Thu, 11 May 2023 09:26:24 +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>, <adrian.hunter@...el.com>,
        <sc.suh@...sung.com>, <hy50.seo@...sung.com>,
        <sh425.lee@...sung.com>, <bhoon95.kim@...sung.com>,
        <kwangwon.min@...sung.com>, <junwoo80.lee@...sung.com>
Subject: RE: [PATCH v2] ufs: poll pmc until another pa request is completed

> > @@ -4138,6 +4141,61 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, u32
> attr_sel,
> >   }
> >   EXPORT_SYMBOL_GPL(ufshcd_dme_get_attr);
> >
> > +static int __ufshcd_poll_uic_pwr(struct ufs_hba *hba, struct
> uic_command *cmd,
> > +		struct completion *cnf)
> 
> What does the name "cnf" 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 request from the host.
I guess maybe 'cnf' stands for 'confirm' but I thought 'confirm' look a little bit abstract.

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

> 
> > +{
> > +	unsigned long flags;
> > +	int ret;
> > +	ktime_t timeout;
> > +	u32 mode = cmd->argument3;
> 
> 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 of cmd->argumen3.

> 
> > +	timeout = ktime_add_ms(ktime_get(), UIC_PA_RDY_TIMEOUT);
> 
> "deadline" is probably a better name for this variable than "timeout".
> Additionally, please consider using jiffies since I think that the
> accuracy of the jiffies counter is sufficient in this context.
> 
> > +	do {
> > +		spin_lock_irqsave(hba->host->host_lock, flags);
> > +		hba->active_uic_cmd = NULL;
> 
> 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 = NULL' after wait_for_completion_timeout looks natural.
But you can see there is one goto case w/ a UIC command not issued for UIC not ready, i.e. !ufshcd_ready_for_uic_cmd.
To cover it together, 'hba->active_uic_cmd = 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 period already wrapped by the spin lock.
That is, it has the same result, I thought.

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

For others, let me change it.

Thanks.
Kiwoong Kim