----- Original Message -----
> From: "Bjorn Helgaas" <helgaas@xxxxxxxxxx>
> To: "Timothy Pearson" <tpearson@xxxxxxxxxxxxxxxxxxxxx>
> Cc: "linuxppc-dev" <linuxppc-dev@xxxxxxxxxxxxxxxx>, "linux-kernel" <linux-kernel@xxxxxxxxxxxxxxx>, "linux-pci"
> <linux-pci@xxxxxxxxxxxxxxx>, "Madhavan Srinivasan" <maddy@xxxxxxxxxxxxx>, "Michael Ellerman" <mpe@xxxxxxxxxxxxxx>,
> "christophe leroy" <christophe.leroy@xxxxxxxxxx>, "Naveen N Rao" <naveen@xxxxxxxxxx>, "Bjorn Helgaas"
> <bhelgaas@xxxxxxxxxx>, "Shawn Anastasio" <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> Sent: Wednesday, June 18, 2025 2:01:46 PM
> Subject: Re: [PATCH v2 6/6] pci/hotplug/pnv_php: Enable third attention indicator
> On Wed, Jun 18, 2025 at 11:58:59AM -0500, Timothy Pearson wrote:
>> state
>
> Weird wrapping of last word of subject to here.
I'll need to see what's up with my git format-patch setup. Apologies for that across the multiple series.
>> 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@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>> 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().
I used the same overall structure as the pciehp_hpc driver here. Do you want me to also fix up that driver 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.
It probably isn't. It's unclear why IBM took this path at all, given pciehp's attention handlers predate pnv-php's by many years.
> 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
If we're looking at normalizing with pciehp with an eye toward eventually deprecating / removing pnv-php, I can't think of a better time to change this behavior. I suspect we're the only major user of this code path at the moment, with most software expecting to see pciehp-style handling. Thoughts?
