Re: [PATCH RT 0/2] serial: 8250: atomic console fixups

[Dick Hollenbeck <dick@softplc.com>]

On 1/10/20 9:39 AM, John Ogness wrote:
> Dick Hollenbeck pointed out[0] that serial usage for non-consoles
> was dramatically affected due to the atomic console
> implementation. Indeed, the implementation was taking the global
> console_atomic_lock for _all_ 8250 ports, regardless if they were
> consoles or not.
>
> This series fixes that by only using the cpu-lock if the port is
> a console. While making those changes, I realized that the clear
> counter/ier-cache is not needed, so that is also removed.
>
> Dick also pointed out that the IER accessor functions were using
> non-typical names. These are updated and moved to inline macros.
>
> Finally, I noticed that the fsl, ingenic, and mtk variants were
> directly accessing IER. These are now also appropriately wrapped.
>
> In his suggestion, Dick did optimizations regarding IER caching.
> However, these are _not_ included because they may have some
> unwanted side-effects. For example, 




> I noticed a few places in
> 8250 code where the value of uart_8250_port->ier is expected to
> be different than the hardware register.

Sure, its not a trivial change, but I still think it would be superior to try and get it
right.  But I certainly have no more influence than the prior sentence.

I see this as unfortunate, and a propagation of a weak original design which stems from a
time when inline C functions were not even available.

  If serial8250_set_IER() function returned the prior value, it could be saved on the stack and still be an indication of difference from the current hardware for the temporary situation at hand.

Then, the compiler would have had a better chance of optimizing out the return value when it is not needed, because it would be a mere reading of the prior value from the struct.

I know I expressed some concern about using *3* functions, set, clear, and restore before, when 2 might suffice.  However we've seen other situations where a clear and restore are used, such as interrupt flags.
( "set" is a mere lower level common gateway to the hardware in our context.)  So down the road, a clear() and restore() may give you the option to do some locking and unlocking, which I know you don't currently think you need.  Pardon me for un-weighting my concern for 3 functions.

It is certainly a major improvement.  To be honest, I will probably simply comment out the console support in the inline function when and if I ever upgrade my code. I like that its inline, and that its in a single function.

Dick