| Message ID | 20240902142953.926891-1-jchapman@katalix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] l2tp: remove unneeded null check in l2tp_v2_session_get_next | expand |
On Mon, Sep 02, 2024 at 03:29:53PM +0100, James Chapman wrote: > Sessions in l2tp_v2_session_idr always have a non-null session->tunnel > pointer since l2tp_session_register sets it before inserting the > session into the IDR. Therefore the null check on session->tunnel in > l2tp_v2_session_get_next is redundant and can be removed. > > Fixes: aa92c1cec92b ("l2tp: add tunnel/session get_next helpers") Hi James, As this patch doesn't appear to fix a bug I don't think a Fixes tag is warranted. With that in mind, if you want to cite the above commit you can just include the following text somewhere in the patch description. commit aa92c1cec92b ("l2tp: add tunnel/session get_next helpers") > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/r/202408111407.HtON8jqa-lkp@intel.com/ > CC: Dan Carpenter <dan.carpenter@linaro.org> > Signed-off-by: James Chapman <jchapman@katalix.com> > Signed-off-by: Tom Parkin <tparkin@katalix.com> And as you posted the patch, it would be slightly more intuitive if your SoB line came last. But I've seen conflicting advice about the order of tags within the past weeks. The above notwithstanding, this looks good to me. Reviewed-by: Simon Horman <horms@kernel.org>
On Tue, Sep 03, 2024 at 08:24:17AM +0100, Simon Horman wrote: > > Reported-by: kernel test robot <lkp@intel.com> > > Closes: https://lore.kernel.org/r/202408111407.HtON8jqa-lkp@intel.com/ > > CC: Dan Carpenter <dan.carpenter@linaro.org> > > Signed-off-by: James Chapman <jchapman@katalix.com> > > Signed-off-by: Tom Parkin <tparkin@katalix.com> > > And as you posted the patch, it would be slightly more intuitive > if your SoB line came last. But I've seen conflicting advice about > the order of tags within the past weeks. It should be in chronological order. People generally aren't going to get too fussed about the order except the Signed-off-by tags. Everyone who handles the patch adds their Signed-off-by to the end. Right now it looks like James wrote the patch and then Tom is the maintainer who merged it. Co-developed-by? regards, dan carpenter
On 03/09/2024 09:02, Dan Carpenter wrote: > On Tue, Sep 03, 2024 at 08:24:17AM +0100, Simon Horman wrote: >>> Reported-by: kernel test robot <lkp@intel.com> >>> Closes: https://lore.kernel.org/r/202408111407.HtON8jqa-lkp@intel.com/ >>> CC: Dan Carpenter <dan.carpenter@linaro.org> >>> Signed-off-by: James Chapman <jchapman@katalix.com> >>> Signed-off-by: Tom Parkin <tparkin@katalix.com> >> >> And as you posted the patch, it would be slightly more intuitive >> if your SoB line came last. But I've seen conflicting advice about >> the order of tags within the past weeks. > > It should be in chronological order. > > People generally aren't going to get too fussed about the order except the > Signed-off-by tags. Everyone who handles the patch adds their Signed-off-by to > the end. Right now it looks like James wrote the patch and then Tom is the > maintainer who merged it. Co-developed-by? I'm probably using tags incorrectly. When Tom or I submit kernel patches to netdev, we usually review each other's work first before sending the patch to netdev. But we thought that adding a Reviewed-by tag might short-cut proper community review, hence we use SoB to indicate that we're both happy with the patch and we're both interested in review feedback on it. On reflection, Acked-by would be better for this. I'll send a v2 with Acked-by to avoid confusion. Thanks! -- pw-bot: cr
On Tue, Sep 03, 2024 at 11:48:00AM +0100, James Chapman wrote: > On 03/09/2024 09:02, Dan Carpenter wrote: > > On Tue, Sep 03, 2024 at 08:24:17AM +0100, Simon Horman wrote: > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > Closes: https://lore.kernel.org/r/202408111407.HtON8jqa-lkp@intel.com/ > > > > CC: Dan Carpenter <dan.carpenter@linaro.org> > > > > Signed-off-by: James Chapman <jchapman@katalix.com> > > > > Signed-off-by: Tom Parkin <tparkin@katalix.com> > > > > > > And as you posted the patch, it would be slightly more intuitive > > > if your SoB line came last. But I've seen conflicting advice about > > > the order of tags within the past weeks. > > > > It should be in chronological order. > > > > People generally aren't going to get too fussed about the order except the > > Signed-off-by tags. Everyone who handles the patch adds their Signed-off-by to > > the end. Right now it looks like James wrote the patch and then Tom is the > > maintainer who merged it. Co-developed-by? > > I'm probably using tags incorrectly. When Tom or I submit kernel patches to > netdev, we usually review each other's work first before sending the patch > to netdev. But we thought that adding a Reviewed-by tag might short-cut > proper community review, hence we use SoB to indicate that we're both happy > with the patch and we're both interested in review feedback on it. > > On reflection, Acked-by would be better for this. I'll send a v2 with > Acked-by to avoid confusion. Signed-off-by is kind of like signing a legal document to say that there is no stolen copyright code from SCO. You don't need to sign it if you're not handling the code. Reviewed-by is fine or Acked-by is also fine. Reviewers will look at them the same way. regards, dan carpenter
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 32102d1ed4cd..3eec23ac5ab1 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -345,7 +345,7 @@ static struct l2tp_session *l2tp_v2_session_get_next(const struct net *net, goto again; } - if (tunnel && tunnel->tunnel_id == tid && + if (tunnel->tunnel_id == tid && refcount_inc_not_zero(&session->ref_count)) { rcu_read_unlock_bh(); return session;
Sessions in l2tp_v2_session_idr always have a non-null session->tunnel pointer since l2tp_session_register sets it before inserting the session into the IDR. Therefore the null check on session->tunnel in l2tp_v2_session_get_next is redundant and can be removed. Fixes: aa92c1cec92b ("l2tp: add tunnel/session get_next helpers") Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/r/202408111407.HtON8jqa-lkp@intel.com/ CC: Dan Carpenter <dan.carpenter@linaro.org> Signed-off-by: James Chapman <jchapman@katalix.com> Signed-off-by: Tom Parkin <tparkin@katalix.com> --- net/l2tp/l2tp_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)