linux-kernel - Re: [PATCH v2] sched: Provide USF for the portable equipment.

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:   Fri, 31 Jul 2020 15:28:35 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Dongdong Yang <contribute.kernel@...il.com>
Cc:     rjw@...ysocki.net, viresh.kumar@...aro.org, mingo@...hat.com,
        peterz@...radead.org, juri.lelli@...hat.com,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
        linux-kernel@...r.kernel.org, devel@...verdev.osuosl.org,
        linux-pm@...r.kernel.org, yangdongdong@...omi.com,
        tanggeliang@...omi.com, taojun@...omi.com, huangqiwu@...omi.com,
        rocking@...ux.alibaba.com, fengwei@...omi.com,
        zhangguoquan@...omi.com, gulinghua@...omi.com, duhui@...omi.com
Subject: Re: [PATCH v2] sched: Provide USF for the portable equipment.

On Fri, Jul 31, 2020 at 08:46:30PM +0800, Dongdong Yang wrote:
> +static ssize_t store_sched_usf_up_l0_r(struct kobject *kobj,
> +				       struct kobj_attribute *attr,
> +				       const char *buf, size_t count)
> +{
> +	int val = 0;
> +	int ret;
> +
> +	ret = kstrtoint(buf, 0, &val);
> +	if (ret) {
> +		pr_err(USF_TAG "set state fail ret=%d\n", ret);
> +		return ret;
> +	}
> +
> +	if ((val >= 0) && (val <= BOOST_MAX_V)) {
> +		usf_vdev.sysctl_sched_usf_up_l0 = val;
> +		usf_vdev.usf_up_l0 = LEVEL_TOP -
> +				DIV_ROUND_UP(val, BOOST_MAX_V / 2);
> +	} else {
> +		pr_err(USF_TAG "%d should fall into [%d %d]",
> +		       val, 0, BOOST_MAX_V);
> +	}
> +	if ((usf_vdev.sysctl_sched_usf_up_l0 == 0) &&
> +	    (usf_vdev.sysctl_sched_usf_down == 0))
> +		static_branch_disable(&adjust_task_pred_set);
> +	else
> +		static_branch_enable(&adjust_task_pred_set);
> +
> +	return count;
> +}
> +
> +static ssize_t store_sched_usf_down_r(struct kobject *kobj,
> +				      struct kobj_attribute *attr,
> +				      const char *buf, size_t count)
> +{
> +	int val = 0;
> +	int ret;
> +
> +	ret = kstrtoint(buf, 0, &val);
> +	if (ret) {
> +		pr_err(USF_TAG "set state fail ret=%d\n", ret);
> +		return ret;
> +	}
> +
> +	if ((val >= BOOST_MIN_V) && (val <= 0)) {
> +		usf_vdev.sysctl_sched_usf_down = val;
> +		usf_vdev.usf_down = DIV_ROUND_UP(-val, -BOOST_MIN_V / 2);
> +	} else {
> +		pr_err(USF_TAG "%d should fall into [%d %d]",
> +		       val, BOOST_MIN_V, 0);
> +	}
> +	if ((usf_vdev.sysctl_sched_usf_up_l0 == 0) &&
> +	    (usf_vdev.sysctl_sched_usf_down == 0))
> +		static_branch_disable(&adjust_task_pred_set);
> +	else
> +		static_branch_enable(&adjust_task_pred_set);
> +
> +	return count;
> +}
> +
> +static ssize_t store_sched_usf_non_ux_r(struct kobject *kobj,
> +					struct kobj_attribute *attr,
> +					const char *buf, size_t count)
> +{
> +	int val = 0;
> +	int ret;
> +
> +	ret = kstrtoint(buf, 0, &val);
> +	if (ret) {
> +		pr_err(USF_TAG "set state fail ret=%d\n", ret);
> +		return ret;
> +	}
> +
> +	if ((val >= BOOST_MIN_V) && (val <= 0)) {
> +		usf_vdev.sysctl_sched_usf_non_ux = val;
> +		usf_vdev.usf_non_ux = DIV_ROUND_UP(-val, -BOOST_MIN_V / 2);
> +	} else {
> +		pr_err(USF_TAG "%d should fall into [%d %d]",
> +		       val, BOOST_MIN_V, 0);
> +	}
> +	if (usf_vdev.sysctl_sched_usf_non_ux == 0)
> +		static_branch_disable(&adjust_task_pred_set);
> +	else
> +		static_branch_enable(&adjust_task_pred_set);
> +
> +	return count;
> +}
> +
> +#define usf_attr_rw(_name)						\
> +static struct kobj_attribute _name =					\
> +__ATTR(_name, 0664, show_##_name, store_##_name)

You have a lot of sysfs files here, but no documentation for them at
all, why not?

> +
> +#define usf_show_node(_name, _value)					\
> +static ssize_t show_##_name						\
> +(struct kobject *kobj, struct kobj_attribute *attr,  char *buf)		\
> +{									\
> +	unsigned int len = 0;						\
> +	unsigned int max_len = 8;					\
> +	len +=								\
> +	    snprintf(buf + len, max_len - len, "%d",			\
> +		     usf_vdev.sysctl_##_value);				\
> +	return len;							\
> +}
> +
> +usf_show_node(sched_usf_up_l0_r, sched_usf_up_l0);
> +usf_show_node(sched_usf_down_r, sched_usf_down);
> +usf_show_node(sched_usf_non_ux_r, sched_usf_non_ux);
> +
> +usf_attr_rw(sched_usf_up_l0_r);
> +usf_attr_rw(sched_usf_down_r);
> +usf_attr_rw(sched_usf_non_ux_r);
> +
> +static struct attribute *sched_attrs[] = {
> +	&sched_usf_up_l0_r.attr,
> +	&sched_usf_down_r.attr,
> +	&sched_usf_non_ux_r.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group sched_attr_group = {
> +	.attrs = sched_attrs,
> +};
> +
> +static int usf_dbg_get(void *data, u64 *val)
> +{
> +	*val = (u64)usf_vdev.enable_debug;
> +
> +	return 0;
> +}
> +
> +static int usf_dbg_set(void *data, u64 val)
> +{
> +	usf_vdev.enable_debug = !!val;
> +	return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(usf_dbg_fops, usf_dbg_get,
> +			usf_dbg_set, "%llu\n");
> +
> +static int __init intera_monitor_init(void)
> +{
> +	int res = -1;
> +	struct attribute_group *attr_group;
> +
> +	res = fb_register_client(&usf_lcd_nb);
> +	if (res < 0) {
> +		pr_err("Failed to register usf_lcd_nb!\n");
> +		return res;
> +	}
> +
> +	/*
> +	 * create a sched_usf in cpu_subsys:
> +	 * /sys/devices/system/cpu/sched_usf/...
> +	 */
> +	attr_group = &sched_attr_group;
> +	usf_vdev.kobj = kobject_create_and_add("sched_usf",
> +					       &cpu_subsys.dev_root->kobj);
> +

Huh?  Why put this there?

And by making a "raw" kobject, you just kept userspace from seeing this
information entirely :(

Use a 'struct device' as you are a driver, do not use a raw kobject.

thanks,

greg k-h