linux-kernel - RE: [EXT] Re: [PATCH RESEND 3/5] USB: serial: cp210x: Added comments to CRTSCT flag code.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 27 Apr 2016 20:06:32 +0000
From:	Konstantin Shkolnyy <Konstantin.Shkolnyy@...abs.com>
To:	Johan Hovold <johan@...nel.org>
CC:	Konstantin Shkolnyy <konstantin.shkolnyy@...il.com>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [EXT] Re: [PATCH RESEND 3/5] USB: serial: cp210x: Added comments
 to CRTSCT flag code.

> -----Original Message-----
> From: Johan Hovold [mailto:jhovold@...il.com] On Behalf Of Johan Hovold
> Sent: Tuesday, April 26, 2016 02:26
> To: Konstantin Shkolnyy
> Cc: Johan Hovold; Konstantin Shkolnyy; linux-usb@...r.kernel.org; linux-
> kernel@...r.kernel.org
> Subject: Re: [EXT] Re: [PATCH RESEND 3/5] USB: serial: cp210x: Added
> comments to CRTSCT flag code.
> 
> On Mon, Apr 25, 2016 at 06:09:01PM +0000, Konstantin Shkolnyy wrote:
> > I was planning to define all these bits in a separate future patch.
> > Would you rather prefer the magic numbers defined before fixing the
> bugs?
> 
> Fixing the RTS bug (patch 1), which is the only "real" bug, should be
> done before adding defines, and fixing and cleaning up the rest.
> 
> > I guess I can do that. Is something like this acceptable?
> >
> > /* CP210X_GET_FLOW/CP210X_SET_FLOW read/write these 0x10 bytes */
> > struct cp210x_flow_ctl {
> > 	u8	SERIAL_DTR_MASK        : 2; /* byte 0 */
> > 	u8	                       : 1;
> > 	u8	SERIAL_CTS_HANDSHAKE   : 1;
> > 	u8	SERIAL_DSR_HANDSHAKE   : 1;
> > 	u8	SERIAL_DCD_HANDSHAKE   : 1;
> > 	u8	SERIAL_DSR_SENSITIVITY : 1;
> > 	u8	                       : 1;
> > 	u8; /* byte 1 */
> > 	u8; /* byte 2 */
> > 	u8; /* byte 3 */
> > 	u8	SERIAL_AUTO_TRANSMIT   : 1; /* byte 4 */
> > 	u8	SERIAL_AUTO_RECEIVE    : 1;
> > 	u8	SERIAL_ERROR_CHAR      : 1;
> > 	u8	SERIAL_NULL_STRIPPING  : 1;
> > 	u8	SERIAL_BREAK_CHAR      : 1;
> > 	u8	                       : 1;
> > 	u8	SERIAL_RTS_MASK        : 2;
> > 	u8; /* byte 5 */
> > 	u8; /* byte 6 */
> > 	u8	                       : 7; /* byte 7 */
> > 	u8	SERIAL_XOFF_CONTINUE   : 1;
> > 	__le32	ulXonLimit;
> > 	__le32	ulXoffLimit;
> > } __packed;
> 
> No, shouldn't rely on the layout of bitfields. Define masks and shifts
> as needed and the message structure as
> 
> struct cp210x_flow_ctl {
> 	__le32	ulControlHandshake;
> 	__le32	ulFlowReplace;
>  	__le32	ulXonLimit;
>  	__le32	ulXoffLimit;
> };
> 
> that is, as per AN571.
> 

OK, from searching www I see that bitfields have bad reputation for unclear reasons, so I guess it's now easier to avoid them.
But doing it like you suggest, instead of splitting it to bytes, would complicate the code with endian conversions.
Is there a reason for this other than making it identical to the spec?\