Re: [PATCH net-next 5/5] net: txgbe: support getting module EEPROM by page

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 13/11/2025 02:21, Jiawen Wu wrote:
> On Wed, Nov 12, 2025 8:49 PM, Vadim Fedorenko wrote:
>> On 12/11/2025 05:58, Jiawen Wu wrote:
>>> Getting module EEPROM has been supported in TXGBE SP devices, since SFP
>>> driver has already implemented it.
>>>
>>> Now add support to read module EEPROM for AML devices. Towards this, add
>>> a new firmware mailbox command to get the page data.
>>>
>>> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>

[...]

>>> +int txgbe_read_eeprom_hostif(struct wx *wx,
>>> +			     struct txgbe_hic_i2c_read *buffer,
>>> +			     u32 length, u8 *data)
>>> +{
>>> +	u32 buf_size = sizeof(struct txgbe_hic_i2c_read) - sizeof(u8);
>>> +	u32 total_len = buf_size + length;
>>> +	u32 dword_len, value, i;
>>> +	u8 local_data[256];
>>> +	int err;
>>> +
>>> +	if (total_len > sizeof(local_data))
>>> +		return -EINVAL;
>>
>> if it's really possible? SFF pages are 128 bytes, you reserve 256 bytes
>> of local buffer. What are you protecting from?
> 
> It can be changed to 128 + sizeof(struct txgbe_hic_i2c_read).

My point is why do you need this check at all?
It looks more like defensive programming which is discouraged in kernel.

> 
>>
>>> +
>>> +	buffer->hdr.cmd = FW_READ_EEPROM_CMD;
>>> +	buffer->hdr.buf_len = sizeof(struct txgbe_hic_i2c_read) -
>>> +			      sizeof(struct wx_hic_hdr);
>>> +	buffer->hdr.cmd_or_resp.cmd_resv = FW_CEM_CMD_RESERVED;
>>> +
>>> +	err = wx_host_interface_command(wx, (u32 *)buffer,
>>> +					sizeof(struct txgbe_hic_i2c_read),
>>> +					WX_HI_COMMAND_TIMEOUT, false);
>>> +	if (err != 0)
>>> +		return err;
>>> +
>>> +	dword_len = (total_len + 3) / 4;
>>
>> round_up()?
>>
>>> +
>>> +	for (i = 0; i < dword_len; i++) {
>>> +		value = rd32a(wx, WX_FW2SW_MBOX, i);
>>> +		le32_to_cpus(&value);
>>> +
>>> +		memcpy(&local_data[i * 4], &value, 4);
>>> +	}
>>
>> the logic here is not clear from the first read of the code. effectively
>> in the reply you have the same txgbe_hic_i2c_read struct but without
>> data field, which is obviously VLA, but then you simply skip the result
>> of read of txgbe_hic_i2c_read and only provide the real data back to the
>> caller. Maybe you can organize the code the way it can avoid double copying?
> 
> Because the length of real data is variable, now it could be 1 or 128. But the total
> length of the command buffer is DWORD aligned. So we designed only a 1-byte
> data field in struct txgbe_hic_i2c_read, to avoid redundant reading and writing
> during the SW-FW interaction.
> 
> For 1-byte data, wx_host_interface_command() can be used to set 'return_data'
> to true, then page->data = buffer->data. For other cases, I think it would be more
> convenient to read directly from the mailbox registers.

With such design you always have your return data starting at offset of
15, which is absolutely unaligned. And then it needs more buffer
dancing.

> 
>>
>>> +
>>> +	memcpy(data, &local_data[buf_size], length);
>>> +	return 0;
>>> +}
>>> +