* Re: [PATCH] [OOPS] apbuart.c: Two problems related to grlib_apbuart_configure()
2010-05-08 21:58 [PATCH] [OOPS] apbuart.c: Two problems related to grlib_apbuart_configure() Miguel Ojeda
@ 2010-05-10 13:20 ` Miguel Ojeda
2010-05-11 7:11 ` Kristoffer Glembo
2010-05-11 13:38 ` Miguel Ojeda
2010-05-20 19:52 ` [alternative-merged] serial-apbuartc-fix-two-problems-related-to-grlib_apbuart_configure.patch removed from -mm tree Miguel Ojeda
2 siblings, 1 reply; 7+ messages in thread
From: Miguel Ojeda @ 2010-05-10 13:20 UTC (permalink / raw)
To: Andrew Morton, kristoffer; +Cc: linux-kernel
On Sat, May 8, 2010 at 11:58 PM, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> Hi,
>
> I found two problems using the Simics' sunfire target, so maybe the
> following does not occur in real machines.
>
> 1. At grlib_apbuart_configure() in apbuart.c, prop can be NULL if
> "clock-frequency" doesn't exist in the OF tree:
>
> /* Get bus frequency */
> rp = of_find_node_by_path("/");
> rp = of_get_next_child(rp, NULL);
> prop = of_get_property(rp, "clock-frequency", NULL);
> freq_khz = *prop;
>
> 2. In addition, apbuart.c does not check if there aren't any ports
> configured after calls to grlib_apbuart_configure(), so other oops will
> occur if no port was configured (e.g. uart_set_options() because of
> port->ops).
>
> In order to solve that, I added a check after both of the calls to
> grlib_apbuart_configure().
>
> The patch that I provide below prevents both problems in the Simics'
> sunfire target.
>
> Tested against 2.6.33.3. Please review.
>
> Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
> ---
> --- drivers/serial/apbuart.c.orig 2010-04-26 16:48:30.000000000 +0200
> +++ drivers/serial/apbuart.c 2010-05-08 23:50:36.786148571 +0200
> @@ -525,6 +525,9 @@ static void grlib_apbuart_configure(void
> static int __init apbuart_console_init(void)
> {
> grlib_apbuart_configure();
> + if (grlib_apbuart_port_nr == 0)
> + return 0;
> +
> register_console(&grlib_apbuart_console);
> return 0;
> }
> @@ -612,6 +615,8 @@ static void grlib_apbuart_configure(void
> rp = of_find_node_by_path("/");
> rp = of_get_next_child(rp, NULL);
> prop = of_get_property(rp, "clock-frequency", NULL);
> + if (prop == NULL)
grlib_apbuart_port_nr = 0; should be added here or in apbuart.h better.
> + return;
> freq_khz = *prop;
>
> line = 0;
> @@ -666,6 +671,11 @@ static int __init grlib_apbuart_init(voi
>
> /* Find all APBUARTS in device the tree and initialize their ports */
> grlib_apbuart_configure();
> + if (grlib_apbuart_port_nr == 0) {
> + printk(KERN_INFO "Serial: GRLIB APBUART: No ports found.\n",
> + __FILE__);
> + return 0;
> + }
>
> printk(KERN_INFO "Serial: GRLIB APBUART driver\n");
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] [OOPS] apbuart.c: Two problems related to grlib_apbuart_configure()
2010-05-10 13:20 ` Miguel Ojeda
@ 2010-05-11 7:11 ` Kristoffer Glembo
0 siblings, 0 replies; 7+ messages in thread
From: Kristoffer Glembo @ 2010-05-11 7:11 UTC (permalink / raw)
To: Miguel Ojeda; +Cc: Andrew Morton, linux-kernel, David Miller
Miguel Ojeda wrote:
> On Sat, May 8, 2010 at 11:58 PM, Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>> Hi,
>>
>> I found two problems using the Simics' sunfire target, so maybe the
>> following does not occur in real machines.
>>
>> 1. At grlib_apbuart_configure() in apbuart.c, prop can be NULL if
>> "clock-frequency" doesn't exist in the OF tree:
>>
>> /* Get bus frequency */
>> rp = of_find_node_by_path("/");
>> rp = of_get_next_child(rp, NULL);
>> prop = of_get_property(rp, "clock-frequency", NULL);
>> freq_khz = *prop;
>>
>> 2. In addition, apbuart.c does not check if there aren't any ports
>> configured after calls to grlib_apbuart_configure(), so other oops will
>> occur if no port was configured (e.g. uart_set_options() because of
>> port->ops).
>>
>> In order to solve that, I added a check after both of the calls to
>> grlib_apbuart_configure().
>>
>> The patch that I provide below prevents both problems in the Simics'
>> sunfire target.
>>
>> Tested against 2.6.33.3. Please review.
>>
>> Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Thanks for the patch, it looks fine to me.
Acked-by: Kristoffer Glembo <kristoffer@gaisler.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] [OOPS] apbuart.c: Two problems related to grlib_apbuart_configure()
2010-05-08 21:58 [PATCH] [OOPS] apbuart.c: Two problems related to grlib_apbuart_configure() Miguel Ojeda
2010-05-10 13:20 ` Miguel Ojeda
@ 2010-05-11 13:38 ` Miguel Ojeda
2010-05-11 13:42 ` Kristoffer Glembo
2010-05-20 19:52 ` [alternative-merged] serial-apbuartc-fix-two-problems-related-to-grlib_apbuart_configure.patch removed from -mm tree Miguel Ojeda
2 siblings, 1 reply; 7+ messages in thread
From: Miguel Ojeda @ 2010-05-11 13:38 UTC (permalink / raw)
To: kristoffer, Andrew Morton; +Cc: linux-kernel
Hi,
Kristoffer, please check this one instead.
Changes:
1. Added the grlib_apbuart_port_nr = 0 line.
2. Returned -ENODEV instead of 0 in the "no ports found branch" in
init(), because if compiled as a module, exit() will try to unregister
things that were not registered. In addition, this will alert the users
that modprobe'd.
3. Deleted the unused __FILE__ argument of the previous patch (oops
sorry :).
Andrew, if Kristoffer agrees, please drop the older patch from -mm and
merge this one instead.
Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
---
--- drivers/serial/apbuart.c.orig 2010-04-26 16:48:30.000000000 +0200
+++ drivers/serial/apbuart.c 2010-05-11 15:21:28.984666230 +0200
@@ -525,6 +525,9 @@ static void grlib_apbuart_configure(void
static int __init apbuart_console_init(void)
{
grlib_apbuart_configure();
+ if (grlib_apbuart_port_nr == 0)
+ return 0;
+
register_console(&grlib_apbuart_console);
return 0;
}
@@ -612,6 +615,10 @@ static void grlib_apbuart_configure(void
rp = of_find_node_by_path("/");
rp = of_get_next_child(rp, NULL);
prop = of_get_property(rp, "clock-frequency", NULL);
+ if (prop == NULL) {
+ grlib_apbuart_port_nr = 0;
+ return;
+ }
freq_khz = *prop;
line = 0;
@@ -666,6 +673,10 @@ static int __init grlib_apbuart_init(voi
/* Find all APBUARTS in device the tree and initialize their ports */
grlib_apbuart_configure();
+ if (grlib_apbuart_port_nr == 0) {
+ printk(KERN_INFO "Serial: GRLIB APBUART: No ports found.\n");
+ return -ENODEV;
+ }
printk(KERN_INFO "Serial: GRLIB APBUART driver\n");
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] [OOPS] apbuart.c: Two problems related to grlib_apbuart_configure()
2010-05-11 13:38 ` Miguel Ojeda
@ 2010-05-11 13:42 ` Kristoffer Glembo
0 siblings, 0 replies; 7+ messages in thread
From: Kristoffer Glembo @ 2010-05-11 13:42 UTC (permalink / raw)
To: Miguel Ojeda; +Cc: Andrew Morton, linux-kernel, David Miller
Hi,
Looks finer :-)
Acked-by: Kristoffer Glembo <kristoffer@gaisler.com>
Miguel Ojeda wrote:
> Hi,
>
> Kristoffer, please check this one instead.
>
> Changes:
> 1. Added the grlib_apbuart_port_nr = 0 line.
> 2. Returned -ENODEV instead of 0 in the "no ports found branch" in
> init(), because if compiled as a module, exit() will try to unregister
> things that were not registered. In addition, this will alert the users
> that modprobe'd.
> 3. Deleted the unused __FILE__ argument of the previous patch (oops
> sorry :).
>
> Andrew, if Kristoffer agrees, please drop the older patch from -mm and
> merge this one instead.
>
> Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
> ---
> --- drivers/serial/apbuart.c.orig 2010-04-26 16:48:30.000000000 +0200
> +++ drivers/serial/apbuart.c 2010-05-11 15:21:28.984666230 +0200
> @@ -525,6 +525,9 @@ static void grlib_apbuart_configure(void
> static int __init apbuart_console_init(void)
> {
> grlib_apbuart_configure();
> + if (grlib_apbuart_port_nr == 0)
> + return 0;
> +
> register_console(&grlib_apbuart_console);
> return 0;
> }
> @@ -612,6 +615,10 @@ static void grlib_apbuart_configure(void
> rp = of_find_node_by_path("/");
> rp = of_get_next_child(rp, NULL);
> prop = of_get_property(rp, "clock-frequency", NULL);
> + if (prop == NULL) {
> + grlib_apbuart_port_nr = 0;
> + return;
> + }
> freq_khz = *prop;
>
> line = 0;
> @@ -666,6 +673,10 @@ static int __init grlib_apbuart_init(voi
>
> /* Find all APBUARTS in device the tree and initialize their ports */
> grlib_apbuart_configure();
> + if (grlib_apbuart_port_nr == 0) {
> + printk(KERN_INFO "Serial: GRLIB APBUART: No ports found.\n");
> + return -ENODEV;
> + }
>
> printk(KERN_INFO "Serial: GRLIB APBUART driver\n");
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [alternative-merged] serial-apbuartc-fix-two-problems-related-to-grlib_apbuart_configure.patch removed from -mm tree
2010-05-08 21:58 [PATCH] [OOPS] apbuart.c: Two problems related to grlib_apbuart_configure() Miguel Ojeda
2010-05-10 13:20 ` Miguel Ojeda
2010-05-11 13:38 ` Miguel Ojeda
@ 2010-05-20 19:52 ` Miguel Ojeda
2010-05-20 20:09 ` Miguel Ojeda
2 siblings, 1 reply; 7+ messages in thread
From: Miguel Ojeda @ 2010-05-20 19:52 UTC (permalink / raw)
To: Greg KH; +Cc: linux-kernel, kristoffer, Andrew Morton, davem, Anton Vorontsov
Hi,
I found two problems using the Simics' sunfire target. It occurs as well on real machines.
1. At grlib_apbuart_configure() in apbuart.c, prop can be NULL if
"clock-frequency" doesn't exist in the OF tree:
/* Get bus frequency */
rp = of_find_node_by_path("/");
rp = of_get_next_child(rp, NULL);
prop = of_get_property(rp, "clock-frequency", NULL);
freq_khz = *prop;
2. In addition, apbuart.c does not check if there aren't any ports
configured after calls to grlib_apbuart_configure(), so other oops will
occur if no port was configured (e.g. uart_set_options() because of
port->ops).
In order to solve that, I added a check after both of the calls to
grlib_apbuart_configure().
The patch that I provide below prevents both problems in the Simics'
sunfire target. In addition, it warns the users if no ports were found.
Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Acked-by: Kristoffer Glembo <kristoffer@gaisler.com>
---
--- drivers/serial/apbuart.c.orig 2010-04-26 16:48:30.000000000 +0200
+++ drivers/serial/apbuart.c 2010-05-11 15:21:28.984666230 +0200
@@ -525,6 +525,9 @@ static void grlib_apbuart_configure(void
static int __init apbuart_console_init(void)
{
grlib_apbuart_configure();
+ if (grlib_apbuart_port_nr == 0)
+ return 0;
+
register_console(&grlib_apbuart_console);
return 0;
}
@@ -612,6 +615,10 @@ static void grlib_apbuart_configure(void
rp = of_find_node_by_path("/");
rp = of_get_next_child(rp, NULL);
prop = of_get_property(rp, "clock-frequency", NULL);
+ if (prop == NULL) {
+ grlib_apbuart_port_nr = 0;
+ return;
+ }
freq_khz = *prop;
line = 0;
@@ -666,6 +673,10 @@ static int __init grlib_apbuart_init(voi
/* Find all APBUARTS in device the tree and initialize their ports */
grlib_apbuart_configure();
+ if (grlib_apbuart_port_nr == 0) {
+ printk(KERN_INFO "Serial: GRLIB APBUART: No ports found.\n");
+ return -ENODEV;
+ }
printk(KERN_INFO "Serial: GRLIB APBUART driver\n");
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [alternative-merged] serial-apbuartc-fix-two-problems-related-to-grlib_apbuart_configure.patch removed from -mm tree
2010-05-20 19:52 ` [alternative-merged] serial-apbuartc-fix-two-problems-related-to-grlib_apbuart_configure.patch removed from -mm tree Miguel Ojeda
@ 2010-05-20 20:09 ` Miguel Ojeda
0 siblings, 0 replies; 7+ messages in thread
From: Miguel Ojeda @ 2010-05-20 20:09 UTC (permalink / raw)
To: Greg KH
Cc: linux-kernel, kristoffer, Andrew Morton, davem, Anton Vorontsov,
stable
On Thu, May 20, 2010 at 9:52 PM, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> Hi,
>
> I found two problems using the Simics' sunfire target. It occurs as well on real machines.
>
> 1. At grlib_apbuart_configure() in apbuart.c, prop can be NULL if
> "clock-frequency" doesn't exist in the OF tree:
>
> /* Get bus frequency */
> rp = of_find_node_by_path("/");
> rp = of_get_next_child(rp, NULL);
> prop = of_get_property(rp, "clock-frequency", NULL);
> freq_khz = *prop;
>
> 2. In addition, apbuart.c does not check if there aren't any ports
> configured after calls to grlib_apbuart_configure(), so other oops will
> occur if no port was configured (e.g. uart_set_options() because of
> port->ops).
>
> In order to solve that, I added a check after both of the calls to
> grlib_apbuart_configure().
>
> The patch that I provide below prevents both problems in the Simics'
> sunfire target. In addition, it warns the users if no ports were found.
CC stable@kernel.org, as .33 is "stable" now and also has apbuart.c.
>
> Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
> Acked-by: Kristoffer Glembo <kristoffer@gaisler.com>
> ---
> --- drivers/serial/apbuart.c.orig 2010-04-26 16:48:30.000000000 +0200
> +++ drivers/serial/apbuart.c 2010-05-11 15:21:28.984666230 +0200
> @@ -525,6 +525,9 @@ static void grlib_apbuart_configure(void
> static int __init apbuart_console_init(void)
> {
> grlib_apbuart_configure();
> + if (grlib_apbuart_port_nr == 0)
> + return 0;
> +
> register_console(&grlib_apbuart_console);
> return 0;
> }
> @@ -612,6 +615,10 @@ static void grlib_apbuart_configure(void
> rp = of_find_node_by_path("/");
> rp = of_get_next_child(rp, NULL);
> prop = of_get_property(rp, "clock-frequency", NULL);
> + if (prop == NULL) {
> + grlib_apbuart_port_nr = 0;
> + return;
> + }
> freq_khz = *prop;
>
> line = 0;
> @@ -666,6 +673,10 @@ static int __init grlib_apbuart_init(voi
>
> /* Find all APBUARTS in device the tree and initialize their ports */
> grlib_apbuart_configure();
> + if (grlib_apbuart_port_nr == 0) {
> + printk(KERN_INFO "Serial: GRLIB APBUART: No ports found.\n");
> + return -ENODEV;
> + }
>
> printk(KERN_INFO "Serial: GRLIB APBUART driver\n");
>
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread