[PWCI] "DPAA specific changes" #181
Conversation
adding a FQ shutdown functionality to ensure proper cleanup of frame queues during queue setup. This helps reset the queues reliably and prevents potential resource leaks or stale state issues. Additionally, update logging to use DPAA_BUS_ERR instead of pr_err for better consistency and clarity in error reporting within the DPAA bus subsystem. These changes enhance maintainability and improve debugging experience. Signed-off-by: Gagandeep Singh <g.singh@nxp.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Enable IEEE1588 by timesync API instead of devargs. DPAA1 HW parser has no capability to identify ptp packets from ingress traffic, remove ptp identification code from driver RX callback which impacts performance significantly. Signed-off-by: Jun Yang <jun.yang@nxp.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
ShareMAC port is still alive after dpdk process quits but RXQ setup in dpdk process is in invalid state. If high loading ingress traffic hits the FMan PCD then it's en-queued to the RXQ to generate frequent interrupts. This causes system stuck. User can disable this kind of interrupt by env to avoid this issue: export DPAA_QMAN_IESR_ISR_DISABLE=1 Signed-off-by: Jun Yang <jun.yang@nxp.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Max push queue number is bus level number affecting all dpaa devices. Move the configuration from PMD driver to bus driver. Signed-off-by: Jun Yang <jun.yang@nxp.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Fix CID:362787 issue. This patch correct the cleanup order of PCD and FMan handles to prevent resource leaks. Fixes: 4defbc8 ("net/dpaa: support FMCless mode") Cc: sachin.saxena@nxp.com Cc: stable@dpdk.org Signed-off-by: Vanshika Shukla <vanshika.shukla@nxp.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
WalkthroughThis PR updates DPAA bus, QMan, and net/dpaa to add push-queue management, per-interface PTP/timestamping, and enhanced QMan portal control/logging. It adjusts APIs (qman_portal_poll_rx signature), introduces IRQ masking via env var, adds a PTP header parser, replaces global IEEE 1588 toggles with per-interface fields, and refactors RX/TX paths accordingly. Docs drop the IEEE 1588 driver arg. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant EthDev as dpaa_ethdev
participant PTP as dpaa_ptp
participant QMan as qman portal
participant IF as dpaa_if
App->>EthDev: rte_eth_timesync_enable(dev)
EthDev->>PTP: dpaa_timesync_enable(dev)
PTP->>IF: init per-core TX conf queues
PTP-->>EthDev: ts_enable = true
App->>QMan: RX poll (qman_portal_poll_rx with cb)
QMan-->>EthDev: deliver DQRR entries via cb
EthDev->>IF: if ts_enable, parse annot timestamp
EthDev-->>App: mbufs with timestamps (if present)
sequenceDiagram
autonumber
participant Bus as rte_dpaa_bus
participant Env as Environment
participant Dev as dpaa_ethdev
participant IF as dpaa_if
Env-->>Bus: DPAA_PUSH_QUEUES_NUMBER (optional)
Bus->>Bus: set max_push_rxq_num (cap, LS1043A override)
Dev->>Bus: dpaa_push_queue_max_num()
Dev->>Bus: dpaa_push_queue_num_update()
alt capacity available
Bus-->>Dev: success
Dev->>IF: enable push-mode RXQ
else capacity reached
Bus-->>Dev: fail
Dev->>IF: fallback to pull-mode
end
sequenceDiagram
autonumber
participant App
participant QMan as qman.c
participant FQ as Frame Queue
App->>QMan: qman_shutdown_fq_new(fqid)
QMan->>QMan: drain MR/DQRR (timeout-aware)
QMan->>FQ: send OOS/retire commands
QMan-->>App: return status (logged via DPAA_BUS_*)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drivers/bus/dpaa/base/qbman/qman.c (1)
1243-1288: Risk of array out-of-bounds access.The loop at lines 1248-1289 uses
rx_numberto index intodq[],shadow[], andfq[]arrays, all sizedQM_DQRR_SIZE. However, the loop condition is++limit < poll_limit(line 1289), wherepoll_limitis user-provided. Ifpoll_limit > QM_DQRR_SIZE,rx_numberwill exceed array bounds at line 1253 onwards.Add bounds check:
do { qm_dqrr_pvb_update(&p->p); if (!dqrr->fill) break; + if (rx_number >= QM_DQRR_SIZE) { + DPAA_BUS_ERR("rx_number %u exceeds QM_DQRR_SIZE %u", + rx_number, QM_DQRR_SIZE); + break; + } dq[rx_number] = dqrr->cursor;Alternatively, clamp
poll_limittoQM_DQRR_SIZEat function entry.
🧹 Nitpick comments (7)
drivers/bus/dpaa/base/qbman/qman.c (1)
2793-2987: Consider decomposing large function for readability.
qman_shutdown_fq_newis 195 lines long and handles multiple FQ states with nested conditionals. While the logic appears correct, extracting helper functions (e.g.,retire_and_drain_fq,transition_to_oos) would improve maintainability and testability.Example refactor:
static int retire_and_drain_fq(struct qman_portal *p, u32 fqid, u32 channel, u32 wq) { /* Lines 2838-2941 extracted here */ } int qman_shutdown_fq_new(u32 fqid) { /* Query state */ /* Switch on state: * case ACTIVE/PARKED: retire_and_drain_fq(...) * case RETIRED: transition_to_oos(...) */ }drivers/net/dpaa/dpaa_rxtx.c (5)
363-368: Consider explicit NULL check for PTP header parsing.The code implicitly relies on the truthiness of the pointer returned by
dpaax_timesync_ptp_parse_header. While this works, an explicit NULL check would improve clarity and make the intent more obvious.Apply this diff for better clarity:
if (unlikely(dpaa_intf->ts_enable)) { - if (dpaax_timesync_ptp_parse_header(m, NULL, NULL)) { + struct rte_dpaax_ptp_header *ptp = dpaax_timesync_ptp_parse_header(m, NULL, NULL); + if (ptp != NULL) { m->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP; m->ol_flags |= RTE_MBUF_F_RX_IEEE1588_TMST; } }
701-705: Consider validating timestamp data before use.The timestamp is extracted from annotations and stored in
dpaa_intf->rx_timestampwithout validating that the annotation contains valid timestamp data. While this may be safe in the current context, explicit validation would improve robustness.Consider adding a validation check or documenting the assumption that annotations always contain valid timestamps when
ts_enableis true.
749-753: Duplicate timestamp extraction logic.The timestamp extraction logic here is identical to lines 704-705. Consider extracting this into a helper function to reduce duplication and improve maintainability.
Example helper function:
static inline void dpaa_extract_rx_timestamp(struct dpaa_if *dpaa_intf, struct annotations_t *annot) { if (unlikely(dpaa_intf->ts_enable)) dpaa_intf->rx_timestamp = rte_be_to_cpu_64(annot->timestamp); }Then call it from both locations:
- if (unlikely(dpaa_intf->ts_enable)) - dpaa_intf->rx_timestamp = rte_be_to_cpu_64(annot->timestamp); + dpaa_extract_rx_timestamp(dpaa_intf, annot);
957-961: Duplicated timestamp extraction logic (third instance).This is the third instance of the same timestamp extraction pattern in this file (also at lines 704-705 and 752-753). Consider extracting this into a helper function as previously suggested to eliminate duplication.
1478-1478: TX timestamp extraction lacks validation.Similar to the RX path, the TX timestamp is extracted without validating that the annotation contains valid data. Consider adding validation or documenting the assumptions.
drivers/bus/dpaa/dpaa_bus.c (1)
53-59: Unused DPAA_PUSH_RXQ_NUM_ARG and mixed config knobsDPAA_PUSH_RXQ_NUM_ARG is unused while env var DPAA_PUSH_QUEUES_NUMBER drives behavior. Either wire this devarg into parsing or remove the macro to avoid confusion; also align naming across docs/code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
doc/guides/nics/dpaa.rst(0 hunks)drivers/bus/dpaa/base/qbman/qman.c(28 hunks)drivers/bus/dpaa/base/qbman/qman_driver.c(3 hunks)drivers/bus/dpaa/bus_dpaa_driver.h(1 hunks)drivers/bus/dpaa/dpaa_bus.c(5 hunks)drivers/bus/dpaa/include/fsl_qman.h(2 hunks)drivers/common/dpaax/dpaax_ptp.h(1 hunks)drivers/net/dpaa/dpaa_ethdev.c(10 hunks)drivers/net/dpaa/dpaa_ethdev.h(3 hunks)drivers/net/dpaa/dpaa_flow.c(2 hunks)drivers/net/dpaa/dpaa_ptp.c(4 hunks)drivers/net/dpaa/dpaa_rxtx.c(22 hunks)drivers/net/dpaa/dpaa_rxtx.h(1 hunks)
💤 Files with no reviewable changes (1)
- doc/guides/nics/dpaa.rst
🧰 Additional context used
🧬 Code graph analysis (8)
drivers/bus/dpaa/bus_dpaa_driver.h (1)
drivers/bus/dpaa/dpaa_bus.c (1)
void(126-129)
drivers/net/dpaa/dpaa_ptp.c (2)
drivers/net/dpaa/dpaa_ethdev.c (1)
dpaa_tx_conf_queue_init(2054-2075)drivers/net/dpaa/dpaa_rxtx.c (1)
dpaa_eth_tx_conf(1430-1485)
drivers/bus/dpaa/base/qbman/qman_driver.c (1)
drivers/common/dpaax/compat.h (2)
in_be32(129-133)out_be32(135-139)
drivers/net/dpaa/dpaa_rxtx.c (2)
drivers/common/dpaax/dpaax_ptp.h (1)
dpaax_timesync_ptp_parse_header(26-93)drivers/bus/dpaa/base/qbman/qman.c (2)
qman_portal_poll_rx(1237-1298)qman_dqrr_consume(1411-1421)
drivers/bus/dpaa/base/qbman/qman.c (2)
drivers/bus/dpaa/include/fsl_qman.h (2)
qm_mc_result(1021-1035)QM_SDQCR_CHANNELS_POOL_CONV(73-76)drivers/bus/dpaa/base/qbman/bman.c (1)
get_affine_portal(50-53)
drivers/net/dpaa/dpaa_ethdev.h (2)
drivers/net/dpaa/dpaa_ethdev.c (1)
dpaa_tx_conf_queue_init(2054-2075)drivers/net/dpaa/dpaa_ptp.c (3)
dpaa_timesync_write_time(71-90)dpaa_timesync_adjust_time(92-107)dpaa_timesync_read_rx_timestamp(132-144)
drivers/bus/dpaa/include/fsl_qman.h (1)
drivers/bus/dpaa/base/qbman/qman.c (1)
qman_portal_poll_rx(1237-1298)
drivers/bus/dpaa/dpaa_bus.c (1)
drivers/net/dpaa/dpaa_ethdev.c (2)
RTE_EXPORT_INTERNAL_SYMBOL(1329-1392)RTE_EXPORT_INTERNAL_SYMBOL(1394-1417)
🔇 Additional comments (31)
drivers/net/dpaa/dpaa_flow.c (2)
2-2: LGTM!The copyright year update is appropriate.
893-894: LGTM! Correct cleanup order.The cleanup sequence now properly closes the PCD handle before the FMAN handle, which is the correct reverse order of initialization. Since the PCD depends on FMAN (line 879:
fm_pcd_params.h_fm = fman_handle), the dependent resource must be released first.drivers/net/dpaa/dpaa_ethdev.h (2)
156-161: LGTM: Per-interface timestamp state added.The new fields
ts_enableandtx_old_timestampproperly support per-interface IEEE 1588 timestamping, replacing the removed globaldpaa_ieee_1588flag. This enables concurrent PTP on multiple interfaces.
248-249: LGTM: TX confirmation queue initialization exposed.The declaration matches the implementation in
drivers/net/dpaa/dpaa_ethdev.cat lines 2054-2075, where it correctly creates and initializes a dynamic FQID for TX confirmation.drivers/net/dpaa/dpaa_ptp.c (2)
20-40: LGTM: Per-interface TX confirmation queues initialized correctly.The function now loops over
MAX_DPAA_CORESand initializes TX confirmation queues for each core whents_enableis activated. This supports per-interface timestamping. Error handling correctly breaks and returns on failure.
110-130: LGTM: TX timestamp polling with timeout.The busy-wait loop polls
tx_timestampuntil it differs fromtx_old_timestamp, indicating a new timestamp arrived. Theread_countlimit prevents infinite loops, returning-EAGAINon timeout. Logic is sound.drivers/net/dpaa/dpaa_ethdev.c (3)
2002-2002: LGTM: Per-interface timestamp enablement.Line 2002 correctly checks
dpaa_intf->ts_enableinstead of the removed globaldpaa_ieee_1588flag. This enables per-interface PTP control as intended by the PR.
1125-1127: No action needed—qman_shutdown_fqis declared in fsl_qman.h and exported via RTE_EXPORT_INTERNAL_SYMBOL in qman.c.
1207-1209: Declarations and exports for dpaa_push_queue_ functions verified*
Both dpaa_push_queue_num_update() and dpaa_push_queue_max_num() are declared in drivers/bus/dpaa/bus_dpaa_driver.h and exported via RTE_EXPORT_INTERNAL_SYMBOL in drivers/bus/dpaa/dpaa_bus.c.drivers/bus/dpaa/base/qbman/qman.c (2)
563-596: Portal reset sequence added but reason unclear.Lines 563-596 duplicate the portal initialization sequence (EQCR, DQRR, MR, MC init) immediately after the first initialization. The comment states "Reset portal before use" but doesn't explain why a double-init is necessary. If this addresses a hardware quirk, document it clearly. If not, this may be redundant.
Is this double-initialization required by hardware? If so, add a detailed comment explaining the necessity. If not, consider removing lines 563-596.
1237-1298: qman_portal_poll_rx signature update verified
drivers/net/dpaa/dpaa_rxtx.c now calls (poll_limit, bufs, p, cb) matching the header; no other callers found.drivers/net/dpaa/dpaa_rxtx.c (17)
48-48: LGTM: PTP header support added.The include is correctly added to support the new per-interface PTP/timestamp parsing functionality used later in the file.
473-473: LGTM: Function signature updated for per-interface context.The signature change is consistent with the per-interface design pattern applied throughout the file.
504-504: LGTM: Port assignment uses per-interface context.Correctly uses
dpaa_intf->ifidinstead of deriving the port ID from elsewhere.
534-534: LGTM: Per-interface context propagated correctly.The
dpaa_intfparameter is correctly passed todpaa_eth_packet_info.
545-545: LGTM: Function signature updated for per-interface context.Consistent with the overall refactoring to support per-interface operations.
556-556: LGTM: Per-interface context propagated in SG path.Correctly passes
dpaa_intfwhen handling scatter-gather frames.
574-574: LGTM: Port assignment uses per-interface context.Correctly uses
dpaa_intf->ifidfor port identification.
583-583: LGTM: Per-interface context propagated correctly.The
dpaa_intfparameter is correctly passed todpaa_eth_packet_info.
675-679: LGTM: Per-interface context retrieved and propagated correctly.The code correctly retrieves
dpaa_intffrom the frame queue and passes it to the conversion functions.
723-727: LGTM: Per-interface context retrieved and propagated correctly.Consistent with the pattern used throughout the RX path.
788-788: LGTM: Function call updated to match new signature.The call correctly matches the updated signature of
qman_portal_poll_rxwhich now accepts a callback structure as the last parameter.
801-801: LGTM: Per-interface context propagated in parallel RX.Correctly retrieves and passes
dpaa_intffor event-based parallel RX processing.
828-828: LGTM: Per-interface context propagated in atomic RX.Correctly retrieves and passes
dpaa_intffor event-based atomic RX processing.
899-899: LGTM: Per-interface context used in error handling.Correctly uses the per-interface context for error queue processing.
1312-1315: LGTM: TX timestamp support conditionally enabled.Correctly checks
dpaa_intf->ts_enableand invokes TX confirmation when per-interface timestamping is enabled.
1323-1328: LGTM: Frame descriptor commands configured for timestamping.The FD commands are correctly set to enable TX timestamp capture when per-interface timestamping is enabled.
238-240: All dpaa_eth_packet_info call sites updated Verified that every invocation now passes thedpaa_intfparameter.drivers/bus/dpaa/dpaa_bus.c (3)
67-69: LGTM: push-queue counters on busFields look appropriate for capping and tracking push-mode RX queues.
124-129: LGTM: accessorAccessor is straightforward and safe to inline/export.
1000-1000: LGTM: sensible defaultDefault to 4 push queues matches the comment/rationale; overridden by SoC/env logic.
| RTE_EXPORT_INTERNAL_SYMBOL(dpaa_push_queue_num_update) | ||
| int | ||
| dpaa_push_queue_num_update(void) | ||
| { | ||
| int ret = false; | ||
| uint16_t current, new_val; | ||
|
|
||
| current = rte_atomic_load_explicit(&rte_dpaa_bus.push_rxq_num, | ||
| rte_memory_order_acquire); | ||
| if (current < rte_dpaa_bus.max_push_rxq_num) { | ||
| new_val = current + 1; | ||
| if (rte_atomic_compare_exchange_strong_explicit(&rte_dpaa_bus.push_rxq_num, | ||
| ¤t, new_val, | ||
| rte_memory_order_release, | ||
| rte_memory_order_acquire)) | ||
| ret = true; | ||
| } | ||
|
|
||
| return ret; | ||
| } | ||
|
|
There was a problem hiding this comment.
Compile-breaker: true/false without stdbool; also tighten atomics and lifecycle
- true/false used without including stdbool.h will not compile. Use 0/1 or include stdbool.h. Minimal fix below.
- Optional: use relaxed atomics; this is a simple counter with no ordering dependency.
- Consider providing a decrement/reset path for push_rxq_num on device stop/remove to prevent leaks.
Apply this minimal fix:
RTE_EXPORT_INTERNAL_SYMBOL(dpaa_push_queue_num_update)
int
dpaa_push_queue_num_update(void)
{
- int ret = false;
+ int ret = 0;
uint16_t current, new_val;
current = rte_atomic_load_explicit(&rte_dpaa_bus.push_rxq_num,
- rte_memory_order_acquire);
+ rte_memory_order_acquire);
if (current < rte_dpaa_bus.max_push_rxq_num) {
new_val = current + 1;
if (rte_atomic_compare_exchange_strong_explicit(&rte_dpaa_bus.push_rxq_num,
¤t, new_val,
rte_memory_order_release,
rte_memory_order_acquire))
- ret = true;
+ ret = 1;
}
return ret;
}Optional atomics simplification (if no ordering dependency exists):
- current = rte_atomic_load_explicit(&rte_dpaa_bus.push_rxq_num,
- rte_memory_order_acquire);
+ current = rte_atomic_load_explicit(&rte_dpaa_bus.push_rxq_num,
+ rte_memory_order_relaxed);
@@
- if (rte_atomic_compare_exchange_strong_explicit(&rte_dpaa_bus.push_rxq_num,
- ¤t, new_val,
- rte_memory_order_release,
- rte_memory_order_acquire))
+ if (rte_atomic_compare_exchange_strong_explicit(&rte_dpaa_bus.push_rxq_num,
+ ¤t, new_val,
+ rte_memory_order_relaxed,
+ rte_memory_order_relaxed))| uint32_t svr_ver; | ||
| int probe_all = rte_dpaa_bus.bus.conf.scan_mode != RTE_BUS_SCAN_ALLOWLIST; | ||
| static int process_once; | ||
| char *penv; |
There was a problem hiding this comment.
Do not abort probe on LS1043A; harden env parsing and avoid negative wrap
- Returning early on LS1043A stops the entire probe (device list build, qman/bman init, etc.). Just set max_push_rxq_num=0 and continue.
- atoi() accepts negatives; when assigned to uint16_t they wrap (e.g., “-1” -> 65535) and then clamp to max (8), unintentionally enabling push mode. Parse safely with strtol and validate.
Apply these diffs:
Change the LS1043A block to not exit early:
/* Disabling the default push mode for LS1043A */
if (rte_dpaa_bus.svr_ver == SVR_LS1043A_FAMILY) {
rte_dpaa_bus.max_push_rxq_num = 0;
- return 0;
}Harden env parsing (and make intent clearer with const):
- static int process_once;
- char *penv;
+ static int process_once;
+ const char *penv;
@@
- penv = getenv("DPAA_PUSH_QUEUES_NUMBER");
- if (penv)
- rte_dpaa_bus.max_push_rxq_num = atoi(penv);
- if (rte_dpaa_bus.max_push_rxq_num > DPAA_MAX_PUSH_MODE_QUEUE)
- rte_dpaa_bus.max_push_rxq_num = DPAA_MAX_PUSH_MODE_QUEUE;
+ penv = getenv("DPAA_PUSH_QUEUES_NUMBER");
+ if (penv) {
+ char *end = NULL;
+ long n = strtol(penv, &end, 10);
+ if (end && *end == '\0' && n >= 0) {
+ if (n > DPAA_MAX_PUSH_MODE_QUEUE)
+ rte_dpaa_bus.max_push_rxq_num = DPAA_MAX_PUSH_MODE_QUEUE;
+ else
+ rte_dpaa_bus.max_push_rxq_num = (uint16_t)n;
+ } else {
+ DPAA_BUS_WARN("Invalid DPAA_PUSH_QUEUES_NUMBER='%s', keeping %u",
+ penv, rte_dpaa_bus.max_push_rxq_num);
+ }
+ }Note: strtol requires stdlib.h (already implicitly used for calloc/free/atoi). If not present via transitive includes in your toolchain, add it explicitly at the top.
Also applies to: 750-755, 756-761
🤖 Prompt for AI Agents
In drivers/bus/dpaa/dpaa_bus.c around lines 721 (and similarly 750-755,
756-761), the LS1043A branch currently aborts probe; change it to set
max_push_rxq_num = 0 and continue rather than returning early. Replace the use
of char *penv and atoi() with a const char * and parse with strtol: check for
NULL, use errno = 0 before call, call strtol(penv, &endptr, 10), ensure endptr
!= penv and *endptr == '\0', errno == 0, and the result is within 0..UINT16_MAX
before assigning; then clamp to the allowed max (8) and assign to the uint16_t
variable. Add an explicit #include <stdlib.h> at the top if not already present.
| static inline struct rte_dpaax_ptp_header * | ||
| dpaax_timesync_ptp_parse_header(struct rte_mbuf *buf, | ||
| uint16_t *ts_offset, int *is_udp) | ||
| { | ||
| struct rte_ether_hdr *eth = rte_pktmbuf_mtod(buf, void *); | ||
| void *next_hdr; | ||
| rte_be16_t ether_type; | ||
| struct rte_vlan_hdr *vlan; | ||
| struct rte_ipv4_hdr *ipv4; | ||
| struct rte_ipv6_hdr *ipv6; | ||
| struct rte_udp_hdr *udp; | ||
| struct rte_dpaax_ptp_header *ptp = NULL; | ||
| uint16_t offset = offsetof(struct rte_dpaax_ptp_header, correction); | ||
|
|
||
| if (is_udp) | ||
| *is_udp = false; | ||
|
|
||
| offset += sizeof(struct rte_ether_hdr); | ||
| if (eth->ether_type == htons(RTE_ETHER_TYPE_1588)) { | ||
| ptp = (void *)(eth + 1); | ||
| goto quit; | ||
| } | ||
|
|
||
| if (eth->ether_type == htons(RTE_ETHER_TYPE_VLAN)) { | ||
| vlan = (void *)(eth + 1); | ||
| ether_type = vlan->eth_proto; | ||
| next_hdr = (void *)(vlan + 1); | ||
| offset += sizeof(struct rte_vlan_hdr); | ||
| if (ether_type == htons(RTE_ETHER_TYPE_1588)) { | ||
| ptp = next_hdr; | ||
| goto quit; | ||
| } | ||
| } else { | ||
| ether_type = eth->ether_type; | ||
| next_hdr = (void *)(eth + 1); | ||
| } | ||
|
|
||
| if (ether_type == htons(RTE_ETHER_TYPE_IPV4)) { | ||
| ipv4 = next_hdr; | ||
| offset += sizeof(struct rte_ipv4_hdr); | ||
| if (ipv4->next_proto_id != IPPROTO_UDP) | ||
| return NULL; | ||
| udp = (void *)(ipv4 + 1); | ||
| goto parse_udp; | ||
| } else if (ether_type == htons(RTE_ETHER_TYPE_IPV6)) { | ||
| ipv6 = next_hdr; | ||
| offset += sizeof(struct rte_ipv6_hdr); | ||
| if (ipv6->proto != IPPROTO_UDP) | ||
| return NULL; | ||
| udp = (void *)(ipv6 + 1); | ||
| goto parse_udp; | ||
| } else { | ||
| return NULL; | ||
| } | ||
| parse_udp: | ||
| offset += sizeof(struct rte_udp_hdr); | ||
| if (udp->dst_port != UDP_PTP_EVENT_DST_PORT && | ||
| udp->dst_port != UDP_PTP_GENERAL_DST_PORT) | ||
| return NULL; | ||
| ptp = (void *)(udp + 1); | ||
| if (is_udp) | ||
| *is_udp = true; | ||
| quit: | ||
| if (ts_offset) | ||
| *ts_offset = offset; | ||
|
|
||
| return ptp; | ||
| } |
There was a problem hiding this comment.
Missing buffer bounds validation poses crash risk.
The parser dereferences header pointers without verifying that the mbuf contains sufficient data. If the packet is truncated or malformed, this will cause out-of-bounds memory access.
For example:
- Line 30 obtains
ethpointer but doesn't checkbuf->data_lencoverssizeof(struct rte_ether_hdr) - Line 50 advances to VLAN without confirming space for
struct rte_vlan_hdr - Lines 64-75 access IP and UDP headers without length validation
Add validation before each pointer dereference:
+static inline bool
+pkt_has_space(struct rte_mbuf *buf, void *ptr, size_t len)
+{
+ uintptr_t end = (uintptr_t)rte_pktmbuf_mtod(buf, void *) + buf->data_len;
+ return ((uintptr_t)ptr + len) <= end;
+}
+
static inline struct rte_dpaax_ptp_header *
dpaax_timesync_ptp_parse_header(struct rte_mbuf *buf,
uint16_t *ts_offset, int *is_udp)
{
struct rte_ether_hdr *eth = rte_pktmbuf_mtod(buf, void *);
+ if (!pkt_has_space(buf, eth, sizeof(*eth)))
+ return NULL;
...
if (eth->ether_type == htons(RTE_ETHER_TYPE_VLAN)) {
vlan = (void *)(eth + 1);
+ if (!pkt_has_space(buf, vlan, sizeof(*vlan)))
+ return NULL;Apply similar checks before accessing ipv4, ipv6, and udp.
| uint16_t | ||
| dpaa_eth_queue_rx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs) | ||
| { | ||
| struct qman_fq *fq = q; | ||
| struct qm_dqrr_entry *dq; | ||
| uint32_t num_rx = 0, ifid = ((struct dpaa_if *)fq->dpaa_intf)->ifid; | ||
| uint32_t num_rx = 0; |
There was a problem hiding this comment.
Type mismatch: num_rx declared as uint32_t but function returns uint16_t.
At line 912, num_rx is declared as uint32_t, but the function signature at line 907 returns uint16_t. This creates a potential truncation issue if num_rx exceeds 65,535. While the maximum dequeue limit likely prevents this in practice, the type inconsistency should be resolved for correctness.
Apply this diff to fix the type mismatch:
- uint32_t num_rx = 0;
+ uint16_t num_rx = 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint16_t | |
| dpaa_eth_queue_rx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs) | |
| { | |
| struct qman_fq *fq = q; | |
| struct qm_dqrr_entry *dq; | |
| uint32_t num_rx = 0, ifid = ((struct dpaa_if *)fq->dpaa_intf)->ifid; | |
| uint32_t num_rx = 0; | |
| uint16_t | |
| dpaa_eth_queue_rx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs) | |
| { | |
| struct qman_fq *fq = q; | |
| struct qm_dqrr_entry *dq; | |
| uint16_t num_rx = 0; |
🤖 Prompt for AI Agents
In drivers/net/dpaa/dpaa_rxtx.c around lines 907 to 912, the local variable
num_rx is declared as uint32_t while the function dpaa_eth_queue_rx returns
uint16_t, creating a type mismatch and possible truncation; change num_rx to
uint16_t to match the function return type, update any arithmetic/increment or
comparisons that assume wider range to use uint16_t or cast appropriately, and
ensure the dequeue loop and final return use the uint16_t variable consistently.
2 participants
Auto-submission for "http://patchwork.dpdk.org/project/dpdk/list/?series=36350"
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation