linux-kernel - Re: 8250.nr_uarts broken in 3.7

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: <513A6A65.9050706@suse.cz>
Date:	Fri, 08 Mar 2013 23:47:01 +0100
From:	Jiri Slaby <jslaby@...e.cz>
To:	Josh Boyer <jwboyer@...hat.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:	linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: 8250.nr_uarts broken in 3.7

On 03/08/2013 10:27 PM, Josh Boyer wrote:
> On Fri, Mar 08, 2013 at 09:39:23AM +0800, Greg Kroah-Hartman wrote:
>>>>> Yes, reverting just 835d844d1 on top of 3.7.0 fixes it.  I also see why
>>>>> now.  That commit changed the module name from 8250 to 8250_core in the
>>>>> makefile, so clearly 8250.nr_uarts = 16 isn't going to get parsed.
>>>>> Adding 8250_core.nr_uarts = 16 seems to work fine.  This wasn't
>>>>> immediately obvious because the whole thing is built-in and not a
>>>>> module.  Thankfully, looking in /sys/modules/ still works and that
>>>>> showed up pretty clearly.
>>>>>
>>>>> So I guess this isn't really a break in functionality as much as it's a
>>>>> driver rename.  Not sure if it's worth fixing in some form or not.
>>>>
>>>> Yes it needs to be fixed, we shouldn't break userspace stuff like that.
>>>> Patches gladly accepted.
>>>
>>> OK... well I don't think renaming it back to 8250 is going to actually
>>> work.  It's already been renamed for 2 releases now, and a renaming
>>> isn't appropriate for 3.8.y.
>>>
>>> I don't remember exactly, but I don't think simply adding a modalias to
>>> 8250 will work either.
>>>
>>> So basically, the only way to fix this that I can see is to create a
>>> new, additional module parameter or something similar that parses
>>> "8250.nr_uarts".  Is that what you had in mind?
>>
>> I think that's the only way we can properly solve this, so yes, that
>> would be good.
> 
> "Be good" is subjective.  The following is functional, but it's pretty
> damn ugly.  Hopefully I'm missing some cleaner way to do this, but in my
> few attempts I didn't find anything.  I really hope I'm just blind or
> wrong or something.
> 
> I think the lesson here is "don't rename modules".
> 
> josh
> 
> From: Josh Boyer <jwboyer@...hat.com>
> Date: Fri, 8 Mar 2013 16:18:24 -0500
> Subject: [PATCH] serial: 8250: Keep 8250.nr_uarts option functional
> 
> With commit 835d844d1 (8250_pnp: do pnp probe before legacy probe), the
> 8250 driver was renamed to 8250_core.  This means any existing usage of
> 8250.nr_uarts= as module parameter or as a kernel command line switch is
> now broken, as the 8250_core driver doesn't parse options belonging to
> something called "8250".
> 
> We need the option to still be called "nr_uarts" and the existing module
> parameter macros do excellent checking to make sure you don't wind up with
> duplicate parameter names, so we can't just call __module_param_call with
> a different prefix.  Thus, we open code the kernel_param struct to add a
> "8250.nr_uarts" parameter to the module.
> 
> Signed-off-by: Josh Boyer <jwboyer@...hat.com>
> ---
>  drivers/tty/serial/8250/8250.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
> index 0efc815..e378e7e 100644
> --- a/drivers/tty/serial/8250/8250.c
> +++ b/drivers/tty/serial/8250/8250.c
> @@ -3388,6 +3388,16 @@ MODULE_PARM_DESC(share_irqs, "Share IRQs with other non-8250/16x50 devices"
>  module_param(nr_uarts, uint, 0644);
>  MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")");
>  
> +/* This module was renamed to 8250_core in 3.7.  Keep the old "8250" name
> + * working as well for nr_uarts so we don't break people.  We open code the
> + * addition of the parameter because we need to actually keep the name
> + * identical and the convenient macros will happily refuse to let us do that
> + * by failing the build with redefinition errors.  I'm really sorry.
> + */
> +static struct kernel_param __moduleparam_const __param_8250_nr_uarts
> +__used __attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *))))
> += { "8250.nr_uarts", &param_ops_uint, 0644, -1, { &nr_uarts } };
> +

Yeah, I agree this is ugly. Just re-definining MODULE_PARAM_PREFIX at
the end of the file should do the trick (followed by
"module_param(nr_uarts, uint, 0644)").

>  module_param(skip_txen_test, uint, 0644);
>  MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init time");

Well, we should do that for all the parameters, right? Sigh.

thanks,
-- 
js
suse labs
--
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/