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
