Re: [PR] arch/arm/rp23xx: update serial code to recent smp fixes from cxd56 serial code [nuttx]
avgoor commented on PR #16331: URL: https://github.com/apache/nuttx/pull/16331#issuecomment-2865486273 @shtirlic I've tested #16349 in my setup and can confirm that it works. Thank you. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/rp23xx: update serial code to recent smp fixes from cxd56 serial code [nuttx]
shtirlic commented on PR #16331: URL: https://github.com/apache/nuttx/pull/16331#issuecomment-2865138141 @avgoor try this pull in draft https://github.com/apache/nuttx/pull/16349 , with this changes I got it working again on SMP, it almost reverts the prev pull in `up_txint` function -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/rp23xx: update serial code to recent smp fixes from cxd56 serial code [nuttx]
shtirlic commented on PR #16331: URL: https://github.com/apache/nuttx/pull/16331#issuecomment-2864939352 @avgoor the way it's done in https://github.com/apache/nuttx/blob/358469e5bbb5e950c8c0b563c761213ececd/drivers/serial/serial_io.c#L57-L64 suggests that there is a small chance between the calls to lock/unlock for race, I will try to find the solution for SMP config, In general from what I saw there is a problem in SMP for rp2350 not in serial code overall, I am still trying to find the cause, enabling debug, different optimization flags introduce latency in scheduling making it almost impossible for me to find the catch the underlying problem. Even stopping on one breakpoint makes whole scheduling behave not the same. Most of the time I see internal tasklists are been corrupted or for example one task recursively links on it's self, so this is for sure some scheduler bug but it is exposed and visible on rp2350 for some reasons. I have a branch where I remade all IRQ and SMP calls handling and it still has issues with tasks dead and freeze. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/rp23xx: update serial code to recent smp fixes from cxd56 serial code [nuttx]
avgoor commented on PR #16331: URL: https://github.com/apache/nuttx/pull/16331#issuecomment-286414 Not sure if it's my local problem, but when I compile the fresh master (this patch included) with no optimizations (-O0 globally) the nsh gets stuck like on first key press. By increasing optimization level I reduce the probability, which is weird. I use a Pico 2w board with `pimoroni-pico-2-plus:smp` configuration. NuttX is built from scratch - `NuttX 12.9.0 bc4a52f910 May 8 2025 22:01:42 arm raspberrypi-pico-2`. The only change from the default config is setting the optimization level to `0`. @shtirlic can you please take a look? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/rp23xx: update serial code to recent smp fixes from cxd56 serial code [nuttx]
acassis merged PR #16331: URL: https://github.com/apache/nuttx/pull/16331 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/rp23xx: update serial code to recent smp fixes from cxd56 serial code [nuttx]
xiaoxiang781216 commented on code in PR #16331: URL: https://github.com/apache/nuttx/pull/16331#discussion_r2079777454 ## arch/arm/src/rp23xx/rp23xx_serial.c: ## @@ -914,16 +914,21 @@ static void up_txint(struct uart_dev_s *dev, bool enable) * interrupts disabled (note this may recurse). */ +# ifdef CONFIG_SMP Review Comment: Ok, let's refine the lock in common code later -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/rp23xx: update serial code to recent smp fixes from cxd56 serial code [nuttx]
shtirlic commented on code in PR #16331: URL: https://github.com/apache/nuttx/pull/16331#discussion_r2076891939 ## arch/arm/src/rp23xx/rp23xx_serial.c: ## @@ -914,16 +914,21 @@ static void up_txint(struct uart_dev_s *dev, bool enable) * interrupts disabled (note this may recurse). */ +# ifdef CONFIG_SMP Review Comment: so in non SMP version without spinlock it will be `spin_lock_irqsave` -> `up_irq_save` and `spin_unlock_irqrestore` -> `up_irq_restore` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/rp23xx: update serial code to recent smp fixes from cxd56 serial code [nuttx]
shtirlic commented on code in PR #16331: URL: https://github.com/apache/nuttx/pull/16331#discussion_r2076901492 ## arch/arm/src/rp23xx/rp23xx_serial.c: ## @@ -914,16 +914,21 @@ static void up_txint(struct uart_dev_s *dev, bool enable) * interrupts disabled (note this may recurse). */ +# ifdef CONFIG_SMP Review Comment: this is the entry `uar_xmitchars` functions for the refrence https://github.com/apache/nuttx/blob/358469e5bbb5e950c8c0b563c761213ececd/drivers/serial/serial_io.c#L57-L64 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/rp23xx: update serial code to recent smp fixes from cxd56 serial code [nuttx]
shtirlic commented on code in PR #16331: URL: https://github.com/apache/nuttx/pull/16331#discussion_r2076889003 ## arch/arm/src/rp23xx/rp23xx_serial.c: ## @@ -914,16 +914,21 @@ static void up_txint(struct uart_dev_s *dev, bool enable) * interrupts disabled (note this may recurse). */ +# ifdef CONFIG_SMP Review Comment: Because it's only needed in SMP version to workaround `uart_xmitchars` in `serial_io.c`, for non SMP there is already whole section critical. We can make it more readable like this, what you think? ```c # ifdef CONFIG_SMP spin_unlock_irqrestore(&priv->lock, flags); uart_xmitchars(dev); flags = spin_lock_irqsave(&priv->lock); # else uart_xmitchars(dev); # endif ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/rp23xx: update serial code to recent smp fixes from cxd56 serial code [nuttx]
shtirlic commented on code in PR #16331: URL: https://github.com/apache/nuttx/pull/16331#discussion_r2076891939 ## arch/arm/src/rp23xx/rp23xx_serial.c: ## @@ -914,16 +914,21 @@ static void up_txint(struct uart_dev_s *dev, bool enable) * interrupts disabled (note this may recurse). */ +# ifdef CONFIG_SMP Review Comment: so in non SMP version without spinlock it will be `spin_lock_irqsave` -> `up_irq_save` and `spin_unlock_irqrestore` -> `spin_unlock_irqrestore` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/rp23xx: update serial code to recent smp fixes from cxd56 serial code [nuttx]
shtirlic commented on code in PR #16331: URL: https://github.com/apache/nuttx/pull/16331#discussion_r2076889003 ## arch/arm/src/rp23xx/rp23xx_serial.c: ## @@ -914,16 +914,21 @@ static void up_txint(struct uart_dev_s *dev, bool enable) * interrupts disabled (note this may recurse). */ +# ifdef CONFIG_SMP Review Comment: Because it's only needed in SMP version to workaround `uart_xmitchars` in `serial_io.c`, for non SMP there is already, so we can make it more readable like this, what you think? ```c # ifdef CONFIG_SMP spin_unlock_irqrestore(&priv->lock, flags); uart_xmitchars(dev); flags = spin_lock_irqsave(&priv->lock); # else uart_xmitchars(dev); # endif ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/rp23xx: update serial code to recent smp fixes from cxd56 serial code [nuttx]
xiaoxiang781216 commented on code in PR #16331: URL: https://github.com/apache/nuttx/pull/16331#discussion_r2076874606 ## arch/arm/src/rp23xx/rp23xx_serial.c: ## @@ -914,16 +914,21 @@ static void up_txint(struct uart_dev_s *dev, bool enable) * interrupts disabled (note this may recurse). */ +# ifdef CONFIG_SMP Review Comment: why not always unblock? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/rp23xx: update serial code to recent smp fixes from cxd56 serial code [nuttx]
shtirlic commented on code in PR #16331: URL: https://github.com/apache/nuttx/pull/16331#discussion_r2076785587 ## arch/arm/src/rp23xx/rp23xx_serial.c: ## @@ -914,16 +914,21 @@ static void up_txint(struct uart_dev_s *dev, bool enable) * interrupts disabled (note this may recurse). */ +# ifdef CONFIG_SMP Review Comment: I think it's because the whole section should be in critical section until the end of the function, in SMP config it's more complicated since `serial_io.c` already have `enter_critical section` in the `uart_xmitchars(dev)` code, so they added unlock before it and lock after -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/rp23xx: update serial code to recent smp fixes from cxd56 serial code [nuttx]
shtirlic commented on code in PR #16331: URL: https://github.com/apache/nuttx/pull/16331#discussion_r2076785587 ## arch/arm/src/rp23xx/rp23xx_serial.c: ## @@ -914,16 +914,21 @@ static void up_txint(struct uart_dev_s *dev, bool enable) * interrupts disabled (note this may recurse). */ +# ifdef CONFIG_SMP Review Comment: I think it's because the whole section should be in critical section until the end of the function, in SMP config it's more complicated since `serial_io.c` already have `enter_critical section` in the code, so they added unlock before it and lock after -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/rp23xx: update serial code to recent smp fixes from cxd56 serial code [nuttx]
xiaoxiang781216 commented on code in PR #16331: URL: https://github.com/apache/nuttx/pull/16331#discussion_r2076725850 ## arch/arm/src/rp23xx/rp23xx_serial.c: ## @@ -914,16 +914,21 @@ static void up_txint(struct uart_dev_s *dev, bool enable) * interrupts disabled (note this may recurse). */ +# ifdef CONFIG_SMP Review Comment: why add check here, but no at line 906 ## arch/arm/src/rp23xx/rp23xx_serial.c: ## @@ -914,16 +914,21 @@ static void up_txint(struct uart_dev_s *dev, bool enable) * interrupts disabled (note this may recurse). */ +# ifdef CONFIG_SMP + spin_unlock_irqrestore(&priv->lock, flags); +# endif uart_xmitchars(dev); +# ifdef CONFIG_SMP Review Comment: ditto -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
