| Message ID | 20240208122012.2597561-1-colin.i.king@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [next] drivers: net: xgene: remove redundant assignment to variable offset | expand |
On 08.02.2024 12:20, Colin Ian King wrote: > The variable offset is being initialized with a value that is never > read, it is being re-assigned later on in either path of an if > statement before being used. The initialization is redundant and > can be removed. > > Cleans up clang scan build warning: > drivers/net/ethernet/apm/xgene/xgene_enet_cle.c:736:2: warning: Value > stored to 'offset' is never read [deadcode.DeadStores] > > Signed-off-by: Colin Ian King <colin.i.king@gmail.com> > --- > drivers/net/ethernet/apm/xgene/xgene_enet_cle.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c b/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c > index de5464322311..8f104642897b 100644 > --- a/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c > +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c > @@ -733,7 +733,6 @@ static int xgene_cle_setup_rss(struct xgene_enet_pdata *pdata) > u32 offset, val = 0; > int i, ret = 0; > > - offset = CLE_PORT_OFFSET; > for (i = 0; i < cle->parsers; i++) { > if (cle->active_parser != PARSER_ALL) > offset = cle->active_parser * CLE_PORT_OFFSET; It looks like more refactoring can be done here. "if (cle->active_parser != PARSER_ALL)" is static, no need to check it inside the loop.
On Thu, Feb 08, 2024 at 12:39:24PM +0000, Vadim Fedorenko wrote: > On 08.02.2024 12:20, Colin Ian King wrote: > > The variable offset is being initialized with a value that is never > > read, it is being re-assigned later on in either path of an if > > statement before being used. The initialization is redundant and > > can be removed. > > > > Cleans up clang scan build warning: > > drivers/net/ethernet/apm/xgene/xgene_enet_cle.c:736:2: warning: Value > > stored to 'offset' is never read [deadcode.DeadStores] > > > > Signed-off-by: Colin Ian King <colin.i.king@gmail.com> > > --- > > drivers/net/ethernet/apm/xgene/xgene_enet_cle.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c b/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c > > index de5464322311..8f104642897b 100644 > > --- a/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c > > +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c > > @@ -733,7 +733,6 @@ static int xgene_cle_setup_rss(struct xgene_enet_pdata *pdata) > > u32 offset, val = 0; > > int i, ret = 0; > > - offset = CLE_PORT_OFFSET; > > for (i = 0; i < cle->parsers; i++) { > > if (cle->active_parser != PARSER_ALL) > > offset = cle->active_parser * CLE_PORT_OFFSET; > > It looks like more refactoring can be done here. > "if (cle->active_parser != PARSER_ALL)" is static, no need to check it inside > the loop. > You still need to check... I don't really think it's an improvement. regards, dan carpenter diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c b/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c index de5464322311..61e31cc55771 100644 --- a/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c @@ -733,11 +733,11 @@ static int xgene_cle_setup_rss(struct xgene_enet_pdata *pdata) u32 offset, val = 0; int i, ret = 0; - offset = CLE_PORT_OFFSET; + if (cle->active_parser != PARSER_ALL) + offset = cle->active_parser * CLE_PORT_OFFSET; + for (i = 0; i < cle->parsers; i++) { - if (cle->active_parser != PARSER_ALL) - offset = cle->active_parser * CLE_PORT_OFFSET; - else + if (cle->active_parser == PARSER_ALL) offset = i * CLE_PORT_OFFSET; /* enable RSS */
On 08/02/2024 13:40, Dan Carpenter wrote: > On Thu, Feb 08, 2024 at 12:39:24PM +0000, Vadim Fedorenko wrote: >> On 08.02.2024 12:20, Colin Ian King wrote: >>> The variable offset is being initialized with a value that is never >>> read, it is being re-assigned later on in either path of an if >>> statement before being used. The initialization is redundant and >>> can be removed. >>> >>> Cleans up clang scan build warning: >>> drivers/net/ethernet/apm/xgene/xgene_enet_cle.c:736:2: warning: Value >>> stored to 'offset' is never read [deadcode.DeadStores] >>> >>> Signed-off-by: Colin Ian King <colin.i.king@gmail.com> >>> --- >>> drivers/net/ethernet/apm/xgene/xgene_enet_cle.c | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c b/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c >>> index de5464322311..8f104642897b 100644 >>> --- a/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c >>> +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c >>> @@ -733,7 +733,6 @@ static int xgene_cle_setup_rss(struct xgene_enet_pdata *pdata) >>> u32 offset, val = 0; >>> int i, ret = 0; >>> - offset = CLE_PORT_OFFSET; >>> for (i = 0; i < cle->parsers; i++) { >>> if (cle->active_parser != PARSER_ALL) >>> offset = cle->active_parser * CLE_PORT_OFFSET; >> >> It looks like more refactoring can be done here. >> "if (cle->active_parser != PARSER_ALL)" is static, no need to check it inside >> the loop. >> > > You still need to check... I don't really think it's an improvement. > > regards, > dan carpenter > > diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c b/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c > index de5464322311..61e31cc55771 100644 > --- a/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c > +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c > @@ -733,11 +733,11 @@ static int xgene_cle_setup_rss(struct xgene_enet_pdata *pdata) > u32 offset, val = 0; > int i, ret = 0; > > - offset = CLE_PORT_OFFSET; > + if (cle->active_parser != PARSER_ALL) > + offset = cle->active_parser * CLE_PORT_OFFSET; > + I think we can add "else" here and avoid the loop in case of != PARSER_ALL and avoid "if" in the loop, wdyt? > for (i = 0; i < cle->parsers; i++) { > - if (cle->active_parser != PARSER_ALL) > - offset = cle->active_parser * CLE_PORT_OFFSET; > - else > + if (cle->active_parser == PARSER_ALL) > offset = i * CLE_PORT_OFFSET; > > /* enable RSS */
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c b/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c index de5464322311..8f104642897b 100644 --- a/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_cle.c @@ -733,7 +733,6 @@ static int xgene_cle_setup_rss(struct xgene_enet_pdata *pdata) u32 offset, val = 0; int i, ret = 0; - offset = CLE_PORT_OFFSET; for (i = 0; i < cle->parsers; i++) { if (cle->active_parser != PARSER_ALL) offset = cle->active_parser * CLE_PORT_OFFSET;
The variable offset is being initialized with a value that is never read, it is being re-assigned later on in either path of an if statement before being used. The initialization is redundant and can be removed. Cleans up clang scan build warning: drivers/net/ethernet/apm/xgene/xgene_enet_cle.c:736:2: warning: Value stored to 'offset' is never read [deadcode.DeadStores] Signed-off-by: Colin Ian King <colin.i.king@gmail.com> --- drivers/net/ethernet/apm/xgene/xgene_enet_cle.c | 1 - 1 file changed, 1 deletion(-)