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

[Claudiu Beznea]

Hi, Pavel,

On 5/15/26 00:18, Pavel Machek wrote:
> Hi!
>
> > From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >
> > 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.
>
> 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.

If I understand correctly your point here, I don't think making the 
otg_initializing atomic (or using test_bit()) and moving it out of the spin lock 
works for all the cases. Suppose the following:

thread1:                               thread2:

0: sleep                               spin_lock_irqsave();

1: still sleep                         otg_initalizing = 1;

2: check otg_initializing              // ...

3: spin_lock_irqsave();                // ...

4:                                     spin_unlock_irqsave();

5:                                     fsleep(20000);


In this way, thread1 will get access to the HW registers once instruction at 
line 4 will be executed and be able to configure other HW registers when it 
should wait for the fsleep() to finish.

The point with the solution provided in this patch was to not allow any other 
thread to configure the HW while the fsleep() is executed.

Thank you,
Claudiu