* [PATCH v2] iommu/amd: Fix extended features logging
@ 2021-04-11 21:13 ` Alexander Monakov
0 siblings, 0 replies; 8+ messages in thread
From: Alexander Monakov @ 2021-04-11 21:13 UTC (permalink / raw)
To: linux-kernel
Cc: Alexander Monakov, Paul Menzel, Joerg Roedel,
Suravee Suthikulpanit, iommu
print_iommu_info prints the EFR register and then the decoded list of
features on a separate line:
pci 0000:00:00.2: AMD-Vi: Extended features (0x206d73ef22254ade):
PPR X2APIC NX GT IA GA PC GA_vAPIC
The second line is emitted via 'pr_cont', which causes it to have a
different ('warn') loglevel compared to the previous line ('info').
Commit 9a295ff0ffc9 attempted to rectify this by removing the newline
from the pci_info format string, but this doesn't work, as pci_info
calls implicitly append a newline anyway.
Printing the decoded features on the same line would make it quite long.
Instead, change pci_info() to pr_info() to omit PCI bus location info,
which is shown in the preceding message anyway. This results in:
pci 0000:00:00.2: AMD-Vi: Found IOMMU cap 0x40
AMD-Vi: Extended features (0x206d73ef22254ade): PPR X2APIC NX GT IA GA PC GA_vAPIC
AMD-Vi: Interrupt remapping enabled
Fixes: 9a295ff0ffc9 ("iommu/amd: Print extended features in one line to fix divergent log levels")
Link: https://lore.kernel.org/lkml/alpine.LNX.2.20.13.2104112326460.11104@monopod.intra.ispras.ru
Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
Cc: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: iommu@lists.linux-foundation.org
---
v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer
solution
drivers/iommu/amd/init.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 596d0c413473..62913f82a21f 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1929,8 +1929,8 @@ static void print_iommu_info(void)
pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr);
if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
- pci_info(pdev, "Extended features (%#llx):",
- iommu->features);
+ pr_info("Extended features (%#llx):", iommu->features);
+
for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
if (iommu_feature(iommu, (1ULL << i)))
pr_cont(" %s", feat_str[i]);
--
2.30.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2] iommu/amd: Fix extended features logging @ 2021-04-11 21:13 ` Alexander Monakov 0 siblings, 0 replies; 8+ messages in thread From: Alexander Monakov @ 2021-04-11 21:13 UTC (permalink / raw) To: linux-kernel; +Cc: Paul Menzel, iommu, Alexander Monakov, Joerg Roedel print_iommu_info prints the EFR register and then the decoded list of features on a separate line: pci 0000:00:00.2: AMD-Vi: Extended features (0x206d73ef22254ade): PPR X2APIC NX GT IA GA PC GA_vAPIC The second line is emitted via 'pr_cont', which causes it to have a different ('warn') loglevel compared to the previous line ('info'). Commit 9a295ff0ffc9 attempted to rectify this by removing the newline from the pci_info format string, but this doesn't work, as pci_info calls implicitly append a newline anyway. Printing the decoded features on the same line would make it quite long. Instead, change pci_info() to pr_info() to omit PCI bus location info, which is shown in the preceding message anyway. This results in: pci 0000:00:00.2: AMD-Vi: Found IOMMU cap 0x40 AMD-Vi: Extended features (0x206d73ef22254ade): PPR X2APIC NX GT IA GA PC GA_vAPIC AMD-Vi: Interrupt remapping enabled Fixes: 9a295ff0ffc9 ("iommu/amd: Print extended features in one line to fix divergent log levels") Link: https://lore.kernel.org/lkml/alpine.LNX.2.20.13.2104112326460.11104@monopod.intra.ispras.ru Signed-off-by: Alexander Monakov <amonakov@ispras.ru> Cc: Paul Menzel <pmenzel@molgen.mpg.de> Cc: Joerg Roedel <jroedel@suse.de> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> Cc: iommu@lists.linux-foundation.org --- v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer solution drivers/iommu/amd/init.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c index 596d0c413473..62913f82a21f 100644 --- a/drivers/iommu/amd/init.c +++ b/drivers/iommu/amd/init.c @@ -1929,8 +1929,8 @@ static void print_iommu_info(void) pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr); if (iommu->cap & (1 << IOMMU_CAP_EFR)) { - pci_info(pdev, "Extended features (%#llx):", - iommu->features); + pr_info("Extended features (%#llx):", iommu->features); + for (i = 0; i < ARRAY_SIZE(feat_str); ++i) { if (iommu_feature(iommu, (1ULL << i))) pr_cont(" %s", feat_str[i]); -- 2.30.0 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] iommu/amd: Fix extended features logging 2021-04-11 21:13 ` Alexander Monakov @ 2021-04-11 22:21 ` Joe Perches -1 siblings, 0 replies; 8+ messages in thread From: Joe Perches @ 2021-04-11 22:21 UTC (permalink / raw) To: Alexander Monakov, linux-kernel Cc: Paul Menzel, Joerg Roedel, Suravee Suthikulpanit, iommu On Mon, 2021-04-12 at 00:13 +0300, Alexander Monakov wrote: > print_iommu_info prints the EFR register and then the decoded list of > features on a separate line: > > pci 0000:00:00.2: AMD-Vi: Extended features (0x206d73ef22254ade): > PPR X2APIC NX GT IA GA PC GA_vAPIC > > The second line is emitted via 'pr_cont', which causes it to have a > different ('warn') loglevel compared to the previous line ('info'). > > Commit 9a295ff0ffc9 attempted to rectify this by removing the newline > from the pci_info format string, but this doesn't work, as pci_info > calls implicitly append a newline anyway. > > Printing the decoded features on the same line would make it quite long. > Instead, change pci_info() to pr_info() to omit PCI bus location info, > which is shown in the preceding message anyway. This results in: > > pci 0000:00:00.2: AMD-Vi: Found IOMMU cap 0x40 > AMD-Vi: Extended features (0x206d73ef22254ade): PPR X2APIC NX GT IA GA PC GA_vAPIC > AMD-Vi: Interrupt remapping enabled > > Fixes: 9a295ff0ffc9 ("iommu/amd: Print extended features in one line to fix divergent log levels") > Link: https://lore.kernel.org/lkml/alpine.LNX.2.20.13.2104112326460.11104@monopod.intra.ispras.ru > Signed-off-by: Alexander Monakov <amonakov@ispras.ru> > Cc: Paul Menzel <pmenzel@molgen.mpg.de> > Cc: Joerg Roedel <jroedel@suse.de> > Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > Cc: iommu@lists.linux-foundation.org > --- > > v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer > solution > > drivers/iommu/amd/init.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c > index 596d0c413473..62913f82a21f 100644 > --- a/drivers/iommu/amd/init.c > +++ b/drivers/iommu/amd/init.c > @@ -1929,8 +1929,8 @@ static void print_iommu_info(void) > pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr); > > > if (iommu->cap & (1 << IOMMU_CAP_EFR)) { > - pci_info(pdev, "Extended features (%#llx):", > - iommu->features); > + pr_info("Extended features (%#llx):", iommu->features); > + > for (i = 0; i < ARRAY_SIZE(feat_str); ++i) { > if (iommu_feature(iommu, (1ULL << i))) > pr_cont(" %s", feat_str[i]); How about avoiding all of this by using a temporary buffer and a single pci_info. Miscellanea: o Move the feat_str and i declarations into the if block for locality --- drivers/iommu/amd/init.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c index 321f5906e6ed..0d219044726e 100644 --- a/drivers/iommu/amd/init.c +++ b/drivers/iommu/amd/init.c @@ -1943,30 +1943,37 @@ static int __init iommu_init_pci(struct amd_iommu *iommu) static void print_iommu_info(void) { - static const char * const feat_str[] = { - "PreF", "PPR", "X2APIC", "NX", "GT", "[5]", - "IA", "GA", "HE", "PC" - }; struct amd_iommu *iommu; for_each_iommu(iommu) { struct pci_dev *pdev = iommu->dev; - int i; pci_info(pdev, "Found IOMMU cap 0x%x\n", iommu->cap_ptr); if (iommu->cap & (1 << IOMMU_CAP_EFR)) { - pci_info(pdev, "Extended features (%#llx):", - iommu->features); + static const char * const feat_str[] = { + "PreF", "PPR", "X2APIC", "NX", "GT", "[5]", + "IA", "GA", "HE", "PC" + }; + int i; + char features[128] = ""; + int len = 0; + for (i = 0; i < ARRAY_SIZE(feat_str); ++i) { - if (iommu_feature(iommu, (1ULL << i))) - pr_cont(" %s", feat_str[i]); + if (!iommu_feature(iommu, BIT_ULL(i))) + continue; + len += scnprintf(features + len, + sizeof(features) - len, + " %s", feat_str[i]); } if (iommu->features & FEATURE_GAM_VAPIC) - pr_cont(" GA_vAPIC"); + len += scnprintf(features + len, + sizeof(features) - len, + " %s", "GA_vPIC"); - pr_cont("\n"); + pci_info(pdev, "Extended features (%#llx):%s\n", + iommu->features, features); } } if (irq_remapping_enabled) { ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] iommu/amd: Fix extended features logging @ 2021-04-11 22:21 ` Joe Perches 0 siblings, 0 replies; 8+ messages in thread From: Joe Perches @ 2021-04-11 22:21 UTC (permalink / raw) To: Alexander Monakov, linux-kernel; +Cc: Paul Menzel, iommu, Joerg Roedel On Mon, 2021-04-12 at 00:13 +0300, Alexander Monakov wrote: > print_iommu_info prints the EFR register and then the decoded list of > features on a separate line: > > pci 0000:00:00.2: AMD-Vi: Extended features (0x206d73ef22254ade): > PPR X2APIC NX GT IA GA PC GA_vAPIC > > The second line is emitted via 'pr_cont', which causes it to have a > different ('warn') loglevel compared to the previous line ('info'). > > Commit 9a295ff0ffc9 attempted to rectify this by removing the newline > from the pci_info format string, but this doesn't work, as pci_info > calls implicitly append a newline anyway. > > Printing the decoded features on the same line would make it quite long. > Instead, change pci_info() to pr_info() to omit PCI bus location info, > which is shown in the preceding message anyway. This results in: > > pci 0000:00:00.2: AMD-Vi: Found IOMMU cap 0x40 > AMD-Vi: Extended features (0x206d73ef22254ade): PPR X2APIC NX GT IA GA PC GA_vAPIC > AMD-Vi: Interrupt remapping enabled > > Fixes: 9a295ff0ffc9 ("iommu/amd: Print extended features in one line to fix divergent log levels") > Link: https://lore.kernel.org/lkml/alpine.LNX.2.20.13.2104112326460.11104@monopod.intra.ispras.ru > Signed-off-by: Alexander Monakov <amonakov@ispras.ru> > Cc: Paul Menzel <pmenzel@molgen.mpg.de> > Cc: Joerg Roedel <jroedel@suse.de> > Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > Cc: iommu@lists.linux-foundation.org > --- > > v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer > solution > > drivers/iommu/amd/init.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c > index 596d0c413473..62913f82a21f 100644 > --- a/drivers/iommu/amd/init.c > +++ b/drivers/iommu/amd/init.c > @@ -1929,8 +1929,8 @@ static void print_iommu_info(void) > pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr); > > > if (iommu->cap & (1 << IOMMU_CAP_EFR)) { > - pci_info(pdev, "Extended features (%#llx):", > - iommu->features); > + pr_info("Extended features (%#llx):", iommu->features); > + > for (i = 0; i < ARRAY_SIZE(feat_str); ++i) { > if (iommu_feature(iommu, (1ULL << i))) > pr_cont(" %s", feat_str[i]); How about avoiding all of this by using a temporary buffer and a single pci_info. Miscellanea: o Move the feat_str and i declarations into the if block for locality --- drivers/iommu/amd/init.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c index 321f5906e6ed..0d219044726e 100644 --- a/drivers/iommu/amd/init.c +++ b/drivers/iommu/amd/init.c @@ -1943,30 +1943,37 @@ static int __init iommu_init_pci(struct amd_iommu *iommu) static void print_iommu_info(void) { - static const char * const feat_str[] = { - "PreF", "PPR", "X2APIC", "NX", "GT", "[5]", - "IA", "GA", "HE", "PC" - }; struct amd_iommu *iommu; for_each_iommu(iommu) { struct pci_dev *pdev = iommu->dev; - int i; pci_info(pdev, "Found IOMMU cap 0x%x\n", iommu->cap_ptr); if (iommu->cap & (1 << IOMMU_CAP_EFR)) { - pci_info(pdev, "Extended features (%#llx):", - iommu->features); + static const char * const feat_str[] = { + "PreF", "PPR", "X2APIC", "NX", "GT", "[5]", + "IA", "GA", "HE", "PC" + }; + int i; + char features[128] = ""; + int len = 0; + for (i = 0; i < ARRAY_SIZE(feat_str); ++i) { - if (iommu_feature(iommu, (1ULL << i))) - pr_cont(" %s", feat_str[i]); + if (!iommu_feature(iommu, BIT_ULL(i))) + continue; + len += scnprintf(features + len, + sizeof(features) - len, + " %s", feat_str[i]); } if (iommu->features & FEATURE_GAM_VAPIC) - pr_cont(" GA_vAPIC"); + len += scnprintf(features + len, + sizeof(features) - len, + " %s", "GA_vPIC"); - pr_cont("\n"); + pci_info(pdev, "Extended features (%#llx):%s\n", + iommu->features, features); } } if (irq_remapping_enabled) { _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] iommu/amd: Fix extended features logging 2021-04-11 22:21 ` Joe Perches @ 2021-04-19 19:23 ` Alexander Monakov -1 siblings, 0 replies; 8+ messages in thread From: Alexander Monakov @ 2021-04-19 19:23 UTC (permalink / raw) To: Joe Perches Cc: linux-kernel, Paul Menzel, Joerg Roedel, Suravee Suthikulpanit, iommu [-- Attachment #1: Type: text/plain, Size: 3751 bytes --] On Sun, 11 Apr 2021, Joe Perches wrote: > > v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer > > solution > > > > drivers/iommu/amd/init.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c > > index 596d0c413473..62913f82a21f 100644 > > --- a/drivers/iommu/amd/init.c > > +++ b/drivers/iommu/amd/init.c > > @@ -1929,8 +1929,8 @@ static void print_iommu_info(void) > > pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr); > > > > > > if (iommu->cap & (1 << IOMMU_CAP_EFR)) { > > - pci_info(pdev, "Extended features (%#llx):", > > - iommu->features); > > + pr_info("Extended features (%#llx):", iommu->features); > > + > > for (i = 0; i < ARRAY_SIZE(feat_str); ++i) { > > if (iommu_feature(iommu, (1ULL << i))) > > pr_cont(" %s", feat_str[i]); > > How about avoiding all of this by using a temporary buffer > and a single pci_info. I think it is mostly up to the maintainers, but from my perspective, it's not good to conflate such a simple bugfix with the substantial rewrite you are proposing (which also increases code complexity). My two-line patch is a straightforward fix to a bug that people already agreed needs to be fixed (just the previous attempt turned out to be insufficient). If there's a desire to eliminate pr_cont calls (which I wouldn't support in this instance), the rewrite can go in separately from the bugfix. A major problem with landing a simple bugfix together with a rewrite in a big patch is that if a rewrite causes a problem, the whole patch gets reverted and we end up without a trivial bugfix. And, once again: can we please not emit the feature list via pci_info, the line is long enough already even without the pci bus location info. Joerg, are you satisfied with my v2 patch, are you waiting for anything before picking it up? Alexander > > Miscellanea: > o Move the feat_str and i declarations into the if block for locality > > --- > drivers/iommu/amd/init.c | 29 ++++++++++++++++++----------- > 1 file changed, 18 insertions(+), 11 deletions(-) > > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c > index 321f5906e6ed..0d219044726e 100644 > --- a/drivers/iommu/amd/init.c > +++ b/drivers/iommu/amd/init.c > @@ -1943,30 +1943,37 @@ static int __init iommu_init_pci(struct amd_iommu *iommu) > > static void print_iommu_info(void) > { > - static const char * const feat_str[] = { > - "PreF", "PPR", "X2APIC", "NX", "GT", "[5]", > - "IA", "GA", "HE", "PC" > - }; > struct amd_iommu *iommu; > > for_each_iommu(iommu) { > struct pci_dev *pdev = iommu->dev; > - int i; > > pci_info(pdev, "Found IOMMU cap 0x%x\n", iommu->cap_ptr); > > if (iommu->cap & (1 << IOMMU_CAP_EFR)) { > - pci_info(pdev, "Extended features (%#llx):", > - iommu->features); > + static const char * const feat_str[] = { > + "PreF", "PPR", "X2APIC", "NX", "GT", "[5]", > + "IA", "GA", "HE", "PC" > + }; > + int i; > + char features[128] = ""; > + int len = 0; > + > for (i = 0; i < ARRAY_SIZE(feat_str); ++i) { > - if (iommu_feature(iommu, (1ULL << i))) > - pr_cont(" %s", feat_str[i]); > + if (!iommu_feature(iommu, BIT_ULL(i))) > + continue; > + len += scnprintf(features + len, > + sizeof(features) - len, > + " %s", feat_str[i]); > } > > if (iommu->features & FEATURE_GAM_VAPIC) > - pr_cont(" GA_vAPIC"); > + len += scnprintf(features + len, > + sizeof(features) - len, > + " %s", "GA_vPIC"); > > - pr_cont("\n"); > + pci_info(pdev, "Extended features (%#llx):%s\n", > + iommu->features, features); > } > } > if (irq_remapping_enabled) { > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] iommu/amd: Fix extended features logging @ 2021-04-19 19:23 ` Alexander Monakov 0 siblings, 0 replies; 8+ messages in thread From: Alexander Monakov @ 2021-04-19 19:23 UTC (permalink / raw) To: Joe Perches; +Cc: Paul Menzel, iommu, Joerg Roedel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3751 bytes --] On Sun, 11 Apr 2021, Joe Perches wrote: > > v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer > > solution > > > > drivers/iommu/amd/init.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c > > index 596d0c413473..62913f82a21f 100644 > > --- a/drivers/iommu/amd/init.c > > +++ b/drivers/iommu/amd/init.c > > @@ -1929,8 +1929,8 @@ static void print_iommu_info(void) > > pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr); > > > > > > if (iommu->cap & (1 << IOMMU_CAP_EFR)) { > > - pci_info(pdev, "Extended features (%#llx):", > > - iommu->features); > > + pr_info("Extended features (%#llx):", iommu->features); > > + > > for (i = 0; i < ARRAY_SIZE(feat_str); ++i) { > > if (iommu_feature(iommu, (1ULL << i))) > > pr_cont(" %s", feat_str[i]); > > How about avoiding all of this by using a temporary buffer > and a single pci_info. I think it is mostly up to the maintainers, but from my perspective, it's not good to conflate such a simple bugfix with the substantial rewrite you are proposing (which also increases code complexity). My two-line patch is a straightforward fix to a bug that people already agreed needs to be fixed (just the previous attempt turned out to be insufficient). If there's a desire to eliminate pr_cont calls (which I wouldn't support in this instance), the rewrite can go in separately from the bugfix. A major problem with landing a simple bugfix together with a rewrite in a big patch is that if a rewrite causes a problem, the whole patch gets reverted and we end up without a trivial bugfix. And, once again: can we please not emit the feature list via pci_info, the line is long enough already even without the pci bus location info. Joerg, are you satisfied with my v2 patch, are you waiting for anything before picking it up? Alexander > > Miscellanea: > o Move the feat_str and i declarations into the if block for locality > > --- > drivers/iommu/amd/init.c | 29 ++++++++++++++++++----------- > 1 file changed, 18 insertions(+), 11 deletions(-) > > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c > index 321f5906e6ed..0d219044726e 100644 > --- a/drivers/iommu/amd/init.c > +++ b/drivers/iommu/amd/init.c > @@ -1943,30 +1943,37 @@ static int __init iommu_init_pci(struct amd_iommu *iommu) > > static void print_iommu_info(void) > { > - static const char * const feat_str[] = { > - "PreF", "PPR", "X2APIC", "NX", "GT", "[5]", > - "IA", "GA", "HE", "PC" > - }; > struct amd_iommu *iommu; > > for_each_iommu(iommu) { > struct pci_dev *pdev = iommu->dev; > - int i; > > pci_info(pdev, "Found IOMMU cap 0x%x\n", iommu->cap_ptr); > > if (iommu->cap & (1 << IOMMU_CAP_EFR)) { > - pci_info(pdev, "Extended features (%#llx):", > - iommu->features); > + static const char * const feat_str[] = { > + "PreF", "PPR", "X2APIC", "NX", "GT", "[5]", > + "IA", "GA", "HE", "PC" > + }; > + int i; > + char features[128] = ""; > + int len = 0; > + > for (i = 0; i < ARRAY_SIZE(feat_str); ++i) { > - if (iommu_feature(iommu, (1ULL << i))) > - pr_cont(" %s", feat_str[i]); > + if (!iommu_feature(iommu, BIT_ULL(i))) > + continue; > + len += scnprintf(features + len, > + sizeof(features) - len, > + " %s", feat_str[i]); > } > > if (iommu->features & FEATURE_GAM_VAPIC) > - pr_cont(" GA_vAPIC"); > + len += scnprintf(features + len, > + sizeof(features) - len, > + " %s", "GA_vPIC"); > > - pr_cont("\n"); > + pci_info(pdev, "Extended features (%#llx):%s\n", > + iommu->features, features); > } > } > if (irq_remapping_enabled) { > > > [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] iommu/amd: Fix extended features logging 2021-04-19 19:23 ` Alexander Monakov @ 2021-04-19 21:59 ` Joe Perches -1 siblings, 0 replies; 8+ messages in thread From: Joe Perches @ 2021-04-19 21:59 UTC (permalink / raw) To: Alexander Monakov Cc: linux-kernel, Paul Menzel, Joerg Roedel, Suravee Suthikulpanit, iommu On Mon, 2021-04-19 at 22:23 +0300, Alexander Monakov wrote: > On Sun, 11 Apr 2021, Joe Perches wrote: > > > > v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer > > > solution > > > > > > drivers/iommu/amd/init.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c > > > index 596d0c413473..62913f82a21f 100644 > > > --- a/drivers/iommu/amd/init.c > > > +++ b/drivers/iommu/amd/init.c > > > @@ -1929,8 +1929,8 @@ static void print_iommu_info(void) > > > pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr); > > > > > > > > > if (iommu->cap & (1 << IOMMU_CAP_EFR)) { > > > - pci_info(pdev, "Extended features (%#llx):", > > > - iommu->features); > > > + pr_info("Extended features (%#llx):", iommu->features); > > > + > > > for (i = 0; i < ARRAY_SIZE(feat_str); ++i) { > > > if (iommu_feature(iommu, (1ULL << i))) > > > pr_cont(" %s", feat_str[i]); > > > > How about avoiding all of this by using a temporary buffer > > and a single pci_info. > > I think it is mostly up to the maintainers, but from my perspective, it's not > good to conflate such a simple bugfix with the substantial rewrite you are > proposing (which also increases code complexity). You and I have _significant_ differences in the definition of substantial. Buffering the output is the preferred code style in preference to pr_cont. Do remember pr_cont should _only_ be used when absolutely necessary as interleaving of other messages from other processes/threads can and does occur. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] iommu/amd: Fix extended features logging @ 2021-04-19 21:59 ` Joe Perches 0 siblings, 0 replies; 8+ messages in thread From: Joe Perches @ 2021-04-19 21:59 UTC (permalink / raw) To: Alexander Monakov; +Cc: Paul Menzel, iommu, Joerg Roedel, linux-kernel On Mon, 2021-04-19 at 22:23 +0300, Alexander Monakov wrote: > On Sun, 11 Apr 2021, Joe Perches wrote: > > > > v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer > > > solution > > > > > > drivers/iommu/amd/init.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c > > > index 596d0c413473..62913f82a21f 100644 > > > --- a/drivers/iommu/amd/init.c > > > +++ b/drivers/iommu/amd/init.c > > > @@ -1929,8 +1929,8 @@ static void print_iommu_info(void) > > > pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr); > > > > > > > > > if (iommu->cap & (1 << IOMMU_CAP_EFR)) { > > > - pci_info(pdev, "Extended features (%#llx):", > > > - iommu->features); > > > + pr_info("Extended features (%#llx):", iommu->features); > > > + > > > for (i = 0; i < ARRAY_SIZE(feat_str); ++i) { > > > if (iommu_feature(iommu, (1ULL << i))) > > > pr_cont(" %s", feat_str[i]); > > > > How about avoiding all of this by using a temporary buffer > > and a single pci_info. > > I think it is mostly up to the maintainers, but from my perspective, it's not > good to conflate such a simple bugfix with the substantial rewrite you are > proposing (which also increases code complexity). You and I have _significant_ differences in the definition of substantial. Buffering the output is the preferred code style in preference to pr_cont. Do remember pr_cont should _only_ be used when absolutely necessary as interleaving of other messages from other processes/threads can and does occur. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-04-19 22:36 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-11 21:13 [PATCH v2] iommu/amd: Fix extended features logging Alexander Monakov 2021-04-11 21:13 ` Alexander Monakov 2021-04-11 22:21 ` Joe Perches 2021-04-11 22:21 ` Joe Perches 2021-04-19 19:23 ` Alexander Monakov 2021-04-19 19:23 ` Alexander Monakov 2021-04-19 21:59 ` Joe Perches 2021-04-19 21:59 ` Joe Perches
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.