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

[Joerg Roedel <joro@8bytes.org>]

On Wed, Sep 07, 2011 at 12:44:45PM -0700, Greg KH wrote:
> 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?

Currently it is the platform_bus besides pci. The pci iommus are on x86
and ia64 while all arm iommus use the platform_bus (by 'iommus' I only
mean those implementing the iommu-api). Currently there are two drivers
for arm iommus in /drivers/iommu.

> 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?

Well, I havn't seen a system yet where multiple iommus are on the same
bus. Or to state it better, multiple iommus of different type that
require different drivers. There is no 1-1 mapping between real hardware
iommus and iommu_ops. There is only such a mapping for iommu drivers and
iommu_ops. An iommu driver usually handles all hardware iommus of the
same type in the system.

So having iommu_ops per-device doesn't make much sense at this point.
With this patch-set they are accessible by dev->bus->iommu_ops anyway.
But if I am wrong on this I can change this of course.

This patch-set improves the current situation where only on active
iommu-driver is allowed to be active on a system (because of the global
iommu_ops). But the main reason to put this into the bus_type structure
is that it allows to generalize the device-handling on a bus between
iommu drivers.


> 
> 
> > 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.)

As I said, I havn't seen such systems. But if they exist or are planned
I am happy to redesign the whole thing.

> > 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.

Will do, thanks.


	Joerg