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

[haozhe chang <haozhe.chang@mediatek.com>]

On Wed, 2022-10-26 at 15:28 +0800, Loic Poulain wrote:
> Hi Haozhe,
> 
> On Wed, 26 Oct 2022 at 03:16, <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.
> 
> The benefit of putting data in an skb is that it is easy to
> manipulate, so not sure why there is an additional copy in the first
> place. Isn't possible for the t7xx driver to consume the skb
> progressively (without intermediate copy), according to its own MTU
> limitation?
> 
t7xx driver needs to add metadata to the SKB head for each fragment, so
the driver has to allocate a new buffer to copy data(skb_put_data) and
insert metadata. 
Providing the option to slice in common layer benefits varieties of
devices with different DMA capabilities. The patch is also compatible
with existing WWAN devices.
> > 
> > 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>
> > ---
> >  drivers/net/wwan/t7xx/t7xx_port_wwan.c | 41 ++++++++++++--------
> > ---
> >  drivers/net/wwan/wwan_core.c           | 45 ++++++++++++++++++--
> > ------
> >  include/linux/wwan.h                   |  5 ++-
> >  3 files changed, 56 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> > b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> > index 33931bfd78fd..5e8589582121 100644
> > --- a/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> > +++ b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> > @@ -54,13 +54,12 @@ 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 t7xx_fsm_ctl *ctl;
> >         enum md_state md_state;
> > +       int 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 +71,33 @@ 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);
> > -               if (ret) {
> > -                       dev_kfree_skb_any(skb_ccci);
> > -                       dev_err(port_private->dev, "Write error on
> > %s port, %d\n",
> > -                               port_conf->name, ret);
> > -                       return ret;
> > -               }
> > +       ret = t7xx_port_send_skb(port_private, skb, 0, 0);
> > +       if (ret) {
> > +               dev_err(port_private->dev, "Write error on %s port,
> > %d\n",
> > +                       port_conf->name, ret);
> > +               return ret;
> >         }
> > -
> >         dev_kfree_skb(skb);
> > +
> >         return 0;
> >  }
> > 
> > +static size_t t7xx_port_get_tx_rsvd_headroom(struct wwan_port
> > *port)
> > +{
> > +       return sizeof(struct ccci_header);
> > +}
> > +
> > +static size_t t7xx_port_get_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,
> > +       .get_tx_rsvd_headroom = t7xx_port_get_tx_rsvd_headroom,
> 
> Can't we have a simple 'skb_headroom' or 'needed_headroom' member
> here?
> 
OK, I will change it in patch v2.
> > +       .get_tx_chunk_len = t7xx_port_get_tx_chunk_len,
> >  };
> >