Re: Subject : [ PATCH ] pci-reset-error_state-to-pci_channel_io_normal-at-report_slot_reset

[Bjorn Helgaas]

On Tue, May 21, 2013 at 1:40 AM, Yanmin Zhang
<yanmin_zhang@xxxxxxxxxxxxxxx> wrote:
> On Mon, 2013-05-20 at 16:48 -0600, Bjorn Helgaas wrote:
>> On Fri, Apr 26, 2013 at 06:28:59AM +0000, Zhang, LongX wrote:
>> > From: Zhang Long <longx.zhang@xxxxxxxxx>
>> >
>> > Specific pci device drivers might have many functions to call
>> > pci_channel_offline to check device states. When slot_reset happens,
>> > drivers' slot_reset callback might call such functions and eventually
>> > abort the reset.
>> >
>> > The patch resets pdev->error_state to pci_channel_io_normal at
>> > the begining of report_slot_reset.
>> >
>> > Thank Liu Joseph for pointing it out.
>> >
>> > Signed-off-by: Zhang Yanmin <yanmin_zhang@xxxxxxxxxxxxxxx>
>> > Signed-off-by: Zhang Long <longx.zhang@xxxxxxxxx>
>> > ---
>> >  drivers/pci/pcie/aer/aerdrv_core.c |    1 +
>> >  drivers/pci/pcie/portdrv_pci.c     |   12 +++++-------
>> >  2 files changed, 6 insertions(+), 7 deletions(-)
> Thank all for the kind comments.
>
>> >
>> > diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
>> > index 564d97f..c61fd44 100644
>> > --- a/drivers/pci/pcie/aer/aerdrv_core.c
>> > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
>> > @@ -286,6 +286,7 @@ static int report_slot_reset(struct pci_dev *dev, void *data)
>> >     result_data = (struct aer_broadcast_data *) data;
>> >
>> >     device_lock(&dev->dev);
>> > +   dev->error_state = pci_channel_io_normal;
>> >     if (!dev->driver ||
>> >             !dev->driver->err_handler ||
>> >             !dev->driver->err_handler->slot_reset)
>> > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
>> > index ed4d094..7abefd9 100644
>> > --- a/drivers/pci/pcie/portdrv_pci.c
>> > +++ b/drivers/pci/pcie/portdrv_pci.c
>> > @@ -332,13 +332,11 @@ static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev)
>> >     pci_ers_result_t status = PCI_ERS_RESULT_RECOVERED;
>> >     int retval;
>> >
>> > -   /* 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);
>> > -   }
>> > +   /* restore cfg space for possible link reset at upstream */
>> > +   dev->state_saved = true;
> It's indeed a dirty trick to change it to true. The reason is suspend. The port would
> suspend/resume at suspend-to-ram. When the port is suspended, PCI power framework calls
> pci_save_state. When the port is resumed, PCI framework calls pci_restore_state and
> dev->state_saved is set to false.

I can read the code as well as you can.  The above does nothing to
explain why dev->saved_config_space is still valid even when
state_saved is false.

But I want to drop this issue because it was there before your patch,
and we're clearly not going to resolve it here.

> A better solution is to add double space in pci_dev->saved_config_space/saved_cap_space,
> which seems consume unnecessary resource.
>
>> > +   pci_restore_state(dev);
>> > +   pcie_portdrv_restore_config(dev);
>> > +   pci_enable_pcie_error_reporting(dev);
>> >
>> >     /* get true return value from &status */
>> >     retval = device_for_each_child(&dev->dev, &status, slot_reset_iter);
>> I think this patch changes the behavior in the case of a non-fatal error
>> where one of the .error_detected() methods returned
>> PCI_ERS_RESULT_NEED_RESET.  In that case, pcie_portdrv_slot_reset()
>> previously did not restore config space, but after your patch, it *will*
>> restore it.  We need an explanation of why this is safe.
> AER standard doesn't define such behavior like if we need reset a slot.
> When we implemented the feature in kernel, we assumed at non-fatal error,
> config space is still good.
>
> With the new patch, we change the behavior a little.
>
>>
>> I think you should split this into two patches: the first would remove the
>> "if (dev->error_state == pci_channel_io_frozen)" test from portdrv_pci.c
> The first patch would depend on the 2nd patch as 2nd patch resets error_state
> to pci_channel_io_normal.

Not true.  The pcie_portdrv_slot_reset() change does not depend on the
report_slot_reset() change.  If the first patch makes
pcie_portdrv_slot_reset() do the restore without even looking at
dev->error_state, there's no dependency on the report_slot_reset()
change.

>> and explain the reason, and the second would make the aerdrv_core.c change.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/