netdev - Re: [PATCH net-next 8/8] net: stmmac: qcom-ethqos: add a DMA-reset quirk for sa8775p-ride-r3

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]

Date: Thu, 20 Jun 2024 11:57:20 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Vinod Koul <vkoul@...nel.org>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Jose Abreu <joabreu@...opsys.com>,
	"David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>,
	Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
	linux-arm-msm@...r.kernel.org,
	linux-stm32@...md-mailman.stormreply.com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH net-next 8/8] net: stmmac: qcom-ethqos: add a DMA-reset
 quirk for sa8775p-ride-r3

On Wed, Jun 19, 2024 at 08:45:49PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> 
> On sa8775p-ride the RX clocks from the AQR115C PHY are not available at
> the time of the DMA reset so we need to loop TX clocks to RX and then
> disable loopback after link-up. Use the provided callbacks to do it for
> this board.

If you're using true Cisco SGMII, there are _no_ clocks transferred
between the PHY and PCS/MAC. There are two balanced pairs of data
lines and that is all - one for transmit and one for receive. So this
explanation doesn't make sense to me.

> +static void qcom_ethqos_set_serdes_loopback(struct qcom_ethqos *ethqos,
> +					    bool enable)
> +{
> +	rgmii_updatel(ethqos,
> +		      SGMII_PHY_CNTRL1_SGMII_TX_TO_RX_LOOPBACK_EN,
> +		      enable ? SGMII_PHY_CNTRL1_SGMII_TX_TO_RX_LOOPBACK_EN : 0,
> +		      EMAC_WRAPPER_SGMII_PHY_CNTRL1);
> +}
> +
> +static void qcom_ethqos_open(struct net_device *pdev, void *priv)
> +{
> +	struct qcom_ethqos *ethqos = priv;
> +
> +	qcom_ethqos_set_serdes_loopback(ethqos, true);
> +}
> +
> +static void qcom_ethqos_link_up(struct net_device *ndev, void *priv)
> +{
> +	struct qcom_ethqos *ethqos = priv;
> +
> +	qcom_ethqos_set_serdes_loopback(ethqos, false);
> +}
> +

So you enable loopback at open time, and disable it when the link comes
up. This breaks inband signalling (should stmmac ever use that) because
enabling loopback prevents the PHY sending the SGMII result to the PCS
to indicate that the link has come up... thus phylink won't call
mac_link_up().

So no, I really hate this proposed change.

What I think would be better is if there were hooks at the appropriate
places to handle the lack of clock over _just_ the period that it needs
to be handled, rather than hacking the driver as this proposal does,
abusing platform callbacks because there's nothing better.

I don't have time to go through stmmac and make any suggestions (sorry)
so I can only to say NAK to this change.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!