[v2,05/12] OMAP: Serial: Hold console lock for console usage.

Commit Message

Govindraj R May 5, 2011, 10:25 a.m. UTC

On Thu, May 5, 2011 at 2:13 AM, Kevin Hilman <[e-mail]> wrote:
> "Govindraj.R" <[e-mail]> writes:
>
>> Acquire console lock before enabling and writing to console-uart
>> to avoid any recursive prints from console write.
>> for ex:
>>       --> printk
>>         --> uart_console_write
>>           --> get_sync
>>             --> printk from omap_device enable
>>               --> uart_console write
>
> By this time, since the device's runtime PM hooks have been called, the
> device's rutime PM state should be set to RPM_SUSPENDING (not yet
> RPM_SUSPENDED).
>
> In addition to the console lock, you should be able to use that flag to
> avoid console writes while the console is in the process of suspending.
>

Yes RPM_SUSPENDING check helps along with console lock

<< Changes as below >>
---------------
                locked = 0;

------------

Is it ok to check the RPM_SUSPENDING flag in driver ?
I can't find any API currently available under runtime.h to use.


>> Also during bootup console_lock is not available.
>>        --> uart_add_one_port
>>          --> console_register
>>              --> console_lock
>>               --> console_unlock
>>                    --> call_console_drivers (here yet console_lock is not released)
>>                         --> uart_console_write
>>
>> Hence convert prints from omap_device_enable/disable to pr_debug.
>
> And what if you add 'debug' to your kernel command line?  IOW, you're
> only solving the problem if you debug printk's are not going to the
> console.
>

yes correct issue will re-surface during boot-up once we have debug enabled
and all pr_debugs with omap_device_enable trying to use console_write.

--
Thanks,
Govindraj.R

> You're also not solving the problem that will happen down the road when
> someone else adds a printk to low-level code in order to debug something
> deep in the idle sequence.
>
> The recursive locking needs to be solved, not avoided.
>
> Kevin
>
>> Signed-off-by: Govindraj.R <[e-mail]>
>> ---
>>  arch/arm/plat-omap/omap_device.c |    8 ++++----
>>  drivers/tty/serial/omap-serial.c |    8 +++++++-
>>  2 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
>> index 9bbda9a..f7c6dca 100644
>> --- a/arch/arm/plat-omap/omap_device.c
>> +++ b/arch/arm/plat-omap/omap_device.c
>> @@ -145,12 +145,12 @@ static int _omap_device_activate(struct omap_device *od, u8 ignore_lat)
>>                       odpl->activate_lat_worst = act_lat;
>>                       if (odpl->flags & OMAP_DEVICE_LATENCY_AUTO_ADJUST) {
>>                               odpl->activate_lat = act_lat;
>> -                             pr_warning("omap_device: %s.%d: new worst case "
>> +                             pr_debug("omap_device: %s.%d: new worst case "
>>                                          "activate latency %d: %llu\n",
>>                                          od->pdev.name, od->pdev.id,
>>                                          od->pm_lat_level, act_lat);
>>                       } else
>> -                             pr_warning("omap_device: %s.%d: activate "
>> +                             pr_debug("omap_device: %s.%d: activate "
>>                                          "latency %d higher than exptected. "
>>                                          "(%llu > %d)\n",
>>                                          od->pdev.name, od->pdev.id,
>> @@ -213,12 +213,12 @@ static int _omap_device_deactivate(struct omap_device *od, u8 ignore_lat)
>>                       odpl->deactivate_lat_worst = deact_lat;
>>                       if (odpl->flags & OMAP_DEVICE_LATENCY_AUTO_ADJUST) {
>>                               odpl->deactivate_lat = deact_lat;
>> -                             pr_warning("omap_device: %s.%d: new worst case "
>> +                             pr_debug("omap_device: %s.%d: new worst case "
>>                                          "deactivate latency %d: %llu\n",
>>                                          od->pdev.name, od->pdev.id,
>>                                          od->pm_lat_level, deact_lat);
>>                       } else
>> -                             pr_warning("omap_device: %s.%d: deactivate "
>> +                             pr_debug("omap_device: %s.%d: deactivate "
>>                                          "latency %d higher than exptected. "
>>                                          "(%llu > %d)\n",
>>                                          od->pdev.name, od->pdev.id,
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index 633dfb4..7d8ca45 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -1007,7 +1007,10 @@ serial_omap_console_write(struct console *co, const char *s,
>>       struct uart_omap_port *up = serial_omap_console_ports[co->index];
>>       unsigned long flags;
>>       unsigned int ier;
>> -     int locked = 1;
>> +     int console_lock = 0, locked = 1;
>> +
>> +     if (console_trylock())
>> +             console_lock = 1;
>>
>>       local_irq_save(flags);
>>       if (up->port.sysrq)
>> @@ -1043,6 +1046,9 @@ serial_omap_console_write(struct console *co, const char *s,
>>       if (up->msr_saved_flags)
>>               check_modem_status(up);
>>
>> +     if (console_lock)
>> +             console_unlock();
>> +
>>       serial_omap_port_disable(up);
>>       if (locked)
>>               spin_unlock(&up->port.lock);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Kevin Hilman May 5, 2011, 2:52 p.m. UTC | #1

Govindraj <[e-mail]> writes:

> On Thu, May 5, 2011 at 2:13 AM, Kevin Hilman <[e-mail]> wrote:
>> "Govindraj.R" <[e-mail]> writes:
>>
>>> Acquire console lock before enabling and writing to console-uart
>>> to avoid any recursive prints from console write.
>>> for ex:
>>>       --> printk
>>>         --> uart_console_write
>>>           --> get_sync
>>>             --> printk from omap_device enable
>>>               --> uart_console write
>>
>> By this time, since the device's runtime PM hooks have been called, the
>> device's rutime PM state should be set to RPM_SUSPENDING (not yet
>> RPM_SUSPENDED).
>>
>> In addition to the console lock, you should be able to use that flag to
>> avoid console writes while the console is in the process of suspending.
>>
>
> Yes RPM_SUSPENDING check helps along with console lock

Great!

> << Changes as below >>
> ---------------
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 59f548f..71964c3 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -1040,6 +1040,18 @@ serial_omap_console_write(struct console *co,
> const char *s,
>         if (console_trylock())
>                 console_lock = 1;
>
> +       /*
> +        * If console_lock is not available and we are in suspending
> +        * state then we can avoid the console usage scenario
> +        * as this may introduce recursive prints.
> +        * Basically this scenario occurs during boot while
> +        * printing debug bootlogs.
> +        */
> +
> +       if (!console_lock &&
> +               up->pdev->dev.power.runtime_status == RPM_SUSPENDING)
> +               return;
> +
>         local_irq_save(flags);
>         if (up->port.sysrq)
>                 locked = 0;
>
> ------------
>
> Is it ok to check the RPM_SUSPENDING flag in driver ?
> I can't find any API currently available under runtime.h to use.

I would propose a new API in pm_runtime.h similar to
pm_runtime_suspended() that checks for this.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 59f548f..71964c3 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1040,6 +1040,18 @@  serial_omap_console_write(struct console *co,
const char *s,
        if (console_trylock())
                console_lock = 1;

+       /*
+        * If console_lock is not available and we are in suspending
+        * state then we can avoid the console usage scenario
+        * as this may introduce recursive prints.
+        * Basically this scenario occurs during boot while
+        * printing debug bootlogs.
+        */
+
+       if (!console_lock &&
+               up->pdev->dev.power.runtime_status == RPM_SUSPENDING)
+               return;
+
        local_irq_save(flags);
        if (up->port.sysrq)