Re: [PATCH 14/32] PCI: hotplug: Demidlayer registration with the core

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Jun 17, 2018 at 07:44:03PM +0300, Andy Shevchenko wrote:
> On Sat, Jun 16, 2018 at 10:37 PM Lukas Wunner <lukas@xxxxxxxxx> wrote:
> >
> > When a hotplug driver calls pci_hp_register(), all steps necessary for
> > registration are carried out in one go, including creation of a kobject
> > and addition to sysfs.  That's a problem for pciehp once it's converted
> > to enable/disable the slot exclusively from the IRQ thread:  The thread
> > needs to be spawned after creation of the kobject (because it uses the
> > kobject's name), but before addition to sysfs (because it will handle
> > enable/disable requests submitted via sysfs).
> >
> > pci_hp_deregister() does offer a ->release callback that's invoked
> > after deletion from sysfs and before destruction of the kobject.  But
> > because pci_hp_register() doesn't offer a counterpart, hotplug drivers'
> > ->probe and ->remove code becomes asymmetric, which is error prone
> > as recently discovered use-after-free bugs in pciehp's ->remove hook
> > have shown.
> >
> > In a sense, this appears to be a case of the midlayer antipattern:
> >
> >    "The core thesis of the "midlayer mistake" is that midlayers are
> >     bad and should not exist.  That common functionality which it is
> >     so tempting to put in a midlayer should instead be provided as
> >     library routines which can [be] used, augmented, or ignored by
> >     each bottom level driver independently.  Thus every subsystem
> >     that supports multiple implementations (or drivers) should
> >     provide a very thin top layer which calls directly into the
> >     bottom layer drivers, and a rich library of support code that
> >     eases the implementation of those drivers.  This library is
> >     available to, but not forced upon, those drivers."
> >         --  Neil Brown (2009), https://lwn.net/Articles/336262/
> >
> > The presence of midlayer traits in the PCI hotplug core might be ascribed
> > to its age:  When it was introduced in February 2002, the blessings of a
> > library approach might not have been well known:
> > https://git.kernel.org/tglx/history/c/a8a2069f432c
> >
> > For comparison, the driver core does offer split functions for creating
> > a kobject (device_initialize()) and addition to sysfs (device_add()) as
> > an alternative to carrying out everything at once (device_register()).
> > This was introduced in October 2002:
> > https://git.kernel.org/tglx/history/c/8b290eb19962
> >
> > The odd ->release callback in the PCI hotplug core was added in 2003:
> > https://git.kernel.org/tglx/history/c/69f8d663b595
> >
> > Clearly, a library approach would not force every hotplug driver to
> > implement a ->release callback, but rather allow the driver to remove
> > the sysfs files, release its data structures and finally destroy the
> > kobject.  Alternatively, a driver may choose to remove everything with
> > pci_hp_deregister(), then release its data structures.
> >
> > To this end, offer drivers pci_hp_initialize() and pci_hp_add() as a
> > split-up version of pci_hp_register().  Likewise, offer pci_hp_del()
> > and pci_hp_destroy() as a split-up version of pci_hp_deregister().
> >
> > Eliminate the ->release callback and move its code into each driver's
> > teardown routine.
> >
> > Declare pci_hp_deregister() void, in keeping with the usual kernel
> > pattern that enablement can fail, but disablement cannot.  It only
> > returned an error if the caller passed in a NULL pointer or a slot which
> > has never or is no longer registered or is sharing its name with another
> > slot.  Those would be bugs, so WARN about them.  Few hotplug drivers
> > actually checked the return value and those that did only printed a
> > useless error message to dmesg.  Remove that.
> >
> > For most drivers the conversion was straightforward since it doesn't
> > matter whether the code in the ->release callback is executed before or
> > after destruction of the kobject.  But in the case of ibmphp, it was
> > unclear to me whether setting slot_cur->ctrl and slot_cur->bus_on to
> > NULL needs to happen before the kobject is destroyed, so I erred on
> > the side of caution and ensured that the order stays the same.  Another
> > nontrivial case is pnv_php, I've found the list and kref logic difficult
> > to understand, however my impression was that it is safe to delete the
> > list element and drop the references until after the kobject is
> > destroyed.
> >
> > Cc: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
> > Cc: Len Brown <lenb@xxxxxxxxxx>
> > Cc: Scott Murray <scott@xxxxxxxxxxxx>
> > Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Paul Mackerras <paulus@xxxxxxxxx>
> > Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
> > Cc: Gavin Shan <gwshan@xxxxxxxxxxxxxxxxxx>
> > Cc: Sebastian Ott <sebott@xxxxxxxxxxxxxxxxxx>
> > Cc: Gerald Schaefer <gerald.schaefer@xxxxxxxxxx>
> > Cc: Corentin Chary <corentin.chary@xxxxxxxxx>
> > Cc: Darren Hart <dvhart@xxxxxxxxxxxxx>
> > Cc: Andy Shevchenko <andy@xxxxxxxxxxxxx>
> > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> > ---
> >  drivers/pci/hotplug/acpiphp_core.c      |  22 +---
> >  drivers/pci/hotplug/cpci_hotplug_core.c |  14 +--
> >  drivers/pci/hotplug/cpqphp_core.c       |  16 +--
> >  drivers/pci/hotplug/ibmphp_core.c       |  15 ++-
> >  drivers/pci/hotplug/ibmphp_ebda.c       |  20 ----
> >  drivers/pci/hotplug/pci_hotplug_core.c  | 139 +++++++++++++++++-------
> >  drivers/pci/hotplug/pciehp_core.c       |  19 +---
> >  drivers/pci/hotplug/pcihp_skeleton.c    |  16 +--
> >  drivers/pci/hotplug/pnv_php.c           |   5 +-
> >  drivers/pci/hotplug/rpaphp_core.c       |   2 +-
> >  drivers/pci/hotplug/rpaphp_slot.c       |  13 +--
> >  drivers/pci/hotplug/s390_pci_hpc.c      |  13 +--
> >  drivers/pci/hotplug/sgi_hotplug.c       |   9 +-
> >  drivers/pci/hotplug/shpchp_core.c       |  20 +---
> 
> >  drivers/platform/x86/asus-wmi.c         |  12 +-
> >  drivers/platform/x86/eeepc-laptop.c     |  12 +-
> 
> So, as far as I can see these are just mechanical changes due to
> internal API change.
> Thus,
> 
> Acked-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> 
> for PDx86 bits only.

Hi Andy, I'm not sure what you intend by "PDx86".  I'm happy to add a note
so your ack looks like

Acked-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>  # drivers/platform/x86

or something similar.  Is that what you have in mind?

> >  include/linux/pci_hotplug.h             |  15 ++-
> >  17 files changed, 171 insertions(+), 191 deletions(-)



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux