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: <YeripbSOeq1s2U3u@lunn.ch> Date: Fri, 21 Jan 2022 17:43:17 +0100 From: Andrew Lunn <andrew@...n.ch> To: Joseph CHAMG <josright123@...il.com> Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Rob Herring <robh+dt@...nel.org>, joseph_chang@...icom.com.tw, netdev@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, andy.shevchenko@...il.com, Leon Romanovsky <leon@...nel.org> Subject: Re: [PATCH v12, 2/2] net: Add dm9051 driver > +static int ctrl_dm9051_phywrite(void *context, unsigned int reg, unsigned int val) > +{ > + /* chip internal operation need wait 1 ms for if power-up phy > + */ > + if (reg == MII_BMCR && !(val & BMCR_PDOWN)) > + mdelay(1); What PHY driver are you using? It would be much better to have this in the PHY driver. The MAC driver should not be touching the PHY. > +static int dm9051_phy_connect(struct board_info *db) > +{ > + char phy_id[MII_BUS_ID_SIZE + 3]; > + > + snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT, > + db->mdiobus->id, DM9051_PHY_ID); > + > + db->phydev = phy_connect(db->ndev, phy_id, dm9051_handle_link_change, > + PHY_INTERFACE_MODE_MII); > + if (IS_ERR(db->phydev)) > + return PTR_ERR_OR_ZERO(db->phydev); Why PTR_ERR_OR_ZERO() > +static int dm9051_direct_fifo_reset(struct board_info *db) > +{ > + struct net_device *ndev = db->ndev; > + int rxlen = le16_to_cpu(db->eth_rxhdr.rxlen); reverse christmas tree. There are a few more cases. Please review the whole driver. > +/* transmit a packet, > + * return value, > + * 0 - succeed > + * -ETIMEDOUT - timeout error > + */ > +static int dm9051_single_tx(struct board_info *db, u8 *buff, unsigned int len) > +{ > + int ret; > + > + ret = dm9051_map_xmitpoll(db); > + if (ret) > + return -ETIMEDOUT; > + If dm9051_map_xmitpoll() returns an error code, use it. There needs to be a good reason to change the error code, and if you have such a good reason, please add a comment about it. > +static irqreturn_t dm9051_rx_threaded_irq(int irq, void *pw) > +{ > + struct board_info *db = pw; > + int result, resul_tx; > + > + mutex_lock(&db->spi_lockm); /* mutex essential */ When are mutex's not essential? This commit seems to be meaningless. It gives the impression you don't understand mutex's and locking in general. You have just added mutex until it seems to work, not that you have a locking design. > + if (netif_carrier_ok(db->ndev)) { > + result = regmap_write(db->regmap_dm, DM9051_IMR, IMR_PAR); /* disable imr */ > + if (unlikely(result)) > + goto spi_err; > + > + do { > + result = dm9051_loop_rx(db); /* threaded irq rx */ > + if (result < 0) > + goto spi_err; > + resul_tx = dm9051_loop_tx(db); /* more tx better performance */ > + if (resul_tx < 0) result_tx ^ > + goto spi_err; > + } while (result > 0); > + > + result = regmap_write(db->regmap_dm, DM9051_IMR, db->imr_all); /* enable imr */ > + if (unlikely(result)) > + goto spi_err; > + } > +spi_err: > + mutex_unlock(&db->spi_lockm); /* mutex essential */ > + return IRQ_HANDLED; > +} > +static int dm9051_map_phyup(struct board_info *db) > +{ > + int ret; > + > + /* ~BMCR_PDOWN to power-up phyxcer > + */ > + ret = mdiobus_modify(db->mdiobus, DM9051_PHY_ID, MII_BMCR, BMCR_PDOWN, 0); > + if (ret < 0) > + return ret; > + > + /* chip internal operation need wait 1 ms for if GPR power-up phy > + */ > + ret = regmap_write(db->regmap_dm, DM9051_GPR, 0); > + if (unlikely(ret)) > + return ret; > + mdelay(1); The phy driver should do this. Again, what PHY driver are you using? > +static int dm9051_map_phydown(struct board_info *db) > +{ > + int ret; > + > + ret = regmap_write(db->regmap_dm, DM9051_GPR, GPR_PHY_ON); /* Power-Down PHY */ > + if (unlikely(ret)) > + return ret; > + return ret; > +} Cam you still access the PHY after this? Does it loose its configuration? > + /* We may have start with auto negotiation */ > + db->phydev->autoneg = AUTONEG_ENABLE; > + db->phydev->speed = 0; > + db->phydev->duplex = 0; If you have to touch these, something is wrong. Please explain. Andrew
Powered by blists - more mailing lists