Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <82a7acf3176c90d9bea773bb4ea365745c1a1971.camel@mediatek.com>
Subject: Re: [PATCH] wwan: core: Support slicing in port TX flow of WWAN
 subsystem
From:   haozhe chang <haozhe.chang@mediatek.com>
To:     Loic Poulain <loic.poulain@linaro.org>,
        "chandrashekar.devegowda@intel.com" 
        <chandrashekar.devegowda@intel.com>,
        "linuxwwan@intel.com" <linuxwwan@intel.com>,
        "chiranjeevi.rapolu@linux.intel.com" 
        <chiranjeevi.rapolu@linux.intel.com>,
        Haijun Liu =?UTF-8?Q?=28=E5=88=98=E6=B5=B7=E5=86=9B=29?= 
        <haijun.liu@mediatek.com>,
        "m.chetan.kumar@linux.intel.com" <m.chetan.kumar@linux.intel.com>,
        "ricardo.martinez@linux.intel.com" <ricardo.martinez@linux.intel.com>,
        "ryazanov.s.a@gmail.com" <ryazanov.s.a@gmail.com>,
        "johannes@sipsolutions.net" <johannes@sipsolutions.net>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     Lambert Wang =?UTF-8?Q?=28=E7=8E=8B=E4=BC=9F=29?= 
        <Lambert.Wang@mediatek.com>,
        "Xiayu Zhang =?UTF-8?Q?=28=E5=BC=A0=E5=A4=8F=E5=AE=87=29?=" 
        <Xiayu.Zhang@mediatek.com>, <srv_heupstream@mediatek.com>
Date:   Wed, 26 Oct 2022 19:45:17 +0800
In-Reply-To: <CAMZdPi_XSWeTf-eP+O2ZXGXtn5yviEp=p1Q0rs_fG76UGf2FsQ@mail.gmail.com>
References: <20221026011540.8499-1-haozhe.chang@mediatek.com>
         <CAMZdPi_XSWeTf-eP+O2ZXGXtn5yviEp=p1Q0rs_fG76UGf2FsQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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,
> >  };
> >