| Message ID | 20230421140725.495-1-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | thunderbolt: Clear registers properly when auto clear isn't in use | expand |
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 <tiwai@suse.de> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217343 > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > Tested-by: Takashi Iwai <tiwai@suse.de> > --- > 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 <tiwai@suse.de> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217343 >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> Tested-by: Takashi Iwai <tiwai@suse.de> >> --- >> 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
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
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 <tiwai@suse.de> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217343 Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> Tested-by: Takashi Iwai <tiwai@suse.de> --- 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(-)