linux-kernel - Re: [PATCH 01/18] thermal: exynos: fix setting rising

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:   Mon, 30 Apr 2018 17:21:40 +0200
From:   Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     Eduardo Valentin <edubezval@...il.com>,
        Zhang Rui <rui.zhang@...el.com>,
        linux-samsung-soc@...r.kernel.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/18] thermal: exynos: fix setting rising_threshold for
 Exynos5433

On Monday, April 30, 2018 04:36:56 PM Daniel Lezcano wrote:
> On Thu, Apr 26, 2018 at 01:51:16PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > Add missing clearing of the previous value when setting rising
> > temperature threshold.
> > 
> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
> > ---
> >  drivers/thermal/samsung/exynos_tmu.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> > index cda716c..523d26e 100644
> > --- a/drivers/thermal/samsung/exynos_tmu.c
> > +++ b/drivers/thermal/samsung/exynos_tmu.c
> > @@ -577,6 +577,7 @@ static int exynos5433_tmu_initialize(struct platform_device *pdev)
> >  		threshold_code = temp_to_code(data, temp);
> >  
> >  		rising_threshold = readl(data->base + rising_reg_offset);
> > +		rising_threshold &= ~(0xff << j * 8);
> >  		rising_threshold |= (threshold_code << j * 8);
> 
> Bartlomiej,
> 
> I see this code is duplicated all around the driver, so I can't blame you to
> fix it in the same way it is written today but this is not how to deal with

This patch is a bugfix (which may be backported later) and it shouldn't be
mixed with cleanups.

> fields in a register mapping. Can you fix it by adding correct macros with
> masks?

After my patch series we still have following occurrences of "RMW" code:

        th = readl(data->base + EXYNOS_THD_TEMP_RISE);
        th &= ~(0xff << 8 * trip);
        th |= temp_to_code(data, temp) << 8 * trip;
        writel(th, data->base + EXYNOS_THD_TEMP_RISE);

        th = readl(data->base + EXYNOS_THD_TEMP_FALL);
        th &= ~(0xff << 8 * trip);
        if (hyst)
                th |= temp_to_code(data, temp - hyst) << 8 * trip;
        writel(th, data->base + EXYNOS_THD_TEMP_FALL);

        th = readl(data->base + reg_off);
        th &= ~(0xff << j * 8);
        th |= (temp_to_code(data, temp) << j * 8);
        writel(th, data->base + reg_off);

        th = readl(data->base + reg_off);
        th &= ~(0xff << j * 8);
        th |= (temp_to_code(data, temp - hyst) << j * 8);
        writel(th, data->base + reg_off);

        th = readl(data->base + EXYNOS7_THD_TEMP_RISE7_6 + reg_off);
        th &= ~(EXYNOS7_TMU_TEMP_MASK << (16 * bit_off));
        th |= temp_to_code(data, temp) << (16 * bit_off);
        writel(th, data->base + EXYNOS7_THD_TEMP_RISE7_6 + reg_off);

        th = readl(data->base + EXYNOS7_THD_TEMP_FALL7_6 + reg_off);
        th &= ~(EXYNOS7_TMU_TEMP_MASK << (16 * bit_off));
        th |= temp_to_code(data, temp - hyst) << (16 * bit_off);
        writel(th, data->base + EXYNOS7_THD_TEMP_FALL7_6 + reg_off);

My personal opinion is that adding new macro for the code above will
only obfuscate it and make it harder to understand,

Anyway how would you like this new macro to look like (how generic or
specific it should be etc.)?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics