linux-kernel - RE: [PATCH v2] Thermal: exynos: Add sysfs node supporting exynos's emulation mode.

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] [day] [month] [year] [list]

Date:	Wed, 31 Oct 2012 08:49:36 +0000
From:	"R, Durgadoss" <durgadoss.r@...el.com>
To:	"jonghwa3.lee@...sung.com" <jonghwa3.lee@...sung.com>
CC:	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Brown, Len" <len.brown@...el.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Amit Dinel Kachhap <amit.kachhap@...aro.org>
Subject: RE: [PATCH v2] Thermal: exynos: Add sysfs node supporting exynos's
 emulation mode.

Hi,

[A big cut.]
> >> +
> >> +	mutex_lock(&data->lock);
> >> +	clk_enable(data->clk);
> >> +
> >> +	reg = readl(data->base + EXYNOS_EMUL_CON);
> >> +	enable = reg & EXYNOS_EMUL_ENABLE;
> >> +	if (!enable && !temp)
> >> +		goto out;
> > I think you what you are trying to do here is this:
> > If the emulation is already disabled, and 'this' write tries to
> > disable it again, you jump to 'out' as an optimization.
> > Please correct me if I am wrong.
> Yes, you're right. If we try to disable the emulation mode despite it already is
> disabled,
> then emulation mode will be failed and we can't use it. Because emulation
> mode has some
> strange characteristic.
> Let me explain more detail. Emulation mode requires synchronous of any
> value changing and
> enabling. It means you can't change any value (next target temperature,
> sensing time delay)
> without enabling. In this code, disabling overlapping makes problem at initial
> state. At the initial,
> there is no next target temperature and only value 1 for delay time. But if
> you try to write 0 again
> to sysfs node, temp, then it write delay time with 938us and next
> temperature with minimum
> temperature automatically. Value is changed but enable bit is still disabled.
> This is what the
> problem is happened.

Thanks for the detailed Explanation.

> >>  	}
> >> +#ifdef CONFIG_EXYNOS_THERMAL_EMUL
> >> +	device_create_file(&pdev->dev, &dev_attr_emulation);
> > Don't we want to capture the return value here ?
> > Also, if we conditionally create this, we should remove this conditionally
> also.
> > I did not see a corresponding device_remove_file for this attr.
> Sorry, It's my mistake. I'll fix it.
> >> +#endif
> > Can we club all the code inside CONFIG_*_EMUL at one place ?
> > This will make it neat, and not spread the #ifdefs all over the driver.
> I'd like to do either. But I just couldn't find a way.
> Could you give me any idea how to do it ?

I was thinking something like below:

inside #ifdef CONFIG_*_EMUL
	int register_emulation_mode(...) { device_create_file(...) };
	int unregister_emulation_mode(...) { device_remove_file(...) };
#else
	static inline int register..(...) { return 0; }
	static inline int unregister..(...) { return 0; }
#endif

Then the rest of the driver can just use register/unregister,
without worrying about #ifdef.

Thanks,
Durga