On Fri, Nov 15, 2024 at 10:53:07AM +0100, Niklas Cassel wrote:
> On Thu, Nov 14, 2024 at 05:52:39PM -0500, Frank Li wrote:
> > Introduce the helper function pci_epf_align_addr() to adjust addresses
> > according to PCI BAR alignment requirements, converting addresses into base
> > and offset values.
> >
> > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > ---
> > change from v6 to v7
> > - new patch
> > ---
> > drivers/pci/endpoint/pci-epf-core.c | 39 +++++++++++++++++++++++++++++++++++++
> > include/linux/pci-epf.h | 13 +++++++++++++
> > 2 files changed, 52 insertions(+)
> >
> > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > index 8fa2797d4169a..a3f172cc786e9 100644
> > --- a/drivers/pci/endpoint/pci-epf-core.c
> > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > @@ -464,6 +464,45 @@ struct pci_epf *pci_epf_create(const char *name)
> > }
> > EXPORT_SYMBOL_GPL(pci_epf_create);
> >
> > +/**
> > + * pci_epf_align_addr() - Get base address and offset that match bar's
> > + * alignment requirement
> > + * @epf: the EPF device
> > + * @addr: the address of the memory
> > + * @bar: the BAR number corresponding to map addr
> > + * @base: return base address, which match BAR's alignment requirement, nothing
> > + * return if NULL
> > + * @off: return offset, nothing return if NULL
> > + *
> > + * Helper function to convert input 'addr' to base and offset, which match
> > + * BAR's alignment requirement.
> > + */
> > +int pci_epf_align_addr(struct pci_epf *epf, enum pci_barno bar, u64 addr, u64 *base, size_t *off)
>
> Nit: perhaps rename this function to:
> pci_epf_align_ib_addr()
> or
> pci_epf_align_inbound_addr()
>
> to more clearly not confuse this with:
> if (epc->ops->align_addr)
> .align_addr()
> (Ideally those functions should have been named align_ob_addr(),
> or align_outbound_addr())
>
>
> > +{
> > + const struct pci_epc_features *epc_features;
> > + u64 align;
> > +
> > + epc_features = pci_epc_get_features(epf->epc, epf->func_no, epf->vfunc_no);
> > + if (!epc_features) {
> > + dev_err(&epf->dev, "epc_features not implemented\n");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + align = epc_features->align;
> > + align = align ? align : 128;
> > + if (epc_features->bar[bar].type == BAR_FIXED)
> > + align = max(epc_features->bar[bar].fixed_size, align);
> > +
> > + if (base)
> > + *base = round_down(addr, align);
> > +
> > + if (off)
> > + *off = addr & (align - 1);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_epf_align_addr);
> > +
> > static void pci_epf_dev_release(struct device *dev)
> > {
> > struct pci_epf *epf = to_pci_epf(dev);
> > diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> > index 5374e6515ffa0..20f4f31ba9b36 100644
> > --- a/include/linux/pci-epf.h
> > +++ b/include/linux/pci-epf.h
> > @@ -238,6 +238,19 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> > enum pci_epc_interface_type type);
> > void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
> > enum pci_epc_interface_type type);
> > +
> > +int pci_epf_align_addr(struct pci_epf *epf, enum pci_barno bar, u64 addr, u64 *base, size_t *off);
> > +static inline int pci_epf_align_addr_lo_hi(struct pci_epf *epf, enum pci_barno bar,
> > + u32 low, u32 high, u64 *base, size_t *off)
> > +{
> > + u64 addr = high;
> > +
> > + addr <<= 32;
> > + addr |= low;
> > +
> > + return pci_epf_align_addr(epf, bar, addr, base, off);
> > +}
>
> I'm not sure if this function deserves to live :)
> Can't the caller just do this before calling pci_epf_align_addr() ?
Ideally, kernel should have macro to combine 32bit macro to a 64bit, but
I have not found it.
It is quite easy to make error or warning by simple
(high << 32 | low)
It needs ((u64) high << 32 | low) at least. I just want to avoid everyone
to struggle simple issue.
And msi_msg use lo and hi. pci_function_test actually just demostrate how
to use doorbell. if other function driver use doorbell in future, avoid
"(u64) high << 32 | low" copy to everywhere.
Maybe like upper_32_bits(), add global helper macro
#define low32_high32_to_64bit(l, h) ((u64)(h) << 32 | low)
Frank
>
>
> Kind regards,
> Niklas
