Re: [PATCH v2 24/26] dmaengine: dw-edma: Skip cleanup procedure if no private data found

From: Serge Semin <fancer.lancer@gmail.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: "Serge Semin" <Sergey.Semin@baikalelectronics.ru>,
	"Gustavo Pimentel" <gustavo.pimentel@synopsys.com>,
	"Vinod Koul" <vkoul@kernel.org>,
	"Jingoo Han" <jingoohan1@gmail.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Frank Li" <Frank.Li@nxp.com>,
	"Alexey Malahov" <Alexey.Malahov@baikalelectronics.ru>,
	"Pavel Parkhomenko" <Pavel.Parkhomenko@baikalelectronics.ru>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	linux-pci@vger.kernel.org, dmaengine@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 24/26] dmaengine: dw-edma: Skip cleanup procedure if no private data found
Date: Thu, 12 May 2022 20:38:35 +0300	[thread overview]
Message-ID: <20220512173835.ffboven6wbtswkek@mobilestation> (raw)
In-Reply-To: <20220512152354.GO35848@thinkpad>

On Thu, May 12, 2022 at 08:53:54PM +0530, Manivannan Sadhasivam wrote:
> On Wed, May 04, 2022 at 01:51:02AM +0300, Serge Semin wrote:
> > DW eDMA driver private data is preserved in the passed DW eDMA chip info
> > structure. If either probe procedure failed or for some reason the passed
> > info object doesn't have private data pointer initialized we need to halt
> > the DMA device cleanup procedure in order to prevent possible system
> > crashes.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 

> As I said in the previous iteration, I'm not sure we will hit this issue.

It's unlikely but still not impossible. Consider the case when your DW
PCIe controller has eDMA capability, but the platform code isn't ready
to have it properly utilized. In that case dw_edma_probe() will fail
at some point while the rest of the DW PCIe code will go on executing
ignoring that error. In that case calling the dw_edma_remove() will
cause the system crash since the dw_edma_chip.dw pointer will be left
uninitialized. For instance possible circumstances can be as follows:

dw_pcie_host_init()
+-> dw_pcie_edma_detect() FAIL ignored.
+-> pci_host_probe() FAIL goto err_stop_link
                      |
                      +-> dw_pcie_edma_remove() CRASH !!!

So to be on a safe side we need to make sure the DW eDMA descriptor
cleanup is skipped in case if it hasn't been properly initialized. The
proper initialization is indicated by having the dw_edma_chip.dw set
with the valid pointer to the private data.

> But I don't object the patch. So,
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks.

-Sergey

> 
> Thanks,
> Mani
> 
> > ---
> >  drivers/dma/dw-edma/dw-edma-core.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > index 908607785401..561686b51915 100644
> > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > @@ -1035,6 +1035,10 @@ int dw_edma_remove(struct dw_edma_chip *chip)
> >  	struct dw_edma *dw = chip->dw;
> >  	int i;
> >  
> > +	/* Skip removal if no private data found */
> > +	if (!dw)
> > +		return -ENODEV;
> > +
> >  	/* Disable eDMA */
> >  	dw_edma_v0_core_off(dw);
> >  
> > -- 
> > 2.35.1
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்