linux-kernel - Re: [PATCH] gpiolib: debugfs: display gpios requested as irq only

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150525203913.GA17840@localhost>
Date:	Mon, 25 May 2015 22:39:13 +0200
From:	Johan Hovold <johan@...nel.org>
To:	"Grygorii.Strashko@...aro.org" <grygorii.strashko@...aro.org>
Cc:	Johan Hovold <johan@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Alexandre Courbot <gnurou@...il.com>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] gpiolib: debugfs: display gpios requested as irq only

On Mon, May 25, 2015 at 09:54:29PM +0300, Grygorii.Strashko@...aro.org wrote:
> On 05/24/2015 08:12 PM, Johan Hovold wrote:
> > On Thu, May 21, 2015 at 11:33:01PM +0300, Grygorii.Strashko@...aro.org wrote:
> >> On 05/21/2015 05:25 PM, Johan Hovold wrote:

> >>> A problem with the current implementation is that it uses as a flag
> >>> rather than a refcount. As I pointed out elsewhere in this thread, it is
> >>> possible to request a shared IRQ (e.g. via the sysfs interface) and
> >>> release it, thereby making it possible to change the direction of the
> >>> pin while still in use for irq.
> >>
> >> Yes (checked). And this is an issue which need to be fixed.
> >> - gpio sysfs should not call gpiochip_un/lock_as_irq()
> > 
> > This is a known but unrelated issue. The lock/unlock call in the sysfs
> > implementation is at worst redundant. I suggested removing it earlier,
> > but Linus pointed out that there were still a few drivers not
> > implementing the irq resource callbacks that need to be updated first.
> > 
> >> - gpio drivers should use gpiochip API or implement
> >>    .irq_release/request_resources() callbacks
> >>
> >> in this case case IRQ core will do just what is required. Right?
> > 
> > No, the problem is that the "lock" is implemented using a flag rather
> > than a refcount.
> 
> I've rechecked __setup_irq() and what I can see from it is that
> irq_request_resources(), __irq_set_trigger() and irq_startup() 
> functions will be called only when the first IRQ action is installed.
> So, It looks like flag should work here. Am I missing smth?

You're absolutely right. I didn't look at the code after I managed to
trigger the bug using sysfs and my memory failed me. Bah, sorry about
that.

It is indeed a sysfs-interface bug.

> > At least the gpio-irq mapping for requested gpios could be useful.
> > 
> > Another issue here is that you would start calling gpio accessors for
> > unrequested gpios. Are you sure all gpio drivers can, and will always be
> > able to, handle that? [ When using the gpiod interface, gpios will always
> > be requested and must not be accessed after having been released. ]
> 
> Agree :(. I'm not sure.  This is very sensible remark:
> - call of gpiod_get_direction() can be avoided, in general, for <irq-only> case
> - but gpiod_to_irq() -- is not.
> 
> .. Looks like it's time to drop this stuff :,,(

You can always export the pins using sysfs or request them using the new
hogging mechanism from DT so that the pins you're interested in
debugging show up in debugfs.

Thanks,
Johan
--
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/