| 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
| ||
|
Message-ID: <7e67abf9ccbbb74d86440642fdf58540785a07db.camel@mediatek.com>
Date: Mon, 21 Nov 2022 11:01:57 +0000
From: Haozhe Chang (常浩哲)
<Haozhe.Chang@...iatek.com>
To: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC: "stephan@...hold.net" <stephan@...hold.net>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mediatek@...ts.infradead.org"
<linux-mediatek@...ts.infradead.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-remoteproc@...r.kernel.org" <linux-remoteproc@...r.kernel.org>,
"linuxwwan@...el.com" <linuxwwan@...el.com>,
"m.chetan.kumar@...el.com" <m.chetan.kumar@...el.com>,
"oneukum@...e.com" <oneukum@...e.com>,
Hua Yang (杨华) <Hua.Yang@...iatek.com>,
"chiranjeevi.rapolu@...ux.intel.com"
<chiranjeevi.rapolu@...ux.intel.com>,
Haijun Liu (刘海军)
<haijun.liu@...iatek.com>,
"ryazanov.s.a@...il.com" <ryazanov.s.a@...il.com>,
"kuba@...nel.org" <kuba@...nel.org>,
"loic.poulain@...aro.org" <loic.poulain@...aro.org>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"johannes@...solutions.net" <johannes@...solutions.net>,
"chandrashekar.devegowda@...el.com"
<chandrashekar.devegowda@...el.com>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"shangxiaojing@...wei.com" <shangxiaojing@...wei.com>,
Lambert Wang (王伟)
<Lambert.Wang@...iatek.com>,
"matthias.bgg@...il.com" <matthias.bgg@...il.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"ricardo.martinez@...ux.intel.com" <ricardo.martinez@...ux.intel.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
Xiayu Zhang (张夏宇)
<Xiayu.Zhang@...iatek.com>
Subject: Re: [PATCH v3] wwan: core: Support slicing in port TX flow of WWAN
subsystem
On Mon, 2022-11-14 at 12:40 +0100, gregkh@...uxfoundation.org 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@...iatek.com
> > > wrote:
> > > > From: haozhe chang <haozhe.chang@...iatek.com>
> > > >
> > > > 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@...iatek.com>
> > > >
> > > > ---
> > > > 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
Powered by blists - more mailing lists