Re: [PATCH net-next v14 6/7] r8169: move struct ethtool_ops

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] drivers/net/ethernet/realtek/r8169_main.c: this patch is pure
  code motion that relocates the `rtl8169_ethtool_ops` table ~4080…

--

r8169: move struct ethtool_ops

This is pure code motion: the rtl8169_ethtool_ops table is removed from
its place right after the ethtool callback block and re-added further
down the file, between r8169_init_napi() and rtl_init_one(). The stated
reason is to allow later additions of rtl8169_get_channels and
rtl8169_set_channels to be referenced without forward declarations.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index ad360f145076..3be6f6e708f2 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -2637,34 +2637,6 @@ static int rtl8169_nway_reset(struct net_device *dev)
>  	return phylink_ethtool_nway_reset(tp->phylink);
>  }
>  
> -static const struct ethtool_ops rtl8169_ethtool_ops = {
> -	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
> -				     ETHTOOL_COALESCE_MAX_FRAMES,
> -	.get_drvinfo		= rtl8169_get_drvinfo,

[Severity: Low]
This isn't a bug, but does the commit message rationale match the rest of
the series?

The message says the move is needed "so that subsequent additions of
rtl8169_get_channels and rtl8169_set_channels can be referenced from the
ops struct without needing forward declarations".

The follow-up patch "r8169: add get_channel support for ethtool" adds only
rtl8169_get_channels(), and its body touches nothing defined after
rtl8169_nway_reset():

	static void rtl8169_get_channels(struct net_device *dev,
					 struct ethtool_channels *ch)
	{
		struct rtl8169_private *tp = netdev_priv(dev);

		if (tp->irq_nvecs > 1) {
			ch->max_rx = tp->hw_supp_num_rx_queues;
			ch->max_tx = 1;
			ch->rx_count = tp->num_rx_rings;
			ch->tx_count = 1;
		} else {
			ch->max_combined = 1;
			ch->combined_count = 1;
		}
	}

It only uses netdev_priv() and the tp->irq_nvecs, tp->hw_supp_num_rx_queues
and tp->num_rx_rings members of struct rtl8169_private, all of which are
declared far earlier in the file. Could rtl8169_get_channels() simply be
defined just above the original location of rtl8169_ethtool_ops, making this
move unnecessary and avoiding any forward declaration?

Also, rtl8169_set_channels is named as a reason for the move but is never
added anywhere in the series. Should that part of the message be dropped?

>  [ ... ]
> -	.get_eth_mac_stats	= rtl8169_get_eth_mac_stats,
> -	.get_eth_ctrl_stats	= rtl8169_get_eth_ctrl_stats,
> -};
> -
>  static const struct rtl_chip_info *rtl8169_get_chip_version(u32 xid, bool gmii)
>  {
>  	/* Chips combining a 1Gbps MAC with a 100Mbps PHY */
> @@ -6741,6 +6713,34 @@ static void r8169_init_napi(struct rtl8169_private *tp)
>  	}
>  }
>  
> +static const struct ethtool_ops rtl8169_ethtool_ops = {
> +	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
> +				     ETHTOOL_COALESCE_MAX_FRAMES,
> +	.get_drvinfo		= rtl8169_get_drvinfo,

[Severity: Low]
The new position places the table roughly four thousand lines away from
every callback it references, between r8169_init_napi() and the PCI probe
function rtl_init_one(). Any future ethtool callback added next to the
existing callback block (rtl8169_get_drvinfo() .. rtl8169_nway_reset())
would then be the one separated from the table. Is keeping the table
adjacent to the callbacks preferable here, given the churn to git blame in
a driver that regularly takes stable backports?

> +	.get_eth_mac_stats	= rtl8169_get_eth_mac_stats,
> +	.get_eth_ctrl_stats	= rtl8169_get_eth_ctrl_stats,
> +};
> +
>  static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	const struct rtl_chip_info *chip;

Since the diff is byte-identical motion with no behaviour change, would it
help to add the customary "No functional change intended." line to the
commit message?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918061923.313-1-javen_xu%40realsil.com.cn