earlycon issues in -next with amba-pl011 updates

[Marc Zyngier]

On 03/09/15 19:03, Peter Hurley wrote:
> On 09/03/2015 01:53 PM, Greg Kroah-Hartman wrote:
>> On Thu, Sep 03, 2015 at 01:49:02PM -0400, Peter Hurley wrote:
>>> On 09/03/2015 12:08 PM, Marc Zyngier wrote:
>>>> On 03/09/15 16:52, Greg Kroah-Hartman wrote:
>>>>> On Thu, Sep 03, 2015 at 11:23:15AM +0100, Marc Zyngier wrote:
>>>>>> On 11/08/15 02:48, Jun Nie wrote:
>>>>>>> 2015-08-11 7:23 GMT+08:00 Leif Lindholm <leif.lindholm at linaro.org>:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> The kernelci.org bot picked up a complete boot failure (no output past
>>>>>>>> UEFI stub) with next-20150806 and Tyler bisected it down to somewhere
>>>>>>>> in
>>>>>>>> 8cd90e5 uart: pl011: Add support to ZTE ZX296702 uart
>>>>>>>> 09dcc7d uart: pl011: Improve LCRH register access decision
>>>>>>>> 2c096a9 uart: pl011: Introduce register look up table
>>>>>>>> 7b753f3 uart: pl011: Introduce register accessor
>>>>>>>>
>>>>>>>> The issue only appears with earlycon on command line, for pl011
>>>>>>>> consoles.
>>>>>>>>
>>>>>>>> Some investigation shows that the cause lies with
>>>>>>>> commit 7b753f318d14 ("uart: pl011: Introduce register accessor")
>>>>>>>> and
>>>>>>>> commit 2c096a9eedc6 ("uart: pl011: Introduce register look up table")
>>>>>>>>
>>>>>>>> Specifically, the changes to pl011_putc() are incorrect:
>>>>>>>> The new pl011_ accessors take a (struct uart_amba_port *) input, but
>>>>>>>> pl011_putc() directly uses the incoming (struct uart_port *) for this.
>>>>>>>>
>>>>>>>> Apart from ending up with an unintended/incorrect UART base address,
>>>>>>>> the introduction of the lookup table for register offsets also means
>>>>>>>> the accessors try to dereference (struct uart_amba_port *)->reg_lut.
>>>>>>>>
>>>>>>>> The below is a hack that shows/resolves the issue, but some
>>>>>>>> refactoring of the original patches might be in order.
>>>>>>>>
>>>>>>>> /
>>>>>>>>     Leif
>>>>>>>
>>>>>>> Leif,
>>>>>>>
>>>>>>> Sorry for the inconvenience. I do not have idea of early console till
>>>>>>> now and I always have debug console for early panic debug. Learned
>>>>>>> more from this issue.
>>>>>>>
>>>>>>> Suppose Peter's patch will resolve your issue.
>>>>>>
>>>>>> [+ Greg KH]
>>>>>>
>>>>>> So -next has now been broken for a while on a number of ARM platforms
>>>>>> because of this (they simply cannot boot), and no progress has been made
>>>>>> towards resolving this problem.
>>>>>>
>>>>>> Can we please drop this series (at least commits 7b753f3 and following)
>>>>>> from -next until is has been reworked and reviewed?
>>>>>
>>>>> I don't have any patches in my -next tree, everything is in Linus's tree
>>>>> now.  So if I've missed something, or need to revert something, please
>>>>> let me know specifcally what to do.
>>>>
>>>> Gahhh... Given that there is no obvious fix
>>>
>>> Fix has been on list since Aug 10.
>>>
>>> http://www.spinics.net/lists/linux-serial/msg18576.html
>>
>> Now if only someone would have at least compile tested it :)
> 
> On 08/11/2015 06:07 AM, Leif Lindholm wrote:
>> It works, but builds with:
>> drivers/tty/serial/amba-pl011.c:329:13: warning: ‘pl011_writeb’
>> defined but not used [-Wunused-function]
>>  static void pl011_writeb(struct uart_amba_port *uap, u8 val, int
>>  index)
>>              ^
>> I was dithering about whether having _relaxed accessors in post boot
>> code and plain ones in earlycon was an issue, but I guess it doesn't
>> matter much.
> 
> Russell wanted me to do a bunch of improvements, but I wrote that
> should wait until 4.3-rc cycle.

It is well known that we merge "improvements" outside of the merge
window. And what about starting with a series that is actually bisectable?

It is quite apparent that this code hasn't been tested enough, hasn't
been reviewed enough, and breaks a wide range of platforms (almost
anything that sits on and under my desk, TBH).

That's really for Greg to decide, but I'm not overly confident with the
state of this series as it stands.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...