[PATCH v3 0/3] MR5211: ntdll/unix: Use termios2 for serial when possible.
Termios2 is a modern Linux feature thats allows usage of any custom baudrate instead of hardcoded values from the 1970's. Also, this patch improves compatibility with the win32 api, because windows allow any custom baudrates by design. Example software for which this patch is needed: http://www.vi-soft.com.ua/index_e.htm This software for reading/writing/patch fullflash of Siemens mobile phones, where serial speed is important. V-Klay uses 1600000 baudrate and perfectly works on Linux with termios2 and Prolific PL2303. -- v3: ntdll/unix: Fix building with -Werror when HAVE_ASM_TERMBITS_H is not set https://gitlab.winehq.org/wine/wine/-/merge_requests/5211
From: Kirill Zhumarin <kirill.zhumarin(a)gmail.com> --- configure | 6 +++ dlls/ntdll/Makefile.in | 1 + dlls/ntdll/unix/serial.c | 14 +++++ dlls/ntdll/unix/serial_termios2.c | 87 +++++++++++++++++++++++++++++++ dlls/ntdll/unix/unix_private.h | 6 +++ include/config.h.in | 3 ++ 6 files changed, 117 insertions(+) create mode 100644 dlls/ntdll/unix/serial_termios2.c diff --git a/configure b/configure index 83a641ff542..bc8141dfe67 100755 --- a/configure +++ b/configure @@ -7989,6 +7989,12 @@ if test "x$ac_cv_header_linux_serial_h" = xyes then : printf "%s\n" "#define HAVE_LINUX_SERIAL_H 1" >>confdefs.h +fi +ac_fn_c_check_header_compile "$LINENO" "asm/termbits.h" "ac_cv_header_asm_termbits_h" "$ac_includes_default" +if test "x$ac_cv_header_asm_termbits_h" = xyes +then : + printf "%s\n" "#define HAVE_ASM_TERMBITS_H 1" >>confdefs.h + fi ac_fn_c_check_header_compile "$LINENO" "linux/types.h" "ac_cv_header_linux_types_h" "$ac_includes_default" if test "x$ac_cv_header_linux_types_h" = xyes diff --git a/dlls/ntdll/Makefile.in b/dlls/ntdll/Makefile.in index b519bcd655f..d6fa26ea1d3 100644 --- a/dlls/ntdll/Makefile.in +++ b/dlls/ntdll/Makefile.in @@ -55,6 +55,7 @@ SOURCES = \ unix/registry.c \ unix/security.c \ unix/serial.c \ + unix/serial_termios2.c \ unix/server.c \ unix/signal_arm.c \ unix/signal_arm64.c \ diff --git a/dlls/ntdll/unix/serial.c b/dlls/ntdll/unix/serial.c index 372103b4df8..b4601bfeb3c 100644 --- a/dlls/ntdll/unix/serial.c +++ b/dlls/ntdll/unix/serial.c @@ -122,6 +122,14 @@ static NTSTATUS get_baud_rate(int fd, SERIAL_BAUD_RATE* sbr) struct termios port; int speed; +#ifdef HAVE_ASM_TERMBITS_H + /* If termios2 is supported, first try to use it. Otherwise, use termios as a fallback. */ + if (serial_get_baud_rate_termios2(fd, &speed) == STATUS_SUCCESS) { + sbr->BaudRate = speed; + return STATUS_SUCCESS; + } +#endif + if (tcgetattr(fd, &port) == -1) { ERR("tcgetattr error '%s'\n", strerror(errno)); @@ -455,6 +463,12 @@ static NTSTATUS set_baud_rate(int fd, const SERIAL_BAUD_RATE* sbr) { struct termios port; +#ifdef HAVE_ASM_TERMBITS_H + /* If termios2 is supported, first try to use it. Otherwise, use termios as a fallback. */ + if (serial_set_baud_rate_termios2(fd, sbr->BaudRate) == STATUS_SUCCESS) + return STATUS_SUCCESS; +#endif + if (tcgetattr(fd, &port) == -1) { ERR("tcgetattr error '%s'\n", strerror(errno)); diff --git a/dlls/ntdll/unix/serial_termios2.c b/dlls/ntdll/unix/serial_termios2.c new file mode 100644 index 00000000000..84be6ac6b5f --- /dev/null +++ b/dlls/ntdll/unix/serial_termios2.c @@ -0,0 +1,87 @@ +/* + * Linux termios2 support + * + * Copyright 2024 Kirill Zhumarin + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#if 0 +#pragma makedep unix +#endif + +#include "config.h" + +#include <errno.h> +#include <sys/ioctl.h> +#ifdef HAVE_ASM_TERMBITS_H +#include <asm/termbits.h> +#endif + +#include "ntstatus.h" +#define WIN32_NO_STATUS +#include "windef.h" +#include "winternl.h" +#include "winioctl.h" +#include "ddk/ntddser.h" +#include "wine/server.h" +#include "unix_private.h" +#include "wine/debug.h" +#include "unix_private.h" + +WINE_DEFAULT_DEBUG_CHANNEL(comm); + +#ifdef HAVE_ASM_TERMBITS_H +NTSTATUS serial_set_baud_rate_termios2( int fd, int baudrate ) { + struct termios2 port; + + if (ioctl(fd, TCGETS2, &port) == -1) + { + ERR("ioctl TCGETS2 error '%s'\n", strerror(errno)); + return errno_to_status( errno ); + } + + port.c_cflag &= ~CBAUD; + port.c_cflag |= BOTHER; + port.c_ospeed = baudrate; + + port.c_cflag &= ~(CBAUD << IBSHIFT); + port.c_cflag |= BOTHER << IBSHIFT; + port.c_ispeed = baudrate; + + if (ioctl(fd, TCSETS2, &port) == -1) + { + ERR("ioctl TCSETS2 error '%s'\n", strerror(errno)); + return errno_to_status( errno ); + } + + return STATUS_SUCCESS; +} + +NTSTATUS serial_get_baud_rate_termios2( int fd, int* baudrate ) { + struct termios2 port; + + if (ioctl(fd, TCGETS2, &port) == -1) + { + ERR("ioctl TCGETS2 error '%s'\n", strerror(errno)); + return errno_to_status( errno ); + } + + /* TERMIOS2 supports separate input and output baudrate, but wine supports only the common baudrate. */ + *baudrate = port.c_ispeed; + + return STATUS_SUCCESS; +} +#endif diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index b249570d421..1fc8a65ef4e 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -308,6 +308,12 @@ extern NTSTATUS serial_DeviceIoControl( HANDLE device, HANDLE event, PIO_APC_ROU IO_STATUS_BLOCK *io, UINT code, void *in_buffer, UINT in_size, void *out_buffer, UINT out_size ); extern NTSTATUS serial_FlushBuffersFile( int fd ); + +#ifdef HAVE_ASM_TERMBITS_H +extern NTSTATUS serial_set_baud_rate_termios2( int fd, int baudrate ); +extern NTSTATUS serial_get_baud_rate_termios2( int fd, int *baudrate ); +#endif + extern NTSTATUS sock_ioctl( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, void *apc_user, IO_STATUS_BLOCK *io, UINT code, void *in_buffer, UINT in_size, void *out_buffer, UINT out_size ); extern NTSTATUS sock_read( HANDLE handle, int fd, HANDLE event, PIO_APC_ROUTINE apc, void *apc_user, diff --git a/include/config.h.in b/include/config.h.in index a910b2c85f1..68b8c932bf7 100644 --- a/include/config.h.in +++ b/include/config.h.in @@ -183,6 +183,9 @@ /* Define to 1 if you have the <linux/serial.h> header file. */ #undef HAVE_LINUX_SERIAL_H +/* Define to 1 if you have the <asm/termbits.h> header file. */ +#undef HAVE_ASM_TERMBITS_H + /* Define to 1 if you have the <linux/types.h> header file. */ #undef HAVE_LINUX_TYPES_H -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5211
From: Kirill Zhumarin <kirill.zhumarin(a)gmail.com> --- dlls/ntdll/unix/serial_termios2.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dlls/ntdll/unix/serial_termios2.c b/dlls/ntdll/unix/serial_termios2.c index 84be6ac6b5f..9d19128b929 100644 --- a/dlls/ntdll/unix/serial_termios2.c +++ b/dlls/ntdll/unix/serial_termios2.c @@ -41,9 +41,10 @@ #include "wine/debug.h" #include "unix_private.h" +#ifdef HAVE_ASM_TERMBITS_H + WINE_DEFAULT_DEBUG_CHANNEL(comm); -#ifdef HAVE_ASM_TERMBITS_H NTSTATUS serial_set_baud_rate_termios2( int fd, int baudrate ) { struct termios2 port; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5211
From: Kirill Zhumarin <kirill.zhumarin(a)gmail.com> --- dlls/ntdll/unix/serial_termios2.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/dlls/ntdll/unix/serial_termios2.c b/dlls/ntdll/unix/serial_termios2.c index 9d19128b929..8c8de49ca2e 100644 --- a/dlls/ntdll/unix/serial_termios2.c +++ b/dlls/ntdll/unix/serial_termios2.c @@ -24,11 +24,10 @@ #include "config.h" +#ifdef HAVE_ASM_TERMBITS_H #include <errno.h> #include <sys/ioctl.h> -#ifdef HAVE_ASM_TERMBITS_H #include <asm/termbits.h> -#endif #include "ntstatus.h" #define WIN32_NO_STATUS @@ -41,8 +40,6 @@ #include "wine/debug.h" #include "unix_private.h" -#ifdef HAVE_ASM_TERMBITS_H - WINE_DEFAULT_DEBUG_CHANNEL(comm); NTSTATUS serial_set_baud_rate_termios2( int fd, int baudrate ) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5211
On Sat Mar 2 16:56:52 2024 +0000, Vijay Kiran Kamuju wrote:
Please fix the build errors, move the ifdef pragma to the earlier position. Now I moved `#ifdef` to the top of the file. But nothing changed. I see that pipeline cache code from the first commit and don't pull my recent changes.
The pipeline is broken? :( 44 line - in the pipeline (like the first commit)  43 line - in the code  -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5211#note_63294
In the first part of serial_termios2.c, you need to put the ifdef HAVE_ASM_TERMBITS_H only for the termios2 header. Second part, you need to handle if the termios2 support is not present for the function, what to return if it's not available. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5211#note_63295
a couple of comments: * you need to add asm/termbits.h to the list of files searched for configuration in \<winesrc\>/configure.ac (no need to add the changes to configure in the merge request, this will be done automatically) (note this is why you don't get the same errors locally as on the pipeline ; but are a good feedback of a compilation on a non Linux box) * it seems overkill to create a dedicated .c file for this (unless you foresee lots of other stuff to go into that file) ; inserting the needed changes in existing functions shoud be ok * please fold your two changes into one (we want every commit to compile without error, obviously the first isn't) * nitpick: existing code returns the output baudrate; yours return the input baudrate. I'd rather keep the same as existing code for consistency. * the fallback to termios ioctl could be improved: * we could have twice the ERR printed in case of real error, * we could have a false positive that will confuse users if termios2 ioctl fails but termios succeeds (very old kernels) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5211#note_63296
Please follow what Eric has suggested for configure code. I have made some changes to your code, which might work: ` --- a/dlls/ntdll/unix/serial.c +++ b/dlls/ntdll/unix/serial.c @@ -63,6 +63,9 @@ #endif #include <linux/serial.h> #endif +#ifdef HAVE_ASM_TERMBITS_H +#include <asm/termbits.h> +#endif #if !defined(TIOCINQ) && defined(FIONREAD) #define TIOCINQ FIONREAD @@ -119,6 +122,16 @@ static const char* iocode2str(UINT ioc) static NTSTATUS get_baud_rate(int fd, SERIAL_BAUD_RATE* sbr) { +#ifdef HAVE_ASM_TERMBITS_H + struct termios2 port; + + if (ioctl(fd, TCGETS2, &port) == -1) + { + ERR("ioctl TCGETS2 error '%s'\n", strerror(errno)); + return errno_to_status( errno ); + } + sbr->BaudRate = port.c_ospeed; +#else struct termios port; int speed; @@ -192,6 +205,7 @@ static NTSTATUS get_baud_rate(int fd, SERIAL_BAUD_RATE* sbr) ERR("unknown speed %x\n", speed); return STATUS_INVALID_PARAMETER; } +#endif return STATUS_SUCCESS; } @@ -453,6 +467,25 @@ static NTSTATUS purge(int fd, DWORD flags) static NTSTATUS set_baud_rate(int fd, const SERIAL_BAUD_RATE* sbr) { +#ifdef HAVE_ASM_TERMBITS_H + struct termios2 port; + + if (ioctl(fd, TCGETS2, &port) == -1) + { + ERR("ioctl TCGETS2 error '%s'\n", strerror(errno)); + return errno_to_status( errno ); + } + + port.c_cflag &= ~CBAUD; + port.c_cflag |= BOTHER; + port.c_ospeed = sbr->BaudRate; + + if (ioctl(fd, TCSETS2, &port) == -1) + { + ERR("ioctl TCSETS2 error '%s'\n", strerror(errno)); + return errno_to_status( errno ); + } +#else struct termios port; if (tcgetattr(fd, &port) == -1) @@ -564,6 +597,7 @@ static NTSTATUS set_baud_rate(int fd, const SERIAL_BAUD_RATE* sbr) ERR("tcsetattr error '%s'\n", strerror(errno)); return errno_to_status( errno ); } +#endif return STATUS_SUCCESS; } ` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5211#note_63298
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=143642 Your paranoid android. === debian11 (build log) === Task: Patch failed to apply === debian11b (build log) === Task: Patch failed to apply
participants (5)
-
eric pouech (＠epo) -
Kirill Zhumarin
-
Kirill Zhumarin (＠kirill.zhumarin)
-
Marvin -
Vijay Kiran Kamuju (＠infyquest)