* [bug report] tty: st-asc: switch to using devm_gpiod_get()
@ 2021-08-13 12:41 Dan Carpenter
2021-08-15 0:15 ` Dmitry Torokhov
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2021-08-13 12:41 UTC (permalink / raw)
To: dmitry.torokhov; +Cc: linux-gpio
Hello Dmitry Torokhov,
The patch 8c44f9b566a3: "tty: st-asc: switch to using
devm_gpiod_get()" from Jan 4, 2020, leads to the following
Smatch static checker warning:
drivers/gpio/gpiolib.c:3066 gpiod_set_consumer_name()
warn: sleeping in atomic context
drivers/gpio/gpiolib.c
3062 int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name)
3063 {
3064 VALIDATE_DESC(desc);
3065 if (name) {
--> 3066 name = kstrdup_const(name, GFP_KERNEL);
asc_set_termios() <- disables preempt
-> gpiod_set_consumer_name()
3067 if (!name)
3068 return -ENOMEM;
3069 }
3070
3071 kfree_const(desc->label);
3072 desc_set_label(desc, name);
3073
3074 return 0;
3075 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] tty: st-asc: switch to using devm_gpiod_get()
2021-08-13 12:41 [bug report] tty: st-asc: switch to using devm_gpiod_get() Dan Carpenter
@ 2021-08-15 0:15 ` Dmitry Torokhov
0 siblings, 0 replies; 4+ messages in thread
From: Dmitry Torokhov @ 2021-08-15 0:15 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-gpio, Lee Jones, Greg Kroah-Hartman, Peter Griffin
Hi Dan,
On Fri, Aug 13, 2021 at 03:41:55PM +0300, Dan Carpenter wrote:
> Hello Dmitry Torokhov,
>
> The patch 8c44f9b566a3: "tty: st-asc: switch to using
> devm_gpiod_get()" from Jan 4, 2020, leads to the following
> Smatch static checker warning:
>
> drivers/gpio/gpiolib.c:3066 gpiod_set_consumer_name()
> warn: sleeping in atomic context
>
> drivers/gpio/gpiolib.c
> 3062 int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name)
> 3063 {
> 3064 VALIDATE_DESC(desc);
> 3065 if (name) {
> --> 3066 name = kstrdup_const(name, GFP_KERNEL);
>
> asc_set_termios() <- disables preempt
> -> gpiod_set_consumer_name()
>
> 3067 if (!name)
> 3068 return -ENOMEM;
> 3069 }
> 3070
> 3071 kfree_const(desc->label);
> 3072 desc_set_label(desc, name);
> 3073
> 3074 return 0;
> 3075 }
Thank you for the bug report, but that is not the offending commit, as
it simply swapped one devm_* call for another, and devres_alloc() that
is being used in almost all devm calls, including the ones in this
particular case, uses GFP_KERNEL allocation.
The problem originally comes from:
commit d7356256488c544b8fdc2c3a775ce069546d7933
Author: Lee Jones <lee.jones@linaro.org>
Date: Fri Feb 3 10:23:13 2017 +0000
serial: st-asc: (De)Register GPIOD and swap Pinctrl profiles
When hardware flow-control is disabled, manual toggling of the UART's
reset line (RTS) using userland applications (e.g. stty) is not
possible, since the ASC IP does not provide this functionality in the
same was as some other IPs do. Thus, we have to do this manually.
This patch ensures that when HW flow-control is disabled the RTS/CTS
lines are free to be registered via the GPIO API. It also ensures
any registered GPIO lines are unregistered when HW flow-control is
requested, allowing the IP to control them automatically.
Acked-by: Peter Griffin <peter.griffin@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
that added the problematic devm use under spinlock; I'll leave it to Lee
and Greg to sort this out.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 4+ messages in thread
* [bug report] tty: st-asc: switch to using devm_gpiod_get()
@ 2022-10-26 14:05 Dan Carpenter
2022-10-26 17:33 ` Dmitry Torokhov
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2022-10-26 14:05 UTC (permalink / raw)
To: dmitry.torokhov; +Cc: linux-gpio
Hello Dmitry Torokhov,
The patch 8c44f9b566a3: "tty: st-asc: switch to using
devm_gpiod_get()" from Jan 4, 2020, leads to the following Smatch
static checker warning:
drivers/gpio/gpiolib-devres.c:118 devm_gpiod_get_index()
warn: sleeping in atomic context
The call tree is:
asc_set_termios() <- disables preempt (spin_lock)
-> devm_gpiod_get()
-> devm_gpiod_get_index()
drivers/tty/serial/st-asc.c
502 static void asc_set_termios(struct uart_port *port, struct ktermios *termios,
503 const struct ktermios *old)
504 {
505 struct asc_port *ascport = to_asc_port(port);
506 struct gpio_desc *gpiod;
507 unsigned int baud;
508 u32 ctrl_val;
509 tcflag_t cflag;
510 unsigned long flags;
511
512 /* Update termios to reflect hardware capabilities */
513 termios->c_cflag &= ~(CMSPAR |
514 (ascport->hw_flow_control ? 0 : CRTSCTS));
515
516 port->uartclk = clk_get_rate(ascport->clk);
517
518 baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk/16);
519 cflag = termios->c_cflag;
520
521 spin_lock_irqsave(&port->lock, flags);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
disables preempt
522
523 /* read control register */
524 ctrl_val = asc_in(port, ASC_CTL);
525
526 /* stop serial port and reset value */
527 asc_out(port, ASC_CTL, (ctrl_val & ~ASC_CTL_RUN));
528 ctrl_val = ASC_CTL_RXENABLE | ASC_CTL_FIFOENABLE;
529
530 /* reset fifo rx & tx */
531 asc_out(port, ASC_TXRESET, 1);
532 asc_out(port, ASC_RXRESET, 1);
533
534 /* set character length */
535 if ((cflag & CSIZE) == CS7) {
536 ctrl_val |= ASC_CTL_MODE_7BIT_PAR;
537 cflag |= PARENB;
538 } else {
539 ctrl_val |= (cflag & PARENB) ? ASC_CTL_MODE_8BIT_PAR :
540 ASC_CTL_MODE_8BIT;
541 cflag &= ~CSIZE;
542 cflag |= CS8;
543 }
544 termios->c_cflag = cflag;
545
546 /* set stop bit */
547 ctrl_val |= (cflag & CSTOPB) ? ASC_CTL_STOP_2BIT : ASC_CTL_STOP_1BIT;
548
549 /* odd parity */
550 if (cflag & PARODD)
551 ctrl_val |= ASC_CTL_PARITYODD;
552
553 /* hardware flow control */
554 if ((cflag & CRTSCTS)) {
555 ctrl_val |= ASC_CTL_CTSENABLE;
556
557 /* If flow-control selected, stop handling RTS manually */
558 if (ascport->rts) {
559 devm_gpiod_put(port->dev, ascport->rts);
560 ascport->rts = NULL;
561
562 pinctrl_select_state(ascport->pinctrl,
563 ascport->states[DEFAULT]);
564 }
565 } else {
566 /* If flow-control disabled, it's safe to handle RTS manually */
567 if (!ascport->rts && ascport->states[NO_HW_FLOWCTRL]) {
568 pinctrl_select_state(ascport->pinctrl,
569 ascport->states[NO_HW_FLOWCTRL]);
570
571 gpiod = devm_gpiod_get(port->dev, "rts", GPIOD_OUT_LOW);
^^^^^^^^^^^^^^
Sleeps
572 if (!IS_ERR(gpiod)) {
573 gpiod_set_consumer_name(gpiod,
574 port->dev->of_node->name);
575 ascport->rts = gpiod;
576 }
577 }
578 }
579
580 if ((baud < 19200) && !ascport->force_m1) {
581 asc_out(port, ASC_BAUDRATE, (port->uartclk / (16 * baud)));
582 } else {
583 /*
584 * MODE 1: recommended for high bit rates (above 19.2K)
[ snip ]
drivers/gpio/gpiolib-devres.c
108 */
109 if (flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) {
110 struct devres *dres;
111
112 dres = devres_find(dev, devm_gpiod_release,
113 devm_gpiod_match, &desc);
114 if (dres)
115 return desc;
116 }
117
--> 118 dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *),
119 GFP_KERNEL);
^^^^^^^^^^
Sleeping here.
120 if (!dr) {
121 gpiod_put(desc);
122 return ERR_PTR(-ENOMEM);
123 }
124
125 *dr = desc;
126 devres_add(dev, dr);
127
128 return desc;
129 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] tty: st-asc: switch to using devm_gpiod_get()
2022-10-26 14:05 Dan Carpenter
@ 2022-10-26 17:33 ` Dmitry Torokhov
0 siblings, 0 replies; 4+ messages in thread
From: Dmitry Torokhov @ 2022-10-26 17:33 UTC (permalink / raw)
To: Dan Carpenter, Lee Jones, Greg Kroah-Hartman; +Cc: linux-gpio
Hi Dan,
On Wed, Oct 26, 2022 at 05:05:16PM +0300, Dan Carpenter wrote:
> Hello Dmitry Torokhov,
>
> The patch 8c44f9b566a3: "tty: st-asc: switch to using
> devm_gpiod_get()" from Jan 4, 2020, leads to the following Smatch
> static checker warning:
>
> drivers/gpio/gpiolib-devres.c:118 devm_gpiod_get_index()
> warn: sleeping in atomic context
You already reported this one:
https://lore.kernel.org/all/20210813124155.GA7367@kili/
As I mentioned back then my change simply swapped one devm invocation
for another, with the problem of sleeping in an atomic context being
already present. In fact the issue was introduced in:
commit d7356256488c544b8fdc2c3a775ce069546d7933
Author: Lee Jones <lee.jones@linaro.org>
Date: Fri Feb 3 10:23:13 2017 +0000
serial: st-asc: (De)Register GPIOD and swap Pinctrl profiles
When hardware flow-control is disabled, manual toggling of the UART's
reset line (RTS) using userland applications (e.g. stty) is not
possible, since the ASC IP does not provide this functionality in the
same was as some other IPs do. Thus, we have to do this manually.
This patch ensures that when HW flow-control is disabled the RTS/CTS
lines are free to be registered via the GPIO API. It also ensures
any registered GPIO lines are unregistered when HW flow-control is
requested, allowing the IP to control them automatically.
Acked-by: Peter Griffin <peter.griffin@linaro.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
and I mentioned that Lee and Greg should look into fixing it. In
general, doing gpio allocation, or pin control switching while holding a
lock and/or with interrupts disabled is problematic...
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-10-26 17:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-13 12:41 [bug report] tty: st-asc: switch to using devm_gpiod_get() Dan Carpenter
2021-08-15 0:15 ` Dmitry Torokhov
-- strict thread matches above, loose matches on Subject: below --
2022-10-26 14:05 Dan Carpenter
2022-10-26 17:33 ` Dmitry Torokhov
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 NNTP newsgroup(s).