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@xxxxxxx/
> 
> 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