netdev - RE: [PATCH 2/2] netdev: driver: ethernet: Add TI CPSW 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] [day] [month] [year] [list]

Message-ID: <EB1619762EAF8B4E97A227FB77B7E0293178BBA9@DBDE01.ent.ti.com>
Date:	Fri, 17 Feb 2012 07:17:43 +0000
From:	"N, Mugunthan V" <mugunthanvnm@...com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>
Subject: RE: [PATCH 2/2] netdev: driver: ethernet: Add TI CPSW driver

Eric Dumazet

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@...il.com]
> Sent: Monday, February 13, 2012 11:45 PM
> To: N, Mugunthan V
> Cc: netdev@...r.kernel.org; davem@...emloft.net
> Subject: Re: [PATCH 2/2] netdev: driver: ethernet: Add TI CPSW driver
> 
> Le lundi 13 février 2012 à 11:34 +0530, Mugunthan V N a écrit :
> > This patch adds support for TI's CPSW driver.
> >
> > The three port switch gigabit ethernet subsystem provides ethernet
> packet
> > communication and can be configured as an ethernet switch. Supports
> > 10/100/1000 Mbps.
> >
> > Signed-off-by: Cyril Chemparathy <cyril@...com>
> > Signed-off-by: Sriramakrishnan A G <srk@...com>
> > Signed-off-by: Mugunthan V N <mugunthanvnm@...com>
> 
> ...
> 
> > +
> > +static netdev_tx_t cpsw_ndo_start_xmit(struct sk_buff *skb,
> > +				       struct net_device *ndev)
> > +{
> > +	struct cpsw_priv *priv = netdev_priv(ndev);
> > +	int ret;
> > +
> > +	ndev->trans_start = jiffies;
> 
> 	Are you sure trans_start needs to  be updated ?
It holds time of last tx, so I am sure it has to be updated on every tx.

> 
> > +
> > +	ret = skb_padto(skb, CPSW_MIN_PACKET_SIZE);
> > +	if (unlikely(ret < 0)) {
> > +		msg(err, tx_err, "packet pad failed");
> > +		goto fail;
> 
> Ouch... if skb_padto() fails, we stop queue forever, and skb is freed
> twice...
> 
> correct sequence is :
> 
> 	if (skb_padto(skb, CPSW_MIN_PACKET_SIZE)) {
> 		priv->stats.tx_dropped++;
> 		return NETDEV_TX_OK;
> 	}
> 
> By the way, skb_padto() doesnt change skb->len, be warned !
Yes, will update the patch in next version

> 
> 
> > +	}
> > +
> > +	ret = cpdma_chan_submit(priv->txch, skb, skb->data,
> > +				skb->len, GFP_KERNEL);
> 
> This GFP_KERNEL cannot be right, even if cpdma_chan_submit() ignores its
> last argument.
Yes, Good catch. I just reviewed davinci_cpdma code, the argument is not used. I will send a separate patch set to remove this argument as this is already used in another driver "davinci_emcac.c"

> 
> > +	if (unlikely(ret != 0)) {
> > +		msg(err, tx_err, "desc submit failed");
> > +		goto fail;
> > +	}
> > +
> > +	return NETDEV_TX_OK;
> > +fail:
> > +	priv->stats.tx_dropped++;
> > +	netif_stop_queue(ndev);
> > +	return NETDEV_TX_BUSY;
> > +}
> > +
> 
> 

--
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