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