[PATCH v2] iommu/amd: Fix extended features logging

[PATCH v2] iommu/amd: Fix extended features logging

[Alexander Monakov]

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

Re: [PATCH v2] iommu/amd: Fix extended features logging

[Joe Perches]

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) {

Re: [PATCH v2] iommu/amd: Fix extended features logging

[Alexander Monakov]

[-- 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) {
> 
> 
> 

Re: [PATCH v2] iommu/amd: Fix extended features logging

[Joe Perches]

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.