On 1/8/23 16:35, Xu Yang wrote:
Hi,
-----Original Message-----
From: Xu Yang
Sent: Friday, December 16, 2022 2:41 PM
To: Guenter Roeck <linux@xxxxxxxxxxxx>; heikki.krogerus@xxxxxxxxxxxxxxx
Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; Jun Li <jun.li@xxxxxxx>
Subject: RE: [EXT] Re: [PATCH] usb: typec: tcpm: fix create duplicate source/sink-capabilities file
Hi Guenter,
-----Original Message-----
From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
Sent: Friday, December 16, 2022 1:13 PM
To: Xu Yang <xu.yang_2@xxxxxxx>; heikki.krogerus@xxxxxxxxxxxxxxx
Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; Jun Li <jun.li@xxxxxxx>
Subject: [EXT] Re: [PATCH] usb: typec: tcpm: fix create duplicate source/sink-capabilities file
Caution: EXT Email
On 12/15/22 19:31, Xu Yang wrote:
After soft reset has completed, an Explicit Contract negotiation occurs.
The sink device will receive source capabilitys again. This will cause
a duplicate source-capabilities file be created.
And the kernel will dump:
sysfs: cannot create duplicate filename '/devices/virtual/usb_power_delivery/pd1/source-capabilities'
This will unregister existing capabilities before register new one.
Fixes: 8203d26905ee ("usb: typec: tcpm: Register USB Power Delivery Capabilities")
cc: <stable@xxxxxxxxxxxxxxx>
Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
---
drivers/usb/typec/tcpm/tcpm.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 904c7b4ce2f0..02d8a704db95 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -2371,6 +2371,9 @@ static int tcpm_register_source_caps(struct tcpm_port *port)
if (IS_ERR(port->partner_pd))
return PTR_ERR(port->partner_pd);
+ /* remove existing capabilities since got new one */
+ usb_power_delivery_unregister_capabilities(port->partner_source_caps);
+
memcpy(caps.pdo, port->source_caps, sizeof(u32) * port->nr_source_caps);
caps.role = TYPEC_SOURCE;
@@ -2394,6 +2397,9 @@ static int tcpm_register_sink_caps(struct tcpm_port *port)
if (IS_ERR(port->partner_pd))
return PTR_ERR(port->partner_pd);
+ /* remove existing capabilities since got new one */
+ usb_power_delivery_unregister_capabilities(port->partner_sink_caps);
+
memcpy(caps.pdo, port->sink_caps, sizeof(u32) * port->nr_sink_caps);
caps.role = TYPEC_SINK;
Shouldn't usb_power_delivery_unregister_capabilities() be called from
the SOFT_RESET state handler ?
Although this issue is triggered by soft reset event, there is also
other possibilities which may produce this behavior. Such as received
2rd source capability or use Get_Source_Cap message. It's a dynamic
process even after source/sink is ready. So I think it's better to handle
it in tcpm_register_source/sink_caps(). Not sure if this is reasonable.
Thanks,
Xu Yang
Do you have any other comments or suggestions with this patch?
First of all, the current code is obviously wrong. If soft reset results in
pd capabilities to be invalid, that should be handled in soft reset,
just like it is handled in hard reset. Otherwise there would be stale pd
devices around which are no longer valid.
Second, if it can indeed happen that multiple source capabilities
messages are received, this should be handled as defined by the protocol.
Either the second set of messages is expected to be ignored, or it is
expected to replace existing capabilities. Either case, that situation
should be handled consciously: unregistering and re-registering
capabilities results in removal and re-creation of devices. Just doing
that unconditionally even if unnecessary (if capabilities are the same)
needs more discussion, and should not be hidden in another patch which
is supposed to address a different problem.
Guenter
