netdev - Re: [net-next PATCH v2 02/14] net: dsa: qca8k: add LEDs basic support

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] [day] [month] [year] [list]

Date:   Fri, 10 Mar 2023 01:58:09 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Christian Marangi <ansuelsmth@...il.com>
Cc:     Florian Fainelli <f.fainelli@...il.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+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Gregory Clement <gregory.clement@...tlin.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        John Crispin <john@...ozen.org>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-arm-msm@...r.kernel.org, Lee Jones <lee@...nel.org>,
        linux-leds@...r.kernel.org
Subject: Re: [net-next PATCH v2 02/14] net: dsa: qca8k: add LEDs basic support

> > > +static enum led_brightness
> > > +qca8k_led_brightness_get(struct qca8k_led *led)
> > > +{
> > > +	struct qca8k_led_pattern_en reg_info;
> > > +	struct qca8k_priv *priv = led->priv;
> > > +	u32 val;
> > > +	int ret;
> > > +
> > > +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> > > +
> > > +	ret = regmap_read(priv->regmap, reg_info.reg, &val);
> > > +	if (ret)
> > > +		return 0;
> > > +
> > > +	val >>= reg_info.shift;
> > > +
> > > +	if (led->port_num == 0 || led->port_num == 4) {
> > > +		val &= QCA8K_LED_PATTERN_EN_MASK;
> > > +		val >>= QCA8K_LED_PATTERN_EN_SHIFT;
> > > +	} else {
> > > +		val &= QCA8K_LED_PHY123_PATTERN_EN_MASK;
> > > +	}
> > > +
> > > +	return val > 0 ? 1 : 0;
> > > +}
> > 
> > What will this return when in the future you add hardware offload, and
> > the LED is actually blinking because of frames being sent etc?
> > 
> > Is it better to not implement _get() when it is unclear what it should
> > return when offload is in operation?
> > 
> 
> My idea was that anything that is not 'always off' will have brightness
> 1. So also in accelerated blink brightness should be 1.
> 
> My idea of get was that it should reflect if the led is active or always
> off. Is it wrong?
 
brigntness_get seems to be used in two situations:

When the LED is first registered, it can be called to get the current
state of the LED. This then initialized cdev->brightness.

When the brightness sysfs file is read, there is first a call to
brightness_get to allow it to update the value in cdev->brightness
before returning the value in the read.

I think always returning 1 could be confusing. Take the example that
the LED is indicating link, there is no link, so it is off. Yet a read
of the brightness sysfs file will return 1?

I would say, it either needs to return the instantaneous brightness,
or it should not be implemented at all. When we come to implement
offloading, we might want to consider hiding the brightness sysfs
file. But we can solve that later.

      Andrew