From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755178AbbHNMeD (ORCPT ); Fri, 14 Aug 2015 08:34:03 -0400 Received: from mail-ob0-f182.google.com ([209.85.214.182]:35446 "EHLO mail-ob0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754091AbbHNMeA (ORCPT ); Fri, 14 Aug 2015 08:34:00 -0400 MIME-Version: 1.0 In-Reply-To: <1439477919-24356-1-git-send-email-grygorii.strashko@ti.com> References: <1439477919-24356-1-git-send-email-grygorii.strashko@ti.com> Date: Fri, 14 Aug 2015 14:34:00 +0200 Message-ID: Subject: Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip From: Linus Walleij To: Grygorii Strashko Cc: Alexandre Courbot , Linux-OMAP , "linux-gpio@vger.kernel.org" , Sekhar Nori , "linux-kernel@vger.kernel.org" , Geert Uytterhoeven , Roger Quadros Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Aug 13, 2015 at 4:58 PM, Grygorii Strashko wrote: > Since IRQ chip helpers were introduced drivers lose ability to > register separate lockdep classes for each registered GPIO IRQ > chip and the gpiolib now is using shared lockdep class for > all GPIO IRQ chips (gpiochip_irq_lock_class). > As result, lockdep will produce warning when there are min two > stacked GPIO chips and all of them are interrupt controllers. > > HW configuration which generates lockdep warning (TI dra7-evm): (...) > > Cc: Geert Uytterhoeven > Cc: Roger Quadros > Reported-by: Roger Quadros > Signed-off-by: Grygorii Strashko Ah, I see... > * implies that if the chip supports IRQs, these IRQs need to be threaded > * as the chip access may sleep when e.g. reading out the IRQ status > * registers. > + * @exported: flags if the gpiochip is exported for use from sysfs. Private. > * @irq_not_threaded: flag must be set if @can_sleep is set but the > * IRQs don't need to be threaded > * > @@ -126,6 +128,7 @@ struct gpio_chip { > irq_flow_handler_t irq_handler; > unsigned int irq_default_type; > int irq_parent; > + struct lock_class_key *lock_key; There is something weird with the kerneldoc. It is documenting something else but not documenting the new member. Anyway, so here: > +int _gpiochip_irqchip_add(struct gpio_chip *gpiochip, > + struct irq_chip *irqchip, > + unsigned int first_irq, > + irq_flow_handler_t handler, > + unsigned int type, > + struct lock_class_key *lock_key); > + > +#ifdef CONFIG_LOCKDEP > +#define gpiochip_irqchip_add(...) \ > +( \ > + ({ \ > + static struct lock_class_key _key; \ > + _gpiochip_irqchip_add(__VA_ARGS__, &_key); \ > + }) \ > +) > +#else > +#define gpiochip_irqchip_add(...) \ > + _gpiochip_irqchip_add(__VA_ARGS__, NULL) > +#endif Every chip will get their own lock class on the heap. But I think it is a bit kludgy. Is it not possible to have the lock key in struct gpio_chip be a real member instead of a pointer and get a per-chip lock that way? (...) struct lock_class_key lock_key; instead of: struct lock_class_key *lock_key; -> problem solved, without kludgy header defines? Yours, Linus Walleij