Re: [PATCH v2 09/34] misc: xlink-pcie: lh: Add PCIe EPF driver for Local Host

[Greg KH]

On Fri, Jan 08, 2021 at 01:25:35PM -0800, mgross@xxxxxxxxxxxxxxx wrote:
> From: Srikanth Thokala <srikanth.thokala@xxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Cc: Dragan Cvetic <dragan.cvetic@xxxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Mark Gross <mgross@xxxxxxxxxxxxxxx>
> Signed-off-by: Srikanth Thokala <srikanth.thokala@xxxxxxxxx>
> ---
>  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