Re: [PATCH] PCI: Call _REG when saving/restoring PCI state

Re: [PATCH] PCI: Call _REG when saving/restoring PCI state

[Bjorn Helgaas]

[+cc Rafael, Len, linux-acpi]

Hi Mario,

On Thu, Jun 01, 2023 at 10:11:22PM -0500, Mario Limonciello wrote:
> ASMedia PCIe GPIO controllers connected to AMD SOC fail functional tests
> after returning from s2idle. This is because the BIOS checks whether the
> OSPM has called the _REG method to determine whether it can interact with
> the OperationRegion assigned to the device.

"s2idle" is a Linux term; I'd prefer something that we can relate to
the ACPI spec.

Maybe a pointer to the specific function in the driver that has a
problem?  Based on the patch, I assume the driver uses some control
method that looks at PCI config space?

> To fix this issue, call acpi_evaluate_reg() when saving and restoring the
> state of PCI devices.

Please include the spec citation: ACPI r6.5, sec 6.5.4.  The URL has
changed in the past and may change in the future, but the name/section
number will not.

> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/pci/pci.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e38c2f6eebd4..071ecba548b0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1068,6 +1068,12 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
>  	return acpi_pci_bridge_d3(dev);
>  }
>  
> +static inline int platform_toggle_reg(struct pci_dev *dev, int c)
> +{
> +	return acpi_evaluate_reg(ACPI_HANDLE(&dev->dev),
> +				 ACPI_ADR_SPACE_PCI_CONFIG, c);
> +}

You never check the return value, so why return it?

The function actually doesn't *toggle*; it connects or disconnects
based on "c".

This looks like it only builds when CONFIG_ACPI=y?

>  /**
>   * pci_update_current_state - Read power state of given device and cache it
>   * @dev: PCI device to handle.
> @@ -1645,6 +1651,9 @@ static void pci_restore_ltr_state(struct pci_dev *dev)
>  int pci_save_state(struct pci_dev *dev)
>  {
>  	int i;
> +
> +	platform_toggle_reg(dev, ACPI_REG_DISCONNECT);

I would expect these to be in the PM code near the power state
transitions, not in the state save/restore code.  These functions
*are* used during suspend/resume, but are used in other places as
well, where we probably don't want _REG executed.

Cc'd Rafael and PM folks, who can give much better feedback.

>  	/* XXX: 100% dword access ok here? */
>  	for (i = 0; i < 16; i++) {
>  		pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]);
> @@ -1790,6 +1799,8 @@ void pci_restore_state(struct pci_dev *dev)
>  	pci_enable_acs(dev);
>  	pci_restore_iov_state(dev);
>  
> +	platform_toggle_reg(dev, ACPI_REG_CONNECT);
> +
>  	dev->state_saved = false;
>  }
>  EXPORT_SYMBOL(pci_restore_state);
> @@ -3203,6 +3214,7 @@ void pci_pm_init(struct pci_dev *dev)
>  	pci_read_config_word(dev, PCI_STATUS, &status);
>  	if (status & PCI_STATUS_IMM_READY)
>  		dev->imm_ready = 1;
> +	platform_toggle_reg(dev, ACPI_REG_CONNECT);
>  }
>  
>  static unsigned long pci_ea_flags(struct pci_dev *dev, u8 prop)
> -- 
> 2.34.1
> 

Re: [PATCH] PCI: Call _REG when saving/restoring PCI state

[Limonciello, Mario]

On 6/2/2023 3:21 PM, Bjorn Helgaas wrote:
> [+cc Rafael, Len, linux-acpi]
>
> Hi Mario,
>
> On Thu, Jun 01, 2023 at 10:11:22PM -0500, Mario Limonciello wrote:
>> ASMedia PCIe GPIO controllers connected to AMD SOC fail functional tests
>> after returning from s2idle. This is because the BIOS checks whether the
>> OSPM has called the _REG method to determine whether it can interact with
>> the OperationRegion assigned to the device.
> "s2idle" is a Linux term; I'd prefer something that we can relate to
> the ACPI spec.
It's important for the symptoms of this issue though, this
problem doesn't trigger "just" by moving D-states.

It happens as a result of system suspend.

>
> Maybe a pointer to the specific function in the driver that has a
> problem?  Based on the patch, I assume the driver uses some control
> method that looks at PCI config space?

The issue isn't in anything Linux code "does"; it's in the "lack"
of Linux code doing what it needs to IE using _REG.

At least for this issue _REG is treated like a lock mechanism.
In the spec it says specifically:

"When an operation region handler is unavailable, AML cannot access
data fields in that region".

That is it's to ensure that OSPM and AML don't both simultaneously
access the same region.

What happens is that AML normally wants to access this region during
suspend, but without the sequence of calling _REG it can't.

>
>> To fix this issue, call acpi_evaluate_reg() when saving and restoring the
>> state of PCI devices.
> Please include the spec citation: ACPI r6.5, sec 6.5.4.  The URL has
> changed in the past and may change in the future, but the name/section
> number will not.
Sure.
>
>> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/pci/pci.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index e38c2f6eebd4..071ecba548b0 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -1068,6 +1068,12 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
>>   	return acpi_pci_bridge_d3(dev);
>>   }
>>   
>> +static inline int platform_toggle_reg(struct pci_dev *dev, int c)
>> +{
>> +	return acpi_evaluate_reg(ACPI_HANDLE(&dev->dev),
>> +				 ACPI_ADR_SPACE_PCI_CONFIG, c);
>> +}
> You never check the return value, so why return it?

_REG isn't mandatory for any of these uses, and I wanted to make
sure that if it does end up being mandatory in a future use that
the return code wasn't thrown away.  If you think it's better to
just throw it away now, I have no qualms making it a void instead.

>
> The function actually doesn't *toggle*; it connects or disconnects
> based on "c".
Can you suggest a better function name?
>
> This looks like it only builds when CONFIG_ACPI=y?

The prototype for acpi_evaluate_reg isn't guarded by CONFIG_ACPI
so I figured it worked both ways.

But looking again I don't see a dummy implementation version for
the lack of CONFIG_ACPI, so I'll double check it.

>
>>   /**
>>    * pci_update_current_state - Read power state of given device and cache it
>>    * @dev: PCI device to handle.
>> @@ -1645,6 +1651,9 @@ static void pci_restore_ltr_state(struct pci_dev *dev)
>>   int pci_save_state(struct pci_dev *dev)
>>   {
>>   	int i;
>> +
>> +	platform_toggle_reg(dev, ACPI_REG_DISCONNECT);
> I would expect these to be in the PM code near the power state
> transitions, not in the state save/restore code.  These functions
> *are* used during suspend/resume, but are used in other places as
> well, where we probably don't want _REG executed.
>
> Cc'd Rafael and PM folks, who can give much better feedback.
My knee jerk reaction when we found the root cause for this issue
was to put the code right around the D-state transitions, but I
decided against this.

I put it in save/restore intentionally because
like I mentioned above it's treated like a locking mechanism between
OSPM and AML and it's not functionally tied to a D-state transition.

When the state is saved it's like Linux says
"I'm done using this region, go ahead and touch it firmware".
When it's restored it's like Linux says
"Don't use that region, I'm claiming it for now".

Think about that other patch I wrote recently that controls D3
availability [1].  If it was only run in the D-state transitions and
the root port stays in D0 but has a _REG method it would never get
called.

>>   	/* XXX: 100% dword access ok here? */
>>   	for (i = 0; i < 16; i++) {
>>   		pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]);
>> @@ -1790,6 +1799,8 @@ void pci_restore_state(struct pci_dev *dev)
>>   	pci_enable_acs(dev);
>>   	pci_restore_iov_state(dev);
>>   
>> +	platform_toggle_reg(dev, ACPI_REG_CONNECT);
>> +
>>   	dev->state_saved = false;
>>   }
>>   EXPORT_SYMBOL(pci_restore_state);
>> @@ -3203,6 +3214,7 @@ void pci_pm_init(struct pci_dev *dev)
>>   	pci_read_config_word(dev, PCI_STATUS, &status);
>>   	if (status & PCI_STATUS_IMM_READY)
>>   		dev->imm_ready = 1;
>> +	platform_toggle_reg(dev, ACPI_REG_CONNECT);
>>   }
>>   
>>   static unsigned long pci_ea_flags(struct pci_dev *dev, u8 prop)
>> -- 
>> 2.34.1
[1] 
https://lore.kernel.org/linux-pci/20230530163947.230418-2-mario.limonciello@amd.com/ 

Re: [PATCH] PCI: Call _REG when saving/restoring PCI state

[Rafael J. Wysocki]

On Fri, Jun 2, 2023 at 11:57 PM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
>
> On 6/2/2023 3:21 PM, Bjorn Helgaas wrote:
> > [+cc Rafael, Len, linux-acpi]
> >
> > Hi Mario,
> >
> > On Thu, Jun 01, 2023 at 10:11:22PM -0500, Mario Limonciello wrote:
> >> ASMedia PCIe GPIO controllers connected to AMD SOC fail functional tests
> >> after returning from s2idle. This is because the BIOS checks whether the
> >> OSPM has called the _REG method to determine whether it can interact with
> >> the OperationRegion assigned to the device.
> > "s2idle" is a Linux term; I'd prefer something that we can relate to
> > the ACPI spec.
> It's important for the symptoms of this issue though, this
> problem doesn't trigger "just" by moving D-states.
>
> It happens as a result of system suspend.

As I said in my response to Bjorn, s2idle is D0 from the ACPI
standpoint.  It is not a system sleep and it has no special meaning in
ACPI.

The problem seems to be related to the low-power S0 idle _DSM calls to me.

> >
> > Maybe a pointer to the specific function in the driver that has a
> > problem?  Based on the patch, I assume the driver uses some control
> > method that looks at PCI config space?
>
> The issue isn't in anything Linux code "does"; it's in the "lack"
> of Linux code doing what it needs to IE using _REG.

So the argument seems to be that under certain conditions the PCI
config space becomes unavailable and so _REG(dev, 0) needs to be
called when this is about to happen and _REG(dev, 1) needs to be
called when the config space becomes available again.  Fair enough,
but I'm not sure why this is limited to system suspend and resume.

Moreover, "PCI_Config operation regions on a PCI root bus containing a
_BBN object" are specifically mentioned as one of the cases when _REG
need not be evaluated at all.  I guess the operation region in
question doesn't fall into that category?

> At least for this issue _REG is treated like a lock mechanism.
> In the spec it says specifically:
>
> "When an operation region handler is unavailable, AML cannot access
> data fields in that region".
>
> That is it's to ensure that OSPM and AML don't both simultaneously
> access the same region.
>
> What happens is that AML normally wants to access this region during
> suspend, but without the sequence of calling _REG it can't.

Is this about being unable to access the opregion or racing with
concurrent accesses on the OS side?

> >
> >> To fix this issue, call acpi_evaluate_reg() when saving and restoring the
> >> state of PCI devices.
> > Please include the spec citation: ACPI r6.5, sec 6.5.4.  The URL has
> > changed in the past and may change in the future, but the name/section
> > number will not.
> Sure.
> >
> >> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region
> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >> ---
> >>   drivers/pci/pci.c | 12 ++++++++++++
> >>   1 file changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >> index e38c2f6eebd4..071ecba548b0 100644
> >> --- a/drivers/pci/pci.c
> >> +++ b/drivers/pci/pci.c
> >> @@ -1068,6 +1068,12 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
> >>      return acpi_pci_bridge_d3(dev);
> >>   }
> >>
> >> +static inline int platform_toggle_reg(struct pci_dev *dev, int c)
> >> +{
> >> +    return acpi_evaluate_reg(ACPI_HANDLE(&dev->dev),
> >> +                             ACPI_ADR_SPACE_PCI_CONFIG, c);
> >> +}
> > You never check the return value, so why return it?
>
> _REG isn't mandatory for any of these uses, and I wanted to make
> sure that if it does end up being mandatory in a future use that
> the return code wasn't thrown away.  If you think it's better to
> just throw it away now, I have no qualms making it a void instead.

I don't think it can reasonably become mandatory without adding a
specific _OSC bit for that.

> >
> > The function actually doesn't *toggle*; it connects or disconnects
> > based on "c".
> Can you suggest a better function name?
> >
> > This looks like it only builds when CONFIG_ACPI=y?
>
> The prototype for acpi_evaluate_reg isn't guarded by CONFIG_ACPI
> so I figured it worked both ways.
>
> But looking again I don't see a dummy implementation version for
> the lack of CONFIG_ACPI, so I'll double check it.
>
> >
> >>   /**
> >>    * pci_update_current_state - Read power state of given device and cache it
> >>    * @dev: PCI device to handle.
> >> @@ -1645,6 +1651,9 @@ static void pci_restore_ltr_state(struct pci_dev *dev)
> >>   int pci_save_state(struct pci_dev *dev)
> >>   {
> >>      int i;
> >> +
> >> +    platform_toggle_reg(dev, ACPI_REG_DISCONNECT);
> > I would expect these to be in the PM code near the power state
> > transitions, not in the state save/restore code.  These functions
> > *are* used during suspend/resume, but are used in other places as
> > well, where we probably don't want _REG executed.
> >
> > Cc'd Rafael and PM folks, who can give much better feedback.
> My knee jerk reaction when we found the root cause for this issue
> was to put the code right around the D-state transitions, but I
> decided against this.
>
> I put it in save/restore intentionally because
> like I mentioned above it's treated like a locking mechanism between
> OSPM and AML and it's not functionally tied to a D-state transition.
>
> When the state is saved it's like Linux says
> "I'm done using this region, go ahead and touch it firmware".
> When it's restored it's like Linux says
> "Don't use that region, I'm claiming it for now".

So it looks like you want to use _REG for protecting PCI config space
against concurrent accesses from AML and the OS.

> Think about that other patch I wrote recently that controls D3
> availability [1].  If it was only run in the D-state transitions and
> the root port stays in D0 but has a _REG method it would never get
> called.

And why should it be evaluated in that case?

Re: [PATCH] PCI: Call _REG when saving/restoring PCI state

[Limonciello, Mario]

On 6/4/2023 6:30 AM, Rafael J. Wysocki wrote:
> On Fri, Jun 2, 2023 at 11:57 PM Limonciello, Mario
> <mario.limonciello@amd.com> wrote:
>>
>> On 6/2/2023 3:21 PM, Bjorn Helgaas wrote:
>>> [+cc Rafael, Len, linux-acpi]
>>>
>>> Hi Mario,
>>>
>>> On Thu, Jun 01, 2023 at 10:11:22PM -0500, Mario Limonciello wrote:
>>>> ASMedia PCIe GPIO controllers connected to AMD SOC fail functional tests
>>>> after returning from s2idle. This is because the BIOS checks whether the
>>>> OSPM has called the _REG method to determine whether it can interact with
>>>> the OperationRegion assigned to the device.
>>> "s2idle" is a Linux term; I'd prefer something that we can relate to
>>> the ACPI spec.
>> It's important for the symptoms of this issue though, this
>> problem doesn't trigger "just" by moving D-states.
>>
>> It happens as a result of system suspend.
> As I said in my response to Bjorn, s2idle is D0 from the ACPI
> standpoint.  It is not a system sleep and it has no special meaning in
> ACPI.
>
> The problem seems to be related to the low-power S0 idle _DSM calls to me.

This particular hardware that triggered this patch can do S3
or s2idle.

Let me confirm with internal guys whether this can reproduce
with BIOS configured to S3 as well.
>>> Maybe a pointer to the specific function in the driver that has a
>>> problem?  Based on the patch, I assume the driver uses some control
>>> method that looks at PCI config space?
>> The issue isn't in anything Linux code "does"; it's in the "lack"
>> of Linux code doing what it needs to IE using _REG.
> So the argument seems to be that under certain conditions the PCI
> config space becomes unavailable and so _REG(dev, 0) needs to be
> called when this is about to happen and _REG(dev, 1) needs to be
> called when the config space becomes available again.  Fair enough,
> but I'm not sure why this is limited to system suspend and resume.
I didn't think it should be limited to suspend/resume
either.

That's why I had put it in save state/restore state.

> Moreover, "PCI_Config operation regions on a PCI root bus containing a
> _BBN object" are specifically mentioned as one of the cases when _REG
> need not be evaluated at all.  I guess the operation region in
> question doesn't fall into that category?

Yes; that's right.  _BBN is only present on \_SB_.PCI0
and the problematic device is on \_SB_.PCI0.GPP5.

>> At least for this issue _REG is treated like a lock mechanism.
>> In the spec it says specifically:
>>
>> "When an operation region handler is unavailable, AML cannot access
>> data fields in that region".
>>
>> That is it's to ensure that OSPM and AML don't both simultaneously
>> access the same region.
>>
>> What happens is that AML normally wants to access this region during
>> suspend, but without the sequence of calling _REG it can't.
> Is this about being unable to access the opregion or racing with
> concurrent accesses on the OS side?
Access.
>
>>>> To fix this issue, call acpi_evaluate_reg() when saving and restoring the
>>>> state of PCI devices.
>>> Please include the spec citation: ACPI r6.5, sec 6.5.4.  The URL has
>>> changed in the past and may change in the future, but the name/section
>>> number will not.
>> Sure.
>>>> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>>    drivers/pci/pci.c | 12 ++++++++++++
>>>>    1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index e38c2f6eebd4..071ecba548b0 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -1068,6 +1068,12 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
>>>>       return acpi_pci_bridge_d3(dev);
>>>>    }
>>>>
>>>> +static inline int platform_toggle_reg(struct pci_dev *dev, int c)
>>>> +{
>>>> +    return acpi_evaluate_reg(ACPI_HANDLE(&dev->dev),
>>>> +                             ACPI_ADR_SPACE_PCI_CONFIG, c);
>>>> +}
>>> You never check the return value, so why return it?
>> _REG isn't mandatory for any of these uses, and I wanted to make
>> sure that if it does end up being mandatory in a future use that
>> the return code wasn't thrown away.  If you think it's better to
>> just throw it away now, I have no qualms making it a void instead.
> I don't think it can reasonably become mandatory without adding a
> specific _OSC bit for that.
OK.
>
>>> The function actually doesn't *toggle*; it connects or disconnects
>>> based on "c".
>> Can you suggest a better function name?
>>> This looks like it only builds when CONFIG_ACPI=y?
>> The prototype for acpi_evaluate_reg isn't guarded by CONFIG_ACPI
>> so I figured it worked both ways.
>>
>> But looking again I don't see a dummy implementation version for
>> the lack of CONFIG_ACPI, so I'll double check it.
>>
>>>>    /**
>>>>     * pci_update_current_state - Read power state of given device and cache it
>>>>     * @dev: PCI device to handle.
>>>> @@ -1645,6 +1651,9 @@ static void pci_restore_ltr_state(struct pci_dev *dev)
>>>>    int pci_save_state(struct pci_dev *dev)
>>>>    {
>>>>       int i;
>>>> +
>>>> +    platform_toggle_reg(dev, ACPI_REG_DISCONNECT);
>>> I would expect these to be in the PM code near the power state
>>> transitions, not in the state save/restore code.  These functions
>>> *are* used during suspend/resume, but are used in other places as
>>> well, where we probably don't want _REG executed.
>>>
>>> Cc'd Rafael and PM folks, who can give much better feedback.
>> My knee jerk reaction when we found the root cause for this issue
>> was to put the code right around the D-state transitions, but I
>> decided against this.
>>
>> I put it in save/restore intentionally because
>> like I mentioned above it's treated like a locking mechanism between
>> OSPM and AML and it's not functionally tied to a D-state transition.
>>
>> When the state is saved it's like Linux says
>> "I'm done using this region, go ahead and touch it firmware".
>> When it's restored it's like Linux says
>> "Don't use that region, I'm claiming it for now".
> So it looks like you want to use _REG for protecting PCI config space
> against concurrent accesses from AML and the OS.
Yeah.  When I discussed it with BIOS guys they
explained to me that the BIOS will typically save/restore the
PCIe device BAR when _REG is called (depending on the argument
to _REG).

They'll only operate on the region when it's in the right
state, and they'll restore it as necessary when OSPM would use
it again.

This is also how Windows works.

>> Think about that other patch I wrote recently that controls D3
>> availability [1].  If it was only run in the D-state transitions and
>> the root port stays in D0 but has a _REG method it would never get
>> called.
> And why should it be evaluated in that case?
No matter what the actual D-state is the OSPM isn't
accessing it anymore, so AML should be able to.

Re: [PATCH] PCI: Call _REG when saving/restoring PCI state

[Limonciello, Mario]

On 6/5/2023 1:33 PM, Limonciello, Mario wrote:
>
> On 6/4/2023 6:30 AM, Rafael J. Wysocki wrote:
>> On Fri, Jun 2, 2023 at 11:57 PM Limonciello, Mario
>> <mario.limonciello@amd.com> wrote:
>>>
>>> On 6/2/2023 3:21 PM, Bjorn Helgaas wrote:
>>>> [+cc Rafael, Len, linux-acpi]
>>>>
>>>> Hi Mario,
>>>>
>>>> On Thu, Jun 01, 2023 at 10:11:22PM -0500, Mario Limonciello wrote:
>>>>> ASMedia PCIe GPIO controllers connected to AMD SOC fail functional 
>>>>> tests
>>>>> after returning from s2idle. This is because the BIOS checks 
>>>>> whether the
>>>>> OSPM has called the _REG method to determine whether it can 
>>>>> interact with
>>>>> the OperationRegion assigned to the device.
>>>> "s2idle" is a Linux term; I'd prefer something that we can relate to
>>>> the ACPI spec.
>>> It's important for the symptoms of this issue though, this
>>> problem doesn't trigger "just" by moving D-states.
>>>
>>> It happens as a result of system suspend.
>> As I said in my response to Bjorn, s2idle is D0 from the ACPI
>> standpoint.  It is not a system sleep and it has no special meaning in
>> ACPI.
>>
>> The problem seems to be related to the low-power S0 idle _DSM calls 
>> to me.
>
> This particular hardware that triggered this patch can do S3
> or s2idle.
>
> Let me confirm with internal guys whether this can reproduce
> with BIOS configured to S3 as well.
I did confirm with internal team that this issue also reproduces on S3 and
this patch fixes S3 case as well.
>>>> Maybe a pointer to the specific function in the driver that has a
>>>> problem?  Based on the patch, I assume the driver uses some control
>>>> method that looks at PCI config space?
>>> The issue isn't in anything Linux code "does"; it's in the "lack"
>>> of Linux code doing what it needs to IE using _REG.
>> So the argument seems to be that under certain conditions the PCI
>> config space becomes unavailable and so _REG(dev, 0) needs to be
>> called when this is about to happen and _REG(dev, 1) needs to be
>> called when the config space becomes available again.  Fair enough,
>> but I'm not sure why this is limited to system suspend and resume.
> I didn't think it should be limited to suspend/resume
> either.
>
> That's why I had put it in save state/restore state.
>
>> Moreover, "PCI_Config operation regions on a PCI root bus containing a
>> _BBN object" are specifically mentioned as one of the cases when _REG
>> need not be evaluated at all.  I guess the operation region in
>> question doesn't fall into that category?
>
> Yes; that's right.  _BBN is only present on \_SB_.PCI0
> and the problematic device is on \_SB_.PCI0.GPP5.
>
>>> At least for this issue _REG is treated like a lock mechanism.
>>> In the spec it says specifically:
>>>
>>> "When an operation region handler is unavailable, AML cannot access
>>> data fields in that region".
>>>
>>> That is it's to ensure that OSPM and AML don't both simultaneously
>>> access the same region.
>>>
>>> What happens is that AML normally wants to access this region during
>>> suspend, but without the sequence of calling _REG it can't.
>> Is this about being unable to access the opregion or racing with
>> concurrent accesses on the OS side?
> Access.
>>
>>>>> To fix this issue, call acpi_evaluate_reg() when saving and 
>>>>> restoring the
>>>>> state of PCI devices.
>>>> Please include the spec citation: ACPI r6.5, sec 6.5.4.  The URL has
>>>> changed in the past and may change in the future, but the name/section
>>>> number will not.
>>> Sure.
>>>>> Link: 
>>>>> https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region
>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>> ---
>>>>>    drivers/pci/pci.c | 12 ++++++++++++
>>>>>    1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>>> index e38c2f6eebd4..071ecba548b0 100644
>>>>> --- a/drivers/pci/pci.c
>>>>> +++ b/drivers/pci/pci.c
>>>>> @@ -1068,6 +1068,12 @@ static inline bool 
>>>>> platform_pci_bridge_d3(struct pci_dev *dev)
>>>>>       return acpi_pci_bridge_d3(dev);
>>>>>    }
>>>>>
>>>>> +static inline int platform_toggle_reg(struct pci_dev *dev, int c)
>>>>> +{
>>>>> +    return acpi_evaluate_reg(ACPI_HANDLE(&dev->dev),
>>>>> +                             ACPI_ADR_SPACE_PCI_CONFIG, c);
>>>>> +}
>>>> You never check the return value, so why return it?
>>> _REG isn't mandatory for any of these uses, and I wanted to make
>>> sure that if it does end up being mandatory in a future use that
>>> the return code wasn't thrown away.  If you think it's better to
>>> just throw it away now, I have no qualms making it a void instead.
>> I don't think it can reasonably become mandatory without adding a
>> specific _OSC bit for that.
> OK.
>>
>>>> The function actually doesn't *toggle*; it connects or disconnects
>>>> based on "c".
>>> Can you suggest a better function name?
>>>> This looks like it only builds when CONFIG_ACPI=y?
>>> The prototype for acpi_evaluate_reg isn't guarded by CONFIG_ACPI
>>> so I figured it worked both ways.
>>>
>>> But looking again I don't see a dummy implementation version for
>>> the lack of CONFIG_ACPI, so I'll double check it.
>>>
>>>>>    /**
>>>>>     * pci_update_current_state - Read power state of given device 
>>>>> and cache it
>>>>>     * @dev: PCI device to handle.
>>>>> @@ -1645,6 +1651,9 @@ static void pci_restore_ltr_state(struct 
>>>>> pci_dev *dev)
>>>>>    int pci_save_state(struct pci_dev *dev)
>>>>>    {
>>>>>       int i;
>>>>> +
>>>>> +    platform_toggle_reg(dev, ACPI_REG_DISCONNECT);
>>>> I would expect these to be in the PM code near the power state
>>>> transitions, not in the state save/restore code.  These functions
>>>> *are* used during suspend/resume, but are used in other places as
>>>> well, where we probably don't want _REG executed.
>>>>
>>>> Cc'd Rafael and PM folks, who can give much better feedback.
>>> My knee jerk reaction when we found the root cause for this issue
>>> was to put the code right around the D-state transitions, but I
>>> decided against this.
>>>
>>> I put it in save/restore intentionally because
>>> like I mentioned above it's treated like a locking mechanism between
>>> OSPM and AML and it's not functionally tied to a D-state transition.
>>>
>>> When the state is saved it's like Linux says
>>> "I'm done using this region, go ahead and touch it firmware".
>>> When it's restored it's like Linux says
>>> "Don't use that region, I'm claiming it for now".
>> So it looks like you want to use _REG for protecting PCI config space
>> against concurrent accesses from AML and the OS.
> Yeah.  When I discussed it with BIOS guys they
> explained to me that the BIOS will typically save/restore the
> PCIe device BAR when _REG is called (depending on the argument
> to _REG).
>
> They'll only operate on the region when it's in the right
> state, and they'll restore it as necessary when OSPM would use
> it again.
>
> This is also how Windows works.
>
>>> Think about that other patch I wrote recently that controls D3
>>> availability [1].  If it was only run in the D-state transitions and
>>> the root port stays in D0 but has a _REG method it would never get
>>> called.
>> And why should it be evaluated in that case?
> No matter what the actual D-state is the OSPM isn't
> accessing it anymore, so AML should be able to.

Re: [PATCH] PCI: Call _REG when saving/restoring PCI state

[Rafael J. Wysocki]

On Fri, Jun 2, 2023 at 10:21 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Rafael, Len, linux-acpi]
>
> Hi Mario,
>
> On Thu, Jun 01, 2023 at 10:11:22PM -0500, Mario Limonciello wrote:
> > ASMedia PCIe GPIO controllers connected to AMD SOC fail functional tests
> > after returning from s2idle. This is because the BIOS checks whether the
> > OSPM has called the _REG method to determine whether it can interact with
> > the OperationRegion assigned to the device.
>
> "s2idle" is a Linux term; I'd prefer something that we can relate to
> the ACPI spec.

From the ACPI spec viewpoint, s2idle is S0, so this means that _REG
needs to be called under specific conditions when the system is in S0.
I'm not sure if this is really ACPI-compliant.

> Maybe a pointer to the specific function in the driver that has a
> problem?  Based on the patch, I assume the driver uses some control
> method that looks at PCI config space?
>
> > To fix this issue, call acpi_evaluate_reg() when saving and restoring the
> > state of PCI devices.
>
> Please include the spec citation: ACPI r6.5, sec 6.5.4.  The URL has
> changed in the past and may change in the future,

That's true, but it has been promised to us that these URLs will be
stable for the foreseeable future.

Besides, the spec version and section information is there in the link itself.

> but the name/section number will not.
>
> > Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> >  drivers/pci/pci.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index e38c2f6eebd4..071ecba548b0 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1068,6 +1068,12 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
> >       return acpi_pci_bridge_d3(dev);
> >  }
> >
> > +static inline int platform_toggle_reg(struct pci_dev *dev, int c)
> > +{
> > +     return acpi_evaluate_reg(ACPI_HANDLE(&dev->dev),
> > +                              ACPI_ADR_SPACE_PCI_CONFIG, c);
> > +}
>
> You never check the return value, so why return it?
>
> The function actually doesn't *toggle*; it connects or disconnects
> based on "c".
>
> This looks like it only builds when CONFIG_ACPI=y?
>
> >  /**
> >   * pci_update_current_state - Read power state of given device and cache it
> >   * @dev: PCI device to handle.
> > @@ -1645,6 +1651,9 @@ static void pci_restore_ltr_state(struct pci_dev *dev)
> >  int pci_save_state(struct pci_dev *dev)
> >  {
> >       int i;
> > +
> > +     platform_toggle_reg(dev, ACPI_REG_DISCONNECT);
>
> I would expect these to be in the PM code near the power state
> transitions, not in the state save/restore code.  These functions
> *are* used during suspend/resume, but are used in other places as
> well, where we probably don't want _REG executed.
>
> Cc'd Rafael and PM folks, who can give much better feedback.

Right, they are not PM-specific.

Also, it looks like this isn't really about system suspend, but about
the device (port?) going into D3(hot/cold?) and back into D0.

I'm not sure if the spec mandates the need to reevaluate _REG on
device power state transitions.

At least, it looks like the operation region becomes invalid when the
device goes into D3 and there is no provision in the spec I can recall
ATM to notify AML about that.

IOW, once _REG has been called, the operation region needs to stay
valid from the perspective of AML using it which I think is not the
case on the affected platform.

> >       /* XXX: 100% dword access ok here? */
> >       for (i = 0; i < 16; i++) {
> >               pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]);
> > @@ -1790,6 +1799,8 @@ void pci_restore_state(struct pci_dev *dev)
> >       pci_enable_acs(dev);
> >       pci_restore_iov_state(dev);
> >
> > +     platform_toggle_reg(dev, ACPI_REG_CONNECT);
> > +
> >       dev->state_saved = false;
> >  }
> >  EXPORT_SYMBOL(pci_restore_state);
> > @@ -3203,6 +3214,7 @@ void pci_pm_init(struct pci_dev *dev)
> >       pci_read_config_word(dev, PCI_STATUS, &status);
> >       if (status & PCI_STATUS_IMM_READY)
> >               dev->imm_ready = 1;
> > +     platform_toggle_reg(dev, ACPI_REG_CONNECT);
> >  }
> >
> >  static unsigned long pci_ea_flags(struct pci_dev *dev, u8 prop)
> > --