linux-kernel - Re: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access functions for large registers

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]

Message-ID: <20160114185311.GA7211@localhost>
Date:	Thu, 14 Jan 2016 19:53:11 +0100
From:	Johan Hovold <johan@...nel.org>
To:	Konstantin Shkolnyy <Konstantin.Shkolnyy@...abs.com>
Cc:	Martyn Welch <martyn.welch@...labora.co.uk>,
	Johan Hovold <johan@...nel.org>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register
 access functions for large registers

On Thu, Jan 14, 2016 at 06:15:42PM +0000, Konstantin Shkolnyy wrote:

> The patching procedure enforced by maintainers dictates that each
> separate patch addresses 1 issue.
> It's much easier to review changes this way. So this particular patch
> just converts from one register access function to another.
> The bugfix patch will come later.

Do one logical change per patch is a good rule of thumb, yes. But we
must never introduce regression by taking that rule too far.

I haven't had time to review your patches yet, but I remember thinking
that those FIXMEs looked odd. How about adding the missing error
handling before you introduce the new helpers?

> While doing this cleanup, I also noticed another bug - this function
> will always set the low 2 bits of byte 0 to  01b: "modem_ctl[0] |=
> 0x01".
> This field is called SERIAL_DTR_MASK. It's 0 by default. ("DTR is held
> inactive"). The function will only write it when CRTSCTS changes.
> So the device will start with 0, then, if CRTSCTS ever changes, it'll
> become 1 and stay 1 forever. Looks wrong to me.
> I'm still researching the subject when and how it should be set.
> 
> 	 * Wikipedia: "DTR and DSR are usually on all the time and, per the
> 	 * RS-232 standard and its successors, are used to signal from each
> 	 * end that the other equipment is actually present and powered-up."
> 	 * So perhaps DTR should be turned ON in open() and OFF in close()?
> 
> I'm waiting on this patch series to be accepted, then submit other
> improvements. Or it may be better to submit a longer patch series that
> has further improvements appended... I'm new here and not really sure.

Generally, you should wait for a series to be reviewed before sending
(too many) follow ups. Unless you find any issues with it and ask for it
to be dropped, that is.

Thanks,
Johan