Currently, USB port is linked to Type C connector, using the component
framework, if they share the same _PLD fields from ACPI table. Type C
port-mapper searches for devices with the same _PLD values, and
aggregate them as components.
When there is another device that share the same _PLD but does not
registers a component, Type C connector (component master) would never
be bound due to a component match entry device without a component
registered. There exists some cases where USB4 port also shares the same
_PLD with USB port and Type C connector, so we need to register a
component for USB4 ports too, linking USB4 port with Type C connector.
Otherwise, link between USB port and Type C connector would not
work either.
Due to the nature of the component framework, all registered components
are shared by all component match despite the relevance. MEI subsystems
also use the component framework to bind to i915 driver, which try to
match components registered by USB ports and USB4 ports. This can be
problematic since MEI assumes that there is a driver bound to the
component device, while USB4 port does not bind to any drivers. MEI's
component match callback functions should handle such case to avoid NULL
pointer dereference when USB4 port registers a component.
In summary this patch series
1. Fixes MEI subsystem's component match callbacks to handle a component
device without any driver bound
2. Registers a component for USB4 ports to link them to Type C
connectors, similar to USB ports.
Heikki Krogerus (1):
thunderbolt: Link USB4 ports to their USB Type-C connectors
Won Chung (1):
misc/mei: Add NULL check to component match callback functions
.../ABI/testing/sysfs-bus-thunderbolt | 10 +++++
drivers/misc/mei/hdcp/mei_hdcp.c | 2 +-
drivers/misc/mei/pxp/mei_pxp.c | 2 +-
drivers/thunderbolt/usb4_port.c | 38 +++++++++++++++++++
4 files changed, 50 insertions(+), 2 deletions(-)
--
2.36.0.rc0.470.gd361397f0d-goog
Hi,
On Mon, Apr 18, 2022 at 05:59:30PM +0000, Won Chung wrote:
> Currently, USB port is linked to Type C connector, using the component
> framework, if they share the same _PLD fields from ACPI table. Type C
> port-mapper searches for devices with the same _PLD values, and
> aggregate them as components.
>
> When there is another device that share the same _PLD but does not
> registers a component, Type C connector (component master) would never
> be bound due to a component match entry device without a component
> registered. There exists some cases where USB4 port also shares the same
> _PLD with USB port and Type C connector, so we need to register a
> component for USB4 ports too, linking USB4 port with Type C connector.
> Otherwise, link between USB port and Type C connector would not
> work either.
>
> Due to the nature of the component framework, all registered components
> are shared by all component match despite the relevance. MEI subsystems
> also use the component framework to bind to i915 driver, which try to
> match components registered by USB ports and USB4 ports. This can be
> problematic since MEI assumes that there is a driver bound to the
> component device, while USB4 port does not bind to any drivers. MEI's
> component match callback functions should handle such case to avoid NULL
> pointer dereference when USB4 port registers a component.
>
> In summary this patch series
> 1. Fixes MEI subsystem's component match callbacks to handle a component
> device without any driver bound
> 2. Registers a component for USB4 ports to link them to Type C
> connectors, similar to USB ports.
>
> Heikki Krogerus (1):
> thunderbolt: Link USB4 ports to their USB Type-C connectors
>
> Won Chung (1):
> misc/mei: Add NULL check to component match callback functions
The Thunderbolt patch looks good to me. Do you want me to take the both
patches through the Thunderbolt tree or they can go separately? I need
an ack from the mei maintainer it goes through my tree.
Currently, component_match callback functions used in mei refers to the
driver name, assuming that the component device being matched has a
driver bound. It can cause a NULL pointer dereference when a device
without a driver bound registers a component. This is due to the nature
of the component framework where all registered components are matched
in any component_match callback functions. So even if a component is
registered by a totally irrelevant device, that component is also
shared to these callbacks for i915 driver.
To prevent totally irrelevant device being matched for i915 and causing
a NULL pointer dereference for checking driver name, add a NULL check on
dev->driver to check if there is a driver bound before checking the
driver name.
In the future, the string compare on the driver name, "i915" may need to
be refactored too.
Reviewed-by: Heikki Krogerus <[email protected]>
Reviewed-by: Prashant Malani <[email protected]>
Signed-off-by: Won Chung <[email protected]>
---
drivers/misc/mei/hdcp/mei_hdcp.c | 2 +-
drivers/misc/mei/pxp/mei_pxp.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
index ec2a4fce8581..e889a8bd7ac8 100644
--- a/drivers/misc/mei/hdcp/mei_hdcp.c
+++ b/drivers/misc/mei/hdcp/mei_hdcp.c
@@ -784,7 +784,7 @@ static int mei_hdcp_component_match(struct device *dev, int subcomponent,
{
struct device *base = data;
- if (strcmp(dev->driver->name, "i915") ||
+ if (!dev->driver || strcmp(dev->driver->name, "i915") ||
subcomponent != I915_COMPONENT_HDCP)
return 0;
diff --git a/drivers/misc/mei/pxp/mei_pxp.c b/drivers/misc/mei/pxp/mei_pxp.c
index f7380d387bab..5c39457e3f53 100644
--- a/drivers/misc/mei/pxp/mei_pxp.c
+++ b/drivers/misc/mei/pxp/mei_pxp.c
@@ -131,7 +131,7 @@ static int mei_pxp_component_match(struct device *dev, int subcomponent,
{
struct device *base = data;
- if (strcmp(dev->driver->name, "i915") ||
+ if (!dev->driver || strcmp(dev->driver->name, "i915") ||
subcomponent != I915_COMPONENT_PXP)
return 0;
--
2.36.0.rc0.470.gd361397f0d-goog
On Wed, Apr 20, 2022 at 3:54 AM Mika Westerberg
<[email protected]> wrote:
>
> Hi,
>
> On Mon, Apr 18, 2022 at 05:59:30PM +0000, Won Chung wrote:
> > Currently, USB port is linked to Type C connector, using the component
> > framework, if they share the same _PLD fields from ACPI table. Type C
> > port-mapper searches for devices with the same _PLD values, and
> > aggregate them as components.
> >
> > When there is another device that share the same _PLD but does not
> > registers a component, Type C connector (component master) would never
> > be bound due to a component match entry device without a component
> > registered. There exists some cases where USB4 port also shares the same
> > _PLD with USB port and Type C connector, so we need to register a
> > component for USB4 ports too, linking USB4 port with Type C connector.
> > Otherwise, link between USB port and Type C connector would not
> > work either.
> >
> > Due to the nature of the component framework, all registered components
> > are shared by all component match despite the relevance. MEI subsystems
> > also use the component framework to bind to i915 driver, which try to
> > match components registered by USB ports and USB4 ports. This can be
> > problematic since MEI assumes that there is a driver bound to the
> > component device, while USB4 port does not bind to any drivers. MEI's
> > component match callback functions should handle such case to avoid NULL
> > pointer dereference when USB4 port registers a component.
> >
> > In summary this patch series
> > 1. Fixes MEI subsystem's component match callbacks to handle a component
> > device without any driver bound
> > 2. Registers a component for USB4 ports to link them to Type C
> > connectors, similar to USB ports.
> >
> > Heikki Krogerus (1):
> > thunderbolt: Link USB4 ports to their USB Type-C connectors
> >
> > Won Chung (1):
> > misc/mei: Add NULL check to component match callback functions
>
> The Thunderbolt patch looks good to me. Do you want me to take the both
> patches through the Thunderbolt tree or they can go separately? I need
> an ack from the mei maintainer it goes through my tree.
Hi Mika,
I would prefer the two patches to go to the same tree since it can
cause a crash with only the second patch (tbt). Would that sound okay?
Hey Alexander, would you be the right person to ask for an ack on this?
Thanks,
Won
Hi,
On Wed, Apr 20, 2022 at 09:39:25AM -0700, Won Chung wrote:
> On Wed, Apr 20, 2022 at 3:54 AM Mika Westerberg
> <[email protected]> wrote:
> >
> > Hi,
> >
> > On Mon, Apr 18, 2022 at 05:59:30PM +0000, Won Chung wrote:
> > > Currently, USB port is linked to Type C connector, using the component
> > > framework, if they share the same _PLD fields from ACPI table. Type C
> > > port-mapper searches for devices with the same _PLD values, and
> > > aggregate them as components.
> > >
> > > When there is another device that share the same _PLD but does not
> > > registers a component, Type C connector (component master) would never
> > > be bound due to a component match entry device without a component
> > > registered. There exists some cases where USB4 port also shares the same
> > > _PLD with USB port and Type C connector, so we need to register a
> > > component for USB4 ports too, linking USB4 port with Type C connector.
> > > Otherwise, link between USB port and Type C connector would not
> > > work either.
> > >
> > > Due to the nature of the component framework, all registered components
> > > are shared by all component match despite the relevance. MEI subsystems
> > > also use the component framework to bind to i915 driver, which try to
> > > match components registered by USB ports and USB4 ports. This can be
> > > problematic since MEI assumes that there is a driver bound to the
> > > component device, while USB4 port does not bind to any drivers. MEI's
> > > component match callback functions should handle such case to avoid NULL
> > > pointer dereference when USB4 port registers a component.
> > >
> > > In summary this patch series
> > > 1. Fixes MEI subsystem's component match callbacks to handle a component
> > > device without any driver bound
> > > 2. Registers a component for USB4 ports to link them to Type C
> > > connectors, similar to USB ports.
> > >
> > > Heikki Krogerus (1):
> > > thunderbolt: Link USB4 ports to their USB Type-C connectors
> > >
> > > Won Chung (1):
> > > misc/mei: Add NULL check to component match callback functions
> >
> > The Thunderbolt patch looks good to me. Do you want me to take the both
> > patches through the Thunderbolt tree or they can go separately? I need
> > an ack from the mei maintainer it goes through my tree.
>
> Hi Mika,
>
> I would prefer the two patches to go to the same tree since it can
> cause a crash with only the second patch (tbt). Would that sound okay?
Sounds good to me.
> > > > Currently, USB port is linked to Type C connector, using the component
> > > > framework, if they share the same _PLD fields from ACPI table. Type C
> > > > port-mapper searches for devices with the same _PLD values, and
> > > > aggregate them as components.
> > > >
> > > > When there is another device that share the same _PLD but does not
> > > > registers a component, Type C connector (component master) would never
> > > > be bound due to a component match entry device without a component
> > > > registered. There exists some cases where USB4 port also shares the same
> > > > _PLD with USB port and Type C connector, so we need to register a
> > > > component for USB4 ports too, linking USB4 port with Type C connector.
> > > > Otherwise, link between USB port and Type C connector would not
> > > > work either.
> > > >
> > > > Due to the nature of the component framework, all registered components
> > > > are shared by all component match despite the relevance. MEI subsystems
> > > > also use the component framework to bind to i915 driver, which try to
> > > > match components registered by USB ports and USB4 ports. This can be
> > > > problematic since MEI assumes that there is a driver bound to the
> > > > component device, while USB4 port does not bind to any drivers. MEI's
> > > > component match callback functions should handle such case to avoid
> NULL
> > > > pointer dereference when USB4 port registers a component.
> > > >
> > > > In summary this patch series
> > > > 1. Fixes MEI subsystem's component match callbacks to handle a
> component
> > > > device without any driver bound
> > > > 2. Registers a component for USB4 ports to link them to Type C
> > > > connectors, similar to USB ports.
> > > >
> > > > Heikki Krogerus (1):
> > > > thunderbolt: Link USB4 ports to their USB Type-C connectors
> > > >
> > > > Won Chung (1):
> > > > misc/mei: Add NULL check to component match callback functions
> > >
> > > The Thunderbolt patch looks good to me. Do you want me to take the both
> > > patches through the Thunderbolt tree or they can go separately? I need
> > > an ack from the mei maintainer it goes through my tree.
> >
> > Hi Mika,
> >
> > I would prefer the two patches to go to the same tree since it can
> > cause a crash with only the second patch (tbt). Would that sound okay?
>
> Sounds good to me.
Tomas is MEI maintainer and should ack MEI patch
--
Thanks,
Sasha
On Sun, Apr 24, 2022 at 2:12 PM Usyskin, Alexander
<[email protected]> wrote:
>
> > > > > Currently, USB port is linked to Type C connector, using the component
> > > > > framework, if they share the same _PLD fields from ACPI table. Type C
> > > > > port-mapper searches for devices with the same _PLD values, and
> > > > > aggregate them as components.
> > > > >
> > > > > When there is another device that share the same _PLD but does not
> > > > > registers a component, Type C connector (component master) would never
> > > > > be bound due to a component match entry device without a component
> > > > > registered. There exists some cases where USB4 port also shares the same
> > > > > _PLD with USB port and Type C connector, so we need to register a
> > > > > component for USB4 ports too, linking USB4 port with Type C connector.
> > > > > Otherwise, link between USB port and Type C connector would not
> > > > > work either.
> > > > >
> > > > > Due to the nature of the component framework, all registered components
> > > > > are shared by all component match despite the relevance. MEI subsystems
> > > > > also use the component framework to bind to i915 driver, which try to
> > > > > match components registered by USB ports and USB4 ports. This can be
> > > > > problematic since MEI assumes that there is a driver bound to the
> > > > > component device, while USB4 port does not bind to any drivers. MEI's
> > > > > component match callback functions should handle such case to avoid
> > NULL
> > > > > pointer dereference when USB4 port registers a component.
> > > > >
> > > > > In summary this patch series
> > > > > 1. Fixes MEI subsystem's component match callbacks to handle a
> > component
> > > > > device without any driver bound
> > > > > 2. Registers a component for USB4 ports to link them to Type C
> > > > > connectors, similar to USB ports.
> > > > >
> > > > > Heikki Krogerus (1):
> > > > > thunderbolt: Link USB4 ports to their USB Type-C connectors
> > > > >
> > > > > Won Chung (1):
> > > > > misc/mei: Add NULL check to component match callback functions
> > > >
> > > > The Thunderbolt patch looks good to me. Do you want me to take the both
> > > > patches through the Thunderbolt tree or they can go separately? I need
> > > > an ack from the mei maintainer it goes through my tree.
> > >
> > > Hi Mika,
> > >
> > > I would prefer the two patches to go to the same tree since it can
> > > cause a crash with only the second patch (tbt). Would that sound okay?
> >
> > Sounds good to me.
>
> Tomas is MEI maintainer and should ack MEI patch
>
> --
> Thanks,
> Sasha
>
>
Hi Tomas,
Can you take a look at this patch series and give an ack if it looks
okay to be merged into Mika's tree?
One of the patches adds NULL checks in MEI:
https://lore.kernel.org/all/[email protected]/
Thank you,
Won
On Tue, Apr 19, 2022 at 2:59 AM Won Chung <[email protected]> wrote:
>
> Currently, component_match callback functions used in mei refers to the
> driver name, assuming that the component device being matched has a
> driver bound. It can cause a NULL pointer dereference when a device
> without a driver bound registers a component. This is due to the nature
> of the component framework where all registered components are matched
> in any component_match callback functions. So even if a component is
> registered by a totally irrelevant device, that component is also
> shared to these callbacks for i915 driver.
>
> To prevent totally irrelevant device being matched for i915 and causing
> a NULL pointer dereference for checking driver name, add a NULL check on
> dev->driver to check if there is a driver bound before checking the
> driver name.
>
> In the future, the string compare on the driver name, "i915" may need to
> be refactored too.
>
> Reviewed-by: Heikki Krogerus <[email protected]>
> Reviewed-by: Prashant Malani <[email protected]>
> Signed-off-by: Won Chung <[email protected]>
> ---
> drivers/misc/mei/hdcp/mei_hdcp.c | 2 +-
> drivers/misc/mei/pxp/mei_pxp.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c
> index ec2a4fce8581..e889a8bd7ac8 100644
> --- a/drivers/misc/mei/hdcp/mei_hdcp.c
> +++ b/drivers/misc/mei/hdcp/mei_hdcp.c
> @@ -784,7 +784,7 @@ static int mei_hdcp_component_match(struct device *dev, int subcomponent,
> {
> struct device *base = data;
>
> - if (strcmp(dev->driver->name, "i915") ||
> + if (!dev->driver || strcmp(dev->driver->name, "i915") ||
> subcomponent != I915_COMPONENT_HDCP)
> return 0;
>
> diff --git a/drivers/misc/mei/pxp/mei_pxp.c b/drivers/misc/mei/pxp/mei_pxp.c
> index f7380d387bab..5c39457e3f53 100644
> --- a/drivers/misc/mei/pxp/mei_pxp.c
> +++ b/drivers/misc/mei/pxp/mei_pxp.c
> @@ -131,7 +131,7 @@ static int mei_pxp_component_match(struct device *dev, int subcomponent,
> {
> struct device *base = data;
>
> - if (strcmp(dev->driver->name, "i915") ||
> + if (!dev->driver || strcmp(dev->driver->name, "i915") ||
> subcomponent != I915_COMPONENT_PXP)
> return 0;
>
> --
> 2.36.0.rc0.470.gd361397f0d-goog
>
Hi Tomas,
I am adding you to this patch since you are the maintainer of MEI.
If this looks okay to you, could you also take a look at the comment
thread on the cover letter and give an ack if it is okay to be merged
into thunderbolt tree?
https://lore.kernel.org/all/[email protected]/
Thank you,
Won
>
> On Tue, Apr 19, 2022 at 2:59 AM Won Chung <[email protected]>
> wrote:
> >
> > Currently, component_match callback functions used in mei refers to
> > the driver name, assuming that the component device being matched has
> > a driver bound. It can cause a NULL pointer dereference when a device
> > without a driver bound registers a component. This is due to the
> > nature of the component framework where all registered components are
> > matched in any component_match callback functions. So even if a
> > component is registered by a totally irrelevant device, that component
> > is also shared to these callbacks for i915 driver.
> >
> > To prevent totally irrelevant device being matched for i915 and
> > causing a NULL pointer dereference for checking driver name, add a
> > NULL check on
> > dev->driver to check if there is a driver bound before checking the
> > driver name.
> >
> > In the future, the string compare on the driver name, "i915" may need
> > to be refactored too.
> >
> > Reviewed-by: Heikki Krogerus <[email protected]>
> > Reviewed-by: Prashant Malani <[email protected]>
> > Signed-off-by: Won Chung <[email protected]>
Acked-by: Tomas Winkler <[email protected]>
> > ---
> > drivers/misc/mei/hdcp/mei_hdcp.c | 2 +-
> > drivers/misc/mei/pxp/mei_pxp.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c
> > b/drivers/misc/mei/hdcp/mei_hdcp.c
> > index ec2a4fce8581..e889a8bd7ac8 100644
> > --- a/drivers/misc/mei/hdcp/mei_hdcp.c
> > +++ b/drivers/misc/mei/hdcp/mei_hdcp.c
> > @@ -784,7 +784,7 @@ static int mei_hdcp_component_match(struct
> device
> > *dev, int subcomponent, {
> > struct device *base = data;
> >
> > - if (strcmp(dev->driver->name, "i915") ||
> > + if (!dev->driver || strcmp(dev->driver->name, "i915") ||
> > subcomponent != I915_COMPONENT_HDCP)
> > return 0;
> >
> > diff --git a/drivers/misc/mei/pxp/mei_pxp.c
> > b/drivers/misc/mei/pxp/mei_pxp.c index f7380d387bab..5c39457e3f53
> > 100644
> > --- a/drivers/misc/mei/pxp/mei_pxp.c
> > +++ b/drivers/misc/mei/pxp/mei_pxp.c
> > @@ -131,7 +131,7 @@ static int mei_pxp_component_match(struct device
> > *dev, int subcomponent, {
> > struct device *base = data;
> >
> > - if (strcmp(dev->driver->name, "i915") ||
> > + if (!dev->driver || strcmp(dev->driver->name, "i915") ||
> > subcomponent != I915_COMPONENT_PXP)
> > return 0;
> >
> > --
> > 2.36.0.rc0.470.gd361397f0d-goog
> >
>
> Hi Tomas,
>
> I am adding you to this patch since you are the maintainer of MEI.
> If this looks okay to you, could you also take a look at the comment thread on
> the cover letter and give an ack if it is okay to be merged into thunderbolt
> tree?
> https://lore.kernel.org/all/20220418175932.1809770-1-
> [email protected]/
>
> Thank you,
> Won
On Tue, Apr 26, 2022 at 1:51 AM Won Chung <[email protected]> wrote:
>
> On Sun, Apr 24, 2022 at 2:12 PM Usyskin, Alexander
> <[email protected]> wrote:
> >
> > > > > > Currently, USB port is linked to Type C connector, using the component
> > > > > > framework, if they share the same _PLD fields from ACPI table. Type C
> > > > > > port-mapper searches for devices with the same _PLD values, and
> > > > > > aggregate them as components.
> > > > > >
> > > > > > When there is another device that share the same _PLD but does not
> > > > > > registers a component, Type C connector (component master) would never
> > > > > > be bound due to a component match entry device without a component
> > > > > > registered. There exists some cases where USB4 port also shares the same
> > > > > > _PLD with USB port and Type C connector, so we need to register a
> > > > > > component for USB4 ports too, linking USB4 port with Type C connector.
> > > > > > Otherwise, link between USB port and Type C connector would not
> > > > > > work either.
> > > > > >
> > > > > > Due to the nature of the component framework, all registered components
> > > > > > are shared by all component match despite the relevance. MEI subsystems
> > > > > > also use the component framework to bind to i915 driver, which try to
> > > > > > match components registered by USB ports and USB4 ports. This can be
> > > > > > problematic since MEI assumes that there is a driver bound to the
> > > > > > component device, while USB4 port does not bind to any drivers. MEI's
> > > > > > component match callback functions should handle such case to avoid
> > > NULL
> > > > > > pointer dereference when USB4 port registers a component.
> > > > > >
> > > > > > In summary this patch series
> > > > > > 1. Fixes MEI subsystem's component match callbacks to handle a
> > > component
> > > > > > device without any driver bound
> > > > > > 2. Registers a component for USB4 ports to link them to Type C
> > > > > > connectors, similar to USB ports.
> > > > > >
> > > > > > Heikki Krogerus (1):
> > > > > > thunderbolt: Link USB4 ports to their USB Type-C connectors
> > > > > >
> > > > > > Won Chung (1):
> > > > > > misc/mei: Add NULL check to component match callback functions
> > > > >
> > > > > The Thunderbolt patch looks good to me. Do you want me to take the both
> > > > > patches through the Thunderbolt tree or they can go separately? I need
> > > > > an ack from the mei maintainer it goes through my tree.
> > > >
> > > > Hi Mika,
> > > >
> > > > I would prefer the two patches to go to the same tree since it can
> > > > cause a crash with only the second patch (tbt). Would that sound okay?
> > >
> > > Sounds good to me.
> >
> > Tomas is MEI maintainer and should ack MEI patch
> >
> > --
> > Thanks,
> > Sasha
> >
> >
>
> Hi Tomas,
>
> Can you take a look at this patch series and give an ack if it looks
> okay to be merged into Mika's tree?
> One of the patches adds NULL checks in MEI:
> https://lore.kernel.org/all/[email protected]/
>
> Thank you,
> Won
Hi Mika,
Tomas has given an ack on the first patch on MEI:
https://lore.kernel.org/all/[email protected]/
Should I resend the patch with an additional "Acked-by" tag?
Or could that be added as the patch is merged into the thunderbolt tree?
Thank you,
Won
Hi,
On Fri, Apr 29, 2022 at 03:34:56AM +0900, Won Chung wrote:
> Hi Mika,
>
> Tomas has given an ack on the first patch on MEI:
> https://lore.kernel.org/all/[email protected]/
>
> Should I resend the patch with an additional "Acked-by" tag?
> Or could that be added as the patch is merged into the thunderbolt tree?
Both applied to thunderbolt.git/next, thanks!