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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 10 Oct 2022 19:52:49 +0530
From:   "Kancharla, Sreehari" <sreehari.kancharla@...ux.intel.com>
To:     Loic Poulain <loic.poulain@...aro.org>
Cc:     netdev@...r.kernel.org, kuba@...nel.org, davem@...emloft.net,
        johannes@...solutions.net, ryazanov.s.a@...il.com,
        m.chetan.kumar@...el.com, chandrashekar.devegowda@...el.com,
        linuxwwan@...el.com, chiranjeevi.rapolu@...ux.intel.com,
        haijun.liu@...iatek.com, ricardo.martinez@...ux.intel.com,
        andriy.shevchenko@...ux.intel.com, dinesh.sharma@...el.com,
        ilpo.jarvinen@...ux.intel.com, moises.veleta@...el.com,
        sreehari.kancharla@...el.com
Subject: Re: [PATCH net-next 2/2] net: wwan: t7xx: Add NAPI support

Hi Loic,

On 9/12/2022 6:23 PM, Loic Poulain wrote:
> Hi Sreehari,
>
>
> On Fri, 9 Sept 2022 at 18:40, Sreehari Kancharla
> <sreehari.kancharla@...ux.intel.com> wrote:
>> From: Haijun Liu <haijun.liu@...iatek.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@...iatek.com>
>> Co-developed-by: Sreehari Kancharla <sreehari.kancharla@...ux.intel.com>
>> Signed-off-by: Sreehari Kancharla <sreehari.kancharla@...ux.intel.com>
>> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@...el.com>
>> Acked-by: Ricardo Martinez <ricardo.martinez@...ux.intel.com>
>> Acked-by: M Chetan Kumar <m.chetan.kumar@...ux.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).

Device will be in resumed state when NAPI poll is invoked from IRQ context,
but host/driver can trigger RPM suspend to device. so we are using pm_runtime_resume_and_get
here to prevent runtime suspend.

>
>> +               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.

we are using device lock inside the NAPI poll to prevent device to
enter into low power mode when there are pending RX. once packet is
collected we release the device lock so that device can go to low power mode.

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

Regards,
Sreehari