When `QUIRK_AUTO_CLEAR_INT` isn't set, interrupt masking should be
cleared by writing to Interrupt Mask Clear (IMR) and interrupt
status should be cleared properly at shutdown/init.
This fixes an error where interrupts are left enabled during resume
from hibernation with `CONFIG_USB4=y`.
Fixes: 468c49f44759 ("thunderbolt: Disable interrupt auto clear for rings")
Reported-by: Takashi Iwai <[email protected]>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=217343
Signed-off-by: Mario Limonciello <[email protected]>
Tested-by: Takashi Iwai <[email protected]>
---
I tried to base this off thunderbolt.git/next (tag: thunderbolt-for-v6.4-rc1)
but the following 3 commits are missing from that branch but are in 6.3-rc7:
58cdfe6f58b3 thunderbolt: Rename shadowed variables bit to interrupt_bit and auto_clear_bit
468c49f44759 thunderbolt: Disable interrupt auto clear for rings
1716efdb0793 thunderbolt: Use const qualifier for `ring_interrupt_index`
I cherry picked them first as this patch builds on them.
---
drivers/thunderbolt/nhi.c | 28 +++++++++++++++++++++-------
drivers/thunderbolt/nhi_regs.h | 1 +
2 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index d76e923fbc6a..7c543a6a5711 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -61,8 +61,9 @@ static int ring_interrupt_index(const struct tb_ring *ring)
*/
static void ring_interrupt_active(struct tb_ring *ring, bool active)
{
- int reg = REG_RING_INTERRUPT_BASE +
- ring_interrupt_index(ring) / 32 * 4;
+ int index = ring_interrupt_index(ring) / 32 * 4;
+ int reg = REG_RING_INTERRUPT_BASE + index;
+ int clear = REG_RING_INTERRUPT_MASK_CLEAR_BASE + index;
int interrupt_bit = ring_interrupt_index(ring) & 31;
int mask = 1 << interrupt_bit;
u32 old, new;
@@ -123,7 +124,11 @@ static void ring_interrupt_active(struct tb_ring *ring, bool active)
"interrupt for %s %d is already %s\n",
RING_TYPE(ring), ring->hop,
active ? "enabled" : "disabled");
- iowrite32(new, ring->nhi->iobase + reg);
+
+ if (active)
+ iowrite32(new, ring->nhi->iobase + reg);
+ else
+ iowrite32(mask, ring->nhi->iobase + clear);
}
/*
@@ -135,12 +140,21 @@ static void nhi_disable_interrupts(struct tb_nhi *nhi)
{
int i = 0;
/* disable interrupts */
- for (i = 0; i < RING_INTERRUPT_REG_COUNT(nhi); i++)
- iowrite32(0, nhi->iobase + REG_RING_INTERRUPT_BASE + 4 * i);
+ for (i = 0; i < RING_INTERRUPT_REG_COUNT(nhi); i++) {
+ if (nhi->quirks & QUIRK_AUTO_CLEAR_INT)
+ iowrite32(0, nhi->iobase + REG_RING_INTERRUPT_BASE + 4 * i);
+ else
+ iowrite32(0xffffffff,
+ nhi->iobase + REG_RING_INTERRUPT_MASK_CLEAR_BASE + 4 * i);
+ }
/* clear interrupt status bits */
- for (i = 0; i < RING_NOTIFY_REG_COUNT(nhi); i++)
- ioread32(nhi->iobase + REG_RING_NOTIFY_BASE + 4 * i);
+ for (i = 0; i < RING_NOTIFY_REG_COUNT(nhi); i++) {
+ if (nhi->quirks & QUIRK_AUTO_CLEAR_INT)
+ ioread32(nhi->iobase + REG_RING_NOTIFY_BASE + 4 * i);
+ else
+ iowrite32(0xffffffff, nhi->iobase + REG_RING_INT_CLEAR + 4 * i);
+ }
}
/* ring helper methods */
diff --git a/drivers/thunderbolt/nhi_regs.h b/drivers/thunderbolt/nhi_regs.h
index faef165a919c..db95ad5d2814 100644
--- a/drivers/thunderbolt/nhi_regs.h
+++ b/drivers/thunderbolt/nhi_regs.h
@@ -92,6 +92,7 @@ struct ring_desc {
*/
#define REG_RING_INTERRUPT_BASE 0x38200
#define RING_INTERRUPT_REG_COUNT(nhi) ((31 + 2 * nhi->hop_count) / 32)
+#define REG_RING_INTERRUPT_MASK_CLEAR_BASE 0x38208
#define REG_INT_THROTTLING_RATE 0x38c00
--
2.34.1
Hi Mario,
On Fri, Apr 21, 2023 at 09:07:24AM -0500, Mario Limonciello wrote:
> When `QUIRK_AUTO_CLEAR_INT` isn't set, interrupt masking should be
> cleared by writing to Interrupt Mask Clear (IMR) and interrupt
> status should be cleared properly at shutdown/init.
>
> This fixes an error where interrupts are left enabled during resume
> from hibernation with `CONFIG_USB4=y`.
>
> Fixes: 468c49f44759 ("thunderbolt: Disable interrupt auto clear for rings")
> Reported-by: Takashi Iwai <[email protected]>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217343
> Signed-off-by: Mario Limonciello <[email protected]>
> Tested-by: Takashi Iwai <[email protected]>
> ---
> I tried to base this off thunderbolt.git/next (tag: thunderbolt-for-v6.4-rc1)
> but the following 3 commits are missing from that branch but are in 6.3-rc7:
>
> 58cdfe6f58b3 thunderbolt: Rename shadowed variables bit to interrupt_bit and auto_clear_bit
> 468c49f44759 thunderbolt: Disable interrupt auto clear for rings
> 1716efdb0793 thunderbolt: Use const qualifier for `ring_interrupt_index`
>
> I cherry picked them first as this patch builds on them.
Okay, so if I take this after v6.4-rc1 is released and send it forward
to for -rc2 Greg it should apply just fine?
> ---
> drivers/thunderbolt/nhi.c | 28 +++++++++++++++++++++-------
> drivers/thunderbolt/nhi_regs.h | 1 +
> 2 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index d76e923fbc6a..7c543a6a5711 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -61,8 +61,9 @@ static int ring_interrupt_index(const struct tb_ring *ring)
> */
> static void ring_interrupt_active(struct tb_ring *ring, bool active)
> {
> - int reg = REG_RING_INTERRUPT_BASE +
> - ring_interrupt_index(ring) / 32 * 4;
> + int index = ring_interrupt_index(ring) / 32 * 4;
> + int reg = REG_RING_INTERRUPT_BASE + index;
> + int clear = REG_RING_INTERRUPT_MASK_CLEAR_BASE + index;
> int interrupt_bit = ring_interrupt_index(ring) & 31;
> int mask = 1 << interrupt_bit;
> u32 old, new;
> @@ -123,7 +124,11 @@ static void ring_interrupt_active(struct tb_ring *ring, bool active)
> "interrupt for %s %d is already %s\n",
> RING_TYPE(ring), ring->hop,
> active ? "enabled" : "disabled");
> - iowrite32(new, ring->nhi->iobase + reg);
> +
> + if (active)
> + iowrite32(new, ring->nhi->iobase + reg);
> + else
> + iowrite32(mask, ring->nhi->iobase + clear);
Since it is doing this for all hardware, even for Intel, I will need to
run some testing to make sure this still works.
> }
>
> /*
> @@ -135,12 +140,21 @@ static void nhi_disable_interrupts(struct tb_nhi *nhi)
> {
> int i = 0;
> /* disable interrupts */
> - for (i = 0; i < RING_INTERRUPT_REG_COUNT(nhi); i++)
> - iowrite32(0, nhi->iobase + REG_RING_INTERRUPT_BASE + 4 * i);
> + for (i = 0; i < RING_INTERRUPT_REG_COUNT(nhi); i++) {
> + if (nhi->quirks & QUIRK_AUTO_CLEAR_INT)
> + iowrite32(0, nhi->iobase + REG_RING_INTERRUPT_BASE + 4 * i);
> + else
> + iowrite32(0xffffffff,
~0
> + nhi->iobase + REG_RING_INTERRUPT_MASK_CLEAR_BASE + 4 * i);
Btw, we have now quite many places with
if (nhi->quirks & QUIRK_AUTO_CLEAR_INT)
// Intel stuff
else
// non-Intel stuff
I wonder if we could move these behind a wrapper and then here (and
similar places) just call
nhi_mask_interrupt(nhi, ...)
> + }
>
> /* clear interrupt status bits */
> - for (i = 0; i < RING_NOTIFY_REG_COUNT(nhi); i++)
> - ioread32(nhi->iobase + REG_RING_NOTIFY_BASE + 4 * i);
> + for (i = 0; i < RING_NOTIFY_REG_COUNT(nhi); i++) {
> + if (nhi->quirks & QUIRK_AUTO_CLEAR_INT)
> + ioread32(nhi->iobase + REG_RING_NOTIFY_BASE + 4 * i);
> + else
> + iowrite32(0xffffffff, nhi->iobase + REG_RING_INT_CLEAR + 4 * i);
~0
nhi_clear_interrupt(nhi, ...)
> + }
> }
>
> /* ring helper methods */
> diff --git a/drivers/thunderbolt/nhi_regs.h b/drivers/thunderbolt/nhi_regs.h
> index faef165a919c..db95ad5d2814 100644
> --- a/drivers/thunderbolt/nhi_regs.h
> +++ b/drivers/thunderbolt/nhi_regs.h
> @@ -92,6 +92,7 @@ struct ring_desc {
> */
> #define REG_RING_INTERRUPT_BASE 0x38200
> #define RING_INTERRUPT_REG_COUNT(nhi) ((31 + 2 * nhi->hop_count) / 32)
Empty line here.
> +#define REG_RING_INTERRUPT_MASK_CLEAR_BASE 0x38208
>
> #define REG_INT_THROTTLING_RATE 0x38c00
>
> --
> 2.34.1
On 4/24/23 00:58, Mika Westerberg wrote:
> Hi Mario,
>
> On Fri, Apr 21, 2023 at 09:07:24AM -0500, Mario Limonciello wrote:
>> When `QUIRK_AUTO_CLEAR_INT` isn't set, interrupt masking should be
>> cleared by writing to Interrupt Mask Clear (IMR) and interrupt
>> status should be cleared properly at shutdown/init.
>>
>> This fixes an error where interrupts are left enabled during resume
>> from hibernation with `CONFIG_USB4=y`.
>>
>> Fixes: 468c49f44759 ("thunderbolt: Disable interrupt auto clear for rings")
>> Reported-by: Takashi Iwai <[email protected]>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217343
>> Signed-off-by: Mario Limonciello <[email protected]>
>> Tested-by: Takashi Iwai <[email protected]>
>> ---
>> I tried to base this off thunderbolt.git/next (tag: thunderbolt-for-v6.4-rc1)
>> but the following 3 commits are missing from that branch but are in 6.3-rc7:
>>
>> 58cdfe6f58b3 thunderbolt: Rename shadowed variables bit to interrupt_bit and auto_clear_bit
>> 468c49f44759 thunderbolt: Disable interrupt auto clear for rings
>> 1716efdb0793 thunderbolt: Use const qualifier for `ring_interrupt_index`
>>
>> I cherry picked them first as this patch builds on them.
> Okay, so if I take this after v6.4-rc1 is released and send it forward
> to for -rc2 Greg it should apply just fine?
Yes; that's right. I'll rework it for your below comments and that plan
sounds good to me.
>> ---
>> drivers/thunderbolt/nhi.c | 28 +++++++++++++++++++++-------
>> drivers/thunderbolt/nhi_regs.h | 1 +
>> 2 files changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
>> index d76e923fbc6a..7c543a6a5711 100644
>> --- a/drivers/thunderbolt/nhi.c
>> +++ b/drivers/thunderbolt/nhi.c
>> @@ -61,8 +61,9 @@ static int ring_interrupt_index(const struct tb_ring *ring)
>> */
>> static void ring_interrupt_active(struct tb_ring *ring, bool active)
>> {
>> - int reg = REG_RING_INTERRUPT_BASE +
>> - ring_interrupt_index(ring) / 32 * 4;
>> + int index = ring_interrupt_index(ring) / 32 * 4;
>> + int reg = REG_RING_INTERRUPT_BASE + index;
>> + int clear = REG_RING_INTERRUPT_MASK_CLEAR_BASE + index;
>> int interrupt_bit = ring_interrupt_index(ring) & 31;
>> int mask = 1 << interrupt_bit;
>> u32 old, new;
>> @@ -123,7 +124,11 @@ static void ring_interrupt_active(struct tb_ring *ring, bool active)
>> "interrupt for %s %d is already %s\n",
>> RING_TYPE(ring), ring->hop,
>> active ? "enabled" : "disabled");
>> - iowrite32(new, ring->nhi->iobase + reg);
>> +
>> + if (active)
>> + iowrite32(new, ring->nhi->iobase + reg);
>> + else
>> + iowrite32(mask, ring->nhi->iobase + clear);
> Since it is doing this for all hardware, even for Intel, I will need to
> run some testing to make sure this still works.
OK.
>> }
>>
>> /*
>> @@ -135,12 +140,21 @@ static void nhi_disable_interrupts(struct tb_nhi *nhi)
>> {
>> int i = 0;
>> /* disable interrupts */
>> - for (i = 0; i < RING_INTERRUPT_REG_COUNT(nhi); i++)
>> - iowrite32(0, nhi->iobase + REG_RING_INTERRUPT_BASE + 4 * i);
>> + for (i = 0; i < RING_INTERRUPT_REG_COUNT(nhi); i++) {
>> + if (nhi->quirks & QUIRK_AUTO_CLEAR_INT)
>> + iowrite32(0, nhi->iobase + REG_RING_INTERRUPT_BASE + 4 * i);
>> + else
>> + iowrite32(0xffffffff,
> ~0
>
>> + nhi->iobase + REG_RING_INTERRUPT_MASK_CLEAR_BASE + 4 * i);
> Btw, we have now quite many places with
>
> if (nhi->quirks & QUIRK_AUTO_CLEAR_INT)
> // Intel stuff
> else
> // non-Intel stuff
>
> I wonder if we could move these behind a wrapper and then here (and
> similar places) just call
>
> nhi_mask_interrupt(nhi, ...)
Sure, I'll experiment with this, it does sound like a generally good
improvement for the code.
>
>> + }
>>
>> /* clear interrupt status bits */
>> - for (i = 0; i < RING_NOTIFY_REG_COUNT(nhi); i++)
>> - ioread32(nhi->iobase + REG_RING_NOTIFY_BASE + 4 * i);
>> + for (i = 0; i < RING_NOTIFY_REG_COUNT(nhi); i++) {
>> + if (nhi->quirks & QUIRK_AUTO_CLEAR_INT)
>> + ioread32(nhi->iobase + REG_RING_NOTIFY_BASE + 4 * i);
>> + else
>> + iowrite32(0xffffffff, nhi->iobase + REG_RING_INT_CLEAR + 4 * i);
> ~0
>
> nhi_clear_interrupt(nhi, ...)
>
>> + }
>> }
>>
>> /* ring helper methods */
>> diff --git a/drivers/thunderbolt/nhi_regs.h b/drivers/thunderbolt/nhi_regs.h
>> index faef165a919c..db95ad5d2814 100644
>> --- a/drivers/thunderbolt/nhi_regs.h
>> +++ b/drivers/thunderbolt/nhi_regs.h
>> @@ -92,6 +92,7 @@ struct ring_desc {
>> */
>> #define REG_RING_INTERRUPT_BASE 0x38200
>> #define RING_INTERRUPT_REG_COUNT(nhi) ((31 + 2 * nhi->hop_count) / 32)
> Empty line here.
>
>> +#define REG_RING_INTERRUPT_MASK_CLEAR_BASE 0x38208
>>
>> #define REG_INT_THROTTLING_RATE 0x38c00
>>
>> --
>> 2.34.1