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
