wow, I see that sorry! ________________________________ De: Alan C. Assis <acas...@gmail.com> Enviado: segunda-feira, 2 de junho de 2025 18:58 Para: dev@nuttx.apache.org <dev@nuttx.apache.org> Assunto: Re: Sv: RFC: fix race conditions in drivers/serial/serial.c
Hi Miguel, Which kind of modification are you looking for? RS-485 is already supported: https://embeddedrelated.com/showarticle/1715.php BR, Alan On Mon, Jun 2, 2025 at 3:52 PM MIGUEL ALEXANDRE WISINTAINER < tcpipc...@hotmail.com> wrote: > should be good change too this driver to enable the RS-485 > ________________________________ > De: kr....@kerogit.eu <kr....@kerogit.eu> > Enviado: segunda-feira, 2 de junho de 2025 16:58 > Para: dev@nuttx.apache.org <dev@nuttx.apache.org> > Assunto: Re: Sv: RFC: fix race conditions in drivers/serial/serial.c > > Hello, > > I noticed a review from Allan C. Assis about a spelling error. Updated > patches are attached and also available in repository in uart_fixes_rfc2 > branch. (Sorry about the extra work, I was even trying to check the word > using web search... but I didn't spot that the search autocorrected it > so my conclusion was that the spellchecker I also used is just tripping > on the plural.) > > Best regards > > On 2025-06-02 09:39, alin.jerpe...@sony.com wrote: > > Hi KR, > > > > thank for submitting the patches > > > > They have been uploaded to mainline and are under review > > https://github.com/apache/nuttx/pull/16466 > > > > > > Best regards > > Alin > > ________________________________ > > Från: kr....@kerogit.eu <kr....@kerogit.eu> > > Skickat: den 1 juni 2025 22:42 > > Till: dev@nuttx.apache.org <dev@nuttx.apache.org> > > Ämne: RFC: fix race conditions in drivers/serial/serial.c > > > > While going through the code in drivers/serial/serial. c, I noticed > > this comment: The head and tail pointers are 16-bit values. The only > > time that the following could be unsafe is if the CPU made two > > non-atomic 8-bit accesses to obtain the 16-bit > > > > > > While going through the code in drivers/serial/serial.c, I noticed this > > comment: > > > > The head and tail pointers are 16-bit values. The only time that > > the following could be unsafe is if the CPU made two non-atomic > > 8-bit accesses to obtain the 16-bit head index. > > > > This is what happens for (at least) AVR architecture. These are 8bit > > microcontrollers and as such, they will fetch the 16-bit value in two > > 8-bit load instructions; interrupt routine may execute between those > > and > > change the value being read. This will result in corrupted read (one > > byte of the value will be from pre-interrupt state and the other from > > post-interrupt state.) > > > > This patch introduces CONFIG_ARCH_LD_16BIT_NOT_ATOMIC configuration > > option, which is automatically selected for architectures known to be > > unable to load 16bit values in one instruction. (It is currently only > > set for AVR. I presume it might be also useful for Z80 but I do not > > have > > any experience with that architecture so I did no changes there.) When > > this configuration option is set, reads of recv.head and xmit.tail are > > enclosed with enter_critical_section and leave_critical_section calls > > to > > prevent interrupt handlers from running, if needed. Not all reads need > > to be protected this way - some are already in existing critical > > sections and some happen with the UART-specific interrupt disabled. > > > > If the configuration option is not set, the code simply loads the value > > into a local variable. Subsequent direct uses of the unprotected > > volatile variable are replaced with the local variables for both cases. > > > > There is also a related change that only applies when > > CONFIG_SERIAL_IFLOWCONTROL_WATERMARKS is set. Aside from the protection > > selected by CONFIG_ARCH_LD_16BIT_NOT_ATOMIC, this patch also fixes > > calculation of the nbuffered value. This calculation is not running > > with > > interrupts disabled and value of rxbuf->head can change between the > > condition and actual computation. Even if the load itself is atomic, > > this leads to TOCTOU error. > > > > Impact: this change should not impact architectures that do not benefit > > from this change at all unless CONFIG_SERIAL_IFLOWCONTROL is set. If > > CONFIG_SERIAL_IFLOWCONTROL is set, the only change that remains in > > effect is the fix of the TOCTOU error. > > > > Testing: Patch was tested with rv-virt:nsh - disassembly of functions > > from drivers/serial/serial.c was compared and did not change after the > > patch was applied - with one exception. The exception was an address of > > a global variable, I assume it was caused by change of g_version length > > (which notes that the tree is dirty) and is therefore inconsequential. > > CONFIG_SERIAL_IFLOWCONTROL was not set in this test. > > > > Configuration with CONFIG_SERIAL_IFLOWCONTROL set was tested by > > building > > it for AVR DA/DB. Configuration with CONFIG_SERIAL_IFLOWCONTROL unset > > was tested by custom echo application running on AVR128DA28 for a few > > hours. > > > > I also ran CI test: 1044 passed, 10 skipped, 14 deselected, 3 warnings > > in 2463.23s (0:41:03) > > > > > > On a related note, it seems to me that there may be a bug in echo > > handling in uart_readv. If dev->tc_lflag & ECHO is true, then > > uart_putxmitchar is called. This function adds the received character > > to > > transmit buffer and increments xmit.head value. However, > > uart_putxmitchar is also called from uart_writev; this function states > > "Only one user can access dev->xmit.head at a time" and takes the > > dev->xmit.lock. As far as I can see, uart_readv does not take the lock > > and may interfere with uart_writev. Evaluating and solving this > > properly > > is currently above my skillset though. (If this is even considered > > serious enough to warrant a fix.) > > > > > > The patch series also fixes a typo in drivers/serial/serial.c > > > > > > Both patches are attached to this message and also available in a git > > repository nuttx.git at git.kerogit.eu accessible through HTTP/S. > > (Trying to prevent bot traffic by not posting the URL in > > machine-readable form.) The relevant branch is called uart_fixes_rfc1. > > If the patches are acceptable, I would like to ask someone with GitHub > > account to open a PR (I don't have a GH account.) Any comments or > > suggestions are welcome. >
