| Message ID | 20221216033150.2683718-1-xu.yang_2@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: tcpm: fix create duplicate source/sink-capabilities file | expand |
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@vger.kernel.org> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > --- > 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 ? Guenter
Hi Guenter, > -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: Friday, December 16, 2022 1:13 PM > To: Xu Yang <xu.yang_2@nxp.com>; heikki.krogerus@linux.intel.com > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Jun Li <jun.li@nxp.com> > 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@vger.kernel.org> > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > > --- > > 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 > > Guenter
Hi, > -----Original Message----- > From: Xu Yang > Sent: Friday, December 16, 2022 2:41 PM > To: Guenter Roeck <linux@roeck-us.net>; heikki.krogerus@linux.intel.com > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Jun Li <jun.li@nxp.com> > Subject: RE: [EXT] Re: [PATCH] usb: typec: tcpm: fix create duplicate source/sink-capabilities file > > Hi Guenter, > > > -----Original Message----- > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > > Sent: Friday, December 16, 2022 1:13 PM > > To: Xu Yang <xu.yang_2@nxp.com>; heikki.krogerus@linux.intel.com > > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Jun Li <jun.li@nxp.com> > > 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@vger.kernel.org> > > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > > > --- > > > 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? Thanks, Xu Yang > > > > > Guenter
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@roeck-us.net>; heikki.krogerus@linux.intel.com >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Jun Li <jun.li@nxp.com> >> Subject: RE: [EXT] Re: [PATCH] usb: typec: tcpm: fix create duplicate source/sink-capabilities file >> >> Hi Guenter, >> >>> -----Original Message----- >>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck >>> Sent: Friday, December 16, 2022 1:13 PM >>> To: Xu Yang <xu.yang_2@nxp.com>; heikki.krogerus@linux.intel.com >>> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Jun Li <jun.li@nxp.com> >>> 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@vger.kernel.org> >>>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com> >>>> --- >>>> 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
> -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: Tuesday, January 10, 2023 11:02 PM > To: Xu Yang <xu.yang_2@nxp.com>; heikki.krogerus@linux.intel.com > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Jun Li <jun.li@nxp.com> > 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@roeck-us.net>; heikki.krogerus@linux.intel.com > >> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Jun Li <jun.li@nxp.com> > >> Subject: RE: [EXT] Re: [PATCH] usb: typec: tcpm: fix create duplicate source/sink-capabilities file > >> > >> Hi Guenter, > >> > >>> -----Original Message----- > >>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > >>> Sent: Friday, December 16, 2022 1:13 PM > >>> To: Xu Yang <xu.yang_2@nxp.com>; heikki.krogerus@linux.intel.com > >>> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Jun Li <jun.li@nxp.com> > >>> 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@vger.kernel.org> > >>>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > >>>> --- > >>>> 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
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;
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@vger.kernel.org> Signed-off-by: Xu Yang <xu.yang_2@nxp.com> --- drivers/usb/typec/tcpm/tcpm.c | 6 ++++++ 1 file changed, 6 insertions(+)