| Message ID | 20221108105352.89801-1-haozhe.chang@mediatek.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] wwan: core: Support slicing in port TX flow of WWAN subsystem | expand |
Hi Haozhe, On Tue, 8 Nov 2022 at 11:54, <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 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 <haozhe.chang@mediatek.com> > > --- > 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, <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 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 <haozhe.chang@mediatek.com> > > > > --- > > 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 (常浩哲) <Haozhe.Chang@mediatek.com> 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, <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 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 <haozhe.chang@mediatek.com> > > > > > > --- > > > 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
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