Re: [PATCH 3/3] [PATCH] staging: greybus: __u8 is sufficient for snd_ctl_elem_type_t and snd_ctl_elem_iface_t

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Sep 25, 2020 at 10:57:27AM -0500, Alex Elder wrote:
On 9/25/20 9:07 AM, Coiby Xu wrote:
On Thu, Sep 24, 2020 at 01:00:57PM +0200, Greg Kroah-Hartman wrote:
On Thu, Sep 24, 2020 at 06:20:39PM +0800, Coiby Xu wrote:
Use __8 to replace int and remove the unnecessary __bitwise type
attribute.

Found by sparse,

. . .

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 535a7229e1d9..8e71a95644ab 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -950,7 +950,7 @@ struct snd_ctl_card_info {
     unsigned char components[128];    /* card components / fine
identification, delimited with one space (AC97 etc..) */
 };

-typedef int __bitwise snd_ctl_elem_type_t;
+typedef __u8 snd_ctl_elem_type_t;
 #define    SNDRV_CTL_ELEM_TYPE_NONE    ((__force
snd_ctl_elem_type_t) 0) /* invalid */
 #define    SNDRV_CTL_ELEM_TYPE_BOOLEAN    ((__force
snd_ctl_elem_type_t) 1) /* boolean type */
 #define    SNDRV_CTL_ELEM_TYPE_INTEGER    ((__force
snd_ctl_elem_type_t) 2) /* integer type */
@@ -960,7 +960,7 @@ typedef int __bitwise snd_ctl_elem_type_t;
 #define    SNDRV_CTL_ELEM_TYPE_INTEGER64    ((__force
snd_ctl_elem_type_t) 6) /* 64-bit integer type */
 #define    SNDRV_CTL_ELEM_TYPE_LAST    SNDRV_CTL_ELEM_TYPE_INTEGER64

-typedef int __bitwise snd_ctl_elem_iface_t;
+typedef __u8 snd_ctl_elem_iface_t;
 #define    SNDRV_CTL_ELEM_IFACE_CARD    ((__force
snd_ctl_elem_iface_t) 0) /* global control */
 #define    SNDRV_CTL_ELEM_IFACE_HWDEP    ((__force
snd_ctl_elem_iface_t) 1) /* hardware dependent device */
 #define    SNDRV_CTL_ELEM_IFACE_MIXER    ((__force
snd_ctl_elem_iface_t) 2) /* virtual mixer device */

I can't take a uapi sound header file patch along with a driver change,
these need to be independant.

Thank you and Alex for reminding me this is a change of public header!

And this is a userspace api, you just changed the size of an externally
facing variable, are you _SURE_ that is ok to do?

The reasons I make this change are, 1) using int to represent 7 enumeration
values seems to be overkill; 2) using one type could avoid worries
about byte order.

(1) might be a valid suggestion, but the file you suggest
changing is part of the Linux user space API, which you
basically can't change.

I'm fairly certain the user space API is defined to have
CPU-local byte order (unless specified explicitly otherwise)
so that is not a concern.

Thank you for sharing me about the byte order of user space API which
prompts me to examine where "info->type" comes from,

	uinfo->type = (snd_ctl_elem_type_t)info->type;

Eventually it comes from the parsed topology data which is obtained via
gb_audio_gb_get_topology(gbmodule->mgmt_connection, &topology). But
since (struct gb_audio_ctl_elem_info*)->type has __u8 type, there is no
endianness concern. Then based on the principles of userspace API
shouldn't be modified and greybus is operating system independent, your
previous suggestion to use __force which means "I know what I'm doing"
is actually a good idea,

        uinfo->type = (__force snd_ctl_elem_type_t)info->type;


I examine one userspace example (libalsa-intf/alsa_mixer.c [1]). This
change won't cause compiling breakage. But I don't the experience to
imagine any other bad consequence.

As a rule, once the user space API has been established, it
stays with us forever.  You can add to it, but modifying
(or removing) an existing interface is essentially forbidden.

Another way to avoid userspace API change is to let
"struct gb_audio_ctl_elem_info" use snd_ctl_elem_iface_t type which may
be more elegant than using "__force" as suggested by Alex,

The Greybus definitions were explicitly defined to be
operating system independent.  For that reason there are
(admittedly cumbersome) definitions of certain types and
values that essentially mimic those defined by Linux
exactly  That way Linux (or another system using Greybus)
can change its internal values without changing the
Greybus API definition.  (This addresses the concern you
mention below.)

What you are suggesting is not consistent with that
principle.

					-Alex


$ git diff
diff --git a/include/linux/greybus/greybus_protocols.h
b/include/linux/greybus/greybus_protocols.h
index aeb8f9243545..7f6753ec7ef7 100644
--- a/include/linux/greybus/greybus_protocols.h
+++ b/include/linux/greybus/greybus_protocols.h
@@ -8,6 +8,7 @@
 #define __GREYBUS_PROTOCOLS_H

 #include <linux/types.h>
+#include <sound/asound.h>

 /* Fixed IDs for control/svc protocols */

@@ -1997,7 +1998,7 @@ struct gb_audio_enumerated {
 } __packed;

 struct gb_audio_ctl_elem_info { /* See snd_ctl_elem_info in Linux
source */
-       __u8            type;           /* GB_AUDIO_CTL_ELEM_TYPE_* */
+       snd_ctl_elem_type_t             type;           /*
GB_AUDIO_CTL_ELEM_TYPE_* */
        __le16          dimen[4];
        union {
                struct gb_audio_integer         integer

My only concern is if greybus authors have any special reason to not
include
sound/asound.h in the first place and re-define things like
SNDRV_CTL_ELEM_TYPE_*,

/* See SNDRV_CTL_ELEM_TYPE_* in Linux source */
#define GB_AUDIO_CTL_ELEM_TYPE_BOOLEAN        0x01
#define GB_AUDIO_CTL_ELEM_TYPE_INTEGER        0x02

/* See SNDRV_CTL_ELEM_IFACE_* in Linux source */
#define GB_AUDIO_CTL_ELEM_IFACE_CARD        0x00
...

/* SNDRV_CTL_ELEM_ACCESS_* in Linux source */
#define GB_AUDIO_ACCESS_READ            BIT(0)
...

[1] https://gitlab.com/Codeaurora/platform_hardware_qcom_audio/-/blob/aee6032826ec7c8b6edda404422fda0ef40ec2db/libalsa-intf/alsa_mixer.c

 alsa_mixer.c

thanks,

greg k-h

--
Best regards,
Coiby


--
Best regards,
Coiby
_______________________________________________
greybus-dev mailing list
greybus-dev@xxxxxxxxxxxxxxxx
https://lists.linaro.org/mailman/listinfo/greybus-dev




[Index of Archives]     [Asterisk App Development]     [PJ SIP]     [Gnu Gatekeeper]     [IETF Sipping]     [Info Cyrus]     [ALSA User]     [Fedora Linux Users]     [Linux SCTP]     [DCCP]     [Gimp]     [Yosemite News]     [Deep Creek Hot Springs]     [Yosemite Campsites]     [ISDN Cause Codes]     [Asterisk Books]

  Powered by Linux