> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Tuesday, January 10, 2023 11:02 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: Re: [EXT] Re: [PATCH] usb: typec: tcpm: fix create duplicate source/sink-capabilities file
>
> Caution: EXT Email
>
> 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.
Thanks for your comments. According to the protocol , it's not possible
for the source to send multiple capabilities. Unless the source doesn't
follow the rule. That is indeed another problem. I agree with you that
it should be handled in soft reset handler if soft reset results in pd
capabilities to be invaild. I will prepare a v2 for this case.
Thanks,
Xu Yang
>
> Guenter
