netdev - Re: [PATCH RFC leds + net-next 7/7] net: phy: marvell: support LEDs connected on Marvell PHYs

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]

Message-ID: <20210301110520.GF31897@duo.ucw.cz>
Date:   Mon, 1 Mar 2021 12:05:20 +0100
From:   Pavel Machek <pavel@....cz>
To:     Marek Behún <kabel@...nel.org>
Cc:     netdev@...r.kernel.org, linux-leds@...r.kernel.org,
        Dan Murphy <dmurphy@...com>,
        Russell King <linux@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>,
        Matthias Schiffer <matthias.schiffer@...tq-group.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Ben Whitten <ben.whitten@...il.com>
Subject: Re: [PATCH RFC leds + net-next 7/7] net: phy: marvell: support LEDs
 connected on Marvell PHYs

Hi!

> Add support for controlling the LEDs connected to several families of
> Marvell PHYs via Linux LED API. These families currently are: 88E1112,
> 88E1116R, 88E1118, 88E1121R, 88E1149R, 88E1240, 88E1318S, 88E1340S,
> 88E1510, 88E1545 and 88E1548P.
> 
> This does not yet add support for compound LED modes. This could be
> achieved via the LED multicolor framework.
> 
> netdev trigger offloading is also implemented.
> 
> Signed-off-by: Marek Behún <kabel@...nel.org>


> +	val = 0;
> +	if (!active_low)
> +		val |= BIT(0);
> +	if (tristate)
> +		val |= BIT(1);

You are parsing these parameters in core... but they are not going to
be common for all phys, right? Should you parse them in the driver?
Should the parameters be same we have for gpio-leds?

> +static const struct marvell_led_mode_info marvell_led_mode_info[] = {
> +	{ LED_MODE(1, 0, 0), { 0x0,  -1, 0x0,  -1,  -1,  -1, }, COMMON },
> +	{ LED_MODE(1, 1, 1), { 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, COMMON },
> +	{ LED_MODE(0, 1, 1), { 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, COMMON },
> +	{ LED_MODE(1, 0, 1), {  -1, 0x2,  -1, 0x2, 0x2, 0x2, }, COMMON },
> +	{ LED_MODE(0, 1, 0), { 0x5,  -1, 0x5,  -1, 0x5, 0x5, }, COMMON },
> +	{ LED_MODE(0, 1, 0), {  -1,  -1,  -1, 0x5,  -1,  -1, }, L3V5_TX },
> +	{ LED_MODE(0, 0, 1), {  -1,  -1,  -1,  -1, 0x0, 0x0, }, COMMON },
> +	{ LED_MODE(0, 0, 1), {  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_RX },
> +};
> +
> +static int marvell_find_led_mode(struct phy_device *phydev, struct phy_led *led,
> +				 struct led_netdev_data *trig)
> +{
> +	const struct marvell_leds_info *info = led->priv;
> +	const struct marvell_led_mode_info *mode;
> +	u32 key;
> +	int i;
> +
> +	key = LED_MODE(trig->link, trig->tx, trig->rx);
> +
> +	for (i = 0; i < ARRAY_SIZE(marvell_led_mode_info); ++i) {
> +		mode = &marvell_led_mode_info[i];
> +
> +		if (key != mode->key || mode->regval[led->addr] == -1 ||
> +		    !(info->flags & mode->flags))
> +			continue;
> +
> +		return mode->regval[led->addr];
> +	}
> +
> +	dev_dbg(led->cdev.dev,
> +		"cannot offload trigger configuration (%s, link=%i, tx=%i, rx=%i)\n",
> +		netdev_name(trig->net_dev), trig->link, trig->tx, trig->rx);
> +
> +	return -1;
> +}

I'm wondering if it makes sense to offload changes on link
state. Those should be fairly infrequent and you are not saving
significant power there... and seems to complicate things.

The "shared frequency blinking" looks quite crazy.

Rest looks reasonably sane.

Best regards,
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)