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

[Kiwoong Kim]

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