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

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

[Oswald Buddenhagen]

I suppose this can't be changed anymore due to binary compat concerns.

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

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index de6810e94abe..595a683968bc 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -570,7 +570,7 @@ 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;	 // BUG: this should have been __pad_after_uframe!
 
 	__pad_before_uframe __pad3;
 	snd_pcm_uframes_t  avail_min;	 /* RW: min available frames for wakeup */
-- 
2.40.0.152.g15d061e6df

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

[Takashi Iwai]

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/

> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
>  include/uapi/sound/asound.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index de6810e94abe..595a683968bc 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -570,7 +570,7 @@ 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;	 // 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.


Takashi

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

[Oswald Buddenhagen]

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. ;-)

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