From patchwork Tue Aug 4 06:09:56 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nicolas Boichat X-Patchwork-Id: 6934201 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id D2807C05AC for ; Tue, 4 Aug 2015 06:12:41 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id A86B02063B for ; Tue, 4 Aug 2015 06:12:40 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 8E2FE20620 for ; Tue, 4 Aug 2015 06:12:39 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZMVQn-0008AX-Vk; Tue, 04 Aug 2015 06:10:33 +0000 Received: from mail-pa0-x22b.google.com ([2607:f8b0:400e:c03::22b]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZMVQk-00085U-8a for linux-arm-kernel@lists.infradead.org; Tue, 04 Aug 2015 06:10:31 +0000 Received: by pawu10 with SMTP id u10so35338paw.1 for ; Mon, 03 Aug 2015 23:10:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=from:to:cc:subject:date:message-id; bh=W8z56wNKBtAtAbvTrrVZgGdo7Id5rjYQBCblafGIsKQ=; b=H1Q1C//bH3AeunFRhA92DD43Yxk11hV03D3mxmbTJviMR9bjlvMcr6yOmORldt3DDa qKEQjpx3xDw3YQEIbS4RKnpMtGtJYa5uHl5Mh4vMOcRMcoZsNr8S20ld1e8BPApuzSxt tXyQxUONdMXng+YWOqEO4nX4FPd8LdufX5ZpI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=W8z56wNKBtAtAbvTrrVZgGdo7Id5rjYQBCblafGIsKQ=; b=R12PqCOtEy837TgcDKWDrzjmg2aVOEyo+t2S5VHyndGxHtqmLqNh9pa7mfkclNTYul yeR7N8aMx8ZRO0MjjPVUmj3BOM+lqLPGlvhM5XWVoc6B9barVJ9/drvZRC+6TPgfyVJr HetcXMk9lSoAAsHfa7pcUvEvHB6WCRSgtwcDP4+Rzc3uZGGgjrl5fwfwoEzNRSM05oWf ZOK5CceHcVivmye15XrQR4sEVg2hwOR24UKfZszvT0UVQkd9rp3c38OohzKveNoW3ZVb PiRXGwdp7T1e19FNrX+4RxvYmc8UBfKUoMxrM6Lf4SjW7lo7wJzqqApTdce/0ChZZ2/O 5gFw== X-Gm-Message-State: ALoCoQmtehebh6z6TdwvuFZT0iucNFhDWHf4DKYJohKFIRFGQy+j28i0YUTHbNZYURznXQ8uibAd X-Received: by 10.66.122.4 with SMTP id lo4mr4265916pab.1.1438668608884; Mon, 03 Aug 2015 23:10:08 -0700 (PDT) Received: from drinkcat.tpe.corp.google.com ([172.30.210.53]) by smtp.gmail.com with ESMTPSA id fv2sm9916456pbd.54.2015.08.03.23.10.07 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 03 Aug 2015 23:10:08 -0700 (PDT) From: Nicolas Boichat To: Mark Brown Subject: [PATCH 1/3] spi: bitbang: Replace spinlock by mutex when calling chipselect Date: Tue, 4 Aug 2015 14:09:56 +0800 Message-Id: <1438668598-6678-1-git-send-email-drinkcat@chromium.org> X-Mailer: git-send-email 2.5.0.rc2.392.g76e840b X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150803_231030_343946_F015885B X-CRM114-Status: GOOD ( 21.65 ) X-Spam-Score: -2.7 (--) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Krzysztof Kozlowski , linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org, Kukjin Kim , linux-arm-kernel@lists.infradead.org MIME-Version: 1.0 Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Enabling CONFIG_DEBUG_ATOMIC_SLEEP in kernel configuration, we get this warning in spi_gpio_setup: [ 1.177747] BUG: sleeping function called from invalid context at drivers/gpio/gpiolib.c:1431 [ 1.190182] in_atomic(): 1, irqs_disabled(): 128, pid: 1, name: swapper/0 [ 1.196922] 3 locks held by swapper/0/1: [ 1.200812] #0: (&dev->mutex){......}, at: [] __driver_attach+0x58/0x98 [ 1.209147] #1: (spi_add_lock){+.+.+.}, at: [] spi_add_device+0x80/0x14c [ 1.217564] #2: (&(&bitbang->lock)->rlock){......}, at: [] spi_bitbang_setup+0x84/0xc4 [ 1.227185] irq event stamp: 279856 [ 1.230645] hardirqs last enabled at (279855): [] __mutex_unlock_slowpath+0x158/0x16c [ 1.240070] hardirqs last disabled at (279856): [] _raw_spin_lock_irqsave+0x20/0x6c [ 1.249233] softirqs last enabled at (262072): [] bdi_register+0x124/0x1d0 [ 1.257707] softirqs last disabled at (262070): [] bdi_register+0x100/0x1d0 [ 1.266185] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 3.18.0 #608 [ 1.277419] Call trace: [ 1.279848] [] dump_backtrace+0x0/0x12c [ 1.285209] [] show_stack+0x10/0x1c [ 1.290223] [] dump_stack+0x80/0xb4 [ 1.295238] [] __might_sleep+0x110/0x11c [ 1.300687] [] gpiod_set_raw_value_cansleep+0x24/0x4c [ 1.307255] [] spi_gpio_chipselect+0x74/0x88 [ 1.313045] [] spi_bitbang_setup+0x98/0xc4 [ 1.318664] [] spi_gpio_setup+0x50/0xc8 [ 1.324022] [] spi_setup+0xe4/0xf8 [ 1.328950] [] spi_add_device+0xd0/0x14c [ 1.334396] [] spi_register_master+0x6a8/0x718 [ 1.340359] [] spi_bitbang_start+0xe8/0x108 [ 1.346064] [] spi_gpio_probe+0x3b4/0x448 [ 1.351595] [] platform_drv_probe+0x4c/0x9c [ 1.357301] [] driver_probe_device+0xd4/0x23c [ 1.363180] [] __driver_attach+0x68/0x98 [ 1.368627] [] bus_for_each_dev+0x7c/0xb0 [ 1.374160] [] driver_attach+0x1c/0x28 [ 1.379434] [] bus_add_driver+0xd8/0x1e0 [ 1.384881] [] driver_register+0xbc/0x10c [ 1.390412] [] __platform_driver_register+0x5c/0x68 [ 1.396808] [] spi_gpio_driver_init+0x14/0x20 [ 1.402685] [] do_one_initcall+0x18c/0x1ac [ 1.408306] [] kernel_init_freeable+0x228/0x2e0 [ 1.414356] [] kernel_init+0x10/0xd8 chipselect (in this case, spi_gpio_chipselect, which calls gpiod_set_raw_value_cansleep), can sleep, so we should not hold a spinlock while calling it. This issue was introduced by this commit, which converted spi-gpio to cansleep variants: d9dda5a191 "spi: spi-gpio: Use 'cansleep' variants to access GPIO" Replace spinlock + busy variable by a mutex, and get rid of spi_bitbang_prepare_hardware and spi_bitbang_unprepare_hardware, which are not useful anymore. Signed-off-by: Nicolas Boichat --- Actually, I'm not sure if I understand the existing code: why are we not waiting for busy to go down to 0, then call chipselect, instead of not calling it at all if the bus happens to be busy when we setup the device? With the current approach, it would be easy to just use an unconditional mutex_lock. Also, is it harmful to deactivate the newly setup device in spi_bitbang_setup, even if the bus is busy with another device? chipselect should be independent for each device (or is it not?). So I'm not clear why we need any locking at all... Hopefully someone can shine some light on this... Anyway, this patch series does not change the existing behaviour, applies on top of broonie-sound/for-next, and, along with the 2 follow-up patches, was compile-tested on x86-64/arm (allyesconfig) and ppc44x (defconfig+SPI driver), and runtime-tested on an arm platform. drivers/spi/spi-bitbang.c | 42 +++++++---------------------------------- include/linux/spi/spi_bitbang.h | 3 +-- 2 files changed, 8 insertions(+), 37 deletions(-) diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c index 840a498..931c37e 100644 --- a/drivers/spi/spi-bitbang.c +++ b/drivers/spi/spi-bitbang.c @@ -180,7 +180,6 @@ int spi_bitbang_setup(struct spi_device *spi) { struct spi_bitbang_cs *cs = spi->controller_state; struct spi_bitbang *bitbang; - unsigned long flags; bitbang = spi_master_get_devdata(spi->master); @@ -210,12 +209,11 @@ int spi_bitbang_setup(struct spi_device *spi) */ /* deselect chip (low or high) */ - spin_lock_irqsave(&bitbang->lock, flags); - if (!bitbang->busy) { + if (mutex_trylock(&bitbang->lock)) { bitbang->chipselect(spi, BITBANG_CS_INACTIVE); ndelay(cs->nsecs); + mutex_unlock(&bitbang->lock); } - spin_unlock_irqrestore(&bitbang->lock, flags); return 0; } @@ -252,20 +250,6 @@ static int spi_bitbang_bufs(struct spi_device *spi, struct spi_transfer *t) * transfer-at-a-time ones to leverage dma or fifo hardware. */ -static int spi_bitbang_prepare_hardware(struct spi_master *spi) -{ - struct spi_bitbang *bitbang; - unsigned long flags; - - bitbang = spi_master_get_devdata(spi); - - spin_lock_irqsave(&bitbang->lock, flags); - bitbang->busy = 1; - spin_unlock_irqrestore(&bitbang->lock, flags); - - return 0; -} - static int spi_bitbang_transfer_one(struct spi_master *master, struct spi_message *m) { @@ -279,6 +263,8 @@ static int spi_bitbang_transfer_one(struct spi_master *master, bitbang = spi_master_get_devdata(master); + mutex_lock(&bitbang->lock); + /* FIXME this is made-up ... the correct value is known to * word-at-a-time bitbang code, and presumably chipselect() * should enforce these requirements too? @@ -372,21 +358,9 @@ static int spi_bitbang_transfer_one(struct spi_master *master, spi_finalize_current_message(master); - return status; -} - -static int spi_bitbang_unprepare_hardware(struct spi_master *spi) -{ - struct spi_bitbang *bitbang; - unsigned long flags; + mutex_unlock(&bitbang->lock); - bitbang = spi_master_get_devdata(spi); - - spin_lock_irqsave(&bitbang->lock, flags); - bitbang->busy = 0; - spin_unlock_irqrestore(&bitbang->lock, flags); - - return 0; + return status; } /*----------------------------------------------------------------------*/ @@ -427,7 +401,7 @@ int spi_bitbang_start(struct spi_bitbang *bitbang) if (!master || !bitbang->chipselect) return -EINVAL; - spin_lock_init(&bitbang->lock); + mutex_init(&bitbang->lock); if (!master->mode_bits) master->mode_bits = SPI_CPOL | SPI_CPHA | bitbang->flags; @@ -435,8 +409,6 @@ int spi_bitbang_start(struct spi_bitbang *bitbang) if (master->transfer || master->transfer_one_message) return -EINVAL; - master->prepare_transfer_hardware = spi_bitbang_prepare_hardware; - master->unprepare_transfer_hardware = spi_bitbang_unprepare_hardware; master->transfer_one_message = spi_bitbang_transfer_one; if (!bitbang->txrx_bufs) { diff --git a/include/linux/spi/spi_bitbang.h b/include/linux/spi/spi_bitbang.h index 85578d4..1329053 100644 --- a/include/linux/spi/spi_bitbang.h +++ b/include/linux/spi/spi_bitbang.h @@ -4,8 +4,7 @@ #include struct spi_bitbang { - spinlock_t lock; - u8 busy; + struct mutex lock; u8 use_dma; u8 flags; /* extra spi->mode support */