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

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(+)


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

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

Regards,

a.

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.

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

Alex


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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).
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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);
}


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html