Skip to content

phytium: phytmac: update phytmac to v1.0.50#1514

Closed
JiakunShuai wants to merge 0 commit into
deepin-community:linux-6.6.yfrom
JiakunShuai:linux-6.6.y
Closed

phytium: phytmac: update phytmac to v1.0.50#1514
JiakunShuai wants to merge 0 commit into
deepin-community:linux-6.6.yfrom
JiakunShuai:linux-6.6.y

Conversation

@JiakunShuai
Copy link
Copy Markdown
Contributor

@JiakunShuai JiakunShuai commented Feb 27, 2026

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:

  • Add XDP program attach, RX processing and TX (including redirect) support for phytmac queues.
  • Expose XDP capabilities and XDP-specific netdev operations in the phytmac network device.
  • Provide a non-posted devm ioremap helper for mapping MMIO resources and use it for platform MAC resources.
  • Report firmware version and per-bus driver names (platform vs PCI) via ethtool driver info.
  • Enable tail pointer based TX/RX queue management when supported by the hardware.

Bug Fixes:

  • Fix MDIO idle waiting by adding timeouts and error reporting and invoking MDIO idle during bus registration.
  • Correct RX descriptor address clearing to avoid leaving stale addresses after skb construction and preserve timestamp bits.
  • Fix RX page refcounting and reuse logic for XDP_REDIRECT to avoid page reference leaks and misuse.
  • Harden message ring command handling with bounded retries, proper wrap semantics, and validation of message parameter length and completion status.
  • Resolve statistics and register access issues by switching some stats reads to direct MMIO, updating lengths and layouts, and sharing constants between v1 and v2.
  • Adjust WoL handling to respect PHY errors and centralize MAC-managed WoL flags, and fix PCI/ACPI power sequencing and capability flags.
  • Prevent unsupported MTU settings when XDP is active and align DMA ring allocations to page boundaries to avoid mapping issues.

Enhancements:

  • Switch message ring protection from a mutex to a spinlock and refine internal indices for transmit message tracking.
  • Refine PTP timestamp handling into helpers and preserve RX timestamp validity bits when zeroing descriptors.
  • Simplify MDIO and PCS control messaging semantics (e.g. making some operations synchronous) and use hardware capability-reported prefetch sizes only when non-zero.
  • Unify hardware statistics constants and make register layouts more explicit for both v1 and v2 hardware revisions.

Build:

  • Bump phytmac driver version from 1.0.29 to 1.0.50 and adjust driver name constants for PCI and platform variants.

@sourcery-ai
Copy link
Copy Markdown

sourcery-ai Bot commented Feb 27, 2026

Reviewer's Guide

Updates 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 phytmac

sequenceDiagram
    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
Loading

Class diagram for updated phytmac core structures and XDP integration

classDiagram
    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
Loading

File-Level Changes

Change Details Files
Add XDP (eBPF) support for RX/TX paths, including XDP program attach/detach, queue metadata, and XDP_TX/REDIRECT handling.
  • Include bpf headers and introduce XDP-related constants and buffer types in driver headers and main file
  • Add per-device and per-queue xdp_prog pointers and xdp_rxq_info registration/unregistration in RX queue setup/teardown
  • Introduce phytmac_run_xdp(), phytmac_rx_xdp_single(), and RX buffer flip logic to run XDP programs on single-buffer RX frames and integrate results into phytmac_rx()
  • Extend TX buffer bookkeeping to distinguish SKB vs XDP frames and correctly account stats and lifetimes in the TX cleanup path
  • Implement XDP TX helpers (phytmac_start_xmit_xdp, phytmac_xdp_xmit_back, phytmac_xdp_xmit) and netdev_ops callbacks (ndo_bpf/ndo_xdp_xmit), including CPU-to-queue mapping and queue locking
  • Enforce MTU limits compatible with XDP frame layout and advertise XDP feature flags and redirect capabilities
drivers/net/ethernet/phytium/phytmac_main.c
drivers/net/ethernet/phytium/phytmac.h
Harden RX/TX buffer management and page reference counting, including descriptor address clearing and page reuse for XDP.
  • Move and reuse phytmac_calc_rx_buf_len() as shared helper and adjust RX buffer size calculations
  • Fix RX page pagecnt_bias handling by bumping initial page refcount to USHRT_MAX and adjusting reuse logic to avoid refcount underflow/overflow
  • Ensure RX descriptors have their address field cleared after SKB construction in both single- and multi-buffer paths while preserving timestamp bits
  • Add RX buffer flip logic for XDP reuse and ensure stats updates only for successfully delivered SKBs
  • Align TX/RX DMA ring chunk sizes to 4K when freeing coherent memory to match allocation layout
drivers/net/ethernet/phytium/phytmac_main.c
drivers/net/ethernet/phytium/phytmac_v1.c
drivers/net/ethernet/phytium/phytmac_v2.c
drivers/net/ethernet/phytium/phytmac.h
Rework MDIO access to use idle-wait helpers with timeouts and integrate them into MDIO registration, preventing hangs on bad hardware.
  • Introduce PHYTMAC_MDIO_TIMEOUT and helper macros to poll MDIO idle bits using readx_poll_timeout for both v1 and v2 hardware variants
  • Change phytmac_v1 and phytmac_v2 mdio_idle() implementations to return int status, log timeouts, and preserve existing idle checks
  • Expose mdio_idle() via the hardware interface struct and call it from phytmac_mdio_register() before registering the MDIO bus, aborting on failure
drivers/net/ethernet/phytium/phytmac.h
drivers/net/ethernet/phytium/phytmac_main.c
drivers/net/ethernet/phytium/phytmac_v1.c
drivers/net/ethernet/phytium/phytmac_v2.c
Revamp v2 message-ring handling and statistics/regs access to avoid deadlocks, enforce bounds, and make access index-safe.
  • Replace mutex-based msg_ring protection with a spinlock and per-ring write/read tail indices, removing the old msg_lock and msg_mutex fields and users
  • Rewrite phytmac_v2_msg_send() to manage TX message ring via hardware head/tail registers, wrap indices with a helper, bound retries with PHYTMAC_RETRY_TIMES, and validate parameter length and completion status
  • Rework message-ring initialization to correctly capture ring size and initial tail pointers, logging improved diagnostics
  • Update v2 helpers that read responses (get_mac_addr, get_feature_all, get_regs, get_hw_stats, PCS linkup, PCS reset) to use the new rd_tail/wrap scheme and new message header/parameter sizes
  • Switch v2 stats collection to direct register reads instead of message-based pulls, using shared PHYTMAC_STATIS_REG_NUM constant
drivers/net/ethernet/phytium/phytmac_v2.c
drivers/net/ethernet/phytium/phytmac_v2.h
drivers/net/ethernet/phytium/phytmac.h
Tighten WoL handling so MAC WoL is only managed when PHY operations succeed and centralize supported flags.
  • Initialize wolopts to 0 and set wol->supported mask to all MAC-supported modes in phytmac_get_wol(), instead of conditionally OR-ing based on current configuration
  • Change phytmac_set_wol() to return early when PHY set_wol fails (and is not -EOPNOTSUPP), and only update MAC-side wol bits and hardware configuration when PHY-side configuration succeeded
  • Ensure WoL bitmask mapping between ethtool flags and internal PHYTMAC_WAKE_* flags remains consistent
drivers/net/ethernet/phytium/phytmac_ethtool.c
Adjust power-control and suspend behavior, removing power-on/off capability in platform config and reordering poweroff sequence.
  • Drop PHYTMAC_CAPS_PWCTRL from the 2.0 platform config so that power control is no longer advertised/capability-driven
  • Reorder suspend sequence to perform reset_hw under lock, then call poweron(PHYTMAC_POWEROFF) afterwards to avoid invalid context issues
  • Keep v2-specific powerup_hw() but only use it when the capability is provided by the platform
drivers/net/ethernet/phytium/phytmac_platform.c
drivers/net/ethernet/phytium/phytmac_v2.c
drivers/net/ethernet/phytium/phytmac_main.c
Improve ethtool integration for regs and driver info, including versioning and firmware name reporting.
  • Increase generic regs length constant and wire it into ethtool get_regs_len(), and adjust zeroing logic to use bytes instead of u32 count
  • Restructure v2 get_regs to use a small command structure and two-step read to fetch all registers into the provided buffer
  • Populate fw_version based on MAC version in default config and export it via ethtool_drvinfo along with version and driver name
  • Differentiate driver names between platform and PCI variants via new PHYTMAC_PCI_DRV_NAME/PHYTMAC_PLAT_DRV_NAME macros and conditionally selecting bus_info and driver fields
drivers/net/ethernet/phytium/phytmac_ethtool.c
drivers/net/ethernet/phytium/phytmac_main.c
drivers/net/ethernet/phytium/phytmac.h
drivers/net/ethernet/phytium/phytmac_v2.c
Introduce a non-posted devm ioremap helper and use it in the platform driver for MAC MMIO resources, plus minor resource cleanup fixes.
  • Add phytmac_devm_ioremap_resource_np() and internal helpers to request a memory region and ioremap_np it with devres-based lifetime management and error reporting
  • Export the new helper symbol and declare it in the main header for reuse
  • Switch platform probe to use phytmac_devm_ioremap_resource_np() instead of devm_platform_get_and_ioremap_resource for MAC registers, and simplify msg_regs error handling
  • Rename PCI and platform driver .name fields and pcim_iomap_regions label to match new driver name macros
drivers/net/ethernet/phytium/phytmac_main.c
drivers/net/ethernet/phytium/phytmac_platform.c
drivers/net/ethernet/phytium/phytmac_pci.c
drivers/net/ethernet/phytium/phytmac.h
Miscellaneous fixes and cleanups: ioctl behavior, PTP, descriptor flags, and defines.
  • Relax ioctl default case to stop returning -EOPNOTSUPP explicitly, letting core handling apply
  • Extract PTP TX timestamping logic into a helper and reuse it in TX cleanup, and ensure RX timestamping is not invoked on error paths
  • Fix RX descriptor zeroing for both v1 and v2 so that timestamp-valid bits are preserved while marking RX_USED/RXUSED
  • Add generic helpers for tx descriptor unused calculation and netdev_queue lookup, and use them in XDP and TX paths
  • Update driver version macro to 1.0.50 and tweak a few comments and spacing/indentation issues
drivers/net/ethernet/phytium/phytmac_main.c
drivers/net/ethernet/phytium/phytmac_v1.c
drivers/net/ethernet/phytium/phytmac_v2.c
drivers/net/ethernet/phytium/phytmac.h

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@deepin-ci-robot
Copy link
Copy Markdown

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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.

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign avenger-285714 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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).
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +2466 to +2467
if (prog)
xdp_features_set_redirect_target(dev, false);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Avenger-285714
Copy link
Copy Markdown
Member

/ok-to-test

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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;

Copilot uses AI. Check for mistakes.
Comment on lines +1730 to +1735
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;
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +2499 to +2513
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;
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +32 to +49
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;
}
}
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
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);
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
msg.cmd_type, msg.cmd_subid, msg.status0, tx_tail);
msg.cmd_type, msg.cmd_subid, msg_rx.status0, tx_tail);

Copilot uses AI. Check for mistakes.
Comment on lines +442 to +446
if (queue->xdp_prog) {
queue->xdp_prog = NULL;
xdp_rxq_info_unreg(&queue->xdp_rxq);
}

Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
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);

Copilot uses AI. Check for mistakes.
Comment on lines +1737 to +1761
/* 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);

Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +76 to +87
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;
}
}
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}

retry = 0;
wait = 1;
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
wait = 1;

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants