Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 18 May 2022 08:43:14 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Kohei Tarumizu <tarumizu.kohei@fujitsu.com>
Cc:     catalin.marinas@arm.com, will@kernel.org, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        x86@kernel.org, hpa@zytor.com, rafael@kernel.org,
        mchehab+huawei@kernel.org, eugenis@google.com, tony.luck@intel.com,
        pcc@google.com, peterz@infradead.org, marcos@orca.pet,
        conor.dooley@microchip.com, nicolas.ferre@microchip.com,
        marcan@marcan.st, linus.walleij@linaro.org, arnd@arndb.de,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4 6/8] x86: Add hardware prefetch control support for x86
Message-ID: <YoSVglItX1PhveEP@kroah.com>
References: <20220518063032.2377351-1-tarumizu.kohei@fujitsu.com>
 <20220518063032.2377351-7-tarumizu.kohei@fujitsu.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220518063032.2377351-7-tarumizu.kohei@fujitsu.com>
Precedence: bulk

On Wed, May 18, 2022 at 03:30:30PM +0900, Kohei Tarumizu wrote:
> +/*
> + * Returns the cpu number of the cpu_device(/sys/devices/system/cpu/cpuX)
> + * in the ancestor directory of prefetch_control.
> + *
> + * When initializing this driver, it is verified that the cache directory exists
> + * under cpuX device. Therefore, the third level up from prefetch_control is
> + * cpuX device as shown below.
> + *
> + * /sys/devices/system/cpu/cpuX/cache/indexX/prefetch_control
> + */
> +static inline unsigned int pfctl_dev_get_cpu(struct device *pfctl_dev)
> +{
> +	return pfctl_dev->parent->parent->parent->id;

As much fun as it is to see a function like this, that is not ok, and
never guaranteed to keep working, sorry.  Please find the device
properly, never assume a specific driver model topology as they are
guaranteed to break over time and never supposed to be static like this.


> +}
> +
> +static ssize_t
> +pfctl_show(struct device *pfctl_dev, struct device_attribute *attr, char *buf)
> +{
> +	unsigned int cpu = pfctl_dev_get_cpu(pfctl_dev);
> +	struct x86_pfctl_attr *xa;
> +	u64 val;
> +
> +	xa = container_of(attr, struct x86_pfctl_attr, attr);
> +
> +	rdmsrl_on_cpu(cpu, MSR_MISC_FEATURE_CONTROL, &val);
> +	return sysfs_emit(buf, "%d\n", val & xa->mask ? 0 : 1);
> +}
> +
> +struct write_info {
> +	u64 mask;
> +	bool enable;
> +};
> +
> +/*
> + * wrmsrl() in this patch is only done inside of an interrupt-disabled region
> + * to avoid a conflict of write access from other drivers,
> + */
> +static void pfctl_write(void *info)
> +{
> +	struct write_info *winfo = info;
> +	u64 reg;
> +
> +	reg = 0;
> +	rdmsrl(MSR_MISC_FEATURE_CONTROL, reg);
> +
> +	if (winfo->enable)
> +		reg &= ~winfo->mask;
> +	else
> +		reg |= winfo->mask;
> +
> +	wrmsrl(MSR_MISC_FEATURE_CONTROL, reg);
> +}
> +
> +/*
> + * MSR_MISC_FEATURE_CONTROL has "core" scope, so define the lock to avoid a
> + * conflict of write access from different logical processors in the same core.
> + */
> +static DEFINE_MUTEX(pfctl_mutex);
> +
> +static ssize_t
> +pfctl_store(struct device *pfctl_dev, struct device_attribute *attr,
> +	    const char *buf, size_t size)
> +{
> +	unsigned int cpu = pfctl_dev_get_cpu(pfctl_dev);
> +	struct x86_pfctl_attr *xa;
> +	struct write_info info;
> +
> +	xa = container_of(attr, struct x86_pfctl_attr, attr);
> +	info.mask = xa->mask;
> +
> +	if (strtobool(buf, &info.enable) < 0)
> +		return -EINVAL;
> +
> +	mutex_lock(&pfctl_mutex);
> +	smp_call_function_single(cpu, pfctl_write, &info, true);
> +	mutex_unlock(&pfctl_mutex);
> +
> +	return size;
> +}
> +
> +#define PFCTL_ATTR(_name, _level, _bit)					\
> +	struct x86_pfctl_attr attr_l##_level##_##_name = {		\
> +		.attr = __ATTR(_name, 0600, pfctl_show, pfctl_store),	\
> +		.mask = BIT_ULL(_bit), }
> +
> +static PFCTL_ATTR(hardware_prefetcher_enable,			1, 2);
> +static PFCTL_ATTR(hardware_prefetcher_enable,			2, 0);
> +static PFCTL_ATTR(ip_prefetcher_enable,				1, 3);
> +static PFCTL_ATTR(adjacent_cache_line_prefetcher_enable,	2, 1);
> +
> +static struct device_attribute *l1_attrs[] __ro_after_init = {

How do you know attributes are to be marked read only after init?  Do
other parts of the kernel do that?  I don't think the driver core
guarantees that at all.

thanks,

greg k-h