> -----Original Message-----
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Tuesday, June 25, 2013 10:27 AM
> To: Sethi Varun-B16395
> Cc: j...@8bytes.org; io...@lists.linux-foundation.org; linuxppc-
> d...@lists.ozlabs.org; linux-ker...@vger.kernel.org;
> b...@kernel.crashing.org; ga...@kernel.crashing.org; Yoder Stuart-B08248;
> Wood Scott-B07421; Timur Tabi
> Subject: Re: [PATCH 3/3 v16] iommu/fsl: Freescale PAMU driver and iommu
> implementation.
>
> On Thu, 2013-06-20 at 21:31 +0530, Varun Sethi wrote:
>
> > +#define REQ_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR |
> PCI_ACS_UF)
> > +
> > +static struct iommu_group *get_device_iommu_group(struct device *dev)
> > +{
> > + struct iommu_group *group;
> > +
> > + group = iommu_group_get(dev);
> > + if (!group)
> > + group = iommu_group_alloc();
> > +
> > + return group;
> > +}
> > +
> [snip]
> > +
>
> This really gets parent or peer, right?
>
> > +static struct iommu_group *get_peer_pci_device_group(struct pci_dev
> > +*pdev) {
> > + struct iommu_group *group = NULL;
> > +
> > + /* check if this is the first device on the bus*/
> > + if (pdev->bus_list.next == pdev->bus_list.prev) {
>
> It's a list_head, use list functions. The list implementation should be
> treated as opaque.
>
> if (list_is_singular(&pdev->bus_list))
>
> > + struct pci_bus *bus = pdev->bus->parent;
> > + /* Traverese the parent bus list to get
> > + * pdev & dev for the sibling device.
> > + */
> > + while (bus) {
> > + if (!list_empty(&bus->devices)) {
> > + pdev = container_of(bus->devices.next,
> > + struct pci_dev, bus_list);
>
> pdev = list_first_entry(&bus->devices, struct pci_dev, bus_list);
>
> > + group = iommu_group_get(&pdev->dev);
> > + break;
> > + } else
> > + bus = bus->parent;
>
> Is this ever reached? Don't you always have bus->self?
>
[Sethi Varun-B16395] Not sure I understand. Trying to get the group information
from the parent bus, if there are no sibling devices on the current bus.
> > + }
> > + } else {
> > + /*
> > + * Get the pdev & dev for the sibling device
> > + */
> > + pdev = container_of(pdev->bus_list.prev,
> > + struct pci_dev, bus_list);
>
> How do you know if you're at the head or tail of the list?
>
> struct pci_dev *tmp;
> list_for_each_entry(tmp, &pdev->bus_list, bus_list) {
> if (tmp == pdev)
> continue;
>
> group = iommu_group_get(&tmp->dev);
> break;
> }
>
> > + group = iommu_group_get(&pdev->dev);
> > + }
> > +
> > + return group;
> > +}
> > +
> > +static struct iommu_group *get_pci_device_group(struct pci_dev *pdev)
> > +{
> > + struct iommu_group *group = NULL;
> > + struct pci_dev *bridge, *dma_pdev = NULL;
> > + struct pci_controller *pci_ctl;
> > + bool pci_endpt_partioning;
> > +
> > + pci_ctl = pci_bus_to_host(pdev->bus);
> > + pci_endpt_partioning = check_pci_ctl_endpt_part(pci_ctl);
> > + /* We can partition PCIe devices so assign device group to the
> device */
> > + if (pci_endpt_partioning) {
> > + bridge = pci_find_upstream_pcie_bridge(pdev);
> > + if (bridge) {
> > + if (pci_is_pcie(bridge))
> > + dma_pdev = pci_get_domain_bus_and_slot(
> > + pci_domain_nr(pdev->bus),
> > + bridge->subordinate->number, 0);
> > + if (!dma_pdev)
> > + dma_pdev = pci_dev_get(bridge);
> > + } else
> > + dma_pdev = pci_dev_get(pdev);
> > +
> > + /* Account for quirked devices */
> > + swap_pci_ref(&dma_pdev, pci_get_dma_source(dma_pdev));
> > +
> > + /*
> > + * If it's a multifunction device that does not support our
> > + * required ACS flags, add to the same group as function 0.
> > + */
>
> See c14d2690 in Joerg's next tree, using function 0 was a poor
> assumption.
[Sethi Varun-B16395] ok.
>
> > + if (dma_pdev->multifunction &&
> > + !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS))
> > + swap_pci_ref(&dma_pdev,
> > + pci_get_slot(dma_pdev->bus,
> > + PCI_DEVFN(PCI_SLOT(dma_pdev-
> >devfn),
> > + 0)));
> > +
> > + group = get_device_iommu_group(&pdev->dev);
> > + pci_dev_put(pdev);
>
> What was the point of all the above if we use pdev here instead of
> dma_pdev? Wrong device and broken reference counting.
[Sethi Varun-B16395] Will fix this
This also isn't
> testing ACS all the way up to the root complex or controller.
[Sethi Varun-B16395] In our case the IOMMU can differentiate transactions based
on the LIODN. The PCIe controller can generate a unique LIODN based on the
bus,device,function number. I believe this would even be true for devices
connected to a PCIe bridge (and not on the root bus). So, do we still need to
check for ACS up to the root node?
-Varun
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
