Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt

[Jim Quinlan <jim2101024@gmail.com>]

> From: Sudeep Holla <sudeep.holla@arm.com>
> Date: Tue, Jan 5, 2021 at 12:35 PM
> Subject: Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
> To: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Jim Quinlan <jim2101024@gmail.com>, Sudeep Holla <sudeep.holla@arm.com>, <bcm-kernel-feedback-list@broadcom.com>, <james.quinlan@broadcom.com>, open list:SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE Mes... <linux-arm-kernel@lists.infradead.org>, open list <linux-kernel@vger.kernel.org>
>
>
> On Tue, Dec 22, 2020 at 07:37:22PM -0800, Florian Fainelli wrote:
> >
> >
> > On 12/22/2020 6:56 AM, Jim Quinlan wrote:
> > > The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
> > > message to be indicated by an interrupt rather than the return of the smc
> > > call.  This accommodates the existing behavior of the BrcmSTB SCMI
> > > "platform" whose SW is already out in the field and cannot be changed.
> > >
> > > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> >
> > This looks good to me, just one question below:
> >
> > [snip]
> >
> > > @@ -111,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> > >     shmem_tx_prepare(scmi_info->shmem, xfer);
> > >
> > >     arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
> > > +   if (scmi_info->irq)
> > > +           wait_for_completion(&scmi_info->tx_complete);
> >
> > Do we need this to have a preceding call to reinit_completion()? It does
> > not look like this is going to make any practical difference but there
> > are drivers doing that for correctness.
>
> Why do you think that might not cause any issue ? After first message
> is completed and ISR is executed, the completion flag remains done for
> ever.
Hi Sudeep,

I don't think that is the case;  the bottom routine,
do_wait_for_common(), decrements the x->done after a completion (which
does an increment).  Regardless, I think it is prudent to add the
reinit patch you've provided below.

BTW, one thing I could have done was incorporate the timeout value but
I thought that since this is the "fast" call we shouldn't be timing
out at all.

Thanks much,
Jim Quinlan
Broadcom STB



So practically 2nd message onwards won't block in wait_for_completion
> which means return from smc/hvc is actually completion too which is clearly
> wrong or am I missing something ?
>
> Jim, please confirm either way. If you agree I can add the below snippet,
> no need to repost.
>
> Regards,
> Sudeep
>
> --
> diff --git i/drivers/firmware/arm_scmi/smc.c w/drivers/firmware/arm_scmi/smc.c
> index fd41d436e34b..86eac0831d3c 100644
> --- i/drivers/firmware/arm_scmi/smc.c
> +++ w/drivers/firmware/arm_scmi/smc.c
> @@ -144,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>
>         shmem_tx_prepare(scmi_info->shmem, xfer);
>
> +       if (scmi_info->irq)
> +               reinit_completion(&scmi_info->tx_complete);
>         arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
>         if (scmi_info->irq)
>                 wait_for_completion(&scmi_info->tx_complete);
>
>
> This electronic communication and the information and any files transmitted with it, or attached to it, are confidential and are intended solely for the use of the individual or entity to whom it is addressed and may contain information that is confidential, legally privileged, protected by privacy laws, or otherwise restricted from disclosure to anyone else. If you are not the intended recipient or the person responsible for delivering the e-mail to the intended recipient, you are hereby notified that any use, copying, distributing, dissemination, forwarding, printing, or copying of this e-mail is strictly prohibited. If you received this e-mail in error, please return the e-mail to the sender, delete it from your computer, and destroy any printed copy of it.