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: <725e121f05362da4328dda08d5814211a0725dac.1635064599.git.leonro@nvidia.com>
 <YXUhyLXsc2egWNKx@shredder> <CAKOOJTzc9pJ1KKDHuGTFDeHb77B2GynA9HEVWKys=zvh_kY+Hw@mail.gmail.com>
 <YXeYjXx92wKdPe02@unreal> <CAKOOJTyrzosizeKpfYcu4jMn6SRYrqxU0BzMf8qudAk5e74R9g@mail.gmail.com>
 <YXhVd16heaHCegL1@unreal>
In-Reply-To: <YXhVd16heaHCegL1@unreal>
From:   Edwin Peer <edwin.peer@broadcom.com>
Date:   Tue, 26 Oct 2021 13:03:41 -0700
Message-ID: <CAKOOJTzrQYz4FTDU_d_R0RLA4u6pfK9=+=E_uKMr4VCNbmF_kA@mail.gmail.com>
Subject: Re: [PATCH net-next] netdevsim: Register and unregister devlink traps
 on probe/remove device
To:     Leon Romanovsky <leon@kernel.org>
Cc:     Ido Schimmel <idosch@idosch.org>,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Ido Schimmel <idosch@mellanox.com>,
        Jiri Pirko <jiri@mellanox.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        netdev <netdev@vger.kernel.org>,
        syzbot+93d5accfaefceedf43c1@syzkaller.appspotmail.com,
        Michael Chan <michael.chan@broadcom.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Oct 26, 2021 at 12:22 PM Leon Romanovsky <leon@kernel.org> wrote:

> At least in mlx5 case, reload_enable() was before register_netdev().
> It stayed like this after swapping it with devlink_register().

What am I missing here?

err = mlx5_init_one(dev);
if (err) {
       mlx5_core_err(dev, "mlx5_init_one failed with error code %d\n", err);
       goto err_init_one;
}

err = mlx5_crdump_enable(dev);
if (err)
        dev_err(&pdev->dev, "mlx5_crdump_enable failed with error code
%d\n", err);

pci_save_state(pdev);
devlink_register(devlink);

Doesn't mlx5_init_one() ultimately result in the netdev being
presented to user space, even if it is via aux bus?

> No, it is not requirement, but my suggestion. You need to be aware that
> after call to devlink_register(), the device will be fully open for devlink
> netlink access. So it is strongly advised to put devlink_register to be the
> last command in PCI initialization sequence.

Right, that's the problem. Once we register the netdev, we're in a
race with user space, which may expect to be able to call devlink
before we get to devlink_register().

> You obviously need to fix your code. Upstream version of bnxt driver
> doesn't have reload_* support, so all this regression blaming it not
> relevant here.

Right, our timing is unfortunate and that's on us. It's still not
clear to me how to actually fix the devlink reload code without the
benefit of something similar to the reload enable API.

> In upstream code, devlink_register() doesn't accept ops like it was
> before and position of that call does only one thing - opens devlink
> netlink access. All kernel devlink APIs continue to be accessible even
> before devlink_register.

This isn't about kernel API. This is precisely about existing user
space that expects devlink to work immediately after the netdev
appears.

> It looks like your failure is in backport code.

Our out-of-tree driver isn't the issue here. I'm talking about the
proposed upstream code. The issue is what to do in order to get
something workable upstream for devlink reload. We can't move
devlink_register() later, that will cause a regression. What do you
suggest instead?

Regards,
Edwin Peer