lkml.org 
[lkml]   [2021]   [Sep]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 0/3] Fix for KSZ DSA switch shutdown
    From
    +Saravana,

    On 9/9/2021 8:47 AM, Vladimir Oltean wrote:
    > On Thu, Sep 09, 2021 at 03:19:52PM +0200, Lino Sanfilippo wrote:
    >>> Do you see similar things on your 5.10 kernel?
    >>
    >> For the master device is see
    >>
    >> lrwxrwxrwx 1 root root 0 Sep 9 14:10 /sys/class/net/eth0/device/consumer:spi:spi3.0 -> ../../../virtual/devlink/platform:fd580000.ethernet--spi:spi3.0
    >
    > So this is the worst of the worst, we have a device link but it doesn't help.
    >
    > Where the device link helps is here:
    >
    > __device_release_driver
    > while (device_links_busy(dev))
    > device_links_unbind_consumers(dev);
    >
    > but during dev_shutdown, device_links_unbind_consumers does not get called
    > (actually I am not even sure whether it should).
    >
    > I've reproduced your issue by making this very simple change:
    >
    > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
    > index 60d94e0a07d6..ec00f34cac47 100644
    > --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
    > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
    > @@ -1372,6 +1372,7 @@ static struct pci_driver enetc_pf_driver = {
    > .id_table = enetc_pf_id_table,
    > .probe = enetc_pf_probe,
    > .remove = enetc_pf_remove,
    > + .shutdown = enetc_pf_remove,
    > #ifdef CONFIG_PCI_IOV
    > .sriov_configure = enetc_sriov_configure,
    > #endif
    >
    > on my DSA master driver. This is what the genet driver has "special".
    >
    > I was led into grave error by Documentation/driver-api/device_link.rst,
    > which I've based my patch on, where it clearly says that device links
    > are supposed to help with shutdown ordering (how?!).

    I was also under the impression that device links were supposed to help
    with shutdown ordering, because it does matter a lot. One thing that I
    had to work before (and seems like it came back recently) is the
    shutdown ordering between gpio_keys.c and the GPIO controller. If you
    suspend the GPIO controller first, gpio_keys.c never gets a chance to
    keep the GPIO pin configured for a wake-up interrupt, therefore no
    wake-up event happens on key presses, whoops.

    >
    > So the question is, why did my DSA trees get torn down on shutdown?
    > Basically the short answer is that my SPI controller driver does
    > implement .shutdown, and calls the same code path as the .remove code,
    > which calls spi_unregister_controller which removes all SPI children..
    >
    > When I added this device link, one of the main objectives was to not
    > modify all DSA drivers. I was certain based on the documentation that
    > device links would help, now I'm not so sure anymore.
    >
    > So what happens is that the DSA master attempts to unregister its net
    > device on .shutdown, but DSA does not implement .shutdown, so it just
    > sits there holding a reference (supposedly via dev_hold, but where from?!)
    > to the master, which makes netdev_wait_allrefs to wait and wait.

    It's not coming from of_find_net_device_by_node() that's for sure and
    with OF we don't go through the code path calling
    dsa_dev_to_net_device() which does call dev_hold() and then shortly
    thereafter the caller calls dev_put() anyway.

    >
    > I need more time for the denial phase to pass, and to understand what
    > can actually be done. I will also be away from the keyboard for the next
    > few days, so it might take a while. Your patches obviously offer a
    > solution only for KSZ switches, we need something more general. If I
    > understand your solution, it works not by virtue of there being any
    > shutdown ordering guarantee at all, but simply due to the fact that
    > DSA's .shutdown hook gets called eventually, and the reference to the
    > master gets freed eventually, which unblocks the unregister_netdevice
    > call from the master. I don't yet understand why DSA holds a long-term
    > reference to the master, that's one thing I need to figure out.
    >

    Agreed.
    --
    Florian

    \
     
     \ /
      Last update: 2021-09-09 18:01    [W:4.069 / U:0.152 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site