Messages in this thread | | | Subject | Re: [PATCH 2/2] iommu/amd: Remove performance counter pre-initialization test | From | Shuah Khan <> | Date | Fri, 9 Apr 2021 11:10:29 -0600 |
| |
On 4/9/21 10:37 AM, Shuah Khan wrote: > On 4/9/21 2:58 AM, Suravee Suthikulpanit wrote: >> In early AMD desktop/mobile platforms (during 2013), when the IOMMU >> Performance Counter (PMC) support was first introduced in >> commit 30861ddc9cca ("perf/x86/amd: Add IOMMU Performance Counter >> resource management"), there was a HW bug where the counters could not >> be accessed. The result was reading of the counter always return zero. >> >> At the time, the suggested workaround was to add a test logic prior >> to initializing the PMC feature to check if the counters can be >> programmed >> and read back the same value. This has been working fine until the more >> recent desktop/mobile platforms start enabling power gating for the PMC, >> which prevents access to the counters. This results in the PMC support >> being disabled unnecesarily. > > unnecessarily > >> >> Unfortunatly, there is no documentation of since which generation > > Unfortunately, > > Rephrase suggestion: > Unfortunately, it is unclear when the PMC HW bug fixed. > >> of hardware the original PMC HW bug was fixed. Although, it was fixed >> soon after the first introduction of the PMC. Base on this, we assume > > Based > >> that the buggy platforms are less likely to be in used, and it should > > in use > >> be relatively safe to remove this legacy logic. > >> >> Link: >> https://lore.kernel.org/linux-iommu/alpine.LNX.3.20.13.2006030935570.3181@monopod.intra.ispras.ru/ >> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201753 >> Cc: Tj (Elloe Linux) <ml.linux@elloe.vision> >> Cc: Shuah Khan <skhan@linuxfoundation.org> >> Cc: Alexander Monakov <amonakov@ispras.ru> >> Cc: David Coe <david.coe@live.co.uk> >> Cc: Paul Menzel <pmenzel@molgen.mpg.de> >> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> >> --- >> drivers/iommu/amd/init.c | 24 +----------------------- >> 1 file changed, 1 insertion(+), 23 deletions(-) >> >> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c >> index 648cdfd03074..247cdda5d683 100644 >> --- a/drivers/iommu/amd/init.c >> +++ b/drivers/iommu/amd/init.c >> @@ -1714,33 +1714,16 @@ static int __init init_iommu_all(struct >> acpi_table_header *table) >> return 0; >> } >> -static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 >> cntr, >> - u8 fxn, u64 *value, bool is_write); >> - >> static void init_iommu_perf_ctr(struct amd_iommu *iommu) >> { >> + u64 val; >> struct pci_dev *pdev = iommu->dev; >> - u64 val = 0xabcd, val2 = 0, save_reg = 0;
Why not leave this u64 val here? Having the pdev assignment as the first line makes it easier to read/follow.
>> if (!iommu_feature(iommu, FEATURE_PC)) >> return; >> amd_iommu_pc_present = true; >> - /* save the value to restore, if writable */ >> - if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, false)) >> - goto pc_false; >> - >> - /* Check if the performance counters can be written to */ >> - if ((iommu_pc_get_set_reg(iommu, 0, 0, 0, &val, true)) || >> - (iommu_pc_get_set_reg(iommu, 0, 0, 0, &val2, false)) || >> - (val != val2))
Aha - this is going away anyway. Please ignore my comment on 1/2 about parenthesis around (val != val2) being unnecessary.
>> - goto pc_false; >> - >> - /* restore */ >> - if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, true)) >> - goto pc_false; >> - >> pci_info(pdev, "IOMMU performance counters supported\n"); >> val = readl(iommu->mmio_base + MMIO_CNTR_CONF_OFFSET); >> @@ -1748,11 +1731,6 @@ static void init_iommu_perf_ctr(struct >> amd_iommu *iommu) >> iommu->max_counters = (u8) ((val >> 7) & 0xf); >> return; >> - >> -pc_false: >> - pci_err(pdev, "Unable to read/write to IOMMU perf counter.\n"); >> - amd_iommu_pc_present = false; >> - return; >> } >> static ssize_t amd_iommu_show_cap(struct device *dev, >> >
thanks, -- Shuah
| |