| Message ID | 1379075438-20240-1-git-send-email-prarit@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Sep 13, 2013 at 3:30 PM, Prarit Bhargava <prarit@redhat.com> wrote: > Fix unitialized variable warnings. Hi, I'd like Jack, who is in charge on this code to take a look, added him. Or. > > drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function ‘mlx4_HW2SW_CQ_wrapper’: > drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:2551:16: error: ‘cq’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > atomic_dec(&cq->mtt->ref_count); > ^ > drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function ‘mlx4_HW2SW_SRQ_wrapper’: > drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:2734:17: error: ‘srq’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > atomic_dec(&srq->mtt->ref_count); > > Signed-off-by: Prarit Bhargava <prarit@redhat.com> > Cc: dledford@redhat.com > Cc: Amir Vadai <amirv@mellanox.com> > Cc: Or Gerlitz <ogerlitz@mellanox.com> > --- > .../net/ethernet/mellanox/mlx4/resource_tracker.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c > index dd68763..d703838 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c > +++ b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c > @@ -2563,7 +2563,7 @@ int mlx4_HW2SW_CQ_wrapper(struct mlx4_dev *dev, int slave, > { > int err; > int cqn = vhcr->in_modifier; > - struct res_cq *cq; > + struct res_cq *uninitialized_var(cq); > > err = cq_res_start_move_to(dev, slave, cqn, RES_CQ_ALLOCATED, &cq); > if (err) > @@ -2746,7 +2746,7 @@ int mlx4_HW2SW_SRQ_wrapper(struct mlx4_dev *dev, int slave, > { > int err; > int srqn = vhcr->in_modifier; > - struct res_srq *srq; > + struct res_srq *uninitialized_var(srq); > > err = srq_res_start_move_to(dev, slave, srqn, RES_SRQ_ALLOCATED, &srq); > if (err) > -- > 1.7.9.3 > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 14 Sep 2013 22:10:19 +0300 Or Gerlitz <or.gerlitz@gmail.com> wrote: > > + struct res_cq *uninitialized_var(cq); > > > > err = cq_res_start_move_to(dev, slave, cqn, > > RES_CQ_ALLOCATED, &cq); I have no objection. However, I don't know if the compiler is being too clever here or too stupid. The cq variable is initialized in cq_res_start_move_to(), but the compiler is ignoring this -- or maybe it is simply not assuming that cq will in fact be initialized by the called procedure? In any event, this change cannot hurt. -Jack -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
hell what! On Sun, Sep 15, 2013 at 09:14:00AM +0300, Jack Morgenstein wrote: > On Sat, 14 Sep 2013 22:10:19 +0300 > Or Gerlitz <or.gerlitz@gmail.com> wrote: > > > > + struct res_cq *uninitialized_var(cq); > > > > > > err = cq_res_start_move_to(dev, slave, cqn, > > > RES_CQ_ALLOCATED, &cq); > > I have no objection. However, I don't know if the compiler is being too > clever here or too stupid. The cq variable is initialized in > cq_res_start_move_to(), but the compiler is ignoring this -- or maybe > it is simply not assuming that cq will in fact be > initialized by the called procedure? > > In any event, this change cannot hurt. > > -Jack > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
sorry for the inconvenince, i'm just practise the use of mutt, sorry again. On Sun, Sep 15, 2013 at 2:22 PM, ZHAO Gang <gamerh2o@gmail.com> wrote: > hell what! > > On Sun, Sep 15, 2013 at 09:14:00AM +0300, Jack Morgenstein wrote: >> On Sat, 14 Sep 2013 22:10:19 +0300 >> Or Gerlitz <or.gerlitz@gmail.com> wrote: >> >> > > + struct res_cq *uninitialized_var(cq); >> > > >> > > err = cq_res_start_move_to(dev, slave, cqn, >> > > RES_CQ_ALLOCATED, &cq); >> >> I have no objection. However, I don't know if the compiler is being too >> clever here or too stupid. The cq variable is initialized in >> cq_res_start_move_to(), but the compiler is ignoring this -- or maybe >> it is simply not assuming that cq will in fact be >> initialized by the called procedure? >> >> In any event, this change cannot hurt. >> >> -Jack >> -- >> To unsubscribe from this list: send the line "unsubscribe netdev" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Sat, 14 Sep 2013 22:10:19 +0300 > Or Gerlitz <or.gerlitz@gmail.com> wrote: > > > > + struct res_cq *uninitialized_var(cq); > > > > > > err = cq_res_start_move_to(dev, slave, cqn, > > > RES_CQ_ALLOCATED, &cq); > > I have no objection. However, I don't know if the compiler is being too > clever here or too stupid. The cq variable is initialized in > cq_res_start_move_to(), but the compiler is ignoring this -- or maybe > it is simply not assuming that cq will in fact be > initialized by the called procedure? Possibly cq_res_start_move_to() got inlined - and the analysis of the inlined code found a path where it wasn't initialised. Most likely one of the paths where the return value is non-zero. > In any event, this change cannot hurt. It could hide a real 'used but not initialised' error later on... David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/16/2013 04:59 AM, David Laight wrote: >> On Sat, 14 Sep 2013 22:10:19 +0300 >> Or Gerlitz <or.gerlitz@gmail.com> wrote: >> >>>> + struct res_cq *uninitialized_var(cq); >>>> >>>> err = cq_res_start_move_to(dev, slave, cqn, >>>> RES_CQ_ALLOCATED, &cq); >> >> I have no objection. However, I don't know if the compiler is being too >> clever here or too stupid. The cq variable is initialized in >> cq_res_start_move_to(), but the compiler is ignoring this -- or maybe >> it is simply not assuming that cq will in fact be >> initialized by the called procedure? > > Possibly cq_res_start_move_to() got inlined - and the analysis > of the inlined code found a path where it wasn't initialised. > > Most likely one of the paths where the return value is non-zero. > >> In any event, this change cannot hurt. > > It could hide a real 'used but not initialised' error later on... Hi David, I certainly don't profess to know the code better than you do of course, but in terms of risk we're no better in than the original code here, and it seems that other places in the kernel the resolution has been to either init to NULL or wrap with uninitialized_var()... P. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 16 Sep 2013 09:59:41 +0100 "David Laight" <David.Laight@ACULAB.COM> wrote: > > In any event, this change cannot hurt. > > It could hide a real 'used but not initialised' error later on... > > David In this case, it doesn't. If cq_res_start_move_to returns 0, the "cq" pointer points to the desired cq object. In all instances where the cq object is not found, you get an error return, and the cq pointer in the caller is never de-referenced. Therefore, the compiler warning is a false positive, and the "uninitialized_var" is just a workaround to satisfy the compiler. Same for the SRQ case. -Jack -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Prarit Bhargava <prarit@redhat.com> Date: Fri, 13 Sep 2013 08:30:38 -0400 > @@ -2563,7 +2563,7 @@ int mlx4_HW2SW_CQ_wrapper(struct mlx4_dev *dev, int slave, > { > int err; > int cqn = vhcr->in_modifier; > - struct res_cq *cq; > + struct res_cq *uninitialized_var(cq); > > err = cq_res_start_move_to(dev, slave, cqn, RES_CQ_ALLOCATED, &cq); > if (err) This gross annotation is not necessary. Make cq_res_start_move_to() return 'cq' as an error pointer instead of setting 'cq' by reference. -- To unsubscribe from this list: send the line "unsubscribe netdev" 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/net/ethernet/mellanox/mlx4/resource_tracker.c b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c index dd68763..d703838 100644 --- a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +++ b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c @@ -2563,7 +2563,7 @@ int mlx4_HW2SW_CQ_wrapper(struct mlx4_dev *dev, int slave, { int err; int cqn = vhcr->in_modifier; - struct res_cq *cq; + struct res_cq *uninitialized_var(cq); err = cq_res_start_move_to(dev, slave, cqn, RES_CQ_ALLOCATED, &cq); if (err) @@ -2746,7 +2746,7 @@ int mlx4_HW2SW_SRQ_wrapper(struct mlx4_dev *dev, int slave, { int err; int srqn = vhcr->in_modifier; - struct res_srq *srq; + struct res_srq *uninitialized_var(srq); err = srq_res_start_move_to(dev, slave, srqn, RES_SRQ_ALLOCATED, &srq); if (err)
Fix unitialized variable warnings. drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function ‘mlx4_HW2SW_CQ_wrapper’: drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:2551:16: error: ‘cq’ may be used uninitialized in this function [-Werror=maybe-uninitialized] atomic_dec(&cq->mtt->ref_count); ^ drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function ‘mlx4_HW2SW_SRQ_wrapper’: drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:2734:17: error: ‘srq’ may be used uninitialized in this function [-Werror=maybe-uninitialized] atomic_dec(&srq->mtt->ref_count); Signed-off-by: Prarit Bhargava <prarit@redhat.com> Cc: dledford@redhat.com Cc: Amir Vadai <amirv@mellanox.com> Cc: Or Gerlitz <ogerlitz@mellanox.com> --- .../net/ethernet/mellanox/mlx4/resource_tracker.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)