All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] tty: serial_core: Add mechanism to identify port closure.
@ 2012-04-20 10:57 Govindraj.R
  2012-04-20 13:27 ` Alan Cox
  0 siblings, 1 reply; 4+ messages in thread
From: Govindraj.R @ 2012-04-20 10:57 UTC (permalink / raw)
  To: linux-serial; +Cc: linux-omap, Govindraj.R, Greg Kroah-Hartman, Alan Cox

From: "Govindraj.R" <govindraj.raja@ti.com>

Currently all low level uart driver register to serial_core layer
this core layer exposes various serial uart ops.

Currently the core layer provides startup and shutdown hooks
to low level driver, but in port suspend case the shutdown gets called
from uart_suspend_port this makes little difficult in low level driver
to any action specific to port closure.

For example if low level driver tries to disable uart platform
specific wakeups after port closure in low level driver it can be only done
in shutdown ops since this ops is called even in suspend case low level
driver needs some info whether the current shutdown ops is result of
port close or if its suspend ops.

To differentiate this expand the core layer uart_port info with closing flag
this flag is set to true during shutdown and set to false while port open.

Thus closing flag added with this patch can be used in low level driver to
identify the port close.

Discussion reference:
http://marc.info/?l=linux-omap&m=133491321605924&w=2

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alan Cox <alan@linux.intel.com>
Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
 drivers/tty/serial/serial_core.c |    3 +++
 include/linux/serial_core.h      |    1 +
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 9c4c05b..0dc246d 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1284,6 +1284,8 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 		uart_wait_until_sent(tty, uport->timeout);
 	}
 
+	state->uart_port->closing = true;
+
 	mutex_lock(&port->mutex);
 	uart_shutdown(tty, state);
 	uart_flush_buffer(tty);
@@ -1518,6 +1520,7 @@ static int uart_open(struct tty_struct *tty, struct file *filp)
 	if (port->count == 1)
 		uart_change_pm(state, 0);
 
+	state->uart_port->closing = false;
 	/*
 	 * Start up the serial port.
 	 */
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 2db407a..f5cd1ee 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -381,6 +381,7 @@ struct uart_port {
 	unsigned char		irq_wake;
 	unsigned char		unused[2];
 	void			*private_data;		/* generic platform data pointer */
+	unsigned char		closing;
 };
 
 static inline int serial_port_in(struct uart_port *up, int offset)
-- 
1.7.9


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] tty: serial_core: Add mechanism to identify port closure.
  2012-04-20 10:57 [PATCH] tty: serial_core: Add mechanism to identify port closure Govindraj.R
@ 2012-04-20 13:27 ` Alan Cox
  2012-04-23  8:55   ` Raja, Govindraj
  0 siblings, 1 reply; 4+ messages in thread
From: Alan Cox @ 2012-04-20 13:27 UTC (permalink / raw)
  To: Govindraj.R; +Cc: linux-serial, linux-omap, Greg Kroah-Hartman, Alan Cox

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 9c4c05b..0dc246d 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1284,6 +1284,8 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
>  		uart_wait_until_sent(tty, uport->timeout);
>  	}
>  
> +	state->uart_port->closing = true;

So what are the locking rules for this - when is it valid and when can it
be tested.

This also still seems a hack to me - the core tty code doesn't have
suspend/resume/open/close confused. The uart layer does, so really the
uart layer needs untangling. I'm skeptical yet another magic state
variable is the answer, particularly when the locking model for the
variable appears undefined.

Teach the uart core code to pass an extra argument indicating whether its
really doing shutdown/open or merely doing suspend/resume. It's perfectly
sensible that it tries to use the same helper for both, its broken that
the called code cannot tell which.

The parameter would also be a local variable which avoids all the locking
questions.

Alan

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] tty: serial_core: Add mechanism to identify port closure.
  2012-04-20 13:27 ` Alan Cox
@ 2012-04-23  8:55   ` Raja, Govindraj
  2012-04-23 10:07     ` Alan Cox
  0 siblings, 1 reply; 4+ messages in thread
From: Raja, Govindraj @ 2012-04-23  8:55 UTC (permalink / raw)
  To: Alan Cox; +Cc: linux-serial, linux-omap, Greg Kroah-Hartman, Alan Cox

Hi Alan,

On Fri, Apr 20, 2012 at 6:57 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index 9c4c05b..0dc246d 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -1284,6 +1284,8 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
>>               uart_wait_until_sent(tty, uport->timeout);
>>       }
>>
>> +     state->uart_port->closing = true;
>
> So what are the locking rules for this - when is it valid and when can it
> be tested.
>
> This also still seems a hack to me - the core tty code doesn't have
> suspend/resume/open/close confused. The uart layer does, so really the
> uart layer needs untangling. I'm skeptical yet another magic state
> variable is the answer, particularly when the locking model for the
> variable appears undefined.
>
> Teach the uart core code to pass an extra argument indicating whether its
> really doing shutdown/open or merely doing suspend/resume. It's perfectly
> sensible that it tries to use the same helper for both, its broken that
> the called code cannot tell which.
>
> The parameter would also be a local variable which avoids all the locking
> questions.

Yes adding a _state_ local variable from port_shutdown from serial_core layer
and passing the same to low level driver can inform the low level driver.
whether current ops is shutdown or a suspend.

Also, looking into the struct uart_ops.

struct uart_ops {
.
.
        int             (*set_wake)(struct uart_port *, unsigned int state);
.
.
};

Unfortunately set wake is not being used in serial_core.c
So this uart ops can be used to enable wakeups
when port is opened and disable wakeups when port
is closed.

If set_wake is not supposed to be used in this manner I
will fall back to option1 to use state variable for shutdown ops.

tmp patch as in here[1] to use set_wake.

--
Thanks,
Govindraj.R

[1]:

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 9c4c05b..c176ff2 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1426,6 +1426,9 @@ static void uart_port_shutdown(struct tty_port *port)
 	 */
 	uport->ops->shutdown(uport);

+	if (uport->ops->set_wake)
+		uport->ops->set_wake(uport, false);
+
 	/*
 	 * Ensure that the IRQ handler isn't running on another CPU.
 	 */
@@ -1512,6 +1515,9 @@ static int uart_open(struct tty_struct *tty,
struct file *filp)
 		goto err_dec_count;
 	}

+	if (uport->ops->set_wake)
+		uport->ops->set_wake(uport, true);
+
 	/*
 	 * Make sure the device is in D0 state.
 	 */
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] tty: serial_core: Add mechanism to identify port closure.
  2012-04-23  8:55   ` Raja, Govindraj
@ 2012-04-23 10:07     ` Alan Cox
  0 siblings, 0 replies; 4+ messages in thread
From: Alan Cox @ 2012-04-23 10:07 UTC (permalink / raw)
  To: Raja, Govindraj; +Cc: linux-serial, linux-omap, Greg Kroah-Hartman, Alan Cox

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 9c4c05b..c176ff2 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1426,6 +1426,9 @@ static void uart_port_shutdown(struct tty_port *port)
>  	 */
>  	uport->ops->shutdown(uport);
> 
> +	if (uport->ops->set_wake)
> +		uport->ops->set_wake(uport, false);
> +
>  	/*
>  	 * Ensure that the IRQ handler isn't running on another CPU.
>  	 */
> @@ -1512,6 +1515,9 @@ static int uart_open(struct tty_struct *tty,
> struct file *filp)
>  		goto err_dec_count;
>  	}
> 
> +	if (uport->ops->set_wake)
> +		uport->ops->set_wake(uport, true);
> +
>  	/*
>  	 * Make sure the device is in D0 state.
>  	 */

This is probably the right approach for now. In the ideal world we should
probably use the struct device * and device power management but that is
a big upheaval and would mean fixing lots of other stuff first.

So this looks good to me - providing it doesn't break anything else.

Alan

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2012-04-23 10:04 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-20 10:57 [PATCH] tty: serial_core: Add mechanism to identify port closure Govindraj.R
2012-04-20 13:27 ` Alan Cox
2012-04-23  8:55   ` Raja, Govindraj
2012-04-23 10:07     ` Alan Cox

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.