| Message ID | 20241014105912.3207374-22-ryan.roberts@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Boot-time page size selection for arm64 | expand |
On Mon, Oct 14, 2024 at 11:58:29AM +0100, Ryan Roberts wrote: > To prepare for supporting boot-time page size selection, refactor code > to remove assumptions about PAGE_SIZE being compile-time constant. Code > intended to be equivalent when compile-time page size is active. Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone. > ***NOTE*** > Any confused maintainers may want to read the cover note here for context: > https://lore.kernel.org/all/20241014105514.3206191-1-ryan.roberts@arm.com/ As documented in submitting-patches.rst please send patches to the maintainers for the code you would like to change. The normal kernel workflow is that people apply patches from their inboxes, if they aren't copied they are likely to not see the patch at all and it is much more difficult to apply patches. > -static const struct snd_pcm_hardware dummy_dma_hardware = { > +static DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST(struct snd_pcm_hardware, dummy_dma_hardware, { > /* Random values to keep userspace happy when checking constraints */ > .info = SNDRV_PCM_INFO_INTERLEAVED | > SNDRV_PCM_INFO_BLOCK_TRANSFER, > @@ -107,7 +107,7 @@ static const struct snd_pcm_hardware dummy_dma_hardware = { > .period_bytes_max = PAGE_SIZE*2, > .periods_min = 2, > .periods_max = 128, > -}; > +}); It's probably better to just use PAGE_SIZE_MAX here and avoid the deferred patching, like the comment says we don't particularly care what the value actually is here given that it's a dummy.
On 14/10/2024 12:38, Mark Brown wrote: > On Mon, Oct 14, 2024 at 11:58:29AM +0100, Ryan Roberts wrote: >> To prepare for supporting boot-time page size selection, refactor code >> to remove assumptions about PAGE_SIZE being compile-time constant. Code >> intended to be equivalent when compile-time page size is active. > > Please submit patches using subject lines reflecting the style for the > subsystem, this makes it easier for people to identify relevant patches. > Look at what existing commits in the area you're changing are doing and > make sure your subject lines visually resemble what they're doing. > There's no need to resubmit to fix this alone. No problem, will fix this in the next round (where I anticipate sending more targetted serieses to maintainers). > >> ***NOTE*** >> Any confused maintainers may want to read the cover note here for context: >> https://lore.kernel.org/all/20241014105514.3206191-1-ryan.roberts@arm.com/ > > As documented in submitting-patches.rst please send patches to the > maintainers for the code you would like to change. The normal kernel > workflow is that people apply patches from their inboxes, if they aren't > copied they are likely to not see the patch at all and it is much more > difficult to apply patches. Sure. I think you're implying that you would have liked to be in To: for this patch? I went to quite a lot of trouble to ensure all maintainers were at least in the To: field for patches touching their code. But get_maintainer.pl lists you as a supporter, not a maintainer when I ran this patch through. Could you clarify what would have been the correct thing to do? I could include all reviewers and supporters as well as maintainers but then I'd be banging up against the limits for some of the patches. Or perhaps I've misunderstood the point you're making here. > >> -static const struct snd_pcm_hardware dummy_dma_hardware = { >> +static DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST(struct snd_pcm_hardware, dummy_dma_hardware, { >> /* Random values to keep userspace happy when checking constraints */ >> .info = SNDRV_PCM_INFO_INTERLEAVED | >> SNDRV_PCM_INFO_BLOCK_TRANSFER, >> @@ -107,7 +107,7 @@ static const struct snd_pcm_hardware dummy_dma_hardware = { >> .period_bytes_max = PAGE_SIZE*2, >> .periods_min = 2, >> .periods_max = 128, >> -}; >> +}); > > It's probably better to just use PAGE_SIZE_MAX here and avoid the > deferred patching, like the comment says we don't particularly care what > the value actually is here given that it's a dummy. OK, so would that be: .buffer_bytes_max = 128*1024, .period_bytes_min = PAGE_SIZE_MAX, <<<<< .period_bytes_max = PAGE_SIZE_MAX*2, <<<<< .periods_min = 2, .periods_max = 128, ? It's not really clear to me how all the parameters interact; the buffer size 128K, which, if PAGE_SIZE_MAX is 64K, would hold 1 period of the maximum size. But periods_min is 2. So not sure that works? Or perhaps I'm trying to apply too much meaning to the param names... Thanks, Ryan
On Mon, 14 Oct 2024 14:24:02 +0200, Ryan Roberts wrote: > > On 14/10/2024 12:38, Mark Brown wrote: > > On Mon, Oct 14, 2024 at 11:58:29AM +0100, Ryan Roberts wrote: > >> -static const struct snd_pcm_hardware dummy_dma_hardware = { > >> +static DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST(struct snd_pcm_hardware, dummy_dma_hardware, { > >> /* Random values to keep userspace happy when checking constraints */ > >> .info = SNDRV_PCM_INFO_INTERLEAVED | > >> SNDRV_PCM_INFO_BLOCK_TRANSFER, > >> @@ -107,7 +107,7 @@ static const struct snd_pcm_hardware dummy_dma_hardware = { > >> .period_bytes_max = PAGE_SIZE*2, > >> .periods_min = 2, > >> .periods_max = 128, > >> -}; > >> +}); > > > > It's probably better to just use PAGE_SIZE_MAX here and avoid the > > deferred patching, like the comment says we don't particularly care what > > the value actually is here given that it's a dummy. > > OK, so would that be: > > .buffer_bytes_max = 128*1024, > .period_bytes_min = PAGE_SIZE_MAX, <<<<< > .period_bytes_max = PAGE_SIZE_MAX*2, <<<<< > .periods_min = 2, > .periods_max = 128, > > ? > > It's not really clear to me how all the parameters interact; the buffer size > 128K, which, if PAGE_SIZE_MAX is 64K, would hold 1 period of the maximum size. > But periods_min is 2. So not sure that works? Or perhaps I'm trying to apply too > much meaning to the param names... Right, when PAGE_SIZE_MAX is 64k, 128k won't be used because of the constrant of periods_min=2. As Mark mentioned, here the actual size itself doesn't matter much. So I suppose it'd be even simpler to define just 4096 and 4096 * 2 for period_bytes_min and *_max instead of sticking with PAGE_SIZE. Then it would become platform-agnostic, too. thanks, Takashi
On 14/10/2024 13:41, Takashi Iwai wrote: > On Mon, 14 Oct 2024 14:24:02 +0200, > Ryan Roberts wrote: >> >> On 14/10/2024 12:38, Mark Brown wrote: >>> On Mon, Oct 14, 2024 at 11:58:29AM +0100, Ryan Roberts wrote: >>>> -static const struct snd_pcm_hardware dummy_dma_hardware = { >>>> +static DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST(struct snd_pcm_hardware, dummy_dma_hardware, { >>>> /* Random values to keep userspace happy when checking constraints */ >>>> .info = SNDRV_PCM_INFO_INTERLEAVED | >>>> SNDRV_PCM_INFO_BLOCK_TRANSFER, >>>> @@ -107,7 +107,7 @@ static const struct snd_pcm_hardware dummy_dma_hardware = { >>>> .period_bytes_max = PAGE_SIZE*2, >>>> .periods_min = 2, >>>> .periods_max = 128, >>>> -}; >>>> +}); >>> >>> It's probably better to just use PAGE_SIZE_MAX here and avoid the >>> deferred patching, like the comment says we don't particularly care what >>> the value actually is here given that it's a dummy. >> >> OK, so would that be: >> >> .buffer_bytes_max = 128*1024, >> .period_bytes_min = PAGE_SIZE_MAX, <<<<< >> .period_bytes_max = PAGE_SIZE_MAX*2, <<<<< >> .periods_min = 2, >> .periods_max = 128, >> >> ? >> >> It's not really clear to me how all the parameters interact; the buffer size >> 128K, which, if PAGE_SIZE_MAX is 64K, would hold 1 period of the maximum size. >> But periods_min is 2. So not sure that works? Or perhaps I'm trying to apply too >> much meaning to the param names... > > Right, when PAGE_SIZE_MAX is 64k, 128k won't be used because of the > constrant of periods_min=2. > > As Mark mentioned, here the actual size itself doesn't matter much. > So I suppose it'd be even simpler to define just 4096 and 4096 * 2 for > period_bytes_min and *_max instead of sticking with PAGE_SIZE. Then > it would become platform-agnostic, too. OK great I'll set these to 4096 and 4096*2 for the next version. Thanks for the feedback! > > > thanks, > > Takashi
On Mon, Oct 14, 2024 at 01:24:02PM +0100, Ryan Roberts wrote: > On 14/10/2024 12:38, Mark Brown wrote: > > On Mon, Oct 14, 2024 at 11:58:29AM +0100, Ryan Roberts wrote: > >> ***NOTE*** > >> Any confused maintainers may want to read the cover note here for context: > >> https://lore.kernel.org/all/20241014105514.3206191-1-ryan.roberts@arm.com/ > > As documented in submitting-patches.rst please send patches to the > > maintainers for the code you would like to change. The normal kernel > > workflow is that people apply patches from their inboxes, if they aren't > > copied they are likely to not see the patch at all and it is much more > > difficult to apply patches. > Sure. I think you're implying that you would have liked to be in To: for this > patch? I went to quite a lot of trouble to ensure all maintainers were at least > in the To: field for patches touching their code. But get_maintainer.pl lists > you as a supporter, not a maintainer when I ran this patch through. Could you > clarify what would have been the correct thing to do? I could include all > reviewers and supporters as well as maintainers but then I'd be banging up > against the limits for some of the patches. The entry in MAINTAINERS for me is a M:, supporter is just the usual get_maintainers noise. Supported is exactly equivalent to a maintainer. Generally if you're going to filter people you should be filtering less specific matches out rather than more and if you're looking to filter very aggressively look at who actually commits changes to whatever you're trying to change, less specific maintainers will generally delegate down to the more specific ones. > > It's probably better to just use PAGE_SIZE_MAX here and avoid the > > deferred patching, like the comment says we don't particularly care what > > the value actually is here given that it's a dummy. > OK, so would that be: > .buffer_bytes_max = 128*1024, > .period_bytes_min = PAGE_SIZE_MAX, <<<<< > .period_bytes_max = PAGE_SIZE_MAX*2, <<<<< > .periods_min = 2, > .periods_max = 128, > It's not really clear to me how all the parameters interact; the buffer size > 128K, which, if PAGE_SIZE_MAX is 64K, would hold 1 period of the maximum size. > But periods_min is 2. So not sure that works? Or perhaps I'm trying to apply too > much meaning to the param names... Like Takashi says just using absolute numbers here is probably just as sensible, the numbers are there to stop userspace tripping over itself but like I say it shouldn't ever get as far as actually using them for anything. So long as we end up with some numbers that don't need any late init patching the specifics aren't super important, the use of PAGE_SIZE was kind of random.
On 14/10/2024 17:01, Mark Brown wrote: > On Mon, Oct 14, 2024 at 01:24:02PM +0100, Ryan Roberts wrote: >> On 14/10/2024 12:38, Mark Brown wrote: >>> On Mon, Oct 14, 2024 at 11:58:29AM +0100, Ryan Roberts wrote: > >>>> ***NOTE*** >>>> Any confused maintainers may want to read the cover note here for context: >>>> https://lore.kernel.org/all/20241014105514.3206191-1-ryan.roberts@arm.com/ > >>> As documented in submitting-patches.rst please send patches to the >>> maintainers for the code you would like to change. The normal kernel >>> workflow is that people apply patches from their inboxes, if they aren't >>> copied they are likely to not see the patch at all and it is much more >>> difficult to apply patches. > >> Sure. I think you're implying that you would have liked to be in To: for this >> patch? I went to quite a lot of trouble to ensure all maintainers were at least >> in the To: field for patches touching their code. But get_maintainer.pl lists >> you as a supporter, not a maintainer when I ran this patch through. Could you >> clarify what would have been the correct thing to do? I could include all >> reviewers and supporters as well as maintainers but then I'd be banging up >> against the limits for some of the patches. > > The entry in MAINTAINERS for me is a M:, supporter is just the usual > get_maintainers noise. Supported is exactly equivalent to a maintainer. Ugh, In my head I always thought "supporter" was somebody who engaged with the subsystem but did not have an official role (like a football supporter). But now that I've gone and read the MAINTAINERS file, I see it's actually referring to status (supported vs maintained). Sorry about this. Due to this buggy filtering, I've missed a few others off other patches in this series. I'll fix that by forwarding to them. > Generally if you're going to filter people you should be filtering less > specific matches out rather than more and if you're looking to filter > very aggressively look at who actually commits changes to whatever > you're trying to change, less specific maintainers will generally > delegate down to the more specific ones. > >>> It's probably better to just use PAGE_SIZE_MAX here and avoid the >>> deferred patching, like the comment says we don't particularly care what >>> the value actually is here given that it's a dummy. > >> OK, so would that be: > >> .buffer_bytes_max = 128*1024, >> .period_bytes_min = PAGE_SIZE_MAX, <<<<< >> .period_bytes_max = PAGE_SIZE_MAX*2, <<<<< >> .periods_min = 2, >> .periods_max = 128, > >> It's not really clear to me how all the parameters interact; the buffer size >> 128K, which, if PAGE_SIZE_MAX is 64K, would hold 1 period of the maximum size. >> But periods_min is 2. So not sure that works? Or perhaps I'm trying to apply too >> much meaning to the param names... > > Like Takashi says just using absolute numbers here is probably just as > sensible, the numbers are there to stop userspace tripping over itself > but like I say it shouldn't ever get as far as actually using them for > anything. So long as we end up with some numbers that don't need any > late init patching the specifics aren't super important, the use of > PAGE_SIZE was kind of random. OK, I'll post a respin of this patch independently of the rest of the series, given it no longer has a dependency. Thanks, Ryan
diff --git a/sound/soc/soc-utils.c b/sound/soc/soc-utils.c index 303823dc45d7a..74e1bc1087de4 100644 --- a/sound/soc/soc-utils.c +++ b/sound/soc/soc-utils.c @@ -98,7 +98,7 @@ int snd_soc_tdm_params_to_bclk(const struct snd_pcm_hw_params *params, } EXPORT_SYMBOL_GPL(snd_soc_tdm_params_to_bclk); -static const struct snd_pcm_hardware dummy_dma_hardware = { +static DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST(struct snd_pcm_hardware, dummy_dma_hardware, { /* Random values to keep userspace happy when checking constraints */ .info = SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER, @@ -107,7 +107,7 @@ static const struct snd_pcm_hardware dummy_dma_hardware = { .period_bytes_max = PAGE_SIZE*2, .periods_min = 2, .periods_max = 128, -}; +}); static const struct snd_soc_component_driver dummy_platform;
To prepare for supporting boot-time page size selection, refactor code to remove assumptions about PAGE_SIZE being compile-time constant. Code intended to be equivalent when compile-time page size is active. Wrap global variables that are initialized with PAGE_SIZE derived values using DEFINE_GLOBAL_PAGE_SIZE_VAR() so their initialization can be deferred for boot-time page size builds. Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> --- ***NOTE*** Any confused maintainers may want to read the cover note here for context: https://lore.kernel.org/all/20241014105514.3206191-1-ryan.roberts@arm.com/ sound/soc/soc-utils.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)