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.