Re: [PATCH 02/10] Driver core: Add iommu_ops to bus_type

[Greg KH <gregkh@suse.de>]

On Wed, Sep 07, 2011 at 09:19:19PM +0200, Joerg Roedel wrote:
> Hi Greg,
> 
> the bus_set_iommu() function will be called by the IOMMU driver. There
> can be different drivers for the same bus, depending on the hardware. On
> PCI for example, there can be the Intel or the AMD IOMMU driver that
> implement the iommu-api and that register for that bus.

Why are you pushing this down into the driver core?  What other busses
becides PCI use/need this?

If you can have a different IOMMU driver on the same bus, then wouldn't
this be a per-device thing instead of a per-bus thing?


> On Wed, Sep 07, 2011 at 11:47:50AM -0700, Greg KH wrote:
> > > +#ifdef CONFIG_IOMMU_API
> > > +int bus_set_iommu(struct bus_type *bus, struct iommu_ops *ops)
> > > +{
> > > +	if (bus->iommu_ops != NULL)
> > > +		return -EBUSY;
> > 
> > Busy?
> 
> Yes, it signals to the IOMMU driver that another driver has already
> registered for that bus. In the previous register_iommu() interface this
> was just a BUG(), but I think returning an error to the caller is
> better. It can be turned back into a BUG() if it is considered better,
> though.

Can you ever have more than one IOMMU driver per bus?  If so, this seems
wrong (see above.)

> > > +
> > > +	bus->iommu_ops = ops;
> > > +
> > > +	/* Do IOMMU specific setup for this bus-type */
> > > +	iommu_bus_init(bus, ops);
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(bus_set_iommu);
> > 
> > I don't understand what this function is for, and who would call it.
> 
> It is called by the IOMMU driver.
> 
> > Please provide kerneldoc that explains this.
> 
> Will do.
> 
> > > @@ -67,6 +68,9 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
> > >   * @resume:	Called to bring a device on this bus out of sleep mode.
> > >   * @pm:		Power management operations of this bus, callback the specific
> > >   *		device driver's pm-ops.
> > > + * @iommu_ops   IOMMU specific operations for this bus, used to attach IOMMU
> > > + *              driver implementations to a bus and allow the driver to do
> > > + *              bus-specific setup
> > 
> > So why is this just not set by the bus itself, making the above function
> > not needed at all?
> 
> The IOMMUs are usually devices on the bus itself, so they are
> initialized after the bus is set up and the devices on it are
> populated.  So the function can not be called on bus initialization
> because the IOMMU is not ready at this point.

Ok, that makes more sense, please state as much in the documentation.

thanks,

greg k-h