Re: [PATCH] phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context

[David Laight <david.laight.linux@gmail.com>]

On Thu, 14 May 2026 23:18:44 +0200
Pavel Machek <pavel@nabladev.com> wrote:

> Hi!
> 
> > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > 
> > The OTG PHY initialization sequence needs to wait for 20 ms at a specific
> > step, as described in commit 72c0339c115b ("phy: renesas:
> > rcar-gen3-usb2: follow the hardware manual procedure").
> > 
> > Commit 55a387ebb921 ("phy: renesas: rcar-gen3-usb2: Lock around hardware
> > registers and driver data") tried to address various problems in the
> > rcar-gen3-usb2 driver and converted the mutex protecting HW register
> > accesses to a spin lock, leaving, however, a long delay in the critical
> > section protected by the spin lock. This may become a problem,
> > especially on RT kernels.
> > 
> > To address this, release the spin lock before sleeping for 20 ms as
> > required by the HW manual and reacquire it afterwards. To avoid other
> > threads entering the critical section and configuring the HW while the
> > software is waiting for the OTG initialization to complete, introduce the
> > otg_initializing variable alongside the otg_init_done completion. Any
> > other thread trying to configure the HW while the OTG PHY initialization
> > is in progress waits for the completion instead of immediately returning
> > errors to PHY users. The IRQs were also disabled while waiting for the OTG
> > PHY initialization to complete, as the interrupt handler may also apply HW
> > settings.  
> 
> Just... there has to be a better way.
> 
> > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> > +static int rcar_gen3_phy_wait_otg_init(struct rcar_gen3_chan *channel,
> > +				       unsigned long *flags)
> > +{
> > +	unsigned long timeout = msecs_to_jiffies(25);
> > +	unsigned long ret = 1;
> > +
> > +	lockdep_assert_held(&channel->lock);
> > +
> > +	/*
> > +	 * The OTG can be initialized only once and needs to release the lock
> > +	 * and wait for 20 ms due to hardware constraints. Wait for the OTG PHY
> > +	 * initialization to complete if another PHY executes configuration
> > +	 * code while the OTG PHY is waiting. This avoids returning failures to
> > +	 * PHY users.
> > +	 */
> > +	if (READ_ONCE(channel->otg_initializing)) {
> > +		spin_unlock_irqrestore(&channel->lock, *flags);  
> 
> This is not nice, passing flags between functions like this is a red flag.

It would be better to just inline the code.
And I'd guess you need to redo the initial tests after re-acquiring the lock?
Or even need to do a state change/reference count before releasing the
lock to stop other threads 'doing anything nasty'.

-- David

> 
> You are only accessing otg_initializing under the spinlock. That means
> that READ_ONCE is reduntant.
> 
> But AFAICT spinlock is only held over this function to protect
> channel->otg_initializing access. I suspect correct answer here is
> getting rid of spinlock over this function, and using
> test_bit(BIT_INITIALIZING, ...) or something similar.
> 
> Best regards,
> 								Pavel
>