Re: [PATCH v6 1/2] tty: serial: Cleanup the bit shift with macro

Re: [PATCH v6 1/2] tty: serial: Cleanup the bit shift with macro

[Christoph Niedermaier]

In the old definition (1 << 3) wasn't used.

> -#define SER_RS485_RX_DURING_TX		(1 << 4)
> -#define SER_RS485_TERMINATE_BUS		(1 << 5)
> -#define SER_RS485_ADDRB			(1 << 6)
> -#define SER_RS485_ADDR_RECV		(1 << 7)
> -#define SER_RS485_ADDR_DEST		(1 << 8)
> +#define SER_RS485_ENABLED		_BITUL(0)
> +#define SER_RS485_RTS_ON_SEND		_BITUL(1)
> +#define SER_RS485_RTS_AFTER_SEND	_BITUL(2)
> +#define SER_RS485_RX_DURING_TX		_BITUL(3)

Isn't it a break if number 3 isn't skipped here as well?

Regards
Christoph

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 1/2] tty: serial: Cleanup the bit shift with macro

[Christoph Niedermaier]

Hi everyone,

> This patch replaces the bit shift code with "_BITUL()" macro inside
> "serial_rs485" struct.
> 
> Signed-off-by: Crescent CY Hsieh <crescentcy.hsieh@moxa.com>
> ---
>  include/uapi/linux/serial.h | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> index 53bc1af67..6c75ebdd7 100644
> --- a/include/uapi/linux/serial.h
> +++ b/include/uapi/linux/serial.h
> @@ -11,6 +11,7 @@
>  #ifndef _UAPI_LINUX_SERIAL_H
>  #define _UAPI_LINUX_SERIAL_H
>  
> +#include <linux/const.h>
>  #include <linux/types.h>
>  
>  #include <linux/tty_flags.h>
> @@ -140,14 +141,14 @@ struct serial_icounter_struct {
>   */
>  struct serial_rs485 {
>  	__u32	flags;
> -#define SER_RS485_ENABLED		(1 << 0)
> -#define SER_RS485_RTS_ON_SEND		(1 << 1)
> -#define SER_RS485_RTS_AFTER_SEND	(1 << 2)

In the old definition (1 << 3) wasn't used.

> -#define SER_RS485_RX_DURING_TX		(1 << 4)
> -#define SER_RS485_TERMINATE_BUS		(1 << 5)
> -#define SER_RS485_ADDRB			(1 << 6)
> -#define SER_RS485_ADDR_RECV		(1 << 7)
> -#define SER_RS485_ADDR_DEST		(1 << 8)
> +#define SER_RS485_ENABLED		_BITUL(0)
> +#define SER_RS485_RTS_ON_SEND		_BITUL(1)
> +#define SER_RS485_RTS_AFTER_SEND	_BITUL(2)
> +#define SER_RS485_RX_DURING_TX		_BITUL(3)

Isn't it a break if number 3 isn't skipped here as well?

Regards
Christoph

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 1/2] tty: serial: Cleanup the bit shift with macro

[Greg KH]

On Wed, Jan 17, 2024 at 03:56:23PM +0100, Christoph Niedermaier wrote:
> Hi everyone,
> 
> > This patch replaces the bit shift code with "_BITUL()" macro inside
> > "serial_rs485" struct.
> > 
> > Signed-off-by: Crescent CY Hsieh <crescentcy.hsieh@moxa.com>
> > ---
> >  include/uapi/linux/serial.h | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> > index 53bc1af67..6c75ebdd7 100644
> > --- a/include/uapi/linux/serial.h
> > +++ b/include/uapi/linux/serial.h
> > @@ -11,6 +11,7 @@
> >  #ifndef _UAPI_LINUX_SERIAL_H
> >  #define _UAPI_LINUX_SERIAL_H
> >  
> > +#include <linux/const.h>
> >  #include <linux/types.h>
> >  
> >  #include <linux/tty_flags.h>
> > @@ -140,14 +141,14 @@ struct serial_icounter_struct {
> >   */
> >  struct serial_rs485 {
> >  	__u32	flags;
> > -#define SER_RS485_ENABLED		(1 << 0)
> > -#define SER_RS485_RTS_ON_SEND		(1 << 1)
> > -#define SER_RS485_RTS_AFTER_SEND	(1 << 2)
> 
> In the old definition (1 << 3) wasn't used.
> 
> > -#define SER_RS485_RX_DURING_TX		(1 << 4)
> > -#define SER_RS485_TERMINATE_BUS		(1 << 5)
> > -#define SER_RS485_ADDRB			(1 << 6)
> > -#define SER_RS485_ADDR_RECV		(1 << 7)
> > -#define SER_RS485_ADDR_DEST		(1 << 8)
> > +#define SER_RS485_ENABLED		_BITUL(0)
> > +#define SER_RS485_RTS_ON_SEND		_BITUL(1)
> > +#define SER_RS485_RTS_AFTER_SEND	_BITUL(2)
> > +#define SER_RS485_RX_DURING_TX		_BITUL(3)
> 
> Isn't it a break if number 3 isn't skipped here as well?

Ugh, yes it is, good catch!

Care to send a patch to fix this up?

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 1/2] tty: serial: Cleanup the bit shift with macro

[Crescent CY Hsieh]

On Thu, Jan 18, 2024 at 08:01:58AM +0100, Greg KH wrote:
> On Wed, Jan 17, 2024 at 03:56:23PM +0100, Christoph Niedermaier wrote:
> > >  struct serial_rs485 {
> > >  	__u32	flags;
> > > -#define SER_RS485_ENABLED		(1 << 0)
> > > -#define SER_RS485_RTS_ON_SEND		(1 << 1)
> > > -#define SER_RS485_RTS_AFTER_SEND	(1 << 2)
> > 
> > In the old definition (1 << 3) wasn't used.
> > 
> > > -#define SER_RS485_RX_DURING_TX		(1 << 4)
> > > -#define SER_RS485_TERMINATE_BUS		(1 << 5)
> > > -#define SER_RS485_ADDRB			(1 << 6)
> > > -#define SER_RS485_ADDR_RECV		(1 << 7)
> > > -#define SER_RS485_ADDR_DEST		(1 << 8)
> > > +#define SER_RS485_ENABLED		_BITUL(0)
> > > +#define SER_RS485_RTS_ON_SEND		_BITUL(1)
> > > +#define SER_RS485_RTS_AFTER_SEND	_BITUL(2)
> > > +#define SER_RS485_RX_DURING_TX		_BITUL(3)
> > 
> > Isn't it a break if number 3 isn't skipped here as well?

Sorry I might have misunderstood the meaning of "broke userspace".

In this case, does it imply splitting the "cleanup" patch and the "add
feature" patch, or leaving the third bit unused? Or perhaps both?

---
Sincerely,
Crescent Hsieh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 1/2] tty: serial: Cleanup the bit shift with macro

[Greg KH]

On Thu, Jan 18, 2024 at 04:14:41PM +0800, Crescent CY Hsieh wrote:
> On Thu, Jan 18, 2024 at 08:01:58AM +0100, Greg KH wrote:
> > On Wed, Jan 17, 2024 at 03:56:23PM +0100, Christoph Niedermaier wrote:
> > > >  struct serial_rs485 {
> > > >  	__u32	flags;
> > > > -#define SER_RS485_ENABLED		(1 << 0)
> > > > -#define SER_RS485_RTS_ON_SEND		(1 << 1)
> > > > -#define SER_RS485_RTS_AFTER_SEND	(1 << 2)
> > > 
> > > In the old definition (1 << 3) wasn't used.
> > > 
> > > > -#define SER_RS485_RX_DURING_TX		(1 << 4)
> > > > -#define SER_RS485_TERMINATE_BUS		(1 << 5)
> > > > -#define SER_RS485_ADDRB			(1 << 6)
> > > > -#define SER_RS485_ADDR_RECV		(1 << 7)
> > > > -#define SER_RS485_ADDR_DEST		(1 << 8)
> > > > +#define SER_RS485_ENABLED		_BITUL(0)
> > > > +#define SER_RS485_RTS_ON_SEND		_BITUL(1)
> > > > +#define SER_RS485_RTS_AFTER_SEND	_BITUL(2)
> > > > +#define SER_RS485_RX_DURING_TX		_BITUL(3)
> > > 
> > > Isn't it a break if number 3 isn't skipped here as well?
> 
> Sorry I might have misunderstood the meaning of "broke userspace".
> 
> In this case, does it imply splitting the "cleanup" patch and the "add
> feature" patch, or leaving the third bit unused? Or perhaps both?

You can not redefine existing defines to different values, like you did
here, that changes the user/kernel api of the system.

Luckily this isn't in any released kernel, I'll either revert this after
-rc1 is out, or take a patch from anyone else to do so if they want to
send it now.

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v6 1/2] tty: serial: Cleanup the bit shift with macro

[Christoph Niedermaier]

From: Greg KH <gregkh@linuxfoundation.org>
Sent: Thursday, January 18, 2024 8:02 AM
> On Wed, Jan 17, 2024 at 03:56:23PM +0100, Christoph Niedermaier wrote:
>> Hi everyone,
>>
>>> This patch replaces the bit shift code with "_BITUL()" macro inside
>>> "serial_rs485" struct.
>>>
>>> Signed-off-by: Crescent CY Hsieh <crescentcy.hsieh@moxa.com>
>>> ---
>>>  include/uapi/linux/serial.h | 17 +++++++++--------
>>>  1 file changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
>>> index 53bc1af67..6c75ebdd7 100644
>>> --- a/include/uapi/linux/serial.h
>>> +++ b/include/uapi/linux/serial.h
>>> @@ -11,6 +11,7 @@
>>>  #ifndef _UAPI_LINUX_SERIAL_H
>>>  #define _UAPI_LINUX_SERIAL_H
>>>
>>> +#include <linux/const.h>
>>>  #include <linux/types.h>
>>>
>>>  #include <linux/tty_flags.h>
>>> @@ -140,14 +141,14 @@ struct serial_icounter_struct {
>>>   */
>>>  struct serial_rs485 {
>>>     __u32   flags;
>>> -#define SER_RS485_ENABLED          (1 << 0)
>>> -#define SER_RS485_RTS_ON_SEND              (1 << 1)
>>> -#define SER_RS485_RTS_AFTER_SEND   (1 << 2)
>>
>> In the old definition (1 << 3) wasn't used.
>>
>>> -#define SER_RS485_RX_DURING_TX             (1 << 4)
>>> -#define SER_RS485_TERMINATE_BUS            (1 << 5)
>>> -#define SER_RS485_ADDRB                    (1 << 6)
>>> -#define SER_RS485_ADDR_RECV                (1 << 7)
>>> -#define SER_RS485_ADDR_DEST                (1 << 8)
>>> +#define SER_RS485_ENABLED          _BITUL(0)
>>> +#define SER_RS485_RTS_ON_SEND              _BITUL(1)
>>> +#define SER_RS485_RTS_AFTER_SEND   _BITUL(2)
>>> +#define SER_RS485_RX_DURING_TX             _BITUL(3)
>>
>> Isn't it a break if number 3 isn't skipped here as well?
> 
> Ugh, yes it is, good catch!
> 
> Care to send a patch to fix this up?

OK, I'll make a patch.

Regards
Christoph
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel