diff mbox series

thunderbolt: Clear registers properly when auto clear isn't in use

Message ID 20230421140725.495-1-mario.limonciello@amd.com
State New
Headers
Series thunderbolt: Clear registers properly when auto clear isn't in use |

Commit Message

Mario Limonciello April 21, 2023, 2:07 p.m. UTC 
  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(-)

Comments

Mika Westerberg April 24, 2023, 5:58 a.m. UTC | #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 <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
Mario Limonciello April 24, 2023, 1:30 p.m. UTC | #2 
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 mbox series

Patch

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