[RFC PATCH v1 22/57] sound: Remove PAGE_SIZE compile-time constant assumption

[RFC PATCH v1 22/57] sound: Remove PAGE_SIZE compile-time constant assumption

[Ryan Roberts]

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

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

Re: [RFC PATCH v1 22/57] sound: Remove PAGE_SIZE compile-time constant assumption

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1806 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH v1 22/57] sound: Remove PAGE_SIZE compile-time constant assumption

[Ryan Roberts]

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

Re: [RFC PATCH v1 22/57] sound: Remove PAGE_SIZE compile-time constant assumption

[Takashi Iwai]

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

Re: [RFC PATCH v1 22/57] sound: Remove PAGE_SIZE compile-time constant assumption

[Ryan Roberts]

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

Re: [RFC PATCH v1 22/57] sound: Remove PAGE_SIZE compile-time constant assumption

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2781 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH v1 22/57] sound: Remove PAGE_SIZE compile-time constant assumption

[Ryan Roberts]

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