Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752549AbcLBV2e (ORCPT ); Fri, 2 Dec 2016 16:28:34 -0500 Received: from mail-lf0-f43.google.com ([209.85.215.43]:36561 "EHLO mail-lf0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751219AbcLBV2c (ORCPT ); Fri, 2 Dec 2016 16:28:32 -0500 Subject: Re: [PATCH] tty: serial: fsl_lpuart: fix del_timer_sync() vs timer routine deadlock To: Stefan Agner References: <1480674490-24718-1-git-send-email-nikita.yoush@cogentembedded.com> <77e2726b8b84c55f9109ff18dfcfbbdf@agner.ch> Cc: Greg Kroah-Hartman , Jiri Slaby , Bhuvanchandra DV , Wei Yongjun , Aaron Brice , Nicolae Rosia , linux-serial@vger.kernel.org, Chris Healy , linux-kernel@vger.kernel.org From: Nikita Yushchenko X-Enigmail-Draft-Status: N1110 Message-ID: Date: Sat, 3 Dec 2016 00:28:27 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.4.0 MIME-Version: 1.0 In-Reply-To: <77e2726b8b84c55f9109ff18dfcfbbdf@agner.ch> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1334 Lines: 37 >> Problem found via lockdep: >> >> - lpuart_set_termios() calls del_timer_sync(&sport->lpuart_timer) while >> holding sport->port.lock >> >> - sport->lpuart_timer routine is lpuart_timer_func() that calls >> lpuart_copy_rx_to_tty() that acquires same lock. >> >> To fix, move Rx DMA stopping out of lock, as it already is in other places >> in the same file. >> >> While at it, also make Rx DMA start/stop code to look the same is in >> other places in the same file. > > Yeah I saw that too, never really came around to look closer into it. > > Thanks for looking into it. > > You removed the check whether there was an old configuration, I think > the idea of that was that we only resize DMA if it was configured > differently before... Per my code reading, checking for sport->lpuart_dma_rx_use should be enough, this flag will be set only if DMA was previously enabled, >> + if (sport->lpuart_dma_rx_use) { >> + if (!lpuart_start_rx_dma(sport)) { >> sport->lpuart_dma_rx_use = true; > > No need to set to true here, it is guaranteed to be true at this point. I've seen this... However elsewhere in this file (namely in lpuart_resume(), in very similar situation, code is exactly the same, i.e. it sets sport->lpuart_dma_rx_use in both clauses. I thought it could be for a reason (i.e. for readability). Nikita