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

[Thokala, Srikanth]

Hi Greg,

Thank you for the review.

> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Wednesday, January 20, 2021 11:28 PM
> To: mgross@xxxxxxxxxxxxxxx
> Cc: markgross@xxxxxxxxxx; arnd@xxxxxxxx; bp@xxxxxxx;
> damien.lemoal@xxxxxxx; dragan.cvetic@xxxxxxxxxx; corbet@xxxxxxx;
> leonard.crestez@xxxxxxx; palmerdabbelt@xxxxxxxxxx;
> paul.walmsley@xxxxxxxxxx; peng.fan@xxxxxxx; robh+dt@xxxxxxxxxx;
> shawnguo@xxxxxxxxxx; jassisinghbrar@xxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Thokala, Srikanth <srikanth.thokala@xxxxxxxxx>;
> Derek Kiernan <derek.kiernan@xxxxxxxxxx>
> Subject: Re: [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@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?

Xlink-pcie comprises of local host (ARM CPU) and remote host (IA CPU)
variants. It is a transport layer that establishes communication between
them. 

local_host/ running on ARM CPU is based on PCI Endpoint Framework
remote_host/ running on IA CPU is a PCIe Endpoint driver

As these two variants are architecturally different, we are maintaining
under two directories.

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

Thanks for pointing out, I will use appropriate error value to return.
 
> > +	}
> > +
> > +	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?

Sure, I will change to return -EINVAL.

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

I had to break this as the checkpatch complained about 80-line warning.
I will fix this to have better 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?

PCI BAR attributes, as inputs, are coming from the PCIe controller driver
through PCIe End Point Framework.  These checks are required to compare the 
configuration this driver is expecting to the configuration coming from
the PCIe controller driver.

FYI, PCIe controller driver for Intel Keem Bay is currently under review:
https://lore.kernel.org/linux-pci/20210122032610.4958-1-srikanth.thokala@xxxxxxxxx/

Thanks!
Srikanth

> 
> thanks,
> 
> greg k-h