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(_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(>dev);
>
> - if (dev_data && __ratelimit(_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(_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
