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