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

[Haozhe Chang (常浩哲)]

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