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;
MIME-Version: 1.0
References: <20200505210253.20311-1-f.fainelli@gmail.com> <20200505172302.GB1170406@t480s.localdomain>
 <d681a82b-5d4b-457f-56de-3a439399cb3d@gmail.com>
In-Reply-To: <d681a82b-5d4b-457f-56de-3a439399cb3d@gmail.com>
From:   Vladimir Oltean <olteanv@gmail.com>
Date:   Thu, 7 May 2020 00:40:30 +0300
Message-ID: <CA+h21hpvC6ST2iv-4xjpwpmRHQJvk-AufYFvG0J=5KzUgcnC5A@mail.gmail.com>
Subject: Re: [RFC net] net: dsa: Add missing reference counting
To:     Florian Fainelli <f.fainelli@gmail.com>
Cc:     Vivien Didelot <vivien.didelot@gmail.com>,
        netdev <netdev@vger.kernel.org>, Andrew Lunn <andrew@lunn.ch>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Florian,

On Thu, 7 May 2020 at 00:24, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 5/5/2020 2:23 PM, Vivien Didelot wrote:
> > On Tue,  5 May 2020 14:02:53 -0700, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >> If we are probed through platform_data we would be intentionally
> >> dropping the reference count on master after dev_to_net_device()
> >> incremented it. If we are probed through Device Tree,
> >> of_find_net_device() does not do a dev_hold() at all.
> >>
> >> Ensure that the DSA master device is properly reference counted by
> >> holding it as soon as the CPU port is successfully initialized and later
> >> released during dsa_switch_release_ports(). dsa_get_tag_protocol() does
> >> a short de-reference, so we hold and release the master at that time,
> >> too.
> >>
> >> Fixes: 83c0afaec7b7 ("net: dsa: Add new binding implementation")
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >
> > Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>
> >
> Andrew, Vladimir, any thoughts on that?
> --
> Florian

I might be completely off because I guess I just don't understand what
is the goal of keeping a reference to the DSA master in this way for
the entire lifetime of the DSA switch. I think that dev_hold is for
short-term things that cannot complete atomically, but I think that
you are trying to prevent the DSA master from getting freed from under
our feet, which at the moment would fault the kernel instantaneously?

If this is correct, it certainly doesn't do what it intends to do:
echo 0000\:00\:00.5> /sys/bus/pci/drivers/mscc_felix/unbind
[   71.576333] unregister_netdevice: waiting for swp0 to become free.
Usage count = 1
(hangs there)

But if I'm right and that's indeed what you want to achieve, shouldn't
we be using device links instead?
https://www.kernel.org/doc/html/v4.14/driver-api/device_link.html

Thanks,
-Vladimir