* [PATCH 0/2] irq/irq_sim: try to improve the API @ 2019-08-12 12:52 Bartosz Golaszewski 2019-08-12 12:52 ` [PATCH 1/2] irq/irq_sim: make the irq_sim structure opaque Bartosz Golaszewski 2019-08-12 12:52 ` [PATCH 2/2] irq/irq_sim: use irq domain Bartosz Golaszewski 0 siblings, 2 replies; 7+ messages in thread From: Bartosz Golaszewski @ 2019-08-12 12:52 UTC (permalink / raw) To: Linus Walleij, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Thomas Gleixner, Marc Zyngier Cc: linux-gpio, linux-kernel, linux-iio, Bartosz Golaszewski From: Bartosz Golaszewski <bgolaszewski@baylibre.com> Currently the interrupt simulator exposes structures that don't need to be public and has a helper that manually maps the simulator's irq offsets to the global interrupt numberspace - something that should be preferably handles by an irq_domain. The first patch addresses the public structures: it moves them into the relevant .c file and makes the init function return an opaque pointer. The second patch adds a linear irq_domain to the simulator and removes the irq_sim_irqnum() routine. Users should now use the standard irq_domain functions. Both users of the irq_sim are converted at the same time as it's much easier than trying to transition them step by step. Tested both the gpio-mockup module as well as the iio_dummy_evgen. Bartosz Golaszewski (2): irq/irq_sim: make the irq_sim structure opaque irq/irq_sim: use irq domain drivers/gpio/gpio-mockup.c | 21 ++-- drivers/iio/dummy/iio_dummy_evgen.c | 34 +++--- include/linux/irq_sim.h | 29 ++--- kernel/irq/Kconfig | 1 + kernel/irq/irq_sim.c | 177 ++++++++++++++++++---------- 5 files changed, 152 insertions(+), 110 deletions(-) -- 2.21.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] irq/irq_sim: make the irq_sim structure opaque 2019-08-12 12:52 [PATCH 0/2] irq/irq_sim: try to improve the API Bartosz Golaszewski @ 2019-08-12 12:52 ` Bartosz Golaszewski 2019-08-18 19:38 ` Jonathan Cameron 2019-08-12 12:52 ` [PATCH 2/2] irq/irq_sim: use irq domain Bartosz Golaszewski 1 sibling, 1 reply; 7+ messages in thread From: Bartosz Golaszewski @ 2019-08-12 12:52 UTC (permalink / raw) To: Linus Walleij, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Thomas Gleixner, Marc Zyngier Cc: linux-gpio, linux-kernel, linux-iio, Bartosz Golaszewski From: Bartosz Golaszewski <bgolaszewski@baylibre.com> There's no good reason to export the interrupt simulator structure to users and have them provide memory for it. Let's make all the related data structures opaque and convert both users. This way we have a lot less APIs exposed in the header. Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> --- drivers/gpio/gpio-mockup.c | 12 ++--- drivers/iio/dummy/iio_dummy_evgen.c | 18 +++---- include/linux/irq_sim.h | 24 ++------- kernel/irq/irq_sim.c | 83 ++++++++++++++++++----------- 4 files changed, 70 insertions(+), 67 deletions(-) diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c index f1a9c0544e3f..9b28ffec5826 100644 --- a/drivers/gpio/gpio-mockup.c +++ b/drivers/gpio/gpio-mockup.c @@ -53,7 +53,7 @@ struct gpio_mockup_line_status { struct gpio_mockup_chip { struct gpio_chip gc; struct gpio_mockup_line_status *lines; - struct irq_sim irqsim; + struct irq_sim *irqsim; struct dentry *dbg_dir; struct mutex lock; }; @@ -186,7 +186,7 @@ static int gpio_mockup_to_irq(struct gpio_chip *gc, unsigned int offset) { struct gpio_mockup_chip *chip = gpiochip_get_data(gc); - return irq_sim_irqnum(&chip->irqsim, offset); + return irq_sim_irqnum(chip->irqsim, offset); } static void gpio_mockup_free(struct gpio_chip *gc, unsigned int offset) @@ -247,7 +247,7 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file, chip = priv->chip; gc = &chip->gc; desc = &gc->gpiodev->descs[priv->offset]; - sim = &chip->irqsim; + sim = chip->irqsim; mutex_lock(&chip->lock); @@ -431,9 +431,9 @@ static int gpio_mockup_probe(struct platform_device *pdev) return rv; } - rv = devm_irq_sim_init(dev, &chip->irqsim, gc->ngpio); - if (rv < 0) - return rv; + chip->irqsim = devm_irq_sim_new(dev, gc->ngpio); + if (IS_ERR(chip->irqsim)) + return PTR_ERR(chip->irqsim); rv = devm_gpiochip_add_data(dev, &chip->gc, chip); if (rv) diff --git a/drivers/iio/dummy/iio_dummy_evgen.c b/drivers/iio/dummy/iio_dummy_evgen.c index a6edf30567aa..efbcd4a5609e 100644 --- a/drivers/iio/dummy/iio_dummy_evgen.c +++ b/drivers/iio/dummy/iio_dummy_evgen.c @@ -37,7 +37,7 @@ struct iio_dummy_eventgen { struct iio_dummy_regs regs[IIO_EVENTGEN_NO]; struct mutex lock; bool inuse[IIO_EVENTGEN_NO]; - struct irq_sim irq_sim; + struct irq_sim *irq_sim; int base; }; @@ -46,19 +46,17 @@ static struct iio_dummy_eventgen *iio_evgen; static int iio_dummy_evgen_create(void) { - int ret; - iio_evgen = kzalloc(sizeof(*iio_evgen), GFP_KERNEL); if (!iio_evgen) return -ENOMEM; - ret = irq_sim_init(&iio_evgen->irq_sim, IIO_EVENTGEN_NO); - if (ret < 0) { + iio_evgen->irq_sim = irq_sim_new(IIO_EVENTGEN_NO); + if (IS_ERR(iio_evgen->irq_sim)) { kfree(iio_evgen); - return ret; + return PTR_ERR(iio_evgen->irq_sim); } - iio_evgen->base = irq_sim_irqnum(&iio_evgen->irq_sim, 0); + iio_evgen->base = irq_sim_irqnum(iio_evgen->irq_sim, 0); mutex_init(&iio_evgen->lock); return 0; @@ -80,7 +78,7 @@ int iio_dummy_evgen_get_irq(void) mutex_lock(&iio_evgen->lock); for (i = 0; i < IIO_EVENTGEN_NO; i++) { if (!iio_evgen->inuse[i]) { - ret = irq_sim_irqnum(&iio_evgen->irq_sim, i); + ret = irq_sim_irqnum(iio_evgen->irq_sim, i); iio_evgen->inuse[i] = true; break; } @@ -115,7 +113,7 @@ EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_regs); static void iio_dummy_evgen_free(void) { - irq_sim_fini(&iio_evgen->irq_sim); + irq_sim_free(iio_evgen->irq_sim); kfree(iio_evgen); } @@ -140,7 +138,7 @@ static ssize_t iio_evgen_poke(struct device *dev, iio_evgen->regs[this_attr->address].reg_id = this_attr->address; iio_evgen->regs[this_attr->address].reg_data = event; - irq_sim_fire(&iio_evgen->irq_sim, this_attr->address); + irq_sim_fire(iio_evgen->irq_sim, this_attr->address); return len; } diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h index 4500d453a63e..4bbf036145e2 100644 --- a/include/linux/irq_sim.h +++ b/include/linux/irq_sim.h @@ -14,27 +14,11 @@ * requested like normal irqs and enqueued from process context. */ -struct irq_sim_work_ctx { - struct irq_work work; - unsigned long *pending; -}; +struct irq_sim; -struct irq_sim_irq_ctx { - int irqnum; - bool enabled; -}; - -struct irq_sim { - struct irq_sim_work_ctx work_ctx; - int irq_base; - unsigned int irq_count; - struct irq_sim_irq_ctx *irqs; -}; - -int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs); -int devm_irq_sim_init(struct device *dev, struct irq_sim *sim, - unsigned int num_irqs); -void irq_sim_fini(struct irq_sim *sim); +struct irq_sim *irq_sim_new(unsigned int num_irqs); +struct irq_sim *devm_irq_sim_new(struct device *dev, unsigned int num_irqs); +void irq_sim_free(struct irq_sim *sim); void irq_sim_fire(struct irq_sim *sim, unsigned int offset); int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset); diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c index b992f88c5613..79f0a6494b6c 100644 --- a/kernel/irq/irq_sim.c +++ b/kernel/irq/irq_sim.c @@ -7,6 +7,23 @@ #include <linux/irq_sim.h> #include <linux/irq.h> +struct irq_sim_work_ctx { + struct irq_work work; + unsigned long *pending; +}; + +struct irq_sim_irq_ctx { + int irqnum; + bool enabled; +}; + +struct irq_sim { + struct irq_sim_work_ctx work_ctx; + int irq_base; + unsigned int irq_count; + struct irq_sim_irq_ctx *irqs; +}; + struct irq_sim_devres { struct irq_sim *sim; }; @@ -63,34 +80,42 @@ static void irq_sim_handle_irq(struct irq_work *work) } /** - * irq_sim_init - Initialize the interrupt simulator: allocate a range of - * dummy interrupts. + * irq_sim_new - Create a new interrupt simulator: allocate a range of + * dummy interrupts. * - * @sim: The interrupt simulator object to initialize. * @num_irqs: Number of interrupts to allocate * - * On success: return the base of the allocated interrupt range. - * On failure: a negative errno. + * On success: return the new irq_sim object. + * On failure: a negative errno wrapped with ERR_PTR(). */ -int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) +struct irq_sim *irq_sim_new(unsigned int num_irqs) { + struct irq_sim *sim; int i; + sim = kmalloc(sizeof(*sim), GFP_KERNEL); + if (!sim) + return ERR_PTR(-ENOMEM); + sim->irqs = kmalloc_array(num_irqs, sizeof(*sim->irqs), GFP_KERNEL); - if (!sim->irqs) - return -ENOMEM; + if (!sim->irqs) { + kfree(sim); + return ERR_PTR(-ENOMEM); + } sim->irq_base = irq_alloc_descs(-1, 0, num_irqs, 0); if (sim->irq_base < 0) { kfree(sim->irqs); - return sim->irq_base; + kfree(sim); + return ERR_PTR(sim->irq_base); } sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL); if (!sim->work_ctx.pending) { kfree(sim->irqs); + kfree(sim); irq_free_descs(sim->irq_base, num_irqs); - return -ENOMEM; + return ERR_PTR(-ENOMEM); } for (i = 0; i < num_irqs; i++) { @@ -106,64 +131,60 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) init_irq_work(&sim->work_ctx.work, irq_sim_handle_irq); sim->irq_count = num_irqs; - return sim->irq_base; + return sim; } -EXPORT_SYMBOL_GPL(irq_sim_init); +EXPORT_SYMBOL_GPL(irq_sim_new); /** - * irq_sim_fini - Deinitialize the interrupt simulator: free the interrupt + * irq_sim_free - Deinitialize the interrupt simulator: free the interrupt * descriptors and allocated memory. * * @sim: The interrupt simulator to tear down. */ -void irq_sim_fini(struct irq_sim *sim) +void irq_sim_free(struct irq_sim *sim) { irq_work_sync(&sim->work_ctx.work); bitmap_free(sim->work_ctx.pending); irq_free_descs(sim->irq_base, sim->irq_count); kfree(sim->irqs); + kfree(sim); } -EXPORT_SYMBOL_GPL(irq_sim_fini); +EXPORT_SYMBOL_GPL(irq_sim_free); static void devm_irq_sim_release(struct device *dev, void *res) { struct irq_sim_devres *this = res; - irq_sim_fini(this->sim); + irq_sim_free(this->sim); } /** - * irq_sim_init - Initialize the interrupt simulator for a managed device. + * devm_irq_sim_new - Create a new interrupt simulator for a managed device. * * @dev: Device to initialize the simulator object for. - * @sim: The interrupt simulator object to initialize. * @num_irqs: Number of interrupts to allocate * - * On success: return the base of the allocated interrupt range. - * On failure: a negative errno. + * On success: return a new irq_sim object. + * On failure: a negative errno wrapped with ERR_PTR(). */ -int devm_irq_sim_init(struct device *dev, struct irq_sim *sim, - unsigned int num_irqs) +struct irq_sim *devm_irq_sim_new(struct device *dev, unsigned int num_irqs) { struct irq_sim_devres *dr; - int rv; dr = devres_alloc(devm_irq_sim_release, sizeof(*dr), GFP_KERNEL); if (!dr) - return -ENOMEM; + return ERR_PTR(-ENOMEM); - rv = irq_sim_init(sim, num_irqs); - if (rv < 0) { + dr->sim = irq_sim_new(num_irqs); + if (IS_ERR(dr->sim)) { devres_free(dr); - return rv; + return dr->sim; } - dr->sim = sim; devres_add(dev, dr); - - return rv; + return dr->sim; } -EXPORT_SYMBOL_GPL(devm_irq_sim_init); +EXPORT_SYMBOL_GPL(devm_irq_sim_new); /** * irq_sim_fire - Enqueue an interrupt. -- 2.21.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] irq/irq_sim: make the irq_sim structure opaque 2019-08-12 12:52 ` [PATCH 1/2] irq/irq_sim: make the irq_sim structure opaque Bartosz Golaszewski @ 2019-08-18 19:38 ` Jonathan Cameron 2019-08-18 19:41 ` Jonathan Cameron 0 siblings, 1 reply; 7+ messages in thread From: Jonathan Cameron @ 2019-08-18 19:38 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Linus Walleij, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Thomas Gleixner, Marc Zyngier, linux-gpio, linux-kernel, linux-iio, Bartosz Golaszewski On Mon, 12 Aug 2019 14:52:55 +0200 Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > There's no good reason to export the interrupt simulator structure to > users and have them provide memory for it. Let's make all the related > data structures opaque and convert both users. This way we have a lot > less APIs exposed in the header. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> I agree in principle, but there is the disadvantage that all drivers that use it now need to bother with another allocation. I guess it is still worthwhile though. One comment inline. Jonathan > --- > drivers/gpio/gpio-mockup.c | 12 ++--- > drivers/iio/dummy/iio_dummy_evgen.c | 18 +++---- > include/linux/irq_sim.h | 24 ++------- > kernel/irq/irq_sim.c | 83 ++++++++++++++++++----------- > 4 files changed, 70 insertions(+), 67 deletions(-) > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > index f1a9c0544e3f..9b28ffec5826 100644 > --- a/drivers/gpio/gpio-mockup.c > +++ b/drivers/gpio/gpio-mockup.c > @@ -53,7 +53,7 @@ struct gpio_mockup_line_status { > struct gpio_mockup_chip { > struct gpio_chip gc; > struct gpio_mockup_line_status *lines; > - struct irq_sim irqsim; > + struct irq_sim *irqsim; > struct dentry *dbg_dir; > struct mutex lock; > }; > @@ -186,7 +186,7 @@ static int gpio_mockup_to_irq(struct gpio_chip *gc, unsigned int offset) > { > struct gpio_mockup_chip *chip = gpiochip_get_data(gc); > > - return irq_sim_irqnum(&chip->irqsim, offset); > + return irq_sim_irqnum(chip->irqsim, offset); > } > > static void gpio_mockup_free(struct gpio_chip *gc, unsigned int offset) > @@ -247,7 +247,7 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file, > chip = priv->chip; > gc = &chip->gc; > desc = &gc->gpiodev->descs[priv->offset]; > - sim = &chip->irqsim; > + sim = chip->irqsim; > > mutex_lock(&chip->lock); > > @@ -431,9 +431,9 @@ static int gpio_mockup_probe(struct platform_device *pdev) > return rv; > } > > - rv = devm_irq_sim_init(dev, &chip->irqsim, gc->ngpio); > - if (rv < 0) > - return rv; > + chip->irqsim = devm_irq_sim_new(dev, gc->ngpio); > + if (IS_ERR(chip->irqsim)) > + return PTR_ERR(chip->irqsim); > > rv = devm_gpiochip_add_data(dev, &chip->gc, chip); > if (rv) > diff --git a/drivers/iio/dummy/iio_dummy_evgen.c b/drivers/iio/dummy/iio_dummy_evgen.c > index a6edf30567aa..efbcd4a5609e 100644 > --- a/drivers/iio/dummy/iio_dummy_evgen.c > +++ b/drivers/iio/dummy/iio_dummy_evgen.c > @@ -37,7 +37,7 @@ struct iio_dummy_eventgen { > struct iio_dummy_regs regs[IIO_EVENTGEN_NO]; > struct mutex lock; > bool inuse[IIO_EVENTGEN_NO]; > - struct irq_sim irq_sim; > + struct irq_sim *irq_sim; > int base; > }; > > @@ -46,19 +46,17 @@ static struct iio_dummy_eventgen *iio_evgen; > > static int iio_dummy_evgen_create(void) > { > - int ret; > - > iio_evgen = kzalloc(sizeof(*iio_evgen), GFP_KERNEL); > if (!iio_evgen) > return -ENOMEM; > > - ret = irq_sim_init(&iio_evgen->irq_sim, IIO_EVENTGEN_NO); > - if (ret < 0) { > + iio_evgen->irq_sim = irq_sim_new(IIO_EVENTGEN_NO); > + if (IS_ERR(iio_evgen->irq_sim)) { > kfree(iio_evgen); > - return ret; > + return PTR_ERR(iio_evgen->irq_sim); > } > > - iio_evgen->base = irq_sim_irqnum(&iio_evgen->irq_sim, 0); > + iio_evgen->base = irq_sim_irqnum(iio_evgen->irq_sim, 0); > mutex_init(&iio_evgen->lock); > > return 0; > @@ -80,7 +78,7 @@ int iio_dummy_evgen_get_irq(void) > mutex_lock(&iio_evgen->lock); > for (i = 0; i < IIO_EVENTGEN_NO; i++) { > if (!iio_evgen->inuse[i]) { > - ret = irq_sim_irqnum(&iio_evgen->irq_sim, i); > + ret = irq_sim_irqnum(iio_evgen->irq_sim, i); > iio_evgen->inuse[i] = true; > break; > } > @@ -115,7 +113,7 @@ EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_regs); > > static void iio_dummy_evgen_free(void) > { > - irq_sim_fini(&iio_evgen->irq_sim); > + irq_sim_free(iio_evgen->irq_sim); > kfree(iio_evgen); > } > > @@ -140,7 +138,7 @@ static ssize_t iio_evgen_poke(struct device *dev, > iio_evgen->regs[this_attr->address].reg_id = this_attr->address; > iio_evgen->regs[this_attr->address].reg_data = event; > > - irq_sim_fire(&iio_evgen->irq_sim, this_attr->address); > + irq_sim_fire(iio_evgen->irq_sim, this_attr->address); > > return len; > } > diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h > index 4500d453a63e..4bbf036145e2 100644 > --- a/include/linux/irq_sim.h > +++ b/include/linux/irq_sim.h > @@ -14,27 +14,11 @@ > * requested like normal irqs and enqueued from process context. > */ > > -struct irq_sim_work_ctx { > - struct irq_work work; > - unsigned long *pending; > -}; > +struct irq_sim; > > -struct irq_sim_irq_ctx { > - int irqnum; > - bool enabled; > -}; > - > -struct irq_sim { > - struct irq_sim_work_ctx work_ctx; > - int irq_base; > - unsigned int irq_count; > - struct irq_sim_irq_ctx *irqs; > -}; > - > -int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs); > -int devm_irq_sim_init(struct device *dev, struct irq_sim *sim, > - unsigned int num_irqs); > -void irq_sim_fini(struct irq_sim *sim); > +struct irq_sim *irq_sim_new(unsigned int num_irqs); > +struct irq_sim *devm_irq_sim_new(struct device *dev, unsigned int num_irqs); > +void irq_sim_free(struct irq_sim *sim); > void irq_sim_fire(struct irq_sim *sim, unsigned int offset); > int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset); > > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c > index b992f88c5613..79f0a6494b6c 100644 > --- a/kernel/irq/irq_sim.c > +++ b/kernel/irq/irq_sim.c > @@ -7,6 +7,23 @@ > #include <linux/irq_sim.h> > #include <linux/irq.h> > > +struct irq_sim_work_ctx { > + struct irq_work work; > + unsigned long *pending; > +}; > + > +struct irq_sim_irq_ctx { > + int irqnum; > + bool enabled; > +}; > + > +struct irq_sim { > + struct irq_sim_work_ctx work_ctx; > + int irq_base; > + unsigned int irq_count; > + struct irq_sim_irq_ctx *irqs; > +}; > + > struct irq_sim_devres { > struct irq_sim *sim; > }; > @@ -63,34 +80,42 @@ static void irq_sim_handle_irq(struct irq_work *work) > } > > /** > - * irq_sim_init - Initialize the interrupt simulator: allocate a range of > - * dummy interrupts. > + * irq_sim_new - Create a new interrupt simulator: allocate a range of > + * dummy interrupts. > * > - * @sim: The interrupt simulator object to initialize. > * @num_irqs: Number of interrupts to allocate > * > - * On success: return the base of the allocated interrupt range. > - * On failure: a negative errno. > + * On success: return the new irq_sim object. > + * On failure: a negative errno wrapped with ERR_PTR(). > */ > -int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) > +struct irq_sim *irq_sim_new(unsigned int num_irqs) > { > + struct irq_sim *sim; > int i; > > + sim = kmalloc(sizeof(*sim), GFP_KERNEL); > + if (!sim) > + return ERR_PTR(-ENOMEM); > + > sim->irqs = kmalloc_array(num_irqs, sizeof(*sim->irqs), GFP_KERNEL); > - if (!sim->irqs) > - return -ENOMEM; > + if (!sim->irqs) { > + kfree(sim); > + return ERR_PTR(-ENOMEM); > + } > > sim->irq_base = irq_alloc_descs(-1, 0, num_irqs, 0); > if (sim->irq_base < 0) { > kfree(sim->irqs); > - return sim->irq_base; > + kfree(sim); > + return ERR_PTR(sim->irq_base); > } > > sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL); > if (!sim->work_ctx.pending) { > kfree(sim->irqs); > + kfree(sim); This rather looks like a function that could benefit from some unified error paths. That was true already, but adding another step makes it an even better idea. Goto fun :) > irq_free_descs(sim->irq_base, num_irqs); > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > } > > for (i = 0; i < num_irqs; i++) { > @@ -106,64 +131,60 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) > init_irq_work(&sim->work_ctx.work, irq_sim_handle_irq); > sim->irq_count = num_irqs; > > - return sim->irq_base; > + return sim; > } > -EXPORT_SYMBOL_GPL(irq_sim_init); > +EXPORT_SYMBOL_GPL(irq_sim_new); > > /** > - * irq_sim_fini - Deinitialize the interrupt simulator: free the interrupt > + * irq_sim_free - Deinitialize the interrupt simulator: free the interrupt > * descriptors and allocated memory. > * > * @sim: The interrupt simulator to tear down. > */ > -void irq_sim_fini(struct irq_sim *sim) > +void irq_sim_free(struct irq_sim *sim) > { > irq_work_sync(&sim->work_ctx.work); > bitmap_free(sim->work_ctx.pending); > irq_free_descs(sim->irq_base, sim->irq_count); > kfree(sim->irqs); > + kfree(sim); > } > -EXPORT_SYMBOL_GPL(irq_sim_fini); > +EXPORT_SYMBOL_GPL(irq_sim_free); > > static void devm_irq_sim_release(struct device *dev, void *res) > { > struct irq_sim_devres *this = res; > > - irq_sim_fini(this->sim); > + irq_sim_free(this->sim); > } > > /** > - * irq_sim_init - Initialize the interrupt simulator for a managed device. > + * devm_irq_sim_new - Create a new interrupt simulator for a managed device. > * > * @dev: Device to initialize the simulator object for. > - * @sim: The interrupt simulator object to initialize. > * @num_irqs: Number of interrupts to allocate > * > - * On success: return the base of the allocated interrupt range. > - * On failure: a negative errno. > + * On success: return a new irq_sim object. > + * On failure: a negative errno wrapped with ERR_PTR(). > */ > -int devm_irq_sim_init(struct device *dev, struct irq_sim *sim, > - unsigned int num_irqs) > +struct irq_sim *devm_irq_sim_new(struct device *dev, unsigned int num_irqs) > { > struct irq_sim_devres *dr; > - int rv; > > dr = devres_alloc(devm_irq_sim_release, sizeof(*dr), GFP_KERNEL); > if (!dr) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > > - rv = irq_sim_init(sim, num_irqs); > - if (rv < 0) { > + dr->sim = irq_sim_new(num_irqs); > + if (IS_ERR(dr->sim)) { > devres_free(dr); > - return rv; > + return dr->sim; > } > > - dr->sim = sim; > devres_add(dev, dr); > - > - return rv; > + return dr->sim; > } > -EXPORT_SYMBOL_GPL(devm_irq_sim_init); > +EXPORT_SYMBOL_GPL(devm_irq_sim_new); > > /** > * irq_sim_fire - Enqueue an interrupt. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] irq/irq_sim: make the irq_sim structure opaque 2019-08-18 19:38 ` Jonathan Cameron @ 2019-08-18 19:41 ` Jonathan Cameron 0 siblings, 0 replies; 7+ messages in thread From: Jonathan Cameron @ 2019-08-18 19:41 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Linus Walleij, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Thomas Gleixner, Marc Zyngier, linux-gpio, linux-kernel, linux-iio, Bartosz Golaszewski On Sun, 18 Aug 2019 20:38:36 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Mon, 12 Aug 2019 14:52:55 +0200 > Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > There's no good reason to export the interrupt simulator structure to > > users and have them provide memory for it. Let's make all the related > > data structures opaque and convert both users. This way we have a lot > > less APIs exposed in the header. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > I agree in principle, but there is the disadvantage that all drivers > that use it now need to bother with another allocation. I guess > it is still worthwhile though. > > One comment inline. Ignore that one as it's in code you get rid of in patch 2 ;) Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Jonathan > > > --- > > drivers/gpio/gpio-mockup.c | 12 ++--- > > drivers/iio/dummy/iio_dummy_evgen.c | 18 +++---- > > include/linux/irq_sim.h | 24 ++------- > > kernel/irq/irq_sim.c | 83 ++++++++++++++++++----------- > > 4 files changed, 70 insertions(+), 67 deletions(-) > > > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > > index f1a9c0544e3f..9b28ffec5826 100644 > > --- a/drivers/gpio/gpio-mockup.c > > +++ b/drivers/gpio/gpio-mockup.c > > @@ -53,7 +53,7 @@ struct gpio_mockup_line_status { > > struct gpio_mockup_chip { > > struct gpio_chip gc; > > struct gpio_mockup_line_status *lines; > > - struct irq_sim irqsim; > > + struct irq_sim *irqsim; > > struct dentry *dbg_dir; > > struct mutex lock; > > }; > > @@ -186,7 +186,7 @@ static int gpio_mockup_to_irq(struct gpio_chip *gc, unsigned int offset) > > { > > struct gpio_mockup_chip *chip = gpiochip_get_data(gc); > > > > - return irq_sim_irqnum(&chip->irqsim, offset); > > + return irq_sim_irqnum(chip->irqsim, offset); > > } > > > > static void gpio_mockup_free(struct gpio_chip *gc, unsigned int offset) > > @@ -247,7 +247,7 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file, > > chip = priv->chip; > > gc = &chip->gc; > > desc = &gc->gpiodev->descs[priv->offset]; > > - sim = &chip->irqsim; > > + sim = chip->irqsim; > > > > mutex_lock(&chip->lock); > > > > @@ -431,9 +431,9 @@ static int gpio_mockup_probe(struct platform_device *pdev) > > return rv; > > } > > > > - rv = devm_irq_sim_init(dev, &chip->irqsim, gc->ngpio); > > - if (rv < 0) > > - return rv; > > + chip->irqsim = devm_irq_sim_new(dev, gc->ngpio); > > + if (IS_ERR(chip->irqsim)) > > + return PTR_ERR(chip->irqsim); > > > > rv = devm_gpiochip_add_data(dev, &chip->gc, chip); > > if (rv) > > diff --git a/drivers/iio/dummy/iio_dummy_evgen.c b/drivers/iio/dummy/iio_dummy_evgen.c > > index a6edf30567aa..efbcd4a5609e 100644 > > --- a/drivers/iio/dummy/iio_dummy_evgen.c > > +++ b/drivers/iio/dummy/iio_dummy_evgen.c > > @@ -37,7 +37,7 @@ struct iio_dummy_eventgen { > > struct iio_dummy_regs regs[IIO_EVENTGEN_NO]; > > struct mutex lock; > > bool inuse[IIO_EVENTGEN_NO]; > > - struct irq_sim irq_sim; > > + struct irq_sim *irq_sim; > > int base; > > }; > > > > @@ -46,19 +46,17 @@ static struct iio_dummy_eventgen *iio_evgen; > > > > static int iio_dummy_evgen_create(void) > > { > > - int ret; > > - > > iio_evgen = kzalloc(sizeof(*iio_evgen), GFP_KERNEL); > > if (!iio_evgen) > > return -ENOMEM; > > > > - ret = irq_sim_init(&iio_evgen->irq_sim, IIO_EVENTGEN_NO); > > - if (ret < 0) { > > + iio_evgen->irq_sim = irq_sim_new(IIO_EVENTGEN_NO); > > + if (IS_ERR(iio_evgen->irq_sim)) { > > kfree(iio_evgen); > > - return ret; > > + return PTR_ERR(iio_evgen->irq_sim); > > } > > > > - iio_evgen->base = irq_sim_irqnum(&iio_evgen->irq_sim, 0); > > + iio_evgen->base = irq_sim_irqnum(iio_evgen->irq_sim, 0); > > mutex_init(&iio_evgen->lock); > > > > return 0; > > @@ -80,7 +78,7 @@ int iio_dummy_evgen_get_irq(void) > > mutex_lock(&iio_evgen->lock); > > for (i = 0; i < IIO_EVENTGEN_NO; i++) { > > if (!iio_evgen->inuse[i]) { > > - ret = irq_sim_irqnum(&iio_evgen->irq_sim, i); > > + ret = irq_sim_irqnum(iio_evgen->irq_sim, i); > > iio_evgen->inuse[i] = true; > > break; > > } > > @@ -115,7 +113,7 @@ EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_regs); > > > > static void iio_dummy_evgen_free(void) > > { > > - irq_sim_fini(&iio_evgen->irq_sim); > > + irq_sim_free(iio_evgen->irq_sim); > > kfree(iio_evgen); > > } > > > > @@ -140,7 +138,7 @@ static ssize_t iio_evgen_poke(struct device *dev, > > iio_evgen->regs[this_attr->address].reg_id = this_attr->address; > > iio_evgen->regs[this_attr->address].reg_data = event; > > > > - irq_sim_fire(&iio_evgen->irq_sim, this_attr->address); > > + irq_sim_fire(iio_evgen->irq_sim, this_attr->address); > > > > return len; > > } > > diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h > > index 4500d453a63e..4bbf036145e2 100644 > > --- a/include/linux/irq_sim.h > > +++ b/include/linux/irq_sim.h > > @@ -14,27 +14,11 @@ > > * requested like normal irqs and enqueued from process context. > > */ > > > > -struct irq_sim_work_ctx { > > - struct irq_work work; > > - unsigned long *pending; > > -}; > > +struct irq_sim; > > > > -struct irq_sim_irq_ctx { > > - int irqnum; > > - bool enabled; > > -}; > > - > > -struct irq_sim { > > - struct irq_sim_work_ctx work_ctx; > > - int irq_base; > > - unsigned int irq_count; > > - struct irq_sim_irq_ctx *irqs; > > -}; > > - > > -int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs); > > -int devm_irq_sim_init(struct device *dev, struct irq_sim *sim, > > - unsigned int num_irqs); > > -void irq_sim_fini(struct irq_sim *sim); > > +struct irq_sim *irq_sim_new(unsigned int num_irqs); > > +struct irq_sim *devm_irq_sim_new(struct device *dev, unsigned int num_irqs); > > +void irq_sim_free(struct irq_sim *sim); > > void irq_sim_fire(struct irq_sim *sim, unsigned int offset); > > int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset); > > > > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c > > index b992f88c5613..79f0a6494b6c 100644 > > --- a/kernel/irq/irq_sim.c > > +++ b/kernel/irq/irq_sim.c > > @@ -7,6 +7,23 @@ > > #include <linux/irq_sim.h> > > #include <linux/irq.h> > > > > +struct irq_sim_work_ctx { > > + struct irq_work work; > > + unsigned long *pending; > > +}; > > + > > +struct irq_sim_irq_ctx { > > + int irqnum; > > + bool enabled; > > +}; > > + > > +struct irq_sim { > > + struct irq_sim_work_ctx work_ctx; > > + int irq_base; > > + unsigned int irq_count; > > + struct irq_sim_irq_ctx *irqs; > > +}; > > + > > struct irq_sim_devres { > > struct irq_sim *sim; > > }; > > @@ -63,34 +80,42 @@ static void irq_sim_handle_irq(struct irq_work *work) > > } > > > > /** > > - * irq_sim_init - Initialize the interrupt simulator: allocate a range of > > - * dummy interrupts. > > + * irq_sim_new - Create a new interrupt simulator: allocate a range of > > + * dummy interrupts. > > * > > - * @sim: The interrupt simulator object to initialize. > > * @num_irqs: Number of interrupts to allocate > > * > > - * On success: return the base of the allocated interrupt range. > > - * On failure: a negative errno. > > + * On success: return the new irq_sim object. > > + * On failure: a negative errno wrapped with ERR_PTR(). > > */ > > -int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) > > +struct irq_sim *irq_sim_new(unsigned int num_irqs) > > { > > + struct irq_sim *sim; > > int i; > > > > + sim = kmalloc(sizeof(*sim), GFP_KERNEL); > > + if (!sim) > > + return ERR_PTR(-ENOMEM); > > + > > sim->irqs = kmalloc_array(num_irqs, sizeof(*sim->irqs), GFP_KERNEL); > > - if (!sim->irqs) > > - return -ENOMEM; > > + if (!sim->irqs) { > > + kfree(sim); > > + return ERR_PTR(-ENOMEM); > > + } > > > > sim->irq_base = irq_alloc_descs(-1, 0, num_irqs, 0); > > if (sim->irq_base < 0) { > > kfree(sim->irqs); > > - return sim->irq_base; > > + kfree(sim); > > + return ERR_PTR(sim->irq_base); > > } > > > > sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL); > > if (!sim->work_ctx.pending) { > > kfree(sim->irqs); > > + kfree(sim); > > This rather looks like a function that could benefit from some > unified error paths. That was true already, but adding another step > makes it an even better idea. Goto fun :) > > > irq_free_descs(sim->irq_base, num_irqs); > > - return -ENOMEM; > > + return ERR_PTR(-ENOMEM); > > } > > > > for (i = 0; i < num_irqs; i++) { > > @@ -106,64 +131,60 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) > > init_irq_work(&sim->work_ctx.work, irq_sim_handle_irq); > > sim->irq_count = num_irqs; > > > > - return sim->irq_base; > > + return sim; > > } > > -EXPORT_SYMBOL_GPL(irq_sim_init); > > +EXPORT_SYMBOL_GPL(irq_sim_new); > > > > /** > > - * irq_sim_fini - Deinitialize the interrupt simulator: free the interrupt > > + * irq_sim_free - Deinitialize the interrupt simulator: free the interrupt > > * descriptors and allocated memory. > > * > > * @sim: The interrupt simulator to tear down. > > */ > > -void irq_sim_fini(struct irq_sim *sim) > > +void irq_sim_free(struct irq_sim *sim) > > { > > irq_work_sync(&sim->work_ctx.work); > > bitmap_free(sim->work_ctx.pending); > > irq_free_descs(sim->irq_base, sim->irq_count); > > kfree(sim->irqs); > > + kfree(sim); > > } > > -EXPORT_SYMBOL_GPL(irq_sim_fini); > > +EXPORT_SYMBOL_GPL(irq_sim_free); > > > > static void devm_irq_sim_release(struct device *dev, void *res) > > { > > struct irq_sim_devres *this = res; > > > > - irq_sim_fini(this->sim); > > + irq_sim_free(this->sim); > > } > > > > /** > > - * irq_sim_init - Initialize the interrupt simulator for a managed device. > > + * devm_irq_sim_new - Create a new interrupt simulator for a managed device. > > * > > * @dev: Device to initialize the simulator object for. > > - * @sim: The interrupt simulator object to initialize. > > * @num_irqs: Number of interrupts to allocate > > * > > - * On success: return the base of the allocated interrupt range. > > - * On failure: a negative errno. > > + * On success: return a new irq_sim object. > > + * On failure: a negative errno wrapped with ERR_PTR(). > > */ > > -int devm_irq_sim_init(struct device *dev, struct irq_sim *sim, > > - unsigned int num_irqs) > > +struct irq_sim *devm_irq_sim_new(struct device *dev, unsigned int num_irqs) > > { > > struct irq_sim_devres *dr; > > - int rv; > > > > dr = devres_alloc(devm_irq_sim_release, sizeof(*dr), GFP_KERNEL); > > if (!dr) > > - return -ENOMEM; > > + return ERR_PTR(-ENOMEM); > > > > - rv = irq_sim_init(sim, num_irqs); > > - if (rv < 0) { > > + dr->sim = irq_sim_new(num_irqs); > > + if (IS_ERR(dr->sim)) { > > devres_free(dr); > > - return rv; > > + return dr->sim; > > } > > > > - dr->sim = sim; > > devres_add(dev, dr); > > - > > - return rv; > > + return dr->sim; > > } > > -EXPORT_SYMBOL_GPL(devm_irq_sim_init); > > +EXPORT_SYMBOL_GPL(devm_irq_sim_new); > > > > /** > > * irq_sim_fire - Enqueue an interrupt. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] irq/irq_sim: use irq domain 2019-08-12 12:52 [PATCH 0/2] irq/irq_sim: try to improve the API Bartosz Golaszewski 2019-08-12 12:52 ` [PATCH 1/2] irq/irq_sim: make the irq_sim structure opaque Bartosz Golaszewski @ 2019-08-12 12:52 ` Bartosz Golaszewski 2019-08-18 19:45 ` Jonathan Cameron 2019-08-19 8:18 ` Marc Zyngier 1 sibling, 2 replies; 7+ messages in thread From: Bartosz Golaszewski @ 2019-08-12 12:52 UTC (permalink / raw) To: Linus Walleij, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Thomas Gleixner, Marc Zyngier Cc: linux-gpio, linux-kernel, linux-iio, Bartosz Golaszewski From: Bartosz Golaszewski <bgolaszewski@baylibre.com> We currently have a dedicated function to map the interrupt simulator offsets to global interrupt numbers. This is something that irq_domain should handle. Create a linear irq_domain when initializing the interrupt simulator and modify the irq_sim_fire() function to only take as parameter the global interrupt number. Convert both users in the same patch to using the new interface. Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> --- drivers/gpio/gpio-mockup.c | 11 ++- drivers/iio/dummy/iio_dummy_evgen.c | 22 ++++-- include/linux/irq_sim.h | 5 +- kernel/irq/Kconfig | 1 + kernel/irq/irq_sim.c | 110 +++++++++++++++++----------- 5 files changed, 94 insertions(+), 55 deletions(-) diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c index 9b28ffec5826..4cf594f0e7cd 100644 --- a/drivers/gpio/gpio-mockup.c +++ b/drivers/gpio/gpio-mockup.c @@ -186,7 +186,7 @@ static int gpio_mockup_to_irq(struct gpio_chip *gc, unsigned int offset) { struct gpio_mockup_chip *chip = gpiochip_get_data(gc); - return irq_sim_irqnum(chip->irqsim, offset); + return irq_create_mapping(irq_sim_get_domain(chip->irqsim), offset); } static void gpio_mockup_free(struct gpio_chip *gc, unsigned int offset) @@ -228,6 +228,7 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file, struct gpio_mockup_dbgfs_private *priv; int rv, val, curr, irq, irq_type; struct gpio_mockup_chip *chip; + struct irq_domain *domain; struct seq_file *sfile; struct gpio_desc *desc; struct gpio_chip *gc; @@ -248,6 +249,7 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file, gc = &chip->gc; desc = &gc->gpiodev->descs[priv->offset]; sim = chip->irqsim; + domain = irq_sim_get_domain(sim); mutex_lock(&chip->lock); @@ -257,12 +259,15 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file, if (curr == val) goto out; - irq = irq_sim_irqnum(sim, priv->offset); + irq = irq_find_mapping(domain, priv->offset); + if (!irq) + return -ENOENT; + irq_type = irq_get_trigger_type(irq); if ((val == 1 && (irq_type & IRQ_TYPE_EDGE_RISING)) || (val == 0 && (irq_type & IRQ_TYPE_EDGE_FALLING))) - irq_sim_fire(sim, priv->offset); + irq_sim_fire(irq); } /* Change the value unless we're actively driving the line. */ diff --git a/drivers/iio/dummy/iio_dummy_evgen.c b/drivers/iio/dummy/iio_dummy_evgen.c index efbcd4a5609e..cc827f60a535 100644 --- a/drivers/iio/dummy/iio_dummy_evgen.c +++ b/drivers/iio/dummy/iio_dummy_evgen.c @@ -31,14 +31,13 @@ * @lock: protect the evgen state * @inuse: mask of which irqs are connected * @irq_sim: interrupt simulator - * @base: base of irq range */ struct iio_dummy_eventgen { struct iio_dummy_regs regs[IIO_EVENTGEN_NO]; struct mutex lock; bool inuse[IIO_EVENTGEN_NO]; struct irq_sim *irq_sim; - int base; + struct irq_domain *domain; }; /* We can only ever have one instance of this 'device' */ @@ -56,7 +55,7 @@ static int iio_dummy_evgen_create(void) return PTR_ERR(iio_evgen->irq_sim); } - iio_evgen->base = irq_sim_irqnum(iio_evgen->irq_sim, 0); + iio_evgen->domain = irq_sim_get_domain(iio_evgen->irq_sim); mutex_init(&iio_evgen->lock); return 0; @@ -78,7 +77,7 @@ int iio_dummy_evgen_get_irq(void) mutex_lock(&iio_evgen->lock); for (i = 0; i < IIO_EVENTGEN_NO; i++) { if (!iio_evgen->inuse[i]) { - ret = irq_sim_irqnum(iio_evgen->irq_sim, i); + ret = irq_create_mapping(iio_evgen->domain, i); iio_evgen->inuse[i] = true; break; } @@ -99,15 +98,21 @@ EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_irq); */ void iio_dummy_evgen_release_irq(int irq) { + struct irq_data *irqd; + + irqd = irq_get_irq_data(irq); + mutex_lock(&iio_evgen->lock); - iio_evgen->inuse[irq - iio_evgen->base] = false; + iio_evgen->inuse[irqd_to_hwirq(irqd)] = false; mutex_unlock(&iio_evgen->lock); } EXPORT_SYMBOL_GPL(iio_dummy_evgen_release_irq); struct iio_dummy_regs *iio_dummy_evgen_get_regs(int irq) { - return &iio_evgen->regs[irq - iio_evgen->base]; + struct irq_data *irqd = irq_get_irq_data(irq); + + return &iio_evgen->regs[irqd_to_hwirq(irqd)]; } EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_regs); @@ -129,7 +134,7 @@ static ssize_t iio_evgen_poke(struct device *dev, { struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); unsigned long event; - int ret; + int ret, irq; ret = kstrtoul(buf, 10, &event); if (ret) @@ -138,7 +143,8 @@ static ssize_t iio_evgen_poke(struct device *dev, iio_evgen->regs[this_attr->address].reg_id = this_attr->address; iio_evgen->regs[this_attr->address].reg_data = event; - irq_sim_fire(iio_evgen->irq_sim, this_attr->address); + irq = irq_create_mapping(iio_evgen->domain, this_attr->address); + irq_sim_fire(irq); return len; } diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h index 4bbf036145e2..4056d0e7f0b4 100644 --- a/include/linux/irq_sim.h +++ b/include/linux/irq_sim.h @@ -7,6 +7,7 @@ #define _LINUX_IRQ_SIM_H #include <linux/irq_work.h> +#include <linux/irqdomain.h> #include <linux/device.h> /* @@ -19,7 +20,7 @@ struct irq_sim; struct irq_sim *irq_sim_new(unsigned int num_irqs); struct irq_sim *devm_irq_sim_new(struct device *dev, unsigned int num_irqs); void irq_sim_free(struct irq_sim *sim); -void irq_sim_fire(struct irq_sim *sim, unsigned int offset); -int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset); +void irq_sim_fire(int virq); +struct irq_domain *irq_sim_get_domain(struct irq_sim *sim); #endif /* _LINUX_IRQ_SIM_H */ diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig index f92d9a687372..d0890f7729d4 100644 --- a/kernel/irq/Kconfig +++ b/kernel/irq/Kconfig @@ -68,6 +68,7 @@ config IRQ_DOMAIN config IRQ_SIM bool select IRQ_WORK + select IRQ_DOMAIN # Support for hierarchical irq domains config IRQ_DOMAIN_HIERARCHY diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c index 79f0a6494b6c..a1c91aefb6cd 100644 --- a/kernel/irq/irq_sim.c +++ b/kernel/irq/irq_sim.c @@ -15,13 +15,14 @@ struct irq_sim_work_ctx { struct irq_sim_irq_ctx { int irqnum; bool enabled; + struct irq_sim_work_ctx *work_ctx; }; struct irq_sim { struct irq_sim_work_ctx work_ctx; int irq_base; unsigned int irq_count; - struct irq_sim_irq_ctx *irqs; + struct irq_domain *domain; }; struct irq_sim_devres { @@ -74,11 +75,46 @@ static void irq_sim_handle_irq(struct irq_work *work) offset = find_next_bit(work_ctx->pending, sim->irq_count, offset); clear_bit(offset, work_ctx->pending); - irqnum = irq_sim_irqnum(sim, offset); + irqnum = irq_find_mapping(sim->domain, offset); handle_simple_irq(irq_to_desc(irqnum)); } } +static int irq_sim_domain_map(struct irq_domain *domain, + unsigned int virq, irq_hw_number_t hw) +{ + struct irq_sim *sim = domain->host_data; + struct irq_sim_irq_ctx *ctx; + + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); + if (!ctx) + return -ENOMEM; + + irq_set_chip(virq, &irq_sim_irqchip); + irq_set_chip_data(virq, ctx); + irq_set_handler(virq, handle_simple_irq); + irq_modify_status(virq, IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE); + ctx->work_ctx = &sim->work_ctx; + + return 0; +} + +static void irq_sim_domain_unmap(struct irq_domain *domain, unsigned int virq) +{ + struct irq_sim_irq_ctx *ctx; + struct irq_data *irqd; + + irqd = irq_domain_get_irq_data(domain, virq); + ctx = irq_data_get_irq_chip_data(irqd); + + kfree(ctx); +} + +static const struct irq_domain_ops irq_sim_domain_ops = { + .map = irq_sim_domain_map, + .unmap = irq_sim_domain_unmap, +}; + /** * irq_sim_new - Create a new interrupt simulator: allocate a range of * dummy interrupts. @@ -91,42 +127,21 @@ static void irq_sim_handle_irq(struct irq_work *work) struct irq_sim *irq_sim_new(unsigned int num_irqs) { struct irq_sim *sim; - int i; sim = kmalloc(sizeof(*sim), GFP_KERNEL); if (!sim) return ERR_PTR(-ENOMEM); - sim->irqs = kmalloc_array(num_irqs, sizeof(*sim->irqs), GFP_KERNEL); - if (!sim->irqs) { - kfree(sim); - return ERR_PTR(-ENOMEM); - } - - sim->irq_base = irq_alloc_descs(-1, 0, num_irqs, 0); - if (sim->irq_base < 0) { - kfree(sim->irqs); - kfree(sim); - return ERR_PTR(sim->irq_base); - } - sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL); if (!sim->work_ctx.pending) { - kfree(sim->irqs); kfree(sim); - irq_free_descs(sim->irq_base, num_irqs); return ERR_PTR(-ENOMEM); } - for (i = 0; i < num_irqs; i++) { - sim->irqs[i].irqnum = sim->irq_base + i; - sim->irqs[i].enabled = false; - irq_set_chip(sim->irq_base + i, &irq_sim_irqchip); - irq_set_chip_data(sim->irq_base + i, &sim->irqs[i]); - irq_set_handler(sim->irq_base + i, &handle_simple_irq); - irq_modify_status(sim->irq_base + i, - IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE); - } + sim->domain = irq_domain_create_linear(NULL, num_irqs, + &irq_sim_domain_ops, sim); + if (!sim->domain) + return ERR_PTR(-ENOMEM); init_irq_work(&sim->work_ctx.work, irq_sim_handle_irq); sim->irq_count = num_irqs; @@ -143,10 +158,17 @@ EXPORT_SYMBOL_GPL(irq_sim_new); */ void irq_sim_free(struct irq_sim *sim) { + int i, irq; + + for (i = 0; i < sim->irq_count; i++) { + irq = irq_find_mapping(sim->domain, i); + if (irq) + irq_dispose_mapping(irq); + } + + irq_domain_remove(sim->domain); irq_work_sync(&sim->work_ctx.work); bitmap_free(sim->work_ctx.pending); - irq_free_descs(sim->irq_base, sim->irq_count); - kfree(sim->irqs); kfree(sim); } EXPORT_SYMBOL_GPL(irq_sim_free); @@ -189,27 +211,31 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_new); /** * irq_sim_fire - Enqueue an interrupt. * - * @sim: The interrupt simulator object. - * @offset: Offset of the simulated interrupt which should be fired. + * @virq: Virtual interrupt number to fire. It must be associated with + * an existing interrupt simulator. */ -void irq_sim_fire(struct irq_sim *sim, unsigned int offset) +void irq_sim_fire(int virq) { - if (sim->irqs[offset].enabled) { - set_bit(offset, sim->work_ctx.pending); - irq_work_queue(&sim->work_ctx.work); + struct irq_sim_irq_ctx *ctx; + struct irq_data *irqd; + + irqd = irq_get_irq_data(virq); + ctx = irq_data_get_irq_chip_data(irqd); + + if (ctx->enabled) { + set_bit(irqd_to_hwirq(irqd), ctx->work_ctx->pending); + irq_work_queue(&ctx->work_ctx->work); } } EXPORT_SYMBOL_GPL(irq_sim_fire); /** - * irq_sim_irqnum - Get the allocated number of a dummy interrupt. + * irq_sim_get_domain - Retrieve the interrupt domain for this simulator. * - * @sim: The interrupt simulator object. - * @offset: Offset of the simulated interrupt for which to retrieve - * the number. + * @sim: The interrupt simulator the domain of which we retrieve. */ -int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset) +struct irq_domain *irq_sim_get_domain(struct irq_sim *sim) { - return sim->irqs[offset].irqnum; + return sim->domain; } -EXPORT_SYMBOL_GPL(irq_sim_irqnum); +EXPORT_SYMBOL(irq_sim_get_domain); -- 2.21.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] irq/irq_sim: use irq domain 2019-08-12 12:52 ` [PATCH 2/2] irq/irq_sim: use irq domain Bartosz Golaszewski @ 2019-08-18 19:45 ` Jonathan Cameron 2019-08-19 8:18 ` Marc Zyngier 1 sibling, 0 replies; 7+ messages in thread From: Jonathan Cameron @ 2019-08-18 19:45 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Linus Walleij, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Thomas Gleixner, Marc Zyngier, linux-gpio, linux-kernel, linux-iio, Bartosz Golaszewski On Mon, 12 Aug 2019 14:52:56 +0200 Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > We currently have a dedicated function to map the interrupt simulator > offsets to global interrupt numbers. This is something that irq_domain > should handle. > > Create a linear irq_domain when initializing the interrupt simulator > and modify the irq_sim_fire() function to only take as parameter the > global interrupt number. > > Convert both users in the same patch to using the new interface. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> Hmm. Again, in principle looks fine, but what advantages do we get from using an irq domain? Pure code wise, it's longer and only a little bit simpler. Jonathan > --- > drivers/gpio/gpio-mockup.c | 11 ++- > drivers/iio/dummy/iio_dummy_evgen.c | 22 ++++-- > include/linux/irq_sim.h | 5 +- > kernel/irq/Kconfig | 1 + > kernel/irq/irq_sim.c | 110 +++++++++++++++++----------- > 5 files changed, 94 insertions(+), 55 deletions(-) > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > index 9b28ffec5826..4cf594f0e7cd 100644 > --- a/drivers/gpio/gpio-mockup.c > +++ b/drivers/gpio/gpio-mockup.c > @@ -186,7 +186,7 @@ static int gpio_mockup_to_irq(struct gpio_chip *gc, unsigned int offset) > { > struct gpio_mockup_chip *chip = gpiochip_get_data(gc); > > - return irq_sim_irqnum(chip->irqsim, offset); > + return irq_create_mapping(irq_sim_get_domain(chip->irqsim), offset); > } > > static void gpio_mockup_free(struct gpio_chip *gc, unsigned int offset) > @@ -228,6 +228,7 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file, > struct gpio_mockup_dbgfs_private *priv; > int rv, val, curr, irq, irq_type; > struct gpio_mockup_chip *chip; > + struct irq_domain *domain; > struct seq_file *sfile; > struct gpio_desc *desc; > struct gpio_chip *gc; > @@ -248,6 +249,7 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file, > gc = &chip->gc; > desc = &gc->gpiodev->descs[priv->offset]; > sim = chip->irqsim; > + domain = irq_sim_get_domain(sim); > > mutex_lock(&chip->lock); > > @@ -257,12 +259,15 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file, > if (curr == val) > goto out; > > - irq = irq_sim_irqnum(sim, priv->offset); > + irq = irq_find_mapping(domain, priv->offset); > + if (!irq) > + return -ENOENT; > + > irq_type = irq_get_trigger_type(irq); > > if ((val == 1 && (irq_type & IRQ_TYPE_EDGE_RISING)) || > (val == 0 && (irq_type & IRQ_TYPE_EDGE_FALLING))) > - irq_sim_fire(sim, priv->offset); > + irq_sim_fire(irq); > } > > /* Change the value unless we're actively driving the line. */ > diff --git a/drivers/iio/dummy/iio_dummy_evgen.c b/drivers/iio/dummy/iio_dummy_evgen.c > index efbcd4a5609e..cc827f60a535 100644 > --- a/drivers/iio/dummy/iio_dummy_evgen.c > +++ b/drivers/iio/dummy/iio_dummy_evgen.c > @@ -31,14 +31,13 @@ > * @lock: protect the evgen state > * @inuse: mask of which irqs are connected > * @irq_sim: interrupt simulator > - * @base: base of irq range > */ > struct iio_dummy_eventgen { > struct iio_dummy_regs regs[IIO_EVENTGEN_NO]; > struct mutex lock; > bool inuse[IIO_EVENTGEN_NO]; > struct irq_sim *irq_sim; > - int base; > + struct irq_domain *domain; > }; > > /* We can only ever have one instance of this 'device' */ > @@ -56,7 +55,7 @@ static int iio_dummy_evgen_create(void) > return PTR_ERR(iio_evgen->irq_sim); > } > > - iio_evgen->base = irq_sim_irqnum(iio_evgen->irq_sim, 0); > + iio_evgen->domain = irq_sim_get_domain(iio_evgen->irq_sim); > mutex_init(&iio_evgen->lock); > > return 0; > @@ -78,7 +77,7 @@ int iio_dummy_evgen_get_irq(void) > mutex_lock(&iio_evgen->lock); > for (i = 0; i < IIO_EVENTGEN_NO; i++) { > if (!iio_evgen->inuse[i]) { > - ret = irq_sim_irqnum(iio_evgen->irq_sim, i); > + ret = irq_create_mapping(iio_evgen->domain, i); > iio_evgen->inuse[i] = true; > break; > } > @@ -99,15 +98,21 @@ EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_irq); > */ > void iio_dummy_evgen_release_irq(int irq) > { > + struct irq_data *irqd; > + > + irqd = irq_get_irq_data(irq); > + > mutex_lock(&iio_evgen->lock); > - iio_evgen->inuse[irq - iio_evgen->base] = false; > + iio_evgen->inuse[irqd_to_hwirq(irqd)] = false; > mutex_unlock(&iio_evgen->lock); > } > EXPORT_SYMBOL_GPL(iio_dummy_evgen_release_irq); > > struct iio_dummy_regs *iio_dummy_evgen_get_regs(int irq) > { > - return &iio_evgen->regs[irq - iio_evgen->base]; > + struct irq_data *irqd = irq_get_irq_data(irq); > + > + return &iio_evgen->regs[irqd_to_hwirq(irqd)]; > } > EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_regs); > > @@ -129,7 +134,7 @@ static ssize_t iio_evgen_poke(struct device *dev, > { > struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); > unsigned long event; > - int ret; > + int ret, irq; > > ret = kstrtoul(buf, 10, &event); > if (ret) > @@ -138,7 +143,8 @@ static ssize_t iio_evgen_poke(struct device *dev, > iio_evgen->regs[this_attr->address].reg_id = this_attr->address; > iio_evgen->regs[this_attr->address].reg_data = event; > > - irq_sim_fire(iio_evgen->irq_sim, this_attr->address); > + irq = irq_create_mapping(iio_evgen->domain, this_attr->address); > + irq_sim_fire(irq); > > return len; > } > diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h > index 4bbf036145e2..4056d0e7f0b4 100644 > --- a/include/linux/irq_sim.h > +++ b/include/linux/irq_sim.h > @@ -7,6 +7,7 @@ > #define _LINUX_IRQ_SIM_H > > #include <linux/irq_work.h> > +#include <linux/irqdomain.h> > #include <linux/device.h> > > /* > @@ -19,7 +20,7 @@ struct irq_sim; > struct irq_sim *irq_sim_new(unsigned int num_irqs); > struct irq_sim *devm_irq_sim_new(struct device *dev, unsigned int num_irqs); > void irq_sim_free(struct irq_sim *sim); > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset); > -int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset); > +void irq_sim_fire(int virq); > +struct irq_domain *irq_sim_get_domain(struct irq_sim *sim); > > #endif /* _LINUX_IRQ_SIM_H */ > diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig > index f92d9a687372..d0890f7729d4 100644 > --- a/kernel/irq/Kconfig > +++ b/kernel/irq/Kconfig > @@ -68,6 +68,7 @@ config IRQ_DOMAIN > config IRQ_SIM > bool > select IRQ_WORK > + select IRQ_DOMAIN > > # Support for hierarchical irq domains > config IRQ_DOMAIN_HIERARCHY > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c > index 79f0a6494b6c..a1c91aefb6cd 100644 > --- a/kernel/irq/irq_sim.c > +++ b/kernel/irq/irq_sim.c > @@ -15,13 +15,14 @@ struct irq_sim_work_ctx { > struct irq_sim_irq_ctx { > int irqnum; > bool enabled; > + struct irq_sim_work_ctx *work_ctx; > }; > > struct irq_sim { > struct irq_sim_work_ctx work_ctx; > int irq_base; > unsigned int irq_count; > - struct irq_sim_irq_ctx *irqs; > + struct irq_domain *domain; > }; > > struct irq_sim_devres { > @@ -74,11 +75,46 @@ static void irq_sim_handle_irq(struct irq_work *work) > offset = find_next_bit(work_ctx->pending, > sim->irq_count, offset); > clear_bit(offset, work_ctx->pending); > - irqnum = irq_sim_irqnum(sim, offset); > + irqnum = irq_find_mapping(sim->domain, offset); > handle_simple_irq(irq_to_desc(irqnum)); > } > } > > +static int irq_sim_domain_map(struct irq_domain *domain, > + unsigned int virq, irq_hw_number_t hw) > +{ > + struct irq_sim *sim = domain->host_data; > + struct irq_sim_irq_ctx *ctx; > + > + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); > + if (!ctx) > + return -ENOMEM; > + > + irq_set_chip(virq, &irq_sim_irqchip); > + irq_set_chip_data(virq, ctx); > + irq_set_handler(virq, handle_simple_irq); > + irq_modify_status(virq, IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE); > + ctx->work_ctx = &sim->work_ctx; > + > + return 0; > +} > + > +static void irq_sim_domain_unmap(struct irq_domain *domain, unsigned int virq) > +{ > + struct irq_sim_irq_ctx *ctx; > + struct irq_data *irqd; > + > + irqd = irq_domain_get_irq_data(domain, virq); > + ctx = irq_data_get_irq_chip_data(irqd); > + > + kfree(ctx); > +} > + > +static const struct irq_domain_ops irq_sim_domain_ops = { > + .map = irq_sim_domain_map, > + .unmap = irq_sim_domain_unmap, > +}; > + > /** > * irq_sim_new - Create a new interrupt simulator: allocate a range of > * dummy interrupts. > @@ -91,42 +127,21 @@ static void irq_sim_handle_irq(struct irq_work *work) > struct irq_sim *irq_sim_new(unsigned int num_irqs) > { > struct irq_sim *sim; > - int i; > > sim = kmalloc(sizeof(*sim), GFP_KERNEL); > if (!sim) > return ERR_PTR(-ENOMEM); > > - sim->irqs = kmalloc_array(num_irqs, sizeof(*sim->irqs), GFP_KERNEL); > - if (!sim->irqs) { > - kfree(sim); > - return ERR_PTR(-ENOMEM); > - } > - > - sim->irq_base = irq_alloc_descs(-1, 0, num_irqs, 0); > - if (sim->irq_base < 0) { > - kfree(sim->irqs); > - kfree(sim); > - return ERR_PTR(sim->irq_base); > - } > - > sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL); > if (!sim->work_ctx.pending) { > - kfree(sim->irqs); > kfree(sim); > - irq_free_descs(sim->irq_base, num_irqs); > return ERR_PTR(-ENOMEM); > } > > - for (i = 0; i < num_irqs; i++) { > - sim->irqs[i].irqnum = sim->irq_base + i; > - sim->irqs[i].enabled = false; > - irq_set_chip(sim->irq_base + i, &irq_sim_irqchip); > - irq_set_chip_data(sim->irq_base + i, &sim->irqs[i]); > - irq_set_handler(sim->irq_base + i, &handle_simple_irq); > - irq_modify_status(sim->irq_base + i, > - IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE); > - } > + sim->domain = irq_domain_create_linear(NULL, num_irqs, > + &irq_sim_domain_ops, sim); > + if (!sim->domain) > + return ERR_PTR(-ENOMEM); > > init_irq_work(&sim->work_ctx.work, irq_sim_handle_irq); > sim->irq_count = num_irqs; > @@ -143,10 +158,17 @@ EXPORT_SYMBOL_GPL(irq_sim_new); > */ > void irq_sim_free(struct irq_sim *sim) > { > + int i, irq; > + > + for (i = 0; i < sim->irq_count; i++) { > + irq = irq_find_mapping(sim->domain, i); > + if (irq) > + irq_dispose_mapping(irq); > + } > + > + irq_domain_remove(sim->domain); > irq_work_sync(&sim->work_ctx.work); > bitmap_free(sim->work_ctx.pending); > - irq_free_descs(sim->irq_base, sim->irq_count); > - kfree(sim->irqs); > kfree(sim); > } > EXPORT_SYMBOL_GPL(irq_sim_free); > @@ -189,27 +211,31 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_new); > /** > * irq_sim_fire - Enqueue an interrupt. > * > - * @sim: The interrupt simulator object. > - * @offset: Offset of the simulated interrupt which should be fired. > + * @virq: Virtual interrupt number to fire. It must be associated with > + * an existing interrupt simulator. > */ > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset) > +void irq_sim_fire(int virq) > { > - if (sim->irqs[offset].enabled) { > - set_bit(offset, sim->work_ctx.pending); > - irq_work_queue(&sim->work_ctx.work); > + struct irq_sim_irq_ctx *ctx; > + struct irq_data *irqd; > + > + irqd = irq_get_irq_data(virq); > + ctx = irq_data_get_irq_chip_data(irqd); > + > + if (ctx->enabled) { > + set_bit(irqd_to_hwirq(irqd), ctx->work_ctx->pending); > + irq_work_queue(&ctx->work_ctx->work); > } > } > EXPORT_SYMBOL_GPL(irq_sim_fire); > > /** > - * irq_sim_irqnum - Get the allocated number of a dummy interrupt. > + * irq_sim_get_domain - Retrieve the interrupt domain for this simulator. > * > - * @sim: The interrupt simulator object. > - * @offset: Offset of the simulated interrupt for which to retrieve > - * the number. > + * @sim: The interrupt simulator the domain of which we retrieve. > */ > -int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset) > +struct irq_domain *irq_sim_get_domain(struct irq_sim *sim) > { > - return sim->irqs[offset].irqnum; > + return sim->domain; > } > -EXPORT_SYMBOL_GPL(irq_sim_irqnum); > +EXPORT_SYMBOL(irq_sim_get_domain); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] irq/irq_sim: use irq domain 2019-08-12 12:52 ` [PATCH 2/2] irq/irq_sim: use irq domain Bartosz Golaszewski 2019-08-18 19:45 ` Jonathan Cameron @ 2019-08-19 8:18 ` Marc Zyngier 1 sibling, 0 replies; 7+ messages in thread From: Marc Zyngier @ 2019-08-19 8:18 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Linus Walleij, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Thomas Gleixner, linux-gpio, linux-kernel, linux-iio, Bartosz Golaszewski On Mon, 12 Aug 2019 13:52:56 +0100, Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > We currently have a dedicated function to map the interrupt simulator > offsets to global interrupt numbers. This is something that irq_domain > should handle. > > Create a linear irq_domain when initializing the interrupt simulator > and modify the irq_sim_fire() function to only take as parameter the > global interrupt number. > > Convert both users in the same patch to using the new interface. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/gpio/gpio-mockup.c | 11 ++- > drivers/iio/dummy/iio_dummy_evgen.c | 22 ++++-- > include/linux/irq_sim.h | 5 +- > kernel/irq/Kconfig | 1 + > kernel/irq/irq_sim.c | 110 +++++++++++++++++----------- > 5 files changed, 94 insertions(+), 55 deletions(-) > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > index 9b28ffec5826..4cf594f0e7cd 100644 > --- a/drivers/gpio/gpio-mockup.c > +++ b/drivers/gpio/gpio-mockup.c > @@ -186,7 +186,7 @@ static int gpio_mockup_to_irq(struct gpio_chip *gc, unsigned int offset) > { > struct gpio_mockup_chip *chip = gpiochip_get_data(gc); > > - return irq_sim_irqnum(chip->irqsim, offset); > + return irq_create_mapping(irq_sim_get_domain(chip->irqsim), offset); > } > > static void gpio_mockup_free(struct gpio_chip *gc, unsigned int offset) > @@ -228,6 +228,7 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file, > struct gpio_mockup_dbgfs_private *priv; > int rv, val, curr, irq, irq_type; > struct gpio_mockup_chip *chip; > + struct irq_domain *domain; > struct seq_file *sfile; > struct gpio_desc *desc; > struct gpio_chip *gc; > @@ -248,6 +249,7 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file, > gc = &chip->gc; > desc = &gc->gpiodev->descs[priv->offset]; > sim = chip->irqsim; > + domain = irq_sim_get_domain(sim); > > mutex_lock(&chip->lock); > > @@ -257,12 +259,15 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file, > if (curr == val) > goto out; > > - irq = irq_sim_irqnum(sim, priv->offset); > + irq = irq_find_mapping(domain, priv->offset); > + if (!irq) > + return -ENOENT; > + > irq_type = irq_get_trigger_type(irq); > > if ((val == 1 && (irq_type & IRQ_TYPE_EDGE_RISING)) || > (val == 0 && (irq_type & IRQ_TYPE_EDGE_FALLING))) > - irq_sim_fire(sim, priv->offset); > + irq_sim_fire(irq); > } > > /* Change the value unless we're actively driving the line. */ > diff --git a/drivers/iio/dummy/iio_dummy_evgen.c b/drivers/iio/dummy/iio_dummy_evgen.c > index efbcd4a5609e..cc827f60a535 100644 > --- a/drivers/iio/dummy/iio_dummy_evgen.c > +++ b/drivers/iio/dummy/iio_dummy_evgen.c > @@ -31,14 +31,13 @@ > * @lock: protect the evgen state > * @inuse: mask of which irqs are connected > * @irq_sim: interrupt simulator > - * @base: base of irq range > */ > struct iio_dummy_eventgen { > struct iio_dummy_regs regs[IIO_EVENTGEN_NO]; > struct mutex lock; > bool inuse[IIO_EVENTGEN_NO]; > struct irq_sim *irq_sim; > - int base; > + struct irq_domain *domain; > }; > > /* We can only ever have one instance of this 'device' */ > @@ -56,7 +55,7 @@ static int iio_dummy_evgen_create(void) > return PTR_ERR(iio_evgen->irq_sim); > } > > - iio_evgen->base = irq_sim_irqnum(iio_evgen->irq_sim, 0); > + iio_evgen->domain = irq_sim_get_domain(iio_evgen->irq_sim); > mutex_init(&iio_evgen->lock); > > return 0; > @@ -78,7 +77,7 @@ int iio_dummy_evgen_get_irq(void) > mutex_lock(&iio_evgen->lock); > for (i = 0; i < IIO_EVENTGEN_NO; i++) { > if (!iio_evgen->inuse[i]) { > - ret = irq_sim_irqnum(iio_evgen->irq_sim, i); > + ret = irq_create_mapping(iio_evgen->domain, i); > iio_evgen->inuse[i] = true; > break; > } > @@ -99,15 +98,21 @@ EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_irq); > */ > void iio_dummy_evgen_release_irq(int irq) > { > + struct irq_data *irqd; > + > + irqd = irq_get_irq_data(irq); > + > mutex_lock(&iio_evgen->lock); > - iio_evgen->inuse[irq - iio_evgen->base] = false; > + iio_evgen->inuse[irqd_to_hwirq(irqd)] = false; > mutex_unlock(&iio_evgen->lock); > } > EXPORT_SYMBOL_GPL(iio_dummy_evgen_release_irq); > > struct iio_dummy_regs *iio_dummy_evgen_get_regs(int irq) > { > - return &iio_evgen->regs[irq - iio_evgen->base]; > + struct irq_data *irqd = irq_get_irq_data(irq); > + > + return &iio_evgen->regs[irqd_to_hwirq(irqd)]; > } > EXPORT_SYMBOL_GPL(iio_dummy_evgen_get_regs); > > @@ -129,7 +134,7 @@ static ssize_t iio_evgen_poke(struct device *dev, > { > struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); > unsigned long event; > - int ret; > + int ret, irq; > > ret = kstrtoul(buf, 10, &event); > if (ret) > @@ -138,7 +143,8 @@ static ssize_t iio_evgen_poke(struct device *dev, > iio_evgen->regs[this_attr->address].reg_id = this_attr->address; > iio_evgen->regs[this_attr->address].reg_data = event; > > - irq_sim_fire(iio_evgen->irq_sim, this_attr->address); > + irq = irq_create_mapping(iio_evgen->domain, this_attr->address); > + irq_sim_fire(irq); That's not the way the API is intended to be used. irq_create_mapping() is something that should only happen once, or at least as rarely as possible. At interrupt handling time, you should call irq_find_mapping() instead. You need to rethink the way this driver is using interrupts. > > return len; > } > diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h > index 4bbf036145e2..4056d0e7f0b4 100644 > --- a/include/linux/irq_sim.h > +++ b/include/linux/irq_sim.h > @@ -7,6 +7,7 @@ > #define _LINUX_IRQ_SIM_H > > #include <linux/irq_work.h> > +#include <linux/irqdomain.h> > #include <linux/device.h> > > /* > @@ -19,7 +20,7 @@ struct irq_sim; > struct irq_sim *irq_sim_new(unsigned int num_irqs); > struct irq_sim *devm_irq_sim_new(struct device *dev, unsigned int num_irqs); > void irq_sim_free(struct irq_sim *sim); > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset); > -int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset); > +void irq_sim_fire(int virq); > +struct irq_domain *irq_sim_get_domain(struct irq_sim *sim); > > #endif /* _LINUX_IRQ_SIM_H */ > diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig > index f92d9a687372..d0890f7729d4 100644 > --- a/kernel/irq/Kconfig > +++ b/kernel/irq/Kconfig > @@ -68,6 +68,7 @@ config IRQ_DOMAIN > config IRQ_SIM > bool > select IRQ_WORK > + select IRQ_DOMAIN > > # Support for hierarchical irq domains > config IRQ_DOMAIN_HIERARCHY > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c > index 79f0a6494b6c..a1c91aefb6cd 100644 > --- a/kernel/irq/irq_sim.c > +++ b/kernel/irq/irq_sim.c > @@ -15,13 +15,14 @@ struct irq_sim_work_ctx { > struct irq_sim_irq_ctx { > int irqnum; > bool enabled; > + struct irq_sim_work_ctx *work_ctx; > }; > > struct irq_sim { > struct irq_sim_work_ctx work_ctx; > int irq_base; > unsigned int irq_count; > - struct irq_sim_irq_ctx *irqs; > + struct irq_domain *domain; > }; > > struct irq_sim_devres { > @@ -74,11 +75,46 @@ static void irq_sim_handle_irq(struct irq_work *work) > offset = find_next_bit(work_ctx->pending, > sim->irq_count, offset); > clear_bit(offset, work_ctx->pending); > - irqnum = irq_sim_irqnum(sim, offset); > + irqnum = irq_find_mapping(sim->domain, offset); > handle_simple_irq(irq_to_desc(irqnum)); > } > } > > +static int irq_sim_domain_map(struct irq_domain *domain, > + unsigned int virq, irq_hw_number_t hw) > +{ > + struct irq_sim *sim = domain->host_data; > + struct irq_sim_irq_ctx *ctx; > + > + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); > + if (!ctx) > + return -ENOMEM; > + > + irq_set_chip(virq, &irq_sim_irqchip); > + irq_set_chip_data(virq, ctx); > + irq_set_handler(virq, handle_simple_irq); > + irq_modify_status(virq, IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE); > + ctx->work_ctx = &sim->work_ctx; > + > + return 0; > +} > + > +static void irq_sim_domain_unmap(struct irq_domain *domain, unsigned int virq) > +{ > + struct irq_sim_irq_ctx *ctx; > + struct irq_data *irqd; > + > + irqd = irq_domain_get_irq_data(domain, virq); > + ctx = irq_data_get_irq_chip_data(irqd); > + > + kfree(ctx); I'd expect something like: irq_set_handler(virq, NULL); irq_domain_reset_irq_data(irqd); in addition to the above. > +} > + > +static const struct irq_domain_ops irq_sim_domain_ops = { > + .map = irq_sim_domain_map, > + .unmap = irq_sim_domain_unmap, > +}; > + > /** > * irq_sim_new - Create a new interrupt simulator: allocate a range of > * dummy interrupts. > @@ -91,42 +127,21 @@ static void irq_sim_handle_irq(struct irq_work *work) > struct irq_sim *irq_sim_new(unsigned int num_irqs) > { > struct irq_sim *sim; > - int i; > > sim = kmalloc(sizeof(*sim), GFP_KERNEL); > if (!sim) > return ERR_PTR(-ENOMEM); > > - sim->irqs = kmalloc_array(num_irqs, sizeof(*sim->irqs), GFP_KERNEL); > - if (!sim->irqs) { > - kfree(sim); > - return ERR_PTR(-ENOMEM); > - } > - > - sim->irq_base = irq_alloc_descs(-1, 0, num_irqs, 0); > - if (sim->irq_base < 0) { > - kfree(sim->irqs); > - kfree(sim); > - return ERR_PTR(sim->irq_base); > - } > - > sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL); > if (!sim->work_ctx.pending) { > - kfree(sim->irqs); > kfree(sim); > - irq_free_descs(sim->irq_base, num_irqs); > return ERR_PTR(-ENOMEM); > } > > - for (i = 0; i < num_irqs; i++) { > - sim->irqs[i].irqnum = sim->irq_base + i; > - sim->irqs[i].enabled = false; > - irq_set_chip(sim->irq_base + i, &irq_sim_irqchip); > - irq_set_chip_data(sim->irq_base + i, &sim->irqs[i]); > - irq_set_handler(sim->irq_base + i, &handle_simple_irq); > - irq_modify_status(sim->irq_base + i, > - IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE); > - } > + sim->domain = irq_domain_create_linear(NULL, num_irqs, > + &irq_sim_domain_ops, sim); > + if (!sim->domain) > + return ERR_PTR(-ENOMEM); Aren't you now leaking memory from the sim structure? > > init_irq_work(&sim->work_ctx.work, irq_sim_handle_irq); > sim->irq_count = num_irqs; > @@ -143,10 +158,17 @@ EXPORT_SYMBOL_GPL(irq_sim_new); > */ > void irq_sim_free(struct irq_sim *sim) > { > + int i, irq; > + > + for (i = 0; i < sim->irq_count; i++) { > + irq = irq_find_mapping(sim->domain, i); > + if (irq) > + irq_dispose_mapping(irq); > + } > + > + irq_domain_remove(sim->domain); > irq_work_sync(&sim->work_ctx.work); > bitmap_free(sim->work_ctx.pending); > - irq_free_descs(sim->irq_base, sim->irq_count); > - kfree(sim->irqs); > kfree(sim); > } > EXPORT_SYMBOL_GPL(irq_sim_free); > @@ -189,27 +211,31 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_new); > /** > * irq_sim_fire - Enqueue an interrupt. > * > - * @sim: The interrupt simulator object. > - * @offset: Offset of the simulated interrupt which should be fired. > + * @virq: Virtual interrupt number to fire. It must be associated with > + * an existing interrupt simulator. > */ > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset) > +void irq_sim_fire(int virq) > { > - if (sim->irqs[offset].enabled) { > - set_bit(offset, sim->work_ctx.pending); > - irq_work_queue(&sim->work_ctx.work); > + struct irq_sim_irq_ctx *ctx; > + struct irq_data *irqd; > + > + irqd = irq_get_irq_data(virq); > + ctx = irq_data_get_irq_chip_data(irqd); > + > + if (ctx->enabled) { > + set_bit(irqd_to_hwirq(irqd), ctx->work_ctx->pending); > + irq_work_queue(&ctx->work_ctx->work); > } > } > EXPORT_SYMBOL_GPL(irq_sim_fire); > > /** > - * irq_sim_irqnum - Get the allocated number of a dummy interrupt. > + * irq_sim_get_domain - Retrieve the interrupt domain for this simulator. > * > - * @sim: The interrupt simulator object. > - * @offset: Offset of the simulated interrupt for which to retrieve > - * the number. > + * @sim: The interrupt simulator the domain of which we retrieve. > */ > -int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset) > +struct irq_domain *irq_sim_get_domain(struct irq_sim *sim) > { > - return sim->irqs[offset].irqnum; > + return sim->domain; > } > -EXPORT_SYMBOL_GPL(irq_sim_irqnum); > +EXPORT_SYMBOL(irq_sim_get_domain); This seems to be reinventing irq_find_matching_fwnode(). Please consider allocating a fwnode as part of the irq_sim structure, and use this fwnode to lookup the domain instead. M. -- Jazz is not dead, it just smells funny. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-19 8:18 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-12 12:52 [PATCH 0/2] irq/irq_sim: try to improve the API Bartosz Golaszewski 2019-08-12 12:52 ` [PATCH 1/2] irq/irq_sim: make the irq_sim structure opaque Bartosz Golaszewski 2019-08-18 19:38 ` Jonathan Cameron 2019-08-18 19:41 ` Jonathan Cameron 2019-08-12 12:52 ` [PATCH 2/2] irq/irq_sim: use irq domain Bartosz Golaszewski 2019-08-18 19:45 ` Jonathan Cameron 2019-08-19 8:18 ` Marc Zyngier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).