Re: [PATCH v9 2/9] mei: late_bind: add late binding component driver

["gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>]

On Fri, Sep 12, 2025 at 12:19:21AM -0500, Lucas De Marchi wrote:
> On Tue, Sep 09, 2025 at 09:43:02AM -0500, Lucas De Marchi wrote:
> > On Tue, Sep 09, 2025 at 04:50:41AM +0000, Usyskin, Alexander wrote:
> > > > > +static int mei_lb_component_match(struct device *dev, int subcomponent,
> > > > > +				  void *data)
> > > > > +{
> > > > > +	/*
> > > > > +	 * This function checks if requester is Intel %PCI_CLASS_DISPLAY_VGA
> > > > or
> > > > > +	 * %PCI_CLASS_DISPLAY_OTHER device, and checks if the requester is
> > > > the
> > > > > +	 * grand parent of mei_if i.e. late bind MEI device
> > > > > +	 */
> > > > > +	struct device *base = data;
> > > > > +	struct pci_dev *pdev;
> > > > > +
> > > > > +	if (!dev)
> > > > > +		return 0;
> > > > > +
> > > > > +	if (!dev_is_pci(dev))
> > > > > +		return 0;
> > > > > +
> > > > > +	pdev = to_pci_dev(dev);
> > > > > +
> > > > > +	if (pdev->vendor != PCI_VENDOR_ID_INTEL)
> > > > > +		return 0;
> > > > > +
> > > > > +	if (pdev->class != (PCI_CLASS_DISPLAY_VGA << 8) &&
> > > > > +	    pdev->class != (PCI_CLASS_DISPLAY_OTHER << 8))
> > > > 
> > > > this doesn't seem right, we should allow other PCI classes. AFAICS this
> > > > check could just be removed and just leave the INTEL_COMPONENT_LB below
> > > > to protect for component match
> > > > 
> > > > Lucas De Marchi
> > > > 
> > > 
> > > The subcomponent is unique only in its own instance of the component framework.
> > > Or I'm wrong here?
> > > We have to ensure that we have Intel display device, not any other device to
> > > subcomponent check to work correctly.
> > 
> > We are matching by child-parent relationship + the component id. So you
> > have both the mei device and another pci device that added that specific
> > subcomponent and both need to have a common parent. Thinking about
> > another device that would match the parent-child relationship:  audio,
> > but audio doesn't add that.
> > 
> > what scenario would cause a false match that I'm not seeing?
> 
> so, I doesn't seem it would fail any, but it's fine as a sanity check.
> This is in fact very similar to mei_pxp_component_match(). If we are
> going to remove the display check, it could be done later on top, making
> sure not to match what it shouldn't.
> 
> So, this looks good to me. I tested this on a Battlemage card
> it's correclty loading the firmware:
> 
> 	xe 0000:03:00.0: [drm:xe_late_bind_init [xe]] Request late binding firmware xe/fan_control_8086_e20b_8086_1100.bin
> 	xe 0000:03:00.0: [drm] Using fan_control firmware from xe/fan_control_8086_e20b_8086_1100.bin version 203.0.0.0
> 	...
> 	mei_lb xe.mei-gscfi.768-e2c2afa2-3817-4d19-9d95-06b16b588a5d: bound 0000:03:00.0 (ops xe_late_bind_component_ops [xe])
> 	xe 0000:03:00.0: [drm:xe_late_bind_work [xe]] Load fan_control firmware
> 	xe 0000:03:00.0: [drm:xe_late_bind_work [xe]] Load fan_control firmware successful
> 
> 
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> Greg, does this look ok to you now for us to merge through drm?

No objection from me:

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>