* Re: [PATCH net-next v4 08/10] net: airoha: Do not stop GDM port if it is shared
From: Lorenzo Bianconi @ 2026-05-09 9:00 UTC (permalink / raw)
To: Jakub Kicinski
Cc: andrew+netdev, davem, edumazet, pabeni, robh, krzk+dt, conor+dt,
ansuelsmth, benjamin.larsson, linux-arm-kernel, linux-mediatek,
netdev, devicetree, madhur.agrawal
In-Reply-To: <20260509014613.3558087-1-kuba@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 6349 bytes --]
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: airoha: Do not stop GDM port if it is shared
>
> This patch adds a per-port users refcount so the GDM forward configuration
> is only reset to FE_PSE_PORT_DROP when the last netdev attached to the
> shared GDM port stops, and it adjusts the MTU/length register updates to
> cope with the shared case.
>
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 0253919714e0..1c4927c1aeb0 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > @@ -1714,8 +1714,8 @@ static int airoha_dev_open(struct net_device *netdev)
> > int err, len = ETH_HLEN + netdev->mtu + ETH_FCS_LEN;
> > struct airoha_gdm_dev *dev = netdev_priv(netdev);
> > struct airoha_gdm_port *port = dev->port;
> > + u32 cur_len, pse_port = FE_PSE_PORT_PPE1;
> > struct airoha_qdma *qdma = dev->qdma;
> > - u32 pse_port = FE_PSE_PORT_PPE1;
> >
> > netif_tx_start_all_queues(netdev);
> > err = airoha_set_vip_for_gdm_port(dev, true);
> > @@ -1729,10 +1729,14 @@ static int airoha_dev_open(struct net_device *netdev)
> > airoha_fe_clear(qdma->eth, REG_GDM_INGRESS_CFG(port->id),
> > GDM_STAG_EN_MASK);
> >
> > - airoha_fe_rmw(qdma->eth, REG_GDM_LEN_CFG(port->id),
> > - GDM_SHORT_LEN_MASK | GDM_LONG_LEN_MASK,
> > - FIELD_PREP(GDM_SHORT_LEN_MASK, 60) |
> > - FIELD_PREP(GDM_LONG_LEN_MASK, len));
> > + cur_len = airoha_fe_get(qdma->eth, REG_GDM_LEN_CFG(port->id),
> > + GDM_LONG_LEN_MASK);
> > + if (!atomic_read(&port->users) || len > cur_len)
> > + airoha_fe_rmw(qdma->eth, REG_GDM_LEN_CFG(port->id),
> > + GDM_SHORT_LEN_MASK | GDM_LONG_LEN_MASK,
> > + FIELD_PREP(GDM_SHORT_LEN_MASK, 60) |
> > + FIELD_PREP(GDM_LONG_LEN_MASK, len));
> > + atomic_inc(&port->users);
>
> [Medium]
> When a second netdev opens with a larger MTU, this unconditionally
> widens GDM_LONG_LEN_MASK for the already-running sibling that has a
> smaller MTU:
>
> if (!atomic_read(&port->users) || len > cur_len)
> airoha_fe_rmw(qdma->eth, REG_GDM_LEN_CFG(port->id),
> GDM_SHORT_LEN_MASK | GDM_LONG_LEN_MASK,
> FIELD_PREP(GDM_SHORT_LEN_MASK, 60) |
> FIELD_PREP(GDM_LONG_LEN_MASK, len));
>
> Meanwhile airoha_dev_change_mtu() rejects shrinks (len < cur_len) with
> -EBUSY when the port is shared. Is this asymmetry intentional?
This is done intentionally since we do not want to break traffic of already
running net_device if we want to increase the MTU on a sibling net_device.
>
> With the new policy the hardware long-length ends up as the maximum of
> the two MTUs, so the smaller-MTU sibling silently accepts frames larger
> than its advertised MTU at the MAC level. Should opens that would
> increase the shared length also be gated (symmetric policy), or should
> the commit message and a comment document the chosen ceiling/floor
> behaviour?
I will add a comment in the code in v5.
>
> >
> > airoha_qdma_set(qdma, REG_QDMA_GLOBAL_CFG,
> > GLOBAL_CFG_TX_DMA_EN_MASK |
> > @@ -1762,8 +1766,12 @@ static int airoha_dev_stop(struct net_device *netdev)
> > for (i = 0; i < netdev->num_tx_queues; i++)
> > netdev_tx_reset_subqueue(netdev, i);
> >
> > - airoha_set_gdm_port_fwd_cfg(qdma->eth, REG_GDM_FWD_CFG(port->id),
> > - FE_PSE_PORT_DROP);
> > + if (atomic_dec_and_test(&port->users)) {
> > + airoha_set_vip_for_gdm_port(dev, false);
> > + airoha_set_gdm_port_fwd_cfg(qdma->eth,
> > + REG_GDM_FWD_CFG(port->id),
> > + FE_PSE_PORT_DROP);
> > + }
>
> [Low]
> Is the airoha_set_vip_for_gdm_port(dev, false) call inside the
> atomic_dec_and_test() block intentional?
>
> Looking at airoha_dev_stop() as a whole, the same call is already made
> unconditionally a few lines above:
>
> netif_tx_disable(netdev);
> airoha_set_vip_for_gdm_port(dev, false);
> for (i = 0; i < netdev->num_tx_queues; i++)
> netdev_tx_reset_subqueue(netdev, i);
>
> if (atomic_dec_and_test(&port->users)) {
> airoha_set_vip_for_gdm_port(dev, false);
> ...
> }
>
> Since get_vip_port() derives the bit mask per-(port, dev->nbq) and
> airoha_fe_clear() of already-cleared bits is a no-op, the second call
> looks like dead code today. The commit log only mentions the
> FE_PSE_PORT_DROP gating, not VIP handling. Should the duplicate be
> dropped in v2?
ack, I will fix in v5.
>
> >
> > if (atomic_dec_and_test(&qdma->users)) {
> > airoha_qdma_clear(qdma, REG_QDMA_GLOBAL_CFG,
> > @@ -1915,10 +1923,22 @@ static void airoha_dev_get_stats64(struct net_device *netdev,
> > static int airoha_dev_change_mtu(struct net_device *netdev, int mtu)
> > {
> > struct airoha_gdm_dev *dev = netdev_priv(netdev);
> > + u32 cur_len, len = ETH_HLEN + mtu + ETH_FCS_LEN;
> > struct airoha_gdm_port *port = dev->port;
> > - u32 len = ETH_HLEN + mtu + ETH_FCS_LEN;
> > struct airoha_eth *eth = dev->eth;
> >
> > + cur_len = airoha_fe_get(eth, REG_GDM_LEN_CFG(port->id),
> > + GDM_LONG_LEN_MASK);
> > + if (len < cur_len) {
> > + u8 port_refcnt = atomic_read(&port->users);
> > +
> > + /* We can decrease the device MTU just if the GDM port is
> > + * not shared or if the other device is not running.
> > + */
> > + if (port_refcnt > 1 || (port_refcnt && !netif_running(netdev)))
> > + return -EBUSY;
> > + }
> > +
>
> [Low]
> Why is port_refcnt typed as u8 here when atomic_read() returns int?
>
> u8 port_refcnt = atomic_read(&port->users);
>
> With AIROHA_MAX_NUM_GDM_DEVS currently 2 this cannot overflow, but a
> future bump of AIROHA_MAX_NUM_GDM_DEVS, or a refcount leak, that pushed
> port->users to 256 would wrap to 0 and quietly bypass both the
> port_refcnt > 1 and port_refcnt && !netif_running(netdev) checks,
> permitting MTU shrinks that are meant to be rejected. Would using int
> here (the type atomic_read() returns) be preferable?
ack, I will fix in v5.
Regards,
Lorenzo
>
> > airoha_fe_rmw(eth, REG_GDM_LEN_CFG(port->id),
> > GDM_LONG_LEN_MASK,
> > FIELD_PREP(GDM_LONG_LEN_MASK, len));
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH net-next v4 07/10] net: airoha: Support multiple net_devices for a single FE GDM port
From: Lorenzo Bianconi @ 2026-05-09 8:25 UTC (permalink / raw)
To: Jakub Kicinski
Cc: andrew+netdev, davem, edumazet, pabeni, robh, krzk+dt, conor+dt,
ansuelsmth, benjamin.larsson, linux-arm-kernel, linux-mediatek,
netdev, devicetree, madhur.agrawal
In-Reply-To: <20260509014611.3558060-1-kuba@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 5434 bytes --]
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: airoha: Support multiple net_devices for a single FE GDM port
>
> This commit introduces support for multiple net_devices connected to the
> same Frame Engine (FE) GDM port (GDM3 or GDM4) via an external hardware
> arbiter on EN7581 or AN7583 SoCs, and adds a get_dev_from_sport callback
> to differentiate between the net_devices attached to each port.
>
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 786bc677af3c..0253919714e0 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
>
> [ ... ]
>
> > @@ -3188,14 +3257,23 @@ static int airoha_probe(struct platform_device *pdev)
> >
> > for (i = 0; i < ARRAY_SIZE(eth->ports); i++) {
> > struct airoha_gdm_port *port = eth->ports[i];
> > - struct airoha_gdm_dev *dev;
> > + int j;
> >
> > if (!port)
> > continue;
> >
> > - dev = port->dev;
> > - if (dev && dev->dev->reg_state == NETREG_REGISTERED)
> > - unregister_netdev(dev->dev);
> > + for (j = 0; j < ARRAY_SIZE(port->devs); j++) {
> > + struct airoha_gdm_dev *dev = port->devs[j];
> > + struct net_device *netdev;
> > +
> > + if (!dev)
> > + continue;
> > +
> > + netdev = dev->dev;
> > + of_node_put(netdev->dev.of_node);
> > + if (netdev->reg_state == NETREG_REGISTERED)
> > + unregister_netdev(netdev);
> > + }
> > airoha_metadata_dst_free(port);
> > }
> > airoha_hw_cleanup(eth);
>
> [Medium]
> Is the ordering of of_node_put() and unregister_netdev() here correct?
>
> airoha_alloc_gdm_device() now does:
>
> netdev->dev.of_node = of_node_get(np);
>
> so the netdev holds its own reference to the of_node for its entire
> registered lifetime. In this error unwind the of_node_put() runs before
> unregister_netdev(), and netdev->dev.of_node is not cleared, so any
> subsequent access to netdev->dev.of_node during unregistration (netdev
> notifiers, sysfs, overlay-aware code) would be looking at a node whose
> reference count has already been dropped.
>
> Other drivers (e.g., drivers/net/ethernet/hisilicon/hns/hns_enet.c in
> hns_nic_dev_remove()) put the node only after unregister_netdev().
>
> Would swapping the order to call unregister_netdev() first, then
> of_node_put(), be more appropriate here?
ack, I will fix it in v5.
>
> > @@ -3216,14 +3294,22 @@ static void airoha_remove(struct platform_device *pdev)
> >
> > for (i = 0; i < ARRAY_SIZE(eth->ports); i++) {
> > struct airoha_gdm_port *port = eth->ports[i];
> > - struct airoha_gdm_dev *dev;
> > + int j;
> >
> > if (!port)
> > continue;
> >
> > - dev = port->dev;
> > - if (dev)
> > - unregister_netdev(dev->dev);
> > + for (j = 0; j < ARRAY_SIZE(port->devs); j++) {
> > + struct airoha_gdm_dev *dev = port->devs[j];
> > + struct net_device *netdev;
> > +
> > + if (!dev)
> > + continue;
> > +
> > + netdev = dev->dev;
> > + of_node_put(netdev->dev.of_node);
> > + unregister_netdev(netdev);
> > + }
> > airoha_metadata_dst_free(port);
> > }
> > airoha_hw_cleanup(eth);
>
> [Medium]
> Same question in airoha_remove(): the of_node_put() fires before
> unregister_netdev(), while netdev->dev.of_node is still assigned. Would
> this be safer with the unregister first and the put afterwards?
ack, I will fix it in v5.
>
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> > index 3e93919a175c..207c75152fde 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.h
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.h
>
> [ ... ]
>
> > @@ -542,14 +543,15 @@ struct airoha_qdma {
> > struct airoha_gdm_dev {
> > struct airoha_gdm_port *port;
> > struct airoha_qdma *qdma;
> > - struct net_device *dev;
> > struct airoha_eth *eth;
> > + struct net_device *dev;
> > +
> > + int nbq;
> > };
> >
> > struct airoha_gdm_port {
> > - struct airoha_gdm_dev *dev;
> > + struct airoha_gdm_dev *devs[AIROHA_MAX_NUM_GDM_DEVS];
> > int id;
> > - int nbq;
> >
> > struct airoha_hw_stats stats;
>
> [Medium]
> With multiple airoha_gdm_dev instances now sharing a single
> airoha_gdm_port, do the per-netdev statistics need to become per-netdev
> rather than per-port?
>
> airoha_dev_get_stats64(), airoha_ethtool_get_mac_stats() and
> airoha_ethtool_get_rmon_stats() all read from dev->port->stats, and the
> underlying hardware counters at REG_FE_GDM_*_CNT(port->id) are per-GDM
> port rather than per-netdev. When two netdevs share a port (for example
> PCIE0/PCIE1 on EN7581 GDM3, or ETH/USB on GDM4), both netdevs would
> report identical port-wide counters via rtnl_link_stats64 (ip -s link
> show) and via the get_eth_mac_stats/get_rmon_stats ethtool callbacks,
> and summing across the two netdevs would double-count.
>
> Would it make sense to maintain per-netdev software counters in the
> xmit/RX paths for the multi-dev case so each netdev reports its own
> traffic through the standard uAPI?
We have already a separated patch to address this issue that is not part of the
series. I will add it to the v5 in the next iteration.
Regards,
Lorenzo
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] wifi: mt76: mt792x: fix NULL dereference during CSA beacon handling
From: Bongani Hlope @ 2026-05-09 8:23 UTC (permalink / raw)
To: Sean Wang
Cc: Felix Fietkau, Lorenzo Bianconi, linux-wireless, linux-mediatek,
Sean Wang
In-Reply-To: <20260503004613.17903-1-sean.wang@kernel.org>
Sorry I had some down time on my mail server. I can confirm this patch
works for me, I have been running with it since you sent it through.
Regards
On Sat, 2 May 2026 19:46:13 -0500
Sean Wang <sean.wang@kernel.org> wrote:
> From: Sean Wang <sean.wang@mediatek.com>
>
> mac80211 may call channel_switch_rx_beacon() while CSA is active, but
> mt76's cached dev->new_ctx is not guaranteed to be valid at that
> point.
>
> Avoid dereferencing dev->new_ctx when the target channel context is
> not available and leave the existing CSA timer unchanged.
>
> kernel: Workqueue: events_unbound cfg80211_wiphy_work [cfg80211]
> kernel: RIP: 0010:mt7921_channel_switch_rx_beacon+0x1f/0x100
> [mt7921_common]
> kernel: RAX: 0000000000000000
> kernel: CR2: 0000000000000000
> kernel: Call Trace:
> kernel: <TASK>
> kernel: ieee80211_sta_process_chanswitch+0x67c/0xee0 [mac80211]
> kernel: ieee80211_rx_mgmt_beacon+0x842/0x22a0 [mac80211]
> kernel: ieee80211_sta_rx_queued_mgmt+0xa7/0xbb0 [mac80211]
> kernel: ieee80211_iface_work+0x62e/0x890 [mac80211]
> kernel: cfg80211_wiphy_work+0x1ee/0x280 [cfg80211]
> kernel: process_scheduled_works+0x180/0x680
> kernel: worker_thread+0x1aa/0x450
> kernel: kthread+0x181/0x1e0
> kernel: ret_from_fork+0x405/0x600
> kernel: ret_from_fork_asm+0x11/0x20
> kernel: </TASK>
> kernel: CR2: 0000000000000000
> kernel: ---[ end trace 0000000000000000 ]---
>
> mt7925 has the same unsafe dev->new_ctx dereference in its CSA beacon
> handling path, so guard both drivers against the missing target
> channel context and leave the existing CSA timer unchanged.
>
> Reported-by: Bongani Hlope <developer@hlope.org.za>
> Closes:
> https://lore.kernel.org/linux-wireless/20260502140616.7672da98@bongani-mini.home.org.za/
> Fixes: 8aa2f59260eb ("wifi: mt76: mt7921: introduce CSA support")
> Fixes: 7900da40e315 ("wifi: mt76: mt7925: introduce CSA support in
> non-MLO mode") Signed-off-by: Sean Wang <sean.wang@mediatek.com> ---
> drivers/net/wireless/mediatek/mt76/mt7921/main.c | 3 +++
> drivers/net/wireless/mediatek/mt76/mt7925/main.c | 3 +++
> 2 files changed, 6 insertions(+)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/main.c
> b/drivers/net/wireless/mediatek/mt76/mt7921/main.c index
> 3d74fabe7408..a326f4c95c7c 100644 ---
> a/drivers/net/wireless/mediatek/mt76/mt7921/main.c +++
> b/drivers/net/wireless/mediatek/mt76/mt7921/main.c @@ -1508,6 +1508,9
> @@ static void mt7921_channel_switch_rx_beacon(struct ieee80211_hw
> *hw, struct mt792x_vif *mvif = (struct mt792x_vif *)vif->drv_priv;
> u16 beacon_interval = vif->bss_conf.beacon_int;
> + if (!dev->new_ctx)
> + return;
> +
> if (cfg80211_chandef_identical(&chsw->chandef,
> &dev->new_ctx->def) &&
> chsw->count) {
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7925/main.c
> b/drivers/net/wireless/mediatek/mt76/mt7925/main.c index
> 73d3722739d0..b96a8e2efcbc 100644 ---
> a/drivers/net/wireless/mediatek/mt76/mt7925/main.c +++
> b/drivers/net/wireless/mediatek/mt76/mt7925/main.c @@ -2402,6 +2402,9
> @@ static void mt7925_channel_switch_rx_beacon(struct ieee80211_hw
> *hw, beacon_interval = vif->bss_conf.beacon_int;
>
> + if (!dev->new_ctx)
> + return;
> +
> if (cfg80211_chandef_identical(&chsw->chandef,
> &dev->new_ctx->def) &&
> chsw->count) {
^ permalink raw reply
* Re: [RESEND,v2 2/2] memory: mtk-smi: Add mt8189 support
From: Yong Wu (吴勇) @ 2026-05-09 8:00 UTC (permalink / raw)
To: robh@kernel.org, matthias.bgg@gmail.com, conor+dt@kernel.org,
Zhengnan Chen (陈征南), krzk@kernel.org,
AngeloGioacchino Del Regno
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group
In-Reply-To: <20260427070444.20247-3-zhengnan.chen@mediatek.com>
On Mon, 2026-04-27 at 15:04 +0800, mtk20898 wrote:
> From: Zhengnan Chen <zhengnan.chen@mediatek.com>
>
> Add the necessary platform data and ostdl setting to enable support
> for mt8189 smi.
>
> Signed-off-by: Zhengnan Chen <zhengnan.chen@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> ---
> drivers/memory/mtk-smi.c | 44
> ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
Reviewed-by: Yong Wu <yong.wu@mediatek.com>
^ permalink raw reply
* Re: [PATCH net-next v4 05/10] net: airoha: Move qos_sq_bmap in airoha_qdma struct
From: Lorenzo Bianconi @ 2026-05-09 7:54 UTC (permalink / raw)
To: Jakub Kicinski
Cc: andrew+netdev, davem, edumazet, pabeni, robh, krzk+dt, conor+dt,
ansuelsmth, benjamin.larsson, linux-arm-kernel, linux-mediatek,
netdev, devicetree, madhur.agrawal
In-Reply-To: <20260509014609.3558041-1-kuba@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 6609 bytes --]
On May 08, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: airoha: Move qos_sq_bmap in airoha_qdma struct
>
> The change moves the HTB-offload channel-allocation bitmap from the
> per-netdev airoha_gdm_port to the per-QDMA airoha_qdma, so sibling
> netdevs attached to different QDMA blocks no longer clobber each other's
> bitmap.
>
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 080705e2f58d..69a4c2e0d58b 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > @@ -2600,7 +2600,7 @@ static int airoha_tc_htb_alloc_leaf_queue(struct net_device *netdev,
> > u32 rate = div_u64(opt->rate, 1000) << 3; /* kbps */
> > int err, num_tx_queues = netdev->real_num_tx_queues;
> > struct airoha_gdm_dev *dev = netdev_priv(netdev);
> > - struct airoha_gdm_port *port = dev->port;
> > + struct airoha_qdma *qdma = dev->qdma;
> >
> > if (opt->parent_classid != TC_HTB_CLASSID_ROOT) {
> > NL_SET_ERR_MSG_MOD(opt->extack, "invalid parent classid");
> > @@ -2627,7 +2627,7 @@ static int airoha_tc_htb_alloc_leaf_queue(struct net_device *netdev,
> > return err;
> > }
> >
> > - set_bit(channel, port->qos_sq_bmap);
> > + set_bit(channel, qdma->qos_sq_bmap);
> > opt->qid = AIROHA_NUM_TX_RING + channel;
> >
> > return 0;
>
> [High]
> Now that qos_sq_bmap is per-QDMA, can two sibling netdevs that share the
> same QDMA block collide on allocation? On EN7581, GDM2/GDM3/GDM4 all
> select the same qdma pointer via:
>
> drivers/net/ethernet/airoha/airoha_eth.c:
> /* QDMA0 is used for lan ports while QDMA1 is used for WAN ports */
> dev->qdma = ð->qdma[!airoha_is_lan_gdm_dev(dev)];
>
> and airoha_is_lan_gdm_dev() returns true only for port->id == 1, so
> every non-LAN GDM netdev resolves to eth->qdma[1] and shares
> qdma->qos_sq_bmap.
>
> The allocation path does not test the bit before setting it:
>
> channel = TC_H_MIN(opt->classid) % AIROHA_NUM_QOS_CHANNELS;
> ...
> set_bit(channel, qdma->qos_sq_bmap);
> opt->qid = AIROHA_NUM_TX_RING + channel;
>
> If two sibling netdevs on the same QDMA pick classids whose TC_H_MIN
> maps to the same channel, both airoha_qdma_set_tx_rate_limit() calls
> program the same HW TRTCM egress slot, the later caller silently
> overwrites the earlier configuration, and both netdevs receive the same
> opt->qid. Should this path use test_and_set_bit() (or similar owner
> tracking) to reject a channel already owned by a sibling netdev?
ack, I think we should move qos_sq_bmap in airoha_gdm_dev struct and track if a
shared qdma qos channel is already in use. I will fix it in v5.
>
> > @@ -2811,11 +2811,11 @@ static int airoha_dev_setup_tc_block(struct net_device *dev,
> > static void airoha_tc_remove_htb_queue(struct net_device *netdev, int queue)
> > {
> > struct airoha_gdm_dev *dev = netdev_priv(netdev);
> > - struct airoha_gdm_port *port = dev->port;
> > + struct airoha_qdma *qdma = dev->qdma;
> >
> > netif_set_real_num_tx_queues(netdev, netdev->real_num_tx_queues - 1);
> > airoha_qdma_set_tx_rate_limit(netdev, queue + 1, 0, 0);
> > - clear_bit(queue, port->qos_sq_bmap);
> > + clear_bit(queue, qdma->qos_sq_bmap);
> > }
>
> [High]
> airoha_tc_remove_htb_queue() operates unconditionally on the caller's
> netdev but clears a bit that may be owned by a sibling netdev on the
> same QDMA. The effects that follow look like cross-netdev interference:
>
> - netif_set_real_num_tx_queues(netdev, real_num_tx_queues - 1)
> decrements the caller's tx-queue count for a queue it never
> allocated, and its return value is discarded (eventually the txq<1
> EINVAL guard in netif_set_real_num_tx_queues() will trip silently).
>
> - airoha_qdma_set_tx_rate_limit(netdev, queue + 1, 0, 0) zeros the
> egress TRTCM rate-limit registers for a channel that the sibling
> netdev still considers active.
>
> - clear_bit(queue, qdma->qos_sq_bmap) then hides the channel from the
> legitimate owner's later test_bit() checks.
This issue will be fixed by the change described above. I will fix it in v5.
>
> > @@ -2838,10 +2838,10 @@ static int airoha_tc_htb_delete_leaf_queue(struct net_device *netdev,
> > static int airoha_tc_htb_destroy(struct net_device *netdev)
> > {
> > struct airoha_gdm_dev *dev = netdev_priv(netdev);
> > - struct airoha_gdm_port *port = dev->port;
> > + struct airoha_qdma *qdma = dev->qdma;
> > int q;
> >
> > - for_each_set_bit(q, port->qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS)
> > + for_each_set_bit(q, qdma->qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS)
> > airoha_tc_remove_htb_queue(netdev, q);
> >
> > return 0;
> > }
>
> [High]
> Does airoha_tc_htb_destroy() now walk bits owned by sibling netdevs?
> The iteration no longer filters by owning netdev:
>
> for_each_set_bit(q, qdma->qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS)
> airoha_tc_remove_htb_queue(netdev, q);
>
> Previously each netdev had a private bitmap, so destroy could only
> touch its own channels. After this change, tearing down HTB on one
> GDM netdev appears to remove channels and clear rate-limit HW state
> belonging to every sibling GDM netdev on the same QDMA. A subsequent
> tc-htb delete or get on the legitimate owner then hits:
>
> if (!test_bit(channel, qdma->qos_sq_bmap)) {
> NL_SET_ERR_MSG_MOD(opt->extack, "invalid queue id");
> return -EINVAL;
> }
>
> Should the bitmap track the owning netdev alongside the bit (for
> example a per-channel owner pointer, or a per-netdev sub-bitmap inside
> airoha_qdma), so that destroy/delete can only touch channels owned by
> the caller?
This issue will be fixed by the change described above. I will fix it in v5.
Regards,
Lorenzo
>
> > @@ -2852,9 +2852,9 @@ static int airoha_tc_get_htb_get_leaf_queue(struct net_device *netdev,
> > {
> > u32 channel = TC_H_MIN(opt->classid) % AIROHA_NUM_QOS_CHANNELS;
> > struct airoha_gdm_dev *dev = netdev_priv(netdev);
> > - struct airoha_gdm_port *port = dev->port;
> > + struct airoha_qdma *qdma = dev->qdma;
> >
> > - if (!test_bit(channel, port->qos_sq_bmap)) {
> > + if (!test_bit(channel, qdma->qos_sq_bmap)) {
> > NL_SET_ERR_MSG_MOD(opt->extack, "invalid queue id");
> > return -EINVAL;
> > }
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH net-next v4 03/10] net: airoha: Move airoha_qdma pointer in airoha_gdm_dev struct
From: Lorenzo Bianconi @ 2026-05-09 6:17 UTC (permalink / raw)
To: Jakub Kicinski
Cc: andrew+netdev, davem, edumazet, pabeni, robh, krzk+dt, conor+dt,
ansuelsmth, benjamin.larsson, linux-arm-kernel, linux-mediatek,
netdev, devicetree, madhur.agrawal
In-Reply-To: <20260509014606.3558018-1-kuba@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 2854 bytes --]
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> Subject: net: airoha: Move airoha_qdma pointer in airoha_gdm_dev struct
>
> This patch moves the airoha_qdma pointer from struct airoha_gdm_port into
> struct airoha_gdm_dev, as a preparation for allowing multiple net_devices
> to share the same GDM{3,4} port via an external hw arbiter.
>
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 4af64f182968..1798b4a6cf5d 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
>
> [ ... ]
>
> > @@ -854,7 +855,8 @@ static void airoha_qdma_wake_netdev_txqs(struct airoha_queue *q)
> > if (!port)
> > continue;
> >
> > - if (port->qdma != qdma)
> > + dev = port->dev;
> > + if (dev->qdma != qdma)
> > continue;
> >
> > dev = port->dev;
>
> [Low]
> The resulting loop body assigns dev = port->dev; twice with nothing in
> between that could change port->dev:
>
> dev = port->dev;
> if (dev->qdma != qdma)
> continue;
>
> dev = port->dev;
> for (j = 0; j < dev->dev->num_tx_queues; j++) {
ack, I will fix it in v5.
>
> Is the second dev = port->dev; intentional, or leftover from the
> refactor that should be removed?
>
> [Medium]
> Can the new dev->qdma dereference run while port->dev is still NULL?
>
> Looking at airoha_alloc_gdm_port(), eth->ports[p] = port is stored before
> airoha_alloc_gdm_device() runs, and airoha_alloc_gdm_device() only sets
> port->dev = dev at its very end, after several failure points
> (devm_alloc_etherdev_mqs, netif_set_real_num_tx_queues,
> of_get_ethdev_address).
>
> The existing cleanup in airoha_probe's error_napi_stop path and in
> airoha_remove guards accesses with if (dev && ...) when reading
> port->dev, which suggests port->dev can legitimately be NULL while
> eth->ports[i] is non-NULL.
>
> Before the patch the equivalent check read port->qdma (an embedded field
> zeroed by devm_kzalloc), so the compare was NULL-tolerant. After the
> patch:
>
> struct airoha_gdm_port *port = eth->ports[i];
> struct airoha_gdm_dev *dev;
> ...
> if (!port)
> continue;
>
> dev = port->dev;
> if (dev->qdma != qdma)
> continue;
>
> would dereference a NULL dev if port->dev has not yet been assigned when
> airoha_qdma_wake_netdev_txqs() runs. Would a NULL check on port->dev (or
> reverting to the original port-field-based test) be appropriate here to
> preserve the prior robustness?
Even if this is not a real issue since it will be fixed by a following patch
in the series ("Support multiple net_devices for a single FE GDM port "), I
will fix it in v5.
Regards,
Lorenzo
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [net-next v7 3/3] net: ethernet: mtk_eth_soc: Add LRO support
From: Frank Wunderlich @ 2026-05-09 6:08 UTC (permalink / raw)
To: Jakub Kicinski, Frank Wunderlich
Cc: Felix Fietkau, Lorenzo Bianconi, Andrew Lunn, David S. Miller,
Eric Dumazet, Paolo Abeni, Matthias Brugger,
AngeloGioacchino Del Regno, Russell King, netdev, linux-kernel,
linux-arm-kernel, linux-mediatek, Mason Chang, Daniel Golle
In-Reply-To: <20260508153702.69b89021@kernel.org>
Am 9. Mai 2026 00:37:02 MESZ schrieb Jakub Kicinski <kuba@kernel.org>:
>On Wed, 6 May 2026 21:28:03 +0200 Frank Wunderlich wrote:
>> - fix u32 vs. be32 reported by patchwork check
>Have you tested this? I still see an extra sparse warning
>(install sparse and build with make C=1)
I only tested functionality as i had not figured
out how to reproduce
these errors till v7 but already noticed that there are still some open.
I had to self compile sparse and call like you mention.
But I already prepare v8 where these are really fixed.
I just testing 16bit versions of ntoh/hton vs beX_to_cpu.
Sorry for extra version here.
>It'd probably be good to fix the pre-exisiting warnings as the first
>patch in the series while at it.
I send out v8 today. But good to know that no other parts
need to be changed :)
regards Frank
^ permalink raw reply
* Re: [PATCH net-next v4 10/10] net: airoha: Support multiple LAN/WAN interfaces for hw MAC address configuration
From: Jakub Kicinski @ 2026-05-09 1:46 UTC (permalink / raw)
To: lorenzo
Cc: Jakub Kicinski, andrew+netdev, davem, edumazet, pabeni, robh,
krzk+dt, conor+dt, ansuelsmth, benjamin.larsson, linux-arm-kernel,
linux-mediatek, netdev, devicetree, madhur.agrawal
In-Reply-To: <20260507-airoha-eth-multi-serdes-v4-10-af613b61ae02@kernel.org>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: airoha: Support multiple LAN/WAN interfaces for hw MAC address configuration
This patch extends airoha_set_macaddr() to derive REG_FE_MAC_LMIN and
REG_FE_MAC_LMAX by iterating over all sibling LAN/WAN net_devices, and
rejects addresses whose upper 3 bytes do not match the already-registered
peers. airoha_dev_set_macaddr() is also changed to split eth_mac_addr()
into eth_prepare_mac_addr_change() and eth_commit_mac_addr_change() around
the hardware programming step.
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 8617dd4f7932..fbfefd0ec355 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -71,20 +71,67 @@ static void airoha_qdma_irq_disable(struct airoha_irq_bank *irq_bank,
> airoha_qdma_set_irqmask(irq_bank, index, mask, 0);
> }
>
> -static void airoha_set_macaddr(struct airoha_gdm_dev *dev, const u8 *addr)
> +static int airoha_set_macaddr(struct airoha_gdm_dev *dev, const u8 *addr)
> {
> struct airoha_eth *eth = dev->eth;
> - u32 val, reg;
> + u8 ref_addr[ETH_ALEN] = {};
> + u32 reg, val, lmin, lmax;
> + int i;
> +
> + lmin = (addr[3] << 16) | (addr[4] << 8) | addr[5];
> + lmax = lmin;
> +
> + for (i = 0; i < ARRAY_SIZE(eth->ports); i++) {
> + struct airoha_gdm_port *port = eth->ports[i];
> + int j;
> +
> + if (!port)
> + continue;
> +
> + for (j = 0; j < ARRAY_SIZE(port->devs); j++) {
> + struct airoha_gdm_dev *iter_dev;
> + struct net_device *netdev;
> +
> + iter_dev = port->devs[j];
> + if (!iter_dev || iter_dev == dev)
> + continue;
> +
> + if (airoha_is_lan_gdm_dev(iter_dev) !=
> + airoha_is_lan_gdm_dev(dev))
> + continue;
> +
> + netdev = iter_dev->dev;
> + if (netdev->reg_state != NETREG_REGISTERED)
> + continue;
> +
> + ether_addr_copy(ref_addr, netdev->dev_addr);
> + val = (netdev->dev_addr[3] << 16) |
> + (netdev->dev_addr[4] << 8) | netdev->dev_addr[5];
> + if (val < lmin)
> + lmin = val;
> + if (val > lmax)
> + lmax = val;
> + }
> + }
> +
> + if (!is_zero_ether_addr(ref_addr) && memcmp(ref_addr, addr, 3)) {
> + /* According to the HW design, hw mac address MS bits
> + * must be the same for each net_device with the same
> + * LAN/WAN configuration.
> + */
> + return -EINVAL;
> + }
[Medium]
The NETREG_REGISTERED filter here means the check only sees peers that
have already completed register_netdev(). airoha_dev_init() is invoked
as ndo_init from register_netdev() before dev->reg_state transitions
to NETREG_REGISTERED, so the constraint is only enforced across
already-registered siblings.
When the device tree does not supply a mac-address property,
airoha_alloc_gdm_device() falls back to eth_hw_addr_random() and each
sibling gets an independently generated random MAC. With two or more
LAN (or two or more WAN) net_devices going through this fallback, the
second and later siblings' upper 3 bytes will almost always differ
from the first, so airoha_set_macaddr() returns -EINVAL:
if (!is_zero_ether_addr(ref_addr) && memcmp(ref_addr, addr, 3))
return -EINVAL;
airoha_dev_init() now propagates that error, which fails
register_netdev() and airoha_register_gdm_devices(), aborting probe.
Before this patch the same configuration still came up (with the single
REG_FE_{LAN,WAN}_MAC_H being last-writer-wins). Should the random
fallback path derive sibling MACs from a shared base, or should the
upper-3-bytes constraint only be enforced for user-triggered
ndo_set_mac_address rather than for init-time programming?
>
> reg = airoha_is_lan_gdm_dev(dev) ? REG_FE_LAN_MAC_H : REG_FE_WAN_MAC_H;
> val = (addr[0] << 16) | (addr[1] << 8) | addr[2];
> airoha_fe_wr(eth, reg, val);
>
> - val = (addr[3] << 16) | (addr[4] << 8) | addr[5];
> - airoha_fe_wr(eth, REG_FE_MAC_LMIN(reg), val);
> - airoha_fe_wr(eth, REG_FE_MAC_LMAX(reg), val);
> + airoha_fe_wr(eth, REG_FE_MAC_LMIN(reg), lmin);
> + airoha_fe_wr(eth, REG_FE_MAC_LMAX(reg), lmax);
>
> airoha_ppe_init_upd_mem(dev);
> +
> + return 0;
> }
>
> static void airoha_set_gdm_port_fwd_cfg(struct airoha_eth *eth, u32 addr,
> @@ -1792,13 +1839,18 @@ static int airoha_dev_stop(struct net_device *netdev)
> static int airoha_dev_set_macaddr(struct net_device *netdev, void *p)
> {
> struct airoha_gdm_dev *dev = netdev_priv(netdev);
> + struct sockaddr *addr = p;
> int err;
>
> - err = eth_mac_addr(netdev, p);
> + err = eth_prepare_mac_addr_change(netdev, p);
> if (err)
> return err;
>
> - airoha_set_macaddr(dev, netdev->dev_addr);
> + err = airoha_set_macaddr(dev, addr->sa_data);
> + if (err)
> + return err;
> +
> + eth_commit_mac_addr_change(netdev, p);
>
> return 0;
> }
[High]
After this reordering, does airoha_ppe_init_upd_mem() end up
programming the PPE update memory with the old MAC rather than the new
one?
eth_prepare_mac_addr_change() only validates the sockaddr and does not
touch netdev->dev_addr; the actual write to netdev->dev_addr happens in
eth_commit_mac_addr_change() via eth_hw_addr_set():
net/ethernet/eth.c:eth_prepare_mac_addr_change() {
if (!(dev->priv_flags & IFF_LIVE_ADDR_CHANGE) && netif_running(dev))
return -EBUSY;
if (!is_valid_ether_addr(addr->sa_data))
return -EADDRNOTAVAIL;
return 0;
}
net/ethernet/eth.c:eth_commit_mac_addr_change() {
eth_hw_addr_set(dev, addr->sa_data);
}
airoha_set_macaddr() is invoked between those two, so when it reaches
airoha_ppe_init_upd_mem(dev) the commit has not yet run. That helper
reads netdev->dev_addr directly rather than using the addr argument:
drivers/net/ethernet/airoha/airoha_ppe.c:airoha_ppe_init_upd_mem() {
const u8 *addr = netdev->dev_addr;
...
val = (addr[2] << 24) | (addr[3] << 16) | (addr[4] << 8) | addr[5];
airoha_fe_wr(eth, REG_UPDMEM_DATA(0), val);
...
val = (addr[0] << 8) | addr[1];
airoha_fe_wr(eth, REG_UPDMEM_DATA(0), val);
...
}
So the FE registers REG_FE_{LAN,WAN}_MAC_H / REG_FE_MAC_LMIN /
REG_FE_MAC_LMAX are written from the new addr parameter, but the PPE
REG_UPDMEM_DATA/REG_UPDMEM_CTRL entries are written from the still-old
netdev->dev_addr.
Previously eth_mac_addr() (prepare + commit combined) ran first, so
netdev->dev_addr was the new MAC by the time airoha_set_macaddr() and
airoha_ppe_init_upd_mem() ran, keeping the two blocks in sync.
Would it be cleaner to either pass addr through to
airoha_ppe_init_upd_mem(), or invoke eth_commit_mac_addr_change() before
airoha_set_macaddr() (with a rollback path if the hardware programming
fails)?
> @@ -1903,6 +1955,7 @@ static int airoha_dev_init(struct net_device *netdev)
[ ... ]
^ permalink raw reply
* Re: [PATCH net-next v4 09/10] net: airoha: Introduce WAN device flag
From: Jakub Kicinski @ 2026-05-09 1:46 UTC (permalink / raw)
To: lorenzo
Cc: Jakub Kicinski, andrew+netdev, davem, edumazet, pabeni, robh,
krzk+dt, conor+dt, ansuelsmth, benjamin.larsson, linux-arm-kernel,
linux-mediatek, netdev, devicetree, madhur.agrawal
In-Reply-To: <20260507-airoha-eth-multi-serdes-v4-9-af613b61ae02@kernel.org>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: airoha: Introduce WAN device flag
This commit replaces the port->id == 1 LAN/WAN test with a per-dev
PRIV_FLAG_WAN bit that is set during airoha_dev_init() based on the
port id and the presence of other WAN devices. It also extracts the
QDMA selection/PPE cpu port programming into a new helper and reworks
the GDM2 loopback path to key off the new flag.
A few questions about the new policy and error paths.
[Medium]
The commit message says:
Introduce WAN flag to specify if a given device is used to
transmit/receive WAN or LAN traffic. [...] Please note it is possible
to specify multiple LAN devices but just a single WAN one.
Under the old helper (port->id == 1), GDM2/GDM3/GDM4 were all treated as
WAN. Under the new logic, when GDM2 (eth->ports[1]) is present, the
GDM3/GDM4 case breaks out of the switch without setting PRIV_FLAG_WAN, so
GDM3 and GDM4 are now classified as LAN. That flips the MAC register
(REG_FE_WAN_MAC_H -> REG_FE_LAN_MAC_H in airoha_set_macaddr()), the QDMA
binding (qdma[1] -> qdma[0] in airoha_dev_set_qdma()), and the PPE cpu
port (CDM2 -> CDM1). Is that role change intentional, and could the
commit message call it out so bisecting a board that relied on the old
GDM3/GDM4=WAN semantics is easier?
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 1c4927c1aeb0..8617dd4f7932 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -1863,36 +1863,77 @@ static int airoha_set_gdm2_loopback(struct airoha_gdm_dev *dev)
> return 0;
> }
>
> -static int airoha_dev_init(struct net_device *netdev)
> +static struct airoha_gdm_dev *
> +airoha_get_wan_gdm_dev(struct airoha_eth *eth)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(eth->ports); i++) {
> + struct airoha_gdm_port *port = eth->ports[i];
> + int j;
> +
> + if (!port)
> + continue;
> +
> + for (j = 0; j < ARRAY_SIZE(port->devs); j++) {
> + struct airoha_gdm_dev *dev = port->devs[j];
> +
> + if (dev && !airoha_is_lan_gdm_dev(dev))
> + return dev;
> + }
> + }
> +
> + return NULL;
> +}
[High]
airoha_register_gdm_devices() invokes airoha_dev_init() for each dev in
port->devs[] sequentially. On EN7581 GDM3 supports two devs (nbq 4/5 for
PCIE0/PCIE1 in airoha_alloc_gdm_device()). When both devs are registered,
devs[0] sets PRIV_FLAG_WAN and then devs[1] sees the first via
airoha_get_wan_gdm_dev() and falls through to LAN. Is that the intended
result for two devs on the same physical GDM3/GDM4 port?
The two devs then diverge:
airoha_dev_set_qdma()
dev->qdma = ð->qdma[!airoha_is_lan_gdm_dev(dev)];
so devs[1] binds to qdma[0] while devs[0] is on qdma[1];
airoha_set_macaddr()
reg = airoha_is_lan_gdm_dev(dev) ? REG_FE_LAN_MAC_H
: REG_FE_WAN_MAC_H;
so devs[1] writes into the LAN MAC range that belongs to GDM1; and in
airoha_ppe_foe_entry_prepare() pse_port becomes port->id instead of 2
(GDM2 loopback) only for devs[1]. Is there an assumption that sibling
devs on the same port must share the WAN/LAN classification?
> +
> +static void airoha_dev_set_qdma(struct airoha_gdm_dev *dev)
> {
> - struct airoha_gdm_dev *dev = netdev_priv(netdev);
> - struct airoha_gdm_port *port = dev->port;
> struct airoha_eth *eth = dev->eth;
> int i;
>
> /* QDMA0 is used for lan ports while QDMA1 is used for WAN ports */
> dev->qdma = ð->qdma[!airoha_is_lan_gdm_dev(dev)];
> dev->dev->irq = dev->qdma->irq_banks[0].irq;
> - airoha_set_macaddr(dev, netdev->dev_addr);
> +
> + for (i = 0; i < eth->soc->num_ppe; i++)
> + airoha_ppe_set_cpu_port(dev, i, airoha_get_fe_port(dev));
> +}
> +
> +static int airoha_dev_init(struct net_device *netdev)
> +{
> + struct airoha_gdm_dev *dev = netdev_priv(netdev);
> + struct airoha_gdm_port *port = dev->port;
>
> switch (port->id) {
> case AIROHA_GDM3_IDX:
> - case AIROHA_GDM4_IDX:
> - /* If GDM2 is active we can't enable loopback */
> - if (!eth->ports[1]) {
> - int err;
> + case AIROHA_GDM4_IDX: {
> + struct airoha_eth *eth = dev->eth;
>
> - err = airoha_set_gdm2_loopback(dev);
> - if (err)
> - return err;
> - }
> + if (eth->ports[1] || airoha_get_wan_gdm_dev(eth))
> + break;
[Medium]
Is the eth->ports[1] check equivalent to asking whether a GDM2 dev
actually exists? airoha_alloc_gdm_port() assigns eth->ports[p] before
iterating the "airoha,eth-port" child nodes, so with all GDM2 child nodes
marked status="disabled" and GDM3 or GDM4 enabled, eth->ports[1] is
non-NULL but port->devs[] is empty. In that case this branch still breaks
out of the switch and the uplink GDM3/GDM4 dev ends up LAN. Would checking
for an actual dev under eth->ports[1] be safer than checking the port
pointer alone?
[Medium, Low]
The policy that "just a single WAN one" is allowed is enforced by silently
breaking here when airoha_get_wan_gdm_dev() returns non-NULL. Would a
netdev_warn() or -EINVAL help an administrator notice that a second WAN
candidate was demoted to LAN? Also, with both GDM3 and GDM4 enabled and
GDM2 absent, the winner of PRIV_FLAG_WAN depends on eth->ports[] iteration
order and the port/dev allocation order; is there a deterministic,
documented way to choose which dev becomes WAN (DT property, for example)?
> + fallthrough;
> + }
> + case AIROHA_GDM2_IDX:
> + /* GDM2 is always used as wan */
> + dev->flags |= PRIV_FLAG_WAN;
> break;
> default:
> break;
> }
>
> - for (i = 0; i < eth->soc->num_ppe; i++)
> - airoha_ppe_set_cpu_port(dev, i, airoha_get_fe_port(dev));
> + airoha_dev_set_qdma(dev);
> + airoha_set_macaddr(dev, netdev->dev_addr);
> +
> + if (!airoha_is_lan_gdm_dev(dev) &&
> + (port->id == AIROHA_GDM3_IDX || port->id == AIROHA_GDM4_IDX)) {
> + int err;
> +
> + err = airoha_set_gdm2_loopback(dev);
> + if (err) {
> + dev->flags &= ~PRIV_FLAG_WAN;
> + return err;
> + }
> + }
[Medium]
On the error path, is clearing PRIV_FLAG_WAN sufficient? By the time
airoha_set_gdm2_loopback() returns a failure, airoha_dev_set_qdma() has
already pointed dev->qdma at ð->qdma[1] and set dev->dev->irq to the
WAN IRQ, airoha_set_macaddr() has written into REG_FE_WAN_MAC_H,
airoha_ppe_set_cpu_port() has programmed CDM2 as the PPE cpu port, and
airoha_set_gdm2_loopback() itself may have already programmed
REG_GDM_FWD_CFG(GDM2), REG_GDM_TXCHN_EN/RXCHN_EN, REG_GDM_LPBK_CFG,
REG_GDM_LEN_CFG, REG_FE_VIP_PORT_EN and REG_FE_IFC_PORT_EN before the
get_sport() branch failed.
After clearing the flag airoha_is_lan_gdm_dev(dev) reports LAN but
dev->qdma and the hardware registers above still describe the WAN setup.
Would it be cleaner to either unwind those steps or drop the flag clear?
>
> return 0;
> }
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index 3a313ac439e7..5715b03e630b 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -540,12 +540,17 @@ struct airoha_qdma {
> u64 fwd_tx_packets;
> };
>
> +enum airoha_priv_flags {
> + PRIV_FLAG_WAN = BIT(0),
> +};
[Low]
This isn't a bug, but PRIV_FLAG_WAN and the u32 flags field below overlap
conceptually with struct net_device::priv_flags and the IFF_* namespace.
Would a driver-prefixed name such as AIROHA_DEV_F_WAN make the
driver-local scope obvious to future readers?
> +
> struct airoha_gdm_dev {
> struct airoha_gdm_port *port;
> struct airoha_qdma *qdma;
> struct airoha_eth *eth;
> struct net_device *dev;
>
> + u32 flags;
> int nbq;
> };
[ ... ]
^ permalink raw reply
* Re: [PATCH net-next v4 07/10] net: airoha: Support multiple net_devices for a single FE GDM port
From: Jakub Kicinski @ 2026-05-09 1:46 UTC (permalink / raw)
To: lorenzo
Cc: Jakub Kicinski, andrew+netdev, davem, edumazet, pabeni, robh,
krzk+dt, conor+dt, ansuelsmth, benjamin.larsson, linux-arm-kernel,
linux-mediatek, netdev, devicetree, madhur.agrawal
In-Reply-To: <20260507-airoha-eth-multi-serdes-v4-7-af613b61ae02@kernel.org>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: airoha: Support multiple net_devices for a single FE GDM port
This commit introduces support for multiple net_devices connected to the
same Frame Engine (FE) GDM port (GDM3 or GDM4) via an external hardware
arbiter on EN7581 or AN7583 SoCs, and adds a get_dev_from_sport callback
to differentiate between the net_devices attached to each port.
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 786bc677af3c..0253919714e0 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
[ ... ]
> @@ -3188,14 +3257,23 @@ static int airoha_probe(struct platform_device *pdev)
>
> for (i = 0; i < ARRAY_SIZE(eth->ports); i++) {
> struct airoha_gdm_port *port = eth->ports[i];
> - struct airoha_gdm_dev *dev;
> + int j;
>
> if (!port)
> continue;
>
> - dev = port->dev;
> - if (dev && dev->dev->reg_state == NETREG_REGISTERED)
> - unregister_netdev(dev->dev);
> + for (j = 0; j < ARRAY_SIZE(port->devs); j++) {
> + struct airoha_gdm_dev *dev = port->devs[j];
> + struct net_device *netdev;
> +
> + if (!dev)
> + continue;
> +
> + netdev = dev->dev;
> + of_node_put(netdev->dev.of_node);
> + if (netdev->reg_state == NETREG_REGISTERED)
> + unregister_netdev(netdev);
> + }
> airoha_metadata_dst_free(port);
> }
> airoha_hw_cleanup(eth);
[Medium]
Is the ordering of of_node_put() and unregister_netdev() here correct?
airoha_alloc_gdm_device() now does:
netdev->dev.of_node = of_node_get(np);
so the netdev holds its own reference to the of_node for its entire
registered lifetime. In this error unwind the of_node_put() runs before
unregister_netdev(), and netdev->dev.of_node is not cleared, so any
subsequent access to netdev->dev.of_node during unregistration (netdev
notifiers, sysfs, overlay-aware code) would be looking at a node whose
reference count has already been dropped.
Other drivers (e.g., drivers/net/ethernet/hisilicon/hns/hns_enet.c in
hns_nic_dev_remove()) put the node only after unregister_netdev().
Would swapping the order to call unregister_netdev() first, then
of_node_put(), be more appropriate here?
> @@ -3216,14 +3294,22 @@ static void airoha_remove(struct platform_device *pdev)
>
> for (i = 0; i < ARRAY_SIZE(eth->ports); i++) {
> struct airoha_gdm_port *port = eth->ports[i];
> - struct airoha_gdm_dev *dev;
> + int j;
>
> if (!port)
> continue;
>
> - dev = port->dev;
> - if (dev)
> - unregister_netdev(dev->dev);
> + for (j = 0; j < ARRAY_SIZE(port->devs); j++) {
> + struct airoha_gdm_dev *dev = port->devs[j];
> + struct net_device *netdev;
> +
> + if (!dev)
> + continue;
> +
> + netdev = dev->dev;
> + of_node_put(netdev->dev.of_node);
> + unregister_netdev(netdev);
> + }
> airoha_metadata_dst_free(port);
> }
> airoha_hw_cleanup(eth);
[Medium]
Same question in airoha_remove(): the of_node_put() fires before
unregister_netdev(), while netdev->dev.of_node is still assigned. Would
this be safer with the unregister first and the put afterwards?
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index 3e93919a175c..207c75152fde 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
[ ... ]
> @@ -542,14 +543,15 @@ struct airoha_qdma {
> struct airoha_gdm_dev {
> struct airoha_gdm_port *port;
> struct airoha_qdma *qdma;
> - struct net_device *dev;
> struct airoha_eth *eth;
> + struct net_device *dev;
> +
> + int nbq;
> };
>
> struct airoha_gdm_port {
> - struct airoha_gdm_dev *dev;
> + struct airoha_gdm_dev *devs[AIROHA_MAX_NUM_GDM_DEVS];
> int id;
> - int nbq;
>
> struct airoha_hw_stats stats;
[Medium]
With multiple airoha_gdm_dev instances now sharing a single
airoha_gdm_port, do the per-netdev statistics need to become per-netdev
rather than per-port?
airoha_dev_get_stats64(), airoha_ethtool_get_mac_stats() and
airoha_ethtool_get_rmon_stats() all read from dev->port->stats, and the
underlying hardware counters at REG_FE_GDM_*_CNT(port->id) are per-GDM
port rather than per-netdev. When two netdevs share a port (for example
PCIE0/PCIE1 on EN7581 GDM3, or ETH/USB on GDM4), both netdevs would
report identical port-wide counters via rtnl_link_stats64 (ip -s link
show) and via the get_eth_mac_stats/get_rmon_stats ethtool callbacks,
and summing across the two netdevs would double-count.
Would it make sense to maintain per-netdev software counters in the
xmit/RX paths for the multi-dev case so each netdev reports its own
traffic through the standard uAPI?
^ permalink raw reply
* Re: [PATCH net-next v4 08/10] net: airoha: Do not stop GDM port if it is shared
From: Jakub Kicinski @ 2026-05-09 1:46 UTC (permalink / raw)
To: lorenzo
Cc: Jakub Kicinski, andrew+netdev, davem, edumazet, pabeni, robh,
krzk+dt, conor+dt, ansuelsmth, benjamin.larsson, linux-arm-kernel,
linux-mediatek, netdev, devicetree, madhur.agrawal
In-Reply-To: <20260507-airoha-eth-multi-serdes-v4-8-af613b61ae02@kernel.org>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: airoha: Do not stop GDM port if it is shared
This patch adds a per-port users refcount so the GDM forward configuration
is only reset to FE_PSE_PORT_DROP when the last netdev attached to the
shared GDM port stops, and it adjusts the MTU/length register updates to
cope with the shared case.
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 0253919714e0..1c4927c1aeb0 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -1714,8 +1714,8 @@ static int airoha_dev_open(struct net_device *netdev)
> int err, len = ETH_HLEN + netdev->mtu + ETH_FCS_LEN;
> struct airoha_gdm_dev *dev = netdev_priv(netdev);
> struct airoha_gdm_port *port = dev->port;
> + u32 cur_len, pse_port = FE_PSE_PORT_PPE1;
> struct airoha_qdma *qdma = dev->qdma;
> - u32 pse_port = FE_PSE_PORT_PPE1;
>
> netif_tx_start_all_queues(netdev);
> err = airoha_set_vip_for_gdm_port(dev, true);
> @@ -1729,10 +1729,14 @@ static int airoha_dev_open(struct net_device *netdev)
> airoha_fe_clear(qdma->eth, REG_GDM_INGRESS_CFG(port->id),
> GDM_STAG_EN_MASK);
>
> - airoha_fe_rmw(qdma->eth, REG_GDM_LEN_CFG(port->id),
> - GDM_SHORT_LEN_MASK | GDM_LONG_LEN_MASK,
> - FIELD_PREP(GDM_SHORT_LEN_MASK, 60) |
> - FIELD_PREP(GDM_LONG_LEN_MASK, len));
> + cur_len = airoha_fe_get(qdma->eth, REG_GDM_LEN_CFG(port->id),
> + GDM_LONG_LEN_MASK);
> + if (!atomic_read(&port->users) || len > cur_len)
> + airoha_fe_rmw(qdma->eth, REG_GDM_LEN_CFG(port->id),
> + GDM_SHORT_LEN_MASK | GDM_LONG_LEN_MASK,
> + FIELD_PREP(GDM_SHORT_LEN_MASK, 60) |
> + FIELD_PREP(GDM_LONG_LEN_MASK, len));
> + atomic_inc(&port->users);
[Medium]
When a second netdev opens with a larger MTU, this unconditionally
widens GDM_LONG_LEN_MASK for the already-running sibling that has a
smaller MTU:
if (!atomic_read(&port->users) || len > cur_len)
airoha_fe_rmw(qdma->eth, REG_GDM_LEN_CFG(port->id),
GDM_SHORT_LEN_MASK | GDM_LONG_LEN_MASK,
FIELD_PREP(GDM_SHORT_LEN_MASK, 60) |
FIELD_PREP(GDM_LONG_LEN_MASK, len));
Meanwhile airoha_dev_change_mtu() rejects shrinks (len < cur_len) with
-EBUSY when the port is shared. Is this asymmetry intentional?
With the new policy the hardware long-length ends up as the maximum of
the two MTUs, so the smaller-MTU sibling silently accepts frames larger
than its advertised MTU at the MAC level. Should opens that would
increase the shared length also be gated (symmetric policy), or should
the commit message and a comment document the chosen ceiling/floor
behaviour?
>
> airoha_qdma_set(qdma, REG_QDMA_GLOBAL_CFG,
> GLOBAL_CFG_TX_DMA_EN_MASK |
> @@ -1762,8 +1766,12 @@ static int airoha_dev_stop(struct net_device *netdev)
> for (i = 0; i < netdev->num_tx_queues; i++)
> netdev_tx_reset_subqueue(netdev, i);
>
> - airoha_set_gdm_port_fwd_cfg(qdma->eth, REG_GDM_FWD_CFG(port->id),
> - FE_PSE_PORT_DROP);
> + if (atomic_dec_and_test(&port->users)) {
> + airoha_set_vip_for_gdm_port(dev, false);
> + airoha_set_gdm_port_fwd_cfg(qdma->eth,
> + REG_GDM_FWD_CFG(port->id),
> + FE_PSE_PORT_DROP);
> + }
[Low]
Is the airoha_set_vip_for_gdm_port(dev, false) call inside the
atomic_dec_and_test() block intentional?
Looking at airoha_dev_stop() as a whole, the same call is already made
unconditionally a few lines above:
netif_tx_disable(netdev);
airoha_set_vip_for_gdm_port(dev, false);
for (i = 0; i < netdev->num_tx_queues; i++)
netdev_tx_reset_subqueue(netdev, i);
if (atomic_dec_and_test(&port->users)) {
airoha_set_vip_for_gdm_port(dev, false);
...
}
Since get_vip_port() derives the bit mask per-(port, dev->nbq) and
airoha_fe_clear() of already-cleared bits is a no-op, the second call
looks like dead code today. The commit log only mentions the
FE_PSE_PORT_DROP gating, not VIP handling. Should the duplicate be
dropped in v2?
>
> if (atomic_dec_and_test(&qdma->users)) {
> airoha_qdma_clear(qdma, REG_QDMA_GLOBAL_CFG,
> @@ -1915,10 +1923,22 @@ static void airoha_dev_get_stats64(struct net_device *netdev,
> static int airoha_dev_change_mtu(struct net_device *netdev, int mtu)
> {
> struct airoha_gdm_dev *dev = netdev_priv(netdev);
> + u32 cur_len, len = ETH_HLEN + mtu + ETH_FCS_LEN;
> struct airoha_gdm_port *port = dev->port;
> - u32 len = ETH_HLEN + mtu + ETH_FCS_LEN;
> struct airoha_eth *eth = dev->eth;
>
> + cur_len = airoha_fe_get(eth, REG_GDM_LEN_CFG(port->id),
> + GDM_LONG_LEN_MASK);
> + if (len < cur_len) {
> + u8 port_refcnt = atomic_read(&port->users);
> +
> + /* We can decrease the device MTU just if the GDM port is
> + * not shared or if the other device is not running.
> + */
> + if (port_refcnt > 1 || (port_refcnt && !netif_running(netdev)))
> + return -EBUSY;
> + }
> +
[Low]
Why is port_refcnt typed as u8 here when atomic_read() returns int?
u8 port_refcnt = atomic_read(&port->users);
With AIROHA_MAX_NUM_GDM_DEVS currently 2 this cannot overflow, but a
future bump of AIROHA_MAX_NUM_GDM_DEVS, or a refcount leak, that pushed
port->users to 256 would wrap to 0 and quietly bypass both the
port_refcnt > 1 and port_refcnt && !netif_running(netdev) checks,
permitting MTU shrinks that are meant to be rejected. Would using int
here (the type atomic_read() returns) be preferable?
> airoha_fe_rmw(eth, REG_GDM_LEN_CFG(port->id),
> GDM_LONG_LEN_MASK,
> FIELD_PREP(GDM_LONG_LEN_MASK, len));
^ permalink raw reply
* Re: [PATCH net-next v4 05/10] net: airoha: Move qos_sq_bmap in airoha_qdma struct
From: Jakub Kicinski @ 2026-05-09 1:46 UTC (permalink / raw)
To: lorenzo
Cc: Jakub Kicinski, andrew+netdev, davem, edumazet, pabeni, robh,
krzk+dt, conor+dt, ansuelsmth, benjamin.larsson, linux-arm-kernel,
linux-mediatek, netdev, devicetree, madhur.agrawal
In-Reply-To: <20260507-airoha-eth-multi-serdes-v4-5-af613b61ae02@kernel.org>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: airoha: Move qos_sq_bmap in airoha_qdma struct
The change moves the HTB-offload channel-allocation bitmap from the
per-netdev airoha_gdm_port to the per-QDMA airoha_qdma, so sibling
netdevs attached to different QDMA blocks no longer clobber each other's
bitmap.
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 080705e2f58d..69a4c2e0d58b 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -2600,7 +2600,7 @@ static int airoha_tc_htb_alloc_leaf_queue(struct net_device *netdev,
> u32 rate = div_u64(opt->rate, 1000) << 3; /* kbps */
> int err, num_tx_queues = netdev->real_num_tx_queues;
> struct airoha_gdm_dev *dev = netdev_priv(netdev);
> - struct airoha_gdm_port *port = dev->port;
> + struct airoha_qdma *qdma = dev->qdma;
>
> if (opt->parent_classid != TC_HTB_CLASSID_ROOT) {
> NL_SET_ERR_MSG_MOD(opt->extack, "invalid parent classid");
> @@ -2627,7 +2627,7 @@ static int airoha_tc_htb_alloc_leaf_queue(struct net_device *netdev,
> return err;
> }
>
> - set_bit(channel, port->qos_sq_bmap);
> + set_bit(channel, qdma->qos_sq_bmap);
> opt->qid = AIROHA_NUM_TX_RING + channel;
>
> return 0;
[High]
Now that qos_sq_bmap is per-QDMA, can two sibling netdevs that share the
same QDMA block collide on allocation? On EN7581, GDM2/GDM3/GDM4 all
select the same qdma pointer via:
drivers/net/ethernet/airoha/airoha_eth.c:
/* QDMA0 is used for lan ports while QDMA1 is used for WAN ports */
dev->qdma = ð->qdma[!airoha_is_lan_gdm_dev(dev)];
and airoha_is_lan_gdm_dev() returns true only for port->id == 1, so
every non-LAN GDM netdev resolves to eth->qdma[1] and shares
qdma->qos_sq_bmap.
The allocation path does not test the bit before setting it:
channel = TC_H_MIN(opt->classid) % AIROHA_NUM_QOS_CHANNELS;
...
set_bit(channel, qdma->qos_sq_bmap);
opt->qid = AIROHA_NUM_TX_RING + channel;
If two sibling netdevs on the same QDMA pick classids whose TC_H_MIN
maps to the same channel, both airoha_qdma_set_tx_rate_limit() calls
program the same HW TRTCM egress slot, the later caller silently
overwrites the earlier configuration, and both netdevs receive the same
opt->qid. Should this path use test_and_set_bit() (or similar owner
tracking) to reject a channel already owned by a sibling netdev?
> @@ -2811,11 +2811,11 @@ static int airoha_dev_setup_tc_block(struct net_device *dev,
> static void airoha_tc_remove_htb_queue(struct net_device *netdev, int queue)
> {
> struct airoha_gdm_dev *dev = netdev_priv(netdev);
> - struct airoha_gdm_port *port = dev->port;
> + struct airoha_qdma *qdma = dev->qdma;
>
> netif_set_real_num_tx_queues(netdev, netdev->real_num_tx_queues - 1);
> airoha_qdma_set_tx_rate_limit(netdev, queue + 1, 0, 0);
> - clear_bit(queue, port->qos_sq_bmap);
> + clear_bit(queue, qdma->qos_sq_bmap);
> }
[High]
airoha_tc_remove_htb_queue() operates unconditionally on the caller's
netdev but clears a bit that may be owned by a sibling netdev on the
same QDMA. The effects that follow look like cross-netdev interference:
- netif_set_real_num_tx_queues(netdev, real_num_tx_queues - 1)
decrements the caller's tx-queue count for a queue it never
allocated, and its return value is discarded (eventually the txq<1
EINVAL guard in netif_set_real_num_tx_queues() will trip silently).
- airoha_qdma_set_tx_rate_limit(netdev, queue + 1, 0, 0) zeros the
egress TRTCM rate-limit registers for a channel that the sibling
netdev still considers active.
- clear_bit(queue, qdma->qos_sq_bmap) then hides the channel from the
legitimate owner's later test_bit() checks.
> @@ -2838,10 +2838,10 @@ static int airoha_tc_htb_delete_leaf_queue(struct net_device *netdev,
> static int airoha_tc_htb_destroy(struct net_device *netdev)
> {
> struct airoha_gdm_dev *dev = netdev_priv(netdev);
> - struct airoha_gdm_port *port = dev->port;
> + struct airoha_qdma *qdma = dev->qdma;
> int q;
>
> - for_each_set_bit(q, port->qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS)
> + for_each_set_bit(q, qdma->qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS)
> airoha_tc_remove_htb_queue(netdev, q);
>
> return 0;
> }
[High]
Does airoha_tc_htb_destroy() now walk bits owned by sibling netdevs?
The iteration no longer filters by owning netdev:
for_each_set_bit(q, qdma->qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS)
airoha_tc_remove_htb_queue(netdev, q);
Previously each netdev had a private bitmap, so destroy could only
touch its own channels. After this change, tearing down HTB on one
GDM netdev appears to remove channels and clear rate-limit HW state
belonging to every sibling GDM netdev on the same QDMA. A subsequent
tc-htb delete or get on the legitimate owner then hits:
if (!test_bit(channel, qdma->qos_sq_bmap)) {
NL_SET_ERR_MSG_MOD(opt->extack, "invalid queue id");
return -EINVAL;
}
Should the bitmap track the owning netdev alongside the bit (for
example a per-channel owner pointer, or a per-netdev sub-bitmap inside
airoha_qdma), so that destroy/delete can only touch channels owned by
the caller?
> @@ -2852,9 +2852,9 @@ static int airoha_tc_get_htb_get_leaf_queue(struct net_device *netdev,
> {
> u32 channel = TC_H_MIN(opt->classid) % AIROHA_NUM_QOS_CHANNELS;
> struct airoha_gdm_dev *dev = netdev_priv(netdev);
> - struct airoha_gdm_port *port = dev->port;
> + struct airoha_qdma *qdma = dev->qdma;
>
> - if (!test_bit(channel, port->qos_sq_bmap)) {
> + if (!test_bit(channel, qdma->qos_sq_bmap)) {
> NL_SET_ERR_MSG_MOD(opt->extack, "invalid queue id");
> return -EINVAL;
> }
^ permalink raw reply
* Re: [PATCH net-next v4 03/10] net: airoha: Move airoha_qdma pointer in airoha_gdm_dev struct
From: Jakub Kicinski @ 2026-05-09 1:46 UTC (permalink / raw)
To: lorenzo
Cc: Jakub Kicinski, andrew+netdev, davem, edumazet, pabeni, robh,
krzk+dt, conor+dt, ansuelsmth, benjamin.larsson, linux-arm-kernel,
linux-mediatek, netdev, devicetree, madhur.agrawal
In-Reply-To: <20260507-airoha-eth-multi-serdes-v4-3-af613b61ae02@kernel.org>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: airoha: Move airoha_qdma pointer in airoha_gdm_dev struct
This patch moves the airoha_qdma pointer from struct airoha_gdm_port into
struct airoha_gdm_dev, as a preparation for allowing multiple net_devices
to share the same GDM{3,4} port via an external hw arbiter.
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 4af64f182968..1798b4a6cf5d 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
[ ... ]
> @@ -854,7 +855,8 @@ static void airoha_qdma_wake_netdev_txqs(struct airoha_queue *q)
> if (!port)
> continue;
>
> - if (port->qdma != qdma)
> + dev = port->dev;
> + if (dev->qdma != qdma)
> continue;
>
> dev = port->dev;
[Low]
The resulting loop body assigns dev = port->dev; twice with nothing in
between that could change port->dev:
dev = port->dev;
if (dev->qdma != qdma)
continue;
dev = port->dev;
for (j = 0; j < dev->dev->num_tx_queues; j++) {
Is the second dev = port->dev; intentional, or leftover from the
refactor that should be removed?
[Medium]
Can the new dev->qdma dereference run while port->dev is still NULL?
Looking at airoha_alloc_gdm_port(), eth->ports[p] = port is stored before
airoha_alloc_gdm_device() runs, and airoha_alloc_gdm_device() only sets
port->dev = dev at its very end, after several failure points
(devm_alloc_etherdev_mqs, netif_set_real_num_tx_queues,
of_get_ethdev_address).
The existing cleanup in airoha_probe's error_napi_stop path and in
airoha_remove guards accesses with if (dev && ...) when reading
port->dev, which suggests port->dev can legitimately be NULL while
eth->ports[i] is non-NULL.
Before the patch the equivalent check read port->qdma (an embedded field
zeroed by devm_kzalloc), so the compare was NULL-tolerant. After the
patch:
struct airoha_gdm_port *port = eth->ports[i];
struct airoha_gdm_dev *dev;
...
if (!port)
continue;
dev = port->dev;
if (dev->qdma != qdma)
continue;
would dereference a NULL dev if port->dev has not yet been assigned when
airoha_qdma_wake_netdev_txqs() runs. Would a NULL check on port->dev (or
reverting to the original port-field-based test) be appropriate here to
preserve the prior robustness?
^ permalink raw reply
* Re: [net-next v7 3/3] net: ethernet: mtk_eth_soc: Add LRO support
From: Jakub Kicinski @ 2026-05-08 22:37 UTC (permalink / raw)
To: Frank Wunderlich
Cc: Mason Chang, AngeloGioacchino Del Regno, netdev, Russell King,
linux-kernel, Andrew Lunn, Eric Dumazet, linux-mediatek,
Daniel Golle, Matthias Brugger, Paolo Abeni, Lorenzo Bianconi,
David S. Miller, linux-arm-kernel, Felix Fietkau
In-Reply-To: <20260506192806.143725-4-linux@fw-web.de>
On Wed, 6 May 2026 21:28:03 +0200 Frank Wunderlich wrote:
> - fix u32 vs. be32 reported by patchwork check
Have you tested this? I still see an extra sparse warning
(install sparse and build with make C=1)
It'd probably be good to fix the pre-exisiting warnings as the first
patch in the series while at it.
--
pw-bot: cr
^ permalink raw reply
* [PATCH] ASoC; dt-bindings: mediatek,mt8173-rt5650-rt5514: Fix mediatek,audio-codec constraints
From: Rob Herring (Arm) @ 2026-05-08 18:24 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Koro Chen,
Khushal Chitturi
Cc: Krzysztof Kozlowski, linux-sound, devicetree, linux-kernel,
linux-arm-kernel, linux-mediatek
A phandle-array is really a matrix and needs constraints on the number
of elements for both the inner and outer dimensions. Add the missing
inner constraints.
Fixes: 472d77bdc511 ("ASoC: dt-bindings: mediatek,mt8173-rt5650-rt5514: convert to DT schema")
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
.../bindings/sound/mediatek,mt8173-rt5650-rt5514.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8173-rt5650-rt5514.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8173-rt5650-rt5514.yaml
index ed698c9ff42b..becc7a11f8dc 100644
--- a/Documentation/devicetree/bindings/sound/mediatek,mt8173-rt5650-rt5514.yaml
+++ b/Documentation/devicetree/bindings/sound/mediatek,mt8173-rt5650-rt5514.yaml
@@ -18,7 +18,9 @@ properties:
description: Phandles of rt5650 and rt5514 codecs
items:
- description: phandle of rt5650 codec
+ maxItems: 1
- description: phandle of rt5514 codec
+ maxItems: 1
mediatek,platform:
$ref: /schemas/types.yaml#/definitions/phandle
--
2.53.0
^ permalink raw reply related
* Re: [PATCH] dt-bindings: PCI: mediatek-gen3: Allow memory-region for restricted DMA buffer
From: Bjorn Helgaas @ 2026-05-08 17:54 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Matthias Brugger, AngeloGioacchino Del Regno, Ryder Lee,
Lorenzo Pieralisi, Krzysztof Wilczyński,
Manivannan Sadhasivam, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, devicetree, linux-pci,
linux-mediatek, linux-kernel
In-Reply-To: <20260508063633.3894348-1-wenst@chromium.org>
On Fri, May 08, 2026 at 02:36:32PM +0800, Chen-Yu Tsai wrote:
> On some SoCs without an IOMMU behind the PCIe controller, the PCIe
> controller memory access could be limited to a small region by the
> firmware configuring a memory protection unit. This memory region
> must be assigned to the PCIe controller so that the OS knows to
> use that region. Otherwise PCIe devices would not work properly.
>
> Allow the memory-region property with one item pointing to a
> restricted DMA buffer.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> This patch compliments another patch that moved the memory-region from
> the PCIe device to the PCIe controller [1].
>
> [1] https://lore.kernel.org/all/20260430120725.241779-1-wenst@chromium.org/
>
> Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> index 4db700fc36ba..4a9e41d01628 100644
> --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> @@ -115,6 +115,10 @@ properties:
> power-domains:
> maxItems: 1
>
> + memory-region:
> + maxItems: 1
> + description: phandle to restricted DMA buffer
I guess this is similar to
https://lore.kernel.org/linux-pci/20250716053950.199079-1-huaqian.li@siemens.com/
and uses
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/reserved-memory/shared-dma-pool.yaml?
Looks like those keystone changes were never merged; I don't know what
happened to them. But it will be good if everybody does it the same
way.
I wish there were a simple way to grep for this restricted DMA
concept. Maybe there is and I just haven't found it :)
> mediatek,pbus-csr:
> $ref: /schemas/types.yaml#/definitions/phandle-array
> items:
> --
> 2.54.0.563.g4f69b47b94-goog
>
^ permalink raw reply
* Re: [PATCH 2/2] drm/mediatek: hdmi: report jack plugged state from bridge enable/disable
From: Daniel Golle @ 2026-05-08 12:19 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Chun-Kuang Hu, Philipp Zabel, David Airlie, Simona Vetter,
Matthias Brugger, AngeloGioacchino Del Regno, Junzhi Zhao,
Jie Qiu, dri-devel, linux-mediatek, linux-kernel,
linux-arm-kernel
In-Reply-To: <mv5qeardy5xy5657hb32ewkwg3gmgll3kow4xuhiklswnmj2sc@mhncl5reoqlc>
On Fri, May 08, 2026 at 03:14:34PM +0300, Dmitry Baryshkov wrote:
> On Fri, May 08, 2026 at 12:44:23PM +0100, Daniel Golle wrote:
> > On Fri, May 08, 2026 at 02:29:29PM +0300, Dmitry Baryshkov wrote:
> > > On Thu, May 07, 2026 at 02:32:45PM +0100, Daniel Golle wrote:
> > > > On Thu, May 07, 2026 at 12:51:56PM +0300, Dmitry Baryshkov wrote:
> > > > > On Wed, Apr 15, 2026 at 04:04:16PM +0100, Daniel Golle wrote:
> > > > > > Notify hdmi-codec of the current sink plugged state from
> > > > > > mtk_hdmi_bridge_atomic_enable() and mtk_hdmi_bridge_atomic_disable()
> > > > > > via mtk_hdmi_update_plugged_status(). This matches the pattern used
> > > > > > by dw-hdmi, which invokes handle_plugged_change() from the bridge
> > > > > > enable and disable paths so that ASoC jack state stays in sync with
> > > > > > the actual sink presence across atomic commit cycles, and not only
> > > > > > on CEC HPD transitions.
> > > > > >
> > > > > > Userspace audio daemons (e.g. pipewire) rely on the jack state to
> > > > > > route streams, restore per-sink volume levels, and recover the last
> > > > > > used device after a reconnect. Without this, those transitions are
> > > > > > missed whenever the sink change is driven by a mode set rather than
> > > > > > by a bare HPD event.
> > > > >
> > > > > I can only hope to see mtk_hdmi to migrate to DRM_BRIDGE_OP_HDMI and
> > > > > DRM_BRIDGE_OP_HDMI_AUDIO...
> > > > >
> > > > > I think the correct timing was discussed several times and the overall
> > > > > conclusion was that the correct time is when the actual HDMI cable is
> > > > > being plugged / unplugged. See the discussion around [1] and the
> > > > > captured response of Mark Brown.
> > > >
> > > > Thank you for bringing this to my attention. I'll follow up on it.
> > > > Meanwhile, can (independent) patch 1/2 already be merged, or at least
> > > > get reviewed?
> > >
> > >
> > > The discussion that I pointed to suggests that the patch is incorrect.
> >
> > I understand that patch 2/2 is incorrect, but (the independent fix in)
> > patch 1/2 as well?
>
> 1/2 moves plugged updates to the atomic_enable() / atomic_disable(),
> which is not correct according to the discussion I pointed to.
So enabling/disabling the clk there as suggested by CK Hu[1] is not
acceptable?
[1]: https://lore.kernel.org/all/effea6b19a05460371c9f6b639c5e08ab0fe1111.camel@mediatek.com/
^ permalink raw reply
* [PATCH v2 0/1] dt-bindings: remoteproc: mtk,scp: Allow multiple memory regions for MT8188
From: Arnab Layek @ 2026-05-08 12:15 UTC (permalink / raw)
To: Bjorn Andersson, Mathieu Poirier, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno
Cc: linux-remoteproc, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, Project_Global_Chrome_Upstream_Group, Arnab Layek
This patch updates the MediaTek SCP device tree binding to support
multiple memory regions for MT8188 SoCs, addressing review feedback
from v1.
The MT8188 SCP requires two memory regions (SRAM + L1TCM) while other
MediaTek SoCs use only one. This patch follows the proper pattern of
using conditional schemas with explicit descriptions.
Changes in v2:
- Use conditional schema (if/then in allOf) to restrict multiple
memory regions only to MT8188 variants, keeping maxItems: 1 as
default for other SoCs (addresses Krzysztof's review)
- Add explicit item descriptions for each memory region (Main SRAM
and optional L1TCM) instead of just setting minItems/maxItems
- Apply to both top-level and patternProperties
Based on linux-next, tag: next-20260505
Arnab Layek (1):
dt-bindings: remoteproc: mtk,scp: Allow multiple memory regions for
MT8188
.../bindings/remoteproc/mtk,scp.yaml | 21 +++++++++++++++++++
1 file changed, 21 insertions(+)
--
2.45.2
^ permalink raw reply
* Re: [PATCH 2/2] drm/mediatek: hdmi: report jack plugged state from bridge enable/disable
From: Dmitry Baryshkov @ 2026-05-08 12:14 UTC (permalink / raw)
To: Daniel Golle
Cc: Chun-Kuang Hu, Philipp Zabel, David Airlie, Simona Vetter,
Matthias Brugger, AngeloGioacchino Del Regno, Junzhi Zhao,
Jie Qiu, dri-devel, linux-mediatek, linux-kernel,
linux-arm-kernel
In-Reply-To: <af3Ml252DaeyXzwe@makrotopia.org>
On Fri, May 08, 2026 at 12:44:23PM +0100, Daniel Golle wrote:
> On Fri, May 08, 2026 at 02:29:29PM +0300, Dmitry Baryshkov wrote:
> > On Thu, May 07, 2026 at 02:32:45PM +0100, Daniel Golle wrote:
> > > On Thu, May 07, 2026 at 12:51:56PM +0300, Dmitry Baryshkov wrote:
> > > > On Wed, Apr 15, 2026 at 04:04:16PM +0100, Daniel Golle wrote:
> > > > > Notify hdmi-codec of the current sink plugged state from
> > > > > mtk_hdmi_bridge_atomic_enable() and mtk_hdmi_bridge_atomic_disable()
> > > > > via mtk_hdmi_update_plugged_status(). This matches the pattern used
> > > > > by dw-hdmi, which invokes handle_plugged_change() from the bridge
> > > > > enable and disable paths so that ASoC jack state stays in sync with
> > > > > the actual sink presence across atomic commit cycles, and not only
> > > > > on CEC HPD transitions.
> > > > >
> > > > > Userspace audio daemons (e.g. pipewire) rely on the jack state to
> > > > > route streams, restore per-sink volume levels, and recover the last
> > > > > used device after a reconnect. Without this, those transitions are
> > > > > missed whenever the sink change is driven by a mode set rather than
> > > > > by a bare HPD event.
> > > >
> > > > I can only hope to see mtk_hdmi to migrate to DRM_BRIDGE_OP_HDMI and
> > > > DRM_BRIDGE_OP_HDMI_AUDIO...
> > > >
> > > > I think the correct timing was discussed several times and the overall
> > > > conclusion was that the correct time is when the actual HDMI cable is
> > > > being plugged / unplugged. See the discussion around [1] and the
> > > > captured response of Mark Brown.
> > >
> > > Thank you for bringing this to my attention. I'll follow up on it.
> > > Meanwhile, can (independent) patch 1/2 already be merged, or at least
> > > get reviewed?
> >
> >
> > The discussion that I pointed to suggests that the patch is incorrect.
>
> I understand that patch 2/2 is incorrect, but (the independent fix in)
> patch 1/2 as well?
1/2 moves plugged updates to the atomic_enable() / atomic_disable(),
which is not correct according to the discussion I pointed to.
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] drm/mediatek: hdmi: report jack plugged state from bridge enable/disable
From: Dmitry Baryshkov @ 2026-05-08 11:29 UTC (permalink / raw)
To: Daniel Golle
Cc: Chun-Kuang Hu, Philipp Zabel, David Airlie, Simona Vetter,
Matthias Brugger, AngeloGioacchino Del Regno, Junzhi Zhao,
Jie Qiu, dri-devel, linux-mediatek, linux-kernel,
linux-arm-kernel
In-Reply-To: <afyUfcr8HuFs6EKF@makrotopia.org>
On Thu, May 07, 2026 at 02:32:45PM +0100, Daniel Golle wrote:
> On Thu, May 07, 2026 at 12:51:56PM +0300, Dmitry Baryshkov wrote:
> > On Wed, Apr 15, 2026 at 04:04:16PM +0100, Daniel Golle wrote:
> > > Notify hdmi-codec of the current sink plugged state from
> > > mtk_hdmi_bridge_atomic_enable() and mtk_hdmi_bridge_atomic_disable()
> > > via mtk_hdmi_update_plugged_status(). This matches the pattern used
> > > by dw-hdmi, which invokes handle_plugged_change() from the bridge
> > > enable and disable paths so that ASoC jack state stays in sync with
> > > the actual sink presence across atomic commit cycles, and not only
> > > on CEC HPD transitions.
> > >
> > > Userspace audio daemons (e.g. pipewire) rely on the jack state to
> > > route streams, restore per-sink volume levels, and recover the last
> > > used device after a reconnect. Without this, those transitions are
> > > missed whenever the sink change is driven by a mode set rather than
> > > by a bare HPD event.
> >
> > I can only hope to see mtk_hdmi to migrate to DRM_BRIDGE_OP_HDMI and
> > DRM_BRIDGE_OP_HDMI_AUDIO...
> >
> > I think the correct timing was discussed several times and the overall
> > conclusion was that the correct time is when the actual HDMI cable is
> > being plugged / unplugged. See the discussion around [1] and the
> > captured response of Mark Brown.
>
> Thank you for bringing this to my attention. I'll follow up on it.
> Meanwhile, can (independent) patch 1/2 already be merged, or at least
> get reviewed?
The discussion that I pointed to suggests that the patch is incorrect.
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] drm/mediatek: hdmi: report jack plugged state from bridge enable/disable
From: Daniel Golle @ 2026-05-08 11:44 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Chun-Kuang Hu, Philipp Zabel, David Airlie, Simona Vetter,
Matthias Brugger, AngeloGioacchino Del Regno, Junzhi Zhao,
Jie Qiu, dri-devel, linux-mediatek, linux-kernel,
linux-arm-kernel
In-Reply-To: <v343uqdgy65yg3lm7rcv57mlpkudqxrnori6lnunaiiewakhyd@wnxkqhhwwvgg>
On Fri, May 08, 2026 at 02:29:29PM +0300, Dmitry Baryshkov wrote:
> On Thu, May 07, 2026 at 02:32:45PM +0100, Daniel Golle wrote:
> > On Thu, May 07, 2026 at 12:51:56PM +0300, Dmitry Baryshkov wrote:
> > > On Wed, Apr 15, 2026 at 04:04:16PM +0100, Daniel Golle wrote:
> > > > Notify hdmi-codec of the current sink plugged state from
> > > > mtk_hdmi_bridge_atomic_enable() and mtk_hdmi_bridge_atomic_disable()
> > > > via mtk_hdmi_update_plugged_status(). This matches the pattern used
> > > > by dw-hdmi, which invokes handle_plugged_change() from the bridge
> > > > enable and disable paths so that ASoC jack state stays in sync with
> > > > the actual sink presence across atomic commit cycles, and not only
> > > > on CEC HPD transitions.
> > > >
> > > > Userspace audio daemons (e.g. pipewire) rely on the jack state to
> > > > route streams, restore per-sink volume levels, and recover the last
> > > > used device after a reconnect. Without this, those transitions are
> > > > missed whenever the sink change is driven by a mode set rather than
> > > > by a bare HPD event.
> > >
> > > I can only hope to see mtk_hdmi to migrate to DRM_BRIDGE_OP_HDMI and
> > > DRM_BRIDGE_OP_HDMI_AUDIO...
> > >
> > > I think the correct timing was discussed several times and the overall
> > > conclusion was that the correct time is when the actual HDMI cable is
> > > being plugged / unplugged. See the discussion around [1] and the
> > > captured response of Mark Brown.
> >
> > Thank you for bringing this to my attention. I'll follow up on it.
> > Meanwhile, can (independent) patch 1/2 already be merged, or at least
> > get reviewed?
>
>
> The discussion that I pointed to suggests that the patch is incorrect.
I understand that patch 2/2 is incorrect, but (the independent fix in)
patch 1/2 as well?
^ permalink raw reply
* Re: [PATCH v2] drm/mediatek: hdmi: Convert DRM_ERROR() to drm_err()
From: CK Hu (胡俊光) @ 2026-05-08 8:25 UTC (permalink / raw)
To: chunkuang.hu@kernel.org, suryasaimadhu369@gmail.com
Cc: p.zabel@pengutronix.de, dri-devel@lists.freedesktop.org,
linux-mediatek@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20260420064544.266478-1-suryasaimadhu369@gmail.com>
On Mon, 2026-04-20 at 06:45 +0000, sai madhu wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
>
>
> The DRM_ERROR() macro is deprecated in favor of drm_err() which
> provides device-specific logging.
>
> Replace DRM_ERROR() with drm_err() in the Mediatek HDMI bridge
> driver and pass the drm_device pointer via bridge->dev.
>
> No functional change intended.
Reviewed-by: CK Hu <ck.hu@mediatek.com>
>
> Signed-off-by: sai madhu <suryasaimadhu369@gmail.com>
> ---
> drivers/gpu/drm/mediatek/mtk_hdmi.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> index 1ea259854780..4ddcdbf7bc8c 100644
> --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> @@ -981,8 +981,8 @@ static int mtk_hdmi_bridge_attach(struct drm_bridge *bridge,
> int ret;
>
> if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> - DRM_ERROR("%s: The flag DRM_BRIDGE_ATTACH_NO_CONNECTOR must be supplied\n",
> - __func__);
> + drm_err(bridge->dev,
> + "DRM_BRIDGE_ATTACH_NO_CONNECTOR must be supplied\n");
> return -EINVAL;
> }
>
> --
> 2.34.1
>
^ permalink raw reply
* Re: [PATCH 1/2] drm/mediatek: hdmi: pulse audio clocks on bridge enable
From: CK Hu (胡俊光) @ 2026-05-08 7:19 UTC (permalink / raw)
To: dri-devel@lists.freedesktop.org, chunkuang.hu@kernel.org,
simona@ffwll.ch, AngeloGioacchino Del Regno,
junzhi.zhao@mediatek.com, airlied@gmail.com,
daniel@makrotopia.org, linux-arm-kernel@lists.infradead.org,
p.zabel@pengutronix.de, matthias.bgg@gmail.com,
linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
jie.qiu@mediatek.com
In-Reply-To: <a3e22cbae528c9a38d854a586d1736b860998d41.1776265222.git.daniel@makrotopia.org>
On Wed, 2026-04-15 at 16:04 +0100, Daniel Golle wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
>
>
> The CLK_MM_HDMI_AUDIO and CLK_MM_HDMI_SPDIF mmsys gates feed the
> HDMI TX internal audio measurement block which derives CTS values
> for the ACR packets embedded in the TMDS stream. These clocks are
> enabled once at driver probe time and then left untouched across
> bridge atomic_enable / atomic_disable cycles.
>
> On every observed stale-state event -- a blank/unblank cycle, or
> a cold boot with the monitor off followed by a later hotplug --
> the measurement block remains armed against the previous state
> and fails to latch a valid CTS on the subsequent bridge enable.
> Video recovers cleanly but the audio data islands never regain
> lock and the HDMI sink sees no audio, even though the ASoC stack,
> the AFE, and the HDMI TX audio packetizer are all programmed
> correctly.
>
> Debugging the issue of audio no longer working after vblank it was
> found that an unbind+bind of the mediatek-drm-hdmi platform driver
> recovers audio in all such scenarios without disturbing video.
> The only audio-relevant side effect of that rebind is an off->on edge
> on CLK_MM_HDMI_AUDIO / CLK_MM_HDMI_SPDIF via the probe path. Pulsing
> those two clocks directly at the end of mtk_hdmi_bridge_atomic_enable
> reproduces that recovery on every enable and doesn't hurt on the
> first enable after boot.
>
> Fixes: 8f83f26891e1 ("drm/mediatek: Add HDMI support")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
> drivers/gpu/drm/mediatek/mtk_hdmi.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> index 1ea2598547800..9050d7785f109 100644
> --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> @@ -1065,6 +1065,22 @@ static void mtk_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
> phy_power_on(hdmi->phy);
> mtk_hdmi_send_infoframe(hdmi, &hdmi->mode);
>
> + /*
> + * Pulse the HDMI TX audio clocks off/on on every bridge enable.
> + * The CLK_MM_HDMI_AUDIO and CLK_MM_HDMI_SPDIF mmsys gates feed
> + * the HDMI TX internal audio measurement block that derives CTS
> + * for the ACR packets embedded in the TMDS stream. Without an
> + * off->on edge at bridge enable the block can stay armed against
> + * stale state from a previous enable (e.g. after blank/unblank,
> + * or after a monitor that was off at boot is plugged in later)
> + * and fails to latch a valid CTS, leaving the audio path silent
> + * even though video recovers. The pulse is what an unbind+bind
> + * of the HDMI platform driver effectively does, and it recovers
> + * audio in all observed stale-state scenarios.
> + */
> + mtk_hdmi_clk_disable_audio(hdmi);
> + mtk_hdmi_clk_enable_audio(hdmi);
Why not align audio clock on/off timing to video clock timing.
I mean, do not turn on audio clock when probe and turn on it in atomic enable and turn off it in atomic disable.
I think it's reasonable to align audio clock and video clock on/off timing.
Regards,
CK
> +
> hdmi->enabled = true;
> }
>
> --
> 2.53.0
>
^ permalink raw reply
* Re: [PATCH] drm/mediatek: mtk_dpi: Open-code drm_simple_encoder_init()
From: CK Hu (胡俊光) @ 2026-05-08 6:54 UTC (permalink / raw)
To: p.zabel@pengutronix.de, chunkuang.hu@kernel.org,
AngeloGioacchino Del Regno, airlied@gmail.com,
matthias.bgg@gmail.com, shivamkalra98@zohomail.in,
simona@ffwll.ch
Cc: dri-devel@lists.freedesktop.org,
linux-mediatek@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, tzimmermann@suse.de
In-Reply-To: <20260403-drm-mediatek-opencode-encoder-init-v1-1-7be86241b876@zohomail.in>
On Fri, 2026-04-03 at 17:30 +0530, Shivam Kalra via B4 Relay wrote:
> From: Shivam Kalra <shivamkalra98@zohomail.in>
>
> The helper drm_simple_encoder_init() is a trivial wrapper around
> drm_encoder_init() that only provides a static drm_encoder_funcs with
> .destroy set to drm_encoder_cleanup(). Open-code the initialization
> with a driver-specific instance of drm_encoder_funcs and remove the
> dependency on drm_simple_kms_helper.
Reviewed-by: CK Hu <ck.hu@mediatek.com>
>
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Shivam Kalra <shivamkalra98@zohomail.in>
> ---
> Addresses the "Open-code drm_simple_encoder_init()" task from
> Documentation/gpu/todo.rst.
> ---
> drivers/gpu/drm/mediatek/mtk_dpi.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
> index 53360b5d12ba..5b83ca6aecb2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> @@ -25,8 +25,8 @@
> #include <drm/drm_bridge_connector.h>
> #include <drm/drm_crtc.h>
> #include <drm/drm_edid.h>
> +#include <drm/drm_encoder.h>
> #include <drm/drm_of.h>
> -#include <drm/drm_simple_kms_helper.h>
>
> #include "mtk_ddp_comp.h"
> #include "mtk_disp_drv.h"
> @@ -993,6 +993,10 @@ static const struct drm_bridge_funcs mtk_dpi_bridge_funcs = {
> .debugfs_init = mtk_dpi_debugfs_init,
> };
>
> +static const struct drm_encoder_funcs mtk_dpi_encoder_funcs = {
> + .destroy = drm_encoder_cleanup,
> +};
> +
> void mtk_dpi_start(struct device *dev)
> {
> struct mtk_dpi *dpi = dev_get_drvdata(dev);
> @@ -1026,8 +1030,8 @@ static int mtk_dpi_bind(struct device *dev, struct device *master, void *data)
> int ret;
>
> dpi->mmsys_dev = priv->mmsys_dev;
> - ret = drm_simple_encoder_init(drm_dev, &dpi->encoder,
> - DRM_MODE_ENCODER_TMDS);
> + ret = drm_encoder_init(drm_dev, &dpi->encoder, &mtk_dpi_encoder_funcs,
> + DRM_MODE_ENCODER_TMDS, NULL);
> if (ret) {
> dev_err(dev, "Failed to initialize decoder: %d\n", ret);
> return ret;
>
> ---
> base-commit: 4b9c36c83b34f710da9573291404f6a2246251c1
> change-id: 20260403-drm-mediatek-opencode-encoder-init-36336e4c4ff3
>
> Best regards,
> --
> Shivam Kalra <shivamkalra98@zohomail.in>
>
>
^ permalink raw reply
* Re: [PATCH] drm/mediatek: simplify mtk_crtc allocation
From: CK Hu (胡俊光) @ 2026-05-08 6:43 UTC (permalink / raw)
To: dri-devel@lists.freedesktop.org, rosenp@gmail.com
Cc: chunkuang.hu@kernel.org, simona@ffwll.ch,
AngeloGioacchino Del Regno, airlied@gmail.com,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, p.zabel@pengutronix.de,
linux-mediatek@lists.infradead.org, matthias.bgg@gmail.com
In-Reply-To: <20260331002357.7995-1-rosenp@gmail.com>
On Mon, 2026-03-30 at 17:23 -0700, Rosen Penev wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
>
>
> Use a flexible array member to combine allocations.
Reviewed-by: CK Hu <ck.hu@mediatek.com>
>
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
> drivers/gpu/drm/mediatek/mtk_crtc.c | 13 ++++---------
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
> index fcb16f3f7b23..914841d2396e 100644
> --- a/drivers/gpu/drm/mediatek/mtk_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_crtc.c
> @@ -62,7 +62,6 @@ struct mtk_crtc {
> struct device *dma_dev;
> struct mtk_mutex *mutex;
> unsigned int ddp_comp_nr;
> - struct mtk_ddp_comp **ddp_comp;
> unsigned int num_conn_routes;
> const struct mtk_drm_route *conn_routes;
>
> @@ -71,6 +70,8 @@ struct mtk_crtc {
> bool config_updating;
> /* lock for config_updating to cmd buffer */
> spinlock_t config_lock;
> +
> + struct mtk_ddp_comp *ddp_comp[];
> };
>
> struct mtk_crtc_state {
> @@ -1048,18 +1049,12 @@ int mtk_crtc_create(struct drm_device *drm_dev, const unsigned int *path,
> }
> }
>
> - mtk_crtc = devm_kzalloc(dev, sizeof(*mtk_crtc), GFP_KERNEL);
> + mtk_crtc = devm_kzalloc(dev, struct_size(mtk_crtc, ddp_comp, path_len + (conn_routes ? 1 : 0)), GFP_KERNEL);
> if (!mtk_crtc)
> return -ENOMEM;
>
> - mtk_crtc->mmsys_dev = priv->mmsys_dev;
> mtk_crtc->ddp_comp_nr = path_len;
> - mtk_crtc->ddp_comp = devm_kcalloc(dev,
> - mtk_crtc->ddp_comp_nr + (conn_routes ? 1 : 0),
> - sizeof(*mtk_crtc->ddp_comp),
> - GFP_KERNEL);
> - if (!mtk_crtc->ddp_comp)
> - return -ENOMEM;
> + mtk_crtc->mmsys_dev = priv->mmsys_dev;
>
> mtk_crtc->mutex = mtk_mutex_get(priv->mutex_dev);
> if (IS_ERR(mtk_crtc->mutex)) {
> --
> 2.53.0
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox