[PATCH 0/3] Enable PPS reporting for USB serial devices V2

[PATCH 0/3] Enable PPS reporting for USB serial devices V2

[Paul Chavent]

Hi.

This series enable the PPS reporting for USB serial devices (second
submission).

As opposed to the previous one this patch doesn't change the
usb_serial_handle_dcd_change interface, and only add the minimum of
code in order to enable the PPS over USB serial devices. It also add
some comments in the PPS documentation to warn about the unprecision
of this solution.

Patch 01 : call dcd_change handler in the ftdi driver.

Patch 02 : add the callback to the line discipline dcd_change handler.

Patch 03 : 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.

Thank for your comments.

Regards.

Paul.

Paul Chavent (3):
  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.

 Documentation/pps/pps.txt     | 15 +++++++++++++++
 drivers/usb/serial/ftdi_sio.c |  8 +++++++-
 drivers/usb/serial/generic.c  |  9 +++++++++
 drivers/usb/serial/pl2303.c   | 17 +++++++++--------
 4 files changed, 40 insertions(+), 9 deletions(-)

-- 
1.7.12.1

[PATCH 1/3] USB : serial : call handle_dcd_change in ftdi driver.

[Paul Chavent]

Signed-off-by: Paul Chavent <paul.chavent@onera.fr>
---
 drivers/usb/serial/ftdi_sio.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index c45f9c0..df66495 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1966,8 +1966,14 @@ 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++;
+			struct tty_struct *tty = tty_port_tty_get(&port->port);
+			if (tty)
+				usb_serial_handle_dcd_change(port, tty,
+						status & FTDI_RS0_RLSD);
+			tty_kref_put(tty);
+		}
 
 		wake_up_interruptible(&port->port.delta_msr_wait);
 		priv->prev_status = status;
-- 
1.7.12.1

Re: [PATCH 1/3] USB : serial : call handle_dcd_change in ftdi driver.

[Greg KH]

On Fri, Sep 13, 2013 at 05:35:11PM +0200, Paul Chavent wrote:
> Signed-off-by: Paul Chavent <paul.chavent@onera.fr>

Why?  We need more information in the changelog body to be able to
accept this.

Re: [PATCH 1/3] USB : serial : call handle_dcd_change in ftdi driver.

[Johan Hovold]

On Fri, Sep 13, 2013 at 05:35:11PM +0200, Paul Chavent wrote:
> Signed-off-by: Paul Chavent <paul.chavent@onera.fr>
> ---
>  drivers/usb/serial/ftdi_sio.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index c45f9c0..df66495 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1966,8 +1966,14 @@ 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++;
> +			struct tty_struct *tty = tty_port_tty_get(&port->port);

Please move the tty_struct definition to the top of the function (or at
least to the top of the block).

Thanks,
Johan

Re: [PATCH 1/3] USB : serial : call handle_dcd_change in ftdi driver.

[Sergei Shtylyov]

Hello.

On 09/13/2013 07:35 PM, Paul Chavent wrote:

> Signed-off-by: Paul Chavent <paul.chavent@onera.fr>
> ---
>   drivers/usb/serial/ftdi_sio.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)

> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index c45f9c0..df66495 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1966,8 +1966,14 @@ 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++;
> +			struct tty_struct *tty = tty_port_tty_get(&port->port);

    Don't mix declaration and code. And add an empty line between them please.

> +			if (tty)
> +				usb_serial_handle_dcd_change(port, tty,
> +						status & FTDI_RS0_RLSD);
> +			tty_kref_put(tty);
> +		}

WBR, Sergei

[PATCH 2/3] USB : serial : invoke dcd_change ldisc's handler.

[Paul Chavent]

Signed-off-by: Paul Chavent <paul.chavent@onera.fr>
---
 Documentation/pps/pps.txt    | 15 +++++++++++++++
 drivers/usb/serial/generic.c |  9 +++++++++
 2 files changed, 24 insertions(+)

diff --git a/Documentation/pps/pps.txt b/Documentation/pps/pps.txt
index d35dcdd..67b9a94 100644
--- a/Documentation/pps/pps.txt
+++ b/Documentation/pps/pps.txt
@@ -66,6 +66,21 @@ In LinuxPPS the PPS sources are simply char devices usually mapped
 into files /dev/pps0, /dev/pps1, etc..
 
 
+PPS with USB to serial devices
+------------------------------
+
+It is possible to grab the PPS from an USB to serial device. However,
+you should take into account the latencies and jitter introduced by
+the USB stack. Users has reported clock instability around +-1ms when
+synchronized with PPS through USB. This isn't suited for time server
+synchronisation.
+
+If your device doesn't report PPS, you can check that the feature is
+supported by its driver. Most of the time, you only need to add a call
+to usb_serial_handle_dcd_change after checking the DCD status (see
+ch341 and pl2303 examples).
+
+
 Coding example
 --------------
 
diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 1f31e6b..877d6e0 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -568,6 +568,15 @@ void usb_serial_handle_dcd_change(struct usb_serial_port *usb_port,
 {
 	struct tty_port *port = &usb_port->port;
 
+	if (tty) {
+		struct tty_ldisc *ld = tty_ldisc_ref(tty);
+		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);
 
 	if (status)
-- 
1.7.12.1

Re: [PATCH 2/3] USB : serial : invoke dcd_change ldisc's handler.

[Greg KH]

On Fri, Sep 13, 2013 at 05:35:12PM +0200, Paul Chavent wrote:
> Signed-off-by: Paul Chavent <paul.chavent@onera.fr>

Again, why?  I need a whole lot more information in here to be able to
accept this.

Yes, you provided much of this in the 0/3 message, but that doesn't end
up in the kernel.

Third time's a charm?

thanks,

greg k-h

Re: [PATCH 2/3] USB : serial : invoke dcd_change ldisc's handler.

[Johan Hovold]

On Fri, Sep 13, 2013 at 05:35:12PM +0200, Paul Chavent wrote:
> Signed-off-by: Paul Chavent <paul.chavent@onera.fr>
> ---
>  Documentation/pps/pps.txt    | 15 +++++++++++++++
>  drivers/usb/serial/generic.c |  9 +++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/pps/pps.txt b/Documentation/pps/pps.txt
> index d35dcdd..67b9a94 100644
> --- a/Documentation/pps/pps.txt
> +++ b/Documentation/pps/pps.txt
> @@ -66,6 +66,21 @@ In LinuxPPS the PPS sources are simply char devices usually mapped
>  into files /dev/pps0, /dev/pps1, etc..
>  
>  
> +PPS with USB to serial devices
> +------------------------------
> +
> +It is possible to grab the PPS from an USB to serial device. However,
> +you should take into account the latencies and jitter introduced by
> +the USB stack. Users has reported clock instability around +-1ms when
> +synchronized with PPS through USB. This isn't suited for time server
> +synchronisation.
> +
> +If your device doesn't report PPS, you can check that the feature is
> +supported by its driver. Most of the time, you only need to add a call
> +to usb_serial_handle_dcd_change after checking the DCD status (see
> +ch341 and pl2303 examples).
> +
> +
>  Coding example
>  --------------
>  
> diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
> index 1f31e6b..877d6e0 100644
> --- a/drivers/usb/serial/generic.c
> +++ b/drivers/usb/serial/generic.c
> @@ -568,6 +568,15 @@ void usb_serial_handle_dcd_change(struct usb_serial_port *usb_port,
>  {
>  	struct tty_port *port = &usb_port->port;
>  
> +	if (tty) {
> +		struct tty_ldisc *ld = tty_ldisc_ref(tty);
> +		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);

Please add your ldisc handling after the debug statement.

Thanks,
Johan

Re: [PATCH 2/3] USB : serial : invoke dcd_change ldisc's handler.

[Rodolfo Giometti]

On Fri, Sep 13, 2013 at 05:35:12PM +0200, Paul Chavent wrote:
> Signed-off-by: Paul Chavent <paul.chavent@onera.fr>

Acked-by: Rodolfo Giometti <giometti@enneenne.com>

-- 

GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: [PATCH 2/3] USB : serial : invoke dcd_change ldisc's handler.

[Sergei Shtylyov]

Hello.

On 09/13/2013 07:35 PM, Paul Chavent wrote:

> Signed-off-by: Paul Chavent <paul.chavent@onera.fr>
> ---
>   Documentation/pps/pps.txt    | 15 +++++++++++++++
>   drivers/usb/serial/generic.c |  9 +++++++++
>   2 files changed, 24 insertions(+)

> diff --git a/Documentation/pps/pps.txt b/Documentation/pps/pps.txt
> index d35dcdd..67b9a94 100644
> --- a/Documentation/pps/pps.txt
> +++ b/Documentation/pps/pps.txt
> @@ -66,6 +66,21 @@ In LinuxPPS the PPS sources are simply char devices usually mapped
>   into files /dev/pps0, /dev/pps1, etc..
>
>
> +PPS with USB to serial devices
> +------------------------------
> +
> +It is possible to grab the PPS from an USB to serial device. However,
> +you should take into account the latencies and jitter introduced by
> +the USB stack. Users has reported clock instability around +-1ms when
> +synchronized with PPS through USB. This isn't suited for time server
> +synchronisation.

    Thunderbird spellchecker underlines this word -- "synchronization" is 
probably better.

[...]
> diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
> index 1f31e6b..877d6e0 100644
> --- a/drivers/usb/serial/generic.c
> +++ b/drivers/usb/serial/generic.c
> @@ -568,6 +568,15 @@ void usb_serial_handle_dcd_change(struct usb_serial_port *usb_port,
>   {
>   	struct tty_port *port = &usb_port->port;
>
> +	if (tty) {
> +		struct tty_ldisc *ld = tty_ldisc_ref(tty);

    Insert an empty line after declaration as above -- keep the style 
consistent, please.

> +		if (ld) {
> +			if (ld->ops->dcd_change)
> +				ld->ops->dcd_change(tty, status);
> +			tty_ldisc_deref(ld);
> +		}
> +	}
> +

WBR, Sergei

[PATCH 3/3] USB : serial : pl2303 wake up after dcd status check.

[Paul Chavent]

Seems to be done this way in other drivers (ch341, 8250, ...).
And get tty reference only if dcd_change need to be called.

Signed-off-by: Paul Chavent <paul.chavent@onera.fr>
---
 drivers/usb/serial/pl2303.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index e7a84f0..8b81188 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -854,15 +854,16 @@ 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);
 
-	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,
-				priv->line_status & UART_DCD);
-	tty_kref_put(tty);
+	if ((priv->line_status ^ prev_line_status) & UART_DCD) {
+		tty = tty_port_tty_get(&port->port);
+		if (tty)
+			usb_serial_handle_dcd_change(port, tty,
+					priv->line_status & UART_DCD);
+		tty_kref_put(tty);
+	}
+
+	wake_up_interruptible(&port->port.delta_msr_wait);
 }
 
 static void pl2303_read_int_callback(struct urb *urb)
-- 
1.7.12.1

Re: [PATCH 3/3] USB : serial : pl2303 wake up after dcd status check.

[Johan Hovold]

On Fri, Sep 13, 2013 at 05:35:13PM +0200, Paul Chavent wrote:
> Seems to be done this way in other drivers (ch341, 8250, ...).
> And get tty reference only if dcd_change need to be called.

This is fine. I have a patch here doing the same two changes as part of
a larger clean-up of the pl2303 interrupt handling (which in turn is
part of the MSR-refactoring I mentioned). I could rebase on top of this,
unless you care to wait another week. :)

Thanks,
Johan

> Signed-off-by: Paul Chavent <paul.chavent@onera.fr>
> ---
>  drivers/usb/serial/pl2303.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index e7a84f0..8b81188 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -854,15 +854,16 @@ 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);
>  
> -	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,
> -				priv->line_status & UART_DCD);
> -	tty_kref_put(tty);
> +	if ((priv->line_status ^ prev_line_status) & UART_DCD) {
> +		tty = tty_port_tty_get(&port->port);
> +		if (tty)
> +			usb_serial_handle_dcd_change(port, tty,
> +					priv->line_status & UART_DCD);
> +		tty_kref_put(tty);
> +	}
> +
> +	wake_up_interruptible(&port->port.delta_msr_wait);
>  }
>  
>  static void pl2303_read_int_callback(struct urb *urb)
> -- 
> 1.7.12.1
> 

Re: [PATCH 3/3] USB : serial : pl2303 wake up after dcd status check.

[Paul Chavent]

Hi.

This patch was just a suggestion. Don't bother with rebasing. I will 
wait your changes and remove this patch from the next submission.

Cheers.
Paul.


On 09/13/2013 06:23 PM, Johan Hovold wrote:
> On Fri, Sep 13, 2013 at 05:35:13PM +0200, Paul Chavent wrote:
>> Seems to be done this way in other drivers (ch341, 8250, ...).
>> And get tty reference only if dcd_change need to be called.
>
> This is fine. I have a patch here doing the same two changes as part of
> a larger clean-up of the pl2303 interrupt handling (which in turn is
> part of the MSR-refactoring I mentioned). I could rebase on top of this,
> unless you care to wait another week. :)
>
> Thanks,
> Johan
>
>> Signed-off-by: Paul Chavent <paul.chavent@onera.fr>
>> ---
>>   drivers/usb/serial/pl2303.c | 17 +++++++++--------
>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
>> index e7a84f0..8b81188 100644
>> --- a/drivers/usb/serial/pl2303.c
>> +++ b/drivers/usb/serial/pl2303.c
>> @@ -854,15 +854,16 @@ 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);
>>
>> -	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,
>> -				priv->line_status & UART_DCD);
>> -	tty_kref_put(tty);
>> +	if ((priv->line_status ^ prev_line_status) & UART_DCD) {
>> +		tty = tty_port_tty_get(&port->port);
>> +		if (tty)
>> +			usb_serial_handle_dcd_change(port, tty,
>> +					priv->line_status & UART_DCD);
>> +		tty_kref_put(tty);
>> +	}
>> +
>> +	wake_up_interruptible(&port->port.delta_msr_wait);
>>   }
>>
>>   static void pl2303_read_int_callback(struct urb *urb)
>> --
>> 1.7.12.1
>>
>