Re: [PATCH v9 0/9] net/zxdh: introduce net zxdh driver

[Ferruh Yigit]

On 11/1/2024 6:21 AM, Junlong Wang wrote:
> v9:
>   - fix 'v8 3/9' patch use PCI bus API,
> and common PCI constants according to David Marchand's comments.
> 
> v8:
>   - fix flexible arrays、Waddress-of-packed-member error.
>   - all structs、enum、define ,etc use zxdh/ZXDH_ prefixed.
>   - use zxdh_try/release_lock,and move loop into zxdh_timedlock,
> make hardware lock follow spinlock pattern.
> 
> v7:
>   - add release notes and modify zxdh.rst issues.
>   - avoid use pthread and use rte_spinlock_lock.
>   - using the prefix ZXDH_ before some definitions.
>   - resole issues according to thomas's comments.
> 
> v6:
>   - Resolve ci/intel compilation issues.
>   - fix meson.build indentation in earlier patch.
> 
> V5:
>   - split driver into multiple patches,part of the zxdh driver,
> later provide dev start/stop,queue_setup,npsdk_init,mac,vlan,rss ,etc.
>   - fix errors reported by scripts.
>   - move the product link in zxdh.rst.
>   - fix meson check use RTE_ARCH_X86_64/RTE_ARCH_ARM64.
>   - modify other comments according to Ferruh's comments.
> 
> Junlong Wang (9):
>   net/zxdh: add zxdh ethdev pmd driver
>   net/zxdh: add logging implementation
>   net/zxdh: add zxdh device pci init implementation
>   net/zxdh: add msg chan and msg hwlock init
>   net/zxdh: add msg chan enable implementation
>   net/zxdh: add zxdh get device backend infos
>   net/zxdh: add configure zxdh intr implementation
>   net/zxdh: add zxdh dev infos get ops
>   net/zxdh: add zxdh dev configure ops
> 

Hi Junlong,

I can see not all of the eth_dev_ops implemented, and datapath not
implemented, so driver is not functional right now.

What happens if you want to run testpmd with the current state of the
driver, I assume it crashes?

And what is the plan for the driver? Are you planning to upstream
remaining support in this release or in future releases?

As the driver is not functional yet, to set the expectation right for
the users, I suggest marking driver as experimental in the maintainers
file and document the restrictions in the driver documentation, also
clarify this in the release notes update, what do you think?

Re: [PATCH v9 4/9] net/zxdh: add msg chan and msg hwlock init

[Ferruh Yigit]

On 11/1/2024 6:21 AM, Junlong Wang wrote:
> Add msg channel and hwlock init implementation.
> 
> Signed-off-by: Junlong Wang 
> 

<...>

> @@ -83,9 +84,23 @@ static int zxdh_eth_dev_init(struct rte_eth_dev *eth_dev)
>  if (ret < 0)
>  goto err_zxdh_init;
>  
> +ret = zxdh_msg_chan_init();
> +if (ret != 0) {
> +PMD_INIT_LOG(ERR, "Failed to init bar msg chan");
> +goto err_zxdh_init;
> +}
> +hw->msg_chan_init = 1;
> +
> +ret = zxdh_msg_chan_hwlock_init(eth_dev);
> +if (ret != 0) {
> +PMD_INIT_LOG(ERR, "zxdh_msg_chan_hwlock_init failed ret %d", ret);
> +goto err_zxdh_init;
> +}
> +
>  return ret;
>  
>  err_zxdh_init:
> +zxdh_bar_msg_chan_exit();
>

Should 'zxdh_bar_msg_chan_exit()' called during zxdh_eth_dev_uninit()?

<...>

> +
> +struct zxdh_dev_stat {
> +bool is_mpf_scanned;
> +bool is_res_init;
> +int16_t dev_cnt; /* probe cnt */
> +};
> +struct zxdh_dev_stat g_dev_stat = {0};
>

Is there a reason to not make this global variable 'static'?
Please remember, when a DPDK application compiled, this will be all
application and other driver and libraries, if there is really a good
reason, please keep all global variables in the scope of driver.

And no need to initialize global variable to 0, that is done by default.

> +
> +struct zxdh_seqid_item {
> +void *reps_addr;
> +uint16_t id;
> +uint16_t buffer_len;
> +uint16_t flag;
> +};
> +
> +struct zxdh_seqid_ring {
> +uint16_t cur_id;
> +rte_spinlock_t lock;
> +struct zxdh_seqid_item reps_info_tbl[ZXDH_BAR_SEQID_NUM_MAX];
> +};
> +struct zxdh_seqid_ring g_seqid_ring = {0};
> +

ditto

<...>

> +/**
> + * Fun: PF init hard_spinlock addr
> + */
> +static int bar_chan_pf_init_spinlock(uint16_t pcie_id, uint64_t 
> bar_base_addr)
> +{
> +int lock_id = pcie_id_to_hard_lock(pcie_id, ZXDH_MSG_CHAN_END_RISC);
> +
> +zxdh_spinlock_unlock(lock_id, bar_base_addr + ZXDH_BAR0_SPINLOCK_OFFSET,
> +bar_base_addr + ZXDH_HW_LABEL_OFFSET);
> +lock_id = pcie_id_to_hard_lock(pcie_id, ZXDH_MSG_CHAN_END_VF);
> +zxdh_spinlock_unlock(lock_id, bar_base_addr + ZXDH_BAR0_SPINLOCK_OFFSET,
> +bar_base_addr + ZXDH_HW_LABEL_OFFSET);
> +return 0;
> +}
> +
> +int zxdh_msg_chan_hwlock_init(struct rte_eth_dev *dev)
> +{
> +struct zxdh_hw *hw = dev->data->dev_private;
> +
> +if (!hw->is_pf)
> +return 0;
> +return bar_chan_pf_init_spinlock(hw->pcie_id, (uint64_t)(hw-
>>bar_addr[ZXDH_BAR0_INDEX]));
> +}
> +
> +static rte_spinlock_t chan_lock;
>

please move global variables to the top of the file, otherwise it is
very easy to miss them.

Re: [PATCH v9 3/9] net/zxdh: add zxdh device pci init implementation

[Ferruh Yigit]

On 11/1/2024 6:21 AM, Junlong Wang wrote:
> Add device pci init implementation,
> to obtain PCI capability and read configuration, etc.
> 
> Signed-off-by: Junlong Wang 
> ---
>  drivers/net/zxdh/meson.build   |   1 +
>  drivers/net/zxdh/zxdh_ethdev.c |  43 +
>  drivers/net/zxdh/zxdh_ethdev.h |  18 ++-
>  drivers/net/zxdh/zxdh_pci.c| 278 +
>  drivers/net/zxdh/zxdh_pci.h| 138 
>  drivers/net/zxdh/zxdh_queue.h  | 109 +
>  drivers/net/zxdh/zxdh_rxtx.h   |  55 +++
>

zxdh_queue.h & zxdh_rxtx.h seems not used in this patch, and they are
not indeed related to the PCI init, or PCI at all.
Can you add them in the patch that they are used?

<...>

> +#define ZXDH_PMD_DEFAULT_GUEST_FEATURES   \
> +(1ULL << ZXDH_NET_F_MRG_RXBUF | \
> + 1ULL << ZXDH_NET_F_STATUS| \
> + 1ULL << ZXDH_NET_F_MQ| \
> + 1ULL << ZXDH_F_ANY_LAYOUT| \
> + 1ULL << ZXDH_F_VERSION_1 | \
> + 1ULL << ZXDH_F_RING_PACKED   | \
> + 1ULL << ZXDH_F_IN_ORDER  | \
> + 1ULL << ZXDH_F_NOTIFICATION_DATA | \
> + 1ULL << ZXDH_NET_F_MAC)
> +
> +static void zxdh_read_dev_config(struct zxdh_hw *hw,
> +   size_t offset,
> +   void *dst,
> +   int32_t length)
>

Syntax issue, with tab stop = 8, above lines looks wrong can you please
check?

<...>

> +struct zxdh_virtqueue {
> +struct zxdh_hw  *hw; /**< zxdh_hw structure pointer. */
> +struct {
> +/**< vring keeping descs and events */
> +struct zxdh_vring_packed ring;
> +uint8_t used_wrap_counter;
> +uint8_t rsv;
> +uint16_t cached_flags; /**< cached flags for descs */
> +uint16_t event_flags_shadow;
> +uint16_t rsv1;
> +} __rte_packed vq_packed;
> +uint16_t vq_used_cons_idx; /**< last consumed descriptor */
> +uint16_t vq_nentries;  /**< vring desc numbers */
> +uint16_t vq_free_cnt;  /**< num of desc available */
> +uint16_t vq_avail_idx; /**< sync until needed */
> +uint16_t vq_free_thresh; /**< free threshold */
> +uint16_t rsv2;
> +
> +void *vq_ring_virt_mem;  /**< linear address of vring*/
> +uint32_t vq_ring_size;
> +
> +union {
> +struct zxdh_virtnet_rx rxq;
> +struct zxdh_virtnet_tx txq;
> +};
> +
> +/** < physical address of vring,
> + * or virtual address
> + **/
>

'/**' is doxygen syntax, I don't know if you are using doxgen.
But '/**<' is doxygen sytax to say comment is for the code before it,
not after. So it is used wrong above.
If you don't have any specific reason, why not using regular comments
(non doxygen syntax), and leave first line empty, and terminate with
'*/', so above becomes:
/*
 * physical address of vring, or virtual address
 */

This comment is for all comment code in all the series. It looks like
there is a mixture of the usage.

Re: [PATCH 00/16] remove use of VLAs for Windows built code

[Andre Muezerie]

On Mon, Oct 07, 2024 at 10:15:46AM -0700, Stephen Hemminger wrote:
> On Wed, 17 Apr 2024 16:41:43 -0700
> Tyler Retzlaff  wrote:
> 
> > As per guidance technical board meeting 2024/04/17. This series
> > removes the use of VLAs from code built for Windows for all 3
> > toolchains. If there are additional opportunities to convert VLAs
> > to regular C arrays please provide the details for incorporation
> > into the series.
> > 
> > MSVC does not support VLAs, replace VLAs with standard C arrays
> > or alloca(). alloca() is available for all toolchain/platform
> > combinations officially supported by DPDK.
> > 
> > Tyler Retzlaff (16):
> >   eal: include header required for alloca
> >   hash: remove use of VLAs for Windows built code
> >   ethdev: remove use of VLAs for Windows built code
> >   gro: remove use of VLAs for Windows built code
> >   latencystats: remove use of VLAs for Windows built code
> >   lpm: remove use of VLAs for Windows built code
> >   rcu: remove use of VLAs for Windows built code
> >   app/testpmd: remove use of VLAs for Windows built code
> >   test: remove use of VLAs for Windows built code
> >   common/idpf: remove use of VLAs for Windows built code
> >   net/i40e: remove use of VLAs for Windows built code
> >   net/ice: remove use of VLAs for Windows built code
> >   net/ixgbe: remove use of VLAs for Windows built code
> >   common/mlx5: remove use of VLAs for Windows built code
> >   net/mlx5: remove use of VLAs for Windows built code
> >   build: enable vla warnings on Windows built code
> > 
> >  app/test-pmd/cmdline.c|  2 +-
> >  app/test-pmd/cmdline_flow.c   |  9 +++--
> >  app/test-pmd/config.c | 16 +
> >  app/test-pmd/shared_rxq_fwd.c |  2 +-
> >  app/test/test.c   |  2 +-
> >  app/test/test_cmdline_string.c|  2 +-
> >  app/test/test_cryptodev.c | 32 +-
> >  app/test/test_cryptodev_blockcipher.c |  4 +--
> >  app/test/test_cryptodev_crosscheck.c  |  2 +-
> >  app/test/test_dmadev.c|  9 +++--
> >  app/test/test_hash.c  |  8 ++---
> >  app/test/test_mempool.c   | 25 +++---
> >  app/test/test_reassembly_perf.c   |  4 +--
> >  app/test/test_reorder.c   | 48 
> > +++
> >  app/test/test_service_cores.c |  9 +++--
> >  app/test/test_thash.c |  7 ++--
> >  config/meson.build|  4 +++
> >  drivers/common/idpf/idpf_common_rxtx.c|  2 +-
> >  drivers/common/idpf/idpf_common_rxtx_avx512.c |  6 ++--
> >  drivers/common/mlx5/mlx5_common.h |  4 +--
> >  drivers/common/mlx5/mlx5_devx_cmds.c  |  7 ++--
> >  drivers/net/i40e/i40e_testpmd.c   |  5 ++-
> >  drivers/net/ice/ice_rxtx.c|  2 +-
> >  drivers/net/ixgbe/ixgbe_ethdev.c  |  5 +--
> >  drivers/net/ixgbe/ixgbe_rxtx_vec_common.h |  2 +-
> >  drivers/net/mlx5/mlx5.c   |  5 ++-
> >  drivers/net/mlx5/mlx5_flow.c  |  6 ++--
> >  lib/eal/linux/include/rte_os.h|  1 +
> >  lib/eal/windows/include/rte_os.h  |  1 +
> >  lib/ethdev/rte_ethdev.c   |  9 ++---
> >  lib/gro/rte_gro.c |  4 +--
> >  lib/hash/rte_cuckoo_hash.c|  4 +--
> >  lib/hash/rte_thash.c  |  2 +-
> >  lib/latencystats/rte_latencystats.c   |  2 +-
> >  lib/lpm/rte_lpm.h |  2 +-
> >  lib/rcu/rte_rcu_qsbr.c|  6 ++--
> >  36 files changed, 137 insertions(+), 123 deletions(-)
> 
> Looks good:
> 
> Series-Acked-by: Stephen Hemminger 
> 
> 
> Series needs to be rebased to current master, some parts have changed.

Hi Stephen, I'll continue the work Tyler was doing on this front.
You can expect a new rebased patch soon. I'll also adapt it for the parts that 
have changed.

Re: [PATCH] igc: fix invalid length and corrupted multi-segment mbufs

[Bruce Richardson]

On Fri, Nov 01, 2024 at 02:42:20PM +0100, Martin Weiser wrote:
> Hi Bruce,
> 
> thank you very much for your feedback.
> Please see my answers inline below.
> 
> I will send a v2 of the patch.
> 
> Best regards,
> Martin
> 
> 
> Am 29.10.24 um 18:42 schrieb Bruce Richardson:
> > On Mon, Oct 28, 2024 at 03:17:07PM +0100, Martin Weiser wrote:
> >>
> >> The issue only appeared with hardware-timestamping enabled
> >> (RTE_ETH_RX_OFFLOAD_TIMESTAMP).
> >>
> >> The length of the prepended hardware timestamp was not subtracted from
> >> the data length so that received packets were 16 bytes longer than
> >> expected.
> >>
> >> In scatter-gather mode only the first mbuf has a timestamp but the
> >> data offset of the follow-up mbufs was not adjusted accordingly.
> >> This caused 16 bytes of packet data to be missing between
> >> the segments.
> >>
> >> Signed-off-by: Martin Weiser 
> >> ---
> > 
> > Hi,
> > 
> > thanks for the patch. Some comments inline below.
> > 
> > /Bruce
> > 
> >>  drivers/net/igc/igc_txrx.c | 9 +
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/net/igc/igc_txrx.c b/drivers/net/igc/igc_txrx.c
> >> index d0cee1b016..2fafa91bd5 100644
> >> --- a/drivers/net/igc/igc_txrx.c
> >> +++ b/drivers/net/igc/igc_txrx.c
> >> @@ -347,6 +347,8 @@ igc_recv_pkts(void *rx_queue, struct rte_mbuf 
> >> **rx_pkts, uint16_t nb_pkts)
> >>  
> >>rxm->data_off = RTE_PKTMBUF_HEADROOM;
> >>data_len = rte_le_to_cpu_16(rxd.wb.upper.length) - rxq->crc_len;
> >> +  if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
> >> +  data_len -= IGC_TS_HDR_LEN;
> >>rxm->data_len = data_len;
> >>rxm->pkt_len = data_len;
> >>rxm->nb_segs = 1;
> >> @@ -509,6 +511,12 @@ igc_recv_scattered_pkts(void *rx_queue, struct 
> >> rte_mbuf **rx_pkts,
> >> */
> >>rxm->data_off = RTE_PKTMBUF_HEADROOM;
> >>data_len = rte_le_to_cpu_16(rxd.wb.upper.length);
> >> +  if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
> >> +  if (first_seg == NULL)
> >> +  data_len -= IGC_TS_HDR_LEN;
> >> +  else
> >> +  rxm->data_off -= IGC_TS_HDR_LEN;
> > 
> > This initially confused me, because I was assuming that data_off was a typo
> > for data_len. However, then I realised on closer examination that, when
> > timestamp offload is enabled, the actual buffer addresses sent down to the
> > hardware are offset by IGC_TS_HDR_LEN (meaning the first buffer of a pkt
> > has the start of data at "normal" data_offset in mbuf, but subsequent
> > buffers need adjustment). Is my understanding of the issue correct?
> 
> This is exactly right. Took me a bit to understand what was happening here and
> I do not know if there might be a better way do this than to change the start
> offset of all descriptors. But there is probably a reason why it was done 
> this way.
> Initially the issue was detected as forwarding of packets that maxed out the 
> MTU
> failed since they now, with the increased length, exceeded the MTU. Only then 
> it
> became apparent that the scatter-gather handling was also broken.
> 
> > 
> > In either case, I have two small bits of feedback on this:
> > * Firstly, I think this needs a comment explaining the logic here, to avoid
> > others being confused as I was.
> 
> This will be part of patch v2.
> 
> > * Secondly, a very minor point, but is the code clearer or shorter, if you
> > merge this extra code down into the next block which is already checking
> > for the first_segment of a packet or not?
> 
> I would actually prefer to keep it that way as not to mix up the the data 
> buffer
> address and length handling with the mbuf chain construction.
> 

Ok, that is fine if you think it better. I'll just expect V2 with some
added comments to clarify things.

Thanks,
/Bruce

[PATCH v2] igc: fix invalid length and corrupted multi-segment mbufs

[Martin Weiser]

The issue only appeared with hardware-timestamping enabled
(RTE_ETH_RX_OFFLOAD_TIMESTAMP).

The length of the prepended hardware timestamp was not subtracted from
the data length so that received packets were 16 bytes longer than
expected.

In scatter-gather mode only the first mbuf has a timestamp but the
data offset of the follow-up mbufs was not adjusted accordingly.
This caused 16 bytes of packet data to be missing between
the segments.

Signed-off-by: Martin Weiser 
---
v2:
* Added comments for clarification.


 drivers/net/igc/igc_txrx.c | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/igc/igc_txrx.c b/drivers/net/igc/igc_txrx.c
index d0cee1b016..fabab5b1a3 100644
--- a/drivers/net/igc/igc_txrx.c
+++ b/drivers/net/igc/igc_txrx.c
@@ -347,6 +347,13 @@ igc_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, 
uint16_t nb_pkts)
 
rxm->data_off = RTE_PKTMBUF_HEADROOM;
data_len = rte_le_to_cpu_16(rxd.wb.upper.length) - rxq->crc_len;
+   /*
+* When the RTE_ETH_RX_OFFLOAD_TIMESTAMP offload is enabled the
+* length in the descriptor still accounts for the timestamp so
+* it must be subtracted.
+*/
+   if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
+   data_len -= IGC_TS_HDR_LEN;
rxm->data_len = data_len;
rxm->pkt_len = data_len;
rxm->nb_segs = 1;
@@ -509,6 +516,24 @@ igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf 
**rx_pkts,
 */
rxm->data_off = RTE_PKTMBUF_HEADROOM;
data_len = rte_le_to_cpu_16(rxd.wb.upper.length);
+   if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
+   /*
+* When the RTE_ETH_RX_OFFLOAD_TIMESTAMP offload is 
enabled
+* the pkt_addr of all software ring entries is moved 
forward
+* by IGC_TS_HDR_LEN (see igc_alloc_rx_queue_mbufs()) 
so that
+* when the hardware writes the packet with a prepended
+* timestamp the actual packet data still starts at the
+* normal data offset. The length in the descriptor 
still
+* accounts for the timestamp so it needs to be 
subtracted.
+* Follow-up mbufs do not have the timestamp so the data
+* offset must be adjusted to point to the start of the 
packet
+* data.
+*/
+   if (first_seg == NULL)
+   data_len -= IGC_TS_HDR_LEN;
+   else
+   rxm->data_off -= IGC_TS_HDR_LEN;
+   }
rxm->data_len = data_len;
 
/*
@@ -557,6 +582,7 @@ igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf 
**rx_pkts,
last_seg->data_len = last_seg->data_len -
 (RTE_ETHER_CRC_LEN - data_len);
last_seg->next = NULL;
+   rxm = last_seg;
} else {
rxm->data_len = (uint16_t)
(data_len - RTE_ETHER_CRC_LEN);
-- 
2.47.0

Re: [PATCH v1 1/2] net/macb: add new driver

[Stephen Hemminger]

On Fri,  1 Nov 2024 10:07:19 +
liwencheng  wrote:

> add Phytium NIC MACB ethdev PMD driver.
> 
> Signed-off-by: liwencheng 

Much better.

When submitting a new version of already submitted patch.
DPDK policy is to use version 2 (v2) and use in-reply-to
so that threads in mail client and patchwork archive stay
organized.

New drivers should be submitted in smaller chunks so that
comments are more organized. 

https://doc.dpdk.org/guides/contributing/new_driver.html

A saw a few things.

1. Don't put developer only printf's in under MACB_DEBUG.
   If you have to leave debugging around use the existing
   log macros with DEBUG as the level.

   This applies to all #ifdef's.
   Please don't use them because it leads to dead untested code.

2. Looks like driver is keeping per queue stats, but they are not filled
   in the get_stats code.

3. Unnecessary casts of void * pointer.

4. The code in interrupt handler is more complex than necessary.
   This does the same thing:

static void macb_interrupt_handler(void *param)
{
struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
struct macb_priv *priv = dev->data->dev_private;
struct rte_eth_link link;
char status[128];

if (priv->stopped)
return;

if (eth_macb_link_update(dev, 0) < 0)
return;

rte_eth_linkstatus_get(dev, &link);
rte_eth_link_to_str(status, sizeof(status), &link);
MACB_INFO("Port %u: %s", dev->data->port_id, status);

macb_link_change(priv->bp);
rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, NULL);
}

5. For files, the convention is to use PATH_MAX (you use MAX_FILE_LEN)

6. Use on stack variables where possible (avoids potential memory leaks).
   Example of where allocation can be avoided is in  macb_get_dev_pclk.

7. lots of extra includes.
   For example, why does macb_rxtx_vec_neon need arp, ioctl, socket and stat?

8. Don't use __builtin_ctz, instead use rte_ctz32()

9. Use __rte_cache_aligned not __rte_aligned(RTE_CACHE_LINE_SIZE)

10. Do not disable warnings it hides bugs

11. Take out this line, it bothers checkpatch

meson.build:
#allow_experimental_apis = true

Re: [PATCH] igc: fix invalid length and corrupted multi-segment mbufs

[Martin Weiser]

Hi Bruce,

thank you very much for your feedback.
Please see my answers inline below.

I will send a v2 of the patch.

Best regards,
Martin


Am 29.10.24 um 18:42 schrieb Bruce Richardson:
> On Mon, Oct 28, 2024 at 03:17:07PM +0100, Martin Weiser wrote:
>>
>> The issue only appeared with hardware-timestamping enabled
>> (RTE_ETH_RX_OFFLOAD_TIMESTAMP).
>>
>> The length of the prepended hardware timestamp was not subtracted from
>> the data length so that received packets were 16 bytes longer than
>> expected.
>>
>> In scatter-gather mode only the first mbuf has a timestamp but the
>> data offset of the follow-up mbufs was not adjusted accordingly.
>> This caused 16 bytes of packet data to be missing between
>> the segments.
>>
>> Signed-off-by: Martin Weiser 
>> ---
> 
> Hi,
> 
> thanks for the patch. Some comments inline below.
> 
> /Bruce
> 
>>  drivers/net/igc/igc_txrx.c | 9 +
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/net/igc/igc_txrx.c b/drivers/net/igc/igc_txrx.c
>> index d0cee1b016..2fafa91bd5 100644
>> --- a/drivers/net/igc/igc_txrx.c
>> +++ b/drivers/net/igc/igc_txrx.c
>> @@ -347,6 +347,8 @@ igc_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, 
>> uint16_t nb_pkts)
>>  
>>  rxm->data_off = RTE_PKTMBUF_HEADROOM;
>>  data_len = rte_le_to_cpu_16(rxd.wb.upper.length) - rxq->crc_len;
>> +if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
>> +data_len -= IGC_TS_HDR_LEN;
>>  rxm->data_len = data_len;
>>  rxm->pkt_len = data_len;
>>  rxm->nb_segs = 1;
>> @@ -509,6 +511,12 @@ igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf 
>> **rx_pkts,
>>   */
>>  rxm->data_off = RTE_PKTMBUF_HEADROOM;
>>  data_len = rte_le_to_cpu_16(rxd.wb.upper.length);
>> +if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
>> +if (first_seg == NULL)
>> +data_len -= IGC_TS_HDR_LEN;
>> +else
>> +rxm->data_off -= IGC_TS_HDR_LEN;
> 
> This initially confused me, because I was assuming that data_off was a typo
> for data_len. However, then I realised on closer examination that, when
> timestamp offload is enabled, the actual buffer addresses sent down to the
> hardware are offset by IGC_TS_HDR_LEN (meaning the first buffer of a pkt
> has the start of data at "normal" data_offset in mbuf, but subsequent
> buffers need adjustment). Is my understanding of the issue correct?

This is exactly right. Took me a bit to understand what was happening here and
I do not know if there might be a better way do this than to change the start
offset of all descriptors. But there is probably a reason why it was done this 
way.
Initially the issue was detected as forwarding of packets that maxed out the MTU
failed since they now, with the increased length, exceeded the MTU. Only then it
became apparent that the scatter-gather handling was also broken.

> 
> In either case, I have two small bits of feedback on this:
> * Firstly, I think this needs a comment explaining the logic here, to avoid
> others being confused as I was.

This will be part of patch v2.

> * Secondly, a very minor point, but is the code clearer or shorter, if you
> merge this extra code down into the next block which is already checking
> for the first_segment of a packet or not?

I would actually prefer to keep it that way as not to mix up the the data buffer
address and length handling with the mbuf chain construction.

> 
>> +}
>>  rxm->data_len = data_len;
>>  
>>  /*
>> @@ -557,6 +565,7 @@ igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf 
>> **rx_pkts,
>>  last_seg->data_len = last_seg->data_len -
>>   (RTE_ETHER_CRC_LEN - data_len);
>>  last_seg->next = NULL;
>> +rxm = last_seg;
>>  } else {
>>  rxm->data_len = (uint16_t)
>>  (data_len - RTE_ETHER_CRC_LEN);
>> -- 
>> 2.47.0

Re: [PATCH v2] igc: fix invalid length and corrupted multi-segment mbufs

[Bruce Richardson]

On Fri, Nov 01, 2024 at 02:57:26PM +0100, Martin Weiser wrote:
> 
> The issue only appeared with hardware-timestamping enabled
> (RTE_ETH_RX_OFFLOAD_TIMESTAMP).
> 
> The length of the prepended hardware timestamp was not subtracted from
> the data length so that received packets were 16 bytes longer than
> expected.
> 
> In scatter-gather mode only the first mbuf has a timestamp but the
> data offset of the follow-up mbufs was not adjusted accordingly.
> This caused 16 bytes of packet data to be missing between
> the segments.
> 

Fixes: 4f6fbbf6f17d ("net/igc: support IEEE 1588 PTP")


> Signed-off-by: Martin Weiser 
> ---
Acked-by: Bruce Richardson 

Re: [PATCH v2] igc: fix invalid length and corrupted multi-segment mbufs

[Bruce Richardson]

On Fri, Nov 01, 2024 at 02:30:00PM +, Bruce Richardson wrote:
> On Fri, Nov 01, 2024 at 02:57:26PM +0100, Martin Weiser wrote:
> > 
> > The issue only appeared with hardware-timestamping enabled
> > (RTE_ETH_RX_OFFLOAD_TIMESTAMP).
> > 
> > The length of the prepended hardware timestamp was not subtracted from
> > the data length so that received packets were 16 bytes longer than
> > expected.
> > 
> > In scatter-gather mode only the first mbuf has a timestamp but the
> > data offset of the follow-up mbufs was not adjusted accordingly.
> > This caused 16 bytes of packet data to be missing between
> > the segments.
> > 
> 
> Fixes: 4f6fbbf6f17d ("net/igc: support IEEE 1588 PTP")
> 
> 
> > Signed-off-by: Martin Weiser 
> > ---
> Acked-by: Bruce Richardson 

Applied to dpdk-next-net-intel.

Thanks,
/Bruce

Re: [PATCH v1 1/2] net/macb: add new driver

[Stephen Hemminger]

On Fri,  1 Nov 2024 10:07:19 +
liwencheng  wrote:

> +int macb_uio_init(const char *name, struct macb_iomem **iomem)
> +{
> + struct macb_iomem *new;
> + int ret;
> +
> + new = malloc(sizeof(struct macb_iomem));
> + if (!new) {
> + MACB_LOG(ERR, "No memory for IOMEM obj.");
> + return -ENOMEM;
> + }
> + memset(new, 0, sizeof(struct macb_iomem));
> +
> + new->name = malloc(strlen(name) + 1);
> + if (!new->name) {
> + MACB_LOG(ERR, "No memory for IOMEM-name obj.");
> + ret = -ENOMEM;
> + goto out_free;
> + }
> +
> + memcpy(new->name, name, strlen(name));
> + new->name[strlen(name)] = '\0';

This looks like you just reinvented strdup() function and
did with calling strlen() multiple times.

Re: [PATCH v2 06/13] net/txgbe: check length of Tx packets

[Ferruh Yigit]

On 11/1/2024 2:56 AM, Stephen Hemminger wrote:
> On Fri, 1 Nov 2024 01:22:47 +
> Ferruh Yigit  wrote:
> 
>> Hi Jiawen,
>>
>> Above are generic checks, we may add this function to ethdev driver
>> header (ethdev_driver.h) so that any PMD can use it, what do you think?
> 
> This is in the fastpath, and additional checks should not be added there.
> Or at least put them under RTE_ETHDEV_DEBUG.
>

It is in the datapath, and ideally there shouldn't be error checks in
datapath.
But if HW does not check for errors and doesn't recover from error
conditions, driver may prefer to get the hit and do the checks.

Not for all drivers, but for the drivers that intentionally wants do the
check, I thought having a common helper function helps but I can see it
can create confusion.

@Jiawen, please continue as it is in next version, I mean have the
checks function in the driver itself.

Re: [PATCH v02] net/af_packet: add rollover and defrag options

[Ferruh Yigit]

On 10/31/2024 1:50 PM, Gur Stavi wrote:
> net_af_packet PMD multi "queue" support relies on Linux FANOUT capability.
> Linux FANOUT is a SW based load balancer that is similar to HW RSS which
> is more common for DPDK PMDs. Instead of multiple HW descriptor queues,
> AF PACKET uses multiple sockets.
> HW RSS will typically drop a packet if its selected RX queue is empty.
> However, Linux FANOUT, as a SW load balancer, can be configured to avoid
> this packet drop by rolling over to the next socket.
> This rollover functionality was ALWAYS enabled in net_af_packet. It is
> surrounded by ifdef, but only to allow compilation on ancient Linux
> versions that did not have it.
> 
> Since DPDK applications are usually designed for HW based PMDs, this
> rollover functionality, which the developers are likely unaware of, could
> be confusing.
> 
> Another option that is part of Linux FANOUT is DEFRAG that instructs Linux
> to compose complete IP packet out of fragments before delivering it to the
> PACKET socket. Again, this behavior typically does not exist for HW based
> PMDs and may confuse users.
> 
> This patch adds 2 options to control these features:
> rollover=[0|1],defrag=[0|1]
> For backward compatibility both features are enabled by default even
> though most users will probably want both of them disabled.
> 
> Signed-off-by: Gur Stavi 
> ---
> v2:
> * Improve error handling for parsing of arg strings. Add a wrapper around
>   strtoul to replace atoi.
> * Fix checkpatch indentation error due to copy-paste.
> 
> v1: https://mails.dpdk.org/archives/dev/2024-October/307451.html
> ---
>  doc/guides/nics/af_packet.rst |  4 ++
>  drivers/net/af_packet/rte_eth_af_packet.c | 77 ++-
>  2 files changed, 65 insertions(+), 16 deletions(-)
> 
> diff --git a/doc/guides/nics/af_packet.rst b/doc/guides/nics/af_packet.rst
> index 66b977e1a2..4bc748b50b 100644
> --- a/doc/guides/nics/af_packet.rst
> +++ b/doc/guides/nics/af_packet.rst
> @@ -29,6 +29,8 @@ Some of these, in turn, will be used to configure the 
> PACKET_MMAP settings.
>  *   ``framesz`` - PACKET_MMAP frame size (optional, default 2048B; Note: 
> multiple
>  of 16B);
>  *   ``framecnt`` - PACKET_MMAP frame count (optional, default 512).
> +*   ``rollover`` - disable(0)/enable(1) PACKET_FANOUT_ROLLOVER (optional, 
> default 1).
> +*   ``defrag`` - disable(0)/enable(1) PACKET_FANOUT_FLAG_DEFRAG (optional, 
> default 1).
> 

+1 to introduce both options, thanks.
Do you have any practical usecase to disable them?


>  Because this implementation is based on PACKET_MMAP, and PACKET_MMAP has its
>  own pre-requisites, it should be noted that the inner workings of PACKET_MMAP
> @@ -47,6 +49,8 @@ For the full details behind PACKET_MMAP's structures and 
> settings, consider
>  reading the `PACKET_MMAP documentation in the Kernel
>  `_.
> 
> +For details of ``rollover`` anb ``defrag`` refer to the *packet(7)* man page.
> +
>  Prerequisites
>  -
> 
> diff --git a/drivers/net/af_packet/rte_eth_af_packet.c 
> b/drivers/net/af_packet/rte_eth_af_packet.c
> index 68870dd3e2..1a043e8398 100644
> --- a/drivers/net/af_packet/rte_eth_af_packet.c
> +++ b/drivers/net/af_packet/rte_eth_af_packet.c
> @@ -36,6 +36,8 @@
>  #define ETH_AF_PACKET_FRAMESIZE_ARG  "framesz"
>  #define ETH_AF_PACKET_FRAMECOUNT_ARG "framecnt"
>  #define ETH_AF_PACKET_QDISC_BYPASS_ARG   "qdisc_bypass"
> +#define ETH_AF_PACKET_ROLLOVER   "rollover"
> +#define ETH_AF_PACKET_DEFRAG "defrag"
> 

Can you please add new arguments to 'RTE_PMD_REGISTER_PARAM_STRING'
macro below?

>  #define DFLT_FRAME_SIZE  (1 << 11)
>  #define DFLT_FRAME_COUNT (1 << 9)
> @@ -91,6 +93,8 @@ static const char *valid_arguments[] = {
>   ETH_AF_PACKET_FRAMESIZE_ARG,
>   ETH_AF_PACKET_FRAMECOUNT_ARG,
>   ETH_AF_PACKET_QDISC_BYPASS_ARG,
> + ETH_AF_PACKET_ROLLOVER,
> + ETH_AF_PACKET_DEFRAG,
>   NULL
>  };
> 
> @@ -659,6 +663,20 @@ open_packet_iface(const char *key __rte_unused,
>   return 0;
>  }
> 
> +static int
> +uint_arg_parse(const char *arg_str, unsigned int *arg_val)
> +{
> + unsigned long val;
> + char *endptr;
> +
> + val = strtoul(arg_str, &endptr, 10);
> + if (*arg_str == '\0' || *endptr != '\0')
> + return -EINVAL;
> +
> + *arg_val = val;
> + return 0;
> +}
> +
>  static int
>  rte_pmd_init_internals(struct rte_vdev_device *dev,
> const int sockfd,
> @@ -676,6 +694,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>   const unsigned int numa_node = dev->device.numa_node;
>   struct rte_eth_dev_data *data = NULL;
>   struct rte_kvargs_pair *pair = NULL;
> + const char *ifname = NULL;
>   struct ifreq ifr;
>   size_t ifnamelen;
>   unsigned k_idx;
> @@ -686,6 +705,8 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>   int rc, tpver, discard;
>  

Re: [PATCH v9 6/9] net/zxdh: add zxdh get device backend infos

[Ferruh Yigit]

On 11/1/2024 6:21 AM, Junlong Wang wrote:
> Add zxdh get device backend infos,
> use msg chan to send msg get.
> 
> Signed-off-by: Junlong Wang 
> 

<...>

> +uint16_t zxdh_vport_to_vfid(union zxdh_virport_num v)
> +{
> +/* epid > 4 is local soft queue. return 1192 */
> +if (v.epid > 4)
> +return 1192;
> +if (v.vf_flag)
> +return v.epid * 256 + v.vfid;
> +else
> +return (v.epid * 8 + v.pfid) + 1152;
> +}
>

Is there a reason to not make this function static? This way can get rid
of the decleration in the header file.

Re: [PATCH 0/4] net/tap: minor cleanups

[Ferruh Yigit]

On 10/28/2024 4:32 PM, Stephen Hemminger wrote:
> These are some minor things in tap device that can be easily fixed.
> Not urgent can wait until 25.03 release but submitting now for review.
> 
> Stephen Hemminger (4):
>   net/tap: replace rte_memcpy
>   net/tap: rename struct nlmsg to tap_nlmsg
>   net/tap: do not use opaque parameter in convert
>   net/tap: do not use rte_malloc for process private data
> 

For the series,
Acked-by: Morten Brørup 
Acked-by: Ferruh Yigit 


Series applied to dpdk-next-net/main, thanks.

Re: [PATCH v9 1/9] net/zxdh: add zxdh ethdev pmd driver

[Ferruh Yigit]

On 11/1/2024 6:21 AM, Junlong Wang wrote:
> Add basic zxdh ethdev init and register PCI probe functions
> Update doc files.
> 
> Signed-off-by: Junlong Wang 
> ---
>  MAINTAINERS|  6 ++
>  doc/guides/nics/features/zxdh.ini  |  9 +++
>  doc/guides/nics/index.rst  |  1 +
>  doc/guides/nics/zxdh.rst   | 31 +
>  doc/guides/rel_notes/release_24_11.rst |  4 ++
>  drivers/net/meson.build|  1 +
>  drivers/net/zxdh/meson.build   | 18 +
>  drivers/net/zxdh/zxdh_ethdev.c | 92 ++
>  drivers/net/zxdh/zxdh_ethdev.h | 44 
>  9 files changed, 206 insertions(+)
>  create mode 100644 doc/guides/nics/features/zxdh.ini
>  create mode 100644 doc/guides/nics/zxdh.rst
>  create mode 100644 drivers/net/zxdh/meson.build
>  create mode 100644 drivers/net/zxdh/zxdh_ethdev.c
>  create mode 100644 drivers/net/zxdh/zxdh_ethdev.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8919d78919..a5534be2ab 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1051,6 +1051,12 @@ F: drivers/net/virtio/
>  F: doc/guides/nics/virtio.rst
>  F: doc/guides/nics/features/virtio*.ini
>  
> +ZTE zxdh
> +M: Lijie Shan 
>

You have your sign-off in the patch series, but adding someone else as
maintainer?
We need someone that has technical expertise on the code, is there a
reason to not add your name as maintainer.

> +F: drivers/net/zxdh/
> +F: doc/guides/nics/zxdh.rst
> +F: doc/guides/nics/features/zxdh.ini
> +
>

Minor comment, rest looks good to me:

Please move this below "Wind River", this list is alphabetically shorted
on company name. Last bit of the list is virtual drivers without
specific company associated with them.

<...>

> diff --git a/drivers/net/zxdh/zxdh_ethdev.c b/drivers/net/zxdh/zxdh_ethdev.c
> new file mode 100644
> index 00..5b6c9ec1bf
> --- /dev/null
> +++ b/drivers/net/zxdh/zxdh_ethdev.c
> @@ -0,0 +1,92 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2024 ZTE Corporation
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#include "zxdh_ethdev.h"
> +
> +static int zxdh_eth_dev_init(struct rte_eth_dev *eth_dev)
> +{
>

DPDK syntax is to have return value in a separate line, like:
static int
zxdh_eth_dev_init(struct rte_eth_dev *eth_dev)
{

This is for all files in this series, can you please update all?

Re: [PATCH v9 7/9] net/zxdh: add configure zxdh intr implementation

[Ferruh Yigit]

On 11/1/2024 6:21 AM, Junlong Wang wrote:
> configure zxdh intr include risc,dtb. and release intr.
> 
> Signed-off-by: Junlong Wang 
> 

<...>

> +static int32_t zxdh_configure_intr(struct rte_eth_dev *dev)
> +{
> +struct zxdh_hw *hw = dev->data->dev_private;
> +int32_t ret = 0;
> +
> +if (!rte_intr_cap_multiple(dev->intr_handle)) {
> +PMD_INIT_LOG(ERR, "Multiple intr vector not supported");
> +return -ENOTSUP;
> +}
> +zxdh_intr_release(dev);
> +uint8_t nb_efd = ZXDH_MSIX_INTR_DTB_VEC_NUM + ZXDH_MSIX_INTR_MSG_VEC_NUM;
> +
> +if (dev->data->dev_conf.intr_conf.rxq)
> +nb_efd += dev->data->nb_rx_queues;
> +
> +if (rte_intr_efd_enable(dev->intr_handle, nb_efd)) {
> +PMD_INIT_LOG(ERR, "Fail to create eventfd");
> +return -1;
> +}
> +
> +if (rte_intr_vec_list_alloc(dev->intr_handle, "intr_vec",
> +hw->max_queue_pairs + ZXDH_INTR_NONQUE_NUM)) {
> +PMD_INIT_LOG(ERR, "Failed to allocate %u rxq vectors",
> +hw->max_queue_pairs + ZXDH_INTR_NONQUE_NUM);
> +return -ENOMEM;
> +}
> +PMD_INIT_LOG(DEBUG, "allocate %u rxq vectors", dev->intr_handle-
>>vec_list_size);
> +if (zxdh_setup_risc_interrupts(dev) != 0) {
> +PMD_INIT_LOG(ERR, "Error setting up rsic_v interrupts!");
> +ret = -1;
> +goto free_intr_vec;
> +}
> +if (zxdh_setup_dtb_interrupts(dev) != 0) {
> +PMD_INIT_LOG(ERR, "Error setting up dtb interrupts!");
> +ret = -1;
> +goto free_intr_vec;
> +}
> +
> +if (zxdh_queues_bind_intr(dev) < 0) {
> +PMD_INIT_LOG(ERR, "Failed to bind queue/interrupt");
> +ret = -1;
> +goto free_intr_vec;
> +}
> +
> +if (zxdh_intr_enable(dev) < 0) {
> +PMD_DRV_LOG(ERR, "interrupt enable failed");
> +ret = -1;
> +goto free_intr_vec;
> +}
>

One of the above log use 'PMD_INIT_LOG()' and other 'PMD_DRV_LOG()', how
do you diffrentiate?
Do you really need two different log type, init and driver? (I
understand need for others, Rx, Tx & msg, but still less is easier)

Re: [PATCH v9 2/9] net/zxdh: add logging implementation

[Ferruh Yigit]

On 11/1/2024 6:21 AM, Junlong Wang wrote:
> Add zxdh logging implementation.
> 
> Signed-off-by: Junlong Wang 
> 

<...>

> +extern int zxdh_logtype_init;
> +#define RTE_LOGTYPE_ZXDH_INIT zxdh_logtype_init
> +#define PMD_INIT_LOG(level, ...) \
> +RTE_LOG_LINE_PREFIX(level, ZXDH_INIT, "offload_zxdh %s(): ", \
>

Are you sure you want "offload_zxdh" prefix for each log, instead of
shorter 'zxdh' one?

> +__func__, __VA_ARGS__)
> +
> +extern int zxdh_logtype_driver;
> +#define RTE_LOGTYPE_ZXDH_DRIVER zxdh_logtype_driver
> +#define PMD_DRV_LOG(level, ...) \
> +RTE_LOG_LINE_PREFIX(level, ZXDH_DRIVER, "offload_zxdh %s(): ", \
>

All log types seems same prefix, which is OK, but just a reminder if you
want to distinguish them?

[PATCH v2] graph: fix memory leak in node clone

[pbhagavatula]

From: Pavan Nikhilesh 

Free memory allocated for the node when xstats memory
allocation fails.

Coverity issue: 445529
Fixes: 070db97e017b ("graph: support node xstats")

Signed-off-by: Pavan Nikhilesh 
---
v2 Changes:
- Fix one more leak. (Huichao cai)

 lib/graph/node.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/graph/node.c b/lib/graph/node.c
index f15922892e..63db629da8 100644
--- a/lib/graph/node.c
+++ b/lib/graph/node.c
@@ -156,7 +156,7 @@ node_clone(struct node *node, const char *name)
 (node->xstats->nb_xstats * 
RTE_NODE_XSTAT_DESC_SIZE));
if (reg->xstats == NULL) {
rte_errno = ENOMEM;
-   goto fail;
+   goto free;
}

for (i = 0; i < node->xstats->nb_xstats; i++)
@@ -178,7 +178,7 @@ node_clone(struct node *node, const char *name)

/* Naming ceremony of the new node. name is node->name + "-" + name */
if (clone_name(reg->name, node->name, name))
-   goto free;
+   goto free_xstat;

rc = __rte_node_register(reg);
 free_xstat:
--
2.25.1

[PATCH] net/ice: fix wrong DDP search path

[Zhichao Zeng]

In the previous implementation, when the user did not enter any value in
"/sys/module/firmware_class/parameters/path", it would incorrectly search
for DDP packages under "/". This commit fixes this issue.

Fixes: 9207f93640a7 ("net/ice: support custom search path for DDP package")

Signed-off-by: Zhichao Zeng 
---
 drivers/net/ice/ice_ethdev.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index d5e94a6685..0705f8e961 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -1922,8 +1922,11 @@ static int ice_read_customized_path(char *pkg_file, 
uint16_t buff_len)
return -EIO;
}
 
-   if (pkg_file[n - 1] == '\n')
+   if (pkg_file[n - 1] == '\n') {
n--;
+   if (n == 0)
+   return -EINVAL;
+   }
 
pkg_file[n] = '\0';
 
-- 
2.34.1

Re: [PATCH] net/ice: fix wrong DDP search path

[Bruce Richardson]

On Fri, Nov 01, 2024 at 04:44:43PM +0800, Zhichao Zeng wrote:
> In the previous implementation, when the user did not enter any value in
> "/sys/module/firmware_class/parameters/path", it would incorrectly search
> for DDP packages under "/". This commit fixes this issue.
> 
> Fixes: 9207f93640a7 ("net/ice: support custom search path for DDP package")
> 
> Signed-off-by: Zhichao Zeng 
> ---
>  drivers/net/ice/ice_ethdev.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index d5e94a6685..0705f8e961 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -1922,8 +1922,11 @@ static int ice_read_customized_path(char *pkg_file, 
> uint16_t buff_len)
>   return -EIO;
>   }
>  
> - if (pkg_file[n - 1] == '\n')
> + if (pkg_file[n - 1] == '\n') {
>   n--;
> + if (n == 0)
> + return -EINVAL;
> + }
>  
>   pkg_file[n] = '\0';
>  

May I suggest a slightly alternative fix, that I think it a little shorter
and neater (assuming it works - if not, let me know.)

Rather than adding an explicit check for n==0 and returning error, I think
we can instead change the return value of the function to be the length of
the data read. So at line 1931 "return 0" we can change that to "return n".
Then at the call site for the function we can change:

if (ice_read_customized_path() == 0) {

to 
if (ice_read_customized_path() > 0) {

What do you think?

/Bruce

PS: if you do take this approach, we can also slightly shorten the function
by changing/removing the block:

if (n == 0) {
close(fp);
return -EIO;
}
... /* length adjust and zeroing */
return n;

to be an inverted check with the length adjustment and zeroing inside it:
if (n > 0){
if (pkg_file[n -1] == '\n')
n--;
pkg_file[n] = '\0';
}
close(fp);
return n;

That gives us a zero return value too in case of reading zero bytes. It
also handles the currently unhandled case of read returning < 0.

Re: [PATCH v4 00/42] replace strerror

[huangdengdui]

On 2024/10/26 5:56, Thomas Monjalon wrote:
> 24/10/2024 08:47, huangdengdui:
>> On 2024/10/23 23:42, Stephen Hemminger wrote:
>>> On Wed, 23 Oct 2024 16:28:10 +0800
>>> Dengdui Huang  wrote:
>>>
 The function strerror() is insecure in a multi-thread environment.
 It is better to use rte_strerror() instead of strerror().
 In this patchset, only the libs and drivers are modified.
>>>
>>> Even rte_strerror is not completely safe. It depends on the calling
>>> thread being a registered lcore.
>>
>> As discussed earlier, it is still safe if used from non-DPDK registered 
>> threads[1]:
>>
>> #define RTE_DEFINE_PER_LCORE(type, name) \
>>  __thread __typeof__(type) per_lcore_##name
>>
>> [1]: 
>> https://elixir.bootlin.com/dpdk/v23.11-rc3/source/lib/eal/include/rte_per_lcore.h#L37
>>
>>>
>>> It would be better to use a coccinelle script to do direct replacement
>>> with strerror_r().
>>>
>>> Also, rte_strerror is not signal safe.
>>
>> Can we use strerror_r() in the signal processing context and replace it with 
>> rte_strerror() everywhere else?
> 
> It does not make sense to use rte_strerror after libc functions.
> Please restrict the use of rte_strerror for error numbers from DPDK functions.
> 
> 

The Windows platform does not support strerror_r(). Using strerror_r() instead 
of strerror() is not a good idea,
Platform differences have been handled in rte_strerror()[1].
The rte_strerror() can also handle errno from libc functions[2][3].
So is it better to use rte_strerror() instead of strerror()?

[1]https://elixir.bootlin.com/dpdk/v23.11-rc3/source/lib/eal/common/eal_common_errno.c#L15
[2]https://elixir.bootlin.com/dpdk/v23.11-rc3/source/lib/eal/common/eal_common_errno.c#L49
[3]https://elixir.bootlin.com/dpdk/v23.11-rc3/source/lib/eal/include/rte_errno.h#L32

[PATCH v1 2/2] /usertools/dpdk-devbind:add the binding and unbinding of platform device

[liwencheng]

This patch mainly adds functions for binding and unbinding platform devices, 
such as bind_platform_one and unbind_platform_one.

Signed-off-by: liwencheng 
---
 usertools/dpdk-devbind.py | 101 +-
 1 file changed, 99 insertions(+), 2 deletions(-)

diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
index 80c35f9..4b65b55 100755
--- a/usertools/dpdk-devbind.py
+++ b/usertools/dpdk-devbind.py
@@ -147,10 +147,32 @@ def module_is_loaded(module):
 return module in loaded_modules
 
 
+def get_platform_devices():
+global platform_devices
+
+platform_device_path = "/sys/bus/platform/devices/"
+platform_devices = os.listdir(platform_device_path)
+
+
+def devices_are_platform(devs):
+all_devices_are_platform = True
+
+get_platform_devices()
+for d in devs:
+if d not in platform_devices:
+all_devices_are_platform = False
+break
+
+return all_devices_are_platform
+
+
 def check_modules():
 '''Checks that igb_uio is loaded'''
 global dpdk_drivers
 
+if devices_are_platform(args):
+return
+
 # list of supported modules
 mods = [{"Name": driver, "Found": False} for driver in dpdk_drivers]
 
@@ -321,11 +343,38 @@ def dev_id_from_dev_name(dev_name):
 for d in devices.keys():
 if dev_name in devices[d]["Interface"].split(","):
 return devices[d]["Slot"]
+
+# Check if it is a platform device
+if dev_name in platform_devices:
+return dev_name
+
 # if nothing else matches - error
 raise ValueError("Unknown device: %s. "
  "Please specify device in \"bus:slot.func\" format" % 
dev_name)
 
 
+def unbind_platform_one(dev_name):
+filename = "/sys/bus/platform/devices/%s/driver" % dev_name
+
+if exists(filename):
+try:
+f = open(os.path.join(filename, "unbind"), "w")
+except OSError as err:
+sys.exit("Error: unbind failed for %s - Cannot open %s: %s" %
+ (dev_name, os.path.join(filename, "unbind"), err))
+f.write(dev_name)
+f.close()
+filename = "/sys/bus/platform/devices/%s/driver_override" % dev_name
+try:
+f = open(filename, "w")
+except OSError as err:
+sys.exit("Error: unbind failed for %s - Cannot open %s: %s" %
+ (dev_name, filename, err))
+f.write("")
+f.close()
+print("Successfully unbind platform device %s" % dev_name)
+
+
 def unbind_one(dev_id, force):
 '''Unbind the device identified by "dev_id" from its current driver'''
 dev = devices[dev_id]
@@ -351,6 +400,48 @@ def unbind_one(dev_id, force):
 f.close()
 
 
+def bind_platform_one(dev_name, driver):
+filename = "/sys/bus/platform/drivers/%s" % driver
+
+if not exists(filename):
+print("The driver %s is not loaded" % driver)
+return
+# unbind any existing drivers we don't want
+filename = "/sys/bus/platform/devices/%s/driver" % dev_name
+if exists(filename):
+unbind_platform_one(dev_name)
+# driver_override can be used to specify the driver
+filename = "/sys/bus/platform/devices/%s/driver_override" % dev_name
+if exists(filename):
+try:
+f = open(filename, "w")
+except OSError as err:
+sys.exit("Error: unbind failed for %s - Cannot open %s: %s"
+ % (dev_name, filename, err))
+try:
+f.write(driver)
+f.close()
+except OSError as err:
+sys.exit("Error: unbind failed for %s - Cannot write %s: %s"
+ % (dev_name, filename, err))
+# do the bind by writing to /sys
+filename = "/sys/bus/platform/drivers/%s/bind" % driver
+try:
+f = open(filename, "w")
+except OSError as err:
+print("Error: bind failed for %s - Cannot open %s: %s"
+  % (dev_name, filename, err), file=sys.stderr)
+return
+try:
+f.write(dev_name)
+f.close()
+except OSError as err:
+print("Error: bind failed for %s - Cannot bind to driver %s: %s"
+  % (dev_name, driver, err), file=sys.stderr)
+return
+print("Successfully bind platform device %s to driver %s" % (dev_name, 
driver))
+
+
 def bind_one(dev_id, driver, force):
 '''Bind the device given by "dev_id" to the driver "driver". If the device
 is already bound to a different driver, it will be unbound first'''
@@ -475,7 +566,10 @@ def unbind_all(dev_list, force=False):
 sys.exit(1)
 
 for d in dev_list:
-unbind_one(d, force)
+if d in platform_devices:
+unbind_platform_one(d)
+else:
+unbind_one(d, force)
 
 
 def has_iommu():
@@ -537,7 +631,10 @@ def bind_all(dev_list, driver, force=False):
 check_noiommu_mode()
 
 for d in dev_list:
-bind_one(d, driver, force)
+if d in platfor

Re: [PATCH] net/ice: enable link speed 200G

[Bruce Richardson]

On Thu, Oct 31, 2024 at 01:18:19AM +, Mingjin Ye wrote:
> ICE Enable 200G link speed capability.
> 
> Fixes: 36afbc269081 ("net/ice: support link speed change")

I don't think this is a fix, more new feature support, so will drop the
fixes line on apply.

> 
> Signed-off-by: Mingjin Ye 

Patch looks good to me, thanks.

Acked-by: Bruce Richardson 

Applied to dpdk-next-net-intel

Thanks,
/Bruce

Re: [PATCH v3 1/2] net/ixgbe: increase the maximum of RX/TX descriptors

[Bruce Richardson]

On Wed, Oct 30, 2024 at 05:26:12PM +0100, Morten Brørup wrote:
> > From: Lukas Sismis [mailto:sis...@cesnet.cz]
> > Sent: Wednesday, 30 October 2024 16.43
> > 
> > Intel PMDs are capped by default to only 4096 RX/TX descriptors.
> > This can be limiting for applications requiring a bigger buffer
> > capabilities. By bufferring more packets with RX/TX
> > descriptors, the applications can better handle the processing
> > peaks.
> > 
> > Setting ixgbe max descriptors to 8192 as per datasheet:
> > Register name: RDLEN
> > Description: Descriptor Ring Length.
> > This register sets the number of bytes
> > allocated for descriptors in the circular descriptor buffer.
> > It must be 128B aligned (7 LS bit must be set to zero).
> > ** Note: validated Lengths up to 128K (8K descriptors). **

FYI: Don't think we need the full quote from the datasheet, reducing this to
a one-line summary on apply.

> > 
> > Signed-off-by: Lukas Sismis 
> > ---
> 
> Drivers should reflect hardware capabilities; it's not up to the driver to 
> impose artificial limits on applications. Thank you for fixing this, Lukas.
> 
> Acked-by: Morten Brørup 
> 
Series-acked-by: Bruce Richardson 

Both patches applied to dpdk-next-net-intel tree.

Thanks,
/Bruce