Re: [PATCH v2] VNIC: Adding support for Cavium ThunderX network controller
On 313, 11 09, 2014 at 09:14:05PM -0800, Robert Richter wrote:
I apologize for possibly repeated mail, my mailsystem was misconfigured :(
Some comments from quick look are below.
> +static int nic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> + struct device *dev = >dev;
> + struct net_device *netdev;
> + struct nicpf *nic;
> + interr;
> +
> + netdev = alloc_etherdev(sizeof(struct nicpf));
> + if (!netdev)
> + return -ENOMEM;
> +
> + pci_set_drvdata(pdev, netdev);
> +
> + SET_NETDEV_DEV(netdev, >dev);
> +
> + nic = netdev_priv(netdev);
> + nic->netdev = netdev;
> + nic->pdev = pdev;
> +
> + err = pci_enable_device(pdev);
> + if (err) {
> + dev_err(dev, "Failed to enable PCI device\n");
> + goto exit;
Looks like you leaked netdev here.
> + }
> +
> + err = pci_request_regions(pdev, DRV_NAME);
> + if (err) {
> + dev_err(dev, "PCI request regions failed 0x%x\n", err);
> + goto err_disable_device;
> + }
> +
> + err = pci_set_dma_mask(pdev, DMA_BIT_MASK(48));
> + if (err) {
> + dev_err(dev, "Unable to get usable DMA configuration\n");
> + goto err_release_regions;
> + }
> +
> + err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(48));
> + if (err) {
> + dev_err(dev, "unable to get 48-bit DMA for consistent
> allocations\n");
> + goto err_release_regions;
> + }
> +
> + /* MAP PF's configuration registers */
> + nic->reg_base = (uint64_t)pci_ioremap_bar(pdev, PCI_CFG_REG_BAR_NUM);
> + if (!nic->reg_base) {
> + dev_err(dev, "Cannot map config register space, aborting\n");
> + err = -ENOMEM;
> + goto err_release_regions;
> + }
> + nic->node = NIC_NODE_ID(pci_resource_start(pdev, PCI_CFG_REG_BAR_NUM));
> +
> + /* By default set NIC in TNS bypass mode */
> + nic->flags &= ~NIC_TNS_ENABLED;
> + nic_set_lmac_vf_mapping(nic);
> +
> + /* Initialize hardware */
> + nic_init_hw(nic);
> +
> + /* Set RSS TBL size for each VF */
> + nic->rss_ind_tbl_size = max((NIC_RSSI_COUNT / nic->num_vf_en),
> + NIC_MAX_RSS_IDR_TBL_SIZE);
> + nic->rss_ind_tbl_size = rounddown_pow_of_two(nic->rss_ind_tbl_size);
> +
> + /* Register interrupts */
> + if (nic_register_interrupts(nic))
> + goto err_unmap_resources;
> +
> + /* Configure SRIOV */
> + if (!nic_sriov_init(pdev, nic))
> + goto err_unmap_resources;
> +
> + goto exit;
Why not simply return 0; ?
> +
> +err_unmap_resources:
> + if (nic->reg_base)
This check looks unnecessary.
> + iounmap((void *)nic->reg_base);
> +err_release_regions:
> + pci_release_regions(pdev);
> +err_disable_device:
> + pci_disable_device(pdev);
> +exit:
> + return err;
> +}
> +static int bgx_register_interrupts(struct bgx *bgx, uint8_t lmac)
> +{
> + int irq, ret = 0;
> +
> + /* Register only link interrupts now */
> + irq = SPUX_INT + (lmac * BGX_LMAC_VEC_OFFSET);
> + sprintf(bgx->irq_name[irq], "LMAC%d", lmac);
> + ret = request_irq(bgx->msix_entries[irq].vector,
> + bgx_lmac_intr_handler, 0, bgx->irq_name[irq], bgx);
> + if (ret)
> + goto fail;
> + else
This else just hurts readability.
> + bgx->irq_allocated[irq] = 1;
> +
> + /* Enable link interrupt */
> + bgx_enable_link_intr(bgx, lmac);
> + return 0;
> +
> +fail:
The code below copypasted from bgx_unregister_interrupts()
Looks like good candidate for helper function.
> + dev_err(>pdev->dev, "Request irq failed\n");
> + for (irq = 0; irq < bgx->num_vec; irq++) {
> + if (bgx->irq_allocated[irq])
> + free_irq(bgx->msix_entries[irq].vector, bgx);
> + bgx->irq_allocated[irq] = 0;
> + }
> + return 1;
> +}
> +
> +static void bgx_unregister_interrupts(struct bgx *bgx)
> +{
> + int irq;
> + /* Free registered interrupts */
> + for (irq = 0; irq < bgx->num_vec; irq++) {
> + if (bgx->irq_allocated[irq])
> + free_irq(bgx->msix_entries[irq].vector, bgx);
> + bgx->irq_allocated[irq] = 0;
> + }
> + /* Disable MSI-X */
> + bgx_disable_msix(bgx);
> +}
> +
> +void bgx_lmac_enable(struct bgx *bgx, int8_t lmac)
> +{
> + uint64_t dmac_bcast = (1ULL << 48) - 1;
> +
> + bgx_reg_write(bgx, lmac, BGX_CMRX_CFG,
> + (1 << 15) | (1 << 14) | (1 << 13));
> +
> + /* Register interrupts */
> + bgx_register_interrupts(bgx, lmac);
bgx_register_interrupts() can fail (due to request_irq), but it's return
value is not checked.
> +
> + /* Add broadcast MAC into all LMAC's DMAC filters */
> + for (lmac = 0; lmac < bgx->lmac_count; lmac++)
> + bgx_add_dmac_addr(dmac_bcast, 0, bgx->bgx_id, lmac);
> +}
>
Re: [PATCH v2] VNIC: Adding support for Cavium ThunderX network controller
On 313, 11 09, 2014 at 09:14:05PM -0800, Robert Richter wrote:
I apologize for possibly repeated mail, my mailsystem was misconfigured :(
Some comments from quick look are below.
+static int nic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+{
+ struct device *dev = pdev-dev;
+ struct net_device *netdev;
+ struct nicpf *nic;
+ interr;
+
+ netdev = alloc_etherdev(sizeof(struct nicpf));
+ if (!netdev)
+ return -ENOMEM;
+
+ pci_set_drvdata(pdev, netdev);
+
+ SET_NETDEV_DEV(netdev, pdev-dev);
+
+ nic = netdev_priv(netdev);
+ nic-netdev = netdev;
+ nic-pdev = pdev;
+
+ err = pci_enable_device(pdev);
+ if (err) {
+ dev_err(dev, Failed to enable PCI device\n);
+ goto exit;
Looks like you leaked netdev here.
+ }
+
+ err = pci_request_regions(pdev, DRV_NAME);
+ if (err) {
+ dev_err(dev, PCI request regions failed 0x%x\n, err);
+ goto err_disable_device;
+ }
+
+ err = pci_set_dma_mask(pdev, DMA_BIT_MASK(48));
+ if (err) {
+ dev_err(dev, Unable to get usable DMA configuration\n);
+ goto err_release_regions;
+ }
+
+ err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(48));
+ if (err) {
+ dev_err(dev, unable to get 48-bit DMA for consistent
allocations\n);
+ goto err_release_regions;
+ }
+
+ /* MAP PF's configuration registers */
+ nic-reg_base = (uint64_t)pci_ioremap_bar(pdev, PCI_CFG_REG_BAR_NUM);
+ if (!nic-reg_base) {
+ dev_err(dev, Cannot map config register space, aborting\n);
+ err = -ENOMEM;
+ goto err_release_regions;
+ }
+ nic-node = NIC_NODE_ID(pci_resource_start(pdev, PCI_CFG_REG_BAR_NUM));
+
+ /* By default set NIC in TNS bypass mode */
+ nic-flags = ~NIC_TNS_ENABLED;
+ nic_set_lmac_vf_mapping(nic);
+
+ /* Initialize hardware */
+ nic_init_hw(nic);
+
+ /* Set RSS TBL size for each VF */
+ nic-rss_ind_tbl_size = max((NIC_RSSI_COUNT / nic-num_vf_en),
+ NIC_MAX_RSS_IDR_TBL_SIZE);
+ nic-rss_ind_tbl_size = rounddown_pow_of_two(nic-rss_ind_tbl_size);
+
+ /* Register interrupts */
+ if (nic_register_interrupts(nic))
+ goto err_unmap_resources;
+
+ /* Configure SRIOV */
+ if (!nic_sriov_init(pdev, nic))
+ goto err_unmap_resources;
+
+ goto exit;
Why not simply return 0; ?
+
+err_unmap_resources:
+ if (nic-reg_base)
This check looks unnecessary.
+ iounmap((void *)nic-reg_base);
+err_release_regions:
+ pci_release_regions(pdev);
+err_disable_device:
+ pci_disable_device(pdev);
+exit:
+ return err;
+}
+static int bgx_register_interrupts(struct bgx *bgx, uint8_t lmac)
+{
+ int irq, ret = 0;
+
+ /* Register only link interrupts now */
+ irq = SPUX_INT + (lmac * BGX_LMAC_VEC_OFFSET);
+ sprintf(bgx-irq_name[irq], LMAC%d, lmac);
+ ret = request_irq(bgx-msix_entries[irq].vector,
+ bgx_lmac_intr_handler, 0, bgx-irq_name[irq], bgx);
+ if (ret)
+ goto fail;
+ else
This else just hurts readability.
+ bgx-irq_allocated[irq] = 1;
+
+ /* Enable link interrupt */
+ bgx_enable_link_intr(bgx, lmac);
+ return 0;
+
+fail:
The code below copypasted from bgx_unregister_interrupts()
Looks like good candidate for helper function.
+ dev_err(bgx-pdev-dev, Request irq failed\n);
+ for (irq = 0; irq bgx-num_vec; irq++) {
+ if (bgx-irq_allocated[irq])
+ free_irq(bgx-msix_entries[irq].vector, bgx);
+ bgx-irq_allocated[irq] = 0;
+ }
+ return 1;
+}
+
+static void bgx_unregister_interrupts(struct bgx *bgx)
+{
+ int irq;
+ /* Free registered interrupts */
+ for (irq = 0; irq bgx-num_vec; irq++) {
+ if (bgx-irq_allocated[irq])
+ free_irq(bgx-msix_entries[irq].vector, bgx);
+ bgx-irq_allocated[irq] = 0;
+ }
+ /* Disable MSI-X */
+ bgx_disable_msix(bgx);
+}
+
+void bgx_lmac_enable(struct bgx *bgx, int8_t lmac)
+{
+ uint64_t dmac_bcast = (1ULL 48) - 1;
+
+ bgx_reg_write(bgx, lmac, BGX_CMRX_CFG,
+ (1 15) | (1 14) | (1 13));
+
+ /* Register interrupts */
+ bgx_register_interrupts(bgx, lmac);
bgx_register_interrupts() can fail (due to request_irq), but it's return
value is not checked.
+
+ /* Add broadcast MAC into all LMAC's DMAC filters */
+ for (lmac = 0; lmac bgx-lmac_count; lmac++)
+ bgx_add_dmac_addr(dmac_bcast, 0, bgx-bgx_id, lmac);
+}
+static int bgx_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+{
+ struct device *dev = pdev-dev;
+ struct bgx *bgx;
+ interr;
+ uint8_t lmac
Re: [PATCH v2] VNIC: Adding support for Cavium ThunderX network controller
From: Robert Richter Date: Sun, 9 Nov 2014 21:14:05 -0800 > +config NET_VENDOR_CAVIUM ... > +config THUNDER_NIC_PF ... > +config THUNDER_NIC_VF ... > +config THUNDER_NIC_BGX These config options seem excessive, if not confusing. What would a distribution be expected to enable? Everything? > +# Don't change the order, NICPF driver is dependent on BGX driver init > +obj-$(CONFIG_THUNDER_NIC_BGX) += thunder_bgx.o > +obj-$(CONFIG_THUNDER_NIC_PF) += nicpf.o > +obj-$(CONFIG_THUNDER_NIC_VF) += nicvf.o Nothing ensures ordering if these things are built all modular. Such ordering dependencies need to be resolved in another way. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] VNIC: Adding support for Cavium ThunderX network controller
From: Robert Richter r...@kernel.org Date: Sun, 9 Nov 2014 21:14:05 -0800 +config NET_VENDOR_CAVIUM ... +config THUNDER_NIC_PF ... +config THUNDER_NIC_VF ... +config THUNDER_NIC_BGX These config options seem excessive, if not confusing. What would a distribution be expected to enable? Everything? +# Don't change the order, NICPF driver is dependent on BGX driver init +obj-$(CONFIG_THUNDER_NIC_BGX) += thunder_bgx.o +obj-$(CONFIG_THUNDER_NIC_PF) += nicpf.o +obj-$(CONFIG_THUNDER_NIC_VF) += nicvf.o Nothing ensures ordering if these things are built all modular. Such ordering dependencies need to be resolved in another way. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
