| Message ID | 20221111100840.105305-1-haozhe.chang@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] wwan: core: Support slicing in port TX flow of WWAN subsystem | expand |
On Fri, Nov 11, 2022 at 06:08:36PM +0800, haozhe.chang@mediatek.com wrote: > From: haozhe chang <haozhe.chang@mediatek.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@mediatek.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_ops, ipc_port); > + &ipc_wwan_ctrl_ops, 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. > @@ -112,7 +117,6 @@ void wwan_port_rx(struct wwan_port *port, struct sk_buff *skb); > */ > void wwan_port_txoff(struct wwan_port *port); > > - > /** Unneeded change. thanks, greg k-h
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@mediatek.com > wrote: > > From: haozhe chang <haozhe.chang@mediatek.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@mediatek.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_ops, > > ipc_port); > > + &ipc_wwan_ctrl_ops, 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? > > @@ -112,7 +117,6 @@ void wwan_port_rx(struct wwan_port *port, > > struct sk_buff *skb); > > */ > > void wwan_port_txoff(struct wwan_port *port); > > > > - > > /** > > Unneeded change. > Yes, I will rollback it. > thanks, > > greg k-h
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@mediatek.com > > wrote: > > > From: haozhe chang <haozhe.chang@mediatek.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@mediatek.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_ops, > > > ipc_port); > > > + &ipc_wwan_ctrl_ops, 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. thanks, greg k-h
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_ops, ipc_port); + &ipc_wwan_ctrl_ops, 0, 0, + ipc_port); return ipc_port; } diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c index f7ca52353f40..29300838531c 100644 --- a/drivers/net/wwan/mhi_wwan_ctrl.c +++ b/drivers/net/wwan/mhi_wwan_ctrl.c @@ -237,7 +237,7 @@ static int mhi_wwan_ctrl_probe(struct mhi_device *mhi_dev, /* Register as a wwan port, id->driver_data contains wwan port type */ port = wwan_create_port(&cntrl->mhi_dev->dev, id->driver_data, - &wwan_pops, mhiwwan); + &wwan_pops, 0, 0, mhiwwan); if (IS_ERR(port)) { kfree(mhiwwan); return PTR_ERR(port); diff --git a/drivers/net/wwan/rpmsg_wwan_ctrl.c b/drivers/net/wwan/rpmsg_wwan_ctrl.c index 31c24420ab2e..060f06072dca 100644 --- a/drivers/net/wwan/rpmsg_wwan_ctrl.c +++ b/drivers/net/wwan/rpmsg_wwan_ctrl.c @@ -129,7 +129,7 @@ static int rpmsg_wwan_ctrl_probe(struct rpmsg_device *rpdev) /* Register as a wwan port, id.driver_data contains wwan port type */ port = wwan_create_port(parent, rpdev->id.driver_data, - &rpmsg_wwan_pops, rpwwan); + &rpmsg_wwan_pops, 0, 0, rpwwan); if (IS_ERR(port)) return PTR_ERR(port); diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c b/drivers/net/wwan/t7xx/t7xx_port_wwan.c index 33931bfd78fd..b75bb272f861 100644 --- a/drivers/net/wwan/t7xx/t7xx_port_wwan.c +++ b/drivers/net/wwan/t7xx/t7xx_port_wwan.c @@ -54,13 +54,13 @@ static void t7xx_port_ctrl_stop(struct wwan_port *port) static int t7xx_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb) { struct t7xx_port *port_private = wwan_port_get_drvdata(port); - size_t len, offset, chunk_len = 0, txq_mtu = CLDMA_MTU; const struct t7xx_port_conf *port_conf; + struct sk_buff *cur = skb, *cloned; struct t7xx_fsm_ctl *ctl; enum md_state md_state; + int cnt = 0, ret; - len = skb->len; - if (!len || !port_private->chan_enable) + if (!port_private->chan_enable) return -EINVAL; port_conf = port_private->port_conf; @@ -72,23 +72,21 @@ static int t7xx_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb) return -ENODEV; } - for (offset = 0; offset < len; offset += chunk_len) { - struct sk_buff *skb_ccci; - int ret; - - chunk_len = min(len - offset, txq_mtu - sizeof(struct ccci_header)); - skb_ccci = t7xx_port_alloc_skb(chunk_len); - if (!skb_ccci) - return -ENOMEM; - - skb_put_data(skb_ccci, skb->data + offset, chunk_len); - ret = t7xx_port_send_skb(port_private, skb_ccci, 0, 0); + while (cur) { + cloned = skb_clone(cur, GFP_KERNEL); + cloned->len = skb_headlen(cur); + ret = t7xx_port_send_skb(port_private, cloned, 0, 0); if (ret) { - dev_kfree_skb_any(skb_ccci); + dev_kfree_skb(cloned); dev_err(port_private->dev, "Write error on %s port, %d\n", port_conf->name, ret); - return ret; + return cnt ? cnt + ret : ret; } + cnt += cur->len; + if (cur == skb) + cur = skb_shinfo(skb)->frag_list; + else + cur = cur->next; } dev_kfree_skb(skb); @@ -154,13 +152,15 @@ static int t7xx_port_wwan_disable_chl(struct t7xx_port *port) static void t7xx_port_wwan_md_state_notify(struct t7xx_port *port, unsigned int state) { const struct t7xx_port_conf *port_conf = port->port_conf; + unsigned int header_len = sizeof(struct ccci_header); if (state != MD_STATE_READY) return; if (!port->wwan_port) { port->wwan_port = wwan_create_port(port->dev, port_conf->port_type, - &wwan_ops, port); + &wwan_ops, CLDMA_MTU - header_len, + header_len, port); if (IS_ERR(port->wwan_port)) dev_err(port->dev, "Unable to create WWWAN port %s", port_conf->name); } diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c index 62e9f7d6c9fe..0d466f4cc2df 100644 --- a/drivers/net/wwan/wwan_core.c +++ b/drivers/net/wwan/wwan_core.c @@ -67,6 +67,8 @@ struct wwan_device { * @rxq: Buffer inbound queue * @waitqueue: The waitqueue for port fops (read/write/poll) * @data_lock: Port specific data access serialization + * @headroom_len: SKB reserved headroom size + * @frag_len: Length to split packet * @at_data: AT port specific data */ struct wwan_port { @@ -79,6 +81,8 @@ struct wwan_port { struct sk_buff_head rxq; wait_queue_head_t waitqueue; struct mutex data_lock; /* Port specific data access serialization */ + size_t headroom_len; + size_t frag_len; union { struct { struct ktermios termios; @@ -422,6 +426,8 @@ static int __wwan_port_dev_assign_name(struct wwan_port *port, const char *fmt) struct wwan_port *wwan_create_port(struct device *parent, enum wwan_port_type type, const struct wwan_port_ops *ops, + size_t frag_len, + unsigned int headroom_len, void *drvdata) { struct wwan_device *wwandev; @@ -455,6 +461,8 @@ struct wwan_port *wwan_create_port(struct device *parent, port->type = type; port->ops = ops; + port->frag_len = frag_len ? frag_len : SIZE_MAX; + port->headroom_len = headroom_len; mutex_init(&port->ops_lock); skb_queue_head_init(&port->rxq); init_waitqueue_head(&port->waitqueue); @@ -698,30 +706,53 @@ static ssize_t wwan_port_fops_read(struct file *filp, char __user *buf, static ssize_t wwan_port_fops_write(struct file *filp, const char __user *buf, size_t count, loff_t *offp) { + struct sk_buff *skb, *head = NULL, *tail = NULL; struct wwan_port *port = filp->private_data; - struct sk_buff *skb; + size_t frag_len, remain = count; int ret; ret = wwan_wait_tx(port, !!(filp->f_flags & O_NONBLOCK)); if (ret) return ret; - skb = alloc_skb(count, GFP_KERNEL); - if (!skb) - return -ENOMEM; + do { + frag_len = min(remain, port->frag_len); + skb = alloc_skb(frag_len + port->headroom_len, GFP_KERNEL); + if (!skb) { + ret = -ENOMEM; + goto freeskb; + } + skb_reserve(skb, port->headroom_len); + + if (!head) { + head = skb; + } else if (!tail) { + skb_shinfo(head)->frag_list = skb; + tail = skb; + } else { + tail->next = skb; + tail = skb; + } - if (copy_from_user(skb_put(skb, count), buf, count)) { - kfree_skb(skb); - return -EFAULT; - } + if (copy_from_user(skb_put(skb, frag_len), buf + count - remain, frag_len)) { + ret = -EFAULT; + goto freeskb; + } - ret = wwan_port_op_tx(port, skb, !!(filp->f_flags & O_NONBLOCK)); - if (ret) { - kfree_skb(skb); - return ret; - } + if (skb != head) { + head->data_len += skb->len; + head->len += skb->len; + head->truesize += skb->truesize; + } + } while (remain -= frag_len); + + ret = wwan_port_op_tx(port, head, !!(filp->f_flags & O_NONBLOCK)); + if (!ret) + return count; - return count; +freeskb: + kfree_skb(head); + return ret; } static __poll_t wwan_port_fops_poll(struct file *filp, poll_table *wait) diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c index ff09a8cedf93..3a02a75d4485 100644 --- a/drivers/net/wwan/wwan_hwsim.c +++ b/drivers/net/wwan/wwan_hwsim.c @@ -205,7 +205,7 @@ static struct wwan_hwsim_port *wwan_hwsim_port_new(struct wwan_hwsim_dev *dev) port->wwan = wwan_create_port(&dev->dev, WWAN_PORT_AT, &wwan_hwsim_port_ops, - port); + 0, 0, port); if (IS_ERR(port->wwan)) { err = PTR_ERR(port->wwan); goto err_free_port; diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c index 1f0951be15ab..ff7494635d0f 100644 --- a/drivers/usb/class/cdc-wdm.c +++ b/drivers/usb/class/cdc-wdm.c @@ -929,7 +929,7 @@ static void wdm_wwan_init(struct wdm_device *desc) return; } - port = wwan_create_port(&intf->dev, desc->wwanp_type, &wdm_wwan_port_ops, desc); + port = wwan_create_port(&intf->dev, desc->wwanp_type, &wdm_wwan_port_ops, 0, 0, desc); if (IS_ERR(port)) { dev_err(&intf->dev, "%s: Unable to create WWAN port\n", dev_name(intf->usb_dev)); diff --git a/include/linux/wwan.h b/include/linux/wwan.h index 5ce2acf444fb..e1a6554792fc 100644 --- a/include/linux/wwan.h +++ b/include/linux/wwan.h @@ -67,6 +67,9 @@ struct wwan_port_ops { * @parent: Device to use as parent and shared by all WWAN ports * @type: WWAN port type * @ops: WWAN port operations + * @frag_len: TX fragments length, if 0 is set, + * the WWAN core will not split the user package. + * @headroom_len: TX fragments reserved headroom length * @drvdata: Pointer to caller driver data * * Allocate and register a new WWAN port. The port will be automatically exposed @@ -84,6 +87,8 @@ struct wwan_port_ops { struct wwan_port *wwan_create_port(struct device *parent, enum wwan_port_type type, const struct wwan_port_ops *ops, + size_t frag_len, + unsigned int headroom_len, void *drvdata); /** @@ -112,7 +117,6 @@ void wwan_port_rx(struct wwan_port *port, struct sk_buff *skb); */ void wwan_port_txoff(struct wwan_port *port); - /** * wwan_port_txon - Restart TX on WWAN port * @port: WWAN port for which TX must be restarted