| Message ID | 20170511121633.3591880-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
> 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.
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
> > 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.
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
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); }
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