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] [day] [month] [year] [list]

Message-ID:
 <MEYP282MB269765ECA0ACAA6373F26560BB7B2@MEYP282MB2697.AUSP282.PROD.OUTLOOK.COM>
Date: Thu, 25 Jan 2024 00:57:09 +0800
From: Jinjian Song <songjinjian@...mail.com>
To: pabeni@...hat.com,
	Jinjian Song <songjinjian@...mail.com>,
	netdev@...r.kernel.org
Cc: chandrashekar.devegowda@...el.com,
	chiranjeevi.rapolu@...ux.intel.com,
	danielwinkler@...gle.com,
	davem@...emloft.net,
	edumazet@...gle.com,
	felix.yan@...ocom.com,
	haijun.liu@...iatek.com,
	jinjian.song@...ocom.com,
	joey.zhao@...ocom.com,
	johannes@...solutions.net,
	kuba@...nel.org,
	linux-kernel@...r.kernel.com,
	liuqf@...ocom.com,
	loic.poulain@...aro.org,
	m.chetan.kumar@...ux.intel.com,
	nmarupaka@...gle.com,
	ricardo.martinez@...ux.intel.com,
	ryazanov.s.a@...il.com,
	vsankar@...ovo.com
Subject: Re: [net-next v5 4/4] net: wwan: t7xx: Add fastboot WWAN port

From: Paolo Abeni <pabeni@...hat.com>

>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

Let me delete the info.

>[...]
>
>
>> +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.

Let me rename it to tx_skb.

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

Let me reorganize the code, I think I should merge fastboot codes to t7xx_port_wwan.c.

Best Regards,
JinJian