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

[Andy Shevchenko <andy@kernel.org>]

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


_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan