From: Dan Carpenter <dan.carpenter@linaro.org>
To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Dan Carpenter <error27@gmail.com>, smatch@vger.kernel.org
Subject: Re: smatch and locking checks
Date: Fri, 15 Mar 2024 13:35:24 +0300 [thread overview]
Message-ID: <ZfQkbGyHM4AjlFsm@moroto> (raw)
In-Reply-To: <fac976aa-6172-4846-9fcb-c7564b291ed8@rasmusvillemoes.dk>
[ 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 |
next parent reply other threads:[~2024-03-15 10:35 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <fac976aa-6172-4846-9fcb-c7564b291ed8@rasmusvillemoes.dk>
2024-03-15 10:35 ` Dan Carpenter [this message]
2024-03-15 10:46 ` smatch and locking checks Dan Carpenter
2024-03-15 10:56 ` Dan Carpenter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZfQkbGyHM4AjlFsm@moroto \
--to=dan.carpenter@linaro.org \
--cc=error27@gmail.com \
--cc=linux@rasmusvillemoes.dk \
--cc=smatch@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).