linux-kernel - Re: [PATCH V1 4/4] usb: serial: f81534: add H/W disable port support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171218161547.GD3374@localhost>
Date:   Mon, 18 Dec 2017 17:15:47 +0100
From:   Johan Hovold <johan@...nel.org>
To:     "Ji-Ze Hong (Peter Hong)" <hpeter@...il.com>
Cc:     johan@...nel.org, gregkh@...uxfoundation.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Ji-Ze Hong (Peter Hong)" <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH V1 4/4] usb: serial: f81534: add H/W disable port support

On Thu, Nov 16, 2017 at 03:46:09PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> The F81532/534 can be disable port by manufacturer with
> following H/W design.
>     1: Connect DCD/DSR/CTS/RI pin to ground.
>     2: Connect RX pin to ground.
> 
> In driver, we'll implements some detect method likes following:
>     1: Read MSR.
>     2: Turn MCR LOOP bit on, off and read LSR after delay with 60ms.
>        It'll contain BREAK status in LSR.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@...il.com>
> ---
>  drivers/usb/serial/f81534.c | 74 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/drivers/usb/serial/f81534.c b/drivers/usb/serial/f81534.c
> index 30b966d71ae8..18bd2a478199 100644
> --- a/drivers/usb/serial/f81534.c
> +++ b/drivers/usb/serial/f81534.c
> @@ -751,6 +751,74 @@ static int f81534_find_config_idx(struct usb_serial *serial, u8 *index)
>  }
>  
>  /*
> + * The F81532/534 will not report serial port to USB serial subsystem when
> + * H/W DCD/DSR/CTS/RI/RX pin connected to ground.
> + *
> + * To detect RX pin status, we'll enable MCR interal loopback, disable it and
> + * delayed for 60ms. It connected to ground If LSR register report UART_LSR_BI.
> + */
> +static int f81534_check_port_hw_disabled(struct usb_serial *serial, int phy)

You treat errors as "disabled" so return a bool instead.

> +{
> +	int status;
> +	u8 old_mcr;
> +	u8 msr;
> +	u8 lsr;
> +	u8 msr_mask;
> +
> +	msr_mask = UART_MSR_DCD | UART_MSR_RI | UART_MSR_DSR | UART_MSR_CTS;
> +
> +	status = f81534_get_register(serial,
> +				F81534_MODEM_STATUS_REG + phy * 0x10, &msr);

You already have a define for the magic 0x10 that you should be using.

And you also have port register accessors that take care of the offset
for you. Perhaps add another helper which takes a phy num and use that
to implement the current accessor functions?

> +	if (status)
> +		return status;
> +
> +	if ((msr & msr_mask) != msr_mask)
> +		return 0;
> +
> +	status = f81534_set_register(serial,
> +				F81534_FIFO_CONTROL_REG + phy * 0x10,
> +				UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR |
> +				UART_FCR_CLEAR_XMIT);
> +	if (status)
> +		return status;
> +
> +	status = f81534_get_register(serial,
> +				F81534_MODEM_CONTROL_REG + phy * 0x10,
> +				&old_mcr);
> +	if (status)
> +		return status;
> +
> +	status = f81534_set_register(serial,
> +				F81534_MODEM_CONTROL_REG + phy * 0x10,
> +				UART_MCR_LOOP);
> +	if (status)
> +		return status;
> +
> +	status = f81534_set_register(serial,
> +				F81534_MODEM_CONTROL_REG + phy * 0x10, 0x0);
> +	if (status)
> +		return status;
> +
> +	msleep(60);
> +
> +	status = f81534_get_register(serial,
> +				F81534_LINE_STATUS_REG + phy * 0x10, &lsr);
> +	if (status)
> +		return status;
> +
> +	status = f81534_set_register(serial,
> +				F81534_MODEM_CONTROL_REG + phy * 0x10,
> +				old_mcr);
> +	if (status)
> +		return status;
> +
> +	if ((lsr & UART_LSR_BI) == UART_LSR_BI)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +/*
>   * We had 2 generation of F81532/534 IC. All has an internal storage.
>   *
>   * 1st is pure USB-to-TTL RS232 IC and designed for 4 ports only, no any
> @@ -832,6 +900,9 @@ static int f81534_calc_num_ports(struct usb_serial *serial,
>  
>  	/* New style, find all possible ports */
>  	for (i = 0; i < F81534_NUM_PORT; ++i) {
> +		if (f81534_check_port_hw_disabled(serial, i))
> +			continue;
> +
>  		if (setting[i] & F81534_PORT_UNAVAILABLE)
>  			continue;
>  
> @@ -1306,6 +1377,9 @@ static int f81534_attach(struct usb_serial *serial)
>  
>  	/* Assign phy-to-logic mapping */
>  	for (i = 0; i < F81534_NUM_PORT; ++i) {
> +		if (f81534_check_port_hw_disabled(serial, i))
> +			serial_priv->conf_data[i] |= F81534_PORT_UNAVAILABLE;
> +
>  		if (serial_priv->conf_data[i] & F81534_PORT_UNAVAILABLE)
>  			continue;

So this adds at least half a second during probe just from the msleep
alone. Can this be reduced somehow?

We should at least try to half that time by only doing that loopback
test once per port. You may need to use devres to allocate memory for
the result in calc_num_ports (or probe).

Johan