netdev - Re: [PATCH net-next v5 08/10] net: libwx: add eswitch switch api for devlink ops

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZrNd1_m3UduA9mT0@nanopsycho.orion>
Date: Wed, 7 Aug 2024 13:43:19 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: "mengyuanlou@...-swift.com" <mengyuanlou@...-swift.com>
Cc: Simon Horman <horms@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v5 08/10] net: libwx: add eswitch switch api for
 devlink ops

Wed, Aug 07, 2024 at 09:41:53AM CEST, mengyuanlou@...-swift.com wrote:
>
>
>> 2024年8月6日 19:42，Jiri Pirko <jiri@...nulli.us> 写道：
>> 
>> Mon, Aug 05, 2024 at 06:43:50PM CEST, horms@...nel.org wrote:
>>> On Sun, Aug 04, 2024 at 08:48:39PM +0800, Mengyuan Lou wrote:
>>> 
>>> Each patch needs a patch description describing not just what is done
>>> but why.
>>> 
>>> Also, please seed the CC list for patch submissions
>>> using get_maintainer this.patch. I believe that b4
>>> will do that for you.
>>> 
>>>> Signed-off-by: Mengyuan Lou <mengyuanlou@...-swift.com>
>>> 
>>> ...
>>> 
>>>> static void wx_devlink_free(void *devlink_ptr)
>>>> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_eswitch.c b/drivers/net/ethernet/wangxun/libwx/wx_eswitch.c
>>>> new file mode 100644
>>>> index 000000000000..a426a352bf96
>>>> --- /dev/null
>>>> +++ b/drivers/net/ethernet/wangxun/libwx/wx_eswitch.c
>>>> @@ -0,0 +1,53 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/* Copyright (C) 2019-2021, Intel Corporation. */
>>> 
>>> Are you sure Intel holds the copyright on this code?
>>> 
>>>> +
>>>> +#include <linux/pci.h>
>>>> +
>>>> +#include "wx_type.h"
>>>> +#include "wx_eswitch.h"
>>>> +#include "wx_devlink.h"
>>>> +
>>>> +int wx_eswitch_mode_set(struct devlink *devlink, u16 mode,
>>>> + struct netlink_ext_ack *extack)
>>>> +{
>>>> + struct wx_dl_priv *dl_priv = devlink_priv(devlink);
>>>> + struct wx *wx = dl_priv->priv_wx;
>>>> +
>>>> + if (wx->eswitch_mode == mode)
>>>> + return 0;
>>>> +
>>>> + if (wx->num_vfs) {
>>>> + dev_info(&(wx)->pdev->dev,
>>>> + "Change eswitch mode is allowed if there is no VFs.");
>>> 
>>> maybe: Changing eswitch mode is only allowed if there are no VFs.
>>> 
>>>> + return -EOPNOTSUPP;
>>>> + }
>>>> +
>>>> + switch (mode) {
>>>> + case DEVLINK_ESWITCH_MODE_LEGACY:
>>>> + dev_info(&(wx)->pdev->dev,
>>>> + "PF%d changed eswitch mode to legacy",
>>>> + wx->bus.func);
>>>> + NL_SET_ERR_MSG_MOD(extack, "Changed eswitch mode to legacy");
>>>> + break;
>>>> + case DEVLINK_ESWITCH_MODE_SWITCHDEV:
>>>> + dev_info(&(wx)->pdev->dev,
>>>> + "Do not support switchdev in eswitch mode.");
>>>> + NL_SET_ERR_MSG_MOD(extack, "Do not support switchdev mode.");
>>> 
>>> maybe: eswitch mode switchdev is not supported
>>> 
>>> I am curious to know if you are planning to implement eswitch mode in the
>>> near future.  If not, is wx_eswitch_mode_set() needed: it seems unused in
>>> this patchset: it should probably be added in a patchset that uses it.
>> 
>> This is wrong.
>> 
>> switchdev mode should be supported from the beginning. There is some odd
>> hybrid mode supported, VF gets devlink port created for representors,
>> yet no netdev. I'm very sure this triggers:
>> 
>> static void devlink_port_type_warn(struct work_struct *work)
>> {
>>        struct devlink_port *port = container_of(to_delayed_work(work),
>>                                                 struct devlink_port,
>>                                                 type_warn_dw);
>>        dev_warn(port->devlink->dev, "Type was not set for devlink port.");
>> }
>> 
>> So, if you don't see this message, leads to my assuptions this patchset
>> was not tested. Certainly odd.
>> 
>
>I have seen this warnning info.

Yet you ignore it. Awesome.


>What I want is that the devlink port Uapi to vfs, but I think the product will
>not to support switchdev mode and the representors representors will not be used.

Implement proper netdevs for VF representors in switchdev mode please.
Legacy mode should not create devlink ports.

>
>Be like:
>https://lore.kernel.org/netdev/20240620002741.1029936-1-kuba@kernel.org/
>
>> 
>>> 
>>>> + return -EINVAL;
>>>> + default:
>>>> + NL_SET_ERR_MSG_MOD(extack, "Unknown eswitch mode");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + wx->eswitch_mode = mode;
>>>> + return 0;
>>>> +}
>>>> +
>>>> +int wx_eswitch_mode_get(struct devlink *devlink, u16 *mode)
>>>> +{
>>>> + struct wx_dl_priv *dl_priv = devlink_priv(devlink);
>>>> + struct wx *wx = dl_priv->priv_wx;
>>>> +
>>>> + *mode = wx->eswitch_mode;
>>>> + return 0;
>>>> +}
>>> 
>>> ...
>>> 
>> 
>