Hi folks,
These patches address sleeping in atomic context. It wasn't obvious at
first this was taking place since the callback sleeps while the caller (a
different driver) called it in a threaded IRQ handler.
since v1:
- Rework patch 1. Moving wake_up() to the threaded handler is enough as
sleeping is allowed in the threaded handler.
Sakari Ailus (3):
mei: vsc: Call wake_up() in the threaded IRQ handler
mei: vsc: Don't use sleeping condition in wait_event_timeout()
mei: vsc: Assign pinfo fields in variable declaration
drivers/misc/mei/vsc-tp.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
--
2.39.2
The hard IRQ handler vsc_tp_irq() is called with a raw spinlock taken.
wake_up() acquires a spinlock, a sleeping lock on PREEMPT_RT. This leads
to sleeping in atomic context.
Move the wake_up() call to the threaded IRQ handler vsc_tp_thread_isr()
where it can be safely called.
Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Signed-off-by: Sakari Ailus <[email protected]>
---
drivers/misc/mei/vsc-tp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index 6f4a4be6ccb5..2b69ada9349e 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -416,8 +416,6 @@ static irqreturn_t vsc_tp_isr(int irq, void *data)
atomic_inc(&tp->assert_cnt);
- wake_up(&tp->xfer_wait);
-
return IRQ_WAKE_THREAD;
}
@@ -425,6 +423,8 @@ static irqreturn_t vsc_tp_thread_isr(int irq, void *data)
{
struct vsc_tp *tp = data;
+ wake_up(&tp->xfer_wait);
+
if (tp->event_notify)
tp->event_notify(tp->event_notify_context);
--
2.39.2
vsc_tp_wakeup_request() called wait_event_timeout() with
gpiod_get_value_cansleep() which may sleep, and does so as the
implementation is that of gpio-ljca.
Move the GPIO state check outside the call.
Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Signed-off-by: Sakari Ailus <[email protected]>
---
drivers/misc/mei/vsc-tp.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index 2b69ada9349e..7b678005652b 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -25,7 +25,8 @@
#define VSC_TP_ROM_BOOTUP_DELAY_MS 10
#define VSC_TP_ROM_XFER_POLL_TIMEOUT_US (500 * USEC_PER_MSEC)
#define VSC_TP_ROM_XFER_POLL_DELAY_US (20 * USEC_PER_MSEC)
-#define VSC_TP_WAIT_FW_ASSERTED_TIMEOUT (2 * HZ)
+#define VSC_TP_WAIT_FW_POLL_TIMEOUT (2 * HZ)
+#define VSC_TP_WAIT_FW_POLL_DELAY_US (20 * USEC_PER_MSEC)
#define VSC_TP_MAX_XFER_COUNT 5
#define VSC_TP_PACKET_SYNC 0x31
@@ -101,13 +102,15 @@ static int vsc_tp_wakeup_request(struct vsc_tp *tp)
gpiod_set_value_cansleep(tp->wakeupfw, 0);
ret = wait_event_timeout(tp->xfer_wait,
- atomic_read(&tp->assert_cnt) &&
- gpiod_get_value_cansleep(tp->wakeuphost),
- VSC_TP_WAIT_FW_ASSERTED_TIMEOUT);
+ atomic_read(&tp->assert_cnt),
+ VSC_TP_WAIT_FW_POLL_TIMEOUT);
if (!ret)
return -ETIMEDOUT;
- return 0;
+ return read_poll_timeout(gpiod_get_value_cansleep, ret, ret,
+ VSC_TP_WAIT_FW_POLL_DELAY_US,
+ VSC_TP_WAIT_FW_POLL_TIMEOUT, false,
+ tp->wakeuphost);
}
static void vsc_tp_wakeup_release(struct vsc_tp *tp)
--
2.39.2
Assign all possible fields of pinfo in variable declaration, instead of
just zeroing it there.
Signed-off-by: Sakari Ailus <[email protected]>
---
drivers/misc/mei/vsc-tp.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index 7b678005652b..9b4584d67a1b 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -445,11 +445,16 @@ static int vsc_tp_match_any(struct acpi_device *adev, void *data)
static int vsc_tp_probe(struct spi_device *spi)
{
- struct platform_device_info pinfo = { 0 };
+ struct vsc_tp *tp;
+ struct platform_device_info pinfo = {
+ .name = "intel_vsc",
+ .data = &tp,
+ .size_data = sizeof(tp),
+ .id = PLATFORM_DEVID_NONE,
+ };
struct device *dev = &spi->dev;
struct platform_device *pdev;
struct acpi_device *adev;
- struct vsc_tp *tp;
int ret;
tp = devm_kzalloc(dev, sizeof(*tp), GFP_KERNEL);
@@ -501,13 +506,8 @@ static int vsc_tp_probe(struct spi_device *spi)
ret = -ENODEV;
goto err_destroy_lock;
}
- pinfo.fwnode = acpi_fwnode_handle(adev);
-
- pinfo.name = "intel_vsc";
- pinfo.data = &tp;
- pinfo.size_data = sizeof(tp);
- pinfo.id = PLATFORM_DEVID_NONE;
+ pinfo.fwnode = acpi_fwnode_handle(adev);
pdev = platform_device_register_full(&pinfo);
if (IS_ERR(pdev)) {
ret = PTR_ERR(pdev);
--
2.39.2
> From: Sakari Ailus <[email protected]>
>
> The hard IRQ handler vsc_tp_irq() is called with a raw spinlock taken.
> wake_up() acquires a spinlock, a sleeping lock on PREEMPT_RT.
Does this mean we can't use wake_up() in isr?
BR,
Wentong
> This leads to sleeping in atomic context.
>
> Move the wake_up() call to the threaded IRQ handler vsc_tp_thread_isr()
> where it can be safely called.
>
> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> Signed-off-by: Sakari Ailus <[email protected]>
> ---
> drivers/misc/mei/vsc-tp.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c index
> 6f4a4be6ccb5..2b69ada9349e 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -416,8 +416,6 @@ static irqreturn_t vsc_tp_isr(int irq, void *data)
>
> atomic_inc(&tp->assert_cnt);
>
> - wake_up(&tp->xfer_wait);
> -
> return IRQ_WAKE_THREAD;
> }
>
> @@ -425,6 +423,8 @@ static irqreturn_t vsc_tp_thread_isr(int irq, void
> *data) {
> struct vsc_tp *tp = data;
>
> + wake_up(&tp->xfer_wait);
> +
> if (tp->event_notify)
> tp->event_notify(tp->event_notify_context);
>
> --
> 2.39.2
> From: Sakari Ailus <[email protected]>
>
> vsc_tp_wakeup_request() called wait_event_timeout() with
> gpiod_get_value_cansleep() which may sleep, and does so as the
> implementation is that of gpio-ljca.
>
> Move the GPIO state check outside the call.
>
> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> Signed-off-by: Sakari Ailus <[email protected]>
Tested-and-Reviewed-by: Wentong Wu <[email protected]>
> ---
> drivers/misc/mei/vsc-tp.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c index
> 2b69ada9349e..7b678005652b 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -25,7 +25,8 @@
> #define VSC_TP_ROM_BOOTUP_DELAY_MS 10
> #define VSC_TP_ROM_XFER_POLL_TIMEOUT_US (500 *
> USEC_PER_MSEC)
> #define VSC_TP_ROM_XFER_POLL_DELAY_US (20 *
> USEC_PER_MSEC)
> -#define VSC_TP_WAIT_FW_ASSERTED_TIMEOUT (2 * HZ)
> +#define VSC_TP_WAIT_FW_POLL_TIMEOUT (2 * HZ)
> +#define VSC_TP_WAIT_FW_POLL_DELAY_US (20 *
> USEC_PER_MSEC)
> #define VSC_TP_MAX_XFER_COUNT 5
>
> #define VSC_TP_PACKET_SYNC 0x31
> @@ -101,13 +102,15 @@ static int vsc_tp_wakeup_request(struct vsc_tp *tp)
> gpiod_set_value_cansleep(tp->wakeupfw, 0);
>
> ret = wait_event_timeout(tp->xfer_wait,
> - atomic_read(&tp->assert_cnt) &&
> - gpiod_get_value_cansleep(tp->wakeuphost),
> - VSC_TP_WAIT_FW_ASSERTED_TIMEOUT);
> + atomic_read(&tp->assert_cnt),
> + VSC_TP_WAIT_FW_POLL_TIMEOUT);
> if (!ret)
> return -ETIMEDOUT;
>
> - return 0;
> + return read_poll_timeout(gpiod_get_value_cansleep, ret, ret,
> + VSC_TP_WAIT_FW_POLL_DELAY_US,
> + VSC_TP_WAIT_FW_POLL_TIMEOUT, false,
> + tp->wakeuphost);
> }
>
> static void vsc_tp_wakeup_release(struct vsc_tp *tp)
> --
> 2.39.2
> From: Sakari Ailus <[email protected]>
>
> Assign all possible fields of pinfo in variable declaration, instead of just
> zeroing it there.
>
> Signed-off-by: Sakari Ailus <[email protected]>
Tested-and-Reviewed-by: Wentong Wu <[email protected]>
> ---
> drivers/misc/mei/vsc-tp.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c index
> 7b678005652b..9b4584d67a1b 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -445,11 +445,16 @@ static int vsc_tp_match_any(struct acpi_device
> *adev, void *data)
>
> static int vsc_tp_probe(struct spi_device *spi) {
> - struct platform_device_info pinfo = { 0 };
> + struct vsc_tp *tp;
> + struct platform_device_info pinfo = {
> + .name = "intel_vsc",
> + .data = &tp,
> + .size_data = sizeof(tp),
> + .id = PLATFORM_DEVID_NONE,
> + };
> struct device *dev = &spi->dev;
> struct platform_device *pdev;
> struct acpi_device *adev;
> - struct vsc_tp *tp;
> int ret;
>
> tp = devm_kzalloc(dev, sizeof(*tp), GFP_KERNEL); @@ -501,13 +506,8
> @@ static int vsc_tp_probe(struct spi_device *spi)
> ret = -ENODEV;
> goto err_destroy_lock;
> }
> - pinfo.fwnode = acpi_fwnode_handle(adev);
> -
> - pinfo.name = "intel_vsc";
> - pinfo.data = &tp;
> - pinfo.size_data = sizeof(tp);
> - pinfo.id = PLATFORM_DEVID_NONE;
>
> + pinfo.fwnode = acpi_fwnode_handle(adev);
> pdev = platform_device_register_full(&pinfo);
> if (IS_ERR(pdev)) {
> ret = PTR_ERR(pdev);
> --
> 2.39.2
Hi Wentong,
On Thu, Feb 22, 2024 at 03:26:18AM +0000, Wu, Wentong wrote:
> > From: Sakari Ailus <[email protected]>
> >
> > The hard IRQ handler vsc_tp_irq() is called with a raw spinlock taken.
> > wake_up() acquires a spinlock, a sleeping lock on PREEMPT_RT.
>
> Does this mean we can't use wake_up() in isr?
Good question. A lot of callers currently do.
In this case, handle_edge_irq() takes the raw spinlock and acquiring the
wake queue spinlock in wake_up() leads to sleeping IRQs disabled (see
below).
I don't think there's any harm in moving the wake_up() to the threaded
handler.
-------8<---------------------------
[ 54.216989] =============================
[ 54.218458] [ BUG: Invalid wait context ]
[ 54.219913] 6.8.0-rc2+ #1972 Not tainted
[ 54.221493] -----------------------------
[ 54.223165] swapper/4/0 is trying to lock:
[ 54.224756] ffff888112d04688 (&tp->xfer_wait){....}-{3:3}, at: __wake_up_common_lock+0x25/0x60
[ 54.226426] other info that might help us debug this:
[ 54.228189] context-{2:2}
[ 54.229817] no locks held by swapper/4/0.
[ 54.231453] stack backtrace:
[ 54.233078] CPU: 4 PID: 0 Comm: swapper/4 Not tainted 6.8.0-rc2+ #1972
[ 54.234720] Hardware name: Dell Inc. XPS 9315/0WWXF6, BIOS 1.3.0 08/16/2022
[ 54.236361] Call Trace:
[ 54.237983] <IRQ>
[ 54.239594] dump_stack_lvl+0x6a/0x9f
[ 54.241147] __lock_acquire+0x43e/0x11fb
[ 54.242704] ? mark_lock+0x96/0x34d
[ 54.244212] ? check_chain_key+0xe5/0x132
[ 54.245759] ? __wake_up_common_lock+0x25/0x60
[ 54.247312] lock_acquire+0x128/0x27c
[ 54.248848] ? __wake_up_common_lock+0x25/0x60
[ 54.250384] _raw_spin_lock_irqsave+0x3e/0x51
[ 54.251922] ? __wake_up_common_lock+0x25/0x60
[ 54.253524] __wake_up_common_lock+0x25/0x60
[ 54.255049] vsc_tp_isr+0x1e/0x28 [mei_vsc_hw]
[ 54.256569] __handle_irq_event_percpu+0xb4/0x1aa
[ 54.258054] handle_irq_event_percpu+0xf/0x32
[ 54.259550] handle_irq_event+0x34/0x53
[ 54.260982] handle_edge_irq+0xb0/0xcf
[ 54.262440] handle_irq_desc+0x39/0x43
[ 54.263898] intel_gpio_irq+0x105/0x15a
[ 54.265348] __handle_irq_event_percpu+0xb4/0x1aa
[ 54.266792] handle_irq_event_percpu+0xf/0x32
[ 54.268244] handle_irq_event+0x34/0x53
[ 54.269634] handle_fasteoi_irq+0xa5/0x131
[ 54.271018] __common_interrupt+0xdc/0xeb
[ 54.272392] common_interrupt+0x96/0xc1
[ 54.273754] </IRQ>
[ 54.275109] <TASK>
[ 54.276391] asm_common_interrupt+0x22/0x40
[ 54.277733] RIP: 0010:cpuidle_enter_state+0x171/0x253
[ 54.279071] Code: 8b 73 04 83 cf ff 49 89 c6 e8 62 fe df ff 31 ff e8 65 29 79 ff 45 84 ff 74 07 31 ff e8 0d c3 7f ff e8 fc 91 85 ff fb 45 85 e4 <0f> 88 ba 00 00 00 48 8b 04 24 49 63 cc 48 6b d1 68 49 29 c6 48 89
[ 54.280496] RSP: 0018:ffffc900001a7e88 EFLAGS: 00000202
[ 54.281935] RAX: 0000000000000004 RBX: ffffe8ffffa310c0 RCX: 000000000000001f
[ 54.283382] RDX: 0000000c9f7cf88f RSI: ffffffff82103e63 RDI: ffffffff8209b592
[ 54.284828] RBP: 0000000000000004 R08: 0000000000000000 R09: 0000000000000001
[ 54.286274] R10: 0000000001151268 R11: 0000000000000020 R12: 0000000000000004
[ 54.287699] R13: ffffffff8266fec0 R14: 0000000c9f7cf88f R15: 0000000000000000
[ 54.289127] ? cpuidle_enter_state+0x16d/0x253
[ 54.290564] cpuidle_enter+0x2a/0x38
[ 54.291987] do_idle+0x190/0x204
[ 54.293394] cpu_startup_entry+0x2a/0x2c
[ 54.294797] start_secondary+0x12d/0x12d
[ 54.296198] secondary_startup_64_no_verify+0x15e/0x16b
[ 54.297595] </TASK>
-------8<---------------------------
>
> BR,
> Wentong
>
> > This leads to sleeping in atomic context.
> >
> > Move the wake_up() call to the threaded IRQ handler vsc_tp_thread_isr()
> > where it can be safely called.
> >
> > Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> > Signed-off-by: Sakari Ailus <[email protected]>
> > ---
> > drivers/misc/mei/vsc-tp.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c index
> > 6f4a4be6ccb5..2b69ada9349e 100644
> > --- a/drivers/misc/mei/vsc-tp.c
> > +++ b/drivers/misc/mei/vsc-tp.c
> > @@ -416,8 +416,6 @@ static irqreturn_t vsc_tp_isr(int irq, void *data)
> >
> > atomic_inc(&tp->assert_cnt);
> >
> > - wake_up(&tp->xfer_wait);
> > -
> > return IRQ_WAKE_THREAD;
> > }
> >
> > @@ -425,6 +423,8 @@ static irqreturn_t vsc_tp_thread_isr(int irq, void
> > *data) {
> > struct vsc_tp *tp = data;
> >
> > + wake_up(&tp->xfer_wait);
> > +
> > if (tp->event_notify)
> > tp->event_notify(tp->event_notify_context);
> >
--
Regards,
Sakari Ailus
> -----Original Message-----
> From: Sakari Ailus <[email protected]>
>
> The hard IRQ handler vsc_tp_irq() is called with a raw spinlock taken.
> wake_up() acquires a spinlock, a sleeping lock on PREEMPT_RT. This leads to
> sleeping in atomic context.
>
> Move the wake_up() call to the threaded IRQ handler vsc_tp_thread_isr()
> where it can be safely called.
>
> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> Signed-off-by: Sakari Ailus <[email protected]>
Tested-and-Reviewed-by: Wentong Wu <[email protected]>
> ---
> drivers/misc/mei/vsc-tp.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c index
> 6f4a4be6ccb5..2b69ada9349e 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -416,8 +416,6 @@ static irqreturn_t vsc_tp_isr(int irq, void *data)
>
> atomic_inc(&tp->assert_cnt);
>
> - wake_up(&tp->xfer_wait);
> -
> return IRQ_WAKE_THREAD;
> }
>
> @@ -425,6 +423,8 @@ static irqreturn_t vsc_tp_thread_isr(int irq, void
> *data) {
> struct vsc_tp *tp = data;
>
> + wake_up(&tp->xfer_wait);
> +
> if (tp->event_notify)
> tp->event_notify(tp->event_notify_context);
>
> --
> 2.39.2
Hi Wentong,
On Wed, Feb 28, 2024 at 08:19:04AM +0000, Wu, Wentong wrote:
> > -----Original Message-----
> > From: Sakari Ailus <[email protected]>
> >
> > The hard IRQ handler vsc_tp_irq() is called with a raw spinlock taken.
> > wake_up() acquires a spinlock, a sleeping lock on PREEMPT_RT. This leads to
> > sleeping in atomic context.
> >
> > Move the wake_up() call to the threaded IRQ handler vsc_tp_thread_isr()
> > where it can be safely called.
> >
> > Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> > Signed-off-by: Sakari Ailus <[email protected]>
>
> Tested-and-Reviewed-by: Wentong Wu <[email protected]>
Thanks!
I dug a little bit deeper and it seems the lockdep warning this patch fixes
is something we can safely ignore, see
<URL:https://wiki.archlinux.org/title/Realtime_kernel_patchset#How_does_the_realtime_patch_work>.
My apologies for the noise.
The two other patches in the set are still unaffected by this.
--
Regards,
Sakari Ailus
On Thu, Feb 22, 2024, at 12:46, Sakari Ailus wrote:
> Hi Wentong,
>
> On Thu, Feb 22, 2024 at 03:26:18AM +0000, Wu, Wentong wrote:
>> > From: Sakari Ailus <[email protected]>
>> >
>> > The hard IRQ handler vsc_tp_irq() is called with a raw spinlock taken.
>> > wake_up() acquires a spinlock, a sleeping lock on PREEMPT_RT.
>>
>> Does this mean we can't use wake_up() in isr?
>
> Good question. A lot of callers currently do.
If driver has a traditional (non-threaded) handler, it should
always work fine: on non-PREEMPT_RT it can take the spinlock
and on PREEMPT_RT it automatically turns into a threaded
handler that can still call it.
> In this case, handle_edge_irq() takes the raw spinlock and acquiring the
> wake queue spinlock in wake_up() leads to sleeping IRQs disabled (see
> below).
>
> I don't think there's any harm in moving the wake_up() to the threaded
> handler.
It causes an extra bit of latency for the non-PREEMPT_RT case
because you now always have to go through two task switches.
This is probably fine if you don't care about latency.
You can probably replace the open-coded completion (wait queue
head plus atomic) with a normal 'struct completion' and
call complete() in the isr instead. This one seems to only
take a raw spinlock already.
Arnd
On Mon, Feb 19, 2024 at 09:58:07PM +0200, Sakari Ailus wrote:
> Assign all possible fields of pinfo in variable declaration, instead of
> just zeroing it there.
..
Side note: Can we actually make acpi_dev_has_children() public and use here
instead of open coding?
--
With Best Regards,
Andy Shevchenko
On Tue, Mar 5, 2024, at 18:11, Andy Shevchenko wrote:
> On Mon, Feb 19, 2024 at 09:58:06PM +0200, Sakari Ailus wrote:
>> vsc_tp_wakeup_request() called wait_event_timeout() with
>> gpiod_get_value_cansleep() which may sleep, and does so as the
>> implementation is that of gpio-ljca.
>>
>> Move the GPIO state check outside the call.
>
> ...
>
>> +#define VSC_TP_WAIT_FW_POLL_TIMEOUT (2 * HZ)
>> +#define VSC_TP_WAIT_FW_POLL_DELAY_US (20 * USEC_PER_MSEC)
>
> ...
>
>> ret = wait_event_timeout(tp->xfer_wait,
>> - atomic_read(&tp->assert_cnt) &&
>> - gpiod_get_value_cansleep(tp->wakeuphost),
>> - VSC_TP_WAIT_FW_ASSERTED_TIMEOUT);
>> + atomic_read(&tp->assert_cnt),
>> + VSC_TP_WAIT_FW_POLL_TIMEOUT);
>
> First of all, there is an API for such cases (wait_woken_up() IIRC).
I think wait_for_completion_timeout() would be the obvious
replacement if the wait_event is no longer waiting for the
gpio but only for the irq handler to complete.
Arnd
On Mon, Feb 19, 2024 at 09:58:06PM +0200, Sakari Ailus wrote:
> vsc_tp_wakeup_request() called wait_event_timeout() with
> gpiod_get_value_cansleep() which may sleep, and does so as the
> implementation is that of gpio-ljca.
>
> Move the GPIO state check outside the call.
..
> +#define VSC_TP_WAIT_FW_POLL_TIMEOUT (2 * HZ)
> +#define VSC_TP_WAIT_FW_POLL_DELAY_US (20 * USEC_PER_MSEC)
..
> ret = wait_event_timeout(tp->xfer_wait,
> - atomic_read(&tp->assert_cnt) &&
> - gpiod_get_value_cansleep(tp->wakeuphost),
> - VSC_TP_WAIT_FW_ASSERTED_TIMEOUT);
> + atomic_read(&tp->assert_cnt),
> + VSC_TP_WAIT_FW_POLL_TIMEOUT);
First of all, there is an API for such cases (wait_woken_up() IIRC).
> if (!ret)
> return -ETIMEDOUT;
..
> + return read_poll_timeout(gpiod_get_value_cansleep, ret, ret,
> + VSC_TP_WAIT_FW_POLL_DELAY_US,
> + VSC_TP_WAIT_FW_POLL_TIMEOUT, false,
Second, this is a bug as you are using jiffies as microseconds. How had it been tested?
> + tp->wakeuphost);
--
With Best Regards,
Andy Shevchenko