[PATCH 0/4] topology: fix some gcc warnings

[PATCH 0/4] topology: fix some gcc warnings

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@linux.intel.com>

This series can fix the following gcc warnings:
-Wunused-function -Wsign-compare -Wunused-variable -Wtype-limits

Mengdong Lin (4):
  topology: Remove unused function write_data_block()
  topology: Change variable type to fix gcc warning
  topology: Remove unused variables
  topology: Fix comparison of unsigned expression < 0

 src/topology/builder.c | 24 ------------------------
 src/topology/channel.c |  6 ++++--
 src/topology/ctl.c     |  6 ++++--
 src/topology/pcm.c     |  9 +++------
 4 files changed, 11 insertions(+), 34 deletions(-)

-- 
2.5.0

[PATCH 1/4] topology: Remove unused function write_data_block()

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@linux.intel.com>

Fix gcc warning: 'write_data_block' defined but not used
[-Wunused-function].

Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

diff --git a/src/topology/builder.c b/src/topology/builder.c
index 154bd63..b0ba54e 100644
--- a/src/topology/builder.c
+++ b/src/topology/builder.c
@@ -82,30 +82,6 @@ static int write_block_header(snd_tplg_t *tplg, unsigned int type,
 	return bytes;
 }
 
-static int write_data_block(snd_tplg_t *tplg, int size, int tplg_type,
-	const char *obj_name, void *data)
-{
-	int ret;
-
-	/* write the header for this block */
-	ret = write_block_header(tplg, tplg_type, 0,
-		tplg->version, 0, size, 1);
-	if (ret < 0) {
-		SNDERR("error: failed to write %s block %d\n", obj_name, ret);
-		return ret;
-	}
-
-	verbose(tplg, " %s : write %d bytes\n", obj_name, size);
-
-	ret = write(tplg->out_fd, data, size);
-	if (ret < 0) {
-		SNDERR("error: failed to write %s %d\n", obj_name, ret);
-		return ret;
-	}
-
-	return 0;
-}
-
 static int write_elem_block(snd_tplg_t *tplg,
 	struct list_head *base, int size, int tplg_type, const char *obj_name)
 {
-- 
2.5.0

Re: [PATCH 1/4] topology: Remove unused function write_data_block()

[Takashi Iwai]

On Wed, 18 Nov 2015 08:22:59 +0100,
mengdong.lin@linux.intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin@linux.intel.com>
> 
> Fix gcc warning: 'write_data_block' defined but not used
> [-Wunused-function].
> 
> Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

I thought this was kept intentionally for the future use.
But it's fine to remove if you are OK, of course.

Applied now.  Thanks.


Takashi

> 
> diff --git a/src/topology/builder.c b/src/topology/builder.c
> index 154bd63..b0ba54e 100644
> --- a/src/topology/builder.c
> +++ b/src/topology/builder.c
> @@ -82,30 +82,6 @@ static int write_block_header(snd_tplg_t *tplg, unsigned int type,
>  	return bytes;
>  }
>  
> -static int write_data_block(snd_tplg_t *tplg, int size, int tplg_type,
> -	const char *obj_name, void *data)
> -{
> -	int ret;
> -
> -	/* write the header for this block */
> -	ret = write_block_header(tplg, tplg_type, 0,
> -		tplg->version, 0, size, 1);
> -	if (ret < 0) {
> -		SNDERR("error: failed to write %s block %d\n", obj_name, ret);
> -		return ret;
> -	}
> -
> -	verbose(tplg, " %s : write %d bytes\n", obj_name, size);
> -
> -	ret = write(tplg->out_fd, data, size);
> -	if (ret < 0) {
> -		SNDERR("error: failed to write %s %d\n", obj_name, ret);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static int write_elem_block(snd_tplg_t *tplg,
>  	struct list_head *base, int size, int tplg_type, const char *obj_name)
>  {
> -- 
> 2.5.0
> 

[PATCH 2/4] topology: Change variable type to fix gcc warning

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@linux.intel.com>

Fix warning: comparison between signed and unsigned integer expressions
[-Wsign-compare]

ABI objects use type _le32, which is converted to host unsigned integer.
So the iterator 'i' in a loop as below should also be unsigned.
for (i = 0; i < pcm->num_streams; i++)
                ^
Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

diff --git a/src/topology/ctl.c b/src/topology/ctl.c
index 7d8787f..6dc3b3d 100644
--- a/src/topology/ctl.c
+++ b/src/topology/ctl.c
@@ -676,7 +676,8 @@ int tplg_add_mixer(snd_tplg_t *tplg, struct snd_tplg_mixer_template *mixer,
 	struct snd_soc_tplg_private *priv = mixer->priv;
 	struct snd_soc_tplg_mixer_control *mc;
 	struct tplg_elem *elem;
-	int ret, i;
+	int ret;
+	unsigned int i;
 
 	tplg_dbg(" Control Mixer: %s\n", mixer->hdr.name);
 
@@ -743,7 +744,8 @@ int tplg_add_enum(snd_tplg_t *tplg, struct snd_tplg_enum_template *enum_ctl,
 {
 	struct snd_soc_tplg_enum_control *ec;
 	struct tplg_elem *elem;
-	int ret, i;
+	int ret;
+	unsigned int i;
 
 	tplg_dbg(" Control Enum: %s\n", enum_ctl->hdr.name);
 
diff --git a/src/topology/pcm.c b/src/topology/pcm.c
index 9b7e402..4b7c058 100644
--- a/src/topology/pcm.c
+++ b/src/topology/pcm.c
@@ -522,7 +522,7 @@ int tplg_add_pcm_object(snd_tplg_t *tplg, snd_tplg_obj_template_t *t)
 	struct snd_tplg_pcm_template *pcm_tpl = t->pcm;
 	struct snd_soc_tplg_pcm *pcm;
 	struct tplg_elem *elem;
-	int i;
+	unsigned int i;
 
 	tplg_dbg("PCM: %s, DAI %s\n", pcm_tpl->pcm_name, pcm_tpl->dai_name);
 
@@ -564,7 +564,7 @@ int tplg_add_link_object(snd_tplg_t *tplg, snd_tplg_obj_template_t *t)
 	struct snd_tplg_link_template *link = t->link;
 	struct snd_soc_tplg_link_config *lk;
 	struct tplg_elem *elem;
-	int i;
+	unsigned int i;
 
 	if (t->type != SND_TPLG_TYPE_BE && t->type != SND_TPLG_TYPE_CC)
 		return -EINVAL;
-- 
2.5.0

Re: [PATCH 2/4] topology: Change variable type to fix gcc warning

[Takashi Iwai]

On Wed, 18 Nov 2015 08:23:07 +0100,
mengdong.lin@linux.intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin@linux.intel.com>
> 
> Fix warning: comparison between signed and unsigned integer expressions
> [-Wsign-compare]
> 
> ABI objects use type _le32, which is converted to host unsigned integer.
> So the iterator 'i' in a loop as below should also be unsigned.
> for (i = 0; i < pcm->num_streams; i++)
>                 ^

Using an unsigned int for the generic loop count like i is strange.

Rather compare with the original value in the template, which is
actually int.


Takashi

> Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>
> 
> diff --git a/src/topology/ctl.c b/src/topology/ctl.c
> index 7d8787f..6dc3b3d 100644
> --- a/src/topology/ctl.c
> +++ b/src/topology/ctl.c
> @@ -676,7 +676,8 @@ int tplg_add_mixer(snd_tplg_t *tplg, struct snd_tplg_mixer_template *mixer,
>  	struct snd_soc_tplg_private *priv = mixer->priv;
>  	struct snd_soc_tplg_mixer_control *mc;
>  	struct tplg_elem *elem;
> -	int ret, i;
> +	int ret;
> +	unsigned int i;
>  
>  	tplg_dbg(" Control Mixer: %s\n", mixer->hdr.name);
>  
> @@ -743,7 +744,8 @@ int tplg_add_enum(snd_tplg_t *tplg, struct snd_tplg_enum_template *enum_ctl,
>  {
>  	struct snd_soc_tplg_enum_control *ec;
>  	struct tplg_elem *elem;
> -	int ret, i;
> +	int ret;
> +	unsigned int i;
>  
>  	tplg_dbg(" Control Enum: %s\n", enum_ctl->hdr.name);
>  
> diff --git a/src/topology/pcm.c b/src/topology/pcm.c
> index 9b7e402..4b7c058 100644
> --- a/src/topology/pcm.c
> +++ b/src/topology/pcm.c
> @@ -522,7 +522,7 @@ int tplg_add_pcm_object(snd_tplg_t *tplg, snd_tplg_obj_template_t *t)
>  	struct snd_tplg_pcm_template *pcm_tpl = t->pcm;
>  	struct snd_soc_tplg_pcm *pcm;
>  	struct tplg_elem *elem;
> -	int i;
> +	unsigned int i;
>  
>  	tplg_dbg("PCM: %s, DAI %s\n", pcm_tpl->pcm_name, pcm_tpl->dai_name);
>  
> @@ -564,7 +564,7 @@ int tplg_add_link_object(snd_tplg_t *tplg, snd_tplg_obj_template_t *t)
>  	struct snd_tplg_link_template *link = t->link;
>  	struct snd_soc_tplg_link_config *lk;
>  	struct tplg_elem *elem;
> -	int i;
> +	unsigned int i;
>  
>  	if (t->type != SND_TPLG_TYPE_BE && t->type != SND_TPLG_TYPE_CC)
>  		return -EINVAL;
> -- 
> 2.5.0
> 

Re: [PATCH 2/4] topology: Change variable type to fix gcc warning

[Lin, Mengdong]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, November 18, 2015 3:16 PM
> To: mengdong.lin@linux.intel.com

> On Wed, 18 Nov 2015 08:23:07 +0100,
> mengdong.lin@linux.intel.com wrote:
> >
> > From: Mengdong Lin <mengdong.lin@linux.intel.com>
> >
> > Fix warning: comparison between signed and unsigned integer
> > expressions [-Wsign-compare]
> >
> > ABI objects use type _le32, which is converted to host unsigned integer.
> > So the iterator 'i' in a loop as below should also be unsigned.
> > for (i = 0; i < pcm->num_streams; i++)
> >                 ^
> 
> Using an unsigned int for the generic loop count like i is strange.

Yes.

> Rather compare with the original value in the template, which is actually int.

Are you suggesting not change the code?

The "pcm->num_streams" here is __le32 defined in ABI:
struct snd_soc_tplg_pcm {
	...
	__le32 num_streams;	/* number of streams */
	...
} __attribute__((packed));

It seems gcc takes it as unsigned integer and so I get the warning.

Thanks
Mengdong

> 
> 
> Takashi
> 
> > Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>
> >
> > diff --git a/src/topology/ctl.c b/src/topology/ctl.c index
> > 7d8787f..6dc3b3d 100644
> > --- a/src/topology/ctl.c
> > +++ b/src/topology/ctl.c
> > @@ -676,7 +676,8 @@ int tplg_add_mixer(snd_tplg_t *tplg, struct
> snd_tplg_mixer_template *mixer,
> >  	struct snd_soc_tplg_private *priv = mixer->priv;
> >  	struct snd_soc_tplg_mixer_control *mc;
> >  	struct tplg_elem *elem;
> > -	int ret, i;
> > +	int ret;
> > +	unsigned int i;
> >
> >  	tplg_dbg(" Control Mixer: %s\n", mixer->hdr.name);
> >
> > @@ -743,7 +744,8 @@ int tplg_add_enum(snd_tplg_t *tplg, struct
> > snd_tplg_enum_template *enum_ctl,  {
> >  	struct snd_soc_tplg_enum_control *ec;
> >  	struct tplg_elem *elem;
> > -	int ret, i;
> > +	int ret;
> > +	unsigned int i;
> >
> >  	tplg_dbg(" Control Enum: %s\n", enum_ctl->hdr.name);
> >
> > diff --git a/src/topology/pcm.c b/src/topology/pcm.c index
> > 9b7e402..4b7c058 100644
> > --- a/src/topology/pcm.c
> > +++ b/src/topology/pcm.c
> > @@ -522,7 +522,7 @@ int tplg_add_pcm_object(snd_tplg_t *tplg,
> snd_tplg_obj_template_t *t)
> >  	struct snd_tplg_pcm_template *pcm_tpl = t->pcm;
> >  	struct snd_soc_tplg_pcm *pcm;
> >  	struct tplg_elem *elem;
> > -	int i;
> > +	unsigned int i;
> >
> >  	tplg_dbg("PCM: %s, DAI %s\n", pcm_tpl->pcm_name,
> pcm_tpl->dai_name);
> >
> > @@ -564,7 +564,7 @@ int tplg_add_link_object(snd_tplg_t *tplg,
> snd_tplg_obj_template_t *t)
> >  	struct snd_tplg_link_template *link = t->link;
> >  	struct snd_soc_tplg_link_config *lk;
> >  	struct tplg_elem *elem;
> > -	int i;
> > +	unsigned int i;
> >
> >  	if (t->type != SND_TPLG_TYPE_BE && t->type != SND_TPLG_TYPE_CC)
> >  		return -EINVAL;
> > --
> > 2.5.0
> >

Re: [PATCH 2/4] topology: Change variable type to fix gcc warning

[Takashi Iwai]

On Wed, 18 Nov 2015 08:28:09 +0100,
Lin, Mengdong wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, November 18, 2015 3:16 PM
> > To: mengdong.lin@linux.intel.com
> 
> > On Wed, 18 Nov 2015 08:23:07 +0100,
> > mengdong.lin@linux.intel.com wrote:
> > >
> > > From: Mengdong Lin <mengdong.lin@linux.intel.com>
> > >
> > > Fix warning: comparison between signed and unsigned integer
> > > expressions [-Wsign-compare]
> > >
> > > ABI objects use type _le32, which is converted to host unsigned integer.
> > > So the iterator 'i' in a loop as below should also be unsigned.
> > > for (i = 0; i < pcm->num_streams; i++)
> > >                 ^
> > 
> > Using an unsigned int for the generic loop count like i is strange.
> 
> Yes.
> 
> > Rather compare with the original value in the template, which is actually int.
> 
> Are you suggesting not change the code?

No.

> The "pcm->num_streams" here is __le32 defined in ABI:
> struct snd_soc_tplg_pcm {
> 	...
> 	__le32 num_streams;	/* number of streams */
> 	...
> } __attribute__((packed));
> 
> It seems gcc takes it as unsigned integer and so I get the warning.

The problem isn't only about the endianess.
Conceptually, it's wrong to refer directly to le32 data, as it's not
being CPU endian.  It should have always an endian conversion.  Of
course, we support only LE32, so it's no big issue, so far.

What you need to refer to is the value in the template instead.  It's
guaranteed to be CPU endianess, and it's even int.
So, the code would look like:

	for (i = 0; i < pcm_tpl->num_streams; i++)


BTW, I haven't checked whether topology API would "work" on big-endian
architecture; does it return an error properly? 


Takashi


> 
> Thanks
> Mengdong
> 
> > 
> > 
> > Takashi
> > 
> > > Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>
> > >
> > > diff --git a/src/topology/ctl.c b/src/topology/ctl.c index
> > > 7d8787f..6dc3b3d 100644
> > > --- a/src/topology/ctl.c
> > > +++ b/src/topology/ctl.c
> > > @@ -676,7 +676,8 @@ int tplg_add_mixer(snd_tplg_t *tplg, struct
> > snd_tplg_mixer_template *mixer,
> > >  	struct snd_soc_tplg_private *priv = mixer->priv;
> > >  	struct snd_soc_tplg_mixer_control *mc;
> > >  	struct tplg_elem *elem;
> > > -	int ret, i;
> > > +	int ret;
> > > +	unsigned int i;
> > >
> > >  	tplg_dbg(" Control Mixer: %s\n", mixer->hdr.name);
> > >
> > > @@ -743,7 +744,8 @@ int tplg_add_enum(snd_tplg_t *tplg, struct
> > > snd_tplg_enum_template *enum_ctl,  {
> > >  	struct snd_soc_tplg_enum_control *ec;
> > >  	struct tplg_elem *elem;
> > > -	int ret, i;
> > > +	int ret;
> > > +	unsigned int i;
> > >
> > >  	tplg_dbg(" Control Enum: %s\n", enum_ctl->hdr.name);
> > >
> > > diff --git a/src/topology/pcm.c b/src/topology/pcm.c index
> > > 9b7e402..4b7c058 100644
> > > --- a/src/topology/pcm.c
> > > +++ b/src/topology/pcm.c
> > > @@ -522,7 +522,7 @@ int tplg_add_pcm_object(snd_tplg_t *tplg,
> > snd_tplg_obj_template_t *t)
> > >  	struct snd_tplg_pcm_template *pcm_tpl = t->pcm;
> > >  	struct snd_soc_tplg_pcm *pcm;
> > >  	struct tplg_elem *elem;
> > > -	int i;
> > > +	unsigned int i;
> > >
> > >  	tplg_dbg("PCM: %s, DAI %s\n", pcm_tpl->pcm_name,
> > pcm_tpl->dai_name);
> > >
> > > @@ -564,7 +564,7 @@ int tplg_add_link_object(snd_tplg_t *tplg,
> > snd_tplg_obj_template_t *t)
> > >  	struct snd_tplg_link_template *link = t->link;
> > >  	struct snd_soc_tplg_link_config *lk;
> > >  	struct tplg_elem *elem;
> > > -	int i;
> > > +	unsigned int i;
> > >
> > >  	if (t->type != SND_TPLG_TYPE_BE && t->type != SND_TPLG_TYPE_CC)
> > >  		return -EINVAL;
> > > --
> > > 2.5.0
> > >
> 

Re: [PATCH 2/4] topology: Change variable type to fix gcc warning

[Lin, Mengdong]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, November 18, 2015 4:19 PM
> 
> On Wed, 18 Nov 2015 08:28:09 +0100,
> Lin, Mengdong wrote:
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Wednesday, November 18, 2015 3:16 PM
> > > To: mengdong.lin@linux.intel.com
> >
> > > On Wed, 18 Nov 2015 08:23:07 +0100,
> > > mengdong.lin@linux.intel.com wrote:
> > > >
> > > > From: Mengdong Lin <mengdong.lin@linux.intel.com>
> > > >
> > > > Fix warning: comparison between signed and unsigned integer
> > > > expressions [-Wsign-compare]
> > > >
> > > > ABI objects use type _le32, which is converted to host unsigned integer.
> > > > So the iterator 'i' in a loop as below should also be unsigned.
> > > > for (i = 0; i < pcm->num_streams; i++)
> > > >                 ^
> > >
> > > Using an unsigned int for the generic loop count like i is strange.
> >
> > Yes.
> >
> > > Rather compare with the original value in the template, which is actually int.
> >
> > Are you suggesting not change the code?
> 
> No.
> 
> > The "pcm->num_streams" here is __le32 defined in ABI:
> > struct snd_soc_tplg_pcm {
> > 	...
> > 	__le32 num_streams;	/* number of streams */
> > 	...
> > } __attribute__((packed));
> >
> > It seems gcc takes it as unsigned integer and so I get the warning.
> 
> The problem isn't only about the endianess.
> Conceptually, it's wrong to refer directly to le32 data, as it's not being CPU
> endian.  It should have always an endian conversion.  Of course, we support
> only LE32, so it's no big issue, so far.
> 
> What you need to refer to is the value in the template instead.  It's guaranteed
> to be CPU endianess, and it's even int.
> So, the code would look like:
> 
> 	for (i = 0; i < pcm_tpl->num_streams; i++)
> 
Okay, I'll revise the patch. Thanks for your suggestion.

> BTW, I haven't checked whether topology API would "work" on big-endian
> architecture; does it return an error properly?

Sorry, no. I'll check if an API can tell us the host endianness and let topology return an error for big-endian machines.

I had thought to use this API when generating the ABI objects from host type to _le32:
uint32_t htole32(uint32_t host_32bits);

But since I have no big-endian machines to test this, maybe it's better to return an error for them.

Thanks
Mengdong
> 
> Takashi
> 
> 
> >
> > Thanks
> > Mengdong
> >
> > >
> > >
> > > Takashi
> > >
> > > > Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>
> > > >
> > > > diff --git a/src/topology/ctl.c b/src/topology/ctl.c index
> > > > 7d8787f..6dc3b3d 100644
> > > > --- a/src/topology/ctl.c
> > > > +++ b/src/topology/ctl.c
> > > > @@ -676,7 +676,8 @@ int tplg_add_mixer(snd_tplg_t *tplg, struct
> > > snd_tplg_mixer_template *mixer,
> > > >  	struct snd_soc_tplg_private *priv = mixer->priv;
> > > >  	struct snd_soc_tplg_mixer_control *mc;
> > > >  	struct tplg_elem *elem;
> > > > -	int ret, i;
> > > > +	int ret;
> > > > +	unsigned int i;
> > > >
> > > >  	tplg_dbg(" Control Mixer: %s\n", mixer->hdr.name);
> > > >
> > > > @@ -743,7 +744,8 @@ int tplg_add_enum(snd_tplg_t *tplg, struct
> > > > snd_tplg_enum_template *enum_ctl,  {
> > > >  	struct snd_soc_tplg_enum_control *ec;
> > > >  	struct tplg_elem *elem;
> > > > -	int ret, i;
> > > > +	int ret;
> > > > +	unsigned int i;
> > > >
> > > >  	tplg_dbg(" Control Enum: %s\n", enum_ctl->hdr.name);
> > > >
> > > > diff --git a/src/topology/pcm.c b/src/topology/pcm.c index
> > > > 9b7e402..4b7c058 100644
> > > > --- a/src/topology/pcm.c
> > > > +++ b/src/topology/pcm.c
> > > > @@ -522,7 +522,7 @@ int tplg_add_pcm_object(snd_tplg_t *tplg,
> > > snd_tplg_obj_template_t *t)
> > > >  	struct snd_tplg_pcm_template *pcm_tpl = t->pcm;
> > > >  	struct snd_soc_tplg_pcm *pcm;
> > > >  	struct tplg_elem *elem;
> > > > -	int i;
> > > > +	unsigned int i;
> > > >
> > > >  	tplg_dbg("PCM: %s, DAI %s\n", pcm_tpl->pcm_name,
> > > pcm_tpl->dai_name);
> > > >
> > > > @@ -564,7 +564,7 @@ int tplg_add_link_object(snd_tplg_t *tplg,
> > > snd_tplg_obj_template_t *t)
> > > >  	struct snd_tplg_link_template *link = t->link;
> > > >  	struct snd_soc_tplg_link_config *lk;
> > > >  	struct tplg_elem *elem;
> > > > -	int i;
> > > > +	unsigned int i;
> > > >
> > > >  	if (t->type != SND_TPLG_TYPE_BE && t->type != SND_TPLG_TYPE_CC)
> > > >  		return -EINVAL;
> > > > --
> > > > 2.5.0
> > > >
> >

Re: [PATCH 2/4] topology: Change variable type to fix gcc warning

[Takashi Iwai]

On Wed, 18 Nov 2015 16:14:43 +0100,
Lin, Mengdong wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, November 18, 2015 4:19 PM
> > 
> > On Wed, 18 Nov 2015 08:28:09 +0100,
> > Lin, Mengdong wrote:
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Wednesday, November 18, 2015 3:16 PM
> > > > To: mengdong.lin@linux.intel.com
> > >
> > > > On Wed, 18 Nov 2015 08:23:07 +0100,
> > > > mengdong.lin@linux.intel.com wrote:
> > > > >
> > > > > From: Mengdong Lin <mengdong.lin@linux.intel.com>
> > > > >
> > > > > Fix warning: comparison between signed and unsigned integer
> > > > > expressions [-Wsign-compare]
> > > > >
> > > > > ABI objects use type _le32, which is converted to host unsigned integer.
> > > > > So the iterator 'i' in a loop as below should also be unsigned.
> > > > > for (i = 0; i < pcm->num_streams; i++)
> > > > >                 ^
> > > >
> > > > Using an unsigned int for the generic loop count like i is strange.
> > >
> > > Yes.
> > >
> > > > Rather compare with the original value in the template, which is actually int.
> > >
> > > Are you suggesting not change the code?
> > 
> > No.
> > 
> > > The "pcm->num_streams" here is __le32 defined in ABI:
> > > struct snd_soc_tplg_pcm {
> > > 	...
> > > 	__le32 num_streams;	/* number of streams */
> > > 	...
> > > } __attribute__((packed));
> > >
> > > It seems gcc takes it as unsigned integer and so I get the warning.
> > 
> > The problem isn't only about the endianess.
> > Conceptually, it's wrong to refer directly to le32 data, as it's not being CPU
> > endian.  It should have always an endian conversion.  Of course, we support
> > only LE32, so it's no big issue, so far.
> > 
> > What you need to refer to is the value in the template instead.  It's guaranteed
> > to be CPU endianess, and it's even int.
> > So, the code would look like:
> > 
> > 	for (i = 0; i < pcm_tpl->num_streams; i++)
> > 
> Okay, I'll revise the patch. Thanks for your suggestion.
> 
> > BTW, I haven't checked whether topology API would "work" on big-endian
> > architecture; does it return an error properly?
> 
> Sorry, no. I'll check if an API can tell us the host endianness and let topology return an error for big-endian machines.
> 
> I had thought to use this API when generating the ABI objects from host type to _le32:
> uint32_t htole32(uint32_t host_32bits);

Yes, that should work.  But then, as I pointed out, many current codes
directly refer to the converted data, and they are broken.

> But since I have no big-endian machines to test this, maybe it's better to return an error for them.

It's the easiest option, of course :)


Takashi

[PATCH 3/4] topology: Remove unused variables

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@linux.intel.com>

Fix gcc warning when -Wunused-variable is set.

Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

diff --git a/src/topology/pcm.c b/src/topology/pcm.c
index 4b7c058..d90a9ea 100644
--- a/src/topology/pcm.c
+++ b/src/topology/pcm.c
@@ -58,7 +58,6 @@ static int tplg_build_pcm_caps(snd_tplg_t *tplg, struct tplg_elem *elem)
 	struct tplg_elem *ref_elem = NULL;
 	struct snd_soc_tplg_pcm *pcm;
 	struct snd_soc_tplg_stream_caps *caps;
-	struct snd_soc_tplg_stream *stream;
 	unsigned int i;
 
 	pcm = elem->pcm;
@@ -273,7 +272,7 @@ int tplg_parse_stream_caps(snd_tplg_t *tplg, snd_config_t *cfg,
 	struct tplg_elem *elem = private;
 	struct snd_soc_tplg_pcm *pcm;
 	const char *id, *value;
-	int err, stream;
+	int stream;
 
 	pcm = elem->pcm;
 
@@ -384,7 +383,6 @@ int tplg_parse_be(snd_tplg_t *tplg,
 	snd_config_iterator_t i, next;
 	snd_config_t *n;
 	const char *id, *val = NULL;
-	int err;
 
 	elem = tplg_elem_new_common(tplg, cfg, NULL, SND_TPLG_TYPE_BE);
 	if (!elem)
@@ -438,7 +436,6 @@ int tplg_parse_cc(snd_tplg_t *tplg,
 	snd_config_iterator_t i, next;
 	snd_config_t *n;
 	const char *id, *val = NULL;
-	int err;
 
 	elem = tplg_elem_new_common(tplg, cfg, NULL, SND_TPLG_TYPE_CC);
 	if (!elem)
-- 
2.5.0

Re: [PATCH 3/4] topology: Remove unused variables

[Takashi Iwai]

On Wed, 18 Nov 2015 08:23:19 +0100,
mengdong.lin@linux.intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin@linux.intel.com>
> 
> Fix gcc warning when -Wunused-variable is set.
> 
> Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

Thanks, applied.


Takashi

> 
> diff --git a/src/topology/pcm.c b/src/topology/pcm.c
> index 4b7c058..d90a9ea 100644
> --- a/src/topology/pcm.c
> +++ b/src/topology/pcm.c
> @@ -58,7 +58,6 @@ static int tplg_build_pcm_caps(snd_tplg_t *tplg, struct tplg_elem *elem)
>  	struct tplg_elem *ref_elem = NULL;
>  	struct snd_soc_tplg_pcm *pcm;
>  	struct snd_soc_tplg_stream_caps *caps;
> -	struct snd_soc_tplg_stream *stream;
>  	unsigned int i;
>  
>  	pcm = elem->pcm;
> @@ -273,7 +272,7 @@ int tplg_parse_stream_caps(snd_tplg_t *tplg, snd_config_t *cfg,
>  	struct tplg_elem *elem = private;
>  	struct snd_soc_tplg_pcm *pcm;
>  	const char *id, *value;
> -	int err, stream;
> +	int stream;
>  
>  	pcm = elem->pcm;
>  
> @@ -384,7 +383,6 @@ int tplg_parse_be(snd_tplg_t *tplg,
>  	snd_config_iterator_t i, next;
>  	snd_config_t *n;
>  	const char *id, *val = NULL;
> -	int err;
>  
>  	elem = tplg_elem_new_common(tplg, cfg, NULL, SND_TPLG_TYPE_BE);
>  	if (!elem)
> @@ -438,7 +436,6 @@ int tplg_parse_cc(snd_tplg_t *tplg,
>  	snd_config_iterator_t i, next;
>  	snd_config_t *n;
>  	const char *id, *val = NULL;
> -	int err;
>  
>  	elem = tplg_elem_new_common(tplg, cfg, NULL, SND_TPLG_TYPE_CC);
>  	if (!elem)
> -- 
> 2.5.0
> 

[PATCH 4/4] topology: Fix comparison of unsigned expression < 0

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@linux.intel.com>

Fix gcc warning: comparison of unsigned expression < 0 is always false
[-Wtype-limits]

The ABI object channel->id is _le32 and is converted to host unsigned
integer. It cannot be < 0.

Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

diff --git a/src/topology/channel.c b/src/topology/channel.c
index 9bc5d5a..c2f1fea 100644
--- a/src/topology/channel.c
+++ b/src/topology/channel.c
@@ -80,6 +80,7 @@ int tplg_parse_channel(snd_tplg_t *tplg,
 	snd_config_t *n;
 	struct snd_soc_tplg_channel *channel = private;
 	const char *id, *value;
+	int channel_id;
 
 	if (tplg->channel_idx >= SND_SOC_TPLG_MAX_CHAN)
 		return -EINVAL;
@@ -88,12 +89,13 @@ int tplg_parse_channel(snd_tplg_t *tplg,
 	snd_config_get_id(cfg, &id);
 	tplg_dbg("\tChannel %s at index %d\n", id, tplg->channel_idx);
 
-	channel->id = lookup_channel(id);
-	if (channel->id < 0) {
+	channel_id = lookup_channel(id);
+	if (channel_id < 0) {
 		SNDERR("error: invalid channel %s\n", id);
 		return -EINVAL;
 	}
 
+	channel->id = channel_id;
 	channel->size = sizeof(*channel);
 	tplg_dbg("\tChan %s = %d\n", id, channel->id);
 
-- 
2.5.0

Re: [PATCH 4/4] topology: Fix comparison of unsigned expression < 0

[Takashi Iwai]

On Wed, 18 Nov 2015 08:23:59 +0100,
mengdong.lin@linux.intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin@linux.intel.com>
> 
> Fix gcc warning: comparison of unsigned expression < 0 is always false
> [-Wtype-limits]
> 
> The ABI object channel->id is _le32 and is converted to host unsigned
> integer. It cannot be < 0.
> 
> Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

Applied, thanks.


Takashi

> 
> diff --git a/src/topology/channel.c b/src/topology/channel.c
> index 9bc5d5a..c2f1fea 100644
> --- a/src/topology/channel.c
> +++ b/src/topology/channel.c
> @@ -80,6 +80,7 @@ int tplg_parse_channel(snd_tplg_t *tplg,
>  	snd_config_t *n;
>  	struct snd_soc_tplg_channel *channel = private;
>  	const char *id, *value;
> +	int channel_id;
>  
>  	if (tplg->channel_idx >= SND_SOC_TPLG_MAX_CHAN)
>  		return -EINVAL;
> @@ -88,12 +89,13 @@ int tplg_parse_channel(snd_tplg_t *tplg,
>  	snd_config_get_id(cfg, &id);
>  	tplg_dbg("\tChannel %s at index %d\n", id, tplg->channel_idx);
>  
> -	channel->id = lookup_channel(id);
> -	if (channel->id < 0) {
> +	channel_id = lookup_channel(id);
> +	if (channel_id < 0) {
>  		SNDERR("error: invalid channel %s\n", id);
>  		return -EINVAL;
>  	}
>  
> +	channel->id = channel_id;
>  	channel->size = sizeof(*channel);
>  	tplg_dbg("\tChan %s = %d\n", id, channel->id);
>  
> -- 
> 2.5.0
>