lkml.org 
[lkml]   [2021]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 09/34] misc: xlink-pcie: lh: Add PCIe EPF driver for Local Host
On Fri, Jan 08, 2021 at 01:25:35PM -0800, mgross@linux.intel.com wrote:
> From: Srikanth Thokala <srikanth.thokala@intel.com>
>
> Add PCIe EPF driver for local host (lh) to configure BAR's and other
> HW resources. Underlying PCIe HW controller is a Synopsys DWC PCIe core.
>
> Cc: Derek Kiernan <derek.kiernan@xilinx.com>
> Cc: Dragan Cvetic <dragan.cvetic@xilinx.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Mark Gross <mgross@linux.intel.com>
> Signed-off-by: Srikanth Thokala <srikanth.thokala@intel.com>
> ---
> MAINTAINERS | 6 +
> drivers/misc/Kconfig | 1 +
> drivers/misc/Makefile | 1 +
> drivers/misc/xlink-pcie/Kconfig | 9 +
> drivers/misc/xlink-pcie/Makefile | 1 +
> drivers/misc/xlink-pcie/local_host/Makefile | 2 +
> drivers/misc/xlink-pcie/local_host/epf.c | 413 ++++++++++++++++++++
> drivers/misc/xlink-pcie/local_host/epf.h | 39 ++
> drivers/misc/xlink-pcie/local_host/xpcie.h | 38 ++

Why such a deep directory tree? Why is "local_host" needed?

Anyway, one thing stood out instantly:

> +static int intel_xpcie_check_bar(struct pci_epf *epf,
> + struct pci_epf_bar *epf_bar,
> + enum pci_barno barno,
> + size_t size, u8 reserved_bar)
> +{
> + if (reserved_bar & (1 << barno)) {
> + dev_err(&epf->dev, "BAR%d is already reserved\n", barno);
> + return -EFAULT;

That error is only allowed when you really have a fault from
reading/writing to/from userspace memory. Not this type of foolish
programming error by the caller.

> + }
> +
> + if (epf_bar->size != 0 && epf_bar->size < size) {
> + dev_err(&epf->dev, "BAR%d fixed size is not enough\n", barno);
> + return -ENOMEM;

Did you really run out of memory or was the parameters sent to you
incorrect? -EINVAL is the properly thing here, right?



> + }
> +
> + return 0;
> +}
> +
> +static int intel_xpcie_configure_bar(struct pci_epf *epf,
> + const struct pci_epc_features
> + *epc_features)

Odd indentation :(

> +{
> + struct pci_epf_bar *epf_bar;
> + bool bar_fixed_64bit;
> + int ret, i;
> +
> + for (i = BAR_0; i <= BAR_5; i++) {
> + epf_bar = &epf->bar[i];
> + bar_fixed_64bit = !!(epc_features->bar_fixed_64bit & (1 << i));
> + if (bar_fixed_64bit)
> + epf_bar->flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> + if (epc_features->bar_fixed_size[i])
> + epf_bar->size = epc_features->bar_fixed_size[i];
> +
> + if (i == BAR_2) {
> + ret = intel_xpcie_check_bar(epf, epf_bar, BAR_2,
> + BAR2_MIN_SIZE,
> + epc_features->reserved_bar);
> + if (ret)
> + return ret;
> + }
> +
> + if (i == BAR_4) {
> + ret = intel_xpcie_check_bar(epf, epf_bar, BAR_4,
> + BAR4_MIN_SIZE,
> + epc_features->reserved_bar);
> + if (ret)
> + return ret;
> + }

Why do you need to check all of this? Where is the data coming from
that could be incorrect?

thanks,

greg k-h

\
 
 \ /
  Last update: 2021-01-20 19:00    [W:0.354 / U:0.760 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site