* RE: [Intel-wired-lan] [PATCH iwl-next 2/6] i40e: Remove _t suffix from enum type names
From: Pucha, HimasekharX Reddy @ 2023-10-31 10:29 UTC (permalink / raw)
To: ivecera, netdev@vger.kernel.org
Cc: Eric Dumazet, dacampbe@redhat.com, Richard Cochran,
Brandeburg, Jesse, open list, Nguyen, Anthony L,
moderated list:INTEL ETHERNET DRIVERS, Keller, Jacob E,
Jakub Kicinski, Paolo Abeni, David S. Miller
In-Reply-To: <20231020193746.2274379-2-ivecera@redhat.com>
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Ivan Vecera
> Sent: Saturday, October 21, 2023 1:08 AM
> To: netdev@vger.kernel.org
> Cc: Eric Dumazet <edumazet@google.com>; dacampbe@redhat.com; Richard Cochran <richardcochran@gmail.com>; Brandeburg, Jesse <jesse.brandeburg@intel.com>; open list <linux-kernel@vger.kernel.org>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; moderated list:INTEL ETHERNET DRIVERS <intel-wired-lan@lists.osuosl.org>; Keller, Jacob E <jacob.e.keller@intel.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S. Miller <davem@davemloft.net>
> Subject: [Intel-wired-lan] [PATCH iwl-next 2/6] i40e: Remove _t suffix from enum type names
>
> Enum type names should not be suffixed by '_t'. Either to use
> 'typedef enum name name_t' to so plain 'name_t var' instead of
> 'enum name_t var'.
>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e.h | 4 ++--
> drivers/net/ethernet/intel/i40e/i40e_ptp.c | 6 +++---
> drivers/net/ethernet/intel/i40e/i40e_txrx.h | 4 ++--
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH iwl-next 1/6] i40e: Remove unused flags
From: Pucha, HimasekharX Reddy @ 2023-10-31 10:28 UTC (permalink / raw)
To: ivecera, netdev@vger.kernel.org
Cc: Eric Dumazet, dacampbe@redhat.com, Brandeburg, Jesse, open list,
Nguyen, Anthony L, moderated list:INTEL ETHERNET DRIVERS,
Keller, Jacob E, Jakub Kicinski, Paolo Abeni, David S. Miller
In-Reply-To: <20231020193746.2274379-1-ivecera@redhat.com>
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Ivan Vecera
> Sent: Saturday, October 21, 2023 1:08 AM
> To: netdev@vger.kernel.org
> Cc: Eric Dumazet <edumazet@google.com>; dacampbe@redhat.com; Brandeburg, Jesse <jesse.brandeburg@intel.com>; open list <linux-kernel@vger.kernel.org>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; moderated list:INTEL ETHERNET DRIVERS <intel-wired-lan@lists.osuosl.org>; Keller, Jacob E <jacob.e.keller@intel.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S. Miller <davem@davemloft.net>
> Subject: [Intel-wired-lan] [PATCH iwl-next 1/6] i40e: Remove unused flags
>
> The flag I40E_FLAG_RX_CSUM_ENABLED and I40E_HW_FLAG_DROP_MODE are
> set and never read. Remove them.
>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e.h | 57 +++++++++----------
> drivers/net/ethernet/intel/i40e/i40e_adminq.c | 4 +-
> drivers/net/ethernet/intel/i40e/i40e_main.c | 4 +-
> drivers/net/ethernet/intel/i40e/i40e_type.h | 3 +-
> 4 files changed, 31 insertions(+), 37 deletions(-)
>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH iwl-next 4/6] i40e: Use DECLARE_BITMAP for flags field in i40e_hw
From: Pucha, HimasekharX Reddy @ 2023-10-31 10:26 UTC (permalink / raw)
To: ivecera, netdev@vger.kernel.org
Cc: Eric Dumazet, dacampbe@redhat.com, Brandeburg, Jesse, open list,
Nguyen, Anthony L, moderated list:INTEL ETHERNET DRIVERS,
Keller, Jacob E, Jakub Kicinski, Paolo Abeni, David S. Miller
In-Reply-To: <20231020193746.2274379-4-ivecera@redhat.com>
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Ivan Vecera
> Sent: Saturday, October 21, 2023 1:08 AM
> To: netdev@vger.kernel.org
> Cc: Eric Dumazet <edumazet@google.com>; dacampbe@redhat.com; Brandeburg, Jesse <jesse.brandeburg@intel.com>; open list <linux-kernel@vger.kernel.org>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; moderated list:INTEL ETHERNET DRIVERS <intel-wired-lan@lists.osuosl.org>; Keller, Jacob E <jacob.e.keller@intel.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S. Miller <davem@davemloft.net>
> Subject: [Intel-wired-lan] [PATCH iwl-next 4/6] i40e: Use DECLARE_BITMAP for flags field in i40e_hw
>
> Convert flags field in i40e_hw from u64 to bitmap and its usage
> to use bit access functions and rename the field to 'caps' as
> this field describes capabilities that are set once on init and
> read many times later.
>
> Changes:
> - Convert "hw_ptr->flags & FLAG" to "test_bit(FLAG, ...)"
> - Convert "hw_ptr->flags |= FLAG" to "set_bit(FLAG, ...)"
> - Convert "hw_ptr->flags &= ~FLAG" to "clear_bit(FLAG, ...)"
> - Rename i40e_hw.flags to i40e_hw.caps
> - Rename i40e_set_hw_flags() to i40e_set_hw_caps()
> - Adjust caps names so they are prefixed by I40E_HW_CAP_ and existing
> _CAPABLE suffixes are stripped
>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e_adminq.c | 38 +++++++++----------
> drivers/net/ethernet/intel/i40e/i40e_common.c | 20 +++++-----
> drivers/net/ethernet/intel/i40e/i40e_dcb.c | 2 +-
> .../net/ethernet/intel/i40e/i40e_ethtool.c | 10 ++---
> drivers/net/ethernet/intel/i40e/i40e_main.c | 4 +-
> drivers/net/ethernet/intel/i40e/i40e_nvm.c | 10 ++---
> drivers/net/ethernet/intel/i40e/i40e_type.h | 22 ++++++-----
> 7 files changed, 55 insertions(+), 51 deletions(-)
>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
^ permalink raw reply
* Re: [PATCH net-next v2 8/9] microchip: lan865x: add driver support for Microchip's LAN865X MACPHY
From: Parthiban.Veerasooran @ 2023-10-31 10:25 UTC (permalink / raw)
To: krzk
Cc: netdev, devicetree, linux-kernel, linux-doc, Horatiu.Vultur,
Woojung.Huh, Nicolas.Ferre, UNGLinuxDriver, Thorsten.Kummermehr,
davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
conor+dt, corbet, Steen.Hegelund, rdunlap, horms, casper.casan,
andrew
In-Reply-To: <59767fb8-8b9a-472a-884c-009cb00ed0b9@kernel.org>
Hi Krzysztof,
On 24/10/23 5:27 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 23/10/2023 17:46, Parthiban Veerasooran wrote:
>> The LAN8650/1 is designed to conform to the OPEN Alliance 10BASE‑T1x
>> MAC‑PHY Serial Interface specification, Version 1.1. The IEEE Clause 4
>> MAC integration provides the low pin count standard SPI interface to any
>> microcontroller therefore providing Ethernet functionality without
>> requiring MAC integration within the microcontroller. The LAN8650/1
>> operates as an SPI client supporting SCLK clock rates up to a maximum of
>> 25 MHz. This SPI interface supports the transfer of both data (Ethernet
>> frames) and control (register access).
>>
>> By default, the chunk data payload is 64 bytes in size. A smaller payload
>> data size of 32 bytes is also supported and may be configured in the
>> Chunk Payload Size (CPS) field of the Configuration 0 (OA_CONFIG0)
>> register. Changing the chunk payload size requires the LAN8650/1 be reset
>> and shall not be done during normal operation.
>>
>> The Ethernet Media Access Controller (MAC) module implements a 10 Mbps
>> half duplex Ethernet MAC, compatible with the IEEE 802.3 standard.
>> 10BASE-T1S physical layer transceiver integrated into the LAN8650/1. The
>> PHY and MAC are connected via an internal Media Independent Interface
>> (MII).
>>
>> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
>> ---
>> MAINTAINERS | 6 +
>> drivers/net/ethernet/microchip/Kconfig | 11 +
>> drivers/net/ethernet/microchip/Makefile | 2 +
>> drivers/net/ethernet/microchip/lan865x.c | 415 +++++++++++++++++++++++
>> 4 files changed, 434 insertions(+)
>> create mode 100644 drivers/net/ethernet/microchip/lan865x.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 9580be91f5e9..1b1bd3218a2d 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -14001,6 +14001,12 @@ L: netdev@vger.kernel.org
>> S: Maintained
>> F: drivers/net/ethernet/microchip/lan743x_*
>>
>> +MICROCHIP LAN8650/1 10BASE-T1S MACPHY ETHERNET DRIVER
>> +M: Parthiban Veerasooran <parthiban.veerasooran@microchip.com>
>> +L: netdev@vger.kernel.org
>> +S: Maintained
>> +F: drivers/net/ethernet/microchip/lan865x.c
>> +
>> MICROCHIP LAN87xx/LAN937x T1 PHY DRIVER
>> M: Arun Ramadoss <arun.ramadoss@microchip.com>
>> R: UNGLinuxDriver@microchip.com
>> diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
>> index 329e374b9539..596caf59dea6 100644
>> --- a/drivers/net/ethernet/microchip/Kconfig
>> +++ b/drivers/net/ethernet/microchip/Kconfig
>> @@ -59,4 +59,15 @@ source "drivers/net/ethernet/microchip/lan966x/Kconfig"
>> source "drivers/net/ethernet/microchip/sparx5/Kconfig"
>> source "drivers/net/ethernet/microchip/vcap/Kconfig"
>>
>> +config LAN865X
>> + tristate "LAN865x support"
>> + depends on SPI
>> + depends on OA_TC6
>> + help
>> + Support for the Microchip LAN8650/1 Rev.B0 MACPHY Ethernet chip. It
>> + uses OPEN Alliance 10BASE-T1x Serial Interface specification.
>> +
>> + To compile this driver as a module, choose M here. The module will be
>> + called lan865x.
>
> That's odd indentation. Kconfig help goes with tab and two spaces.
Ah yes, will correct it.
>
>> +
>> endif # NET_VENDOR_MICROCHIP
>> diff --git a/drivers/net/ethernet/microchip/Makefile b/drivers/net/ethernet/microchip/Makefile
>> index bbd349264e6f..1fa4e15a067d 100644
>> --- a/drivers/net/ethernet/microchip/Makefile
>> +++ b/drivers/net/ethernet/microchip/Makefile
>> @@ -12,3 +12,5 @@ lan743x-objs := lan743x_main.o lan743x_ethtool.o lan743x_ptp.o
>> obj-$(CONFIG_LAN966X_SWITCH) += lan966x/
>> obj-$(CONFIG_SPARX5_SWITCH) += sparx5/
>> obj-$(CONFIG_VCAP) += vcap/
>
> ...
>
>> +static void lan865x_remove(struct spi_device *spi)
>> +{
>> + struct lan865x_priv *priv = spi_get_drvdata(spi);
>> +
>> + oa_tc6_exit(priv->tc6);
>> + unregister_netdev(priv->netdev);
>> + free_netdev(priv->netdev);
>> +}
>> +
>> +#ifdef CONFIG_OF
>
> Drop ifdef
Yes ok.
>
>> +static const struct of_device_id lan865x_dt_ids[] = {
>> + { .compatible = "microchip,lan865x" },
>> + { /* Sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, lan865x_dt_ids);
>> +#endif
>> +
>> +#ifdef CONFIG_ACPI
I think I need to remove this ifdef as well?
>> +static const struct acpi_device_id lan865x_acpi_ids[] = {
>> + { .id = "LAN865X",
>> + },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(acpi, lan865x_acpi_ids);
>> +#endif
>> +
>> +static struct spi_driver lan865x_driver = {
>> + .driver = {
>> + .name = DRV_NAME,
>> +#ifdef CONFIG_OF
>
> Drop ifdef
Yes ok.
>
>> + .of_match_table = lan865x_dt_ids,
>> +#endif
>> +#ifdef CONFIG_ACPI
>
> Why do you need this ifdef?
Ya it is not needed. Will remove it.
>
>> + .acpi_match_table = ACPI_PTR(lan865x_acpi_ids),
>> +#endif
>> + },
>> + .probe = lan865x_probe,
>> + .remove = lan865x_remove,
>> +};
>> +module_spi_driver(lan865x_driver);
>> +
>> +MODULE_DESCRIPTION(DRV_NAME " 10Base-T1S MACPHY Ethernet Driver");
>> +MODULE_AUTHOR("Parthiban Veerasooran <parthiban.veerasooran@microchip.com>");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("spi:" DRV_NAME);
>
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong.
Ok, will remove it.
Best Regards,
Parthiban V
>
>
> Best regards,
> Krzysztof
>
^ permalink raw reply
* [PATCH net-next V2] ptp: fix corrupted list in ptp_open
From: Edward Adam Davis @ 2023-10-31 10:25 UTC (permalink / raw)
To: habetsm.xilinx
Cc: davem, linux-kernel, netdev, reibax, richardcochran,
syzbot+df3f3ef31f60781fa911, syzkaller-bugs
There is no lock protection when writing ptp->tsevqs in ptp_open(),
ptp_release(), which can cause data corruption, use mutex lock to avoid this
issue.
Moreover, ptp_release() should not be used to release the queue in ptp_read(),
and it should be deleted together.
Reported-and-tested-by: syzbot+df3f3ef31f60781fa911@syzkaller.appspotmail.com
Fixes: 8f5de6fb2453 ("ptp: support multiple timestamp event readers")
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
drivers/ptp/ptp_chardev.c | 11 +++++++++--
drivers/ptp/ptp_clock.c | 3 +++
drivers/ptp/ptp_private.h | 1 +
3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 282cd7d24077..e31551d2697d 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -109,6 +109,9 @@ int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode)
struct timestamp_event_queue *queue;
char debugfsname[32];
+ if (mutex_lock_interruptible(&ptp->tsevq_mux))
+ return -ERESTARTSYS;
+
queue = kzalloc(sizeof(*queue), GFP_KERNEL);
if (!queue)
return -EINVAL;
@@ -132,15 +135,20 @@ int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode)
debugfs_create_u32_array("mask", 0444, queue->debugfs_instance,
&queue->dfs_bitmap);
+ mutex_unlock(&ptp->tsevq_mux);
return 0;
}
int ptp_release(struct posix_clock_context *pccontext)
{
struct timestamp_event_queue *queue = pccontext->private_clkdata;
+ struct ptp_clock *ptp =
+ container_of(pccontext->clk, struct ptp_clock, clock);
unsigned long flags;
if (queue) {
+ if (mutex_lock_interruptible(&ptp->tsevq_mux))
+ return -ERESTARTSYS;
debugfs_remove(queue->debugfs_instance);
pccontext->private_clkdata = NULL;
spin_lock_irqsave(&queue->lock, flags);
@@ -148,6 +156,7 @@ int ptp_release(struct posix_clock_context *pccontext)
spin_unlock_irqrestore(&queue->lock, flags);
bitmap_free(queue->mask);
kfree(queue);
+ mutex_unlock(&ptp->tsevq_mux);
}
return 0;
}
@@ -585,7 +594,5 @@ ssize_t ptp_read(struct posix_clock_context *pccontext, uint rdflags,
free_event:
kfree(event);
exit:
- if (result < 0)
- ptp_release(pccontext);
return result;
}
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 3d1b0a97301c..7930db6ec18d 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -176,6 +176,7 @@ static void ptp_clock_release(struct device *dev)
ptp_cleanup_pin_groups(ptp);
kfree(ptp->vclock_index);
+ mutex_destroy(&ptp->tsevq_mux);
mutex_destroy(&ptp->pincfg_mux);
mutex_destroy(&ptp->n_vclocks_mux);
/* Delete first entry */
@@ -247,6 +248,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
if (!queue)
goto no_memory_queue;
list_add_tail(&queue->qlist, &ptp->tsevqs);
+ mutex_init(&ptp->tsevq_mux);
queue->mask = bitmap_alloc(PTP_MAX_CHANNELS, GFP_KERNEL);
if (!queue->mask)
goto no_memory_bitmap;
@@ -356,6 +358,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
if (ptp->kworker)
kthread_destroy_worker(ptp->kworker);
kworker_err:
+ mutex_destroy(&ptp->tsevq_mux);
mutex_destroy(&ptp->pincfg_mux);
mutex_destroy(&ptp->n_vclocks_mux);
bitmap_free(queue->mask);
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index 52f87e394aa6..1525bd2059ba 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -44,6 +44,7 @@ struct ptp_clock {
struct pps_device *pps_source;
long dialed_frequency; /* remembers the frequency adjustment */
struct list_head tsevqs; /* timestamp fifo list */
+ struct mutex tsevq_mux; /* one process at a time reading the fifo */
struct mutex pincfg_mux; /* protect concurrent info->pin_config access */
wait_queue_head_t tsev_wq;
int defunct; /* tells readers to go away when clock is being removed */
--
2.25.1
^ permalink raw reply related
* RE: [Intel-wired-lan] [PATCH iwl-next 3/6] i40e: Use DECLARE_BITMAP for flags and hw_features fields in i40e_pf
From: Pucha, HimasekharX Reddy @ 2023-10-31 10:22 UTC (permalink / raw)
To: ivecera, netdev@vger.kernel.org
Cc: Eric Dumazet, dacampbe@redhat.com, Richard Cochran,
Brandeburg, Jesse, open list, Nguyen, Anthony L,
moderated list:INTEL ETHERNET DRIVERS, Keller, Jacob E,
Jakub Kicinski, Paolo Abeni, David S. Miller
In-Reply-To: <20231020193746.2274379-3-ivecera@redhat.com>
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Ivan Vecera
> Sent: Saturday, October 21, 2023 1:08 AM
> To: netdev@vger.kernel.org
> Cc: Eric Dumazet <edumazet@google.com>; dacampbe@redhat.com; Richard Cochran <richardcochran@gmail.com>; Brandeburg, Jesse <jesse.brandeburg@intel.com>; open list <linux-kernel@vger.kernel.org>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; moderated list:INTEL ETHERNET DRIVERS <intel-wired-lan@lists.osuosl.org>; Keller, Jacob E <jacob.e.keller@intel.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S. Miller <davem@davemloft.net>
> Subject: [Intel-wired-lan] [PATCH iwl-next 3/6] i40e: Use DECLARE_BITMAP for flags and hw_features fields in i40e_pf
>
> Convert flags and hw_features fields from i40e_pf from u32 to
> bitmaps and their usage to use bit access functions.
>
> Changes:
> - Convert "pf_ptr->(flags|hw_features) & FL" to "test_bit(FL, ...)"
> - Convert "pf_ptr->(flags|hw_features) |= FL" to "set_bit(FL, ...)"
> - Convert "pf_ptr->(flags|hw_features) &= ~FL" to "clear_bit(FL, ...)"
> - Rename flag field to bitno in i40e_priv_flags and adjust ethtool
> callbacks to work with flags bitmap
> - Rename flag names where '_ENABLED'->'_ENA' and '_DISABLED'->'_DIS'
> like in ice driver
>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e.h | 165 ++---
> drivers/net/ethernet/intel/i40e/i40e_dcb_nl.c | 24 +-
> .../net/ethernet/intel/i40e/i40e_debugfs.c | 4 +-
> .../net/ethernet/intel/i40e/i40e_ethtool.c | 209 ++++---
> drivers/net/ethernet/intel/i40e/i40e_main.c | 587 +++++++++---------
> drivers/net/ethernet/intel/i40e/i40e_ptp.c | 26 +-
> drivers/net/ethernet/intel/i40e/i40e_txrx.c | 20 +-
> drivers/net/ethernet/intel/i40e/i40e_txrx.h | 4 +-
> .../ethernet/intel/i40e/i40e_virtchnl_pf.c | 20 +-
> 9 files changed, 544 insertions(+), 515 deletions(-)
>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH iwl-next 5/6] i40e: Consolidate hardware capabilities
From: Pucha, HimasekharX Reddy @ 2023-10-31 10:19 UTC (permalink / raw)
To: ivecera, netdev@vger.kernel.org
Cc: Eric Dumazet, dacampbe@redhat.com, Richard Cochran,
Brandeburg, Jesse, open list, Nguyen, Anthony L,
moderated list:INTEL ETHERNET DRIVERS, Keller, Jacob E,
Jakub Kicinski, Paolo Abeni, David S. Miller
In-Reply-To: <20231020193746.2274379-5-ivecera@redhat.com>
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Ivan Vecera
> Sent: Saturday, October 21, 2023 1:08 AM
> To: netdev@vger.kernel.org
> Cc: Eric Dumazet <edumazet@google.com>; dacampbe@redhat.com; Richard Cochran <richardcochran@gmail.com>; Brandeburg, Jesse <jesse.brandeburg@intel.com>; open list <linux-kernel@vger.kernel.org>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; moderated list:INTEL ETHERNET DRIVERS <intel-wired-lan@lists.osuosl.org>; Keller, Jacob E <jacob.e.keller@intel.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S. Miller <davem@davemloft.net>
> Subject: [Intel-wired-lan] [PATCH iwl-next 5/6] i40e: Consolidate hardware capabilities
>
> Fields .caps in i40e_hw and .hw_features in i40e_pf both indicate
> capabilities provided by hardware. Move and merge i40e_pf.hw_features
> into i40e_hw.caps as this is more appropriate place for them and
> adjust their names to I40E_HW_CAP_... convention.
>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e.h | 27 +------
> .../net/ethernet/intel/i40e/i40e_ethtool.c | 36 ++++-----
> drivers/net/ethernet/intel/i40e/i40e_main.c | 78 +++++++++----------
> drivers/net/ethernet/intel/i40e/i40e_ptp.c | 6 +-
> drivers/net/ethernet/intel/i40e/i40e_txrx.h | 2 +-
> drivers/net/ethernet/intel/i40e/i40e_type.h | 18 +++++
> .../ethernet/intel/i40e/i40e_virtchnl_pf.c | 8 +-
> 7 files changed, 85 insertions(+), 90 deletions(-)
>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
^ permalink raw reply
* Re: [PATCH net-next 0/2] net: Use SMP threads for backlog NAPI (or optional).
From: Sebastian Andrzej Siewior @ 2023-10-31 10:14 UTC (permalink / raw)
To: Jakub Kicinski, Wander Lairson Costa, juri.lelli
Cc: linux-kernel, netdev, David S. Miller, Eric Dumazet, Paolo Abeni,
Peter Zijlstra, Thomas Gleixner, Come On Now
In-Reply-To: <20231016145337.4ZIt_sqL@linutronix.de>
On 2023-10-16 16:53:39 [+0200], To Jakub Kicinski wrote:
> On 2023-10-16 07:17:56 [-0700], Jakub Kicinski wrote:
> > On Mon, 16 Oct 2023 11:53:21 +0200 Sebastian Andrzej Siewior wrote:
> > > > Do we have reason to believe nobody uses RPS?
> > >
> > > Not sure what you relate to. I would assume that RPS is used in general
> > > on actual devices and not on loopback where backlog is used. But it is
> > > just an assumption.
> > > The performance drop, which I observed with RPS and stress-ng --udp, is
> > > within the same range with threads and IPIs (based on memory). I can
> > > re-run the test and provide actual numbers if you want.
> >
> > I was asking about RPS because with your current series RPS processing
> > is forced into threads. IDK how well you can simulate the kind of
> > workload which requires RPS. I've seen it used mostly on proxyies
> > and gateways. For proxies Meta's experiments with threaded NAPI show
> > regressions across the board. So "force-threading" RPS will most likely
> > also cause regressions.
>
> Understood.
>
> Wandere/ Juri: Do you have any benchmark/ workload where you would see
> whether RPS with IPI (now) vs RPS (this patch) shows any regression?
So I poked offlist other RH people and I've been told that they hardly
ever test RPS since the NICs these days have RSS in hardware.
Sebastian
^ permalink raw reply
* [PATCH net] net/tcp_sigpool: Fix some off by one bugs
From: Dan Carpenter @ 2023-10-31 9:51 UTC (permalink / raw)
To: Dmitry Safonov
Cc: Eric Dumazet, David S. Miller, David Ahern, Jakub Kicinski,
Paolo Abeni, Steen Hegelund, netdev, kernel-janitors
The "cpool_populated" variable is the number of elements in the cpool[]
array that have been populated. It is incremented in
tcp_sigpool_alloc_ahash() every time we populate a new element.
Unpopulated elements are NULL but if we have populated every element then
this code will read one element beyond the end of the array.
Fixes: 8c73b26315aa ("net/tcp: Prepare tcp_md5sig_pool for TCP-AO")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
From static analysis and review.
net/ipv4/tcp_sigpool.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/tcp_sigpool.c b/net/ipv4/tcp_sigpool.c
index 65a8eaae2fec..55b310a722c7 100644
--- a/net/ipv4/tcp_sigpool.c
+++ b/net/ipv4/tcp_sigpool.c
@@ -231,7 +231,7 @@ static void cpool_schedule_cleanup(struct kref *kref)
*/
void tcp_sigpool_release(unsigned int id)
{
- if (WARN_ON_ONCE(id > cpool_populated || !cpool[id].alg))
+ if (WARN_ON_ONCE(id >= cpool_populated || !cpool[id].alg))
return;
/* slow-path */
@@ -245,7 +245,7 @@ EXPORT_SYMBOL_GPL(tcp_sigpool_release);
*/
void tcp_sigpool_get(unsigned int id)
{
- if (WARN_ON_ONCE(id > cpool_populated || !cpool[id].alg))
+ if (WARN_ON_ONCE(id >= cpool_populated || !cpool[id].alg))
return;
kref_get(&cpool[id].kref);
}
@@ -256,7 +256,7 @@ int tcp_sigpool_start(unsigned int id, struct tcp_sigpool *c) __cond_acquires(RC
struct crypto_ahash *hash;
rcu_read_lock_bh();
- if (WARN_ON_ONCE(id > cpool_populated || !cpool[id].alg)) {
+ if (WARN_ON_ONCE(id >= cpool_populated || !cpool[id].alg)) {
rcu_read_unlock_bh();
return -EINVAL;
}
@@ -301,7 +301,7 @@ EXPORT_SYMBOL_GPL(tcp_sigpool_end);
*/
size_t tcp_sigpool_algo(unsigned int id, char *buf, size_t buf_len)
{
- if (WARN_ON_ONCE(id > cpool_populated || !cpool[id].alg))
+ if (WARN_ON_ONCE(id >= cpool_populated || !cpool[id].alg))
return -EINVAL;
return strscpy(buf, cpool[id].alg, buf_len);
--
2.42.0
^ permalink raw reply related
* Re: [PATCH RFC net-next v2 0/2] selftests/dpll: DPLL subsystem integration tests
From: Jiri Pirko @ 2023-10-31 9:47 UTC (permalink / raw)
To: Michal Michalik
Cc: netdev, vadim.fedorenko, arkadiusz.kubalewski, jonathan.lemon,
pabeni, poros, milena.olech, mschmidt, linux-clk, bvanassche,
kuba, davem, edumazet
In-Reply-To: <20231030165326.24453-1-michal.michalik@intel.com>
Mon, Oct 30, 2023 at 05:53:24PM CET, michal.michalik@intel.com wrote:
>The recently merged common DPLL interface discussed on a newsletter[1]
"newsletter"? Sounds a bit odd to me :)
>is introducing new, complex subsystem which requires proper integration
>testing - this patch adds core for such framework, as well as the
"Patchset" perhaps? Also, what do you mean by "core"? The sentence
sounds a bit weird to me.
>initial test cases. Framework does not require neither any special
>hardware nor any special system architecture.
>
>To properly test the DPLL subsystem this patch adds fake DPLL devices and it's
For patch desctiption, please stay within 72cols.
Also, "it's" is most probably wrong in this sentence.
>pins implementation to netdevsim. Creating netdevsim devices and adding ports
>to it register new DPLL devices and pins. First port of each netdevsim device
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This sentence does not make
sense to me. Pehaps rephrase a bit?
>acts as a entitiy which registers two DPLL devices: EEC and PPS DPLLs. First
typo: "entitiy"
>port also register the common pins: PPS and GNSS. Additionally each port
>register also RCLK (recovered clock) pin for itself. That allow us to check
>mutliple scenarios which might be problematic in real implementations (like
>different ordering etc.)
>
>Patch adds few helper scripts, which are:
>1) tools/testing/selftests/dpll/run_dpll_tests.sh
Please make this part of
tools/testing/selftests/drivers/net/netdevsim/
No special threat of dpll needed.
> Script is checking for all dependencies, creates temporary
> environment, installs required libraries and run all tests - can be
> used standalone
>2) tools/testing/selftests/dpll/ynlfamilyhandler.py˙
> Library for easier ynl use in the pytest framework - can be used
> standalone
>
>[1] https://lore.kernel.org/netdev/169494842736.21621.10730860855645661664.git-patchwork-notify@kernel.org/
>
>Changelog:
>v1 -> v2:
>- moved from separate module to implementation in netdevsim
>
>Michal Michalik (2):
> netdevsim: implement DPLL for subsystem selftests
> selftests/dpll: add DPLL system integration selftests
>
> drivers/net/Kconfig | 1 +
> drivers/net/netdevsim/Makefile | 2 +-
> drivers/net/netdevsim/dpll.c | 438 +++++++++++++++++++++++
> drivers/net/netdevsim/dpll.h | 81 +++++
> drivers/net/netdevsim/netdev.c | 20 ++
> drivers/net/netdevsim/netdevsim.h | 4 +
> tools/testing/selftests/Makefile | 1 +
> tools/testing/selftests/dpll/Makefile | 8 +
> tools/testing/selftests/dpll/__init__.py | 0
> tools/testing/selftests/dpll/config | 2 +
> tools/testing/selftests/dpll/consts.py | 34 ++
> tools/testing/selftests/dpll/dpll_utils.py | 109 ++++++
> tools/testing/selftests/dpll/requirements.txt | 3 +
> tools/testing/selftests/dpll/run_dpll_tests.sh | 75 ++++
> tools/testing/selftests/dpll/test_dpll.py | 414 +++++++++++++++++++++
> tools/testing/selftests/dpll/ynlfamilyhandler.py | 49 +++
> 16 files changed, 1240 insertions(+), 1 deletion(-)
> create mode 100644 drivers/net/netdevsim/dpll.c
> create mode 100644 drivers/net/netdevsim/dpll.h
> create mode 100644 tools/testing/selftests/dpll/Makefile
> create mode 100644 tools/testing/selftests/dpll/__init__.py
> create mode 100644 tools/testing/selftests/dpll/config
> create mode 100644 tools/testing/selftests/dpll/consts.py
> create mode 100644 tools/testing/selftests/dpll/dpll_utils.py
> create mode 100644 tools/testing/selftests/dpll/requirements.txt
> create mode 100755 tools/testing/selftests/dpll/run_dpll_tests.sh
> create mode 100644 tools/testing/selftests/dpll/test_dpll.py
> create mode 100644 tools/testing/selftests/dpll/ynlfamilyhandler.py
>
>--
>2.9.5
>
>base-commit: 55c900477f5b3897d9038446f72a281cae0efd86
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH iwl-next 6/6] i40e: Initialize hardware capabilities at single place
From: Pucha, HimasekharX Reddy @ 2023-10-31 9:41 UTC (permalink / raw)
To: ivecera, netdev@vger.kernel.org
Cc: Eric Dumazet, dacampbe@redhat.com, Brandeburg, Jesse, open list,
Nguyen, Anthony L, moderated list:INTEL ETHERNET DRIVERS,
Keller, Jacob E, Jakub Kicinski, Paolo Abeni, David S. Miller
In-Reply-To: <20231020193746.2274379-6-ivecera@redhat.com>
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Ivan Vecera
> Sent: Saturday, October 21, 2023 1:08 AM
> To: netdev@vger.kernel.org
> Cc: Eric Dumazet <edumazet@google.com>; dacampbe@redhat.com; Brandeburg, Jesse <jesse.brandeburg@intel.com>; open list <linux-kernel@vger.kernel.org>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; moderated list:INTEL ETHERNET DRIVERS <intel-wired-lan@lists.osuosl.org>; Keller, Jacob E <jacob.e.keller@intel.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S. Miller <davem@davemloft.net>
> Subject: [Intel-wired-lan] [PATCH iwl-next 6/6] i40e: Initialize hardware capabilities at single place
>
> Some i40e_hw.caps bits are set in i40e_set_hw_caps(), some of them
> in i40e_init_adminq() and the rest of them in i40e_sw_init().
> Consolidate the initialization to single proper place i40e_set_hw_caps().
>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e_adminq.c | 66 ++++++++++++++-----
> drivers/net/ethernet/intel/i40e/i40e_debug.h | 1 +
> drivers/net/ethernet/intel/i40e/i40e_main.c | 55 +---------------
> .../net/ethernet/intel/i40e/i40e_register.h | 1 +
> 4 files changed, 51 insertions(+), 72 deletions(-)
>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
^ permalink raw reply
* Re: [PATCH bpf-next] net, xdp: allow metadata > 32
From: Larysa Zaremba @ 2023-10-31 9:39 UTC (permalink / raw)
To: Jakub Kicinski
Cc: bpf, netdev, Alexei Starovoitov, Daniel Borkmann, David S. Miller,
Jesper Dangaard Brouer, Eric Dumazet, Magnus Karlsson,
Willem de Bruijn, Yunsheng Lin, Simon Horman, Maciej Fijalkowski,
John Fastabend, Aleksander Lobakin
In-Reply-To: <20231027130930.7d6014df@kernel.org>
On Fri, Oct 27, 2023 at 01:09:30PM -0700, Jakub Kicinski wrote:
> On Thu, 26 Oct 2023 18:56:59 +0200 Larysa Zaremba wrote:
> > static inline bool xdp_metalen_invalid(unsigned long metalen)
> > {
> > - return (metalen & (sizeof(__u32) - 1)) || (metalen > 32);
> > + typeof(metalen) meta_max;
>
> The use of typeof() looks a bit unnecessary..
>
You are probably right, will send v2 without it.
^ permalink raw reply
* Re: [PATCH net-next V2] ptp: fix corrupted list in ptp_open
From: Martin Habets @ 2023-10-31 9:28 UTC (permalink / raw)
To: Edward Adam Davis
Cc: richardcochran, davem, linux-kernel, netdev, reibax,
syzbot+df3f3ef31f60781fa911, syzkaller-bugs
In-Reply-To: <tencent_24C96E7894D0EBA2EDD2CFB87BB66EC02D0A@qq.com>
Please use a separate mail thread for a new patch revision.
See the section "Resending after review" in
Documentation/process/maintainer-netdev.rst.
Martin
On Tue, Oct 31, 2023 at 05:07:08AM +0800, Edward Adam Davis wrote:
> There is no lock protection when writing ptp->tsevqs in ptp_open(),
> ptp_release(), which can cause data corruption, use mutex lock to avoid this
> issue.
>
> Moreover, ptp_release() should not be used to release the queue in ptp_read(),
> and it should be deleted together.
>
> Reported-and-tested-by: syzbot+df3f3ef31f60781fa911@syzkaller.appspotmail.com
> Fixes: 8f5de6fb2453 ("ptp: support multiple timestamp event readers")
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
> drivers/ptp/ptp_chardev.c | 11 +++++++++--
> drivers/ptp/ptp_clock.c | 3 +++
> drivers/ptp/ptp_private.h | 1 +
> 3 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> index 282cd7d24077..e31551d2697d 100644
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -109,6 +109,9 @@ int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode)
> struct timestamp_event_queue *queue;
> char debugfsname[32];
>
> + if (mutex_lock_interruptible(&ptp->tsevq_mux))
> + return -ERESTARTSYS;
> +
> queue = kzalloc(sizeof(*queue), GFP_KERNEL);
> if (!queue)
> return -EINVAL;
> @@ -132,15 +135,20 @@ int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode)
> debugfs_create_u32_array("mask", 0444, queue->debugfs_instance,
> &queue->dfs_bitmap);
>
> + mutex_unlock(&ptp->tsevq_mux);
> return 0;
> }
>
> int ptp_release(struct posix_clock_context *pccontext)
> {
> struct timestamp_event_queue *queue = pccontext->private_clkdata;
> + struct ptp_clock *ptp =
> + container_of(pccontext->clk, struct ptp_clock, clock);
> unsigned long flags;
>
> if (queue) {
> + if (mutex_lock_interruptible(&ptp->tsevq_mux))
> + return -ERESTARTSYS;
> debugfs_remove(queue->debugfs_instance);
> pccontext->private_clkdata = NULL;
> spin_lock_irqsave(&queue->lock, flags);
> @@ -148,6 +156,7 @@ int ptp_release(struct posix_clock_context *pccontext)
> spin_unlock_irqrestore(&queue->lock, flags);
> bitmap_free(queue->mask);
> kfree(queue);
> + mutex_unlock(&ptp->tsevq_mux);
> }
> return 0;
> }
> @@ -585,7 +594,5 @@ ssize_t ptp_read(struct posix_clock_context *pccontext, uint rdflags,
> free_event:
> kfree(event);
> exit:
> - if (result < 0)
> - ptp_release(pccontext);
> return result;
> }
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index 3d1b0a97301c..7930db6ec18d 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -176,6 +176,7 @@ static void ptp_clock_release(struct device *dev)
>
> ptp_cleanup_pin_groups(ptp);
> kfree(ptp->vclock_index);
> + mutex_destroy(&ptp->tsevq_mux);
> mutex_destroy(&ptp->pincfg_mux);
> mutex_destroy(&ptp->n_vclocks_mux);
> /* Delete first entry */
> @@ -247,6 +248,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
> if (!queue)
> goto no_memory_queue;
> list_add_tail(&queue->qlist, &ptp->tsevqs);
> + mutex_init(&ptp->tsevq_mux);
> queue->mask = bitmap_alloc(PTP_MAX_CHANNELS, GFP_KERNEL);
> if (!queue->mask)
> goto no_memory_bitmap;
> @@ -356,6 +358,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
> if (ptp->kworker)
> kthread_destroy_worker(ptp->kworker);
> kworker_err:
> + mutex_destroy(&ptp->tsevq_mux);
> mutex_destroy(&ptp->pincfg_mux);
> mutex_destroy(&ptp->n_vclocks_mux);
> bitmap_free(queue->mask);
> diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
> index 52f87e394aa6..1525bd2059ba 100644
> --- a/drivers/ptp/ptp_private.h
> +++ b/drivers/ptp/ptp_private.h
> @@ -44,6 +44,7 @@ struct ptp_clock {
> struct pps_device *pps_source;
> long dialed_frequency; /* remembers the frequency adjustment */
> struct list_head tsevqs; /* timestamp fifo list */
> + struct mutex tsevq_mux; /* one process at a time reading the fifo */
> struct mutex pincfg_mux; /* protect concurrent info->pin_config access */
> wait_queue_head_t tsev_wq;
> int defunct; /* tells readers to go away when clock is being removed */
> --
> 2.25.1
>
^ permalink raw reply
* Re: [PATCH v2] selftests/net: synchronize udpgso_bench rx and tx
From: Paolo Abeni @ 2023-10-31 9:22 UTC (permalink / raw)
To: Lucas Karpinski, davem, edumazet, kuba, shuah
Cc: netdev, linux-kselftest, linux-kernel
In-Reply-To: <6ceki76bcv7qz6de5rxc26ot6aezdmeoz2g4ubtve7qwozmyyw@zibbg64wsdjp>
On Mon, 2023-10-30 at 13:40 -0400, Lucas Karpinski wrote:
> The sockets used by udpgso_bench_tx aren't always ready when
> udpgso_bench_tx transmits packets. This issue is more prevalent in -rt
> kernels, but can occur in both. Replace the hacky sleep calls with a
> function that checks whether the ports in the namespace are ready for
> use.
>
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Lucas Karpinski <lkarpins@redhat.com>
> ---
> https://lore.kernel.org/all/t7v6mmuobrbucyfpwqbcujtvpa3wxnsrc36cz5rr6kzzrzkwtj@toz6mr4ggnyp/
>
I almost forgot ...
> Changelog v2:
> - applied synchronization method suggested by Pablo
^^^^^ most common typo
since match 2022 ;)
Less irrelevant, please include the target tree in the next submission,
in this case 'net-next'.
Thanks,
Paolo
^ permalink raw reply
* Re: [PATCH v2] selftests/net: synchronize udpgso_bench rx and tx
From: Paolo Abeni @ 2023-10-31 9:18 UTC (permalink / raw)
To: Lucas Karpinski, davem, edumazet, kuba, shuah
Cc: netdev, linux-kselftest, linux-kernel
In-Reply-To: <6ceki76bcv7qz6de5rxc26ot6aezdmeoz2g4ubtve7qwozmyyw@zibbg64wsdjp>
On Mon, 2023-10-30 at 13:40 -0400, Lucas Karpinski wrote:
> The sockets used by udpgso_bench_tx aren't always ready when
> udpgso_bench_tx transmits packets. This issue is more prevalent in -rt
> kernels, but can occur in both. Replace the hacky sleep calls with a
> function that checks whether the ports in the namespace are ready for
> use.
>
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Lucas Karpinski <lkarpins@redhat.com>
> ---
> https://lore.kernel.org/all/t7v6mmuobrbucyfpwqbcujtvpa3wxnsrc36cz5rr6kzzrzkwtj@toz6mr4ggnyp/
>
> Changelog v2:
> - applied synchronization method suggested by Pablo
> - changed commit message to code
>
> tools/testing/selftests/net/udpgro.sh | 27 ++++++++++++++-----
> tools/testing/selftests/net/udpgro_bench.sh | 19 +++++++++++--
> tools/testing/selftests/net/udpgro_frglist.sh | 19 +++++++++++--
> 3 files changed, 54 insertions(+), 11 deletions(-)
>
> diff --git a/tools/testing/selftests/net/udpgro.sh b/tools/testing/selftests/net/udpgro.sh
> index 0c743752669a..04792a315729 100755
> --- a/tools/testing/selftests/net/udpgro.sh
> +++ b/tools/testing/selftests/net/udpgro.sh
> @@ -24,6 +24,22 @@ cleanup() {
> }
> trap cleanup EXIT
>
> +wait_local_port_listen()
> +{
> + local port="${1}"
> +
> + local port_hex
> + port_hex="$(printf "%04X" "${port}")"
> +
> + local i
Minor nit: I think the code would be more readable, if you will group
the variable together:
local port="${1}"
local port_hex
local i
port_hex="$(printf "%04X" "${port}")"
# ...
> + for i in $(seq 10); do
> + ip netns exec "${PEER_NS}" cat /proc/net/udp* | \
> + awk "BEGIN {rc=1} {if (\$2 ~ /:${port_hex}\$/) {rc=0; exit}} END {exit rc}" &&
> + break
> + sleep 0.1
> + done
> +}
Since you wrote the same function verbatim in 3 different files, I
think it would be better place it in separate, new, net_helper.sh file
and include such file from the various callers. Possibly additionally
rename the function as wait_local_udp_port_listen.
Thanks!
Paolo
^ permalink raw reply
* Re: [PATCH net] net: sched: fix warn on htb offloaded class creation
From: Paolo Abeni @ 2023-10-31 9:11 UTC (permalink / raw)
To: Maxim Mikityanskiy
Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Eric Dumazet, Jakub Kicinski, Tariq Toukan, Gal Pressman,
Saeed Mahameed
In-Reply-To: <ZTvBoQHfu23ynWf-@mail.gmail.com>
Hi,
I'm sorry for the late reply.
On Fri, 2023-10-27 at 16:57 +0300, Maxim Mikityanskiy wrote:
> I believe this is not the right fix.
>
> On Thu, 26 Oct 2023 at 17:36:48 +0200, Paolo Abeni wrote:
> > The following commands:
> >
> > tc qdisc add dev eth1 handle 2: root htb offload
> > tc class add dev eth1 parent 2: classid 2:1 htb rate 5mbit burst 15k
> >
> > yeld to a WARN in the HTB qdisc:
>
> Something is off here. These are literally the most basic commands one
> could invoke with HTB offload, I'm sure they worked. Is it something
> that broke recently? Tariq/Gal/Saeed, could you check them on a Mellanox
> NIC?
>
> >
> > WARNING: CPU: 2 PID: 1583 at net/sched/sch_htb.c:1959
> > CPU: 2 PID: 1583 Comm: tc Kdump: loaded 6.6.0-rc2.mptcp_7895773e5235+ #59
> > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-1.fc37 04/01/2014
> > RIP: 0010:htb_change_class+0x25c4/0x2e30 [sch_htb]
> > Code: 24 58 48 b8 00 00 00 00 00 fc ff df 48 89 ca 48 c1 ea 03 80 3c 02 00 0f 85 92 01 00 00 49 89 8c 24 b0 01 00 00 e9 77 fc ff ff <0f> 0b e9 15 ec ff ff 80 3d f8 35 00 00 00 0f 85 d4 f9 ff ff ba 32
> > RSP: 0018:ffffc900015df240 EFLAGS: 00010246
> > RAX: 0000000000000000 RBX: ffff88811b4ca000 RCX: ffff88811db42800
> > RDX: 1ffff11023b68502 RSI: ffffffffaf2e6a00 RDI: ffff88811db42810
> > RBP: ffff88811db45000 R08: 0000000000000001 R09: fffffbfff664bbc9
> > R10: ffffffffb325de4f R11: ffffffffb2d33748 R12: 0000000000000000
> > R13: ffff88811db43000 R14: ffff88811b4caaac R15: ffff8881252c0030
> > FS: 00007f6c1f126740(0000) GS:ffff88815aa00000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 000055dca8e5b4a8 CR3: 000000011bc7a006 CR4: 0000000000370ee0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > Call Trace:
> > <TASK>
> > tc_ctl_tclass+0x394/0xeb0
> > rtnetlink_rcv_msg+0x2f5/0xaa0
> > netlink_rcv_skb+0x12e/0x3a0
> > netlink_unicast+0x421/0x730
> > netlink_sendmsg+0x79e/0xc60
> > ____sys_sendmsg+0x95a/0xc20
> > ___sys_sendmsg+0xee/0x170
> > __sys_sendmsg+0xc6/0x170
> > do_syscall_64+0x58/0x80
> > entry_SYSCALL_64_after_hwframe+0x6e/0xd8
> >
> > The first command creates per TX queue pfifo qdiscs in
> > tc_modify_qdisc() -> htb_init() and grafts the pfifo to each dev_queue
> > via tc_modify_qdisc() -> qdisc_graft() -> htb_attach().
>
> Not exactly; it grafts pfifo to direct queues only. htb_attach_offload
> explicitly grafts noop to all the remaining queues.
num_direct_qdiscs == real_num_tx_queues:
https://elixir.bootlin.com/linux/latest/source/net/sched/sch_htb.c#L1101
pfifo will be configured on all the TX queues available at TC creation
time, right?
Lacking a mlx card with offload support I hack basic htb support in
netdevsim and I observe the splat on top of such device. I can as well
share the netdevsim patch - it will need some clean-up.
>
> > When the command completes, the qdisc_sleeping for each dev_queue is a
> > pfifo one. The next class creation will trigger the reported splat.
> >
> > Address the issue taking care of old non-builtin qdisc in
> > htb_change_class().
> >
> > Fixes: d03b195b5aa0 ("sch_htb: Hierarchical QoS hardware offload")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > net/sched/sch_htb.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
> > index 0d947414e616..dc682bd542b4 100644
> > --- a/net/sched/sch_htb.c
> > +++ b/net/sched/sch_htb.c
> > @@ -1955,8 +1955,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
> > qdisc_refcount_inc(new_q);
> > }
> > old_q = htb_graft_helper(dev_queue, new_q);
> > - /* No qdisc_put needed. */
> > - WARN_ON(!(old_q->flags & TCQ_F_BUILTIN));
> > + qdisc_put(old_q);
>
> We can get here after one of two cases above:
>
> 1. A new queue is allocated with TC_HTB_LEAF_ALLOC_QUEUE. It's supposed
> to have a noop qdisc by default (after htb_attach_offload).
So most likely the trivial netdevsim implementation I used was not good
enough.
Which constrains should respect TC_HTB_LEAF_ALLOC_QUEUE WRT the
returned qid value? should it in the (real_num_tx_queues,
num_tx_queues] range? Can HTB actually configure H/W shaping on
real_num_tx_queues?
I find no clear documentation WRT the above.
Thanks!
Paolo
^ permalink raw reply
* Re: [PATCH net-next 1/1] net: stmmac: add check for advertising linkmode request for set-eee
From: Russell King (Oracle) @ 2023-10-31 9:08 UTC (permalink / raw)
To: Gan, Yi Fang
Cc: Alexandre Torgue, Jose Abreu, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin,
netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Looi, Hong Aun, Voon, Weifeng,
Song, Yoong Siang, Ahmad Tarmizi, Noor Azura
In-Reply-To: <DM6PR11MB3306A3162F6A6086A4CBA049B9A0A@DM6PR11MB3306.namprd11.prod.outlook.com>
On Tue, Oct 31, 2023 at 08:44:23AM +0000, Gan, Yi Fang wrote:
> Hi Russell King,
>
> > Why should this functionality be specific to stmmac?
> This functionality is not specific to stmmac but other drivers can have their
> own implementation.
> (e.g. https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/qlogic/qede/qede_ethtool.c#L1855)
This is probably wrong (see below.)
>
> > Why do we need this?
> Current implementation will not take any effect if user enters unsupported value but user might
> not aware. With this, an error will be prompted if unsupported value is given.
Why can't the user read back what settings were actually set like the
other ethtool APIs? This is how ETHTOOL_GLINKSETTINGS works.
> > What is wrong with the checking and masking that phylib is doing?
> Nothing wrong with the phylib but there is no error return back to ethtool commands
> if unsupported value is given.
Maybe because that is the correct implementation?
> > Why should we trust the value in edata->supported provided by the user?
> The edata->supported is getting from the current setting and the value is set upon bootup.
> Users are not allowed to change it.
"not allowed" but there is nothing that prevents it. So an easy way to
bypass your check is:
struct ethtool_eee eeecmd;
eeecmd.cmd = ETHTOOL_GEEE;
send_ioctl(..., &eeecmd);
eeecmd.cmd = ETHTOOL_SEEE;
eeecmd.supported = ~0;
eeecmd.advertised = ~0;
error = send_ioctl(..., &eeecmd);
and that won't return any error. So your check is weak at best, and
relies upon the user doing the right thing.
> > Sorry, but no. I see no reason that this should be done, especially not in the stmmac driver.
> I understand your reasoning. From your point of view, is this kind of error message/ error handling
> not needed?
It is not - ethtool APIs don't return errors if the advertise mask is
larger than the supported mask - they merely limit to what is supported
and set that. When subsequently querying the settings, they return what
is actually set (so the advertise mask will always be a subset of the
supported mask at that point.)
So, if in userspace you really want to know if some modes were dropped,
then you have to do a set-get-check sequence.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply
* RE: [PATCH net-next 1/1] net: stmmac: add check for advertising linkmode request for set-eee
From: Gan, Yi Fang @ 2023-10-31 8:44 UTC (permalink / raw)
To: Russell King
Cc: Alexandre Torgue, Jose Abreu, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin,
netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Looi, Hong Aun, Voon, Weifeng,
Song, Yoong Siang, Ahmad Tarmizi, Noor Azura
In-Reply-To: <ZTtpBCZuB+bjVt9D@shell.armlinux.org.uk>
Hi Russell King,
Why should this functionality be specific to stmmac?
This functionality is not specific to stmmac but other drivers can have their
own implementation.
(e.g. https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/qlogic/qede/qede_ethtool.c#L1855)
Why do we need this?
Current implementation will not take any effect if user enters unsupported value but user might
not aware. With this, an error will be prompted if unsupported value is given.
What is wrong with the checking and masking that phylib is doing?
Nothing wrong with the phylib but there is no error return back to ethtool commands
if unsupported value is given.
Why should we trust the value in edata->supported provided by the user?
The edata->supported is getting from the current setting and the value is set upon bootup.
Users are not allowed to change it.
Sorry, but no. I see no reason that this should be done, especially not in the stmmac driver.
I understand your reasoning. From your point of view, is this kind of error message/ error handling
not needed?
Best Regards,
Fang
> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Friday, October 27, 2023 3:39 PM
> To: Gan, Yi Fang <yi.fang.gan@intel.com>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose Abreu
> <joabreu@synopsys.com>; David S . Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-
> md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Looi, Hong Aun <hong.aun.looi@intel.com>; Voon,
> Weifeng <weifeng.voon@intel.com>; Song, Yoong Siang
> <yoong.siang.song@intel.com>; Ahmad Tarmizi, Noor Azura
> <noor.azura.ahmad.tarmizi@intel.com>
> Subject: Re: [PATCH net-next 1/1] net: stmmac: add check for advertising
> linkmode request for set-eee
>
> On Fri, Oct 27, 2023 at 02:50:54PM +0800, Gan Yi Fang wrote:
> > From: Noor Azura Ahmad Tarmizi <noor.azura.ahmad.tarmizi@intel.com>
> >
> > Add check for advertising linkmode set request with what is currently
> > being supported by PHY before configuring the EEE. Unsupported setting
> > will be rejected and a message will be prompted. No checking is
> > required while setting the EEE to off.
>
> Why should this functionality be specific to stmmac?
>
> Why do we need this?
>
> What is wrong with the checking and masking that phylib is doing?
>
> Why should we trust the value in edata->supported provided by the user?
>
> Sorry, but no. I see no reason that this should be done, especially not in the
> stmmac driver.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply
* Re: [PATCH net-next v2 7/9] net: ethernet: oa_tc6: implement data transaction interface
From: Parthiban.Veerasooran @ 2023-10-31 8:26 UTC (permalink / raw)
To: andrew
Cc: davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
conor+dt, corbet, Steen.Hegelund, rdunlap, horms, casper.casan,
netdev, devicetree, linux-kernel, linux-doc, Horatiu.Vultur,
Woojung.Huh, Nicolas.Ferre, UNGLinuxDriver, Thorsten.Kummermehr
In-Reply-To: <9b7a5ed9-840f-4346-a168-e538a8477714@lunn.ch>
Hi Andrew,
On 24/10/23 7:37 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> +static u16 oa_tc6_prepare_empty_chunk(struct oa_tc6 *tc6, u8 *buf, u8 cp_count)
>> +{
>> + u32 hdr;
>> +
>> + /* Prepare empty chunks used for getting interrupt information or if
>> + * receive data available.
>> + */
>> + for (u8 i = 0; i < cp_count; i++) {
>> + hdr = FIELD_PREP(DATA_HDR_DNC, 1);
>> + hdr |= FIELD_PREP(DATA_HDR_P, oa_tc6_get_parity(hdr));
>> + *(__be32 *)&buf[i * (tc6->cps + TC6_HDR_SIZE)] = cpu_to_be32(hdr);
>> + memset(&buf[TC6_HDR_SIZE + (i * (tc6->cps + TC6_HDR_SIZE))], 0,
>> + tc6->cps);
>> + }
>
> This is not simple, and its the sort of code which makes me wounder if
> its gone off the end of the buffer. It would be good to find somebody
> internally within Microchip to review this code.
I think, I don't need to do memset here as the header itself doesn't
describe any valid information about the payload except data not control
as it is an empty chunk and no matter what the payload contains. Apart
from that I don't get what's wrong here, anyway I will ask our internal
reviewers to review the code.
>
>> +static void oa_tc6_rx_eth_ready(struct oa_tc6 *tc6)
>> +{
>> + struct sk_buff *skb;
>> +
>> + /* Send the received ethernet packet to network layer */
>> + skb = netdev_alloc_skb(tc6->netdev, tc6->rxd_bytes + NET_IP_ALIGN);
>> + if (!skb) {
>> + tc6->netdev->stats.rx_dropped++;
>> + netdev_dbg(tc6->netdev, "Out of memory for rx'd frame");
>
> You can just return here, and skip the else. Less indentation is
> better, it generally makes the code more readable.
Ah yes. Noted.
>
>> + } else {
>> + skb_reserve(skb, NET_IP_ALIGN);
>> + memcpy(skb_put(skb, tc6->rxd_bytes), &tc6->eth_rx_buf[0],
>> + tc6->rxd_bytes);
>> + skb->protocol = eth_type_trans(skb, tc6->netdev);
>> + tc6->netdev->stats.rx_packets++;
>> + tc6->netdev->stats.rx_bytes += tc6->rxd_bytes;
>> + /* 0 for NET_RX_SUCCESS and 1 for NET_RX_DROP */
>> + if (netif_rx(skb))
>> + tc6->netdev->stats.rx_dropped++;
>
> Rather than have a comment do:
Ah ok, will do it.
>
> if (netif_rx(skb) == NET_RX_DROP)
> tc6->netdev->stats.rx_dropped++;
>
>
>> +static void oa_tc6_rx_eth_complete2(struct oa_tc6 *tc6, u8 *payload, u32 ftr)
>> +{
>> + u16 ebo;
>
> What does ftr and ebo mean? Its really hard to read this code because
> the names are not really meaningful.
Ok, ftr -> footer and ebo -> end_byte_offset. Will update in the next
revision.
>
>> +
>> + if (FIELD_GET(DATA_FTR_EV, ftr))
>> + ebo = FIELD_GET(DATA_FTR_EBO, ftr) + 1;
>> + else
>> + ebo = tc6->cps;
>> +
>> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[0], ebo);
>> + tc6->rxd_bytes += ebo;
>> + if (FIELD_GET(DATA_FTR_EV, ftr)) {
>> + /* If EV set then send the received ethernet frame to n/w */
>> + oa_tc6_rx_eth_ready(tc6);
>> + tc6->rxd_bytes = 0;
>> + tc6->rx_eth_started = false;
>> + }
>> +}
>> +
>> +static void oa_tc6_rx_eth_complete1(struct oa_tc6 *tc6, u8 *payload, u32 ftr)
>> +{
>> + u16 ebo;
>> + u16 sbo;
>> +
>> + sbo = FIELD_GET(DATA_FTR_SWO, ftr) * 4;
>> + ebo = FIELD_GET(DATA_FTR_EBO, ftr) + 1;
>> +
>> + if (ebo <= sbo) {
>> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[0], ebo);
>> + tc6->rxd_bytes += ebo;
>> + oa_tc6_rx_eth_ready(tc6);
>> + tc6->rxd_bytes = 0;
>> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[sbo],
>> + tc6->cps - sbo);
>> + tc6->rxd_bytes += (tc6->cps - sbo);
>> + } else {
>> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[sbo],
>> + ebo - sbo);
>> + tc6->rxd_bytes += (ebo - sbo);
>> + oa_tc6_rx_eth_ready(tc6);
>> + tc6->rxd_bytes = 0;
>> + }
>> +}
>> +
>> +static void oa_tc6_start_rx_eth(struct oa_tc6 *tc6, u8 *payload, u32 ftr)
>> +{
>> + u16 sbo;
>> +
>> + tc6->rxd_bytes = 0;
>> + tc6->rx_eth_started = true;
>> + sbo = FIELD_GET(DATA_FTR_SWO, ftr) * 4;
>> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], &payload[sbo], tc6->cps - sbo);
>> + tc6->rxd_bytes += (tc6->cps - sbo);
>> +}
>> +
>> +static u32 oa_tc6_get_footer(struct oa_tc6 *tc6, u8 *buf, u8 cp_num)
>> +{
>> + __be32 ftr;
>> +
>> + ftr = *(__be32 *)&buf[tc6->cps + (cp_num * (tc6->cps + TC6_FTR_SIZE))];
>> +
>> + return be32_to_cpu(ftr);
>> +}
>> +
>> +static void oa_tc6_update_txc_rca(struct oa_tc6 *tc6, u32 ftr)
>> +{
>> + tc6->txc = FIELD_GET(DATA_FTR_TXC, ftr);
>> + tc6->rca = FIELD_GET(DATA_FTR_RCA, ftr);
>> +}
>> +
>> +static int oa_tc6_check_ftr_errors(struct oa_tc6 *tc6, u32 ftr)
>> +{
>> + /* Check for footer parity error */
>> + if (oa_tc6_get_parity(ftr)) {
>> + net_err_ratelimited("%s: Footer parity error\n",
>> + tc6->netdev->name);
>> + return FTR_ERR;
>> + }
>> + /* If EXST set in the footer then read STS0 register to get the
>> + * status information.
>> + */
>> + if (FIELD_GET(DATA_FTR_EXST, ftr)) {
>> + if (oa_tc6_process_exst(tc6))
>> + net_err_ratelimited("%s: Failed to process EXST\n",
>> + tc6->netdev->name);
>> + return FTR_ERR;
>> + }
>> + if (FIELD_GET(DATA_FTR_HDRB, ftr)) {
>> + net_err_ratelimited("%s: Footer eeceived header bad\n",
>> + tc6->netdev->name);
>> + return FTR_ERR;
>> + }
>> + if (!FIELD_GET(DATA_FTR_SYNC, ftr)) {
>> + net_err_ratelimited("%s: Footer configuration unsync\n",
>> + tc6->netdev->name);
>> + return FTR_ERR;
>> + }
>> + return FTR_OK;
>> +}
>> +
>> +static void oa_tc6_drop_rx_eth(struct oa_tc6 *tc6)
>> +{
>> + tc6->rxd_bytes = 0;
>> + tc6->rx_eth_started = false;
>> + tc6->netdev->stats.rx_dropped++;
>> + net_err_ratelimited("%s: Footer frame drop\n",
>> + tc6->netdev->name);
>> +}
>> +
>> +static int oa_tc6_process_rx_chunks(struct oa_tc6 *tc6, u8 *buf, u16 len)
>> +{
>> + u8 cp_count;
>> + u8 *payload;
>> + u32 ftr;
>> + int ret;
>> +
>> + /* Calculate the number of chunks received */
>> + cp_count = len / (tc6->cps + TC6_FTR_SIZE);
>> +
>> + for (u8 i = 0; i < cp_count; i++) {
>> + /* Get the footer and payload */
>> + ftr = oa_tc6_get_footer(tc6, buf, i);
>> + payload = &buf[(i * (tc6->cps + TC6_FTR_SIZE))];
>
> This would be more readable:
>
> /* Calculate the number of chunks received */
> chunks = len / (tc6->cps + TC6_FTR_SIZE);
>
> for (u8 chunk = 0; chunk < chunks; chunk++) {
> /* Get the footer and payload */
> ftr = oa_tc6_get_footer(tc6, buf, chunk);
> payload = &buf[(chunk * (tc6->cps + TC6_FTR_SIZE))];
>
> etc.
Ok thanks for the input. Will do it.
>
> And maybe move most of this code into a function
> oa_tc6_process_rx_chunk(). With lots of small functions with good
> names, you need less comments.
Yes sure. Will do it.
>
>
>> + /* Check for data valid */
>> + if (FIELD_GET(DATA_FTR_DV, ftr)) {
>> + /* Check whether both start valid and end valid are in a
>> + * single chunk payload means a single chunk payload may
>> + * contain an entire ethernet frame.
>> + */
>> + if (FIELD_GET(DATA_FTR_SV, ftr) &&
>> + FIELD_GET(DATA_FTR_EV, ftr)) {
>
>
> if (FIELD_GET(DATA_FOOTER_START_VALID, footer) &&
> FIELD_GET(DATA_FOOTER_END_VALID, footer)) {
>
> Don't you think that is more readable?
Yes it is more readable now. Will update in the next revision.
>
>> +static void oa_tc6_prepare_tx_chunks(struct oa_tc6 *tc6, u8 *buf,
>> + struct sk_buff *skb)
>> +{
>> + bool frame_started = false;
>> + u16 copied_bytes = 0;
>> + u16 copy_len;
>> + u32 hdr;
>> +
>> + /* Calculate the number tx credit counts needed to transport the tx
>> + * ethernet frame.
>> + */
>> + tc6->txc_needed = (skb->len / tc6->cps) + ((skb->len % tc6->cps) ? 1 : 0);
>
> Why call it a credit here, but a chunk when receiving?
I named as the tx path always gives the number of tx credits counts can
be enqueued for transfer. So used txc needed name to represent. Ok I
will change it as chunks_needed.
>
>> +static int oa_tc6_perform_spi_xfer(struct oa_tc6 *tc6)
>> +{
>> + bool do_tx_again;
>> + u16 total_len;
>> + u16 rca_len;
>> + u16 tx_len;
>> + int ret;
>> +
>> + do {
>> + do_tx_again = false;
>> + rca_len = 0;
>> + tx_len = 0;
>> +
>> + /* In case of an interrupt, perform an empty chunk transfer to
>> + * know the purpose of the interrupt. Interrupt may occur in
>> + * case of RCA (Receive Chunk Available) and TXC (Transmit
>> + * Credit Count). Both will occur if they are not indicated
>> + * through the previous footer.
>> + */
>> + if (tc6->int_flag) {
>> + tc6->int_flag = false;
>> + total_len = oa_tc6_prepare_empty_chunk(tc6,
>> + tc6->spi_tx_buf,
>> + 1);
>> + } else {
>> + /* Calculate the transfer length */
>> + if (tc6->tx_flag && tc6->txc) {
>> + tx_len = oa_tc6_calculate_tx_len(tc6);
>> + memcpy(&tc6->spi_tx_buf[0],
>> + &tc6->eth_tx_buf[tc6->tx_pos], tx_len);
>> + }
>> +
>> + if (tc6->rca)
>> + rca_len = oa_tc6_calculate_rca_len(tc6, tx_len);
>> +
>> + total_len = tx_len + rca_len;
>> + }
>> + ret = oa_tc6_spi_transfer(tc6->spi, tc6->spi_tx_buf,
>> + tc6->spi_rx_buf, total_len);
>> + if (ret)
>> + return ret;
>> + /* Process the rxd chunks to get the ethernet frame or status */
>> + ret = oa_tc6_process_rx_chunks(tc6, tc6->spi_rx_buf, total_len);
>> + if (ret)
>> + return ret;
>> + if (tc6->tx_flag) {
>> + tc6->tx_pos += tx_len;
>> + tc6->txc_needed = tc6->txc_needed -
>> + (tx_len / (tc6->cps + TC6_HDR_SIZE));
>> + /* If the complete ethernet frame is transmitted then
>> + * return the skb and update the details to n/w layer.
>> + */
>> + if (!tc6->txc_needed)
>> + oa_tc6_tx_eth_complete(tc6);
>> + else if (tc6->txc)
>> + /* If txc is available again and updated from
>> + * the previous footer then perform tx again.
>> + */
>> + do_tx_again = true;
>> + }
>> +
>> + /* If rca is updated from the previous footer then perform empty
>> + * tx to receive ethernet frame.
>> + */
>> + if (tc6->rca)
>> + do_tx_again = true;
>> + } while (do_tx_again);
>
> The coding standard say:
>
> Functions should be short and sweet, and do just one thing. They
> should fit on one or two screenfuls of text (the ISO/ANSI screen size
> is 80x24, as we all know), and do one thing and do that well.
>
> This is too long, and does too many things.
Sure, will split into multiple small functions.
Best Regards,
Parthiban V
>
> Andrew
^ permalink raw reply
* Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select
From: Oleksij Rempel @ 2023-10-31 7:28 UTC (permalink / raw)
To: Andrew Lunn
Cc: Vladimir Oltean, Ante Knezic, conor+dt, UNGLinuxDriver, davem,
devicetree, edumazet, f.fainelli, krzysztof.kozlowski+dt, kuba,
linux-kernel, marex, netdev, pabeni, robh+dt, woojung.huh
In-Reply-To: <aad5ac41-3c05-421d-a483-0546b579585c@lunn.ch>
On Tue, Oct 31, 2023 at 02:00:05AM +0100, Andrew Lunn wrote:
> > So, my opinion is that although what Oleksij would like to see is
> > admirable, I don't think that the REF_CLK direction is a matter of RMII
> > MAC vs PHY role, and thus, we wouldn't need to change "rmii" to "rev-rmii"
> > and cause breakage everywhere. It's just that - a matter of REF_CLK
> > direction. It's true, though, that this is a generic problem and that
> > the generic bindings for RMII that we currently have are under-specified.
> > We could try to devise an extended RMII binding which makes it clear for
> > both the MAC and the PHY who is responsible to drive this signal. You
> > are not attempting that, you are just coming up with yet another
> > vendor-specific MAC property which solves a generic problem. I can't say
> > I am completely opposed to that, either, which is why I haven't really
> > spoken out against it. The PHY maintainers would also have to weigh in,
> > and not all of them are CCed here.
>
> I would recommend looking around other PHYs and find a property which
> does what you want, and copy it.
>
> We do have all sorts of properties. There are some to enable the
> REF_CLK out of the PHY. Some to disable the REF_CLK out, some to
> disable it when the link is down, some to indicate what frequency it
> should tick at, etc.
>
> If you want to go the extra mile, maybe you can make a summary of all
> these properties, and maybe we can produce a guide line for what we
> want the properties to be called going forward.
>
> > I am afraid that creating a CCF style binding for REF_CLK will be an
> > enormous hammer for a very small nail and will see very limited adoption
> > to other drivers, but I might as well be wrong about it. Compatibility
> > between RMII MACs and PHYs which may or may not be CCF-ready might also
> > be a concern.
>
> I also don't think using the CCF makes too much sense, except for
> where the SoC provides the lock, and already has a CCF covering it.
>
> I would also be hesitant to add more dependencies between the MAC and
> the PHY. The DT often has circular dependencies and we have had issues
> with probing being deferred because the core does not always
> understand these circular dependencies.
Heh, this are unsolved problems making me pain in different projects.
Here are some real life examples, which are unsolved in one or another project
and till now didn't went mainline:
1. In scenarios where PHYs require an RMII clock from the MAC, initialization
becomes complex. This is often resolved through bootloader and kernel
modifications. Right now it kind of works and postponed until it will make
real pain :)
2. Complexity increases in designs with multiple PHYs used by different MACs
but connected to one MDIO bus. Same is here, there was already some
regressions but the pain is still not enough for making things right.
3. For some MACs like STMMAC, configuration is challenging without an external
clock from the PHY. For example, VLAN configuration isn't possible with EEE
enabled unless deep power saving states are disabled during register access.
If I remember it correctly, there was floating discussions and patches trying
to address similar issues.
Transferring these issues to KSZ8863, we might face difficulties configuring
STMMAC if KSZ8863, acting as the clock provider, isn't enabled early before MAC
driver probing, a tricky scenario in the DSA framework.
Working on deep sleep states for the KSZ switch driver, I find that dynamic
clock control, potentially offered by CCF, could be quite handy.
Please do not see this answer as a request to Ante for complex rework. It's
more of a red flag notifying that the clocking issue is still unsolved, and
someone (may be me), sooner or later, will have enough motivation to jump into
this wasp nest :)
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH] staging: Revert "staging: qlge: Retire the driver"
From: Greg Kroah-Hartman @ 2023-10-31 7:00 UTC (permalink / raw)
To: Benjamin Poirier
Cc: Jakub Kicinski, Kira, David S. Miller, Eric Dumazet, Paolo Abeni,
Jonathan Corbet, Manish Chopra, GR-Linux-NIC-Dev, Coiby Xu,
James E.J. Bottomley, Helge Deller, Sven Joachim, Ian Kent,
netdev, linux-doc, linux-kernel, linux-parisc, linux-staging
In-Reply-To: <ZT_YntDOYEdlpx5x@d3>
On Mon, Oct 30, 2023 at 12:33:55PM -0400, Benjamin Poirier wrote:
> On 2023-10-30 16:25 +0100, Greg Kroah-Hartman wrote:
> > On Tue, Oct 31, 2023 at 02:04:00AM +1100, Benjamin Poirier wrote:
> > > This reverts commit 875be090928d19ff4ae7cbaadb54707abb3befdf.
> > >
> > > On All Hallows' Eve, fear and cower for it is the return of the undead
> > > driver.
> > >
> > > There was a report [1] from a user of a QLE8142 device. They would like for
> > > the driver to remain in the kernel. Therefore, revert the removal of the
> > > qlge driver.
> > >
> > > [1] https://lore.kernel.org/netdev/566c0155-4f80-43ec-be2c-2d1ad631bf25@gmail.com/
> >
> > Who's going to maintain this?
>
> I was planning to update the MAINTAINERS entry to
> S: Orphan
> when moving it back to drivers/net/. Would you prefer that I do that
> change in a second patch right after the revert in staging? That would
> certainly make things clearer.
I would prefer not having orphaned code in the kernel tree. Again, who
is going to support this? It was dropped because there is no owner and
the company doesn't care anymore. We can't add it back if there is no
one who will do the real-work to fix it up and get it out of staging.
Just magically moving it there isn't going to be a solution either.
> > > Reported by: Kira <nyakov13@gmail.com>
> > > Signed-off-by: Benjamin Poirier <benjamin.poirier@gmail.com>
> > > ---
> > >
> > > Notes:
> > > Once the removal and revert show up in the net-next tree, I plan to send a
> > > followup patch to move the driver to drivers/net/ as discussed earlier:
> > > https://lore.kernel.org/netdev/20231019074237.7ef255d7@kernel.org/
> >
> > are you going to be willing to maintain this and keep it alive?
>
> No.
>
> > I'm all this, if you want to, but I would like it out of staging. So
>
> I'd like it out of staging as well. Since nobody wants to maintain it, I
> think it should be deleted. However, my understanding is that Jakub is
> willing to take it back into drivers/net/ as-is given that there is at
> least one user. Jakub, did I understand that correctly?
>
> > how about applying this, and a follow-on one that moves it there once
> > -rc1 is out? And it probably should be in the 'net' tree, as you don't
> > want 6.7 to come out without the driver at all, right?
>
> Right about making sure 6.7 includes the driver. The 'net' tree is
> usually for fixes hence why I would send to net-next. So the driver
> would still be in staging for 6.7 (if you include the revert in your
> 6.7-rc1 submission) and would be back in drivers/net/ for 6.8.
Let's wait until 6.7-rc1 is out and then, if the netdev developers want
to take this on, they can revert it and move it to drivers/net/.
But right now, my tree is frozen, it's the middle of the merge window,
let's wait 2 weeks please.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH bpf-next v8 02/10] bpf, net: introduce bpf_struct_ops_desc.
From: Martin KaFai Lau @ 2023-10-31 6:40 UTC (permalink / raw)
To: thinker.li, bpf, ast, song, kernel-team, andrii, drosen
Cc: sinquersw, kuifeng, netdev
In-Reply-To: <20231030192810.382942-3-thinker.li@gmail.com>
On 10/30/23 12:28 PM, thinker.li@gmail.com wrote:
> +static void bpf_struct_ops_init_one(struct bpf_struct_ops_desc *st_ops_desc,
> struct btf *btf,
> struct bpf_verifier_log *log)
nit. I think this should be renamed to bpf_struct_ops_desc_init() instead. It is
initializing 'struct bpf_struct_ops_desc' now.
^ permalink raw reply
* Re: [PATCH bpf-next v8 07/10] bpf, net: switch to dynamic registration
From: Martin KaFai Lau @ 2023-10-31 6:36 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, netdev, bpf, ast, song, kernel-team, andrii,
drosen
In-Reply-To: <20231030192810.382942-8-thinker.li@gmail.com>
On 10/30/23 12:28 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
>
> Replace the static list of struct_ops types with per-btf struct_ops_tab to
> enable dynamic registration.
>
> Both bpf_dummy_ops and bpf_tcp_ca now utilize the registration function
> instead of being listed in bpf_struct_ops_types.h.
>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
> include/linux/bpf.h | 36 ++++++--
> include/linux/btf.h | 5 +-
> kernel/bpf/bpf_struct_ops.c | 140 +++++++++---------------------
> kernel/bpf/bpf_struct_ops_types.h | 12 ---
> kernel/bpf/btf.c | 41 ++++++++-
> net/bpf/bpf_dummy_struct_ops.c | 14 ++-
> net/ipv4/bpf_tcp_ca.c | 16 +++-
> 7 files changed, 140 insertions(+), 124 deletions(-)
> delete mode 100644 kernel/bpf/bpf_struct_ops_types.h
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index c993df3cf699..9d7105ff06db 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1644,7 +1644,6 @@ struct bpf_struct_ops_desc {
> #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL)
> #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA))
> const struct bpf_struct_ops_desc *bpf_struct_ops_find(struct btf *btf, u32 type_id);
> -void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log);
> bool bpf_struct_ops_get(const void *kdata);
> void bpf_struct_ops_put(const void *kdata);
> int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,
> @@ -1690,10 +1689,6 @@ static inline const struct bpf_struct_ops_desc *bpf_struct_ops_find(struct btf *
> {
> return NULL;
> }
> -static inline void bpf_struct_ops_init(struct btf *btf,
> - struct bpf_verifier_log *log)
> -{
> -}
> static inline bool bpf_try_module_get(const void *data, struct module *owner)
> {
> return try_module_get(owner);
> @@ -3232,4 +3227,35 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog)
> return prog->aux->func_idx != 0;
> }
>
> +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops);
> +
> +enum bpf_struct_ops_state {
> + BPF_STRUCT_OPS_STATE_INIT,
> + BPF_STRUCT_OPS_STATE_INUSE,
> + BPF_STRUCT_OPS_STATE_TOBEFREE,
> + BPF_STRUCT_OPS_STATE_READY,
> +};
> +
> +struct bpf_struct_ops_common_value {
> + refcount_t refcnt;
> + enum bpf_struct_ops_state state;
> +};
> +
> +/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
> + * the map's value exposed to the userspace and its btf-type-id is
> + * stored at the map->btf_vmlinux_value_type_id.
> + *
> + */
> +#define DEFINE_STRUCT_OPS_VALUE_TYPE(_name) \
> +extern struct bpf_struct_ops bpf_##_name; \
> + \
> +struct bpf_struct_ops_##_name { \
> + struct bpf_struct_ops_common_value common; \
> + struct _name data ____cacheline_aligned_in_smp; \
> +}
> +
> +extern int bpf_struct_ops_init(struct bpf_struct_ops_desc *st_ops_desc,
> + struct btf *btf,
> + struct bpf_verifier_log *log);
> +
> #endif /* _LINUX_BPF_H */
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index a8813605f2f6..954536431e0b 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -12,6 +12,8 @@
> #include <uapi/linux/bpf.h>
>
> #define BTF_TYPE_EMIT(type) ((void)(type *)0)
> +#define BTF_STRUCT_OPS_TYPE_EMIT(type) {((void)(struct type *)0); \
((void)(struct type *)0); is new. Why is it needed?
> + ((void)(struct bpf_struct_ops_##type *)0); }
> #define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
>
> /* These need to be macros, as the expressions are used in assembler input */
> @@ -201,6 +203,7 @@ u32 btf_obj_id(const struct btf *btf);
> bool btf_is_kernel(const struct btf *btf);
> bool btf_is_module(const struct btf *btf);
> struct module *btf_try_get_module(const struct btf *btf);
> +struct btf *btf_get_module_btf(const struct module *module);
> u32 btf_nr_types(const struct btf *btf);
> bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
> const struct btf_member *m,
> @@ -575,8 +578,6 @@ static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type
> struct bpf_struct_ops;
> struct bpf_struct_ops_desc;
>
> -struct bpf_struct_ops_desc *
> -btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops);
> const struct bpf_struct_ops_desc *
> btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
>
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index db2bbba50e38..f3ec72be9c63 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -13,21 +13,8 @@
> #include <linux/btf_ids.h>
> #include <linux/rcupdate_wait.h>
>
> -enum bpf_struct_ops_state {
> - BPF_STRUCT_OPS_STATE_INIT,
> - BPF_STRUCT_OPS_STATE_INUSE,
> - BPF_STRUCT_OPS_STATE_TOBEFREE,
> - BPF_STRUCT_OPS_STATE_READY,
> -};
> -
> -struct bpf_struct_ops_common_value {
> - refcount_t refcnt;
> - enum bpf_struct_ops_state state;
> -};
> -#define BPF_STRUCT_OPS_COMMON_VALUE struct bpf_struct_ops_common_value common
> -
> struct bpf_struct_ops_value {
> - BPF_STRUCT_OPS_COMMON_VALUE;
> + struct bpf_struct_ops_common_value common;
This cleanup is good. It should have been done together in patch 5 instead when
refcnt and state were grouped into a new 'struct bpf_struct_ops_common_value'.
> char data[] ____cacheline_aligned_in_smp;
> };
>
> @@ -72,35 +59,6 @@ static DEFINE_MUTEX(update_mutex);
> #define VALUE_PREFIX "bpf_struct_ops_"
> #define VALUE_PREFIX_LEN (sizeof(VALUE_PREFIX) - 1)
>
> -/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
> - * the map's value exposed to the userspace and its btf-type-id is
> - * stored at the map->btf_vmlinux_value_type_id.
> - *
> - */
> -#define BPF_STRUCT_OPS_TYPE(_name) \
> -extern struct bpf_struct_ops bpf_##_name; \
> - \
> -struct bpf_struct_ops_##_name { \
> - BPF_STRUCT_OPS_COMMON_VALUE; \
> - struct _name data ____cacheline_aligned_in_smp; \
> -};
> -#include "bpf_struct_ops_types.h"
> -#undef BPF_STRUCT_OPS_TYPE
> -
> -enum {
> -#define BPF_STRUCT_OPS_TYPE(_name) BPF_STRUCT_OPS_TYPE_##_name,
> -#include "bpf_struct_ops_types.h"
> -#undef BPF_STRUCT_OPS_TYPE
> - __NR_BPF_STRUCT_OPS_TYPE,
> -};
> -
> -static struct bpf_struct_ops_desc bpf_struct_ops[] = {
> -#define BPF_STRUCT_OPS_TYPE(_name) \
> - [BPF_STRUCT_OPS_TYPE_##_name] = { .st_ops = &bpf_##_name },
> -#include "bpf_struct_ops_types.h"
> -#undef BPF_STRUCT_OPS_TYPE
> -};
> -
> const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = {
> };
>
> @@ -110,13 +68,22 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
> #endif
> };
>
> -static const struct btf_type *module_type;
> -static const struct btf_type *common_value_type;
> +BTF_ID_LIST(st_ops_ids)
> +BTF_ID(struct, module)
> +BTF_ID(struct, bpf_struct_ops_common_value)
This should have been done in a separated patch immediately after patch 1. The
patch 7 has unrelated changes/cleanups like this and the above
BPF_STRUCT_OPS_COMMON_VALUE which could have been done earlier as preparation
patches instead of packing them together with the main change here: "switch to
dynamic registration". The commit message for the BTF_ID_LIST changes could be
like: "A preparation to completely retire the bpf_struct_ops_init() function in
the latter patch...".
> +
> +enum {
> + idx_module_id,
> + idx_st_ops_common_value_id,
nit. upper case to stay consistent with other similar usages.
> +};
> +
[ ... ]
> +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops)
> +{
> + struct bpf_struct_ops_desc *desc;
> + struct bpf_verifier_log *log;
> + struct btf *btf;
> + int err = 0;
> +
> + if (st_ops == NULL)
NULL check is not needed. caller will never do that. If it really wanted to try,
other values would have similar effect.
> + return -EINVAL;
> +
> + btf = btf_get_module_btf(st_ops->owner);
> + if (!btf)
> + return -EINVAL;
> +
> + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
> + if (!log) {
> + err = -ENOMEM;
> + goto errout;
> + }
> +
> + log->level = BPF_LOG_KERNEL;
> +
> + desc = btf_add_struct_ops(btf, st_ops);
> + if (IS_ERR(desc)) {
> + err = PTR_ERR(desc);
> + goto errout;
> + }
> +
> + err = bpf_struct_ops_init(desc, btf, log);
When bpf_struct_ops_init() returns err, desc is in btf_struct_ops_tab but it is
in an uninitialized state. May be do the bpf_struct_ops_init() in
btf_add_struct_ops() and only increment struct_ops_tab->cnt when everything is
correct.
> +
> +errout:
> + kfree(log);
> + btf_put(btf);
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(register_bpf_struct_ops);
^ permalink raw reply
* Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select
From: Oleksij Rempel @ 2023-10-31 6:27 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Ante Knezic, conor+dt, UNGLinuxDriver, andrew, davem, devicetree,
edumazet, f.fainelli, krzysztof.kozlowski+dt, kuba, linux-kernel,
marex, netdev, pabeni, robh+dt, woojung.huh
In-Reply-To: <20231030174225.hqhc3afbayi7dmos@skbuf>
On Mon, Oct 30, 2023 at 07:42:25PM +0200, Vladimir Oltean wrote:
> On Fri, Oct 27, 2023 at 08:37:43AM +0200, Ante Knezic wrote:
> > On Tue, 24 Oct 2023 16:24:26 +0200, Oleksij Rampel wrote:
> >
> > > > That is correct, I guess its a matter of nomenclature, but how do you
> > > > "tell" the switch whether it has REFCLKI routed externally or not if not by
> > > > setting the 0xC6 bit 3? Is there another way to achieve this?
> > >
> > > I do not see any other way to "tell" it. The only thing to change in you
> > > patches is a different way to tell it to the kernel.
> > > Instead of introducing a new devicetree property, you need to reuse
> > > phy-mode property.
> >
> > > ...
> >
> > > Since phy-mode for RMII was never set correctly, it will most probably
> > > break every single devicetree using KSZ switches. It is the price of fixing
> > > things :/
> >
> > To Vladimir Oltean: What are your thoughts on this?
> >
>
> In addition to all of that, the MAC/PHY roles are not just about the
> direction of the REF_CLK, but also about the /J/ /K/ codewords that are
> placed by the PHY in the inter packet gap on RXD[1:0]. A MAC doesn't do
> this, and if it did, the PHY wouldn't expect it, and AFAIK, would
> blindly propagate those code words onto the BASE-T wire, which is
> undesirable.
Interesting detail. I didn't knew it, it would be good to document
it somewhere near to revrmii binding :)
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* [net-next PATCH] octeontx2-pf: TC flower offload support for ICMP type and code
From: Geetha sowjanya @ 2023-10-31 6:19 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: kuba, davem, pabeni, edumazet, sgoutham, gakula, sbhatta, hkelam
Adds tc offload support for matching on ICMP type and code.
Example usage:
To enable adding tc ingress rules
tc qdisc add dev eth0 ingress
TC rule drop the ICMP echo reply:
tc filter add dev eth0 protocol ip parent ffff: \
flower ip_proto icmp type 8 code 0 skip_sw action drop
TC rule to drop ICMPv6 echo reply:
tc filter add dev eth0 protocol ipv6 parent ffff: flower \
indev eth0 ip_proto icmpv6 type 128 code 0 action drop
Signed-off-by: Sunil Goutham <sgoutham@marvell.com>
Signed-off-by: Geetha sowjanya <gakula@marvell.com>
---
.../net/ethernet/marvell/octeontx2/af/mbox.h | 2 ++
.../net/ethernet/marvell/octeontx2/af/npc.h | 2 ++
.../marvell/octeontx2/af/rvu_debugfs.c | 8 +++++++
.../marvell/octeontx2/af/rvu_npc_fs.c | 23 ++++++++++++++-----
.../ethernet/marvell/octeontx2/nic/otx2_tc.c | 14 +++++++++++
5 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
index 6b5b06c2b4e9..78088dd4e2f9 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
+++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
@@ -1473,6 +1473,8 @@ struct flow_msg {
u8 next_header;
};
__be16 vlan_itci;
+ u8 icmp_type;
+ u8 icmp_code;
};
struct npc_install_flow_req {
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/npc.h b/drivers/net/ethernet/marvell/octeontx2/af/npc.h
index de9fbd98dfb7..2f1ed5411d75 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/npc.h
+++ b/drivers/net/ethernet/marvell/octeontx2/af/npc.h
@@ -206,6 +206,8 @@ enum key_fields {
NPC_SPORT_SCTP,
NPC_DPORT_SCTP,
NPC_IPSEC_SPI,
+ NPC_TYPE_ICMP,
+ NPC_CODE_ICMP,
NPC_HEADER_FIELDS_MAX,
NPC_CHAN = NPC_HEADER_FIELDS_MAX, /* Valid when Rx */
NPC_PF_FUNC, /* Valid when Tx */
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
index d30e84803481..2b32b9d6c625 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
@@ -2836,6 +2836,14 @@ static void rvu_dbg_npc_mcam_show_flows(struct seq_file *s,
seq_printf(s, "0x%x ", ntohl(rule->packet.spi));
seq_printf(s, "mask 0x%x\n", ntohl(rule->mask.spi));
break;
+ case NPC_TYPE_ICMP:
+ seq_printf(s, "%d ", rule->packet.icmp_type);
+ seq_printf(s, "mask 0x%x\n", rule->mask.icmp_type);
+ break;
+ case NPC_CODE_ICMP:
+ seq_printf(s, "%d ", rule->packet.icmp_code);
+ seq_printf(s, "mask 0x%x\n", rule->mask.icmp_code);
+ break;
default:
seq_puts(s, "\n");
break;
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
index 237f82082ebe..ad204e21867b 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
@@ -43,6 +43,8 @@ static const char * const npc_flow_names[] = {
[NPC_DPORT_SCTP] = "sctp destination port",
[NPC_LXMB] = "Mcast/Bcast header ",
[NPC_IPSEC_SPI] = "SPI ",
+ [NPC_TYPE_ICMP] = "icmp type",
+ [NPC_CODE_ICMP] = "icmp code",
[NPC_UNKNOWN] = "unknown",
};
@@ -518,6 +520,8 @@ do { \
NPC_SCAN_HDR(NPC_DPORT_TCP, NPC_LID_LD, NPC_LT_LD_TCP, 2, 2);
NPC_SCAN_HDR(NPC_SPORT_SCTP, NPC_LID_LD, NPC_LT_LD_SCTP, 0, 2);
NPC_SCAN_HDR(NPC_DPORT_SCTP, NPC_LID_LD, NPC_LT_LD_SCTP, 2, 2);
+ NPC_SCAN_HDR(NPC_TYPE_ICMP, NPC_LID_LD, NPC_LT_LD_ICMP, 0, 1);
+ NPC_SCAN_HDR(NPC_CODE_ICMP, NPC_LID_LD, NPC_LT_LD_ICMP, 1, 1);
NPC_SCAN_HDR(NPC_ETYPE_ETHER, NPC_LID_LA, NPC_LT_LA_ETHER, 12, 2);
NPC_SCAN_HDR(NPC_ETYPE_TAG1, NPC_LID_LB, NPC_LT_LB_CTAG, 4, 2);
NPC_SCAN_HDR(NPC_ETYPE_TAG2, NPC_LID_LB, NPC_LT_LB_STAG_QINQ, 8, 2);
@@ -539,7 +543,7 @@ static void npc_set_features(struct rvu *rvu, int blkaddr, u8 intf)
{
struct npc_mcam *mcam = &rvu->hw->mcam;
u64 *features = &mcam->rx_features;
- u64 tcp_udp_sctp;
+ u64 proto_flags;
int hdr;
if (is_npc_intf_tx(intf))
@@ -550,18 +554,21 @@ static void npc_set_features(struct rvu *rvu, int blkaddr, u8 intf)
*features |= BIT_ULL(hdr);
}
- tcp_udp_sctp = BIT_ULL(NPC_SPORT_TCP) | BIT_ULL(NPC_SPORT_UDP) |
+ proto_flags = BIT_ULL(NPC_SPORT_TCP) | BIT_ULL(NPC_SPORT_UDP) |
BIT_ULL(NPC_DPORT_TCP) | BIT_ULL(NPC_DPORT_UDP) |
- BIT_ULL(NPC_SPORT_SCTP) | BIT_ULL(NPC_DPORT_SCTP);
+ BIT_ULL(NPC_SPORT_SCTP) | BIT_ULL(NPC_DPORT_SCTP) |
+ BIT_ULL(NPC_SPORT_SCTP) | BIT_ULL(NPC_DPORT_SCTP) |
+ BIT_ULL(NPC_TYPE_ICMP) | BIT_ULL(NPC_CODE_ICMP);
/* for tcp/udp/sctp corresponding layer type should be in the key */
- if (*features & tcp_udp_sctp) {
+ if (*features & proto_flags) {
if (!npc_check_field(rvu, blkaddr, NPC_LD, intf))
- *features &= ~tcp_udp_sctp;
+ *features &= ~proto_flags;
else
*features |= BIT_ULL(NPC_IPPROTO_TCP) |
BIT_ULL(NPC_IPPROTO_UDP) |
- BIT_ULL(NPC_IPPROTO_SCTP);
+ BIT_ULL(NPC_IPPROTO_SCTP) |
+ BIT_ULL(NPC_IPPROTO_ICMP);
}
/* for AH/ICMP/ICMPv6/, check if corresponding layer type is present in the key */
@@ -950,6 +957,10 @@ do { \
ntohs(mask->sport), 0);
NPC_WRITE_FLOW(NPC_DPORT_SCTP, dport, ntohs(pkt->dport), 0,
ntohs(mask->dport), 0);
+ NPC_WRITE_FLOW(NPC_TYPE_ICMP, icmp_type, pkt->icmp_type, 0,
+ mask->icmp_type, 0);
+ NPC_WRITE_FLOW(NPC_CODE_ICMP, icmp_code, pkt->icmp_code, 0,
+ mask->icmp_code, 0);
NPC_WRITE_FLOW(NPC_IPSEC_SPI, spi, ntohl(pkt->spi), 0,
ntohl(mask->spi), 0);
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
index fab9d85bfb37..bede05dfad7b 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
@@ -519,6 +519,7 @@ static int otx2_tc_prepare_flow(struct otx2_nic *nic, struct otx2_tc_flow *node,
BIT_ULL(FLOW_DISSECTOR_KEY_IPV6_ADDRS) |
BIT_ULL(FLOW_DISSECTOR_KEY_PORTS) |
BIT(FLOW_DISSECTOR_KEY_IPSEC) |
+ BIT_ULL(FLOW_DISSECTOR_KEY_ICMP) |
BIT_ULL(FLOW_DISSECTOR_KEY_IP)))) {
netdev_info(nic->netdev, "unsupported flow used key 0x%llx",
dissector->used_keys);
@@ -738,6 +739,19 @@ static int otx2_tc_prepare_flow(struct otx2_nic *nic, struct otx2_tc_flow *node,
}
}
+ if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ICMP)) {
+ struct flow_match_icmp match;
+
+ flow_rule_match_icmp(rule, &match);
+
+ flow_spec->icmp_type = match.key->type;
+ flow_mask->icmp_type = match.mask->type;
+ req->features |= BIT_ULL(NPC_TYPE_ICMP);
+
+ flow_spec->icmp_code = match.key->code;
+ flow_mask->icmp_code = match.mask->code;
+ req->features |= BIT_ULL(NPC_CODE_ICMP);
+ }
return otx2_tc_parse_actions(nic, &rule->action, req, f, node);
}
--
2.25.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox