netdev - Re: [PATCH net-next 4/6] drivers: net: xgene-v2: Add base driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c6ae0303-4ac9-987b-5477-fa28ddc00f83@gmail.com>
Date:   Tue, 31 Jan 2017 12:31:52 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Iyappan Subramanian <isubramanian@....com>, davem@...emloft.net,
        netdev@...r.kernel.org
Cc:     kchudgar@....com, patches@....com,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH net-next 4/6] drivers: net: xgene-v2: Add base driver

On 01/31/2017 11:03 AM, Iyappan Subramanian wrote:
> This patch adds,
> 
>      - probe, remove, shutdown
>      - open, close and stats
>      - create and delete ring
>      - request and delete irq
> 
> Signed-off-by: Iyappan Subramanian <isubramanian@....com>
> Signed-off-by: Keyur Chudgar <kchudgar@....com>
> ---

> +static void xge_delete_desc_rings(struct net_device *ndev)
> +{
> +	struct xge_pdata *pdata = netdev_priv(ndev);
> +	struct device *dev = &pdata->pdev->dev;
> +	struct xge_desc_ring *ring;
> +
> +	ring = pdata->tx_ring;
> +	if (ring) {
> +		if (ring->skbs)
> +			devm_kfree(dev, ring->skbs);
> +		if (ring->pkt_bufs)
> +			devm_kfree(dev, ring->pkt_bufs);
> +		devm_kfree(dev, ring);
> +	}

The very fact that you have to do the devm_kfree suggests that the way
you manage the lifetime of the ring is not appropriate, and in fact, if
we look at how xge_create_desc_ring() is called, in the driver's probe
function indicates that if the network interface is never openeded, we
are just wasting memory sitting there and doing nothing. You should
consider moving this to the ndo_open(), resp. ndo_close() functions to
optimize memory consumption wrt. the network interface state.

> +
> +	ring = pdata->rx_ring;
> +	if (ring) {
> +		if (ring->skbs)
> +			devm_kfree(dev, ring->skbs);
> +		devm_kfree(dev, ring);
> +	}
> +}
> +
> +static struct xge_desc_ring *xge_create_desc_ring(struct net_device *ndev)
> +{
> +	struct xge_pdata *pdata = netdev_priv(ndev);
> +	struct device *dev = &pdata->pdev->dev;
> +	struct xge_desc_ring *ring;
> +	u16 size;
> +
> +	ring = devm_kzalloc(dev, sizeof(struct xge_desc_ring), GFP_KERNEL);
> +	if (!ring)
> +		return NULL;
> +
> +	ring->ndev = ndev;
> +
> +	size = XGENE_ENET_DESC_SIZE * XGENE_ENET_NUM_DESC;
> +	ring->desc_addr = dmam_alloc_coherent(dev, size, &ring->dma_addr,
> +					      GFP_KERNEL | __GFP_ZERO);

There is no dmam_zalloc_coherent()? Then again, that seems to be a
candidate for dma_zalloc_coherent() and moving this to the ndo_open()
function.

> +	if (!ring->desc_addr) {
> +		devm_kfree(dev, ring);
> +		return NULL;
> +	}
> +
> +	xge_setup_desc(ring);
> +
> +	return ring;
> +}
> +
> +static int xge_refill_buffers(struct net_device *ndev, u32 nbuf)
> +{
> +	struct xge_pdata *pdata = netdev_priv(ndev);
> +	struct xge_desc_ring *ring = pdata->rx_ring;
> +	const u8 slots = XGENE_ENET_NUM_DESC - 1;
> +	struct device *dev = &pdata->pdev->dev;
> +	struct xge_raw_desc *raw_desc;
> +	u64 addr_lo, addr_hi;
> +	u8 tail = ring->tail;
> +	struct sk_buff *skb;
> +	dma_addr_t dma_addr;
> +	u16 len;
> +	int i;
> +
> +	for (i = 0; i < nbuf; i++) {
> +		raw_desc = &ring->raw_desc[tail];
> +
> +		len = XGENE_ENET_STD_MTU;
> +		skb = netdev_alloc_skb(ndev, len);
> +		if (unlikely(!skb))
> +			return -ENOMEM;

Are not you leaving holes in your RX ring if you do that?

> +
> +		dma_addr = dma_map_single(dev, skb->data, len, DMA_FROM_DEVICE);
> +		if (dma_mapping_error(dev, dma_addr)) {
> +			netdev_err(ndev, "DMA mapping error\n");
> +			dev_kfree_skb_any(skb);
> +			return -EINVAL;
> +		}


> +static void xge_timeout(struct net_device *ndev)
> +{
> +	struct xge_pdata *pdata = netdev_priv(ndev);
> +	struct netdev_queue *txq;
> +
> +	xge_mac_reset(pdata);
> +
> +	txq = netdev_get_tx_queue(ndev, 0);
> +	txq->trans_start = jiffies;
> +	netif_tx_start_queue(txq);

It most likely is not that simple, don't you want to walk the list of
pending transmissed SKBs and free them all?

> +}
> +
> +static void xge_get_stats64(struct net_device *ndev,
> +			    struct rtnl_link_stats64 *storage)
> +{
> +	struct xge_pdata *pdata = netdev_priv(ndev);
> +	struct xge_stats *stats = &pdata->stats;
> +
> +	storage->tx_packets += stats->tx_packets;
> +	storage->tx_bytes += stats->tx_bytes;
> +
> +	storage->rx_packets += stats->rx_packets;
> +	storage->rx_bytes += stats->rx_bytes;

Pretty sure you need some synchronization primitives here for non 64-bit
architectures (maybe this driver is not used outside of 64-bit, but still).


> +
> +	ndev->hw_features = ndev->features;
> +
> +	ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64));
> +	if (ret) {
> +		netdev_err(ndev, "No usable DMA configuration\n");
> +		goto err;
> +	}
> +
> +	ret = xge_init_hw(ndev);
> +	if (ret)
> +		goto err;

Missing netif_carrier_off() right before the register_netdev().

> +
> +	ret = register_netdev(ndev);
> +	if (ret) {
> +		netdev_err(ndev, "Failed to register netdev\n");
> +		goto err;
> +	}



-- 
Florian