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

[Thokala, Srikanth]

Hi Greg,

> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Sunday, January 24, 2021 5:27 PM
> To: Thokala, Srikanth <srikanth.thokala@xxxxxxxxx>
> Cc: mgross@xxxxxxxxxxxxxxx; 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; Derek Kiernan <derek.kiernan@xxxxxxxxxx>
> Subject: Re: [PATCH v2 09/34] misc: xlink-pcie: lh: Add PCIe EPF driver
> for Local Host
> 
> On Sun, Jan 24, 2021 at 11:48:29AM +0000, Thokala, Srikanth wrote:
> > > > +{
> > > > +	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.
> 
> So why do you not trust that information coming from the caller?
> Shouldn't it always be correct as it already is validated by that
> in-kernel caller?  Don't check for things you don't have to check for
> because you control the code that calls this stuff.

Sure, I agree to your point.
I will fix it in my next version.

Thanks!
Srikanth

> 
> thanks,
> 
> greg k-h