From patchwork Thu May 21 20:33:01 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Grygorii.Strashko@linaro.org" X-Patchwork-Id: 48875 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-wg0-f69.google.com (mail-wg0-f69.google.com [74.125.82.69]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 1772921411 for ; Thu, 21 May 2015 20:33:22 +0000 (UTC) Received: by wgbgf7 with SMTP id gf7sf28116635wgb.2 for ; Thu, 21 May 2015 13:33:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:from:message-id:date:user-agent :mime-version:to:cc:subject:references:in-reply-to:content-type :content-transfer-encoding:sender:precedence:list-id :x-original-sender:x-original-authentication-results:mailing-list :list-post:list-help:list-archive:list-unsubscribe; bh=IkVGbNRStjO2M6JPP7jsnn5fA8PTwA3HIoed2Lm4kvA=; b=nAdALHIEdwZqh1VQsOJU6p8wp8o2vJYpHduPdGDSWE5ENjsDnNDvIx1OyQYS3/4/Xx f6H6tuOnCrcvwli9pX4sP1ef1+1nWwA6gnX9Z8uN/8lacpMME2NTpdXl6YaWQDipG5GG E3q9kiyLJ2yOZ9nX1k4UrUhw1uo9j3UH39DPFn0eYplIFPnBZno8kCxVoxyLqOhXx+h8 I6y2iGuWNFAnaMq1Lar2K8gfHjAwNl8O9uN1n6mo7vop0km4Tx/7kZbduXSUuuryeKAE fpIT5Q7L7S7ISDPJyOJxnLHO9d8jhvh3IL4rONnEsPPgqdqULfUkD6OeDG8bq1+cebtQ FYRw== X-Gm-Message-State: ALoCoQnU1NCrMRAWZbkUFEp8LpEEokNacf4r2xQy9EX6vqi41WeHyeOUKQtK876qzu2gjAUopEW/ X-Received: by 10.180.106.136 with SMTP id gu8mr517253wib.6.1432240401409; Thu, 21 May 2015 13:33:21 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.30.74 with SMTP id q10ls425197lah.35.gmail; Thu, 21 May 2015 13:33:21 -0700 (PDT) X-Received: by 10.112.17.68 with SMTP id m4mr484223lbd.10.1432240401242; Thu, 21 May 2015 13:33:21 -0700 (PDT) Received: from mail-lb0-f182.google.com (mail-lb0-f182.google.com. [209.85.217.182]) by mx.google.com with ESMTPS id l3si13925231lbc.147.2015.05.21.13.33.20 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 21 May 2015 13:33:20 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.182 as permitted sender) client-ip=209.85.217.182; Received: by lbbuc2 with SMTP id uc2so26666460lbb.2 for ; Thu, 21 May 2015 13:33:20 -0700 (PDT) X-Received: by 10.152.6.69 with SMTP id y5mr3628891lay.72.1432240400739; Thu, 21 May 2015 13:33:20 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.108.230 with SMTP id hn6csp854217lbb; Thu, 21 May 2015 13:33:19 -0700 (PDT) X-Received: by 10.68.237.40 with SMTP id uz8mr8841568pbc.140.1432240399282; Thu, 21 May 2015 13:33:19 -0700 (PDT) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id b1si7692063pbu.175.2015.05.21.13.33.16; Thu, 21 May 2015 13:33:19 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-gpio-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756135AbbEUUdQ (ORCPT + 1 other); Thu, 21 May 2015 16:33:16 -0400 Received: from mail-wi0-f171.google.com ([209.85.212.171]:36627 "EHLO mail-wi0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756079AbbEUUdP (ORCPT ); Thu, 21 May 2015 16:33:15 -0400 Received: by wizk4 with SMTP id k4so27368682wiz.1 for ; Thu, 21 May 2015 13:33:13 -0700 (PDT) X-Received: by 10.180.8.98 with SMTP id q2mr897635wia.53.1432240393846; Thu, 21 May 2015 13:33:13 -0700 (PDT) Received: from [172.22.39.17] ([195.238.92.128]) by mx.google.com with ESMTPSA id xy5sm33773196wjc.35.2015.05.21.13.33.07 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 21 May 2015 13:33:08 -0700 (PDT) From: "Grygorii.Strashko@linaro.org" Message-ID: <555E40FD.7010209@linaro.org> Date: Thu, 21 May 2015 23:33:01 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Johan Hovold , Linus Walleij CC: Grygorii Strashko , Alexandre Courbot , "linux-gpio@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH] gpiolib: debugfs: display gpios requested as irq only References: <1431696321-7257-1-git-send-email-grygorii.strashko@linaro.org> <20150518110214.GC28127@localhost> <20150521142524.GA30660@localhost> In-Reply-To: <20150521142524.GA30660@localhost> Sender: linux-gpio-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: linux-gpio@vger.kernel.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: grygorii.strashko@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.182 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , On 05/21/2015 05:25 PM, Johan Hovold wrote: > On Tue, May 19, 2015 at 04:28:55PM +0200, Linus Walleij wrote: >> On Mon, May 18, 2015 at 1:02 PM, Johan Hovold wrote: >>> On Fri, May 15, 2015 at 04:25:21PM +0300, grygorii.strashko@linaro.org wrote: >> >>>> GPIOs 192-223, platform/48051000.gpio, gpio: >>>> gpio-203 (vtt_fixed ) out hi requested >>> >>> This is backwards. All gpios *should* be requested. *If* we are to >>> include not-requested gpios in the debug output, then it is those pins >>> that need to be marked as not-requested. >> >> It depends, really. As concluded in earlier discussions when we >> introduced gpiochip_[un]lock_as_irq() the gpiolib and irqchip APIs >> are essentially orthogonal. > > [...] > >> So to atleast try to safeguard from a scenario such as >> >> - Client A requests IRQ from the irqchip side of the API >> and sets up a level active-low IRQ on it >> >> - Client B request the same line as GPIO >> >> - Client B sets it to output and drivers it low. >> >> - Client A crashes in an infinite IRQ loop as that line >> is not hammered low and will generate IRQs until >> the end of time. >> >> I introduced the gpiochip_[un]lock_as_irq() calls so we >> could safeguard against this. Notably that blocks client A >> from setting the line as output. I hope. > > 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() - 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? > >> I thought this would mean the line would only be used as IRQ >> in this case, so I could block any gpiod_get() calls to that >> line but *of course* some driver is using the IRQ and snooping >> into the GPIO value at the same time. So can't simplify things >> like so either. >> >> Maybe I'm smashing open doors here... > > No, I understand that use case. But there are some issues with how it's > currently implemented. Besides the example above, nothing pins a gpio > chip driver in memory unless it has requested gpios, specifically, > requesting a pin for irq use is not enough. ok. An issue. Possible fix below: > >> Anyway to get back to the original statement: >> >>> This is backwards. All gpios *should* be requested. *If* we are to >>> include not-requested gpios in the debug output, then it is those pins >>> that need to be marked as not-requested. >> >> This is correct, all GPIOs accessed *as gpios* should be >> requested, no matter if they are then cast to IRQs by gpiod_to_irq >> or not. However if the same hardware is used as only "some IRQ" >> through it's irqchip interface, it needs not be requested, but >> that is by definition not a GPIO, it is an IRQ. > > True. And since it is not a GPIO, should it show up in > /sys/kernel/debug/gpio? ;) "Nice" idea :) This information needed for debugging and testing which includes checking of pin state (hi/lo) - especially useful during board's bring-up when a lot of mistakes are detected related to wrong usage of IRQ/GPIO numbers. diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index ea11706..64392ad 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -514,6 +514,9 @@ static int gpiochip_irq_reqres(struct irq_data *d) { struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + if (!try_module_get(chip->owner)) + return -ENODEV; + if (gpiochip_lock_as_irq(chip, d->hwirq)) { chip_err(chip, "unable to lock HW IRQ %lu for IRQ\n", @@ -528,6 +531,7 @@ static void gpiochip_irq_relres(struct irq_data *d) struct gpio_chip *chip = irq_data_get_irq_chip_data(d); gpiochip_unlock_as_irq(chip, d->hwirq); + module_put(chip->owner); }