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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 14 Jan 2022 23:25:17 +0800 From: Kai-Heng Feng <kai.heng.feng@...onical.com> To: Andrew Lunn <andrew@...n.ch> Cc: peppe.cavallaro@...com, alexandre.torgue@...s.st.com, joabreu@...opsys.com, "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Maxime Coquelin <mcoquelin.stm32@...il.com>, Heiner Kallweit <hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>, Marek Behún <kabel@...nel.org>, Ivan Bornyakov <i.bornyakov@...rotek.ru>, Pali Rohár <pali@...nel.org>, netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com, linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware On Fri, Jan 14, 2022 at 9:20 PM Andrew Lunn <andrew@...n.ch> wrote: > > > static void marvell_config_led(struct phy_device *phydev) > > { > > - u16 def_config; > > + struct marvell_priv *priv = phydev->priv; > > int err; > > > > - switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) { > > - /* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */ > > - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1121R): > > - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1318S): > > - def_config = MII_88E1121_PHY_LED_DEF; > > - break; > > - /* Default PHY LED config: > > - * LED[0] .. 1000Mbps Link > > - * LED[1] .. 100Mbps Link > > - * LED[2] .. Blink, Activity > > - */ > > - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1510): > > - if (phydev->dev_flags & MARVELL_PHY_LED0_LINK_LED1_ACTIVE) > > - def_config = MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE; > > - else > > - def_config = MII_88E1510_PHY_LED_DEF; > > - break; > > - default: > > + if (priv->led_def_config == -1) > > return; > > + > > + if (priv->led_def_config) > > + goto write; > > Really? > > Please restructure this code. Take it apart into helpers. You need: > > A function to set the actual LED configuration. > A function to decide what, if any, configuration to set > A function to store the current configuration on suspend. > A function to restore the current configuration on resume. > > Lots of little functions will make it much easier to understand, and > avoid 1980s BASIC style. Sure, will turn these into helper functions. > > I'm also surprised you need to deal with suspend/resume. Why does the > BIOS not set the LED mode on resume, same as it does on power up? I was told this is a BIOS limitation. I'll ask vendor _why_ the LED can't be restored by BIOS. Kai-Heng > > Andrew
Powered by blists - more mailing lists