re: qla2xxx: ISP27xx add tests for incomplete template.

re: qla2xxx: ISP27xx add tests for incomplete template.

[Dan Carpenter]

Hello Joe Carnuccio,

The patch 299f5e27ac5f: "qla2xxx: ISP27xx add tests for incomplete
template." from Sep 25, 2014, leads to the following static checker
warning:

	drivers/scsi/qla2xxx/qla_tmpl.c:779 qla27xx_walk_template()
	warn: should this be 'count == -1'

drivers/scsi/qla2xxx/qla_tmpl.c
   764  static void
   765  qla27xx_walk_template(struct scsi_qla_host *vha,
   766          struct qla27xx_fwdt_template *tmp, void *buf, ulong *len)
   767  {
   768          struct qla27xx_fwdt_entry *ent = (void *)tmp + tmp->entry_offset;
   769          ulong count = tmp->entry_count;
   770  
   771          ql_dbg(ql_dbg_misc, vha, 0xd01a,
   772              "%s: entry count %lx\n", __func__, count);
   773          while (count--) {
   774                  if (qla27xx_find_entry(ent->hdr.entry_type)(vha, ent, buf, len))
   775                          break;
   776                  ent = qla27xx_next_entry(ent);
   777          }
   778  
   779          if (count)
   780                  ql_dbg(ql_dbg_misc, vha, 0xd018,
   781                      "%s: residual count (%lx)\n", __func__, count);

The static checker assumes that we are testing to see if the loop ended
with a break or not.  These are normally bugs because if we did not
break the count is set to (ulong)-1.

This code is sort of weird.  It's just debugging so I guess it doesn't
matter.  Are we expecting -1?

   782  
   783          if (ent->hdr.entry_type != ENTRY_TYPE_TMP_END)
   784                  ql_dbg(ql_dbg_misc, vha, 0xd019,
   785                      "%s: missing end (%lx)\n", __func__, count);
   786  
   787          ql_dbg(ql_dbg_misc, vha, 0xd01b,
   788              "%s: len=%lx\n", __func__, *len);
   789  
   790          if (buf) {
   791                  ql_log(ql_log_warn, vha, 0xd015,
   792                      "Firmware dump saved to temp buffer (%ld/%p)\n",
   793                      vha->host_no, vha->hw->fw_dump);
   794                  qla2x00_post_uevent_work(vha, QLA_UEVENT_CODE_FW_DUMP);
   795          }
   796  }

regards,
dan carpenter

RE: qla2xxx: ISP27xx add tests for incomplete template.

[Joe Carnuccio]

[-- Attachment #1: Type: text/plain, Size: 3261 bytes --]

Hi Dan,



Thanks for reading the code...



That loop would normally exit via the break at line 775;

if there were exactly count entries, that break avoids count being decremented to -1, so count remains zero.



if the loop is ever terminated by the while, then either/or:

- if there were more than count entries, then count is a relatively small positive number;

- if the END entry was missing, then count wraps to -1 (~0), and line 780/781 prints all F's.



i.e. if count is not zero at line 779, then the template was wrong (too many entries, or no END entry).



( btw: yes, the redundant test for missing END entry at line 783 was added later than the while loop )



Regards

Joe





-----Original Message-----
From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
Sent: Tuesday, December 15, 2015 12:05 PM
To: Joe Carnuccio <joe.carnuccio@qlogic.com>
Cc: linux-scsi <linux-scsi@vger.kernel.org>
Subject: re: qla2xxx: ISP27xx add tests for incomplete template.



Hello Joe Carnuccio,



The patch 299f5e27ac5f: "qla2xxx: ISP27xx add tests for incomplete template." from Sep 25, 2014, leads to the following static checker

warning:



               drivers/scsi/qla2xxx/qla_tmpl.c:779 qla27xx_walk_template()

               warn: should this be 'count == -1'



drivers/scsi/qla2xxx/qla_tmpl.c

   764  static void

   765  qla27xx_walk_template(struct scsi_qla_host *vha,

   766          struct qla27xx_fwdt_template *tmp, void *buf, ulong *len)

   767  {

   768          struct qla27xx_fwdt_entry *ent = (void *)tmp + tmp->entry_offset;

   769          ulong count = tmp->entry_count;

   770

   771          ql_dbg(ql_dbg_misc, vha, 0xd01a,

   772              "%s: entry count %lx\n", __func__, count);

   773          while (count--) {

   774                  if (qla27xx_find_entry(ent->hdr.entry_type)(vha, ent, buf, len))

   775                          break;

   776                  ent = qla27xx_next_entry(ent);

   777          }

   778

   779          if (count)

   780                  ql_dbg(ql_dbg_misc, vha, 0xd018,

   781                      "%s: residual count (%lx)\n", __func__, count);



The static checker assumes that we are testing to see if the loop ended with a break or not.  These are normally bugs because if we did not break the count is set to (ulong)-1.



This code is sort of weird.  It's just debugging so I guess it doesn't matter.  Are we expecting -1?



   782

   783          if (ent->hdr.entry_type != ENTRY_TYPE_TMP_END)

   784                  ql_dbg(ql_dbg_misc, vha, 0xd019,

   785                      "%s: missing end (%lx)\n", __func__, count);

   786

   787          ql_dbg(ql_dbg_misc, vha, 0xd01b,

   788              "%s: len=%lx\n", __func__, *len);

   789

   790          if (buf) {

   791                  ql_log(ql_log_warn, vha, 0xd015,

   792                      "Firmware dump saved to temp buffer (%ld/%p)\n",

   793                      vha->host_no, vha->hw->fw_dump);

   794                  qla2x00_post_uevent_work(vha, QLA_UEVENT_CODE_FW_DUMP);

   795          }

   796  }



regards,

dan carpenter

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 13228 bytes --]