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.

Reply via email to