linux-kernel - Re: [PATCH 2/2] mailbox: mtk-cmdq: Move pm_runimte_get and put to mbox_chan

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 linux-cve-announce 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]

Message-ID: <CABb+yY0mYrUyfpNLVKZt2aDdzZPf+VjawcwVSpMG8H_=pwKLbw@mail.gmail.com>
Date: Wed, 10 Jul 2024 22:47:31 -0500
From: Jassi Brar <jassisinghbrar@...il.com>
To: Jason-JH Lin (林睿祥) <Jason-JH.Lin@...iatek.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>, 
	Singo Chang (張興國) <Singo.Chang@...iatek.com>, 
	"chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>, Nancy Lin (林欣螢) <Nancy.Lin@...iatek.com>, 
	Project_Global_Chrome_Upstream_Group <Project_Global_Chrome_Upstream_Group@...iatek.com>, 
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, 
	"matthias.bgg@...il.com" <matthias.bgg@...il.com>, 
	"angelogioacchino.delregno@...labora.com" <angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH 2/2] mailbox: mtk-cmdq: Move pm_runimte_get and put to
 mbox_chan_ops API

On Wed, Jul 10, 2024 at 9:00 PM Jason-JH Lin (林睿祥)
<Jason-JH.Lin@...iatek.com> wrote:
>
> On Fri, 2024-07-05 at 11:43 -0500, Jassi Brar wrote:
> >
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >  On Fri, Jul 5, 2024 at 1:11 AM Jason-JH Lin (林睿祥)
> > <Jason-JH.Lin@...iatek.com> wrote:
> > >
> > > [snip]
> > >
> > > > You may disable one to make it easy to capture.
> > > > Make sure your kernel prints have timestamps.
> > > > Over your patchset, apply the following diff and execute your
> > > > usecase.
> > > > Then share
> > > >  $ dmesg | grep Jason
> > > >
> > > > diff --git a/drivers/mailbox/mailbox.c
> > b/drivers/mailbox/mailbox.c
> > > > index bafcc7b0c0b8d..90c0620c0ae63 100644
> > > > --- a/drivers/mailbox/mailbox.c
> > > > +++ b/drivers/mailbox/mailbox.c
> > > > @@ -284,6 +284,7 @@ int mbox_send_message(struct mbox_chan *chan,
> > > > void *mssg)
> > > >  {
> > > >   int t;
> > > >
> > > > + printk("Jason %s: %p\n", __func__, (void *)chan);
> > > >   if (!chan || !chan->cl)
> > > >   return -EINVAL;
> > > >
> > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> > > > b/drivers/mailbox/mtk-cmdq-mailbox.c
> > > > index 02cef3eee35a5..6436e1b22f353 100644
> > > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > > > @@ -317,6 +317,7 @@ static int cmdq_runtime_resume(struct device
> > > > *dev)
> > > >  {
> > > >   struct cmdq *cmdq = dev_get_drvdata(dev);
> > > >
> > > > + printk("Jason %s: %p\n", __func__, (void *)dev);
> > > >   return clk_bulk_enable(cmdq->pdata->gce_num, cmdq->clocks);
> > > >  }
> > > >
> > > > @@ -324,6 +325,7 @@ static int cmdq_runtime_suspend(struct device
> > > > *dev)
> > > >  {
> > > >   struct cmdq *cmdq = dev_get_drvdata(dev);
> > > >
> > > > + printk("Jason %s: %p\n", __func__, (void *)dev);
> > > >   clk_bulk_disable(cmdq->pdata->gce_num, cmdq->clocks);
> > > >   return 0;
> > > >  }
> > > >
> > > > Thanks
> > >
> > > It seems CMDQ driver is not suspend over 100ms.
> > >
> > here are the time deltas in milli-seconds
> > 10 6005 52 85 304 59 27 203 29 24 25 38 33 57 34 20 41 28 36 48 71 35
> > 21 45 51 22 30 46 46 23 16 16 16 16 16 17 17 31 16 19 31 16 16 17 16
> > 16 16 32 17 32 17 30 21 16 32 18 30 28 25 29 24 31 27 16 17 19 34 30
> > 15 33 16 35 35 29 18 31 16 16 16 16 17 33 35 33 39 20 21 14 34 49 22
> > 42 32 13 21 24 9 130 23 32 35 31 32 16 18 31 36 37 26 14 68 76 111
> > 515
> > 452 64 484 487 497 501 507 161 336 516 486 495 504 495 504 501 500
> > 495
> > 507 495 499 505 496 502 501 502 500 495 501 507 499 499 496 506 501
> > 498 507 490 504 496 501 506 413 28 207 31731 34550 3784 30 23
> >
> > For detlas <100 we don't expect suspend because your set
> > autosuspend_delay to 100ms.
> > For deltas >100 you should have seen suspend. Apparently the changes
> > you want in the api still don't help you.
>
> I think "deltas > 100 ms not autosuspend" and "calling
> pm_runtime_get_sync() in atomic context" are 2 different problems.
>
Fixing the first may avoid the need of the second on your platform.

> >
> > Also I see messages are being submitted on 4 channels, but only for
> > the  '000000000a4d37b5' channel cmdq_mbox_send_data is ever called.
> > Unless you selectively printk only for that channel, your driver has
> > some even fundamental problems.  Maybe trace without your patch and
> > with pm_runtime_xxx commented out (to avoid stack dump).
>
> I've confirmed that autosuspend didn't work without these API patches.
>
You mean _with_ the api patches. right? Because I had suggested "Over
your patchset, apply the following diff ..."

cheers.