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;
MIME-Version: 1.0
References: <20221026011540.8499-1-haozhe.chang@mediatek.com>
In-Reply-To: <20221026011540.8499-1-haozhe.chang@mediatek.com>
From:   Loic Poulain <loic.poulain@linaro.org>
Date:   Wed, 26 Oct 2022 09:28:57 +0200
Message-ID: <CAMZdPi_XSWeTf-eP+O2ZXGXtn5yviEp=p1Q0rs_fG76UGf2FsQ@mail.gmail.com>
Subject: Re: [PATCH] wwan: core: Support slicing in port TX flow of WWAN subsystem
To:     haozhe.chang@mediatek.com
Cc:     chandrashekar.devegowda@intel.com, linuxwwan@intel.com,
        chiranjeevi.rapolu@linux.intel.com, haijun.liu@mediatek.com,
        m.chetan.kumar@linux.intel.com, ricardo.martinez@linux.intel.com,
        ryazanov.s.a@gmail.com, johannes@sipsolutions.net,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, lambert.wang@mediatek.com,
        xiayu.zhang@mediatek.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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?

>
> 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?

> +       .get_tx_chunk_len = t7xx_port_get_tx_chunk_len,
>  };
>