linux-kernel - Re: [PATCH v4 1/3] bus: mhi: core: Introduce internal register poll helper function

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: <43c83caf8a6b71207b107ac8457f22d6@codeaurora.org>
Date:   Wed, 17 Mar 2021 14:26:48 -0700
From:   Bhaumik Bhatt <bbhatt@...eaurora.org>
To:     Jeffrey Hugo <jhugo@...eaurora.org>
Cc:     Loic Poulain <loic.poulain@...aro.org>,
        Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Hemant Kumar <hemantk@...eaurora.org>,
        open list <linux-kernel@...r.kernel.org>,
        Carl Yin(殷张成) 
        <carl.yin@...ctel.com>, Naveen Kumar <naveen.kumar@...ctel.com>,
        jhugo=codeaurora.org@...eaurora.org
Subject: Re: [PATCH v4 1/3] bus: mhi: core: Introduce internal register poll
 helper function

On 2021-03-11 11:59 AM, Jeffrey Hugo wrote:
> On 3/11/2021 1:00 AM, Loic Poulain wrote:
>> Hi Bhaumik,
>> 
>> On Thu, 11 Mar 2021 at 00:31, Bhaumik Bhatt <bbhatt@...eaurora.org> 
>> wrote:
>>> 
>>> Introduce helper function to allow MHI core driver to poll for
>>> a value in a register field. This helps reach a common path to
>>> read and poll register values along with a retry time interval.
>>> 
>>> Signed-off-by: Bhaumik Bhatt <bbhatt@...eaurora.org>
>>> ---
>>>   drivers/bus/mhi/core/internal.h |  3 +++
>>>   drivers/bus/mhi/core/main.c     | 23 +++++++++++++++++++++++
>>>   2 files changed, 26 insertions(+)
>>> 
>>> diff --git a/drivers/bus/mhi/core/internal.h 
>>> b/drivers/bus/mhi/core/internal.h
>>> index 6f80ec3..005286b 100644
>>> --- a/drivers/bus/mhi/core/internal.h
>>> +++ b/drivers/bus/mhi/core/internal.h
>>> @@ -643,6 +643,9 @@ int __must_check mhi_read_reg(struct 
>>> mhi_controller *mhi_cntrl,
>>>   int __must_check mhi_read_reg_field(struct mhi_controller 
>>> *mhi_cntrl,
>>>                                      void __iomem *base, u32 offset, 
>>> u32 mask,
>>>                                      u32 shift, u32 *out);
>>> +int __must_check mhi_poll_reg_field(struct mhi_controller 
>>> *mhi_cntrl,
>>> +                                   void __iomem *base, u32 offset, 
>>> u32 mask,
>>> +                                   u32 shift, u32 val, u32 delayus);
>>>   void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem 
>>> *base,
>>>                     u32 offset, u32 val);
>>>   void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void 
>>> __iomem *base,
>>> diff --git a/drivers/bus/mhi/core/main.c 
>>> b/drivers/bus/mhi/core/main.c
>>> index 4e0131b..7c7f41a 100644
>>> --- a/drivers/bus/mhi/core/main.c
>>> +++ b/drivers/bus/mhi/core/main.c
>>> @@ -4,6 +4,7 @@
>>>    *
>>>    */
>>> 
>>> +#include <linux/delay.h>
>>>   #include <linux/device.h>
>>>   #include <linux/dma-direction.h>
>>>   #include <linux/dma-mapping.h>
>>> @@ -37,6 +38,28 @@ int __must_check mhi_read_reg_field(struct 
>>> mhi_controller *mhi_cntrl,
>>>          return 0;
>>>   }
>>> 
>>> +int __must_check mhi_poll_reg_field(struct mhi_controller 
>>> *mhi_cntrl,
>>> +                                   void __iomem *base, u32 offset,
>>> +                                   u32 mask, u32 shift, u32 val, u32 
>>> delayus)
>>> +{
>>> +       int ret;
>>> +       u32 out, retry = (mhi_cntrl->timeout_ms * 1000) / delayus;
>>> +
>>> +       while (retry--) {
>>> +               ret = mhi_read_reg_field(mhi_cntrl, base, offset, 
>>> mask, shift,
>>> +                                        &out);
>>> +               if (ret)
>>> +                       return ret;
>>> +
>>> +               if (out == val)
>>> +                       return 0;
>>> +
>>> +               udelay(delayus);
>> 
>> Have you read my previous comment?
>> Do you really want to risk hogging the CPU for several seconds? we
>> know that some devices take several seconds to start/boot.
>> Why not using msleep variant here?
> 
> usleep_range() if there is a desire to stay in us units?
> 
> Given that the use of this function is for 25ms in one case, I wonder
> if this warning is applicable:
> https://elixir.bootlin.com/linux/latest/source/include/linux/delay.h#L28
> 
> Counter point, 1ms latency over PCIe is not unusual.  I know we've
> removed the PCIe dependencies from MHI, but PCIe is the real usecase
> at this time.  Seems like this function could behave a bit weird if
> the parameter to udelay is something like "100", but the
> mhi_read_reg_field() call takes significantly longer than that.  Feels
> like in some scenarios, we could actually exceed the timeout by a
> non-trivial margin.
> 
> I guess I'm going back and forth in determining if us scale timing is
> a benefit in any way.
Thanks for all the inputs. I think a good idea here would be to use 
fsleep()
API as we need to allow any timeout the caller specifies. Also, plan is 
to
drop the patch #3 in this series since that will require a busywait due 
to
the code being in panic path.

I don't wish to accommodate another variable here for busywait but that
would be an option to pick sleep or delay depending on the caller's 
path.

Please respond if there are any concerns.

Thanks,
Bhaumik
---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project