| Message ID | 20241116-ep-msi-v8-3-6f1f68ffd1bb@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: EP: Add RC-to-EP doorbell with platform MSI controller | expand |
On Sat, Nov 16, 2024 at 09:40:43AM -0500, Frank Li wrote: > Introduce the helper function pci_epf_align_addr() to adjust addresses pci_epf_align_inbound_addr()? > according to PCI BAR alignment requirements, converting addresses into base > and offset values. > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > change from v7 to v8 > - change name to pci_epf_align_inbound_addr() > - update comment said only need for memory, which not allocated by > pci_epf_alloc_space(). > > change from v6 to v7 > - new patch > --- > drivers/pci/endpoint/pci-epf-core.c | 45 +++++++++++++++++++++++++++++++++++++ > include/linux/pci-epf.h | 14 ++++++++++++ > 2 files changed, 59 insertions(+) > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c > index 8fa2797d4169a..4dfc218ebe20b 100644 > --- a/drivers/pci/endpoint/pci-epf-core.c > +++ b/drivers/pci/endpoint/pci-epf-core.c > @@ -464,6 +464,51 @@ struct pci_epf *pci_epf_create(const char *name) > } > EXPORT_SYMBOL_GPL(pci_epf_create); > > +/** > + * pci_epf_align_inbound_addr() - Get base address and offset that match bar's 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 Below, you are updating 'base' only if it is not NULL. Why would anyone call this API with 'base' and 'offset' set to NULL? > + * @off: return offset, nothing return if NULL > + * > + * Helper function to convert input 'addr' to base and offset, which match > + * BAR's alignment requirement. > + * > + * The pci_epf_alloc_space() function already accounts for alignment. This is > + * primarily intended for use with other memory regions not allocated by > + * pci_epf_alloc_space(), such as peripheral register spaces or the trigger > + * address for a platform MSI controller. > + */ > +int pci_epf_align_inbound_addr(struct pci_epf *epf, enum pci_barno bar, > + u64 addr, u64 *base, size_t *off) > +{ > + 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_inbound_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..eff73ccb5e702 100644 > --- a/include/linux/pci-epf.h > +++ b/include/linux/pci-epf.h > @@ -238,6 +238,20 @@ 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_inbound_addr(struct pci_epf *epf, enum pci_barno bar, > + u64 addr, u64 *base, size_t *off); > +static inline int pci_epf_align_inbound_addr_lo_hi(struct pci_epf *epf, enum pci_barno bar, > + u32 low, u32 high, u64 *base, size_t *off) Why can't you just use pci_epf_align_inbound_addr() directly? Or the caller could pass u64 address directly. - Mani > +{ > + u64 addr = high; > + > + addr <<= 32; > + addr |= low; > + > + return pci_epf_align_inbound_addr(epf, bar, addr, base, off); > +} > + > int pci_epf_bind(struct pci_epf *epf); > void pci_epf_unbind(struct pci_epf *epf); > int pci_epf_add_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf); > > -- > 2.34.1 >
On Sun, Nov 24, 2024 at 01:02:39PM +0530, Manivannan Sadhasivam wrote: > On Sat, Nov 16, 2024 at 09:40:43AM -0500, Frank Li wrote: > > Introduce the helper function pci_epf_align_addr() to adjust addresses > > pci_epf_align_inbound_addr()? > > > according to PCI BAR alignment requirements, converting addresses into base > > and offset values. > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > change from v7 to v8 > > - change name to pci_epf_align_inbound_addr() > > - update comment said only need for memory, which not allocated by > > pci_epf_alloc_space(). > > > > change from v6 to v7 > > - new patch > > --- > > drivers/pci/endpoint/pci-epf-core.c | 45 +++++++++++++++++++++++++++++++++++++ > > include/linux/pci-epf.h | 14 ++++++++++++ > > 2 files changed, 59 insertions(+) > > > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c > > index 8fa2797d4169a..4dfc218ebe20b 100644 > > --- a/drivers/pci/endpoint/pci-epf-core.c > > +++ b/drivers/pci/endpoint/pci-epf-core.c > > @@ -464,6 +464,51 @@ struct pci_epf *pci_epf_create(const char *name) > > } > > EXPORT_SYMBOL_GPL(pci_epf_create); > > > > +/** > > + * pci_epf_align_inbound_addr() - Get base address and offset that match bar's > > 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 > > Below, you are updating 'base' only if it is not NULL. Why would anyone call > this API with 'base' and 'offset' set to NULL? Some time, they may just want one of two. > > > + * @off: return offset, nothing return if NULL > > + * > > + * Helper function to convert input 'addr' to base and offset, which match > > + * BAR's alignment requirement. > > + * > > + * The pci_epf_alloc_space() function already accounts for alignment. This is > > + * primarily intended for use with other memory regions not allocated by > > + * pci_epf_alloc_space(), such as peripheral register spaces or the trigger > > + * address for a platform MSI controller. > > + */ > > +int pci_epf_align_inbound_addr(struct pci_epf *epf, enum pci_barno bar, > > + u64 addr, u64 *base, size_t *off) > > +{ > > + 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_inbound_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..eff73ccb5e702 100644 > > --- a/include/linux/pci-epf.h > > +++ b/include/linux/pci-epf.h > > @@ -238,6 +238,20 @@ 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_inbound_addr(struct pci_epf *epf, enum pci_barno bar, > > + u64 addr, u64 *base, size_t *off); > > +static inline int pci_epf_align_inbound_addr_lo_hi(struct pci_epf *epf, enum pci_barno bar, > > + u32 low, u32 high, u64 *base, size_t *off) > > Why can't you just use pci_epf_align_inbound_addr() directly? Or the caller > could pass u64 address directly. msi message sperate low32 and high32. (h << 32 | low) is quite easy to cause build warning. it should be ((u64) h << 32) | low. Avoid copy this logic code at many EPF places. Frank > > - Mani > > > +{ > > + u64 addr = high; > > + > > + addr <<= 32; > > + addr |= low; > > + > > + return pci_epf_align_inbound_addr(epf, bar, addr, base, off); > > +} > > + > > int pci_epf_bind(struct pci_epf *epf); > > void pci_epf_unbind(struct pci_epf *epf); > > int pci_epf_add_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf); > > > > -- > > 2.34.1 > > > > -- > மணிவண்ணன் சதாசிவம்
On Mon, Nov 25, 2024 at 02:22:23PM -0500, Frank Li wrote: > On Sun, Nov 24, 2024 at 01:02:39PM +0530, Manivannan Sadhasivam wrote: > > On Sat, Nov 16, 2024 at 09:40:43AM -0500, Frank Li wrote: > > > Introduce the helper function pci_epf_align_addr() to adjust addresses > > > > pci_epf_align_inbound_addr()? > > > > > according to PCI BAR alignment requirements, converting addresses into base > > > and offset values. > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > > --- > > > change from v7 to v8 > > > - change name to pci_epf_align_inbound_addr() > > > - update comment said only need for memory, which not allocated by > > > pci_epf_alloc_space(). > > > > > > change from v6 to v7 > > > - new patch > > > --- > > > drivers/pci/endpoint/pci-epf-core.c | 45 +++++++++++++++++++++++++++++++++++++ > > > include/linux/pci-epf.h | 14 ++++++++++++ > > > 2 files changed, 59 insertions(+) > > > > > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c > > > index 8fa2797d4169a..4dfc218ebe20b 100644 > > > --- a/drivers/pci/endpoint/pci-epf-core.c > > > +++ b/drivers/pci/endpoint/pci-epf-core.c > > > @@ -464,6 +464,51 @@ struct pci_epf *pci_epf_create(const char *name) > > > } > > > EXPORT_SYMBOL_GPL(pci_epf_create); > > > > > > +/** > > > + * pci_epf_align_inbound_addr() - Get base address and offset that match bar's > > > > 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 > > > > Below, you are updating 'base' only if it is not NULL. Why would anyone call > > this API with 'base' and 'offset' set to NULL? > > Some time, they may just want one of two. > What would be the purpose? I fail to see it. > > > > > + * @off: return offset, nothing return if NULL > > > + * > > > + * Helper function to convert input 'addr' to base and offset, which match > > > + * BAR's alignment requirement. > > > + * > > > + * The pci_epf_alloc_space() function already accounts for alignment. This is > > > + * primarily intended for use with other memory regions not allocated by > > > + * pci_epf_alloc_space(), such as peripheral register spaces or the trigger > > > + * address for a platform MSI controller. > > > + */ > > > +int pci_epf_align_inbound_addr(struct pci_epf *epf, enum pci_barno bar, > > > + u64 addr, u64 *base, size_t *off) > > > +{ > > > + 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_inbound_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..eff73ccb5e702 100644 > > > --- a/include/linux/pci-epf.h > > > +++ b/include/linux/pci-epf.h > > > @@ -238,6 +238,20 @@ 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_inbound_addr(struct pci_epf *epf, enum pci_barno bar, > > > + u64 addr, u64 *base, size_t *off); > > > +static inline int pci_epf_align_inbound_addr_lo_hi(struct pci_epf *epf, enum pci_barno bar, > > > + u32 low, u32 high, u64 *base, size_t *off) > > > > Why can't you just use pci_epf_align_inbound_addr() directly? Or the caller > > could pass u64 address directly. > > > msi message sperate low32 and high32. (h << 32 | low) is quite easy to > cause build warning. it should be ((u64) h << 32) | low. Avoid copy this > logic code at many EPF places. > There is absolutely no overhead in doing so. Also the concern for me is, pci_epf_align_inbound_addr() is exported but only used within pci_epf_align_inbound_addr_lo_hi(). This causes confusion. So I'd prefer to have a single exported API that is used by the callers. - Mani
On Tue, Nov 26, 2024 at 09:49:03AM +0530, Manivannan Sadhasivam wrote: > On Mon, Nov 25, 2024 at 02:22:23PM -0500, Frank Li wrote: > > On Sun, Nov 24, 2024 at 01:02:39PM +0530, Manivannan Sadhasivam wrote: > > > On Sat, Nov 16, 2024 at 09:40:43AM -0500, Frank Li wrote: > > > > Introduce the helper function pci_epf_align_addr() to adjust addresses > > > > > > pci_epf_align_inbound_addr()? > > > > > > > according to PCI BAR alignment requirements, converting addresses into base > > > > and offset values. > > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > > > --- > > > > change from v7 to v8 > > > > - change name to pci_epf_align_inbound_addr() > > > > - update comment said only need for memory, which not allocated by > > > > pci_epf_alloc_space(). > > > > > > > > change from v6 to v7 > > > > - new patch > > > > --- > > > > drivers/pci/endpoint/pci-epf-core.c | 45 +++++++++++++++++++++++++++++++++++++ > > > > include/linux/pci-epf.h | 14 ++++++++++++ > > > > 2 files changed, 59 insertions(+) > > > > > > > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c > > > > index 8fa2797d4169a..4dfc218ebe20b 100644 > > > > --- a/drivers/pci/endpoint/pci-epf-core.c > > > > +++ b/drivers/pci/endpoint/pci-epf-core.c > > > > @@ -464,6 +464,51 @@ struct pci_epf *pci_epf_create(const char *name) > > > > } > > > > EXPORT_SYMBOL_GPL(pci_epf_create); > > > > > > > > +/** > > > > + * pci_epf_align_inbound_addr() - Get base address and offset that match bar's > > > > > > 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 > > > > > > Below, you are updating 'base' only if it is not NULL. Why would anyone call > > > this API with 'base' and 'offset' set to NULL? > > > > Some time, they may just want one of two. > > > > What would be the purpose? I fail to see it. Currently, the only user of this function is the call: ret = pci_epf_align_inbound_addr_lo_hi(epf, bar, msg->address_lo, msg->address_hi, &db_bar.phys_addr, &offset); Which doesn't send in NULL as either 'base' or 'offset', so these NULL checks do currently look meaningless to me. I suggest to just kill them. Kind regards, Niklas
On Tue, Nov 26, 2024 at 09:49:03AM +0530, Manivannan Sadhasivam wrote: > On Mon, Nov 25, 2024 at 02:22:23PM -0500, Frank Li wrote: > > On Sun, Nov 24, 2024 at 01:02:39PM +0530, Manivannan Sadhasivam wrote: > > > On Sat, Nov 16, 2024 at 09:40:43AM -0500, Frank Li wrote: > > > > +static inline int pci_epf_align_inbound_addr_lo_hi(struct pci_epf *epf, enum pci_barno bar, > > > > + u32 low, u32 high, u64 *base, size_t *off) > > > > > > Why can't you just use pci_epf_align_inbound_addr() directly? Or the caller > > > could pass u64 address directly. > > > > > > msi message sperate low32 and high32. (h << 32 | low) is quite easy to > > cause build warning. it should be ((u64) h << 32) | low. Avoid copy this > > logic code at many EPF places. > > > > There is absolutely no overhead in doing so. Also the concern for me is, > pci_epf_align_inbound_addr() is exported but only used within > pci_epf_align_inbound_addr_lo_hi(). This causes confusion. So I'd prefer to have > a single exported API that is used by the callers. Yes, other EPF drivers will need to copy the line: pci_epf_align_inbound_addr(..., ((u64) h << 32) | low, ...) instead of: pci_epf_align_inbound_addr_lo_hi(..., low, high, ...) which I think is fine to be honest. Probably simplest thing is just to kill pci_epf_align_inbound_addr_lo_hi(). Kind regards, Niklas
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c index 8fa2797d4169a..4dfc218ebe20b 100644 --- a/drivers/pci/endpoint/pci-epf-core.c +++ b/drivers/pci/endpoint/pci-epf-core.c @@ -464,6 +464,51 @@ struct pci_epf *pci_epf_create(const char *name) } EXPORT_SYMBOL_GPL(pci_epf_create); +/** + * pci_epf_align_inbound_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. + * + * The pci_epf_alloc_space() function already accounts for alignment. This is + * primarily intended for use with other memory regions not allocated by + * pci_epf_alloc_space(), such as peripheral register spaces or the trigger + * address for a platform MSI controller. + */ +int pci_epf_align_inbound_addr(struct pci_epf *epf, enum pci_barno bar, + u64 addr, u64 *base, size_t *off) +{ + 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_inbound_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..eff73ccb5e702 100644 --- a/include/linux/pci-epf.h +++ b/include/linux/pci-epf.h @@ -238,6 +238,20 @@ 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_inbound_addr(struct pci_epf *epf, enum pci_barno bar, + u64 addr, u64 *base, size_t *off); +static inline int pci_epf_align_inbound_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_inbound_addr(epf, bar, addr, base, off); +} + int pci_epf_bind(struct pci_epf *epf); void pci_epf_unbind(struct pci_epf *epf); int pci_epf_add_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
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@nxp.com> --- change from v7 to v8 - change name to pci_epf_align_inbound_addr() - update comment said only need for memory, which not allocated by pci_epf_alloc_space(). change from v6 to v7 - new patch --- drivers/pci/endpoint/pci-epf-core.c | 45 +++++++++++++++++++++++++++++++++++++ include/linux/pci-epf.h | 14 ++++++++++++ 2 files changed, 59 insertions(+)