| 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: <4B4B15ED.7070201@qlogic.com>
Date: Mon, 11 Jan 2010 17:43:33 +0530
From: Amit Salecha <amit.salecha@...gic.com>
To: Ben Hutchings <bhutchings@...arflare.com>
CC: "davem@...emloft.net" <davem@...emloft.net>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Dhananjay Phadke <dhananjay.phadke@...gic.com>
Subject: Re: [PATCHv3 NEXT 1/2] qlcnic: Qlogic ethernet driver for CNA devices
Hi
Thanks for review.
I will send updated patches as v4, incorporating valid suggestion.
Regards
Amit Salecha
Ben Hutchings wrote:
> On Thu, 2010-01-07 at 04:35 -0800, Amit Kumar Salecha wrote:
>
>> o 1G/10G Ethernet Driver for Qlgic QLE8240 and QLE8242 CNA devices.
>>
>
> 'o'? You're addressing this message to the driver? :-)
>
> [...]
>
>> --- /dev/null
>> +++ b/drivers/net/qlcnic/Makefile
>> @@ -0,0 +1,28 @@
>> +# Copyright (C) 2009 - QLogic Corporation.
>> +# All rights reserved.
>> +#
>> +# This program is free software; you can redistribute it and/or
>> +# modify it under the terms of the GNU General Public License
>> +# as published by the Free Software Foundation; either version 2
>> +# of the License, or (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful, but
>> +# WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write to the Free Software
>> +# Foundation, Inc., 59 Temple Place - Suite 330, Boston,
>> +# MA 02111-1307, USA.
>> +#
>> +# The full GNU General Public License is included in this distribution
>> +# in the file called LICENSE.
>> +#
>> +#
>>
>
> This is a trivial Makefile; do you really think you need to use 5 times
> as many lines to assert copyright over it?
>
>
>> +obj-$(CONFIG_QLCNIC) := qlcnic.o
>> +
>> +qlcnic-y := qlcnic_hw.o qlcnic_main.o qlcnic_init.o \
>> + qlcnic_ethtool.o qlcnic_ctx.o
>> diff --git a/drivers/net/qlcnic/qlcnic.h b/drivers/net/qlcnic/qlcnic.h
>> new file mode 100644
>> index 0000000..be48472
>> --- /dev/null
>> +++ b/drivers/net/qlcnic/qlcnic.h
>> @@ -0,0 +1,1318 @@
>> +/*
>> + * Copyright (C) 2009 - QLogic Corporation.
>> + * All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version 2
>> + * of the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston,
>> + * MA 02111-1307, USA.
>> + *
>> + * The full GNU General Public License is included in this distribution
>> + * in the file called LICENSE.
>>
>
> It may be called 'LICENSE' in your out-of-tree distribution, but it is
> called 'COPYING' in the kernel distribution.
>
> [...]
>
>> +#define _QLCNIC_LINUX_MAJOR 5
>> +#define _QLCNIC_LINUX_MINOR 0
>> +#define _QLCNIC_LINUX_SUBVERSION 0
>> +#define QLCNIC_LINUX_VERSIONID "5.0.0"
>> +
>> +#define QLCNIC_VERSION_CODE(a, b, c) (((a) << 24) + ((b) << 16) + (c))
>> +#define _major(v) (((v) >> 24) & 0xff)
>> +#define _minor(v) (((v) >> 16) & 0xff)
>> +#define _build(v) ((v) & 0xffff)
>> +
>> +/* version in image has weird encoding:
>> + * 7:0 - major
>> + * 15:8 - minor
>> + * 31:16 - build (little endian)
>> + */
>> +#define QLCNIC_DECODE_VERSION(v) \
>> + QLCNIC_VERSION_CODE(((v) & 0xff), (((v) >> 8) & 0xff), ((v) >> 16))
>>
>
> You might be over-engineering this...
>
>
>> +#define QLCNIC_NUM_FLASH_SECTORS (64)
>> +#define QLCNIC_FLASH_SECTOR_SIZE (64 * 1024)
>> +#define QLCNIC_FLASH_TOTAL_SIZE (QLCNIC_NUM_FLASH_SECTORS \
>> + * QLCNIC_FLASH_SECTOR_SIZE)
>>
>
> You might consider probing for this rather than assuming the flash
> dimensions are going to stay constant.
>
> [...]
>
>> +#define find_diff_among(a, b, range) ((a) < (b) ? ((b)-(a)) : ((b)+(range)-(a)))
>>
>
> This is not a very clear name. It seems to be used only once, in
> qlcnic_tx_avail(). Perhaps it would be clearer to open-code it there?
>
>
>> +#define QLCNIC_RCV_PRODUCER_OFFSET 0
>> +#define QLCNIC_RCV_PEG_DB_ID 2
>> +#define QLCNIC_HOST_DUMMY_DMA_SIZE 1024
>> +#define FLASH_SUCCESS 0
>> +
>> +#define ADDR_IN_WINDOW1(off) \
>> + ((off > QLCNIC_CRB_PCIX_HOST2) && (off < QLCNIC_CRB_MAX)) ? 1 : 0
>>
>
> The final '? 1 : 0' is redundant since the '&&' operator always yields a
> value of 1 or 0. It is also unsafe since there are no parentheses
> around the whole of the macro expansion.
>
> Also 'off' should be parenthesised.
>
> [...]
>
>> +#define QLCNIC_ETHERMTU 1500
>>
>
> Already named ETH_DATA_LEN.
>
> [...]
>
>> +#define MAX_NUM_CARDS 4
>>
>
> You want to limit customers to 4 NICs? Ah, but you don't actually use
> this anywhere. So delete it.
>
> [...]
>
>> +#define qlcnic_set_msg_peg_id(config_word, val) \
>> + ((config_word) &= ~3, (config_word) |= val & 3)
>> +#define qlcnic_set_msg_privid(config_word) \
>> + ((config_word) |= 1 << 2)
>> +#define qlcnic_set_msg_count(config_word, val) \
>> + ((config_word) &= ~(0x7fff<<3), (config_word) |= (val & 0x7fff) << 3)
>> +#define qlcnic_set_msg_ctxid(config_word, val) \
>> + ((config_word) &= ~(0x3ff<<18), (config_word) |= (val & 0x3ff) << 18)
>> +#define qlcnic_set_msg_opcode(config_word, val) \
>> + ((config_word) &= ~(0xf<<28), (config_word) |= (val & 0xf) << 28)
>>
>
> 'val' should be in parentheses.
>
> [...]
>
>> +#define MINIMUM_ETHERNET_FRAME_SIZE 64 /* With FCS */
>>
>
> This name isn't used.
>
>
>> +#define ETHERNET_FCS_SIZE 4
>>
>
> Already named ETH_FCS_LEN.
>
> [...]
>
>> +union qlcnic_nic_intr_coalesce_data_t {
>> + struct {
>> + uint16_t rx_packets;
>> + uint16_t rx_time_us;
>> + uint16_t tx_packets;
>> + uint16_t tx_time_us;
>> + } data;
>> + uint64_t word;
>> +};
>> +
>> +struct qlcnic_nic_intr_coalesce_t {
>> + uint16_t stats_time_us;
>> + uint16_t rate_sample_time;
>> + uint16_t flags;
>> + uint16_t rsvd_1;
>> + uint32_t low_threshold;
>> + uint32_t high_threshold;
>> + union qlcnic_nic_intr_coalesce_data_t normal;
>> + union qlcnic_nic_intr_coalesce_data_t low;
>> + union qlcnic_nic_intr_coalesce_data_t high;
>> + union qlcnic_nic_intr_coalesce_data_t irq;
>> +};
>>
>
> I believe these are hardware structure definitions so you need to use
> __le16, __le32, __le64. If they are in host byte order, use u16, u32, u64.
>
> Also drop the '_t'; we don't use such suffixes for aggregate type names.
>
> [...]
>
>> diff --git a/drivers/net/qlcnic/qlcnic_ctx.c b/drivers/net/qlcnic/qlcnic_ctx.c
>> new file mode 100644
>> index 0000000..f056b73
>> --- /dev/null
>>
> [...]
>
>> +static u32
>> +qlcnic_issue_cmd(struct qlcnic_adapter *adapter,
>> + u32 pci_fn, u32 version, u32 arg1, u32 arg2, u32 arg3, u32 cmd)
>> +{
>> + u32 rsp;
>> + u32 signature = 0;
>> + u32 rcode = QLCNIC_RCODE_SUCCESS;
>> +
>> + signature = QLCNIC_CDRP_SIGNATURE_MAKE(pci_fn, version);
>> +
>> + /* Acquire semaphore before accessing CRB */
>> + if (qlcnic_api_lock(adapter))
>> + return QLCNIC_RCODE_TIMEOUT;
>> +
>> + QLCWR32(adapter, QLCNIC_SIGN_CRB_OFFSET, signature);
>> +
>> + QLCWR32(adapter, QLCNIC_ARG1_CRB_OFFSET, arg1);
>> +
>> + QLCWR32(adapter, QLCNIC_ARG2_CRB_OFFSET, arg2);
>> +
>> + QLCWR32(adapter, QLCNIC_ARG3_CRB_OFFSET, arg3);
>> +
>> + QLCWR32(adapter, QLCNIC_CDRP_CRB_OFFSET, QLCNIC_CDRP_FORM_CMD(cmd));
>>
>
> I'm not sure
>
> that spacing this out
>
> really makes it more readable.
>
> [...]
>
>> + if (err) {
>> + printk(KERN_WARNING
>> + "Failed to create rx ctx in firmware%d\n", err);
>> + goto out_free_rsp;
>> + }
>>
>
> Please use dev_warn(), dev_err(), dev_info() etc. consistently. I note
> that you are doing so in some places, but there are a lot of printk()s
> left.
>
> [...]
>
>> +static int
>> +qlcnic_fw_cmd_create_tx_ctx(struct qlcnic_adapter *adapter)
>> +{
>>
> [...]
>
>> +#if 0
>> + adapter->tx_state =
>> + le32_to_cpu(prsp->host_ctx_state);
>> +#endif
>>
>
> Don't use #if 0 - either enable the code or remove it.
>
> [...]
>
>> +void qlcnic_free_hw_resources(struct qlcnic_adapter *adapter)
>> +{
>> + struct qlcnic_recv_context *recv_ctx;
>> + struct qlcnic_host_rds_ring *rds_ring;
>> + struct qlcnic_host_sds_ring *sds_ring;
>> + struct qlcnic_host_tx_ring *tx_ring;
>> + int ring;
>> +
>> +
>> + if (!test_and_clear_bit(__QLCNIC_FW_ATTACHED, &adapter->state))
>> + goto done;
>> +
>> + qlcnic_fw_cmd_destroy_rx_ctx(adapter);
>> + qlcnic_fw_cmd_destroy_tx_ctx(adapter);
>> +
>> + /* Allow dma queues to drain after context reset */
>> + msleep(20);
>> +
>> +done:
>>
>
> It would be clearer to make the previous three statements a conditional
> block.
>
> [...]
>
>> diff --git a/drivers/net/qlcnic/qlcnic_ethtool.c b/drivers/net/qlcnic/qlcnic_ethtool.c
>> new file mode 100644
>> index 0000000..daa192f
>> --- /dev/null
>>
> [...]
>
>> +#define QLC_SIZEOF(m) sizeof(((struct qlcnic_adapter *)0)->m)
>>
>
> Could be defined as 'FIELD_SIZEOF(struct qlnic_adapter, m)'.
>
> [...]
>
>> +static void
>> +qlcnic_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *drvinfo)
>> +{
>> + struct qlcnic_adapter *adapter = netdev_priv(dev);
>> + u32 fw_major = 0;
>> + u32 fw_minor = 0;
>> + u32 fw_build = 0;
>>
>
> Don't initialise variables to dummy values unless you actually intend them to
> be used (clearly not the case here). This 'defensive coding' just stops the
> compiler from warning if you do forget to assign the proper value later.
>
>
>> + strncpy(drvinfo->driver, qlcnic_driver_name, 32);
>> + strncpy(drvinfo->version, QLCNIC_LINUX_VERSIONID, 32);
>> + fw_major = QLCRD32(adapter, QLCNIC_FW_VERSION_MAJOR);
>> + fw_minor = QLCRD32(adapter, QLCNIC_FW_VERSION_MINOR);
>> + fw_build = QLCRD32(adapter, QLCNIC_FW_VERSION_SUB);
>> + sprintf(drvinfo->fw_version, "%d.%d.%d", fw_major, fw_minor, fw_build);
>> +
>> + strncpy(drvinfo->bus_info, pci_name(adapter->pdev), 32);
>>
>
> It shouldn't make any difference here, but the proper functions for limited-
> length null-terminated strings are strlcpy() and snprintf().
>
>
>> + drvinfo->regdump_len = qlcnic_get_regs_len(dev);
>> + drvinfo->eedump_len = qlcnic_get_eeprom_len(dev);
>>
>
> The ethtool core does this for you.
>
>
>> +}
>> +
>> +static int
>> +qlcnic_get_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
>> +{
>>
> [...]
>
>> + } else if (adapter->ahw.port_type == QLCNIC_XGBE) {
>> + u32 val;
>> +
>> + val = QLCRD32(adapter, QLCNIC_PORT_MODE_ADDR);
>> + if (val == QLCNIC_PORT_MODE_802_3_AP) {
>> + ecmd->supported = SUPPORTED_1000baseT_Full;
>> + ecmd->advertising = ADVERTISED_1000baseT_Full;
>> + } else {
>> + ecmd->supported = SUPPORTED_10000baseT_Full;
>> + ecmd->advertising = ADVERTISED_10000baseT_Full;
>> + }
>>
>
> Don't abuse the TP mode flags and port type for other media.
>
> Coax and twinax should be reported as port = PORT_OTHER.
>
> If you have KX/KX4/KR autoneg ports then there are separate mode flags
> for them.
>
> [...]
>
>> diff --git a/drivers/net/qlcnic/qlcnic_hdr.h b/drivers/net/qlcnic/qlcnic_hdr.h
>> new file mode 100644
>> index 0000000..5d95ea3
>> --- /dev/null
>> +++ b/drivers/net/qlcnic/qlcnic_hdr.h
>>
> [...]
>
>> +#define FW_POLL_DELAY round_jiffies(2 * HZ)
>>
>
> I think you're looking for round_jiffies_relative(). It would make more
> sense to do the rounding in qlcnic_schedule_work() than each caller,
> though.
>
> [...]
>
>> diff --git a/drivers/net/qlcnic/qlcnic_hw.c b/drivers/net/qlcnic/qlcnic_hw.c
>> new file mode 100644
>> index 0000000..aa7afbe
>> --- /dev/null
>> +++ b/drivers/net/qlcnic/qlcnic_hw.c
>>
> [...]
>
>> +/*
>> + * qlcnic_change_mtu - Change the Maximum Transfer Unit
>> + * @returns 0 on success, negative on failure
>> + */
>> +
>> +#define MTU_FUDGE_FACTOR 100
>>
>
> Unused?
>
>
>> +int qlcnic_change_mtu(struct net_device *netdev, int mtu)
>> +{
>> + struct qlcnic_adapter *adapter = netdev_priv(netdev);
>> + int max_mtu;
>> + int rc = 0;
>> +
>> + max_mtu = P3_MAX_MTU;
>> +
>> + if (mtu > max_mtu) {
>> + printk(KERN_ERR "%s: mtu > %d bytes unsupported\n",
>> + netdev->name, max_mtu);
>>
>
> Why not just use P3_MAX_MTU directly instead of the 'variable' max_mtu?
>
> [...]
>
>> +int qlcnic_get_mac_addr(struct qlcnic_adapter *adapter, __le64 *mac)
>> +{
>> + uint32_t crbaddr, mac_hi, mac_lo;
>> + int pci_func = adapter->ahw.pci_func;
>> +
>> + crbaddr = CRB_MAC_BLOCK_START +
>> + (4 * ((pci_func/2) * 3)) + (4 * (pci_func & 1));
>> +
>> + mac_lo = QLCRD32(adapter, crbaddr);
>> + mac_hi = QLCRD32(adapter, crbaddr+4);
>> +
>> + if (pci_func & 1)
>> + *mac = le64_to_cpu((mac_lo >> 16) | ((u64)mac_hi << 16));
>> + else
>> + *mac = le64_to_cpu((u64)mac_lo | ((u64)mac_hi << 32));
>>
> [...]
>
> *mac has type __le64, so either these le64_to_cpu() calls are wrong or
> mac has been declared wrongly.
>
> I didn't read beyond here.
>
> Ben.
>
> --
> Ben Hutchings, Senior Software Engineer, Solarflare Communications
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
--
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