| 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
| ||
|
Message-ID: <001401dc5444$3e897f60$bb9c7e20$@trustnetic.com>
Date: Thu, 13 Nov 2025 10:21:39 +0800
From: Jiawen Wu <jiawenwu@...stnetic.com>
To: "'Vadim Fedorenko'" <vadim.fedorenko@...ux.dev>,
<netdev@...r.kernel.org>,
"'Andrew Lunn'" <andrew+netdev@...n.ch>,
"'David S. Miller'" <davem@...emloft.net>,
"'Eric Dumazet'" <edumazet@...gle.com>,
"'Jakub Kicinski'" <kuba@...nel.org>,
"'Paolo Abeni'" <pabeni@...hat.com>,
"'Russell King'" <linux@...linux.org.uk>,
"'Simon Horman'" <horms@...nel.org>,
"'Jacob Keller'" <jacob.e.keller@...el.com>,
<netdev@...r.kernel.org>,
"'Andrew Lunn'" <andrew+netdev@...n.ch>,
"'David S. Miller'" <davem@...emloft.net>,
"'Eric Dumazet'" <edumazet@...gle.com>,
"'Jakub Kicinski'" <kuba@...nel.org>,
"'Paolo Abeni'" <pabeni@...hat.com>,
"'Russell King'" <linux@...linux.org.uk>,
"'Simon Horman'" <horms@...nel.org>,
"'Jacob Keller'" <jacob.e.keller@...el.com>
Cc: "'Mengyuan Lou'" <mengyuanlou@...-swift.com>,
"'Mengyuan Lou'" <mengyuanlou@...-swift.com>
Subject: RE: [PATCH net-next 5/5] net: txgbe: support getting module EEPROM by page
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@...stnetic.com>
> > ---
> > .../net/ethernet/wangxun/txgbe/txgbe_aml.c | 37 +++++++++++++++++++
> > .../net/ethernet/wangxun/txgbe/txgbe_aml.h | 3 ++
> > .../ethernet/wangxun/txgbe/txgbe_ethtool.c | 30 +++++++++++++++
> > .../net/ethernet/wangxun/txgbe/txgbe_type.h | 11 ++++++
> > 4 files changed, 81 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
> > index 3b6ea456fbf7..12900abfa91a 100644
> > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
> > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
> > @@ -73,6 +73,43 @@ int txgbe_test_hostif(struct wx *wx)
> > WX_HI_COMMAND_TIMEOUT, true);
> > }
> >
> > +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).
>
> > +
> > + 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.
>
> > +
> > + memcpy(data, &local_data[buf_size], length);
> > + return 0;
> > +}
> > +
> > static int txgbe_identify_module_hostif(struct wx *wx,
> > struct txgbe_hic_get_module_info *buffer)
> > {
> > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
> > index 7c8fa48e68d3..4f6df0ee860b 100644
> > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
> > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
> > @@ -7,6 +7,9 @@
> > void txgbe_gpio_init_aml(struct wx *wx);
> > irqreturn_t txgbe_gpio_irq_handler_aml(int irq, void *data);
> > int txgbe_test_hostif(struct wx *wx);
> > +int txgbe_read_eeprom_hostif(struct wx *wx,
> > + struct txgbe_hic_i2c_read *buffer,
> > + u32 length, u8 *data);
> > int txgbe_set_phy_link(struct wx *wx);
> > int txgbe_identify_module(struct wx *wx);
> > void txgbe_setup_link(struct wx *wx);
> > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
> > index f553ec5f8238..1f60121fe73c 100644
> > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
> > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
> > @@ -10,6 +10,7 @@
> > #include "../libwx/wx_lib.h"
> > #include "txgbe_type.h"
> > #include "txgbe_fdir.h"
> > +#include "txgbe_aml.h"
> > #include "txgbe_ethtool.h"
> >
> > int txgbe_get_link_ksettings(struct net_device *netdev,
> > @@ -534,6 +535,34 @@ static int txgbe_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
> > return ret;
> > }
> >
> > +static int
> > +txgbe_get_module_eeprom_by_page(struct net_device *netdev,
> > + const struct ethtool_module_eeprom *page_data,
> > + struct netlink_ext_ack *extack)
> > +{
> > + struct wx *wx = netdev_priv(netdev);
> > + struct txgbe_hic_i2c_read buffer;
> > + int err;
> > +
> > + if (!test_bit(WX_FLAG_SWFW_RING, wx->flags))
> > + return -EOPNOTSUPP;
> > +
> > + buffer.length = (__force u32)cpu_to_be32(page_data->length);
> > + buffer.offset = (__force u32)cpu_to_be32(page_data->offset);
>
> maybe txgbe_hic_i2c_read::length and txgbe_hic_i2c_read::offset should
> be __be32 ?
Sure.
>
> > + buffer.page = page_data->page;
> > + buffer.bank = page_data->bank;
> > + buffer.i2c_address = page_data->i2c_address;
> > +
> > + err = txgbe_read_eeprom_hostif(wx, &buffer, page_data->length,
> > + page_data->data);
> > + if (err) {
> > + wx_err(wx, "Failed to read module EEPROM\n");
> > + return err;
> > + }
> > +
> > + return page_data->length;
> > +}
> > +
> > static const struct ethtool_ops txgbe_ethtool_ops = {
> > .supported_coalesce_params = ETHTOOL_COALESCE_USECS |
> > ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ |
> > @@ -568,6 +597,7 @@ static const struct ethtool_ops txgbe_ethtool_ops = {
> > .set_msglevel = wx_set_msglevel,
> > .get_ts_info = wx_get_ts_info,
> > .get_ts_stats = wx_get_ptp_stats,
> > + .get_module_eeprom_by_page = txgbe_get_module_eeprom_by_page,
> > };
> >
> > void txgbe_set_ethtool_ops(struct net_device *netdev)
> > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> > index e72edb9ef084..3d1bec39d74c 100644
> > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> > @@ -353,6 +353,7 @@ void txgbe_do_reset(struct net_device *netdev);
> > #define FW_PHY_GET_LINK_CMD 0xC0
> > #define FW_PHY_SET_LINK_CMD 0xC1
> > #define FW_GET_MODULE_INFO_CMD 0xC5
> > +#define FW_READ_EEPROM_CMD 0xC6
> >
> > struct txgbe_sff_id {
> > u8 identifier; /* A0H 0x00 */
> > @@ -394,6 +395,16 @@ struct txgbe_hic_ephy_getlink {
> > u8 resv[6];
> > };
> >
> > +struct txgbe_hic_i2c_read {
> > + struct wx_hic_hdr hdr;
> > + u32 offset;
> > + u32 length;
> > + u8 page;
> > + u8 bank;
> > + u8 i2c_address;
> > + u8 data;
> > +};
> you removed struct txgbe_hic_i2c_read in the patch 2 just to introduce
> it in this patch?
Yes, the name of these structure would be more appropriate.
>
> > +
> > #define NODE_PROP(_NAME, _PROP) \
> > (const struct software_node) { \
> > .name = _NAME, \
>
>
Powered by blists - more mailing lists