On Fri, Jan 08, 2021 at 01:25:35PM -0800, [email protected] wrote:
> From: Srikanth Thokala <[email protected]>
>
> 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 <[email protected]>
> Cc: Dragan Cvetic <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Reviewed-by: Mark Gross <[email protected]>
> Signed-off-by: Srikanth Thokala <[email protected]>
> ---
> 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
