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
On Tue 01 Apr 2025 at 10:55, Frank Li <Frank.li@nxp.com> wrote: > 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 Ok. Thanks for the feedback. I think the same type of change should probably be applied to the NTB endpoint function. It also tries to handle the alignment on its own, but that's mixed up with msix doorbell things I don't have the necessary HW to test that function so it would be a bit risky for me to modify it, but it would be nice for the two endpoint functions to be somehow aligned, especially since they share the same RC side driver. If anyone is able to help on this, that would be greatly appreciated :) > >> >> > >> > 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(-)