* [PATCH v2] sound/hda: Add NULL check to component match callback function @ 2022-03-30 21:19 Won Chung 2022-03-31 7:27 ` Takashi Iwai 0 siblings, 1 reply; 25+ messages in thread From: Won Chung @ 2022-03-30 21:19 UTC (permalink / raw) To: Jaroslav Kysela, Takashi Iwai Cc: Heikki Krogerus, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable, Won Chung 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; -- 2.35.1.1021.g381101b075-goog ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-30 21:19 [PATCH v2] sound/hda: Add NULL check to component match callback function Won Chung @ 2022-03-31 7:27 ` Takashi Iwai 2022-03-31 8:38 ` Won Chung 0 siblings, 1 reply; 25+ messages in thread From: Takashi Iwai @ 2022-03-31 7:27 UTC (permalink / raw) To: Won Chung Cc: Jaroslav Kysela, Takashi Iwai, Heikki Krogerus, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 7:27 ` Takashi Iwai @ 2022-03-31 8:38 ` Won Chung 2022-03-31 9:12 ` Takashi Iwai 0 siblings, 1 reply; 25+ messages in thread From: Won Chung @ 2022-03-31 8:38 UTC (permalink / raw) To: Takashi Iwai Cc: Jaroslav Kysela, Takashi Iwai, Heikki Krogerus, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 8:38 ` Won Chung @ 2022-03-31 9:12 ` Takashi Iwai 2022-03-31 9:25 ` Heikki Krogerus 0 siblings, 1 reply; 25+ messages in thread From: Takashi Iwai @ 2022-03-31 9:12 UTC (permalink / raw) To: Won Chung Cc: Takashi Iwai, Jaroslav Kysela, Takashi Iwai, Heikki Krogerus, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 9:12 ` Takashi Iwai @ 2022-03-31 9:25 ` Heikki Krogerus 2022-03-31 9:28 ` Takashi Iwai 2022-03-31 11:39 ` Greg KH 0 siblings, 2 replies; 25+ messages in thread From: Heikki Krogerus @ 2022-03-31 9:25 UTC (permalink / raw) To: Takashi Iwai Cc: Won Chung, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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, -- heikki ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 9:25 ` Heikki Krogerus @ 2022-03-31 9:28 ` Takashi Iwai 2022-03-31 9:34 ` Heikki Krogerus 2022-03-31 11:39 ` Greg KH 1 sibling, 1 reply; 25+ messages in thread From: Takashi Iwai @ 2022-03-31 9:28 UTC (permalink / raw) To: Heikki Krogerus Cc: Takashi Iwai, Won Chung, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 9:28 ` Takashi Iwai @ 2022-03-31 9:34 ` Heikki Krogerus 2022-03-31 9:45 ` Takashi Iwai 0 siblings, 1 reply; 25+ messages in thread From: Heikki Krogerus @ 2022-03-31 9:34 UTC (permalink / raw) To: Takashi Iwai Cc: Won Chung, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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, -- heikki ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 9:34 ` Heikki Krogerus @ 2022-03-31 9:45 ` Takashi Iwai 2022-03-31 13:29 ` Takashi Iwai 0 siblings, 1 reply; 25+ messages in thread From: Takashi Iwai @ 2022-03-31 9:45 UTC (permalink / raw) To: Heikki Krogerus Cc: Takashi Iwai, Won Chung, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 9:45 ` Takashi Iwai @ 2022-03-31 13:29 ` Takashi Iwai 2022-03-31 14:19 ` Takashi Iwai 0 siblings, 1 reply; 25+ messages in thread From: Takashi Iwai @ 2022-03-31 13:29 UTC (permalink / raw) To: Heikki Krogerus Cc: Won Chung, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 13:29 ` Takashi Iwai @ 2022-03-31 14:19 ` Takashi Iwai 2022-03-31 15:33 ` Benson Leung 2022-04-01 7:02 ` Heikki Krogerus 0 siblings, 2 replies; 25+ messages in thread From: Takashi Iwai @ 2022-03-31 14:19 UTC (permalink / raw) To: Heikki Krogerus Cc: Won Chung, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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 */ ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 14:19 ` Takashi Iwai @ 2022-03-31 15:33 ` Benson Leung 2022-03-31 15:37 ` Takashi Iwai ` (2 more replies) 2022-04-01 7:02 ` Heikki Krogerus 1 sibling, 3 replies; 25+ messages in thread From: Benson Leung @ 2022-03-31 15:33 UTC (permalink / raw) To: Takashi Iwai Cc: Heikki Krogerus, Won Chung, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable [-- Attachment #1: Type: text/plain, Size: 4883 bytes --] 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 */ > -- Benson Leung Staff Software Engineer Chrome OS Kernel Google Inc. bleung@google.com Chromium OS Project bleung@chromium.org [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 15:33 ` Benson Leung @ 2022-03-31 15:37 ` Takashi Iwai 2022-03-31 16:10 ` Mika Westerberg 2022-03-31 16:38 ` Greg KH 2 siblings, 0 replies; 25+ messages in thread From: Takashi Iwai @ 2022-03-31 15:37 UTC (permalink / raw) To: Benson Leung Cc: Takashi Iwai, Heikki Krogerus, Won Chung, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 15:33 ` Benson Leung 2022-03-31 15:37 ` Takashi Iwai @ 2022-03-31 16:10 ` Mika Westerberg 2022-03-31 16:38 ` Greg KH 2 siblings, 0 replies; 25+ messages in thread From: Mika Westerberg @ 2022-03-31 16:10 UTC (permalink / raw) To: Benson Leung Cc: Takashi Iwai, Heikki Krogerus, Won Chung, Jaroslav Kysela, Takashi Iwai, Benson Leung, Prashant Malani, linux-kernel, stable 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). ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 15:33 ` Benson Leung 2022-03-31 15:37 ` Takashi Iwai 2022-03-31 16:10 ` Mika Westerberg @ 2022-03-31 16:38 ` Greg KH 2022-03-31 16:58 ` Won Chung 2 siblings, 1 reply; 25+ messages in thread From: Greg KH @ 2022-03-31 16:38 UTC (permalink / raw) To: Benson Leung Cc: Takashi Iwai, Heikki Krogerus, Won Chung, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 16:38 ` Greg KH @ 2022-03-31 16:58 ` Won Chung 2022-03-31 17:18 ` Greg KH 0 siblings, 1 reply; 25+ messages in thread From: Won Chung @ 2022-03-31 16:58 UTC (permalink / raw) To: Greg KH Cc: Benson Leung, Takashi Iwai, Heikki Krogerus, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 16:58 ` Won Chung @ 2022-03-31 17:18 ` Greg KH 2022-03-31 17:33 ` Benson Leung 0 siblings, 1 reply; 25+ messages in thread From: Greg KH @ 2022-03-31 17:18 UTC (permalink / raw) To: Won Chung Cc: Benson Leung, Takashi Iwai, Heikki Krogerus, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 17:18 ` Greg KH @ 2022-03-31 17:33 ` Benson Leung 2022-03-31 17:46 ` Greg KH 0 siblings, 1 reply; 25+ messages in thread From: Benson Leung @ 2022-03-31 17:33 UTC (permalink / raw) To: Greg KH Cc: Won Chung, Takashi Iwai, Heikki Krogerus, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable [-- Attachment #1: Type: text/plain, Size: 946 bytes --] 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 -- Benson Leung Staff Software Engineer Chrome OS Kernel Google Inc. bleung@google.com Chromium OS Project bleung@chromium.org [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 17:33 ` Benson Leung @ 2022-03-31 17:46 ` Greg KH 0 siblings, 0 replies; 25+ messages in thread From: Greg KH @ 2022-03-31 17:46 UTC (permalink / raw) To: Benson Leung Cc: Won Chung, Takashi Iwai, Heikki Krogerus, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 14:19 ` Takashi Iwai 2022-03-31 15:33 ` Benson Leung @ 2022-04-01 7:02 ` Heikki Krogerus 1 sibling, 0 replies; 25+ messages in thread From: Heikki Krogerus @ 2022-04-01 7:02 UTC (permalink / raw) To: Takashi Iwai Cc: Won Chung, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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, -- heikki ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 9:25 ` Heikki Krogerus 2022-03-31 9:28 ` Takashi Iwai @ 2022-03-31 11:39 ` Greg KH 2022-03-31 12:52 ` Heikki Krogerus 1 sibling, 1 reply; 25+ messages in thread From: Greg KH @ 2022-03-31 11:39 UTC (permalink / raw) To: Heikki Krogerus Cc: Takashi Iwai, Won Chung, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 11:39 ` Greg KH @ 2022-03-31 12:52 ` Heikki Krogerus 2022-03-31 12:56 ` Greg KH 2022-03-31 13:15 ` Takashi Iwai 0 siblings, 2 replies; 25+ messages in thread From: Heikki Krogerus @ 2022-03-31 12:52 UTC (permalink / raw) To: Greg KH Cc: Takashi Iwai, Won Chung, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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, -- heikki ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 12:52 ` Heikki Krogerus @ 2022-03-31 12:56 ` Greg KH 2022-03-31 13:15 ` Takashi Iwai 1 sibling, 0 replies; 25+ messages in thread From: Greg KH @ 2022-03-31 12:56 UTC (permalink / raw) To: Heikki Krogerus Cc: Takashi Iwai, Won Chung, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-31 12:52 ` Heikki Krogerus 2022-03-31 12:56 ` Greg KH @ 2022-03-31 13:15 ` Takashi Iwai 1 sibling, 0 replies; 25+ messages in thread From: Takashi Iwai @ 2022-03-31 13:15 UTC (permalink / raw) To: Heikki Krogerus Cc: Greg KH, Takashi Iwai, Won Chung, Jaroslav Kysela, Takashi Iwai, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2] sound/hda: Add NULL check to component match callback function @ 2022-03-30 20:55 Won Chung 2022-03-30 21:15 ` Won Chung 0 siblings, 1 reply; 25+ messages in thread From: Won Chung @ 2022-03-30 20:55 UTC (permalink / raw) To: Jaroslav Kysela, Takashi Iwai Cc: Heikki Krogerus, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable, Won Chung 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; -- 2.35.1.1021.g381101b075-goog ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2] sound/hda: Add NULL check to component match callback function 2022-03-30 20:55 Won Chung @ 2022-03-30 21:15 ` Won Chung 0 siblings, 0 replies; 25+ messages in thread From: Won Chung @ 2022-03-30 21:15 UTC (permalink / raw) To: Jaroslav Kysela, Takashi Iwai Cc: Heikki Krogerus, Mika Westerberg, Benson Leung, Prashant Malani, linux-kernel, stable On Wed, Mar 30, 2022 at 1:55 PM Won Chung <wonchung@google.com> 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 > > 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; > -- > 2.35.1.1021.g381101b075-goog > Hi, I am resending this patch to correct email accounts. Sorry for confusion. Thanks, Won ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2022-04-01 7:02 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-30 21:19 [PATCH v2] sound/hda: Add NULL check to component match callback function Won Chung 2022-03-31 7:27 ` Takashi Iwai 2022-03-31 8:38 ` Won Chung 2022-03-31 9:12 ` Takashi Iwai 2022-03-31 9:25 ` Heikki Krogerus 2022-03-31 9:28 ` Takashi Iwai 2022-03-31 9:34 ` Heikki Krogerus 2022-03-31 9:45 ` Takashi Iwai 2022-03-31 13:29 ` Takashi Iwai 2022-03-31 14:19 ` Takashi Iwai 2022-03-31 15:33 ` Benson Leung 2022-03-31 15:37 ` Takashi Iwai 2022-03-31 16:10 ` Mika Westerberg 2022-03-31 16:38 ` Greg KH 2022-03-31 16:58 ` Won Chung 2022-03-31 17:18 ` Greg KH 2022-03-31 17:33 ` Benson Leung 2022-03-31 17:46 ` Greg KH 2022-04-01 7:02 ` Heikki Krogerus 2022-03-31 11:39 ` Greg KH 2022-03-31 12:52 ` Heikki Krogerus 2022-03-31 12:56 ` Greg KH 2022-03-31 13:15 ` Takashi Iwai -- strict thread matches above, loose matches on Subject: below -- 2022-03-30 20:55 Won Chung 2022-03-30 21:15 ` Won Chung
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.