[v2,7/8] usb: typec: fusb302: Improve suspend/resume handling

[Guenter Roeck <linux@roeck-us.net>]

On Thu, Mar 07, 2019 at 05:36:06PM +0100, Hans de Goede wrote:
> Remove the code which avoids doing i2c-transfers while our parent
> i2c-adapter may be suspended by busy-waiting for our resume handler
> to be called.
> 
> Instead move the interrupt handling from a threaded interrupt handler
> to a work-queue and install a non-threaded interrupt handler which
> normally queues the new interrupt handling work directly.
> 
> When our suspend handler gets called we set a flag which makes the new
> non-threaded interrupt handler skip queueing the work before our parent
> i2c-adapter is ready, instead the new non-threaded handler will record an
> interrupt has happened during suspend and then our resume handler will
> queue the work (at which point our parent will be ready).
> 
> Note normally i2c drivers solve the problem of not being able to access
> the i2c bus until the i2c-controller is resumed by simply disabling their
> irq from the suspend handler and re-enable it on resume.
> 
> That is not possible with the fusb302 since the irq is a wakeup source
> (it must be a wakeup source so that we can do PD negotiation when a
> charger gets plugged in while suspended).
> 
> Besides avoiding the ugly busy-wait, this also fixes the following errors
> which were logged by the busy-wait code when woken from suspend by plugging
> in a Type-C device:
> 
> fusb302: i2c: pm suspend, retry 1 / 10
> fusb302: i2c: pm suspend, retry 2 / 10
> etc.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/usb/typec/tcpm/fusb302.c | 109 +++++++++++++------------------
>  1 file changed, 45 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index 23f773d07514..6cdc38b8da74 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -23,6 +23,7 @@
>  #include <linux/sched/clock.h>
>  #include <linux/seq_file.h>
>  #include <linux/slab.h>
> +#include <linux/spinlock.h>
>  #include <linux/string.h>
>  #include <linux/types.h>
>  #include <linux/usb/typec.h>
> @@ -78,6 +79,10 @@ struct fusb302_chip {
>  
>  	struct regulator *vbus;
>  
> +	spinlock_t irq_lock;
> +	struct work_struct irq_work;
> +	bool irq_suspended;
> +	bool irq_while_suspended;
>  	int gpio_int_n;
>  	int gpio_int_n_irq;
>  	struct extcon_dev *extcon;
> @@ -85,9 +90,6 @@ struct fusb302_chip {
>  	struct workqueue_struct *wq;
>  	struct delayed_work bc_lvl_handler;
>  
> -	atomic_t pm_suspend;
> -	atomic_t i2c_busy;
> -
>  	/* lock for sharing chip states */
>  	struct mutex lock;
>  
> @@ -233,43 +235,15 @@ static void fusb302_debugfs_exit(const struct fusb302_chip *chip) { }
>  
>  #endif
>  
> -#define FUSB302_RESUME_RETRY 10
> -#define FUSB302_RESUME_RETRY_SLEEP 50
> -
> -static bool fusb302_is_suspended(struct fusb302_chip *chip)
> -{
> -	int retry_cnt;
> -
> -	for (retry_cnt = 0; retry_cnt < FUSB302_RESUME_RETRY; retry_cnt++) {
> -		if (atomic_read(&chip->pm_suspend)) {
> -			dev_err(chip->dev, "i2c: pm suspend, retry %d/%d\n",
> -				retry_cnt + 1, FUSB302_RESUME_RETRY);
> -			msleep(FUSB302_RESUME_RETRY_SLEEP);
> -		} else {
> -			return false;
> -		}
> -	}
> -
> -	return true;
> -}
> -
>  static int fusb302_i2c_write(struct fusb302_chip *chip,
>  			     u8 address, u8 data)
>  {
>  	int ret = 0;
>  
> -	atomic_set(&chip->i2c_busy, 1);
> -
> -	if (fusb302_is_suspended(chip)) {
> -		atomic_set(&chip->i2c_busy, 0);
> -		return -ETIMEDOUT;
> -	}
> -
>  	ret = i2c_smbus_write_byte_data(chip->i2c_client, address, data);
>  	if (ret < 0)
>  		fusb302_log(chip, "cannot write 0x%02x to 0x%02x, ret=%d",
>  			    data, address, ret);
> -	atomic_set(&chip->i2c_busy, 0);
>  
>  	return ret;
>  }
> @@ -281,19 +255,12 @@ static int fusb302_i2c_block_write(struct fusb302_chip *chip, u8 address,
>  
>  	if (length <= 0)
>  		return ret;
> -	atomic_set(&chip->i2c_busy, 1);
> -
> -	if (fusb302_is_suspended(chip)) {
> -		atomic_set(&chip->i2c_busy, 0);
> -		return -ETIMEDOUT;
> -	}
>  
>  	ret = i2c_smbus_write_i2c_block_data(chip->i2c_client, address,
>  					     length, data);
>  	if (ret < 0)
>  		fusb302_log(chip, "cannot block write 0x%02x, len=%d, ret=%d",
>  			    address, length, ret);
> -	atomic_set(&chip->i2c_busy, 0);
>  
>  	return ret;
>  }
> @@ -303,18 +270,10 @@ static int fusb302_i2c_read(struct fusb302_chip *chip,
>  {
>  	int ret = 0;
>  
> -	atomic_set(&chip->i2c_busy, 1);
> -
> -	if (fusb302_is_suspended(chip)) {
> -		atomic_set(&chip->i2c_busy, 0);
> -		return -ETIMEDOUT;
> -	}
> -
>  	ret = i2c_smbus_read_byte_data(chip->i2c_client, address);
>  	*data = (u8)ret;
>  	if (ret < 0)
>  		fusb302_log(chip, "cannot read %02x, ret=%d", address, ret);
> -	atomic_set(&chip->i2c_busy, 0);
>  
>  	return ret;
>  }
> @@ -326,12 +285,6 @@ static int fusb302_i2c_block_read(struct fusb302_chip *chip, u8 address,
>  
>  	if (length <= 0)
>  		return ret;
> -	atomic_set(&chip->i2c_busy, 1);
> -
> -	if (fusb302_is_suspended(chip)) {
> -		atomic_set(&chip->i2c_busy, 0);
> -		return -ETIMEDOUT;
> -	}
>  
>  	ret = i2c_smbus_read_i2c_block_data(chip->i2c_client, address,
>  					    length, data);
> @@ -347,8 +300,6 @@ static int fusb302_i2c_block_read(struct fusb302_chip *chip, u8 address,
>  	}
>  
>  done:
> -	atomic_set(&chip->i2c_busy, 0);
> -
>  	return ret;
>  }
>  
> @@ -1485,6 +1436,25 @@ static int fusb302_pd_read_message(struct fusb302_chip *chip,
>  static irqreturn_t fusb302_irq_intn(int irq, void *dev_id)
>  {
>  	struct fusb302_chip *chip = dev_id;
> +	unsigned long flags;
> +
> +	/* Disable our level triggered IRQ until our irq_work has cleared it */
> +	disable_irq_nosync(chip->gpio_int_n_irq);
> +
> +	spin_lock_irqsave(&chip->irq_lock, flags);
> +	if (chip->irq_suspended)
> +		chip->irq_while_suspended = true;
> +	else
> +		schedule_work(&chip->irq_work);
> +	spin_unlock_irqrestore(&chip->irq_lock, flags);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +void fusb302_irq_work(struct work_struct *work)
> +{
> +	struct fusb302_chip *chip = container_of(work, struct fusb302_chip,
> +						 irq_work);
>  	int ret = 0;
>  	u8 interrupt;
>  	u8 interrupta;
> @@ -1613,8 +1583,7 @@ static irqreturn_t fusb302_irq_intn(int irq, void *dev_id)
>  	}
>  done:
>  	mutex_unlock(&chip->lock);
> -
> -	return IRQ_HANDLED;
> +	enable_irq(chip->gpio_int_n_irq);
>  }
>  
>  static int init_gpio(struct fusb302_chip *chip)
> @@ -1730,6 +1699,8 @@ static int fusb302_probe(struct i2c_client *client,
>  	if (!chip->wq)
>  		return -ENOMEM;
>  
> +	spin_lock_init(&chip->irq_lock);
> +	INIT_WORK(&chip->irq_work, fusb302_irq_work);
>  	INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work);
>  	init_tcpc_dev(&chip->tcpc_dev);
>  
> @@ -1749,10 +1720,10 @@ static int fusb302_probe(struct i2c_client *client,
>  		goto destroy_workqueue;
>  	}
>  
> -	ret = devm_request_threaded_irq(chip->dev, chip->gpio_int_n_irq,
> -					NULL, fusb302_irq_intn,
> -					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> -					"fsc_interrupt_int_n", chip);
> +	ret = devm_request_irq(chip->dev, chip->gpio_int_n_irq,
> +			       fusb302_irq_intn,
> +			       IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> +			       "fsc_interrupt_int_n", chip);
>  	if (ret < 0) {
>  		dev_err(dev, "cannot request IRQ for GPIO Int_N, ret=%d", ret);
>  		goto tcpm_unregister_port;
> @@ -1785,19 +1756,29 @@ static int fusb302_remove(struct i2c_client *client)
>  static int fusb302_pm_suspend(struct device *dev)
>  {
>  	struct fusb302_chip *chip = dev->driver_data;
> +	unsigned long flags;
>  
> -	if (atomic_read(&chip->i2c_busy))
> -		return -EBUSY;
> -	atomic_set(&chip->pm_suspend, 1);
> +	spin_lock_irqsave(&chip->irq_lock, flags);
> +	chip->irq_suspended = true;
> +	spin_unlock_irqrestore(&chip->irq_lock, flags);
>  
> +	/* Make sure any pending irq_work is finished before the bus suspends */
> +	flush_work(&chip->irq_work);
>  	return 0;
>  }
>  
>  static int fusb302_pm_resume(struct device *dev)
>  {
>  	struct fusb302_chip *chip = dev->driver_data;
> +	unsigned long flags;
>  
> -	atomic_set(&chip->pm_suspend, 0);
> +	spin_lock_irqsave(&chip->irq_lock, flags);
> +	if (chip->irq_while_suspended) {
> +		schedule_work(&chip->irq_work);
> +		chip->irq_while_suspended = false;
> +	}
> +	chip->irq_suspended = false;
> +	spin_unlock_irqrestore(&chip->irq_lock, flags);
>  
>  	return 0;
>  }
> -- 
> 2.20.1
>