On Wed, Jun 18, 2025 at 07:37:54PM -0500, Timothy Pearson wrote:
> ----- Original Message -----
> > From: "Bjorn Helgaas" <helg...@kernel.org>
> > To: "Timothy Pearson" <tpear...@raptorengineering.com>
> > Cc: "linuxppc-dev" <linuxppc-dev@lists.ozlabs.org>, "linux-kernel"
> > <linux-ker...@vger.kernel.org>, "linux-pci"
> > <linux-...@vger.kernel.org>, "Madhavan Srinivasan" <ma...@linux.ibm.com>,
> > "Michael Ellerman" <m...@ellerman.id.au>,
> > "christophe leroy" <christophe.le...@csgroup.eu>, "Naveen N Rao"
> > <nav...@kernel.org>, "Bjorn Helgaas"
> > <bhelg...@google.com>, "Shawn Anastasio" <sanasta...@raptorengineering.com>
> > 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.
No worries. If you can figure out how to make your mailer use the
normal "On xxx, somebody wrote:" attribution instead of duplicating
all those headers, that would be far more useful :)
> >> +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()?
Nope, I think it's fine to keep this looking like pciehp for now.
If somebody wants to use FIELD_GET() in pciehp, I'd probably be OK
with that, but no need for you to open that can of worms.
> > 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?
I'm OK with changing this, but I do think it would be worth calling
out the different behavior in the commit log.
Bjorn
