* Re: [PATCH iwl-next,v1 2/3] igc: Add default Rx queue configuration via sysfs
2024-07-30 1:23 [PATCH iwl-next,v1 2/3] igc: Add default Rx queue configuration via sysfs Song Yoong Siang
@ 2024-07-30 9:58 ` Kurt Kanzenbach
2024-08-01 7:20 ` Song, Yoong Siang
2024-07-30 10:08 ` Wojciech Drewek
2024-07-30 13:19 ` [Intel-wired-lan] [PATCH iwl-next, v1 " Marcin Szycik
2 siblings, 1 reply; 7+ messages in thread
From: Kurt Kanzenbach @ 2024-07-30 9:58 UTC (permalink / raw)
To: Song Yoong Siang, Tony Nguyen, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Richard Cochran, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Vinicius Costa Gomes, Jonathan Corbet, Przemek Kitszel,
Shinas Rasheed, Kevin Tian, Brett Creeley, Blanco Alcaine Hector,
Joshua Hay, Sasha Neftin
Cc: intel-wired-lan, netdev, linux-kernel, bpf, linux-doc
[-- Attachment #1: Type: text/plain, Size: 4937 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH iwl-next,v1 2/3] igc: Add default Rx queue configuration via sysfs
2024-07-30 9:58 ` Kurt Kanzenbach
@ 2024-08-01 7:20 ` Song, Yoong Siang
0 siblings, 0 replies; 7+ messages in thread
From: Song, Yoong Siang @ 2024-08-01 7:20 UTC (permalink / raw)
To: Kurt Kanzenbach, Nguyen, Anthony L, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Richard Cochran,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Gomes, Vinicius, Jonathan Corbet,
Kitszel, Przemyslaw, Shinas Rasheed, Tian, Kevin, Brett Creeley,
Blanco Alcaine, Hector, Hay, Joshua A, Neftin, Sasha
Cc: intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
linux-doc@vger.kernel.org
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iwl-next,v1 2/3] igc: Add default Rx queue configuration via sysfs
2024-07-30 1:23 [PATCH iwl-next,v1 2/3] igc: Add default Rx queue configuration via sysfs Song Yoong Siang
2024-07-30 9:58 ` Kurt Kanzenbach
@ 2024-07-30 10:08 ` Wojciech Drewek
2024-08-01 7:45 ` Song, Yoong Siang
2024-07-30 13:19 ` [Intel-wired-lan] [PATCH iwl-next, v1 " Marcin Szycik
2 siblings, 1 reply; 7+ messages in thread
From: Wojciech Drewek @ 2024-07-30 10:08 UTC (permalink / raw)
To: Song Yoong Siang, Tony Nguyen, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Richard Cochran, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Vinicius Costa Gomes, Jonathan Corbet, Przemek Kitszel,
Shinas Rasheed, Kevin Tian, Brett Creeley, Blanco Alcaine Hector,
Joshua Hay, Sasha Neftin
Cc: intel-wired-lan, netdev, linux-kernel, bpf, linux-doc
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_ */
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH iwl-next,v1 2/3] igc: Add default Rx queue configuration via sysfs
2024-07-30 10:08 ` Wojciech Drewek
@ 2024-08-01 7:45 ` Song, Yoong Siang
0 siblings, 0 replies; 7+ messages in thread
From: Song, Yoong Siang @ 2024-08-01 7:45 UTC (permalink / raw)
To: Drewek, Wojciech, Nguyen, Anthony L, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Richard Cochran,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Gomes, Vinicius, Jonathan Corbet,
Kitszel, Przemyslaw, Shinas Rasheed, Tian, Kevin, Brett Creeley,
Blanco Alcaine, Hector, Hay, Joshua A, Neftin, Sasha
Cc: intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
linux-doc@vger.kernel.org
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-next, v1 2/3] igc: Add default Rx queue configuration via sysfs
2024-07-30 1:23 [PATCH iwl-next,v1 2/3] igc: Add default Rx queue configuration via sysfs Song Yoong Siang
2024-07-30 9:58 ` Kurt Kanzenbach
2024-07-30 10:08 ` Wojciech Drewek
@ 2024-07-30 13:19 ` Marcin Szycik
2024-08-01 7:56 ` Song, Yoong Siang
2 siblings, 1 reply; 7+ messages in thread
From: Marcin Szycik @ 2024-07-30 13:19 UTC (permalink / raw)
To: Song Yoong Siang, Tony Nguyen, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Richard Cochran, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Vinicius Costa Gomes, Jonathan Corbet, Przemek Kitszel,
Shinas Rasheed, Kevin Tian, Brett Creeley, Blanco Alcaine Hector,
Joshua Hay, Sasha Neftin
Cc: netdev, bpf, intel-wired-lan, linux-kernel, linux-doc
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
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [Intel-wired-lan] [PATCH iwl-next, v1 2/3] igc: Add default Rx queue configuration via sysfs
2024-07-30 13:19 ` [Intel-wired-lan] [PATCH iwl-next, v1 " Marcin Szycik
@ 2024-08-01 7:56 ` Song, Yoong Siang
0 siblings, 0 replies; 7+ messages in thread
From: Song, Yoong Siang @ 2024-08-01 7:56 UTC (permalink / raw)
To: Marcin Szycik, Nguyen, Anthony L, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Richard Cochran, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Gomes, Vinicius, Jonathan Corbet, Kitszel, Przemyslaw,
Shinas Rasheed, Tian, Kevin, Brett Creeley,
Blanco Alcaine, Hector, Hay, Joshua A, Neftin, Sasha
Cc: netdev@vger.kernel.org, bpf@vger.kernel.org,
intel-wired-lan@lists.osuosl.org, linux-kernel@vger.kernel.org,
linux-doc@vger.kernel.org
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
^ permalink raw reply [flat|nested] 7+ messages in thread