Message-ID: <9216e5a5-c2aa-4f08-8c53-7622b95b92ca@linux.intel.com>
Date: Tue, 30 Jul 2024 15:19:58 +0200
From: Marcin Szycik <marcin.szycik@...ux.intel.com>
To: Song Yoong Siang <yoong.siang.song@...el.com>,
 Tony Nguyen <anthony.l.nguyen@...el.com>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Richard Cochran <richardcochran@...il.com>,
 Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
 Jesper Dangaard Brouer <hawk@...nel.org>,
 John Fastabend <john.fastabend@...il.com>,
 Vinicius Costa Gomes <vinicius.gomes@...el.com>,
 Jonathan Corbet <corbet@....net>,
 Przemek Kitszel <przemyslaw.kitszel@...el.com>,
 Shinas Rasheed <srasheed@...vell.com>, Kevin Tian <kevin.tian@...el.com>,
 Brett Creeley <brett.creeley@....com>,
 Blanco Alcaine Hector <hector.blanco.alcaine@...el.com>,
 Joshua Hay <joshua.a.hay@...el.com>, Sasha Neftin <sasha.neftin@...el.com>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org,
 intel-wired-lan@...ts.osuosl.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org
Subject: Re: [Intel-wired-lan] [PATCH iwl-next, v1 2/3] igc: Add default Rx
 queue configuration via sysfs



On 30.07.2024 03:23, Song Yoong Siang wrote:
> From: Blanco Alcaine Hector <hector.blanco.alcaine@...el.com>
> 
> This commit introduces the support to configure default Rx queue during

Use imperative mood.

> runtime. A new sysfs attribute "default_rx_queue" has been added, allowing
> users to check and modify the default Rx queue.
> 
> 1. Command to check the currently configured default Rx queue:
>    cat /sys/devices/pci0000:00/.../default_rx_queue
> 
> 2. Command to set the default Rx queue to a desired value, for example 3:
>    echo 3 > /sys/devices/pci0000:00/.../default_rx_queue
> 
> Signed-off-by: Blanco Alcaine Hector <hector.blanco.alcaine@...el.com>
> Signed-off-by: Song Yoong Siang <yoong.siang.song@...el.com>
> ---
>  drivers/net/ethernet/intel/igc/Makefile    |   3 +-
>  drivers/net/ethernet/intel/igc/igc_main.c  |   6 +
>  drivers/net/ethernet/intel/igc/igc_regs.h  |   6 +
>  drivers/net/ethernet/intel/igc/igc_sysfs.c | 156 +++++++++++++++++++++
>  drivers/net/ethernet/intel/igc/igc_sysfs.h |  10 ++
>  5 files changed, 180 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/ethernet/intel/igc/igc_sysfs.c
>  create mode 100644 drivers/net/ethernet/intel/igc/igc_sysfs.h
> 
> diff --git a/drivers/net/ethernet/intel/igc/Makefile b/drivers/net/ethernet/intel/igc/Makefile
> index efc5e7983dad..c31bc18ede13 100644
> --- a/drivers/net/ethernet/intel/igc/Makefile
> +++ b/drivers/net/ethernet/intel/igc/Makefile
> @@ -8,5 +8,6 @@
>  obj-$(CONFIG_IGC) += igc.o
>  
>  igc-y := igc_main.o igc_mac.o igc_i225.o igc_base.o igc_nvm.o igc_phy.o \
> -	 igc_diag.o igc_ethtool.o igc_ptp.o igc_dump.o igc_tsn.o igc_xdp.o
> +	 igc_diag.o igc_ethtool.o igc_ptp.o igc_dump.o igc_tsn.o igc_xdp.o \
> +	 igc_sysfs.o
>  igc-$(CONFIG_IGC_LEDS) += igc_leds.o
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index cb5c7b09e8a0..6a925615911a 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -18,6 +18,7 @@
>  
>  #include "igc.h"
>  #include "igc_hw.h"
> +#include "igc_sysfs.h"
>  #include "igc_tsn.h"
>  #include "igc_xdp.h"
>  
> @@ -7069,6 +7070,9 @@ static int igc_probe(struct pci_dev *pdev,
>  			goto err_register;
>  	}
>  
> +	if (igc_sysfs_init(adapter))
> +		dev_err(&pdev->dev, "Failed to allocate sysfs resources\n");
> +
>  	return 0;
>  
>  err_register:
> @@ -7124,6 +7128,8 @@ static void igc_remove(struct pci_dev *pdev)
>  	if (IS_ENABLED(CONFIG_IGC_LEDS))
>  		igc_led_free(adapter);
>  
> +	igc_sysfs_exit(adapter);
> +
>  	/* Release control of h/w to f/w.  If f/w is AMT enabled, this
>  	 * would have already happened in close and is redundant.
>  	 */
> diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
> index e5b893fc5b66..df96800f6e3b 100644
> --- a/drivers/net/ethernet/intel/igc/igc_regs.h
> +++ b/drivers/net/ethernet/intel/igc/igc_regs.h
> @@ -63,6 +63,12 @@
>  /* RSS registers */
>  #define IGC_MRQC		0x05818 /* Multiple Receive Control - RW */
>  
> +/* MRQC register bit definitions */
> +#define IGC_MRQC_ENABLE_MQ		0x00000000

Just 0.

> +#define IGC_MRQC_ENABLE_MASK		GENMASK(2, 0)
> +#define IGC_MRQC_DEFAULT_QUEUE_MASK	GENMASK(5, 3)
> +#define IGC_MRQC_DEFAULT_QUEUE_SHIFT	3
> +
>  /* Filtering Registers */
>  #define IGC_ETQF(_n)		(0x05CB0 + (4 * (_n))) /* EType Queue Fltr */
>  #define IGC_FHFT(_n)		(0x09000 + (256 * (_n))) /* Flexible Host Filter */
> diff --git a/drivers/net/ethernet/intel/igc/igc_sysfs.c b/drivers/net/ethernet/intel/igc/igc_sysfs.c
> new file mode 100644
> index 000000000000..34d838e6a019
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/igc/igc_sysfs.c
> @@ -0,0 +1,156 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2024 Intel Corporation */
> +
> +#include <linux/device.h>
> +#include <linux/kobject.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +
> +#include "igc.h"
> +#include "igc_regs.h"
> +#include "igc_sysfs.h"
> +
> +/**
> + * igc_is_default_queue_supported - Checks if default Rx queue can be configured
> + * @mrqc: MRQC register content
> + *
> + * Checks if the current configuration of the device supports changing the
> + * default Rx queue configuration.
> + *
> + * Return: true if the default Rx queue can be configured, false otherwise.
> + */
> +static bool igc_is_default_queue_supported(u32 mrqc)
> +{
> +	u32 mrqe = mrqc & IGC_MRQC_ENABLE_MASK;
> +
> +	/* The default Rx queue setting is applied only if Multiple Receive
> +	 * Queues (MRQ) as defined by filters (2-tuple filters, L2 Ether-type
> +	 * filters, SYN filter and flex filters) is enabled.
> +	 */
> +	if (mrqe != IGC_MRQC_ENABLE_MQ && mrqe != IGC_MRQC_ENABLE_RSS_MQ)
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * igc_get_default_rx_queue - Returns the index of default Rx queue
> + * @adapter: address of board private structure
> + *
> + * Return: index of the default Rx queue.
> + */
> +static u32 igc_get_default_rx_queue(struct igc_adapter *adapter)
> +{
> +	struct igc_hw *hw = &adapter->hw;
> +	u32 mrqc = rd32(IGC_MRQC);
> +
> +	if (!igc_is_default_queue_supported(mrqc)) {
> +		netdev_warn(adapter->netdev,
> +			    "MRQ disabled: default RxQ is ignored.\n");
> +	}
> +
> +	return (mrqc & IGC_MRQC_DEFAULT_QUEUE_MASK) >>
> +		IGC_MRQC_DEFAULT_QUEUE_SHIFT;
> +}
> +
> +/**
> + * igc_set_default_rx_queue - Sets the default Rx queue
> + * @adapter: address of board private structure
> + * @queue: index of the queue to be set as default Rx queue
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +static int igc_set_default_rx_queue(struct igc_adapter *adapter, u32 queue)
> +{
> +	struct igc_hw *hw = &adapter->hw;
> +	u32 mrqc = rd32(IGC_MRQC);
> +
> +	if (!igc_is_default_queue_supported(mrqc)) {
> +		netdev_err(adapter->netdev,
> +			   "Default RxQ not supported. Please enable MRQ.\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (queue > adapter->rss_queues - 1) {

if (queue >= adapter->rss_queues)

> +		netdev_err(adapter->netdev,
> +			   "Invalid default RxQ index %d. Valid range: 0-%u.\n",
> +			   queue, adapter->rss_queues - 1);
> +		return -EINVAL;
> +	}
> +
> +	/* Set the default Rx queue */
> +	mrqc = rd32(IGC_MRQC);
> +	mrqc &= ~IGC_MRQC_DEFAULT_QUEUE_MASK;
> +	mrqc |= queue << IGC_MRQC_DEFAULT_QUEUE_SHIFT;
> +	wr32(IGC_MRQC, mrqc);
> +
> +	return 0;
> +}
> +
> +static ssize_t default_rx_queue_show(struct device *dev,
> +				     struct device_attribute *attr,
> +				     char *buf)

Why no igc_ prefix (and function doc)?

> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct net_device *netdev = pci_get_drvdata(pdev);
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +	u32 default_rx_queue = igc_get_default_rx_queue(adapter);
> +
> +	return sysfs_emit(buf, "%d\n", default_rx_queue);
> +}
> +
> +static ssize_t default_rx_queue_store(struct device *dev,
> +				      struct device_attribute *attr,
> +				      const char *buf, size_t count)

Ditto

> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct net_device *netdev = pci_get_drvdata(pdev);
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +	u32 default_rx_queue;
> +	int err;
> +
> +	err = kstrtou32(buf, 10, &default_rx_queue);
> +	if (err) {
> +		netdev_err(adapter->netdev,
> +			   "Invalid default RxQ index. Valid range: 0-%u.\n",
> +			   adapter->rss_queues - 1);
> +		return err;
> +	}
> +
> +	err = igc_set_default_rx_queue(adapter, default_rx_queue);
> +	if (err < 0)
> +		return -EINVAL;

Why discard return error here?

> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(default_rx_queue);
> +
> +static struct attribute *attrs[] = {
> +	&dev_attr_default_rx_queue.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group attr_group = {
> +	.attrs = attrs,
> +};
> +
> +int igc_sysfs_init(struct igc_adapter *adapter)
> +{
> +	int err;
> +
> +	err = sysfs_create_group(&adapter->pdev->dev.kobj, &attr_group);
> +	if (err) {
> +		netdev_err(adapter->netdev,
> +			   "Failed to create sysfs attribute group.\n");
> +	}
> +
> +	return err;
> +}
> +
> +void igc_sysfs_exit(struct igc_adapter *adapter)
> +{
> +	sysfs_remove_group(&adapter->pdev->dev.kobj, &attr_group);
> +}
> diff --git a/drivers/net/ethernet/intel/igc/igc_sysfs.h b/drivers/net/ethernet/intel/igc/igc_sysfs.h
> new file mode 100644
> index 000000000000..b074ad4bc63a
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/igc/igc_sysfs.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2024 Intel Corporation */
> +
> +#ifndef _IGC_SYSFS_H_
> +#define _IGC_SYSFS_H_
> +
> +int igc_sysfs_init(struct igc_adapter *adapter);
> +void igc_sysfs_exit(struct igc_adapter *adapter);
> +
> +#endif /* _IGC_SYSFS_H_ */

Thanks,
Marcin

Powered by blists - more mailing lists