netdev - Re: [net-next PATCH 12/13] drivers: net: dsa: qca8k: add support for pws config reg

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: <YV759SSdKu0w83UB@Ansuel-xps.localdomain>
Date:   Thu, 7 Oct 2021 15:45:25 +0200
From:   Ansuel Smith <ansuelsmth@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next PATCH 12/13] drivers: net: dsa: qca8k: add support for
 pws config reg

On Thu, Oct 07, 2021 at 02:41:10AM +0200, Andrew Lunn wrote:
> > +static int
> > +qca8k_setup_of_pws_reg(struct qca8k_priv *priv)
> > +{
> > +	struct device_node *node = priv->dev->of_node;
> > +	u32 val = 0;
> > +
> > +	if (priv->switch_id == QCA8K_ID_QCA8327)
> > +		if (of_property_read_bool(node, "qca,package48"))
> > +			val |= QCA8327_PWS_PACKAGE48_EN;
> 
> What does this actually do? How is PACKAGE48 different to normal mode?
>

I actually made a typo.
Anyway the difference is that they made 2 different package version of
the qca8327. One with 176 pin and one with 148 pin. Setting the wrong
layout cause the switch malfunction (no traffic, we found this on one
xiaomi device). This is from Documenation and it does toggle the MAC
interface configuration for the 2 different package.

> > +
> > +	if (of_property_read_bool(node, "qca,power-on-sel"))
> > +		val |= QCA8K_PWS_POWER_ON_SEL;
> 
> What happens if you unconditionally do this? Why is a DT property
> required?
> 

This is needed to bypass the power on strapping and use the regs config.
The switch can use hardware pin to set eeprom presence and leds open
drain. Setting this bit on bypass the hardware strapping and sets these
2 thing based on the regs. We didn't add the eeprom binding as we didn't
find any switch using it and we don't have any support for it.

> > +
> > +	if (of_property_read_bool(node, "qca,led-open-drain"))
> > +		/* POWER_ON_SEL needs to be set when configuring led to open drain */
> > +		val |= QCA8K_PWS_LED_OPEN_EN_CSR | QCA8K_PWS_POWER_ON_SEL;
> 
> This is getting into territory of adding LED support for PHYs, which
> we want to do via the LED subsystem.
> 

Don't know if it would be the correct way. Without this the switch leds
chaese to work. I think this should be handled in a dedicated way than
defined in a binding in the leds configuration. But I could be wrong.

>    Andrew

-- 
	Ansuel