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

[Pavel Machek]

Hi!

> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> 
> 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 wait
> queue. Any

This is quite complex. How is this solved in mainline?

> To avoid failures when multiple PHYs call struct
> phy_ops::rcar_gen3_phy_usb2_init() simultaneously, and the PHY responsible
> for initializing the OTG either fails or deinit quiqly and another PHY
> takes over the PHY init role), the code waiting for the
> channel->otg_init_done wait queue retries up to NUM_OF_PHYS times.

And more complexity.

Example of the code is quoted below, and we are returning EBUSY to
userspace if it tries to change role at the wrong time. Not great.

As far as I understand, the initialization on needs to be done
once. Instead of exposing /sys interfaces before hardware is ready,
and then doing complex dance when /sys is accessed, could we
initialize hardware in rcar_gen3_phy_usb2_probe or something?

Looking at the code:

        /* If current and new mode is the same, this returns the error */
        if (cur_mode == new_mode)
                return -EINVAL;

this should probably just return success? (EINVAL is certainly wrong
error code here.)

Best regards,
								Pavel

> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -392,26 +408,58 @@ static ssize_t role_store(struct device *dev, struct device_attribute *attr,
>  	struct rcar_gen3_chan *ch = dev_get_drvdata(dev);
>  	bool is_b_device;
>  	enum phy_mode cur_mode, new_mode;
> +	int retries = NUM_OF_PHYS;
> +	unsigned long flags;
> +	int ret = -EIO;
>  
> -	guard(spinlock_irqsave)(&ch->lock);
> +	spin_lock_irqsave(&ch->lock, flags);
>  
> -	if (!ch->is_otg_channel || !rcar_gen3_is_any_otg_rphy_initialized(ch))
> -		return -EIO;
> +	if (!ch->is_otg_channel)
> +		goto unlock;
> +
> +	while (retries-- && ch->otg_initializing) {
> +		spin_unlock_irqrestore(&ch->lock, flags);
> +
> +		ret = wait_event_timeout(ch->otg_init_done, !ch->otg_initializing,
> +					 USB2_OTG_INIT_TIMEOUT);
> +		ret = ret ? 0 : -ETIMEDOUT;
> +		if (ret && !retries)
> +			goto exit;
> +
> +		spin_lock_irqsave(&ch->lock, flags);
> +	}
> +
> +	/* If another thread started a new initialization just return -EBUSY. */
> +	if (ch->otg_initializing) {
> +		ret = -EBUSY;
> +		goto unlock;
...
> @@ -1007,6 +1226,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	spin_lock_init(&channel->lock);
> +	init_waitqueue_head(&channel->otg_init_done);
>  	for (i = 0; i < NUM_OF_PHYS; i++) {
>  		channel->rphys[i].phy = devm_phy_create(dev, NULL,
>  							channel->phy_data->phy_usb2_ops);

Attachment: signature.asc
Description: PGP signature