netdev - Re: [PATCH net-next 2/2] net: dsa: microchip: configure strap pins during reset

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: <14114502-f087-4d3b-a91e-cff0dfe59045@lunn.ch>
Date: Wed, 10 Sep 2025 18:46:15 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Bastien Curutchet <bastien.curutchet@...tlin.com>
Cc: Woojung Huh <woojung.huh@...rochip.com>, UNGLinuxDriver@...rochip.com,
	Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Marek Vasut <marex@...x.de>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	Miquèl Raynal <miquel.raynal@...tlin.com>,
	Pascal Eberhard <pascal.eberhard@...com>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] net: dsa: microchip: configure strap pins
 during reset

> Support the KSZ8463's strap configuration that enforces SPI as
> communication bus, since it is the only bus supported by the driver.

So this is the key sentence for this patchset, which should of been in
patch 0/X. You have a chicken/egg problem. You cannot talk to the
switch to put it into SPI mode because you cannot talk to the switch
using SPI.

The current patch descriptions, and the patches themselves don't make
this clear. They just vaguely mention configuration via strapping.

> +static int ksz_configure_strap(struct ksz_device *dev)

Please make it clear this function straps the switch for SPI. If
somebody does add support for I2C, they need to understand that...

> +{
> +	struct pinctrl_state *state = NULL;
> +	struct pinctrl *pinctrl;
> +	int ret;
> +
> +	if (of_device_is_compatible(dev->dev->of_node, "microchip,ksz8463")) {

I would not hide this here. Please move this if into
ksz_switch_register(). I also think this function should have the
ksz8463 prefix, since how you strap other devices might differ. So
ksz8463_configure_straps_spi() ?

> +		struct gpio_desc *rxd0;
> +		struct gpio_desc *rxd1;
> +
> +		rxd0 = devm_gpiod_get_index_optional(dev->dev, "strap", 0, GPIOD_OUT_LOW);
> +		if (IS_ERR(rxd0))
> +			return PTR_ERR(rxd0);
> +
> +		rxd1 = devm_gpiod_get_index_optional(dev->dev, "strap", 1, GPIOD_OUT_HIGH);
> +		if (IS_ERR(rxd1))
> +			return PTR_ERR(rxd1);
> +
> +		/* If at least one strap definition is missing we don't do anything */
> +		if (!rxd0 || !rxd1)
> +			return 0;

I would say, if you have one, not two, the DT blob is broken, and you
should return -EINVAL.

> +
> +		pinctrl = devm_pinctrl_get(dev->dev);
> +		if (IS_ERR(pinctrl))
> +			return PTR_ERR(pinctrl);
> +
> +		state = pinctrl_lookup_state(pinctrl, "reset");
> +		if (IS_ERR(state))
> +			return PTR_ERR(state);
> +
> +		ret = pinctrl_select_state(pinctrl, state);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  int ksz_switch_register(struct ksz_device *dev)
>  {
>  	const struct ksz_chip_data *info;
> @@ -5353,10 +5392,18 @@ int ksz_switch_register(struct ksz_device *dev)
>  		return PTR_ERR(dev->reset_gpio);
>  
>  	if (dev->reset_gpio) {
> +		ret = ksz_configure_strap(dev);
> +		if (ret)
> +			return ret;
> +
>  		gpiod_set_value_cansleep(dev->reset_gpio, 1);
>  		usleep_range(10000, 12000);
>  		gpiod_set_value_cansleep(dev->reset_gpio, 0);
>  		msleep(100);
> +
> +		ret = pinctrl_select_default_state(dev->dev);
> +		if (ret)
> +			return ret;

This does not look symmetrical. Maybe put the
pinctrl_select_default_state() inside a function called
ksz8463_release_straps_spi()?

	Andrew