[PATCH 1/2] soc: qcom: cmd-db: Fix incorrect errno checks

[PATCH 1/2] soc: qcom: cmd-db: Fix incorrect errno checks

[Andy Gross]

This patch fixes a couple of checks for errno that should have been
checking for ret < 0.

Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
 drivers/soc/qcom/cmd-db.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
index a6f6462..deb2b90 100644
--- a/drivers/soc/qcom/cmd-db.c
+++ b/drivers/soc/qcom/cmd-db.c
@@ -121,7 +121,7 @@ rsc_offset(struct rsc_hdr *hdr, struct entry_header *ent)
 /**
  * cmd_db_ready - Indicates if command DB is available
  *
- * Return: 0 on success, errno otherwise
+ * Return: 0 on success, -errno otherwise
  */
 int cmd_db_ready(void)
 {
@@ -143,7 +143,7 @@ static int cmd_db_get_header(const char *id, struct entry_header *eh,
 	u8 query[8];
 
 	ret = cmd_db_ready();
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	if (!eh || !rh)
@@ -202,7 +202,7 @@ EXPORT_SYMBOL(cmd_db_read_addr);
  *  @data: Data buffer to copy returned aux data to. Returns size on NULL
  *  @len: Caller provides size of data buffer passed in.
  *
- *  Return: size of data on success, errno otherwise
+ *  Return: size of data on success, -errno otherwise
  */
 int cmd_db_read_aux_data(const char *id, u8 *data, size_t len)
 {
@@ -215,7 +215,7 @@ int cmd_db_read_aux_data(const char *id, u8 *data, size_t len)
 		return -EINVAL;
 
 	ret = cmd_db_get_header(id, &ent, &rsc_hdr);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	ent_len = le16_to_cpu(ent.len);
-- 
2.7.4

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

[Andy Gross]

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(-)

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;
 
-- 
2.7.4

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

[Andy Gross]

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

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

[Arnd Bergmann]

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

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

[Andy Gross]

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

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

[Arnd Bergmann]

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

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

[Bjorn Andersson]

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

[PATCH 1/2] soc: qcom: cmd-db: Fix incorrect errno checks

[Bjorn Andersson]

On Mon 25 Jun 10:05 PDT 2018, Andy Gross wrote:

> This patch fixes a couple of checks for errno that should have been
> checking for ret < 0.
> 
> Signed-off-by: Andy Gross <andy.gross@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/soc/qcom/cmd-db.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
> index a6f6462..deb2b90 100644
> --- a/drivers/soc/qcom/cmd-db.c
> +++ b/drivers/soc/qcom/cmd-db.c
> @@ -121,7 +121,7 @@ rsc_offset(struct rsc_hdr *hdr, struct entry_header *ent)
>  /**
>   * cmd_db_ready - Indicates if command DB is available
>   *
> - * Return: 0 on success, errno otherwise
> + * Return: 0 on success, -errno otherwise
>   */
>  int cmd_db_ready(void)
>  {
> @@ -143,7 +143,7 @@ static int cmd_db_get_header(const char *id, struct entry_header *eh,
>  	u8 query[8];
>  
>  	ret = cmd_db_ready();
> -	if (ret)
> +	if (ret < 0)
>  		return ret;
>  
>  	if (!eh || !rh)
> @@ -202,7 +202,7 @@ EXPORT_SYMBOL(cmd_db_read_addr);
>   *  @data: Data buffer to copy returned aux data to. Returns size on NULL
>   *  @len: Caller provides size of data buffer passed in.
>   *
> - *  Return: size of data on success, errno otherwise
> + *  Return: size of data on success, -errno otherwise
>   */
>  int cmd_db_read_aux_data(const char *id, u8 *data, size_t len)
>  {
> @@ -215,7 +215,7 @@ int cmd_db_read_aux_data(const char *id, u8 *data, size_t len)
>  		return -EINVAL;
>  
>  	ret = cmd_db_get_header(id, &ent, &rsc_hdr);
> -	if (ret)
> +	if (ret < 0)
>  		return ret;
>  
>  	ent_len = le16_to_cpu(ent.len);
> -- 
> 2.7.4
>