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 */