lkml.org 
[lkml]   [2018]   [May]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V2] PCI/portdrv: do not disable device on reboot/shutdown
    On Wed, May 23, 2018 at 06:57:18PM -0400, Sinan Kaya wrote:
    > On 5/23/2018 5:32 PM, Bjorn Helgaas wrote:
    > >
    > > The crash seems to indicate that the hpsa device attempted a DMA after
    > > we cleared the Root Port's PCI_COMMAND_MASTER, which means
    > > hpsa_shutdown() didn't stop DMA from the device (it looks like *most*
    > > shutdown methods don't disable device DMA, so it's in good company).
    >
    > All drivers are expected to shutdown DMA and interrupts in their shutdown()
    > routines. They can skip removing threads, data structures etc. but DMA and
    > interrupt disabling are required. This is the difference between shutdown()
    > and remove() callbacks.
    >
    > If you see that this is not being done in HPSA, then that is where the
    > bugfix should be.
    >
    > Counter argument is that if shutdown() is not implemented, at least remove()
    > should be called. Expecting all drivers to implement shutdown() callbacks
    > is just bad by design in my opinion.
    >
    > Code should have fallen back to remove() if shutdown() doesn't exist.
    > I can propose a patch for this but this is yet another story to chase.

    That sounds like a reasonable idea, and it is definitely another can
    of worms. I looked briefly at some of the .shutdown() cases:

    - device_shutdown() doesn't fall back to remove().

    - It looks like most bus_types don't implement .shutdown() at all (I
    didn't look at them all).

    - Of the bus_types that do implement .shutdown(), most do not fall
    back to .remove(). ps3_system_bus_type() is an example of one
    that *does* fall back to a driver's .remove() if there is no
    .shutdown().

    Implement shutdown (no fallback unless indicated):

    ecard_bus_type
    gio_bus_type
    ps3_system_bus_type # does fallback to remove
    ibmebus_bus_type
    isa_bus_type
    platform_bus_type # not direct implementation
    fmc_bus_type # fmc_shutdown() looks spurious
    mipi_dsi_bus_type
    hv_bus

    > >> This has been found to cause crashes on HP DL360 Gen9 machines during
    > >> reboot. Besides, kexec is already clearing the bus master bit in
    > >> pci_device_shutdown() after all PCI drivers are removed.
    > >
    > > The original path was:
    > >
    > > pci_device_shutdown(hpsa)
    > > drv->shutdown
    > > hpsa_shutdown # hpsa_pci_driver.shutdown
    > > ...
    > > pci_device_shutdown(RP) # root port
    > > drv->shutdown
    > > pcie_portdrv_remove # pcie_portdriver.shutdown
    > > pcie_port_device_remove
    > > pci_disable_device
    > > do_pci_disable_device
    > > # clear RP PCI_COMMAND_MASTER
    > > if (kexec)
    > > pci_clear_master(RP)
    > > # clear RP PCI_COMMAND_MASTER
    > >
    > > If I understand correctly, the new path after this patch is:
    > >
    > > pci_device_shutdown(hpsa)
    > > drv->shutdown
    > > hpsa_shutdown # hpsa_pci_driver.shutdown
    > > ...
    > > pci_device_shutdown(RP) # root port
    > > drv->shutdown
    > > pcie_portdrv_shutdown # pcie_portdriver.shutdown
    > > __pcie_portdrv_remove(RP, false)
    > > pcie_port_device_remove(RP, false)
    > > # do NOT clear RP PCI_COMMAND_MASTER
    >
    > yup
    >
    > > if (kexec)
    > > pci_clear_master(RP)
    > > # clear RP PCI_COMMAND_MASTER
    > >
    > > I guess this patch avoids the panic during reboot because we're not in
    > > the kexec path, so we never clear PCI_COMMAND_MASTER for the Root
    > > Port, so the hpsa device can DMA happily until the lights go out.
    > >
    > > But DMA continuing for some random amount of time before the reboot or
    > > shutdown happens makes me a little queasy. That doesn't sound safe.
    > > The more I think about this, the more confused I get. What am I
    > > missing?
    >
    > see above.
    >
    > >
    > >> Just remove the extra clear in shutdown path by seperating the remove and
    > >> shutdown APIs in the PORTDRV.
    > >>
    > >> static pci_ers_result_t pcie_portdrv_error_detected(struct pci_dev *dev,
    > >> @@ -218,7 +228,7 @@ static struct pci_driver pcie_portdriver = {
    > >>
    > >> .probe = pcie_portdrv_probe,
    > >> .remove = pcie_portdrv_remove,
    > >> - .shutdown = pcie_portdrv_remove,
    > >> + .shutdown = pcie_portdrv_shutdown,
    > >
    > > What are the circumstances when we call .remove() vs .shutdown()?
    > >
    > > I guess the main (maybe only) way to call .remove() is to hot-remove
    > > the port? And .shutdown() is basically used in the reboot and kexec
    > > paths?
    >
    > Correct. shutdown() is only called during reboot/shutdown calls. If you echo
    > 1 into the remove file, remove() gets called. Handy for hotplug use cases.
    > It needs to be the exact opposite of the probe. It needs to clean up resources
    > etc. and have the HW in a state where it can be reinitialized via probe again.
    >
    > >
    > >> .err_handler = &pcie_portdrv_err_handler,
    > >>
    > >> --
    > >> 2.7.4
    > >>
    > >
    >
    >
    > --
    > Sinan Kaya
    > Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
    > Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

    \
     
     \ /
      Last update: 2018-05-24 20:36    [W:4.663 / U:0.188 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site