* Re: [PATCH net-next V2 4/7] net/mlx5: E-Switch, serialize representor lifecycle
From: Mark Bloch @ 2026-05-02 20:05 UTC (permalink / raw)
To: Tariq Toukan, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Lunn, David S. Miller
Cc: Leon Romanovsky, Jason Gunthorpe, Saeed Mahameed, Shay Drory,
Or Har-Toov, Edward Srouji, Maher Sanalla, Simon Horman,
Gerd Bayer, Moshe Shemesh, Kees Cook, Patrisious Haddad,
Parav Pandit, Carolina Jubran, Cosmin Ratiu, linux-rdma,
linux-kernel, netdev, Gal Pressman, Dragos Tatulea
In-Reply-To: <20260501041633.231662-5-tariqt@nvidia.com>
On 01/05/2026 7:16, Tariq Toukan wrote:
> From: Mark Bloch <mbloch@nvidia.com>
>
> Representor callbacks can be registered and unregistered while the
> E-Switch is already in switchdev mode, and the same E-Switch may also be
> reconfigured by devlink, VF changes and SF changes. Serialize these paths
> with the per-E-Switch representor mutex instead of relying on ad-hoc bit
> state and wait queues.
>
> Take the representor lock around the mode transition, VF/SF representor
> changes and representor ops registration. Keep mode_lock and the
> representor lock unnested by using the operation flag while the mode lock
> is dropped. During mode changes, drop the representor lock around the
> auxiliary bus rescan because driver bind/unbind may register or unregister
> representor ops.
>
> Split representor ops registration into locked public wrappers and blocked
> internal helpers, clear the ops pointer on unregister, and add nested
> wrappers for the shared-FDB master IB path that registers peer
> representor ops while another E-Switch representor lock is already held.
>
> Signed-off-by: Mark Bloch <mbloch@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ---
> drivers/infiniband/hw/mlx5/ib_rep.c | 6 +-
> .../net/ethernet/mellanox/mlx5/core/eswitch.c | 10 ++
> .../mellanox/mlx5/core/eswitch_offloads.c | 102 ++++++++++++++++--
> .../ethernet/mellanox/mlx5/core/sf/devlink.c | 5 +
> include/linux/mlx5/eswitch.h | 6 ++
> 5 files changed, 119 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
> index 1709b628702e..65d8767d1830 100644
> --- a/drivers/infiniband/hw/mlx5/ib_rep.c
> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c
> @@ -262,9 +262,10 @@ mlx5_ib_vport_rep_unload(struct mlx5_eswitch_rep *rep)
> struct mlx5_core_dev *peer_mdev;
> struct mlx5_eswitch *esw;
>
> + /* Called while the master E-Switch reps_lock is held. */
> mlx5_lag_for_each_peer_mdev(mdev, peer_mdev, i) {
> esw = peer_mdev->priv.eswitch;
> - mlx5_eswitch_unregister_vport_reps(esw, REP_IB);
> + mlx5_eswitch_unregister_vport_reps_nested(esw, REP_IB);
> }
> mlx5_ib_release_transport(mdev);
> }
> @@ -284,9 +285,10 @@ static void mlx5_ib_register_peer_vport_reps(struct mlx5_core_dev *mdev)
> struct mlx5_eswitch *esw;
> int i;
>
> + /* Called while the master E-Switch reps_lock is held. */
> mlx5_lag_for_each_peer_mdev(mdev, peer_mdev, i) {
> esw = peer_mdev->priv.eswitch;
> - mlx5_eswitch_register_vport_reps(esw, &rep_ops, REP_IB);
> + mlx5_eswitch_register_vport_reps_nested(esw, &rep_ops, REP_IB);
> }
> }
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> index 66a773a99876..f70737437954 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> @@ -1712,6 +1712,7 @@ int mlx5_eswitch_enable(struct mlx5_eswitch *esw, int num_vfs)
> mlx5_lag_disable_change(esw->dev);
>
> mlx5_eswitch_invalidate_wq(esw);
> + mlx5_esw_reps_block(esw);
>
> if (!mlx5_esw_is_fdb_created(esw)) {
> ret = mlx5_eswitch_enable_locked(esw, num_vfs);
> @@ -1735,6 +1736,8 @@ int mlx5_eswitch_enable(struct mlx5_eswitch *esw, int num_vfs)
> }
> }
>
> + mlx5_esw_reps_unblock(esw);
> +
> if (toggle_lag)
> mlx5_lag_enable_change(esw->dev);
>
> @@ -1759,6 +1762,7 @@ void mlx5_eswitch_disable_sriov(struct mlx5_eswitch *esw, bool clear_vf)
> esw->esw_funcs.num_vfs, esw->esw_funcs.num_ec_vfs, esw->enabled_vports);
>
> mlx5_eswitch_invalidate_wq(esw);
> + mlx5_esw_reps_block(esw);
>
> if (!mlx5_core_is_ecpf(esw->dev)) {
> mlx5_eswitch_unload_vf_vports(esw, esw->esw_funcs.num_vfs);
> @@ -1770,6 +1774,8 @@ void mlx5_eswitch_disable_sriov(struct mlx5_eswitch *esw, bool clear_vf)
> mlx5_eswitch_clear_ec_vf_vports_info(esw);
> }
>
> + mlx5_esw_reps_unblock(esw);
> +
> if (esw->mode == MLX5_ESWITCH_OFFLOADS) {
> struct devlink *devlink = priv_to_devlink(esw->dev);
>
> @@ -1825,7 +1831,11 @@ void mlx5_eswitch_disable(struct mlx5_eswitch *esw)
>
> devl_assert_locked(priv_to_devlink(esw->dev));
> mlx5_lag_disable_change(esw->dev);
> +
> + mlx5_esw_reps_block(esw);
> mlx5_eswitch_disable_locked(esw);
> + mlx5_esw_reps_unblock(esw);
> +
> esw->mode = MLX5_ESWITCH_LEGACY;
> mlx5_lag_enable_change(esw->dev);
> }
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> index 6a5143b63dfd..d4ac07c995b9 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> @@ -36,6 +36,7 @@
> #include <linux/mlx5/mlx5_ifc.h>
> #include <linux/mlx5/vport.h>
> #include <linux/mlx5/fs.h>
> +#include <linux/lockdep.h>
> #include "mlx5_core.h"
> #include "eswitch.h"
> #include "esw/indir_table.h"
> @@ -2413,11 +2414,21 @@ static int esw_create_restore_table(struct mlx5_eswitch *esw)
> return err;
> }
>
> +static void mlx5_esw_assert_reps_locked(struct mlx5_eswitch *esw)
> +{
> + lockdep_assert_held(&esw->offloads.reps_lock);
> +}
> +
> void mlx5_esw_reps_block(struct mlx5_eswitch *esw)
> {
> mutex_lock(&esw->offloads.reps_lock);
> }
>
> +static void mlx5_esw_reps_block_nested(struct mlx5_eswitch *esw)
> +{
> + mutex_lock_nested(&esw->offloads.reps_lock, SINGLE_DEPTH_NESTING);
> +}
> +
> void mlx5_esw_reps_unblock(struct mlx5_eswitch *esw)
> {
> mutex_unlock(&esw->offloads.reps_lock);
> @@ -2425,21 +2436,22 @@ void mlx5_esw_reps_unblock(struct mlx5_eswitch *esw)
>
> static void esw_mode_change(struct mlx5_eswitch *esw, u16 mode)
> {
> + mlx5_esw_reps_unblock(esw);
> mlx5_devcom_comp_lock(esw->dev->priv.hca_devcom_comp);
> if (esw->dev->priv.flags & MLX5_PRIV_FLAGS_DISABLE_IB_ADEV ||
> mlx5_core_mp_enabled(esw->dev)) {
> esw->mode = mode;
> - mlx5_rescan_drivers_locked(esw->dev);
> - mlx5_devcom_comp_unlock(esw->dev->priv.hca_devcom_comp);
> - return;
> + goto out;
> }
>
> esw->dev->priv.flags |= MLX5_PRIV_FLAGS_DISABLE_IB_ADEV;
> mlx5_rescan_drivers_locked(esw->dev);
> esw->mode = mode;
> esw->dev->priv.flags &= ~MLX5_PRIV_FLAGS_DISABLE_IB_ADEV;
> +out:
> mlx5_rescan_drivers_locked(esw->dev);
> mlx5_devcom_comp_unlock(esw->dev->priv.hca_devcom_comp);
> + mlx5_esw_reps_block(esw);
> }
>
> static void mlx5_esw_fdb_drop_destroy(struct mlx5_eswitch *esw)
> @@ -2776,6 +2788,8 @@ void esw_offloads_cleanup(struct mlx5_eswitch *esw)
> static int __esw_offloads_load_rep(struct mlx5_eswitch *esw,
> struct mlx5_eswitch_rep *rep, u8 rep_type)
> {
> + mlx5_esw_assert_reps_locked(esw);
> +
> if (atomic_cmpxchg(&rep->rep_data[rep_type].state,
> REP_REGISTERED, REP_LOADED) == REP_REGISTERED)
> return esw->offloads.rep_ops[rep_type]->load(esw->dev, rep);
> @@ -2786,6 +2800,8 @@ static int __esw_offloads_load_rep(struct mlx5_eswitch *esw,
> static void __esw_offloads_unload_rep(struct mlx5_eswitch *esw,
> struct mlx5_eswitch_rep *rep, u8 rep_type)
> {
> + mlx5_esw_assert_reps_locked(esw);
> +
> if (atomic_cmpxchg(&rep->rep_data[rep_type].state,
> REP_LOADED, REP_REGISTERED) == REP_LOADED) {
> if (rep_type == REP_ETH)
> @@ -3691,6 +3707,7 @@ static void esw_vfs_changed_event_handler(struct mlx5_eswitch *esw)
> if (new_num_vfs == esw->esw_funcs.num_vfs || host_pf_disabled)
> goto free;
>
> + mlx5_esw_reps_block(esw);
> /* Number of VFs can only change from "0 to x" or "x to 0". */
> if (esw->esw_funcs.num_vfs > 0) {
> mlx5_eswitch_unload_vf_vports(esw, esw->esw_funcs.num_vfs);
> @@ -3700,9 +3717,11 @@ static void esw_vfs_changed_event_handler(struct mlx5_eswitch *esw)
> err = mlx5_eswitch_load_vf_vports(esw, new_num_vfs,
> MLX5_VPORT_UC_ADDR_CHANGE);
> if (err)
> - goto free;
> + goto unblock;
> }
> esw->esw_funcs.num_vfs = new_num_vfs;
> +unblock:
> + mlx5_esw_reps_unblock(esw);
> free:
> kvfree(out);
> }
> @@ -4188,9 +4207,14 @@ int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode,
> goto unlock;
> }
>
> + /* Keep mode_lock and reps_lock unnested. The operation flag excludes
> + * mode users while mode_lock is dropped before taking reps_lock.
> + */
> esw->eswitch_operation_in_progress = true;
> up_write(&esw->mode_lock);
>
> + mlx5_esw_reps_block(esw);
> +
> if (mlx5_mode == MLX5_ESWITCH_OFFLOADS &&
> !mlx5_devlink_netdev_netns_immutable_set(devlink, true)) {
> NL_SET_ERR_MSG_MOD(extack,
> @@ -4223,6 +4247,10 @@ int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode,
> skip:
> if (mlx5_mode == MLX5_ESWITCH_OFFLOADS && err)
> mlx5_devlink_netdev_netns_immutable_set(devlink, false);
> + /* Reconfiguration is done; drop reps_lock before taking mode_lock again
> + * to clear the operation flag.
> + */
> + mlx5_esw_reps_unblock(esw);
> down_write(&esw->mode_lock);
> esw->eswitch_operation_in_progress = false;
> unlock:
> @@ -4496,9 +4524,10 @@ mlx5_eswitch_vport_has_rep(const struct mlx5_eswitch *esw, u16 vport_num)
> return true;
> }
>
> -void mlx5_eswitch_register_vport_reps(struct mlx5_eswitch *esw,
> - const struct mlx5_eswitch_rep_ops *ops,
> - u8 rep_type)
> +static void
> +mlx5_eswitch_register_vport_reps_blocked(struct mlx5_eswitch *esw,
> + const struct mlx5_eswitch_rep_ops *ops,
> + u8 rep_type)
> {
> struct mlx5_eswitch_rep_data *rep_data;
> struct mlx5_eswitch_rep *rep;
> @@ -4513,9 +4542,40 @@ void mlx5_eswitch_register_vport_reps(struct mlx5_eswitch *esw,
> }
> }
> }
> +
> +static void
> +mlx5_eswitch_register_vport_reps_locked(struct mlx5_eswitch *esw,
> + const struct mlx5_eswitch_rep_ops *ops,
> + u8 rep_type, bool nested)
> +{
> + if (nested)
> + mlx5_esw_reps_block_nested(esw);
> + else
> + mlx5_esw_reps_block(esw);
> + mlx5_eswitch_register_vport_reps_blocked(esw, ops, rep_type);
> + mlx5_esw_reps_unblock(esw);
> +}
> +
> +void mlx5_eswitch_register_vport_reps(struct mlx5_eswitch *esw,
> + const struct mlx5_eswitch_rep_ops *ops,
> + u8 rep_type)
> +{
> + mlx5_eswitch_register_vport_reps_locked(esw, ops, rep_type, false);
> +}
> EXPORT_SYMBOL(mlx5_eswitch_register_vport_reps);
>
> -void mlx5_eswitch_unregister_vport_reps(struct mlx5_eswitch *esw, u8 rep_type)
> +void
> +mlx5_eswitch_register_vport_reps_nested(struct mlx5_eswitch *esw,
> + const struct mlx5_eswitch_rep_ops *ops,
> + u8 rep_type)
> +{
> + mlx5_eswitch_register_vport_reps_locked(esw, ops, rep_type, true);
> +}
> +EXPORT_SYMBOL(mlx5_eswitch_register_vport_reps_nested);
> +
> +static void
> +mlx5_eswitch_unregister_vport_reps_blocked(struct mlx5_eswitch *esw,
> + u8 rep_type)
> {
> struct mlx5_eswitch_rep *rep;
> unsigned long i;
> @@ -4525,9 +4585,35 @@ void mlx5_eswitch_unregister_vport_reps(struct mlx5_eswitch *esw, u8 rep_type)
>
> mlx5_esw_for_each_rep(esw, i, rep)
> atomic_set(&rep->rep_data[rep_type].state, REP_UNREGISTERED);
> +
> + esw->offloads.rep_ops[rep_type] = NULL;
sashiko.dev says:
"
Could this assignment cause a NULL pointer dereference in concurrent readers?
In mlx5_eswitch_get_proto_dev(), the state is checked before accessing the ops
pointer without holding reps_lock:
if (atomic_read(&rep->rep_data[rep_type].state) == REP_LOADED &&
esw->offloads.rep_ops[rep_type]->get_proto_dev)
return esw->offloads.rep_ops[rep_type]->get_proto_dev(rep);
If a thread in mlx5_eswitch_get_proto_dev() evaluates the state check to true
and is then preempted, can the unregister path execute and set rep_ops to NULL?
When the preempted thread resumes, it might dereference the now-NULL pointer.
Also, since the ops pointer isn't fetched into a local variable using
READ_ONCE(), could the compiler emit multiple loads, further widening the
race window?
"
The REP_LOADED check is not the only protection here, get_proto_dev()
is only reached from representor-owned contexts, and unregister first
unloads all reps under reps_lock. That unload tears down the users
that can call into this helper before the state is set to REP_UNREGISTERED
and before rep_ops is cleared. So clearing rep_ops does not create a new
live-reader window; it only removes the stale ops pointer after the
representor lifecycle is already quiesced.
Mark
> +}
> +
> +static void
> +mlx5_eswitch_unregister_vport_reps_locked(struct mlx5_eswitch *esw,
> + u8 rep_type, bool nested)
> +{
> + if (nested)
> + mlx5_esw_reps_block_nested(esw);
> + else
> + mlx5_esw_reps_block(esw);
> + mlx5_eswitch_unregister_vport_reps_blocked(esw, rep_type);
> + mlx5_esw_reps_unblock(esw);
> +}
> +
> +void mlx5_eswitch_unregister_vport_reps(struct mlx5_eswitch *esw, u8 rep_type)
> +{
> + mlx5_eswitch_unregister_vport_reps_locked(esw, rep_type, false);
> }
> EXPORT_SYMBOL(mlx5_eswitch_unregister_vport_reps);
>
> +void mlx5_eswitch_unregister_vport_reps_nested(struct mlx5_eswitch *esw,
> + u8 rep_type)
> +{
> + mlx5_eswitch_unregister_vport_reps_locked(esw, rep_type, true);
> +}
> +EXPORT_SYMBOL(mlx5_eswitch_unregister_vport_reps_nested);
> +
> void *mlx5_eswitch_get_uplink_priv(struct mlx5_eswitch *esw, u8 rep_type)
> {
> struct mlx5_eswitch_rep *rep;
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c
> index 8503e532f423..2fc69897e35b 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c
> @@ -245,8 +245,10 @@ static int mlx5_sf_add(struct mlx5_core_dev *dev, struct mlx5_sf_table *table,
> if (IS_ERR(sf))
> return PTR_ERR(sf);
>
> + mlx5_esw_reps_block(esw);
> err = mlx5_eswitch_load_sf_vport(esw, sf->hw_fn_id, MLX5_VPORT_UC_ADDR_CHANGE,
> &sf->dl_port, new_attr->controller, new_attr->sfnum);
> + mlx5_esw_reps_unblock(esw);
> if (err)
> goto esw_err;
> *dl_port = &sf->dl_port.dl_port;
> @@ -367,7 +369,10 @@ int mlx5_devlink_sf_port_del(struct devlink *devlink,
> struct mlx5_sf_table *table = dev->priv.sf_table;
> struct mlx5_sf *sf = mlx5_sf_by_dl_port(dl_port);
>
> + mlx5_esw_reps_block(dev->priv.eswitch);
> mlx5_sf_del(table, sf);
> + mlx5_esw_reps_unblock(dev->priv.eswitch);
> +
> return 0;
> }
>
> diff --git a/include/linux/mlx5/eswitch.h b/include/linux/mlx5/eswitch.h
> index 3b29a3c6794d..a0dd162baa78 100644
> --- a/include/linux/mlx5/eswitch.h
> +++ b/include/linux/mlx5/eswitch.h
> @@ -63,7 +63,13 @@ struct mlx5_eswitch_rep {
> void mlx5_eswitch_register_vport_reps(struct mlx5_eswitch *esw,
> const struct mlx5_eswitch_rep_ops *ops,
> u8 rep_type);
> +void
> +mlx5_eswitch_register_vport_reps_nested(struct mlx5_eswitch *esw,
> + const struct mlx5_eswitch_rep_ops *ops,
> + u8 rep_type);
> void mlx5_eswitch_unregister_vport_reps(struct mlx5_eswitch *esw, u8 rep_type);
> +void mlx5_eswitch_unregister_vport_reps_nested(struct mlx5_eswitch *esw,
> + u8 rep_type);
> void *mlx5_eswitch_get_proto_dev(struct mlx5_eswitch *esw,
> u16 vport_num,
> u8 rep_type);
^ permalink raw reply
* Re: [PATCH net-next V2 3/7] net/mlx5: Lag, avoid LAG and representor lock cycles
From: Mark Bloch @ 2026-05-02 20:04 UTC (permalink / raw)
To: Tariq Toukan, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Lunn, David S. Miller
Cc: Leon Romanovsky, Jason Gunthorpe, Saeed Mahameed, Shay Drory,
Or Har-Toov, Edward Srouji, Maher Sanalla, Simon Horman,
Gerd Bayer, Moshe Shemesh, Kees Cook, Patrisious Haddad,
Parav Pandit, Carolina Jubran, Cosmin Ratiu, linux-rdma,
linux-kernel, netdev, Gal Pressman, Dragos Tatulea
In-Reply-To: <20260501041633.231662-4-tariqt@nvidia.com>
On 01/05/2026 7:16, Tariq Toukan wrote:
> From: Mark Bloch <mbloch@nvidia.com>
>
> The LAG shared-FDB and multiport E-Switch transitions rescan auxiliary
> devices and reload IB representors while holding ldev->lock. Driver
> bind/unbind paths may register or unregister E-Switch representor ops, and
> representor load paths may enter LAG code, so holding ldev->lock across
> those calls creates lock-order cycles with the E-Switch representor lock.
>
> Keep the devcom component locked for the transition, but drop ldev->lock
> before rescanning auxiliary devices or reloading IB representors. Mark the
> LAG transition as in progress while the lock is dropped and assert the
> devcom lock where the helper relies on it. This preserves LAG serialization
> while avoiding ldev->lock nesting under E-Switch representor registration.
>
> Signed-off-by: Mark Bloch <mbloch@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ---
> .../net/ethernet/mellanox/mlx5/core/lag/lag.c | 142 ++++++++++++++----
> .../net/ethernet/mellanox/mlx5/core/lag/lag.h | 7 +-
> .../ethernet/mellanox/mlx5/core/lag/mpesw.c | 10 +-
> .../ethernet/mellanox/mlx5/core/lib/devcom.c | 8 +
> .../ethernet/mellanox/mlx5/core/lib/devcom.h | 1 +
> 5 files changed, 134 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index a474f970e056..e77f9931c39c 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -1063,37 +1063,99 @@ bool mlx5_lag_check_prereq(struct mlx5_lag *ldev)
> return true;
> }
>
> -void mlx5_lag_add_devices(struct mlx5_lag *ldev)
> +static void mlx5_lag_assert_locked_transition(struct mlx5_lag *ldev)
> {
> + struct mlx5_devcom_comp_dev *devcom = NULL;
> struct lag_func *pf;
> int i;
>
> - mlx5_ldev_for_each(i, 0, ldev) {
> - pf = mlx5_lag_pf(ldev, i);
> - if (pf->dev->priv.flags & MLX5_PRIV_FLAGS_DISABLE_ALL_ADEV)
> - continue;
> + lockdep_assert_held(&ldev->lock);
>
> - pf->dev->priv.flags &= ~MLX5_PRIV_FLAGS_DISABLE_IB_ADEV;
> - mlx5_rescan_drivers_locked(pf->dev);
> + i = mlx5_get_next_ldev_func(ldev, 0);
> + if (i < MLX5_MAX_PORTS) {
> + pf = mlx5_lag_pf(ldev, i);
> + devcom = pf->dev->priv.hca_devcom_comp;
> }
> + mlx5_devcom_comp_assert_locked(devcom);
> }
>
> -void mlx5_lag_remove_devices(struct mlx5_lag *ldev)
> +static void mlx5_lag_drop_lock_for_reps(struct mlx5_lag *ldev)
> +{
> + mlx5_lag_assert_locked_transition(ldev);
> +
> + /* Keep PF membership stable while ldev->lock is dropped. Device add
> + * and remove paths observe mode_changes_in_progress and retry.
> + */
> + ldev->mode_changes_in_progress++;
> + mutex_unlock(&ldev->lock);
> +}
> +
> +static void mlx5_lag_retake_lock_after_reps(struct mlx5_lag *ldev)
> {
> + mutex_lock(&ldev->lock);
> + ldev->mode_changes_in_progress--;
> +}x
sashiko.dev says:
"
Is it possible this introduces an ad-hoc synchronization mechanism?
The networking subsystem guidelines suggest that using a flag or integer
counter to guard a section of code, particularly when concurrent paths
observe it and retry, can bypass lockdep deadlock detection. These patterns
also often lack proper fairness and memory ordering guarantees compared to
standard locking primitives.
Could a standard synchronization primitive like an rwsem be used here instead
of the mode_changes_in_progress counter?
"
The counter is pre-existing, this patch only reuses it while ldev->lock is
dropped, and all accesses remain protected by ldev->lock.
The transition itself is still serialized by the HCA devcom write lock,
which is lockdep-visible; the counter only makes affected paths retry.
I agree this gating can be cleaned up separately, but I’d prefer
not to fold that broader LAG locking rework into this patch series.
Mark
> +
> +void mlx5_lag_rescan_dev_locked(struct mlx5_lag *ldev,
> + struct mlx5_core_dev *dev,
> + bool enable)
> +{
> + if (dev->priv.flags & MLX5_PRIV_FLAGS_DISABLE_ALL_ADEV)
> + return;
> +
> + if (enable)
> + dev->priv.flags &= ~MLX5_PRIV_FLAGS_DISABLE_IB_ADEV;
> + else
> + dev->priv.flags |= MLX5_PRIV_FLAGS_DISABLE_IB_ADEV;
> +
> + /* Auxiliary bus probe/remove can register or unregister representor
> + * callbacks and take reps_lock. Drop ldev->lock so the only ordering
> + * remains reps_lock -> ldev->lock from representor callbacks.
> + */
> + mlx5_lag_drop_lock_for_reps(ldev);
> + mlx5_rescan_drivers_locked(dev);
> + mlx5_lag_retake_lock_after_reps(ldev);
> +}
> +
> +static void mlx5_lag_rescan_devices_locked(struct mlx5_lag *ldev, bool enable)
> +{
> + struct mlx5_core_dev *devs[MLX5_MAX_PORTS];
> struct lag_func *pf;
> + int num_devs = 0;
> int i;
>
> + mlx5_lag_assert_locked_transition(ldev);
> +
> mlx5_ldev_for_each(i, 0, ldev) {
> pf = mlx5_lag_pf(ldev, i);
> if (pf->dev->priv.flags & MLX5_PRIV_FLAGS_DISABLE_ALL_ADEV)
> continue;
>
> - pf->dev->priv.flags |= MLX5_PRIV_FLAGS_DISABLE_IB_ADEV;
> - mlx5_rescan_drivers_locked(pf->dev);
> + if (enable)
> + pf->dev->priv.flags &= ~MLX5_PRIV_FLAGS_DISABLE_IB_ADEV;
> + else
> + pf->dev->priv.flags |= MLX5_PRIV_FLAGS_DISABLE_IB_ADEV;
> + devs[num_devs++] = pf->dev;
> }
> +
> + mlx5_lag_drop_lock_for_reps(ldev);
> + for (i = 0; i < num_devs; i++)
> + mlx5_rescan_drivers_locked(devs[i]);
> + mlx5_lag_retake_lock_after_reps(ldev);
> }
>
> -int mlx5_lag_reload_ib_reps(struct mlx5_lag *ldev, u32 flags, bool cont_on_fail)
> +void mlx5_lag_add_devices(struct mlx5_lag *ldev)
> +{
> + mlx5_lag_rescan_devices_locked(ldev, true);
> +}
> +
> +void mlx5_lag_remove_devices(struct mlx5_lag *ldev)
> +{
> + mlx5_lag_rescan_devices_locked(ldev, false);
> +}
> +
> +static int mlx5_lag_reload_ib_reps_unlocked(struct mlx5_lag *ldev, u32 flags,
> + bool cont_on_fail)
> {
> struct lag_func *pf;
> int ret;
> @@ -1105,7 +1167,9 @@ int mlx5_lag_reload_ib_reps(struct mlx5_lag *ldev, u32 flags, bool cont_on_fail)
> struct mlx5_eswitch *esw;
>
> esw = pf->dev->priv.eswitch;
> + mlx5_esw_reps_block(esw);
> ret = mlx5_eswitch_reload_ib_reps(esw);
> + mlx5_esw_reps_unblock(esw);
> if (ret && !cont_on_fail)
> return ret;
> }
> @@ -1114,6 +1178,34 @@ int mlx5_lag_reload_ib_reps(struct mlx5_lag *ldev, u32 flags, bool cont_on_fail)
> return 0;
> }
>
> +static int mlx5_lag_reload_ib_reps(struct mlx5_lag *ldev, u32 flags,
> + bool cont_on_fail)
> +{
> + int ret;
> +
> + /* The HCA devcom component lock serializes LAG mode transitions while
> + * ldev->lock is dropped here. Dropping ldev->lock is required because
> + * the reload takes the per-E-Switch reps_lock, and representor
> + * load/unload callbacks can re-enter LAG netdev add/remove and take
> + * ldev->lock. Keep the ordering reps_lock -> ldev->lock.
> + */
> + mlx5_lag_drop_lock_for_reps(ldev);
> + ret = mlx5_lag_reload_ib_reps_unlocked(ldev, flags, cont_on_fail);
> + mlx5_lag_retake_lock_after_reps(ldev);
> +
> + return ret;
> +}
> +
> +int mlx5_lag_reload_ib_reps_from_locked(struct mlx5_lag *ldev, u32 flags,
> + bool cont_on_fail)
> +{
> + int ret;
> +
> + ret = mlx5_lag_reload_ib_reps(ldev, flags, cont_on_fail);
> +
> + return ret;
> +}
> +
> void mlx5_disable_lag(struct mlx5_lag *ldev)
> {
> bool shared_fdb = test_bit(MLX5_LAG_MODE_FLAG_SHARED_FDB, &ldev->mode_flags);
> @@ -1132,10 +1224,7 @@ void mlx5_disable_lag(struct mlx5_lag *ldev)
> if (shared_fdb) {
> mlx5_lag_remove_devices(ldev);
> } else if (roce_lag) {
> - if (!(dev0->priv.flags & MLX5_PRIV_FLAGS_DISABLE_ALL_ADEV)) {
> - dev0->priv.flags |= MLX5_PRIV_FLAGS_DISABLE_IB_ADEV;
> - mlx5_rescan_drivers_locked(dev0);
> - }
> + mlx5_lag_rescan_dev_locked(ldev, dev0, false);
> mlx5_ldev_for_each(i, 0, ldev) {
> if (i == idx)
> continue;
> @@ -1151,8 +1240,9 @@ void mlx5_disable_lag(struct mlx5_lag *ldev)
> mlx5_lag_add_devices(ldev);
>
> if (shared_fdb)
> - mlx5_lag_reload_ib_reps(ldev, MLX5_PRIV_FLAGS_DISABLE_ALL_ADEV,
> - true);
> + mlx5_lag_reload_ib_reps_from_locked(ldev,
> + MLX5_PRIV_FLAGS_DISABLE_ALL_ADEV,
> + true);
> }
>
> bool mlx5_lag_shared_fdb_supported(struct mlx5_lag *ldev)
> @@ -1409,7 +1499,8 @@ static void mlx5_do_bond(struct mlx5_lag *ldev)
> if (shared_fdb || roce_lag)
> mlx5_lag_add_devices(ldev);
> if (shared_fdb)
> - mlx5_lag_reload_ib_reps(ldev, 0, true);
> + mlx5_lag_reload_ib_reps_from_locked(ldev, 0,
> + true);
>
> return;
> }
> @@ -1417,8 +1508,7 @@ static void mlx5_do_bond(struct mlx5_lag *ldev)
> if (roce_lag) {
> struct mlx5_core_dev *dev;
>
> - dev0->priv.flags &= ~MLX5_PRIV_FLAGS_DISABLE_IB_ADEV;
> - mlx5_rescan_drivers_locked(dev0);
> + mlx5_lag_rescan_dev_locked(ldev, dev0, true);
> mlx5_ldev_for_each(i, 0, ldev) {
> if (i == idx)
> continue;
> @@ -1427,15 +1517,15 @@ static void mlx5_do_bond(struct mlx5_lag *ldev)
> mlx5_nic_vport_enable_roce(dev);
> }
> } else if (shared_fdb) {
> - dev0->priv.flags &= ~MLX5_PRIV_FLAGS_DISABLE_IB_ADEV;
> - mlx5_rescan_drivers_locked(dev0);
> - err = mlx5_lag_reload_ib_reps(ldev, 0, false);
> + mlx5_lag_rescan_dev_locked(ldev, dev0, true);
> + err = mlx5_lag_reload_ib_reps_from_locked(ldev, 0,
> + false);
> if (err) {
> - dev0->priv.flags |= MLX5_PRIV_FLAGS_DISABLE_IB_ADEV;
> - mlx5_rescan_drivers_locked(dev0);
> + mlx5_lag_rescan_dev_locked(ldev, dev0, false);
> mlx5_deactivate_lag(ldev);
> mlx5_lag_add_devices(ldev);
> - mlx5_lag_reload_ib_reps(ldev, 0, true);
> + mlx5_lag_reload_ib_reps_from_locked(ldev, 0,
> + true);
> mlx5_core_err(dev0, "Failed to enable lag\n");
> return;
> }
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
> index daca8ebd5256..6afe7707d076 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.h
> @@ -164,6 +164,9 @@ void mlx5_disable_lag(struct mlx5_lag *ldev);
> void mlx5_lag_remove_devices(struct mlx5_lag *ldev);
> int mlx5_deactivate_lag(struct mlx5_lag *ldev);
> void mlx5_lag_add_devices(struct mlx5_lag *ldev);
> +void mlx5_lag_rescan_dev_locked(struct mlx5_lag *ldev,
> + struct mlx5_core_dev *dev,
> + bool enable);
> struct mlx5_devcom_comp_dev *mlx5_lag_get_devcom_comp(struct mlx5_lag *ldev);
>
> #ifdef CONFIG_MLX5_ESWITCH
> @@ -199,6 +202,6 @@ int mlx5_get_next_ldev_func(struct mlx5_lag *ldev, int start_idx);
> int mlx5_lag_get_dev_index_by_seq(struct mlx5_lag *ldev, int seq);
> int mlx5_lag_num_devs(struct mlx5_lag *ldev);
> int mlx5_lag_num_netdevs(struct mlx5_lag *ldev);
> -int mlx5_lag_reload_ib_reps(struct mlx5_lag *ldev, u32 flags,
> - bool cont_on_fail);
> +int mlx5_lag_reload_ib_reps_from_locked(struct mlx5_lag *ldev, u32 flags,
> + bool cont_on_fail);
> #endif /* __MLX5_LAG_H__ */
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c
> index edcd06f3be7a..8a349f8fd823 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/mpesw.c
> @@ -100,9 +100,8 @@ static int mlx5_lag_enable_mpesw(struct mlx5_lag *ldev)
> goto err_add_devices;
> }
>
> - dev0->priv.flags &= ~MLX5_PRIV_FLAGS_DISABLE_IB_ADEV;
> - mlx5_rescan_drivers_locked(dev0);
> - err = mlx5_lag_reload_ib_reps(ldev, 0, false);
> + mlx5_lag_rescan_dev_locked(ldev, dev0, true);
> + err = mlx5_lag_reload_ib_reps_from_locked(ldev, 0, false);
> if (err)
> goto err_rescan_drivers;
>
> @@ -111,12 +110,11 @@ static int mlx5_lag_enable_mpesw(struct mlx5_lag *ldev)
> return 0;
>
> err_rescan_drivers:
> - dev0->priv.flags |= MLX5_PRIV_FLAGS_DISABLE_IB_ADEV;
> - mlx5_rescan_drivers_locked(dev0);
> + mlx5_lag_rescan_dev_locked(ldev, dev0, false);
> mlx5_deactivate_lag(ldev);
> err_add_devices:
> mlx5_lag_add_devices(ldev);
> - mlx5_lag_reload_ib_reps(ldev, 0, true);
> + mlx5_lag_reload_ib_reps_from_locked(ldev, 0, true);
> mlx5_mpesw_metadata_cleanup(ldev);
> return err;
> }
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.c
> index 4b5ac2db55ce..d40c53193ea8 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.c
> @@ -3,6 +3,7 @@
>
> #include <linux/mlx5/vport.h>
> #include <linux/list.h>
> +#include <linux/lockdep.h>
> #include "lib/devcom.h"
> #include "lib/mlx5.h"
> #include "mlx5_core.h"
> @@ -438,3 +439,10 @@ int mlx5_devcom_comp_trylock(struct mlx5_devcom_comp_dev *devcom)
> return 0;
> return down_write_trylock(&devcom->comp->sem);
> }
> +
> +void mlx5_devcom_comp_assert_locked(struct mlx5_devcom_comp_dev *devcom)
> +{
> + if (!devcom)
> + return;
> + lockdep_assert_held_write(&devcom->comp->sem);
> +}
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.h b/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.h
> index 91e5ae529d5c..316052a85ca5 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.h
> @@ -75,5 +75,6 @@ void *mlx5_devcom_get_next_peer_data_rcu(struct mlx5_devcom_comp_dev *devcom,
> void mlx5_devcom_comp_lock(struct mlx5_devcom_comp_dev *devcom);
> void mlx5_devcom_comp_unlock(struct mlx5_devcom_comp_dev *devcom);
> int mlx5_devcom_comp_trylock(struct mlx5_devcom_comp_dev *devcom);
> +void mlx5_devcom_comp_assert_locked(struct mlx5_devcom_comp_dev *devcom);
>
> #endif /* __LIB_MLX5_DEVCOM_H__ */
^ permalink raw reply
* Re: [PATCH net-next] net: phy: realtek: replace magic number with register bit macros
From: Daniel Golle @ 2026-05-02 19:08 UTC (permalink / raw)
To: Aleksander Jan Bajkowski
Cc: andrew, hkallweit1, linux, davem, edumazet, kuba, pabeni,
vladimir.oltean, michael, ih, rmk+kernel, marek.vasut, netdev,
linux-kernel
In-Reply-To: <20260502092857.156831-1-olek2@wp.pl>
On Sat, May 02, 2026 at 11:28:47AM +0200, Aleksander Jan Bajkowski wrote:
> Replace magic number with register bit macros. The description of the
> RTL8211B interrupt register is obtained from publicly available
> datasheet[1].
>
> 1. RTL8211B(L) Rev. 1.5 Datasheet
> Signed-off-by: Aleksander Jan Bajkowski <olek2@wp.pl>
Reviewed-by: Daniel Golle <daniel@makrotopia.org>
^ permalink raw reply
* Re: [PATCH net v4 1/8] xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
From: Jason Xing @ 2026-05-02 19:04 UTC (permalink / raw)
To: Paolo Abeni; +Cc: bpf, netdev@vger.kernel.org, sashiko
In-Reply-To: <99816901-6026-49ec-9c20-b548ed362d11@redhat.com>
On Tue, Apr 28, 2026 at 4:13 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On 4/25/26 7:40 AM, sashiko-bot@kernel.org wrote:
> > Sashiko AI review found 1 potential issue(s):
> > - [High] The patch incompletely fixes the WARN_ON_ONCE in skb_checksum_help() triggered by pure-fragment SKBs. It successfully prevents the explicit fallback in xsk_skb_metadata() (which wedged the TX ring), but leaves the implicit generic network stack fallback vulnerable. If a user sets XDP_TXMD_FLAGS_CHECKSUM on an IFF_TX_SKB_NO_LINEAR device and the hardware checksum offload is disabled at runtime (e.g., via `ethtool -K eth0 tx off`), the core stack's validate_xmit_skb() takes over. Because NETIF_F_SG is allowed to remain active even without checksum features, skb_needs_linearize() evaluates to false, leaving the SKB unlinearized. The SKB is then passed to skb_csum_hwoffload_help() -> skb_checksum_help(), which hits the exact same WARN_ON_ONCE(offset >= skb_headlen(skb)) because skb_headlen is 0.
> > --
> >
> > commit 86ab0a4900aa60bee0c3df7393a2846b5ff74ccb
> > Author: Jason Xing <kerneljasonxing@gmail.com>
> >
> > xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
> >
> > This commit attempts to fix a warning triggered in skb_checksum_help()
> > by AF_XDP pure-fragment SKBs. It rejects sw-csum UMEM binding for
> > drivers advertising IFF_TX_SKB_NO_LINEAR at setup time.
> >
> > Closes: https://lore.kernel.org/all/20260419045822.843BFC2BCAF@smtp.kernel.org/#t
> > Fixes: 30c3055f9c0d ("xsk: wrap generic metadata handling onto separate function")
> > Acked-by: Stanislav Fomichev <sdf@fomichev.me>
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> >
> >> diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> >> index cd7bc50872f6b5..d981cfdd853578 100644
> >> --- a/net/xdp/xsk_buff_pool.c
> >> +++ b/net/xdp/xsk_buff_pool.c
> >> @@ -175,6 +175,9 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
> >> if (force_zc && force_copy)
> >> return -EINVAL;
> >>
> >> + if (pool->tx_sw_csum && (netdev->priv_flags & IFF_TX_SKB_NO_LINEAR))
> >> + return -EOPNOTSUPP;
> >> +
> >
> > Does this still leave a path for the warning in skb_checksum_help() to
> > trigger if hardware checksum offloading is disabled at runtime?
> >
> > If a user sets XDP_TXMD_FLAGS_CHECKSUM on an IFF_TX_SKB_NO_LINEAR device,
> > and hardware checksum offload is later disabled (e.g., via ethtool -K
> > eth0 tx off), the core stack's validate_xmit_skb() will take over.
> >
> > Because NETIF_F_SG can remain active even without checksum features,
> > skb_needs_linearize() evaluates to false, leaving the SKB unlinearized.
> >
> > The SKB is then passed to skb_csum_hwoffload_help() which falls back to
> > skb_checksum_help(). Since the pure-fragment SKB still has a headlen of 0,
> > won't this hit the exact same WARN_ON_ONCE(offset >= skb_headlen(skb))
> > warning?
>
> My take here is the issue describe above should be handled separately.
> (And on top of my head I don't see an easy way to address it short of an
> additional check in the tx path, which I think we want to avoid).
Agreed. I've already reworded the commit message locally :)
Thanks,
Jason
^ permalink raw reply
* Re: [PATCH rc 00/15] Various bug fixes for RDMA drivers in the uapi functions
From: Jason Gunthorpe @ 2026-05-02 18:39 UTC (permalink / raw)
To: Andrew Lunn, Broadcom internal kernel review list, Bryan Tan,
Eric Dumazet, Junxian Huang, Konstantin Taranov, Jakub Kicinski,
Leon Romanovsky, linux-hyperv, linux-rdma, netdev, Paolo Abeni,
Selvin Xavier, Chengchang Tang, Tariq Toukan, Vishnu Dasa,
Yishai Hadas
Cc: Abhijit Gangurde, Adit Ranadive, Allen Hubbe, Andrew Boyer,
Aditya Sarwade, Brad Spengler, Bryan Tan, David S. Miller,
Dexuan Cui, Doug Ledford, George Zhang, Jorgen Hansen, Jianbo Liu,
Kai Aizen, Leon Romanovsky, Leon Romanovsky, Yixian Liu, Long Li,
Lijun Ou, Parav Pandit, patches, Roland Dreier, Roland Dreier,
Sagi Grimberg, Ajay Sharma, stable, Tariq Toukan, Wei Hu (Xavier),
Shaobo Xu, Nenglong Zhao
In-Reply-To: <0-v1-41f3135e5565+9d2-rdma_ai_fixes1_jgg@nvidia.com>
On Tue, Apr 28, 2026 at 01:17:33PM -0300, Jason Gunthorpe wrote:
> All were found by Sashiko or Claude AI tools. They vary in severity, but
> are all things that shouldn't be present.
>
> Jason Gunthorpe (15):
> RDMA/ionic: Fix typo in format string
> RDMA/mlx5: Restore zero-init to mlx5_ib_modify_qp() ucmd
> RDMA/mlx5: Add missing store/release for lock elision pattern
> RDMA/mana: Validate rx_hash_key_len
> RDMA/mana: Remove user triggerable WARN_ON() in
> mana_ib_create_qp_rss()
> RDMA/mana: Fix mana_destroy_wq_obj() cleanup in
> mana_ib_create_qp_rss()
> RDMA/mana: Fix error unwind in mana_ib_create_qp_rss()
> RDMA/ocrdma: Clarify the mm_head searching
> RDMA/ocrdma: Don't NULL deref uctx on errors in ocrdma_copy_pd_uresp()
> RDMA/vmw_pvrdma: Fix double free on pvrdma_alloc_ucontext() error path
> RDMA/mlx4: Fix resource leak on error in mlx4_ib_create_srq()
> RDMA/mlx4: Fix mis-use of RCU in mlx4_srq_event()
> RDMA/hns: Fix xarray race in hns_roce_create_srq()
> RDMA/hns: Fix xarray race in hns_roce_create_qp_common()
> RDMA/hns: Fix unlocked call to hns_roce_qp_remove()
>
> drivers/infiniband/hw/hns/hns_roce_qp.c | 13 ++++++++++---
> drivers/infiniband/hw/hns/hns_roce_srq.c | 12 ++++++------
> drivers/infiniband/hw/ionic/ionic_ibdev.c | 2 +-
> drivers/infiniband/hw/mana/cq.c | 5 +++--
> drivers/infiniband/hw/mana/qp.c | 16 ++++++++++------
> drivers/infiniband/hw/mlx4/srq.c | 4 +++-
> drivers/infiniband/hw/mlx5/main.c | 8 ++++----
> drivers/infiniband/hw/mlx5/qp.c | 2 +-
> drivers/infiniband/hw/mlx5/umr.c | 4 ++--
> drivers/infiniband/hw/ocrdma/ocrdma_verbs.c | 8 ++++----
> drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c | 2 +-
> drivers/net/ethernet/mellanox/mlx4/srq.c | 13 +++++++------
> 12 files changed, 52 insertions(+), 37 deletions(-)
Applied to for-rc
Jason
^ permalink raw reply
* Re: [PATCH iproute2-next 2/2] dpll: add frequency monitoring support
From: David Ahern @ 2026-05-02 18:29 UTC (permalink / raw)
To: Ivan Vecera, netdev; +Cc: Stephen Hemminger, Petr Oros
In-Reply-To: <20260428152115.2815860-3-ivecera@redhat.com>
On 4/28/26 9:21 AM, Ivan Vecera wrote:
> diff --git a/man/man8/dpll.8 b/man/man8/dpll.8
> index 89f17af74923..59ec4208f251 100644
> --- a/man/man8/dpll.8
> +++ b/man/man8/dpll.8
> @@ -111,7 +111,7 @@ Temperature (if supported)
> Type (PPS or EEC)
> .RE
>
> -.SS dpll device set id ID [ mode { automatic | manual } ] [ phase-offset-monitor { enable | disable } ] [ phase-offset-avg-factor FACTOR ]
> +.SS dpll device set id ID [ mode { automatic | manual } ] [ phase-offset-monitor { enable | disable } ] [ phase-offset-avg-factor FACTOR ] [ frequency-monitor { enable | disable } ]
very long line; please fix up.
Keep Petr reviewed by on patch 1.
Claude has some comments on the man page in general:
● I found three issues. Let me show them clearly:
Issue 1 — Typo: "locked-ho-ack" should be "locked-ho-acq" (line 107)
The man page description of device show output says locked-ho-ack but
the code (dpll_lock_status_name) returns locked-ho-acq
(ACQuired, not ACKnowledge).
Issue 2 — Wrong mode values in output description (lines 104–105)
The "Output includes" section says:
▎ Operating mode (manual, automatic, holdover, freerun)
But dpll_mode_map and DPLL_MODE_* only define manual and automatic.
Holdover and freerun are not DPLL modes — they are lock statuses.
This should read (manual, automatic).
Issue 3 — Undocumented but accepted alternate values for monitors
(lines 134, 144)
The man page documents { enable | disable | true | false | 0 | 1 } for
phase-offset-monitor and frequency-monitor. The tool's own
help text shows only { enable | disable } and the error message says
"use enable/disable". While str_to_bool technically accepts
true/false/0/1, the canonical interface is enable | disable. The extra
forms in the man page are inconsistent with the help text.
^ permalink raw reply
* Re: [PATCH net-next] net/mlx5: Add vhca_id_type support to IPsec alias creation
From: patchwork-bot+netdevbpf @ 2026-05-02 18:20 UTC (permalink / raw)
To: Tariq Toukan
Cc: edumazet, kuba, pabeni, andrew+netdev, davem, saeedm, leon,
mbloch, phaddad, jianbol, kees, dtatulea, netdev, linux-rdma,
linux-kernel, gal, leonro
In-Reply-To: <20260430061958.225245-1-tariqt@nvidia.com>
Hello:
This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Thu, 30 Apr 2026 09:19:58 +0300 you wrote:
> From: Patrisious Haddad <phaddad@nvidia.com>
>
> When creating an alias FT for MPV IPsec, if alias creation with
> sw_vhca_id is supported use it instead of using the hw_vhca_id.
>
> This in turn allows IPsec to work properly after live migration,
> in case a VF was live migrated and his hw_vhca_id changed due to
> migration which can happen if you migrate to a VF with a different index
> than yours, IPsec would fail to start post migration, this patch
> resolves the issue by using sw_vhca_id instead which doesn't change post
> migration.
>
> [...]
Here is the summary with links:
- [net-next] net/mlx5: Add vhca_id_type support to IPsec alias creation
https://git.kernel.org/netdev/net-next/c/c8300af614b2
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH net-next v6 0/3] net: dsa: yt921x: Add port police support
From: patchwork-bot+netdevbpf @ 2026-05-02 18:20 UTC (permalink / raw)
To: David Yang
Cc: netdev, vladimir.oltean, UNGLinuxDriver, andrew, davem, edumazet,
kuba, pabeni, horms, linux-kernel, olteanv
In-Reply-To: <20260430114529.3536911-1-mmyangfl@gmail.com>
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Thu, 30 Apr 2026 19:45:23 +0800 you wrote:
> v5: https://lore.kernel.org/r/20260428112606.1917230-1-mmyangfl@gmail.com
> - remove new line chars in extact messages
> v4: https://lore.kernel.org/r/20260409171209.2575583-1-mmyangfl@gmail.com
> - split series and drop patch for qdisc tbf
> v3: https://lore.kernel.org/r/20260407160559.1747616-1-mmyangfl@gmail.com
> - explain long registers more accurately
> - fix missing packet mode flag
> - rearrange function layout, in preparation for further patches
> v2: https://lore.kernel.org/r/20260402223437.109097-1-mmyangfl@gmail.com
> - refine commit messages and code styles, no functional changes
> v1: https://lore.kernel.org/r/20260225090853.2021140-1-mmyangfl@gmail.com
> - pass extack to user tc policers
> - keep reg64 helpers along with reg96
> - avoid macros in favor of functions
> - adjust log messages
>
> [...]
Here is the summary with links:
- [net-next,v6,1/3] net: dsa: pass extack to dsa_switch_ops :: port_policer_add()
https://git.kernel.org/netdev/net-next/c/09e4d1298b36
- [net-next,v6,2/3] net: dsa: yt921x: Refactor long register helpers
https://git.kernel.org/netdev/net-next/c/9f8888984eb0
- [net-next,v6,3/3] net: dsa: yt921x: Add port police support
https://git.kernel.org/netdev/net-next/c/39716c00ed0a
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH net-next] Documentation/tcp_ao: Document the supported MAC algorithms and lengths
From: patchwork-bot+netdevbpf @ 2026-05-02 18:20 UTC (permalink / raw)
To: Eric Biggers
Cc: netdev, linux-crypto, linux-kernel, edumazet, ncardwell, kuniyu,
davem, dsahern, kuba, pabeni, horms, ardb, Jason, herbert,
0x7f454c46
In-Reply-To: <20260429210856.725667-1-ebiggers@kernel.org>
Hello:
This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Wed, 29 Apr 2026 21:08:56 +0000 you wrote:
> Update the TCP-AO documentation to fix some incorrect terminology and
> claims regarding the MAC algorithms, and document which MAC algorithms
> and lengths the Linux implementation supports.
>
> Signed-off-by: Eric Biggers <ebiggers@kernel.org>
> ---
> Documentation/networking/tcp_ao.rst | 38 ++++++++++++++++++++---------
> 1 file changed, 27 insertions(+), 11 deletions(-)
>
> [...]
Here is the summary with links:
- [net-next] Documentation/tcp_ao: Document the supported MAC algorithms and lengths
https://git.kernel.org/netdev/net-next/c/34d67417c8cf
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH net v2] ip6_gre: Use cached t->net in ip6erspan_changelink().
From: patchwork-bot+netdevbpf @ 2026-05-02 18:20 UTC (permalink / raw)
To: Maoyi Xie
Cc: netdev, davem, kuba, pabeni, edumazet, dsahern, horms, willemb,
kuniyu, shaw.leon, linux-kernel, stable
In-Reply-To: <20260430103318.3206018-1-maoyi.xie@ntu.edu.sg>
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Thu, 30 Apr 2026 18:33:18 +0800 you wrote:
> After commit 5e72ce3e3980 ("net: ipv6: Use link netns in newlink() of
> rtnl_link_ops"), ip6erspan_newlink() correctly resolves the per-netns
> ip6gre hash via link_net. ip6erspan_changelink() was not converted in
> that series and still uses dev_net(dev), which diverges from the
> device's creation netns after IFLA_NET_NS_FD migration.
>
> This re-inserts the tunnel into the wrong per-netns hash. The
> original netns keeps a stale entry. When that netns is later
> destroyed, ip6gre_exit_rtnl_net() walks the stale entry, producing a
> slab-use-after-free reported by KASAN, followed by a kernel BUG at
> net/core/dev.c (LIST_POISON1) in unregister_netdevice_many_notify().
>
> [...]
Here is the summary with links:
- [net,v2] ip6_gre: Use cached t->net in ip6erspan_changelink().
https://git.kernel.org/netdev/net/c/1d324c2f43f7
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH] amd-xgbe: fix PTP addend overflow causing frozen clock
From: patchwork-bot+netdevbpf @ 2026-05-02 18:20 UTC (permalink / raw)
To: Gregory Fuchedgi
Cc: Raju.Rangoju, PrashanthKumar.K.R, andrew+netdev, davem, edumazet,
kuba, pabeni, richardcochran, netdev, linux-kernel
In-Reply-To: <20260429-fix-xgbe-ptp-addend-v1-1-fca5b0ca5e62@gmail.com>
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Wed, 29 Apr 2026 14:54:14 -0700 you wrote:
> From: Gregory Fuchedgi <gfuchedgi@gmail.com>
>
> XGBE_PTP_ACT_CLK_FREQ and XGBE_V2_PTP_ACT_CLK_FREQ were 10x too
> large (500MHz/1GHz instead of 50MHz/100MHz), causing the computed
> addend to overflow the 32-bit tstamp_addend. In the general case
> this would result in the clock advancing at the wrong rate. For v2
> (PCI), ptpclk_rate is hardcoded to 125MHz, so the addend formula
> (ACT_CLK_FREQ << 32) / ptpclk_rate yields exactly 8 * 2^32, and
> when stored to the 32-bit tstamp_addend the value is zero. With
> addend = 0 the hardware accumulator never overflows and the PTP
> clock is fully stopped. For v1 (platform), ptpclk_rate is read from
> ACPI/DT so the exact overflow behavior depends on the
> firmware-reported frequency.
>
> [...]
Here is the summary with links:
- amd-xgbe: fix PTP addend overflow causing frozen clock
https://git.kernel.org/netdev/net/c/383d0fb89469
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH net 0/3] Replace direct dequeue call with qdisc_dequeue_peeked
From: patchwork-bot+netdevbpf @ 2026-05-02 18:20 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: netdev, davem, kuba, edumazet, pabeni, horms, jiri, victor,
pctammela, ghandatmanas, rakshitawasthi17, security
In-Reply-To: <20260430152957.194015-1-jhs@mojatatu.com>
Hello:
This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Thu, 30 Apr 2026 11:29:54 -0400 you wrote:
> When sfb and red qdiscs have children (eg qfq qdisc) whose peek() callback is
> qdisc_peek_dequeued(), we could get a kernel panic. When the parent of such
> qdiscs (eg illustrated in patch #3 as tbf) wants to retrieve an skb from
> its child (red/sfb in this case), it will do the following:
> 1a. do a peek() - and when sensing there's an skb the child can offer, then
> - the child in this case(red/sfb) calls its child's (qfq) peek.
> qfq does the right thing and will return the gso_skb queue packet.
> Note: if there wasnt a gso_skb entry then qfq will store it there.
> 1b. invoke a dequeue() on the child (red/sfb). And herein lies the problem.
> - red/sfb will call the child's dequeue() which will essentially just
> try to grab something of qfq's queue.
>
> [...]
Here is the summary with links:
- [net,1/3] net/sched: sch_red: Replace direct dequeue call with peek and qdisc_dequeue_peeked
https://git.kernel.org/netdev/net/c/458d5615272d
- [net,2/3] net/sched: sch_sfb: Replace direct dequeue call with peek and qdisc_dequeue_peeked
https://git.kernel.org/netdev/net/c/1b9bc71153b0
- [net,3/3] selftests/tc-testing: Add tests that force red and sfb to dequeue from child's gso_skb
https://git.kernel.org/netdev/net/c/3a3a30c14d7f
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH iproute2-next] tc: use ll_init_map() only when needed
From: David Ahern @ 2026-05-02 18:10 UTC (permalink / raw)
To: Eric Dumazet, Jamal Hadi Salim
Cc: Stephen Hemminger, David S . Miller, Jakub Kicinski, Paolo Abeni,
netdev, eric.dumazet
In-Reply-To: <CANn89iLTH=_=qYS6RzqpGeCo2FuuVW70h_yRycy2L59G+APiew@mail.gmail.com>
On 5/2/26 11:25 AM, Eric Dumazet wrote:
> On Thu, Apr 30, 2026 at 11:20 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>
>> On Thu, Apr 30, 2026 at 11:36 AM David Ahern <dsahern@kernel.org> wrote:
>>>
>>> Jamal: waiting for your review ...
>>>
>>> On 4/28/26 2:28 AM, Eric Dumazet wrote:
>>>> Some setups can have thousands of devices.
>>>>
>>>> ll_init_map() is rather expensive for them.
>>>>
>>>> Only call ll_init_map() in the following cases:
>>>>
>>>> 1) tc runs in batch mode.
>>>> 2) tc runs in monitor mode.
>>>> 3) tc dumps qdiscs/classes/filters for all netdev.
>>>>
>>>> This greatly reduces RTNL pressure on common operations.
>>>>
>>>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>>
>> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>
> Gentle reminder, this patch has not been merged.
>
> I have other patches coming that depend on this one.
>
> Thanks!
fixed the long line length and applied. please cc Jamal on all tc
patches; there is a MAINTAINERS file for iproute2 to indicate who needs
to be added to patches.
^ permalink raw reply
* [PATCH v1 net] ipmr: Add __rcu to netns_ipv4.mrt.
From: Kuniyuki Iwashima @ 2026-05-02 18:07 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Simon Horman, Kuniyuki Iwashima, Kuniyuki Iwashima, netdev,
kernel test robot
kernel test robot reported this Sparse warning:
$ make C=1 net/ipv4/ipmr.o
net/ipv4/ipmr.c:312:24: error: incompatible types in comparison expression (different address spaces):
net/ipv4/ipmr.c:312:24: struct mr_table [noderef] __rcu *
net/ipv4/ipmr.c:312:24: struct mr_table *
Let's add __rcu annotation to netns_ipv4.mrt.
Fixes: b3b6babf4751 ("ipmr: Free mr_table after RCU grace period.")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202605030032.glNApko7-lkp@intel.com/
Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
---
include/net/netns/ipv4.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index 80ccd4dda8e0..6e27c56514df 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -275,7 +275,7 @@ struct netns_ipv4 {
#ifdef CONFIG_IP_MROUTE
#ifndef CONFIG_IP_MROUTE_MULTIPLE_TABLES
- struct mr_table *mrt;
+ struct mr_table __rcu *mrt;
#else
struct list_head mr_tables;
struct fib_rules_ops *mr_rules_ops;
--
2.54.0.545.g6539524ca2-goog
^ permalink raw reply related
* Re: [PATCH net 0/2] net/sched: sch_cake: annotate data-races in cake_dump_class_stats (series)
From: Simon Horman @ 2026-05-02 17:58 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Jamal Hadi Salim,
Toke Høiland-Jørgensen, Jiri Pirko, netdev,
eric.dumazet
In-Reply-To: <20260430061610.3503483-1-edumazet@google.com>
On Thu, Apr 30, 2026 at 06:16:08AM +0000, Eric Dumazet wrote:
> cake_dump_class_stats() runs without qdisc spinlock being held.
>
> In this series (of two), I add READ_ONCE()/WRITE_ONCE() annotations for:
>
> - flow->head
> - flow->dropped
> - b->backlogs[]
> - flow->deficit
> - flow->cvars.dropping
> - flow->cvars.count
> - flow->cvars.p_drop
> - flow->cvars.blue_timer
> - flow->cvars.drop_next
>
> Eric Dumazet (2):
> net/sched: sch_cake: annotate data-races in cake_dump_class_stats (I)
> net/sched: sch_cake: annotate data-races in cake_dump_class_stats (II)
For the series:
Reviewed-by: Simon Horman <horms@kernel.org>
^ permalink raw reply
* Re: [PATCH v7 net-next 6/8] ice: implement CPI support for E825C
From: Jakub Kicinski @ 2026-05-02 17:33 UTC (permalink / raw)
To: grzegorz.nitka
Cc: Jakub Kicinski, netdev, ivecera, vadim.fedorenko, jiri, edumazet,
przemyslaw.kitszel, richardcochran, donald.hunter, linux-kernel,
arkadiusz.kubalewski, andrew+netdev, intel-wired-lan, horms,
Prathosh.Satish, anthony.l.nguyen, pabeni, davem
In-Reply-To: <20260430094238.987976-7-grzegorz.nitka@intel.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ice: implement CPI support for E825C
This patch adds CPI (Converged PHY Interface) command handling for E825C
devices, introducing a new ice_cpi.c/ice_cpi.h subsystem that implements
the REQ/ACK handshake and a helper to enable or disable Tx reference
clocks. It also adds per-PHY mutexes on the adapter and a non-posted
write opcode (wr_np) to SBQ.
> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
> index e95266c7f20b..fa238a6a0e1a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adapter.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
> @@ -31,6 +34,8 @@ struct ice_port_list {
> * @ptp_gltsyn_time_lock: Spinlock protecting access to the GLTSYN_TIME
> * register of the PTP clock.
> * @txq_ctx_lock: Spinlock protecting access to the GLCOMM_QTX_CNTX_CTL register
> + * @cpi_phy_lock: Per-PHY mutex serializing CPI REQ/ACK transactions.
> + * Index 0 = PHY0, index 1 = PHY1. Only used on E825C.
> * @ctrl_pf: Control PF of the adapter
> * @ports: Ports list
> * @index: 64-bit index cached for collision detection on 32bit systems
The kernel-doc says cpi_phy_lock is "Only used on E825C", but in
ice_adapter_new() and ice_adapter_free() the mutexes are unconditionally
initialized and destroyed for every adapter:
for (int i = 0; i < ARRAY_SIZE(adapter->cpi_phy_lock); i++)
mutex_init(&adapter->cpi_phy_lock[i]);
Would it be better to either relax the comment or gate the
init/destroy on an E825C device check so that the behavior matches the
documentation?
> diff --git a/drivers/net/ethernet/intel/ice/ice_cpi.c b/drivers/net/ethernet/intel/ice/ice_cpi.c
> new file mode 100644
> index 000000000000..d5be36c60697
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/ice/ice_cpi.c
> @@ -0,0 +1,364 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2018-2026 Intel Corporation */
The sibling ice_cpi.h added in the same commit uses
"SPDX-License-Identifier: GPL-2.0-only" and "Copyright (C) 2018-2025".
Should the two newly added files use the same SPDX form (GPL-2.0 is the
deprecated alias of GPL-2.0-only) and the same copyright end year?
[ ... ]
> +/**
> + * ice_cpi_wait_req0_ack0 - waits for CPI interface to be available
> + * @hw: pointer to the HW struct
> + * @phy: phy index of port the CPI action is taken on
> + *
> + * This function checks if CPI interface is ready to use by CPI client.
> + * It's done by assuring LM.CMD.REQ and PHY.CMD.ACK bit in CPI
> + * interface registers to be 0.
> + *
> + * Return: 0 on success, negative on error
> + */
> +static int ice_cpi_wait_req0_ack0(struct ice_hw *hw, int phy)
> +{
> + u32 phy_val;
> + u32 lm_val;
> +
> + for (int i = 0; i < CPI_RETRIES_COUNT; i++) {
> + int err;
> +
> + /* check if another CPI Client is also accessing CPI */
> + err = ice_cpi_read_phy(hw, phy, CPI0_LM1_CMD_DATA, &lm_val);
> + if (err)
> + return err;
> + if (FIELD_GET(CPI_LM_CMD_REQ_M, lm_val))
> + return -EBUSY;
The function is shaped as a retry loop and the kernel-doc says it
"waits for CPI interface to be available", but when LM.REQ is observed
set it returns -EBUSY on the first iteration with no retry.
Only the ACK-deasserted check actually benefits from the loop and the
msleep(CPI_RETRIES_CADENCE_MS) cadence.
If another CPI client transiently holds REQ, should this path also be
retried (up to CPI_RETRIES_COUNT times) rather than failing
immediately?
[ ... ]
> +/**
> + * ice_cpi_exec - executes CPI command
> + * @hw: pointer to the HW struct
> + * @phy: phy index of port the CPI action is taken on
> + * @cmd: pointer to the command struct to execute
> + * @resp: pointer to user allocated CPI response struct
> + *
> + * This function executes CPI request with respect to CPI handshake
> + * mechanism.
> + *
> + * Return: 0 on success, otherwise negative on error
> + */
> +int ice_cpi_exec(struct ice_hw *hw, u8 phy,
> + const struct ice_cpi_cmd *cmd,
> + struct ice_cpi_resp *resp)
> +{
[ ... ]
> + /* 1. Try to acquire the bus, PHY ACK should be low before we begin */
> + err = ice_cpi_wait_req0_ack0(hw, phy);
> + if (err)
> + goto cpi_exec_exit;
> +
> + /* 2. We start the CPI request */
> + err = ice_cpi_exec_cmd(hw, phy, lm_cmd);
> + if (err)
> + goto cpi_exec_exit;
Can this leave LM.REQ stuck asserted on the hardware?
ice_cpi_exec_cmd() writes lm_cmd with CPI_LM_CMD_REQ_M set. If
ice_sbq_rw_reg() returns an error after the hardware has already latched
the LM.REQ=1 write (for example an admin-queue completion timeout after
the write was dispatched), control jumps to cpi_exec_exit and skips the
REQ deassert at cpi_deassert (steps 4 and 5).
Every subsequent ice_cpi_exec() on that PHY would then fail immediately
in ice_cpi_wait_req0_ack0() with -EBUSY, because that helper returns
-EBUSY on the first read that observes LM.REQ==1 and does not retry.
The step-3 error path already falls through to cpi_deassert; should the
step-2 error path do the same so REQ is always cleared? Deasserting REQ
when it was never latched is harmless.
> +
> + /*
> + * 3. Wait for CPI confirmation, PHY ACK should be asserted and opcode
> + * echoed in the response
> + */
> + err = ice_cpi_wait_ack1(hw, phy, &phy_cmd);
> + if (err)
> + goto cpi_deassert;
> +
> + if (FIELD_GET(CPI_PHY_CMD_ACK_M, phy_cmd) &&
> + FIELD_GET(CPI_LM_CMD_OPCODE_M, lm_cmd) !=
> + FIELD_GET(CPI_PHY_CMD_OPCODE_M, phy_cmd)) {
> + err = -EFAULT;
> + goto cpi_deassert;
> + }
Is the FIELD_GET(CPI_PHY_CMD_ACK_M, phy_cmd) conjunct here ever false?
ice_cpi_wait_ack1() only returns 0 with asserted=true when
CPI_PHY_CMD_ACK_M is set in the value it stored into phy_cmd:
if (asserted && FIELD_GET(CPI_PHY_CMD_ACK_M, phy_val)) {
if (data)
*data = phy_val;
return 0;
}
So the ACK sub-expression is always true on this path, and reading the
code suggests a NACK/ACK=0 case is handled here that isn't. Would it be
clearer to drop the redundant check?
[ ... ]
> +/**
> + * ice_cpi_set_cmd - execute CPI SET command
> + * @hw: pointer to the HW struct
> + * @opcode: CPI command opcode
> + * @phy: phy index CPI command is applied for
> + * @port_lane: ephy index CPI command is applied for
> + * @data: CPI opcode context specific data
> + *
> + * Return: 0 on success.
> + */
> +static int ice_cpi_set_cmd(struct ice_hw *hw, u16 opcode, u8 phy, u8 port_lane,
> + u16 data)
[ ... ]
> +/**
> + * ice_cpi_ena_dis_clk_ref - enables/disables Tx reference clock on port
> + * @hw: pointer to the HW struct
> + * @phy: phy index of port for which Tx reference clock is enabled/disabled
> + * @clk: Tx reference clock to enable or disable
> + * @enable: bool value to enable or disable Tx reference clock
> + *
> + * This function executes CPI request to enable or disable specific
> + * Tx reference clock on given PHY.
> + *
> + * Return: 0 on success.
> + */
> +int ice_cpi_ena_dis_clk_ref(struct ice_hw *hw, u8 phy,
> + enum ice_e825c_ref_clk clk, bool enable)
The kernel-doc here and on ice_cpi_set_cmd() says "Return: 0 on
success." but both functions forward any negative errno produced by
ice_cpi_exec() (such as -EINVAL, -EBUSY, -EFAULT, or -ETIMEDOUT).
Should the Return: documentation also cover the error path?
> diff --git a/drivers/net/ethernet/intel/ice/ice_cpi.h b/drivers/net/ethernet/intel/ice/ice_cpi.h
> new file mode 100644
> index 000000000000..932fe0c0824a
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/ice/ice_cpi.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright (C) 2018-2025 Intel Corporation */
> +
> +#ifndef _ICE_CPI_H_
> +#define _ICE_CPI_H_
[ ... ]
> +/* CPI LM CMD DATA register (CPI0_LM1_CMD_DATA) */
> +#define CPI_LM_CMD_DATA_M GENMASK(15, 0)
> +#define CPI_LM_CMD_OPCODE_M GENMASK(23, 16)
> +#define CPI_LM_CMD_PORTLANE_M GENMASK(26, 24)
> +#define CPI_LM_CMD_RSVD_M GENMASK(28, 27)
> +#define CPI_LM_CMD_GET_SET_M BIT(29)
> +#define CPI_LM_CMD_RESET_M BIT(30)
> +#define CPI_LM_CMD_REQ_M BIT(31)
> +
> +#define CPI_OPCODE_PHY_CLK 0xF1
> +#define CPI_OPCODE_PHY_CLK_PHY_SEL_M GENMASK(9, 6)
> +#define CPI_OPCODE_PHY_CLK_REF_CTRL_M GENMASK(5, 4)
> +#define CPI_OPCODE_PHY_CLK_PORT_SEL 0
> +#define CPI_OPCODE_PHY_CLK_DISABLE 1
> +#define CPI_OPCODE_PHY_CLK_ENABLE 2
> +#define CPI_OPCODE_PHY_CLK_REF_SEL_M GENMASK(3, 0)
> +
> +#define CPI_OPCODE_PHY_PCS_RESET 0xF0
> +#define CPI_OPCODE_PHY_PCS_ONPI_RESET_VAL 0x3F
> +
> +#define CPI_LM_CMD_REQ 1
> +#define CPI_LM_CMD_SET 1
A few of these macros appear unused anywhere in this patch and through
the end of the series: CPI_LM_CMD_RESET_M, CPI_OPCODE_PHY_CLK_PORT_SEL,
CPI_OPCODE_PHY_PCS_RESET, CPI_OPCODE_PHY_PCS_ONPI_RESET_VAL, and
CPI_LM_CMD_SET. CPI_LM_CMD_SET (value 1) looks like it was intended to
be passed to FIELD_PREP(CPI_LM_CMD_GET_SET_M, ...), but ice_cpi_exec()
uses the bool cmd->set directly and leaves the named constant dead.
Would it make sense to drop the unused defines, or wire them in if
they're intended for a follow-up?
Also, the commit message mentions "enabling/disabling/selection of PHY
reference clock" and CPI_OPCODE_PHY_CLK_PORT_SEL is defined for the
selection variant, but no API in this patch uses it;
ice_cpi_ena_dis_clk_ref() only issues ENABLE or DISABLE. Should the
commit message be trimmed to match what the patch actually delivers?
> +
> +struct ice_cpi_cmd {
> + u8 port;
> + u8 opcode;
> + u16 data;
> + bool set;
> +};
> +
> +struct ice_cpi_resp {
> + u8 port;
> + u8 opcode;
> + u16 data;
> +};
> +
> +int ice_cpi_exec(struct ice_hw *hw, u8 phy,
> + const struct ice_cpi_cmd *cmd,
> + struct ice_cpi_resp *resp);
> +int ice_cpi_ena_dis_clk_ref(struct ice_hw *hw, u8 port,
> + enum ice_e825c_ref_clk clk, bool enable);
> +#endif /* _ICE_CPI_H_ */
Is ice_cpi.h intended to be self-contained?
The header declares ice_cpi_ena_dis_clk_ref() with an
enum ice_e825c_ref_clk parameter and uses struct ice_hw * in both
prototypes, but it does not include ice_ptp_hw.h (which defines
enum ice_e825c_ref_clk) and does not pull in anything that forward-
declares struct ice_hw. ice_cpi.c compiles because it includes
ice_type.h, ice_common.h and ice_ptp_hw.h before ice_cpi.h.
Since C does not allow a portable forward declaration of an enum type,
would a future consumer that includes ice_cpi.h without first including
ice_ptp_hw.h fail to compile? Should the header include what it needs?
^ permalink raw reply
* Re: [PATCH v7 net-next 5/8] ice: introduce TXC DPLL device and TX ref clock pin framework for E825
From: Jakub Kicinski @ 2026-05-02 17:33 UTC (permalink / raw)
To: grzegorz.nitka
Cc: Jakub Kicinski, netdev, ivecera, vadim.fedorenko, jiri, edumazet,
przemyslaw.kitszel, richardcochran, donald.hunter, linux-kernel,
arkadiusz.kubalewski, andrew+netdev, intel-wired-lan, horms,
Prathosh.Satish, anthony.l.nguyen, pabeni, davem
In-Reply-To: <20260430094238.987976-6-grzegorz.nitka@intel.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ice: introduce TXC DPLL device and TX ref clock pin framework for E825
This patch adds a new TXC DPLL device of DPLL_TYPE_GENERIC for E825
devices, together with a TXCLK pin type and two TX reference pins
(EXT_EREF0 and a fwnode-described SYNCE pin). Pin state callbacks and
TXC lock status are placeholders at this stage.
> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
> index 62f75701d652..d839b50187ba 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
[ ... ]
> @@ -3199,19 +3276,40 @@ static bool ice_dpll_is_fwnode_pin(struct ice_dpll_pin *pin)
> return !IS_ERR_OR_NULL(pin->fwnode);
> }
>
> +static bool ice_dpll_fwnode_eq(const struct fwnode_handle *a,
> + const struct fwnode_handle *b)
> +{
> + return a && b && a == b;
> +}
> +
> static void ice_dpll_pin_notify_work(struct work_struct *work)
> {
> struct ice_dpll_pin_work *w = container_of(work,
> struct ice_dpll_pin_work,
> work);
> struct ice_dpll_pin *pin, *parent = w->pin;
> + bool is_tx_synce_parent = false;
> struct ice_pf *pf = parent->pf;
> + bool is_rclk_parent = false;
> int ret;
>
> wait_for_completion(&pf->dplls.dpll_init);
> if (!test_bit(ICE_FLAG_DPLL, pf->flags))
> goto out; /* DPLL initialization failed */
>
> + /* Decide which parent we are handling, defensively checking FWNs */
[ ... ]
> + } else if (is_tx_synce_parent) {
> + /* Register TX-CLK SYNCE pin directly to TXC DPLL */
> + pin = &pf->dplls.txclks[E825_EXT_SYNCE_PIN_IDX];
> + ret = dpll_pin_register(pf->dplls.txc.dpll, pin->pin,
> + &ice_dpll_txclk_ops, pin);
> + if (ret) {
> + dev_err(ice_pf_to_dev(pf),
> + "TX SYNCE pin register failed: %pe\n",
> + ERR_PTR(ret));
> + goto drop_parent_ref;
> + }
> }
> break;
[ ... ]
> @@ -3401,6 +3524,19 @@ ice_dpll_deinit_fwnode_pins(struct ice_pf *pf, struct ice_dpll_pin *pins,
> destroy_workqueue(pf->dplls.wq);
> }
>
> +static int ice_dpll_deinit_txclk_pins(struct ice_pf *pf)
> +{
> + struct ice_dpll_pin *synce_pin = &pf->dplls.txclks[E825_EXT_SYNCE_PIN_IDX];
> + struct ice_dpll *dt = &pf->dplls.txc;
> +
> + ice_dpll_unregister_pins(dt->dpll, pf->dplls.txclks,
> + &ice_dpll_txclk_ops,
> + ARRAY_SIZE(pf->dplls.txclks));
> + ice_dpll_release_pins(&pf->dplls.txclks[E825_EXT_EREF_PIN_IDX], 1);
> + ice_dpll_deinit_fwnode_pin(synce_pin);
> + return 0;
> +}
Is there a teardown race between this function and a concurrent
ice_dpll_pin_notify_work() that is about to register the SYNCE pin on
the TXC DPLL?
The work's registration path runs:
parent->pin = fwnode_dpll_pin_find(parent->fwnode,
&parent->tracker);
...
} else if (is_tx_synce_parent) {
pin = &pf->dplls.txclks[E825_EXT_SYNCE_PIN_IDX];
ret = dpll_pin_register(pf->dplls.txc.dpll, pin->pin,
&ice_dpll_txclk_ops, pin);
while ice_dpll_deinit_txclk_pins() first scans pf->dplls.txclks[] with
ice_dpll_unregister_pins(), which skips any slot where pins[i].pin is
still NULL:
for (i = 0; i < count; i++) {
if (pins[i].hidden)
continue;
if (IS_ERR_OR_NULL(pins[i].pin))
continue;
dpll_pin_unregister(dpll, pins[i].pin, ops, &pins[i]);
}
Only afterward does it call ice_dpll_deinit_fwnode_pin(synce_pin), which
unregisters the notifier and flushes pf->dplls.wq.
If an external fwnode DPLL provider emits a DPLL_PIN_CREATED for
clk_ref_synce between the two steps, can this sequence occur?
deinit thread notifier work
------------- -------------
ice_dpll_unregister_pins(txclks)
sees txclks[1].pin == NULL
skips SYNCE slot
parent->pin = fwnode_dpll_pin_find()
dpll_pin_register(txc->dpll,
pin->pin, &ops, pin)
-> adds to txc->dpll->pin_refs
and pin->dpll_refs
ice_dpll_release_pins(EREF0, 1)
ice_dpll_deinit_fwnode_pin(synce_pin)
unregister_dpll_notifier()
flush_workqueue() /* no work pending */
dpll_pin_put(synce_pin->pin, ...)
After this, dpll_pin_put() only drops the pin refcount; it does not call
dpll_pin_unregister(). So the TXC dpll's pin_refs still contains the
SYNCE entry when ice_dpll_deinit_dpll(pf, &pf->dplls.txc, false) later
runs dpll_device_unregister() and dpll_device_put(). Does this trip
WARN_ON_ONCE(!xa_empty(&dpll->pin_refs)) in __dpll_device_put() and then
free the dpll_device while pin->dpll_refs still references it, leaving a
use-after-free on any subsequent walk of pin->dpll_refs?
Would it be safer to call ice_dpll_deinit_fwnode_pin(synce_pin) first so
the notifier is torn down and pf->dplls.wq drained before txclks[] is
scanned, or to hold pf->dplls.lock across both the deinit scan and the
work's parent->pin = ...; dpll_pin_register(...) sequence?
A similar structural race exists for the rclk path via
dpll_pin_on_pin_register(), but this patch introduces a new instance on
the direct-on-TXC SYNCE registration. The ordering in
ice_dpll_deinit_txclk_pins() appears unchanged by later patches in the
series, so the same race seems to persist beyond this commit.
^ permalink raw reply
* Re: [PATCH v7 net-next 4/8] dpll: zl3073x: allow SyncE_Ref pin state change
From: Jakub Kicinski @ 2026-05-02 17:33 UTC (permalink / raw)
To: grzegorz.nitka
Cc: Jakub Kicinski, netdev, ivecera, vadim.fedorenko, jiri, edumazet,
przemyslaw.kitszel, richardcochran, donald.hunter, linux-kernel,
arkadiusz.kubalewski, andrew+netdev, intel-wired-lan, horms,
Prathosh.Satish, anthony.l.nguyen, pabeni, davem
In-Reply-To: <20260430094238.987976-5-grzegorz.nitka@intel.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
dpll: zl3073x: allow SyncE_Ref pin state change
This patch advertises DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE on the
output-direction branch of zl3073x_pin_props_get() with the stated intent
of allowing dynamic enable/disable of the SyncE_Ref pin.
> diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
> index ac9d41d0f978..acd7061a741a 100644
> --- a/drivers/dpll/zl3073x/prop.c
> +++ b/drivers/dpll/zl3073x/prop.c
> @@ -215,6 +215,15 @@ struct zl3073x_pin_props *zl3073x_pin_props_get(struct zl3073x_dev *zldev,
>
> props->dpll_props.type = DPLL_PIN_TYPE_GNSS;
>
> + /*
> + * The SyncE_Ref pin supports enabling/disabling dynamically.
> + * Some platforms may choose to expose this through firmware
> + * configuration later. For now, advertise this capability
> + * universally since the hardware allows state toggling.
> + */
> + props->dpll_props.capabilities |=
> + DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE;
Is this capability actually supported by the output pin ops?
Looking at drivers/dpll/zl3073x/dpll.c, the output pin ops vector does
not install a .state_on_dpll_set callback:
static const struct dpll_pin_ops zl3073x_dpll_output_pin_ops = {
.direction_get = zl3073x_dpll_pin_direction_get,
.esync_get = zl3073x_dpll_output_pin_esync_get,
.esync_set = zl3073x_dpll_output_pin_esync_set,
.frequency_get = zl3073x_dpll_output_pin_frequency_get,
.frequency_set = zl3073x_dpll_output_pin_frequency_set,
.phase_adjust_get = zl3073x_dpll_output_pin_phase_adjust_get,
.phase_adjust_set = zl3073x_dpll_output_pin_phase_adjust_set,
.state_on_dpll_get = zl3073x_dpll_output_pin_state_on_dpll_get,
};
and the corresponding getter hardcodes the state to CONNECTED with no
hardware read:
zl3073x_dpll_output_pin_state_on_dpll_get(...)
{
/* If the output pin is registered then it is always connected */
*state = DPLL_PIN_STATE_CONNECTED;
return 0;
}
On the netlink side, dpll_pin_state_set() first checks the capability
bit and then requires the setter to exist:
if (!(DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE & pin->prop.capabilities)) {
NL_SET_ERR_MSG(extack, "state changing is not allowed");
return -EOPNOTSUPP;
}
...
if (!ops->state_on_dpll_set)
return -EOPNOTSUPP;
With this patch, the capability check now passes for every output pin,
but the operation then fails with -EOPNOTSUPP because no setter is
provided, and the reported state will remain CONNECTED regardless of
what userspace requested.
Should this series also add a .state_on_dpll_set callback (and a
matching hardware path, for example via zl3073x_out_state_set or the
output-enable mailbox) before advertising the capability, so the commit
subject "allow SyncE_Ref pin state change" can actually be honored?
Separately, should the scope of this change match the commit message?
The subject and the first sentence of the new comment describe a change
scoped to the SyncE_Ref pin:
* The SyncE_Ref pin supports enabling/disabling dynamically.
but the assignment lives in the generic else-branch of
zl3073x_pin_props_get() that runs for every output pin (SMA outputs,
1PPS outputs, internal-oscillator outputs, MUX outputs, etc.), and it
executes before the fwnode "connection-type" parsing below can classify
the pin as ext/gnss/int/synce/mux. The last sentence of the same
comment acknowledges this:
* universally since the hardware allows state toggling.
Would it be clearer to either gate the flag on the SyncE_Ref pin
specifically (e.g., via firmware-node property, as the comment hints
at), or to update the subject and leading comment sentence to reflect
that the capability is being advertised for all output pins?
> + /*
> + * The SyncE_Ref pin supports enabling/disabling dynamically.
> + * Some platforms may choose to expose this through firmware
> + * configuration later. For now, advertise this capability
> + * universally since the hardware allows state toggling.
> + */
This isn't a bug, but the indentation of the comment block is
inconsistent. The opening "/*" is prefixed with one tab followed by
seven spaces, while the " *" continuation lines and the closing " */"
are prefixed with two tabs, so the opening "/" does not align with the
continuation asterisks. Adjacent comment blocks in the same function
(for example "The output pin phase adjustment granularity...") use pure
tab alignment.
> + props->dpll_props.capabilities |=
> + DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE;
This isn't a bug, but props was just obtained from kzalloc_obj() and
nothing in this else-branch writes to capabilities before this point.
The symmetric input branch immediately above uses plain "=":
props->dpll_props.capabilities =
DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE |
DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE;
Would "=" be more consistent here, since there are no pre-existing bits
to merge with?
--
pw-bot: cr
^ permalink raw reply
* Re: [PATCH v7 net-next 3/8] dpll: extend pin notifier and netlink events with notification source ID
From: Jakub Kicinski @ 2026-05-02 17:31 UTC (permalink / raw)
To: Grzegorz Nitka
Cc: netdev, linux-kernel, intel-wired-lan, poros, richardcochran,
andrew+netdev, przemyslaw.kitszel, anthony.l.nguyen,
Prathosh.Satish, ivecera, jiri, arkadiusz.kubalewski,
vadim.fedorenko, donald.hunter, horms, pabeni, davem, edumazet,
Aleksandr Loktionov
In-Reply-To: <20260430094238.987976-4-grzegorz.nitka@intel.com>
On Thu, 30 Apr 2026 11:42:33 +0200 Grzegorz Nitka wrote:
> Subject: [PATCH v7 net-next 3/8] dpll: extend pin notifier and netlink events with notification source ID
As AI points out you're only adding this for the internal notification,
but the commit title mentions netlink too. If we don't need it in
netlink we can just drop the references in the commit msg?
^ permalink raw reply
* Re: [PATCH v7 net-next 3/8] dpll: extend pin notifier and netlink events with notification source ID
From: Jakub Kicinski @ 2026-05-02 17:29 UTC (permalink / raw)
To: Grzegorz Nitka
Cc: netdev, linux-kernel, intel-wired-lan, poros, richardcochran,
andrew+netdev, przemyslaw.kitszel, anthony.l.nguyen,
Prathosh.Satish, ivecera, jiri, arkadiusz.kubalewski,
vadim.fedorenko, donald.hunter, horms, pabeni, davem, edumazet,
Aleksandr Loktionov
In-Reply-To: <20260430094238.987976-4-grzegorz.nitka@intel.com>
On Thu, 30 Apr 2026 11:42:33 +0200 Grzegorz Nitka wrote:
>
> -int dpll_pin_delete_ntf(struct dpll_pin *pin)
> +int dpll_pin_delete_ntf(struct dpll_pin *pin, u64 src_clock_id)
double space
^ permalink raw reply
* Re: [PATCH v7 net-next 2/8] dpll: allow registering FW-identified pin with a different DPLL
From: Jakub Kicinski @ 2026-05-02 17:27 UTC (permalink / raw)
To: Grzegorz Nitka
Cc: netdev, linux-kernel, intel-wired-lan, poros, richardcochran,
andrew+netdev, przemyslaw.kitszel, anthony.l.nguyen,
Prathosh.Satish, ivecera, jiri, arkadiusz.kubalewski,
vadim.fedorenko, donald.hunter, horms, pabeni, davem, edumazet,
Jiri Pirko, Aleksandr Loktionov
In-Reply-To: <20260430094238.987976-3-grzegorz.nitka@intel.com>
On Thu, 30 Apr 2026 11:42:32 +0200 Grzegorz Nitka wrote:
> Relax the (module, clock_id) equality requirement when registering a
> pin identified by firmware (pin->fwnode). Some platforms associate a
> FW-described pin with a DPLL instance that differs from the pin's
> (module, clock_id) tuple. For such pins, permit registration without
> requiring the strict match. Non-FW pins still require equality.
AI asks what prevents the modules from disappearing:
Does this relaxed check expose pin->module to a use-after-free during
netlink queries?
If module A registers a firmware-described pin allocated by module B,
they will have different module pointers.
Because fwnode_dpll_pin_find() increases the pin's refcount but does
not take a reference to module B via try_module_get(), it appears module B
could be unloaded while module A still holds an active reference to the pin.
When module B unloads, its struct module memory is freed, leaving
pin->module as a dangling pointer.
A subsequent user-space Netlink query using DPLL_CMD_PIN_GET iterates over
the registered pins and calls nla_put_string() with module_name(pin->module),
which would dereference the freed module memory.
^ permalink raw reply
* Re: [PATCH iproute2-next] tc: use ll_init_map() only when needed
From: Eric Dumazet @ 2026-05-02 17:25 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: David Ahern, Stephen Hemminger, David S . Miller, Jakub Kicinski,
Paolo Abeni, netdev, eric.dumazet
In-Reply-To: <CAM0EoMmHMMsAbORxETh72c0=CyyAY+PhXReXwP-woGp31YOkHw@mail.gmail.com>
On Thu, Apr 30, 2026 at 11:20 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Thu, Apr 30, 2026 at 11:36 AM David Ahern <dsahern@kernel.org> wrote:
> >
> > Jamal: waiting for your review ...
> >
> > On 4/28/26 2:28 AM, Eric Dumazet wrote:
> > > Some setups can have thousands of devices.
> > >
> > > ll_init_map() is rather expensive for them.
> > >
> > > Only call ll_init_map() in the following cases:
> > >
> > > 1) tc runs in batch mode.
> > > 2) tc runs in monitor mode.
> > > 3) tc dumps qdiscs/classes/filters for all netdev.
> > >
> > > This greatly reduces RTNL pressure on common operations.
> > >
> > > Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
Gentle reminder, this patch has not been merged.
I have other patches coming that depend on this one.
Thanks!
^ permalink raw reply
* Re: [PATCH net-next] net/sched: taprio: prepare taprio_dump() for RTNL removal
From: Eric Dumazet @ 2026-05-02 17:22 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, Paolo Abeni
Cc: Simon Horman, Jamal Hadi Salim, Jiri Pirko, netdev, eric.dumazet
In-Reply-To: <20260501064247.2027688-1-edumazet@google.com>
On Thu, Apr 30, 2026 at 11:42 PM Eric Dumazet <edumazet@google.com> wrote:
>
> We soon will no longer hold RTNL in qdisc dumps.
>
> Add READ_ONCE()/WRITE_ONCE() annotations.
>
> Note: taprio already uses RCU to protect most of its fields.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
I think sashiko feedback is a bit wrong.
<quote>
Should the reads of q->flags in taprio_dump() also be annotated, given
that the write side in taprio_change() is now using WRITE_ONCE()?
The same expression reads q->flags twice:
if (q->flags && nla_put_u32(skb, TCA_TAPRIO_ATTR_FLAGS, q->flags))
and the earlier check in taprio_dump() also does a plain read:
if (!FULL_OFFLOAD_IS_ENABLED(q->flags) &&
nla_put_s32(skb, TCA_TAPRIO_ATTR_SCHED_CLOCKID, q->clockid))
Without an annotation, is the compiler free to reload q->flags between
the guard and the nla_put_u32() argument, so the guard could observe one
value while the emitted attribute reflects a different one once RTNL is
dropped from the dump path?
</quote>
My answer: q->flags can not be changed on a live taprio qdisc, it is
set once (and the logic is even visible in the patch diff)
<quote>
On a related note, q->clockid is read in taprio_dump() as:
nla_put_s32(skb, TCA_TAPRIO_ATTR_SCHED_CLOCKID, q->clockid)
and written plainly in taprio_parse_clockid() (q->clockid = clockid;)
and in taprio_init() (q->clockid = -1;).
Is q->clockid intentionally left out of this preparation, or should it
get READ_ONCE()/WRITE_ONCE() treatment along with q->flags and
q->txtime_delay? If it is intentional, would a note in the commit
message help explain why clockid is exempt?
</quote>
Same answer. q->clockid can not change on a live taprio qdisc.
^ permalink raw reply
* Re: [PATCH 1/2] net: wan: fsl_ucc_hdlc: fix uhdlc_memclean
From: patchwork-bot+netdevbpf @ 2026-05-02 17:20 UTC (permalink / raw)
To: Holger Brunck; +Cc: netdev, linuxppc-dev, andrew+netdev, chleroy, qiang.zhao
In-Reply-To: <20260429114208.941011-1-holger.brunck@hitachienergy.com>
Hello:
This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Wed, 29 Apr 2026 13:42:07 +0200 you wrote:
> Unmapping of uf_regs is done from ucc_fast_free and doesn't need to be
> done explicitly. If already unmapped ucc_fast_free will crash.
>
> Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
> ---
> drivers/net/wan/fsl_ucc_hdlc.c | 5 -----
> 1 file changed, 5 deletions(-)
Here is the summary with links:
- [1/2] net: wan: fsl_ucc_hdlc: fix uhdlc_memclean
https://git.kernel.org/netdev/net/c/1a57efe250a1
- [2/2] net: wan: fsl_ucc_hdlc: fix ucc_hdlc_remove
https://git.kernel.org/netdev/net/c/851bba8068d1
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH net] net: eth: fbnic: Fix addr validation in pcs write
From: Mike Marciniszyn @ 2026-05-02 17:16 UTC (permalink / raw)
To: Andrew Lunn
Cc: Simon Horman, Alexander Duyck, Jakub Kicinski, kernel-team,
Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni, netdev,
linux-kernel, stable
In-Reply-To: <f500e75a-5672-4c62-b2b1-04f59bed3368@lunn.ch>
On Sat, May 02, 2026 at 04:02:04PM +0200, Andrew Lunn wrote:
> On Sat, May 02, 2026 at 05:45:08AM -0400, Mike Marciniszyn wrote:
> > On Fri, May 01, 2026 at 02:46:36PM +0100, Simon Horman wrote:
> > > On Wed, Apr 29, 2026 at 11:00:49AM -0400, mike.marciniszyn@gmail.com wrote:
> > > > From: "Mike Marciniszyn (Meta)" <mike.marciniszyn@gmail.com>
> > > >
> > > > This patch contains a fix for addr validation in fbnic_mdio_write_pcs().
> > >
> > > Hi Mike,
> > >
> > > I think this warrants a bit more explanation: Why should addr 2 be
> > > accepted? What happens from a user-perspective when it is not?
> > >
> >
> > The DW IP part has two distinct PCS address ranges cooresponding
> > to the C45 PCS registers.
> >
> > The shim translates the PCS mmd/addr/regno into specific CSR writes
> > to one of two zero-relative addr values into one of those two
> > ranges.
> >
> > This patch fixes a one off in the test that could allow an invalid
> > CSR write if an addr == 2 was called.
>
> Stable runs say:
>
> It must either fix a real bug that bothers people, ...
>
> Can this bug be triggered with the current driver? Are there any
> noticeable effects? How would somebody inside Meta know they need this
> fix? This should be included in the commit message.
>
> Andrew
Thanks Andrew!
I am working inside Meta with Alex and Kuba. I noticed the one off when
doing the patch that reworks the shim.
As to a real impact, that depends on the part2 series, but before that
series no one would care, which is why I had in as part of
the patch 1 series.
Without the follow on work, I suspect that no one cares or would
see any issue as I have yet to present the xpcs changes in part2.
Perhaps the best thing to do is beef up the commit and remove the
stable Cc, leaving the Fixes linkage?
Mike
^ 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