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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250624223413.GA1550003@bhelgaas>
Date: Tue, 24 Jun 2025 17:34:13 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Timothy Pearson <tpearson@...torengineering.com>
Cc: linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	linux-pci <linux-pci@...r.kernel.org>,
	Madhavan Srinivasan <maddy@...ux.ibm.com>,
	Michael Ellerman <mpe@...erman.id.au>,
	christophe leroy <christophe.leroy@...roup.eu>,
	Naveen N Rao <naveen@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Shawn Anastasio <sanastasio@...torengineering.com>
Subject: Re: [PATCH v2 6/6] pci/hotplug/pnv_php: Enable third attention
 indicator

On Wed, Jun 18, 2025 at 07:37:54PM -0500, Timothy Pearson wrote:
> ----- Original Message -----
> > From: "Bjorn Helgaas" <helgaas@...nel.org>
> > To: "Timothy Pearson" <tpearson@...torengineering.com>
> > Cc: "linuxppc-dev" <linuxppc-dev@...ts.ozlabs.org>, "linux-kernel" <linux-kernel@...r.kernel.org>, "linux-pci"
> > <linux-pci@...r.kernel.org>, "Madhavan Srinivasan" <maddy@...ux.ibm.com>, "Michael Ellerman" <mpe@...erman.id.au>,
> > "christophe leroy" <christophe.leroy@...roup.eu>, "Naveen N Rao" <naveen@...nel.org>, "Bjorn Helgaas"
> > <bhelgaas@...gle.com>, "Shawn Anastasio" <sanastasio@...torengineering.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