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

[Sudip Mukherjee]

On Wed, Jul 30, 2014 at 8:22 PM, Sudip Mukherjee
<sudipm.mukherjee@xxxxxxxxx> wrote:
> On Wed, Jul 30, 2014 at 8:07 PM, Jiri Slaby <jslaby@xxxxxxx> 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@xxxxxxxxx>
>>> ---
>>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/