Re: [net-next v5 4/4] net: wwan: t7xx: Add fastboot WWAN port

[Paolo Abeni <pabeni@redhat.com>]

On Mon, 2024-01-22 at 17:09 +0800, Jinjian Song wrote:
> From: Jinjian Song <jinjian.song@fibocom.com>
> 
> On early detection of wwan device in fastboot mode, driver sets
> up CLDMA0 HW tx/rx queues for raw data transfer and then create
> fastboot port to userspace.
> 
> Application can use this port to flash firmware and collect
> core dump by fastboot protocol commands.
> 
> Signed-off-by: Jinjian Song <jinjian.song@fibocom.com>
> ---
> v5:
>  * no change 
> v4:
>  * change function prefix to t7xx_port_fastboot
>  * change the name 'FASTBOOT' to fastboot in struct t7xx_early_port_conf
> v3:
>  * no change
> v2:
>  * no change

Minor nit: you could/should omit the 'no change' entries

[...]


> +static int t7xx_port_fastboot_tx(struct wwan_port *port, struct sk_buff *skb)
> +{
> +	struct t7xx_port *port_private = wwan_port_get_drvdata(port);
> +	struct sk_buff *cur = skb, *cloned;
> +	size_t actual, len, offset = 0;
> +	int ret;
> +	int txq_mtu;
> +
> +	if (!port_private->chan_enable)
> +		return -EINVAL;
> +
> +	txq_mtu = t7xx_get_port_mtu(port_private);
> +	if (txq_mtu < 0)
> +		return -EINVAL;
> +
> +	actual = cur->len;
> +	while (actual) {
> +		len = min_t(size_t, actual, txq_mtu);
> +		cloned = __dev_alloc_skb(len, GFP_KERNEL);

Minor nit: the variable name is misleading, as the skb is not cloned.

> +		if (!cloned)
> +			return -ENOMEM;
> +
> +		skb_put_data(cloned, cur->data + offset, len);
> +
> +		ret = t7xx_port_send_raw_skb(port_private, cloned);
> +		if (ret) {
> +			dev_kfree_skb(cloned);
> +			dev_err(port_private->dev, "Write error on fastboot port, %d\n", ret);
> +			break;
> +		}
> +		offset += len;
> +		actual -= len;
> +	}
> +
> +	dev_kfree_skb(skb);
> +	return 0;
> +}
> 

[...]
> ++static void t7xx_port_fastboot_uninit(struct t7xx_port *port)
> +{
> +	if (!port->wwan.wwan_port)
> +		return;
> +
> +	port->rx_length_th = 0;
> +	wwan_remove_port(port->wwan.wwan_port);
> +	port->wwan.wwan_port = NULL;
> +}
> +
> +static int t7xx_port_fastboot_recv_skb(struct t7xx_port *port, struct sk_buff *skb)
> +{
> +	if (!atomic_read(&port->usage_cnt) || !port->chan_enable) {
> +		const struct t7xx_port_conf *port_conf = port->port_conf;
> +
> +		dev_kfree_skb_any(skb);
> +		dev_err_ratelimited(port->dev, "Port %s is not opened, drop packets\n",
> +				    port_conf->name);
> +		/* Dropping skb, caller should not access skb.*/
> +		return 0;
> +	}
> +
> +	wwan_port_rx(port->wwan.wwan_port, skb);
> +
> +	return 0;
> +}
> +
> +static int t7xx_port_fastboot_enable_chl(struct t7xx_port *port)
> +{
> +	spin_lock(&port->port_update_lock);
> +	port->chan_enable = true;
> +	spin_unlock(&port->port_update_lock);
> +
> +	return 0;
> +}
> +
> +static int t7xx_port_fastboot_disable_chl(struct t7xx_port *port)
> +{
> +	spin_lock(&port->port_update_lock);
> +	port->chan_enable = false;
> +	spin_unlock(&port->port_update_lock);
> +
> +	return 0;
> +}

The above 4 functions implementation are exact copies of
t7xx_port_wwan_*() functions in drivers/net/wwan/t7xx/t7xx_port_wwan.c

Please reorganize the code to avoid such duplication, e.g. renaming the
exiting function to something more generic, making them non static, and
declaring them in some t7xx specific header.

Cheers,

Paolo