On Mon, 2022-11-14 at 12:40 +0100, gregkh@xxxxxxxxxxxxxxxxxxx wrote: > On Mon, Nov 14, 2022 at 11:23:19AM +0000, Haozhe Chang (常浩哲) wrote: > > Hi Greg Kroah-Hartman > > > > On Fri, 2022-11-11 at 16:02 +0100, Greg Kroah-Hartman wrote: > > > On Fri, Nov 11, 2022 at 06:08:36PM +0800, > > > haozhe.chang@xxxxxxxxxxxx > > > wrote: > > > > From: haozhe chang <haozhe.chang@xxxxxxxxxxxx> > > > > > > > > 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 <haozhe.chang@xxxxxxxxxxxx> > > > > > > > > --- > > > > 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
