Hi.
This series enable the PPS reporting for USB serial devices.
Patch 01 : change the interface of handle_dcd_change for usb serial in
order to avoid duplicating code when calling this function and to be
closer of the uart handle_dcd_change interface.
Patch 02 : this patch depends on the previous one. It is optional. It
restores the way the tty is retreived : the serial drivers used to
call tty_port_tty_get as opposed to the uart handle_dcd_change
implementation that simply get port->tty. As i don't know wich way you
prefer...
Patch 03 : this patch add the handling of dcd_change in the ftdi
driver.
Patch 04 : this patch add the callback to the line discipline
dcd_change handler.
Patch 05 : this patch is optional. While reading the code i've found
that wake_up_interruptible was often called after status processing
(ch341, 8250, ...). So i suggest to move this one.
This patchset have been tested with a pl2303 device and ftdi
device. These devices coupled to the usb serial stack introduce
latencies. I think that the jitter may depend on devices, and I'm not
even sure it is constant. But the PPS reporting works and allows to
play with it.
Thank for your comments.
Cheers.
Paul.
Paul Chavent (5):
USB : serial : remove tty arg of handle_dcd_change.
USB : serial : get protected tty in handle_dcd_change.
USB : serial : call handle_dcd_change in ftdi driver.
USB : serial : invoke dcd_change ldisc's handler.
USB : serial : pl2303 wake up after dcd status check.
drivers/usb/serial/ch341.c | 7 ++-----
drivers/usb/serial/ftdi_sio.c | 5 ++++-
drivers/usb/serial/generic.c | 13 +++++++++++--
drivers/usb/serial/pl2303.c | 11 +++--------
include/linux/usb/serial.h | 1 -
5 files changed, 20 insertions(+), 17 deletions(-)
--
1.7.12.1
Signed-off-by: Paul Chavent <[email protected]>
---
drivers/usb/serial/ftdi_sio.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index c45f9c0..2d3d3a0 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1966,8 +1966,11 @@ static int ftdi_process_packet(struct usb_serial_port *port,
port->icount.dsr++;
if (diff_status & FTDI_RS0_RI)
port->icount.rng++;
- if (diff_status & FTDI_RS0_RLSD)
+ if (diff_status & FTDI_RS0_RLSD) {
port->icount.dcd++;
+ usb_serial_handle_dcd_change(port,
+ status & FTDI_RS0_RLSD);
+ }
wake_up_interruptible(&port->port.delta_msr_wait);
priv->prev_status = status;
--
1.7.12.1
This patch depends on 72df17e... (PATCH 1). It restores the retreiving
of a protected instance of tty. As opposed to the serialcore.c
dcd_change implementation, the callers of dcd_change used to
get protected tty instance.
Signed-off-by: Paul Chavent <[email protected]>
---
drivers/usb/serial/generic.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 33f1df1..91f0592 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -566,7 +566,7 @@ void usb_serial_handle_dcd_change(struct usb_serial_port *usb_port,
unsigned int status)
{
struct tty_port *port = &usb_port->port;
- struct tty_struct *tty = port->tty;
+ struct tty_struct *tty = tty_port_tty_get(port);
dev_dbg(&usb_port->dev, "%s - status %d\n", __func__, status);
@@ -574,6 +574,8 @@ void usb_serial_handle_dcd_change(struct usb_serial_port *usb_port,
wake_up_interruptible(&port->open_wait);
else if (tty && !C_CLOCAL(tty))
tty_hangup(tty);
+
+ tty_kref_put(tty);
}
EXPORT_SYMBOL_GPL(usb_serial_handle_dcd_change);
--
1.7.12.1
In order to have the PPS line discipline to work with usb devices.
Signed-off-by: Paul Chavent <[email protected]>
---
drivers/usb/serial/generic.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 91f0592..a18a086 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -567,6 +567,13 @@ void usb_serial_handle_dcd_change(struct usb_serial_port *usb_port,
{
struct tty_port *port = &usb_port->port;
struct tty_struct *tty = tty_port_tty_get(port);
+ struct tty_ldisc *ld = tty ? tty_ldisc_ref(tty) : NULL;
+
+ if (ld) {
+ if (ld->ops->dcd_change)
+ ld->ops->dcd_change(tty, status);
+ tty_ldisc_deref(ld);
+ }
dev_dbg(&usb_port->dev, "%s - status %d\n", __func__, status);
--
1.7.12.1
Seems to be done this way in other drivers (ch341, 8250, ...).
Signed-off-by: Paul Chavent <[email protected]>
---
drivers/usb/serial/pl2303.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index 3299f3a..6bb405b 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -853,11 +853,11 @@ static void pl2303_update_line_status(struct usb_serial_port *port,
spin_unlock_irqrestore(&priv->lock, flags);
if (priv->line_status & UART_BREAK_ERROR)
usb_serial_handle_break(port);
- wake_up_interruptible(&port->port.delta_msr_wait);
-
if ((priv->line_status ^ prev_line_status) & UART_DCD)
usb_serial_handle_dcd_change(port,
priv->line_status & UART_DCD);
+
+ wake_up_interruptible(&port->port.delta_msr_wait);
}
static void pl2303_read_int_callback(struct urb *urb)
--
1.7.12.1
Do the same way as in serialcore.c for uart_handle_dcd_change. It
removes duplicated code around the usb_serial_handle_dcd_change calls.
Signed-off-by: Paul Chavent <[email protected]>
---
drivers/usb/serial/ch341.c | 7 ++-----
drivers/usb/serial/generic.c | 4 ++--
drivers/usb/serial/pl2303.c | 7 +------
include/linux/usb/serial.h | 1 -
4 files changed, 5 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index c2a4171..51c3d3a 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -481,11 +481,8 @@ static void ch341_read_int_callback(struct urb *urb)
spin_unlock_irqrestore(&priv->lock, flags);
if ((priv->line_status ^ prev_line_status) & CH341_BIT_DCD) {
- struct tty_struct *tty = tty_port_tty_get(&port->port);
- if (tty)
- usb_serial_handle_dcd_change(port, tty,
- priv->line_status & CH341_BIT_DCD);
- tty_kref_put(tty);
+ usb_serial_handle_dcd_change(port,
+ priv->line_status & CH341_BIT_DCD);
}
wake_up_interruptible(&port->port.delta_msr_wait);
diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 1f31e6b..33f1df1 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -560,13 +560,13 @@ EXPORT_SYMBOL_GPL(usb_serial_handle_break);
/**
* usb_serial_handle_dcd_change - handle a change of carrier detect state
* @port: usb_serial_port structure for the open port
- * @tty: tty_struct structure for the port
* @status: new carrier detect status, nonzero if active
*/
void usb_serial_handle_dcd_change(struct usb_serial_port *usb_port,
- struct tty_struct *tty, unsigned int status)
+ unsigned int status)
{
struct tty_port *port = &usb_port->port;
+ struct tty_struct *tty = port->tty;
dev_dbg(&usb_port->dev, "%s - status %d\n", __func__, status);
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index e7a84f0..3299f3a 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -823,7 +823,6 @@ static void pl2303_update_line_status(struct usb_serial_port *port,
{
struct pl2303_private *priv = usb_get_serial_port_data(port);
- struct tty_struct *tty;
unsigned long flags;
u8 status_idx = UART_STATE;
u8 length = UART_STATE + 1;
@@ -856,13 +855,9 @@ static void pl2303_update_line_status(struct usb_serial_port *port,
usb_serial_handle_break(port);
wake_up_interruptible(&port->port.delta_msr_wait);
- tty = tty_port_tty_get(&port->port);
- if (!tty)
- return;
if ((priv->line_status ^ prev_line_status) & UART_DCD)
- usb_serial_handle_dcd_change(port, tty,
+ usb_serial_handle_dcd_change(port,
priv->line_status & UART_DCD);
- tty_kref_put(tty);
}
static void pl2303_read_int_callback(struct urb *urb)
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index d528b80..facec97 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -347,7 +347,6 @@ extern int usb_serial_handle_sysrq_char(struct usb_serial_port *port,
unsigned int ch);
extern int usb_serial_handle_break(struct usb_serial_port *port);
extern void usb_serial_handle_dcd_change(struct usb_serial_port *usb_port,
- struct tty_struct *tty,
unsigned int status);
--
1.7.12.1
On Mon, Sep 09, 2013 at 06:01:19PM +0200, Paul Chavent wrote:
> In order to have the PPS line discipline to work with usb devices.
>
> Signed-off-by: Paul Chavent <[email protected]>
> ---
> drivers/usb/serial/generic.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
> index 91f0592..a18a086 100644
> --- a/drivers/usb/serial/generic.c
> +++ b/drivers/usb/serial/generic.c
> @@ -567,6 +567,13 @@ void usb_serial_handle_dcd_change(struct usb_serial_port *usb_port,
> {
> struct tty_port *port = &usb_port->port;
> struct tty_struct *tty = tty_port_tty_get(port);
> + struct tty_ldisc *ld = tty ? tty_ldisc_ref(tty) : NULL;
> +
> + if (ld) {
Minor nit, I hate the ? : mode of C, and as you are doing another if
statement right after it, it's pretty redundant here. So can you just
merge them together into one if chain?
thanks,
greg k-h
On Mon, Sep 09, 2013 at 06:01:16PM +0200, Paul Chavent wrote:
> Do the same way as in serialcore.c for uart_handle_dcd_change. It
> removes duplicated code around the usb_serial_handle_dcd_change calls.
>
> Signed-off-by: Paul Chavent <[email protected]>
> ---
> drivers/usb/serial/ch341.c | 7 ++-----
> drivers/usb/serial/generic.c | 4 ++--
> drivers/usb/serial/pl2303.c | 7 +------
> include/linux/usb/serial.h | 1 -
> 4 files changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index c2a4171..51c3d3a 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -481,11 +481,8 @@ static void ch341_read_int_callback(struct urb *urb)
> spin_unlock_irqrestore(&priv->lock, flags);
>
> if ((priv->line_status ^ prev_line_status) & CH341_BIT_DCD) {
> - struct tty_struct *tty = tty_port_tty_get(&port->port);
> - if (tty)
> - usb_serial_handle_dcd_change(port, tty,
> - priv->line_status & CH341_BIT_DCD);
> - tty_kref_put(tty);
> + usb_serial_handle_dcd_change(port,
> + priv->line_status & CH341_BIT_DCD);
> }
>
> wake_up_interruptible(&port->port.delta_msr_wait);
> diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
> index 1f31e6b..33f1df1 100644
> --- a/drivers/usb/serial/generic.c
> +++ b/drivers/usb/serial/generic.c
> @@ -560,13 +560,13 @@ EXPORT_SYMBOL_GPL(usb_serial_handle_break);
> /**
> * usb_serial_handle_dcd_change - handle a change of carrier detect state
> * @port: usb_serial_port structure for the open port
> - * @tty: tty_struct structure for the port
> * @status: new carrier detect status, nonzero if active
> */
> void usb_serial_handle_dcd_change(struct usb_serial_port *usb_port,
> - struct tty_struct *tty, unsigned int status)
> + unsigned int status)
> {
> struct tty_port *port = &usb_port->port;
> + struct tty_struct *tty = port->tty;
No, this is not right. There's a reason tty_port_tty_get was used. You
cannot simply remove it (even if your next patch adds it back, but
without the NULL check).
I'm actually preparing a series of changes to the MSR handling and
considered doing something like this, but came to the conclusion that
keeping the current interface was preferred (e.g. the same reference
could be used to add handle CTS changes as well). I'm refactoring at a
different level instead.
I suggest keeping the current interface for a while still, and that you
add the tty_port_tty_get to your ftdi patch instead.
Johan
On Mon, Sep 09, 2013 at 07:45:23PM +0200, Johan Hovold wrote:
> On Mon, Sep 09, 2013 at 06:01:16PM +0200, Paul Chavent wrote:
> > Do the same way as in serialcore.c for uart_handle_dcd_change. It
> > removes duplicated code around the usb_serial_handle_dcd_change calls.
> >
> > Signed-off-by: Paul Chavent <[email protected]>
> > ---
> > drivers/usb/serial/ch341.c | 7 ++-----
> > drivers/usb/serial/generic.c | 4 ++--
> > drivers/usb/serial/pl2303.c | 7 +------
> > include/linux/usb/serial.h | 1 -
> > 4 files changed, 5 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> > index c2a4171..51c3d3a 100644
> > --- a/drivers/usb/serial/ch341.c
> > +++ b/drivers/usb/serial/ch341.c
> > @@ -481,11 +481,8 @@ static void ch341_read_int_callback(struct urb *urb)
> > spin_unlock_irqrestore(&priv->lock, flags);
> >
> > if ((priv->line_status ^ prev_line_status) & CH341_BIT_DCD) {
> > - struct tty_struct *tty = tty_port_tty_get(&port->port);
> > - if (tty)
> > - usb_serial_handle_dcd_change(port, tty,
> > - priv->line_status & CH341_BIT_DCD);
> > - tty_kref_put(tty);
> > + usb_serial_handle_dcd_change(port,
> > + priv->line_status & CH341_BIT_DCD);
> > }
> >
> > wake_up_interruptible(&port->port.delta_msr_wait);
> > diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
> > index 1f31e6b..33f1df1 100644
> > --- a/drivers/usb/serial/generic.c
> > +++ b/drivers/usb/serial/generic.c
> > @@ -560,13 +560,13 @@ EXPORT_SYMBOL_GPL(usb_serial_handle_break);
> > /**
> > * usb_serial_handle_dcd_change - handle a change of carrier detect state
> > * @port: usb_serial_port structure for the open port
> > - * @tty: tty_struct structure for the port
> > * @status: new carrier detect status, nonzero if active
> > */
> > void usb_serial_handle_dcd_change(struct usb_serial_port *usb_port,
> > - struct tty_struct *tty, unsigned int status)
> > + unsigned int status)
> > {
> > struct tty_port *port = &usb_port->port;
> > + struct tty_struct *tty = port->tty;
>
> No, this is not right. There's a reason tty_port_tty_get was used. You
> cannot simply remove it (even if your next patch adds it back, but
> without the NULL check).
My bad, the NULL check was already there. But this still would have to
be done as one patch, although I prefer keeping the current interface
for now.
Johan
Hello.
On 09/09/2013 08:01 PM, Paul Chavent wrote:
> This patch depends on 72df17e... (PATCH 1).
You don't need to say that when you publish the patches as a series. It's
assumed.
> It restores the retreiving
> of a protected instance of tty. As opposed to the serialcore.c
> dcd_change implementation, the callers of dcd_change used to
> get protected tty instance.
> Signed-off-by: Paul Chavent <[email protected]>
WBR, Sergei
On Mon, Sep 09, 2013 at 06:01:15PM +0200, Paul Chavent wrote:
> Hi.
>
> This series enable the PPS reporting for USB serial devices.
I have nothing against with this solution but consider that reporting
a PPS signal through USB bus may add unknown delays that may vanish
clock stability... as far as I know you should not use USB serial port
with PPS. Have you some statistics about clock stability in a real
implementation?
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
Hi.
At the bottom of the description of the patch set, i warn about the
latencies and jitter.
I did "real" test with a pps generated by a workstation (i haven't
tested with a precise pps yet), so the the jitter don't mean anything.
But the magnitude of difference between uart and usb pps isn't
negligible (~400?s with uart compared to ~1ms with usb).
Do you think that, depending on the application, such clock instability
is a flaw ?
I will run new tests with a precise pps source later.
I will also try to measure the latency of the pss signal through the
usb, and see if it is constant. Perhaps we could introduce parameters to
add offset to the pps signal used by the kernel consumer (the same way
we can add the offset with PPS_FETCH ioctl) later.
Regards.
Paul.
On 09/10/2013 09:31 AM, Rodolfo Giometti wrote:
> On Mon, Sep 09, 2013 at 06:01:15PM +0200, Paul Chavent wrote:
>> Hi.
>>
>> This series enable the PPS reporting for USB serial devices.
>
> I have nothing against with this solution but consider that reporting
> a PPS signal through USB bus may add unknown delays that may vanish
> clock stability... as far as I know you should not use USB serial port
> with PPS. Have you some statistics about clock stability in a real
> implementation?
>
> Ciao,
>
> Rodolfo
>
On 09/09/2013 07:45 PM, Johan Hovold wrote:
> On Mon, Sep 09, 2013 at 06:01:16PM +0200, Paul Chavent wrote:
>> Do the same way as in serialcore.c for uart_handle_dcd_change. It
>> removes duplicated code around the usb_serial_handle_dcd_change calls.
>>
>> Signed-off-by: Paul Chavent <[email protected]>
>> ---
>> drivers/usb/serial/ch341.c | 7 ++-----
>> drivers/usb/serial/generic.c | 4 ++--
>> drivers/usb/serial/pl2303.c | 7 +------
>> include/linux/usb/serial.h | 1 -
>> 4 files changed, 5 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
>> index c2a4171..51c3d3a 100644
>> --- a/drivers/usb/serial/ch341.c
>> +++ b/drivers/usb/serial/ch341.c
>> @@ -481,11 +481,8 @@ static void ch341_read_int_callback(struct urb *urb)
>> spin_unlock_irqrestore(&priv->lock, flags);
>>
>> if ((priv->line_status ^ prev_line_status) & CH341_BIT_DCD) {
>> - struct tty_struct *tty = tty_port_tty_get(&port->port);
>> - if (tty)
>> - usb_serial_handle_dcd_change(port, tty,
>> - priv->line_status & CH341_BIT_DCD);
>> - tty_kref_put(tty);
>> + usb_serial_handle_dcd_change(port,
>> + priv->line_status & CH341_BIT_DCD);
>> }
>>
>> wake_up_interruptible(&port->port.delta_msr_wait);
>> diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
>> index 1f31e6b..33f1df1 100644
>> --- a/drivers/usb/serial/generic.c
>> +++ b/drivers/usb/serial/generic.c
>> @@ -560,13 +560,13 @@ EXPORT_SYMBOL_GPL(usb_serial_handle_break);
>> /**
>> * usb_serial_handle_dcd_change - handle a change of carrier detect state
>> * @port: usb_serial_port structure for the open port
>> - * @tty: tty_struct structure for the port
>> * @status: new carrier detect status, nonzero if active
>> */
>> void usb_serial_handle_dcd_change(struct usb_serial_port *usb_port,
>> - struct tty_struct *tty, unsigned int status)
>> + unsigned int status)
>> {
>> struct tty_port *port = &usb_port->port;
>> + struct tty_struct *tty = port->tty;
>
> No, this is not right. There's a reason tty_port_tty_get was used. You
> cannot simply remove it (even if your next patch adds it back, but
> without the NULL check).
>
> I'm actually preparing a series of changes to the MSR handling and
> considered doing something like this, but came to the conclusion that
> keeping the current interface was preferred (e.g. the same reference
> could be used to add handle CTS changes as well). I'm refactoring at a
> different level instead.
>
> I suggest keeping the current interface for a while still, and that you
> add the tty_port_tty_get to your ftdi patch instead.
>
> Johan
>
Hi.
If a refactoring is in progress, the code in driver/tty/serial_core.c
should be considered too. The signature of handle_dcd_change is simply :
void uart_handle_dcd_change(struct uart_port *uport, unsigned int status);
Yo Rodolfo!
On Tue, 10 Sep 2013 09:31:05 +0200
Rodolfo Giometti <[email protected]> wrote:
> I have nothing against with this solution but consider that reporting
> a PPS signal through USB bus may add unknown delays that may vanish
> clock stability... as far as I know you should not use USB serial port
> with PPS. Have you some statistics about clock stability in a real
> implementation?
gpsd project has been using PPS over USB 1.1 for over a year. Typical
accuracy and repeatability in the range of +/- 1 milliSec. This is much
better than you can get using simple ntp over gigabit ethernet to an
adjacent stratum 1 server.
Sure PPS over a local UART is 10 to 100 times better, but often PPS
over USB is your best second choice. As serial ports become extinct
it may become your best choice.
RGDS
GARY
---------------------------------------------------------------------------
Gary E. Miller Rellim 109 NW Wilmington Ave., Suite E, Bend, OR 97701
[email protected] Tel:+1(541)382-8588
On Tue, Sep 10, 2013 at 01:02:34PM -0700, Gary E. Miller wrote:
>
> Sure PPS over a local UART is 10 to 100 times better, but often PPS
> over USB is your best second choice. As serial ports become extinct
> it may become your best choice.
Ok. But, please, add a note about it into PPS documentation and/or
(better) in configuration menu! :)
Then you can add my "Acked-by: Rodolfo Giometti <[email protected]>"
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it