linux-kernel - Re: [PATCH v3 02/14] intel_gna: add component of hardware operation

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]

Message-ID: <YJ0KYRuZK3xn0bdO@kroah.com>
Date:   Thu, 13 May 2021 13:15:45 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Maciej Kwapulinski <maciej.kwapulinski@...ux.intel.com>
Cc:     Arnd Bergmann <arnd@...db.de>, Jonathan Corbet <corbet@....net>,
        Derek Kiernan <derek.kiernan@...inx.com>,
        Dragan Cvetic <dragan.cvetic@...inx.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
        Tomasz Jankowski <tomasz1.jankowski@...el.com>,
        Savo Novakovic <savox.novakovic@...el.com>,
        Jianxun Zhang <jianxun.zhang@...ux.intel.com>
Subject: Re: [PATCH v3 02/14] intel_gna: add component of hardware operation

On Thu, May 13, 2021 at 01:00:28PM +0200, Maciej Kwapulinski wrote:
> +void gna_start_scoring(struct gna_private *gna_priv,
> +		       struct gna_compute_cfg *compute_cfg)
> +{
> +	u32 ctrl = gna_reg_read(gna_priv, GNA_MMIO_CTRL);
> +
> +	ctrl |= GNA_CTRL_START_ACCEL | GNA_CTRL_COMP_INT_EN | GNA_CTRL_ERR_INT_EN;
> +
> +	ctrl &= ~GNA_CTRL_COMP_STATS_EN;
> +	ctrl |= FIELD_PREP(GNA_CTRL_COMP_STATS_EN,
> +			compute_cfg->hw_perf_encoding & FIELD_MAX(GNA_CTRL_COMP_STATS_EN));
> +
> +	ctrl &= ~GNA_CTRL_ACTIVE_LIST_EN;
> +	ctrl |= FIELD_PREP(GNA_CTRL_ACTIVE_LIST_EN,
> +			compute_cfg->active_list_on & FIELD_MAX(GNA_CTRL_ACTIVE_LIST_EN));
> +
> +	ctrl &= ~GNA_CTRL_OP_MODE;
> +	ctrl |= FIELD_PREP(GNA_CTRL_OP_MODE,
> +			compute_cfg->gna_mode & FIELD_MAX(GNA_CTRL_OP_MODE));
> +
> +	gna_reg_write(gna_priv, GNA_MMIO_CTRL, ctrl);
> +
> +	dev_dbg(gna_dev(gna_priv), "scoring started...\n");

ftrace is your friend, no need for lines like this.

> +void gna_abort_hw(struct gna_private *gna_priv)
> +{
> +	u32 val;
> +	int ret;
> +
> +	/* saturation bit in the GNA status register needs
> +	 * to be explicitly cleared.
> +	 */
> +	gna_clear_saturation(gna_priv);
> +
> +	val = gna_reg_read(gna_priv, GNA_MMIO_STS);
> +	dev_dbg(gna_dev(gna_priv), "status before abort: %#x\n", val);
> +
> +	val = gna_reg_read(gna_priv, GNA_MMIO_CTRL);
> +	val |= GNA_CTRL_ABORT_CLR_ACCEL;
> +	gna_reg_write(gna_priv, GNA_MMIO_CTRL, val);
> +
> +	ret = readl_poll_timeout(gna_priv->iobase + GNA_MMIO_STS, val,
> +				!(val & 0x1),
> +				0, 1000);
> +
> +	if (ret)
> +		dev_err(gna_dev(gna_priv), "abort did not complete\n");
> +}

If "abort_hw" can fail, then return an error.  What could a user do with
an error message in the kernel log like the above one?  The driver
obviously did not recover from it, so what can they do?

> --- /dev/null
> +++ b/include/uapi/misc/intel/gna.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
> +/* Copyright(c) 2017-2021 Intel Corporation */
> +
> +#ifndef _UAPI_GNA_H_
> +#define _UAPI_GNA_H_
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif

These C++ things should not be needed in kernel uapi header files,
right?

thanks,

greg k-h