netdev - Re: [net-next,v2] net: ethernet: rtsn: Add support for Renesas Ethernet-TSN

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: <b51b7b2d-c6d0-49ef-8b84-b9ed8368c797@lunn.ch>
Date: Tue, 7 May 2024 22:57:47 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [net-next,v2] net: ethernet: rtsn: Add support for Renesas
 Ethernet-TSN

> +RENESAS ETHERNET TSN DRIVER
> +M:	Niklas Söderlund <niklas.soderlund@...natech.se>
> +L:	netdev@...r.kernel.org
> +L:	linux-renesas-soc@...r.kernel.org
> +S:	Supported
> +F:	Documentation/devicetree/bindings/net/renesas,ethertsn.yaml
> +F:	drivers/net/ethernet/renesas/rtsn.*
> +
>  RENESAS IDT821034 ASoC CODEC
>  M:	Herve Codina <herve.codina@...tlin.com>
>  L:	alsa-devel@...a-project.org (moderated for non-subscribers)
> diff --git a/drivers/net/ethernet/renesas/Kconfig b/drivers/net/ethernet/renesas/Kconfig
> index b03fae7a0f72..ea4aca5f406f 100644
> --- a/drivers/net/ethernet/renesas/Kconfig
> +++ b/drivers/net/ethernet/renesas/Kconfig
> @@ -58,4 +58,15 @@ config RENESAS_GEN4_PTP
>  	help
>  	  Renesas R-Car Gen4 gPTP device driver.
>  
> +config RTSN
> +	tristate "Renesas Ethernet-TSN support"
> +	depends on ARCH_RENESAS || COMPILE_TEST
> +	depends on PTP_1588_CLOCK
> +	select CRC32
> +	select MII

That is an interesting one. What are you using from MII?

> +static int rtsn_request_irq(unsigned int irq, irq_handler_t handler,
> +			    unsigned long flags, struct rtsn_private *priv,
> +			    const char *ch)
> +{
> +	char *name;
> +	int ret;
> +
> +	name = devm_kasprintf(&priv->pdev->dev, GFP_KERNEL, "%s:%s",
> +			      priv->ndev->name, ch);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	ret = request_irq(irq, handler, flags, name, priv);
> +	if (ret) {
> +		netdev_err(priv->ndev, "Cannot request IRQ %s\n", name);
> +		free_irq(irq, priv);

If requesting an IRQ fails, do you need to free it?

> +static void rtsn_free_irqs(struct rtsn_private *priv)
> +{
> +	free_irq(priv->tx_data_irq, priv);
> +	free_irq(priv->rx_data_irq, priv);
> +}
> +
> +static int rtsn_request_irqs(struct rtsn_private *priv)
> +{
> +	int ret;
> +
> +	priv->rx_data_irq = platform_get_irq_byname(priv->pdev, "rx");
> +	if (priv->rx_data_irq < 0)
> +		return priv->rx_data_irq;
> +
> +	priv->tx_data_irq = platform_get_irq_byname(priv->pdev, "tx");
> +	if (priv->tx_data_irq < 0)
> +		return priv->tx_data_irq;
> +
> +	ret = rtsn_request_irq(priv->tx_data_irq, rtsn_irq, 0, priv, "tx");
> +	if (ret)
> +		goto error;
> +
> +	ret = rtsn_request_irq(priv->rx_data_irq, rtsn_irq, 0, priv, "rx");
> +	if (ret)
> +		goto error;
> +
> +	return 0;
> +error:
> +	rtsn_free_irqs(priv);

This also looks to free IRQs which we potentially never requested.

> +static void rtsn_set_delay_mode(struct rtsn_private *priv)
> +{
> +	struct device_node *np = priv->ndev->dev.parent->of_node;
> +	u32 delay;
> +	u32 val;
> +
> +	val = 0;
> +
> +	/* The MAC is capable of applying a delay on both Rx and Tx. Each
> +	 * delay can either be on or off, there is no way to set its length.
> +	 *
> +	 * The exact delay applied depends on electric characteristics of the
> +	 * board. The datasheet describes a typical Rx delay of 1800 ps and a
> +	 * typical Tx delay of 2000 ps.
> +	 *
> +	 * There are boards where the RTSN device is used together with PHYs
> +	 * who do not support a large enough internal delays to function. These
> +	 * boards depends on the MAC applying these inexact delays.
> +	 */
> +
> +	/* If the phy-mode is rgmii or rgmii-txid apply Rx delay on the MAC */
> +	if (priv->iface == PHY_INTERFACE_MODE_RGMII ||
> +	    priv->iface == PHY_INTERFACE_MODE_RGMII_TXID)
> +		if (!of_property_read_u32(np, "rx-internal-delay-ps", &delay))
> +			if (delay)
> +				val |= GPOUT_RDM;
> +
> +	/* If the phy-mode is rgmii or rgmii-rxid apply Tx delay on the MAC */
> +	if (priv->iface == PHY_INTERFACE_MODE_RGMII ||
> +	    priv->iface == PHY_INTERFACE_MODE_RGMII_RXID)
> +		if (!of_property_read_u32(np, "tx-internal-delay-ps", &delay))
> +			if (delay)
> +				val |= GPOUT_TDM;

This looks wrong. You should be applying delays for rgmii-id and
rgmii-rxid. Plain rgmii means no delays are required, because the
board has extra long clock lines. Same for TX delays, only for
rgmii-tx or rgmii-id.

> +static int rtsn_phy_init(struct rtsn_private *priv)
> +{
> +	struct device_node *np = priv->ndev->dev.parent->of_node;
> +	struct phy_device *phydev;
> +	struct device_node *phy;
> +
> +	priv->link = 0;
> +
> +	phy = of_parse_phandle(np, "phy-handle", 0);
> +	if (!phy)
> +		return -ENOENT;
> +
> +	phydev = of_phy_connect(priv->ndev, phy, rtsn_adjust_link, 0,
> +				priv->iface);

This also looks wrong. Since you have applied the delays in the MAC,
you need to mask the value passed to the PHY so it also does not apply
delays.

    Andrew

---
pw-bot: cr