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

[Marcin Szycik <marcin.szycik@linux.intel.com>]

On 21.07.2023 16:54, Andy Shevchenko wrote:
> 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...?

In this case I'm only a sender, I didn't help in development.

> 
>> 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.

Right, I'll leave only first (c) line.

>> + * Author: Wojciech Drewek <wojciech.drewek@intel.com>
>> + */
> 
> ...
> 
>> +struct pfcp_dev {
>> +	struct list_head	list;
> 
> This is defined in types.h which is missing.

Will add.

> 
>> +	struct socket		*sock;
>> +	struct net_device	*dev;
>> +	struct net		*net;
>> +};
> 
> ...
> 
>> +	dev->needs_free_netdev	= true;
> 
> Single space is enough.

Will fix.

> 
> ...
> 
>> +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:

Another convention I've seen is `err_what_failed`. But yeah,
exit_reg_netdev doesn't match either convention, will change to your
suggestion.

> 
> Ditto for all labels in your code.
> 
>> +	pfcp_del_sock(pfcp);
>> +exit:
> 
> Shouldn't here be
> 
> 	->net = NULL;

Good catch, will add.

> 
> ?
> 
>> +	return err;
>> +}
> 
> ...
> 
>> +#ifndef _PFCP_H_
>> +#define _PFCP_H_
> 
> Missing headers:
> For net_device internals, bool type, and strcpm() call.

Will add.

> 
>> +#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
> 

Thank you for review!
Marcin