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: <55ACF0E5.6040107@hurleysoftware.com>
Date:	Mon, 20 Jul 2015 09:00:21 -0400
From:	Peter Hurley <peter@...leysoftware.com>
To:	Juergen Borleis <jbe@...gutronix.de>
CC:	linux-kernel@...r.kernel.org,
	Stefan Wahren <stefan.wahren@...e.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.cz>, linux-serial@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, kernel@...gutronix.de
Subject: Re: [PATCH v3] serial: mxs-auart: keep the AUART unit in reset state
 when not in use

On 07/20/2015 04:04 AM, Juergen Borleis wrote:
> Hi Peter,
> 
> On Friday 17 July 2015 15:10:06 Peter Hurley wrote:
>> 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.
>>
>>>      - typo fixed
>>>
>>>  drivers/tty/serial/mxs-auart.c | 38
>>> ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+),
>>> 8 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/mxs-auart.c
>>> b/drivers/tty/serial/mxs-auart.c index 13cf773..135ee6c 100644
>>> --- a/drivers/tty/serial/mxs-auart.c
>>> +++ b/drivers/tty/serial/mxs-auart.c
>>> @@ -858,6 +858,30 @@ static void mxs_auart_reset(struct uart_port *u)
>>>  	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_CLR);
>>>  }
>>>
>>> +static void mxs_auart_keep_reset(struct uart_port *u)
>>> +{
>>> +	int i;
>>> +	u32 reg;
>>> +
>>> +	reg = readl(u->membase + AUART_CTRL0);
>>> +	/* if already in reset state, keep it untouched */
>>> +	if (reg & AUART_CTRL0_SFTRST)
>>> +		return;
>>> +
>>> +	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_CLR);
>>> +	writel(AUART_CTRL0_SFTRST, u->membase + AUART_CTRL0_SET);
>>> +
>>> +	for (i = 0; i < 10000; i++) {
>>> +		reg = readl(u->membase + AUART_CTRL0);
>>> +		/* reset is finished when the clock is gated */
>>> +		if (reg & AUART_CTRL0_CLKGATE)
>>> +			return;
>>> +		udelay(3);
>>> +	}
>>> +
>>> +	dev_err(u->dev, "Failed to reset the unit.\n");
>>> +}
>>> +
>>>  static int mxs_auart_startup(struct uart_port *u)
>>>  {
>>>  	int ret;
>>> @@ -867,7 +891,10 @@ static int mxs_auart_startup(struct uart_port *u)
>>>  	if (ret)
>>>  		return ret;
>>>
>>> -	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_CLR);
>>> +	/* reset the unit if not aleady done */
>>> +	mxs_auart_keep_reset(u);
>>
>> Why is this performing a soft-reset on open now? Didn't
>> mxs_auart_shutdown() already leave it in this state?
> 
> The probe function let the device left without reset. That is why I do a reset 
> here (which is skipped if the device is already in reset state).

So you're "fixing" something else (reset @ probe) without _any_
indication in the changelog?


>> Because if you're performing a soft-reset deliberately as part of the
>> initial condition, you make no mention of that in the changelog.
> 
> If I switch the device into the reset state immediately after the probe 
> function has finished, I could skip this additional reset.
> 
>>> +	/* 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.
> 
> mxs_auart_reset() is a bad name from the beginning. But I just wanted to keep 
> this change small and only change things that are really required.
> I will try with a V4.

ISTM this should be split into 2 or 3 separate patches:
1. Rename mxs_auart_reset()
2. Fix stale rx on re-open
3. Reset @ probe


Regards,
Peter Hurley
--
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/