[PATCH] tty: serial: serial_core.c: printk replacement

[PATCH] tty: serial: serial_core.c: printk replacement

[Sudip Mukherjee]

printk replaced with corresponding dev_err, dev_alert, dev_notice and pr_info.
pr_info has been used instead of dev_info as there is a check for port->dev being NULL or not.
fixed two broken user-visible strings used by the corresponding printk

Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
---

For my previous patch ,Jiri Slaby told to investigate further as I have used uport->dev and there was a check for whether it is NULL. 
For this patch instead of uport->dev , i have used tty_dev as that one gave a better desciption of the device than uport->dev.


 drivers/tty/serial/serial_core.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 29a7be4..78aa38d 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -892,10 +892,11 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
 			 */
 			if (uport->flags & UPF_SPD_MASK) {
 				char buf[64];
-				printk(KERN_NOTICE
-				       "%s sets custom speed on %s. This "
-				       "is deprecated.\n", current->comm,
-				       tty_name(port->tty, buf));
+
+				dev_notice(uport->dev,
+				       "%s sets custom speed on %s. This is deprecated.\n",
+				      current->comm,
+				      tty_name(port->tty, buf));
 			}
 			uart_change_speed(tty, state, NULL);
 		}
@@ -1975,12 +1976,11 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
 		for (tries = 3; !ops->tx_empty(uport) && tries; tries--)
 			msleep(10);
 		if (!tries)
-			printk(KERN_ERR "%s%s%s%d: Unable to drain "
-					"transmitter\n",
-			       uport->dev ? dev_name(uport->dev) : "",
-			       uport->dev ? ": " : "",
-			       drv->dev_name,
-			       drv->tty_driver->name_base + uport->line);
+			dev_err(tty_dev, "%s%s%s%d: Unable to drain transmitter\n",
+				uport->dev ? dev_name(uport->dev) : "",
+				uport->dev ? ": " : "",
+				drv->dev_name,
+				drv->tty_driver->name_base + uport->line);
 
 		if (console_suspend_enabled || !uart_console(uport))
 			ops->shutdown(uport);
@@ -2109,7 +2109,7 @@ uart_report_port(struct uart_driver *drv, struct uart_port *port)
 		break;
 	}
 
-	printk(KERN_INFO "%s%s%s%d at %s (irq = %d, base_baud = %d) is a %s\n",
+	pr_info("%s%s%s%d at %s (irq = %d, base_baud = %d) is a %s\n",
 	       port->dev ? dev_name(port->dev) : "",
 	       port->dev ? ": " : "",
 	       drv->dev_name,
@@ -2640,7 +2640,7 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 	if (likely(!IS_ERR(tty_dev))) {
 		device_set_wakeup_capable(tty_dev, 1);
 	} else {
-		printk(KERN_ERR "Cannot register tty device on line %d\n",
+		dev_err(uport->dev, "Cannot register tty device on line %d\n",
 		       uport->line);
 	}
 
@@ -2675,7 +2675,7 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
 	BUG_ON(in_interrupt());
 
 	if (state->uart_port != uport)
-		printk(KERN_ALERT "Removing wrong port: %p != %p\n",
+		dev_alert(uport->dev, "Removing wrong port: %p != %p\n",
 			state->uart_port, uport);
 
 	mutex_lock(&port_mutex);
-- 
1.8.1.2

Re: [PATCH] tty: serial: serial_core.c: printk replacement

[Jiri Slaby]

On 08/05/2014, 02:35 PM, Sudip Mukherjee wrote:
> printk replaced with corresponding dev_err, dev_alert, dev_notice and pr_info.
> pr_info has been used instead of dev_info as there is a check for port->dev being NULL or not.
> fixed two broken user-visible strings used by the corresponding printk
> 
> Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> ---
> 
> For my previous patch ,Jiri Slaby told to investigate further as I have used uport->dev and there was a check for whether it is NULL. 
> For this patch instead of uport->dev , i have used tty_dev as that one gave a better desciption of the device than uport->dev.

Hi, that will not work either, as there is put_device(tty_dev) before
you use tty_dev.

-- 
js
suse labs

Re: [PATCH] tty: serial: serial_core.c: printk replacement

[Sudip Mukherjee]

>
> Hi, that will not work either, as there is put_device(tty_dev) before
> you use tty_dev.
>
> --
> js
> suse labs
>
Hi,
i missed that. I put my test message to test tty_dev immediately after
tty_dev was initialized and so never got the error when the port was
suspended.
Will change it to pr_err and resend the patch for you to review.

Thanks
Sudip

[PATCH] tty: serial: serial_core.c: printk replacement

[Sudip Mukherjee]

printk replaced with corresponding dev_* 
fixed two broken user-visible strings used by the corresponding printk


Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
---
 drivers/tty/serial/serial_core.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 8bb19da..ef48d0f 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -891,10 +891,10 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
 			 */
 			if (uport->flags & UPF_SPD_MASK) {
 				char buf[64];
-				printk(KERN_NOTICE
-				       "%s sets custom speed on %s. This "
-				       "is deprecated.\n", current->comm,
-				       tty_name(port->tty, buf));
+
+				dev_notice(uport->dev,
+				"%s sets custom speed on %s. This is deprecated.\n",
+				current->comm, tty_name(port->tty, buf));
 			}
 			uart_change_speed(tty, state, NULL);
 		}
@@ -1974,8 +1974,8 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
 		for (tries = 3; !ops->tx_empty(uport) && tries; tries--)
 			msleep(10);
 		if (!tries)
-			printk(KERN_ERR "%s%s%s%d: Unable to drain "
-					"transmitter\n",
+			dev_err(uport->dev,
+				"%s%s%s%d: Unable to drain transmitter\n",
 			       uport->dev ? dev_name(uport->dev) : "",
 			       uport->dev ? ": " : "",
 			       drv->dev_name,
@@ -2108,7 +2108,8 @@ uart_report_port(struct uart_driver *drv, struct uart_port *port)
 		break;
 	}
 
-	printk(KERN_INFO "%s%s%s%d at %s (irq = %d, base_baud = %d) is a %s\n",
+	dev_info(port->dev,
+		       "%s%s%s%d at %s (irq = %d, base_baud = %d) is a %s\n",
 	       port->dev ? dev_name(port->dev) : "",
 	       port->dev ? ": " : "",
 	       drv->dev_name,
@@ -2637,7 +2638,7 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 	if (likely(!IS_ERR(tty_dev))) {
 		device_set_wakeup_capable(tty_dev, 1);
 	} else {
-		printk(KERN_ERR "Cannot register tty device on line %d\n",
+		dev_err(uport->dev, "Cannot register tty device on line %d\n",
 		       uport->line);
 	}
 
@@ -2672,7 +2673,7 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
 	BUG_ON(in_interrupt());
 
 	if (state->uart_port != uport)
-		printk(KERN_ALERT "Removing wrong port: %p != %p\n",
+		dev_alert(uport->dev, "Removing wrong port: %p != %p\n",
 			state->uart_port, uport);
 
 	mutex_lock(&port_mutex);
-- 
1.8.1.2

build tested on x86_64 and arm (mini2440 board)

Re: [PATCH] tty: serial: serial_core.c: printk replacement

[Jiri Slaby]

On 07/30/2014, 04:07 PM, Sudip Mukherjee wrote:
> printk replaced with corresponding dev_* 
> fixed two broken user-visible strings used by the corresponding printk
> 
> 
> Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> ---
>  drivers/tty/serial/serial_core.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 8bb19da..ef48d0f 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
...
> @@ -1974,8 +1974,8 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
>  		for (tries = 3; !ops->tx_empty(uport) && tries; tries--)
>  			msleep(10);
>  		if (!tries)
> -			printk(KERN_ERR "%s%s%s%d: Unable to drain "
> -					"transmitter\n",
> +			dev_err(uport->dev,
> +				"%s%s%s%d: Unable to drain transmitter\n",
>  			       uport->dev ? dev_name(uport->dev) : "",

Hi, this does not look correct. You use uport->dev, but there is a test
whether it is NULL in the parameters.

You would have to investigate if and when uport->dev can be NULL and
document it in the commit log above.

thanks,
-- 
js
suse labs

Re: [PATCH] tty: serial: serial_core.c: printk replacement

[Sudip Mukherjee]

On Wed, Jul 30, 2014 at 8:07 PM, Jiri Slaby <jslaby@suse.cz> wrote:
> On 07/30/2014, 04:07 PM, Sudip Mukherjee wrote:
>> printk replaced with corresponding dev_*
>> fixed two broken user-visible strings used by the corresponding printk
>>
>>
>> Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
>> ---
>>  drivers/tty/serial/serial_core.c | 19 ++++++++++---------
>>  1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index 8bb19da..ef48d0f 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
> ...
>> @@ -1974,8 +1974,8 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
>>               for (tries = 3; !ops->tx_empty(uport) && tries; tries--)
>>                       msleep(10);
>>               if (!tries)
>> -                     printk(KERN_ERR "%s%s%s%d: Unable to drain "
>> -                                     "transmitter\n",
>> +                     dev_err(uport->dev,
>> +                             "%s%s%s%d: Unable to drain transmitter\n",
>>                              uport->dev ? dev_name(uport->dev) : "",
>
> Hi, this does not look correct. You use uport->dev, but there is a test
> whether it is NULL in the parameters.
>
> You would have to investigate if and when uport->dev can be NULL and
> document it in the commit log above.

HI
Thanks for reviewing it . I am checking out all the cases in which
uport can be null and will resend the patch.

Thanks
Sudip

>
> thanks,
> --
> js
> suse labs

Re: [PATCH] tty: serial: serial_core.c: printk replacement

[Sudip Mukherjee]

On Wed, Jul 30, 2014 at 8:22 PM, Sudip Mukherjee
<sudipm.mukherjee@gmail.com> wrote:
> On Wed, Jul 30, 2014 at 8:07 PM, Jiri Slaby <jslaby@suse.cz> wrote:
>> On 07/30/2014, 04:07 PM, Sudip Mukherjee wrote:
>>> printk replaced with corresponding dev_*
>>> fixed two broken user-visible strings used by the corresponding printk
>>>
>>>
>>> Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
>>> ---
>>>  drivers/tty/serial/serial_core.c | 19 ++++++++++---------
>>>  1 file changed, 10 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>>> index 8bb19da..ef48d0f 100644
>>> --- a/drivers/tty/serial/serial_core.c
>>> +++ b/drivers/tty/serial/serial_core.c
>> ...
>>> @@ -1974,8 +1974,8 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
>>>               for (tries = 3; !ops->tx_empty(uport) && tries; tries--)
>>>                       msleep(10);
>>>               if (!tries)
>>> -                     printk(KERN_ERR "%s%s%s%d: Unable to drain "
>>> -                                     "transmitter\n",
>>> +                     dev_err(uport->dev,
>>> +                             "%s%s%s%d: Unable to drain transmitter\n",
>>>                              uport->dev ? dev_name(uport->dev) : "",
>>
>> Hi, this does not look correct. You use uport->dev, but there is a test
>> whether it is NULL in the parameters.
>>
>> You would have to investigate if and when uport->dev can be NULL and
>> document it in the commit log above.
>
> HI
> Thanks for reviewing it . I am checking out all the cases in which
> uport can be null and will resend the patch.
>
> Thanks
> Sudip
>
>>
>> thanks,
>> --
>> js
>> suse labs

Hi
All the investigations has made me even more confused , and I am
looking for a bit of guidance from you now.

uport->dev can only be null if it is a virtual device and no real
device is attached to the driver , but that again should not be the
case with a serial port . And , for the function uart_suspend_port ,
if i use dev_err , then i feel it wllll be much more better to use
tty_dev instead of uport->dev . ( in that function we have tty_dev =
device_find_child(uport->dev, &match, serial_match_port); ) - using
tty_dev as the device in the dev_err is giving better device
description.

Now to my confusion :
we have  tty_dev = device_find_child(uport->dev, &match,
serial_match_port);  - so if uport->dev is NULL , then
device_find_child also returns NULL , making tty_dev = NULL .  But
immediately after that statement we have : if
(device_may_wakeup(tty_dev)) -  so if tty_dev is NULL , then
device_may_wakeup will try to access tty_dev->power , in short it will
try to access NULL->power . So shouldn't we have kernel panic if we
try dereferencing a NULL pointer or then should we assume that
uport->dev can never by NULL ....

Thanks
Sudip

[PATCH] tty: serial: serial_core.c: printk replacement

[Sudip Mukherjee]

printk replaced with corresponding pr_* 
fixed two broken user-visible strings used by the printk


Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
---
 drivers/tty/serial/serial_core.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 8bb19da..eb6e663 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -891,10 +891,11 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
 			 */
 			if (uport->flags & UPF_SPD_MASK) {
 				char buf[64];
-				printk(KERN_NOTICE
-				       "%s sets custom speed on %s. This "
-				       "is deprecated.\n", current->comm,
-				       tty_name(port->tty, buf));
+
+				pr_notice(
+					"%s sets custom speed on %s. This is deprecated.\n",
+					current->comm,
+					tty_name(port->tty, buf));
 			}
 			uart_change_speed(tty, state, NULL);
 		}
@@ -1974,8 +1975,7 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
 		for (tries = 3; !ops->tx_empty(uport) && tries; tries--)
 			msleep(10);
 		if (!tries)
-			printk(KERN_ERR "%s%s%s%d: Unable to drain "
-					"transmitter\n",
+			pr_err("%s%s%s%d: Unable to drain transmitter\n",
 			       uport->dev ? dev_name(uport->dev) : "",
 			       uport->dev ? ": " : "",
 			       drv->dev_name,
@@ -2108,7 +2108,7 @@ uart_report_port(struct uart_driver *drv, struct uart_port *port)
 		break;
 	}
 
-	printk(KERN_INFO "%s%s%s%d at %s (irq = %d, base_baud = %d) is a %s\n",
+	pr_info("%s%s%s%d at %s (irq = %d, base_baud = %d) is a %s\n",
 	       port->dev ? dev_name(port->dev) : "",
 	       port->dev ? ": " : "",
 	       drv->dev_name,
@@ -2637,7 +2637,7 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 	if (likely(!IS_ERR(tty_dev))) {
 		device_set_wakeup_capable(tty_dev, 1);
 	} else {
-		printk(KERN_ERR "Cannot register tty device on line %d\n",
+		pr_err("Cannot register tty device on line %d\n",
 		       uport->line);
 	}
 
@@ -2672,7 +2672,7 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
 	BUG_ON(in_interrupt());
 
 	if (state->uart_port != uport)
-		printk(KERN_ALERT "Removing wrong port: %p != %p\n",
+		pr_alert("Removing wrong port: %p != %p\n",
 			state->uart_port, uport);
 
 	mutex_lock(&port_mutex);
-- 
1.8.1.2

build tested on x86_64 and arm (mini2440 board)

Re: [PATCH] tty: serial: serial_core.c: printk replacement

[Greg KH]

On Mon, Jul 28, 2014 at 08:28:01PM +0530, Sudip Mukherjee wrote:
> printk replaced with corresponding pr_* 
> fixed two broken user-visible strings used by the printk

Can you change to use dev_err() instead?  That would help identify the
device and driver causing this problem better.

thanks,

greg k-h