| Message ID | 20250328-pci-ep-size-alignment-v1-2-ee5b78b15a9a@baylibre.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: endpoint: space allocation fixups | expand |
On Fri, Mar 28, 2025 at 03:53:43PM +0100, Jerome Brunet wrote: > When allocating the shared ctrl/spad space, epf_ntb_config_spad_bar_alloc() > should not try to handle the size quirks for the underlying BAR, whether it > is fixed size or alignment. This is already handled by > pci_epf_alloc_space(). > > Also, when handling the alignment, this allocate more space than necessary. > For example, with a spad size of 1024B and a ctrl size of 308B, the space > necessary is 1332B. If the alignment is 1MB, > epf_ntb_config_spad_bar_alloc() tries to allocate 2MB where 1MB would have > been more than enough. > > Just drop all the handling of the BAR size quirks and let > pci_epf_alloc_space() handle that. > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/pci/endpoint/functions/pci-epf-vntb.c | 24 ++---------------------- > 1 file changed, 2 insertions(+), 22 deletions(-) > > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c > index 874cb097b093ae645bbc4bf3c9d28ca812d7689d..c20a60fcb99e6e16716dd78ab59ebf7cf074b2a6 100644 > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c > @@ -408,11 +408,9 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb) > */ > static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb) > { > - size_t align; > enum pci_barno barno; > struct epf_ntb_ctrl *ctrl; > u32 spad_size, ctrl_size; > - u64 size; > struct pci_epf *epf = ntb->epf; > struct device *dev = &epf->dev; > u32 spad_count; > @@ -422,31 +420,13 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb) > epf->func_no, > epf->vfunc_no); > barno = ntb->epf_ntb_bar[BAR_CONFIG]; > - size = epc_features->bar[barno].fixed_size; > - align = epc_features->align; > - > - if ((!IS_ALIGNED(size, align))) > - return -EINVAL; > - > spad_count = ntb->spad_count; > > ctrl_size = sizeof(struct epf_ntb_ctrl); I think keep ctrl_size at least align to 4 bytes. keep align 2^n is more safe to keep spad area start at align possition. ctrl_size = roundup_pow_of_two(sizeof(struct epf_ntb_ctrl)); Frank > spad_size = 2 * spad_count * sizeof(u32); > > - if (!align) { > - ctrl_size = roundup_pow_of_two(ctrl_size); > - spad_size = roundup_pow_of_two(spad_size); > - } else { > - ctrl_size = ALIGN(ctrl_size, align); > - spad_size = ALIGN(spad_size, align); > - } > - > - if (!size) > - size = ctrl_size + spad_size; > - else if (size < ctrl_size + spad_size) > - return -EINVAL; > - > - base = pci_epf_alloc_space(epf, size, barno, epc_features, 0); > + base = pci_epf_alloc_space(epf, ctrl_size + spad_size, > + barno, epc_features, 0); > if (!base) { > dev_err(dev, "Config/Status/SPAD alloc region fail\n"); > return -ENOMEM; > > -- > 2.47.2 >
On Mon 31 Mar 2025 at 10:48, Frank Li <Frank.li@nxp.com> wrote: > On Fri, Mar 28, 2025 at 03:53:43PM +0100, Jerome Brunet wrote: >> When allocating the shared ctrl/spad space, epf_ntb_config_spad_bar_alloc() >> should not try to handle the size quirks for the underlying BAR, whether it >> is fixed size or alignment. This is already handled by >> pci_epf_alloc_space(). >> >> Also, when handling the alignment, this allocate more space than necessary. >> For example, with a spad size of 1024B and a ctrl size of 308B, the space >> necessary is 1332B. If the alignment is 1MB, >> epf_ntb_config_spad_bar_alloc() tries to allocate 2MB where 1MB would have >> been more than enough. >> >> Just drop all the handling of the BAR size quirks and let >> pci_epf_alloc_space() handle that. >> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >> --- >> drivers/pci/endpoint/functions/pci-epf-vntb.c | 24 ++---------------------- >> 1 file changed, 2 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c >> index 874cb097b093ae645bbc4bf3c9d28ca812d7689d..c20a60fcb99e6e16716dd78ab59ebf7cf074b2a6 100644 >> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c >> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c >> @@ -408,11 +408,9 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb) >> */ >> static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb) >> { >> - size_t align; >> enum pci_barno barno; >> struct epf_ntb_ctrl *ctrl; >> u32 spad_size, ctrl_size; >> - u64 size; >> struct pci_epf *epf = ntb->epf; >> struct device *dev = &epf->dev; >> u32 spad_count; >> @@ -422,31 +420,13 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb) >> epf->func_no, >> epf->vfunc_no); >> barno = ntb->epf_ntb_bar[BAR_CONFIG]; >> - size = epc_features->bar[barno].fixed_size; >> - align = epc_features->align; >> - >> - if ((!IS_ALIGNED(size, align))) >> - return -EINVAL; >> - >> spad_count = ntb->spad_count; >> >> ctrl_size = sizeof(struct epf_ntb_ctrl); > > I think keep ctrl_size at least align to 4 bytes. Sure, makes sense > keep align 2^n is more safe to keep spad area start at align > possition. That's something else. Both region are registers (or the emulation of it) so a 32bits aligned is enough, AFAICT. What purpose would 2^n aligned serve ? If it is safer, what's is the risk exactly ? > > ctrl_size = roundup_pow_of_two(sizeof(struct epf_ntb_ctrl)); > > Frank > >> spad_size = 2 * spad_count * sizeof(u32); >> >> - if (!align) { >> - ctrl_size = roundup_pow_of_two(ctrl_size); >> - spad_size = roundup_pow_of_two(spad_size); >> - } else { >> - ctrl_size = ALIGN(ctrl_size, align); >> - spad_size = ALIGN(spad_size, align); >> - } >> - >> - if (!size) >> - size = ctrl_size + spad_size; >> - else if (size < ctrl_size + spad_size) >> - return -EINVAL; >> - >> - base = pci_epf_alloc_space(epf, size, barno, epc_features, 0); >> + base = pci_epf_alloc_space(epf, ctrl_size + spad_size, >> + barno, epc_features, 0); >> if (!base) { >> dev_err(dev, "Config/Status/SPAD alloc region fail\n"); >> return -ENOMEM; >> >> -- >> 2.47.2 >>
On Tue, Apr 01, 2025 at 09:39:10AM +0200, Jerome Brunet wrote: > On Mon 31 Mar 2025 at 10:48, Frank Li <Frank.li@nxp.com> wrote: > > > On Fri, Mar 28, 2025 at 03:53:43PM +0100, Jerome Brunet wrote: > >> When allocating the shared ctrl/spad space, epf_ntb_config_spad_bar_alloc() > >> should not try to handle the size quirks for the underlying BAR, whether it > >> is fixed size or alignment. This is already handled by > >> pci_epf_alloc_space(). > >> > >> Also, when handling the alignment, this allocate more space than necessary. > >> For example, with a spad size of 1024B and a ctrl size of 308B, the space > >> necessary is 1332B. If the alignment is 1MB, > >> epf_ntb_config_spad_bar_alloc() tries to allocate 2MB where 1MB would have > >> been more than enough. > >> > >> Just drop all the handling of the BAR size quirks and let > >> pci_epf_alloc_space() handle that. > >> > >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > >> --- > >> drivers/pci/endpoint/functions/pci-epf-vntb.c | 24 ++---------------------- > >> 1 file changed, 2 insertions(+), 22 deletions(-) > >> > >> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c > >> index 874cb097b093ae645bbc4bf3c9d28ca812d7689d..c20a60fcb99e6e16716dd78ab59ebf7cf074b2a6 100644 > >> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c > >> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c > >> @@ -408,11 +408,9 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb) > >> */ > >> static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb) > >> { > >> - size_t align; > >> enum pci_barno barno; > >> struct epf_ntb_ctrl *ctrl; > >> u32 spad_size, ctrl_size; > >> - u64 size; > >> struct pci_epf *epf = ntb->epf; > >> struct device *dev = &epf->dev; > >> u32 spad_count; > >> @@ -422,31 +420,13 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb) > >> epf->func_no, > >> epf->vfunc_no); > >> barno = ntb->epf_ntb_bar[BAR_CONFIG]; > >> - size = epc_features->bar[barno].fixed_size; > >> - align = epc_features->align; > >> - > >> - if ((!IS_ALIGNED(size, align))) > >> - return -EINVAL; > >> - > >> spad_count = ntb->spad_count; > >> > >> ctrl_size = sizeof(struct epf_ntb_ctrl); > > > > I think keep ctrl_size at least align to 4 bytes. > > Sure, makes sense > > > keep align 2^n is more safe to keep spad area start at align > > possition. > > That's something else. Both region are registers (or the emulation of > it) so a 32bits aligned is enough, AFAICT. > > What purpose would 2^n aligned serve ? If it is safer, what's is the risk > exactly ? After second think, it should be fine if 4 bytes align. Frank > > > > > ctrl_size = roundup_pow_of_two(sizeof(struct epf_ntb_ctrl)); > > > > Frank > > > >> spad_size = 2 * spad_count * sizeof(u32); > >> > >> - if (!align) { > >> - ctrl_size = roundup_pow_of_two(ctrl_size); > >> - spad_size = roundup_pow_of_two(spad_size); > >> - } else { > >> - ctrl_size = ALIGN(ctrl_size, align); > >> - spad_size = ALIGN(spad_size, align); > >> - } > >> - > >> - if (!size) > >> - size = ctrl_size + spad_size; > >> - else if (size < ctrl_size + spad_size) > >> - return -EINVAL; > >> - > >> - base = pci_epf_alloc_space(epf, size, barno, epc_features, 0); > >> + base = pci_epf_alloc_space(epf, ctrl_size + spad_size, > >> + barno, epc_features, 0); > >> if (!base) { > >> dev_err(dev, "Config/Status/SPAD alloc region fail\n"); > >> return -ENOMEM; > >> > >> -- > >> 2.47.2 > >> > > -- > Jerome
diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c index 874cb097b093ae645bbc4bf3c9d28ca812d7689d..c20a60fcb99e6e16716dd78ab59ebf7cf074b2a6 100644 --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c @@ -408,11 +408,9 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb) */ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb) { - size_t align; enum pci_barno barno; struct epf_ntb_ctrl *ctrl; u32 spad_size, ctrl_size; - u64 size; struct pci_epf *epf = ntb->epf; struct device *dev = &epf->dev; u32 spad_count; @@ -422,31 +420,13 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb) epf->func_no, epf->vfunc_no); barno = ntb->epf_ntb_bar[BAR_CONFIG]; - size = epc_features->bar[barno].fixed_size; - align = epc_features->align; - - if ((!IS_ALIGNED(size, align))) - return -EINVAL; - spad_count = ntb->spad_count; ctrl_size = sizeof(struct epf_ntb_ctrl); spad_size = 2 * spad_count * sizeof(u32); - if (!align) { - ctrl_size = roundup_pow_of_two(ctrl_size); - spad_size = roundup_pow_of_two(spad_size); - } else { - ctrl_size = ALIGN(ctrl_size, align); - spad_size = ALIGN(spad_size, align); - } - - if (!size) - size = ctrl_size + spad_size; - else if (size < ctrl_size + spad_size) - return -EINVAL; - - base = pci_epf_alloc_space(epf, size, barno, epc_features, 0); + base = pci_epf_alloc_space(epf, ctrl_size + spad_size, + barno, epc_features, 0); if (!base) { dev_err(dev, "Config/Status/SPAD alloc region fail\n"); return -ENOMEM;
When allocating the shared ctrl/spad space, epf_ntb_config_spad_bar_alloc() should not try to handle the size quirks for the underlying BAR, whether it is fixed size or alignment. This is already handled by pci_epf_alloc_space(). Also, when handling the alignment, this allocate more space than necessary. For example, with a spad size of 1024B and a ctrl size of 308B, the space necessary is 1332B. If the alignment is 1MB, epf_ntb_config_spad_bar_alloc() tries to allocate 2MB where 1MB would have been more than enough. Just drop all the handling of the BAR size quirks and let pci_epf_alloc_space() handle that. Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/pci/endpoint/functions/pci-epf-vntb.c | 24 ++---------------------- 1 file changed, 2 insertions(+), 22 deletions(-)