[PATCH v1] usb: typec: tcpm: Correct the PDO counting in pd_set

[PATCH v1] usb: typec: tcpm: Correct the PDO counting in pd_set

[Kyle Tso]

The index in the loop has already been added one and is equal to the
number of PDOs to be updated when leaving the loop.

Fixes: cd099cde4ed2 ("usb: typec: tcpm: Support multiple capabilities")
Cc: stable@vger.kernel.org
Signed-off-by: Kyle Tso <kyletso@google.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 3d505614bff1..566dad0cb9d3 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -6852,14 +6852,14 @@ static int tcpm_pd_set(struct typec_port *p, struct usb_power_delivery *pd)
 	if (data->sink_desc.pdo[0]) {
 		for (i = 0; i < PDO_MAX_OBJECTS && data->sink_desc.pdo[i]; i++)
 			port->snk_pdo[i] = data->sink_desc.pdo[i];
-		port->nr_snk_pdo = i + 1;
+		port->nr_snk_pdo = i;
 		port->operating_snk_mw = data->operating_snk_mw;
 	}
 
 	if (data->source_desc.pdo[0]) {
 		for (i = 0; i < PDO_MAX_OBJECTS && data->source_desc.pdo[i]; i++)
 			port->snk_pdo[i] = data->source_desc.pdo[i];
-		port->nr_src_pdo = i + 1;
+		port->nr_src_pdo = i;
 	}
 
 	switch (port->state) {
-- 
2.44.0.291.gc1ea87d7ee-goog

Re: [PATCH v1] usb: typec: tcpm: Correct the PDO counting in pd_set

[Greg KH]

On Tue, Mar 19, 2024 at 03:43:37PM +0800, Kyle Tso wrote:
> The index in the loop has already been added one and is equal to the
> number of PDOs to be updated when leaving the loop.

That says what is happening but not the issue that is being addressed.
What is the problem with the number being off by one?  Is this a "crash
the system" or merely "our accounting is wrong"?

thank,

greg k-h

Re: [PATCH v1] usb: typec: tcpm: Correct the PDO counting in pd_set

[Kyle Tso]

On Tue, Mar 26, 2024 at 5:30 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Mar 19, 2024 at 03:43:37PM +0800, Kyle Tso wrote:
> > The index in the loop has already been added one and is equal to the
> > number of PDOs to be updated when leaving the loop.
>
> That says what is happening but not the issue that is being addressed.
> What is the problem with the number being off by one?  Is this a "crash
> the system" or merely "our accounting is wrong"?
>
> thank,
>
> greg k-h

When doing the power negotiation, TCPM relies on the "nr_snk_pdo" as
the size of the local sink PDO array to match the Source capabilities
of the partner port. If the off-by-one overflow  occurs, a wrong RDO
might be sent and unexpected power transfer might happen such as over
voltage or over current (than expected).

"nr_src_pdo" is used to set the Rp level when the port is in Source
role. It is also the array size of the local Source capabilities when
filling up the buffer which will be sent as the Source PDOs (such as
in Power Negotiation). If the off-by-one overflow occurs, a wrong Rp
level might be set and wrong Source PDOs will be sent to the partner
port. This could potentially cause over current or port resets.

I will update the commit message in the next version of this patch.

Kyle