Re: [PATCH] PCI: pciehp: Differentiate between surprise and safe removal

[gokul cg <gokuljnpr@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4619 bytes --]

Hi ,

>After the pci_dev is removed from the
>> hierarchy, accessing it seems at least questionable.

What about AER driver .  I was discussing the same in another mail chain
with subject  "Possible race condition in the kernel between PCI driver and
AER handling"

Regards
Gokul,

--------------------FYI---------------

I am suspecting a possible race condition in the kernel between PCI driver
and AER handling.

Because of the same kernel panic happens from worker thread which handles
bottom half of aer irq.


I am seeing this issue when I suddenly power off PCI card which
supports/enabled PCIE AER error reporting.

While powering off PCI device, AER driver will get AER IRQ for the device,
from AER IRQ handler, it will cache AER error code and schedule worker
thread to handle error.

The PCIe device will get removed from PCI tree before worker thread
completes its task and kernel panic is  happening when worker thread tries
to access PCI device's config space.


Issue:

crash>

crash> bt

PID: 2727   TASK: ffff880272adc530  CPU: 0   COMMAND: "kworker/0:2"

#0 [ffff88027469fac8] machine_kexec at ffffffff8102cf18

#1 [ffff88027469fb28] crash_kexec at ffffffff810a6b05

#2 [ffff88027469fbf0] oops_end at ffffffff8176d960

#3 [ffff88027469fc18] die at ffffffff810060db

#4 [ffff88027469fc48] do_general_protection at ffffffff8176d452

#5 [ffff88027469fc70] general_protection at ffffffff8176cdf2

    [exception RIP: pci_bus_read_config_dword+100]

    RIP: ffffffff813405f4  RSP: ffff88027469fd20  RFLAGS: 00010046

    RAX: 435f494350006963  RBX: ffff880274892000  RCX: 0000000000000004

    RDX: 0000000000000100  RSI: 0000000000000060  RDI: ffff880274892000

    RBP: ffff88027469fd48   R8: ffff88027469fd2c   R9: 00000000000012c0

    R10: 0000000000000006  R11: 00000000000012bf  R12: ffff88027469fd5c

    R13: 0000000000000246  R14: 0000000000000000  R15: ffff8802741a4000

    ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0000

#6 [ffff88027469fd50] pci_find_next_ext_capability at ffffffff81345d7b

#7 [ffff88027469fd90] pci_find_ext_capability at ffffffff81347225

#8 [ffff88027469fda0] get_device_error_info at ffffffff81356c4d

#9 [ffff88027469fdd0] aer_isr at ffffffff81357a38

#10 [ffff88027469fe28] process_one_work at ffffffff8105d4c0

#11 [ffff88027469fe70] worker_thread at ffffffff8105e251

#12 [ffff88027469fed0] kthread at ffffffff81064260

#13 [ffff88027469ff50] ret_from_fork at ffffffff81773a38


crash>


I have tested it on kernel 3.10 . But from source i could see that this
case is still relevant for latest Linux source .

--------------------END--------------

On Thu, Aug 2, 2018 at 12:50 PM, Mika Westerberg <
mika.westerberg@linux.intel.com> wrote:

> On Wed, Aug 01, 2018 at 07:15:12PM +0200, Lukas Wunner wrote:
> > On Wed, Aug 01, 2018 at 07:43:58PM +0300, Mika Westerberg wrote:
> > > On Tue, Jul 31, 2018 at 07:50:37AM +0200, Lukas Wunner wrote:
> > > > -static void remove_board(struct slot *p_slot)
> > > > +static void remove_board(struct slot *p_slot, bool safe_removal)
> > > >  {
> > > >   struct controller *ctrl = p_slot->ctrl;
> > > >
> > > > - pciehp_unconfigure_device(p_slot);
> > > > + pciehp_unconfigure_device(p_slot, safe_removal);
> > >
> > > Below we turn off power to the slot if it has power controller. Even if
> > > we disable slot from sysfs, I think it ends up being inaccessible after
> > > power is turned off. I wonder if we should mark the devices
> disconnected
> > > in that case as well?
> > >
> > > >
> > > >   if (POWER_CTRL(ctrl)) {
> > > >           pciehp_power_off_slot(p_slot);
> >
> > No, when pciehp_unconfigure_device() returns, the PCI devices below
> > the hotplug bridge are unbound and removed from the system.  They're
> > gone, so the bit set in their pci_dev struct would no longer be
> > accessible anyway.  Unless of course something is holding a ref on
> > the pci_dev, but that would seem to be a bug.  (Accessing a device
> > that's already removed from the system, that is.)
> >
> > Calling pci_dev_set_disconnected() only gives the PCI core and the
> > driver bound to the device an indication that's it's inaccessible,
> > so any code paths during unbound and PCI device teardown can skip
> > accesses.  (Because pci_dev_is_disconnected() is currently scoped
> > to the PCI core, the disconnected status can only be queried from
> > drivers that live in the PCI core, such as portdrv and all the
> > port services drivers.)  After the pci_dev is removed from the
> > hierarchy, accessing it seems at least questionable.
> >
> > Does this make things clearer?  Shout if it not. :-)
>
> Yes it does. Thank you :)
>

[-- Attachment #2: Type: text/html, Size: 18776 bytes --]