phytium: phytmac: update phytmac to v1.0.50#1514
Conversation
Reviewer's GuideUpdates the Phytium phytmac Ethernet driver from v1.0.29 to v1.0.50 by adding XDP support, tightening MDIO/message-ring handling and timeouts, fixing several RX/TX, stats, and power-management bugs, and refining probe/remove, WOL, and ethtool integration, along with renaming the PCI/platform drivers and exposing a non-posted devm ioremap helper. Sequence diagram for RX path with XDP processing in phytmacsequenceDiagram
participant CPU as cpu
participant NAPI as napi_struct
participant Queue as phytmac_queue
participant MAC as phytmac
participant HW as dma_engine
participant XDP as bpf_prog
participant Stack as net_stack
CPU->>NAPI: poll()
NAPI->>Queue: phytmac_rx(queue, napi, budget)
loop while budget and RX desc owned by HW
Queue->>HW: phytmac_get_rx_desc()
Queue->>MAC: hw_if->rx_single_buffer(desc)?
alt single_buffer
Queue->>Queue: phytmac_rx_xdp_single(queue, desc, xdp_xmit)
Queue->>MAC: phytmac_run_xdp(pdata, &xdp_buff)
MAC->>XDP: bpf_prog_run_xdp(xdp_prog, xdp_buff)
alt XDP_PASS
XDP-->>MAC: act = XDP_PASS
MAC-->>Queue: ERR_PTR(-PHYTMAC_XDP_PASS)
Queue->>Queue: phytmac_rx_single(queue, desc)
Queue->>Stack: napi_gro_receive(napi, skb)
else XDP_TX
XDP-->>MAC: act = XDP_TX
MAC->>MAC: phytmac_xdp_xmit_back(pdata, xdp_buff)
MAC-->>Queue: ERR_PTR(-PHYTMAC_XDP_TX)
Note over Queue: page reused, no skb to stack
else XDP_REDIRECT
XDP-->>MAC: act = XDP_REDIRECT
MAC->>XDP: xdp_do_redirect(ndev, xdp_buff, xdp_prog)
MAC-->>Queue: ERR_PTR(-PHYTMAC_XDP_REDIR)
Note over Queue: redirect tracked for xdp_do_flush_map()
else XDP_DROP/ABORTED
XDP-->>MAC: act = XDP_DROP or XDP_ABORTED
MAC-->>Queue: ERR_PTR(-PHYTMAC_XDP_CONSUMED)
Note over Queue: buffer recycled, stats updated
end
else multi_buffer
alt XDP program installed
Queue->>MAC: netdev_warn("xdp does not support multiple buffers")
end
Queue->>Queue: phytmac_rx_mbuffer(queue)
Queue->>Stack: napi_gro_receive(napi, skb)
end
Queue->>Queue: update rx_tail, stats
end
alt any_redirect
Queue->>XDP: xdp_do_flush_map()
end
Queue->>HW: phytmac_rx_clean(queue)
Queue-->>NAPI: return work_done
Class diagram for updated phytmac core structures and XDP integrationclassDiagram
class phytmac {
+struct platform_device *platdev
+struct pci_dev *pcidev
+struct net_device *ndev
+struct device *dev
+struct bpf_prog *xdp_prog
+struct phytmac_hw_if *hw_if
+struct phytmac_msg msg_ring
+u32 rx_ring_size
+u32 tx_ring_size
+u32 dma_data_width
+u32 version
+char fw_version[32]
+u32 wol
+spinlock_t lock
+spinlock_t ts_clk_lock
+spinlock_t rx_fs_lock
}
class phytmac_queue {
+struct phytmac *pdata
+int irq
+int index
+unsigned int tx_head
+unsigned int tx_tail
+struct phytmac_tx_skb *tx_skb
+struct phytmac_dma_desc *tx_ring
+struct phytmac_rx_buffer *rx_buffer_info
+struct napi_struct rx_napi
+struct phytmac_queue_stats stats
+struct bpf_prog *xdp_prog
+struct xdp_rxq_info xdp_rxq
}
class phytmac_tx_skb {
+struct sk_buff *skb
+struct xdp_frame *xdpf
+enum phytmac_tx_buf_type type
+dma_addr_t addr
+size_t length
+bool mapped_as_page
+DEFINE_DMA_UNMAP_ADDR(addr)
+DEFINE_DMA_UNMAP_LEN(length)
}
class phytmac_tx_buf_type {
<<enum>>
+PHYTMAC_TYPE_SKB = 0
+PHYTMAC_TYPE_XDP
}
class phytmac_msg {
+void __iomem *msg_regs
+u32 tx_msg_ring_size
+u32 rx_msg_ring_size
+u32 tx_msg_head
+u32 tx_msg_wr_tail
+u32 tx_msg_rd_tail
+u32 rx_msg_head
+u32 rx_msg_tail
+spinlock_t msg_lock
}
class phytmac_hw_if {
+int (*init_msg_ring)(struct phytmac *pdata)
+int (*init_hw)(struct phytmac *pdata)
+int (*get_regs)(struct phytmac *pdata, u32 *reg_buff)
+void (*get_stats)(struct phytmac *pdata)
+int (*set_mac_address)(struct phytmac *pdata, u8 *addr)
+int (*get_mac_address)(struct phytmac *pdata, u8 *addr)
+int (*mdio_read)(struct phytmac *pdata, int mii_id, int regnum)
+int (*mdio_write)(struct phytmac *pdata, int mii_id, int regnum, u16 data)
+int (*mdio_idle)(struct phytmac *pdata)
+int (*mdio_read_c45)(struct phytmac *pdata, int mii_id, int devad, int regnum)
+int (*mdio_write_c45)(struct phytmac *pdata, int mii_id, int devad, int regnum, int regnum2)
+unsigned int (*zero_rx_desc_addr)(struct phytmac_dma_desc *desc)
+void (*tx_map)(struct phytmac_queue *queue, u16 tx_tail, struct packet_info *packet)
+unsigned int (*rx_single_buffer)(struct phytmac_dma_desc *desc)
}
class phytmac_msg_info {
+u8 module_id
+u8 queue_id
+u8 cmd_type
+u8 cmd_subid
+u16 len
+u8 status1
+u8 status0
+u8 para[PHYTMAC_MSG_PARA_LEN]
}
class phytmac_ethtool_reg {
+u8 interface
+u8 cnt
}
class phytmac_queue_stats {
+u64 rx_bytes
+u64 rx_packets
+u64 tx_bytes
+u64 tx_packets
}
class phytmac_dma_desc {
+u32 desc0
+u32 desc1
+u32 desc2
+u32 desc3
}
phytmac "1" o-- "*" phytmac_queue : queues
phytmac "1" o-- "1" phytmac_msg : msg_ring
phytmac "1" o-- "1" phytmac_hw_if : hw_if
phytmac_queue "1" o-- "*" phytmac_tx_skb : tx_skb
phytmac_queue "1" o-- "*" phytmac_dma_desc : tx_ring
phytmac_queue "1" o-- "*" phytmac_rx_buffer : rx_buffer_info
phytmac_tx_skb --> phytmac_tx_buf_type : type
phytmac_msg_info --> phytmac_msg : uses_ring_indices
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Hi @JiakunShuai. Thanks for your PR. I'm waiting for a deepin-community member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the XDP RX path (
phytmac_rx_xdp_single+phytmac_rx), the XDP_PASS case leaves the acquiredrx_bufferneither flipped nor returned beforephytmac_rx_single()is called (which re-fetches the buffer), which risks refcount and reuse bugs; consider havingphytmac_rx_xdp_singleeither not grab the buffer up front or explicitly handle the PASS case by handing the existing buffer to the skb builder instead of acquiring it twice. - The new
phytmac_xdp_setuplogic updatespdata->xdp_progand per-queuexdp_progpointers both before and after close/open; double-check that no RX can run with a stale per-queue program pointer (e.g. by ordering the xchg and queue updates around open/close more tightly or documenting the intended RCU guarantees).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the XDP RX path (`phytmac_rx_xdp_single` + `phytmac_rx`), the XDP_PASS case leaves the acquired `rx_buffer` neither flipped nor returned before `phytmac_rx_single()` is called (which re-fetches the buffer), which risks refcount and reuse bugs; consider having `phytmac_rx_xdp_single` either not grab the buffer up front or explicitly handle the PASS case by handing the existing buffer to the skb builder instead of acquiring it twice.
- The new `phytmac_xdp_setup` logic updates `pdata->xdp_prog` and per-queue `xdp_prog` pointers both before and after close/open; double-check that no RX can run with a stale per-queue program pointer (e.g. by ordering the xchg and queue updates around open/close more tightly or documenting the intended RCU guarantees).
## Individual Comments
### Comment 1
<location path="drivers/net/ethernet/phytium/phytmac_main.c" line_range="2466-2467" />
<code_context>
+ if (!need_reset)
+ return 0;
+
+ if (prog)
+ xdp_features_set_redirect_target(dev, false);
+ else
+ xdp_features_clear_redirect_target(dev);
</code_context>
<issue_to_address>
**question (bug_risk):** xdp_features_set_redirect_target() is called with 'false' when enabling XDP, which looks inverted.
In `phytmac_xdp_setup()`, when attaching an XDP program (`prog` non-NULL) you call `xdp_features_set_redirect_target(dev, false);`, and use `xdp_features_clear_redirect_target(dev);` when detaching. Other drivers typically call `xdp_features_set_redirect_target(dev, true);` when XDP is enabled to mark themselves as native redirect targets.
Please confirm the intended boolean semantics—if the aim is to enable native XDP redirect support on attach, this likely should pass `true` instead of `false`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (prog) | ||
| xdp_features_set_redirect_target(dev, false); |
There was a problem hiding this comment.
question (bug_risk): xdp_features_set_redirect_target() is called with 'false' when enabling XDP, which looks inverted.
In phytmac_xdp_setup(), when attaching an XDP program (prog non-NULL) you call xdp_features_set_redirect_target(dev, false);, and use xdp_features_clear_redirect_target(dev); when detaching. Other drivers typically call xdp_features_set_redirect_target(dev, true); when XDP is enabled to mark themselves as native redirect targets.
Please confirm the intended boolean semantics—if the aim is to enable native XDP redirect support on attach, this likely should pass true instead of false.
|
/ok-to-test |
There was a problem hiding this comment.
Pull request overview
Updates the Phytium phytmac Ethernet driver to v1.0.50, including XDP support plus a set of reliability fixes around MDIO/message handling, buffer management, and platform/PCI identification.
Changes:
- Add XDP RX/TX (including redirect/TX-back) support and expose XDP capabilities via netdev ops.
- Rework v2 message ring handling and add MDIO idle timeouts.
- Refine platform/PCI integration (resource mapping helper, driver naming) and ethtool reporting (fw_version/regs length/WoL).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| drivers/net/ethernet/phytium/phytmac_v2.h | Updates v2 message/register constants and adds helpers/structs used by new msg/ethtool flows. |
| drivers/net/ethernet/phytium/phytmac_v2.c | Reworks msg send/wait logic, adds MDIO idle timeout polling, updates stats/regs access. |
| drivers/net/ethernet/phytium/phytmac_v1.c | Adds MDIO idle timeout polling and preserves RX timestamp-valid bit when clearing descriptors. |
| drivers/net/ethernet/phytium/phytmac_platform.c | Switches platform MMIO mapping helper and updates platform driver name. |
| drivers/net/ethernet/phytium/phytmac_pci.c | Uses PCI-specific driver name for iomap/driver registration. |
| drivers/net/ethernet/phytium/phytmac_main.c | Adds XDP plumbing, changes RX/TX resource handling, adds devm ioremap_np helpers, updates msg ring locking. |
| drivers/net/ethernet/phytium/phytmac_ethtool.c | Updates regs length handling, improves WoL behavior, reports per-bus driver name + fw_version. |
| drivers/net/ethernet/phytium/phytmac.h | Bumps driver version, adds XDP types/helpers, new driver-name constants, msg ring lock changes. |
| drivers/net/ethernet/phytium/Kconfig | Restricts NET_VENDOR_PHYTIUM to ARCH_PHYTIUM. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (regs) { | ||
| pdata->msg_regs = ioremap_wt(regs->start, MEMORY_SIZE); | ||
| if (!pdata->msg_regs) { | ||
| dev_err(&pdev->dev, "msg_regs ioremap failed, i=%d\n", i); |
There was a problem hiding this comment.
In the msg_regs ioremap failure path, ret is not set before goto err_mem;, so probe may return a stale/zero return code and incorrectly report success after freeing pdata. Set ret to an appropriate negative errno (e.g., -ENOMEM) when ioremap_wt() returns NULL.
| dev_err(&pdev->dev, "msg_regs ioremap failed, i=%d\n", i); | |
| dev_err(&pdev->dev, "msg_regs ioremap failed, i=%d\n", i); | |
| ret = -ENOMEM; |
| if (unlikely(!phytmac_txdesc_unused(queue))) | ||
| return PHYTMAC_XDP_CONSUMED; | ||
|
|
||
| dma = dma_map_single(pdata->dev, xdpf->data, len, DMA_TO_DEVICE); | ||
| if (dma_mapping_error(pdata->dev, dma)) | ||
| return PHYTMAC_XDP_CONSUMED; |
There was a problem hiding this comment.
In the XDP TX path, failures (no free descriptors / DMA mapping error) return PHYTMAC_XDP_CONSUMED without releasing the xdp_frame. This will leak the frame because callers treat CONSUMED as “driver took ownership”. Ensure the failure path returns the frame via xdp_return_frame*() (and unmaps DMA if applicable) before returning CONSUMED.
| struct xdp_frame *xdpf = xdp_convert_buff_to_frame(xdp); | ||
| int cpu = smp_processor_id(); | ||
| struct phytmac_queue *queue; | ||
| struct netdev_queue *nq; | ||
| u32 ret; | ||
|
|
||
| if (unlikely(!xdpf)) | ||
| return PHYTMAC_XDP_CONSUMED; | ||
|
|
||
| /* During program transitions its possible adapter->xdp_prog is assigned | ||
| * but ring has not been configured yet. In this case simply abort xmit. | ||
| */ | ||
| queue = pdata->xdp_prog ? phytmac_xdp_txq_mapping(pdata) : NULL; | ||
| if (unlikely(!queue)) | ||
| return PHYTMAC_XDP_CONSUMED; |
There was a problem hiding this comment.
phytmac_xdp_xmit_back() converts the buffer to an xdp_frame and then can return PHYTMAC_XDP_CONSUMED early when no queue is available, without returning/freeing the xdp_frame. That leaks the frame. Return the frame (e.g., xdp_return_frame(xdpf)) before returning CONSUMED on these early exits.
| spin_lock_irqsave(&pdata->msg_ring.msg_lock, flags); | ||
| tx_head = PHYTMAC_READ(pdata, PHYTMAC_TX_MSG_HEAD) & 0xff; | ||
| tx_tail = phytmac_v2_tx_ring_wrap(pdata, pdata->msg_ring.tx_msg_wr_tail); | ||
| pdata->msg_ring.tx_msg_rd_tail = tx_tail; | ||
| ring_size = pdata->msg_ring.tx_msg_ring_size; | ||
|
|
||
| while ((tx_tail + 1) % ring_size == tx_head) { | ||
| udelay(1); | ||
| tx_head = PHYTMAC_READ(pdata, PHYTMAC_TX_MSG_HEAD) & 0xff; | ||
| retry++; | ||
| if (retry >= PHYTMAC_RETRY_TIMES) { | ||
| netdev_err(pdata->ndev, | ||
| "Time out waiting for Tx msg ring free, tx_tail:0x%x, tx_head:0x%x", | ||
| tx_tail, tx_head); | ||
| ret = -EINVAL; | ||
| goto err_out; | ||
| } | ||
| } |
There was a problem hiding this comment.
phytmac_v2_msg_send() holds msg_lock with IRQs disabled and busy-waits with udelay(1) in a potentially long retry loop. This can keep interrupts disabled for up to PHYTMAC_RETRY_TIMES microseconds (tens of ms) and risks soft lockups/latency spikes. Consider reserving the ring slot under the spinlock, then releasing the lock while polling, and/or using readx_poll_timeout_atomic() with a tighter bound.
| PHYTMAC_MSG_HDR_LEN); | ||
| if (!(msg_rx.status0 & PHYTMAC_CMD_PRC_SUCCESS)) { | ||
| netdev_err(pdata->ndev, "Msg process error, cmdid:%d, subid:%d, status0:%d, tail:%d", | ||
| msg.cmd_type, msg.cmd_subid, msg.status0, tx_tail); |
There was a problem hiding this comment.
The error log for message processing prints msg.status0, but the completion status is read into msg_rx.status0. This makes the log misleading and can hide the actual failure status. Log msg_rx.status0 (and consider including other status fields) instead.
| msg.cmd_type, msg.cmd_subid, msg.status0, tx_tail); | |
| msg.cmd_type, msg.cmd_subid, msg_rx.status0, tx_tail); |
| if (queue->xdp_prog) { | ||
| queue->xdp_prog = NULL; | ||
| xdp_rxq_info_unreg(&queue->xdp_rxq); | ||
| } | ||
|
|
There was a problem hiding this comment.
xdp_rxq_info_reg() is called unconditionally in phytmac_alloc_rx_resource(), but xdp_rxq_info_unreg() is only called when queue->xdp_prog is non-NULL. If no XDP program is attached, the RXQ info remains registered and phytmac_free_rx_resource() will skip the unreg. Unregister the RXQ info unconditionally (or track registration with a dedicated flag).
| if (queue->xdp_prog) { | |
| queue->xdp_prog = NULL; | |
| xdp_rxq_info_unreg(&queue->xdp_rxq); | |
| } | |
| if (queue->xdp_prog) | |
| queue->xdp_prog = NULL; | |
| xdp_rxq_info_unreg(&queue->xdp_rxq); |
| /* record the location of the first descriptor for this packet */ | ||
| tx_buffer = phytmac_get_tx_skb(queue, queue->tx_tail); | ||
| tx_buffer->mapped_as_page = false; | ||
|
|
||
| /* Temporarily set the tail pointer for the next package */ | ||
| tx_tail = queue->tx_tail + 1; | ||
|
|
||
| dma_unmap_len_set(tx_buffer, length, len); | ||
| dma_unmap_addr_set(tx_buffer, addr, dma); | ||
| tx_buffer->type = PHYTMAC_TYPE_XDP; | ||
| tx_buffer->xdpf = xdpf; | ||
|
|
||
| packet.lso = 0; | ||
| packet.mss = 0; | ||
| packet.nocrc = 0; | ||
|
|
||
| /* Avoid any potential race with xdp_xmit and cleanup */ | ||
| smp_wmb(); | ||
|
|
||
| hw_if->tx_map(queue, tx_tail, &packet); | ||
|
|
||
| queue->tx_tail = tx_tail & (pdata->tx_ring_size - 1); | ||
|
|
||
| hw_if->transmit(queue); | ||
|
|
There was a problem hiding this comment.
XDP transmit modifies queue->tx_tail / TX descriptors but does not take queue->tx_lock (used by both phytmac_start_xmit() and phytmac_tx_clean()). Locking only the netdev queue (__netif_tx_lock) does not protect against races with the driver’s TX cleanup path. Use the same lock (queue->tx_lock) (or a dedicated shared lock) around descriptor/ring updates in the XDP TX path to avoid corruption.
| if (wait) { | ||
| memcpy(&msg_rx, pdata->msg_regs + PHYTMAC_MSG(index), MSG_HDR_LEN); | ||
| while (!(msg_rx.status0 & PHYTMAC_CMD_PRC_COMPLETED)) { | ||
| cpu_relax(); | ||
| memcpy(&msg_rx, pdata->msg_regs + PHYTMAC_MSG(index), MSG_HDR_LEN); | ||
| tx_head = PHYTMAC_READ(pdata, PHYTMAC_TX_MSG_HEAD) & 0xff; | ||
| while (tx_head != tx_tail) { | ||
| udelay(1); | ||
| tx_head = PHYTMAC_READ(pdata, PHYTMAC_TX_MSG_HEAD) & 0xff; | ||
| retry++; | ||
| if (retry >= PHYTMAC_RETRY_TIMES) { | ||
| netdev_err(pdata->ndev, "Msg process time out!"); | ||
| ret = -EINVAL; | ||
| goto err_out; | ||
| } | ||
| } |
There was a problem hiding this comment.
The completion wait loop in phytmac_v2_msg_send() continues to busy-wait (udelay(1)) while holding msg_lock with IRQs disabled. This blocks any other message sender and can keep IRQs off for a long time. Release the spinlock before the completion polling and re-acquire only when accessing shared ring state.
| } | ||
|
|
||
| retry = 0; | ||
| wait = 1; |
There was a problem hiding this comment.
wait is forced to 1 (wait = 1;), which makes the wait parameter effectively ignored and changes behavior for callers passing wait = 0 (e.g. reset/init paths). If unconditional waiting is intended, remove the parameter and update callers; otherwise, respect the caller-provided wait value.
| wait = 1; |
Update phytmac from v1.0.29 to v1.0.50. Including following details.
net/phytmac: Modify cmd processing function
net/phytmac: Add XDP feature support
net/phytmac: Bugfix set WOL failed issue
net/phytmac: Bugfix invalid wait context
net/phytmac: Change the requested resource type from nRE to nRnE
net/phytmac: BugFix Memory leak when releasing resource
net/phytmac: Limit the number of retries to avoid deadlock
net/phytmac: Set the read timeout period to the NIC register
net/phytmac: Slove left-shift out of bound issue
net/phytmac: Manage WOL on MAC if PHY supports WOL feature
net/phytmac: Fixed the PTP test failure issue
net/phytmac: Cancels the power-on/off capability
net/phytmac: Exit probe when MDIO times out
net/phytmac: Clear RX descriptor address after the skb construction
net/phytmac: Enable the tail pointer by the driver
net/phytmac: Update hardware identifiers and the driver name
net/phytmac: Fix sleeping function called from invalid context
net/phytmac: Fix compile error on X86 architecture
net/phytmac: Fix page refcounting issue for XDP_REDIRECT
net/phytmac: Bugfix the potential deadlock issue
Summary by Sourcery
Update the Phytium phytmac Ethernet driver to v1.0.50 with XDP support, safer MDIO/message handling, improved buffer/descriptor management, and refined platform/PCI integration and identification.
New Features:
Bug Fixes:
Enhancements:
Build: