Re: [PATCH 3/3] mailbox: pcc: Fix command timeout due to missed interrupt

On Thu, Jul 23, 2026 at 05:47:35PM +0800, lihuisong (C) wrote:
> 
> On 7/23/2026 5:16 PM, Sudeep Holla wrote:
> > On Wed, Jul 22, 2026 at 11:10:51PM -0400, Adam Young wrote:
> > > +	/*
> > > +	 * Set chan_in_use before ringing the doorbell so a fast completion
> > > +	 * interrupt is not mistaken for a shared interrupt from another
> > > +	 * subspace. Use WRITE_ONCE() for the lockless flag update. The
> > > +	 * ordered MMIO accessor used to ring the doorbell keeps this store
> > > +	 * visible before the platform is notified.
> > > +	 */
> > > +	if (pchan->plat_irq > 0)
> > > +		WRITE_ONCE(pchan->chan_in_use, true);
> > >   	ret = pcc_chan_reg_read_modify_write(&pchan->db);
> > >   	if (!ret && pchan->plat_irq > 0)
> > > -		pchan->chan_in_use = true;
> > > +		WRITE_ONCE(pchan->chan_in_use, false);
> > >   	return ret;
> > >   }
> > > -- 
> > > 2.43.0
> > > iAt the end of the above code,
> > >   WRITE_ONCE(pchan->chan_in_use, false);
> > >   should be
> > >   WRITE_ONCE(pchan->chan_in_use, true);
> > > 
> > > In order to keep the original semantics.  The flag is cleared when the
> > > messages is ACKed, not here.  This version causes a hang.
> > > 
> > Did you run and seeing hang or just code inspection. If latter, have you
> > considered that modified code sets it true before doorbell is rung and
> > set to false only if there is a failure to ring the doorbell ?
> +       /*
> +        * Set chan_in_use before ringing the doorbell so a fast completion
> +        * interrupt is not mistaken for a shared interrupt from another
> +        * subspace. Use WRITE_ONCE() for the lockless flag update. The
> +        * ordered MMIO accessor used to ring the doorbell keeps this store
> +        * visible before the platform is notified.
> +        */
> +       if (pchan->plat_irq > 0)
> +               WRITE_ONCE(pchan->chan_in_use, true);
>         ret = pcc_chan_reg_read_modify_write(&pchan->db);
> -       if (!ret && pchan->plat_irq > 0)
> -               pchan->chan_in_use = true;
> +       if (ret && pchan->plat_irq > 0)
> +               WRITE_ONCE(pchan->chan_in_use, false);
> 
> 
> !ret -->ret.
> should set to false on failure.  I remember that I fixed this place when I
> tested. But I missed this when I sent it out.
> Sorry for my mistake.
> > 

Ah right, my brain was somehow wired to read it w/o !. My mistake as well,
didn't spot it. I will sent v2, so Jassi can pick that up instead.

-- 
Regards,
Sudeep