| 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: <ZK2VRYwW8DxIZCY2@corigine.com>
Date: Tue, 11 Jul 2023 18:45:41 +0100
From: Simon Horman <simon.horman@...igine.com>
To: MD Danish Anwar <danishanwar@...com>
Cc: Randy Dunlap <rdunlap@...radead.org>, Roger Quadros <rogerq@...com>,
Vignesh Raghavendra <vigneshr@...com>, Andrew Lunn <andrew@...n.ch>,
Richard Cochran <richardcochran@...il.com>,
Conor Dooley <conor+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Rob Herring <robh+dt@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
"David S. Miller" <davem@...emloft.net>, nm@...com, srk@...com,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
netdev@...r.kernel.org, linux-omap@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v8 2/2] net: ti: icssg-prueth: Add ICSSG ethernet driver
On Mon, Jul 10, 2023 at 11:05:50AM +0530, MD Danish Anwar wrote:
> From: Roger Quadros <rogerq@...com>
>
> This is the Ethernet driver for TI AM654 Silicon rev. 2
> with the ICSSG PRU Sub-system running dual-EMAC firmware.
>
> The Programmable Real-time Unit and Industrial Communication Subsystem
> Gigabit (PRU_ICSSG) is a low-latency microcontroller subsystem in the TI
> SoCs. This subsystem is provided for the use cases like implementation of
> custom peripheral interfaces, offloading of tasks from the other
> processor cores of the SoC, etc.
>
> Every ICSSG core has two Programmable Real-Time Unit(PRUs),
> two auxiliary Real-Time Transfer Unit (RT_PRUs), and
> two Transmit Real-Time Transfer Units (TX_PRUs). Each one of these runs
> its own firmware. Every ICSSG core has two MII ports connect to these
> PRUs and also a MDIO port.
>
> The cores can run different firmwares to support different protocols and
> features like switch-dev, timestamping, etc.
>
> It uses System DMA to transfer and receive packets and
> shared memory register emulation between the firmware and
> driver for control and configuration.
>
> This patch adds support for basic EMAC functionality with 1Gbps
> and 100Mbps link speed. 10M and half duplex mode are not supported
> currently as they require IEP, the support for which will be added later.
> Support for switch-dev, timestamp, etc. will be added later
> by subsequent patch series.
>
> Signed-off-by: Roger Quadros <rogerq@...com>
> [Vignesh Raghavendra: add 10M full duplex support]
> Signed-off-by: Vignesh Raghavendra <vigneshr@...com>
> [Grygorii Strashko: add support for half duplex operation]
> Signed-off-by: Grygorii Strashko <grygorii.strashko@...com>
> Signed-off-by: Puranjay Mohan <p-mohan@...com>
> Reviewed-by: Andrew Lunn <andrew@...n.ch>
> Signed-off-by: MD Danish Anwar <danishanwar@...com>
...
> +/**
> + * struct map - ICSSG Queue Map
> + * @queue: Queue number
> + * @pd_addr_start: Packet descriptor queue reserved memory
> + * @flags: Flags
> + * @special: Indicates whether this queue is a special queue or not
> + */
> +struct map {
> + int queue;
> + u32 pd_addr_start;
> + u32 flags;
> + bool special;
> +};
> +
> +/* Hardware queue map for ICSSG */
> +const struct map hwq_map[2][ICSSG_NUM_OTHER_QUEUES] = {
Should this be static?
> + {
> + { PORT_HI_Q_SLICE0, PORT_DESC0_HI, 0x200000, 0 },
> + { PORT_LO_Q_SLICE0, PORT_DESC0_LO, 0, 0 },
> + { HOST_HI_Q_SLICE0, HOST_DESC0_HI, 0x200000, 0 },
> + { HOST_LO_Q_SLICE0, HOST_DESC0_LO, 0, 0 },
> + { HOST_SPL_Q_SLICE0, HOST_SPPD0, 0x400000, 1 },
> + },
> + {
> + { PORT_HI_Q_SLICE1, PORT_DESC1_HI, 0xa00000, 0 },
> + { PORT_LO_Q_SLICE1, PORT_DESC1_LO, 0x800000, 0 },
> + { HOST_HI_Q_SLICE1, HOST_DESC1_HI, 0xa00000, 0 },
> + { HOST_LO_Q_SLICE1, HOST_DESC1_LO, 0x800000, 0 },
> + { HOST_SPL_Q_SLICE1, HOST_SPPD1, 0xc00000, 1 },
> + },
> +};
> +
> +static void icssg_config_mii_init(struct prueth_emac *emac)
> +{
> + u32 rxcfg, txcfg, rxcfg_reg, txcfg_reg, pcnt_reg;
> + struct prueth *prueth = emac->prueth;
> + int slice = prueth_emac_slice(emac);
> + struct regmap *mii_rt;
> +
> + mii_rt = prueth->mii_rt;
> +
> + rxcfg_reg = (slice == ICSS_MII0) ? PRUSS_MII_RT_RXCFG0 :
> + PRUSS_MII_RT_RXCFG1;
> + txcfg_reg = (slice == ICSS_MII0) ? PRUSS_MII_RT_TXCFG0 :
> + PRUSS_MII_RT_TXCFG1;
> + pcnt_reg = (slice == ICSS_MII0) ? PRUSS_MII_RT_RX_PCNT0 :
> + PRUSS_MII_RT_RX_PCNT1;
> +
> + rxcfg = MII_RXCFG_DEFAULT;
> + txcfg = MII_TXCFG_DEFAULT;
> +
> + if (slice == ICSS_MII1)
> + rxcfg |= PRUSS_MII_RT_RXCFG_RX_MUX_SEL;
> +
> + /* In MII mode TX lines swapped inside ICSSG, so TX_MUX_SEL cfg need
> + * to be swapped also comparing to RGMII mode.
> + */
> + if (emac->phy_if == PHY_INTERFACE_MODE_MII && slice == ICSS_MII0)
> + txcfg |= PRUSS_MII_RT_TXCFG_TX_MUX_SEL;
> + else if (emac->phy_if != PHY_INTERFACE_MODE_MII && slice == ICSS_MII1)
> + txcfg |= PRUSS_MII_RT_TXCFG_TX_MUX_SEL;
> +
> + regmap_write(mii_rt, rxcfg_reg, rxcfg);
> + regmap_write(mii_rt, txcfg_reg, txcfg);
> + regmap_write(mii_rt, pcnt_reg, 0x1);
> +}
> +
> +static void icssg_miig_queues_init(struct prueth *prueth, int slice)
> +{
> + struct regmap *miig_rt = prueth->miig_rt;
> + void __iomem *smem = prueth->shram.va;
> + u8 pd[ICSSG_SPECIAL_PD_SIZE];
> + int queue = 0, i, j;
> + u32 *pdword;
> +
> + /* reset hwqueues */
> + if (slice)
> + queue = ICSSG_NUM_TX_QUEUES;
> +
> + for (i = 0; i < ICSSG_NUM_TX_QUEUES; i++) {
> + regmap_write(miig_rt, ICSSG_QUEUE_RESET_OFFSET, queue);
> + queue++;
> + }
> +
> + queue = slice ? RECYCLE_Q_SLICE1 : RECYCLE_Q_SLICE0;
> + regmap_write(miig_rt, ICSSG_QUEUE_RESET_OFFSET, queue);
> +
> + for (i = 0; i < ICSSG_NUM_OTHER_QUEUES; i++) {
> + regmap_write(miig_rt, ICSSG_QUEUE_RESET_OFFSET,
> + hwq_map[slice][i].queue);
> + }
> +
> + /* initialize packet descriptors in SMEM */
> + /* push pakcet descriptors to hwqueues */
> +
> + pdword = (u32 *)pd;
> + for (j = 0; j < ICSSG_NUM_OTHER_QUEUES; j++) {
> + const struct map *mp;
> + int pd_size, num_pds;
> + u32 pdaddr;
> +
> + mp = &hwq_map[slice][j];
> + if (mp->special) {
> + pd_size = ICSSG_SPECIAL_PD_SIZE;
> + num_pds = ICSSG_NUM_SPECIAL_PDS;
> + } else {
> + pd_size = ICSSG_NORMAL_PD_SIZE;
> + num_pds = ICSSG_NUM_NORMAL_PDS;
> + }
> +
> + for (i = 0; i < num_pds; i++) {
> + memset(pd, 0, pd_size);
> +
> + pdword[0] &= cpu_to_le32(ICSSG_FLAG_MASK);
> + pdword[0] |= cpu_to_le32(mp->flags);
Sparse warns that the endieness of pdword is not le32.
There are also other sparse warnings added by this patch.
Please look over them.
> + pdaddr = mp->pd_addr_start + i * pd_size;
> +
> + memcpy_toio(smem + pdaddr, pd, pd_size);
> + queue = mp->queue;
> + regmap_write(miig_rt, ICSSG_QUEUE_OFFSET + 4 * queue,
> + pdaddr);
> + }
> + }
> +}
...
> +static int prueth_netdev_init(struct prueth *prueth,
> + struct device_node *eth_node)
> +{
> + int ret, num_tx_chn = PRUETH_MAX_TX_QUEUES;
> + struct prueth_emac *emac;
> + struct net_device *ndev;
> + enum prueth_port port;
> + enum prueth_mac mac;
> +
> + port = prueth_node_port(eth_node);
> + if (port < 0)
> + return -EINVAL;
> +
> + mac = prueth_node_mac(eth_node);
> + if (mac < 0)
> + return -EINVAL;
> +
> + ndev = alloc_etherdev_mq(sizeof(*emac), num_tx_chn);
> + if (!ndev)
> + return -ENOMEM;
...
> + return 0;
> +
> +free:
> + pruss_release_mem_region(prueth->pruss, &emac->dram);
> +free_wq:
> + destroy_workqueue(emac->cmd_wq);
> +free_ndev:
> + free_netdev(ndev);
> + prueth->emac[mac] = NULL;
> +
> + return ret;
ndev appears to be leaked here.
...
> + prueth->dev = dev;
> + eth_ports_node = of_get_child_by_name(np, "ethernet-ports");
> + if (!eth_ports_node)
> + return -ENOENT;
> +
> + for_each_child_of_node(eth_ports_node, eth_node) {
> + u32 reg;
> +
> + if (strcmp(eth_node->name, "port"))
> + continue;
> + ret = of_property_read_u32(eth_node, "reg", ®);
> + if (ret < 0) {
> + dev_err(dev, "%pOF error reading port_id %d\n",
> + eth_node, ret);
> + }
> +
> + of_node_get(eth_node);
> +
> + if (reg == 0) {
> + eth0_node = eth_node;
> + if (!of_device_is_available(eth0_node)) {
> + of_node_put(eth0_node);
> + eth0_node = NULL;
> + }
> + } else if (reg == 1) {
> + eth1_node = eth_node;
> + if (!of_device_is_available(eth1_node)) {
> + of_node_put(eth1_node);
> + eth1_node = NULL;
> + }
> + } else {
> + dev_err(dev, "port reg should be 0 or 1\n");
Should this be treated as an error and either return or goto an
unwind path?
> + }
> + }
> +
> + of_node_put(eth_ports_node);
> +
> + /* At least one node must be present and available else we fail */
> + if (!eth0_node && !eth1_node) {
Smatch warns that eth0_node and eth1_node may be uninitialised here.
> + dev_err(dev, "neither port0 nor port1 node available\n");
> + return -ENODEV;
> + }
> +
> + if (eth0_node == eth1_node) {
> + dev_err(dev, "port0 and port1 can't have same reg\n");
> + of_node_put(eth0_node);
> + return -ENODEV;
> + }
...
--
pw-bot: changes-requested
Powered by blists - more mailing lists