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

[Pavel Machek]

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.

Best regards,
								Pavel

Attachment: signature.asc
Description: PGP signature