Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 30 Dec 2019 08:07:27 -0800
From:   Guenter Roeck <linux@roeck-us.net>
To:     Douglas Gilbert <dgilbert@interlog.com>
Cc:     linux-usb@vger.kernel.org, stable@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] USB-PD tcpm: bad warning+size, PPS adapters
Message-ID: <20191230160727.GA12958@roeck-us.net>
References: <20191230033544.1809-1-dgilbert@interlog.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191230033544.1809-1-dgilbert@interlog.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Dec 29, 2019 at 10:35:44PM -0500, Douglas Gilbert wrote:
> Augmented Power Delivery Objects (A)PDO_s are used by USB-C
> PD power adapters to advertize the voltages and currents
> they support. There can be up to 7 PDO_s but before PPS
> (programmable power supply) there were seldom more than 4
> or 5. Recently Samsung released an optional PPS 45 Watt power
> adapter (EP-TA485) that has 7 PDO_s. It is for the Galaxy 10+
> tablet and charges it quicker than the adapter supplied at
> purchase. The EP-TA485 causes an overzealous WARN_ON to soil
> the log plus it miscalculates the number of bytes to read.
> 
> So this bug has been there for some time but goes
> undetected for the majority of USB-C PD power adapters on
> the market today that have 6 or less PDO_s. That may soon
> change as more USB-C PD adapters with PPS come to market.
> 
> Tested on a EP-TA485 and an older Lenovo PN: SA10M13950
> USB-C 65 Watt adapter (without PPS and has 4 PDO_s) plus
> several other PD power adapters.
> 
> Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/usb/typec/tcpm/tcpci.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index c1f7073a56de..8b4ff9fff340 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -432,20 +432,30 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  
>  	if (status & TCPC_ALERT_RX_STATUS) {
>  		struct pd_message msg;
> -		unsigned int cnt;
> +		unsigned int cnt, payload_cnt;
>  		u16 header;
>  
>  		regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
> +		/*
> +		 * 'cnt' corresponds to READABLE_BYTE_COUNT in section 4.4.14
> +		 * of the TCPCI spec [Rev 2.0 Ver 1.0 October 2017] and is
> +		 * defined in table 4-36 as one greater than the number of
> +		 * bytes received. And that number includes the header. So:
> +		 */
> +		if (cnt > 3)
> +			payload_cnt = cnt - (1 + sizeof(msg.header));
> +		else
> +			payload_cnt = 0;
>  
>  		tcpci_read16(tcpci, TCPC_RX_HDR, &header);
>  		msg.header = cpu_to_le16(header);
>  
> -		if (WARN_ON(cnt > sizeof(msg.payload)))
> -			cnt = sizeof(msg.payload);
> +		if (WARN_ON(payload_cnt > sizeof(msg.payload)))
> +			payload_cnt = sizeof(msg.payload);
>  
> -		if (cnt > 0)
> +		if (payload_cnt > 0)
>  			regmap_raw_read(tcpci->regmap, TCPC_RX_DATA,
> -					&msg.payload, cnt);
> +					&msg.payload, payload_cnt);
>  
>  		/* Read complete, clear RX status alert bit */
>  		tcpci_write16(tcpci, TCPC_ALERT, TCPC_ALERT_RX_STATUS);
> -- 
> 2.24.1
>