qed: fix uninitialized data in aRFS intrastructure

Message ID 20170511121633.3591880-1-arnd@arndb.de
State New
Headers show

Commit Message

Arnd Bergmann May 11, 2017, 12:15 p.m.
The new code contains an incredibly elaborate way of setting a 64-bit
register, which went subtly wrong due to the wrong size in a memset():

ethernet/qlogic/qed/qed_init_fw_funcs.c: In function 'qed_set_rfs_mode_disable':
ethernet/qlogic/qed/qed_init_fw_funcs.c:993:3: error: '*((void *)&ramline+4)' is used uninitialized in this function [-Werror=uninitialized]

This removes the silly loop and memset, and instead directly writes
the correct value to the register.

Fixes: d51e4af5c209 ("qed: aRFS infrastructure support")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 .../net/ethernet/qlogic/qed/qed_init_fw_funcs.c    | 48 +++++-----------------
 1 file changed, 11 insertions(+), 37 deletions(-)

-- 
2.9.0

Comments

Mintz, Yuval May 11, 2017, 2:03 p.m. | #1
> register, which went subtly wrong due to the wrong size in a memset():

> 

> ethernet/qlogic/qed/qed_init_fw_funcs.c: In function

> 'qed_set_rfs_mode_disable':

> ethernet/qlogic/qed/qed_init_fw_funcs.c:993:3: error: '*((void

> *)&ramline+4)' is used uninitialized in this function [-Werror=uninitialized]

> 

> This removes the silly loop and memset, and instead directly writes the

> correct value to the register.


Hi Arnd,

For the most part - I'm almost all in favor of this change.
But just to make it clear - the actual fix could have been a one-liner, right?
The rest are style changes.

> +#define CAM_REG(pf_id) (PRS_REG_GFT_CAM + CAM_LINE_SIZE * (pf_id))

> +#define RAM_REG(pf_id) (PRS_REG_GFT_PROFILE_MASK_RAM +


Not sure I'm a huge fan of this specific style change;
Seems like we could easily manage without these macros.
Arnd Bergmann May 11, 2017, 2:31 p.m. | #2
On Thu, May 11, 2017 at 4:03 PM, Mintz, Yuval <Yuval.Mintz@cavium.com> wrote:
>> register, which went subtly wrong due to the wrong size in a memset():

>>

>> ethernet/qlogic/qed/qed_init_fw_funcs.c: In function

>> 'qed_set_rfs_mode_disable':

>> ethernet/qlogic/qed/qed_init_fw_funcs.c:993:3: error: '*((void

>> *)&ramline+4)' is used uninitialized in this function [-Werror=uninitialized]

>>

>> This removes the silly loop and memset, and instead directly writes the

>> correct value to the register.

>

> Hi Arnd,

>

> For the most part - I'm almost all in favor of this change.

> But just to make it clear - the actual fix could have been a one-liner, right?

> The rest are style changes.


Correct. Having the correct length in the memset is a sufficient fix for
the warning, but it felt wrong to send it since the root of the problem
seems to be the complexity of the code that was hiding it.

>> +#define CAM_REG(pf_id) (PRS_REG_GFT_CAM + CAM_LINE_SIZE * (pf_id))

>> +#define RAM_REG(pf_id) (PRS_REG_GFT_PROFILE_MASK_RAM +

>

> Not sure I'm a huge fan of this specific style change;

> Seems like we could easily manage without these macros.


I tried first and ended up with really long lines that I did not like.

Generally speaking, feel free to treat any of my compile-time warning
fix patches as simple bug reports and apply a different fix that seems
more appropriate. I mainly send it in patch form since that seems to be
the quickest way to address any issues.

      Arnd
Mintz, Yuval May 11, 2017, 2:37 p.m. | #3
> > For the most part - I'm almost all in favor of this change.

> > But just to make it clear - the actual fix could have been a one-liner, right?

> > The rest are style changes.


> Correct. Having the correct length in the memset is a sufficient fix for the warning,

> but it felt wrong to send it since the root of the problem seems to be the

> complexity of the code that was hiding it.


...

> Generally speaking, feel free to treat any of my compile-time warning fix

> patches as simple bug reports and apply a different fix that seems more

> appropriate. I mainly send it in patch form since that seems to be the

> quickest way to address any issues.


Sure. 

Once net-next is re-opened I intend to push our next FW version which
is also going to change some of the aRFS related configurations.

So I think we should stick to the single-liner fix for now,
and I'll revise the style [if still needed; I'll have to check] on that submission.
Arnd Bergmann May 11, 2017, 3:01 p.m. | #4
On Thu, May 11, 2017 at 4:37 PM, Mintz, Yuval <Yuval.Mintz@cavium.com> wrote:
>> > For the most part - I'm almost all in favor of this change.

>> > But just to make it clear - the actual fix could have been a one-liner, right?

>> > The rest are style changes.

>

>> Correct. Having the correct length in the memset is a sufficient fix for the warning,

>> but it felt wrong to send it since the root of the problem seems to be the

>> complexity of the code that was hiding it.

>

> ...

>

>> Generally speaking, feel free to treat any of my compile-time warning fix

>> patches as simple bug reports and apply a different fix that seems more

>> appropriate. I mainly send it in patch form since that seems to be the

>> quickest way to address any issues.

>

> Sure.

>

> Once net-next is re-opened I intend to push our next FW version which

> is also going to change some of the aRFS related configurations.

>

> So I think we should stick to the single-liner fix for now,

> and I'll revise the style [if still needed; I'll have to check] on that submission.


Sounds good, thanks!

     Arnd

Patch

diff --git a/drivers/net/ethernet/qlogic/qed/qed_init_fw_funcs.c b/drivers/net/ethernet/qlogic/qed/qed_init_fw_funcs.c
index 67200c5498ab..a7c2c147a738 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_init_fw_funcs.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_init_fw_funcs.c
@@ -966,45 +966,29 @@  void qed_set_geneve_enable(struct qed_hwfn *p_hwfn,
 #define PARSER_ETH_CONN_CM_HDR (0x0)
 #define CAM_LINE_SIZE sizeof(u32)
 #define RAM_LINE_SIZE sizeof(u64)
-#define REG_SIZE sizeof(u32)
+#define CAM_REG(pf_id) (PRS_REG_GFT_CAM + CAM_LINE_SIZE * (pf_id))
+#define RAM_REG(pf_id) (PRS_REG_GFT_PROFILE_MASK_RAM + RAM_LINE_SIZE * (pf_id))
 
 void qed_set_rfs_mode_disable(struct qed_hwfn *p_hwfn,
 			      struct qed_ptt *p_ptt, u16 pf_id)
 {
-	union gft_cam_line_union camline;
-	struct gft_ram_line ramline;
-	u32 *p_ramline, i;
-
-	p_ramline = (u32 *)&ramline;
-
 	/*stop using gft logic */
 	qed_wr(p_hwfn, p_ptt, PRS_REG_SEARCH_GFT, 0);
 	qed_wr(p_hwfn, p_ptt, PRS_REG_CM_HDR_GFT, 0x0);
-	memset(&camline, 0, sizeof(union gft_cam_line_union));
-	qed_wr(p_hwfn, p_ptt, PRS_REG_GFT_CAM + CAM_LINE_SIZE * pf_id,
-	       camline.cam_line_mapped.camline);
-	memset(&ramline, 0, sizeof(union gft_cam_line_union));
-
-	for (i = 0; i < RAM_LINE_SIZE / REG_SIZE; i++) {
-		u32 hw_addr = PRS_REG_GFT_PROFILE_MASK_RAM;
-
-		hw_addr += (RAM_LINE_SIZE * pf_id + i * REG_SIZE);
-
-		qed_wr(p_hwfn, p_ptt, hw_addr, *(p_ramline + i));
-	}
+	qed_wr(p_hwfn, p_ptt, CAM_REG(pf_id), 0);
+	qed_wr(p_hwfn, p_ptt, RAM_REG(pf_id), 0);
+	qed_wr(p_hwfn, p_ptt, RAM_REG(pf_id) + 4, 0);
 }
 
 void qed_set_rfs_mode_enable(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt,
 			     u16 pf_id, bool tcp, bool udp,
 			     bool ipv4, bool ipv6)
 {
-	u32 rfs_cm_hdr_event_id, *p_ramline;
+	u32 rfs_cm_hdr_event_id;
 	union gft_cam_line_union camline;
 	struct gft_ram_line ramline;
-	int i;
 
 	rfs_cm_hdr_event_id = qed_rd(p_hwfn, p_ptt, PRS_REG_CM_HDR_GFT);
-	p_ramline = (u32 *)&ramline;
 
 	if (!ipv6 && !ipv4)
 		DP_NOTICE(p_hwfn,
@@ -1060,8 +1044,7 @@  void qed_set_rfs_mode_enable(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt,
 	}
 
 	/* write characteristics to cam */
-	qed_wr(p_hwfn, p_ptt, PRS_REG_GFT_CAM + CAM_LINE_SIZE * pf_id,
-	       camline.cam_line_mapped.camline);
+	qed_wr(p_hwfn, p_ptt, CAM_REG(pf_id), camline.cam_line_mapped.camline);
 	camline.cam_line_mapped.camline = qed_rd(p_hwfn, p_ptt,
 						 PRS_REG_GFT_CAM +
 						 CAM_LINE_SIZE * pf_id);
@@ -1074,19 +1057,10 @@  void qed_set_rfs_mode_enable(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt,
 	SET_FIELD(ramline.low32bits, GFT_RAM_LINE_SRC_PORT, 1);
 	SET_FIELD(ramline.low32bits, GFT_RAM_LINE_DST_PORT, 1);
 
-	/* each iteration write to reg */
-	for (i = 0; i < RAM_LINE_SIZE / REG_SIZE; i++)
-		qed_wr(p_hwfn, p_ptt,
-		       PRS_REG_GFT_PROFILE_MASK_RAM + RAM_LINE_SIZE * pf_id +
-		       i * REG_SIZE, *(p_ramline + i));
+	qed_wr(p_hwfn, p_ptt, RAM_REG(pf_id),     ramline.low32bits);
+	qed_wr(p_hwfn, p_ptt, RAM_REG(pf_id) + 4, ramline.high32bits);
 
 	/* set default profile so that no filter match will happen */
-	ramline.low32bits = 0xffff;
-	ramline.high32bits = 0xffff;
-
-	for (i = 0; i < RAM_LINE_SIZE / REG_SIZE; i++)
-		qed_wr(p_hwfn, p_ptt,
-		       PRS_REG_GFT_PROFILE_MASK_RAM + RAM_LINE_SIZE *
-		       PRS_GFT_CAM_LINES_NO_MATCH + i * REG_SIZE,
-		       *(p_ramline + i));
+	qed_wr(p_hwfn, p_ptt, RAM_REG(PRS_GFT_CAM_LINES_NO_MATCH),     0xffff);
+	qed_wr(p_hwfn, p_ptt, RAM_REG(PRS_GFT_CAM_LINES_NO_MATCH) + 4, 0xffff);
 }