netdev - RE: [PATCH net-next v3 3/9] net: rswitch: Use build

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 for Android: free password hash cracker in your pocket

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

Date: Thu, 7 Dec 2023 00:25:54 +0000
From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To: Simon Horman <horms@...nel.org>
CC: "s.shtylyov@....ru" <s.shtylyov@....ru>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>
Subject: RE: [PATCH net-next v3 3/9] net: rswitch: Use build_skb() for RX

Hello Simon-san,

> From: Simon Horman, Sent: Thursday, December 7, 2023 4:08 AM
> 
> On Mon, Dec 04, 2023 at 10:20:52AM +0900, Yoshihiro Shimoda wrote:
> > If this hardware receives a jumbo frame like 2KiB or more, it will be
> > split into multiple queues. In the near future, to support this, use
> > build_skb() instead of netdev_alloc_skb_ip_align().
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
> 
> ...
> 
> >  static void rswitch_gwca_ts_queue_free(struct rswitch_private *priv)
> > @@ -308,17 +308,19 @@ static int rswitch_gwca_queue_alloc(struct net_device *ndev,
> >  	gq->ring_size = ring_size;
> >  	gq->ndev = ndev;
> >
> > -	gq->skbs = kcalloc(gq->ring_size, sizeof(*gq->skbs), GFP_KERNEL);
> > -	if (!gq->skbs)
> > -		return -ENOMEM;
> > -
> >  	if (!dir_tx) {
> > -		rswitch_gwca_queue_alloc_skb(gq, 0, gq->ring_size);
> > +		gq->rx_bufs = kcalloc(gq->ring_size, sizeof(*gq->rx_bufs), GFP_KERNEL);
> > +		if (!gq->rx_bufs)
> > +			goto out;
> 
> Hi Shimoda-san,
> 
> there is no need to re-spin because of this,
> but I have some commends on error handling.
> 
> I think that for consistency this can just return -ENOMEM.

Thank you for your comments! I think so. So, I'll fix it on v4.

> Or alternatively, perhaps 'goto out' can be used for the (!gq->skbs)
> condition below.
> 
> > +		rswitch_gwca_queue_alloc_rx_buf(gq, 0, gq->ring_size);
> 
> Not strictly related to this patch, but should
> the return value of rswitch_gwca_queue_alloc_rx_buf be checked?

I think so. I'll fix it.

Best regards,
Yoshihiro Shimoda


> >
> >  		gq->rx_ring = dma_alloc_coherent(ndev->dev.parent,
> >  						 sizeof(struct rswitch_ext_ts_desc) *
> >  						 (gq->ring_size + 1), &gq->ring_dma, GFP_KERNEL);
> >  	} else {
> > +		gq->skbs = kcalloc(gq->ring_size, sizeof(*gq->skbs), GFP_KERNEL);
> > +		if (!gq->skbs)
> > +			return -ENOMEM;
> >  		gq->tx_ring = dma_alloc_coherent(ndev->dev.parent,
> >  						 sizeof(struct rswitch_ext_desc) *
> >  						 (gq->ring_size + 1), &gq->ring_dma, GFP_KERNEL);
> 
> ...