linux-kernel - Re: [PATCH 03/14] coresight: tmc: re-implementing tmc_read

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: <56F271FE.8060904@arm.com>
Date:	Wed, 23 Mar 2016 10:37:50 +0000
From:	"Suzuki K. Poulose" <Suzuki.Poulose@....com>
To:	Mathieu Poirier <mathieu.poirier@...aro.org>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 03/14] coresight: tmc: re-implementing
 tmc_read_prepare/unprepare() functions

On 22/03/16 20:23, Mathieu Poirier wrote:
> In their current implementation the tmc_read_prepare/unprepare()
> are a lump of if/else that is difficult to read.  This patch is
> alleviating that by using a switch statement.  The latter also
> allows for a better control on the error path.
>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@...aro.org>
> ---
>   drivers/hwtracing/coresight/coresight-tmc.c | 56 ++++++++++++++++++-----------
>   1 file changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
> index f4ba837a0810..208d47dd3083 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc.c
> @@ -430,7 +430,7 @@ static const struct coresight_ops tmc_etf_cs_ops = {
>

> -	if (drvdata->config_type == TMC_CONFIG_TYPE_ETB) {
> +	switch (drvdata->config_type) {
> +	case TMC_CONFIG_TYPE_ETB:
>   		tmc_etb_disable_hw(drvdata);
> -	} else if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
> -		tmc_etr_disable_hw(drvdata);
> -	} else {
> +		break;
> +	case TMC_CONFIG_TYPE_ETF:
> +		/* There is no point in reading a TMC in HW FIFO mode */
>   		mode = readl_relaxed(drvdata->base + TMC_MODE);
> -		if (mode == TMC_MODE_CIRCULAR_BUFFER) {
> -			tmc_etb_disable_hw(drvdata);
> -		} else {
> -			ret = -ENODEV;
> +		if (mode != TMC_MODE_CIRCULAR_BUFFER) {
> +			ret = -EINVAL;
>   			goto err;
>   		}
> +
> +		tmc_etb_disable_hw(drvdata);
> +		break;
> +	case TMC_CONFIG_TYPE_ETR:
> +		tmc_etr_disable_hw(drvdata);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto err;
>   	}

We seem to be doing this switch at different places in the code just for enable_hw/disable_hw.
e.g, tmc_enable, tmc_disable

Could we make this a bit more cleaner by introducing something like this ?

struct tmc_hw_ops {
	int (*enable_hw)(struct tmc_drvdata *drvdata, enum tmc_mode mode);
	int (*disable_hw)(struct tmc_drvdata *drvdata, enum tmc_mode mode);
};

struct tmc_hw_ops tmc_etf_ops = {
	tmc_etf_enable_hw,
	tmc_etf_disable_hw,
};

similiarly for etb and etr and then add struct tmc_hw_ops *hw_ops to tmc_drvdata, initialised
at probe time (while reading the config_type).


Suzuki