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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <18cf401f00c953d434a0ef36b0d68df0270dfaa6.camel@redhat.com>
Date: Tue, 23 Jan 2024 13:15:19 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Jinjian Song <songjinjian@...mail.com>, netdev@...r.kernel.org
Cc: chandrashekar.devegowda@...el.com, chiranjeevi.rapolu@...ux.intel.com, 
 haijun.liu@...iatek.com, m.chetan.kumar@...ux.intel.com, 
 ricardo.martinez@...ux.intel.com, loic.poulain@...aro.org, 
 ryazanov.s.a@...il.com, johannes@...solutions.net, davem@...emloft.net, 
 edumazet@...gle.com, kuba@...nel.org, linux-kernel@...r.kernel.com, 
 vsankar@...ovo.com, danielwinkler@...gle.com, nmarupaka@...gle.com, 
 joey.zhao@...ocom.com, liuqf@...ocom.com, felix.yan@...ocom.com, Jinjian
 Song <jinjian.song@...ocom.com>
Subject: Re: [net-next v5 4/4] net: wwan: t7xx: Add fastboot WWAN port

On Mon, 2024-01-22 at 17:09 +0800, Jinjian Song wrote:
> From: Jinjian Song <jinjian.song@...ocom.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@...ocom.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