Message-ID: <19689b53-ac23-4b48-97c7-b26f360a7b75@linux.ibm.com>
Date: Fri, 20 Jun 2025 14:56:51 +0530
From: Krishna Kumar <krishnak@...ux.ibm.com>
To: linuxppc-dev@...ts.ozlabs.org,
        Timothy Pearson <tpearson@...torengineering.com>,
        Shawn Anastasio <sanastasio@...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

Shawn, Timothy:

Thanks for posting the series of patch. Few things I wanted to do better understand Raptor problem -


1. Last time my two patches solved all the hotunplug operation and Kernel crash issue except nvme case. It did not work with

    NVME since dpc support was not there. I was not able to do that due to being  occupied in some other work.

2. I want to understand the delta from last yr problem to this problem. Is the PHB freeze or hotunplug failure happens

    only for particular Microsemi switch or it happens with all the switches. When did this problem started coming ? Till last yr 

    it was not there. Is it specific to particular Hardware ? Can I get your setup to test this problem and your patch ?

3. To me, hot unplug opertaion  --> AER triggering --> DPC support, this flow should mask the error to reach root port/cpu and it 

    should solve the PHB freeze/ hot unplug failure operation. If there are AER/EEH related synchronization issue we need to solve them. 

    Can you pls list the issue, I will pass it to EEH/AER team. But yeah, to me if AER implementation is correct and we add DPC support,

    all the error will be contained by switch itself. The PHB/root port/cpu will not be impacted by this and there should not be any freeze.

4. Ofcourse we can pick some of the fixes from pciehp driver if its missing in pnv_php.c. Also at the same time you have done

    some cleanup in hot unplug path and fixed the attenuation button related code. If these works fine, we can pick it. But I want to test it.

     Pls provide me setup.

5. If point 3 and 4 does not solve the problem, then only we should move to pciehp.c. But AFAIK, PPC/Powernv is DT based while pciehp.c

     may be only supporting acpi (I have to check it on this).  We need to provide PHB related information via DTB and maintain the related 

     topology information via dtb and then it can be doable. Also , we need to do thorough planning/testing if we think to choose pciehp.c. 

     But yeah, lets not jump here and lets try to fix the current issues via point 3 & 4. Point 5 will be our last option.


Best Regards,

Krishna




On 6/19/25 6:07 AM, 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.
>
>>> 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@...torengineering.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().
> 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?
>

Powered by blists - more mailing lists