diff mbox series

[iwl-next,v1,2/3] igc: Add default Rx queue configuration via sysfs

Message ID 20240730012312.775893-1-yoong.siang.song@intel.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series Add Default Rx Queue Setting for igc driver | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Guessed tree name to be net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 42 this patch: 42
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 13 of 13 maintainers
netdev/build_clang success Errors and warnings before: 46 this patch: 46
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 49 this patch: 49
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 29 this patch: 29
netdev/source_inline success Was 0 now: 0

Commit Message

Song Yoong Siang July 30, 2024, 1:23 a.m. UTC 
From: Blanco Alcaine Hector <hector.blanco.alcaine@intel.com>

This commit introduces the support to configure default Rx queue during
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@intel.com>
Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.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

Comments

Kurt Kanzenbach July 30, 2024, 9:58 a.m. UTC | #1 
On Tue Jul 30 2024, Song Yoong Siang wrote:
> From: Blanco Alcaine Hector <hector.blanco.alcaine@intel.com>
>
> This commit introduces the support to configure default Rx queue during
> 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@intel.com>
> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>

[...]

> 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 */

Nit: Now, the MRQC register definitions are scattered over two files:
igc_regs.h and igc.h. igc.h has

#define IGC_MRQC_ENABLE_RSS_MQ		0x00000002
#define IGC_MRQC_RSS_FIELD_IPV4_UDP	0x00400000
#define IGC_MRQC_RSS_FIELD_IPV6_UDP	0x00800000

Maybe combine them into a single location?

> +#define IGC_MRQC_ENABLE_MQ		0x00000000
> +#define IGC_MRQC_ENABLE_MASK		GENMASK(2, 0)
> +#define IGC_MRQC_DEFAULT_QUEUE_MASK	GENMASK(5, 3)
> +#define IGC_MRQC_DEFAULT_QUEUE_SHIFT	3

Nit: FIELD_GET() and FIELD_PREP() can help to get rid of the manual
shifting. See below. 

> +
>  /* 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;

Nit: return FIELD_GET(IGC_MRQC_DEFAULT_QUEUE_MASK, mrqc);

> +}
> +
> +/**
> + * 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) {
> +		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;

Nit: mrqc |= FIELD_PREP(IGC_MRQC_DEFAULT_QUEUE_MASK, queue);

Thanks,
Kurt
Wojciech Drewek July 30, 2024, 10:08 a.m. UTC | #2 
On 30.07.2024 03:23, Song Yoong Siang wrote:
> From: Blanco Alcaine Hector <hector.blanco.alcaine@intel.com>
> 
> This commit introduces the support to configure default Rx queue during
> 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@intel.com>
> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.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
> +#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;

just:
return mrqe != IGC_MRQC_ENABLE_MQ && mrqe != IGC_MRQC_ENABLE_RSS_MQ

> +
> +	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");

Should we return an error here?
> +	}
> +
> +	return (mrqc & IGC_MRQC_DEFAULT_QUEUE_MASK) >>
> +		IGC_MRQC_DEFAULT_QUEUE_SHIFT;

use FIELD_GET here

> +}
> +
> +/**
> + * 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) {
> +		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)
> +{
> +	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);

Use RCT rule

> +
> +	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)
> +{
> +	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;

RCT

> +
> +	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;
> +
> +	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) {

no need for brackets

> +		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_ */
Marcin Szycik July 30, 2024, 1:19 p.m. UTC | #3 
On 30.07.2024 03:23, Song Yoong Siang wrote:
> From: Blanco Alcaine Hector <hector.blanco.alcaine@intel.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@intel.com>
> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.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
Song Yoong Siang Aug. 1, 2024, 7:20 a.m. UTC | #4 
On Tuesday, July 30, 2024 5:59 PM, Kurt Kanzenbach <kurt@linutronix.de> wrote:
>> --- 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 */
>
>Nit: Now, the MRQC register definitions are scattered over two files:
>igc_regs.h and igc.h. igc.h has
>
>#define IGC_MRQC_ENABLE_RSS_MQ		0x00000002
>#define IGC_MRQC_RSS_FIELD_IPV4_UDP	0x00400000
>#define IGC_MRQC_RSS_FIELD_IPV6_UDP	0x00800000
>
>Maybe combine them into a single location?
>

Hi Kurt Kanzenbach,

Thanks for your review comment.
Sure, I will try to combine them into single location.

>> +#define IGC_MRQC_ENABLE_MQ		0x00000000
>> +#define IGC_MRQC_ENABLE_MASK		GENMASK(2, 0)
>> +#define IGC_MRQC_DEFAULT_QUEUE_MASK	GENMASK(5, 3)
>> +#define IGC_MRQC_DEFAULT_QUEUE_SHIFT	3
>
>Nit: FIELD_GET() and FIELD_PREP() can help to get rid of the manual
>shifting. See below.
>

Noted.
[...]

>> +	return (mrqc & IGC_MRQC_DEFAULT_QUEUE_MASK) >>
>> +		IGC_MRQC_DEFAULT_QUEUE_SHIFT;
>
>Nit: return FIELD_GET(IGC_MRQC_DEFAULT_QUEUE_MASK, mrqc);
>

Noted.
[...]

>> +	mrqc &= ~IGC_MRQC_DEFAULT_QUEUE_MASK;
>> +	mrqc |= queue << IGC_MRQC_DEFAULT_QUEUE_SHIFT;
>
>Nit: mrqc |= FIELD_PREP(IGC_MRQC_DEFAULT_QUEUE_MASK, queue);
>

Noted.
[...]

>Thanks,
>Kurt

Thanks & Regards
Siang
Song Yoong Siang Aug. 1, 2024, 7:45 a.m. UTC | #5 
On Tuesday, July 30, 2024 6:09 PM, Drewek, Wojciech <wojciech.drewek@intel.com> wrote:

[...]

>> +	if (mrqe != IGC_MRQC_ENABLE_MQ && mrqe !=
>IGC_MRQC_ENABLE_RSS_MQ)
>> +		return false;
>
>just:
>return mrqe != IGC_MRQC_ENABLE_MQ && mrqe != IGC_MRQC_ENABLE_RSS_MQ
>

Hi Drewek Wojciech,
Thanks for your review comments.
I will improve the code accordingly in v2.
[...]

>> +	if (!igc_is_default_queue_supported(mrqc)) {
>> +		netdev_warn(adapter->netdev,
>> +			    "MRQ disabled: default RxQ is ignored.\n");
>
>Should we return an error here?

Yes, we should. I plan to refactor this patch to use
ethtool ntuple method, instead of sysfs.
I will consider your suggestion in v2.

>> +	}
>> +
>> +	return (mrqc & IGC_MRQC_DEFAULT_QUEUE_MASK) >>
>> +		IGC_MRQC_DEFAULT_QUEUE_SHIFT;
>
>use FIELD_GET here
>

Noted.
[...]

>> +	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);
>
>Use RCT rule
>

Noted.
[...]

>> +	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;
>
>RCT
>

Noted.
[...]

>> +	err = sysfs_create_group(&adapter->pdev->dev.kobj, &attr_group);
>> +	if (err) {
>
>no need for brackets
>

Noted.
[...]

Thanks & Regards
Siang
Song Yoong Siang Aug. 1, 2024, 7:56 a.m. UTC | #6 
On Tuesday, July 30, 2024 9:20 PM, Marcin Szycik <marcin.szycik@linux.intel.com> wrote:
>On 30.07.2024 03:23, Song Yoong Siang wrote:
>> From: Blanco Alcaine Hector <hector.blanco.alcaine@intel.com>
>>
>> This commit introduces the support to configure default Rx queue during
>
>Use imperative mood.
>

Hi Marcin Szycik,
Thanks for your review comments.
Sure, I will use imperative mood in the commit msg.
[...]

>> +/* MRQC register bit definitions */
>> +#define IGC_MRQC_ENABLE_MQ		0x00000000
>
>Just 0.
>

Noted.
[...]

>> +	if (queue > adapter->rss_queues - 1) {
>
>if (queue >= adapter->rss_queues)
>

Noted.
[...]

>> +static ssize_t default_rx_queue_show(struct device *dev,
>> +				     struct device_attribute *attr,
>> +				     char *buf)
>
>Why no igc_ prefix (and function doc)?
>

Sure. Will add igc prefix in the function name.
[...]

>> +static ssize_t default_rx_queue_store(struct device *dev,
>> +				      struct device_attribute *attr,
>> +				      const char *buf, size_t count)
>
>Ditto
>

Noted.
[...]

>> +	err = igc_set_default_rx_queue(adapter, default_rx_queue);
>> +	if (err < 0)
>> +		return -EINVAL;
>
>Why discard return error here?
>

Will use "return err" in v2 submission.
[...]

>
>Thanks,
>Marcin

Thanks & Regards
Siang
diff mbox series

Patch

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
+#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) {
+		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)
+{
+	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)
+{
+	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;
+
+	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_ */