[v3,0/4] Simplify PCIe hotplug indicator control
mbox series

Message ID 20190819160643.27998-1-efremov@linux.com
Headers show
Series
  • Simplify PCIe hotplug indicator control
Related show

Message

Denis Efremov Aug. 19, 2019, 4:06 p.m. UTC
PCIe defines two optional hotplug indicators: a Power indicator and an
Attention indicator. Both are controlled by the same register, and each
can be on, off or blinking. The current interfaces
(pciehp_green_led_{on,off,blink}() and pciehp_set_attention_status()) are
non-uniform and require two register writes in many cases where we could
do one.

This patchset introduces the new function pciehp_set_indicators(). It
allows one to set two indicators with a single register write. All
calls to previous interfaces (pciehp_green_led_* and
pciehp_set_attention_status()) are replaced with a new one. Thus,
the amount of duplicated code for setting indicators is reduced.

Changes in v3:
  - Changed pciehp_set_indicators() to work with existing
    PCI_EXP_SLTCTL_* macros
  - Reworked the inputs validation in pciehp_set_indicators()
  - Removed pciehp_set_attention_status() and pciehp_green_led_*()
    completely

Denis Efremov (4):
  PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
  PCI: pciehp: Switch LED indicators with a single write
  PCI: pciehp: Remove pciehp_set_attention_status()
  PCI: pciehp: Remove pciehp_green_led_{on,off,blink}()

 drivers/pci/hotplug/pciehp.h      |  5 +-
 drivers/pci/hotplug/pciehp_core.c |  7 ++-
 drivers/pci/hotplug/pciehp_ctrl.c | 31 +++++++-----
 drivers/pci/hotplug/pciehp_hpc.c  | 82 ++++++++++---------------------
 include/uapi/linux/pci_regs.h     |  3 ++
 5 files changed, 54 insertions(+), 74 deletions(-)

Comments

Denis Efremov Aug. 20, 2019, 12:16 p.m. UTC | #1
On 8/19/19 7:06 PM, Denis Efremov wrote:
> PCIe defines two optional hotplug indicators: a Power indicator and an
> Attention indicator. Both are controlled by the same register, and each
> can be on, off or blinking. The current interfaces
> (pciehp_green_led_{on,off,blink}() and pciehp_set_attention_status()) are
> non-uniform and require two register writes in many cases where we could
> do one.
> 
> This patchset introduces the new function pciehp_set_indicators(). It
> allows one to set two indicators with a single register write. All
> calls to previous interfaces (pciehp_green_led_* and
> pciehp_set_attention_status()) are replaced with a new one. Thus,
> the amount of duplicated code for setting indicators is reduced.
> 
> Changes in v3:
>   - Changed pciehp_set_indicators() to work with existing
>     PCI_EXP_SLTCTL_* macros
>   - Reworked the inputs validation in pciehp_set_indicators()
>   - Removed pciehp_set_attention_status() and pciehp_green_led_*()
>     completely
> 
> Denis Efremov (4):
>   PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
>   PCI: pciehp: Switch LED indicators with a single write
>   PCI: pciehp: Remove pciehp_set_attention_status()
>   PCI: pciehp: Remove pciehp_green_led_{on,off,blink}()

Lukas, Sathyanarayanan, sorry that I've dropped most of yours "Reviewed-by".
The changes in the last 2 patches were significant.

> 
>  drivers/pci/hotplug/pciehp.h      |  5 +-
>  drivers/pci/hotplug/pciehp_core.c |  7 ++-
>  drivers/pci/hotplug/pciehp_ctrl.c | 31 +++++++-----
>  drivers/pci/hotplug/pciehp_hpc.c  | 82 ++++++++++---------------------
>  include/uapi/linux/pci_regs.h     |  3 ++
>  5 files changed, 54 insertions(+), 74 deletions(-)
>
Bjorn Helgaas Aug. 27, 2019, 10:32 p.m. UTC | #2
On Tue, Aug 20, 2019 at 03:16:43PM +0300, Denis Efremov wrote:
> On 8/19/19 7:06 PM, Denis Efremov wrote:
> > PCIe defines two optional hotplug indicators: a Power indicator and an
> > Attention indicator. Both are controlled by the same register, and each
> > can be on, off or blinking. The current interfaces
> > (pciehp_green_led_{on,off,blink}() and pciehp_set_attention_status()) are
> > non-uniform and require two register writes in many cases where we could
> > do one.
> > 
> > This patchset introduces the new function pciehp_set_indicators(). It
> > allows one to set two indicators with a single register write. All
> > calls to previous interfaces (pciehp_green_led_* and
> > pciehp_set_attention_status()) are replaced with a new one. Thus,
> > the amount of duplicated code for setting indicators is reduced.
> > 
> > Changes in v3:
> >   - Changed pciehp_set_indicators() to work with existing
> >     PCI_EXP_SLTCTL_* macros
> >   - Reworked the inputs validation in pciehp_set_indicators()
> >   - Removed pciehp_set_attention_status() and pciehp_green_led_*()
> >     completely
> > 
> > Denis Efremov (4):
> >   PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
> >   PCI: pciehp: Switch LED indicators with a single write
> >   PCI: pciehp: Remove pciehp_set_attention_status()
> >   PCI: pciehp: Remove pciehp_green_led_{on,off,blink}()
> 
> Lukas, Sathyanarayanan, sorry that I've dropped most of yours "Reviewed-by".
> The changes in the last 2 patches were significant.

Anybody want to review these?
Kuppuswamy Sathyanarayanan Aug. 27, 2019, 10:50 p.m. UTC | #3
On Tue, Aug 27, 2019 at 05:32:54PM -0500, Bjorn Helgaas wrote:
> On Tue, Aug 20, 2019 at 03:16:43PM +0300, Denis Efremov wrote:
> > On 8/19/19 7:06 PM, Denis Efremov wrote:
> > > PCIe defines two optional hotplug indicators: a Power indicator and an
> > > Attention indicator. Both are controlled by the same register, and each
> > > can be on, off or blinking. The current interfaces
> > > (pciehp_green_led_{on,off,blink}() and pciehp_set_attention_status()) are
> > > non-uniform and require two register writes in many cases where we could
> > > do one.
> > > 
> > > This patchset introduces the new function pciehp_set_indicators(). It
> > > allows one to set two indicators with a single register write. All
> > > calls to previous interfaces (pciehp_green_led_* and
> > > pciehp_set_attention_status()) are replaced with a new one. Thus,
> > > the amount of duplicated code for setting indicators is reduced.
> > > 
> > > Changes in v3:
> > >   - Changed pciehp_set_indicators() to work with existing
> > >     PCI_EXP_SLTCTL_* macros
> > >   - Reworked the inputs validation in pciehp_set_indicators()
> > >   - Removed pciehp_set_attention_status() and pciehp_green_led_*()
> > >     completely
> > > 
> > > Denis Efremov (4):
> > >   PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
> > >   PCI: pciehp: Switch LED indicators with a single write
> > >   PCI: pciehp: Remove pciehp_set_attention_status()
> > >   PCI: pciehp: Remove pciehp_green_led_{on,off,blink}()
> > 
> > Lukas, Sathyanarayanan, sorry that I've dropped most of yours "Reviewed-by".
> > The changes in the last 2 patches were significant.
> 
> Anybody want to review these?

Except for one nitpick in [PATCH v3 1/4], rest seems fine to me.
Bjorn Helgaas Aug. 27, 2019, 10:53 p.m. UTC | #4
On Tue, Aug 27, 2019 at 05:32:54PM -0500, Bjorn Helgaas wrote:
> On Tue, Aug 20, 2019 at 03:16:43PM +0300, Denis Efremov wrote:
> > On 8/19/19 7:06 PM, Denis Efremov wrote:
> > > PCIe defines two optional hotplug indicators: a Power indicator and an
> > > Attention indicator. Both are controlled by the same register, and each
> > > can be on, off or blinking. The current interfaces
> > > (pciehp_green_led_{on,off,blink}() and pciehp_set_attention_status()) are
> > > non-uniform and require two register writes in many cases where we could
> > > do one.
> > > 
> > > This patchset introduces the new function pciehp_set_indicators(). It
> > > allows one to set two indicators with a single register write. All
> > > calls to previous interfaces (pciehp_green_led_* and
> > > pciehp_set_attention_status()) are replaced with a new one. Thus,
> > > the amount of duplicated code for setting indicators is reduced.
> > > 
> > > Changes in v3:
> > >   - Changed pciehp_set_indicators() to work with existing
> > >     PCI_EXP_SLTCTL_* macros
> > >   - Reworked the inputs validation in pciehp_set_indicators()
> > >   - Removed pciehp_set_attention_status() and pciehp_green_led_*()
> > >     completely
> > > 
> > > Denis Efremov (4):
> > >   PCI: pciehp: Add pciehp_set_indicators() to jointly set LED indicators
> > >   PCI: pciehp: Switch LED indicators with a single write
> > >   PCI: pciehp: Remove pciehp_set_attention_status()
> > >   PCI: pciehp: Remove pciehp_green_led_{on,off,blink}()
> > 
> > Lukas, Sathyanarayanan, sorry that I've dropped most of yours "Reviewed-by".
> > The changes in the last 2 patches were significant.
> 
> Anybody want to review these?

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.

Bjorn
Lukas Wunner Aug. 28, 2019, 3:33 a.m. UTC | #5
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