Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 7 Sep 2023 11:00:24 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Maxime Chevallier <maxime.chevallier@bootlin.com>
Cc:     davem@davemloft.net, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, Andrew Lunn <andrew@lunn.ch>,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Vladimir Oltean <vladimir.oltean@nxp.com>,
        Oleksij Rempel <linux@rempel-privat.de>,
        =?iso-8859-1?Q?Nicol=F2?= Veronese <nicveronese@gmail.com>,
        thomas.petazzoni@bootlin.com,
        Christophe Leroy <christophe.leroy@csgroup.eu>
Subject: Re: [RFC PATCH net-next 4/7] net: ethtool: add a netlink command to
 list PHYs
Message-ID: <ZPmfOOsqoO02AcBH@shell.armlinux.org.uk>
References: <20230907092407.647139-1-maxime.chevallier@bootlin.com>
 <20230907092407.647139-5-maxime.chevallier@bootlin.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230907092407.647139-5-maxime.chevallier@bootlin.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Thu, Sep 07, 2023 at 11:24:02AM +0200, Maxime Chevallier wrote:
> +#define PHY_MAX_ENTRIES	16
> +
> +struct phy_list_reply_data {
> +	struct ethnl_reply_data		base;
> +	u8 n_phys;
> +	u32 phy_indices[PHY_MAX_ENTRIES];

Please could you detail the decision making behind 16 entries - is this
arbitary or based on something?

Also, please consider what we should do if we happen to have more than
16 entries.

Finally, using u8 before an array of u32 can leave 3 bytes of padding.
It would be better to use u32 for n_phys to avoid that padding.

> +	mutex_lock(&phy_ns->ns_lock);
> +	list_for_each_entry(phydev, &phy_ns->phys, node)
> +		data->phy_indices[data->n_phys++] = phydev->phyindex;

I think this loop should limit its iterations to ensure that the
array can't overflow.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!