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]
Date:   Tue, 12 May 2020 11:05:28 +0200
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Lukasz Luba <lukasz.luba@....com>
Cc:     Bernard Zhao <bernard@...o.com>, Kukjin Kim <kgene@...nel.org>,
        linux-pm@...r.kernel.org,
        "linux-samsung-soc@...r.kernel.org" 
        <linux-samsung-soc@...r.kernel.org>,
        linux-arm-kernel@...ts.infradead.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        opensource.kernel@...o.com
Subject: Re: [PATCH] memory/samsung: reduce unnecessary mutex lock area

On Tue, 12 May 2020 at 10:47, Lukasz Luba <lukasz.luba@....com> wrote:
>
> Hi Krzysztof,
>
> I am sorry, I was a bit busy recently.
>
> On 5/12/20 7:50 AM, Krzysztof Kozlowski wrote:
> > On Fri, May 08, 2020 at 06:13:38AM -0700, Bernard Zhao wrote:
> >> Maybe dmc->df->lock is unnecessary to protect function
> >> exynos5_dmc_perf_events_check(dmc). If we have to protect,
> >> dmc->lock is more better and more effective.
> >> Also, it seems not needed to protect "if (ret) & dev_warn"
> >> branch.
> >>
> >> Signed-off-by: Bernard Zhao <bernard@...o.com>
> >> ---
> >>   drivers/memory/samsung/exynos5422-dmc.c | 6 ++----
> >>   1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > I checked the concurrent accesses and it looks correct.
> >
> > Lukasz, any review from your side?
>
> The lock from devfreq lock protects from a scenario when
> concurrent access from devfreq framework uses internal dmc fields 'load'
> and 'total' (which are set to 'busy_time', 'total_time').
> The .get_dev_status can be called at any time (even due to thermal
> devfreq cooling action) and reads above fields.
> That's why the calculation of the new values inside dmc is protected.

I looked at this path (get_dev_status) and currently in devfreq it
will be only called from update_devfreq() -> get_target_freq()... at
least when looking at devfreq core and governors. On the other hand
you are right that this is public function and this call scenario
might change. It could be called directly from other paths sooner or
later.

> This patch should not be taken IMO. Maybe we can release lock before the
> if statement, just to speed-up.

Yep.

Bernard, you can send just this part of the patch.

Best regards,
Krzysztof

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ