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