Message-ID: <55E4761A.6040503@kernel.org>
Date:	Mon, 31 Aug 2015 16:43:22 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Cristina Opriceana <cristina.opriceana@...il.com>
Cc:	daniel.baluta@...el.com, octavian.purdila@...el.com,
	knaack.h@....de, lars@...afoo.de, pmeerw@...erw.net,
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Staging: iio: Move evgen interrupt generation to irq_work

On 18/08/15 13:55, Cristina Opriceana wrote:
> A more abstract way of handling interrupt generation in the dummy driver
> is represented by irq_work which substitutes the irq_chip, used to
> provide an IRQ line number. irq_work runs tasks from hardirq context,
> but in a NMI-safe environment and deals better with synchronization
> problems. An interrupt can be triggered by calling irq_work_queue()
> and the work will be performed by a pre-registered handler, which
> acts as a callback.
> 
> Cc: Daniel Baluta <daniel.baluta@...el.com>
> Suggested-by: Jonathan Cameron <jic23@...nel.org>
> Signed-off-by: Cristina Opriceana <cristina.opriceana@...il.com>
Hi Cristina,

Sorry for the slow response on this!

Anyhow, my main thought here is whether we can take advantage of the irq
work approach to allow a true simulation of an irq chip rather than
using the nested_irq as in the original.

The suggestion in that original email was just to move from the 'naughty'
nested_irq call over to irq work, I wasn't suggesting dropping the irq chip.

The aim of this is to allow the dummy driver to fully support
anything that can be done in a normal driver, without it being in anyway
modified in form from that of a normal hardware driver.

The result of the change here is to move the actual example code
futher from a normal driver rather than nearer to it...

Sorry for the confusion on this,

Jonathan
> ---
> This is the first draft for the irq_work replacement.
> As we want to move the iio dummy driver out of staging,
> we started to implement Jonathan's suggestions from here:
> <http://marc.info/?l=linux-iio&m=142403390705515&w=2>
> 
>  drivers/staging/iio/iio_dummy_evgen.c         | 171 +++++++++-----------------
>  drivers/staging/iio/iio_dummy_evgen.h         |  15 ++-
>  drivers/staging/iio/iio_simple_dummy.c        |   6 +-
>  drivers/staging/iio/iio_simple_dummy.h        |   8 +-
>  drivers/staging/iio/iio_simple_dummy_events.c |  71 ++++-------
>  5 files changed, 105 insertions(+), 166 deletions(-)
> 
> diff --git a/drivers/staging/iio/iio_dummy_evgen.c b/drivers/staging/iio/iio_dummy_evgen.c
> index 6d38854..974ef8a 100644
> --- a/drivers/staging/iio/iio_dummy_evgen.c
> +++ b/drivers/staging/iio/iio_dummy_evgen.c
> @@ -25,132 +25,87 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  
> -/* Fiddly bit of faking and irq without hardware */
> -#define IIO_EVENTGEN_NO 10
> -/**
> - * struct iio_dummy_evgen - evgen state
> - * @chip: irq chip we are faking
> - * @base: base of irq range
> - * @enabled: mask of which irqs are enabled
> - * @inuse: mask of which irqs are connected
> - * @regs: irq regs we are faking
> - * @lock: protect the evgen state
> - */
> -struct iio_dummy_eventgen {
> -	struct irq_chip chip;
> -	int base;
> -	bool enabled[IIO_EVENTGEN_NO];
> -	bool inuse[IIO_EVENTGEN_NO];
> -	struct iio_dummy_regs regs[IIO_EVENTGEN_NO];
> -	struct mutex lock;
> -};
> +static LIST_HEAD(iio_dummy_event_list);
> +static DEFINE_MUTEX(iio_dummy_event_list_mutex);
>  
> -/* We can only ever have one instance of this 'device' */
> -static struct iio_dummy_eventgen *iio_evgen;
> -static const char *iio_evgen_name = "iio_dummy_evgen";
> -
> -static void iio_dummy_event_irqmask(struct irq_data *d)
> +struct iio_dummy_event *get_event(int index)
>  {
> -	struct irq_chip *chip = irq_data_get_irq_chip(d);
> -	struct iio_dummy_eventgen *evgen =
> -		container_of(chip, struct iio_dummy_eventgen, chip);
> +	struct list_head *ptr, *tmp;
> +	struct iio_dummy_event *iio_event = NULL;
>  
> -	evgen->enabled[d->irq - evgen->base] = false;
> -}
> -
> -static void iio_dummy_event_irqunmask(struct irq_data *d)
> -{
> -	struct irq_chip *chip = irq_data_get_irq_chip(d);
> -	struct iio_dummy_eventgen *evgen =
> -		container_of(chip, struct iio_dummy_eventgen, chip);
> -
> -	evgen->enabled[d->irq - evgen->base] = true;
> +	mutex_lock(&iio_dummy_event_list_mutex);
> +	list_for_each_safe(ptr, tmp, &iio_dummy_event_list) {
> +		iio_event = list_entry(ptr, struct iio_dummy_event, l);
> +		if (iio_event->regs.reg_id == index)
> +			break;
> +	}
> +	mutex_unlock(&iio_dummy_event_list_mutex);
> +	return iio_event;
>  }
>  
> -static int iio_dummy_evgen_create(void)
> +int iio_dummy_evgen_create(struct iio_dev *indio_dev, int index)
>  {
> -	int ret, i;
> +	struct iio_dummy_event *iio_event;
>  
> -	iio_evgen = kzalloc(sizeof(*iio_evgen), GFP_KERNEL);
> -	if (!iio_evgen)
> +	iio_event = kzalloc(sizeof(*iio_event), GFP_KERNEL);
> +	if (!iio_event)
>  		return -ENOMEM;
>  
> -	iio_evgen->base = irq_alloc_descs(-1, 0, IIO_EVENTGEN_NO, 0);
> -	if (iio_evgen->base < 0) {
> -		ret = iio_evgen->base;
> -		kfree(iio_evgen);
> -		return ret;
> -	}
> -	iio_evgen->chip.name = iio_evgen_name;
> -	iio_evgen->chip.irq_mask = &iio_dummy_event_irqmask;
> -	iio_evgen->chip.irq_unmask = &iio_dummy_event_irqunmask;
> -	for (i = 0; i < IIO_EVENTGEN_NO; i++) {
> -		irq_set_chip(iio_evgen->base + i, &iio_evgen->chip);
> -		irq_set_handler(iio_evgen->base + i, &handle_simple_irq);
> -		irq_modify_status(iio_evgen->base + i,
> -				  IRQ_NOREQUEST | IRQ_NOAUTOEN,
> -				  IRQ_NOPROBE);
> -	}
> -	mutex_init(&iio_evgen->lock);
> +	iio_event->dev = indio_dev;
> +	iio_event->regs.reg_id = index;
> +	mutex_lock(&iio_dummy_event_list_mutex);
> +	list_add(&iio_event->l, &iio_dummy_event_list);
> +	mutex_unlock(&iio_dummy_event_list_mutex);
> +
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(iio_dummy_evgen_create);
>  
> -/**
> - * iio_dummy_evgen_get_irq() - get an evgen provided irq for a device
> - *
> - * This function will give a free allocated irq to a client device.
> - * That irq can then be caused to 'fire' by using the associated sysfs file.
> - */
> -int iio_dummy_evgen_get_irq(void)
> +void iio_dummy_init_work_handler(int index, void (*f)(struct irq_work *))
>  {
> -	int i, ret = 0;
> -
> -	if (!iio_evgen)
> -		return -ENODEV;
> +	struct iio_dummy_event *iio_event = get_event(index);
>  
> -	mutex_lock(&iio_evgen->lock);
> -	for (i = 0; i < IIO_EVENTGEN_NO; i++)
> -		if (!iio_evgen->inuse[i]) {
> -			ret = iio_evgen->base + i;
> -			iio_evgen->inuse[i] = true;
> -			break;
> -		}
> -	mutex_unlock(&iio_evgen->lock);
> -	if (i == IIO_EVENTGEN_NO)
> -		return -ENOMEM;
> -	return ret;
> +	if (iio_event)
> +		init_irq_work(&iio_event->work, f);
>  }
> -EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_irq);
> +EXPORT_SYMBOL_GPL(iio_dummy_init_work_handler);
>  
> -/**
> - * iio_dummy_evgen_release_irq() - give the irq back.
> - * @irq: irq being returned to the pool
> - *
> - * Used by client driver instances to give the irqs back when they disconnect
> - */
> -void iio_dummy_evgen_release_irq(int irq)
> +struct iio_dummy_regs *iio_dummy_evgen_get_regs(int index)
>  {
> -	mutex_lock(&iio_evgen->lock);
> -	iio_evgen->inuse[irq - iio_evgen->base] = false;
> -	mutex_unlock(&iio_evgen->lock);
> -}
> -EXPORT_SYMBOL_GPL(iio_dummy_evgen_release_irq);
> +	struct iio_dummy_event *iio_event;
> +	struct iio_dummy_regs *regs = NULL;
>  
> -struct iio_dummy_regs *iio_dummy_evgen_get_regs(int irq)
> -{
> -	return &iio_evgen->regs[irq - iio_evgen->base];
> +	iio_event = get_event(index);
> +	if (!iio_event)
> +		goto err_regs;
> +
> +	regs = &iio_event->regs;
> +err_regs:
> +	return regs;
>  }
>  EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_regs);
>  
> -static void iio_dummy_evgen_free(void)
> +void iio_dummy_evgen_free(int index)
>  {
> -	irq_free_descs(iio_evgen->base, IIO_EVENTGEN_NO);
> -	kfree(iio_evgen);
> +	struct list_head *ptr, *tmp;
> +	struct iio_dummy_event *iio_event;
> +
> +	mutex_lock(&iio_dummy_event_list_mutex);
> +	list_for_each_safe(ptr, tmp, &iio_dummy_event_list) {
> +		iio_event = list_entry(ptr, struct iio_dummy_event, l);
> +		if (iio_event->regs.reg_id == index) {
> +			list_del(ptr);
> +			kfree(iio_event);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&iio_dummy_event_list_mutex);
>  }
> +EXPORT_SYMBOL_GPL(iio_dummy_evgen_free);
>  
> +/* Do nothing upon release */
>  static void iio_evgen_release(struct device *dev)
>  {
> -	iio_dummy_evgen_free();
>  }
>  
>  static ssize_t iio_evgen_poke(struct device *dev,
> @@ -159,6 +114,7 @@ static ssize_t iio_evgen_poke(struct device *dev,
>  			      size_t len)
>  {
>  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +	struct iio_dummy_event *iio_event = get_event(this_attr->address);
>  	unsigned long event;
>  	int ret;
>  
> @@ -166,12 +122,11 @@ static ssize_t iio_evgen_poke(struct device *dev,
>  	if (ret)
>  		return ret;
>  
> -	iio_evgen->regs[this_attr->address].reg_id   = this_attr->address;
> -	iio_evgen->regs[this_attr->address].reg_data = event;
> -
> -	if (iio_evgen->enabled[this_attr->address])
> -		handle_nested_irq(iio_evgen->base + this_attr->address);
> -
> +	if (iio_event) {
> +		iio_event->regs.reg_data = event;
> +		/* trigger interrupt */
> +		irq_work_queue(&iio_event->work);
> +	}
>  	return len;
>  }
>  
> @@ -217,10 +172,6 @@ static struct device iio_evgen_dev = {
>  
>  static __init int iio_dummy_evgen_init(void)
>  {
> -	int ret = iio_dummy_evgen_create();
> -
> -	if (ret < 0)
> -		return ret;
>  	device_initialize(&iio_evgen_dev);
>  	dev_set_name(&iio_evgen_dev, "iio_evgen");
>  	return device_add(&iio_evgen_dev);
> diff --git a/drivers/staging/iio/iio_dummy_evgen.h b/drivers/staging/iio/iio_dummy_evgen.h
> index d044b94..b78b1eb 100644
> --- a/drivers/staging/iio/iio_dummy_evgen.h
> +++ b/drivers/staging/iio/iio_dummy_evgen.h
> @@ -1,13 +1,22 @@
>  #ifndef _IIO_DUMMY_EVGEN_H_
>  #define _IIO_DUMMY_EVGEN_H_
> +#include <linux/irq_work.h>
>  
>  struct iio_dummy_regs {
>  	u32 reg_id;
>  	u32 reg_data;
>  };
>  
> -struct iio_dummy_regs *iio_dummy_evgen_get_regs(int irq);
> -int iio_dummy_evgen_get_irq(void);
> -void iio_dummy_evgen_release_irq(int irq);
> +struct iio_dummy_event {
> +	struct iio_dev *dev;
> +	struct iio_dummy_regs regs;
> +	struct irq_work work;
> +	struct list_head l;
> +};
> +
> +int iio_dummy_evgen_create(struct iio_dev *indio_dev, int index);
> +void iio_dummy_init_work_handler(int index, void (*f)(struct irq_work *));
> +struct iio_dummy_regs *iio_dummy_evgen_get_regs(int index);
> +void iio_dummy_evgen_free(int index);
>  
>  #endif /* _IIO_DUMMY_EVGEN_H_ */
> diff --git a/drivers/staging/iio/iio_simple_dummy.c b/drivers/staging/iio/iio_simple_dummy.c
> index 381f90f..1ae082d 100644
> --- a/drivers/staging/iio/iio_simple_dummy.c
> +++ b/drivers/staging/iio/iio_simple_dummy.c
> @@ -635,7 +635,7 @@ static int iio_dummy_probe(int index)
>  	/* Specify that device provides sysfs type interfaces */
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> -	ret = iio_simple_dummy_events_register(indio_dev);
> +	ret = iio_simple_dummy_events_register(indio_dev, index);
>  	if (ret < 0)
>  		goto error_free_device;
>  
> @@ -651,7 +651,7 @@ static int iio_dummy_probe(int index)
>  error_unconfigure_buffer:
>  	iio_simple_dummy_unconfigure_buffer(indio_dev);
>  error_unregister_events:
> -	iio_simple_dummy_events_unregister(indio_dev);
> +	iio_simple_dummy_events_unregister(index);
>  error_free_device:
>  	iio_device_free(indio_dev);
>  error_ret:
> @@ -682,7 +682,7 @@ static void iio_dummy_remove(int index)
>  	/* Buffered capture related cleanup */
>  	iio_simple_dummy_unconfigure_buffer(indio_dev);
>  
> -	iio_simple_dummy_events_unregister(indio_dev);
> +	iio_simple_dummy_events_unregister(index);
>  
>  	/* Free all structures */
>  	iio_device_free(indio_dev);
> diff --git a/drivers/staging/iio/iio_simple_dummy.h b/drivers/staging/iio/iio_simple_dummy.h
> index 8d00224..05b84f7 100644
> --- a/drivers/staging/iio/iio_simple_dummy.h
> +++ b/drivers/staging/iio/iio_simple_dummy.h
> @@ -78,19 +78,19 @@ int iio_simple_dummy_write_event_value(struct iio_dev *indio_dev,
>  				       enum iio_event_info info, int val,
>  				       int val2);
>  
> -int iio_simple_dummy_events_register(struct iio_dev *indio_dev);
> -void iio_simple_dummy_events_unregister(struct iio_dev *indio_dev);
> +int iio_simple_dummy_events_register(struct iio_dev *indio_dev, int index);
> +void iio_simple_dummy_events_unregister(int index);
>  
>  #else /* Stubs for when events are disabled at compile time */
>  
>  static inline int
> -iio_simple_dummy_events_register(struct iio_dev *indio_dev)
> +iio_simple_dummy_events_register(struct iio_dev *indio_dev, int index)
>  {
>  	return 0;
>  };
>  
>  static inline void
> -iio_simple_dummy_events_unregister(struct iio_dev *indio_dev)
> +iio_simple_dummy_events_unregister(int index)
>  { };
>  
>  #endif /* CONFIG_IIO_SIMPLE_DUMMY_EVENTS*/
> diff --git a/drivers/staging/iio/iio_simple_dummy_events.c b/drivers/staging/iio/iio_simple_dummy_events.c
> index 73108ba..cf777b1 100644
> --- a/drivers/staging/iio/iio_simple_dummy_events.c
> +++ b/drivers/staging/iio/iio_simple_dummy_events.c
> @@ -155,23 +155,25 @@ int iio_simple_dummy_write_event_value(struct iio_dev *indio_dev,
>  
>  /**
>   * iio_simple_dummy_event_handler() - identify and pass on event
> - * @irq: irq of event line
> - * @private: pointer to device instance state.
> + * @work:	struct irq_work which handles interrupt generation
>   *
>   * This handler is responsible for querying the device to find out what
>   * event occurred and for then pushing that event towards userspace.
>   * Here only one event occurs so we push that directly on with locally
>   * grabbed timestamp.
>   */
> -static irqreturn_t iio_simple_dummy_event_handler(int irq, void *private)
> +void iio_simple_dummy_event_handler(struct irq_work *work)
>  {
> -	struct iio_dev *indio_dev = private;
> +	struct iio_dummy_event *iio_event = container_of(work,
> +							struct iio_dummy_event,
> +							work);
> +	struct iio_dev *indio_dev = iio_event->dev;
>  	struct iio_dummy_state *st = iio_priv(indio_dev);
>  
>  	dev_dbg(&indio_dev->dev, "id %x event %x\n",
> -		st->regs->reg_id, st->regs->reg_data);
> +		iio_event->regs.reg_id, iio_event->regs.reg_data);
>  
> -	switch (st->regs->reg_data) {
> +	switch (iio_event->regs.reg_data) {
>  	case 0:
>  		iio_push_event(indio_dev,
>  			       IIO_EVENT_CODE(IIO_VOLTAGE, 0, 0,
> @@ -209,59 +211,36 @@ static irqreturn_t iio_simple_dummy_event_handler(int irq, void *private)
>  	default:
>  		break;
>  	}
> -
> -	return IRQ_HANDLED;
>  }
>  
>  /**
>   * iio_simple_dummy_events_register() - setup interrupt handling for events
> - * @indio_dev: device instance data
> + * @indio_dev:	device instance data
> + * @index:	integer which identifies the dummy instance
>   *
> - * This function requests the threaded interrupt to handle the events.
> - * Normally the irq is a hardware interrupt and the number comes
> - * from board configuration files.  Here we get it from a companion
> - * module that fakes the interrupt for us. Note that module in
> - * no way forms part of this example. Just assume that events magically
> - * appear via the provided interrupt.
> + * Normally the irq is a hardware interrupt and the number comes from
> + * board configuration files. Here, the interrupt generation is handled
> + * by a companion module that will generate hardware interrupts using
> + * the irq_work API. Note that module in no way forms part of this example.
> + * Just assume that events magically appear via the provided interrupt.
>   */
> -int iio_simple_dummy_events_register(struct iio_dev *indio_dev)
> +int iio_simple_dummy_events_register(struct iio_dev *indio_dev, int index)
>  {
> -	struct iio_dummy_state *st = iio_priv(indio_dev);
>  	int ret;
>  
> -	/* Fire up event source - normally not present */
> -	st->event_irq = iio_dummy_evgen_get_irq();
> -	if (st->event_irq < 0) {
> -		ret = st->event_irq;
> -		goto error_ret;
> -	}
> -	st->regs = iio_dummy_evgen_get_regs(st->event_irq);
> -
> -	ret = request_threaded_irq(st->event_irq,
> -				   NULL,
It's this null that I never liked.  The one thing you 'always'
do on an event is grab a timestamp in the top half handler.  With
the nested irq handler we could never do that (as not in irq context).

> -				   &iio_simple_dummy_event_handler,
> -				   IRQF_ONESHOT,
> -				   "iio_simple_event",
> -				   indio_dev);
> -	if (ret < 0)
> -		goto error_free_evgen;
> +	ret = iio_dummy_evgen_create(indio_dev, index);
> +	if (ret)
> +		return ret;
> +	/* register handler */
> +	iio_dummy_init_work_handler(index, iio_simple_dummy_event_handler);
>  	return 0;
> -
> -error_free_evgen:
> -	iio_dummy_evgen_release_irq(st->event_irq);
> -error_ret:
> -	return ret;
>  }
>  
>  /**
> - * iio_simple_dummy_events_unregister() - tidy up interrupt handling on remove
> - * @indio_dev: device instance data
> + * iio_simple_dummy_events_unregister()
> + * @index:	device instance identification
>   */
> -void iio_simple_dummy_events_unregister(struct iio_dev *indio_dev)
> +void iio_simple_dummy_events_unregister(int index)
>  {
> -	struct iio_dummy_state *st = iio_priv(indio_dev);
> -
> -	free_irq(st->event_irq, indio_dev);
> -	/* Not part of normal driver */
> -	iio_dummy_evgen_release_irq(st->event_irq);
> +	 iio_dummy_evgen_free(index);
>  }
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists