| 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: <20120308114859.GA23807@electric-eye.fr.zoreil.com>
Date: Thu, 8 Mar 2012 12:48:59 +0100
From: Francois Romieu <romieu@...zoreil.com>
To: parav.pandit@...lex.com
Cc: netdev@...r.kernel.org
Subject: Re: [RFC 2/2] be2net: Added functionality to support RoCE driver
parav.pandit@...lex.com <parav.pandit@...lex.com> :
[...]
> diff --git a/drivers/net/ethernet/emulex/benet/Makefile b/drivers/net/ethernet/emulex/benet/Makefile
> index a60cd80..1a91b27 100644
> --- a/drivers/net/ethernet/emulex/benet/Makefile
> +++ b/drivers/net/ethernet/emulex/benet/Makefile
[...]
> @@ -372,6 +374,14 @@ struct be_adapter {
> u8 transceiver;
> u8 autoneg;
> u8 generation; /* BladeEngine ASIC generation */
> + u32 if_type;
> + u8 __iomem *roce_db; /* Door Bell */
> + u32 roce_db_size;
> + u32 roce_db_total_size;
> + u64 roce_db_io_addr;
There may be room for some 'struct roce_db'.
> + void *ocrdma_dev;
Is there a reason why it could not be 'struct ocrdma_dev *o' ?
> + struct list_head entry;
> +
> u32 flash_status;
> struct completion flash_compl;
>
> @@ -398,6 +408,9 @@ struct be_adapter {
> #define OFF 0
> #define lancer_chip(adapter) ((adapter->pdev->device == OC_DEVICE_ID3) || \
> (adapter->pdev->device == OC_DEVICE_ID4))
> +#define be_roce_supported(adapter) ((adapter->if_type == SLI_INTF_TYPE_3 || \
> + adapter->sli_family == SKYHAWK_SLI_FAMILY) && \
> + (adapter->function_mode & RDMA_ENABLED))
No need for a macro. Could be static inline.
>
> extern const struct ethtool_ops be_ethtool_ops;
>
> @@ -544,4 +557,17 @@ extern void be_cq_notify(struct be_adapter *adapter, u16 qid, bool arm,
> extern void be_link_status_update(struct be_adapter *adapter, u8 link_status);
> extern void be_parse_stats(struct be_adapter *adapter);
> extern int be_load_fw(struct be_adapter *adapter, u8 *func);
> +
> +/*
> + * internal function to initialize-cleanup roce device.
> + */
> +extern void be_roce_dev_add(struct be_adapter *adapter);
> +extern void be_roce_dev_remove(struct be_adapter *adapter);
> +
> +/*
> + * internal function to open-close roce device during ifup-ifdown.
> + */
> +extern void be_roce_dev_open(struct be_adapter *adapter);
> +extern void be_roce_dev_close(struct be_adapter *adapter);
Nit: the name of the argument does not matter much here.
[...]
> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
> index e703d64..9554cc9 100644
> --- a/drivers/net/ethernet/emulex/benet/be_main.c
> +++ b/drivers/net/ethernet/emulex/benet/be_main.c
[...]
> @@ -3139,19 +3155,45 @@ static void be_unmap_pci_bars(struct be_adapter *adapter)
> iounmap(adapter->csr);
> if (adapter->db)
> iounmap(adapter->db);
> + if (adapter->roce_db)
> + iounmap(adapter->roce_db);
> +}
> +
> +static int lancer_roce_map_pci_bars(struct be_adapter *adapter)
> +{
> + struct pci_dev *pdev = adapter->pdev;
> + u8 __iomem *addr;
> + addr = ioremap_nocache(pci_resource_start(pdev, 2),
> + pci_resource_len(pdev, 2));
- missing empty line.
- use pci_iomap
> + if (addr == NULL)
> + return -ENOMEM;
> +
> + adapter->roce_db = addr;
> + adapter->roce_db_io_addr = pci_resource_start(pdev, 2);
> + adapter->roce_db_size = 8192;
> + adapter->roce_db_total_size = pci_resource_len(pdev, 2);
> + return 0;
> }
>
> static int be_map_pci_bars(struct be_adapter *adapter)
> {
> + struct pci_dev *pdev = adapter->pdev;
> u8 __iomem *addr;
> int db_reg;
>
> if (lancer_chip(adapter)) {
> - addr = ioremap_nocache(pci_resource_start(adapter->pdev, 0),
> - pci_resource_len(adapter->pdev, 0));
> - if (addr == NULL)
> - return -ENOMEM;
> - adapter->db = addr;
> + if (adapter->if_type == SLI_INTF_TYPE_2 ||
> + adapter->if_type == SLI_INTF_TYPE_3) {
> + addr = ioremap_nocache(pci_resource_start(pdev, 0),
> + pci_resource_len(adapter->pdev, 0));
pci_iomap
> + if (addr == NULL)
> + return -ENOMEM;
> + adapter->db = addr;
> + }
> + if (adapter->if_type == SLI_INTF_TYPE_3) {
> + if (lancer_roce_map_pci_bars(adapter))
> + goto pci_map_err;
> + }
> return 0;
> }
>
[...]
> @@ -3353,7 +3400,21 @@ static int be_dev_family_check(struct be_adapter *adapter)
> SLI_INTF_IF_TYPE_SHIFT;
>
> if (((sli_intf & SLI_INTF_VALID_MASK) != SLI_INTF_VALID) ||
> - if_type != 0x02) {
> + ((if_type != SLI_INTF_TYPE_2) &&
> + (if_type != SLI_INTF_TYPE_3))) {
You can remove some parenthesis.
It may benefit from a 'bool be_type_2_3' (could be used twice).
[...]
> diff --git a/drivers/net/ethernet/emulex/benet/be_roce.c b/drivers/net/ethernet/emulex/benet/be_roce.c
> new file mode 100644
> index 0000000..7002160
> --- /dev/null
> +++ b/drivers/net/ethernet/emulex/benet/be_roce.c
> @@ -0,0 +1,185 @@
[...]
> +static struct ocrdma_driver *ocrdma_drv;
> +static LIST_HEAD(be_adapter_list);
> +static DEFINE_MUTEX(be_adapter_list_lock);
> +
> +static void _be_roce_dev_add(struct be_adapter *adapter)
> +{
> + struct be_dev_info dev_info;
> + int i, roce_vect_cnt = 0;
roce_vect_cnt : excess visibility and useless init
> + struct pci_dev *pdev = adapter->pdev;
> +
> + if (!ocrdma_drv || !ocrdma_drv->add || adapter->ocrdma_dev)
> + return;
The registration logic seems a bit convoluted.
Are there real use-cases where the device and the ocrdma_drv could be
loaded in different relative ordering ?
> + if (pdev->device == OC_DEVICE_ID5) {
> + /* only msix is supported on these devices */
> + if (!msix_enabled(adapter))
> + return;
> + /* DPP region address and length */
> + dev_info.dpp_unmapped_addr = pci_resource_start(pdev, 2);
> + dev_info.dpp_unmapped_len = pci_resource_len(pdev, 2);
> + } else {
> + dev_info.dpp_unmapped_addr = 0;
> + dev_info.dpp_unmapped_len = 0;
> + }
> + dev_info.pdev = adapter->pdev;
> + if (adapter->sli_family == SKYHAWK_SLI_FAMILY)
> + dev_info.db = adapter->db;
> + else
> + dev_info.db = adapter->roce_db;
> + dev_info.unmapped_db = adapter->roce_db_io_addr;
> + dev_info.db_page_size = adapter->roce_db_size;
> + dev_info.db_total_size = adapter->roce_db_total_size;
> + dev_info.netdev = adapter->netdev;
> + memcpy(dev_info.mac_addr, adapter->netdev->dev_addr, ETH_ALEN);
> + dev_info.dev_family = adapter->sli_family;
> + dev_info.vendor_id = adapter->pdev->vendor;
> + dev_info.device_id = adapter->pdev->device;
I wonder why you peel struct pci_dev so early.
> + if (msix_enabled(adapter)) {
> + /* provide all the vectors, so that EQ creation response
> + * can decide which one to use.
> + */
> + roce_vect_cnt = adapter->num_msix_vec;
> + dev_info.intr_mode = BE_INTERRUPT_MODE_MSIX;
> + dev_info.msix.num_vectors =
> + min(roce_vect_cnt, MAX_ROCE_MSIX_VECTORS);
> + /* provide start index of the vector,
> + * so in case of linear usage,
> + * it can use the base as starting point.
> + */
> + dev_info.msix.start_vector = adapter->num_eqs;
> + for (i = 0; i < dev_info.msix.num_vectors; i++)
> + dev_info.msix.vector_list[i] =
> + adapter->msix_entries[i].vector;
Parenthesis around the for statement ?
[...]
> +void be_roce_dev_close(struct be_adapter *adapter)
> +{
> + if (be_roce_supported(adapter)) {
> + mutex_lock(&be_adapter_list_lock);
> + _be_roce_dev_close(adapter);
> + mutex_unlock(&be_adapter_list_lock);
> + }
> +}
There should be no need to check for be_roce_supported() once the probe
is done.
> +
> +int be_roce_register_driver(struct ocrdma_driver *drv)
> +{
> + struct be_adapter *dev;
> + struct net_device *netdev = NULL;
netdev should be in the list_for_each_entry loop.
> + int status = 0;
> +
> + mutex_lock(&be_adapter_list_lock);
> + if (ocrdma_drv) {
> + mutex_unlock(&be_adapter_list_lock);
> + return -EINVAL;
Either use goto or the status variable is not needed.
> + }
> + ocrdma_drv = drv;
> + list_for_each_entry(dev, &be_adapter_list, entry) {
> + _be_roce_dev_add(dev);
> + netdev = dev->netdev;
> + if (netif_running(netdev) && netif_oper_up(netdev))
> + _be_roce_dev_open(dev);
> + }
> + mutex_unlock(&be_adapter_list_lock);
> + return status;
> +}
> +EXPORT_SYMBOL(be_roce_register_driver);
> +
> +int be_roce_unregister_driver(struct ocrdma_driver *drv)
It always return 0 : make it void.
> +{
> + struct be_adapter *dev;
> + mutex_lock(&be_adapter_list_lock);
Missing empty line.
[...]
> diff --git a/drivers/net/ethernet/emulex/benet/be_roce.h b/drivers/net/ethernet/emulex/benet/be_roce.h
> new file mode 100644
> index 0000000..0bb3634
> --- /dev/null
> +++ b/drivers/net/ethernet/emulex/benet/be_roce.h
[...]
> +struct pci_dev;
> +struct net_device;
Why not a proper #include ?
> +
> +enum be_interrupt_mode {
> + BE_INTERRUPT_MODE_MSIX = 0,
> + BE_INTERRUPT_MODE_INTX = 1,
> + BE_INTERRUPT_MODE_MSI = 2,
Please <tab>= instead of <space>=
> +};
> +
> +#define MAX_ROCE_MSIX_VECTORS 16
> +struct be_dev_info {
> + u16 vendor_id;
> + u16 device_id;
See remark above. I'd rather keep the relevant pci_dev around.
[...]
> +/* ocrdma driver register's the callback functions with nic driver. */
> +struct ocrdma_driver {
> + unsigned char name[32];
> + void *(*add) (struct be_dev_info *dev_info);
> + void (*remove) (void *device_handle);
> + void (*state_change_handler) (void *roce_handle, u32 new_state);
Please don't abuse void * like that. :o(
--
Ueimor
--
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