Received-SPF: pass (google.com: domain of linux-kernel+bounces-35763-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 23 Jan 2024 17:17:46 +0000
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Danielle Ratson <danieller@nvidia.com>
Cc: netdev@vger.kernel.org, davem@davemloft.net, edumazet@google.com,
	kuba@kernel.org, pabeni@redhat.com, corbet@lwn.net, sdf@google.com,
	kory.maincent@bootlin.com, maxime.chevallier@bootlin.com,
	vladimir.oltean@nxp.com, przemyslaw.kitszel@intel.com,
	ahmed.zaki@intel.com, richardcochran@gmail.com, shayagr@amazon.com,
	paul.greenwalt@intel.com, jiri@resnulli.us,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	mlxsw@nvidia.com, petrm@nvidia.com, idosch@nvidia.com
Subject: Re: [RFC PATCH net-next 7/9] ethtool: cmis_cdb: Add a layer for
 supporting CDB commands
Message-ID: <Za/0um6oUo3CQaR2@shell.armlinux.org.uk>
References: <20240122084530.32451-1-danieller@nvidia.com>
 <20240122084530.32451-8-danieller@nvidia.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240122084530.32451-8-danieller@nvidia.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>

On Mon, Jan 22, 2024 at 10:45:28AM +0200, Danielle Ratson wrote:
> +int ethtool_cmis_page_init(struct ethtool_module_eeprom *page_data,
> +			   u8 page, u32 offset, u32 length)
> +{
> +	page_data->page = page;
> +	page_data->offset = offset;
> +	page_data->length = length;
> +	page_data->i2c_address = ETHTOOL_CMIS_CDB_PAGE_I2C_ADDR;
> +	page_data->data = kmalloc(page_data->length, GFP_KERNEL);
> +	if (!page_data->data)
> +		return -ENOMEM;

Hmm, so every use is forced to use kmalloc() even when it's just one
byte? That seems rather wasteful.

> +/* See section 9.4.1 "CMD 0040h: Module Features" in CMIS standard revision 5.2.
> + * struct cmis_cdb_module_features_rpl is structured layout of the flat
> + * array, ethtool_cmis_cdb_rpl::payload.
> + */
> +struct cmis_cdb_module_features_rpl {
> +	u8	resv1[CMIS_CDB_MODULE_FEATURES_RESV_DATA];
> +	__be16	max_completion_time;
> +};

Does this structure need to be packed? I would suggest it does to
ensure that the __be16 is correctly placed after the 34 bytes of u8.

Overall, I think the idea of always kmalloc()ing the data is a bad idea
at the moment. We have no implementations that DMA to/from this buffer,
and it means extra cycles spent, and an extra failure point each time
we want to do a CMIS command.

It also introduces extra complexity, where we could just be passing
a pointer to a function local variable or function local structure.

Unless we decide that the data pointer should be DMA-able from (in
which case, that needs documenting as such) then I would suggest
getting rid of the extra kmalloc()...kfree() bits.

Thanks.

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