| Message ID | 20240730012312.775893-1-yoong.siang.song@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add Default Rx Queue Setting for igc driver | expand |
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
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_ */
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
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
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
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 --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_ */