Hi Boris,
Thank you very much for catching several styling issues. I should have
taken care of that earlier. I'll clean them up. Please see my other
comments below.
Thanks,
Suravee
On 02/11/2016 12:14 AM, Borislav Petkov wrote:
On Tue, Feb 09, 2016 at 04:53:55PM -0600, Suravee Suthikulpanit wrote:
[...]
- /* IOMMU pc counter register is only 48 bits */
- count &= 0xFFFFFFFFFFFFULL;
+ pr_debug("perf: amd_iommu:perf_iommu_read\n");
What is that debug statement good for? Can it go?
From looking at that file, it has a bunch more which are completely
useless and can be deleted. In a separate patch.
Sure. I'll add a new patch to remove all these pr_debug.
[...]
+ if (!perf_iommu_cnts)
+ return -ENOMEM;
+
ret = perf_pmu_register(&perf_iommu->pmu, name, -1);
if (ret) {
pr_err("perf: amd_iommu: Failed to initialized.\n");
You can define pr_fmt to "perf/amd_iommu: " at the beginning of this
file and then you don't need to supply that prefix each time you call
pr_*
Good point. I'll take care of this.
And that sentence above it wrong. It should be:
pr_err("Error initializing AMD IOMMU perf counters.\n");
or something like that.
OK.
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 531b2e1..7b1b561 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1133,6 +1133,9 @@ static int __init init_iommu_all(struct acpi_table_header
*table)
return 0;
}
+static int _amd_iommu_pc_get_set_reg_val(struct amd_iommu *iommu,
+ u8 bank, u8 cntr, u8 fxn,
+ u64 *value, bool is_write);
static int _amd_iommu_this_func_name_is_def_too_long
I'll rename all these functions to make it shorter and less confusing.
[...]
+
+int amd_iommu_pc_set_reg_val(u16 devid, u8 bank, u8 cntr, u8 fxn, u64 *value)
+{
+ struct amd_iommu *iommu;
+
+ for_each_iommu(iommu) {
+ int ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
+ fxn, value, true);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(amd_iommu_pc_set_reg_val);
Why isn't this EXPORT_SYMBOL_GPL?
Actually, I am just trying to match other exported functions in the AMD
IOMMU driver. May be Joerg can let us know why these functions are not
EXPORT_SYMBOL_GPL to begin with.
+ * which should have been programmed the same way.
+ * and aggregate the counter values.
That comment is a sentence with a fullstop in the middle.
+ */
+ for_each_iommu(iommu) {
+ u64 tmp;
+
+ if (i >= num)
+ return -EINVAL;
+
+ ret = _amd_iommu_pc_get_set_reg_val(iommu, bank, cntr,
+ IOMMU_PC_COUNTER_REG,
+ &tmp, false);
+ if (ret)
+ return ret;
So if one of those intermediary calls of _amd_iommu_pc_get_set_reg_val()
fails, we return here not having read all counters. Unwind and handle
that error properly maybe?
Same issue above.
Good point. So, for writing, I'll make sure to unset the
registers/counters when we encounter errors.
For reading counters, I think it is acceptable to say that users cannot
rely on the return values in the array if the function returns non-zero
value (success).
Thanks,
Suravee
