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] [thread-next>] [day] [month] [year] [list]

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

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.



>
>> +		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;
>> +}
>
>...
>