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

From: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	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: Mon, 21 Jan 2019 09:21:27 +0000	[thread overview]
Message-ID: <40b97ba5-0a25-8707-d328-45c35d304366@synopsys.com> (raw)
In-Reply-To: <20190119154522.GD9170@smile.fi.intel.com>

On 19/01/2019 15:45, Andy Shevchenko wrote:
> 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.

Ok.

> 
>>>>>> +	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.

I thought there was some API or code design to force that. Sorry my bad.

> 
>>
>>> 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.
> 