Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201022094152.17662-1-houlong.wei@mediatek.com>
In-Reply-To: <20201022094152.17662-1-houlong.wei@mediatek.com>
From:   Chun-Kuang Hu <chunkuang.hu@kernel.org>
Date:   Mon, 2 Nov 2020 08:44:16 +0800
Message-ID: <CAAOTY_95Y9Jfq3FmhVa6QG7DdTWOOuekpvQj9OuRZ6GjHnVTKQ@mail.gmail.com>
Subject: Re: [PATCH] soc: mediatek: cmdq: fixup possible timeout issue
To:     Houlong Wei <houlong.wei@mediatek.com>
Cc:     Matthias Brugger <matthias.bgg@gmail.com>,
        Jassi Brar <jassisinghbrar@gmail.com>,
        Nicolas Boichat <drinkcat@chromium.org>,
        Yongqiang Niu <yongqiang.niu@mediatek.com>,
        srv_heupstream <srv_heupstream@mediatek.com>,
        Daoyuan Huang <daoyuan.huang@mediatek.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Dennis-YC Hsieh <dennis-yc.hsieh@mediatek.com>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@lists.infradead.org>,
        Bibby Hsieh <bibby.hsieh@mediatek.com>,
        CK HU <ck.hu@mediatek.com>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        ginny.chen@mediatek.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi, Houlong:

Houlong Wei <houlong.wei@mediatek.com> =E6=96=BC 2020=E5=B9=B410=E6=9C=8822=
=E6=97=A5 =E9=80=B1=E5=9B=9B =E4=B8=8B=E5=8D=885:55=E5=AF=AB=E9=81=93=EF=BC=
=9A
>
> Fixes: 576f1b4bc802 ("soc: mediatek: Add Mediatek CMDQ helper")
>
> There may be possible timeout issue when lots of cmdq packets are
> flushed to the same cmdq client. The necessary modifications are as
> below.
> 1.Adjust the timer timeout period as client->timeout_ms * client->pkt_cnt=
.
> 2.Optimize the time to start the timer.
>
> Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-cmdq-helper.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediate=
k/mtk-cmdq-helper.c
> index dc644cfb6419..31142c193527 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -350,7 +350,8 @@ static void cmdq_pkt_flush_async_cb(struct cmdq_cb_da=
ta data)
>                         del_timer(&client->timer);
>                 else
>                         mod_timer(&client->timer, jiffies +
> -                                 msecs_to_jiffies(client->timeout_ms));
> +                                 msecs_to_jiffies(client->timeout_ms *
> +                                                  client->pkt_cnt));
>                 spin_unlock_irqrestore(&client->lock, flags);
>         }
>
> @@ -379,9 +380,7 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_a=
sync_flush_cb cb,
>
>         if (client->timeout_ms !=3D CMDQ_NO_TIMEOUT) {
>                 spin_lock_irqsave(&client->lock, flags);
> -               if (client->pkt_cnt++ =3D=3D 0)
> -                       mod_timer(&client->timer, jiffies +
> -                                 msecs_to_jiffies(client->timeout_ms));
> +               client->pkt_cnt++;
>                 spin_unlock_irqrestore(&client->lock, flags);
>         }
>
> @@ -391,6 +390,21 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_=
async_flush_cb cb,
>         /* We can send next packet immediately, so just call txdone. */
>         mbox_client_txdone(client->chan, 0);
>
> +       if (client->timeout_ms !=3D CMDQ_NO_TIMEOUT) {
> +               spin_lock_irqsave(&client->lock, flags);
> +               /*
> +                * GCE HW maybe execute too quickly and the callback func=
tion
> +                * may be invoked earlier. If this happens, pkt_cnt is re=
duced
> +                * by 1 in cmdq_pkt_flush_async_cb(). The timer is set on=
ly if
> +                * pkt_cnt is greater than 0.
> +                */
> +               if (client->pkt_cnt > 0)
> +                       mod_timer(&client->timer, jiffies +
> +                                 msecs_to_jiffies(client->timeout_ms *
> +                                                  client->pkt_cnt));

I think for some client, it care about execution time of one packet,
but some client care about total execution time of all packets, so we
should let client driver implement its own timeout handler and remove
handler in cmdq helper [1].

[1] https://patchwork.kernel.org/project/linux-mediatek/patch/2020110200043=
8.29225-1-chunkuang.hu@kernel.org/

Regards,
Chun-Kuang.

> +               spin_unlock_irqrestore(&client->lock, flags);
> +       }
> +
>         return 0;
>  }
>  EXPORT_SYMBOL(cmdq_pkt_flush_async);
> --
> 2.18.0
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek