| 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: <0be63e70-74bb-465a-a933-0258a45033a8@intel.com>
Date: Mon, 13 Jan 2025 16:13:05 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Jiawen Wu <jiawenwu@...stnetic.com>
CC: <mengyuanlou@...-swift.com>, <andrew+netdev@...n.ch>,
<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
<pabeni@...hat.com>, <linux@...linux.org.uk>, <horms@...nel.org>,
<netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 1/2] net: txgbe: Add basic support for new AML
devices
On 1/13/25 11:31, Jiawen Wu wrote:
> There is a new 40/25/10 Gigabit Ethernet device.
>
> To support basic functions, PHYLINK is temporarily skipped as it is
> intended to implement these configurations in the firmware. And the
> associated link IRQ is also skipped.
>
> And Implement the new SW-FW interaction interface, which use 64 Byte
> message buffer.
>
> Signed-off-by: Jiawen Wu <jiawenwu@...stnetic.com>
> ---
> Changes in v2:
> - Add missing 40G devide IDs
> - Add condition for wx->do_reset != NULL
> ---
> .../net/ethernet/wangxun/libwx/wx_ethtool.c | 44 +++-
> drivers/net/ethernet/wangxun/libwx/wx_hw.c | 209 +++++++++++++++---
> drivers/net/ethernet/wangxun/libwx/wx_lib.c | 25 ++-
> drivers/net/ethernet/wangxun/libwx/wx_type.h | 27 ++-
> drivers/net/ethernet/wangxun/txgbe/txgbe_hw.c | 6 +
> .../net/ethernet/wangxun/txgbe/txgbe_irq.c | 7 +
> .../net/ethernet/wangxun/txgbe/txgbe_main.c | 43 +++-
> .../net/ethernet/wangxun/txgbe/txgbe_phy.c | 6 +
> .../net/ethernet/wangxun/txgbe/txgbe_type.h | 14 ++
> 9 files changed, 328 insertions(+), 53 deletions(-)
> +static int wx_host_interface_command_r(struct wx *wx, u32 *buffer,
> + u32 length, u32 timeout, bool return_data)
> +{
> + struct wx_hic_hdr *send_hdr = (struct wx_hic_hdr *)buffer;
> + u32 hdr_size = sizeof(struct wx_hic_hdr);
> + struct wx_hic_hdr *recv_hdr;
> + int status = 0;
"err" is a better name than "status"
> + u32 dword_len;
> + u16 buf_len;
> + u8 send_cmd;
> + u32 i, bi;
> +
> + /* wait max to 50ms to get lock */
> + WARN_ON(in_interrupt());
the comment does not belong here (@timeout is a param, not a const=50ms)
the warning would be better left to be triggered by lockdep
(sleeping in atomic context is reported then)
> + while (test_and_set_bit(WX_STATE_SWFW_BUSY, wx->state)) {
> + timeout--;
> + if (!timeout)
> + return -ETIMEDOUT;
it is rather
ETIME 62 Timer expired
not
ETIMEDOUT 110 Connection timed out
> + usleep_range(1000, 2000);
> + }
> +
> + /* index to unique seq id for each mbox message */
> + send_hdr->index = wx->swfw_index;
> + send_cmd = send_hdr->cmd;
> +
> + dword_len = length >> 2;
> + /* write data to SW-FW mbox array */
> + for (i = 0; i < dword_len; i++) {
> + wr32a(wx, WX_SW2FW_MBOX, i, (__force u32)cpu_to_le32(buffer[i]));
> + /* write flush */
> + rd32a(wx, WX_SW2FW_MBOX, i);
do you really need to flush all registers?
> + }
> +
> + /* generate interrupt to notify FW */
> + wr32m(wx, WX_SW2FW_MBOX_CMD, WX_SW2FW_MBOX_CMD_VLD, 0);
> + wr32m(wx, WX_SW2FW_MBOX_CMD, WX_SW2FW_MBOX_CMD_VLD, WX_SW2FW_MBOX_CMD_VLD);
> +
> + dword_len = hdr_size >> 2;
> +
> + /* polling reply from FW */
> + timeout = 50;
> + do {
> + timeout--;
> + usleep_range(1000, 2000);
> +
> + /* read hdr */
> + for (bi = 0; bi < dword_len; bi++)
> + buffer[bi] = rd32a(wx, WX_FW2SW_MBOX, bi);
no need for le32_to_cpu()?
(if so, reexamine whole patch)
> +
> + /* check hdr */
> + recv_hdr = (struct wx_hic_hdr *)buffer;
> + if (recv_hdr->cmd == send_cmd &&
> + recv_hdr->index == wx->swfw_index)
> + break;
> + } while (timeout);
> +
> + if (!timeout) {
you will enter here when operation suceeded after exactly 50 steps
please check inlcude/linux/iopoll.h for ready wrappers instead of
reinventing them
> + wx_err(wx, "Polling from FW messages timeout, cmd: 0x%x, index: %d\n",
> + send_cmd, wx->swfw_index);
> + status = -ETIMEDOUT;
> + goto rel_out;
> + }
> +
> + /* expect no reply from FW then return */
> + if (!return_data)
> + goto rel_out;
> +
> + /* If there is any thing in data position pull it in */
> + buf_len = recv_hdr->buf_len;
> + if (buf_len == 0)
> + goto rel_out;
> +
> + if (length < buf_len + hdr_size) {
> + wx_err(wx, "Buffer not large enough for reply message.\n");
> + status = -EFAULT;
> + goto rel_out;
> + }
> +
> + /* Calculate length in DWORDs, add 3 for odd lengths */
> + dword_len = (buf_len + 3) >> 2;
> + for (; bi <= dword_len; bi++)
> + buffer[bi] = rd32a(wx, WX_FW2SW_MBOX, bi);
> +
> +rel_out:
> + /* index++, index replace wx_hic_hdr.checksum */
> + if (send_hdr->index == WX_HIC_HDR_INDEX_MAX)
> + wx->swfw_index = 0;
> + else
> + wx->swfw_index = send_hdr->index + 1;
> +
> + clear_bit(WX_STATE_SWFW_BUSY, wx->state);
> + return status;
> +}
> +
> +/**
> + * wx_host_interface_command - Issue command to manageability block
> + * @wx: pointer to the HW structure
> + * @buffer: contains the command to write and where the return status will
> + * be placed
> + * @length: length of buffer, must be multiple of 4 bytes
> + * @timeout: time in ms to wait for command completion
> + * @return_data: read and return data from the buffer (true) or not (false)
> + * Needed because FW structures are big endian and decoding of
In other places you were using cpu_to_le32(), this comment seems to
contradict that
> + * these fields can be 8 bit or 16 bit based on command. Decoding
> + * is not easily understood without making a table of commands.
> + * So we will leave this up to the caller to read back the data
> + * in these cases.
> + **/
> +int wx_host_interface_command(struct wx *wx, u32 *buffer,
[...]
> @@ -716,7 +731,8 @@ struct wx_thermal_sensor_data {
> enum wx_mac_type {
> wx_mac_unknown = 0,
> wx_mac_sp,
> - wx_mac_em
> + wx_mac_em,
> + wx_mac_aml
always add comma (,) at the end of enums that are expected to be
extended, to avoid git-blame churn as here
> };
>
> enum sp_media_type {
> @@ -1026,10 +1042,12 @@ struct wx_hw_stats {
>
> enum wx_state {
> WX_STATE_RESETTING,
> + WX_STATE_SWFW_BUSY,
> WX_STATE_NBITS, /* must be last */
BTW the entries expected to be least should be not ended by
the guard
> };
Powered by blists - more mailing lists