lkml.org 
[lkml]   [2016]   [Feb]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH V3 5/5] perf/amd/iommu: Enable support for multiple IOMMUs
From
Date
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

\
 
 \ /
  Last update: 2016-02-11 10:01    [W:0.109 / U:8.464 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site