Re: [PATCH] PCI: Exit restore process when device is still powerdown

Re: [PATCH] PCI: Exit restore process when device is still powerdown

[jiantao zhang]

在 2023/1/13 6:13, Bjorn Helgaas 写道:
> On Thu, Dec 22, 2022 at 12:41:04PM +0000, Jiantao Zhang wrote:
>> We get this stack when the rp doesn't power up in resume noirq:
> 
> s/rp/Root Port/
> 
> "resume noirq" seems to refer to a function, so please mention the
> exact function name.
> 
>>      dump_backtrace.cfi_jt+0x0/0x4
>>      dump_stack_lvl+0xb4/0x10c
>>      show_regs_before_dump_stack+0x1c/0x30
>>      arm64_serror_panic+0x110/0x1a8
>>      do_serror+0x16c/0x1cc
>>      el1_error+0x8c/0x10c
>>      do_raw_spin_unlock+0x74/0xdc
>>      pci_bus_read_config_word+0xdc/0x1dc
>>      pci_restore_msi_state+0x2f4/0x36c
>>      pci_restore_state+0x13f0/0x1444
>>      pci_pm_resume_noirq+0x158/0x318
>>      dpm_run_callback+0x178/0x5e8
>>      device_resume_noirq+0x250/0x264
>>      async_resume_noirq+0x20/0xf8
>>      async_run_entry_fn+0xfc/0x364
>>      process_one_work+0x37c/0x7f4
>>      worker_thread+0x3e8/0x754
>>      kthread+0x168/0x204
>>      ret_from_fork+0x10/0x18
>> The ep device uses msix, the restore process will write bar space
>> in __pci_msix_desc_mask_irq, which will result in accessing the
>> powerdown area when the rp doesn't power on.
> 
> s/ep/endpoint/
> s/msix/MSI-X/ to match spec usage
> s/bar/BAR/
> Add "()" after function names, e.g., __pci_msix_desc_mask_irq()
> s/rp/Root Port/
> 
>> It makes sense we should do nothing when the device is still powerdown.
>>
>> Signed-off-by: Jianrong Zhang <zhangjianrong5@huawei.com>
>> Signed-off-by: Jiantao Zhang <water.zhangjiantao@huawei.com>
>> ---
>>   drivers/pci/pci.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index fba95486caaf..279f6e8c5a00 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -1764,7 +1764,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
>>    */
>>   void pci_restore_state(struct pci_dev *dev)
>>   {
>> -	if (!dev->state_saved)
>> +	if (!dev->state_saved || dev->current_state == PCI_UNKNOWN)
>>   		return;
> 
> This doesn't seem right to me because it seems like we're covering up
> a problem elsewhere.
> 
> If we need access to the endpoint to restore state, shouldn't we
> ensure that the endpoint is powered up before we try to access it?
> 
> We depend on the state being restored, so if we skip the restore here,
> where *will* it happen?
As the call stack shows the serror happens in pci_pm_resume_noirq(),
which belongs to pci pm framework. The resume process related to pci
devices goes like this:

stage noirq:
Root Port's call stack: device_resume_noirq() --> pci_pm_resume_noirq() 
--> resume_noirq callback
endpoint's call stack: device_resume_noirq() --> pci_pm_resume_noirq() 
--> resume_noirq callback

stage early:
Root Port's call stack: device_resume_early() --> pci_pm_resume_early() 
--> device resume_early callback
endpoint's call stack: device_resume_early() --> pci_pm_resume_early() 
--> device resume_early callback

stage normal:
Root Port's call stack: device_resume() --> pci_pm_resume() --> device 
resume callback
endpoint's call stack: device_resume() --> pci_pm_resume() --> device 
resume callback

The problem is we don't power up the controller in Root Port's 
resume_noirq callback
(actually we don't even register resume_noirq callback for some reason),
so the serror happens because of accessing powerdown area when 
endpoint's pci_pm_resume_noirq()
calls pci_restore_state() which will call pci_restore_msi_state() to 
restore MSI-X state.
So we wonder if there is strong restriction that we must poweron in Root 
Port's resume_noirq callback.
The pci_restore_state() can't restore anything when the device is still 
at PCI_UNKNOWN state,
and if the device is accessible it can't be at PCI_UNKNOWN state, so the 
patch doesn't make any difference
for original process.
> 
> Bjorn
> .

Re: [PATCH] PCI: Exit restore process when device is still powerdown

[Bjorn Helgaas]

On Fri, Jan 20, 2023 at 02:18:24PM +0800, jiantao zhang wrote:
> 在 2023/1/13 6:13, Bjorn Helgaas 写道:
> > On Thu, Dec 22, 2022 at 12:41:04PM +0000, Jiantao Zhang wrote:
> > > We get this stack when the rp doesn't power up in resume noirq:
> > >      dump_backtrace.cfi_jt+0x0/0x4
> > >      dump_stack_lvl+0xb4/0x10c
> > >      show_regs_before_dump_stack+0x1c/0x30
> > >      arm64_serror_panic+0x110/0x1a8
> > >      do_serror+0x16c/0x1cc
> > >      el1_error+0x8c/0x10c
> > >      do_raw_spin_unlock+0x74/0xdc
> > >      pci_bus_read_config_word+0xdc/0x1dc
> > >      pci_restore_msi_state+0x2f4/0x36c
> > >      pci_restore_state+0x13f0/0x1444
> > >      pci_pm_resume_noirq+0x158/0x318
> > >      dpm_run_callback+0x178/0x5e8
> > >      device_resume_noirq+0x250/0x264
> > >      async_resume_noirq+0x20/0xf8
> > >      async_run_entry_fn+0xfc/0x364
> > >      process_one_work+0x37c/0x7f4
> > >      worker_thread+0x3e8/0x754
> > >      kthread+0x168/0x204
> > >      ret_from_fork+0x10/0x18
> > > The ep device uses msix, the restore process will write bar space
> > > in __pci_msix_desc_mask_irq, which will result in accessing the
> > > powerdown area when the rp doesn't power on.
> > 
> > > It makes sense we should do nothing when the device is still powerdown.
> > > 
> > > Signed-off-by: Jianrong Zhang <zhangjianrong5@huawei.com>
> > > Signed-off-by: Jiantao Zhang <water.zhangjiantao@huawei.com>
> > > ---
> > >   drivers/pci/pci.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index fba95486caaf..279f6e8c5a00 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -1764,7 +1764,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
> > >    */
> > >   void pci_restore_state(struct pci_dev *dev)
> > >   {
> > > -	if (!dev->state_saved)
> > > +	if (!dev->state_saved || dev->current_state == PCI_UNKNOWN)
> > >   		return;
> > 
> > This doesn't seem right to me because it seems like we're covering up
> > a problem elsewhere.
> > 
> > If we need access to the endpoint to restore state, shouldn't we
> > ensure that the endpoint is powered up before we try to access it?
> > 
> > We depend on the state being restored, so if we skip the restore here,
> > where *will* it happen?
>
> As the call stack shows the serror happens in pci_pm_resume_noirq(),
> which belongs to pci pm framework. The resume process related to pci
> devices goes like this:
> 
> stage noirq:
> Root Port's call stack: device_resume_noirq() --> pci_pm_resume_noirq() -->
> resume_noirq callback
> endpoint's call stack: device_resume_noirq() --> pci_pm_resume_noirq() -->
> resume_noirq callback
> 
> stage early:
> Root Port's call stack: device_resume_early() --> pci_pm_resume_early() -->
> device resume_early callback
> endpoint's call stack: device_resume_early() --> pci_pm_resume_early() -->
> device resume_early callback
> 
> stage normal:
> Root Port's call stack: device_resume() --> pci_pm_resume() --> device
> resume callback
> endpoint's call stack: device_resume() --> pci_pm_resume() --> device resume
> callback
> 
> The problem is we don't power up the controller in Root Port's
> resume_noirq callback (actually we don't even register resume_noirq
> callback for some reason), so the serror happens because of
> accessing powerdown area when endpoint's pci_pm_resume_noirq() calls
> pci_restore_state() which will call pci_restore_msi_state() to
> restore MSI-X state.

Yes.  So when is the MSI-X state restored?

If I understand correctly, your patch just skips pci_restore_state()
when the device power state is PCI_UNKNOWN.  But I assume we still
need to restore the MSI-X and other state *somewhere*.

Your patch changes the generic code that *all* platforms use, but I
have not heard of this problem on other hardware, so I assume there's
something different about power management on your platform.  That
makes me think the solution would be a platform-specific change, not
this generic code change.

> So we wonder if there is strong restriction that we must poweron in
> Root Port's resume_noirq callback.  The pci_restore_state() can't
> restore anything when the device is still at PCI_UNKNOWN state, and
> if the device is accessible it can't be at PCI_UNKNOWN state, so the
> patch doesn't make any difference for original process.

[PATCH] PCI: Exit restore process when device is still powerdown

[Jiantao Zhang]

We get this stack when the rp doesn't power up in resume noirq:
    dump_backtrace.cfi_jt+0x0/0x4
    dump_stack_lvl+0xb4/0x10c
    show_regs_before_dump_stack+0x1c/0x30
    arm64_serror_panic+0x110/0x1a8
    do_serror+0x16c/0x1cc
    el1_error+0x8c/0x10c
    do_raw_spin_unlock+0x74/0xdc
    pci_bus_read_config_word+0xdc/0x1dc
    pci_restore_msi_state+0x2f4/0x36c
    pci_restore_state+0x13f0/0x1444
    pci_pm_resume_noirq+0x158/0x318
    dpm_run_callback+0x178/0x5e8
    device_resume_noirq+0x250/0x264
    async_resume_noirq+0x20/0xf8
    async_run_entry_fn+0xfc/0x364
    process_one_work+0x37c/0x7f4
    worker_thread+0x3e8/0x754
    kthread+0x168/0x204
    ret_from_fork+0x10/0x18
The ep device uses msix, the restore process will write bar space
in __pci_msix_desc_mask_irq, which will result in accessing the
powerdown area when the rp doesn't power on.

It makes sense we should do nothing when the device is still powerdown.

Signed-off-by: Jianrong Zhang <zhangjianrong5@huawei.com>
Signed-off-by: Jiantao Zhang <water.zhangjiantao@huawei.com>
---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index fba95486caaf..279f6e8c5a00 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1764,7 +1764,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
  */
 void pci_restore_state(struct pci_dev *dev)
 {
-	if (!dev->state_saved)
+	if (!dev->state_saved || dev->current_state == PCI_UNKNOWN)
 		return;
 
 	/*
-- 
2.17.1

Re: [PATCH] PCI: Exit restore process when device is still powerdown

[Bjorn Helgaas]

On Thu, Dec 22, 2022 at 12:41:04PM +0000, Jiantao Zhang wrote:
> We get this stack when the rp doesn't power up in resume noirq:

s/rp/Root Port/

"resume noirq" seems to refer to a function, so please mention the
exact function name.

>     dump_backtrace.cfi_jt+0x0/0x4
>     dump_stack_lvl+0xb4/0x10c
>     show_regs_before_dump_stack+0x1c/0x30
>     arm64_serror_panic+0x110/0x1a8
>     do_serror+0x16c/0x1cc
>     el1_error+0x8c/0x10c
>     do_raw_spin_unlock+0x74/0xdc
>     pci_bus_read_config_word+0xdc/0x1dc
>     pci_restore_msi_state+0x2f4/0x36c
>     pci_restore_state+0x13f0/0x1444
>     pci_pm_resume_noirq+0x158/0x318
>     dpm_run_callback+0x178/0x5e8
>     device_resume_noirq+0x250/0x264
>     async_resume_noirq+0x20/0xf8
>     async_run_entry_fn+0xfc/0x364
>     process_one_work+0x37c/0x7f4
>     worker_thread+0x3e8/0x754
>     kthread+0x168/0x204
>     ret_from_fork+0x10/0x18
> The ep device uses msix, the restore process will write bar space
> in __pci_msix_desc_mask_irq, which will result in accessing the
> powerdown area when the rp doesn't power on.

s/ep/endpoint/
s/msix/MSI-X/ to match spec usage
s/bar/BAR/
Add "()" after function names, e.g., __pci_msix_desc_mask_irq()
s/rp/Root Port/

> It makes sense we should do nothing when the device is still powerdown.
> 
> Signed-off-by: Jianrong Zhang <zhangjianrong5@huawei.com>
> Signed-off-by: Jiantao Zhang <water.zhangjiantao@huawei.com>
> ---
>  drivers/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index fba95486caaf..279f6e8c5a00 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1764,7 +1764,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
>   */
>  void pci_restore_state(struct pci_dev *dev)
>  {
> -	if (!dev->state_saved)
> +	if (!dev->state_saved || dev->current_state == PCI_UNKNOWN)
>  		return;

This doesn't seem right to me because it seems like we're covering up
a problem elsewhere.

If we need access to the endpoint to restore state, shouldn't we
ensure that the endpoint is powered up before we try to access it?

We depend on the state being restored, so if we skip the restore here,
where *will* it happen?

Bjorn