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]

Date:   Thu, 7 Oct 2021 20:25:22 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Ansuel Smith <ansuelsmth@...il.com>
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

> Anyway the difference is that they made 2 different package version of
> the qca8327. One with 176 pin and one with 148 pin.

I assume they have different product numbers. So you can quote them in
the DT binding? Are they BGP or QFP? Can somebody easily count the
pins?

> > > +
> > > +	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.

So first off, it sounds like you have the DT property named wrong. It
should be 'qca,ignore-power-on-sel'.

However, why do you even need this? Generally, strapping gives you the
defaults. Registers get loaded with a value determined by the
strapping. But after that, you can change the value, based on
additional information. Or are you saying the register is read only
when strapping is used?

> > > +	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;

At minimum, you need to clearly document that qca,led-open-drain
implies 'qca,ignore-power-on-sel'. I would probably go further and
return -EINVAL if qca,led-open-drain is set and
'qca,ignore-power-on-sel' is not.

	Andrew