[PATCH v2] ALSA: document that struct __snd_pcm_mmap_control64 is messed up

[PATCH v2] ALSA: document that struct __snd_pcm_mmap_control64 is messed up

[Oswald Buddenhagen]

I'm not the first one to run into this, see e.g.
https://lore.kernel.org/all/29QBMJU8DE71E.2YZSH8IHT5HMH@mforney.org/

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 include/uapi/sound/asound.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index de6810e94abe..7eecc99ddef7 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -570,7 +570,8 @@ struct __snd_pcm_mmap_status64 {
 struct __snd_pcm_mmap_control64 {
 	__pad_before_uframe __pad1;
 	snd_pcm_uframes_t appl_ptr;	 /* RW: appl ptr (0...boundary-1) */
-	__pad_before_uframe __pad2;
+	__pad_before_uframe __pad2;	 // This should be __pad_after_uframe, but binary
+					 // backwards compatibility constraints prevent a fix.
 
 	__pad_before_uframe __pad3;
 	snd_pcm_uframes_t  avail_min;	 /* RW: min available frames for wakeup */
-- 
2.40.0.152.g15d061e6df

Re: [PATCH v2] ALSA: document that struct __snd_pcm_mmap_control64 is messed up

[Takashi Iwai]

On Thu, 06 Apr 2023 15:25:21 +0200,
Oswald Buddenhagen wrote:
> 
> I'm not the first one to run into this, see e.g.
> https://lore.kernel.org/all/29QBMJU8DE71E.2YZSH8IHT5HMH@mforney.org/
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

Applied now to for-next branch.

BTW, it'd be helpful if you put maintainers to Cc at the next time.
This will avoid overlooking patches that have been posted on ML.


thanks,

Takashi

Re: [PATCH] ALSA: document that struct __snd_pcm_mmap_control64 is messed up

[Takashi Iwai]

On Thu, 06 Apr 2023 08:46:51 +0200,
Oswald Buddenhagen wrote:
> 
> On Thu, Apr 06, 2023 at 08:03:39AM +0200, Takashi Iwai wrote:
> > On Wed, 05 Apr 2023 22:12:19 +0200,
> > Oswald Buddenhagen wrote:
> >> 
> >> I suppose this can't be changed anymore due to binary compat concerns.
> > 
> > Yes, please check the thread at
> >  https://lore.kernel.org/all/29QBMJU8DE71E.2YZSH8IHT5HMH@mforney.org/
> > 
> >> +	__pad_before_uframe __pad2;	 // BUG: this should have been __pad_after_uframe!
> > 
> > Writing this alone doesn't help much.  Actual help would be to mention
> > that this typo is kept intentionally.
> > 
> hmm, my thinking is that the immediate response to reading that
> comment would be "so why don't you change it?!", at which point the
> person would either realize by themselves that this is subject to
> binary compat constraints, or would git-blame it and see the
> explanation.
> 
> anyway, my concern is keeping this *short*, so it doesn't distract.
> 
> maybe
> 
>   // BUG: should be __pad_after_uframe, but binary compat
> 
> would do, though obviously the grammar kinda sucks.
> 
> the (too) long version could be
> 
>   // BUG: this should be __pad_after_uframe, but
>   // binary compatibility constraints prevent a fix.
> 
> choose your death, i'll deliver it. ;-)

The "BUG:" suffix should be dropped.  This would catch eyes of (badly)
trained kernel programmers as if it were a kernel panic message :)

Also the term "binary compatibility" is ambiguous in this context --
especially because we're dealing with the code that treats the
32/64bit binary compatibility.

But, yeah, I'm for that direction.


thanks,

Takashi

[PATCH v2] ALSA: document that struct __snd_pcm_mmap_control64 is messed up

[Oswald Buddenhagen]

On Thu, Apr 06, 2023 at 09:48:40AM +0200, Takashi Iwai wrote:
>The "BUG:" suffix should be dropped.  This would catch eyes of (badly)
>trained kernel programmers as if it were a kernel panic message :)
>
done

>Also the term "binary compatibility" is ambiguous in this context --
>especially because we're dealing with the code that treats the
>32/64bit binary compatibility.
>
i wasn't sure what to make of that. how about this:

-- >8 --

I'm not the first one to run into this, see e.g.
https://lore.kernel.org/all/29QBMJU8DE71E.2YZSH8IHT5HMH@mforney.org/

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 include/uapi/sound/asound.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index de6810e94abe..7eecc99ddef7 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -570,7 +570,8 @@ struct __snd_pcm_mmap_status64 {
 struct __snd_pcm_mmap_control64 {
 	__pad_before_uframe __pad1;
 	snd_pcm_uframes_t appl_ptr;	 /* RW: appl ptr (0...boundary-1) */
-	__pad_before_uframe __pad2;
+	__pad_before_uframe __pad2;	 // This should be __pad_after_uframe, but binary
+					 // backwards compatibility constraints prevent a fix.
 
 	__pad_before_uframe __pad3;
 	snd_pcm_uframes_t  avail_min;	 /* RW: min available frames for wakeup */
-- 
2.40.0.152.g15d061e6df

Re: [PATCH v2] ALSA: document that struct __snd_pcm_mmap_control64 is messed up

[Takashi Iwai]

On Thu, 06 Apr 2023 13:15:45 +0200,
Oswald Buddenhagen wrote:
> 
> On Thu, Apr 06, 2023 at 09:48:40AM +0200, Takashi Iwai wrote:
> >The "BUG:" suffix should be dropped.  This would catch eyes of (badly)
> >trained kernel programmers as if it were a kernel panic message :)
> >
> done
> 
> >Also the term "binary compatibility" is ambiguous in this context --
> >especially because we're dealing with the code that treats the
> >32/64bit binary compatibility.
> >
> i wasn't sure what to make of that. how about this:
> 
> -- >8 --
> 
> I'm not the first one to run into this, see e.g.
> https://lore.kernel.org/all/29QBMJU8DE71E.2YZSH8IHT5HMH@mforney.org/
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
>  include/uapi/sound/asound.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index de6810e94abe..7eecc99ddef7 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -570,7 +570,8 @@ struct __snd_pcm_mmap_status64 {
>  struct __snd_pcm_mmap_control64 {
>  	__pad_before_uframe __pad1;
>  	snd_pcm_uframes_t appl_ptr;	 /* RW: appl ptr (0...boundary-1) */
> -	__pad_before_uframe __pad2;
> +	__pad_before_uframe __pad2;	 // This should be __pad_after_uframe, but binary
> +					 // backwards compatibility constraints prevent a fix.

Looks much better.
Care to resubmit v2 patch?


Thanks!

Takashi

Re: [PATCH v2] ALSA: document that struct __snd_pcm_mmap_control64 is messed up

[Oswald Buddenhagen]

On Thu, Apr 06, 2023 at 02:29:44PM +0200, Takashi Iwai wrote:
>Care to resubmit v2 patch?
>
that *was* a v2 patch - you can git-am the mail with --scissors (at 
least if i got it right). not acceptable?

Re: [PATCH v2] ALSA: document that struct __snd_pcm_mmap_control64 is messed up

[Takashi Iwai]

On Thu, 06 Apr 2023 14:34:36 +0200,
Oswald Buddenhagen wrote:
> 
> On Thu, Apr 06, 2023 at 02:29:44PM +0200, Takashi Iwai wrote:
> > Care to resubmit v2 patch?
> > 
> that *was* a v2 patch - you can git-am the mail with --scissors (at
> least if i got it right). not acceptable?

Please resubmit properly.  Nowadays commits have the link to the mail
of the patch submission, and such mixup makes it harder to follow.


Takashi