Message-ID: <169a23a2-3602-8c24-b42a-37fb05564fdf@gmail.com>
Date:   Thu, 15 Nov 2018 09:19:44 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] serial: 8250: Default SERIAL_OF_PLATFORM to SERIAL_8250



On 11/14/2018 9:36 PM, Guenter Roeck wrote:
> On Wed, Nov 14, 2018 at 07:56:47PM -0800, Florian Fainelli wrote:
>>
>>
>> On November 14, 2018 5:11:25 PM PST, Guenter Roeck <linux@...ck-us.net> wrote:
>>> On Thu, Nov 01, 2018 at 11:26:06AM -0700, Florian Fainelli wrote:
>>>> It is way too easy to miss enabling SERIAL_OF_PLATFORM which would
>>>> result in the inability for the kernel to have a valid console
>>> device,
>>>> which can be seen with:
>>>>
>>>> Warning: unable to open an initial console.
>>>>
>>>> and then:
>>>>
>>>> Run /init as init process
>>>> Kernel panic - not syncing: Attempted to kill init!
>>> exitcode=0x00000100
>>>>
>>>> Since SERIAL_OF_PLATFORM already depends on SERIAL_8250 && OF there
>>>> really is no drawback to defaulting this config to the value of
>>>> SERIAL_8250.
>>>>
>>>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
>>>> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>>>
>>> This patch results in situations where CONFIG_SERIAL_OF_PLATFORM is now
>>> defined where it was not previously. Example mpc85xx_defconfig. This in
>>> turn results in boot failures for those configurations, with an error
>>> message of
>>>
>>> of_serial: probe of e0004500.serial failed with error -22
>>>
>>> which wasn't seen before.
>>
>> Do you know which Device Tree is being used here? The most obvious thing that could be done is to add a !PPC condition but this might be missing other platforms doing their own 8250 registration yet being OF aware (sparc?).
> 
> This is a qemu boot test. No, I don't know what exactly is happening,
> except that this (emulated) system obviously does not expect
> CONFIG_SERIAL_OF_PLATFORM to be enabled and, afaik, the devicetree
> is generated internally by qemu. I would have thought that just enabling
> a configuration by default out of the blue might be considered problematic
> by itself, but maybe I am wrong.
> 
>>
>>>
>>> Not sure if replacing a potential problem with a real one is really an
>>> improvement.`
>>
>> That comment is not particularly helpful though I have an appreciation for when a change breaks things in unexpected ways and how frustrating that can be.
> 
> What is really frustrating (and let me think about dropping all those
> boot tests) is that one ends up having to argue if the problem is real
> or only applies to a presumably or possibly wrong qemu emulation, and
> that one ends up having to discuss the validity of the test case. 

What I was objecting to is your qualification of the issue, this is
unfortunately not a potential/latent problem, it happens more often than
not and the fact that my patch causes another platform to break is not
expected and deserves fixing (which I am looking at the moment). That is
all my comment was supposed to mean, and

> 
> Since this is "only" an emulation and thus not a "real" system,
> please feel free to ignore this report. I'll just drop all boot tests
> using this configuration once the patch hits mainline.

Sounds like we did not start on the right foot with my reply to your
comment, so let's move on and just agree that this needs fixing, period.
This does not question the value of your tests which are extremely valuable.
-- 
Florian

Powered by blists - more mailing lists