Re: [PATCH v7 03/10] i2c: i2c-smbus: Use threaded irq for smbalert

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jun 15, 2017 at 09:59:31PM +0800, Phil Reid wrote:
> Prior to this commit the smbalert_irq was handling in the hard irq
> context. This change switch to using a thread irq which avoids the need
> for the work thread. Using threaded irq also removes the need for the
> edge_triggered flag as the enabling / disabling of the hard irq for level
> triggered interrupts will be handled by the irq core.
> 
> Without this change have an irq connected to something like an i2c gpio
> resulted in a null ptr deferences. Specifically handle_nested_irq calls
> the threaded irq handler.
> 
> There are currently 3 in tree drivers affected by this change.
> 
> i2c-parport driver calls i2c_handle_smbus_alert in a hard irq context.
> This driver use edge trigger interrupts which skip the enable / disable
> calls. But it still need to handle the smbus transaction on a thread. So
> the work thread is kept for this driver.
> 
> i2c-parport-light & i2c-thunderx-pcidrv provide the irq number in the
> setup which will result in the thread irq being used.
> 
> i2c-parport-light is edge trigger so the enable / disable call was
> skipped as well.
> 
> i2c-thunderx-pcidrv is getting the edge / level trigger setting from of
> data and was setting the flag as required. However the irq core should
> handle this automatically.
> 
> Signed-off-by: Phil Reid <preid@xxxxxxxxxxxxxxxxx>

CCing Benjamin for smbus changes (like last time). Please CC him in the
future in case we need another revision of this series.

> ---
>  drivers/i2c/busses/i2c-parport-light.c   |  1 -
>  drivers/i2c/busses/i2c-parport.c         |  1 -
>  drivers/i2c/busses/i2c-thunderx-pcidrv.c |  6 -----
>  drivers/i2c/i2c-smbus.c                  | 41 +++++++++++++-------------------
>  include/linux/i2c-smbus.h                |  1 -
>  5 files changed, 17 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-parport-light.c b/drivers/i2c/busses/i2c-parport-light.c
> index 1bcdd10..e6e22b8 100644
> --- a/drivers/i2c/busses/i2c-parport-light.c
> +++ b/drivers/i2c/busses/i2c-parport-light.c
> @@ -123,7 +123,6 @@ static int parport_getsda(void *data)
>  
>  /* SMBus alert support */
>  static struct i2c_smbus_alert_setup alert_data = {
> -	.alert_edge_triggered	= 1,
>  };
>  static struct i2c_client *ara;
>  static struct lineop parport_ctrl_irq = {
> diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
> index a8e54df..319209a 100644
> --- a/drivers/i2c/busses/i2c-parport.c
> +++ b/drivers/i2c/busses/i2c-parport.c
> @@ -237,7 +237,6 @@ static void i2c_parport_attach(struct parport *port)
>  
>  	/* Setup SMBus alert if supported */
>  	if (adapter_parm[type].smbus_alert) {
> -		adapter->alert_data.alert_edge_triggered = 1;
>  		adapter->ara = i2c_setup_smbus_alert(&adapter->adapter,
>  						     &adapter->alert_data);
>  		if (adapter->ara)
> diff --git a/drivers/i2c/busses/i2c-thunderx-pcidrv.c b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
> index 1d4c2be..c27a50f 100644
> --- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
> @@ -112,8 +112,6 @@ static void thunder_i2c_clock_disable(struct device *dev, struct clk *clk)
>  static int thunder_i2c_smbus_setup_of(struct octeon_i2c *i2c,
>  				      struct device_node *node)
>  {
> -	u32 type;
> -
>  	if (!node)
>  		return -EINVAL;
>  
> @@ -121,10 +119,6 @@ static int thunder_i2c_smbus_setup_of(struct octeon_i2c *i2c,
>  	if (!i2c->alert_data.irq)
>  		return -EINVAL;
>  
> -	type = irqd_get_trigger_type(irq_get_irq_data(i2c->alert_data.irq));
> -	i2c->alert_data.alert_edge_triggered =
> -		(type & IRQ_TYPE_LEVEL_MASK) ? 1 : 0;
> -
>  	i2c->ara = i2c_setup_smbus_alert(&i2c->adap, &i2c->alert_data);
>  	if (!i2c->ara)
>  		return -ENODEV;
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index f9271c7..d4af270 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -25,8 +25,6 @@
>  #include <linux/workqueue.h>
>  
>  struct i2c_smbus_alert {
> -	unsigned int		alert_edge_triggered:1;
> -	int			irq;
>  	struct work_struct	alert;
>  	struct i2c_client	*ara;		/* Alert response address */
>  };
> @@ -72,13 +70,12 @@ static int smbus_do_alert(struct device *dev, void *addrp)
>   * The alert IRQ handler needs to hand work off to a task which can issue
>   * SMBus calls, because those sleeping calls can't be made in IRQ context.
>   */
> -static void smbus_alert(struct work_struct *work)
> +static irqreturn_t smbus_alert(int irq, void *d)
>  {
> -	struct i2c_smbus_alert *alert;
> +	struct i2c_smbus_alert *alert = d;
>  	struct i2c_client *ara;
>  	unsigned short prev_addr = 0;	/* Not a valid address */
>  
> -	alert = container_of(work, struct i2c_smbus_alert, alert);
>  	ara = alert->ara;
>  
>  	for (;;) {
> @@ -115,21 +112,17 @@ static void smbus_alert(struct work_struct *work)
>  		prev_addr = data.addr;
>  	}
>  
> -	/* We handled all alerts; re-enable level-triggered IRQs */
> -	if (!alert->alert_edge_triggered)
> -		enable_irq(alert->irq);
> +	return IRQ_HANDLED;
>  }
>  
> -static irqreturn_t smbalert_irq(int irq, void *d)
> +static void smbalert_work(struct work_struct *work)
>  {
> -	struct i2c_smbus_alert *alert = d;
> +	struct i2c_smbus_alert *alert;
> +
> +	alert = container_of(work, struct i2c_smbus_alert, alert);
>  
> -	/* Disable level-triggered IRQs until we handle them */
> -	if (!alert->alert_edge_triggered)
> -		disable_irq_nosync(irq);
> +	smbus_alert(0, alert);
>  
> -	schedule_work(&alert->alert);
> -	return IRQ_HANDLED;
>  }
>  
>  /* Setup SMBALERT# infrastructure */
> @@ -139,28 +132,28 @@ static int smbalert_probe(struct i2c_client *ara,
>  	struct i2c_smbus_alert_setup *setup = dev_get_platdata(&ara->dev);
>  	struct i2c_smbus_alert *alert;
>  	struct i2c_adapter *adapter = ara->adapter;
> -	int res;
> +	int res, irq;
>  
>  	alert = devm_kzalloc(&ara->dev, sizeof(struct i2c_smbus_alert),
>  			     GFP_KERNEL);
>  	if (!alert)
>  		return -ENOMEM;
>  
> -	alert->alert_edge_triggered = setup->alert_edge_triggered;
> -	alert->irq = setup->irq;
> -	INIT_WORK(&alert->alert, smbus_alert);
> +	irq = setup->irq;
> +	INIT_WORK(&alert->alert, smbalert_work);
>  	alert->ara = ara;
>  
> -	if (setup->irq > 0) {
> -		res = devm_request_irq(&ara->dev, setup->irq, smbalert_irq,
> -				       0, "smbus_alert", alert);
> +	if (irq > 0) {
> +		res = devm_request_threaded_irq(&ara->dev, irq,
> +						NULL, smbus_alert,
> +						IRQF_SHARED | IRQF_ONESHOT,
> +						"smbus_alert", alert);
>  		if (res)
>  			return res;
>  	}
>  
>  	i2c_set_clientdata(ara, alert);
> -	dev_info(&adapter->dev, "supports SMBALERT#, %s trigger\n",
> -		 setup->alert_edge_triggered ? "edge" : "level");
> +	dev_info(&adapter->dev, "supports SMBALERT#\n");
>  
>  	return 0;
>  }
> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> index a138502..19efbd1 100644
> --- a/include/linux/i2c-smbus.h
> +++ b/include/linux/i2c-smbus.h
> @@ -42,7 +42,6 @@
>   * properly set.
>   */
>  struct i2c_smbus_alert_setup {
> -	unsigned int		alert_edge_triggered:1;
>  	int			irq;
>  };
>  
> -- 
> 1.8.3.1
> 

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux