[2/2] soc: qcom: cmd-db: Fix compiler warnings

Message ID 1529946347-12740-2-git-send-email-andy.gross@linaro.org
State New
Headers show
Series
  • [1/2] soc: qcom: cmd-db: Fix incorrect errno checks
Related show

Commit Message

Andy Gross June 25, 2018, 5:05 p.m.
This patch adds initialization of the entry_header structs to avoid
compiler warnings like the following:

  CC      drivers/soc/qcom/cmd-db.o
drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_read_addr’:
drivers/soc/qcom/cmd-db.c:194:21: warning: ‘ent.addr’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
  return ret < 0 ? 0 : le32_to_cpu(ent.addr);

drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_read_aux_data_len’:
drivers/soc/qcom/cmd-db.c:247:38: warning: ‘ent.len’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
  return ret < 0 ? 0 : le16_to_cpu(ent.len);
                                      ^

drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_read_slave_id’:
drivers/soc/qcom/cmd-db.c:269:7: warning: ‘ent.addr’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
  addr = le32_to_cpu(ent.addr);

drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_read_aux_data’:
drivers/soc/qcom/cmd-db.c:221:10: warning: ‘ent.len’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
  ent_len = le16_to_cpu(ent.len);

drivers/soc/qcom/cmd-db.c:226:15: warning: ‘*((void *)&rsc_hdr+4)’ may
be used uninitialized in this function [-Wmaybe-uninitialized]
  memcpy(data, rsc_offset(&rsc_hdr, &ent), len);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/soc/qcom/cmd-db.c:226:15: warning: ‘*((void *)&ent+22)’ may be
used uninitialized in this function [-Wmaybe-uninitialized]

Signed-off-by: Andy Gross <andy.gross@linaro.org>

---
 drivers/soc/qcom/cmd-db.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Andy Gross June 25, 2018, 7:44 p.m. | #1
On Mon, Jun 25, 2018 at 12:05:47PM -0500, Andy Gross wrote:
> This patch adds initialization of the entry_header structs to avoid

> compiler warnings like the following:

> 

>   CC      drivers/soc/qcom/cmd-db.o

> drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_read_addr’:

> drivers/soc/qcom/cmd-db.c:194:21: warning: ‘ent.addr’ may be used

> uninitialized in this function [-Wmaybe-uninitialized]

>   return ret < 0 ? 0 : le32_to_cpu(ent.addr);

> 

> drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_read_aux_data_len’:

> drivers/soc/qcom/cmd-db.c:247:38: warning: ‘ent.len’ may be used

> uninitialized in this function [-Wmaybe-uninitialized]

>   return ret < 0 ? 0 : le16_to_cpu(ent.len);

>                                       ^

> 

> drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_read_slave_id’:

> drivers/soc/qcom/cmd-db.c:269:7: warning: ‘ent.addr’ may be used

> uninitialized in this function [-Wmaybe-uninitialized]

>   addr = le32_to_cpu(ent.addr);

> 

> drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_read_aux_data’:

> drivers/soc/qcom/cmd-db.c:221:10: warning: ‘ent.len’ may be used

> uninitialized in this function [-Wmaybe-uninitialized]

>   ent_len = le16_to_cpu(ent.len);

> 

> drivers/soc/qcom/cmd-db.c:226:15: warning: ‘*((void *)&rsc_hdr+4)’ may

> be used uninitialized in this function [-Wmaybe-uninitialized]

>   memcpy(data, rsc_offset(&rsc_hdr, &ent), len);

>                ^~~~~~~~~~~~~~~~~~~~~~~~~~

> drivers/soc/qcom/cmd-db.c:226:15: warning: ‘*((void *)&ent+22)’ may be

> used uninitialized in this function [-Wmaybe-uninitialized]

> 

> Signed-off-by: Andy Gross <andy.gross@linaro.org>


I flubbed this one due to missing an already existing patch that fixes
the warnings.  I'm going to package that one up with the errno check
patch and send a fixes pull request.

precursor patch:
https://patchwork.kernel.org/patch/10384143/


Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann June 25, 2018, 8:30 p.m. | #2
On Mon, Jun 25, 2018 at 7:05 PM, Andy Gross <andy.gross@linaro.org> wrote:
> This patch adds initialization of the entry_header structs to avoid

> compiler warnings like the following:

>

>   CC      drivers/soc/qcom/cmd-db.o

> drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_read_addr’:

> drivers/soc/qcom/cmd-db.c:194:21: warning: ‘ent.addr’ may be used

> uninitialized in this function [-Wmaybe-uninitialized]

>   return ret < 0 ? 0 : le32_to_cpu(ent.addr);

>

> drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_read_aux_data_len’:

> drivers/soc/qcom/cmd-db.c:247:38: warning: ‘ent.len’ may be used

> uninitialized in this function [-Wmaybe-uninitialized]

>   return ret < 0 ? 0 : le16_to_cpu(ent.len);


Anything wrong with my fix that I posted a while ago?
See https://lkml.org/lkml/2018/5/25/569

I generally don't like patches that add incorrect initializations to
work around warnings like this.

       Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Gross June 25, 2018, 8:54 p.m. | #3
On Mon, Jun 25, 2018 at 10:30:17PM +0200, Arnd Bergmann wrote:
> On Mon, Jun 25, 2018 at 7:05 PM, Andy Gross <andy.gross@linaro.org> wrote:

> > This patch adds initialization of the entry_header structs to avoid

> > compiler warnings like the following:

> >

> >   CC      drivers/soc/qcom/cmd-db.o

> > drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_read_addr’:

> > drivers/soc/qcom/cmd-db.c:194:21: warning: ‘ent.addr’ may be used

> > uninitialized in this function [-Wmaybe-uninitialized]

> >   return ret < 0 ? 0 : le32_to_cpu(ent.addr);

> >

> > drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_read_aux_data_len’:

> > drivers/soc/qcom/cmd-db.c:247:38: warning: ‘ent.len’ may be used

> > uninitialized in this function [-Wmaybe-uninitialized]

> >   return ret < 0 ? 0 : le16_to_cpu(ent.len);

> 

> Anything wrong with my fix that I posted a while ago?

> See https://lkml.org/lkml/2018/5/25/569


Not entirely, see the comment below.  I missed this one and the other fix
somehow.

> 

> I generally don't like patches that add incorrect initializations to

> work around warnings like this.


Correct me if I am wrong, but this just masks the warnings on toolchain/
platforms that exhibit these warnings.  I'm not entirely fond of doing
blanket init either to make the warnings go away either...


Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann June 25, 2018, 9:08 p.m. | #4
On Mon, Jun 25, 2018 at 10:54 PM, Andy Gross <andy.gross@linaro.org> wrote:
> On Mon, Jun 25, 2018 at 10:30:17PM +0200, Arnd Bergmann wrote:

>> On Mon, Jun 25, 2018 at 7:05 PM, Andy Gross <andy.gross@linaro.org> wrote:

>>

>> I generally don't like patches that add incorrect initializations to

>> work around warnings like this.

>

> Correct me if I am wrong, but this just masks the warnings on toolchain/

> platforms that exhibit these warnings.  I'm not entirely fond of doing

> blanket init either to make the warnings go away either...


My patch disallows the case where the compiler can see that the variable
is actually used without an initialization, and that code wouldn't
work properly.

The problem is that in this loop:

        for (i = 0; i < MAX_SLV_ID; i++) {
                rsc_hdr = &cmd_db_header->header[i];
                if (!rsc_hdr->slv_id)
                        break;

                ent = rsc_to_entry_header(rsc_hdr);
                for (j = 0; j < le16_to_cpu(rsc_hdr->cnt); j++, ent++) {
                        if (memcmp(ent->id, query, sizeof(ent->id)) == 0)
                                break;
                }

                if (j < le16_to_cpu(rsc_hdr->cnt)) {
                        memcpy(eh, ent, sizeof(*ent));
                        memcpy(rh, rsc_hdr, sizeof(*rh));
                        return 0;
                }
        }

I think gcc sees that cmd_db_header is initialized to NULL and never
written to after of_reserved_mem_lookup() returns NULL during
cmd_db_dev_probe().

This means we are that cmd_db_get_header never intializes the
resulting structure. It also never returns zero, but gcc doesn't
see that here.

A slightly more drastic workaround might be to initialize *eh
at the start of cmd_db_get_header().

     Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Andersson June 25, 2018, 11:26 p.m. | #5
On Mon 25 Jun 14:08 PDT 2018, Arnd Bergmann wrote:

> On Mon, Jun 25, 2018 at 10:54 PM, Andy Gross <andy.gross@linaro.org> wrote:

> > On Mon, Jun 25, 2018 at 10:30:17PM +0200, Arnd Bergmann wrote:

> >> On Mon, Jun 25, 2018 at 7:05 PM, Andy Gross <andy.gross@linaro.org> wrote:

> >>

> >> I generally don't like patches that add incorrect initializations to

> >> work around warnings like this.

> >

> > Correct me if I am wrong, but this just masks the warnings on toolchain/

> > platforms that exhibit these warnings.  I'm not entirely fond of doing

> > blanket init either to make the warnings go away either...

> 

> My patch disallows the case where the compiler can see that the variable

> is actually used without an initialization, and that code wouldn't

> work properly.

> 

> The problem is that in this loop:

> 

>         for (i = 0; i < MAX_SLV_ID; i++) {

>                 rsc_hdr = &cmd_db_header->header[i];

>                 if (!rsc_hdr->slv_id)

>                         break;

> 

>                 ent = rsc_to_entry_header(rsc_hdr);

>                 for (j = 0; j < le16_to_cpu(rsc_hdr->cnt); j++, ent++) {

>                         if (memcmp(ent->id, query, sizeof(ent->id)) == 0)

>                                 break;

>                 }

> 

>                 if (j < le16_to_cpu(rsc_hdr->cnt)) {

>                         memcpy(eh, ent, sizeof(*ent));

>                         memcpy(rh, rsc_hdr, sizeof(*rh));

>                         return 0;

>                 }

>         }

> 

> I think gcc sees that cmd_db_header is initialized to NULL and never

> written to after of_reserved_mem_lookup() returns NULL during

> cmd_db_dev_probe().

> 

> This means we are that cmd_db_get_header never intializes the

> resulting structure. It also never returns zero, but gcc doesn't

> see that here.

> 


The optimization that I can see would lead to a difference here is if
the conditional in cmd_db_read() is just removed as the compiler see it
always being true, but then doesn't propagate this to the error handling
in cmd_db_get_header(); but that wouldn't make much sense...

Otherwise I think cmd_db_header would be equally uninitialized in the
case where CONFIG_OF_RESERVED_MEM is set, but we haven't seen any issues
related to this on arm64.


Unless there's something I'm missing I would prefer that we just take
Anders' proposed fix, it does mask the problem in a more direct way.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
index deb2b90..1007e79 100644
--- a/drivers/soc/qcom/cmd-db.c
+++ b/drivers/soc/qcom/cmd-db.c
@@ -186,7 +186,7 @@  static int cmd_db_get_header(const char *id, struct entry_header *eh,
 u32 cmd_db_read_addr(const char *id)
 {
 	int ret;
-	struct entry_header ent;
+	struct entry_header ent = {};
 	struct rsc_hdr rsc_hdr;
 
 	ret = cmd_db_get_header(id, &ent, &rsc_hdr);
@@ -207,7 +207,7 @@  EXPORT_SYMBOL(cmd_db_read_addr);
 int cmd_db_read_aux_data(const char *id, u8 *data, size_t len)
 {
 	int ret;
-	struct entry_header ent;
+	struct entry_header ent = {};
 	struct rsc_hdr rsc_hdr;
 	u16 ent_len;
 
@@ -239,7 +239,7 @@  EXPORT_SYMBOL(cmd_db_read_aux_data);
 size_t cmd_db_read_aux_data_len(const char *id)
 {
 	int ret;
-	struct entry_header ent;
+	struct entry_header ent = {};
 	struct rsc_hdr rsc_hdr;
 
 	ret = cmd_db_get_header(id, &ent, &rsc_hdr);
@@ -258,7 +258,7 @@  EXPORT_SYMBOL(cmd_db_read_aux_data_len);
 enum cmd_db_hw_type cmd_db_read_slave_id(const char *id)
 {
 	int ret;
-	struct entry_header ent;
+	struct entry_header ent = {};
 	struct rsc_hdr rsc_hdr;
 	u32 addr;