| 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
| ||
|
Date: Fri, 22 Jan 2016 16:52:20 +0800
From: Wei Ni <wni@...dia.com>
To: Thierry Reding <thierry.reding@...il.com>
CC: <rui.zhang@...el.com>, <MLongnecker@...dia.com>,
<swarren@...dotorg.org>, <mikko.perttunen@...si.fi>,
<linux-tegra@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3 04/11] thermal: tegra: split tegra_soctherm driver
On 2016年01月21日 22:46, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Mon, Jan 18, 2016 at 06:02:29PM +0800, Wei Ni wrote:
> [...]
>> +int tegra_soctherm_calculate_tsensor_calibration(
>> + struct tegra_tsensor *sensor,
>> + const struct tsensor_shared_calibration *shared)
>
> The need to ident weirdly here should be an indication that the function
> name is too long, how about:
Hmm, yes, it's too long.
>
> int tegra_tsensor_calc_calib(struct tegra_tsensor *sensor,
> const struct tsensor_shared_calibration *shared)
>
> ?
There have two functions about calibration, I prefer to name them as:
tegra_calc_tsensor_calib() and tegra_calc_shared_calib().
>
>> +{
>> + const struct tegra_tsensor_group *sensor_group;
>> + u32 val, calib;
>> + s32 actual_tsensor_ft, actual_tsensor_cp;
>> + s32 delta_sens, delta_temp;
>> + s32 mult, div;
>> + s16 therma, thermb;
>> + int err;
>> +
>> + sensor_group = sensor->group;
>> +
>> + err = tegra_fuse_readl(sensor->calib_fuse_offset, &val);
>> + if (err)
>> + return err;
>> +
>> + actual_tsensor_cp = (shared->base_cp * 64) + sign_extend32(val, 12);
>> + val = (val & FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK)
>> + >> FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT;
>
> I think it's more canonical to put the >> on the first line line.
Ok, will fix it.
>
>> + actual_tsensor_ft = (shared->base_ft * 32) + sign_extend32(val, 12);
>> +
>> + delta_sens = actual_tsensor_ft - actual_tsensor_cp;
>> + delta_temp = shared->actual_temp_ft - shared->actual_temp_cp;
>> +
>> + mult = sensor_group->pdiv * sensor->config->tsample_ate;
>> + div = sensor->config->tsample * sensor_group->pdiv_ate;
>> +
>> + therma = div64_s64_precise((s64)delta_temp * (1LL << 13) * mult,
>> + (s64)delta_sens * div);
>
> Are the explicit casts necessary? Shouldn't an s32 be automatically
> promoted to s64? Also arguments on subsequent lines should be aligned
> with the first argument on the first line.
I made a mistake, the div64_s64_precise(s64 a, s64 b) should be
div64_s64_precise(s64 a, s32b), so to make the value more precise, I added (s64)
cast in here.
And I will use temporary variable, and align the arguments.
>
>> + thermb = div64_s64_precise(
>> + ((s64)actual_tsensor_ft * shared->actual_temp_cp) -
>> + ((s64)actual_tsensor_cp * shared->actual_temp_ft),
>> + (s64)delta_sens);
>
> Perhaps add a temporary variable for the first parameter here for
> readability?
Yes, will use temporary variable, and remove the cast for delta_sens.
>
>> +
>> + therma = div64_s64_precise((s64)therma * sensor->fuse_corr_alpha,
>> + (s64)1000000LL);
>> + thermb = div64_s64_precise((s64)thermb * sensor->fuse_corr_alpha +
>> + sensor->fuse_corr_beta,
>> + (s64)1000000LL);
>
> What are the 1000000LL? Does it perhaps make sense to have a macro for
> it, or perhaps a comment would help.
It's the coefficient data for the therma and thermb. I will use macro for it.
#define CALIB_COEFFICIENT 1000000LL
>
>> + calib = ((u16)therma << SENSOR_CONFIG2_THERMA_SHIFT) |
>> + ((u16)thermb << SENSOR_CONFIG2_THERMB_SHIFT);
>
> Alignment here isn't right.
will do it.
>
>> diff --git a/drivers/thermal/tegra/soctherm.h b/drivers/thermal/tegra/soctherm.h
> [...]
>> +struct tegra_soctherm_soc {
>> + struct tegra_tsensor *tsensors;
>
> Can't these be const? Do they ever need to be modified? If so, they
> should probably not be part of this structure. Or at least only part of
> them should be. The invariant part.
>
> The reason is that if ever a second instance of this device was present
> both instances would share this same data. I know it's unlikely to
> happen, but setting a bad example would be... well... bad.
>
> Instead I think if you need to have non-const fields you could separate
> this further into struct tegra_tsensor_soc, with only the static
> information about the sensor, and make struct tegra_tsensor contain a
> pointer to that SoC structure and provide the variable fields in
> addition. That way you can create a struct tegra_tsensor for each struct
> tegra_tsensor_soc and store those per-instance.
There has a member "calib" in the tsensors which will be written in the driver,
so I didn't make it as const.
But you are right, I need to consider the risk that the system have two more SOCs.
I will remove the "calib" to struct tegra_soctherm, then can fix this issue, and
can make all data to "const" in the chip-specific file.
>
>> diff --git a/drivers/thermal/tegra/tegra124-soctherm.c b/drivers/thermal/tegra/tegra124-soctherm.c
> [...]
>> +static struct tegra_tsensor tegra124_tsensors[] = {
>
> Can this be "static const" instead?
Will fix this one.
>
>> + {
>> + .name = "cpu0",
>> + .base = 0xc0,
>> + .config = &t124_tsensor_config,
>> + .calib_fuse_offset = 0x098,
>> + .fuse_corr_alpha = 1135400,
>> + .fuse_corr_beta = -6266900,
>> + .group = &tegra124_tsensor_group_cpu,
>> + },
>> + {
>
> "}," and "{" can go on the same line.
Got it, will change it.
>
>> +struct tegra_soctherm_soc tegra124_soctherm = {
>
> "const"?
Will fix it.
>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>
Powered by blists - more mailing lists