Re: [PATCH 2/2] PCI: pciehp: Deduplicate presence check on probe & resume

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Sat, Jul 28, 2018 at 07:22:00AM +0200, Lukas Wunner wrote:
> On driver probe and on resume from system sleep, pciehp checks the
> Presence Detect State bit in the Slot Status register to bring up an
> occupied slot or bring down an unoccupied slot.  Both code paths are
> identical, so deduplicate them per Mika's request.
> 
> On probe, an additional check is performed to disable power of an
> unoccupied slot.  This can e.g. happen if power was enabled by BIOS.
> It cannot happen once pciehp has taken control, hence is not necessary
> on resume:  The Slot Control register is set to the same value that it
> had on suspend by pci_restore_state(), so if the slot was occupied,
> power is enabled and if it wasn't, power is disabled.  Should occupancy
> have changed during the system sleep transition, power is adjusted by
> bringing up or down the slot per the paragraph above.
> 
> To allow for deduplication of the presence check, move the power check
> to pcie_init().  This seems safer anyway, because right now it is
> performed while interrupts are already enabled, and although I can't
> think of a scenario where pciehp_power_off_slot() and the IRQ thread
> collide, it does feel brittle.
> 
> However this means that pcie_init() may now write to the Slot Control
> register before the IRQ is requested.  If both the CCIE and HPIE bits
> happen to be set, pcie_wait_cmd() will wait for an interrupt (instead
> of polling the Command Completed bit) and eventually emit a timeout
> message.  Additionally, if a level-triggered INTx interrupt is used,
> the user may see a spurious interrupt splat.  Avoid by disabling
> interrupts before disabling power.  (Normally the HPIE and CCIE bits
> should be clear on probe, but conceivably they may already have been
> set e.g. by BIOS.)
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

One small nit, see below:

> ---
>  drivers/pci/hotplug/pciehp_core.c | 63 ++++++++++++++++---------------
>  drivers/pci/hotplug/pciehp_hpc.c  | 14 +++++++
>  2 files changed, 46 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 01fcf1fa0f66..ec48c9433ae5 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -200,12 +200,40 @@ static int reset_slot(struct hotplug_slot *hotplug_slot, int probe)
>  	return pciehp_reset_slot(slot, probe);
>  }
>  
> +/**
> + * pciehp_check_presence() - synthesize event if presence has changed

You should document the argument as well so that the tool that is used
to process these knows about it:

  @ctrl: ....

> + *
> + * On probe and resume, an explicit presence check is necessary to bring up an
> + * occupied slot or bring down an unoccupied slot.  This can't be triggered by
> + * events in the Slot Status register, they may be stale and are therefore
> + * cleared.  Secondly, sending an interrupt for "events that occur while
> + * interrupt generation is disabled [when] interrupt generation is subsequently
> + * enabled" is optional per PCIe r4.0, sec 6.7.3.4.
> + */
> +static void pciehp_check_presence(struct controller *ctrl)