Re: [PATCH v2 6/6] pci/hotplug/pnv_php: Enable third attention indicator

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Jun 18, 2025 at 11:58:59AM -0500, Timothy Pearson wrote:
>  state

Weird wrapping of last word of subject to here.

> The PCIe specification allows three attention indicator states,
> on, off, and blink.  Enable all three states instead of basic
> on / off control.
> 
> Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>
> ---
>  drivers/pci/hotplug/pnv_php.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
> index 0ceb4a2c3c79..c3005324be3d 100644
> --- a/drivers/pci/hotplug/pnv_php.c
> +++ b/drivers/pci/hotplug/pnv_php.c
> @@ -440,10 +440,23 @@ static int pnv_php_get_adapter_state(struct hotplug_slot *slot, u8 *state)
>  	return ret;
>  }
>  
> +static int pnv_php_get_raw_indicator_status(struct hotplug_slot *slot, u8 *state)
> +{
> +	struct pnv_php_slot *php_slot = to_pnv_php_slot(slot);
> +	struct pci_dev *bridge = php_slot->pdev;
> +	u16 status;
> +
> +	pcie_capability_read_word(bridge, PCI_EXP_SLTCTL, &status);
> +	*state = (status & (PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC)) >> 6;

Should be able to do this with FIELD_GET().

Is the PCI_EXP_SLTCTL_PIC part needed?  It wasn't there before, commit
log doesn't mention it, and as far as I can tell, this would be the
only driver to do that.  Most expose only the attention status (0=off,
1=on, 2=identify/blink).

> +	return 0;
> +}
> +
> +
>  static int pnv_php_get_attention_state(struct hotplug_slot *slot, u8 *state)
>  {
>  	struct pnv_php_slot *php_slot = to_pnv_php_slot(slot);
>  
> +	pnv_php_get_raw_indicator_status(slot, &php_slot->attention_state);

This is a change worth noting.  Previously we didn't read the AIC
state from PCI_EXP_SLTCTL at all; we used php_slot->attention_state to
keep track of whatever had been previously set via
pnv_php_set_attention_state().

Now we read the current state from PCI_EXP_SLTCTL.  It's not clear
that php_slot->attention_state is still needed at all.

Previously, the user could write any value at all to the sysfs
"attention" file and then read that same value back.  After this
patch, the user can still write anything, but reads will only return
values with PCI_EXP_SLTCTL_AIC and PCI_EXP_SLTCTL_PIC.

>  	*state = php_slot->attention_state;
>  	return 0;
>  }
> @@ -461,7 +474,7 @@ static int pnv_php_set_attention_state(struct hotplug_slot *slot, u8 state)
>  	mask = PCI_EXP_SLTCTL_AIC;
>  
>  	if (state)
> -		new = PCI_EXP_SLTCTL_ATTN_IND_ON;
> +		new = FIELD_PREP(PCI_EXP_SLTCTL_AIC, state);

This changes the behavior in some cases:

  write 0: previously turned indicator off, now writes reserved value
  write 2: previously turned indicator on, now sets to blink
  write 3: previously turned indicator on, now turns it off

>  	else
>  		new = PCI_EXP_SLTCTL_ATTN_IND_OFF;
>  
> -- 
> 2.39.5