| 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: <20091118110608.GA2358@dhcp-lab-161.englab.brq.redhat.com>
Date: Wed, 18 Nov 2009 12:06:10 +0100
From: Stanislaw Gruszka <sgruszka@...hat.com>
To: Michael Chan <mchan@...adcom.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org, michaelc@...wisc.edu,
shmulikr@...adcom.com, eilong@...adcom.com
Subject: Re: [PATCH 2/7] bnx2x: Refactor MAC address setup code.
Hi Michael
On Sat, Oct 10, 2009 at 04:46:54PM -0700, Michael Chan wrote:
> For iSCSI MAC address setup in later patches.
>
> Signed-off-by: Shmulik Ravid - Rabinovitz <shmulikr@...adcom.com>
> Signed-off-by: Michael Chan <mchan@...adcom.com>
> Acked-by: Eilon Greenstein <eilong@...adcom.com>
In this patch you become to use ->set_mac_pending as a counter. It looks
like you are trying to do smb_wmb() to make incrementing/decrementing
operations correct on many processors. But this seems to be wrong,
since there is no data dependency and smp_wmb()/smp_rmb() does noting.
In particular these barriers not prevent to race when two processors
decrement/increment counter. IMHO atomic_t should be used.
Stanislaw
> ---
> drivers/net/bnx2x.h | 4 +-
> drivers/net/bnx2x_main.c | 162 ++++++++++++++++++++++++++++++++--------------
> 2 files changed, 114 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/net/bnx2x.h b/drivers/net/bnx2x.h
> index bbf8422..1f07063 100644
> --- a/drivers/net/bnx2x.h
> +++ b/drivers/net/bnx2x.h
> @@ -863,8 +863,8 @@ struct bnx2x {
>
> /* Flags for marking that there is a STAT_QUERY or
> SET_MAC ramrod pending */
> - u8 stats_pending;
> - u8 set_mac_pending;
> + int stats_pending;
> + int set_mac_pending;
>
> /* End of fields used in the performance code paths */
>
> diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
> index 713d669..02ce3b3 100644
> --- a/drivers/net/bnx2x_main.c
> +++ b/drivers/net/bnx2x_main.c
> @@ -1026,12 +1026,15 @@ static void bnx2x_sp_event(struct bnx2x_fastpath *fp,
> case (RAMROD_CMD_ID_ETH_SET_MAC | BNX2X_STATE_OPEN):
> case (RAMROD_CMD_ID_ETH_SET_MAC | BNX2X_STATE_DIAG):
> DP(NETIF_MSG_IFUP, "got set mac ramrod\n");
> - bp->set_mac_pending = 0;
> + bp->set_mac_pending--;
> + smp_wmb();
> break;
>
> case (RAMROD_CMD_ID_ETH_SET_MAC | BNX2X_STATE_CLOSING_WAIT4_HALT):
> case (RAMROD_CMD_ID_ETH_SET_MAC | BNX2X_STATE_DISABLED):
> DP(NETIF_MSG_IFDOWN, "got (un)set mac ramrod\n");
> + bp->set_mac_pending--;
> + smp_wmb();
> break;
>
> default:
> @@ -2530,7 +2533,7 @@ u32 bnx2x_fw_command(struct bnx2x *bp, u32 command)
> }
>
> static void bnx2x_set_storm_rx_mode(struct bnx2x *bp);
> -static void bnx2x_set_mac_addr_e1h(struct bnx2x *bp, int set);
> +static void bnx2x_set_eth_mac_addr_e1h(struct bnx2x *bp, int set);
> static void bnx2x_set_rx_mode(struct net_device *dev);
>
> static void bnx2x_e1h_disable(struct bnx2x *bp)
> @@ -2546,7 +2549,7 @@ static void bnx2x_e1h_disable(struct bnx2x *bp)
>
> REG_WR(bp, NIG_REG_LLH0_FUNC_EN + port*8, 0);
>
> - bnx2x_set_mac_addr_e1h(bp, 0);
> + bnx2x_set_eth_mac_addr_e1h(bp, 0);
>
> for (i = 0; i < MC_HASH_SIZE; i++)
> REG_WR(bp, MC_HASH_OFFSET(bp, i), 0);
> @@ -2560,7 +2563,7 @@ static void bnx2x_e1h_enable(struct bnx2x *bp)
>
> REG_WR(bp, NIG_REG_LLH0_FUNC_EN + port*8, 1);
>
> - bnx2x_set_mac_addr_e1h(bp, 1);
> + bnx2x_set_eth_mac_addr_e1h(bp, 1);
>
> /* Tx queue should be only reenabled */
> netif_tx_wake_all_queues(bp->dev);
> @@ -7036,7 +7039,19 @@ static void bnx2x_netif_stop(struct bnx2x *bp, int disable_hw)
> * Init service functions
> */
>
> -static void bnx2x_set_mac_addr_e1(struct bnx2x *bp, int set)
> +/**
> + * Sets a MAC in a CAM for a few L2 Clients for E1 chip
> + *
> + * @param bp driver descriptor
> + * @param set set or clear an entry (1 or 0)
> + * @param mac pointer to a buffer containing a MAC
> + * @param cl_bit_vec bit vector of clients to register a MAC for
> + * @param cam_offset offset in a CAM to use
> + * @param with_bcast set broadcast MAC as well
> + */
> +static void bnx2x_set_mac_addr_e1_gen(struct bnx2x *bp, int set, u8 *mac,
> + u32 cl_bit_vec, u8 cam_offset,
> + u8 with_bcast)
> {
> struct mac_configuration_cmd *config = bnx2x_sp(bp, mac_config);
> int port = BP_PORT(bp);
> @@ -7045,25 +7060,25 @@ static void bnx2x_set_mac_addr_e1(struct bnx2x *bp, int set)
> * unicasts 0-31:port0 32-63:port1
> * multicast 64-127:port0 128-191:port1
> */
> - config->hdr.length = 2;
> - config->hdr.offset = port ? 32 : 0;
> - config->hdr.client_id = bp->fp->cl_id;
> + config->hdr.length = 1 + (with_bcast ? 1 : 0);
> + config->hdr.offset = cam_offset;
> + config->hdr.client_id = 0xff;
> config->hdr.reserved1 = 0;
>
> /* primary MAC */
> config->config_table[0].cam_entry.msb_mac_addr =
> - swab16(*(u16 *)&bp->dev->dev_addr[0]);
> + swab16(*(u16 *)&mac[0]);
> config->config_table[0].cam_entry.middle_mac_addr =
> - swab16(*(u16 *)&bp->dev->dev_addr[2]);
> + swab16(*(u16 *)&mac[2]);
> config->config_table[0].cam_entry.lsb_mac_addr =
> - swab16(*(u16 *)&bp->dev->dev_addr[4]);
> + swab16(*(u16 *)&mac[4]);
> config->config_table[0].cam_entry.flags = cpu_to_le16(port);
> if (set)
> config->config_table[0].target_table_entry.flags = 0;
> else
> CAM_INVALIDATE(config->config_table[0]);
> config->config_table[0].target_table_entry.clients_bit_vector =
> - cpu_to_le32(1 << BP_L_ID(bp));
> + cpu_to_le32(cl_bit_vec);
> config->config_table[0].target_table_entry.vlan_id = 0;
>
> DP(NETIF_MSG_IFUP, "%s MAC (%04x:%04x:%04x)\n",
> @@ -7073,47 +7088,58 @@ static void bnx2x_set_mac_addr_e1(struct bnx2x *bp, int set)
> config->config_table[0].cam_entry.lsb_mac_addr);
>
> /* broadcast */
> - config->config_table[1].cam_entry.msb_mac_addr = cpu_to_le16(0xffff);
> - config->config_table[1].cam_entry.middle_mac_addr = cpu_to_le16(0xffff);
> - config->config_table[1].cam_entry.lsb_mac_addr = cpu_to_le16(0xffff);
> - config->config_table[1].cam_entry.flags = cpu_to_le16(port);
> - if (set)
> - config->config_table[1].target_table_entry.flags =
> - TSTORM_CAM_TARGET_TABLE_ENTRY_BROADCAST;
> - else
> - CAM_INVALIDATE(config->config_table[1]);
> - config->config_table[1].target_table_entry.clients_bit_vector =
> - cpu_to_le32(1 << BP_L_ID(bp));
> - config->config_table[1].target_table_entry.vlan_id = 0;
> + if (with_bcast) {
> + config->config_table[1].cam_entry.msb_mac_addr =
> + cpu_to_le16(0xffff);
> + config->config_table[1].cam_entry.middle_mac_addr =
> + cpu_to_le16(0xffff);
> + config->config_table[1].cam_entry.lsb_mac_addr =
> + cpu_to_le16(0xffff);
> + config->config_table[1].cam_entry.flags = cpu_to_le16(port);
> + if (set)
> + config->config_table[1].target_table_entry.flags =
> + TSTORM_CAM_TARGET_TABLE_ENTRY_BROADCAST;
> + else
> + CAM_INVALIDATE(config->config_table[1]);
> + config->config_table[1].target_table_entry.clients_bit_vector =
> + cpu_to_le32(cl_bit_vec);
> + config->config_table[1].target_table_entry.vlan_id = 0;
> + }
>
> bnx2x_sp_post(bp, RAMROD_CMD_ID_ETH_SET_MAC, 0,
> U64_HI(bnx2x_sp_mapping(bp, mac_config)),
> U64_LO(bnx2x_sp_mapping(bp, mac_config)), 0);
> }
>
> -static void bnx2x_set_mac_addr_e1h(struct bnx2x *bp, int set)
> +/**
> + * Sets a MAC in a CAM for a few L2 Clients for E1H chip
> + *
> + * @param bp driver descriptor
> + * @param set set or clear an entry (1 or 0)
> + * @param mac pointer to a buffer containing a MAC
> + * @param cl_bit_vec bit vector of clients to register a MAC for
> + * @param cam_offset offset in a CAM to use
> + */
> +static void bnx2x_set_mac_addr_e1h_gen(struct bnx2x *bp, int set, u8 *mac,
> + u32 cl_bit_vec, u8 cam_offset)
> {
> struct mac_configuration_cmd_e1h *config =
> (struct mac_configuration_cmd_e1h *)bnx2x_sp(bp, mac_config);
>
> - /* CAM allocation for E1H
> - * unicasts: by func number
> - * multicast: 20+FUNC*20, 20 each
> - */
> config->hdr.length = 1;
> - config->hdr.offset = BP_FUNC(bp);
> - config->hdr.client_id = bp->fp->cl_id;
> + config->hdr.offset = cam_offset;
> + config->hdr.client_id = 0xff;
> config->hdr.reserved1 = 0;
>
> /* primary MAC */
> config->config_table[0].msb_mac_addr =
> - swab16(*(u16 *)&bp->dev->dev_addr[0]);
> + swab16(*(u16 *)&mac[0]);
> config->config_table[0].middle_mac_addr =
> - swab16(*(u16 *)&bp->dev->dev_addr[2]);
> + swab16(*(u16 *)&mac[2]);
> config->config_table[0].lsb_mac_addr =
> - swab16(*(u16 *)&bp->dev->dev_addr[4]);
> + swab16(*(u16 *)&mac[4]);
> config->config_table[0].clients_bit_vector =
> - cpu_to_le32(1 << BP_L_ID(bp));
> + cpu_to_le32(cl_bit_vec);
> config->config_table[0].vlan_id = 0;
> config->config_table[0].e1hov_id = cpu_to_le16(bp->e1hov);
> if (set)
> @@ -7122,11 +7148,11 @@ static void bnx2x_set_mac_addr_e1h(struct bnx2x *bp, int set)
> config->config_table[0].flags =
> MAC_CONFIGURATION_ENTRY_E1H_ACTION_TYPE;
>
> - DP(NETIF_MSG_IFUP, "%s MAC (%04x:%04x:%04x) E1HOV %d CLID %d\n",
> + DP(NETIF_MSG_IFUP, "%s MAC (%04x:%04x:%04x) E1HOV %d CLID mask %d\n",
> (set ? "setting" : "clearing"),
> config->config_table[0].msb_mac_addr,
> config->config_table[0].middle_mac_addr,
> - config->config_table[0].lsb_mac_addr, bp->e1hov, BP_L_ID(bp));
> + config->config_table[0].lsb_mac_addr, bp->e1hov, cl_bit_vec);
>
> bnx2x_sp_post(bp, RAMROD_CMD_ID_ETH_SET_MAC, 0,
> U64_HI(bnx2x_sp_mapping(bp, mac_config)),
> @@ -7178,6 +7204,31 @@ static int bnx2x_wait_ramrod(struct bnx2x *bp, int state, int idx,
> return -EBUSY;
> }
>
> +static void bnx2x_set_eth_mac_addr_e1h(struct bnx2x *bp, int set)
> +{
> + bp->set_mac_pending++;
> + smp_wmb();
> +
> + bnx2x_set_mac_addr_e1h_gen(bp, set, bp->dev->dev_addr,
> + (1 << bp->fp->cl_id), BP_FUNC(bp));
> +
> + /* Wait for a completion */
> + bnx2x_wait_ramrod(bp, 0, 0, &bp->set_mac_pending, set ? 0 : 1);
> +}
> +
> +static void bnx2x_set_eth_mac_addr_e1(struct bnx2x *bp, int set)
> +{
> + bp->set_mac_pending++;
> + smp_wmb();
> +
> + bnx2x_set_mac_addr_e1_gen(bp, set, bp->dev->dev_addr,
> + (1 << bp->fp->cl_id), (BP_PORT(bp) ? 32 : 0),
> + 1);
> +
> + /* Wait for a completion */
> + bnx2x_wait_ramrod(bp, 0, 0, &bp->set_mac_pending, set ? 0 : 1);
> +}
> +
> static int bnx2x_setup_leading(struct bnx2x *bp)
> {
> int rc;
> @@ -7452,9 +7503,9 @@ static int bnx2x_nic_load(struct bnx2x *bp, int load_mode)
> }
>
> if (CHIP_IS_E1(bp))
> - bnx2x_set_mac_addr_e1(bp, 1);
> + bnx2x_set_eth_mac_addr_e1(bp, 1);
> else
> - bnx2x_set_mac_addr_e1h(bp, 1);
> + bnx2x_set_eth_mac_addr_e1h(bp, 1);
> }
>
> if (bp->port.pmf)
> @@ -7717,7 +7768,7 @@ static int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode)
> struct mac_configuration_cmd *config =
> bnx2x_sp(bp, mcast_config);
>
> - bnx2x_set_mac_addr_e1(bp, 0);
> + bnx2x_set_eth_mac_addr_e1(bp, 0);
>
> for (i = 0; i < config->hdr.length; i++)
> CAM_INVALIDATE(config->config_table[i]);
> @@ -7730,6 +7781,9 @@ static int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode)
> config->hdr.client_id = bp->fp->cl_id;
> config->hdr.reserved1 = 0;
>
> + bp->set_mac_pending++;
> + smp_wmb();
> +
> bnx2x_sp_post(bp, RAMROD_CMD_ID_ETH_SET_MAC, 0,
> U64_HI(bnx2x_sp_mapping(bp, mcast_config)),
> U64_LO(bnx2x_sp_mapping(bp, mcast_config)), 0);
> @@ -7737,7 +7791,7 @@ static int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode)
> } else { /* E1H */
> REG_WR(bp, NIG_REG_LLH0_FUNC_EN + port*8, 0);
>
> - bnx2x_set_mac_addr_e1h(bp, 0);
> + bnx2x_set_eth_mac_addr_e1h(bp, 0);
>
> for (i = 0; i < MC_HASH_SIZE; i++)
> REG_WR(bp, MC_HASH_OFFSET(bp, i), 0);
> @@ -8520,6 +8574,14 @@ static void __devinit bnx2x_link_settings_requested(struct bnx2x *bp)
> bp->link_params.req_flow_ctrl, bp->port.advertising);
> }
>
> +static void __devinit bnx2x_set_mac_buf(u8 *mac_buf, u32 mac_lo, u16 mac_hi)
> +{
> + mac_hi = cpu_to_be16(mac_hi);
> + mac_lo = cpu_to_be32(mac_lo);
> + memcpy(mac_buf, &mac_hi, sizeof(mac_hi));
> + memcpy(mac_buf + sizeof(mac_hi), &mac_lo, sizeof(mac_lo));
> +}
> +
> static void __devinit bnx2x_get_port_hwinfo(struct bnx2x *bp)
> {
> int port = BP_PORT(bp);
> @@ -8601,12 +8663,7 @@ static void __devinit bnx2x_get_port_hwinfo(struct bnx2x *bp)
>
> val2 = SHMEM_RD(bp, dev_info.port_hw_config[port].mac_upper);
> val = SHMEM_RD(bp, dev_info.port_hw_config[port].mac_lower);
> - bp->dev->dev_addr[0] = (u8)(val2 >> 8 & 0xff);
> - bp->dev->dev_addr[1] = (u8)(val2 & 0xff);
> - bp->dev->dev_addr[2] = (u8)(val >> 24 & 0xff);
> - bp->dev->dev_addr[3] = (u8)(val >> 16 & 0xff);
> - bp->dev->dev_addr[4] = (u8)(val >> 8 & 0xff);
> - bp->dev->dev_addr[5] = (u8)(val & 0xff);
> + bnx2x_set_mac_buf(bp->dev->dev_addr, val, val2);
> memcpy(bp->link_params.mac_addr, bp->dev->dev_addr, ETH_ALEN);
> memcpy(bp->dev->perm_addr, bp->dev->dev_addr, ETH_ALEN);
> }
> @@ -10232,14 +10289,16 @@ static int bnx2x_test_intr(struct bnx2x *bp)
> config->hdr.client_id = bp->fp->cl_id;
> config->hdr.reserved1 = 0;
>
> + bp->set_mac_pending++;
> + smp_wmb();
> rc = bnx2x_sp_post(bp, RAMROD_CMD_ID_ETH_SET_MAC, 0,
> U64_HI(bnx2x_sp_mapping(bp, mac_config)),
> U64_LO(bnx2x_sp_mapping(bp, mac_config)), 0);
> if (rc == 0) {
> - bp->set_mac_pending++;
> for (i = 0; i < 10; i++) {
> if (!bp->set_mac_pending)
> break;
> + smp_rmb();
> msleep_interruptible(10);
> }
> if (i == 10)
> @@ -11337,6 +11396,9 @@ static void bnx2x_set_rx_mode(struct net_device *dev)
> config->hdr.client_id = bp->fp->cl_id;
> config->hdr.reserved1 = 0;
>
> + bp->set_mac_pending++;
> + smp_wmb();
> +
> bnx2x_sp_post(bp, RAMROD_CMD_ID_ETH_SET_MAC, 0,
> U64_HI(bnx2x_sp_mapping(bp, mcast_config)),
> U64_LO(bnx2x_sp_mapping(bp, mcast_config)),
> @@ -11386,9 +11448,9 @@ static int bnx2x_change_mac_addr(struct net_device *dev, void *p)
> memcpy(dev->dev_addr, addr->sa_data, dev->addr_len);
> if (netif_running(dev)) {
> if (CHIP_IS_E1(bp))
> - bnx2x_set_mac_addr_e1(bp, 1);
> + bnx2x_set_eth_mac_addr_e1(bp, 1);
> else
> - bnx2x_set_mac_addr_e1h(bp, 1);
> + bnx2x_set_eth_mac_addr_e1h(bp, 1);
> }
>
> return 0;
> --
> 1.6.4.GIT
>
>
> --
> 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
--
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