From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161941AbdAIPXJ (ORCPT ); Mon, 9 Jan 2017 10:23:09 -0500 Received: from mail-lf0-f68.google.com ([209.85.215.68]:34075 "EHLO mail-lf0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161065AbdAIPVu (ORCPT ); Mon, 9 Jan 2017 10:21:50 -0500 Date: Mon, 9 Jan 2017 16:21:48 +0100 From: Johan Hovold To: "Ji-Ze Hong (Peter Hong)" Cc: johan@kernel.org, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, tom_tsai@fintek.com.tw, peter_hong@fintek.com.tw, hpeter+linux_kernel@gmail.com Subject: Re: [PATCH V1 2/2] usb:serial: Implement Fintek f81534 break on/off Message-ID: <20170109152148.GR21536@localhost> References: <1481261974-10914-1-git-send-email-hpeter+linux_kernel@gmail.com> <1481261974-10914-3-git-send-email-hpeter+linux_kernel@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1481261974-10914-3-git-send-email-hpeter+linux_kernel@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Dec 09, 2016 at 01:39:34PM +0800, Ji-Ze Hong (Peter Hong) wrote: > Implement Fintek f81534 break on/off with LCR register > It's the same with 16550A LCR register layout > > We'll add a shadow LCR variable to save the final LCR we > had set due to the "read ep0" operations maybe slow down all > the serial ports performance. > > Signed-off-by: Ji-Ze Hong (Peter Hong) > --- > drivers/usb/serial/f81534.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c > index 8282a6a..1b6ba81 100644 > --- a/drivers/usb/serial/f81534.c > +++ b/drivers/usb/serial/f81534.c > @@ -126,8 +126,10 @@ struct f81534_serial_private { > > struct f81534_port_private { > struct mutex mcr_mutex; > + struct mutex lcr_mutex; Using a single mutex is preferred. > unsigned long tx_empty; > spinlock_t msr_lock; > + u8 shadow_lcr; > u8 shadow_mcr; > u8 shadow_msr; > u8 phy_num; > @@ -683,6 +685,7 @@ static void f81534_set_termios(struct tty_struct *tty, > struct usb_serial_port *port, > struct ktermios *old_termios) > { > + struct f81534_port_private *port_priv = usb_get_serial_port_data(port); > u8 new_lcr = 0; > int status; > u32 baud; > @@ -721,6 +724,10 @@ static void f81534_set_termios(struct tty_struct *tty, > break; > } > > + mutex_lock(&port_priv->lcr_mutex); > + port_priv->shadow_lcr = new_lcr; > + mutex_unlock(&port_priv->lcr_mutex); And this is not sufficient to prevent races with break_ctl. Hold the mutex while updating LCR in set_port_config at least (and honour the break state). > + > baud = tty_get_baud_rate(tty); > if (!baud) > return; > @@ -812,6 +819,35 @@ static int f81534_read_msr(struct usb_serial_port *port) > return 0; > } > > +static void f81534_set_break(struct usb_serial_port *port, bool enable) > +{ > + struct f81534_port_private *port_priv = usb_get_serial_port_data(port); > + int status; > + > + mutex_lock(&port_priv->lcr_mutex); > + > + if (enable) > + port_priv->shadow_lcr |= UART_LCR_SBC; > + else > + port_priv->shadow_lcr &= ~UART_LCR_SBC; > + > + status = f81534_set_port_register(port, F81534_LINE_CONTROL_REG, > + port_priv->shadow_lcr); > + if (status) { > + dev_err(&port->dev, "%s: set break failed: %x\n", __func__, > + status); %d, __func__ not needed. > + } > + > + mutex_unlock(&port_priv->lcr_mutex); > +} > + > +static void f81534_break_ctl(struct tty_struct *tty, int break_state) > +{ > + struct usb_serial_port *port = tty->driver_data; > + > + f81534_set_break(port, break_state); > +} > + > static int f81534_open(struct tty_struct *tty, struct usb_serial_port *port) > { > struct f81534_serial_private *serial_priv = > @@ -877,6 +913,8 @@ static void f81534_close(struct usb_serial_port *port) > } > > mutex_unlock(&serial_priv->urb_mutex); > + > + f81534_set_break(port, false); Drop this too, and fold set_break into break_ctl above. > } > > static int f81534_get_serial_info(struct usb_serial_port *port, > @@ -1244,6 +1282,7 @@ static int f81534_port_probe(struct usb_serial_port *port) > > spin_lock_init(&port_priv->msr_lock); > mutex_init(&port_priv->mcr_mutex); > + mutex_init(&port_priv->lcr_mutex); > > /* Assign logic-to-phy mapping */ > port_priv->phy_num = f81534_logic_to_phy_port(port->serial, port); > @@ -1389,6 +1428,7 @@ static int f81534_resume(struct usb_serial *serial) > .dtr_rts = f81534_dtr_rts, > .process_read_urb = f81534_process_read_urb, > .ioctl = f81534_ioctl, > + .break_ctl = f81534_break_ctl, > .tiocmget = f81534_tiocmget, > .tiocmset = f81534_tiocmset, > .write_bulk_callback = f81534_write_usb_callback, Thanks, Johan