Re: [PATCH v3 0/4] Simplify PCIe hotplug indicator control

From: Lukas Wunner <lukas@wunner.de>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Denis Efremov <efremov@linux.com>,
	sathyanarayanan kuppuswamy 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 0/4] Simplify PCIe hotplug indicator control
Date: Wed, 28 Aug 2019 05:33:09 +0200	[thread overview]
Message-ID: <20190828033309.e65mmunihlyrzzgz@wunner.de> (raw)
In-Reply-To: <20190827225319.GE9987@google.com>

On Tue, Aug 27, 2019 at 05:53:19PM -0500, Bjorn Helgaas wrote:
> Unrelated, but if anybody is looking at pciehp, is there value in
> having pciehp split across five files?
> 
>   drivers/pci/hotplug/pciehp.h
>   drivers/pci/hotplug/pciehp_core.c
>   drivers/pci/hotplug/pciehp_ctrl.c
>   drivers/pci/hotplug/pciehp_hpc.c
>   drivers/pci/hotplug/pciehp_pci.c
> 
> To me, it just makes things harder because when I'm browsing for
> something in pciehp and I don't know the exact name of it, I have to
> guess which file it's in, and I'm invariably wrong.
> 
> It seems like it would be much simpler if everything were in a single
> pciehp.c file.  Then we could also get rid of the header and make
> several more things static.

I wouldn't go so far as to say there's *value* in the split.

It's a historic artefact, it's been like that since pciehp was
introduced back in 2004:
https://git.kernel.org/tglx/history/c/c16b4b14d980

I was hesitant to change it so far, in particular because it makes
it harder to backport stable patches.

That said, I did think about rearranging things to reduce the number
of files and non-static declarations or to give the files better names.
I wasn't thinking of squashing everything into one file, it would
probably be quite large and not make things simpler.

The general logic is that everything touching the registers is in
pciehp_hpc.c.  You won't (or shouldn't) find register access in any
of the other files.

pciehp_ctrl.c is the control logic, reaction to events, etc.
Though portions of the control logic are also in pciehp_hpc.c,
in particular the interrupt servicing.

pciehp_core.c contains the interface to the PCI hotplug core and
the probe/remove/PM routines.

pciehp_pci.c contains bringup/teardown of the slot.

One thing that I think deserves changing is this comment at the
top of each file:

"Send feedback to <greg@kroah.com>, <kristen.c.accardi@intel.com>"

We now use MAINTAINERS to do this.  Kristen took over maintainership
in 2005 with commit 8cf4c19523b7 but by 2009 she had stepped down per
commit be3652b8a253.  So providing her address is no longer accurate.
And I imagine Greg is no longer as familiar with the driver as it has
changed considerably since 2004.  He'd probably have to defer to others
who have done work on it recently (such as me).

Thanks,

Lukas