| 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
| ||
|
Date: Mon, 24 Jul 2023 17:01:24 +0200
From: Simon Horman <simon.horman@...igine.com>
To: nick.hawkins@....com
Cc: verdun@....com, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
netdev@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 4/5] net: hpe: Add GXP UMAC Driver
On Fri, Jul 21, 2023 at 04:20:43PM -0500, nick.hawkins@....com wrote:
...
Hi Nick,
> diff --git a/drivers/net/ethernet/hpe/gxp-umac.c b/drivers/net/ethernet/hpe/gxp-umac.c
...
> +static void umac_set_mac_address(struct net_device *ndev, void *p_addr)
> +{
> + struct umac_priv *umac = netdev_priv(ndev);
> + char *addr = (char *)p_addr;
> + unsigned int value;
> +
> + /* update address to register */
> + value = addr[0] << 8 | addr[1];
> + writel(value, umac->base + UMAC_MAC_ADDR_HI);
> + value = addr[2] << 8 | addr[3];
> + writel(value, umac->base + UMAC_MAC_ADDR_MID);
> + value = addr[4] << 8 | addr[5];
> + writel(value, umac->base + UMAC_MAC_ADDR_LO);
> +}
> +
> +static int umac_eth_mac_addr(struct net_device *ndev, void *p)
> +{
> + int ret;
> + struct sockaddr *addr = p;
Please use reverse xmas tree - longest like to shortest - for
new Networking code. Likewise in some other places in this patch/series.
This tool can be helpful in this regard.
https://github.com/ecree-solarflare/xmastree
...
> +static int umac_init_hw(struct net_device *ndev)
> +{
> + struct umac_priv *umac = netdev_priv(ndev);
> + unsigned int value;
> +
> + /* initialize tx and rx rings to first entry */
> + writel(0, umac->base + UMAC_RING_PTR);
> +
> + /* clear the missed bit */
> + writel(0, umac->base + UMAC_CLEAR_STATUS);
> +
> + /* disable checksum generation */
> + writel(0, umac->base + UMAC_CKSUM_CONFIG);
> +
> + /* write the ring size register */
> + value = ((UMAC_RING_SIZE_256 << UMAC_TX_RING_SIZE_SHIFT) &
> + UMAC_TX_RING_SIZE_MASK) |
> + ((UMAC_RING_SIZE_256 << UMAC_RX_RING_SIZE_SHIFT) &
> + UMAC_RX_RING_SIZE_MASK);
> + writel(value, umac->base + UMAC_RING_SIZE);
> +
> + /* write rx ring base address */
> + writel(cpu_to_le32(umac->rx_descs_dma_addr),
> + umac->base + UMAC_RX_RING_ADDR);
It is my understanding that writel will convert the value
from host byte order to little endien. If so then pre-converting value
seems incorrect. Perhaps this should be:
writel(umac->rx_descs_dma_addr, umac->base + UMAC_RX_RING_ADDR);
Flagged by Sparse.
> +
> + /* write tx ring base address */
> + writel(cpu_to_le32(umac->tx_descs_dma_addr),
> + umac->base + UMAC_TX_RING_ADDR);
Ditto.
> +
> + /* write burst size */
> + writel(0x22, umac->base + UMAC_DMA_CONFIG);
> +
> + umac_channel_disable(umac);
> +
> + /* disable clocks and gigabit mode (leave channels disabled) */
> + value = readl(umac->base + UMAC_CONFIG_STATUS);
> + value &= 0xfffff9ff;
> + writel(value, umac->base + UMAC_CONFIG_STATUS);
> + udelay(2);
> +
> + if (umac->use_ncsi) {
> + /* set correct tx clock */
> + value &= UMAC_CFG_TX_CLK_EN;
> + value &= ~UMAC_CFG_GTX_CLK_EN;
> + value &= ~UMAC_CFG_GIGABIT_MODE; /* RMII mode */
> + value |= UMAC_CFG_FULL_DUPLEX; /* full duplex */
> + } else {
> + if (ndev->phydev->duplex)
> + value |= UMAC_CFG_FULL_DUPLEX;
> + else
> + value &= ~UMAC_CFG_FULL_DUPLEX;
> +
> + if (ndev->phydev->speed == SPEED_1000) {
> + value &= ~UMAC_CFG_TX_CLK_EN;
> + value |= UMAC_CFG_GTX_CLK_EN;
> + value |= UMAC_CFG_GIGABIT_MODE;
> + } else {
> + value |= UMAC_CFG_TX_CLK_EN;
> + value &= ~UMAC_CFG_GTX_CLK_EN;
> + value &= ~UMAC_CFG_GIGABIT_MODE;
> + }
> + }
> + writel(value, umac->base + UMAC_CONFIG_STATUS);
> + udelay(2);
> +
> + umac_channel_enable(umac);
> +
> + return 0;
> +}
...
> diff --git a/drivers/net/ethernet/hpe/gxp-umac.h b/drivers/net/ethernet/hpe/gxp-umac.h
...
> +struct umac_rx_desc_entry {
> + u32 dmaaddress; // Start address for DMA operationg
1. operationg -> operating
2. It appears that this field is used to hold __le32 values.
Perhaps it's type should be __le32.
As is, Sparse complains about this.
> + u16 status; // Packet tx status and ownership flag
> + u16 count; // Number of bytes received
> + u16 checksum; // On-the-fly packet checksum
> + u16 control; // Checksum-in-time flag
> + u32 reserved;
> +} __aligned(16);
> +
> +struct umac_rx_descs {
> + struct umac_rx_desc_entry entrylist[UMAC_MAX_RX_DESC_ENTRIES];
> + u8 framelist[UMAC_MAX_RX_DESC_ENTRIES][UMAC_MAX_RX_FRAME_SIZE];
> +} __packed;
> +
> +struct umac_tx_desc_entry {
> + u32 dmaaddress; // Start address for DMA operationg
Ditto.
> + u16 status; // Packet rx status, type, and ownership flag
> + u16 count; // Number of bytes received
> + u32 cksumoffset; // Specifies where to place packet checksum
> + u32 reserved;
> +} __aligned(16);
> +
> +struct umac_tx_descs {
> + struct umac_tx_desc_entry entrylist[UMAC_MAX_TX_DESC_ENTRIES];
> + u8 framelist[UMAC_MAX_TX_DESC_ENTRIES][UMAC_MAX_TX_FRAME_SIZE];
> +} __packed;
> +
> +#endif
> --
> 2.17.1
>
>
Powered by blists - more mailing lists