Re: [PATCH iwl-next v3 3/6] pfcp: add PFCP module

From: Andy Shevchenko <andy@kernel.org>
To: Marcin Szycik <marcin.szycik@linux.intel.com>
Cc: intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
	wojciech.drewek@intel.com, michal.swiatkowski@linux.intel.com,
	aleksander.lobakin@intel.com, davem@davemloft.net,
	kuba@kernel.org, jiri@resnulli.us, pabeni@redhat.com,
	jesse.brandeburg@intel.com, simon.horman@corigine.com,
	idosch@nvidia.com
Subject: Re: [PATCH iwl-next v3 3/6] pfcp: add PFCP module
Date: Fri, 21 Jul 2023 17:54:05 +0300	[thread overview]
Message-ID: <ZLqcDf68HgB6Knnk@smile.fi.intel.com> (raw)
In-Reply-To: <20230721071532.613888-4-marcin.szycik@linux.intel.com>

On Fri, Jul 21, 2023 at 09:15:29AM +0200, Marcin Szycik wrote:
> From: Wojciech Drewek <wojciech.drewek@intel.com>
> 
> Packet Forwarding Control Protocol (PFCP) is a 3GPP Protocol
> used between the control plane and the user plane function.
> It is specified in TS 29.244[1].
> 
> Note that this module is not designed to support this Protocol
> in the kernel space. There is no support for parsing any PFCP messages.
> There is no API that could be used by any userspace daemon.
> Basically it does not support PFCP. This protocol is sophisticated
> and there is no need for implementing it in the kernel. The purpose
> of this module is to allow users to setup software and hardware offload
> of PFCP packets using tc tool.
> 
> When user requests to create a PFCP device, a new socket is created.
> The socket is set up with port number 8805 which is specific for
> PFCP [29.244 4.2.2]. This allow to receive PFCP request messages,
> response messages use other ports.
> 
> Note that only one PFCP netdev can be created.
> 
> Only IPv4 is supported at this time.
> 
> [1] https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=3111

> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>

Co-developed-by: Marcin...?

> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>

...

> +/* PFCP according to 3GPP TS 29.244
> + *
> + * Copyright (C) 2022, Intel Corporation.

> + * (C) 2022 by Wojciech Drewek <wojciech.drewek@intel.com>

Is it approved by our Legal? First time I see such (c) together with Intel's
and correct authorship.

> + * Author: Wojciech Drewek <wojciech.drewek@intel.com>
> + */

...

> +struct pfcp_dev {
> +	struct list_head	list;

This is defined in types.h which is missing.

> +	struct socket		*sock;
> +	struct net_device	*dev;
> +	struct net		*net;
> +};

...

> +	dev->needs_free_netdev	= true;

Single space is enough.

...

> +static int pfcp_newlink(struct net *net, struct net_device *dev,
> +			struct nlattr *tb[], struct nlattr *data[],
> +			struct netlink_ext_ack *extack)
> +{
> +	struct pfcp_dev *pfcp = netdev_priv(dev);
> +	struct pfcp_net *pn;
> +	int err;
> +
> +	pfcp->net = net;
> +
> +	err = pfcp_add_sock(pfcp);
> +	if (err) {
> +		netdev_dbg(dev, "failed to add pfcp socket %d\n", err);
> +		goto exit;
> +	}
> +
> +	err = register_netdevice(dev);
> +	if (err) {
> +		netdev_dbg(dev, "failed to register pfcp netdev %d\n", err);
> +		goto exit_reg_netdev;
> +	}
> +
> +	pn = net_generic(dev_net(dev), pfcp_net_id);
> +	list_add_rcu(&pfcp->list, &pn->pfcp_dev_list);
> +
> +	netdev_dbg(dev, "registered new PFCP interface\n");
> +
> +	return 0;
> +
> +exit_reg_netdev:

The label naming should tell what _will_ happen if goto $LABEL.
Something like

exit_del_pfcp_sock:

Ditto for all labels in your code.

> +	pfcp_del_sock(pfcp);
> +exit:

Shouldn't here be

	->net = NULL;

?

> +	return err;
> +}

...

> +#ifndef _PFCP_H_
> +#define _PFCP_H_

Missing headers:
For net_device internals, bool type, and strcpm() call.

> +#define PFCP_PORT 8805
> +
> +static inline bool netif_is_pfcp(const struct net_device *dev)
> +{
> +	return dev->rtnl_link_ops &&
> +	       !strcmp(dev->rtnl_link_ops->kind, "pfcp");
> +}
> +
> +#endif

-- 
With Best Regards,
Andy Shevchenko