From: Lennert Buytenhek <buytenh@wantstofly.org>
To: "Suthikulpanit, Suravee" <suravee.suthikulpanit@amd.com>
Cc: iommu@lists.linux-foundation.org
Subject: Re: [PATCH] iommu/amd: Fix I/O page fault logging ratelimit test
Date: Wed, 21 Jul 2021 16:46:52 +0300 [thread overview]
Message-ID: <YPglTERWMajeo6Sj@wantstofly.org> (raw)
In-Reply-To: <79c1cd8b-c680-b3fb-08f0-47c42290d1a4@amd.com>
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
prev parent reply other threads:[~2021-07-21 13:46 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-19 0:47 [PATCH] iommu/amd: Fix I/O page fault logging ratelimit test Lennert Buytenhek
2021-07-21 0:05 ` Suthikulpanit, Suravee via iommu
2021-07-21 13:46 ` Lennert Buytenhek [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YPglTERWMajeo6Sj@wantstofly.org \
--to=buytenh@wantstofly.org \
--cc=iommu@lists.linux-foundation.org \
--cc=suravee.suthikulpanit@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).