Re: [PATCH v3] wwan: core: Support slicing in port TX flow of WWAN subsystem

On Mon, 2022-11-14 at 12:40 +0100, gregkh@linuxfoundation.org wrote:

On Mon, Nov 14, 2022 at 11:23:19AM +0000, Haozhe Chang (常浩哲) wrote:

quoted

Hi Greg Kroah-Hartman

On Fri, 2022-11-11 at 16:02 +0100, Greg Kroah-Hartman wrote:

quoted

On Fri, Nov 11, 2022 at 06:08:36PM +0800, 
haozhe.chang@mediatek.com
wrote:

quoted

From: haozhe chang <redacted>

wwan_port_fops_write inputs the SKB parameter to the TX
callback of
the WWAN device driver. However, the WWAN device (e.g., t7xx)
may
have an MTU less than the size of SKB, causing the TX buffer to
be
sliced and copied once more in the WWAN device driver.

This patch implements the slicing in the WWAN subsystem and
gives
the WWAN devices driver the option to slice(by frag_len) or
not. By
doing so, the additional memory copy is reduced.

Meanwhile, this patch gives WWAN devices driver the option to
reserve
headroom in fragments for the device-specific metadata.

Signed-off-by: haozhe chang <redacted>

---
Changes in v2
  -send fragments to device driver by skb frag_list.

Changes in v3
  -move frag_len and headroom_len setting to wwan_create_port.
---
 drivers/net/wwan/iosm/iosm_ipc_port.c  |  3 +-
 drivers/net/wwan/mhi_wwan_ctrl.c       |  2 +-
 drivers/net/wwan/rpmsg_wwan_ctrl.c     |  2 +-
 drivers/net/wwan/t7xx/t7xx_port_wwan.c | 34 +++++++--------
 drivers/net/wwan/wwan_core.c           | 59
++++++++++++++++++++
------
 drivers/net/wwan/wwan_hwsim.c          |  2 +-
 drivers/usb/class/cdc-wdm.c            |  2 +-
 include/linux/wwan.h                   |  6 ++-
 8 files changed, 73 insertions(+), 37 deletions(-)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_port.c

b/drivers/net/wwan/iosm/iosm_ipc_port.c
index b6d81c627277..dc43b8f0d1af 100644

--- a/drivers/net/wwan/iosm/iosm_ipc_port.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_port.c

@@ -63,7 +63,8 @@ struct iosm_cdev *ipc_port_init(struct

iosm_imem
*ipc_imem,
 	ipc_port->ipc_imem = ipc_imem;
 
 	ipc_port->iosm_port = wwan_create_port(ipc_port->dev,
port_type,
-					       &ipc_wwan_ctrl_o
ps,
ipc_port);
+					       &ipc_wwan_ctrl_o
ps, 0,
0,
+					       ipc_port);

How is 0, 0 a valid option here?

and if it is a valid option, shouldn't you just have 2 different
functions, one that needs these values and one that does
not?  That
would make it more descriptive as to what those options are, and
ensure
that you get them right.

0 is a valid option. 
frag_len set to 0 means no split, and headroom set to 0 means no 
reserved headroom in skb. 

Sorry, I can't understand why it's more descriptive, could you help
with more information? It seems to me that the device driver needs
to
know what each parameter is and how to set them, and that process
is
also required in your proposed solution - "with 2 different
functions",
right?

When you see random integers in the middle of a function call like
this,
you then have to go and look up the function call to determine what
exactly those values are and what is happening.  Using 0, 0 as valid
values helps no one out here at all.

While if the code said:
	ipc_port->iosm_port = wwan_create_port(ipc_port->dev,
port_type,
						&ipc_wwan_ctrl_ops,
						NO_SPLIT,
						NO_RESERVED_HEADROOM,
						ipc_port);


or something like that, it would make more sense, right?

Remember, we write code for people to read and understand and
maintain
it over time first, for the compiler second.

Yes, you're right, I'll change it: change the random integer to the
macro definition to make it more readable, thanks.

thanks,

greg k-h