Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <84e443d76c4c450770ade83aa48cc631c101d383.camel@perches.com>
Subject: Re: [PATCH v2] iommu/amd: Fix extended features logging
From:   Joe Perches <joe@perches.com>
To:     Alexander Monakov <amonakov@ispras.ru>
Cc:     linux-kernel@vger.kernel.org, Paul Menzel <pmenzel@molgen.mpg.de>,
        Joerg Roedel <jroedel@suse.de>,
        Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
        iommu@lists.linux-foundation.org
Date:   Mon, 19 Apr 2021 14:59:59 -0700
In-Reply-To: <alpine.LNX.2.20.13.2104192207130.19608@monopod.intra.ispras.ru>
References: <20210411211330.2252-1-amonakov@ispras.ru>
         <0362ad3912473d24e5927c0b54ed8fd3648c68a9.camel@perches.com>
         <alpine.LNX.2.20.13.2104192207130.19608@monopod.intra.ispras.ru>
Content-Type: text/plain; charset="ISO-8859-1"
User-Agent: Evolution 3.38.1-1 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.