On Fri, Mar 15, 2024 at 11:09:03AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Mar 13, 2024 at 11:57:57AM +0100, Niklas Cassel wrote:
> > Simplify the loop in pci_epf_test_set_bar().
> > If we allocated memory for the BAR, we need to call set_bar() for that
> > BAR, if we did not allocated memory for that BAR, we need to skip.
> > It is as simple as that. This also matches the logic in
> > pci_epf_test_unbind().
> >
> > A 64-bit BAR will still only be one allocation, with the BAR succeeding
> > the 64-bit BAR being null.
> >
> > While at it, remove the misleading comment.
> > A EPC .set_bar() callback should never change the epf_bar->flags.
> > (E.g. to set a 64-bit BAR if we requested a 32-bit BAR.)
> >
> > A .set_bar() callback should do what we request it to do.
> > If it can't satisfy the request, it should return an error.
> >
>
> That's a valid assumption. But...
>
> > If platform has a specific requirement, e.g. that a certain BAR has to
> > be a 64-bit BAR, then it should specify that by setting the .only_64bit
> > flag for that specific BAR in epc_features->bar[], such that
> > pci_epf_alloc_space() will return a epf_bar with the 64-bit flag set.
> > (Such that .set_bar() will receive a request to set a 64-bit BAR.)
> >
>
> Looks like pcie-cadence-ep is setting the PCI_BASE_ADDRESS_MEM_TYPE_64 flag if
> the requested size is >2GB (I don't know why 2GB instead of 4GB in the first
> place).
That is dead code that will never be able to execute.
Ever since commit f25b5fae29d4 ("PCI: endpoint: Setting a BAR size > 4 GB
is invalid if 64-bit flag is not set") it has been impossible to get the
.set_bar() callback with a BAR size > 2 GB.
Yes, 2GB!
The author of f25b5fae29d4 is an idiot (yes, it is me).
Anyway, the code itself in that commit is doing the right thing...
2GB is the maximum size of a 32-bit BAR.
So, since the the code in pcie-cadence-ep is dead code, I don't see
any problem with this commit.
Kind regards,
Niklas
>
> - Mani
>
> > Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
> > ---
> > drivers/pci/endpoint/functions/pci-epf-test.c | 21 ++++---------------
> > 1 file changed, 4 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index 20c79610712d..91bbfcb1b3ed 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -709,31 +709,18 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
> >
> > static int pci_epf_test_set_bar(struct pci_epf *epf)
> > {
> > - int bar, add;
> > - int ret;
> > - struct pci_epf_bar *epf_bar;
> > + int bar, ret;
> > struct pci_epc *epc = epf->epc;
> > struct device *dev = &epf->dev;
> > struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> > enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> > - const struct pci_epc_features *epc_features;
> > -
> > - epc_features = epf_test->epc_features;
> >
> > - for (bar = 0; bar < PCI_STD_NUM_BARS; bar += add) {
> > - epf_bar = &epf->bar[bar];
> > - /*
> > - * pci_epc_set_bar() sets PCI_BASE_ADDRESS_MEM_TYPE_64
> > - * if the specific implementation required a 64-bit BAR,
> > - * even if we only requested a 32-bit BAR.
> > - */
> > - add = (epf_bar->flags & PCI_BASE_ADDRESS_MEM_TYPE_64) ? 2 : 1;
> > -
> > - if (epc_features->bar[bar].type == BAR_RESERVED)
> > + for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> > + if (!epf_test->reg[bar])
> > continue;
> >
> > ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no,
> > - epf_bar);
> > + &epf->bar[bar]);
> > if (ret) {
> > pci_epf_free_space(epf, epf_test->reg[bar], bar,
> > PRIMARY_INTERFACE);
> > --
> > 2.44.0
> >
>
> --
> மணிவண்ணன் சதாசிவம்
