| 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
| ||
|
Message-ID: <CACRpkdZO7Mo_JaecjypR_kXKwaatgU+4oLLKheQ22OcNMG_-nQ@mail.gmail.com>
Date: Wed, 3 Sep 2014 10:37:22 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: Mathieu Poirier <mathieu.poirier@...aro.org>
Cc: Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
Will Deacon <will.deacon@....com>,
Russell King - ARM Linux <linux@....linux.org.uk>,
Greg KH <gregkh@...uxfoundation.org>,
Arnd Bergmann <arnd@...aro.org>,
John Stultz <john.stultz@...aro.org>,
Pratik Patel <pratikp@...eaurora.org>,
Vikas Varshney <varshney@...com>, Al Grant <Al.Grant@....com>,
Jonas Svennebring <jonas.svennebring@...gotech.com>,
James King <james.king@...aro.org>,
Panchaxari Prasannamurthy Tumkur
<panchaxari.prasannamurthy@...aro.org>,
Kaixu Xia <kaixu.xia@...aro.org>,
Marcin Jabrzyk <marcin.jabrzyk@...il.com>,
r.sengupta@...sung.com, Robert Marklund <robbelibobban@...il.com>,
Tony.Armitstead@....com, Patch Tracking <patches@...aro.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 07/11 v5] coresight-etm: add CoreSight ETM/PTM driver
On Wed, Aug 27, 2014 at 7:17 PM, <mathieu.poirier@...aro.org> wrote:
> From: Pratik Patel <pratikp@...eaurora.org>
>
> This driver manages CoreSight ETM (Embedded Trace Macrocell) that
> supports processor tracing. Currently supported version are ARM
> ETMv3.x and PTM1.x.
>
> Signed-off-by: Pratik Patel <pratikp@...eaurora.org>
> Signed-off-by: Panchaxari Prasannamurthy <panchaxari.prasannamurthy@...aro.org>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@...aro.org>
(...)
> +/* Accessors for CP14 registers */
> +#define dbg_read(reg) RCP14_##reg()
> +#define dbg_write(val, reg) WCP14_##reg(val)
> +#define etm_read(reg) RCP14_##reg()
> +#define etm_write(val, reg) WCP14_##reg(val)
> +
> +/* MRC14 and MCR14 */
> +#define MRC14(op1, crn, crm, op2) \
> +({ \
> +u32 val; \
> +asm volatile("mrc p14, "#op1", %0, "#crn", "#crm", "#op2 : "=r" (val)); \
> +val; \
> +})
> +
> +#define MCR14(val, op1, crn, crm, op2) \
> +({ \
> +asm volatile("mcr p14, "#op1", %0, "#crn", "#crm", "#op2 : : "r" (val));\
> +})
OK... some ARMish funny assembly...
(...)
But this:
> +static unsigned int etm_read_reg(u32 reg)
> +{
> + switch (reg) {
> + case ETMCR:
> + return etm_read(ETMCR);
> + case ETMCCR:
> + return etm_read(ETMCCR);
(...)
And this:
> +static void etm_write_reg(u32 val, u32 reg)
> +{
> + switch (reg) {
> + case ETMCR:
> + etm_write(val, ETMCR);
> + return;
> + case ETMTRIGGER:
> + etm_write(val, ETMTRIGGER);
> + return;
And then this:
> +unsigned int etm_readl_cp14(u32 off)
> +{
> + return etm_read_reg(off);
> +}
> +
> +void etm_writel_cp14(u32 val, u32 off)
> +{
> + etm_write_reg(val, off);
> +}
And then this:
+static inline void etm_writel(struct etm_drvdata *drvdata,
+ u32 val, u32 off)
+{
+ if (drvdata->use_cp14)
+ etm_writel_cp14(val, off);
+ else
+ writel_relaxed(val, drvdata->base + off);
+}
+
+static inline unsigned int etm_readl(struct etm_drvdata *drvdata, u32 off)
+{
+ u32 val;
+
+ if (drvdata->use_cp14)
+ val = etm_readl_cp14(off);
+ else
+ val = readl_relaxed(drvdata->base + off);
+ return val;
+}
Just looks like an extra layer or two or three of indirection of
absolutely no use or value. It seems to be done because someone
doesn't know how to get parameters to assembly inlines and
added all the switch statements for get #defined enumerators
from variables.
The code actually using these layered accessors look like this:
etmcr = etm_readl(drvdata, ETMCR);
I would rewrite the last function like this:
static inline unsigned int etm_readl(struct etm_drvdata *drvdata, u32 off)
{
u32 val;
if (drvdata->use_cp14)
asm_volatile()...
else
val = readl_relaxed(drvdata->base + off);
return val;
}
The asm_volatile() and parametrizing it is the trick. Just figure it out ;)
http://www.ethernut.de/en/documents/arm-inline-asm.html
> diff --git a/drivers/coresight/coresight-etm.h b/drivers/coresight/coresight-etm.h
(...)
> +struct etm_drvdata {
> + void __iomem *base;
> + struct device *dev;
> + struct coresight_device *csdev;
> + struct clk *clk;
> + spinlock_t spinlock;
> + int cpu;
> + int port_size;
> + u8 arch;
> + bool use_cp14;
> + bool enable;
> + bool sticky_enable;
> + bool boot_enable;
> + bool os_unlock;
> + u8 nr_addr_cmp;
> + u8 nr_cntr;
> + u8 nr_ext_inp;
> + u8 nr_ext_out;
> + u8 nr_ctxid_cmp;
> + u8 reset;
> + u32 etmccr;
> + u32 etmccer;
> + u32 traceid;
> + u32 mode;
> + u32 ctrl;
> + u32 trigger_event;
> + u32 startstop_ctrl;
> + u32 enable_event;
> + u32 enable_ctrl1;
> + u32 fifofull_level;
> + u8 addr_idx;
> + u32 addr_val[ETM_MAX_ADDR_CMP];
> + u32 addr_acctype[ETM_MAX_ADDR_CMP];
> + u32 addr_type[ETM_MAX_ADDR_CMP];
> + u8 cntr_idx;
> + u32 cntr_rld_val[ETM_MAX_CNTR];
> + u32 cntr_event[ETM_MAX_CNTR];
> + u32 cntr_rld_event[ETM_MAX_CNTR];
> + u32 cntr_val[ETM_MAX_CNTR];
> + u32 seq_12_event;
> + u32 seq_21_event;
> + u32 seq_23_event;
> + u32 seq_31_event;
> + u32 seq_32_event;
> + u32 seq_13_event;
> + u32 seq_curr_state;
> + u8 ctxid_idx;
> + u32 ctxid_val[ETM_MAX_CTXID_CMP];
> + u32 ctxid_mask;
> + u32 sync_freq;
> + u32 timestamp_event;
> +};
Unless all of this is self-evident I would kerneldoc it. If I don't
understand something of this, and if you don't, who's going to
understand it in a few years...
Apart from this it looks OK. Albeit big. But that may be unavoidable.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists