On Tue, Jul 20, 2021 at 07:05:50PM -0500, Suthikulpanit, Suravee wrote: > Hi Lennert,
Hi Suravee, > > On an AMD system, I/O page faults are usually logged like this: > > .... > > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c > > index 811a49a95d04..7ae426b092f2 100644 > > --- a/drivers/iommu/amd/iommu.c > > +++ b/drivers/iommu/amd/iommu.c > > @@ -483,7 +483,7 @@ static void amd_iommu_report_page_fault(u16 devid, u16 > > domain_id, > > if (dev_data && __ratelimit(&dev_data->rs)) { > > pci_err(pdev, "Event logged [IO_PAGE_FAULT domain=0x%04x > > address=0x%llx flags=0x%04x]\n", > > domain_id, address, flags); > > - } else if (printk_ratelimit()) { > > + } else if (!dev_data && printk_ratelimit()) { > > This seems a bit confusing. Also, according to the following comment in > include/linux/printk.h: > > /* > * Please don't use printk_ratelimit(), because it shares ratelimiting state > * with all other unrelated printk_ratelimit() callsites. Instead use > * printk_ratelimited() or plain old __ratelimit(). > */ > > We probably should move away from using printk_ratelimit() here. > What about the following change instead? > > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c > index 811a49a95d04..8eb5d3519743 100644 > --- a/drivers/iommu/amd/iommu.c > +++ b/drivers/iommu/amd/iommu.c > @@ -480,11 +480,12 @@ static void amd_iommu_report_page_fault(u16 devid, u16 > domain_id, > if (pdev) > dev_data = dev_iommu_priv_get(&pdev->dev); > > - if (dev_data && __ratelimit(&dev_data->rs)) { > - pci_err(pdev, "Event logged [IO_PAGE_FAULT domain=0x%04x > address=0x%llx flags=0x%04x]\n", > - domain_id, address, flags); > - } else if (printk_ratelimit()) { > - pr_err("Event logged [IO_PAGE_FAULT device=%02x:%02x.%x > domain=0x%04x address=0x%llx flags=0x%04x]\n", > + if (dev_data) { > + if (__ratelimit(&dev_data->rs)) > + pci_err(pdev, "Event logged [IO_PAGE_FAULT > domain=0x%04x address=0x%llx flags=0x%04x]\n", > + domain_id, address, flags); > + } else { > + pr_err_ratelimited("Event logged [IO_PAGE_FAULT > device=%02x:%02x.%x domain=0x%04x address=0x%llx flags=0x%04x]\n", > PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid), > domain_id, address, flags); > } Looks good! > Note also that there might be other places in this file that would need > similar modification as well. Indeed, there are two more sites like these. I've sent a new patch that incorporates your feedback. Thank you! Cheers, Lennert _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu