From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> To: Borislav Petkov <bp@alien8.de> Cc: <joro@8bytes.org>, <peterz@infradead.org>, <mingo@redhat.com>, <acme@kernel.org>, <andihartmann@freenet.de>, <linux-kernel@vger.kernel.org>, <iommu@lists.linux-foundation.org> Subject: Re: [PATCH V3 5/5] perf/amd/iommu: Enable support for multiple IOMMUs Date: Thu, 11 Feb 2016 15:34:38 +0700 [thread overview] Message-ID: <56BC479E.2070004@amd.com> (raw) In-Reply-To: <20160210171401.GI23914@pd.tnic> 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
WARNING: multiple messages have this Message-ID (diff)
From: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org> To: Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org> Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, acme-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, andihartmann-KuiJ5kEpwI6ELgA04lAiVw@public.gmane.org, mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org Subject: Re: [PATCH V3 5/5] perf/amd/iommu: Enable support for multiple IOMMUs Date: Thu, 11 Feb 2016 15:34:38 +0700 [thread overview] Message-ID: <56BC479E.2070004@amd.com> (raw) In-Reply-To: <20160210171401.GI23914-fF5Pk5pvG8Y@public.gmane.org> 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
next prev parent reply other threads:[~2016-02-11 8:35 UTC|newest] Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-02-09 22:53 [PATCH V3 0/5] perf/amd/iommu: Enable multi-IOMMU support Suravee Suthikulpanit 2016-02-09 22:53 ` Suravee Suthikulpanit 2016-02-09 22:53 ` [PATCH V3 1/5] perf/amd/iommu: Consolidate and move perf_event_amd_iommu header Suravee Suthikulpanit 2016-02-09 22:53 ` Suravee Suthikulpanit 2016-02-10 16:41 ` Borislav Petkov 2016-02-10 18:42 ` Suravee Suthikulpanit 2016-02-10 18:42 ` Suravee Suthikulpanit 2016-02-10 18:51 ` Borislav Petkov 2016-02-10 18:51 ` Borislav Petkov 2016-02-10 18:56 ` Suravee Suthikulpanit 2016-02-10 18:56 ` Suravee Suthikulpanit 2016-02-10 19:00 ` Borislav Petkov 2016-02-10 19:00 ` Borislav Petkov 2016-02-09 22:53 ` [PATCH V3 2/5] perf/amd/iommu: Modify functions to query max banks and counters Suravee Suthikulpanit 2016-02-09 22:53 ` Suravee Suthikulpanit 2016-02-09 22:53 ` [PATCH V3 3/5] iommu/amd: Introduce amd_iommu_get_num_iommus() Suravee Suthikulpanit 2016-02-09 22:53 ` Suravee Suthikulpanit 2016-02-09 22:53 ` [PATCH V3 4/5] perf/amd/iommu: Introduce get_iommu_bnk_cnt_evt_idx Suravee Suthikulpanit 2016-02-09 22:53 ` Suravee Suthikulpanit 2016-02-10 16:43 ` Borislav Petkov 2016-02-10 16:43 ` Borislav Petkov 2016-02-09 22:53 ` [PATCH V3 5/5] perf/amd/iommu: Enable support for multiple IOMMUs Suravee Suthikulpanit 2016-02-09 22:53 ` Suravee Suthikulpanit 2016-02-10 17:14 ` Borislav Petkov 2016-02-10 17:14 ` Borislav Petkov 2016-02-11 8:34 ` Suravee Suthikulpanit [this message] 2016-02-11 8:34 ` Suravee Suthikulpanit 2016-02-10 0:08 ` [PATCH V3 0/5] perf/amd/iommu: Enable multi-IOMMU support Borislav Petkov 2016-02-10 0:08 ` Borislav Petkov 2016-02-10 13:43 ` Suravee Suthikulpanit 2016-02-10 13:43 ` Suravee Suthikulpanit
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=56BC479E.2070004@amd.com \ --to=suravee.suthikulpanit@amd.com \ --cc=acme@kernel.org \ --cc=andihartmann@freenet.de \ --cc=bp@alien8.de \ --cc=iommu@lists.linux-foundation.org \ --cc=joro@8bytes.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mingo@redhat.com \ --cc=peterz@infradead.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.