[PATCH net-next] l2tp: remove unneeded null check in l2tp_v2_session_get_next

[PATCH net-next] l2tp: remove unneeded null check in l2tp_v2_session_get_next

[James Chapman]

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(-)

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;
-- 
2.34.1

Re: [PATCH net-next] l2tp: remove unneeded null check in l2tp_v2_session_get_next

[Simon Horman]

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>

Re: [PATCH net-next] l2tp: remove unneeded null check in l2tp_v2_session_get_next

[Dan Carpenter]

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

Re: [PATCH net-next] l2tp: remove unneeded null check in l2tp_v2_session_get_next

[James Chapman]

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

Re: [PATCH net-next] l2tp: remove unneeded null check in l2tp_v2_session_get_next

[Dan Carpenter]

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