Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 0/3] Fix for KSZ DSA switch shutdown
To:     Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>
Cc:     p.rosenberger@kunbus.com, woojung.huh@microchip.com,
        UNGLinuxDriver@microchip.com, andrew@lunn.ch,
        vivien.didelot@gmail.com, davem@davemloft.net, kuba@kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20210909095324.12978-1-LinoSanfilippo@gmx.de>
 <20210909101451.jhfk45gitpxzblap@skbuf>
 <81c1a19f-c5dc-ab4a-76ff-59704ea95849@gmx.de>
 <20210909114248.aijujvl7xypkh7qe@skbuf>
 <20210909125606.giiqvil56jse4bjk@skbuf>
 <trinity-85ae3f9c-38f9-4442-98d3-bdc01279c7a8-1631193592256@3c-app-gmx-bs01>
 <20210909154734.ujfnzu6omcjuch2a@skbuf>
 <8498b0ce-99bb-aef9-05e1-d359f1cad6cf@gmx.de>
 <2b316d9f-1249-9008-2901-4ab3128eed81@gmail.com>
From:   Lino Sanfilippo <LinoSanfilippo@gmx.de>
Message-ID: <5b899bb3-ed37-19ae-8856-3dabce534cc6@gmx.de>
Date:   Thu, 9 Sep 2021 19:07:33 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <2b316d9f-1249-9008-2901-4ab3128eed81@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On 09.09.21 at 18:44, Florian Fainelli wrote:
>
>
> On 9/9/2021 9:37 AM, Lino Sanfilippo wrote:
>> On 09.09.21 at 17:47, 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=C2=A0 9 14:10 /sys/class/net/eth0/device=
/consumer:spi:spi3.0 -> ../../../virtual/devlink/platform:fd580000.etherne=
t--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
>>> =C2=A0=C2=A0=C2=A0=C2=A0while (device_links_busy(dev))
>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 device_links_unbind_consume=
rs(dev);
>>>
>>> but during dev_shutdown, device_links_unbind_consumers does not get ca=
lled
>>> (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 =3D {
>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .id_table =3D enetc_pf_id_table,
>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .probe =3D enetc_pf_probe,
>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .remove =3D enetc_pf_remove,
>>> +=C2=A0=C2=A0=C2=A0 .shutdown =3D enetc_pf_remove,
>>> =C2=A0 #ifdef CONFIG_PCI_IOV
>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .sriov_configure =3D enetc_sriov_config=
ure,
>>> =C2=A0 #endif
>>>
>>> on my DSA master driver. This is what the genet driver has "special".
>>>
>>
>> Ah, that is interesting.
>>
>>> 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?!).
>>>
>>> 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 fro=
m?!)
>>> to the master, which makes netdev_wait_allrefs to wait and wait.
>>>
>>
>> Right, that was also my conclusion.
>>
>>> 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 ne=
xt
>>> 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.
>>
>> Well actually the SPI shutdown hook gets called which then calls ksz947=
7_shutdown
>> (formerly ksz9477_reset_switch) which then shuts down the switch by
>> stopping the worker thread and tearing down the DSA tree (via dsa_tree_=
shutdown()).
>>
>> While it is right that the patch series only fixes the KSZ case for now=
, the idea was that
>> other drivers could use a similar approach in by calling the new functi=
on dsa_tree_shutdown()
>> in their shutdown handler to make sure that all refs to the master devi=
ce are released.
> It does not scale really well to have individual drivers call dsa_tree_s=
hutdown() in their respective .shutdown callback, and in a multi-switch co=
nfiguration, I am not sure what the results would look like.
>
> In premise, each driver ought to be able to call dsa_unregister_switch()=
, along with all of the driver specific shutdown and eventually, given pro=
per device ordering the DSA tree would get automatically torn down, and th=
en the DSA master's .shutdown() callback would be called.
>
> FWIW, the reason why we call .shutdown() in bcmgenet is to turn off DMA =
and clocks, which matters for kexec (DMA) as well as power savings (S5 mod=
e).

I agree with the scalability. Concerning the multi-switch case I dont know=
 about the possible issues (I am quite new to working with DSA).
So lets wait for Vladimirs solution.

Regards,
Lino