Received-SPF: pass (google.com: domain of linux-kernel+bounces-82352-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 27 Feb 2024 00:32:44 +0300
From: Serge Semin <fancer.lancer@gmail.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>, 
	Gustavo Pimentel <gustavo.pimentel@synopsys.com>, Lorenzo Pieralisi <lpieralisi@kernel.org>, 
	Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>, Rob Herring <robh@kernel.org>, 
	Bjorn Helgaas <bhelgaas@google.com>, Marek Vasut <marek.vasut+renesas@gmail.com>, 
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>, Kishon Vijay Abraham I <kishon@kernel.org>, 
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org, 
	linux-arm-msm@vger.kernel.org, mhi@lists.linux.dev, Siddharth Vadapalli <s-vadapalli@ti.com>
Subject: Re: [PATCH v3 2/5] PCI: dwc: Skip finding eDMA channels count if
 glue drivers have passed them
Message-ID: <4p4z5eyhpdhxzi36drhrmz6z7krupszddudg6c2baypkbnnj7t@nqcmk2wdntts>
References: <20240226-dw-hdma-v3-0-cfcb8171fc24@linaro.org>
 <20240226-dw-hdma-v3-2-cfcb8171fc24@linaro.org>
 <5gzkxdpx6u3jhw5twbncjhtozgekmlzxrpj3m6is3ijadm2svb@f6ng4owyakup>
 <20240226153014.GG8422@thinkpad>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240226153014.GG8422@thinkpad>

On Mon, Feb 26, 2024 at 09:00:14PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Feb 26, 2024 at 03:53:20PM +0300, Serge Semin wrote:
> > On Mon, Feb 26, 2024 at 05:07:27PM +0530, Manivannan Sadhasivam wrote:
> > > In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way
> > > the drivers can auto detect the number of read/write channels as like its
> > > predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA
> > > have to pass the channels count during probe.
> > > 
> > > To accommodate that, let's skip finding the channels if the channels count
> > > were already passed by glue drivers. If the channels count passed were
> > > wrong in any form, then the existing sanity check will catch it.
> > > 
> > > Suggested-by: Serge Semin <fancer.lancer@gmail.com>
> > > Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++-------
> > >  1 file changed, 9 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > index 193fcd86cf93..ce273c3c5421 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci)
> > >  {
> > >  	u32 val;
> > >  
> > > -	if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> > > -		val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > > -	else
> > > -		val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > > -
> > > -	pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> > > -	pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> > 
> > > +	if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) {
> > 
> > Are you sure that the partly initialized case should be considered as
> > a request for the auto-detection? IMO &&-ing here and letting the
> > sanity check to fail further would be more correct since thus the
> > developer would know about improper initialized data.
> > 
> 
> We already have the check below. So the partly initialized case will fail
> anyway.

Not really. If the partly initialized case activates the
auto-detection procedure it will override both non-initialized and
_initialized_ number of channels with the values retrieved from the
hardware, which the glue driver has been willing not to use. This
prone to undefined behavior depending on the reasons of skipping the
auto-detection procedure. For instance, assume the DMA_CTRL register
reports an invalid number of read channels. A glue driver by mistake
or willingly overwrites the pci->edma.ll_rd_cnt field only. This won't
solve the problem since the auto-detection will be proceeded due to
the pci->edma.ll_wr_cnt field being left uninitialized.

So to speak it would be better to implement a strictly determined case
for activating the auto-detection procedure: both number of channels
aren't initialized; if only one field is initialized then report an
error.

Alternatively we can have the auto-detection executed on the
per-channel basis:

+	if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) {
+		val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
+
+		if (!pci->edma.ll_wr_cnt)
+			pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
+
+		if (!pci->edma.ll_rd_cnt)
+			pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
+	}

-Serge(y)

> 
> - Mani
> 
> > -Serge(y)
> > 
> > > +		if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> > > +			val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > > +		else
> > > +			val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > > +
> > > +		pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> > > +		pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> > > +	}
> > >  
> > >  	/* Sanity check the channels count if the mapping was incorrect */
> > >  	if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > EDMA_MAX_WR_CH ||
> > > 
> > > -- 
> > > 2.25.1
> > > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்