netdev - RE: [RFC PATCH net-next 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <DM6PR12MB451643692E69D94D6A7484D8D87D2@DM6PR12MB4516.namprd12.prod.outlook.com>
Date: Tue, 30 Jan 2024 07:55:32 +0000
From: Danielle Ratson <danieller@...dia.com>
To: Russell King <linux@...linux.org.uk>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"corbet@....net" <corbet@....net>, "sdf@...gle.com" <sdf@...gle.com>,
	"kory.maincent@...tlin.com" <kory.maincent@...tlin.com>,
	"maxime.chevallier@...tlin.com" <maxime.chevallier@...tlin.com>,
	"vladimir.oltean@....com" <vladimir.oltean@....com>,
	"przemyslaw.kitszel@...el.com" <przemyslaw.kitszel@...el.com>,
	"ahmed.zaki@...el.com" <ahmed.zaki@...el.com>, "richardcochran@...il.com"
	<richardcochran@...il.com>, "shayagr@...zon.com" <shayagr@...zon.com>,
	"paul.greenwalt@...el.com" <paul.greenwalt@...el.com>, "jiri@...nulli.us"
	<jiri@...nulli.us>, "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, mlxsw
	<mlxsw@...dia.com>, Petr Machata <petrm@...dia.com>, Ido Schimmel
	<idosch@...dia.com>
Subject: RE: [RFC PATCH net-next 7/9] ethtool: cmis_cdb: Add a layer for
 supporting CDB commands

> > +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.
> 

Will fix, thanks!

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