From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com [209.85.221.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4B9B818EAF for ; Fri, 15 Mar 2024 10:35:34 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.44 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1710498937; cv=none; b=BiWKI1M2zrXo4I4T4YdMkHszloUTEy96KN32yxtRepzUp+2MxvltZyZNYb/0Au2BJ8uxhiSaFKUQeMFoV0uHhtvj7bMdRX6s0wBEKh4gI5nrMLaDpJ9dvfSWsI4rzlbuhXdUUGATWUCbiStZceA0nSC2MiiQRXp8vJ/ucFFFek4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1710498937; c=relaxed/simple; bh=ddK85e9HaQMlg4WxOXBx1kUnS/e+ksCCSLAFCn1A4AY=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=SI6m/haYMUinv+27L1ztvCCgjre7SZbTriWBKqefZ5kloJluKXmeP17BItnJ8kJGeShsZBgUx52EgBWm6cgvggwhrUVnaS+BbbmdOGFWELnggE7ad4wY/2Gqa33eT2rQQpiP+qjNCRi1gZRWuxc1qez/7bWSrcKCF9mm7x3PZ2U= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=B3ST2FvW; arc=none smtp.client-ip=209.85.221.44 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="B3ST2FvW" Received: by mail-wr1-f44.google.com with SMTP id ffacd0b85a97d-33e8f906f3dso1566091f8f.3 for ; Fri, 15 Mar 2024 03:35:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1710498933; x=1711103733; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=2/Px3TDK1UCWE3EAuvMks7oCW9mXdtXfrgO477/G3hE=; b=B3ST2FvWE0qz11iXhHoDXh2tbaaiI2cn9vR+AU3os23zVbIAnb1qlJvh5ld8JqED36 6foguGZQG0xfzt52dPaca/h69Ly8SxkaU1+4tcfL4ppVvGE1JiPd0u+bvLAqz1fwXOvK 7PJMAWqE/WAQPgmljSTNTgW7oETxew4GWsc7q2EO8WniBBShlYOWU6nKVT36zXEBp7QU +ykMpsVYXvmt0dJq92zeDdOetS9VmQM/riYTjAVKli93Yeps8UYOp0aGmsiTfV+WgxSL m/fnUFyLUh7Z+we9IfvASP5GA1GRi3nJMiSibPkb2hKw80svQRQCMwpfpNLu4GeidYwx jjPA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1710498933; x=1711103733; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=2/Px3TDK1UCWE3EAuvMks7oCW9mXdtXfrgO477/G3hE=; b=dFzzvkqaqmPpIIqzIoHL+Y2rUh5bvgowVo7GHvuikeRvQxVLqPG5xBHBtm+1CuMqEX ko4dip103A3jg5OiUdziwro3wBLRyZmOthDnMjH4jICkH/cXqXanVdk2CdjOvBhWhIkB MaNB7s/iDzuD24n9AZqDNMGwdeVlxmXzKlkf+/94cDlSEOINo451KefGPkO/bTduDXI5 IqVaipXMQpAECTXREAz5jdm3j/DA7jWJ61OG4hXTTGA5jDupTG/SiJA2FavZvfRNuXNE lnwbvIGhc8j+Ilb+7CCLihw/ZoPK3xauGJmeXalz0Lv2XQw++F31DsUpkzmEtcGvsnM2 jfOA== X-Forwarded-Encrypted: i=1; AJvYcCWW1v4X1E+cACQXfpjoNBbQx8vqtPz6F9Iuk42NfI+/jLUIc0+GSmwh5W/rcUJ/IHTrAspuz3xbprsU2aFhwW8DgoGsMpGV X-Gm-Message-State: AOJu0YxhC2tGI5GMSk3NURIGXONFP0oHMng903/mF28rDsvuh03/lf+p AnbmaO6B2b5fn+BmTpj0m3UzEqjAykfEeNPH9l4tarBV52eQAnFWv0n+XKzhjc1mynEdUJ86FwA W X-Google-Smtp-Source: AGHT+IF39Yuvo7Pqx3lAELS/tfzQXpHT5rvTyEvwwWV7FSWe5R5qaiRYyBZoQCTTcOPsT8J1tItK/Q== X-Received: by 2002:adf:e882:0:b0:33e:c69f:2cae with SMTP id d2-20020adfe882000000b0033ec69f2caemr1853783wrm.23.1710498933006; Fri, 15 Mar 2024 03:35:33 -0700 (PDT) Received: from localhost ([102.222.70.76]) by smtp.gmail.com with ESMTPSA id g14-20020a5d540e000000b0033e95bf4796sm2900299wrv.27.2024.03.15.03.35.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 15 Mar 2024 03:35:32 -0700 (PDT) Date: Fri, 15 Mar 2024 13:35:24 +0300 From: Dan Carpenter To: Rasmus Villemoes Cc: Dan Carpenter , smatch@vger.kernel.org Subject: Re: smatch and locking checks Message-ID: References: Precedence: bulk X-Mailing-List: smatch@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: [ Heh. I asked Rasmus if I could add the list the the CC but I forgot to do that. Let me resend - dan ] Hi Rasmus, On Mon, Mar 11, 2024 at 01:18:35PM +0100, Rasmus Villemoes wrote: > Hi Dan > > It's been a million years since I had time to work with smatch. I do > vaguely remember that it can do some lock sanity checks, but I don't > know how much it can do, or if the code needs some special markup to > help smatch. As a rule, I'd prefer to avoid markup. These days we prefer to put stuff into tables like in check_preempt_info.c and check_unwind.c. Use that along with the cross function database instead of markup. > > A colleague of mine hit the lockdep_assert_held_once() in > uart_handle_cts_change(). The driver in question is mxs-auart.c, which > sure enough calls uart_handle_cts_change() from mxs_auart_irq_handle() > [both directly and via the mxs_auart_modem_status() helper] without > holding that lock. > > The question is, can smatch read lockdep annotations, and if so, can it > also know that on entry to an irq handler, no locks are held? Smatch doesn't read lockdep annotations. It would be interesting to check for lockdep assert failures. Smatch does tracking locking information in the DB. The cross function DB is explained in this blog. (I had thought I added it to the Documentation/ directory but apparently not). https://staticthinking.wordpress.com/2023/05/02/the-cross-function-db/ Below is the output from: smdb.py uart_handle_cts_change | egrep '(INTERNAL|LOCK)' INTERNAL means the start of a new call. As you can see, to Smatch it looks like only three of the callers are holding &uport->lock. The problem is that &icom_port->uart_port.lock and &u->lock are probably the lock but Smatch isn't clever enough to map those names correctly. For serial8250_modem_status() it knows that the callers are each holding a lock but neither of them are mapped correctly. drivers/tty/serial/8250/8250_port.c | serial8250_modem_status | uart_handle_cts_change | HALF_LOCKED | -2 | &port->lock | drivers/tty/serial/8250/8250_port.c | serial8250_modem_status | uart_handle_cts_change | HALF_LOCKED | -2 | &up->port.lock | The -2 in the lines above means that Smatch can't connect the lock to a function parameter. I recently created a module to track IRQ handlers so that I could print a warning about scheduling in IRQ handlers. But that's probably not the best way to handle this warning. The locking information is *almost* good enough to be useful on its own. My idea is that instead of recording the lock as '&port->lock', we'd create a new module that would store basically the same information but with the lock name as '(struct struct uart_port)->lock'. That would be easier except it wouldn't be really useful for anything except warning about lockdep_assert_held(). regards, dan carpenter drivers/tty/serial/icom.c | check_modem_status | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/icom.c | check_modem_status | uart_handle_cts_change | HALF_LOCKED | -2 | &desc->request_mutex | drivers/tty/serial/icom.c | check_modem_status | uart_handle_cts_change | LOCKED | -2 | &icom_port->uart_port.lock | drivers/tty/serial/icom.c | check_modem_status | uart_handle_cts_change | HALF_LOCKED | -2 | flags | drivers/tty/serial/serial-tegra.c | tegra_uart_handle_modem_signal_change | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/serial-tegra.c | tegra_uart_handle_modem_signal_change | uart_handle_cts_change | HALF_LOCKED | -2 | &desc->request_mutex | drivers/tty/serial/serial-tegra.c | tegra_uart_handle_modem_signal_change | uart_handle_cts_change | LOCKED | -2 | &u->lock | drivers/tty/serial/serial-tegra.c | tegra_uart_handle_modem_signal_change | uart_handle_cts_change | HALF_LOCKED | -2 | flags | drivers/tty/serial/sc16is7xx.c | sc16is7xx_update_mlines | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/sc16is7xx.c | sc16is7xx_update_mlines | uart_handle_cts_change | LOCKED | 0 | &uport->lock | drivers/tty/serial/sc16is7xx.c | sc16is7xx_update_mlines | uart_handle_cts_change | LOCKED | -2 | &one->efr_lock | drivers/tty/serial/sc16is7xx.c | sc16is7xx_update_mlines | uart_handle_cts_change | LOCKED | -2 | flags | drivers/tty/serial/8250/8250_port.c | serial8250_modem_status | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/8250/8250_port.c | serial8250_modem_status | uart_handle_cts_change | HALF_LOCKED | -2 | &port->lock | drivers/tty/serial/8250/8250_port.c | serial8250_modem_status | uart_handle_cts_change | HALF_LOCKED | -2 | &up->port.lock | drivers/tty/serial/8250/8250_port.c | serial8250_modem_status | uart_handle_cts_change | HALF_LOCKED | -2 | flags | drivers/tty/serial/mxs-auart.c | mxs_auart_modem_status | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/mxs-auart.c | mxs_auart_modem_status | uart_handle_cts_change | HALF_LOCKED | -2 | &desc->request_mutex | drivers/tty/serial/mxs-auart.c | mxs_auart_modem_status | uart_handle_cts_change | HALF_LOCKED | -2 | flags | drivers/tty/serial/mxs-auart.c | mxs_auart_irq_handle | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/mxs-auart.c | mxs_auart_irq_handle | uart_handle_cts_change | HALF_LOCKED | -2 | &desc->request_mutex | drivers/tty/serial/mxs-auart.c | mxs_auart_irq_handle | uart_handle_cts_change | HALF_LOCKED | -2 | flags | drivers/tty/serial/serial_mctrl_gpio.c | mctrl_gpio_irq_handle | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/serial_mctrl_gpio.c | mctrl_gpio_irq_handle | uart_handle_cts_change | LOCKED | 0 | &uport->lock | drivers/tty/serial/serial_mctrl_gpio.c | mctrl_gpio_irq_handle | uart_handle_cts_change | HALF_LOCKED | -2 | &desc->request_mutex | drivers/tty/serial/serial_mctrl_gpio.c | mctrl_gpio_irq_handle | uart_handle_cts_change | LOCKED | -2 | flags | drivers/tty/serial/serial_mctrl_gpio.c | mctrl_gpio_irq_handle | uart_handle_cts_change | HALF_LOCKED | -2 | flags | drivers/tty/serial/atmel_serial.c | atmel_handle_status | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/atmel_serial.c | atmel_handle_status | uart_handle_cts_change | LOCKED | -2 | &atmel_port->lock_suspended | drivers/tty/serial/atmel_serial.c | atmel_handle_status | uart_handle_cts_change | HALF_LOCKED | -2 | flags | drivers/tty/serial/max310x.c | max310x_port_irq | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/imx.c | __imx_uart_rtsint | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/imx.c | __imx_uart_rtsint | uart_handle_cts_change | HALF_LOCKED | -2 | &desc->request_mutex | drivers/tty/serial/imx.c | __imx_uart_rtsint | uart_handle_cts_change | LOCKED | -2 | &sport->port.lock | drivers/tty/serial/imx.c | __imx_uart_rtsint | uart_handle_cts_change | HALF_LOCKED | -2 | flags | drivers/tty/serial/imx.c | imx_uart_mctrl_check | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/imx.c | imx_uart_mctrl_check | uart_handle_cts_change | LOCKED | -2 | &sport->port.lock | drivers/tty/serial/imx.c | imx_uart_mctrl_check | uart_handle_cts_change | HALF_LOCKED | -2 | flags | drivers/tty/serial/omap-serial.c | check_modem_status | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/omap-serial.c | check_modem_status | uart_handle_cts_change | HALF_LOCKED | -2 | &port->lock | drivers/tty/serial/omap-serial.c | check_modem_status | uart_handle_cts_change | HALF_LOCKED | -2 | &up->port.lock | drivers/tty/serial/men_z135_uart.c | men_z135_handle_modem_status | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/men_z135_uart.c | men_z135_handle_modem_status | uart_handle_cts_change | HALF_LOCKED | -2 | &desc->request_mutex | drivers/tty/serial/men_z135_uart.c | men_z135_handle_modem_status | uart_handle_cts_change | LOCKED | -2 | &port->lock | drivers/tty/serial/men_z135_uart.c | men_z135_handle_modem_status | uart_handle_cts_change | HALF_LOCKED | -2 | flags | drivers/tty/serial/timbuart.c | timbuart_mctrl_check | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/timbuart.c | timbuart_mctrl_check | uart_handle_cts_change | LOCKED | -2 | &uart->port.lock | drivers/tty/serial/jsm/jsm_neo.c | neo_parse_modem | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/jsm/jsm_neo.c | neo_parse_modem | uart_handle_cts_change | HALF_LOCKED | -2 | &brd->bd_intr_lock | drivers/tty/serial/jsm/jsm_neo.c | neo_parse_modem | uart_handle_cts_change | HALF_LOCKED | -2 | &ch->uart_port.lock | drivers/tty/serial/jsm/jsm_neo.c | neo_parse_modem | uart_handle_cts_change | HALF_LOCKED | -2 | &port->lock | drivers/tty/serial/jsm/jsm_neo.c | neo_parse_modem | uart_handle_cts_change | LOCKED | -2 | lock_flags | drivers/tty/serial/bcm63xx_uart.c | bcm_uart_interrupt | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/bcm63xx_uart.c | bcm_uart_interrupt | uart_handle_cts_change | LOCKED | 0 | &uport->lock | drivers/tty/serial/bcm63xx_uart.c | bcm_uart_interrupt | uart_handle_cts_change | HALF_LOCKED | -2 | &desc->request_mutex | drivers/tty/serial/bcm63xx_uart.c | bcm_uart_interrupt | uart_handle_cts_change | HALF_LOCKED | -2 | flags | drivers/tty/serial/max3100.c | max3100_handlerx | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/max3100.c | max3100_handlerx | uart_handle_cts_change | HALF_LOCKED | -2 | &pool->lock | drivers/tty/serial/amba-pl010.c | pl010_modem_status | uart_handle_cts_change | INTERNAL | -1 | | void(*)(struct uart_port*, bool) drivers/tty/serial/amba-pl010.c | pl010_modem_status | uart_handle_cts_change | HALF_LOCKED | -2 | &desc->request_mutex | drivers/tty/serial/amba-pl010.c | pl010_modem_status | uart_handle_cts_change | LOCKED | -2 | &port->lock | drivers/tty/serial/amba-pl010.c | pl010_modem_status | uart_handle_cts_change | HALF_LOCKED | -2 | flags |