netdev - Re: [PATCH net-next v5 3/3] tsnep: Add TSN endpoint Ethernet MAC driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YZLnhOUg7A66AL5p@lunn.ch>
Date:   Tue, 16 Nov 2021 00:04:36 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Gerhard Engleder <gerhard@...leder-embedded.com>
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v5 3/3] tsnep: Add TSN endpoint Ethernet MAC
 driver

> +static int tsnep_ethtool_get_regs_len(struct net_device *netdev)
> +{
> +	struct tsnep_adapter *adapter = netdev_priv(netdev);
> +	int len;
> +	int num_queues;
> +
> +	len = TSNEP_MAC_SIZE;
> +	num_queues = max(adapter->num_tx_queues, adapter->num_rx_queues);
> +	len += TSNEP_QUEUE_SIZE * (num_queues - 1);

Why the num_queues - 1 ? A comment here might be good explaining it.

> +
> +	return len;
> +}
> +
> +static void tsnep_ethtool_get_regs(struct net_device *netdev,
> +				   struct ethtool_regs *regs,
> +				   void *p)
> +{
> +	struct tsnep_adapter *adapter = netdev_priv(netdev);
> +
> +	regs->version = 1;
> +
> +	memcpy_fromio(p, adapter->addr, regs->len);
> +}

So the registers and the queues are contiguous?

> +static int tsnep_ethtool_get_ts_info(struct net_device *dev,
> +				     struct ethtool_ts_info *info)
> +{
> +	struct tsnep_adapter *adapter = netdev_priv(dev);
> +
> +	info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE |
> +				SOF_TIMESTAMPING_RX_SOFTWARE |
> +				SOF_TIMESTAMPING_SOFTWARE |
> +				SOF_TIMESTAMPING_TX_HARDWARE |
> +				SOF_TIMESTAMPING_RX_HARDWARE |
> +				SOF_TIMESTAMPING_RAW_HARDWARE;
> +
> +	if (adapter->ptp_clock)
> +		info->phc_index = ptp_clock_index(adapter->ptp_clock);
> +	else
> +		info->phc_index = -1;
> +
> +	info->tx_types = BIT(HWTSTAMP_TX_OFF) |
> +			 BIT(HWTSTAMP_TX_ON);
> +	info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> +			   BIT(HWTSTAMP_FILTER_ALL);
> +
> +	return 0;
> +}

You should Cc: Richard Cochran <richardcochran@...il.com> for the PTP
parts.

> +static int tsnep_mdio_init(struct tsnep_adapter *adapter)
> +{
> +	struct device_node *np = adapter->pdev->dev.of_node;
> +	int retval;
> +
> +	if (np) {
> +		np = of_get_child_by_name(np, "mdio");
> +		if (!np)
> +			return 0;
> +
> +		adapter->suppress_preamble =
> +			of_property_read_bool(np, "suppress-preamble");
> +	}
> +
> +	adapter->mdiobus = devm_mdiobus_alloc(&adapter->pdev->dev);
> +	if (!adapter->mdiobus) {
> +		retval = -ENOMEM;
> +
> +		goto out;
> +	}
> +
> +	adapter->mdiobus->priv = (void *)adapter;
> +	adapter->mdiobus->parent = &adapter->pdev->dev;
> +	adapter->mdiobus->read = tsnep_mdiobus_read;
> +	adapter->mdiobus->write = tsnep_mdiobus_write;
> +	adapter->mdiobus->name = TSNEP "-mdiobus";
> +	snprintf(adapter->mdiobus->id, MII_BUS_ID_SIZE, "%s",
> +		 adapter->pdev->name);
> +
> +	if (np) {
> +		retval = of_mdiobus_register(adapter->mdiobus, np);
> +
> +		of_node_put(np);
> +	} else {
> +		/* do not scan broadcast address */
> +		adapter->mdiobus->phy_mask = 0x0000001;
> +
> +		retval = mdiobus_register(adapter->mdiobus);

You can probably simply this. of_mdiobus_register() is happy to take a
NULL pointer for np, and will fall back to mdiobus_register().


> diff --git a/drivers/net/ethernet/engleder/tsnep_test.c b/drivers/net/ethernet/engleder/tsnep_test.c

You have quite a lot of code in this file. Could it either be

1) A loadable module which extends the base driver?
2) A build time configuration option?

What percentage of the overall driver binary does this test code take
up?

Apart from the minor comments above, ethtool, mdio, phy all looks
good.

	Andrew