[NEXT,6/6] PCI/PORTDRV: Remove ERR_FATAL handling from pcie_portdrv_slot_reset()

Message ID 1528351234-26914-6-git-send-email-poza@codeaurora.org
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series
  • [NEXT,1/6] PCI/AER: Take mask into account while clearing error bits
Related show

Commit Message

Oza Pawandeep June 7, 2018, 6 a.m.
We are handling ERR_FATAL by resetting the Link in software,skipping the
driver pci_error_handlers callbacks, removing the devices from the PCI
subsystem, and re-enumerating, as a result of that, no more calling
pcie_portdrv_slot_reset in ERR_FATAL case.

Signed-off-by: Oza Pawandeep <poza@codeaurora.org>

Comments

Oza Pawandeep June 7, 2018, 1:48 p.m. | #1
On 2018-06-07 11:30, Oza Pawandeep wrote:
> We are handling ERR_FATAL by resetting the Link in software,skipping 
> the
> driver pci_error_handlers callbacks, removing the devices from the PCI
> subsystem, and re-enumerating, as a result of that, no more calling
> pcie_portdrv_slot_reset in ERR_FATAL case.
> 
> Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
> 
> diff --git a/drivers/pci/pcie/portdrv_pci.c 
> b/drivers/pci/pcie/portdrv_pci.c
> index 973f1b8..92f5d330 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -42,17 +42,6 @@ __setup("pcie_ports=", pcie_port_setup);
> 
>  /* global data */
> 
> -static int pcie_portdrv_restore_config(struct pci_dev *dev)
> -{
> -	int retval;
> -
> -	retval = pci_enable_device(dev);
> -	if (retval)
> -		return retval;
> -	pci_set_master(dev);
> -	return 0;
> -}
> -
>  #ifdef CONFIG_PM
>  static int pcie_port_runtime_suspend(struct device *dev)
>  {
> @@ -162,14 +151,6 @@ static pci_ers_result_t
> pcie_portdrv_mmio_enabled(struct pci_dev *dev)
> 
>  static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev)
>  {
> -	/* If fatal, restore cfg space for possible link reset at upstream */
> -	if (dev->error_state == pci_channel_io_frozen) {
> -		dev->state_saved = true;
> -		pci_restore_state(dev);
> -		pcie_portdrv_restore_config(dev);
> -		pci_enable_pcie_error_reporting(dev);
> -	}
> -
>  	return PCI_ERS_RESULT_RECOVERED;
>  }


Hi Bjorn,

the above patch removes ERR_FATAL handling from 
pcie_portdrv_slot_reset()
because now we are handling ERR_FATAL differently than before.

I tried to dig into pcie_portdrv_slot_reset() handling for ERR_FATAL 
case where it
restores the config space, enable device, set master and enable error 
reporting....
and as far as I understand this is being done for upstream link (bridges 
etc..)

why was it done at the first point (I checked the commit description, 
but could not really get it)
and do we need to handle the same thing in ERR_FATAL now ?

Regards,
Oza.
Bjorn Helgaas June 7, 2018, 9:34 p.m. | #2
On Thu, Jun 07, 2018 at 07:18:03PM +0530, poza@codeaurora.org wrote:
> On 2018-06-07 11:30, Oza Pawandeep wrote:
> > We are handling ERR_FATAL by resetting the Link in software,skipping the
> > driver pci_error_handlers callbacks, removing the devices from the PCI
> > subsystem, and re-enumerating, as a result of that, no more calling
> > pcie_portdrv_slot_reset in ERR_FATAL case.
> > 
> > Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
> > 
> > diff --git a/drivers/pci/pcie/portdrv_pci.c
> > b/drivers/pci/pcie/portdrv_pci.c
> > index 973f1b8..92f5d330 100644
> > --- a/drivers/pci/pcie/portdrv_pci.c
> > +++ b/drivers/pci/pcie/portdrv_pci.c
> > @@ -42,17 +42,6 @@ __setup("pcie_ports=", pcie_port_setup);
> > 
> >  /* global data */
> > 
> > -static int pcie_portdrv_restore_config(struct pci_dev *dev)
> > -{
> > -	int retval;
> > -
> > -	retval = pci_enable_device(dev);
> > -	if (retval)
> > -		return retval;
> > -	pci_set_master(dev);
> > -	return 0;
> > -}
> > -
> >  #ifdef CONFIG_PM
> >  static int pcie_port_runtime_suspend(struct device *dev)
> >  {
> > @@ -162,14 +151,6 @@ static pci_ers_result_t
> > pcie_portdrv_mmio_enabled(struct pci_dev *dev)
> > 
> >  static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev)
> >  {
> > -	/* If fatal, restore cfg space for possible link reset at upstream */
> > -	if (dev->error_state == pci_channel_io_frozen) {
> > -		dev->state_saved = true;
> > -		pci_restore_state(dev);
> > -		pcie_portdrv_restore_config(dev);
> > -		pci_enable_pcie_error_reporting(dev);
> > -	}
> > -
> >  	return PCI_ERS_RESULT_RECOVERED;
> >  }
> 
> 
> Hi Bjorn,
> 
> the above patch removes ERR_FATAL handling from pcie_portdrv_slot_reset()
> because now we are handling ERR_FATAL differently than before.
> 
> I tried to dig into pcie_portdrv_slot_reset() handling for ERR_FATAL case
> where it
> restores the config space, enable device, set master and enable error
> reporting....
> and as far as I understand this is being done for upstream link (bridges
> etc..)
> 
> why was it done at the first point (I checked the commit description, but
> could not really get it)
> and do we need to handle the same thing in ERR_FATAL now ?

You mean 4bf3392e0bf5 ("PCI-Express AER implemetation: pcie_portdrv
error handler"), which added pcie_portdrv_slot_reset()?  I agree, that
commit log has no useful information.  I don't know any of the history
behind it.
Oza Pawandeep June 8, 2018, 4:47 a.m. | #3
On 2018-06-08 03:04, Bjorn Helgaas wrote:
> On Thu, Jun 07, 2018 at 07:18:03PM +0530, poza@codeaurora.org wrote:
>> On 2018-06-07 11:30, Oza Pawandeep wrote:
>> > We are handling ERR_FATAL by resetting the Link in software,skipping the
>> > driver pci_error_handlers callbacks, removing the devices from the PCI
>> > subsystem, and re-enumerating, as a result of that, no more calling
>> > pcie_portdrv_slot_reset in ERR_FATAL case.
>> >
>> > Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
>> >
>> > diff --git a/drivers/pci/pcie/portdrv_pci.c
>> > b/drivers/pci/pcie/portdrv_pci.c
>> > index 973f1b8..92f5d330 100644
>> > --- a/drivers/pci/pcie/portdrv_pci.c
>> > +++ b/drivers/pci/pcie/portdrv_pci.c
>> > @@ -42,17 +42,6 @@ __setup("pcie_ports=", pcie_port_setup);
>> >
>> >  /* global data */
>> >
>> > -static int pcie_portdrv_restore_config(struct pci_dev *dev)
>> > -{
>> > -	int retval;
>> > -
>> > -	retval = pci_enable_device(dev);
>> > -	if (retval)
>> > -		return retval;
>> > -	pci_set_master(dev);
>> > -	return 0;
>> > -}
>> > -
>> >  #ifdef CONFIG_PM
>> >  static int pcie_port_runtime_suspend(struct device *dev)
>> >  {
>> > @@ -162,14 +151,6 @@ static pci_ers_result_t
>> > pcie_portdrv_mmio_enabled(struct pci_dev *dev)
>> >
>> >  static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev)
>> >  {
>> > -	/* If fatal, restore cfg space for possible link reset at upstream */
>> > -	if (dev->error_state == pci_channel_io_frozen) {
>> > -		dev->state_saved = true;
>> > -		pci_restore_state(dev);
>> > -		pcie_portdrv_restore_config(dev);
>> > -		pci_enable_pcie_error_reporting(dev);
>> > -	}
>> > -
>> >  	return PCI_ERS_RESULT_RECOVERED;
>> >  }
>> 
>> 
>> Hi Bjorn,
>> 
>> the above patch removes ERR_FATAL handling from 
>> pcie_portdrv_slot_reset()
>> because now we are handling ERR_FATAL differently than before.
>> 
>> I tried to dig into pcie_portdrv_slot_reset() handling for ERR_FATAL 
>> case
>> where it
>> restores the config space, enable device, set master and enable error
>> reporting....
>> and as far as I understand this is being done for upstream link 
>> (bridges
>> etc..)
>> 
>> why was it done at the first point (I checked the commit description, 
>> but
>> could not really get it)
>> and do we need to handle the same thing in ERR_FATAL now ?
> 
> You mean 4bf3392e0bf5 ("PCI-Express AER implemetation: pcie_portdrv
> error handler"), which added pcie_portdrv_slot_reset()?  I agree, that
> commit log has no useful information.  I don't know any of the history
> behind it.

Keith,

do you know why in ERR_FATAL case following was done ?
have a look at pcie_portdrv_slot_reset() handling (for bridges, switches 
etc..)

Regards,
Oza.
Oza Pawandeep June 8, 2018, 4:57 a.m. | #4
On 2018-06-08 03:04, Bjorn Helgaas wrote:
> On Thu, Jun 07, 2018 at 07:18:03PM +0530, poza@codeaurora.org wrote:
>> On 2018-06-07 11:30, Oza Pawandeep wrote:
>> > We are handling ERR_FATAL by resetting the Link in software,skipping the
>> > driver pci_error_handlers callbacks, removing the devices from the PCI
>> > subsystem, and re-enumerating, as a result of that, no more calling
>> > pcie_portdrv_slot_reset in ERR_FATAL case.
>> >
>> > Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
>> >
>> > diff --git a/drivers/pci/pcie/portdrv_pci.c
>> > b/drivers/pci/pcie/portdrv_pci.c
>> > index 973f1b8..92f5d330 100644
>> > --- a/drivers/pci/pcie/portdrv_pci.c
>> > +++ b/drivers/pci/pcie/portdrv_pci.c
>> > @@ -42,17 +42,6 @@ __setup("pcie_ports=", pcie_port_setup);
>> >
>> >  /* global data */
>> >
>> > -static int pcie_portdrv_restore_config(struct pci_dev *dev)
>> > -{
>> > -	int retval;
>> > -
>> > -	retval = pci_enable_device(dev);
>> > -	if (retval)
>> > -		return retval;
>> > -	pci_set_master(dev);
>> > -	return 0;
>> > -}
>> > -
>> >  #ifdef CONFIG_PM
>> >  static int pcie_port_runtime_suspend(struct device *dev)
>> >  {
>> > @@ -162,14 +151,6 @@ static pci_ers_result_t
>> > pcie_portdrv_mmio_enabled(struct pci_dev *dev)
>> >
>> >  static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev)
>> >  {
>> > -	/* If fatal, restore cfg space for possible link reset at upstream */
>> > -	if (dev->error_state == pci_channel_io_frozen) {
>> > -		dev->state_saved = true;
>> > -		pci_restore_state(dev);
>> > -		pcie_portdrv_restore_config(dev);
>> > -		pci_enable_pcie_error_reporting(dev);
>> > -	}
>> > -
>> >  	return PCI_ERS_RESULT_RECOVERED;
>> >  }
>> 
>> 
>> Hi Bjorn,
>> 
>> the above patch removes ERR_FATAL handling from 
>> pcie_portdrv_slot_reset()
>> because now we are handling ERR_FATAL differently than before.
>> 
>> I tried to dig into pcie_portdrv_slot_reset() handling for ERR_FATAL 
>> case
>> where it
>> restores the config space, enable device, set master and enable error
>> reporting....
>> and as far as I understand this is being done for upstream link 
>> (bridges
>> etc..)
>> 
>> why was it done at the first point (I checked the commit description, 
>> but
>> could not really get it)
>> and do we need to handle the same thing in ERR_FATAL now ?
> 
> You mean 4bf3392e0bf5 ("PCI-Express AER implemetation: pcie_portdrv
> error handler"), which added pcie_portdrv_slot_reset()?  I agree, that
> commit log has no useful information.  I don't know any of the history
> behind it.


Yes Bjorn thats right.
I am trying to understand it but no clue.
since it is restoring the stuffs in ERR_FATAL case, why would PCIe 
bridge loose all the settings ?  [config space, aer bits, master, device 
enable etc..)
Max we do is link_reset in ERR_FATAL case, and Secondary bus reset 
should affect downstream components (not upstream)
Sinan Kaya June 8, 2018, 10:41 a.m. | #5
On 2018-06-08 00:57, poza@codeaurora.org wrote:
> On 2018-06-08 03:04, Bjorn Helgaas wrote:
>> On Thu, Jun 07, 2018 at 07:18:03PM +0530, poza@codeaurora.org wrote:
>>> On 2018-06-07 11:30, Oza Pawandeep wrote:
>>> > We are handling ERR_FATAL by resetting the Link in software,skipping the
>>> > driver pci_error_handlers callbacks, removing the devices from the PCI
>>> > subsystem, and re-enumerating, as a result of that, no more calling
>>> > pcie_portdrv_slot_reset in ERR_FATAL case.
>>> >
>>> > Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
>>> >
>>> > diff --git a/drivers/pci/pcie/portdrv_pci.c
>>> > b/drivers/pci/pcie/portdrv_pci.c
>>> > index 973f1b8..92f5d330 100644
>>> > --- a/drivers/pci/pcie/portdrv_pci.c
>>> > +++ b/drivers/pci/pcie/portdrv_pci.c
>>> > @@ -42,17 +42,6 @@ __setup("pcie_ports=", pcie_port_setup);
>>> >
>>> >  /* global data */
>>> >
>>> > -static int pcie_portdrv_restore_config(struct pci_dev *dev)
>>> > -{
>>> > -	int retval;
>>> > -
>>> > -	retval = pci_enable_device(dev);
>>> > -	if (retval)
>>> > -		return retval;
>>> > -	pci_set_master(dev);
>>> > -	return 0;
>>> > -}
>>> > -
>>> >  #ifdef CONFIG_PM
>>> >  static int pcie_port_runtime_suspend(struct device *dev)
>>> >  {
>>> > @@ -162,14 +151,6 @@ static pci_ers_result_t
>>> > pcie_portdrv_mmio_enabled(struct pci_dev *dev)
>>> >
>>> >  static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev)
>>> >  {
>>> > -	/* If fatal, restore cfg space for possible link reset at upstream */
>>> > -	if (dev->error_state == pci_channel_io_frozen) {
>>> > -		dev->state_saved = true;
>>> > -		pci_restore_state(dev);
>>> > -		pcie_portdrv_restore_config(dev);
>>> > -		pci_enable_pcie_error_reporting(dev);
>>> > -	}
>>> > -
>>> >  	return PCI_ERS_RESULT_RECOVERED;
>>> >  }
>>> 
>>> 
>>> Hi Bjorn,
>>> 
>>> the above patch removes ERR_FATAL handling from 
>>> pcie_portdrv_slot_reset()
>>> because now we are handling ERR_FATAL differently than before.
>>> 
>>> I tried to dig into pcie_portdrv_slot_reset() handling for ERR_FATAL 
>>> case
>>> where it
>>> restores the config space, enable device, set master and enable error
>>> reporting....
>>> and as far as I understand this is being done for upstream link 
>>> (bridges
>>> etc..)
>>> 
>>> why was it done at the first point (I checked the commit description, 
>>> but
>>> could not really get it)
>>> and do we need to handle the same thing in ERR_FATAL now ?
>> 
>> You mean 4bf3392e0bf5 ("PCI-Express AER implemetation: pcie_portdrv
>> error handler"), which added pcie_portdrv_slot_reset()?  I agree, that
>> commit log has no useful information.  I don't know any of the history
>> behind it.
> 
> 
> Yes Bjorn thats right.
> I am trying to understand it but no clue.
> since it is restoring the stuffs in ERR_FATAL case, why would PCIe
> bridge loose all the settings ?  [config space, aer bits, master,
> device enable etc..)
> Max we do is link_reset in ERR_FATAL case, and Secondary bus reset
> should affect downstream components (not upstream)

Our first generation controller had this problem. There could be others 
too.
Keith Busch June 8, 2018, 10:43 p.m. | #6
On Thu, Jun 07, 2018 at 09:47:42PM -0700, poza@codeaurora.org wrote:
> Keith,
> 
> do you know why in ERR_FATAL case following was done ?
> have a look at pcie_portdrv_slot_reset() handling (for bridges, switches 
> etc..)

Not sure, but I was looking into some issues in this area anyway.

I'm finding that non-hotpluggable bridges that support D3 are getting
put into that low-power mode, and that pretty much breaks the
re-enumeration.
Oza Pawandeep June 11, 2018, 10:01 a.m. | #7
On 2018-06-08 03:04, Bjorn Helgaas wrote:
> On Thu, Jun 07, 2018 at 07:18:03PM +0530, poza@codeaurora.org wrote:
>> On 2018-06-07 11:30, Oza Pawandeep wrote:
>> > We are handling ERR_FATAL by resetting the Link in software,skipping the
>> > driver pci_error_handlers callbacks, removing the devices from the PCI
>> > subsystem, and re-enumerating, as a result of that, no more calling
>> > pcie_portdrv_slot_reset in ERR_FATAL case.
>> >
>> > Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
>> >
>> > diff --git a/drivers/pci/pcie/portdrv_pci.c
>> > b/drivers/pci/pcie/portdrv_pci.c
>> > index 973f1b8..92f5d330 100644
>> > --- a/drivers/pci/pcie/portdrv_pci.c
>> > +++ b/drivers/pci/pcie/portdrv_pci.c
>> > @@ -42,17 +42,6 @@ __setup("pcie_ports=", pcie_port_setup);
>> >
>> >  /* global data */
>> >
>> > -static int pcie_portdrv_restore_config(struct pci_dev *dev)
>> > -{
>> > -	int retval;
>> > -
>> > -	retval = pci_enable_device(dev);
>> > -	if (retval)
>> > -		return retval;
>> > -	pci_set_master(dev);
>> > -	return 0;
>> > -}
>> > -
>> >  #ifdef CONFIG_PM
>> >  static int pcie_port_runtime_suspend(struct device *dev)
>> >  {
>> > @@ -162,14 +151,6 @@ static pci_ers_result_t
>> > pcie_portdrv_mmio_enabled(struct pci_dev *dev)
>> >
>> >  static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev)
>> >  {
>> > -	/* If fatal, restore cfg space for possible link reset at upstream */
>> > -	if (dev->error_state == pci_channel_io_frozen) {
>> > -		dev->state_saved = true;
>> > -		pci_restore_state(dev);
>> > -		pcie_portdrv_restore_config(dev);
>> > -		pci_enable_pcie_error_reporting(dev);
>> > -	}
>> > -
>> >  	return PCI_ERS_RESULT_RECOVERED;
>> >  }
>> 
>> 
>> Hi Bjorn,
>> 
>> the above patch removes ERR_FATAL handling from 
>> pcie_portdrv_slot_reset()
>> because now we are handling ERR_FATAL differently than before.
>> 
>> I tried to dig into pcie_portdrv_slot_reset() handling for ERR_FATAL 
>> case
>> where it
>> restores the config space, enable device, set master and enable error
>> reporting....
>> and as far as I understand this is being done for upstream link 
>> (bridges
>> etc..)
>> 
>> why was it done at the first point (I checked the commit description, 
>> but
>> could not really get it)
>> and do we need to handle the same thing in ERR_FATAL now ?
> 
> You mean 4bf3392e0bf5 ("PCI-Express AER implemetation: pcie_portdrv
> error handler"), which added pcie_portdrv_slot_reset()?  I agree, that
> commit log has no useful information.  I don't know any of the history
> behind it.

Hi Bjorn and Keith,

broadcast_error_message()
if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
         .....
         pci_walk_bus(dev->subordinate, cb, &result_data);


so in case of ERR_FATAL, the walk bus is happening on subordinates, and 
if I understand the walk right
then, pcie_portdrv_slot_reset() is called only on BRIDGES/Switches

If is never called on Root-Ports

having said that, now since we are removing the devices (compare to 
previous error callback handling in ERR_FATAL)
I dont see the need of the above code anymore.

because there is nothing to restore to any more. as we are initiating 
re-enumeration.

Regards,
Oza.
Oza Pawandeep June 11, 2018, 12:50 p.m. | #8
On 2018-06-11 15:31, poza@codeaurora.org wrote:
> On 2018-06-08 03:04, Bjorn Helgaas wrote:
>> On Thu, Jun 07, 2018 at 07:18:03PM +0530, poza@codeaurora.org wrote:
>>> On 2018-06-07 11:30, Oza Pawandeep wrote:
>>> > We are handling ERR_FATAL by resetting the Link in software,skipping the
>>> > driver pci_error_handlers callbacks, removing the devices from the PCI
>>> > subsystem, and re-enumerating, as a result of that, no more calling
>>> > pcie_portdrv_slot_reset in ERR_FATAL case.
>>> >
>>> > Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
>>> >
>>> > diff --git a/drivers/pci/pcie/portdrv_pci.c
>>> > b/drivers/pci/pcie/portdrv_pci.c
>>> > index 973f1b8..92f5d330 100644
>>> > --- a/drivers/pci/pcie/portdrv_pci.c
>>> > +++ b/drivers/pci/pcie/portdrv_pci.c
>>> > @@ -42,17 +42,6 @@ __setup("pcie_ports=", pcie_port_setup);
>>> >
>>> >  /* global data */
>>> >
>>> > -static int pcie_portdrv_restore_config(struct pci_dev *dev)
>>> > -{
>>> > -	int retval;
>>> > -
>>> > -	retval = pci_enable_device(dev);
>>> > -	if (retval)
>>> > -		return retval;
>>> > -	pci_set_master(dev);
>>> > -	return 0;
>>> > -}
>>> > -
>>> >  #ifdef CONFIG_PM
>>> >  static int pcie_port_runtime_suspend(struct device *dev)
>>> >  {
>>> > @@ -162,14 +151,6 @@ static pci_ers_result_t
>>> > pcie_portdrv_mmio_enabled(struct pci_dev *dev)
>>> >
>>> >  static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev)
>>> >  {
>>> > -	/* If fatal, restore cfg space for possible link reset at upstream */
>>> > -	if (dev->error_state == pci_channel_io_frozen) {
>>> > -		dev->state_saved = true;
>>> > -		pci_restore_state(dev);
>>> > -		pcie_portdrv_restore_config(dev);
>>> > -		pci_enable_pcie_error_reporting(dev);
>>> > -	}
>>> > -
>>> >  	return PCI_ERS_RESULT_RECOVERED;
>>> >  }
>>> 
>>> 
>>> Hi Bjorn,
>>> 
>>> the above patch removes ERR_FATAL handling from 
>>> pcie_portdrv_slot_reset()
>>> because now we are handling ERR_FATAL differently than before.
>>> 
>>> I tried to dig into pcie_portdrv_slot_reset() handling for ERR_FATAL 
>>> case
>>> where it
>>> restores the config space, enable device, set master and enable error
>>> reporting....
>>> and as far as I understand this is being done for upstream link 
>>> (bridges
>>> etc..)
>>> 
>>> why was it done at the first point (I checked the commit description, 
>>> but
>>> could not really get it)
>>> and do we need to handle the same thing in ERR_FATAL now ?
>> 
>> You mean 4bf3392e0bf5 ("PCI-Express AER implemetation: pcie_portdrv
>> error handler"), which added pcie_portdrv_slot_reset()?  I agree, that
>> commit log has no useful information.  I don't know any of the history
>> behind it.
> 
> Hi Bjorn and Keith,
> 
> broadcast_error_message()
> if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>         .....
>         pci_walk_bus(dev->subordinate, cb, &result_data);
> 
> 
> so in case of ERR_FATAL, the walk bus is happening on subordinates,
> and if I understand the walk right
> then, pcie_portdrv_slot_reset() is called only on BRIDGES/Switches
> 
> If is never called on Root-Ports
> 
> having said that, now since we are removing the devices (compare to
> previous error callback handling in ERR_FATAL)
> I dont see the need of the above code anymore.
> 

when I say above code, I meant this patch itself which removes ERR_FATAL 
handling out of pcie_portdrv_slot_reset

> because there is nothing to restore to any more. as we are initiating
> re-enumeration.
> 
> Regards,
> Oza.

Patch

diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
index 973f1b8..92f5d330 100644
--- a/drivers/pci/pcie/portdrv_pci.c
+++ b/drivers/pci/pcie/portdrv_pci.c
@@ -42,17 +42,6 @@  __setup("pcie_ports=", pcie_port_setup);
 
 /* global data */
 
-static int pcie_portdrv_restore_config(struct pci_dev *dev)
-{
-	int retval;
-
-	retval = pci_enable_device(dev);
-	if (retval)
-		return retval;
-	pci_set_master(dev);
-	return 0;
-}
-
 #ifdef CONFIG_PM
 static int pcie_port_runtime_suspend(struct device *dev)
 {
@@ -162,14 +151,6 @@  static pci_ers_result_t pcie_portdrv_mmio_enabled(struct pci_dev *dev)
 
 static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev)
 {
-	/* If fatal, restore cfg space for possible link reset at upstream */
-	if (dev->error_state == pci_channel_io_frozen) {
-		dev->state_saved = true;
-		pci_restore_state(dev);
-		pcie_portdrv_restore_config(dev);
-		pci_enable_pcie_error_reporting(dev);
-	}
-
 	return PCI_ERS_RESULT_RECOVERED;
 }