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

[Greg Kroah-Hartman]

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