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(+)

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);

-- 
2.34.1

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:
> > > > +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

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