linux-kernel - Re: [PATCH v8.1, 4/7] thermal: mediatek: Add LVTS driver for mt8192 thermal zones

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:   Thu, 4 Aug 2022 19:39:02 -0400
From:   Nícolas F. R. A. Prado 
        <nfraprado@...labora.com>
To:     bchihi@...libre.com
Cc:     rafael@...nel.org, rui.zhang@...el.com, daniel.lezcano@...aro.org,
        amitk@...nel.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org, khilman@...libre.com,
        mka@...omium.org, robh+dt@...nel.org, krzk+dt@...nel.org,
        matthias.bgg@...il.com, p.zabel@...gutronix.de,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, james.lo@...iatek.com,
        fan.chen@...iatek.com, louis.yu@...iatek.com,
        rex-bc.chen@...iatek.com, abailon@...libre.com
Subject: Re: [PATCH v8.1, 4/7] thermal: mediatek: Add LVTS driver for mt8192
 thermal zones

On Thu, Aug 04, 2022 at 03:09:09PM +0200, bchihi@...libre.com wrote:
> From: Michael Kao <michael.kao@...iatek.com>
> 
> Add a LVTS V4 (Low Voltage Thermal Sensor) driver to report junction
> temperatures in MediaTek SoC mt8192 and register the maximum temperature
> of sensors and each sensor as a thermal zone.
> 
> Signed-off-by: Yu-Chia Chang <ethan.chang@...iatek.com>
> Signed-off-by: Michael Kao <michael.kao@...iatek.com>
> Signed-off-by: Ben Tseng <ben.tseng@...iatek.com>
> Signed-off-by: Alexandre Bailon <abailon@...libre.com>
> Signed-off-by: Balsam CHIHI <bchihi@...libre.com>

You should have a Co-developed-by tag for each person that wrote the code [1].

[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

> ---
[..]
> --- /dev/null
> +++ b/drivers/thermal/mediatek/lvts_thermal.c
[..]
> +static int prepare_calibration_data(struct lvts_data *lvts_data)
> +{
[..]
> +	lvts_data->coeff.golden_temp = cal_data->golden_temp;
> +	dev_dbg(dev, "golden_temp = %d\n", cal_data->golden_temp);
> +	offset = snprintf(buffer, sizeof(buffer), "[lvts_cal] num:g_count:g_count_rc ");
> +	for (i = 0; i < lvts_data->num_sensor; i++)
> +		offset += snprintf(buffer + offset, sizeof(buffer) - offset, "%d:%d:%d ",
> +			i, cal_data->count_r[i], cal_data->count_rc[i]);

Like Angelo already mentioned [2], you're not using this string for anything, so
just remove the code.

[2] https://lore.kernel.org/linux-mediatek/82f6cd96-65e2-57f7-b7c5-05c111874087@collabora.com/

> +
> +	return 0;
> +}
[..]
> +int lvts_device_read_count_rc_n_v4(struct lvts_data *lvts_data)
> +{
[..]
> +	offset = snprintf(buffer, sizeof(buffer), "[COUNT_RC_NOW] ");
> +	for (i = 0; i < lvts_data->num_sensor; i++)
> +		offset += snprintf(buffer + offset, sizeof(buffer) - offset, "%d:%d ", i,
> +			cal_data->count_rc_now[i]);

Again, just formatting a string and throwing it away, please just drop the code.

> +
> +	return 0;
> +}
[..]
> +int lvts_resume(struct platform_device *pdev)
> +{
> +	struct lvts_data *lvts_data;
> +	int ret;
> +
> +	lvts_data = (struct lvts_data *)platform_get_drvdata(pdev);
> +	ret = lvts_init(lvts_data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

You have a bug in your resume path somewhere. I get this error during resume on
mt8192-asurada-spherion:

<1>[  237.487428] Unable to handle kernel write to read-only memory at virtual address ffffc15096e491c0
<1>[  237.496663] Mem abort info:
<1>[  237.499794]   ESR = 0x000000009600004e
<1>[  237.504334]   EC = 0x25: DABT (current EL), IL = 32 bits
<1>[  237.509982]   SET = 0, FnV = 0
<1>[  237.513444]   EA = 0, S1PTW = 0
<1>[  237.516911]   FSC = 0x0e: level 2 permission fault
<1>[  237.522041] Data abort info:
<1>[  237.525263]   ISV = 0, ISS = 0x0000004e
<1>[  237.529458]   CM = 0, WnR = 1
<1>[  237.532739] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000041782000
<1>[  237.539754] [ffffc15096e491c0] pgd=100000023ffff003, p4d=100000023ffff003, pud=100000023fffe003, pmd=0060000041200f81
<0>[  237.550821] Internal error: Oops: 9600004e [#1] PREEMPT SMP
<4>[  237.556669] Modules linked in: af_alg mtk_vcodec_dec_hw snd_soc_hdmi_codec panel_edp mtk_vcodec_dec v4l2_vp9 v4l2_h264 mtk_vcodec_enc btusb mtk_vcodec_common btrtl videobuf2_dma_contig uvcvideo btintel videobuf2_vmalloc mtk_vpu btmtk videobuf2_memops v4l2_mem2mem mt7921e btbcm cdc_ether usbnet videobuf2_v4l2 mt7921_common bluetooth crct10dif_ce videobuf2_common r8152 mt76_connac_lib videodev ecdh_generic ecc mc mt76 mac80211 sbs_battery cros_usbpd_charger cros_usbpd_logger cros_ec_chardev libarc4 pwm_cros_ec anx7625 snd_soc_rt5682_i2c elan_i2c snd_soc_rt5682 cros_ec_typec snd_soc_rl6231 drm_dp_aux_bus typec drm_display_helper panfrost mediatek_drm drm_cma_helper rtc_mt6397 drm_shmem_helper phy_mtk_mipi_dsi_drv gpu_sched mt8192_mt6359_rt1015_rt5682 drm_kms_helper pwm_mtk_disp pwm_bl snd_soc_rt1015p snd_soc_mt8192_afe snd_soc_dmic snd_soc_mtk_common cfg80211 rfkill drm fuse backlight ip_tables x_tables ipv6
<4>[  237.637538] CPU: 1 PID: 533 Comm: bash Tainted: G        W          5.19.0-rc8-next-20220725+ #254
<4>[  237.646781] Hardware name: Google Spherion (rev0 - 3) (DT)
<4>[  237.652539] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
<4>[  237.659778] pc : lvts_device_enable_and_init+0xdc/0xf0
<4>[  237.665198] lr : lvts_device_enable_and_init+0xc8/0xf0
<4>[  237.670609] sp : ffff8000098eb9b0
<4>[  237.674188] x29: ffff8000098eb9b0 x28: 0000000000000000 x27: 0000000000000003
<4>[  237.681609] x26: ffff41b141151010 x25: ffffc15097260c50 x24: 0000000000000000
<4>[  237.689028] x23: 0000000000000038 x22: 0000000000000001 x21: 000000008502fc00
<4>[  237.696448] x20: ffffc15096e49188 x19: 0000000000000003 x18: 0000000000000000
<4>[  237.703867] x17: 0000000000000001 x16: ffffc15095cc9838 x15: 00000000000001ca
<4>[  237.711287] x14: 0000000000000002 x13: 0000000000000000 x12: 000000000000045d
<4>[  237.718706] x11: 0000000000000000 x10: 00000000000027d0 x9 : ffffc15095d81d18
<4>[  237.726126] x8 : ffffc150974eb008 x7 : ffff41b15fdc2f80 x6 : ffffc15095ce58c0
<4>[  237.733545] x5 : 0000000000000000 x4 : ffff8000098eb6f0 x3 : 0000000000000000
<4>[  237.740963] x2 : 7ec5fa8849bae900 x1 : 7ec5fa8849bae900 x0 : 0000000000000014
<4>[  237.748383] Call trace:
<4>[  237.751091]  lvts_device_enable_and_init+0xdc/0xf0
<4>[  237.756155]  lvts_init+0x180/0x520
<4>[  237.759824]  lvts_resume+0x1c/0x78
<4>[  237.763494]  platform_pm_resume+0x5c/0x80
<4>[  237.767777]  dpm_run_callback+0x80/0x2e8
<4>[  237.771972]  device_resume+0x90/0x1c0
<4>[  237.775904]  dpm_resume+0x110/0x470
<4>[  237.779663]  dpm_resume_end+0x20/0x38
<4>[  237.783596]  suspend_devices_and_enter+0x1e4/0xb90
<4>[  237.788662]  pm_suspend+0x270/0x318
<4>[  237.792419]  state_store+0x94/0x120
<4>[  237.796176]  kobj_attr_store+0x18/0x30
<4>[  237.800198]  sysfs_kf_write+0x54/0x80
<4>[  237.804131]  kernfs_fop_write_iter+0x128/0x1c0
<4>[  237.808845]  vfs_write+0x39c/0x510
<4>[  237.812517]  ksys_write+0x74/0x100
<4>[  237.816187]  __arm64_sys_write+0x24/0x30
<4>[  237.820380]  invoke_syscall+0x4c/0x110
<4>[  237.824401]  el0_svc_common.constprop.0+0x68/0x128
<4>[  237.829464]  do_el0_svc+0x34/0xc0
<4>[  237.833048]  el0_svc+0x4c/0xc0
<4>[  237.836371]  el0t_64_sync_handler+0xb8/0xc0
<4>[  237.840824]  el0t_64_sync+0x18c/0x190
<0>[  237.844759] Code: 11000673 6b13001f 54fffaa8 52800280 (b9003a80)
<4>[  237.851129] ---[ end trace 0000000000000000 ]---

Thanks,
Nícolas