Re: [PATCH V3 1/6] perf/x86/intel/uncore: Add Sapphire Rapids server support

From: Greg KH <gregkh@linuxfoundation.org>
To: "Liang, Kan" <kan.liang@linux.intel.com>
Cc: peterz@infradead.org, mingo@redhat.com, acme@kernel.org,
	linux-kernel@vger.kernel.org, eranian@google.com,
	namhyung@kernel.org, jolsa@redhat.com, ak@linux.intel.com,
	yao.jin@linux.intel.com
Subject: Re: [PATCH V3 1/6] perf/x86/intel/uncore: Add Sapphire Rapids server support
Date: Wed, 30 Jun 2021 15:05:33 +0200	[thread overview]
Message-ID: <YNxsHfzX4PGdM1rP@kroah.com> (raw)
In-Reply-To: <1b7ecd8e-ba0d-5448-5c43-f730ec7e5b35@linux.intel.com>

On Wed, Jun 30, 2021 at 08:56:10AM -0400, Liang, Kan wrote:
> 
> 
> On 6/30/2021 5:36 AM, Greg KH wrote:
> > On Tue, Jun 29, 2021 at 11:13:58AM -0700, kan.liang@linux.intel.com wrote:
> > > From: Kan Liang <kan.liang@linux.intel.com>
> > > 
> > > Intel Sapphire Rapids supports a discovery mechanism, that allows an
> > > uncore driver to discover the different components ("boxes") of the
> > > chip.
> > > 
> > > All the generic information of the uncore boxes should be retrieved from
> > > the discovery tables. This has been enabled with the commit edae1f06c2cd
> > > ("perf/x86/intel/uncore: Parse uncore discovery tables"). Add
> > > use_discovery to indicate the case. The uncore driver doesn't need to
> > > hard code the generic information for each uncore box.
> > > 
> > > But we still need to enable various functionality that cannot be
> > > directly discovered. This is done here.
> > >   - Add a meaningful name for each uncore block.
> > >   - Add CHA filter support.
> > >   - The layout of the control registers for each uncore block is a little
> > >     bit different from the generic one. Set the platform specific format
> > >     and ops. Expose the common ops which can be reused.
> > >   - Add a fixed counter for IMC
> > > 
> > > All the undiscovered platform-specific features are hard code in the
> > > spr_uncores[]. Add uncore_type_customized_copy(), instead of the memcpy,
> > > to only overwrite these features.
> > > 
> > > Only the uncore blocks which are inculded in the discovery tables are
> > > enabled here. Other uncore blocks, e.g., free-running counters, will be
> > > supported in the following patch.
> > > 
> > > Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> > > ---
> > 
> > Why is there no other intel.com review on this before sending it out?
> > 
> 
> For the perf related code, currently I follow a loose internal review
> process. Before posting any patches to LKML, I start an internal review
> process by sending the patches to an internal mailing list. People (mainly
> Andi) will review the patches and give some comments.
> After several rounds of reviews, the reviewers may give a reviewed-by tag or
> just keep silent. I usually wait for several days. If there is no objection,
> I will post the patches in LKML for further review. That's why some patches
> have a reviewed-by, some doesn't in this patchset.
> But for the patches which you are the key maintainer, I followed the
> standard internal review process. As you can see, the reviewed-by from
> Rafael is tagged in the first patch of V1.

Thanks for the explaination, it does look very odd to see some patches
with a reviewed-by and others not.  Makes me thing that the reviewers
really did not read them all :(

> Please let me know if you'd like me to follow the standard internal review
> process in the future.

The "process" is there to help you all do better work.  If you feel it
is somehow making it harder then feel free to take it up with Intel
internally as people there know why it is in place.

thanks,

greg k-h