RE: [PATCH 2/2] fpga: dfl: look for vendor specific capability

[Wu, Hao]

> > +
> > +		start = pci_resource_start(pcidev, bar) + offset;
> > +		len -= offset;
> 
> With these code, I have the following assumption:
> 
> 1. There is only one DFL in one bar, multiple DFLs requires multiple
> bars.
> 
> 2. The DFL region is from the "offset" to the end of the bar.

I think we should not have such kind of limitation, but at least it 
requires user clearly from spec, to make sure no overlap case could
happen. We all know that BAR number is small, but we won't limit
DFL numbers by BAR number here.

> 
> Are they correct? If yes maybe we should specify them clearly in Doc.
> 
> > +
> > +		if (!PAGE_ALIGNED(start)) {
> > +			dev_err(&pcidev->dev, "%s unaliged start 0x%llx\n",
> > +				__func__, start);
> > +			return -EINVAL;
> > +		}
> > +
> > +		dfl_fpga_enum_info_add_dfl(info, start, len);
> 
> Do we need some region overlapping check in this func? So we could find
> the HW problem (e.g. same bar num for multiple DFLs) in early stage.

Not sure if VSEC can add a length field for this purpose, otherwise overlapping
check only can be done after enumeration (walk the DFL to know the end).

> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int find_dfl_in_bar0(struct pci_dev *pcidev,
> >  			    struct dfl_fpga_enum_info *info)
> >  {
> > @@ -221,7 +304,10 @@ static int cci_enumerate_feature_devs(struct
> pci_dev *pcidev)
> >  			goto irq_free_exit;
> >  	}
> >
> > -	ret = find_dfl_in_bar0(pcidev, info);
> > +	ret = find_dfl_in_cfg(pcidev, info);
> > +
> > +	if (ret)
> > +		ret = find_dfl_in_bar0(pcidev, info);
> 
> The patch is more than the relocation support for DFL. Actually it
> introduced a different way of DFL finding.
> 
> Previously it starts at bar0 offset 0, find dfl fme first, then find
> dfl port according to fme header registers. Now it enumerates every DFL
> by PCIe VSEC.

Yes, the name is a little confusing, maybe we can rename them.

find_dfls_by_default or find_dfls - to handle original cases.
find_dfls_by_vsec - to handle vsec case.

Thanks
Hao

> 
> Maybe we should add more description about the change and why.
> 
> Thanks,
> Yilun
> 
> >
> >  	if (ret)
> >  		goto irq_free_exit;
> > --
> > 2.25.2