Component match callback function needs to check if expected data is passed to it. Without this check, it can cause a NULL pointer dereference when another driver registers a component before i915 drivers have their component master fully bind.
Fixes: 7b882fe3e3e8b ("ALSA: hda - handle multiple i915 device instances") Signed-off-by: Heikki Krogerus heikki.krogerus@linux.intel.com Signed-off-by: Mika Westerberg mika.westerberg@linux.intel.com Signed-off-by: Won Chung wonchung@google.com --- - Add "Fixes" tag - Send to stable@vger.kernel.org
sound/hda/hdac_i915.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c index efe810af28c5..958b0975fa40 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -102,13 +102,13 @@ static int i915_component_master_match(struct device *dev, int subcomponent, struct pci_dev *hdac_pci, *i915_pci; struct hdac_bus *bus = data;
- if (!dev_is_pci(dev)) + if (!dev_is_pci(dev) || !bus) return 0;
hdac_pci = to_pci_dev(bus->dev); i915_pci = to_pci_dev(dev);
- if (!strcmp(dev->driver->name, "i915") && + if (dev->driver && !strcmp(dev->driver->name, "i915") && subcomponent == I915_COMPONENT_AUDIO && connectivity_check(i915_pci, hdac_pci)) return 1;
On Wed, 30 Mar 2022 23:19:13 +0200, Won Chung wrote:
Component match callback function needs to check if expected data is passed to it. Without this check, it can cause a NULL pointer dereference when another driver registers a component before i915 drivers have their component master fully bind.
Fixes: 7b882fe3e3e8b ("ALSA: hda - handle multiple i915 device instances") Signed-off-by: Heikki Krogerus heikki.krogerus@linux.intel.com Signed-off-by: Mika Westerberg mika.westerberg@linux.intel.com Signed-off-by: Won Chung wonchung@google.com
- Add "Fixes" tag
- Send to stable@vger.kernel.org
You rather need to add "Cc: stable@vger.kernel.org" line to the patch itself (around sign-off block), not actually Cc'ing the mail. I edited manually, but please do it so at the next time.
Although I applied the patch as-is now, I wonder...
- if (!strcmp(dev->driver->name, "i915") &&
- if (dev->driver && !strcmp(dev->driver->name, "i915") &&
Can NULL dev->driver be really seen? I thought the components are added by the drivers, hence they ought to have the driver field set. But there can be corner cases I overlooked.
thanks,
Takashi
On Thu, Mar 31, 2022 at 12:27 AM Takashi Iwai tiwai@suse.de wrote:
On Wed, 30 Mar 2022 23:19:13 +0200, Won Chung wrote:
Component match callback function needs to check if expected data is passed to it. Without this check, it can cause a NULL pointer dereference when another driver registers a component before i915 drivers have their component master fully bind.
Fixes: 7b882fe3e3e8b ("ALSA: hda - handle multiple i915 device instances") Signed-off-by: Heikki Krogerus heikki.krogerus@linux.intel.com Signed-off-by: Mika Westerberg mika.westerberg@linux.intel.com Signed-off-by: Won Chung wonchung@google.com
- Add "Fixes" tag
- Send to stable@vger.kernel.org
You rather need to add "Cc: stable@vger.kernel.org" line to the patch itself (around sign-off block), not actually Cc'ing the mail. I edited manually, but please do it so at the next time.
Although I applied the patch as-is now, I wonder...
if (!strcmp(dev->driver->name, "i915") &&
if (dev->driver && !strcmp(dev->driver->name, "i915") &&Can NULL dev->driver be really seen? I thought the components are added by the drivers, hence they ought to have the driver field set. But there can be corner cases I overlooked.
thanks,
Takashi
Hi Takashi,
When I try using component_add in a different driver (usb4 in my case), I think dev->driver here is NULL because the i915 drivers do not have their component master fully bound when this new component is registered. When I test it, it seems to be causing a crash.
Would it be okay for me to resend a new patch with the flags corrected? I have mistakenly added Heikki and Mika as "Signed-off-by" instead of "Suggested-by". I am sorry for that.
Thanks, Won
On Thu, 31 Mar 2022 10:38:34 +0200, Won Chung wrote:
On Thu, Mar 31, 2022 at 12:27 AM Takashi Iwai tiwai@suse.de wrote:
On Wed, 30 Mar 2022 23:19:13 +0200, Won Chung wrote:
Component match callback function needs to check if expected data is passed to it. Without this check, it can cause a NULL pointer dereference when another driver registers a component before i915 drivers have their component master fully bind.
Fixes: 7b882fe3e3e8b ("ALSA: hda - handle multiple i915 device instances") Signed-off-by: Heikki Krogerus heikki.krogerus@linux.intel.com Signed-off-by: Mika Westerberg mika.westerberg@linux.intel.com Signed-off-by: Won Chung wonchung@google.com
- Add "Fixes" tag
- Send to stable@vger.kernel.org
You rather need to add "Cc: stable@vger.kernel.org" line to the patch itself (around sign-off block), not actually Cc'ing the mail. I edited manually, but please do it so at the next time.
Although I applied the patch as-is now, I wonder...
if (!strcmp(dev->driver->name, "i915") &&
if (dev->driver && !strcmp(dev->driver->name, "i915") &&Can NULL dev->driver be really seen? I thought the components are added by the drivers, hence they ought to have the driver field set. But there can be corner cases I overlooked.
thanks,
Takashi
Hi Takashi,
When I try using component_add in a different driver (usb4 in my case), I think dev->driver here is NULL because the i915 drivers do not have their component master fully bound when this new component is registered. When I test it, it seems to be causing a crash.
Hm, from where component_add*() is called? Basically dev->driver must be already set before the corresponding driver gets bound at __driver_probe_deviec(). So, if the device is added to component from the corresponding driver's probe, dev->driver must be non-NULL.
Would it be okay for me to resend a new patch with the flags corrected? I have mistakenly added Heikki and Mika as "Signed-off-by" instead of "Suggested-by". I am sorry for that.
Ah that's bad. I'll reset the branch, then. Please resubmit properly.
thanks,
Takashi
On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
if (!strcmp(dev->driver->name, "i915") &&
if (dev->driver && !strcmp(dev->driver->name, "i915") &&Can NULL dev->driver be really seen? I thought the components are added by the drivers, hence they ought to have the driver field set. But there can be corner cases I overlooked.
thanks,
Takashi
Hi Takashi,
When I try using component_add in a different driver (usb4 in my case), I think dev->driver here is NULL because the i915 drivers do not have their component master fully bound when this new component is registered. When I test it, it seems to be causing a crash.
Hm, from where component_add*() is called? Basically dev->driver must be already set before the corresponding driver gets bound at __driver_probe_deviec(). So, if the device is added to component from the corresponding driver's probe, dev->driver must be non-NULL.
The code that declares a device as component does not have to be the driver of that device.
In our case the components are USB ports, and they are devices that are actually never bind to any drivers: drivers/usb/core/port.c
thanks,
On Thu, 31 Mar 2022 11:25:43 +0200, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
if (!strcmp(dev->driver->name, "i915") &&
if (dev->driver && !strcmp(dev->driver->name, "i915") &&Can NULL dev->driver be really seen? I thought the components are added by the drivers, hence they ought to have the driver field set. But there can be corner cases I overlooked.
thanks,
Takashi
Hi Takashi,
When I try using component_add in a different driver (usb4 in my case), I think dev->driver here is NULL because the i915 drivers do not have their component master fully bound when this new component is registered. When I test it, it seems to be causing a crash.
Hm, from where component_add*() is called? Basically dev->driver must be already set before the corresponding driver gets bound at __driver_probe_deviec(). So, if the device is added to component from the corresponding driver's probe, dev->driver must be non-NULL.
The code that declares a device as component does not have to be the driver of that device.
In our case the components are USB ports, and they are devices that are actually never bind to any drivers: drivers/usb/core/port.c
OK, that's what I wanted to know. It'd be helpful if it's more clearly mentioned in the commit log.
BTW, the same problem must be seen in MEI drivers, too.
Takashi
On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:25:43 +0200, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
if (!strcmp(dev->driver->name, "i915") &&
if (dev->driver && !strcmp(dev->driver->name, "i915") &&Can NULL dev->driver be really seen? I thought the components are added by the drivers, hence they ought to have the driver field set. But there can be corner cases I overlooked.
thanks,
Takashi
Hi Takashi,
When I try using component_add in a different driver (usb4 in my case), I think dev->driver here is NULL because the i915 drivers do not have their component master fully bound when this new component is registered. When I test it, it seems to be causing a crash.
Hm, from where component_add*() is called? Basically dev->driver must be already set before the corresponding driver gets bound at __driver_probe_deviec(). So, if the device is added to component from the corresponding driver's probe, dev->driver must be non-NULL.
The code that declares a device as component does not have to be the driver of that device.
In our case the components are USB ports, and they are devices that are actually never bind to any drivers: drivers/usb/core/port.c
OK, that's what I wanted to know. It'd be helpful if it's more clearly mentioned in the commit log.
Agree.
BTW, the same problem must be seen in MEI drivers, too.
Wasn't there a patch for those too? I lost track...
thanks,
On Thu, 31 Mar 2022 11:34:38 +0200, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:25:43 +0200, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> - if (!strcmp(dev->driver->name, "i915") && > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
Can NULL dev->driver be really seen? I thought the components are added by the drivers, hence they ought to have the driver field set. But there can be corner cases I overlooked.
thanks,
Takashi
Hi Takashi,
When I try using component_add in a different driver (usb4 in my case), I think dev->driver here is NULL because the i915 drivers do not have their component master fully bound when this new component is registered. When I test it, it seems to be causing a crash.
Hm, from where component_add*() is called? Basically dev->driver must be already set before the corresponding driver gets bound at __driver_probe_deviec(). So, if the device is added to component from the corresponding driver's probe, dev->driver must be non-NULL.
The code that declares a device as component does not have to be the driver of that device.
In our case the components are USB ports, and they are devices that are actually never bind to any drivers: drivers/usb/core/port.c
OK, that's what I wanted to know. It'd be helpful if it's more clearly mentioned in the commit log.
Agree.
BTW, the same problem must be seen in MEI drivers, too.
Wasn't there a patch for those too? I lost track...
I don't know, I just checked the latest Linus tree.
And, looking at the HD-audio code, I still wonder how NULL dev->driver can reach there. Is there any PCI device that is added to component without binding to a driver? We have dev_is_pci() check at the beginning, so non-PCI devices should bail out there...
I'm not against adding NULL checks, but just for better understanding the situation.
Takashi
On Thu, 31 Mar 2022 11:45:47 +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:34:38 +0200, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:25:43 +0200, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > - if (!strcmp(dev->driver->name, "i915") && > > + if (dev->driver && !strcmp(dev->driver->name, "i915") && > > Can NULL dev->driver be really seen? I thought the components are > added by the drivers, hence they ought to have the driver field set. > But there can be corner cases I overlooked. > > > thanks, > > Takashi
Hi Takashi,
When I try using component_add in a different driver (usb4 in my case), I think dev->driver here is NULL because the i915 drivers do not have their component master fully bound when this new component is registered. When I test it, it seems to be causing a crash.
Hm, from where component_add*() is called? Basically dev->driver must be already set before the corresponding driver gets bound at __driver_probe_deviec(). So, if the device is added to component from the corresponding driver's probe, dev->driver must be non-NULL.
The code that declares a device as component does not have to be the driver of that device.
In our case the components are USB ports, and they are devices that are actually never bind to any drivers: drivers/usb/core/port.c
OK, that's what I wanted to know. It'd be helpful if it's more clearly mentioned in the commit log.
Agree.
BTW, the same problem must be seen in MEI drivers, too.
Wasn't there a patch for those too? I lost track...
I don't know, I just checked the latest Linus tree.
And, looking at the HD-audio code, I still wonder how NULL dev->driver can reach there. Is there any PCI device that is added to component without binding to a driver? We have dev_is_pci() check at the beginning, so non-PCI devices should bail out there...
Further reading on, I'm really confused. How data=NULL can be passed to this function? The data argument is the value passed from the component_match_add_typed() call in HD-audio driver, hence it must be always the snd_hdac_bus object.
And, I guess the i915 string check can be omitted completely, at least, for HD-audio driver. It already have a check of the parent of the device and that should be enough.
Takashi
On Thu, 31 Mar 2022 15:29:10 +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:45:47 +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:34:38 +0200, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:25:43 +0200, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> > > - if (!strcmp(dev->driver->name, "i915") && > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") && > > > > Can NULL dev->driver be really seen? I thought the components are > > added by the drivers, hence they ought to have the driver field set. > > But there can be corner cases I overlooked. > > > > > > thanks, > > > > Takashi > > Hi Takashi, > > When I try using component_add in a different driver (usb4 in my > case), I think dev->driver here is NULL because the i915 drivers do > not have their component master fully bound when this new component is > registered. When I test it, it seems to be causing a crash.
Hm, from where component_add*() is called? Basically dev->driver must be already set before the corresponding driver gets bound at __driver_probe_deviec(). So, if the device is added to component from the corresponding driver's probe, dev->driver must be non-NULL.
The code that declares a device as component does not have to be the driver of that device.
In our case the components are USB ports, and they are devices that are actually never bind to any drivers: drivers/usb/core/port.c
OK, that's what I wanted to know. It'd be helpful if it's more clearly mentioned in the commit log.
Agree.
BTW, the same problem must be seen in MEI drivers, too.
Wasn't there a patch for those too? I lost track...
I don't know, I just checked the latest Linus tree.
And, looking at the HD-audio code, I still wonder how NULL dev->driver can reach there. Is there any PCI device that is added to component without binding to a driver? We have dev_is_pci() check at the beginning, so non-PCI devices should bail out there...
Further reading on, I'm really confused. How data=NULL can be passed to this function? The data argument is the value passed from the component_match_add_typed() call in HD-audio driver, hence it must be always the snd_hdac_bus object.
And, I guess the i915 string check can be omitted completely, at least, for HD-audio driver. It already have a check of the parent of the device and that should be enough.
That said, something like below (supposing data NULL check being superfluous), instead.
Takashi
--- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -102,18 +102,13 @@ static int i915_component_master_match(struct device *dev, int subcomponent, struct pci_dev *hdac_pci, *i915_pci; struct hdac_bus *bus = data;
- if (!dev_is_pci(dev)) + if (subcomponent != I915_COMPONENT_AUDIO || !dev_is_pci(dev)) return 0;
hdac_pci = to_pci_dev(bus->dev); i915_pci = to_pci_dev(dev);
- if (!strcmp(dev->driver->name, "i915") && - subcomponent == I915_COMPONENT_AUDIO && - connectivity_check(i915_pci, hdac_pci)) - return 1; - - return 0; + return connectivity_check(i915_pci, hdac_pci); }
/* check whether intel graphics is present */
Hi Takashi,
On Thu, Mar 31, 2022 at 04:19:15PM +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 15:29:10 +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:45:47 +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:34:38 +0200, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:25:43 +0200, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote: > > > > - if (!strcmp(dev->driver->name, "i915") && > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") && > > > > > > Can NULL dev->driver be really seen? I thought the components are > > > added by the drivers, hence they ought to have the driver field set. > > > But there can be corner cases I overlooked. > > > > > > > > > thanks, > > > > > > Takashi > > > > Hi Takashi, > > > > When I try using component_add in a different driver (usb4 in my > > case), I think dev->driver here is NULL because the i915 drivers do > > not have their component master fully bound when this new component is > > registered. When I test it, it seems to be causing a crash. > > Hm, from where component_add*() is called? Basically dev->driver must > be already set before the corresponding driver gets bound at > __driver_probe_deviec(). So, if the device is added to component from > the corresponding driver's probe, dev->driver must be non-NULL.
The code that declares a device as component does not have to be the driver of that device.
In our case the components are USB ports, and they are devices that are actually never bind to any drivers: drivers/usb/core/port.c
OK, that's what I wanted to know. It'd be helpful if it's more clearly mentioned in the commit log.
Agree.
BTW, the same problem must be seen in MEI drivers, too.
Wasn't there a patch for those too? I lost track...
I don't know, I just checked the latest Linus tree.
And, looking at the HD-audio code, I still wonder how NULL dev->driver can reach there. Is there any PCI device that is added to component without binding to a driver? We have dev_is_pci() check at the beginning, so non-PCI devices should bail out there...
Further reading on, I'm really confused. How data=NULL can be passed to this function? The data argument is the value passed from the component_match_add_typed() call in HD-audio driver, hence it must be always the snd_hdac_bus object.
And, I guess the i915 string check can be omitted completely, at least, for HD-audio driver. It already have a check of the parent of the device and that should be enough.
That said, something like below (supposing data NULL check being superfluous), instead.
Takashi
--- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -102,18 +102,13 @@ static int i915_component_master_match(struct device *dev, int subcomponent, struct pci_dev *hdac_pci, *i915_pci; struct hdac_bus *bus = data;
- if (!dev_is_pci(dev))
- if (subcomponent != I915_COMPONENT_AUDIO || !dev_is_pci(dev)) return 0;
If I recall this bug correctly, it's not the usb port perse that is falling through this !dev_is_pci(dev) check, it's actually the usb4-port in a new proposed patch by Heikki and Mika to extend the usb type-c component to encompass the usb4 specific pieces too. Is it possible usb4 ports are considered pci devices, and that's how we got into this situation?
Also, a little more background information: This crash happens because in our kernel configs, we config'd the usb4 driver as =y (built in) instead of =m module, which meant that the usb4 port's driver was adding a component likely much earlier than hdac_i915.
Thanks, Benson
hdac_pci = to_pci_dev(bus->dev); i915_pci = to_pci_dev(dev);
- if (!strcmp(dev->driver->name, "i915") &&
subcomponent == I915_COMPONENT_AUDIO &&connectivity_check(i915_pci, hdac_pci))return 1;- return 0;
- return connectivity_check(i915_pci, hdac_pci);
} /* check whether intel graphics is present */
On Thu, 31 Mar 2022 17:33:03 +0200, Benson Leung wrote:
Hi Takashi,
On Thu, Mar 31, 2022 at 04:19:15PM +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 15:29:10 +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:45:47 +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:34:38 +0200, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:25:43 +0200, Heikki Krogerus wrote: > > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote: > > > > > - if (!strcmp(dev->driver->name, "i915") && > > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") && > > > > > > > > Can NULL dev->driver be really seen? I thought the components are > > > > added by the drivers, hence they ought to have the driver field set. > > > > But there can be corner cases I overlooked. > > > > > > > > > > > > thanks, > > > > > > > > Takashi > > > > > > Hi Takashi, > > > > > > When I try using component_add in a different driver (usb4 in my > > > case), I think dev->driver here is NULL because the i915 drivers do > > > not have their component master fully bound when this new component is > > > registered. When I test it, it seems to be causing a crash. > > > > Hm, from where component_add*() is called? Basically dev->driver must > > be already set before the corresponding driver gets bound at > > __driver_probe_deviec(). So, if the device is added to component from > > the corresponding driver's probe, dev->driver must be non-NULL. > > The code that declares a device as component does not have to be the > driver of that device. > > In our case the components are USB ports, and they are devices that > are actually never bind to any drivers: drivers/usb/core/port.c
OK, that's what I wanted to know. It'd be helpful if it's more clearly mentioned in the commit log.
Agree.
BTW, the same problem must be seen in MEI drivers, too.
Wasn't there a patch for those too? I lost track...
I don't know, I just checked the latest Linus tree.
And, looking at the HD-audio code, I still wonder how NULL dev->driver can reach there. Is there any PCI device that is added to component without binding to a driver? We have dev_is_pci() check at the beginning, so non-PCI devices should bail out there...
Further reading on, I'm really confused. How data=NULL can be passed to this function? The data argument is the value passed from the component_match_add_typed() call in HD-audio driver, hence it must be always the snd_hdac_bus object.
And, I guess the i915 string check can be omitted completely, at least, for HD-audio driver. It already have a check of the parent of the device and that should be enough.
That said, something like below (supposing data NULL check being superfluous), instead.
Takashi
--- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -102,18 +102,13 @@ static int i915_component_master_match(struct device *dev, int subcomponent, struct pci_dev *hdac_pci, *i915_pci; struct hdac_bus *bus = data;
- if (!dev_is_pci(dev))
- if (subcomponent != I915_COMPONENT_AUDIO || !dev_is_pci(dev)) return 0;
If I recall this bug correctly, it's not the usb port perse that is falling through this !dev_is_pci(dev) check, it's actually the usb4-port in a new proposed patch by Heikki and Mika to extend the usb type-c component to encompass the usb4 specific pieces too. Is it possible usb4 ports are considered pci devices, and that's how we got into this situation?
Yes, that explains for one of two changes in the original patch. But why data==NULL check is needed is still unclear.
Also, a little more background information: This crash happens because in our kernel configs, we config'd the usb4 driver as =y (built in) instead of =m module, which meant that the usb4 port's driver was adding a component likely much earlier than hdac_i915.
Thanks, it's what I supposed, too.
Takashi
Hi,
On Thu, Mar 31, 2022 at 08:33:03AM -0700, Benson Leung wrote:
--- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -102,18 +102,13 @@ static int i915_component_master_match(struct device *dev, int subcomponent, struct pci_dev *hdac_pci, *i915_pci; struct hdac_bus *bus = data;
- if (!dev_is_pci(dev))
- if (subcomponent != I915_COMPONENT_AUDIO || !dev_is_pci(dev)) return 0;
If I recall this bug correctly, it's not the usb port perse that is falling through this !dev_is_pci(dev) check, it's actually the usb4-port in a new proposed patch by Heikki and Mika to extend the usb type-c component to encompass the usb4 specific pieces too. Is it possible usb4 ports are considered pci devices, and that's how we got into this situation?
For usb4_port the dev_ic_pci(dev) returns false:
#define dev_is_pci(d) ((d)->bus == &pci_bus_type)
We don't attach them to PCI bus (well they are not PCI devices).
On Thu, Mar 31, 2022 at 08:33:03AM -0700, Benson Leung wrote:
Hi Takashi,
On Thu, Mar 31, 2022 at 04:19:15PM +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 15:29:10 +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:45:47 +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:34:38 +0200, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:25:43 +0200, Heikki Krogerus wrote: > > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote: > > > > > - if (!strcmp(dev->driver->name, "i915") && > > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") && > > > > > > > > Can NULL dev->driver be really seen? I thought the components are > > > > added by the drivers, hence they ought to have the driver field set. > > > > But there can be corner cases I overlooked. > > > > > > > > > > > > thanks, > > > > > > > > Takashi > > > > > > Hi Takashi, > > > > > > When I try using component_add in a different driver (usb4 in my > > > case), I think dev->driver here is NULL because the i915 drivers do > > > not have their component master fully bound when this new component is > > > registered. When I test it, it seems to be causing a crash. > > > > Hm, from where component_add*() is called? Basically dev->driver must > > be already set before the corresponding driver gets bound at > > __driver_probe_deviec(). So, if the device is added to component from > > the corresponding driver's probe, dev->driver must be non-NULL. > > The code that declares a device as component does not have to be the > driver of that device. > > In our case the components are USB ports, and they are devices that > are actually never bind to any drivers: drivers/usb/core/port.c
OK, that's what I wanted to know. It'd be helpful if it's more clearly mentioned in the commit log.
Agree.
BTW, the same problem must be seen in MEI drivers, too.
Wasn't there a patch for those too? I lost track...
I don't know, I just checked the latest Linus tree.
And, looking at the HD-audio code, I still wonder how NULL dev->driver can reach there. Is there any PCI device that is added to component without binding to a driver? We have dev_is_pci() check at the beginning, so non-PCI devices should bail out there...
Further reading on, I'm really confused. How data=NULL can be passed to this function? The data argument is the value passed from the component_match_add_typed() call in HD-audio driver, hence it must be always the snd_hdac_bus object.
And, I guess the i915 string check can be omitted completely, at least, for HD-audio driver. It already have a check of the parent of the device and that should be enough.
That said, something like below (supposing data NULL check being superfluous), instead.
Takashi
--- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -102,18 +102,13 @@ static int i915_component_master_match(struct device *dev, int subcomponent, struct pci_dev *hdac_pci, *i915_pci; struct hdac_bus *bus = data;
- if (!dev_is_pci(dev))
- if (subcomponent != I915_COMPONENT_AUDIO || !dev_is_pci(dev)) return 0;
If I recall this bug correctly, it's not the usb port perse that is falling through this !dev_is_pci(dev) check, it's actually the usb4-port in a new proposed patch by Heikki and Mika to extend the usb type-c component to encompass the usb4 specific pieces too. Is it possible usb4 ports are considered pci devices, and that's how we got into this situation?
Also, a little more background information: This crash happens because in our kernel configs, we config'd the usb4 driver as =y (built in) instead of =m module, which meant that the usb4 port's driver was adding a component likely much earlier than hdac_i915.
So is this actually triggering on 5.17 right now? Or is it due to some other not-applied changes you are testing at the moment?
confused,
greg k-h
On Thu, Mar 31, 2022 at 9:38 AM Greg KH gregkh@linuxfoundation.org wrote:
On Thu, Mar 31, 2022 at 08:33:03AM -0700, Benson Leung wrote:
Hi Takashi,
On Thu, Mar 31, 2022 at 04:19:15PM +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 15:29:10 +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:45:47 +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:34:38 +0200, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote: > On Thu, 31 Mar 2022 11:25:43 +0200, > Heikki Krogerus wrote: > > > > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote: > > > > > > - if (!strcmp(dev->driver->name, "i915") && > > > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") && > > > > > > > > > > Can NULL dev->driver be really seen? I thought the components are > > > > > added by the drivers, hence they ought to have the driver field set. > > > > > But there can be corner cases I overlooked. > > > > > > > > > > > > > > > thanks, > > > > > > > > > > Takashi > > > > > > > > Hi Takashi, > > > > > > > > When I try using component_add in a different driver (usb4 in my > > > > case), I think dev->driver here is NULL because the i915 drivers do > > > > not have their component master fully bound when this new component is > > > > registered. When I test it, it seems to be causing a crash. > > > > > > Hm, from where component_add*() is called? Basically dev->driver must > > > be already set before the corresponding driver gets bound at > > > __driver_probe_deviec(). So, if the device is added to component from > > > the corresponding driver's probe, dev->driver must be non-NULL. > > > > The code that declares a device as component does not have to be the > > driver of that device. > > > > In our case the components are USB ports, and they are devices that > > are actually never bind to any drivers: drivers/usb/core/port.c > > OK, that's what I wanted to know. It'd be helpful if it's more > clearly mentioned in the commit log.
Agree.
> BTW, the same problem must be seen in MEI drivers, too.
Wasn't there a patch for those too? I lost track...
I don't know, I just checked the latest Linus tree.
And, looking at the HD-audio code, I still wonder how NULL dev->driver can reach there. Is there any PCI device that is added to component without binding to a driver? We have dev_is_pci() check at the beginning, so non-PCI devices should bail out there...
Further reading on, I'm really confused. How data=NULL can be passed to this function? The data argument is the value passed from the component_match_add_typed() call in HD-audio driver, hence it must be always the snd_hdac_bus object.
And, I guess the i915 string check can be omitted completely, at least, for HD-audio driver. It already have a check of the parent of the device and that should be enough.
That said, something like below (supposing data NULL check being superfluous), instead.
Takashi
--- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -102,18 +102,13 @@ static int i915_component_master_match(struct device *dev, int subcomponent, struct pci_dev *hdac_pci, *i915_pci; struct hdac_bus *bus = data;
- if (!dev_is_pci(dev))
- if (subcomponent != I915_COMPONENT_AUDIO || !dev_is_pci(dev)) return 0;
If I recall this bug correctly, it's not the usb port perse that is falling through this !dev_is_pci(dev) check, it's actually the usb4-port in a new proposed patch by Heikki and Mika to extend the usb type-c component to encompass the usb4 specific pieces too. Is it possible usb4 ports are considered pci devices, and that's how we got into this situation?
Also, a little more background information: This crash happens because in our kernel configs, we config'd the usb4 driver as =y (built in) instead of =m module, which meant that the usb4 port's driver was adding a component likely much earlier than hdac_i915.
So is this actually triggering on 5.17 right now? Or is it due to some other not-applied changes you are testing at the moment?
confused,
greg k-h
Hi Greg,
I believe it is not causing an issue in 5.17 at the moment. It is triggered when we try to apply new changes and test it locally. (registering a component for usb4_port)
Thanks, Won
On Thu, Mar 31, 2022 at 09:58:43AM -0700, Won Chung wrote:
On Thu, Mar 31, 2022 at 9:38 AM Greg KH gregkh@linuxfoundation.org wrote:
On Thu, Mar 31, 2022 at 08:33:03AM -0700, Benson Leung wrote:
Hi Takashi,
On Thu, Mar 31, 2022 at 04:19:15PM +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 15:29:10 +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:45:47 +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:34:38 +0200, Heikki Krogerus wrote: > > On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote: > > On Thu, 31 Mar 2022 11:25:43 +0200, > > Heikki Krogerus wrote: > > > > > > On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote: > > > > > > > - if (!strcmp(dev->driver->name, "i915") && > > > > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") && > > > > > > > > > > > > Can NULL dev->driver be really seen? I thought the components are > > > > > > added by the drivers, hence they ought to have the driver field set. > > > > > > But there can be corner cases I overlooked. > > > > > > > > > > > > > > > > > > thanks, > > > > > > > > > > > > Takashi > > > > > > > > > > Hi Takashi, > > > > > > > > > > When I try using component_add in a different driver (usb4 in my > > > > > case), I think dev->driver here is NULL because the i915 drivers do > > > > > not have their component master fully bound when this new component is > > > > > registered. When I test it, it seems to be causing a crash. > > > > > > > > Hm, from where component_add*() is called? Basically dev->driver must > > > > be already set before the corresponding driver gets bound at > > > > __driver_probe_deviec(). So, if the device is added to component from > > > > the corresponding driver's probe, dev->driver must be non-NULL. > > > > > > The code that declares a device as component does not have to be the > > > driver of that device. > > > > > > In our case the components are USB ports, and they are devices that > > > are actually never bind to any drivers: drivers/usb/core/port.c > > > > OK, that's what I wanted to know. It'd be helpful if it's more > > clearly mentioned in the commit log. > > Agree. > > > BTW, the same problem must be seen in MEI drivers, too. > > Wasn't there a patch for those too? I lost track...
I don't know, I just checked the latest Linus tree.
And, looking at the HD-audio code, I still wonder how NULL dev->driver can reach there. Is there any PCI device that is added to component without binding to a driver? We have dev_is_pci() check at the beginning, so non-PCI devices should bail out there...
Further reading on, I'm really confused. How data=NULL can be passed to this function? The data argument is the value passed from the component_match_add_typed() call in HD-audio driver, hence it must be always the snd_hdac_bus object.
And, I guess the i915 string check can be omitted completely, at least, for HD-audio driver. It already have a check of the parent of the device and that should be enough.
That said, something like below (supposing data NULL check being superfluous), instead.
Takashi
--- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -102,18 +102,13 @@ static int i915_component_master_match(struct device *dev, int subcomponent, struct pci_dev *hdac_pci, *i915_pci; struct hdac_bus *bus = data;
- if (!dev_is_pci(dev))
- if (subcomponent != I915_COMPONENT_AUDIO || !dev_is_pci(dev)) return 0;
If I recall this bug correctly, it's not the usb port perse that is falling through this !dev_is_pci(dev) check, it's actually the usb4-port in a new proposed patch by Heikki and Mika to extend the usb type-c component to encompass the usb4 specific pieces too. Is it possible usb4 ports are considered pci devices, and that's how we got into this situation?
Also, a little more background information: This crash happens because in our kernel configs, we config'd the usb4 driver as =y (built in) instead of =m module, which meant that the usb4 port's driver was adding a component likely much earlier than hdac_i915.
So is this actually triggering on 5.17 right now? Or is it due to some other not-applied changes you are testing at the moment?
confused,
greg k-h
Hi Greg,
I believe it is not causing an issue in 5.17 at the moment. It is triggered when we try to apply new changes and test it locally. (registering a component for usb4_port)
Then why would it ever be needed to be backported to a stable kernel?
Please be more careful.
greg k-h
Hi Greg,
On Thu, Mar 31, 2022 at 07:18:00PM +0200, Greg KH wrote:
On Thu, Mar 31, 2022 at 09:58:43AM -0700, Won Chung wrote:
So is this actually triggering on 5.17 right now? Or is it due to some other not-applied changes you are testing at the moment?
confused,
greg k-h
Hi Greg,
I believe it is not causing an issue in 5.17 at the moment. It is triggered when we try to apply new changes and test it locally. (registering a component for usb4_port)
Then why would it ever be needed to be backported to a stable kernel?
Please be more careful.
greg k-h
Sorry about that. I gave Won bad advice to cc stable. You're right, it will only be relevant when a future patch lands in usb4.
Thanks, Benson
On Thu, Mar 31, 2022 at 10:33:57AM -0700, Benson Leung wrote:
Hi Greg,
On Thu, Mar 31, 2022 at 07:18:00PM +0200, Greg KH wrote:
On Thu, Mar 31, 2022 at 09:58:43AM -0700, Won Chung wrote:
So is this actually triggering on 5.17 right now? Or is it due to some other not-applied changes you are testing at the moment?
confused,
greg k-h
Hi Greg,
I believe it is not causing an issue in 5.17 at the moment. It is triggered when we try to apply new changes and test it locally. (registering a component for usb4_port)
Then why would it ever be needed to be backported to a stable kernel?
Please be more careful.
greg k-h
Sorry about that. I gave Won bad advice to cc stable. You're right, it will only be relevant when a future patch lands in usb4.
It isn't even relevant now, please only worry about this when you have your patches ready for submission that causes this breakage.
thanks,
greg k-h
Hi Takashi,
On Thu, Mar 31, 2022 at 04:19:15PM +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 15:29:10 +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:45:47 +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:34:38 +0200, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:28:20AM +0200, Takashi Iwai wrote:
On Thu, 31 Mar 2022 11:25:43 +0200, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote: > > > > - if (!strcmp(dev->driver->name, "i915") && > > > > + if (dev->driver && !strcmp(dev->driver->name, "i915") && > > > > > > Can NULL dev->driver be really seen? I thought the components are > > > added by the drivers, hence they ought to have the driver field set. > > > But there can be corner cases I overlooked. > > > > > > > > > thanks, > > > > > > Takashi > > > > Hi Takashi, > > > > When I try using component_add in a different driver (usb4 in my > > case), I think dev->driver here is NULL because the i915 drivers do > > not have their component master fully bound when this new component is > > registered. When I test it, it seems to be causing a crash. > > Hm, from where component_add*() is called? Basically dev->driver must > be already set before the corresponding driver gets bound at > __driver_probe_deviec(). So, if the device is added to component from > the corresponding driver's probe, dev->driver must be non-NULL.
The code that declares a device as component does not have to be the driver of that device.
In our case the components are USB ports, and they are devices that are actually never bind to any drivers: drivers/usb/core/port.c
OK, that's what I wanted to know. It'd be helpful if it's more clearly mentioned in the commit log.
Agree.
BTW, the same problem must be seen in MEI drivers, too.
Wasn't there a patch for those too? I lost track...
I don't know, I just checked the latest Linus tree.
And, looking at the HD-audio code, I still wonder how NULL dev->driver can reach there. Is there any PCI device that is added to component without binding to a driver? We have dev_is_pci() check at the beginning, so non-PCI devices should bail out there...
Further reading on, I'm really confused. How data=NULL can be passed to this function? The data argument is the value passed from the component_match_add_typed() call in HD-audio driver, hence it must be always the snd_hdac_bus object.
And, I guess the i915 string check can be omitted completely, at least, for HD-audio driver. It already have a check of the parent of the device and that should be enough.
That said, something like below (supposing data NULL check being superfluous), instead.
I don't think data can be NULL. There is no need for that check like you said.
--- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -102,18 +102,13 @@ static int i915_component_master_match(struct device *dev, int subcomponent, struct pci_dev *hdac_pci, *i915_pci; struct hdac_bus *bus = data;
- if (!dev_is_pci(dev))
- if (subcomponent != I915_COMPONENT_AUDIO || !dev_is_pci(dev)) return 0;
hdac_pci = to_pci_dev(bus->dev); i915_pci = to_pci_dev(dev);
- if (!strcmp(dev->driver->name, "i915") &&
subcomponent == I915_COMPONENT_AUDIO &&connectivity_check(i915_pci, hdac_pci))return 1;- return 0;
- return connectivity_check(i915_pci, hdac_pci);
} /* check whether intel graphics is present */
That looks really nice to me!
thanks,
On Thu, Mar 31, 2022 at 12:25:43PM +0300, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
if (!strcmp(dev->driver->name, "i915") &&
if (dev->driver && !strcmp(dev->driver->name, "i915") &&Can NULL dev->driver be really seen? I thought the components are added by the drivers, hence they ought to have the driver field set. But there can be corner cases I overlooked.
thanks,
Takashi
Hi Takashi,
When I try using component_add in a different driver (usb4 in my case), I think dev->driver here is NULL because the i915 drivers do not have their component master fully bound when this new component is registered. When I test it, it seems to be causing a crash.
Hm, from where component_add*() is called? Basically dev->driver must be already set before the corresponding driver gets bound at __driver_probe_deviec(). So, if the device is added to component from the corresponding driver's probe, dev->driver must be non-NULL.
The code that declares a device as component does not have to be the driver of that device.
In our case the components are USB ports, and they are devices that are actually never bind to any drivers: drivers/usb/core/port.c
Why is a USB device being passed to this code that assumes it is looking for a PCI device with a specific driver name? As I mentioned on the mei patch, triggering off of a name is really a bad idea, as is assuming the device type without any assurance it is such a device (there's a reason we didn't provide device type identification in the driver core, don't abuse that please...)
thanks,
greg k-h
Hi Greg,
On Thu, Mar 31, 2022 at 01:39:51PM +0200, Greg KH wrote:
On Thu, Mar 31, 2022 at 12:25:43PM +0300, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
if (!strcmp(dev->driver->name, "i915") &&
if (dev->driver && !strcmp(dev->driver->name, "i915") &&Can NULL dev->driver be really seen? I thought the components are added by the drivers, hence they ought to have the driver field set. But there can be corner cases I overlooked.
thanks,
Takashi
Hi Takashi,
When I try using component_add in a different driver (usb4 in my case), I think dev->driver here is NULL because the i915 drivers do not have their component master fully bound when this new component is registered. When I test it, it seems to be causing a crash.
Hm, from where component_add*() is called? Basically dev->driver must be already set before the corresponding driver gets bound at __driver_probe_deviec(). So, if the device is added to component from the corresponding driver's probe, dev->driver must be non-NULL.
The code that declares a device as component does not have to be the driver of that device.
In our case the components are USB ports, and they are devices that are actually never bind to any drivers: drivers/usb/core/port.c
Why is a USB device being passed to this code that assumes it is looking for a PCI device with a specific driver name? As I mentioned on the mei patch, triggering off of a name is really a bad idea, as is assuming the device type without any assurance it is such a device (there's a reason we didn't provide device type identification in the driver core, don't abuse that please...)
I totally agree. This driver is making a whole bunch of assumptions when it should not make any assumptions. And yes, one of those assumptions is that the driver of the device has a specific name, and that is totally crazy. So why is it making those assumptions? I have no idea, but is does, and they are now causing the first problem - NULL pointer dereference.
This patch (and that other) is only proposing a simple way to solve that NULL pointer dereference issue by adding some sanity checks. If that's no OK, and the whole driver should be refactored instead, then that is perfectly OK by me, but that has to be done by somebody who understands what exactly is the driver and the device it's controlling doing (and for).
thanks,
On Thu, Mar 31, 2022 at 03:52:14PM +0300, Heikki Krogerus wrote:
Hi Greg,
On Thu, Mar 31, 2022 at 01:39:51PM +0200, Greg KH wrote:
On Thu, Mar 31, 2022 at 12:25:43PM +0300, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> - if (!strcmp(dev->driver->name, "i915") && > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
Can NULL dev->driver be really seen? I thought the components are added by the drivers, hence they ought to have the driver field set. But there can be corner cases I overlooked.
thanks,
Takashi
Hi Takashi,
When I try using component_add in a different driver (usb4 in my case), I think dev->driver here is NULL because the i915 drivers do not have their component master fully bound when this new component is registered. When I test it, it seems to be causing a crash.
Hm, from where component_add*() is called? Basically dev->driver must be already set before the corresponding driver gets bound at __driver_probe_deviec(). So, if the device is added to component from the corresponding driver's probe, dev->driver must be non-NULL.
The code that declares a device as component does not have to be the driver of that device.
In our case the components are USB ports, and they are devices that are actually never bind to any drivers: drivers/usb/core/port.c
Why is a USB device being passed to this code that assumes it is looking for a PCI device with a specific driver name? As I mentioned on the mei patch, triggering off of a name is really a bad idea, as is assuming the device type without any assurance it is such a device (there's a reason we didn't provide device type identification in the driver core, don't abuse that please...)
I totally agree. This driver is making a whole bunch of assumptions when it should not make any assumptions. And yes, one of those assumptions is that the driver of the device has a specific name, and that is totally crazy. So why is it making those assumptions? I have no idea, but is does, and they are now causing the first problem - NULL pointer dereference.
This patch (and that other) is only proposing a simple way to solve that NULL pointer dereference issue by adding some sanity checks. If that's no OK, and the whole driver should be refactored instead, then that is perfectly OK by me, but that has to be done by somebody who understands what exactly is the driver and the device it's controlling doing (and for).
This all needs to be refactored to not do this at all.
thanks,
greg k-h
On Thu, 31 Mar 2022 14:52:14 +0200, Heikki Krogerus wrote:
Hi Greg,
On Thu, Mar 31, 2022 at 01:39:51PM +0200, Greg KH wrote:
On Thu, Mar 31, 2022 at 12:25:43PM +0300, Heikki Krogerus wrote:
On Thu, Mar 31, 2022 at 11:12:55AM +0200, Takashi Iwai wrote:
> - if (!strcmp(dev->driver->name, "i915") && > + if (dev->driver && !strcmp(dev->driver->name, "i915") &&
Can NULL dev->driver be really seen? I thought the components are added by the drivers, hence they ought to have the driver field set. But there can be corner cases I overlooked.
thanks,
Takashi
Hi Takashi,
When I try using component_add in a different driver (usb4 in my case), I think dev->driver here is NULL because the i915 drivers do not have their component master fully bound when this new component is registered. When I test it, it seems to be causing a crash.
Hm, from where component_add*() is called? Basically dev->driver must be already set before the corresponding driver gets bound at __driver_probe_deviec(). So, if the device is added to component from the corresponding driver's probe, dev->driver must be non-NULL.
The code that declares a device as component does not have to be the driver of that device.
In our case the components are USB ports, and they are devices that are actually never bind to any drivers: drivers/usb/core/port.c
Why is a USB device being passed to this code that assumes it is looking for a PCI device with a specific driver name? As I mentioned on the mei patch, triggering off of a name is really a bad idea, as is assuming the device type without any assurance it is such a device (there's a reason we didn't provide device type identification in the driver core, don't abuse that please...)
I totally agree. This driver is making a whole bunch of assumptions when it should not make any assumptions. And yes, one of those assumptions is that the driver of the device has a specific name, and that is totally crazy. So why is it making those assumptions? I have no idea, but is does, and they are now causing the first problem - NULL pointer dereference.
Well, it's a sort of best-effort approach for the component framework. Currently the framework passes a device pointer without knowing what it is and every master component tries to match with it unless it's already bound. Because of that, the driver has to judge which one is the right one by itself. The device driver's string is a loose matching target that practically worked, so far.
Maybe we should define unique subcomponent numbers and rather check the passed number at matching. (Though, only relying on the number is dangerous, too.)
Takashi
linux-stable-mirror@lists.linaro.org
-
Benson Leung -
Greg KH -
Heikki Krogerus -
Mika Westerberg -
Takashi Iwai -
Won Chung