| 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: <87tzgc283g.fsf@basil.nowhere.org>
Date: Sun, 01 Jun 2008 22:50:11 +0200
From: Andi Kleen <andi@...stfloor.org>
To: eilong@...adcom.com
Cc: netdev <netdev@...r.kernel.org>, jeff@...zik.org,
davem@...emloft.net, "Eliezer Tamir" <eliezert@...adcom.com>,
"Michael Chan" <mchan@...adcom.com>
Subject: Re: [PATCH net-next 2/13]bnx2x: Adding bnx2x_link
"Eilon Greenstein" <eilong@...adcom.com> writes:
Just quick general review. Logic is pretty much unscrutinizable without
a datasheet.
In general your code would be much easier readable if you shortened
some of the defines and names a bit.
> +++ b/drivers/net/bnx2x_link.c
> @@ -0,0 +1,4456 @@
> +/* Copyright 2008 Broadcom Corporation
> + *
> + * Unless you and Broadcom execute a separate written software license
> + * agreement governing use of this software, this software is licensed to you
> + * under the terms of the GNU General Public License version 2, available
> + * at http://www.gnu.org/licenses/old-licenses/gpl-2.0.html (the "GPL").
> + *
> + * Notwithstanding the above, under no circumstances may you combine this
> + * software in any way with any other Broadcom software provided under a
> + * license other than the GPL, without Broadcom's express prior written
> + * consent.
Not a lawyer, but doesn't that conflict with the GPL as additional
restriction in case Broadcom ever released anything else e.g. under
a BSD license which can be normally combined with GPL?
> + * Written by Yaniv Rosner
And shouldn't that person be in Signed-off-by?
A high level comment what this file is supposed to do at the beginning
of the file would be good.
> +static u8 bnx2x_reset_unicore(struct link_params *params)
> +{
> + struct bnx2x *bp = params->bp;
> + u16 mii_control;
> + u16 i;
> +
> + CL45_RD_OVER_CL22(bp, params->port,
> + params->phy_addr,
> + MDIO_REG_BANK_COMBO_IEEE0,
> + MDIO_COMBO_IEEE0_MII_CONTROL, &mii_control);
> +
> + /* reset the unicore */
> + CL45_WR_OVER_CL22(bp, params->port,
> + params->phy_addr,
> + MDIO_REG_BANK_COMBO_IEEE0,
> + MDIO_COMBO_IEEE0_MII_CONTROL,
> + (mii_control |
> + MDIO_COMBO_IEEO_MII_CONTROL_RESET));
Your macro naming is certainly "interesting".
> +
> + /* wait for the reset to self clear */
> + for (i = 0; i < MDIO_ACCESS_TIMEOUT; i++) {
> + udelay(5);
Hopefully that doesn't loop too often. Could tie down the machine
for a long time.
> + if (rx_lane_swap != 0x1b) {
> + CL45_WR_OVER_CL22(bp, params->port,
> + params->phy_addr,
> + MDIO_REG_BANK_XGXS_BLOCK2,
> + MDIO_XGXS_BLOCK2_RX_LN_SWAP,
> + (rx_lane_swap |
> + MDIO_XGXS_BLOCK2_RX_LN_SWAP_ENABLE |
> + MDIO_XGXS_BLOCK2_RX_LN_SWAP_FORCE_ENABLE));
> + } else {
> + CL45_WR_OVER_CL22(bp, params->port,
> + params->phy_addr,
> + MDIO_REG_BANK_XGXS_BLOCK2,
> + MDIO_XGXS_BLOCK2_RX_LN_SWAP, 0);
> + }
That could be written much shorter with two variables.
> +
> + if (tx_lane_swap != 0x1b) {
> + CL45_WR_OVER_CL22(bp, params->port,
> + params->phy_addr,
> + MDIO_REG_BANK_XGXS_BLOCK2,
> + MDIO_XGXS_BLOCK2_TX_LN_SWAP,
> + (tx_lane_swap |
> + MDIO_XGXS_BLOCK2_TX_LN_SWAP_ENABLE));
> + } else {
> + CL45_WR_OVER_CL22(bp, params->port,
> + params->phy_addr,
> + MDIO_REG_BANK_XGXS_BLOCK2,
> + MDIO_XGXS_BLOCK2_TX_LN_SWAP, 0);
Same.
> + PORT_HW_CFG_XGXS_EXT_PHY_TYPE_BCM8073) {
> + /* Disable 2.5Ghz */
> + bnx2x_cl45_read(bp, params->port,
> + ext_phy_type,
> + ext_phy_addr,
> + MDIO_AN_DEVAD,
> + 0x8329, &tmp1);
> +/* SUPPORT_SPEED_CAPABILITY
> + if (params->speed_cap_mask &
> + PORT_HW_CFG_SPEED_CAPABILITY_D0_2_5G)
> +*/
You seem to have a lot of commented out code like this. Remove it all?
> +u8 bnx2x_phy_init(struct link_params *params, struct link_vars *vars)
> +{
> + struct bnx2x *bp = params->bp;
> +
> + u32 val;
> + DP(NETIF_MSG_LINK, "Phy Initialization started\n");
> + DP(NETIF_MSG_LINK, "req_speed = %d, req_flowctrl=%d\n",
> + params->req_line_speed, params->req_flow_ctrl);
> + vars->link_status = 0;
> + if (params->switch_cfg == SWITCH_CFG_1G)
> + vars->phy_flags = PHY_SERDES_FLAG;
> + else
> + vars->phy_flags = PHY_XGXS_FLAG;
> +
> + /* disable attentions */
> + bnx2x_bits_dis(bp, NIG_REG_MASK_INTERRUPT_PORT0 + params->port*4,
> + (NIG_MASK_XGXS0_LINK_STATUS |
> + NIG_MASK_XGXS0_LINK10G |
> + NIG_MASK_SERDES0_LINK_STATUS |
> + NIG_MASK_MI_INT));
> +
> + bnx2x_emac_init(params, vars);
> +
> + if (CHIP_REV_IS_FPGA(bp)) {
Do the FPGA and IS_EMUL cases really need to be in a production driver?
> +u8 bnx2x_link_reset(struct link_params *params, struct link_vars *vars)
> +{
> +
> + struct bnx2x *bp = params->bp;
> + u32 ext_phy_config = params->ext_phy_config;
> + u16 hw_led_mode = params->hw_led_mode;
> + u32 chip_id = params->chip_id;
> + u8 port = params->port;
> + u32 ext_phy_type = XGXS_EXT_PHY_TYPE(ext_phy_config);
> + /* disable attentions */
> +
> + vars->link_status = 0;
> + bnx2x_update_mng(params, vars->link_status);
> + bnx2x_bits_dis(bp, NIG_REG_MASK_INTERRUPT_PORT0 + port*4,
> + (NIG_MASK_XGXS0_LINK_STATUS |
> + NIG_MASK_XGXS0_LINK10G |
> + NIG_MASK_SERDES0_LINK_STATUS |
> + NIG_MASK_MI_INT));
> +
> + /* activate nig drain */
> + REG_WR(bp, NIG_REG_EGRESS_DRAIN0_MODE + port*4, 1);
> +
> + /* disable nig egress interface */
> + REG_WR(bp, NIG_REG_BMAC0_OUT_EN + port*4, 0);
> + REG_WR(bp, NIG_REG_EGRESS_EMAC0_OUT_EN + port*4, 0);
> +
> + /* Stop BigMac rx */
> + bnx2x_bmac_rx_disable(bp, port);
> +
> + /* disable emac */
> + REG_WR(bp, NIG_REG_NIG_EMAC0_EN + port*4, 0);
> +
> + msleep(10);
Didn't see the caller, but hopefully that's not a standard shared workqueue.
Tieing those up for 10ms would be quite nasty.
There are more such long sleeps that should be double checked.
> + /* avoid fast toggling */
> + for (i = 0; i < 10; i++) {
> + msleep(10);
> + CL45_RD_OVER_CL22(bp, port, params->phy_addr,
> + MDIO_REG_BANK_GP_STATUS,
> + MDIO_GP_STATUS_TOP_AN_STATUS1,
> + &gp_status);
> + }
... like this.
> + for (cnt = 0; cnt < 10; cnt++) {
> + msleep(50);
or this
> +/* Programs an image to DSP's flash via the SPI port*/
> +static u8 bnx2x_sfx7101_flash_download(struct bnx2x *bp, u8 port,
> + u8 ext_phy_addr,
> + char data[], u32 size)
Hmm does the driver really need a flash updater as standard
functionality? That could be a separate program.
> + /* wait 0.5 sec to allow it to run */
> + for (cnt = 0; cnt < 100; cnt++)
> + msleep(5);
> +
> + bnx2x_hw_reset(bp);
> +
> + for (cnt = 0; cnt < 100; cnt++)
> + msleep(5);
Especially when it contains long delays like this.
-Andi
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists