Re: [PATCH 03/13] PCI: quirk dma_func_alias for Ricoh devices

[Alex Williamson <alex.williamson@redhat.com>]

On Sat, 2014-05-10 at 12:46 +0800, Andrew Cooks wrote:
> Hi Alex
> 
> On Sat, May 3, 2014 at 1:15 PM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Sat, 2014-05-03 at 10:29 +0800, Andrew Cooks wrote:
> >> Hi Alex
> >>
> >> On Fri, May 2, 2014 at 12:27 AM, Alex Williamson
> >> <alex.williamson@redhat.com> wrote:
> >> > The existing quirk for these devices doesn't really solve the problem,
> >> > re-implement it using the DMA alias iterator.  We'll come back later
> >> > and remove the existing quirk and dma_source interface.
> >> >
> >> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >> > ---
> >> >  drivers/pci/quirks.c |   16 ++++++++++++++++
> >> >  1 file changed, 16 insertions(+)
> >> >
> >> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >> > index e729206..a458c6b 100644
> >> > --- a/drivers/pci/quirks.c
> >> > +++ b/drivers/pci/quirks.c
> >> > @@ -3333,6 +3333,22 @@ int pci_dev_specific_reset(struct pci_dev *dev, int probe)
> >> >         return -ENOTTY;
> >> >  }
> >> >
> >> > +static void quirk_dma_func0_alias(struct pci_dev *dev)
> >> > +{
> >> > +       if (PCI_SLOT(dev->devfn) != 0)
> >> > +               dev->dma_func_alias |= (1 << 0);
> >> > +}
> >> > +
> >> > +/*
> >> > + * https://bugzilla.redhat.com/show_bug.cgi?id=605888
> >> > + *
> >> > + * Some Ricoh devices use function 0 as the PCIe requester ID for DMA.
> >> > + */
> >> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe822, quirk_dma_func0_alias);
> >> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe230, quirk_dma_func0_alias);
> >> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe832, quirk_dma_func0_alias);
> >> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe832, quirk_dma_func0_alias);
> >>
> >> 0xe832 is listed twice.
> >
> > oops, copy-paste error
> >
> >> Previously only 0xe832 needed the dma alias on my thinkpad T410, which
> >> has all three devices.
> >>
> >> > +
> >> >  static struct pci_dev *pci_func_0_dma_source(struct pci_dev *dev)
> >> >  {
> >> >         if (!PCI_FUNC(dev->devfn))
> >> >
> >>
> >> Unfortunately, this quirk doesn't work for me. I tried it without
> >> modification, as well as with each alias individually. I get:
> >>
> >> Set context mapping for 0d:00.3
> >> firewire_ohci 0000:0d:00.3: added OHCI v1.10 device as card 0, 4 IR +
> >> 4 IT contexts, quirks 0x11
> >> dmar: DRHD: handling fault status reg 2
> >> dmar: DMAR:[DMA Read] Request device [0d:00.0] fault addr fffff000
> >> DMAR:[fault reason 02] Present bit in context entry is clear
> >>
> >> I think I need to see
> >> Set context mapping for 0d:00.0
> >> before
> >> Set context mapping for 0d:00.3
> >
> > It would actually be the reverse, we always set the device, then the
> > alias for the device.
> 
> Ok, excuse my ignorance, but is there a window where the driver can
> start doing DMA, before the alias is registered?
> 
> >
> >> in the log, but it's not there. I'd love to look into this and
> >> understand it properly, but I don't have time for the next four weeks.
> >>
> >> The devices are attached as follows:
> >> BDF, device ID
> >> 0d:00.0, e822
> >> 0d:00.1, e230
> >> 0d:00.3, e832
> >>
> >> The kernel log is attached.
> >
> > Hmm, there are only a few reasons why you'd never see 0d:00.3 followed
> > by 0d:00.0...
> >
> > 1) dma_func_alias bit 0 isn't getting set on 0d:00.3; we are building
> > with CONFIG_PCI_QUIRKS=y, right?
> 
> Yes.
> 
> > 2) domain_context_mapping_one called from domain_context_mapping_cb
> > returns !0; there's only one possible non-zero return for a non-vm,
> > non-si domain
> 
> domain_context_mapping_one returns 0 - I've checked.
> 
> > 3) something is broken in the first loop of pci_for_each_dma_alias; I'm
> > not seeing anything obvious
> >
> > Anyway, appreciate an debugging you're able to fit in, my only ricoh
> > device has only function 0.  Thanks,
> >
> 
> Please note that this device works with the patch I attached to
> https://bugzilla.kernel.org/show_bug.cgi?id=42679 (despite its many
> faults).

Is it by chance this:

drivers/iommu/intel-iommu.c:
static int domain_context_mapped(struct device *dev)
{
        struct intel_iommu *iommu;
        u8 bus, devfn;

        iommu = device_to_iommu(dev, &bus, &devfn);
        if (!iommu)
                return -ENODEV;

        if (dev_is_pci(dev))

// Should be !dev_is_pci

                return device_context_mapped(iommu, bus, devfn);

        return !pci_for_each_dma_alias(to_pci_dev(dev),
                                       domain_context_mapped_cb, iommu);
}