Re: [RFC v3 5/7] dmaengine: Add Synopsys eDMA IP PCIe glue-logic

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Cc: "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>,
	Vinod Koul <vkoul@kernel.org>,
	Dan Williams <dan.j.williams@intel.com>,
	Eugeniy Paltsev <eugeniy.paltsev@synopsys.com>,
	Russell King <rmk+kernel@armlinux.org.uk>,
	Niklas Cassel <niklas.cassel@linaro.org>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Joao Pinto <joao.pinto@synopsys.com>,
	Jose Abreu <jose.abreu@synopsys.com>,
	Luis Oliveira <luis.oliveira@synopsys.com>,
	Vitor Soares <vitor.soares@synopsys.com>,
	Nelson Costa <nelson.costa@synopsys.com>,
	Pedro Sousa <pedrom.sousa@synopsys.com>
Subject: Re: [RFC v3 5/7] dmaengine: Add Synopsys eDMA IP PCIe glue-logic
Date: Tue, 15 Jan 2019 07:43:58 +0200	[thread overview]
Message-ID: <20190115054358.GB9170@smile.fi.intel.com> (raw)
In-Reply-To: <916f319b-9e45-6d78-4ecc-850feda84bb3@synopsys.com>

On Mon, Jan 14, 2019 at 11:38:02AM +0000, Gustavo Pimentel wrote:
> On 11/01/2019 19:47, Andy Shevchenko wrote:
> > On Fri, Jan 11, 2019 at 07:33:41PM +0100, Gustavo Pimentel wrote:

> >> +static bool disable_msix;
> >> +module_param(disable_msix, bool, 0644);
> >> +MODULE_PARM_DESC(disable_msix, "Disable MSI-X interrupts");
> > 
> > Why?!
> > We are no allow new module parameters without very strong arguments.
> 
> Since this is a reference driver and might be used to test customized HW
> solutions, I added this parameter to allow the possibility to test the solution
> forcing the MSI feature binding. This is required specially if who will test
> this solution has a Root Complex with both features available (MSI and MSI-X),
> because the Kernel will give always preference to MSI-X binding (assuming that
> the EP has also both features available).

Yes, you may do it for testing purposes, but it doesn't fit the kernel standards.

> >> +	if (!pdata) {
> >> +		dev_err(dev, "%s missing data structure\n", pci_name(pdev));
> >> +		return -EFAULT;
> >> +	}
> > 
> > Useless check.
> 
> Why? It's just a precaution, isn't it a good practice always to think of the
> worst case?

You just can put an implicit requirement of pdata rather than doing this
useless check. I don't believe it would make sense to have NULL pdata for the
driver since it wouldn't be functional anyhow.

> >> +	/* Mapping PCI BAR regions */
> >> +	err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar) |
> >> +				       BIT(pdata->ll_bar) |
> >> +				       BIT(pdata->dt_bar),
> >> +				 pci_name(pdev));
> >> +	if (err) {

> >> +		return err;
> >> +	}

> >> +	if (!pcim_iomap_table(pdev))
> >> +		return -EACCES;
> > 
> > Never happen condition. Thus useless.
> 
> pcim_iomap_table() can return NULL in case of allocation failure. Besides that,
> isn't it a good practice always to think of the worst case?

No, it can't in the conditions your have in the code. See above the lines I left.
If pcim_iomap_regions() successfully finished...

> >> +	dev_info(dev, "DesignWare eDMA PCIe driver loaded completely\n");
> > 
> > Useless.
> 
> It's helpful for bring up, I can pass it to dbg.

It just shows that someone didn't use existing tools and features. This message and similar are useless.
Hint: initcall_debug.

> >> +	dev_info(dev, "DesignWare eDMA PCIe driver unloaded completely\n");
> > 
> > Ditto.
> 
> It's helpful for bring up, I can pass it to dbg.

Ditto.

-- 
With Best Regards,
Andy Shevchenko