2014-11-18 06:52:35

by Kiran Kumar Raparthy

[permalink] [raw]
Subject: [RFC] serial_core: Add wake_peer uart operation

From: San Mehat <[email protected]>

serial_core: Add wake_peer uart operation

Add wake_peer which is called before starting UART TX. The idea here
is to provide a mechanism where we can wakeup our peer before sending
data.

Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jiri Slaby <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Android Kernel Team <[email protected]>
Cc: John Stultz <[email protected]>
Cc: Sumit Semwal <[email protected]>
Signed-off-by: San Mehat <[email protected]>
[Kiran: Added context to commit message]
Signed-off-by: Kiran Raparthy <[email protected]>
---
This is one of the number of patches from the Android AOSP common.git tree,
which is used on almost all Android devices. I wanted to submit it for review
to see if it should go upstream.

drivers/tty/serial/serial_core.c | 3 +++
include/linux/serial_core.h | 1 +
2 files changed, 4 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index df3a8c7..dc45c4b 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -95,6 +95,9 @@ static void __uart_start(struct tty_struct *tty)
struct uart_state *state = tty->driver_data;
struct uart_port *port = state->uart_port;

+ if (port->ops->wake_peer)
+ port->ops->wake_peer(port);
+
if (!uart_tx_stopped(port))
port->ops->start_tx(port);
}
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 21c2e05..219b6a3 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -66,6 +66,7 @@ struct uart_ops {
void (*set_ldisc)(struct uart_port *, int new);
void (*pm)(struct uart_port *, unsigned int state,
unsigned int oldstate);
+ void (*wake_peer)(struct uart_port *);

/*
* Return a string describing the type of the port
--
1.8.2.1


2014-11-18 16:25:49

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [RFC] serial_core: Add wake_peer uart operation

On Tue, Nov 18, 2014 at 12:20:01PM +0530, Kiran Kumar Raparthy wrote:
> From: San Mehat <[email protected]>
>
> serial_core: Add wake_peer uart operation
>
> Add wake_peer which is called before starting UART TX. The idea here
> is to provide a mechanism where we can wakeup our peer before sending
> data.
>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Jiri Slaby <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: Android Kernel Team <[email protected]>
> Cc: John Stultz <[email protected]>
> Cc: Sumit Semwal <[email protected]>
> Signed-off-by: San Mehat <[email protected]>
> [Kiran: Added context to commit message]
> Signed-off-by: Kiran Raparthy <[email protected]>
> ---
> This is one of the number of patches from the Android AOSP common.git tree,
> which is used on almost all Android devices. I wanted to submit it for review
> to see if it should go upstream.
>
> drivers/tty/serial/serial_core.c | 3 +++
> include/linux/serial_core.h | 1 +
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index df3a8c7..dc45c4b 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -95,6 +95,9 @@ static void __uart_start(struct tty_struct *tty)
> struct uart_state *state = tty->driver_data;
> struct uart_port *port = state->uart_port;
>
> + if (port->ops->wake_peer)
> + port->ops->wake_peer(port);
> +
> if (!uart_tx_stopped(port))
> port->ops->start_tx(port);
> }
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 21c2e05..219b6a3 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -66,6 +66,7 @@ struct uart_ops {
> void (*set_ldisc)(struct uart_port *, int new);
> void (*pm)(struct uart_port *, unsigned int state,
> unsigned int oldstate);
> + void (*wake_peer)(struct uart_port *);
>
> /*
> * Return a string describing the type of the port

Do you have a driver that uses this callback to submit? I really don't
like taking hooks that are not used in the tree, otherwise it usually
gets removed by someone who says, "look this hook isn't being used,
let's delete it!"

thanks,

greg k-h

2014-11-18 16:43:43

by Kiran Kumar Raparthy

[permalink] [raw]
Subject: Re: [RFC] serial_core: Add wake_peer uart operation

On 18 November 2014 21:55, Greg Kroah-Hartman
<[email protected]> wrote:
> On Tue, Nov 18, 2014 at 12:20:01PM +0530, Kiran Kumar Raparthy wrote:
>> From: San Mehat <[email protected]>
>>
>> serial_core: Add wake_peer uart operation
>>
>> Add wake_peer which is called before starting UART TX. The idea here
>> is to provide a mechanism where we can wakeup our peer before sending
>> data.
>>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Jiri Slaby <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: Android Kernel Team <[email protected]>
>> Cc: John Stultz <[email protected]>
>> Cc: Sumit Semwal <[email protected]>
>> Signed-off-by: San Mehat <[email protected]>
>> [Kiran: Added context to commit message]
>> Signed-off-by: Kiran Raparthy <[email protected]>
>> ---
>> This is one of the number of patches from the Android AOSP common.git tree,
>> which is used on almost all Android devices. I wanted to submit it for review
>> to see if it should go upstream.
>>
>> drivers/tty/serial/serial_core.c | 3 +++
>> include/linux/serial_core.h | 1 +
>> 2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index df3a8c7..dc45c4b 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -95,6 +95,9 @@ static void __uart_start(struct tty_struct *tty)
>> struct uart_state *state = tty->driver_data;
>> struct uart_port *port = state->uart_port;
>>
>> + if (port->ops->wake_peer)
>> + port->ops->wake_peer(port);
>> +
>> if (!uart_tx_stopped(port))
>> port->ops->start_tx(port);
>> }
>> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
>> index 21c2e05..219b6a3 100644
>> --- a/include/linux/serial_core.h
>> +++ b/include/linux/serial_core.h
>> @@ -66,6 +66,7 @@ struct uart_ops {
>> void (*set_ldisc)(struct uart_port *, int new);
>> void (*pm)(struct uart_port *, unsigned int state,
>> unsigned int oldstate);
>> + void (*wake_peer)(struct uart_port *);
>>
>> /*
>> * Return a string describing the type of the port
>
> Do you have a driver that uses this callback to submit? I really don't
> like taking hooks that are not used in the tree, otherwise it usually
> gets removed by someone who says, "look this hook isn't being used,
> let's delete it!"
Okay,let me check and resubmit the patch which uses the callback in driver.
Thanks for the review comments.
Regards,
Kiran
>
> thanks,
>
> greg k-h

2014-11-24 09:05:28

by Alan Cox

[permalink] [raw]
Subject: Re: [RFC] serial_core: Add wake_peer uart operation

> This is one of the number of patches from the Android AOSP common.git tree,
> which is used on almost all Android devices. I wanted to submit it for review
> to see if it should go upstream.
>
> drivers/tty/serial/serial_core.c | 3 +++
> include/linux/serial_core.h | 1 +
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index df3a8c7..dc45c4b 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -95,6 +95,9 @@ static void __uart_start(struct tty_struct *tty)
> struct uart_state *state = tty->driver_data;
> struct uart_port *port = state->uart_port;
>
> + if (port->ops->wake_peer)
> + port->ops->wake_peer(port);
> +
> if (!uart_tx_stopped(port))
> port->ops->start_tx(port);
> }
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 21c2e05..219b6a3 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -66,6 +66,7 @@ struct uart_ops {
> void (*set_ldisc)(struct uart_port *, int new);
> void (*pm)(struct uart_port *, unsigned int state,
> unsigned int oldstate);
> + void (*wake_peer)(struct uart_port *);
>
> /*
> * Return a string describing the type of the port

Can we please document the locking and call semantics for this - eg what
stop the peer going back to sleep as this call is made ? when does the
peer go back to sleep ? how does the peer decide to go back to sleep ?
Who sets wake_peer, when may it be set, why is it a uart_op when it's
likely to be per port and really ought to be a bit more dynamic ?

It's probably obvious to Android folks, but the rules are not afaik
written down anywhere in kernel land.

Also I guess why is the power management framework not appropriate for
this should also be answered.

I'm less worried about users - there are a lot of GPL out of tree drivers
using it, and several in tree ones with patches you add for Android use
which exist solely to fix up wake peer and the old wakelocks.

Alan