Re: [PATCH v2] serial: 8250: Apply FSL workarounds also without SERIAL_8250_CONSOLE

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3809 bytes --]

Hello Ilpo,

On Wed, May 31, 2023 at 12:40:10PM +0200, Uwe Kleine-König wrote:
> On Wed, May 31, 2023 at 01:09:01PM +0300, Ilpo Järvinen wrote:
> > On Wed, 31 May 2023, Uwe Kleine-König wrote:
> > 
> > > On Wed, May 31, 2023 at 12:47:54PM +0300, Ilpo Järvinen wrote:
> > > > On Wed, 31 May 2023, Uwe Kleine-König wrote:
> > > > 
> > > > > The need to handle the FSL variant of 8250 in a special way is also
> > > > > present without console support. So soften the dependency for
> > > > > SERIAL_8250_FSL accordingly.
> > > > > 
> > > > > This issue was identified by Dominik Andreas Schorpp.
> > > > > 
> > > > > To cope for CONFIG_SERIAL_8250=m + CONFIG_SERIAL_8250_FSL=y, 8250_fsl.o
> > > > > must be put in the same compilation unit as 8250_port.o because the
> > > > > latter defines some functions needed in the former and so 8250_fsl.o
> > > > > must not be built-in if 8250_port.o is available in a module.
> > > > > 
> > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > ---
> > > > > Hello,
> > > > > 
> > > > > (implicit) v1 was already applied by Greg (a0807ca158e0 in tty-testing)
> > > > > but that didn't handle CONFIG_SERIAL_8250=m + CONFIG_SERIAL_8250_FSL=y
> > > > > correctly which was pointed out by the 0-day bot. (Thanks!)
> > > > 
> > > > That would warrant Reported-by (0-day's reports give you the tag).
> > > 
> > > I'd add this tag if I created a commit that fixes the broken commit.
> > > However I understood that if a v2 patch fixes a v1 that was broken, the
> > > tag is not to be added?! I don't feel strong here however, so if people
> > > agree that the tag should be there, I can add it.
> > > 
> > > > > That wasn't a problem before because SERIAL_8250_CONSOLE depends on
> > > > > SERIAL_8250=y.
> > > > > 
> > > > > Having said that I wonder if there are a few more .o files that should
> > > > > better be used with 8250_base-$(CONFIG_SERIAL_8250_XXX) instead of
> > > > > obj-$(CONFIG_SERIAL_8250_XXX).
> > > > > 
> > > > > Best regards
> > > > > Uwe
> > > > > 
> > > > >  drivers/tty/serial/8250/Kconfig  | 2 +-
> > > > >  drivers/tty/serial/8250/Makefile | 2 +-
> > > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> > > > > index 5313aa31930f..10c09b19c871 100644
> > > > > --- a/drivers/tty/serial/8250/Kconfig
> > > > > +++ b/drivers/tty/serial/8250/Kconfig
> > > > > @@ -378,7 +378,7 @@ config SERIAL_8250_BCM2835AUX
> > > > >  
> > > > >  config SERIAL_8250_FSL
> > > > >  	bool "Freescale 16550 UART support" if COMPILE_TEST && !(PPC || ARM || ARM64)
> > > > > -	depends on SERIAL_8250_CONSOLE
> > > > > +	depends on SERIAL_8250
> > > > 
> > > > Why this cannot simply be:
> > > > 	depends on SERIAL_8250=y
> > > 
> > > This doesn't work, because then the FSL-workarounds are missing if the
> > > 8250 driver is compiled as a module.
> > 
> > How can 8250 driver be a module and fsl still get enabled?
> 
> It works. With my patch applied:
> 
> 	$ make allmodconfig
> 	$ grep -E 'CONFIG_SERIAL_8250(_FSL)?\>' .config
> 	CONFIG_SERIAL_8250=m
> 	CONFIG_SERIAL_8250_FSL=y
> 
> > What I think (not a Kconfig expert for sure) would happen is that 8250_fsl 
> > won't be enabled at all if CONFIG_SERIAL_8250=m because it depends on 
> > SERIAL_8250=y.
> 
> That's not how it seems to be ...

If this convinces you that the patch is fine, an ack would be nice as
gregkh signaled that there is some pending discussion he is waiting to
end before applying this patch.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]