Re: [PATCH net-next 2/2] net: wwan: t7xx: Add NAPI support

[Loic Poulain <loic.poulain@linaro.org>]

Hi Sreehari,


On Fri, 9 Sept 2022 at 18:40, Sreehari Kancharla
<sreehari.kancharla@linux.intel.com> wrote:
>
> From: Haijun Liu <haijun.liu@mediatek.com>
>
> Replace the work queue based RX flow with a NAPI implementation
> Remove rx_thread and dpmaif_rxq_work.
> Introduce dummy network device. its responsibility is
>     - Binds one NAPI object for each DL HW queue and acts as
>       the agent of all those network devices.
>     - Use NAPI object to poll DL packets.
>     - Helps to dispatch each packet to the network interface.
>
> Signed-off-by: Haijun Liu <haijun.liu@mediatek.com>
> Co-developed-by: Sreehari Kancharla <sreehari.kancharla@linux.intel.com>
> Signed-off-by: Sreehari Kancharla <sreehari.kancharla@linux.intel.com>
> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com>
> Acked-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
> Acked-by: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
> ---
>  drivers/net/wwan/t7xx/t7xx_hif_dpmaif.h    |  14 +-
>  drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c | 220 +++++++--------------
>  drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.h |   1 +
>  drivers/net/wwan/t7xx/t7xx_netdev.c        |  93 ++++++++-
>  drivers/net/wwan/t7xx/t7xx_netdev.h        |   5 +
>  5 files changed, 167 insertions(+), 166 deletions(-)
>
> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.h b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.h
> index 1225ca0ed51e..0ce4505e813d 100644
> --- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.h
> +++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif.h
> @@ -20,6 +20,7 @@

[...]

> -static void t7xx_dpmaif_rxq_work(struct work_struct *work)
> +int t7xx_dpmaif_napi_rx_poll(struct napi_struct *napi, const int budget)
>  {
> -       struct dpmaif_rx_queue *rxq = container_of(work, struct dpmaif_rx_queue, dpmaif_rxq_work);
> -       struct dpmaif_ctrl *dpmaif_ctrl = rxq->dpmaif_ctrl;
> -       int ret;
> +       struct dpmaif_rx_queue *rxq = container_of(napi, struct dpmaif_rx_queue, napi);
> +       struct t7xx_pci_dev *t7xx_dev = rxq->dpmaif_ctrl->t7xx_dev;
> +       int ret, once_more = 0, work_done = 0;
>
>         atomic_set(&rxq->rx_processing, 1);
>         /* Ensure rx_processing is changed to 1 before actually begin RX flow */
> @@ -917,22 +840,54 @@ static void t7xx_dpmaif_rxq_work(struct work_struct *work)
>
>         if (!rxq->que_started) {
>                 atomic_set(&rxq->rx_processing, 0);
> -               dev_err(dpmaif_ctrl->dev, "Work RXQ: %d has not been started\n", rxq->index);
> -               return;
> +               dev_err(rxq->dpmaif_ctrl->dev, "Work RXQ: %d has not been started\n", rxq->index);
> +               return work_done;
>         }
>
> -       ret = pm_runtime_resume_and_get(dpmaif_ctrl->dev);
> -       if (ret < 0 && ret != -EACCES)
> -               return;
> +       if (!rxq->sleep_lock_pending) {
> +               ret = pm_runtime_resume_and_get(rxq->dpmaif_ctrl->dev);

AFAIK, polling is not called in a context allowing you to sleep (e.g.
performing a synced pm runtime operation).

> +               if (ret < 0 && ret != -EACCES)
> +                       return work_done;
>
> -       t7xx_pci_disable_sleep(dpmaif_ctrl->t7xx_dev);
> -       if (t7xx_pci_sleep_disable_complete(dpmaif_ctrl->t7xx_dev))
> -               t7xx_dpmaif_do_rx(dpmaif_ctrl, rxq);
> +               t7xx_pci_disable_sleep(t7xx_dev);
> +       }
>
> -       t7xx_pci_enable_sleep(dpmaif_ctrl->t7xx_dev);
> -       pm_runtime_mark_last_busy(dpmaif_ctrl->dev);
> -       pm_runtime_put_autosuspend(dpmaif_ctrl->dev);
> +       ret = try_wait_for_completion(&t7xx_dev->sleep_lock_acquire);

The logic seems odd, why not simply scheduling napi polling when you
are really ready to handle it, i.e when you have awake condition & rx
ready.

> +       if (!ret) {
> +               napi_complete_done(napi, work_done);
> +               rxq->sleep_lock_pending = true;
> +               napi_reschedule(napi);
> +               return work_done;
> +       }
> +

[...]

Regards,
Loic