| 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
| ||
|
Message-ID: <89f237e0-75d4-4690-9d43-903e087e4f46@lunn.ch>
Date: Sun, 3 Mar 2024 18:29:20 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Eric Woudstra <ericwouds@...il.com>
Cc: "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>,
Conor Dooley <conor+dt@...nel.org>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>,
Matthias Brugger <matthias.bgg@...il.com>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
Frank Wunderlich <frank-w@...lic-files.de>,
Daniel Golle <daniel@...rotopia.org>,
Lucien Jheng <lucien.jheng@...oha.com>,
Zhi-Jun You <hujy652@...tonmail.com>, netdev@...r.kernel.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH v2 net-next 2/2] net: phy: air_en8811h: Add the Airoha
EN8811H PHY driver
> +enum {
> + AIR_PHY_LED_DUR_BLINK_32M,
> + AIR_PHY_LED_DUR_BLINK_64M,
> + AIR_PHY_LED_DUR_BLINK_128M,
> + AIR_PHY_LED_DUR_BLINK_256M,
> + AIR_PHY_LED_DUR_BLINK_512M,
> + AIR_PHY_LED_DUR_BLINK_1024M,
DUR meaning duration? It has a blinks on for a little over a
kilometre? So a wave length of a little over 2 kilometres, or a
frequency of around 0.0005Hz :-)
> +static int __air_buckpbus_reg_write(struct phy_device *phydev,
> + u32 pbus_address, u32 pbus_data,
> + bool set_mode)
> +{
> + int ret;
> +
> + if (set_mode) {
> + ret = __phy_write(phydev, AIR_BPBUS_MODE,
> + AIR_BPBUS_MODE_ADDR_FIXED);
> + if (ret < 0)
> + return ret;
> + }
What does set_mode mean?
> +static int en8811h_load_firmware(struct phy_device *phydev)
> +{
> + struct device *dev = &phydev->mdio.dev;
> + const struct firmware *fw1, *fw2;
> + int ret;
> +
> + ret = request_firmware_direct(&fw1, EN8811H_MD32_DM, dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = request_firmware_direct(&fw2, EN8811H_MD32_DSP, dev);
> + if (ret < 0)
> + goto en8811h_load_firmware_rel1;
> +
How big are these firmwares? This will map the entire contents into
memory. There is an alternative interface which allows you to get the
firmware in chunks. I the firmware is big, just getting 4K at a time
might be better, especially if this is an OpenWRT class device.
> +static int en8811h_restart_host(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
> + EN8811H_FW_CTRL_1_START);
> + if (ret < 0)
> + return ret;
> +
> + return air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
> + EN8811H_FW_CTRL_1_FINISH);
> +}
What is host in this context?
> +static int air_led_hw_control_set(struct phy_device *phydev, u8 index,
> + unsigned long rules)
> +{
> + struct en8811h_priv *priv = phydev->priv;
> + u16 on = 0, blink = 0;
> + int ret;
> +
> + if (index >= EN8811H_LED_COUNT)
> + return -EINVAL;
> +
> + priv->led[index].rules = rules;
> +
> + if (rules & (BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK))) {
> + on |= AIR_PHY_LED_ON_LINK10;
> + if (rules & BIT(TRIGGER_NETDEV_RX))
> + blink |= AIR_PHY_LED_BLINK_10RX;
> + if (rules & BIT(TRIGGER_NETDEV_TX))
> + blink |= AIR_PHY_LED_BLINK_10TX;
> + }
> +
> + if (rules & (BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_LINK))) {
> + on |= AIR_PHY_LED_ON_LINK100;
> + if (rules & BIT(TRIGGER_NETDEV_RX))
> + blink |= AIR_PHY_LED_BLINK_100RX;
> + if (rules & BIT(TRIGGER_NETDEV_TX))
> + blink |= AIR_PHY_LED_BLINK_100TX;
> + }
> +
> + if (rules & (BIT(TRIGGER_NETDEV_LINK_1000) | BIT(TRIGGER_NETDEV_LINK))) {
> + on |= AIR_PHY_LED_ON_LINK1000;
> + if (rules & BIT(TRIGGER_NETDEV_RX))
> + blink |= AIR_PHY_LED_BLINK_1000RX;
> + if (rules & BIT(TRIGGER_NETDEV_TX))
> + blink |= AIR_PHY_LED_BLINK_1000TX;
> + }
> +
> + if (rules & (BIT(TRIGGER_NETDEV_LINK_2500) | BIT(TRIGGER_NETDEV_LINK))) {
> + on |= AIR_PHY_LED_ON_LINK2500;
> + if (rules & BIT(TRIGGER_NETDEV_RX))
> + blink |= AIR_PHY_LED_BLINK_2500RX;
> + if (rules & BIT(TRIGGER_NETDEV_TX))
> + blink |= AIR_PHY_LED_BLINK_2500TX;
> + }
> +
> + if (on == 0) {
> + if (rules & BIT(TRIGGER_NETDEV_RX)) {
> + blink |= AIR_PHY_LED_BLINK_10RX |
> + AIR_PHY_LED_BLINK_100RX |
> + AIR_PHY_LED_BLINK_1000RX |
> + AIR_PHY_LED_BLINK_2500RX;
> + }
> + if (rules & BIT(TRIGGER_NETDEV_TX)) {
> + blink |= AIR_PHY_LED_BLINK_10TX |
> + AIR_PHY_LED_BLINK_100TX |
> + AIR_PHY_LED_BLINK_1000TX |
> + AIR_PHY_LED_BLINK_2500TX;
> + }
> + }
Vendors do like making LED control unique. I've not seen any other MAC
or PHY where you can blink for activity at a given speed. You cannot
have 10 and 100 at the same time, so why are there different bits for
them?
I _think_ this can be simplified
+ if (rules & (BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK))) {
+ on |= AIR_PHY_LED_ON_LINK10;
+ }
+
+ if (rules & (BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_LINK))) {
+ on |= AIR_PHY_LED_ON_LINK100;
+ }
+
+ if (rules & (BIT(TRIGGER_NETDEV_LINK_1000) | BIT(TRIGGER_NETDEV_LINK))) {
+ on |= AIR_PHY_LED_ON_LINK1000;
+ }
+
+ if (rules & (BIT(TRIGGER_NETDEV_LINK_2500) | BIT(TRIGGER_NETDEV_LINK))) {
+ on |= AIR_PHY_LED_ON_LINK2500;
+ }
+
+ if (rules & BIT(TRIGGER_NETDEV_RX)) {
+ blink |= AIR_PHY_LED_BLINK_10RX |
+ AIR_PHY_LED_BLINK_100RX |
+ AIR_PHY_LED_BLINK_1000RX |
+ AIR_PHY_LED_BLINK_2500RX;
+ }
+ if (rules & BIT(TRIGGER_NETDEV_TX)) {
+ blink |= AIR_PHY_LED_BLINK_10TX |
+ AIR_PHY_LED_BLINK_100TX |
+ AIR_PHY_LED_BLINK_1000TX |
+ AIR_PHY_LED_BLINK_2500TX;
Does this work?
Andrew
---
pw-bot: cr
Powered by blists - more mailing lists