From: haozhe chang <[email protected]>
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 chunk) or not. By
doing so, the additional memory copy is reduced.
Meanwhile, this patch gives WWAN devices driver the option to reserve
headroom in SKB for the device-specific metadata.
Signed-off-by: haozhe chang <[email protected]>
---
Changes in v2
-send fragments to device driver by skb frag_list.
---
drivers/net/wwan/t7xx/t7xx_port_wwan.c | 42 ++++++++++-------
drivers/net/wwan/wwan_core.c | 65 ++++++++++++++++++++------
include/linux/wwan.h | 5 +-
3 files changed, 80 insertions(+), 32 deletions(-)
diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
index 33931bfd78fd..74fa58575d5a 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,33 +72,43 @@ 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 += cloned->len;
+ if (cur == skb)
+ cur = skb_shinfo(skb)->frag_list;
+ else
+ cur = cur->next;
}
dev_kfree_skb(skb);
return 0;
}
+static size_t t7xx_port_tx_headroom(struct wwan_port *port)
+{
+ return sizeof(struct ccci_header);
+}
+
+static size_t t7xx_port_tx_chunk_len(struct wwan_port *port)
+{
+ return CLDMA_MTU - sizeof(struct ccci_header);
+}
+
static const struct wwan_port_ops wwan_ops = {
.start = t7xx_port_ctrl_start,
.stop = t7xx_port_ctrl_stop,
.tx = t7xx_port_ctrl_tx,
+ .needed_headroom = t7xx_port_tx_headroom,
+ .tx_chunk_len = t7xx_port_tx_chunk_len,
};
static int t7xx_port_wwan_init(struct t7xx_port *port)
diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index 62e9f7d6c9fe..ed78471f9e38 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -20,7 +20,7 @@
#include <uapi/linux/wwan.h>
/* Maximum number of minors in use */
-#define WWAN_MAX_MINORS (1 << MINORBITS)
+#define WWAN_MAX_MINORS BIT(MINORBITS)
static DEFINE_MUTEX(wwan_register_lock); /* WWAN device create|remove lock */
static DEFINE_IDA(minors); /* minors for WWAN port chardevs */
@@ -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
+ * @chunk_len: Chunk len 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 chunk_len;
union {
struct {
struct ktermios termios;
@@ -550,8 +554,13 @@ static int wwan_port_op_start(struct wwan_port *port)
}
/* If port is already started, don't start again */
- if (!port->start_count)
+ if (!port->start_count) {
ret = port->ops->start(port);
+ if (port->ops->tx_chunk_len)
+ port->chunk_len = port->ops->tx_chunk_len(port);
+ if (port->ops->needed_headroom)
+ port->headroom_len = port->ops->needed_headroom(port);
+ }
if (!ret)
port->start_count++;
@@ -698,30 +707,56 @@ 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)
{
+ size_t len, chunk_len, offset, allowed_chunk_len;
+ struct sk_buff *skb, *head = NULL, *tail = NULL;
struct wwan_port *port = filp->private_data;
- struct sk_buff *skb;
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;
+ allowed_chunk_len = port->chunk_len ? port->chunk_len : count;
+ for (offset = 0; offset < count; offset += chunk_len) {
+ chunk_len = min(count - offset, allowed_chunk_len);
+ len = chunk_len + port->headroom_len;
+ skb = alloc_skb(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, chunk_len), buf + offset, chunk_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;
+ }
}
- return count;
+ if (head) {
+ ret = wwan_port_op_tx(port, head, !!(filp->f_flags & O_NONBLOCK));
+ if (!ret)
+ return count;
+ }
+freeskb:
+ kfree_skb(head);
+ return ret;
}
static __poll_t wwan_port_fops_poll(struct file *filp, poll_table *wait)
diff --git a/include/linux/wwan.h b/include/linux/wwan.h
index 5ce2acf444fb..bdeeef59bbfd 100644
--- a/include/linux/wwan.h
+++ b/include/linux/wwan.h
@@ -46,6 +46,8 @@ struct wwan_port;
* @tx: Non-blocking routine that sends WWAN port protocol data to the device.
* @tx_blocking: Optional blocking routine that sends WWAN port protocol data
* to the device.
+ * @needed_headroom: Optional routine that sets reserve headroom of skb.
+ * @tx_chunk_len: Optional routine that sets chunk len to split.
* @tx_poll: Optional routine that sets additional TX poll flags.
*
* The wwan_port_ops structure contains a list of low-level operations
@@ -58,6 +60,8 @@ struct wwan_port_ops {
/* Optional operations */
int (*tx_blocking)(struct wwan_port *port, struct sk_buff *skb);
+ size_t (*needed_headroom)(struct wwan_port *port);
+ size_t (*tx_chunk_len)(struct wwan_port *port);
__poll_t (*tx_poll)(struct wwan_port *port, struct file *filp,
poll_table *wait);
};
@@ -112,7 +116,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
--
2.17.0
Hi Haozhe,
On Tue, 8 Nov 2022 at 11:54, <[email protected]> wrote:
>
> From: haozhe chang <[email protected]>
>
> 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 chunk) or not. By
> doing so, the additional memory copy is reduced.
>
> Meanwhile, this patch gives WWAN devices driver the option to reserve
> headroom in SKB for the device-specific metadata.
>
> Signed-off-by: haozhe chang <[email protected]>
>
> ---
> Changes in v2
> -send fragments to device driver by skb frag_list.
> ---
> drivers/net/wwan/t7xx/t7xx_port_wwan.c | 42 ++++++++++-------
> drivers/net/wwan/wwan_core.c | 65 ++++++++++++++++++++------
> include/linux/wwan.h | 5 +-
> 3 files changed, 80 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> index 33931bfd78fd..74fa58575d5a 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 const struct wwan_port_ops wwan_ops = {
> .start = t7xx_port_ctrl_start,
> .stop = t7xx_port_ctrl_stop,
> .tx = t7xx_port_ctrl_tx,
> + .needed_headroom = t7xx_port_tx_headroom,
> + .tx_chunk_len = t7xx_port_tx_chunk_len,
Can you replace 'chunk' with 'frag' everywhere?
> };
>
> static int t7xx_port_wwan_init(struct t7xx_port *port)
> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index 62e9f7d6c9fe..ed78471f9e38 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c
> @@ -20,7 +20,7 @@
> #include <uapi/linux/wwan.h>
>
> /* Maximum number of minors in use */
> -#define WWAN_MAX_MINORS (1 << MINORBITS)
> +#define WWAN_MAX_MINORS BIT(MINORBITS)
>
> static DEFINE_MUTEX(wwan_register_lock); /* WWAN device create|remove lock */
> static DEFINE_IDA(minors); /* minors for WWAN port chardevs */
> @@ -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
> + * @chunk_len: Chunk len 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 chunk_len;
> union {
> struct {
> struct ktermios termios;
> @@ -550,8 +554,13 @@ static int wwan_port_op_start(struct wwan_port *port)
> }
>
> /* If port is already started, don't start again */
> - if (!port->start_count)
> + if (!port->start_count) {
> ret = port->ops->start(port);
> + if (port->ops->tx_chunk_len)
> + port->chunk_len = port->ops->tx_chunk_len(port);
So, maybe frag len and headroom should be parameters of
wwan_create_port() instead of port ops, as we really need this info
only once.
> + if (port->ops->needed_headroom)
> + port->headroom_len = port->ops->needed_headroom(port);
> + }
>
> if (!ret)
> port->start_count++;
> @@ -698,30 +707,56 @@ 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)
> {
> + size_t len, chunk_len, offset, allowed_chunk_len;
> + struct sk_buff *skb, *head = NULL, *tail = NULL;
> struct wwan_port *port = filp->private_data;
> - struct sk_buff *skb;
> 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;
> + allowed_chunk_len = port->chunk_len ? port->chunk_len : count;
I would suggest making port->chunk_len (frag_len) always valid, by
setting it to -1 (MAX size_t) when creating a port without frag_len
requirement.
> + for (offset = 0; offset < count; offset += chunk_len) {
> + chunk_len = min(count - offset, allowed_chunk_len);
> + len = chunk_len + port->headroom_len;
> + skb = alloc_skb(len, GFP_KERNEL);
That works but would prefer a simpler solution like:
do {
len = min(port->frag_len, remain);
skb = alloc_skb(len + port->needed_headroom; GFP_KERNEL);
[...]
copy_from_user(skb_put(skb, len), buf + count - remain)
} while ((remain -= len));
> + 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, chunk_len), buf + offset, chunk_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;
> + }
> }
>
> - return count;
> + if (head) {
How head can be null here?
> + ret = wwan_port_op_tx(port, head, !!(filp->f_flags & O_NONBLOCK));
> + if (!ret)
> + return count;
> + }
> +freeskb:
> + kfree_skb(head);
> + return ret;
> }
>
> static __poll_t wwan_port_fops_poll(struct file *filp, poll_table *wait)
> diff --git a/include/linux/wwan.h b/include/linux/wwan.h
> index 5ce2acf444fb..bdeeef59bbfd 100644
> --- a/include/linux/wwan.h
> +++ b/include/linux/wwan.h
> @@ -46,6 +46,8 @@ struct wwan_port;
> * @tx: Non-blocking routine that sends WWAN port protocol data to the device.
> * @tx_blocking: Optional blocking routine that sends WWAN port protocol data
> * to the device.
> + * @needed_headroom: Optional routine that sets reserve headroom of skb.
> + * @tx_chunk_len: Optional routine that sets chunk len to split.
> * @tx_poll: Optional routine that sets additional TX poll flags.
> *
> * The wwan_port_ops structure contains a list of low-level operations
> @@ -58,6 +60,8 @@ struct wwan_port_ops {
>
> /* Optional operations */
> int (*tx_blocking)(struct wwan_port *port, struct sk_buff *skb);
> + size_t (*needed_headroom)(struct wwan_port *port);
> + size_t (*tx_chunk_len)(struct wwan_port *port);
As said above, maybe move that as variables, or parameter of wwan_create_port.
Regards,
Loic
Hi Loic
On Tue, 2022-11-08 at 13:14 +0100, Loic Poulain wrote:
> Hi Haozhe,
>
> On Tue, 8 Nov 2022 at 11:54, <[email protected]> wrote:
> >
> > From: haozhe chang <[email protected]>
> >
> > 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 chunk) or not. By
> > doing so, the additional memory copy is reduced.
> >
> > Meanwhile, this patch gives WWAN devices driver the option to
> > reserve
> > headroom in SKB for the device-specific metadata.
> >
> > Signed-off-by: haozhe chang <[email protected]>
> >
> > ---
> > Changes in v2
> > -send fragments to device driver by skb frag_list.
> > ---
> > drivers/net/wwan/t7xx/t7xx_port_wwan.c | 42 ++++++++++-------
> > drivers/net/wwan/wwan_core.c | 65 ++++++++++++++++++++
> > ------
> > include/linux/wwan.h | 5 +-
> > 3 files changed, 80 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> > b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> > index 33931bfd78fd..74fa58575d5a 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 const struct wwan_port_ops wwan_ops = {
> > .start = t7xx_port_ctrl_start,
> > .stop = t7xx_port_ctrl_stop,
> > .tx = t7xx_port_ctrl_tx,
> > + .needed_headroom = t7xx_port_tx_headroom,
> > + .tx_chunk_len = t7xx_port_tx_chunk_len,
>
> Can you replace 'chunk' with 'frag' everywhere?
>
OK
> > };
> >
> > static int t7xx_port_wwan_init(struct t7xx_port *port)
> > diff --git a/drivers/net/wwan/wwan_core.c
> > b/drivers/net/wwan/wwan_core.c
> > index 62e9f7d6c9fe..ed78471f9e38 100644
> > --- a/drivers/net/wwan/wwan_core.c
> > +++ b/drivers/net/wwan/wwan_core.c
> > @@ -20,7 +20,7 @@
> > #include <uapi/linux/wwan.h>
> >
> > /* Maximum number of minors in use */
> > -#define WWAN_MAX_MINORS (1 << MINORBITS)
> > +#define WWAN_MAX_MINORS BIT(MINORBITS)
> >
> > static DEFINE_MUTEX(wwan_register_lock); /* WWAN device
> > create|remove lock */
> > static DEFINE_IDA(minors); /* minors for WWAN port chardevs */
> > @@ -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
> > + * @chunk_len: Chunk len 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 chunk_len;
> > union {
> > struct {
> > struct ktermios termios;
> > @@ -550,8 +554,13 @@ static int wwan_port_op_start(struct wwan_port
> > *port)
> > }
> >
> > /* If port is already started, don't start again */
> > - if (!port->start_count)
> > + if (!port->start_count) {
> > ret = port->ops->start(port);
> > + if (port->ops->tx_chunk_len)
> > + port->chunk_len = port->ops-
> > >tx_chunk_len(port);
>
> So, maybe frag len and headroom should be parameters of
> wwan_create_port() instead of port ops, as we really need this info
> only once.
>
If frag_len and headroom are added, wwan_create_port will have 6
parameters, is it too much? And for similar requirements in the future,
it may be difficult to add more parameters.
Is it a good solution to provide wwan_port_set_frag_len and
wwan_port_set_headroom_len to the device driver? if so, the device
driver has a chance to modify the wwan port's field after calling
wwan_create_port.
> > + if (port->ops->needed_headroom)
> > + port->headroom_len = port->ops-
> > >needed_headroom(port);
> > + }
> >
> > if (!ret)
> > port->start_count++;
> > @@ -698,30 +707,56 @@ 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)
> > {
> > + size_t len, chunk_len, offset, allowed_chunk_len;
> > + struct sk_buff *skb, *head = NULL, *tail = NULL;
> > struct wwan_port *port = filp->private_data;
> > - struct sk_buff *skb;
> > 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;
> > + allowed_chunk_len = port->chunk_len ? port->chunk_len :
> > count;
>
> I would suggest making port->chunk_len (frag_len) always valid, by
> setting it to -1 (MAX size_t) when creating a port without frag_len
> requirement.
>
Ok, it will help to reduce some code.
> > + for (offset = 0; offset < count; offset += chunk_len) {
> > + chunk_len = min(count - offset, allowed_chunk_len);
> > + len = chunk_len + port->headroom_len;
> > + skb = alloc_skb(len, GFP_KERNEL);
>
> That works but would prefer a simpler solution like:
> do {
> len = min(port->frag_len, remain);
> skb = alloc_skb(len + port->needed_headroom; GFP_KERNEL);
> [...]
> copy_from_user(skb_put(skb, len), buf + count - remain)
> } while ((remain -= len));
>
May I know if the suggestion is because "while" is more efficient
than "for", or is it more readable?
> > + 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, chunk_len), buf +
> > offset, chunk_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;
> > + }
> > }
> >
> > - return count;
> > + if (head) {
>
> How head can be null here?
>
if the parameter "count" is 0, the for loop will not be executed.
> > + ret = wwan_port_op_tx(port, head, !!(filp->f_flags
> > & O_NONBLOCK));
> > + if (!ret)
> > + return count;
> > + }
> > +freeskb:
> > + kfree_skb(head);
> > + return ret;
> > }
> >
> > static __poll_t wwan_port_fops_poll(struct file *filp, poll_table
> > *wait)
> > diff --git a/include/linux/wwan.h b/include/linux/wwan.h
> > index 5ce2acf444fb..bdeeef59bbfd 100644
> > --- a/include/linux/wwan.h
> > +++ b/include/linux/wwan.h
> > @@ -46,6 +46,8 @@ struct wwan_port;
> > * @tx: Non-blocking routine that sends WWAN port protocol data to
> > the device.
> > * @tx_blocking: Optional blocking routine that sends WWAN port
> > protocol data
> > * to the device.
> > + * @needed_headroom: Optional routine that sets reserve headroom
> > of skb.
> > + * @tx_chunk_len: Optional routine that sets chunk len to split.
> > * @tx_poll: Optional routine that sets additional TX poll flags.
> > *
> > * The wwan_port_ops structure contains a list of low-level
> > operations
> > @@ -58,6 +60,8 @@ struct wwan_port_ops {
> >
> > /* Optional operations */
> > int (*tx_blocking)(struct wwan_port *port, struct sk_buff
> > *skb);
> > + size_t (*needed_headroom)(struct wwan_port *port);
> > + size_t (*tx_chunk_len)(struct wwan_port *port);
>
> As said above, maybe move that as variables, or parameter of
> wwan_create_port.
>
> Regards,
> Loic
On Wed, 9 Nov 2022 at 12:23, Haozhe Chang (常浩哲)
<[email protected]> wrote:
>
> Hi Loic
>
> On Tue, 2022-11-08 at 13:14 +0100, Loic Poulain wrote:
> > Hi Haozhe,
> >
> > On Tue, 8 Nov 2022 at 11:54, <[email protected]> wrote:
> > >
> > > From: haozhe chang <[email protected]>
> > >
> > > 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 chunk) or not. By
> > > doing so, the additional memory copy is reduced.
> > >
> > > Meanwhile, this patch gives WWAN devices driver the option to
> > > reserve
> > > headroom in SKB for the device-specific metadata.
> > >
> > > Signed-off-by: haozhe chang <[email protected]>
> > >
> > > ---
> > > Changes in v2
> > > -send fragments to device driver by skb frag_list.
> > > ---
> > > drivers/net/wwan/t7xx/t7xx_port_wwan.c | 42 ++++++++++-------
> > > drivers/net/wwan/wwan_core.c | 65 ++++++++++++++++++++
> > > ------
> > > include/linux/wwan.h | 5 +-
> > > 3 files changed, 80 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> > > b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> > > index 33931bfd78fd..74fa58575d5a 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 const struct wwan_port_ops wwan_ops = {
> > > .start = t7xx_port_ctrl_start,
> > > .stop = t7xx_port_ctrl_stop,
> > > .tx = t7xx_port_ctrl_tx,
> > > + .needed_headroom = t7xx_port_tx_headroom,
> > > + .tx_chunk_len = t7xx_port_tx_chunk_len,
> >
> > Can you replace 'chunk' with 'frag' everywhere?
> >
> OK
> > > };
> > >
> > > static int t7xx_port_wwan_init(struct t7xx_port *port)
> > > diff --git a/drivers/net/wwan/wwan_core.c
> > > b/drivers/net/wwan/wwan_core.c
> > > index 62e9f7d6c9fe..ed78471f9e38 100644
> > > --- a/drivers/net/wwan/wwan_core.c
> > > +++ b/drivers/net/wwan/wwan_core.c
> > > @@ -20,7 +20,7 @@
> > > #include <uapi/linux/wwan.h>
> > >
> > > /* Maximum number of minors in use */
> > > -#define WWAN_MAX_MINORS (1 << MINORBITS)
> > > +#define WWAN_MAX_MINORS BIT(MINORBITS)
> > >
> > > static DEFINE_MUTEX(wwan_register_lock); /* WWAN device
> > > create|remove lock */
> > > static DEFINE_IDA(minors); /* minors for WWAN port chardevs */
> > > @@ -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
> > > + * @chunk_len: Chunk len 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 chunk_len;
> > > union {
> > > struct {
> > > struct ktermios termios;
> > > @@ -550,8 +554,13 @@ static int wwan_port_op_start(struct wwan_port
> > > *port)
> > > }
> > >
> > > /* If port is already started, don't start again */
> > > - if (!port->start_count)
> > > + if (!port->start_count) {
> > > ret = port->ops->start(port);
> > > + if (port->ops->tx_chunk_len)
> > > + port->chunk_len = port->ops-
> > > >tx_chunk_len(port);
> >
> > So, maybe frag len and headroom should be parameters of
> > wwan_create_port() instead of port ops, as we really need this info
> > only once.
> >
> If frag_len and headroom are added, wwan_create_port will have 6
> parameters, is it too much? And for similar requirements in the future,
> it may be difficult to add more parameters.
I think 6 is still fine, if we need more fields in the future we can
always have a port_config struct as a parameter instead.
>
> Is it a good solution to provide wwan_port_set_frag_len and
> wwan_port_set_headroom_len to the device driver? if so, the device
> driver has a chance to modify the wwan port's field after calling
> wwan_create_port.
Would be preferable to not have these values changeable at runtime.
> > > + if (port->ops->needed_headroom)
> > > + port->headroom_len = port->ops-
> > > >needed_headroom(port);
> > > + }
> > >
> > > if (!ret)
> > > port->start_count++;
> > > @@ -698,30 +707,56 @@ 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)
> > > {
> > > + size_t len, chunk_len, offset, allowed_chunk_len;
> > > + struct sk_buff *skb, *head = NULL, *tail = NULL;
> > > struct wwan_port *port = filp->private_data;
> > > - struct sk_buff *skb;
> > > 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;
> > > + allowed_chunk_len = port->chunk_len ? port->chunk_len :
> > > count;
> >
> > I would suggest making port->chunk_len (frag_len) always valid, by
> > setting it to -1 (MAX size_t) when creating a port without frag_len
> > requirement.
> >
> Ok, it will help to reduce some code.
> > > + for (offset = 0; offset < count; offset += chunk_len) {
> > > + chunk_len = min(count - offset, allowed_chunk_len);
> > > + len = chunk_len + port->headroom_len;
> > > + skb = alloc_skb(len, GFP_KERNEL);
> >
> > That works but would prefer a simpler solution like:
> > do {
> > len = min(port->frag_len, remain);
> > skb = alloc_skb(len + port->needed_headroom; GFP_KERNEL);
> > [...]
> > copy_from_user(skb_put(skb, len), buf + count - remain)
> > } while ((remain -= len));
> >
> May I know if the suggestion is because "while" is more efficient
> than "for", or is it more readable?
It's more readable to me, but it's a subjective opinion here.
> > > + 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, chunk_len), buf +
> > > offset, chunk_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;
> > > + }
> > > }
> > >
> > > - return count;
> > > + if (head) {
> >
> > How head can be null here?
> >
> if the parameter "count" is 0, the for loop will not be executed.
Ok, right (with do/while version it should not happen).
But maybe the !count case should be caught earlier, if even possible in fops.
Regards,
Loic