Date: Wed, 25 Feb 2015 13:53:57 +0700
From: Johan Hovold <johan@kernel.org>
To: Peter Hung <hpeter@gmail.com>
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, Peter Hung <hpeter+linux_kernel@gmail.com>
Subject: Re: [PATCH V7 05/11] USB: f81232: implement read IIR/MSR with
 endpoint
Message-ID: <20150225065357.GW12405@localhost>
References: <1424772986-5542-1-git-send-email-hpeter+linux_kernel@gmail.com>
 <1424772986-5542-6-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: <1424772986-5542-6-git-send-email-hpeter+linux_kernel@gmail.com>
User-Agent: Mutt/1.5.22 (2013-10-16)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4466
Lines: 140

On Tue, Feb 24, 2015 at 06:16:20PM +0800, Peter Hung wrote:
> The interrupt endpoint will report current IIR. If we got IIR with MSR changed
> , We will do read MSR with interrupt_work worker to do f81232_read_msr()
> function.
> 
> We also confirmd MSR strange delta value is not locking-issue. The issue
> is set MCR & get MSR before IIR notice with MSR changed (Loopback only).

But you are still not doing the actual register access under the mutex,
so this still sounds like it could be due to the shadow registers not
matching the hardware registers.

You need to update dtr_rts as well.

> When we use RS232 loopback, assume doing RTS change will cause
> CTS change, DTR change will cause DCD/DSR change too.
> 
> Sometimes we got 7~4 bits of MSR changed but the 3~0 bits of
> MSR(delta) maybe not changed when set & get MCR rapidly.
> 
> So we add more check not only UART_MSR_ANY_DELTA but also with
> comparing DCD/RI/DSR/CTS change with old value. Because of the state
> bit is always correct, we direct save msr when read.
> 
> The following step to reproduce this problem with while loop step 1~4:
> 1. ioctl(fd, TIOCMSET, &data) to set RTS or DTR
> 2. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state
> 3. ioctl(fd, TIOCMSET, &data) to unset RTS or DTR
> 4. ioctl(fd, TIOCMGET, &data) to read CTS or DCD/DSR state
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/usb/serial/f81232.c | 106 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 98 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
> index bf072fe..339be30 100644
> --- a/drivers/usb/serial/f81232.c
> +++ b/drivers/usb/serial/f81232.c
> @@ -36,6 +36,9 @@ MODULE_DEVICE_TABLE(usb, id_table);
>  #define F81232_GET_REGISTER	0xc0
>  #define F81232_SET_REGISTER	0x40
>  
> +#define SERIAL_BASE_ADDRESS	0x0120
> +#define MODEM_STATUS_REGISTER	(0x06 + SERIAL_BASE_ADDRESS)
> +
>  #define CONTROL_DTR			0x01
>  #define CONTROL_RTS			0x02
>  
> @@ -54,6 +57,8 @@ struct f81232_private {
>  	struct mutex lock;
>  	u8 line_control;
>  	u8 modem_status;
> +	struct work_struct interrupt_work;
> +	struct usb_serial_port *port;
>  };
>  
>  static int f81232_get_register(struct usb_serial_port *port, u16 reg, u8 *data)
> @@ -130,17 +135,92 @@ static int f81232_set_register(struct usb_serial_port *port, u16 reg, u8 data)
>  	kfree(tmp);
>  	return status;
>  }
> +
> +static void f81232_read_msr(struct usb_serial_port *port)
> +{
> +	int status;
> +	u8 current_msr, prev_msr;
> +	u8 msr_mask = ~UART_MSR_ANY_DELTA;
> +	u8 msr_changed_bit;
> +	struct tty_struct *tty;
> +	struct f81232_private *priv = usb_get_serial_port_data(port);
> +
> +	status = f81232_get_register(port, MODEM_STATUS_REGISTER,
> +			&current_msr);
> +	if (status) {
> +		dev_err(&port->dev, "%s fail, status: %d\n", __func__, status);
> +		return;
> +	}
> +
> +	/*
> +	*  The 7~4 bits of MSR will change but the 3~0 bits of MSR(delta)
> +	*  maybe not change when set MCR & get MSR rapidly.
> +	*
> +	*  So we add more check with comparing DCD/RI/DSR/CTS
> +	*  change. and direct save msr when read.
> +	*/

The asterisks should be aligned.

> +
> +	mutex_lock(&priv->lock);
> +	prev_msr = priv->modem_status;
> +	priv->modem_status = current_msr;
> +	mutex_unlock(&priv->lock);
> +
> +	if (!(current_msr & UART_MSR_ANY_DELTA) &&
> +		!((prev_msr ^ current_msr) & msr_mask))
> +		return;
> +
> +	/* find checked delta bits set */
> +	msr_changed_bit =
> +		(current_msr & UART_MSR_ANY_DELTA) << 4;
> +
> +	/* append with not delta but changed bits */
> +	msr_changed_bit |= (prev_msr ^ current_msr) & msr_mask;
> +
> +	if (msr_changed_bit & UART_MSR_CTS)
> +		port->icount.cts++;
> +	if (msr_changed_bit & UART_MSR_DSR)
> +		port->icount.dsr++;
> +	if (msr_changed_bit & UART_MSR_RI)
> +		port->icount.rng++;
> +	if (msr_changed_bit & UART_MSR_DCD) {
> +

No empty line.

> +		port->icount.dcd++;
> +		tty = tty_port_tty_get(&port->port);
> +		if (tty) {
> +

No empty line.

> +			usb_serial_handle_dcd_change(port, tty,
> +					current_msr & UART_MSR_DCD);
> +
> +			tty_kref_put(tty);
> +		}
> +	}
> +
> +	wake_up_interruptible(&port->port.delta_msr_wait);
> +}

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/