usb: chipidea: local_irq_save/restore added for hw_ep_prime

PATCH In review Jeaho Hwang 3 participants 9 messages Aug 17, 2021 – Aug 23, 2021

Subsystems (2)

USB Chipidea USB high speed dual role controller

lore

Share & tools

lore

Jeaho Hwang

author

2021-08-17 09:54 UTC #0 lore raw

PATCH v2 1/1 usb: chipidea: local_irq_save/restore added for hw_ep_prime

hw_ep_prime sometimes fails if irq occurs while it rus on RT kernel. local_irq_save/restore is added inside the function to gurantee atomicity. only effective for preempt_rt since hw_ep_prime is called inside top half or spin_lock_irqsave. No effect is expected for standard linux.

Signed-off-by Jeaho Hwang <[email protected]>

drivers/usb/chipidea/udc.c +12 -3

index 8834ca613721..a624eddb3e22 100644
@@ -191,22 +191,31 @@ static int hw_ep_get_halt(struct ci_hdrc *ci, int num, int dir)
 static int hw_ep_prime(struct ci_hdrc *ci, int num, int dir, int is_ctrl)
 {
 	int n = hw_ep_bit(num, dir);
+	unsigned long flags;
+	int ret = 0;
 
 	/* Synchronize before ep prime */
 	wmb();
 
-	if (is_ctrl && dir == RX && hw_read(ci, OP_ENDPTSETUPSTAT, BIT(num)))
+	/* irq affects this routine so irq should be disabled on RT.
+	 * on standard kernel, irq is already disabled by callers.
+	 */
+	local_irq_save(flags);
+	if (is_ctrl && dir == RX && hw_read(ci, OP_ENDPTSETUPSTAT, BIT(num))) {
+		local_irq_restore(flags);
 		return -EAGAIN;
+	}
 
 	hw_write(ci, OP_ENDPTPRIME, ~0, BIT(n));
 
 	while (hw_read(ci, OP_ENDPTPRIME, BIT(n)))
 		cpu_relax();
 	if (is_ctrl && dir == RX && hw_read(ci, OP_ENDPTSETUPSTAT, BIT(num)))
-		return -EAGAIN;
+		ret = -EAGAIN;
 
+	local_irq_restore(flags);
 	/* status shoult be tested according with manual but it doesn't work */
-	return 0;
+	return ret;
 }
 
 /**

Signature · 1 line

2.25.1

Sebastian Andrzej Siewior

2021-08-18 16:17 UTC #1 lore raw

Quoted 1 line · Jeaho Hwang

hw_ep_prime sometimes fails if irq occurs while it rus on RT kernel.

How/ why does it fail? Which IRQ occurs? Does it also occur without RT and with threadirqs enabled?

Quoted 3 lines

local_irq_save/restore is added inside the function to gurantee atomicity. only effective for preempt_rt since hw_ep_prime is called inside top half or spin_lock_irqsave. No effect is expected for standard linux.

How is that helping?
#1
udc_irq() -> isr_tr_complete_handler() -> isr_tr_complete_low ->
_hardware_dequeue() -> reprime_dtd() -> hw_ep_prime()

udc_irq() acquires ci->lock.

#2
ep_queue -> _ep_queue() ->_hardware_enqueue() -> hw_ep_prime()

ep_queue acquires hwep->lock. Which is actually ci->lock.

So if I read this right then hw_ep_prime() may not be interrupted in the middle of its operation (but preempted) because each path is protected by the lock.

isr_tr_complete_low() drops hwep->lock and acquires it again so it that phase another thread may acquire it.

Quoted 9 lines

Signed-off-by Jeaho Hwang <[email protected]>

drivers/usb/chipidea/udc.c +0 -0

index 8834ca613721..a624eddb3e22 100644
@@ -191,22 +191,31 @@ static int hw_ep_get_halt(struct ci_hdrc *ci, int num, int dir)
 static int hw_ep_prime(struct ci_hdrc *ci, int num, int dir, int is_ctrl)
 {

drivers/usb/chipidea/udc.c static int hw_ep_get_halt(struct ci_hdrc *ci, int num, int dir) · line 201

Quoted 10 lines

drivers/usb/chipidea/udc.c +4 -1

 	int n = hw_ep_bit(num, dir);
+	unsigned long flags;
+	int ret = 0;
 
 	/* Synchronize before ep prime */
 	wmb();
 
-	if (is_ctrl && dir == RX && hw_read(ci, OP_ENDPTSETUPSTAT, BIT(num)))
+	/* irq affects this routine so irq should be disabled on RT.
+	 * on standard kernel, irq is already disabled by callers.

The important part is _how_ it is affected. If locking works then nothing should read/ write the HW register. If the lock is briefly dropped then another thread _may_ read/ write the registers but not within this function.

If this function here is sensitive to timing (say the cpu_relax() loop gets interrupt for 1ms) then it has to be documented as such.

Quoted 12 lines

drivers/usb/chipidea/udc.c +5 -0

+	 */
+	local_irq_save(flags);
+	if (is_ctrl && dir == RX && hw_read(ci, OP_ENDPTSETUPSTAT, BIT(num))) {
+		local_irq_restore(flags);
 		return -EAGAIN;
+	}
 
 	hw_write(ci, OP_ENDPTPRIME, ~0, BIT(n));
 
 	while (hw_read(ci, OP_ENDPTPRIME, BIT(n)))
 		cpu_relax();
 	if (is_ctrl && dir == RX && hw_read(ci, OP_ENDPTSETUPSTAT, BIT(num)))

drivers/usb/chipidea/udc.c static int hw_ep_get_halt(struct ci_hdrc *ci, int num, int dir) · line 221

Quoted 10 lines

drivers/usb/chipidea/udc.c +3 -2

-		return -EAGAIN;
+		ret = -EAGAIN;
 
+	local_irq_restore(flags);
 	/* status shoult be tested according with manual but it doesn't work */
-	return 0;
+	return ret;
 }
 
 /**

Sebastian

Jeaho Hwang

author

2021-08-18 23:50 UTC #2 lore raw

2021년 8월 19일 (목) 오전 1:17, Sebastian Andrzej Siewior <[email protected]>님이 작성:

Quoted 7 lines · Jeaho Hwang

Quoted 1 line
hw_ep_prime sometimes fails if irq occurs while it rus on RT kernel.
How/ why does it fail? Which IRQ occurs? Does it also occur without RT and with threadirqs enabled?

We experienced priming failure while trying cable connection to a Windows RNDIS host.
And we found that a twd interrupt occurs during execution of hw_ep_prime for failure cases.
According to the manual, checking ENDPTSETUPSTAT before/after priming should be done immediately since the host could send a setup packet which makes the priming invalidated. So we think that the twd (or any) interrupt could make a timing issue between udc_irq and the RNDIS host.
Without RT, udc_irq runs as a forced threaded irq handler, so it runs without any interruption or preemption. NO similar case is found on non-RT.

Quoted 48 lines

Quoted 3 lines
local_irq_save/restore is added inside the function to gurantee atomicity. only effective for preempt_rt since hw_ep_prime is called inside top half or spin_lock_irqsave. No effect is expected for standard linux.
How is that helping?
#1
udc_irq() -> isr_tr_complete_handler() -> isr_tr_complete_low ->
_hardware_dequeue() -> reprime_dtd() -> hw_ep_prime()
udc_irq() acquires ci->lock.
#2
ep_queue -> _ep_queue() ->_hardware_enqueue() -> hw_ep_prime()
ep_queue acquires hwep->lock. Which is actually ci->lock.
So if I read this right then hw_ep_prime() may not be interrupted in the middle of its operation (but preempted) because each path is protected by the lock.
isr_tr_complete_low() drops hwep->lock and acquires it again so it that phase another thread may acquire it.
Quoted 9 lines
Signed-off-by Jeaho Hwang <[email protected]>
drivers/usb/chipidea/udc.c +0 -0
index 8834ca613721..a624eddb3e22 100644
@@ -191,22 +191,31 @@ static int hw_ep_get_halt(struct ci_hdrc *ci, int num, int dir)
 static int hw_ep_prime(struct ci_hdrc *ci, int num, int dir, int is_ctrl)
 {
drivers/usb/chipidea/udc.c static int hw_ep_get_halt(struct ci_hdrc *ci, int num, int dir) · line 201
Quoted 10 lines
drivers/usb/chipidea/udc.c +4 -1
      int n = hw_ep_bit(num, dir);
+     unsigned long flags;
+     int ret = 0;
 
      /* Synchronize before ep prime */
      wmb();
 
-     if (is_ctrl && dir == RX && hw_read(ci, OP_ENDPTSETUPSTAT, BIT(num)))
+     /* irq affects this routine so irq should be disabled on RT.
+      * on standard kernel, irq is already disabled by callers.
The important part is _how_ it is affected. If locking works then nothing should read/ write the HW register. If the lock is briefly dropped then another thread _may_ read/ write the registers but not within this function.

Quoted 2 lines

If this function here is sensitive to timing (say the cpu_relax() loop gets interrupt for 1ms) then it has to be documented as such.

The controller sets ENDPTSETUPSTAT register if the host sent a setup packet. yes it is a timing problem. I will document that and resubmit again if you agree that local_irq_save could help from the timing problem.

Thanks for the advice.

Quoted 25 lines

Quoted 13 lines

drivers/usb/chipidea/udc.c +5 -0

+      */
+     local_irq_save(flags);
+     if (is_ctrl && dir == RX && hw_read(ci, OP_ENDPTSETUPSTAT, BIT(num))) {
+             local_irq_restore(flags);
              return -EAGAIN;
+     }
 
      hw_write(ci, OP_ENDPTPRIME, ~0, BIT(n));
 
      while (hw_read(ci, OP_ENDPTPRIME, BIT(n)))
              cpu_relax();
      if (is_ctrl && dir == RX && hw_read(ci, OP_ENDPTSETUPSTAT, BIT(num)))

drivers/usb/chipidea/udc.c static int hw_ep_get_halt(struct ci_hdrc *ci, int num, int dir) · line 221

Quoted 10 lines

drivers/usb/chipidea/udc.c +3 -2

-             return -EAGAIN;
+             ret = -EAGAIN;
 
+     local_irq_restore(flags);
      /* status shoult be tested according with manual but it doesn't work */
-     return 0;
+     return ret;
 }
 
 /**

Sebastian

Signature · 2 lines

황재호, Jay Hwang, linux team manager of RTst
010-7242-1593

Sebastian Andrzej Siewior

2021-08-19 08:48 UTC #3 lore raw

Quoted 3 lines · Jeaho Hwang

Without RT, udc_irq runs as a forced threaded irq handler, so it runs without any interruption or preemption. NO similar case is found on non-RT.

I see only a devm_request_irq() so no force-threading here. Booting with threadirqs would not lead to the problem since commit
81e2073c175b8 ("genirq: Disable interrupts for force threaded handlers")

…

Quoted 8 lines

Quoted 2 lines
If this function here is sensitive to timing (say the cpu_relax() loop gets interrupt for 1ms) then it has to be documented as such.
The controller sets ENDPTSETUPSTAT register if the host sent a setup packet. yes it is a timing problem. I will document that and resubmit again if you agree that local_irq_save could help from the timing problem.
Thanks for the advice.

If it is really a timing issue in the function as you describe below then disabling interrupts would help and it is indeed an RT only issue.

So you read OP_ENDPTSETUPSTAT, it is 0, all good.
You write OP_ENDPTPRIME, wait for it to be cleared. Then you read OP_ENDPTSETUPSTAT again and if it is 0, all good.

And the TWD interrupt could delay say the second read would read 1 and it is invalidated. Which looks odd.
However, it is "okay" if the TWD interrupt happens after the second read? Even if the host sends a setup packet, nothing breaks? Do you have numbers on how long irq-off section is here? It seems to depend on how long the HW needs to clear the OP_ENDPTPRIME bits.

Sebastian

Jeaho Hwang

author

2021-08-20 05:16 UTC #4 lore raw

2021년 8월 19일 (목) 오후 5:48, Sebastian Andrzej Siewior <[email protected]>님이 작성:

Quoted 10 lines · Jeaho Hwang

Quoted 3 lines
Without RT, udc_irq runs as a forced threaded irq handler, so it runs without any interruption or preemption. NO similar case is found on non-RT.
I see only a devm_request_irq() so no force-threading here. Booting with threadirqs would not lead to the problem since commit
81e2073c175b8 ("genirq: Disable interrupts for force threaded handlers")

I was wrong. udc threaded irq handler allows twd interrupt even on non-RT and with threaded irq.
I believed Chen's comment "The function hw_ep_prime is only called at udc_irq which is registered as top-half irq handlers. Why the timer interrupt is occurred when hw_ep_prime is executing?".
We have additional experiments and got the results like below. RNDIS host was Windows.

RT, 1ms delay between first ENDPTSETUPSTAT read and priming : error case occurred
RT, 1ms delay + irq_save : no error case occurred. non-RT, threaded irq, 1ms delay : no error case occurred even twd fires inside the function execution.

It doesn't seem to be a timing issue. But irq definitely affects priming on the RT kernel. Do you RT experts have any idea about the causes?
If isr_tr_complete_handler fails ep priming it calls _ep_set_halt and goes an infinite loop in hw_ep_set_halt. It was an actual problem we experienced.
So we protect irqs inside hw_ep_priming not to make error cases and also add a timeout inside the hw_ep_set_halt loop for a walkaround. The timeout patch is submitted to linux-usb.
( https://marc.info/?l=linux-usb&m=162918269024007&w=2 )

We withdrew this patch since we don't know if disabling irq is the best solution to solve the problem and udc would work fine with hw_ep_set_halt walkaround even though hw_ep_prime fails. But we are still trying to find out the cause of this symptom so We'd so appreciate it if RT or USB experts share some ideas or ways to report somewhere. Xilinx doesn't provide any support without their official kernel :(

Thanks for the discussion Sebastian.

Jeaho Hwang.

Quoted 25 lines

…
Quoted 8 lines
Quoted 2 lines
If this function here is sensitive to timing (say the cpu_relax() loop gets interrupt for 1ms) then it has to be documented as such.
The controller sets ENDPTSETUPSTAT register if the host sent a setup packet. yes it is a timing problem. I will document that and resubmit again if you agree that local_irq_save could help from the timing problem.
Thanks for the advice.
If it is really a timing issue in the function as you describe below then disabling interrupts would help and it is indeed an RT only issue.
So you read OP_ENDPTSETUPSTAT, it is 0, all good.
You write OP_ENDPTPRIME, wait for it to be cleared. Then you read OP_ENDPTSETUPSTAT again and if it is 0, all good.
And the TWD interrupt could delay say the second read would read 1 and it is invalidated. Which looks odd.
However, it is "okay" if the TWD interrupt happens after the second read? Even if the host sends a setup packet, nothing breaks? Do you have numbers on how long irq-off section is here? It seems to depend on how long the HW needs to clear the OP_ENDPTPRIME bits.
Sebastian

Signature · 2 lines

황재호, Jay Hwang, linux team manager of RTst
010-7242-1593

Peter Chen

maintainer

2021-08-21 05:05 UTC #5 lore raw

Quoted 13 lines · Jeaho Hwang

2021년 8월 19일 (목) 오후 5:48, Sebastian Andrzej Siewior <[email protected]>님이 작성:
Quoted 10 lines · Jeaho Hwang
Quoted 3 lines
Without RT, udc_irq runs as a forced threaded irq handler, so it runs without any interruption or preemption. NO similar case is found on non-RT.
I see only a devm_request_irq() so no force-threading here. Booting with threadirqs would not lead to the problem since commit
81e2073c175b8 ("genirq: Disable interrupts for force threaded handlers")

Quoted 5 lines

I was wrong. udc threaded irq handler allows twd interrupt even on non-RT and with threaded irq.
I believed Chen's comment "The function hw_ep_prime is only called at udc_irq which is registered as top-half irq handlers. Why the timer interrupt is occurred when hw_ep_prime is executing?".

Hi Jeaho,

How could you let udc irq as threaded irq? The chipidea interrupt is registered using devm_request_irq.

Quoted 8 lines

We have additional experiments and got the results like below. RNDIS host was Windows.
RT, 1ms delay between first ENDPTSETUPSTAT read and priming : error case occurred
RT, 1ms delay + irq_save : no error case occurred. non-RT, threaded irq, 1ms delay : no error case occurred even twd fires inside the function execution.

Again, how do you observe it?

Peter

Quoted 55 lines

It doesn't seem to be a timing issue. But irq definitely affects priming on the RT kernel. Do you RT experts have any idea about the causes?
If isr_tr_complete_handler fails ep priming it calls _ep_set_halt and goes an infinite loop in hw_ep_set_halt. It was an actual problem we experienced.
So we protect irqs inside hw_ep_priming not to make error cases and also add a timeout inside the hw_ep_set_halt loop for a walkaround. The timeout patch is submitted to linux-usb.
( https://marc.info/?l=linux-usb&m=162918269024007&w=2 )
We withdrew this patch since we don't know if disabling irq is the best solution to solve the problem and udc would work fine with hw_ep_set_halt walkaround even though hw_ep_prime fails. But we are still trying to find out the cause of this symptom so We'd so appreciate it if RT or USB experts share some ideas or ways to report somewhere. Xilinx doesn't provide any support without their official kernel :(
Thanks for the discussion Sebastian.
Jeaho Hwang.
Quoted 25 lines
…
Quoted 8 lines
Quoted 2 lines
If this function here is sensitive to timing (say the cpu_relax() loop gets interrupt for 1ms) then it has to be documented as such.
The controller sets ENDPTSETUPSTAT register if the host sent a setup packet. yes it is a timing problem. I will document that and resubmit again if you agree that local_irq_save could help from the timing problem.
Thanks for the advice.
If it is really a timing issue in the function as you describe below then disabling interrupts would help and it is indeed an RT only issue.
So you read OP_ENDPTSETUPSTAT, it is 0, all good.
You write OP_ENDPTPRIME, wait for it to be cleared. Then you read OP_ENDPTSETUPSTAT again and if it is 0, all good.
And the TWD interrupt could delay say the second read would read 1 and it is invalidated. Which looks odd.
However, it is "okay" if the TWD interrupt happens after the second read? Even if the host sends a setup packet, nothing breaks? Do you have numbers on how long irq-off section is here? It seems to depend on how long the HW needs to clear the OP_ENDPTPRIME bits.
Sebastian
Signature · 2 lines
황재호, Jay Hwang, linux team manager of RTst
010-7242-1593

Signature · 2 lines

Thanks,
Peter Chen

Jeaho Hwang

author

2021-08-21 07:04 UTC #6 lore raw

2021년 8월 21일 (토) 오후 2:05, Peter Chen <[email protected]>님이 작성:

Quoted 21 lines · Jeaho Hwang

Quoted 13 lines
2021년 8월 19일 (목) 오후 5:48, Sebastian Andrzej Siewior <[email protected]>님이 작성:
Quoted 10 lines · Jeaho Hwang
Quoted 3 lines
Without RT, udc_irq runs as a forced threaded irq handler, so it runs without any interruption or preemption. NO similar case is found on non-RT.
I see only a devm_request_irq() so no force-threading here. Booting with threadirqs would not lead to the problem since commit
81e2073c175b8 ("genirq: Disable interrupts for force threaded handlers")
Quoted 5 lines
I was wrong. udc threaded irq handler allows twd interrupt even on non-RT and with threaded irq.
I believed Chen's comment "The function hw_ep_prime is only called at udc_irq which is registered as top-half irq handlers. Why the timer interrupt is occurred when hw_ep_prime is executing?".

Quoted 5 lines

Hi Jeaho,
How could you let udc irq as threaded irq? The chipidea interrupt is registered using devm_request_irq.

HI Peter.

We configured the kernel as "low latency desktop" and added "threadirqs" inside the cmdline parameter.
Then udc irq handler runs as a thread and shows no suspicious working. I Hope It will help.

Thanks.

Quoted 74 lines

Quoted 8 lines
We have additional experiments and got the results like below. RNDIS host was Windows.
RT, 1ms delay between first ENDPTSETUPSTAT read and priming : error case occurred
RT, 1ms delay + irq_save : no error case occurred. non-RT, threaded irq, 1ms delay : no error case occurred even twd fires inside the function execution.
Again, how do you observe it?
Peter
Quoted 48 lines
It doesn't seem to be a timing issue. But irq definitely affects priming on the RT kernel. Do you RT experts have any idea about the causes?
If isr_tr_complete_handler fails ep priming it calls _ep_set_halt and goes an infinite loop in hw_ep_set_halt. It was an actual problem we experienced.
So we protect irqs inside hw_ep_priming not to make error cases and also add a timeout inside the hw_ep_set_halt loop for a walkaround. The timeout patch is submitted to linux-usb.
( https://marc.info/?l=linux-usb&m=162918269024007&w=2 )
We withdrew this patch since we don't know if disabling irq is the best solution to solve the problem and udc would work fine with hw_ep_set_halt walkaround even though hw_ep_prime fails. But we are still trying to find out the cause of this symptom so We'd so appreciate it if RT or USB experts share some ideas or ways to report somewhere. Xilinx doesn't provide any support without their official kernel :(
Thanks for the discussion Sebastian.
Jeaho Hwang.
Quoted 24 lines
…
Quoted 8 lines
Quoted 2 lines
If this function here is sensitive to timing (say the cpu_relax() loop gets interrupt for 1ms) then it has to be documented as such.
The controller sets ENDPTSETUPSTAT register if the host sent a setup packet. yes it is a timing problem. I will document that and resubmit again if you agree that local_irq_save could help from the timing problem.
Thanks for the advice.
If it is really a timing issue in the function as you describe below then disabling interrupts would help and it is indeed an RT only issue.
So you read OP_ENDPTSETUPSTAT, it is 0, all good.
You write OP_ENDPTPRIME, wait for it to be cleared. Then you read OP_ENDPTSETUPSTAT again and if it is 0, all good.
And the TWD interrupt could delay say the second read would read 1 and it is invalidated. Which looks odd.
However, it is "okay" if the TWD interrupt happens after the second read? Even if the host sends a setup packet, nothing breaks? Do you have numbers on how long irq-off section is here? It seems to depend on how long the HW needs to clear the OP_ENDPTPRIME bits.
Quoted 7 lines
Quoted 1 line
Sebastian
--
황재호, Jay Hwang, linux team manager of RTst
010-7242-1593
--
Thanks,
Peter Chen

Signature · 1 line

황재호, Jay Hwang, linux team manager of RTst

Sebastian Andrzej Siewior

2021-08-23 16:14 UTC #7 lore raw

Quoted 8 lines · Jeaho Hwang

2021년 8월 21일 (토) 오후 2:05, Peter Chen <[email protected]>님이 작성:
Quoted 1 line
HI Peter.
We configured the kernel as "low latency desktop" and added "threadirqs" inside the cmdline parameter.
Then udc irq handler runs as a thread and shows no suspicious working. I Hope It will help.

May I again point out that since commit
81e2073c175b8 ("genirq: Disable interrupts for force threaded handlers")

the threaded handler runs with disabled interrupts. This time more verbose. It is available since v5.12-rc4 but it has been also applied for stable.

Quoted 1 line

Thanks.

Sebastian

Sebastian Andrzej Siewior

2021-08-23 16:51 UTC #8 lore raw

In reply to Jeaho Hwang

Quoted 5 lines · Jeaho Hwang

So we protect irqs inside hw_ep_priming not to make error cases and also add a timeout inside the hw_ep_set_halt loop for a walkaround. The timeout patch is submitted to linux-usb.
( https://marc.info/?l=linux-usb&m=162918269024007&w=2 )

Quoted 7 lines

We withdrew this patch since we don't know if disabling irq is the best solution to solve the problem and udc would work fine with hw_ep_set_halt walkaround even though hw_ep_prime fails. But we are still trying to find out the cause of this symptom so We'd so appreciate it if RT or USB experts share some ideas or ways to report somewhere. Xilinx doesn't provide any support without their official kernel :(

The UDC driver sometimes drops the lock and acquires it again. On UP it would not matter but on SMP/RT the other may acquire lock and do something with the HW. One thing that you could check if there is any HW access in between which would affect the behaviour.

Quoted 3 lines

Thanks for the discussion Sebastian.
Jeaho Hwang.

Sebastian