8250.nr_uarts broken in 3.7

8250.nr_uarts broken in 3.7

[Josh Boyer]

Hi All,

We've had a report [1] in Fedora that the nr_uarts parameter on the 8250
serial driver is no longer working.  Our config hasn't changed in this
area in quite a while, and dmesg clearly shows setting 8250.nr_uarts=16
on the kernel command line results in:

[    0.345459] Serial: 8250/16550 driver, 16 ports, IRQ sharing enabled

whereas if you boot a 3.7 kernel with the same setting it prints out the
default value of 4 for ports.  Fedora builds 8250 into the kernel and
not as a module, but the option is still clearly parsed in that case for
3.6.

I've looked through the commits from 3.6..3.7 in the 8250 driver and the
only one that seems relevant at all is

commit 835d844d1a28efba81d5aca7385e24c29d3a6db2
Author: Sean Young <sean@mess.org>
Date:   Fri Sep 7 19:06:23 2012 +0100

    8250_pnp: do pnp probe before legacy probe

but that is simply moving code around.

I ran the same test on a 3.9-rc1 kernel and it isn't being picked up
there either.  Before I go off and do a bisect to track this down, has
anyone else seen or heard of this no longer working?

josh

Re: 8250.nr_uarts broken in 3.7

[Josh Boyer]

On Thu, Mar 07, 2013 at 01:56:42PM -0500, Josh Boyer wrote:
> Hi All,
> 
> We've had a report [1] in Fedora that the nr_uarts parameter on the 8250

Sigh.  Link to the report:

https://bugzilla.redhat.com/show_bug.cgi?id=911771

josh

Re: 8250.nr_uarts broken in 3.7

[Jiri Slaby]

On 03/07/2013 07:56 PM, Josh Boyer wrote:
> commit 835d844d1a28efba81d5aca7385e24c29d3a6db2
> Author: Sean Young <sean@mess.org>
> Date:   Fri Sep 7 19:06:23 2012 +0100
> 
>     8250_pnp: do pnp probe before legacy probe
> 
> but that is simply moving code around.

Hi, not quite. Does it still happen when you revert that one on the top
of 3.[789]*?

> I ran the same test on a 3.9-rc1 kernel and it isn't being picked up
> there either.  Before I go off and do a bisect to track this down, has
> anyone else seen or heard of this no longer working?

I haven't heard of that yet.

thanks,
-- 
js
suse labs

Re: 8250.nr_uarts broken in 3.7

[Josh Boyer]

On Thu, Mar 07, 2013 at 08:07:23PM +0100, Jiri Slaby wrote:
> On 03/07/2013 07:56 PM, Josh Boyer wrote:
> > commit 835d844d1a28efba81d5aca7385e24c29d3a6db2
> > Author: Sean Young <sean@mess.org>
> > Date:   Fri Sep 7 19:06:23 2012 +0100
> > 
> >     8250_pnp: do pnp probe before legacy probe
> > 
> > but that is simply moving code around.
> 
> Hi, not quite. Does it still happen when you revert that one on the top
> of 3.[789]*?

That was going to be my first attempt.  I'll let you know how it goes.

> > I ran the same test on a 3.9-rc1 kernel and it isn't being picked up
> > there either.  Before I go off and do a bisect to track this down, has
> > anyone else seen or heard of this no longer working?
> 
> I haven't heard of that yet.

Oh fun!  New bugs to fix.  I always like it when I'm not wasting my time
chasing something that was already solved ;).

josh

Re: 8250.nr_uarts broken in 3.7

[Josh Boyer]

On Thu, Mar 07, 2013 at 02:10:05PM -0500, Josh Boyer wrote:
> On Thu, Mar 07, 2013 at 08:07:23PM +0100, Jiri Slaby wrote:
> > On 03/07/2013 07:56 PM, Josh Boyer wrote:
> > > commit 835d844d1a28efba81d5aca7385e24c29d3a6db2
> > > Author: Sean Young <sean@mess.org>
> > > Date:   Fri Sep 7 19:06:23 2012 +0100
> > > 
> > >     8250_pnp: do pnp probe before legacy probe
> > > 
> > > but that is simply moving code around.
> > 
> > Hi, not quite. Does it still happen when you revert that one on the top
> > of 3.[789]*?
> 
> That was going to be my first attempt.  I'll let you know how it goes.

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.

josh

Re: 8250.nr_uarts broken in 3.7

[Greg Kroah-Hartman]

On Thu, Mar 07, 2013 at 04:14:27PM -0500, Josh Boyer wrote:
> On Thu, Mar 07, 2013 at 02:10:05PM -0500, Josh Boyer wrote:
> > On Thu, Mar 07, 2013 at 08:07:23PM +0100, Jiri Slaby wrote:
> > > On 03/07/2013 07:56 PM, Josh Boyer wrote:
> > > > commit 835d844d1a28efba81d5aca7385e24c29d3a6db2
> > > > Author: Sean Young <sean@mess.org>
> > > > Date:   Fri Sep 7 19:06:23 2012 +0100
> > > > 
> > > >     8250_pnp: do pnp probe before legacy probe
> > > > 
> > > > but that is simply moving code around.
> > > 
> > > Hi, not quite. Does it still happen when you revert that one on the top
> > > of 3.[789]*?
> > 
> > That was going to be my first attempt.  I'll let you know how it goes.
> 
> 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.

thanks,

greg k-h

Re: 8250.nr_uarts broken in 3.7

[Josh Boyer]

On Fri, Mar 08, 2013 at 07:12:42AM +0800, Greg Kroah-Hartman wrote:
> On Thu, Mar 07, 2013 at 04:14:27PM -0500, Josh Boyer wrote:
> > On Thu, Mar 07, 2013 at 02:10:05PM -0500, Josh Boyer wrote:
> > > On Thu, Mar 07, 2013 at 08:07:23PM +0100, Jiri Slaby wrote:
> > > > On 03/07/2013 07:56 PM, Josh Boyer wrote:
> > > > > commit 835d844d1a28efba81d5aca7385e24c29d3a6db2
> > > > > Author: Sean Young <sean@mess.org>
> > > > > Date:   Fri Sep 7 19:06:23 2012 +0100
> > > > > 
> > > > >     8250_pnp: do pnp probe before legacy probe
> > > > > 
> > > > > but that is simply moving code around.
> > > > 
> > > > Hi, not quite. Does it still happen when you revert that one on the top
> > > > of 3.[789]*?
> > > 
> > > That was going to be my first attempt.  I'll let you know how it goes.
> > 
> > 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?

josh

Re: 8250.nr_uarts broken in 3.7

[Greg Kroah-Hartman]

On Thu, Mar 07, 2013 at 08:01:40PM -0500, Josh Boyer wrote:
> On Fri, Mar 08, 2013 at 07:12:42AM +0800, Greg Kroah-Hartman wrote:
> > On Thu, Mar 07, 2013 at 04:14:27PM -0500, Josh Boyer wrote:
> > > On Thu, Mar 07, 2013 at 02:10:05PM -0500, Josh Boyer wrote:
> > > > On Thu, Mar 07, 2013 at 08:07:23PM +0100, Jiri Slaby wrote:
> > > > > On 03/07/2013 07:56 PM, Josh Boyer wrote:
> > > > > > commit 835d844d1a28efba81d5aca7385e24c29d3a6db2
> > > > > > Author: Sean Young <sean@mess.org>
> > > > > > Date:   Fri Sep 7 19:06:23 2012 +0100
> > > > > > 
> > > > > >     8250_pnp: do pnp probe before legacy probe
> > > > > > 
> > > > > > but that is simply moving code around.
> > > > > 
> > > > > Hi, not quite. Does it still happen when you revert that one on the top
> > > > > of 3.[789]*?
> > > > 
> > > > That was going to be my first attempt.  I'll let you know how it goes.
> > > 
> > > 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.

thanks,

greg k-h

Re: 8250.nr_uarts broken in 3.7

[Josh Boyer]

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@redhat.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@redhat.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 } };
+
 module_param(skip_txen_test, uint, 0644);
 MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init time");
 
-- 
1.8.1.2

Re: 8250.nr_uarts broken in 3.7

[Jiri Slaby]

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@redhat.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@redhat.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

Re: 8250.nr_uarts broken in 3.7

[Josh Boyer]

On Fri, Mar 08, 2013 at 11:47:01PM +0100, Jiri Slaby wrote:
> 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)").

For some reason, I thought I had tried that.  Maybe I didn't.  I'll look
into it again.

> >  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.

Yeah.

josh

Re: 8250.nr_uarts broken in 3.7

[Jiri Slaby]

On 03/08/2013 11:49 PM, Josh Boyer wrote:
> On Fri, Mar 08, 2013 at 11:47:01PM +0100, Jiri Slaby wrote:
>> 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)").
> 
> For some reason, I thought I had tried that.  Maybe I didn't.  I'll look
> into it again.

I see. Because we would re-define some global variables. What if we put
module_param into a function?

-- 
js
suse labs

Re: 8250.nr_uarts broken in 3.7

[Jiri Slaby]

On 03/08/2013 11:58 PM, Jiri Slaby wrote:
> On 03/08/2013 11:49 PM, Josh Boyer wrote:
>> On Fri, Mar 08, 2013 at 11:47:01PM +0100, Jiri Slaby wrote:
>>> 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)").
>>
>> For some reason, I thought I had tried that.  Maybe I didn't.  I'll look
>> into it again.
> 
> I see. Because we would re-define some global variables. What if we put
> module_param into a function?

Something like this?
#ifdef MODULE
static void __unused splat(void) {
#       undef MODULE_PARAM_PREFIX
#       define MODULE_PARAM_PREFIX "8250."
        module_param_cb(nr_uarts, &param_ops_uint, &nr_uarts, 0644);
        ...
}
#endif

Not nice, but should work. The other way is to have those in a separate
file linked to 8250 (to avoid re-definition errors).

thanks,
-- 
js
suse labs

Re: 8250.nr_uarts broken in 3.7

[Josh Boyer]

On Fri, Mar 08, 2013 at 11:58:39PM +0100, Jiri Slaby wrote:
> On 03/08/2013 11:49 PM, Josh Boyer wrote:
> > On Fri, Mar 08, 2013 at 11:47:01PM +0100, Jiri Slaby wrote:
> >> 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)").
> > 
> > For some reason, I thought I had tried that.  Maybe I didn't.  I'll look
> > into it again.
> 
> I see. Because we would re-define some global variables. What if we put

Right.  For the peanut gallery, you get this error:

CC      drivers/tty/serial/8250/8250.o
drivers/tty/serial/8250/8250.c:3351:1: error: redefinition of ‘__check_share_irqs’
drivers/tty/serial/8250/8250.c:3333:1: note: previous definition of ‘__check_share_irqs’ was here
drivers/tty/serial/8250/8250.c:3351:1: error: redefinition of ‘__param_perm_check_share_irqs’
drivers/tty/serial/8250/8250.c:3333:1: note: previous definition of ‘__param_perm_check_share_irqs’ was here
drivers/tty/serial/8250/8250.c:3351:1: error: redefinition of ‘__param_str_share_irqs’
drivers/tty/serial/8250/8250.c:3333:1: note: previous definition of ‘__param_str_share_irqs’ was here
drivers/tty/serial/8250/8250.c:3351:1: error: redefinition of ‘__param_share_irqs’
drivers/tty/serial/8250/8250.c:3333:1: note: previous definition of ‘__param_share_irqs’ was here

for each variable you redefine like that.

> module_param into a function?

Not sure what you mean by that.

josh

Re: 8250.nr_uarts broken in 3.7

[Josh Boyer]

On Fri, Mar 08, 2013 at 11:58:39PM +0100, Jiri Slaby wrote:
> On 03/08/2013 11:49 PM, Josh Boyer wrote:
> > On Fri, Mar 08, 2013 at 11:47:01PM +0100, Jiri Slaby wrote:
> >> 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)").
> > 
> > For some reason, I thought I had tried that.  Maybe I didn't.  I'll look
> > into it again.
> 
> I see. Because we would re-define some global variables. What if we put

Right.  For the peanut gallery, you get this error:

CC      drivers/tty/serial/8250/8250.o
drivers/tty/serial/8250/8250.c:3351:1: error: redefinition of ‘__check_share_irqs’
drivers/tty/serial/8250/8250.c:3333:1: note: previous definition of ‘__check_share_irqs’ was here
drivers/tty/serial/8250/8250.c:3351:1: error: redefinition of ‘__param_perm_check_share_irqs’
drivers/tty/serial/8250/8250.c:3333:1: note: previous definition of ‘__param_perm_check_share_irqs’ was here
drivers/tty/serial/8250/8250.c:3351:1: error: redefinition of ‘__param_str_share_irqs’
drivers/tty/serial/8250/8250.c:3333:1: note: previous definition of ‘__param_str_share_irqs’ was here
drivers/tty/serial/8250/8250.c:3351:1: error: redefinition of ‘__param_share_irqs’
drivers/tty/serial/8250/8250.c:3333:1: note: previous definition of ‘__param_share_irqs’ was here

for each variable you redefine like that.

> module_param into a function?

Not sure what you mean by that.

josh
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 8250.nr_uarts broken in 3.7

[Jiri Slaby]

On 03/09/2013 12:10 AM, Jiri Slaby wrote:
> On 03/08/2013 11:58 PM, Jiri Slaby wrote:
>> On 03/08/2013 11:49 PM, Josh Boyer wrote:
>>> On Fri, Mar 08, 2013 at 11:47:01PM +0100, Jiri Slaby wrote:
>>>> 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)").
>>>
>>> For some reason, I thought I had tried that.  Maybe I didn't.  I'll look
>>> into it again.
>>
>> I see. Because we would re-define some global variables. What if we put
>> module_param into a function?
> 
> Something like this?
> #ifdef MODULE
> static void __unused splat(void) {

I meant __used. It should make no difference though.

> #       undef MODULE_PARAM_PREFIX
> #       define MODULE_PARAM_PREFIX "8250."
>         module_param_cb(nr_uarts, &param_ops_uint, &nr_uarts, 0644);
>         ...
> }
> #endif
> 
> Not nice, but should work. The other way is to have those in a separate
> file linked to 8250 (to avoid re-definition errors).
> 
> thanks,
-- 
js
suse labs

Re: 8250.nr_uarts broken in 3.7

[Josh Boyer]

On Sat, Mar 09, 2013 at 12:14:23AM +0100, Jiri Slaby wrote:
> On 03/09/2013 12:10 AM, Jiri Slaby wrote:
> > On 03/08/2013 11:58 PM, Jiri Slaby wrote:
> >> On 03/08/2013 11:49 PM, Josh Boyer wrote:
> >>> On Fri, Mar 08, 2013 at 11:47:01PM +0100, Jiri Slaby wrote:
> >>>> 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)").
> >>>
> >>> For some reason, I thought I had tried that.  Maybe I didn't.  I'll look
> >>> into it again.
> >>
> >> I see. Because we would re-define some global variables. What if we put
> >> module_param into a function?
> > 
> > Something like this?
> > #ifdef MODULE

I don't think you want this surrounded in #ifdef MODULE, do you?  That
won't let people building the driver into the kernel continue to use
8250.<whatever> on the kernel command line.

> > static void __unused splat(void) {
> 
> I meant __used. It should make no difference though.
> 
> > #       undef MODULE_PARAM_PREFIX
> > #       define MODULE_PARAM_PREFIX "8250."
> >         module_param_cb(nr_uarts, &param_ops_uint, &nr_uarts, 0644);
> >         ...
> > }
> > #endif
> > 
> > Not nice, but should work. The other way is to have those in a separate
> > file linked to 8250 (to avoid re-definition errors).

Ew.  I'll try the function first.

josh

Re: 8250.nr_uarts broken in 3.7

[Josh Boyer]

On Fri, Mar 08, 2013 at 06:28:09PM -0500, Josh Boyer wrote:
> On Sat, Mar 09, 2013 at 12:14:23AM +0100, Jiri Slaby wrote:
> > On 03/09/2013 12:10 AM, Jiri Slaby wrote:
> > > On 03/08/2013 11:58 PM, Jiri Slaby wrote:
> > >> On 03/08/2013 11:49 PM, Josh Boyer wrote:
> > >>> On Fri, Mar 08, 2013 at 11:47:01PM +0100, Jiri Slaby wrote:
> > >>>> 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)").
> > >>>
> > >>> For some reason, I thought I had tried that.  Maybe I didn't.  I'll look
> > >>> into it again.
> > >>
> > >> I see. Because we would re-define some global variables. What if we put
> > >> module_param into a function?
> > > 
> > > Something like this?
> > > #ifdef MODULE
> 
> I don't think you want this surrounded in #ifdef MODULE, do you?  That
> won't let people building the driver into the kernel continue to use
> 8250.<whatever> on the kernel command line.
> 
> > > static void __unused splat(void) {
> > 
> > I meant __used. It should make no difference though.
> > 
> > > #       undef MODULE_PARAM_PREFIX
> > > #       define MODULE_PARAM_PREFIX "8250."
> > >         module_param_cb(nr_uarts, &param_ops_uint, &nr_uarts, 0644);
> > >         ...
> > > }
> > > #endif
> > > 
> > > Not nice, but should work. The other way is to have those in a separate
> > > file linked to 8250 (to avoid re-definition errors).
> 
> Ew.  I'll try the function first.

OK, the function (without the surrounding ifdef) seems to be working OK.
I'll do a bit more testing and send out a v2 in a bit.

Thanks for the tip.  It's still not pretty, but at least I don't feel
ashamed about it.

josh

Re: 8250.nr_uarts broken in 3.7

[Jiri Slaby]

On 03/09/2013 12:44 AM, Josh Boyer wrote:
> On Fri, Mar 08, 2013 at 06:28:09PM -0500, Josh Boyer wrote:
>> On Sat, Mar 09, 2013 at 12:14:23AM +0100, Jiri Slaby wrote:
>>> On 03/09/2013 12:10 AM, Jiri Slaby wrote:
>>>> On 03/08/2013 11:58 PM, Jiri Slaby wrote:
>>>>> On 03/08/2013 11:49 PM, Josh Boyer wrote:
>>>>>> On Fri, Mar 08, 2013 at 11:47:01PM +0100, Jiri Slaby wrote:
>>>>>>> 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)").
>>>>>>
>>>>>> For some reason, I thought I had tried that.  Maybe I didn't.  I'll look
>>>>>> into it again.
>>>>>
>>>>> I see. Because we would re-define some global variables. What if we put
>>>>> module_param into a function?
>>>>
>>>> Something like this?
>>>> #ifdef MODULE
>>
>> I don't think you want this surrounded in #ifdef MODULE, do you?  That
>> won't let people building the driver into the kernel continue to use
>> 8250.<whatever> on the kernel command line.

Yes, you're right. I sort of thought the prefix is not used for
non-modules. But it is.

>>>> static void __unused splat(void) {
>>>
>>> I meant __used. It should make no difference though.
>>>
>>>> #       undef MODULE_PARAM_PREFIX
>>>> #       define MODULE_PARAM_PREFIX "8250."
>>>>         module_param_cb(nr_uarts, &param_ops_uint, &nr_uarts, 0644);
>>>>         ...
>>>> }
>>>> #endif
>>>>
>>>> Not nice, but should work. The other way is to have those in a separate
>>>> file linked to 8250 (to avoid re-definition errors).
>>
>> Ew.  I'll try the function first.
> 
> OK, the function (without the surrounding ifdef) seems to be working OK.
> I'll do a bit more testing and send out a v2 in a bit.
> 
> Thanks for the tip.  It's still not pretty, but at least I don't feel
> ashamed about it.

I'm thinking about deprecating the 8250_core.* in some way. Better
sooner than later. The view of having both of them with that hack in the
kernel forever drives me bananas.

-- 
js
suse labs

Re: 8250.nr_uarts broken in 3.7

[Josh Boyer]

On Sat, Mar 09, 2013 at 10:14:14AM +0100, Jiri Slaby wrote:
> On 03/09/2013 12:44 AM, Josh Boyer wrote:
> > On Fri, Mar 08, 2013 at 06:28:09PM -0500, Josh Boyer wrote:
> >> On Sat, Mar 09, 2013 at 12:14:23AM +0100, Jiri Slaby wrote:
> >>> On 03/09/2013 12:10 AM, Jiri Slaby wrote:
> >>>> On 03/08/2013 11:58 PM, Jiri Slaby wrote:
> >>>>> On 03/08/2013 11:49 PM, Josh Boyer wrote:
> >>>>>> On Fri, Mar 08, 2013 at 11:47:01PM +0100, Jiri Slaby wrote:
> >>>>>>> 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)").
> >>>>>>
> >>>>>> For some reason, I thought I had tried that.  Maybe I didn't.  I'll look
> >>>>>> into it again.
> >>>>>
> >>>>> I see. Because we would re-define some global variables. What if we put
> >>>>> module_param into a function?
> >>>>
> >>>> Something like this?
> >>>> #ifdef MODULE
> >>
> >> I don't think you want this surrounded in #ifdef MODULE, do you?  That
> >> won't let people building the driver into the kernel continue to use
> >> 8250.<whatever> on the kernel command line.
> 
> Yes, you're right. I sort of thought the prefix is not used for
> non-modules. But it is.

Right.  It's used when built-in, and it's used by modprobe if it's a module
but the user passed the parameters on the kernel command line.

We actually need the opposite #ifndef MODULE in place, otherwise the .ko
winds up with e.g. "nr_uarts" and "8250.nr_uarts".  That is a module
option named that, not something that would parse as expected.

> >>>> static void __unused splat(void) {
> >>>
> >>> I meant __used. It should make no difference though.
> >>>
> >>>> #       undef MODULE_PARAM_PREFIX
> >>>> #       define MODULE_PARAM_PREFIX "8250."
> >>>>         module_param_cb(nr_uarts, &param_ops_uint, &nr_uarts, 0644);
> >>>>         ...
> >>>> }
> >>>> #endif
> >>>>
> >>>> Not nice, but should work. The other way is to have those in a separate
> >>>> file linked to 8250 (to avoid re-definition errors).
> >>
> >> Ew.  I'll try the function first.
> > 
> > OK, the function (without the surrounding ifdef) seems to be working OK.
> > I'll do a bit more testing and send out a v2 in a bit.
> > 
> > Thanks for the tip.  It's still not pretty, but at least I don't feel
> > ashamed about it.
> 
> I'm thinking about deprecating the 8250_core.* in some way. Better
> sooner than later. The view of having both of them with that hack in the
> kernel forever drives me bananas.

I don't disagree.  I did finish up the patch, and this is what I came up
with.

josh

From: Josh Boyer <jwboyer@redhat.com>
Date: Fri, 8 Mar 2013 21:13:52 -0500
Subject: [PATCH] serial: 8250: Keep 8250.<xxxx> module options functional
 after driver rename

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
the 8259.<xxxx> module parameters or as a kernel command line switch is
now broken, as the 8250_core driver doesn't parse options belonging to
something called "8250".

To solve this, we redefine the module options in a dummy function using
a redefined MODULE_PARAM_PREFX when built into the kernel.  In the case
where we're building as a module, we provide an alias to the old 8250
name.  The dummy function prevents compiler errors due to global variable
redefinitions that happen as part of the module_param_ macro expansions.

Signed-off-by: Josh Boyer <jwboyer@redhat.com>
---
 drivers/tty/serial/8250/8250.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
index 0efc815..446beb7 100644
--- a/drivers/tty/serial/8250/8250.c
+++ b/drivers/tty/serial/8250/8250.c
@@ -3396,3 +3396,34 @@ module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
 MODULE_PARM_DESC(probe_rsa, "Probe I/O ports for RSA");
 #endif
 MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
+
+#ifndef MODULE
+/* This module was renamed to 8250_core in 3.7.  Keep the old "8250" name
+ * working as well for the module options so we don't break people.  We
+ * need to keep the names identical and the convenient macros will happily
+ * refuse to let us do that by failing the build with redefinition errors
+ * of global variables.  So we stick them inside a dummy function to avoid
+ * those conflicts.  The options still get parsed, and the redefined
+ * MODULE_PARAM_PREFIX lets us keep the "8250." syntax alive.  We redefine
+ * __param_check to a throw away value to avoid type conflicts from the
+ * expansion that would happen otherwise.
+ *
+ * This is hacky.  I'm sorry.
+ */
+static void __used s8250_options(void)
+{
+#undef MODULE_PARAM_PREFIX
+#define MODULE_PARAM_PREFIX "8250."
+
+	module_param_cb(share_irqs, &param_ops_uint, &share_irqs, 0644);
+	module_param_cb(nr_uarts, &param_ops_uint, &nr_uarts, 0644);
+	module_param_cb(skip_txen_test, &param_ops_uint, &skip_txen_test, 0644);
+#ifdef CONFIG_SERIAL_8250_RSA
+#undef __param_check
+#define __param_check(name, p, type) int __attribute((unused)) tmp
+	module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
+#endif
+}
+#else
+MODULE_ALIAS("8250");
+#endif
-- 
1.8.1.2

Re: 8250.nr_uarts broken in 3.7

[Jiri Slaby]

On 03/09/2013 02:30 PM, Josh Boyer wrote:
> From: Josh Boyer <jwboyer@redhat.com>
> Date: Fri, 8 Mar 2013 21:13:52 -0500
> Subject: [PATCH] serial: 8250: Keep 8250.<xxxx> module options functional
>  after driver rename
> 
> 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
> the 8259.<xxxx> module parameters or as a kernel command line switch is
> now broken, as the 8250_core driver doesn't parse options belonging to
> something called "8250".
> 
> To solve this, we redefine the module options in a dummy function using
> a redefined MODULE_PARAM_PREFX when built into the kernel.  In the case
> where we're building as a module, we provide an alias to the old 8250
> name.  The dummy function prevents compiler errors due to global variable
> redefinitions that happen as part of the module_param_ macro expansions.
> 
> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> ---
>  drivers/tty/serial/8250/8250.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
> index 0efc815..446beb7 100644
> --- a/drivers/tty/serial/8250/8250.c
> +++ b/drivers/tty/serial/8250/8250.c
> @@ -3396,3 +3396,34 @@ module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
>  MODULE_PARM_DESC(probe_rsa, "Probe I/O ports for RSA");
>  #endif
>  MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
> +
> +#ifndef MODULE
> +/* This module was renamed to 8250_core in 3.7.  Keep the old "8250" name
> + * working as well for the module options so we don't break people.  We
> + * need to keep the names identical and the convenient macros will happily
> + * refuse to let us do that by failing the build with redefinition errors
> + * of global variables.  So we stick them inside a dummy function to avoid
> + * those conflicts.  The options still get parsed, and the redefined
> + * MODULE_PARAM_PREFIX lets us keep the "8250." syntax alive.  We redefine
> + * __param_check to a throw away value to avoid type conflicts from the
> + * expansion that would happen otherwise.
> + *
> + * This is hacky.  I'm sorry.
> + */
> +static void __used s8250_options(void)
> +{
> +#undef MODULE_PARAM_PREFIX
> +#define MODULE_PARAM_PREFIX "8250."
> +
> +	module_param_cb(share_irqs, &param_ops_uint, &share_irqs, 0644);
> +	module_param_cb(nr_uarts, &param_ops_uint, &nr_uarts, 0644);
> +	module_param_cb(skip_txen_test, &param_ops_uint, &skip_txen_test, 0644);
> +#ifdef CONFIG_SERIAL_8250_RSA
> +#undef __param_check
> +#define __param_check(name, p, type) int __attribute((unused)) tmp
> +	module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);

Aiee, we havo no _cb for arrays. But we can do just:
__module_param_call(MODULE_PARAM_PREFIX, probe_rsa,
       &param_array_ops, .arr = &__param_arr_probe_rsa,
       0444, -1);

> +#endif
> +}
> +#else
> +MODULE_ALIAS("8250");

This is so disgusting that I will do the following:
* ack your patch after you change the above (if that works)
* rename 8250.c to 8250_core.c
* change 8250_core.ko back to 8250.ko (ie. bring back the old module name)
* thus switch MODULE_PARAM_PREFIX above to "8250_core."
* deprecate all the newly added 8250_core.* params somehow and schedule
for removal. IMO this warrants a kernel config option like
CONFIG_8250_DEPRECATED_MODULE_PARAMS.

We have a lesson learned.

thanks,
-- 
js
suse labs

Re: 8250.nr_uarts broken in 3.7

[Josh Boyer]

On Sat, Mar 09, 2013 at 03:14:18PM +0100, Jiri Slaby wrote:
> On 03/09/2013 02:30 PM, Josh Boyer wrote:
> > From: Josh Boyer <jwboyer@redhat.com>
> > Date: Fri, 8 Mar 2013 21:13:52 -0500
> > Subject: [PATCH] serial: 8250: Keep 8250.<xxxx> module options functional
> >  after driver rename
> > 
> > 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
> > the 8259.<xxxx> module parameters or as a kernel command line switch is
> > now broken, as the 8250_core driver doesn't parse options belonging to
> > something called "8250".
> > 
> > To solve this, we redefine the module options in a dummy function using
> > a redefined MODULE_PARAM_PREFX when built into the kernel.  In the case
> > where we're building as a module, we provide an alias to the old 8250
> > name.  The dummy function prevents compiler errors due to global variable
> > redefinitions that happen as part of the module_param_ macro expansions.
> > 
> > Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> > ---
> >  drivers/tty/serial/8250/8250.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
> > index 0efc815..446beb7 100644
> > --- a/drivers/tty/serial/8250/8250.c
> > +++ b/drivers/tty/serial/8250/8250.c
> > @@ -3396,3 +3396,34 @@ module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
> >  MODULE_PARM_DESC(probe_rsa, "Probe I/O ports for RSA");
> >  #endif
> >  MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
> > +
> > +#ifndef MODULE
> > +/* This module was renamed to 8250_core in 3.7.  Keep the old "8250" name
> > + * working as well for the module options so we don't break people.  We
> > + * need to keep the names identical and the convenient macros will happily
> > + * refuse to let us do that by failing the build with redefinition errors
> > + * of global variables.  So we stick them inside a dummy function to avoid
> > + * those conflicts.  The options still get parsed, and the redefined
> > + * MODULE_PARAM_PREFIX lets us keep the "8250." syntax alive.  We redefine
> > + * __param_check to a throw away value to avoid type conflicts from the
> > + * expansion that would happen otherwise.
> > + *
> > + * This is hacky.  I'm sorry.
> > + */
> > +static void __used s8250_options(void)
> > +{
> > +#undef MODULE_PARAM_PREFIX
> > +#define MODULE_PARAM_PREFIX "8250."
> > +
> > +	module_param_cb(share_irqs, &param_ops_uint, &share_irqs, 0644);
> > +	module_param_cb(nr_uarts, &param_ops_uint, &nr_uarts, 0644);
> > +	module_param_cb(skip_txen_test, &param_ops_uint, &skip_txen_test, 0644);
> > +#ifdef CONFIG_SERIAL_8250_RSA
> > +#undef __param_check
> > +#define __param_check(name, p, type) int __attribute((unused)) tmp
> > +	module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
> 
> Aiee, we havo no _cb for arrays. But we can do just:
> __module_param_call(MODULE_PARAM_PREFIX, probe_rsa,
>        &param_array_ops, .arr = &__param_arr_probe_rsa,
>        0444, -1);

Right.  I can change that.  I just went for the simpler fix.

> > +#endif
> > +}
> > +#else
> > +MODULE_ALIAS("8250");
> 
> This is so disgusting that I will do the following:
> * ack your patch after you change the above (if that works)
> * rename 8250.c to 8250_core.c
> * change 8250_core.ko back to 8250.ko (ie. bring back the old module name)
> * thus switch MODULE_PARAM_PREFIX above to "8250_core."
> * deprecate all the newly added 8250_core.* params somehow and schedule
> for removal. IMO this warrants a kernel config option like
> CONFIG_8250_DEPRECATED_MODULE_PARAMS.
> 
> We have a lesson learned.

Heh, ok.  That all sounds fine.  I'll try to make the small revision
tonight and test it.

josh

Re: 8250.nr_uarts broken in 3.7

[Sean Young]

On Sat, Mar 09, 2013 at 03:14:18PM +0100, Jiri Slaby wrote:
> This is so disgusting that I will do the following:
> * ack your patch after you change the above (if that works)
> * rename 8250.c to 8250_core.c
> * change 8250_core.ko back to 8250.ko (ie. bring back the old module name)
> * thus switch MODULE_PARAM_PREFIX above to "8250_core."
> * deprecate all the newly added 8250_core.* params somehow and schedule
> for removal. IMO this warrants a kernel config option like
> CONFIG_8250_DEPRECATED_MODULE_PARAMS.
> 
> We have a lesson learned.

Indeed. In the patch that broke things, I renamed the module rather than 
the 8250.c source file in order to keep the patch small, but obviously 
did not consider the consequences well enough. I'm sorry about the mess 
I created.


Sean

[PATCH v2] serial: 8250: Keep 8250.<xxxx> module options functional after driver rename

[Josh Boyer]

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
the 8259.<xxxx> module parameters or as a kernel command line switch is
now broken, as the 8250_core driver doesn't parse options belonging to
something called "8250".

To solve this, we redefine the module options in a dummy function using
a redefined MODULE_PARAM_PREFX when built into the kernel.  In the case
where we're building as a module, we provide an alias to the old 8250
name.  The dummy function prevents compiler errors due to global variable
redefinitions that happen as part of the module_param_ macro expansions.

Signed-off-by: Josh Boyer <jwboyer@redhat.com>
---
 v2: Use __module_param_call for probe_rsa instead of undefining the
     __param_check macro as suggested by Jiri Slaby

 drivers/tty/serial/8250/8250.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
index 0efc815..f982633 100644
--- a/drivers/tty/serial/8250/8250.c
+++ b/drivers/tty/serial/8250/8250.c
@@ -3396,3 +3396,32 @@ module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
 MODULE_PARM_DESC(probe_rsa, "Probe I/O ports for RSA");
 #endif
 MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
+
+#ifndef MODULE
+/* This module was renamed to 8250_core in 3.7.  Keep the old "8250" name
+ * working as well for the module options so we don't break people.  We
+ * need to keep the names identical and the convenient macros will happily
+ * refuse to let us do that by failing the build with redefinition errors
+ * of global variables.  So we stick them inside a dummy function to avoid
+ * those conflicts.  The options still get parsed, and the redefined
+ * MODULE_PARAM_PREFIX lets us keep the "8250." syntax alive.
+ *
+ * This is hacky.  I'm sorry.
+ */
+static void __used s8250_options(void)
+{
+#undef MODULE_PARAM_PREFIX
+#define MODULE_PARAM_PREFIX "8250."
+
+	module_param_cb(share_irqs, &param_ops_uint, &share_irqs, 0644);
+	module_param_cb(nr_uarts, &param_ops_uint, &nr_uarts, 0644);
+	module_param_cb(skip_txen_test, &param_ops_uint, &skip_txen_test, 0644);
+#ifdef CONFIG_SERIAL_8250_RSA
+	__module_param_call(MODULE_PARAM_PREFIX, probe_rsa,
+		&param_array_ops, .arr = &__param_arr_probe_rsa,
+		0444, -1);
+#endif
+}
+#else
+MODULE_ALIAS("8250");
+#endif
-- 
1.8.1.2

Re: [PATCH v2] serial: 8250: Keep 8250.<xxxx> module options functional after driver rename

[Jiri Slaby]

On 03/10/2013 03:33 PM, Josh Boyer wrote:
> 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
> the 8259.<xxxx> module parameters or as a kernel command line switch is
> now broken, as the 8250_core driver doesn't parse options belonging to
> something called "8250".
> 
> To solve this, we redefine the module options in a dummy function using
> a redefined MODULE_PARAM_PREFX when built into the kernel.  In the case
> where we're building as a module, we provide an alias to the old 8250
> name.  The dummy function prevents compiler errors due to global variable
> redefinitions that happen as part of the module_param_ macro expansions.
> 
> Signed-off-by: Josh Boyer <jwboyer@redhat.com>

Despite the inevitable ugly nature of the approach:

Acked-by: Jiri Slaby <jslaby@suse.cz>

> ---
>  v2: Use __module_param_call for probe_rsa instead of undefining the
>      __param_check macro as suggested by Jiri Slaby
> 
>  drivers/tty/serial/8250/8250.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
> index 0efc815..f982633 100644
> --- a/drivers/tty/serial/8250/8250.c
> +++ b/drivers/tty/serial/8250/8250.c
> @@ -3396,3 +3396,32 @@ module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
>  MODULE_PARM_DESC(probe_rsa, "Probe I/O ports for RSA");
>  #endif
>  MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
> +
> +#ifndef MODULE
> +/* This module was renamed to 8250_core in 3.7.  Keep the old "8250" name
> + * working as well for the module options so we don't break people.  We
> + * need to keep the names identical and the convenient macros will happily
> + * refuse to let us do that by failing the build with redefinition errors
> + * of global variables.  So we stick them inside a dummy function to avoid
> + * those conflicts.  The options still get parsed, and the redefined
> + * MODULE_PARAM_PREFIX lets us keep the "8250." syntax alive.
> + *
> + * This is hacky.  I'm sorry.
> + */
> +static void __used s8250_options(void)
> +{
> +#undef MODULE_PARAM_PREFIX
> +#define MODULE_PARAM_PREFIX "8250."
> +
> +	module_param_cb(share_irqs, &param_ops_uint, &share_irqs, 0644);
> +	module_param_cb(nr_uarts, &param_ops_uint, &nr_uarts, 0644);
> +	module_param_cb(skip_txen_test, &param_ops_uint, &skip_txen_test, 0644);
> +#ifdef CONFIG_SERIAL_8250_RSA
> +	__module_param_call(MODULE_PARAM_PREFIX, probe_rsa,
> +		&param_array_ops, .arr = &__param_arr_probe_rsa,
> +		0444, -1);
> +#endif
> +}
> +#else
> +MODULE_ALIAS("8250");
> +#endif
> 


-- 
js
suse labs