linux-kernel - Re: [PATCH v3] serial: mxs-auart: keep the AUART unit in reset state when not in use

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: <1437143728.3254.10.camel@pengutronix.de>
Date:	Fri, 17 Jul 2015 16:35:28 +0200
From:	Philipp Zabel <p.zabel@...gutronix.de>
To:	Peter Hurley <peter@...leysoftware.com>
Cc:	Juergen Borleis <jbe@...gutronix.de>,
	Stefan Wahren <stefan.wahren@...e.com>, kernel@...gutronix.de,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
	Jiri Slaby <jslaby@...e.cz>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3] serial: mxs-auart: keep the AUART unit in reset
 state when not in use

Am Freitag, den 17.07.2015, 09:10 -0400 schrieb Peter Hurley:
> Hi Juergen,
> 
> On 07/16/2015 03:40 AM, Juergen Borleis wrote:
> > Whenever the UART device driver gets closed from userland, the driver
> > disables the UART unit and then stops its clock to save power.
> > 
> > The bit which disabled the UART unit is described as:
> > 
> >   "UART Enable. If this bit is set to 1, the UART is enabled. Data
> >   transmission and reception occurs for the UART signals. When the
> >   UART is disabled in the middle of transmission or reception, it
> >   completes the current character before stopping."
> > 
> > The important part is the "it completes the current character". Whenever
> > a reception is ongoing when the UART gets disabled (including the clock
> > off) the statemachine freezes and "remembers" this state on the next
> > open() and re-enabling of the unit's clock.
> > 
> > In this case we end up receiving an additional bogus character
> > immediately.
> > 
> > The solution in this change is to move the AUART unit into its reset
> > state on close() and only release it from its reset state on the next
> > open().
> > 
> > Signed-off-by: Juergen Borleis <jbe@...gutronix.de>
> > ---
> > 
> > Notes:
> >     v3:
> >      - missing newline added to an error message
> >     
> >     v2:
> >      - rename mxs_auart_do_reset() to mxs_auart_keep_reset() to better reflect
> >        what it really does
> >      - adapt the delay in mxs_auart_keep_reset() to wait for the reset of the
> >        AURAT unit to what is used in mxs_auart_reset()
> 
> The function names and semantics are not clear. See below.
[...]
> > +	/* bring it out of reset now */
> > +	mxs_auart_reset(u);
> 
> mxs_auart_reset() is really not resetting but simply performing a block enable.
> Isn't there a generic block enable for the iMX.2x SoCs? (Maybe there should be)
> 
> The names of these functions don't match expected operations of startup().
> Start up should be enabling the device, not keeping it in reset.
> 
> And "keep reset" immediately followed by "reset" makes no sense.

I'd call the pair mxs_auart_reset_assert and mxs_auart_reset_deassert.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/