Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <64898745.5d0a0220.546a.a6f1@mx.google.com>
Date:   Wed, 14 Jun 2023 04:16:08 +0200
From:   Christian Marangi <ansuelsmth@gmail.com>
To:     Jose Abreu <Jose.Abreu@synopsys.com>
Cc:     Giuseppe Cavallaro <peppe.cavallaro@st.com>,
        Alexandre Torgue <alexandre.torgue@foss.st.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-stm32@st-md-mailman.stormreply.com" 
        <linux-stm32@st-md-mailman.stormreply.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [net-next PATCH v5 4/5] net: ethernet: stmicro: stmmac: generate
 stmmac dma conf before open
References: <20220723142933.16030-1-ansuelsmth@gmail.com>
 <20220723142933.16030-5-ansuelsmth@gmail.com>
 <DM4PR12MB508882D5BE351BD756A7A9A4D35AA@DM4PR12MB5088.namprd12.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <DM4PR12MB508882D5BE351BD756A7A9A4D35AA@DM4PR12MB5088.namprd12.prod.outlook.com>
Precedence: bulk

On Wed, Jun 14, 2023 at 07:15:03AM +0000, Jose Abreu wrote:
> Hi Christian,
> 
> From: Christian Marangi <ansuelsmth@gmail.com>
> Date: Sat, Jul 23, 2022 at 15:29:32
> 
> > +static int __stmmac_open(struct net_device *dev,
> > +			 struct stmmac_dma_conf *dma_conf)
> >  {
> >  	struct stmmac_priv *priv = netdev_priv(dev);
> >  	int mode = priv->plat->phy_interface;
> > -	int bfsize = 0;
> >  	u32 chan;
> >  	int ret;
> >  
> > @@ -3657,45 +3794,10 @@ static int stmmac_open(struct net_device *dev)
> >  	memset(&priv->xstats, 0, sizeof(struct stmmac_extra_stats));
> >  	priv->xstats.threshold = tc;
> >  
> > -	bfsize = stmmac_set_16kib_bfsize(priv, dev->mtu);
> > -	if (bfsize < 0)
> > -		bfsize = 0;
> > -
> > -	if (bfsize < BUF_SIZE_16KiB)
> > -		bfsize = stmmac_set_bfsize(dev->mtu, priv->dma_conf.dma_buf_sz);
> > -
> > -	priv->dma_conf.dma_buf_sz = bfsize;
> > -	buf_sz = bfsize;
> > -
> >  	priv->rx_copybreak = STMMAC_RX_COPYBREAK;
> >  
> > -	if (!priv->dma_conf.dma_tx_size)
> > -		priv->dma_conf.dma_tx_size = DMA_DEFAULT_TX_SIZE;
> > -	if (!priv->dma_conf.dma_rx_size)
> > -		priv->dma_conf.dma_rx_size = DMA_DEFAULT_RX_SIZE;
> > -
> > -	/* Earlier check for TBS */
> > -	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++) {
> > -		struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[chan];
> > -		int tbs_en = priv->plat->tx_queues_cfg[chan].tbs_en;
> > -
> > -		/* Setup per-TXQ tbs flag before TX descriptor alloc */
> > -		tx_q->tbs |= tbs_en ? STMMAC_TBS_AVAIL : 0;
> > -	}
> > -
> > -	ret = alloc_dma_desc_resources(priv);
> > -	if (ret < 0) {
> > -		netdev_err(priv->dev, "%s: DMA descriptors allocation failed\n",
> > -			   __func__);
> > -		goto dma_desc_error;
> > -	}
> > -
> > -	ret = init_dma_desc_rings(dev, GFP_KERNEL);
> > -	if (ret < 0) {
> > -		netdev_err(priv->dev, "%s: DMA descriptors initialization failed\n",
> > -			   __func__);
> > -		goto init_error;
> > -	}
> > +	buf_sz = dma_conf->dma_buf_sz;
> > +	memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf));
> 
> This memcpy() needs to be the first thing to be done on __stmmac_open(), otherwise
> you'll leak the dma_conf when stmmac_init_phy() fails.
>

I'm not following the meaning of leak here. If it's intended as a memory
leak then dma_conf is correctly freed in the 2 user of __stmmac_open.

stmmac_init_phy also doesn't seems to use dma_conf. Am I missing
something here?

> Can you please send follow-up patch?

Happy to push a follow-up patch with these concern cleared!

> 
> Thanks,
> Jose

-- 
	Ansuel