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: Sat, 19 Jan 2019 17:45:22 +0200	[thread overview]
Message-ID: <20190119154522.GD9170@smile.fi.intel.com> (raw)
In-Reply-To: <9b887e39-9de3-2285-f3b4-174a72e6c4d3@synopsys.com>

On Tue, Jan 15, 2019 at 12:48:34PM +0000, Gustavo Pimentel wrote:
> On 15/01/2019 05:43, Andy Shevchenko wrote:
> > 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.
> 
> Ok, but how should I proceed? May I leave it or substitute by another way to do
> it? If so, how?
> As I said, the intended is to be only used for this test case, on normal
> operation the parameter it should be always false.

Keep out-of-tree patch for your needs.

> >>>> +	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
> 
> Ok, how can I do it? What I should add to the code to force that?

Not adding, removing. That's what I put before.

> 
> > useless check. I don't believe it would make sense to have NULL pdata for the
> > driver since it wouldn't be functional anyhow.
> 
> Yes, you're right without pdata the driver can't do anything.

-- 
With Best Regards,
Andy Shevchenko