* 8250.nr_uarts broken in 3.7
@ 2013-03-07 18:56 Josh Boyer
2013-03-07 18:58 ` Josh Boyer
2013-03-07 19:07 ` Jiri Slaby
0 siblings, 2 replies; 25+ messages in thread
From: Josh Boyer @ 2013-03-07 18:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby; +Cc: linux-serial, linux-kernel
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: 8250.nr_uarts broken in 3.7 2013-03-07 18:56 8250.nr_uarts broken in 3.7 Josh Boyer @ 2013-03-07 18:58 ` Josh Boyer 2013-03-07 19:07 ` Jiri Slaby 1 sibling, 0 replies; 25+ messages in thread From: Josh Boyer @ 2013-03-07 18:58 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby; +Cc: linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-07 18:56 8250.nr_uarts broken in 3.7 Josh Boyer 2013-03-07 18:58 ` Josh Boyer @ 2013-03-07 19:07 ` Jiri Slaby 2013-03-07 19:10 ` Josh Boyer 1 sibling, 1 reply; 25+ messages in thread From: Jiri Slaby @ 2013-03-07 19:07 UTC (permalink / raw) To: Josh Boyer, Greg Kroah-Hartman; +Cc: linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-07 19:07 ` Jiri Slaby @ 2013-03-07 19:10 ` Josh Boyer 2013-03-07 21:14 ` Josh Boyer 0 siblings, 1 reply; 25+ messages in thread From: Josh Boyer @ 2013-03-07 19:10 UTC (permalink / raw) To: Jiri Slaby; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-07 19:10 ` Josh Boyer @ 2013-03-07 21:14 ` Josh Boyer 2013-03-07 23:12 ` Greg Kroah-Hartman 0 siblings, 1 reply; 25+ messages in thread From: Josh Boyer @ 2013-03-07 21:14 UTC (permalink / raw) To: Jiri Slaby; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-07 21:14 ` Josh Boyer @ 2013-03-07 23:12 ` Greg Kroah-Hartman 2013-03-08 1:01 ` Josh Boyer 0 siblings, 1 reply; 25+ messages in thread From: Greg Kroah-Hartman @ 2013-03-07 23:12 UTC (permalink / raw) To: Josh Boyer; +Cc: Jiri Slaby, linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-07 23:12 ` Greg Kroah-Hartman @ 2013-03-08 1:01 ` Josh Boyer 2013-03-08 1:39 ` Greg Kroah-Hartman 0 siblings, 1 reply; 25+ messages in thread From: Josh Boyer @ 2013-03-08 1:01 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Jiri Slaby, linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-08 1:01 ` Josh Boyer @ 2013-03-08 1:39 ` Greg Kroah-Hartman 2013-03-08 21:27 ` Josh Boyer 0 siblings, 1 reply; 25+ messages in thread From: Greg Kroah-Hartman @ 2013-03-08 1:39 UTC (permalink / raw) To: Josh Boyer; +Cc: Jiri Slaby, linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-08 1:39 ` Greg Kroah-Hartman @ 2013-03-08 21:27 ` Josh Boyer 2013-03-08 22:47 ` Jiri Slaby 0 siblings, 1 reply; 25+ messages in thread From: Josh Boyer @ 2013-03-08 21:27 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Jiri Slaby, linux-serial, linux-kernel 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", ¶m_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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-08 21:27 ` Josh Boyer @ 2013-03-08 22:47 ` Jiri Slaby 2013-03-08 22:49 ` Josh Boyer 0 siblings, 1 reply; 25+ messages in thread From: Jiri Slaby @ 2013-03-08 22:47 UTC (permalink / raw) To: Josh Boyer, Greg Kroah-Hartman; +Cc: linux-serial, linux-kernel 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", ¶m_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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-08 22:47 ` Jiri Slaby @ 2013-03-08 22:49 ` Josh Boyer 2013-03-08 22:58 ` Jiri Slaby 0 siblings, 1 reply; 25+ messages in thread From: Josh Boyer @ 2013-03-08 22:49 UTC (permalink / raw) To: Jiri Slaby; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-08 22:49 ` Josh Boyer @ 2013-03-08 22:58 ` Jiri Slaby 2013-03-08 23:10 ` Jiri Slaby 2013-03-08 23:11 ` Josh Boyer 0 siblings, 2 replies; 25+ messages in thread From: Jiri Slaby @ 2013-03-08 22:58 UTC (permalink / raw) To: Josh Boyer; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-08 22:58 ` Jiri Slaby @ 2013-03-08 23:10 ` Jiri Slaby 2013-03-08 23:14 ` Jiri Slaby 2013-03-08 23:11 ` Josh Boyer 1 sibling, 1 reply; 25+ messages in thread From: Jiri Slaby @ 2013-03-08 23:10 UTC (permalink / raw) To: Josh Boyer; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel 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, ¶m_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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-08 23:10 ` Jiri Slaby @ 2013-03-08 23:14 ` Jiri Slaby 2013-03-08 23:28 ` Josh Boyer 0 siblings, 1 reply; 25+ messages in thread From: Jiri Slaby @ 2013-03-08 23:14 UTC (permalink / raw) To: Josh Boyer; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel 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, ¶m_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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-08 23:14 ` Jiri Slaby @ 2013-03-08 23:28 ` Josh Boyer 2013-03-08 23:44 ` Josh Boyer 0 siblings, 1 reply; 25+ messages in thread From: Josh Boyer @ 2013-03-08 23:28 UTC (permalink / raw) To: Jiri Slaby; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel 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, ¶m_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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-08 23:28 ` Josh Boyer @ 2013-03-08 23:44 ` Josh Boyer 2013-03-09 9:14 ` Jiri Slaby 0 siblings, 1 reply; 25+ messages in thread From: Josh Boyer @ 2013-03-08 23:44 UTC (permalink / raw) To: Jiri Slaby; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel 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, ¶m_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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-08 23:44 ` Josh Boyer @ 2013-03-09 9:14 ` Jiri Slaby 2013-03-09 13:30 ` Josh Boyer 0 siblings, 1 reply; 25+ messages in thread From: Jiri Slaby @ 2013-03-09 9:14 UTC (permalink / raw) To: Josh Boyer; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel 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, ¶m_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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-09 9:14 ` Jiri Slaby @ 2013-03-09 13:30 ` Josh Boyer 2013-03-09 14:14 ` Jiri Slaby 0 siblings, 1 reply; 25+ messages in thread From: Josh Boyer @ 2013-03-09 13:30 UTC (permalink / raw) To: Jiri Slaby; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel 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, ¶m_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, ¶m_ops_uint, &share_irqs, 0644); + module_param_cb(nr_uarts, ¶m_ops_uint, &nr_uarts, 0644); + module_param_cb(skip_txen_test, ¶m_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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-09 13:30 ` Josh Boyer @ 2013-03-09 14:14 ` Jiri Slaby 2013-03-09 17:02 ` Josh Boyer 2013-03-10 12:21 ` 8250.nr_uarts broken in 3.7 Sean Young 0 siblings, 2 replies; 25+ messages in thread From: Jiri Slaby @ 2013-03-09 14:14 UTC (permalink / raw) To: Josh Boyer; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel 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, ¶m_ops_uint, &share_irqs, 0644); > + module_param_cb(nr_uarts, ¶m_ops_uint, &nr_uarts, 0644); > + module_param_cb(skip_txen_test, ¶m_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, ¶m_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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-09 14:14 ` Jiri Slaby @ 2013-03-09 17:02 ` Josh Boyer 2013-03-10 14:33 ` [PATCH v2] serial: 8250: Keep 8250.<xxxx> module options functional after driver rename Josh Boyer 2013-03-10 12:21 ` 8250.nr_uarts broken in 3.7 Sean Young 1 sibling, 1 reply; 25+ messages in thread From: Josh Boyer @ 2013-03-09 17:02 UTC (permalink / raw) To: Jiri Slaby; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel 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, ¶m_ops_uint, &share_irqs, 0644); > > + module_param_cb(nr_uarts, ¶m_ops_uint, &nr_uarts, 0644); > > + module_param_cb(skip_txen_test, ¶m_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, > ¶m_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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2] serial: 8250: Keep 8250.<xxxx> module options functional after driver rename 2013-03-09 17:02 ` Josh Boyer @ 2013-03-10 14:33 ` Josh Boyer 2013-03-10 22:33 ` Jiri Slaby 0 siblings, 1 reply; 25+ messages in thread From: Josh Boyer @ 2013-03-10 14:33 UTC (permalink / raw) To: Jiri Slaby; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel 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, ¶m_ops_uint, &share_irqs, 0644); + module_param_cb(nr_uarts, ¶m_ops_uint, &nr_uarts, 0644); + module_param_cb(skip_txen_test, ¶m_ops_uint, &skip_txen_test, 0644); +#ifdef CONFIG_SERIAL_8250_RSA + __module_param_call(MODULE_PARAM_PREFIX, probe_rsa, + ¶m_array_ops, .arr = &__param_arr_probe_rsa, + 0444, -1); +#endif +} +#else +MODULE_ALIAS("8250"); +#endif -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2] serial: 8250: Keep 8250.<xxxx> module options functional after driver rename 2013-03-10 14:33 ` [PATCH v2] serial: 8250: Keep 8250.<xxxx> module options functional after driver rename Josh Boyer @ 2013-03-10 22:33 ` Jiri Slaby 0 siblings, 0 replies; 25+ messages in thread From: Jiri Slaby @ 2013-03-10 22:33 UTC (permalink / raw) To: Josh Boyer; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel 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, ¶m_ops_uint, &share_irqs, 0644); > + module_param_cb(nr_uarts, ¶m_ops_uint, &nr_uarts, 0644); > + module_param_cb(skip_txen_test, ¶m_ops_uint, &skip_txen_test, 0644); > +#ifdef CONFIG_SERIAL_8250_RSA > + __module_param_call(MODULE_PARAM_PREFIX, probe_rsa, > + ¶m_array_ops, .arr = &__param_arr_probe_rsa, > + 0444, -1); > +#endif > +} > +#else > +MODULE_ALIAS("8250"); > +#endif > -- js suse labs ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-09 14:14 ` Jiri Slaby 2013-03-09 17:02 ` Josh Boyer @ 2013-03-10 12:21 ` Sean Young 1 sibling, 0 replies; 25+ messages in thread From: Sean Young @ 2013-03-10 12:21 UTC (permalink / raw) To: Jiri Slaby; +Cc: Josh Boyer, Greg Kroah-Hartman, linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 2013-03-08 22:58 ` Jiri Slaby @ 2013-03-08 23:11 ` Josh Boyer 2013-03-08 23:11 ` Josh Boyer 1 sibling, 0 replies; 25+ messages in thread From: Josh Boyer @ 2013-03-08 23:11 UTC (permalink / raw) To: Jiri Slaby; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: 8250.nr_uarts broken in 3.7 @ 2013-03-08 23:11 ` Josh Boyer 0 siblings, 0 replies; 25+ messages in thread From: Josh Boyer @ 2013-03-08 23:11 UTC (permalink / raw) To: Jiri Slaby; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2013-03-10 22:33 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-03-07 18:56 8250.nr_uarts broken in 3.7 Josh Boyer 2013-03-07 18:58 ` Josh Boyer 2013-03-07 19:07 ` Jiri Slaby 2013-03-07 19:10 ` Josh Boyer 2013-03-07 21:14 ` Josh Boyer 2013-03-07 23:12 ` Greg Kroah-Hartman 2013-03-08 1:01 ` Josh Boyer 2013-03-08 1:39 ` Greg Kroah-Hartman 2013-03-08 21:27 ` Josh Boyer 2013-03-08 22:47 ` Jiri Slaby 2013-03-08 22:49 ` Josh Boyer 2013-03-08 22:58 ` Jiri Slaby 2013-03-08 23:10 ` Jiri Slaby 2013-03-08 23:14 ` Jiri Slaby 2013-03-08 23:28 ` Josh Boyer 2013-03-08 23:44 ` Josh Boyer 2013-03-09 9:14 ` Jiri Slaby 2013-03-09 13:30 ` Josh Boyer 2013-03-09 14:14 ` Jiri Slaby 2013-03-09 17:02 ` Josh Boyer 2013-03-10 14:33 ` [PATCH v2] serial: 8250: Keep 8250.<xxxx> module options functional after driver rename Josh Boyer 2013-03-10 22:33 ` Jiri Slaby 2013-03-10 12:21 ` 8250.nr_uarts broken in 3.7 Sean Young 2013-03-08 23:11 ` Josh Boyer 2013-03-08 23:11 ` Josh Boyer
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.