[ 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 |
