On 18 April 2017 at 18:18, Leo Yan <leo.yan@xxxxxxxxxx> wrote: > On Tue, Apr 18, 2017 at 11:40:50AM -0600, Mathieu Poirier wrote: >> On Thu, Apr 06, 2017 at 09:30:59PM +0800, Leo Yan wrote: >> > Coresight includes debug module and usually the module connects with CPU >> > debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has >> > description for related info in "Part H: External Debug". >> > >> > Chapter H7 "The Sample-based Profiling Extension" introduces several >> > sampling registers, e.g. we can check program counter value with >> > combined CPU exception level, secure state, etc. So this is helpful for >> > analysis CPU lockup scenarios, e.g. if one CPU has run into infinite >> > loop with IRQ disabled. In this case the CPU cannot switch context and >> > handle any interrupt (including IPIs), as the result it cannot handle >> > SMP call for stack dump. >> > >> > This patch is to enable coresight debug module, so firstly this driver >> > is to bind apb clock for debug module and this is to ensure the debug >> > module can be accessed from program or external debugger. And the driver >> > uses sample-based registers for debug purpose, e.g. when system triggers >> > panic, the driver will dump program counter and combined context >> > registers (EDCIDSR, EDVIDSR); by parsing context registers so can >> > quickly get to know CPU secure state, exception level, etc. >> > >> > Some of the debug module registers are located in CPU power domain, so >> > this requires the CPU power domain stays on when access related debug >> > registers, but the power management for CPU power domain is quite >> > dependent on SoC integration for power management. For the platforms >> > which with sane power controller implementations, this driver follows >> > the method to set EDPRCR to try to pull the CPU out of low power state >> > and then set 'no power down request' bit so the CPU has no chance to >> > lose power. >> > >> > If the SoC has not followed up this design well for power management >> > controller, the user should use the command line parameter or sysfs >> > to constrain all or partial idle states to ensure the CPU power >> > domain is enabled and access coresight CPU debug component safely. >> > >> > Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx> >> >> This is coming along well - a few comment below. In your next revision please >> add GKH to the 'To' list. > > Sure. Will send out in this week and add Greg in 'To' list. > >> > --- >> > drivers/hwtracing/coresight/Kconfig | 14 + >> > drivers/hwtracing/coresight/Makefile | 1 + >> > drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 ++++++++++++++++++++++ >> > 3 files changed, 682 insertions(+) >> > create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c >> > >> > diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig >> > index 130cb21..8d55d6d 100644 >> > --- a/drivers/hwtracing/coresight/Kconfig >> > +++ b/drivers/hwtracing/coresight/Kconfig >> > @@ -89,4 +89,18 @@ config CORESIGHT_STM >> > logging useful software events or data coming from various entities >> > in the system, possibly running different OSs >> > >> > +config CORESIGHT_CPU_DEBUG >> > + tristate "CoreSight CPU Debug driver" >> > + depends on ARM || ARM64 >> > + depends on DEBUG_FS >> > + help >> > + This driver provides support for coresight debugging module. This >> > + is primarily used to dump sample-based profiling registers when >> > + system triggers panic, the driver will parse context registers so >> > + can quickly get to know program counter (PC), secure state, >> > + exception level, etc. Before use debugging functionality, platform >> > + needs to ensure the clock domain and power domain are enabled >> > + properly, please refer Documentation/trace/coresight-cpu-debug.txt >> > + for detailed description and the example for usage. >> > + >> > endif >> > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile >> > index af480d9..433d590 100644 >> > --- a/drivers/hwtracing/coresight/Makefile >> > +++ b/drivers/hwtracing/coresight/Makefile >> > @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \ >> > coresight-etm4x-sysfs.o >> > obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o >> > obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o >> > +obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o >> > diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c >> > new file mode 100644 >> > index 0000000..8470e31 >> > --- /dev/null >> > +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c >> > @@ -0,0 +1,667 @@ >> > +/* >> > + * Copyright (c) 2017 Linaro Limited. All rights reserved. >> > + * >> > + * Author: Leo Yan <leo.yan@xxxxxxxxxx> >> > + * >> > + * This program is free software; you can redistribute it and/or modify it >> > + * under the terms of the GNU General Public License version 2 as published by >> > + * the Free Software Foundation. >> > + * >> > + * This program is distributed in the hope that it will be useful, but WITHOUT >> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for >> > + * more details. >> > + * >> > + * You should have received a copy of the GNU General Public License along with >> > + * this program. If not, see <http://www.gnu.org/licenses/>. >> > + * >> > + */ >> > +#include <linux/amba/bus.h> >> > +#include <linux/coresight.h> >> > +#include <linux/cpu.h> >> > +#include <linux/debugfs.h> >> > +#include <linux/delay.h> >> > +#include <linux/device.h> >> > +#include <linux/err.h> >> > +#include <linux/init.h> >> > +#include <linux/io.h> >> > +#include <linux/iopoll.h> >> > +#include <linux/kernel.h> >> > +#include <linux/module.h> >> > +#include <linux/moduleparam.h> >> > +#include <linux/pm_qos.h> >> > +#include <linux/slab.h> >> > +#include <linux/smp.h> >> > +#include <linux/types.h> >> > +#include <linux/uaccess.h> >> > + >> > +#include "coresight-priv.h" >> > + >> > +#define EDPCSR 0x0A0 >> > +#define EDCIDSR 0x0A4 >> > +#define EDVIDSR 0x0A8 >> > +#define EDPCSR_HI 0x0AC >> > +#define EDOSLAR 0x300 >> > +#define EDPRCR 0x310 >> > +#define EDPRSR 0x314 >> > +#define EDDEVID1 0xFC4 >> > +#define EDDEVID 0xFC8 >> > + >> > +#define EDPCSR_PROHIBITED 0xFFFFFFFF >> > + >> > +/* bits definition for EDPCSR */ >> > +#define EDPCSR_THUMB BIT(0) >> > +#define EDPCSR_ARM_INST_MASK GENMASK(31, 2) >> > +#define EDPCSR_THUMB_INST_MASK GENMASK(31, 1) >> > + >> > +/* bits definition for EDPRCR */ >> > +#define EDPRCR_COREPURQ BIT(3) >> > +#define EDPRCR_CORENPDRQ BIT(0) >> > + >> > +/* bits definition for EDPRSR */ >> > +#define EDPRSR_DLK BIT(6) >> > +#define EDPRSR_PU BIT(0) >> > + >> > +/* bits definition for EDVIDSR */ >> > +#define EDVIDSR_NS BIT(31) >> > +#define EDVIDSR_E2 BIT(30) >> > +#define EDVIDSR_E3 BIT(29) >> > +#define EDVIDSR_HV BIT(28) >> > +#define EDVIDSR_VMID GENMASK(7, 0) >> > + >> > +/* >> > + * bits definition for EDDEVID1:PSCROffset >> > + * >> > + * NOTE: armv8 and armv7 have different definition for the register, >> > + * so consolidate the bits definition as below: >> > + * >> > + * 0b0000 - Sample offset applies based on the instruction state, we >> > + * rely on EDDEVID to check if EDPCSR is implemented or not >> > + * 0b0001 - No offset applies. >> > + * 0b0010 - No offset applies, but do not use in AArch32 mode >> > + * >> > + */ >> > +#define EDDEVID1_PCSR_OFFSET_MASK GENMASK(3, 0) >> > +#define EDDEVID1_PCSR_OFFSET_INS_SET (0x0) >> > +#define EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 (0x2) >> > + >> > +/* bits definition for EDDEVID */ >> > +#define EDDEVID_PCSAMPLE_MODE GENMASK(3, 0) >> > +#define EDDEVID_IMPL_EDPCSR (0x1) >> > +#define EDDEVID_IMPL_EDPCSR_EDCIDSR (0x2) >> > +#define EDDEVID_IMPL_FULL (0x3) >> > + >> > +#define DEBUG_WAIT_SLEEP 1000 >> > +#define DEBUG_WAIT_TIMEOUT 32000 >> > + >> > +struct debug_drvdata { >> > + void __iomem *base; >> > e struct device *dev; >> > + int cpu; >> > + >> > + bool edpcsr_present; >> > + bool edcidsr_present; >> > + bool edvidsr_present; >> > + bool pc_has_offset; >> > + >> > + u32 edpcsr; >> > + u32 edpcsr_hi; >> > + u32 edprsr; >> > + u32 edvidsr; >> > + u32 edcidsr; >> > +}; >> > + >> > +static DEFINE_MUTEX(debug_lock); >> > +static DEFINE_PER_CPU(struct debug_drvdata *, debug_drvdata); >> > +static int debug_count; >> > +static struct dentry *debug_debugfs_dir; >> > + >> > +static bool debug_enable; >> > +module_param_named(enable, debug_enable, bool, 0600); >> > +MODULE_PARM_DESC(enable, "Knob to enable debug functionality " >> > + "(default is 0, which means is disabled by default)"); >> >> For this driver we have a debugFS interface so I question the validity of a >> kernel module parameter. Other than adding complexity to the code it offers no >> real added value. If a user is to insmod a module, it is just as easy to switch >> on the functionality using debugFS in a second step. > > This module parameter can be used for kernel command line, so > it's useful when user wants to dynamically turn on/off the > functionality at boot up time. > > Does this make sense for you? Removing this parameter is okay for > me, but this means users need to decide if use it by Kernel config > with static building in. This is a bit contradictory with before's > discussion. My hope was to use the kernel command line and the debugFS interface, avoiding the module parameter. Look at what the baycom_par and blacklist drivers are doing with the "__setup()" function and see if we can void a module parameter. If not then let it be, unless someone else has a better idea. [1]. drivers/net/hamradio/baycom_par.c [2]. drivers/se90/cio/blacklist.c > >> > +static void debug_os_unlock(struct debug_drvdata *drvdata) >> > +{ >> > + /* Unlocks the debug registers */ >> > + writel_relaxed(0x0, drvdata->base + EDOSLAR); >> >> Here the wmb is to make sure reordering (either at compile time or in the CPU) >> doesn't happend. You need a comment here otherwise checkpatch will complain. >> Speaking of which, did you run this through checkpatch? > > Right. Everytime I run checkpatch before sending patch and it has > complain for this. Will add comment. Sorry for imprecise working. > >> > + wmb(); >> > +} >> > + >> > +/* >> > + * According to ARM DDI 0487A.k, before access external debug >> > + * registers should firstly check the access permission; if any >> > + * below condition has been met then cannot access debug >> > + * registers to avoid lockup issue: >> > + * >> > + * - CPU power domain is powered off; >> > + * - The OS Double Lock is locked; >> > + * >> > + * By checking EDPRSR can get to know if meet these conditions. >> > + */ >> > +static bool debug_access_permitted(struct debug_drvdata *drvdata) >> > +{ >> > + /* CPU is powered off */ >> > + if (!(drvdata->edprsr & EDPRSR_PU)) >> > + return false; >> > + >> > + /* The OS Double Lock is locked */ >> > + if (drvdata->edprsr & EDPRSR_DLK) >> > + return false; >> > + >> > + return true; >> > +} >> > + >> > +static void debug_force_cpu_powered_up(struct debug_drvdata *drvdata) >> > +{ >> > + bool retried = false; >> > + u32 edprcr; >> > + >> > +try_again: >> > + >> > + /* >> > + * Send request to power management controller and assert >> > + * DBGPWRUPREQ signal; if power management controller has >> > + * sane implementation, it should enable CPU power domain >> > + * in case CPU is in low power state. >> > + */ >> > + edprcr = readl_relaxed(drvdata->base + EDPRCR); >> > + edprcr |= EDPRCR_COREPURQ; >> > + writel_relaxed(edprcr, drvdata->base + EDPRCR); >> > + >> > + /* Wait for CPU to be powered up (timeout~=32ms) */ >> > + if (readx_poll_timeout_atomic(readl_relaxed, drvdata->base + EDPRSR, >> > + drvdata->edprsr, (drvdata->edprsr & EDPRSR_PU), >> > + DEBUG_WAIT_SLEEP, DEBUG_WAIT_TIMEOUT)) { >> > + /* >> > + * Unfortunately the CPU cannot be powered up, so return >> > + * back and later has no permission to access other >> > + * registers. For this case, should disable CPU low power >> > + * states to ensure CPU power domain is enabled! >> > + */ >> > + pr_err("%s: power up request for CPU%d failed\n", >> > + __func__, drvdata->cpu); >> > + return; >> > + } >> > + >> > + /* >> > + * At this point the CPU is powered up, so set the no powerdown >> > + * request bit so we don't lose power and emulate power down. >> > + */ >> > + edprcr = readl_relaxed(drvdata->base + EDPRCR); >> > + edprcr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ; >> > + writel_relaxed(edprcr, drvdata->base + EDPRCR); >> > + >> > + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR); >> > + >> > + /* Bail out if CPU is powered up */ >> > + if (likely(drvdata->edprsr & EDPRSR_PU)) >> > + return; >> >> /* The core power domain got switched off on use, try again */ >> if (unlikely(!drvdata->edprsr & EDPRSR_PU)) >> goto try_again; >> >> I understand you don't want to introduce a infinite loop but if that happens >> here, something else has gone very wrong. The above readx_poll_timeout_atomic >> loop should take care of bailing out in case of problems. That way you also get >> to rid of the retried variable and the code is more simple. > > Okay. I will follow your method. Thinking the duration of CPU power > on/off is not same magnitude with this piece code, the infinite loop > will not happen. > >> > + >> > + /* >> > + * Handle race condition if CPU has been waken up but it sleeps >> > + * again if EDPRCR_CORENPDRQ has been flipped, so try to run >> > + * waken flow one more time. >> > + */ >> > + if (!retried) { >> > + retried = true; >> > + goto try_again; >> > + } >> > +} >> > + >> > +static void debug_read_regs(struct debug_drvdata *drvdata) >> > +{ >> > + u32 save_edprcr; >> > + >> > + CS_UNLOCK(drvdata->base); >> > + >> > + /* Unlock os lock */ >> > + debug_os_unlock(drvdata); >> > + >> > + /* Save EDPRCR register */ >> > + save_edprcr = readl_relaxed(drvdata->base + EDPRCR); >> > + >> > + /* >> > + * Ensure CPU power domain is enabled to let registers >> > + * are accessiable. >> > + */ >> > + debug_force_cpu_powered_up(drvdata); >> > + >> > + if (!debug_access_permitted(drvdata)) >> > + goto out; >> > + >> > + drvdata->edpcsr = readl_relaxed(drvdata->base + EDPCSR); >> > + >> > + /* >> > + * As described in ARM DDI 0487A.k, if the processing >> > + * element (PE) is in debug state, or sample-based >> > + * profiling is prohibited, EDPCSR reads as 0xFFFFFFFF; >> > + * EDCIDSR, EDVIDSR and EDPCSR_HI registers also become >> > + * UNKNOWN state. So directly bail out for this case. >> > + */ >> > + if (drvdata->edpcsr == EDPCSR_PROHIBITED) >> > + goto out; >> > + >> > + /* >> > + * A read of the EDPCSR normally has the side-effect of >> > + * indirectly writing to EDCIDSR, EDVIDSR and EDPCSR_HI; >> > + * at this point it's safe to read value from them. >> > + */ >> > + if (IS_ENABLED(CONFIG_64BIT)) >> > + drvdata->edpcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI); >> > + >> > + if (drvdata->edcidsr_present) >> > + drvdata->edcidsr = readl_relaxed(drvdata->base + EDCIDSR); >> > + >> > + if (drvdata->edvidsr_present) >> > + drvdata->edvidsr = readl_relaxed(drvdata->base + EDVIDSR); >> > + >> > +out: >> > + /* Restore EDPRCR register */ >> > + writel_relaxed(save_edprcr, drvdata->base + EDPRCR); >> > + >> > + CS_LOCK(drvdata->base); >> > +} >> > + >> > +static unsigned long debug_adjust_pc(struct debug_drvdata *drvdata) >> > +{ >> > + unsigned long arm_inst_offset = 0, thumb_inst_offset = 0; >> > + unsigned long pc; >> > + >> > + if (IS_ENABLED(CONFIG_64BIT)) >> > + return (unsigned long)drvdata->edpcsr_hi << 32 | >> > + (unsigned long)drvdata->edpcsr; >> > + >> > + pc = (unsigned long)drvdata->edpcsr; >> > + >> > + if (drvdata->pc_has_offset) { >> > + arm_inst_offset = 8; >> > + thumb_inst_offset = 4; >> > + } >> > + >> > + /* Handle thumb instruction */ >> > + if (pc & EDPCSR_THUMB) { >> > + pc = (pc & EDPCSR_THUMB_INST_MASK) - thumb_inst_offset; >> > + return pc; >> > + } >> > + >> > + /* >> > + * Handle arm instruction offset, if the arm instruction >> > + * is not 4 byte alignment then it's possible the case >> > + * for implementation defined; keep original value for this >> > + * case and print info for notice. >> > + */ >> > + if (pc & BIT(1)) >> > + pr_emerg("Instruction offset is implementation defined\n"); >> > + else >> > + pc = (pc & EDPCSR_ARM_INST_MASK) - arm_inst_offset; >> > + >> > + return pc; >> > +} >> > + >> > +static void debug_dump_regs(struct debug_drvdata *drvdata) >> > +{ >> > + unsigned long pc; >> > + >> > + pr_emerg("\tEDPRSR: %08x (Power:%s DLK:%s)\n", drvdata->edprsr, >> > + drvdata->edprsr & EDPRSR_PU ? "On" : "Off", >> > + drvdata->edprsr & EDPRSR_DLK ? "Lock" : "Unlock"); >> > + >> > + if (!debug_access_permitted(drvdata)) { >> > + pr_emerg("No permission to access debug registers!\n"); >> > + return; >> > + } >> > + >> > + if (drvdata->edpcsr == EDPCSR_PROHIBITED) { >> > + pr_emerg("CPU is in Debug state or profiling is prohibited!\n"); >> > + return; >> > + } >> > + >> > + pc = debug_adjust_pc(drvdata); >> > + pr_emerg("\tEDPCSR: [<%p>] %pS\n", (void *)pc, (void *)pc); >> > + >> > + if (drvdata->edcidsr_present) >> > + pr_emerg("\tEDCIDSR: %08x\n", drvdata->edcidsr); >> > + >> > + if (drvdata->edvidsr_present) >> > + pr_emerg("\tEDVIDSR: %08x (State:%s Mode:%s Width:%dbits VMID:%x)\n", >> > + drvdata->edvidsr, >> > + drvdata->edvidsr & EDVIDSR_NS ? "Non-secure" : "Secure", >> > + drvdata->edvidsr & EDVIDSR_E3 ? "EL3" : >> > + (drvdata->edvidsr & EDVIDSR_E2 ? "EL2" : "EL1/0"), >> > + drvdata->edvidsr & EDVIDSR_HV ? 64 : 32, >> > + drvdata->edvidsr & (u32)EDVIDSR_VMID); >> > +} >> > + >> > +static void debug_init_arch_data(void *info) >> > +{ >> > + struct debug_drvdata *drvdata = info; >> > + u32 mode, pcsr_offset; >> > + u32 eddevid, eddevid1; >> > + >> > + CS_UNLOCK(drvdata->base); >> > + >> > + /* Read device info */ >> > + eddevid = readl_relaxed(drvdata->base + EDDEVID); >> > + eddevid1 = readl_relaxed(drvdata->base + EDDEVID1); >> > + >> > + CS_LOCK(drvdata->base); >> > + >> > + /* Parse implementation feature */ >> > + mode = eddevid & EDDEVID_PCSAMPLE_MODE; >> > + pcsr_offset = eddevid1 & EDDEVID1_PCSR_OFFSET_MASK; >> > + >> > + drvdata->edpcsr_present = false; >> > + drvdata->edcidsr_present = false; >> > + drvdata->edvidsr_present = false; >> > + drvdata->pc_has_offset = false; >> > + >> > + switch (mode) { >> > + case EDDEVID_IMPL_FULL: >> > + drvdata->edvidsr_present = true; >> > + /* Fall through */ >> > + case EDDEVID_IMPL_EDPCSR_EDCIDSR: >> > + drvdata->edcidsr_present = true; >> > + /* Fall through */ >> > + case EDDEVID_IMPL_EDPCSR: >> > + /* >> > + * In ARM DDI 0487A.k, the EDDEVID1.PCSROffset is used to >> > + * define if has the offset for PC sampling value; if read >> > + * back EDDEVID1.PCSROffset == 0x2, then this means the debug >> > + * module does not sample the instruction set state when >> > + * armv8 CPU in AArch32 state. >> > + */ >> > + drvdata->edpcsr_present = (IS_ENABLED(CONFIG_64BIT) || >> > + (pcsr_offset != EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32)); >> > + >> > + drvdata->pc_has_offset = >> > + (pcsr_offset == EDDEVID1_PCSR_OFFSET_INS_SET); >> > + break; >> > + default: >> > + break; >> > + } >> > +} >> > + >> > +/* >> > + * Dump out information on panic. >> > + */ >> > +static int debug_notifier_call(struct notifier_block *self, >> > + unsigned long v, void *p) >> > +{ >> > + int cpu; >> > + struct debug_drvdata *drvdata; >> > + >> > + pr_emerg("ARM external debug module:\n"); >> > + >> > + for_each_possible_cpu(cpu) { >> > + drvdata = per_cpu(debug_drvdata, cpu); >> > + if (!drvdata) >> > + continue; >> > + >> > + pr_emerg("CPU[%d]:\n", drvdata->cpu); >> > + >> > + debug_read_regs(drvdata); >> > + debug_dump_regs(drvdata); >> > + } >> > + >> > + return 0; >> > +} >> > + >> > +static struct notifier_block debug_notifier = { >> > + .notifier_call = debug_notifier_call, >> > +}; >> > + >> > +static int debug_enable_func(void) >> > +{ >> > + struct debug_drvdata *drvdata; >> > + int cpu; >> > + >> > + for_each_possible_cpu(cpu) { >> > + drvdata = per_cpu(debug_drvdata, cpu); >> > + if (!drvdata) >> > + continue; >> > + >> > + pm_runtime_get_sync(drvdata->dev); >> > + } >> > + >> > + return atomic_notifier_chain_register(&panic_notifier_list, >> > + &debug_notifier); >> > +} >> > + >> > +static int debug_disable_func(void) >> > +{ >> > + struct debug_drvdata *drvdata; >> > + int cpu; >> > + >> > + for_each_possible_cpu(cpu) { >> > + drvdata = per_cpu(debug_drvdata, cpu); >> > + if (!drvdata) >> > + continue; >> > + >> > + pm_runtime_put(drvdata->dev); >> > + } >> > + >> > + return atomic_notifier_chain_unregister(&panic_notifier_list, >> > + &debug_notifier); >> > +} >> > + >> > +static ssize_t debug_func_knob_write(struct file *f, >> > + const char __user *buf, size_t count, loff_t *ppos) >> > +{ >> > + u8 val; >> > + int ret; >> > + >> > + ret = kstrtou8_from_user(buf, count, 2, &val); >> > + if (ret) >> > + return ret; >> > + >> > + mutex_lock(&debug_lock); >> > + >> > + if (val == debug_enable) >> > + goto out; >> > + >> > + if (val) >> > + ret = debug_enable_func(); >> > + else >> > + ret = debug_disable_func(); >> >> I don't think you need to install the handler every time the functionality is >> switched on/off. I suggest to install the handler at boot time (or module >> insertion time) and in the notifier handler, check the debug_enable flag before >> moving on with the output. > > Will do. > >> > + >> > + if (ret) { >> > + pr_err("%s: unable to %s debug function: %d\n", >> > + __func__, val ? "enable" : "disable", ret); >> > + goto err; >> > + } >> > + >> > + debug_enable = val; >> >> Using a true/false value is probably better here. That way you don't end up >> with miscellaneous values in debugFS. > > From my testing here will not assign other values rather than 0 or 1. > This is controlled by function kstrtou8_from_user(), the 3rd parameter > '2' is to define the value range [0..1], so other values will report > error after return back from kstrtou8_from_user(). Ok that works. > >> > +out: >> > + ret = count; >> > +err: >> > + mutex_unlock(&debug_lock); >> > + return ret; >> > +} >> > + >> > +static ssize_t debug_func_knob_read(struct file *f, >> > + char __user *ubuf, size_t count, loff_t *ppos) >> > +{ >> > + ssize_t ret; >> > + char buf[2]; >> > + >> > + mutex_lock(&debug_lock); >> > + >> > + buf[0] = '0' + debug_enable; >> > + buf[1] = '\n'; >> >> snprintf() > > Will fix. > >> > + ret = simple_read_from_buffer(ubuf, count, ppos, buf, sizeof(buf)); >> > + >> > + mutex_unlock(&debug_lock); >> > + return ret; >> > +} >> > + >> > +static const struct file_operations debug_func_knob_fops = { >> > + .open = simple_open, >> > + .read = debug_func_knob_read, >> > + .write = debug_func_knob_write, >> > +}; >> > + >> > +static int debug_func_init(void) >> > +{ >> > + struct dentry *file; >> > + int ret; >> > + >> > + /* Create debugfs node */ >> > + debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL); >> > + if (!debug_debugfs_dir) { >> > + pr_err("%s: unable to create debugfs directory\n", __func__); >> > + return -ENOMEM; >> > + } >> > + >> > + file = debugfs_create_file("enable", S_IRUGO | S_IWUSR, >> > + debug_debugfs_dir, NULL, &debug_func_knob_fops); >> >> Please align this properly. > > Will fix. Thanks a lot for detailed reviewing. > > [...] > > Thanks, > Leo Yan -- To unsubscribe from this list: send the line "unsubscribe linux-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
