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

[Andrew Cooks <acooks@gmail.com>]

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