Re: [PATCH RESEND 3/4] docs: Add HiSilicon PTT device driver documentation
On Sat, Apr 17, 2021 at 06:17:10PM +0800, Yicong Yang wrote: > Document the introduction and usage of HiSilicon PTT device driver. > > Signed-off-by: Yicong Yang > --- > Documentation/trace/hisi-ptt.rst | 326 > +++ > 1 file changed, 326 insertions(+) > create mode 100644 Documentation/trace/hisi-ptt.rst > > diff --git a/Documentation/trace/hisi-ptt.rst > b/Documentation/trace/hisi-ptt.rst > new file mode 100644 > index 000..f093846 > --- /dev/null > +++ b/Documentation/trace/hisi-ptt.rst > @@ -0,0 +1,326 @@ > [...] > +On Kunpeng 930 SoC, the PCIe Root Complex is composed of several > +PCIe cores. Each PCIe core includes several Root Ports and a PTT > +RCiEP, like below. The PTT device is capable of tuning and > +tracing the link of the PCIe core. > +:: > + +--Core 0---+ > + | | [ PTT ] | > + | | [Root Port]---[Endpoint] > + | | [Root Port]---[Endpoint] > + | | [Root Port]---[Endpoint] > +Root Complex |--Core 1---+ > + | | [ PTT ] | > + | | [Root Port]---[ Switch ]---[Endpoint] > + | | [Root Port]---[Endpoint] `-[Endpoint] > + | | [Root Port]---[Endpoint] > + +---+ > + > +The PTT device driver cannot be loaded if debugfs is not mounted. This can't be right can it? Obviously debugfs must be enabled but why mounted? > +Each PTT device will be presented under /sys/kernel/debugfs/hisi_ptt > +as its root directory, with name of its BDF number. > +:: > + > +/sys/kernel/debug/hisi_ptt/::. > + > +Tune > + > + > +PTT tune is designed for monitoring and adjusting PCIe link parameters > (events). > +Currently we support events in 4 classes. The scope of the events > +covers the PCIe core to which the PTT device belongs. > + > +Each event is presented as a file under $(PTT root dir)/$(BDF)/tune, and > +mostly a simple open/read/write/close cycle will be used to tune > +the event. > +:: > +$ cd /sys/kernel/debug/hisi_ptt/$(BDF)/tune > +$ ls > +qos_tx_cplqos_tx_npqos_tx_p > +tx_path_rx_req_alloc_buf_level > +tx_path_tx_req_alloc_buf_level > +$ cat qos_tx_dp > +1 > +$ echo 2 > qos_tx_dp > +$ cat qos_tx_dp > +2 > + > +Current value (numerical value) of the event can be simply read > +from the file, and the desired value written to the file to tune. I saw that this RFC asks about whether debugfs is an appropriate interface for the *tracing* capability of the platform. Have similar questions been raised about the tuning interfaces? It looks to me like tuning could be handled entirely using sysfs attributes. I think trying to handle these mostly decoupled feature in the same place is likely to be a mistake. Daniel.
Re: [PATCH v3] kdb: Refactor env variables get/set code
On Mon, Feb 08, 2021 at 01:32:22PM +0530, Sumit Garg wrote:
>
> Add two new kdb environment access methods as kdb_setenv() and
> kdb_printenv() in order to abstract out environment access code
> from kdb command functions.
>
> Also, replace (char *)0 with NULL as an initializer for environment
> variables array.
Neither (char *)0 nor NULL are great initializers since, for static
data, these are the default value anyway. Better to just give the array
a explicit dimension and be done.
However... that's a fairly small change and I can fix it up when
applying this.
Daniel.
> Signed-off-by: Sumit Garg
> Reviewed-by: Douglas Anderson
> ---
>
> Changes in v3:
> - Remove redundant '\0' char assignment.
> - Pick up Doug's review tag.
>
> Changes in v2:
> - Get rid of code motion to separate kdb_env.c file.
> - Replace (char *)0 with NULL.
> - Use kernel-doc style function comments.
> - s/kdb_prienv/kdb_printenv/
>
> kernel/debug/kdb/kdb_main.c | 164
>
> 1 file changed, 91 insertions(+), 73 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index 588062a..69b8f55 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -142,40 +142,40 @@ static const int __nkdb_err = ARRAY_SIZE(kdbmsgs);
>
> static char *__env[] = {
> #if defined(CONFIG_SMP)
> - "PROMPT=[%d]kdb> ",
> + "PROMPT=[%d]kdb> ",
> #else
> - "PROMPT=kdb> ",
> + "PROMPT=kdb> ",
> #endif
> - "MOREPROMPT=more> ",
> - "RADIX=16",
> - "MDCOUNT=8",/* lines of md output */
> - KDB_PLATFORM_ENV,
> - "DTABCOUNT=30",
> - "NOSECT=1",
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> - (char *)0,
> + "MOREPROMPT=more> ",
> + "RADIX=16",
> + "MDCOUNT=8",/* lines of md output */
> + KDB_PLATFORM_ENV,
> + "DTABCOUNT=30",
> + "NOSECT=1",
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> + NULL,
> };
>
> static const int __nenv = ARRAY_SIZE(__env);
> @@ -318,6 +318,63 @@ int kdbgetintenv(const char *match, int *value)
> }
>
> /*
> + * kdb_setenv() - Alter an existing environment variable or create a new one.
> + * @var: Name of the variable
> + * @val: Value of the variable
> + *
> + * Return: Zero on success, a kdb diagnostic on failure.
> + */
> +static int kdb_setenv(const char *var, const char *val)
> +{
> + int i;
> + char *ep;
> + size_t varlen, vallen;
> +
> + varlen = strlen(var);
> + vallen = strlen(val);
> + ep = kdballocenv(varlen + vallen + 2);
> + if (ep == (char *)0)
> + return KDB_ENVBUFFULL;
> +
> + sprintf(ep, "%s=%s", var, val);
> +
> + for (i = 0; i < __nenv; i++) {
> + if (__env[i]
> + && ((strncmp(__env[i], var, varlen) == 0)
> +&& ((__env[i][varlen] == '\0')
> + || (__env[i][varlen] == '=' {
> + __env[i] = ep;
> + return 0;
> + }
> + }
> +
> + /*
> + * Wasn't existing variable. Fit into slot.
> + */
> + for (i = 0; i < __nenv-1; i++) {
> + if (__env[i] == (char *)0) {
> + __env[i] = ep;
> + return 0;
> + }
> + }
> +
> + return KDB_ENVFULL;
> +}
> +
> +/*
> + * kdb_printenv() - Display the current environment variables.
> + */
> +static void kdb_printenv(void)
> +{
> + int i;
> +
> + for (i = 0; i < __nenv; i++) {
> + if (__env[i])
> + kdb_printf("%s\n", __env[i]);
> + }
> +}
> +
> +/*
> * kdbgetularg - This function will convert a numeric string into an
> * unsigned long value.
> * Parameters:
> @@ -374,10 +431,6 @@ int kdbgetu64arg(const char *arg, u64 *value)
> */
> int kdb_set(int argc, const char **argv)
> {
> - int i;
> - char *ep;
> - size_t varlen, vallen;
> -
> /*
>* we can be invoked two ways:
>* set var=valueargv[1]="var", argv[2]="value"
> @@ -422,37 +475,7 @@ int kdb_set(int argc, const char **argv)
>* Tokenizer squashed the '=' sign. argv[1] is variable
>* name, argv[2] = value.
>*/
> - varlen = strlen(argv[1]);
> - vallen = strlen(argv[2]);
> - ep = kdballocenv(varlen + vallen + 2);
> - if (ep == (char *)0)
> - return KDB_ENVBUFFULL;
> -
> - sprintf(ep, "%s=%s",
Re: [PATCH v2] sched/debug: Use sched_debug_lock to serialize use of cgroup_path[] only
On Mon, Mar 29, 2021 at 03:32:35PM -0400, Waiman Long wrote: > The handling of sysrq keys should normally be done in an user context > except when MAGIC_SYSRQ_SERIAL is set and the magic sequence is typed > in a serial console. This seems to be a poor summary of the typical calling context for handle_sysrq() except in the trivial case of using /proc/sysrq-trigger. For example on my system then the backtrace when I do sysrq-h on a USB keyboard shows us running from a softirq handler and with interrupts locked. Note also that the interrupt lock is present even on systems that handle keyboard input from a kthread due to the interrupt lock in report_input_key(). > Currently in print_cpu() of kernel/sched/debug.c, sched_debug_lock is taken > with interrupt disabled for the whole duration of the calls to print_*_stats() > and print_rq() which could last for the quite some time if the information > dump > happens on the serial console. > > If the system has many cpus and the sched_debug_lock is somehow busy > (e.g. parallel sysrq-t), the system may hit a hard lockup panic, like > The purpose of sched_debug_lock is to serialize the use of the global > cgroup_path[] buffer in print_cpu(). The rests of the printk() calls > don't need serialization from sched_debug_lock. > > Calling printk() with interrupt disabled can still be > problematic. Allocating a stack buffer of PATH_MAX bytes is not > feasible. So a compromised solution is used where a small stack buffer > is allocated for pathname. If the actual pathname is short enough, it > is copied to the stack buffer with sched_debug_lock release afterward > before printk(). Otherwise, the global group_path[] buffer will be > used with sched_debug_lock held until after printk(). Does this actually fix the problem in any circumstance except when the sysrq is triggered using /proc/sysrq-trigger? Daniel. >
Re: [PATCH v6 2/4] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
On Sun, Mar 28, 2021 at 11:24:17PM +0800, cy_huang wrote: > From: ChiYuan Huang > > Adds DT binding document for Richtek RT4831 backlight. > > Signed-off-by: ChiYuan Huang Reviewed-by: Daniel Thompson
Re: [PATCH v6 4/4] backlight: rt4831: Adds support for Richtek RT4831 backlight
On Sun, Mar 28, 2021 at 11:24:19PM +0800, cy_huang wrote: > From: ChiYuan Huang > > Adds support for Richtek RT4831 backlight. > > Signed-off-by: ChiYuan Huang > --- > since v6 > - Fix Kconfig typo. > - Remove internal mutex lock. > - Add the prefix for max brightness. > - rename init_device_properties to parse_backlight_properties. > - Remove some warning message if default value is adopted. > - Add backlight property scale to LINEAR mapping. > - Fix regmap get to check NULL not IS_ERR. > --- > drivers/video/backlight/Kconfig| 8 ++ > drivers/video/backlight/Makefile | 1 + > drivers/video/backlight/rt4831-backlight.c | 203 > + > 3 files changed, 212 insertions(+) > create mode 100644 drivers/video/backlight/rt4831-backlight.c > > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > index d83c87b..de96441 100644 > --- a/drivers/video/backlight/Kconfig > +++ b/drivers/video/backlight/Kconfig > @@ -289,6 +289,14 @@ config BACKLIGHT_QCOM_WLED > If you have the Qualcomm PMIC, say Y to enable a driver for the > WLED block. Currently it supports PM8941 and PMI8998. > > +config BACKLIGHT_RT4831 > + tristate "Richtek RT4831 Backlight Driver" > + depends on MFD_RT4831 > + help > + This enables support for Richtek RT4831 Backlight driver. > + It's common used to drive the display WLED. There're four channels Nitpicking but I was expecting the original typo be converted to "commonly". With that addressed: Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 1/2] sched/debug: Don't disable IRQ when acquiring sched_debug_lock
On Sat, Mar 27, 2021 at 07:25:28PM -0400, Waiman Long wrote:
> The sched_debug_lock was used only in print_cpu(). The
> print_cpu() function has two callers - sched_debug_show() and
> sysrq_sched_debug_show(). Both of them are invoked by user action
> (sched_debug file and sysrq-t). As print_cpu() won't be called from
> interrupt context at all, there is no point in disabling IRQ when
> acquiring sched_debug_lock.
This looks like it introduces a deadlock risk if sysrq-t triggers from an
interrupt context. Has the behaviour of sysrq changed recently or will
tools like MAGIC_SYSRQ_SERIAL still trigger from interrupt context?
Daniel.
>
> Besides, if the system has many cpus and the sched_debug_lock is somehow
> busy (e.g. parallel sysrq-t), the system may hit a hard lockup panic, like
>
> [ 7809.796262] Kernel panic - not syncing: Hard LOCKUP
> [ 7809.796264] CPU: 13 PID: 79867 Comm: reproducer.sh Kdump: loaded Tainted:
> G I - - - 4.18.0-301.el8.x86_64 #1
> [ 7809.796264] Hardware name: Dell Inc. PowerEdge R640/0W23H8, BIOS 1.4.9
> 06/29/2018
> [ 7809.796265] Call Trace:
> [ 7809.796265]
> [ 7809.796266] dump_stack+0x5c/0x80
> [ 7809.796266] panic+0xe7/0x2a9
> [ 7809.796267] nmi_panic.cold.9+0xc/0xc
> [ 7809.796267] watchdog_overflow_callback.cold.7+0x5c/0x70
> [ 7809.796268] __perf_event_overflow+0x52/0xf0
> [ 7809.796268] handle_pmi_common+0x204/0x2a0
> [ 7809.796269] ? __set_pte_vaddr+0x32/0x50
> [ 7809.796269] ? __native_set_fixmap+0x24/0x30
> [ 7809.796270] ? ghes_copy_tofrom_phys+0xd3/0x1c0
> [ 7809.796271] intel_pmu_handle_irq+0xbf/0x160
> [ 7809.796271] perf_event_nmi_handler+0x2d/0x50
> [ 7809.796272] nmi_handle+0x63/0x110
> [ 7809.796272] default_do_nmi+0x49/0x100
> [ 7809.796273] do_nmi+0x17e/0x1e0
> [ 7809.796273] end_repeat_nmi+0x16/0x6f
> [ 7809.796274] RIP: 0010:native_queued_spin_lock_slowpath+0x5b/0x1d0
> [ 7809.796275] Code: 6d f0 0f ba 2f 08 0f 92 c0 0f b6 c0 c1 e0 08 89 c2 8b 07
> 30 e4 09 d0 a9 00 01 ff ff 75 47 85 c0 74 0e 8b 07 84 c0 74 08 f3 90 <8b> 07
> 84 c0 75 f8 b8 01 00 00 00 66 89 07 c3 8b 37 81 fe 00 01 00
> [ 7809.796276] RSP: 0018:aa54cd887df8 EFLAGS: 0002
> [ 7809.796277] RAX: 0101 RBX: 0246 RCX:
>
> [ 7809.796278] RDX: RSI: RDI:
> 936b66d0
> [ 7809.796278] RBP: 9301fb40 R08: 0004 R09:
> 004f
> [ 7809.796279] R10: R11: aa54cd887cc0 R12:
> 907fd0a29ec0
> [ 7809.796280] R13: R14: 926ab7c0 R15:
>
> [ 7809.796280] ? native_queued_spin_lock_slowpath+0x5b/0x1d0
> [ 7809.796281] ? native_queued_spin_lock_slowpath+0x5b/0x1d0
> [ 7809.796281]
> [ 7809.796282] _raw_spin_lock_irqsave+0x32/0x40
> [ 7809.796283] print_cpu+0x261/0x7c0
> [ 7809.796283] sysrq_sched_debug_show+0x34/0x50
> [ 7809.796284] sysrq_handle_showstate+0xc/0x20
> [ 7809.796284] __handle_sysrq.cold.11+0x48/0xfb
> [ 7809.796285] write_sysrq_trigger+0x2b/0x30
> [ 7809.796285] proc_reg_write+0x39/0x60
> [ 7809.796286] vfs_write+0xa5/0x1a0
> [ 7809.796286] ksys_write+0x4f/0xb0
> [ 7809.796287] do_syscall_64+0x5b/0x1a0
> [ 7809.796287] entry_SYSCALL_64_after_hwframe+0x65/0xca
> [ 7809.796288] RIP: 0033:0x7fabe4ceb648
>
> It is not to disable IRQ for so long that a hard lockup panic is
> triggered. Fix that by not disabling IRQ when taking sched_debug_lock.
>
> Fixes: efe25c2c7b3a ("sched: Reinstate group names in /proc/sched_debug")
> Signed-off-by: Waiman Long
> ---
> kernel/sched/debug.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
> index 486f403a778b..c4ae8a0853a1 100644
> --- a/kernel/sched/debug.c
> +++ b/kernel/sched/debug.c
> @@ -666,7 +666,6 @@ void print_dl_rq(struct seq_file *m, int cpu, struct
> dl_rq *dl_rq)
> static void print_cpu(struct seq_file *m, int cpu)
> {
> struct rq *rq = cpu_rq(cpu);
> - unsigned long flags;
>
> #ifdef CONFIG_X86
> {
> @@ -717,13 +716,13 @@ do {
> \
> }
> #undef P
>
> - spin_lock_irqsave(_debug_lock, flags);
> + spin_lock(_debug_lock);
> print_cfs_stats(m, cpu);
> print_rt_stats(m, cpu);
> print_dl_stats(m, cpu);
>
> print_rq(m, rq, cpu);
> - spin_unlock_irqrestore(_debug_lock, flags);
> + spin_unlock(_debug_lock);
> SEQ_printf(m, "\n");
> }
>
> --
> 2.18.1
>
Re: [PATCH v5 5/6] backlight: rt4831: Adds support for Richtek RT4831 backlight
On Thu, Mar 25, 2021 at 04:22:11PM +0800, ChiYuan Huang wrote: > Dear reviewers: > >Didn't get any response about this backlight patch. > Is there any part need to be refined? Thanks for the reminders and sorry for the delay. Have just replied to the original message! Daniel.
Re: [PATCH v5 5/6] backlight: rt4831: Adds support for Richtek RT4831 backlight
On Thu, Dec 17, 2020 at 11:00:43PM +0800, cy_huang wrote:
> From: ChiYuan Huang
>
> Adds support for Richtek RT4831 backlight.
>
> Signed-off-by: ChiYuan Huang
Looks ok but there are a few minor niggles.
> ---
> drivers/video/backlight/Kconfig| 8 ++
> drivers/video/backlight/Makefile | 1 +
> drivers/video/backlight/rt4831-backlight.c | 219
> +
> 3 files changed, 228 insertions(+)
> create mode 100644 drivers/video/backlight/rt4831-backlight.c
>
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index d83c87b..666bdb0 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -289,6 +289,14 @@ config BACKLIGHT_QCOM_WLED
> If you have the Qualcomm PMIC, say Y to enable a driver for the
> WLED block. Currently it supports PM8941 and PMI8998.
>
> +config BACKLIGHT_RT4831
> + tristate "Richtek RT4831 Backlight Driver"
> + depends on MFD_RT4831
> + help
> + This enables support for Richtek RT4831 Backlight driver.
> + It's commont used to drive the display WLED. There're four channels
^^^
> diff --git a/drivers/video/backlight/rt4831-backlight.c
> b/drivers/video/backlight/rt4831-backlight.c
> new file mode 100644
> index ..816c4d6
> --- /dev/null
> +++ b/drivers/video/backlight/rt4831-backlight.c
> @@ -0,0 +1,219 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define RT4831_REG_BLCFG 0x02
> +#define RT4831_REG_BLDIML0x04
> +#define RT4831_REG_ENABLE0x08
> +
> +#define BL_MAX_BRIGHTNESS2048
Would be better with a prefix.
> +
> +#define RT4831_BLOVP_MASKGENMASK(7, 5)
> +#define RT4831_BLOVP_SHIFT 5
> +#define RT4831_BLPWMEN_MASK BIT(0)
> +#define RT4831_BLEN_MASK BIT(4)
> +#define RT4831_BLCH_MASK GENMASK(3, 0)
> +#define RT4831_BLDIML_MASK GENMASK(2, 0)
> +#define RT4831_BLDIMH_MASK GENMASK(10, 3)
> +#define RT4831_BLDIMH_SHIFT 3
> +
> +struct rt4831_priv {
> + struct regmap *regmap;
> + struct mutex lock;
Locks aren't very common in backlight drivers. Why isn't the ops_lock
sufficient?
> + struct backlight_device *bl;
> +};
> +
> +static int rt4831_bl_update_status(struct backlight_device *bl_dev)
> +{
> + struct rt4831_priv *priv = bl_get_data(bl_dev);
> + int brightness = backlight_get_brightness(bl_dev);
> + unsigned int enable = brightness ? RT4831_BLEN_MASK : 0;
> + u8 v[2];
> + int ret;
> +
> + mutex_lock(>lock);
> +
> + if (brightness) {
> + v[0] = (brightness - 1) & RT4831_BLDIML_MASK;
> + v[1] = ((brightness - 1) & RT4831_BLDIMH_MASK) >>
> RT4831_BLDIMH_SHIFT;
> +
> + ret = regmap_raw_write(priv->regmap, RT4831_REG_BLDIML, v,
> sizeof(v));
> + if (ret)
> + goto unlock;
> + }
> +
> + ret = regmap_update_bits(priv->regmap, RT4831_REG_ENABLE,
> RT4831_BLEN_MASK, enable);
> +
> +unlock:
> + mutex_unlock(>lock);
> + return ret;
> +}
> +
> +static int rt4831_bl_get_brightness(struct backlight_device *bl_dev)
> +{
> + struct rt4831_priv *priv = bl_get_data(bl_dev);
> + unsigned int val;
> + u8 v[2];
> + int ret;
> +
> + mutex_lock(>lock);
> +
> + ret = regmap_read(priv->regmap, RT4831_REG_ENABLE, );
> + if (ret)
> + return ret;
Deadlock.
> +
> + if (!(val & RT4831_BLEN_MASK)) {
> + ret = 0;
> + goto unlock;
> + }
> +
> + ret = regmap_raw_read(priv->regmap, RT4831_REG_BLDIML, v, sizeof(v));
> + if (ret)
> + goto unlock;
> +
> + ret = (v[1] << RT4831_BLDIMH_SHIFT) + (v[0] & RT4831_BLDIML_MASK) + 1;
> +
> +unlock:
> + mutex_unlock(>lock);
> + return ret;
> +}
> +
> +static const struct backlight_ops rt4831_bl_ops = {
> + .options = BL_CORE_SUSPENDRESUME,
> + .update_status = rt4831_bl_update_status,
> + .get_brightness = rt4831_bl_get_brightness,
> +};
> +
> +static int rt4831_init_device_properties(struct rt4831_priv *priv, struct
> device *dev,
This is not the idiomatic name usually used for this type of function.
In fact since this driver purely uses device properties then this code
could just be merged into the probe function.
> + struct backlight_properties *bl_props)
> +{
> + u8 propval;
> + u32 brightness;
> + unsigned int val = 0;
> + int ret;
> +
> + /* common properties */
> + ret = device_property_read_u32(dev, "max-brightness", );
> + if (ret) {
> + dev_warn(dev, "max-brightness DT property missing, use HW max
> as default\n");
Does there need to be a warning on this?
It's code pattern is common but the DT docs have formalized a lot
recently. The DT docs in patch 1 say these are optional... so
why does it
[PATCH] kgdbts: Switch to do_sys_openat2() for breakpoint testing
Currently kgdbts can get stuck waiting for do_sys_open() to be called
in some of the current tests. This is because C compilers often
automatically inline this function, which is a very thin wrapper around
do_sys_openat2(), into some of its callers. gcc-10 does this on (at least)
both x86 and arm64.
We can fix the test suite by placing the breakpoints on do_sys_openat2()
instead since that isn't (currently) inlined. However do_sys_openat2() is
a static function so we cannot simply use an addressof. Since we are
testing debug machinery it is acceptable to use kallsyms to lookup a
suitable address because this is more or less what kdb does in the same
circumstances. Re-implement lookup_addr() to be based on kallsyms rather
than function pointers.
Signed-off-by: Daniel Thompson
---
Notes:
So less than a week ago I said I had nothing pending for kgdbts.
That was entirely true when I said it... but then this came up.
drivers/misc/kgdbts.c | 48 ++-
1 file changed, 29 insertions(+), 19 deletions(-)
diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
index 945701bce5536..2679374ca8361 100644
--- a/drivers/misc/kgdbts.c
+++ b/drivers/misc/kgdbts.c
@@ -92,6 +92,7 @@
#include
#include
#include
+#include
#include
@@ -200,21 +201,30 @@ static noinline void kgdbts_break_test(void)
v2printk("kgdbts: breakpoint complete\n");
}
-/* Lookup symbol info in the kernel */
+/*
+ * This is a cached wrapper for kallsyms_lookup_name().
+ *
+ * The cache is a big win for several tests. For example it more the doubles
+ * the cycles per second during the sys_open test. This is not theoretic,
+ * the performance improvement shows up at human scale, especially when
+ * testing using emulators.
+ *
+ * Obviously neither re-entrant nor thread-safe but that is OK since it
+ * can only be called from the debug trap (and therefore all other CPUs
+ * are halted).
+ */
static unsigned long lookup_addr(char *arg)
{
- unsigned long addr = 0;
-
- if (!strcmp(arg, "kgdbts_break_test"))
- addr = (unsigned long)kgdbts_break_test;
- else if (!strcmp(arg, "sys_open"))
- addr = (unsigned long)do_sys_open;
- else if (!strcmp(arg, "kernel_clone"))
- addr = (unsigned long)kernel_clone;
- else if (!strcmp(arg, "hw_break_val"))
- addr = (unsigned long)_break_val;
- addr = (unsigned long) dereference_function_descriptor((void *)addr);
- return addr;
+ static char cached_arg[KSYM_NAME_LEN];
+ static unsigned long cached_addr;
+
+ if (strcmp(arg, cached_arg)) {
+ strscpy(cached_arg, arg, KSYM_NAME_LEN);
+ cached_addr = kallsyms_lookup_name(arg);
+ }
+
+ return (unsigned long)dereference_function_descriptor(
+ (void *)cached_addr);
}
static void break_helper(char *bp_type, char *arg, unsigned long vaddr)
@@ -310,7 +320,7 @@ static int check_and_rewind_pc(char *put_str, char *arg)
if (arch_needs_sstep_emulation && sstep_addr &&
ip + offset == sstep_addr &&
- ((!strcmp(arg, "sys_open") || !strcmp(arg, "kernel_clone" {
+ ((!strcmp(arg, "do_sys_openat2") || !strcmp(arg, "kernel_clone"
{
/* This is special case for emulated single step */
v2printk("Emul: rewind hit single step bp\n");
restart_from_top_after_write = 1;
@@ -619,14 +629,14 @@ static struct test_struct do_kernel_clone_test[] = {
*/
static struct test_struct sys_open_test[] = {
{ "?", "S0*" }, /* Clear break points */
- { "sys_open", "OK", sw_break, }, /* set sw breakpoint */
+ { "do_sys_openat2", "OK", sw_break, }, /* set sw breakpoint */
{ "c", "T0*", NULL, get_thread_id_continue }, /* Continue */
- { "sys_open", "OK", sw_rem_break }, /*remove breakpoint */
- { "g", "sys_open", NULL, check_and_rewind_pc }, /* check location */
+ { "do_sys_openat2", "OK", sw_rem_break }, /*remove breakpoint */
+ { "g", "do_sys_openat2", NULL, check_and_rewind_pc }, /* check location
*/
{ "write", "OK", write_regs, emul_reset }, /* Write registers */
{ "s", "T0*", emul_sstep_get, emul_sstep_put }, /* Single step */
- { "g", "sys_open", NULL, check_single_step },
- { "sys_open", "OK", sw_break, }, /* set sw breakpoint */
+ { "g", "do_sys_openat2", NULL, check_single_step },
+ { "do_sys_openat2", "OK", sw_break, }, /* set sw breakpoint */
{ "7", "T0*", skip_back_repeat_test }, /* Loop based on repeat_test */
{ "D", "OK", NULL, final_ack_set }, /* detach and unregister I/O */
{ "", "", get_cont_catch, put_cont_catch },
base-commit: 1e28eed17697bcf343c6743f0028cc3b5dd88bf0
--
2.29.2
Re: [PATCH 2/2] video: backlight: qcom-wled: Add PMI8994 compatible
On Tue, Mar 23, 2021 at 08:39:35AM +, Lee Jones wrote:
> On Mon, 22 Mar 2021, Daniel Thompson wrote:
>
> > On Sun, Feb 28, 2021 at 01:41:05PM +0100, Konrad Dybcio wrote:
> > > Add a compatible for PMI8994 WLED. It uses the V4 of WLED IP.
> > >
> > > Signed-off-by: Konrad Dybcio
> >
> > Reviewed-by: Daniel Thompson
>
> Why are you Reviewing/Acking a patch that was applied on the 10th?
Simply an oversight. Looks like I forgot to remove it from my backlog
when it was applied.
Daniel.
>
> > > ---
> > > drivers/video/backlight/qcom-wled.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/video/backlight/qcom-wled.c
> > > b/drivers/video/backlight/qcom-wled.c
> > > index 3bc7800eb0a9..497b9035a908 100644
> > > --- a/drivers/video/backlight/qcom-wled.c
> > > +++ b/drivers/video/backlight/qcom-wled.c
> > > @@ -1704,6 +1704,7 @@ static int wled_remove(struct platform_device *pdev)
> > >
> > > static const struct of_device_id wled_match_table[] = {
> > > { .compatible = "qcom,pm8941-wled", .data = (void *)3 },
> > > + { .compatible = "qcom,pmi8994-wled", .data = (void *)4 },
> > > { .compatible = "qcom,pmi8998-wled", .data = (void *)4 },
> > > { .compatible = "qcom,pm660l-wled", .data = (void *)4 },
> > > { .compatible = "qcom,pm8150l-wled", .data = (void *)5 },
>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH] backlight: journada720: fix Wmisleading-indentation warning
On Mon, Mar 22, 2021 at 05:41:28PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> With gcc-11, we get a warning about code that looks correct
> but badly indented:
>
> drivers/video/backlight/jornada720_bl.c: In function
> ‘jornada_bl_update_status’:
> drivers/video/backlight/jornada720_bl.c:66:11: error: this ‘else’ clause does
> not guard... [-Werror=misleading-indentation]
>66 | } else /* turn on backlight */
> | ^~~~
>
> Change the formatting according to our normal conventions.
>
> Fixes: 13a7b5dc0d17 ("backlight: Adds HP Jornada 700 series backlight driver")
> Signed-off-by: Arnd Bergmann
I'm dubious that the re-indent matches the original authors intent...
but it certainly does match what was actually written and tested so
on that basis:
Reviewed-by: Daniel Thompson
Daniel.
---
> drivers/video/backlight/jornada720_bl.c | 44 -
> 1 file changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/video/backlight/jornada720_bl.c
> b/drivers/video/backlight/jornada720_bl.c
> index 996f7ba3b373..066d0dc98f60 100644
> --- a/drivers/video/backlight/jornada720_bl.c
> +++ b/drivers/video/backlight/jornada720_bl.c
> @@ -66,30 +66,30 @@ static int jornada_bl_update_status(struct
> backlight_device *bd)
> } else /* turn on backlight */
> PPSR |= PPC_LDD1;
>
> - /* send command to our mcu */
> - if (jornada_ssp_byte(SETBRIGHTNESS) != TXDUMMY) {
> - dev_info(>dev, "failed to set brightness\n");
> - ret = -ETIMEDOUT;
> - goto out;
> - }
> + /* send command to our mcu */
> + if (jornada_ssp_byte(SETBRIGHTNESS) != TXDUMMY) {
> + dev_info(>dev, "failed to set brightness\n");
> + ret = -ETIMEDOUT;
> + goto out;
> + }
>
> - /*
> - * at this point we expect that the mcu has accepted
> - * our command and is waiting for our new value
> - * please note that maximum brightness is 255,
> - * but due to physical layout it is equal to 0, so we simply
> - * invert the value (MAX VALUE - NEW VALUE).
> - */
> - if (jornada_ssp_byte(BL_MAX_BRIGHT - bd->props.brightness)
> - != TXDUMMY) {
> - dev_err(>dev, "set brightness failed\n");
> - ret = -ETIMEDOUT;
> - }
> + /*
> + * at this point we expect that the mcu has accepted
> + * our command and is waiting for our new value
> + * please note that maximum brightness is 255,
> + * but due to physical layout it is equal to 0, so we simply
> + * invert the value (MAX VALUE - NEW VALUE).
> + */
> + if (jornada_ssp_byte(BL_MAX_BRIGHT - bd->props.brightness)
> + != TXDUMMY) {
> + dev_err(>dev, "set brightness failed\n");
> + ret = -ETIMEDOUT;
> + }
>
> - /*
> - * If infact we get an TXDUMMY as output we are happy and dont
> - * make any further comments about it
> - */
> + /*
> + * If infact we get an TXDUMMY as output we are happy and dont
> + * make any further comments about it
> + */
> out:
> jornada_ssp_end();
>
> --
> 2.29.2
>
Re: [PATCH] kgdb: fix gcc-11 warning on indentation
On Mon, Mar 22, 2021 at 10:04:57AM -0700, Doug Anderson wrote:
> > + if (verbose)\
> > + printk(KERN_INFO a);\
> > +} while (0)
> > +#define v2printk(a...) do {\
> > + if (verbose > 1)\
> > + printk(KERN_INFO a);\
> > + touch_nmi_watchdog(); \
>
> This touch_nmi_watchdog() is pretty wonky. I guess maybe the
> assumption is that the "verbose level 2" prints are so chatty that the
> printing might prevent us from touching the NMI watchdog in the way
> that we normally do and thus we need an extra one here?
>
> ...but, in that case, I think the old code was _wrong_ and that the
> intention was that the touch_nmi_watchdog() should only be if "verose
> > 1" as the indentation implied. There doesn't feel like a reason to
> touch the watchdog if we're not doing anything slow.
I'm not entirely sure I'd like to second guess the intent here. This
macro has been there since this file was introduced but several callers
have been added since then. We have to guess their intent too!
So, whilst I think you are probably right, v2printk() does appears in
places such as the single step test loop which makes it pretty
difficult to decide by inspection whether or not touching the watchdog
is useful.
It's something that could be further examined... but I'd be a little
reluctant to combine it directly with a whitespace change!
Daniel.
Re: [PATCH] kgdb: fix gcc-11 warning on indentation
On Mon, Mar 22, 2021 at 05:43:03PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> gcc-11 starts warning about misleading indentation inside of macros:
>
> drivers/misc/kgdbts.c: In function ‘kgdbts_break_test’:
> drivers/misc/kgdbts.c:103:9: error: this ‘if’ clause does not guard...
> [-Werror=misleading-indentation]
> 103 | if (verbose > 1) \
> | ^~
> drivers/misc/kgdbts.c:200:9: note: in expansion of macro ‘v2printk’
> 200 | v2printk("kgdbts: breakpoint complete\n");
> | ^~~~
> drivers/misc/kgdbts.c:105:17: note: ...this statement, but the latter is
> misleadingly indented as if it were guarded by the ‘if’
> 105 | touch_nmi_watchdog(); \
> | ^~
>
> The code looks correct to me, so just reindent it for readability.
>
> Fixes: e8d31c204e36 ("kgdb: add kgdb internal test suite")
> Signed-off-by: Arnd Bergmann
Acked-by: Daniel Thompson
Which tree do you want to merge this one though? I've got nothing else
pending for this file so I am very relaxed about the route...
Daniel.
> ---
> drivers/misc/kgdbts.c | 26 +-
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
> index 945701bce553..2e081a58da6c 100644
> --- a/drivers/misc/kgdbts.c
> +++ b/drivers/misc/kgdbts.c
> @@ -95,19 +95,19 @@
>
> #include
>
> -#define v1printk(a...) do { \
> - if (verbose) \
> - printk(KERN_INFO a); \
> - } while (0)
> -#define v2printk(a...) do { \
> - if (verbose > 1) \
> - printk(KERN_INFO a); \
> - touch_nmi_watchdog(); \
> - } while (0)
> -#define eprintk(a...) do { \
> - printk(KERN_ERR a); \
> - WARN_ON(1); \
> - } while (0)
> +#define v1printk(a...) do { \
> + if (verbose)\
> + printk(KERN_INFO a);\
> +} while (0)
> +#define v2printk(a...) do { \
> + if (verbose > 1)\
> + printk(KERN_INFO a);\
> + touch_nmi_watchdog(); \
> +} while (0)
> +#define eprintk(a...) do { \
> + printk(KERN_ERR a); \
> + WARN_ON(1); \
> +} while (0)
> #define MAX_CONFIG_LEN 40
>
> static struct kgdb_io kgdbts_io_ops;
> --
> 2.29.2
>
Re: [PATCH V2] kernel: debug: Ordinary typo fixes in the file gdbstub.c
On Wed, Mar 17, 2021 at 04:16:58PM +0530, Bhaskar Chowdhury wrote:
> s/overwitten/overwritten/
> s/procesing/processing/
>
> Signed-off-by: Bhaskar Chowdhury
Applied, Thanks!
Daniel.
> ---
> Changes from V1:
> As Daniel pointed out, I was misdoing a check,so corrected
>
> kernel/debug/gdbstub.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
> index e149a0ac9e9e..8372897402f4 100644
> --- a/kernel/debug/gdbstub.c
> +++ b/kernel/debug/gdbstub.c
> @@ -321,7 +321,7 @@ int kgdb_hex2long(char **ptr, unsigned long *long_val)
> /*
> * Copy the binary array pointed to by buf into mem. Fix $, #, and
> * 0x7d escaped with 0x7d. Return -EFAULT on failure or 0 on success.
> - * The input buf is overwitten with the result to write to mem.
> + * The input buf is overwritten with the result to write to mem.
> */
> static int kgdb_ebin2mem(char *buf, char *mem, int count)
> {
> @@ -952,7 +952,7 @@ static int gdb_cmd_exception_pass(struct kgdb_state *ks)
> }
>
> /*
> - * This function performs all gdbserial command procesing
> + * This function performs all gdbserial command processing
> */
> int gdb_serial_stub(struct kgdb_state *ks)
> {
> --
> 2.30.2
Re: [PATCH 2/2] video: backlight: qcom-wled: Add PMI8994 compatible
On Sun, Feb 28, 2021 at 01:41:05PM +0100, Konrad Dybcio wrote:
> Add a compatible for PMI8994 WLED. It uses the V4 of WLED IP.
>
> Signed-off-by: Konrad Dybcio
Reviewed-by: Daniel Thompson
Daniel.
> ---
> drivers/video/backlight/qcom-wled.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/video/backlight/qcom-wled.c
> b/drivers/video/backlight/qcom-wled.c
> index 3bc7800eb0a9..497b9035a908 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -1704,6 +1704,7 @@ static int wled_remove(struct platform_device *pdev)
>
> static const struct of_device_id wled_match_table[] = {
> { .compatible = "qcom,pm8941-wled", .data = (void *)3 },
> + { .compatible = "qcom,pmi8994-wled", .data = (void *)4 },
> { .compatible = "qcom,pmi8998-wled", .data = (void *)4 },
> { .compatible = "qcom,pm660l-wled", .data = (void *)4 },
> { .compatible = "qcom,pm8150l-wled", .data = (void *)5 },
> --
> 2.30.1
Re: [PATCH 1/2] dt-bindings: leds: backlight: qcom-wled: Add PMI8994 compatible
On Sun, Feb 28, 2021 at 01:41:04PM +0100, Konrad Dybcio wrote: > Document the newly added PMI8994 compatible. > > Signed-off-by: Konrad Dybcio (For Lee) Acked-by: Daniel Thompson Daniel. > --- > Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml > b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml > index 47938e372987..d839e75d9788 100644 > --- a/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml > +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-wled.yaml > @@ -19,6 +19,7 @@ properties: >compatible: > enum: >- qcom,pm8941-wled > + - qcom,pmi8994-wled >- qcom,pmi8998-wled >- qcom,pm660l-wled >- qcom,pm8150l-wled > -- > 2.30.1
Re: [PATCH v5] kdb: Simplify kdb commands registration
On Wed, Feb 24, 2021 at 12:38:27PM +0530, Sumit Garg wrote:
> Simplify kdb commands registration via using linked list instead of
> static array for commands storage.
>
> Signed-off-by: Sumit Garg
Applied, thanks!
> ---
>
> Changes in v5:
> - Introduce new method: kdb_register_table() to register static kdb
> main and breakpoint command tables instead of using statically
> allocated commands.
>
> Changes in v4:
> - Fix kdb commands memory allocation issue prior to slab being available
> with an array of statically allocated commands. Now it works fine with
> kgdbwait.
> - Fix a misc checkpatch warning.
> - I have dropped Doug's review tag as I think this version includes a
> major fix that should be reviewed again.
>
> Changes in v3:
> - Remove redundant "if" check.
> - Pick up review tag from Doug.
>
> Changes in v2:
> - Remove redundant NULL check for "cmd_name".
> - Incorporate misc. comment.
>
> kernel/debug/kdb/kdb_bp.c | 81 --
> kernel/debug/kdb/kdb_main.c| 472 -
> kernel/debug/kdb/kdb_private.h | 3 +
> 3 files changed, 343 insertions(+), 213 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_bp.c b/kernel/debug/kdb/kdb_bp.c
> index ec4940146612..c15a1c6abfd6 100644
> --- a/kernel/debug/kdb/kdb_bp.c
> +++ b/kernel/debug/kdb/kdb_bp.c
> @@ -522,6 +522,60 @@ static int kdb_ss(int argc, const char **argv)
> return KDB_CMD_SS;
> }
>
> +static kdbtab_t bptab[] = {
> + { .cmd_name = "bp",
> + .cmd_func = kdb_bp,
> + .cmd_usage = "[]",
> + .cmd_help = "Set/Display breakpoints",
> + .cmd_minlen = 0,
> + .cmd_flags = KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS,
> + },
> + { .cmd_name = "bl",
> + .cmd_func = kdb_bp,
> + .cmd_usage = "[]",
> + .cmd_help = "Display breakpoints",
> + .cmd_minlen = 0,
> + .cmd_flags = KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS,
> + },
> + { .cmd_name = "bc",
> + .cmd_func = kdb_bc,
> + .cmd_usage = "",
> + .cmd_help = "Clear Breakpoint",
> + .cmd_minlen = 0,
> + .cmd_flags = KDB_ENABLE_FLOW_CTRL,
> + },
> + { .cmd_name = "be",
> + .cmd_func = kdb_bc,
> + .cmd_usage = "",
> + .cmd_help = "Enable Breakpoint",
> + .cmd_minlen = 0,
> + .cmd_flags = KDB_ENABLE_FLOW_CTRL,
> + },
> + { .cmd_name = "bd",
> + .cmd_func = kdb_bc,
> + .cmd_usage = "",
> + .cmd_help = "Disable Breakpoint",
> + .cmd_minlen = 0,
> + .cmd_flags = KDB_ENABLE_FLOW_CTRL,
> + },
> + { .cmd_name = "ss",
> + .cmd_func = kdb_ss,
> + .cmd_usage = "",
> + .cmd_help = "Single Step",
> + .cmd_minlen = 1,
> + .cmd_flags = KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS,
> + },
> +};
> +
> +static kdbtab_t bphcmd = {
> + .cmd_name = "bph",
> + .cmd_func = kdb_bp,
> + .cmd_usage = "[]",
> + .cmd_help = "[datar [length]|dataw [length]] Set hw brk",
> + .cmd_minlen = 0,
> + .cmd_flags = KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS,
> +};
> +
> /* Initialize the breakpoint table and register breakpoint commands. */
>
> void __init kdb_initbptab(void)
> @@ -537,30 +591,7 @@ void __init kdb_initbptab(void)
> for (i = 0, bp = kdb_breakpoints; i < KDB_MAXBPT; i++, bp++)
> bp->bp_free = 1;
>
> - kdb_register_flags("bp", kdb_bp, "[]",
> - "Set/Display breakpoints", 0,
> - KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS);
> - kdb_register_flags("bl", kdb_bp, "[]",
> - "Display breakpoints", 0,
> - KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS);
> + kdb_register_table(bptab, ARRAY_SIZE(bptab));
> if (arch_kgdb_ops.flags & KGDB_HW_BREAKPOINT)
> - kdb_register_flags("bph", kdb_bp, "[]",
> - "[datar [length]|dataw [length]] Set hw brk", 0,
> - KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS);
> - kdb_register_flags("bc", kdb_bc, "",
> - "Clear Breakpoint", 0,
> - KDB_ENABLE_FLOW_CTRL);
> - kdb_register_flags("be", kdb_bc, "",
> - "Enable Breakpoint", 0,
> - KDB_ENABLE_FLOW_CTRL);
> - kdb_register_flags("bd", kdb_bc, "",
> - "Disable Breakpoint", 0,
> - KDB_ENABLE_FLOW_CTRL);
> -
> - kdb_register_flags("ss", kdb_ss, "",
> - "Single Step", 1,
> - KDB_ENABLE_FLOW_CTRL | KDB_REPEAT_NO_ARGS);
> - /*
> - * Architecture dependent initialization.
> - */
> + kdb_register_table(, 1);
> }
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index 930ac1b25ec7..1e0c2c37df94 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++
Re: [PATCH v2] kdb: Get rid of custom debug heap allocator
On Mon, Mar 01, 2021 at 11:33:00AM +0530, Sumit Garg wrote: > On Fri, 26 Feb 2021 at 23:07, Daniel Thompson > wrote: > > > > On Fri, Feb 26, 2021 at 06:12:13PM +0530, Sumit Garg wrote: > > > On Fri, 26 Feb 2021 at 16:29, Daniel Thompson > > > wrote: > > > > > > > > On Fri, Feb 26, 2021 at 03:23:06PM +0530, Sumit Garg wrote: > > > > > Currently the only user for debug heap is kdbnearsym() which can be > > > > > modified to rather ask the caller to supply a buffer for symbol name. > > > > > So do that and modify kdbnearsym() callers to pass a symbol name > > > > > buffer > > > > > allocated statically and hence remove custom debug heap allocator. > > > > > > > > Why make the callers do this? > > > > > > > > The LRU buffers were managed inside kdbnearsym() why does switching to > > > > an approach with a single buffer require us to push that buffer out to > > > > the callers? > > > > > > > > > > Earlier the LRU buffers managed namebuf uniqueness per caller (upto > > > 100 callers) > > > > The uniqueness is per symbol, not per caller. > > > > Agree. > > > > but if we switch to single entry in kdbnearsym() then all > > > callers need to share common buffer which will lead to incorrect > > > results from following simple sequence: > > > > > > kdbnearsym(word, ); > > > kdbnearsym(word, ); > > > kdb_symbol_print(word, , 0); > > > kdb_symbol_print(word, , 0); > > > > > > But if we change to a unique static namebuf per caller then the > > > following sequence will work: > > > > > > kdbnearsym(word, , namebuf1); > > > kdbnearsym(word, , namebuf2); > > > kdb_symbol_print(word, , 0); > > > kdb_symbol_print(word, , 0); > > > > This is true but do any of the callers of kdbnearsym ever do this? > > No, but any of prospective callers may need this. > > > The > > main reaason that heap stuck out as redundant was that I've only ever > > seen the output of kdbnearsym() consumed almost immediately by a print. > > > > Yeah but I think the alternative proposed in this patch isn't as > burdensome as the heap and tries to somewhat match existing > functionality. > > > I wrote an early version of a patch like this that just shrunk the LRU > > cache down to 2 and avoided any heap usage... but I threw it away > > when I realized we never carry cached values outside the function > > that obtained them. > > > > Okay, so if you still think that having a single static buffer inside > kdbnearsym() is an appropriate approach for time being then I will > switch to use that instead. Sorry to drop this thread for so long. On reflection I still have a few concerns about the current code. To be clear this is not really about wasting 128 bytes of RAM (your patch saves 256K after all). It's more that the current static buffers "look weird". They are static so any competent OS programmer reads them and thinks "but what about concurrency/reentrancy"). With the static buffers scattered through the code they don't have a single place to find the answer. I originally proposed handling this by the static buffer horror in kdbnearsym() and describing how it all works in the header comment! As much as anything this was to centralize the commentary in the contract for calling kdbnearsym(). Hence nobody should write the theoretic bug you describe because they read the contract! You are welcome to counter propose but you must ensure that there are equivalent comments so our "competent OS programmer" from the paragraph above can figure out how the static buffer works without having to run git blame` and digging out the patch history. Daniel. > > -Sumit > > > > > > > > @@ -526,6 +526,7 @@ int kdbgetaddrarg(int argc, const char **argv, > > > > > int *nextarg, > > > > > > > > > > > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > > > > > index 9d69169582c6..6efe9ec53906 100644 > > > > > --- a/kernel/debug/kdb/kdb_main.c > > > > > +++ b/kernel/debug/kdb/kdb_main.c > > > > > @@ -526,6 +526,7 @@ int kdbgetaddrarg(int argc, const char **argv, > > > > > int *nextarg, > > > > > > > > The documentation comment for this function has not been updated to > > > > describe the new contract on callers of this function (e.g. if they > > > > consume the symbol name they must
Re: [PATCH] kdb: Refactor kdb_defcmd implementation
On Tue, Mar 09, 2021 at 05:47:47PM +0530, Sumit Garg wrote:
> Switch to use kdbtab_t instead of separate struct defcmd_set since
> now we have kdb_register_table() to register pre-allocated kdb commands.
This needs rewriting. I've been struggling for some time to figure out
what it actually means means and how it related to the patch. I'm
starting to conclude that this might not be my fault!
> Also, switch to use a linked list for sub-commands instead of dynamic
> array which makes traversing the sub-commands list simpler.
We can't call these things sub-commands! These days a sub-commands
implies something like `git subcommand` and kdb doesn't have anything
like that.
> +struct kdb_subcmd {
> + char*scmd_name; /* Sub-command name */
> + struct list_head list_node;/* Sub-command node */
> +};
> +
> /* The KDB shell command table */
> typedef struct _kdbtab {
> char*cmd_name; /* Command name */
> @@ -175,6 +181,7 @@ typedef struct _kdbtab {
> kdb_cmdflags_t cmd_flags; /* Command behaviour flags */
> struct list_head list_node; /* Command list */
> boolis_dynamic; /* Command table allocation type */
> + struct list_head kdb_scmds_head; /* Sub-commands list */
> } kdbtab_t;
Perhaps this should be more like:
struct defcmd_set {
kdbtab_t cmd;
struct list_head commands;
};
This still gets registers using kdb_register_table() but it keeps the
macro code all in once place:
kdb_register_table(>cmd, 1);
I think that is what I *meant* to suggest ;-) . It also avoids having to
talk about sub-commands! BTW I'm open to giving defcmd_set a better name
(kdb_macro?) but I don't see why we want to give all commands a macro
list.
Daniel.
Re: [net-next PATCH v7 04/16] of: mdio: Refactor of_phy_find_device()
On Wed, Mar 17, 2021 at 02:15:20PM +0530, Calvin Johnson wrote: > Hi Daniel, > > On Tue, Mar 16, 2021 at 07:17:19PM +0000, Daniel Thompson wrote: > > On Thu, Mar 11, 2021 at 11:49:59AM +0530, Calvin Johnson wrote: > > > Refactor of_phy_find_device() to use fwnode_phy_find_device(). > > > > > > Signed-off-by: Calvin Johnson > > > > This patch series is provoking depmod dependency cycles for me and > > it bisected down to this patch (although I think later patches in > > the series add further cycles). > > > > The problems emerge when running modules_install either directly or > > indirectly via packaging rules such as bindeb-pkg. > > > > ~~~ > > make -j16 INSTALL_MOD_PATH=$PWD/modules modules_install > > ... > > INSTALL sound/usb/misc/snd-ua101.ko > > INSTALL sound/usb/snd-usb-audio.ko > > INSTALL sound/usb/snd-usbmidi-lib.ko > > INSTALL sound/xen/snd_xen_front.ko > > DEPMOD 5.12.0-rc3-9-g1fda33bf463d > > depmod: ERROR: Cycle detected: fwnode_mdio -> of_mdio -> fwnode_mdio > > depmod: ERROR: Found 2 modules in dependency cycles! > > ~~~ > > > > Kconfig can be found here: > > https://gist.github.com/daniel-thompson/6a7d224f3d3950ffa3f63f979b636474 > > > > This Kconfig file is for a highly modular kernel derived from the Debian > > 5.10 arm64 kernel config. I was not able to reproduce using the defconfig > > kernel for arm64. > > > Thanks for catching this. I'm able to reproduce the issue and will fix it. > > By the way, is there any integration tool/mechanism out there to which I can > submit the patch series and build for various possible configs like these? Not sure which autotester would be most likely to pick this up. This issue is slightly unusual because it broke the install rather then the build... and lots of people (including me) primarily run build tests ;-) . Anyhow, I guess the best way to pick up module problems like this is going to be an `allmodconfig` build followed up with `rm -rf modtest; make modules_install INSTALL_MOD_PATH=$PWD/modtest`. Daniel.
Re: [PATCH] kernel: debug: Ordinary typo fixes in the file gdbstub.c
On Wed, Mar 17, 2021 at 02:40:22PM +0530, Bhaskar Chowdhury wrote:
>
> s/'O'utput/'Output/
> s/overwitten/overwritten/
> s/procesing/processing/
>
> Signed-off-by: Bhaskar Chowdhury
> ---
> kernel/debug/gdbstub.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
> index e149a0ac9e9e..5c96590725f1 100644
> --- a/kernel/debug/gdbstub.c
> +++ b/kernel/debug/gdbstub.c
> @@ -204,7 +204,7 @@ void gdbstub_msg_write(const char *s, int len)
> if (len == 0)
> len = strlen(s);
>
> - /* 'O'utput */
> + /* 'Output */
This is not a typo.
It is showing that the 'O' being writing into the message buffer is a
mnemonic and describing what it expands to.
Other changes look good, please can you resend with this one removed?
Daniel.
> gdbmsgbuf[0] = 'O';
>
> /* Fill and send buffers... */
> @@ -321,7 +321,7 @@ int kgdb_hex2long(char **ptr, unsigned long *long_val)
> /*
> * Copy the binary array pointed to by buf into mem. Fix $, #, and
> * 0x7d escaped with 0x7d. Return -EFAULT on failure or 0 on success.
> - * The input buf is overwitten with the result to write to mem.
> + * The input buf is overwritten with the result to write to mem.
> */
> static int kgdb_ebin2mem(char *buf, char *mem, int count)
> {
> @@ -952,7 +952,7 @@ static int gdb_cmd_exception_pass(struct kgdb_state *ks)
> }
>
> /*
> - * This function performs all gdbserial command procesing
> + * This function performs all gdbserial command processing
> */
> int gdb_serial_stub(struct kgdb_state *ks)
> {
> --
> 2.30.2
>
Re: [net-next PATCH v7 04/16] of: mdio: Refactor of_phy_find_device()
On Thu, Mar 11, 2021 at 11:49:59AM +0530, Calvin Johnson wrote:
> Refactor of_phy_find_device() to use fwnode_phy_find_device().
>
> Signed-off-by: Calvin Johnson
This patch series is provoking depmod dependency cycles for me and
it bisected down to this patch (although I think later patches in
the series add further cycles).
The problems emerge when running modules_install either directly or
indirectly via packaging rules such as bindeb-pkg.
~~~
make -j16 INSTALL_MOD_PATH=$PWD/modules modules_install
...
INSTALL sound/usb/misc/snd-ua101.ko
INSTALL sound/usb/snd-usb-audio.ko
INSTALL sound/usb/snd-usbmidi-lib.ko
INSTALL sound/xen/snd_xen_front.ko
DEPMOD 5.12.0-rc3-9-g1fda33bf463d
depmod: ERROR: Cycle detected: fwnode_mdio -> of_mdio -> fwnode_mdio
depmod: ERROR: Found 2 modules in dependency cycles!
~~~
Kconfig can be found here:
https://gist.github.com/daniel-thompson/6a7d224f3d3950ffa3f63f979b636474
This Kconfig file is for a highly modular kernel derived from the Debian
5.10 arm64 kernel config. I was not able to reproduce using the defconfig
kernel for arm64.
Daniel.
> ---
>
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
> drivers/net/mdio/of_mdio.c | 13 +
> 1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
> index d5e0970b2561..b5e0b5b22f1a 100644
> --- a/drivers/net/mdio/of_mdio.c
> +++ b/drivers/net/mdio/of_mdio.c
> @@ -360,18 +360,7 @@ EXPORT_SYMBOL(of_mdio_find_device);
> */
> struct phy_device *of_phy_find_device(struct device_node *phy_np)
> {
> - struct mdio_device *mdiodev;
> -
> - mdiodev = of_mdio_find_device(phy_np);
> - if (!mdiodev)
> - return NULL;
> -
> - if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
> - return to_phy_device(>dev);
> -
> - put_device(>dev);
> -
> - return NULL;
> + return fwnode_phy_find_device(of_fwnode_handle(phy_np));
> }
> EXPORT_SYMBOL(of_phy_find_device);
>
Re: [PATCH] backlight: qcom-wled: Use sink_addr for sync toggle
On Sun, Mar 14, 2021 at 11:11:10AM +0100, Marijn Suijten wrote: > From: Obeida Shamoun > > WLED3_SINK_REG_SYNC is, as the name implies, a sink register offset. > Therefore, use the sink address as base instead of the ctrl address. > > This fixes the sync toggle on wled4, which can be observed by the fact > that adjusting brightness now works. > > It has no effect on wled3 because sink and ctrl base addresses are the > same. This allows adjusting the brightness without having to disable > then reenable the module. > > Signed-off-by: Obeida Shamoun > Signed-off-by: Konrad Dybcio > Signed-off-by: Marijn Suijten LGTM, although an acked-by from Kiran would be nice to have: Reviewed-by: Daniel Thompson Daniel. > --- > drivers/video/backlight/qcom-wled.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/video/backlight/qcom-wled.c > b/drivers/video/backlight/qcom-wled.c > index 091f07e7c145..fc8b443d10fd 100644 > --- a/drivers/video/backlight/qcom-wled.c > +++ b/drivers/video/backlight/qcom-wled.c > @@ -336,13 +336,13 @@ static int wled3_sync_toggle(struct wled *wled) > unsigned int mask = GENMASK(wled->max_string_count - 1, 0); > > rc = regmap_update_bits(wled->regmap, > - wled->ctrl_addr + WLED3_SINK_REG_SYNC, > + wled->sink_addr + WLED3_SINK_REG_SYNC, > mask, mask); > if (rc < 0) > return rc; > > rc = regmap_update_bits(wled->regmap, > - wled->ctrl_addr + WLED3_SINK_REG_SYNC, > + wled->sink_addr + WLED3_SINK_REG_SYNC, > mask, WLED3_SINK_REG_SYNC_CLEAR); > > return rc; > -- > 2.30.2 >
Re: [PATCH] drm/nouveau/kms/nve4-nv108: Limit cursors to 128x128
On Thu, Mar 04, 2021 at 08:52:41PM -0500, Lyude Paul wrote:
> While Kepler does technically support 256x256 cursors, it turns out that
> Kepler actually has some additional requirements for scanout surfaces that
> we're not enforcing correctly, which aren't present on Maxwell and later.
> Cursor surfaces must always use small pages (4K), and overlay surfaces must
> always use large pages (128K).
>
> Fixing this correctly though will take a bit more work: as we'll need to
> add some code in prepare_fb() to move cursor FBs in large pages to small
> pages, and vice-versa for overlay FBs. So until we have the time to do
> that, just limit cursor surfaces to 128x128 - a size small enough to always
> default to small pages.
>
> This means small ovlys are still broken on Kepler, but it is extremely
> unlikely anyone cares about those anyway :).
>
> Signed-off-by: Lyude Paul
> Fixes: d3b2f0f7921c ("drm/nouveau/kms/nv50-: Report max cursor size to
> userspace")
> Cc: # v5.11+
I was experiencing problems with the mouse cursor on my system in v5.11
and after a bisect to help me search the web I found my way to this
patch, which fixed the problem for me.
Mine is an Armv8 system but there is nothing particularly exotic from a
graphics card or software point of view: Debian bullseye/wayland
(gnome-shell 3.38.3, mesa-20.3.4) running on a GT-710.
However FWIW:
Tested-by: Daniel Thompson
Daniel.
> ---
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 196612addfd6..1c9c0cdf85db 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -2693,9 +2693,20 @@ nv50_display_create(struct drm_device *dev)
> else
> nouveau_display(dev)->format_modifiers = disp50xx_modifiers;
>
> - if (disp->disp->object.oclass >= GK104_DISP) {
> + /* FIXME: 256x256 cursors are supported on Kepler, however unlike
> Maxwell and later
> + * generations Kepler requires that we use small pages (4K) for cursor
> scanout surfaces. The
> + * proper fix for this is to teach nouveau to migrate fbs being used
> for the cursor plane to
> + * small page allocations in prepare_fb(). When this is implemented, we
> should also force
> + * large pages (128K) for ovly fbs in order to fix Kepler ovlys.
> + * But until then, just limit cursors to 128x128 - which is small
> enough to avoid ever using
> + * large pages.
> + */
> + if (disp->disp->object.oclass >= GM107_DISP) {
> dev->mode_config.cursor_width = 256;
> dev->mode_config.cursor_height = 256;
> + } else if (disp->disp->object.oclass >= GK104_DISP) {
> + dev->mode_config.cursor_width = 128;
> + dev->mode_config.cursor_height = 128;
> } else {
> dev->mode_config.cursor_width = 64;
> dev->mode_config.cursor_height = 64;
> --
> 2.29.2
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH V3 2/2] backlight: qcom-wled: Correct the sync_toggle sequence
On Mon, Mar 01, 2021 at 02:58:36PM +0530, Kiran Gunda wrote: > As per the current implementation, after FSC (Full Scale Current) > and brightness update the sync bits are transitioned from set-then-clear. This does not makes sense since there are too many verbs. Set and clear are both verbs so in this context: "the code will set the bit and then the code will clear the bit". Either: s/transitioned from set-then-clear/set-then-cleared/. Or: s/transitioned from set-then-clear/using a set-then-clear approach/. > But, the FSC and brightness sync takes place during a clear-then-set > transition of the sync bits. Likewise this no longer makes sense and had also become misleading. Two changes of state, clear and then set, do not usually result in a single transition. Either: s/clear-then-set/0 to 1/ Alternatively, if you want to stick exclusively to the set/clear terminology then replace the whole quoted section with: But, the FSC and brightness sync takes place when the sync bits are set (e.g. on a rising edge). > So the hardware team recommends a > clear-then-set approach in order to guarantee such a transition > regardless of the previous register state. > > Signed-off-by: Kiran Gunda With one of each of the changes proposed above: Reviewed-by: Daniel Thompson Daniel. > --- > drivers/video/backlight/qcom-wled.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/video/backlight/qcom-wled.c > b/drivers/video/backlight/qcom-wled.c > index aef52b9..19f83ac 100644 > --- a/drivers/video/backlight/qcom-wled.c > +++ b/drivers/video/backlight/qcom-wled.c > @@ -337,13 +337,13 @@ static int wled3_sync_toggle(struct wled *wled) > > rc = regmap_update_bits(wled->regmap, > wled->ctrl_addr + WLED3_SINK_REG_SYNC, > - mask, mask); > + mask, WLED3_SINK_REG_SYNC_CLEAR); > if (rc < 0) > return rc; > > rc = regmap_update_bits(wled->regmap, > wled->ctrl_addr + WLED3_SINK_REG_SYNC, > - mask, WLED3_SINK_REG_SYNC_CLEAR); > + mask, mask); > > return rc; > } > @@ -353,17 +353,17 @@ static int wled5_mod_sync_toggle(struct wled *wled) > int rc; > u8 val; > > - val = (wled->cfg.mod_sel == MOD_A) ? WLED5_SINK_REG_SYNC_MOD_A_BIT : > - WLED5_SINK_REG_SYNC_MOD_B_BIT; > rc = regmap_update_bits(wled->regmap, > wled->sink_addr + WLED5_SINK_REG_MOD_SYNC_BIT, > - WLED5_SINK_REG_SYNC_MASK, val); > + WLED5_SINK_REG_SYNC_MASK, 0); > if (rc < 0) > return rc; > > + val = (wled->cfg.mod_sel == MOD_A) ? WLED5_SINK_REG_SYNC_MOD_A_BIT : > + WLED5_SINK_REG_SYNC_MOD_B_BIT; > return regmap_update_bits(wled->regmap, > wled->sink_addr + WLED5_SINK_REG_MOD_SYNC_BIT, > - WLED5_SINK_REG_SYNC_MASK, 0); > + WLED5_SINK_REG_SYNC_MASK, val); > } > > static int wled_ovp_fault_status(struct wled *wled, bool *fault_set) > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
Re: [PATCH V2 2/2] backlight: qcom-wled: Correct the sync_toggle sequence
On Mon, Mar 01, 2021 at 02:15:12PM +0530, kgu...@codeaurora.org wrote: > On 2021-02-26 22:56, Daniel Thompson wrote: > > On Fri, Feb 26, 2021 at 05:42:24PM +0530, Kiran Gunda wrote: > > > As per the current implementation, after FSC (Full Scale Current) > > > and brightness update the sync bits are transitioned from 1 to 0. > > > > This still seems to incorrectly describe the current behaviour. > > > > Surely in most cases (i.e. every time except the first) the value of the > > sync bit is 0 when the function is called and we get both a 0 to 1 > > and then a 1 to 0 transition. > > > > That is why I recommended set-then-clear terminology to describe the > > current behaviour. It is concise and correct. > > Okay. Actually I have mentioned the "clear-and-set" in explaining the fix. > Let me modify the same terminology in explaining the problem case also. Yes please. In my original review I took time to explain why patch descriptions require care and attention and, also, why expressing the original behaviour as 1 to 0 was inadequate. Based on the previous feedback (and reply) I was rather surprised that the problem was only half corrected in the next revision. Daniel.
Re: [PATCH v2] kdb: Get rid of custom debug heap allocator
On Fri, Feb 26, 2021 at 06:12:13PM +0530, Sumit Garg wrote: > On Fri, 26 Feb 2021 at 16:29, Daniel Thompson > wrote: > > > > On Fri, Feb 26, 2021 at 03:23:06PM +0530, Sumit Garg wrote: > > > Currently the only user for debug heap is kdbnearsym() which can be > > > modified to rather ask the caller to supply a buffer for symbol name. > > > So do that and modify kdbnearsym() callers to pass a symbol name buffer > > > allocated statically and hence remove custom debug heap allocator. > > > > Why make the callers do this? > > > > The LRU buffers were managed inside kdbnearsym() why does switching to > > an approach with a single buffer require us to push that buffer out to > > the callers? > > > > Earlier the LRU buffers managed namebuf uniqueness per caller (upto > 100 callers) The uniqueness is per symbol, not per caller. > but if we switch to single entry in kdbnearsym() then all > callers need to share common buffer which will lead to incorrect > results from following simple sequence: > > kdbnearsym(word, ); > kdbnearsym(word, ); > kdb_symbol_print(word, , 0); > kdb_symbol_print(word, , 0); > > But if we change to a unique static namebuf per caller then the > following sequence will work: > > kdbnearsym(word, , namebuf1); > kdbnearsym(word, , namebuf2); > kdb_symbol_print(word, , 0); > kdb_symbol_print(word, , 0); This is true but do any of the callers of kdbnearsym ever do this? The main reaason that heap stuck out as redundant was that I've only ever seen the output of kdbnearsym() consumed almost immediately by a print. I wrote an early version of a patch like this that just shrunk the LRU cache down to 2 and avoided any heap usage... but I threw it away when I realized we never carry cached values outside the function that obtained them. > > > @@ -526,6 +526,7 @@ int kdbgetaddrarg(int argc, const char **argv, int > > > *nextarg, > > > > > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > > > index 9d69169582c6..6efe9ec53906 100644 > > > --- a/kernel/debug/kdb/kdb_main.c > > > +++ b/kernel/debug/kdb/kdb_main.c > > > @@ -526,6 +526,7 @@ int kdbgetaddrarg(int argc, const char **argv, int > > > *nextarg, > > > > The documentation comment for this function has not been updated to > > describe the new contract on callers of this function (e.g. if they > > consume the symbol name they must do so before calling kdbgetaddrarg() > > (and maybe kdbnearsym() again). > > > > I am not sure if I follow you here. If we have a unique static buffer > per caller then why do we need this new contract? I traced the code wrong. I thought it shared symtab->sym_name with its own caller... but it doesn't it shares synname with its caller and that's totally different... Daniel. > > > > > > char symbol = '\0'; > > > char *cp; > > > kdb_symtab_t symtab; > > > + static char namebuf[KSYM_NAME_LEN]; > > > > > > /* > > >* If the enable flags prohibit both arbitrary memory access > > > diff --git a/kernel/debug/kdb/kdb_support.c > > > b/kernel/debug/kdb/kdb_support.c > > > index b59aad1f0b55..9b907a84f2db 100644 > > > --- a/kernel/debug/kdb/kdb_support.c > > > +++ b/kernel/debug/kdb/kdb_support.c > > > @@ -57,8 +57,6 @@ int kdbgetsymval(const char *symname, kdb_symtab_t > > > *symtab) > > > } > > > EXPORT_SYMBOL(kdbgetsymval); > > > > > > -static char *kdb_name_table[100];/* arbitrary size */ > > > - > > > /* > > > * kdbnearsym - Return the name of the symbol with the nearest > > > address > > > * less than 'addr'. > > > > Again the documentation comment has not been updated and, in this case, > > is now misleading. > > Okay, I will fix it. > > > > > If we move the static buffer here then the remarks section on this > > function is a really good place to describe what the callers must do to > > manage the static buffer safely as well as a convenient place to mention > > that we tolerate the reuse of the static buffer if kdb is re-entered > > becase a) kdb is broken if that happens and b) we are crash resilient > > if if does. > > > > > > > @@ -79,13 +77,11 @@ static char *kdb_name_table[100]; /* arbitrary size */ > > > * hold active strings, no kdb caller of kdbnearsym makes more > > > * than ~20 later calls before using a saved value. > > > */ > > > -int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab) > > > +int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab, char *namebuf) > > > > As above, I don't understand why we need to add namebuf here. I think > > the prototype can remain the same. > > > > Think of it simple that we have reduce the cache from having 100 entries > > to having just 1 ;-) . > > Please see my response above. > > -Sumit > > > > > > > Daniel.
Re: [PATCH V2 2/2] backlight: qcom-wled: Correct the sync_toggle sequence
On Fri, Feb 26, 2021 at 05:42:24PM +0530, Kiran Gunda wrote: > As per the current implementation, after FSC (Full Scale Current) > and brightness update the sync bits are transitioned from 1 to 0. This still seems to incorrectly describe the current behaviour. Surely in most cases (i.e. every time except the first) the value of the sync bit is 0 when the function is called and we get both a 0 to 1 and then a 1 to 0 transition. That is why I recommended set-then-clear terminology to describe the current behaviour. It is concise and correct. Daniel. > But, the FSC and brightness sync takes place during a 0 to 1 > transition of the sync bits. So the hardware team recommends a > clear-then-set approach in order to guarantee such a transition > regardless of the previous register state. > > Signed-off-by: Kiran Gunda > --- > drivers/video/backlight/qcom-wled.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/video/backlight/qcom-wled.c > b/drivers/video/backlight/qcom-wled.c > index aef52b9..19f83ac 100644 > --- a/drivers/video/backlight/qcom-wled.c > +++ b/drivers/video/backlight/qcom-wled.c > @@ -337,13 +337,13 @@ static int wled3_sync_toggle(struct wled *wled) > > rc = regmap_update_bits(wled->regmap, > wled->ctrl_addr + WLED3_SINK_REG_SYNC, > - mask, mask); > + mask, WLED3_SINK_REG_SYNC_CLEAR); > if (rc < 0) > return rc; > > rc = regmap_update_bits(wled->regmap, > wled->ctrl_addr + WLED3_SINK_REG_SYNC, > - mask, WLED3_SINK_REG_SYNC_CLEAR); > + mask, mask); > > return rc; > } > @@ -353,17 +353,17 @@ static int wled5_mod_sync_toggle(struct wled *wled) > int rc; > u8 val; > > - val = (wled->cfg.mod_sel == MOD_A) ? WLED5_SINK_REG_SYNC_MOD_A_BIT : > - WLED5_SINK_REG_SYNC_MOD_B_BIT; > rc = regmap_update_bits(wled->regmap, > wled->sink_addr + WLED5_SINK_REG_MOD_SYNC_BIT, > - WLED5_SINK_REG_SYNC_MASK, val); > + WLED5_SINK_REG_SYNC_MASK, 0); > if (rc < 0) > return rc; > > + val = (wled->cfg.mod_sel == MOD_A) ? WLED5_SINK_REG_SYNC_MOD_A_BIT : > + WLED5_SINK_REG_SYNC_MOD_B_BIT; > return regmap_update_bits(wled->regmap, > wled->sink_addr + WLED5_SINK_REG_MOD_SYNC_BIT, > - WLED5_SINK_REG_SYNC_MASK, 0); > + WLED5_SINK_REG_SYNC_MASK, val); > } > > static int wled_ovp_fault_status(struct wled *wled, bool *fault_set) > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
Re: [PATCH V2 1/2] backlight: qcom-wled: Fix FSC update issue for WLED5
On Fri, Feb 26, 2021 at 05:42:23PM +0530, Kiran Gunda wrote:
> Currently, for WLED5, the FSC (Full scale current) setting is not
> updated properly due to driver toggling the wrong register after
> an FSC update.
>
> On WLED5 we should only toggle the MOD_SYNC bit after a brightness
> update. For an FSC update we need to toggle the SYNC bits instead.
>
> Fix it by adopting the common wled3_sync_toggle() for WLED5 and
> introducing new code to the brightness update path to compensate.
>
> Signed-off-by: Kiran Gunda
Reviewed-by: Daniel Thompson
Daniel.
> ---
> drivers/video/backlight/qcom-wled.c | 25 +++--
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/backlight/qcom-wled.c
> b/drivers/video/backlight/qcom-wled.c
> index 3bc7800..aef52b9 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -348,7 +348,7 @@ static int wled3_sync_toggle(struct wled *wled)
> return rc;
> }
>
> -static int wled5_sync_toggle(struct wled *wled)
> +static int wled5_mod_sync_toggle(struct wled *wled)
> {
> int rc;
> u8 val;
> @@ -445,10 +445,23 @@ static int wled_update_status(struct backlight_device
> *bl)
> goto unlock_mutex;
> }
>
> - rc = wled->wled_sync_toggle(wled);
> - if (rc < 0) {
> - dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
> - goto unlock_mutex;
> + if (wled->version < 5) {
> + rc = wled->wled_sync_toggle(wled);
> + if (rc < 0) {
> + dev_err(wled->dev, "wled sync failed rc:%d\n",
> rc);
> + goto unlock_mutex;
> + }
> + } else {
> + /*
> + * For WLED5 toggling the MOD_SYNC_BIT updates the
> + * brightness
> + */
> + rc = wled5_mod_sync_toggle(wled);
> + if (rc < 0) {
> + dev_err(wled->dev, "wled mod sync failed
> rc:%d\n",
> + rc);
> + goto unlock_mutex;
> + }
> }
> }
>
> @@ -1459,7 +1472,7 @@ static int wled_configure(struct wled *wled)
> size = ARRAY_SIZE(wled5_opts);
> *cfg = wled5_config_defaults;
> wled->wled_set_brightness = wled5_set_brightness;
> - wled->wled_sync_toggle = wled5_sync_toggle;
> + wled->wled_sync_toggle = wled3_sync_toggle;
> wled->wled_cabc_config = wled5_cabc_config;
> wled->wled_ovp_delay = wled5_ovp_delay;
> wled->wled_auto_detection_required =
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
Re: [PATCH v2] kdb: Get rid of custom debug heap allocator
On Fri, Feb 26, 2021 at 03:23:06PM +0530, Sumit Garg wrote: > Currently the only user for debug heap is kdbnearsym() which can be > modified to rather ask the caller to supply a buffer for symbol name. > So do that and modify kdbnearsym() callers to pass a symbol name buffer > allocated statically and hence remove custom debug heap allocator. Why make the callers do this? The LRU buffers were managed inside kdbnearsym() why does switching to an approach with a single buffer require us to push that buffer out to the callers? > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > index 9d69169582c6..6efe9ec53906 100644 > --- a/kernel/debug/kdb/kdb_main.c > +++ b/kernel/debug/kdb/kdb_main.c > @@ -526,6 +526,7 @@ int kdbgetaddrarg(int argc, const char **argv, int > *nextarg, The documentation comment for this function has not been updated to describe the new contract on callers of this function (e.g. if they consume the symbol name they must do so before calling kdbgetaddrarg() (and maybe kdbnearsym() again). > char symbol = '\0'; > char *cp; > kdb_symtab_t symtab; > + static char namebuf[KSYM_NAME_LEN]; > > /* >* If the enable flags prohibit both arbitrary memory access > diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c > index b59aad1f0b55..9b907a84f2db 100644 > --- a/kernel/debug/kdb/kdb_support.c > +++ b/kernel/debug/kdb/kdb_support.c > @@ -57,8 +57,6 @@ int kdbgetsymval(const char *symname, kdb_symtab_t *symtab) > } > EXPORT_SYMBOL(kdbgetsymval); > > -static char *kdb_name_table[100];/* arbitrary size */ > - > /* > * kdbnearsym - Return the name of the symbol with the nearest address > * less than 'addr'. Again the documentation comment has not been updated and, in this case, is now misleading. If we move the static buffer here then the remarks section on this function is a really good place to describe what the callers must do to manage the static buffer safely as well as a convenient place to mention that we tolerate the reuse of the static buffer if kdb is re-entered becase a) kdb is broken if that happens and b) we are crash resilient if if does. > @@ -79,13 +77,11 @@ static char *kdb_name_table[100]; /* arbitrary size */ > * hold active strings, no kdb caller of kdbnearsym makes more > * than ~20 later calls before using a saved value. > */ > -int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab) > +int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab, char *namebuf) As above, I don't understand why we need to add namebuf here. I think the prototype can remain the same. Think of it simple that we have reduce the cache from having 100 entries to having just 1 ;-) . Daniel.
Re: [PATCH] kgdb: Fix to kill breakpoints on initmem after boot
On Wed, Feb 24, 2021 at 01:46:52PM +0530, Sumit Garg wrote:
> Currently breakpoints in kernel .init.text section are not handled
> correctly while allowing to remove them even after corresponding pages
> have been freed.
>
> Fix it via killing .init.text section breakpoints just prior to initmem
> pages being freed.
>
> Suggested-by: Doug Anderson
> Signed-off-by: Sumit Garg
I saw Andrew has picked this one up. That's ok for me:
Acked-by: Daniel Thompson
I already enriched kgdbtest to cover this (and they pass) so I guess
this is also:
Tested-by: Daniel Thompson
BTW this is not Cc:ed to stable and I do wonder if it crosses the
threshold to be considered a fix rather than a feature. Normally I
consider adding safety rails for kgdb to be a new feature but, in this
case, the problem would easily ensnare an inexperienced developer who is
doing nothing more than debugging their own driver (assuming they
correctly marked their probe function as .init) so I think this weighs
in favour of being a fix.
Daniel.
> ---
> include/linux/kgdb.h | 2 ++
> init/main.c | 1 +
> kernel/debug/debug_core.c | 11 +++
> 3 files changed, 14 insertions(+)
>
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> index 57b8885708e5..3aa503ef06fc 100644
> --- a/include/linux/kgdb.h
> +++ b/include/linux/kgdb.h
> @@ -361,9 +361,11 @@ extern atomic_t kgdb_active;
> extern bool dbg_is_early;
> extern void __init dbg_late_init(void);
> extern void kgdb_panic(const char *msg);
> +extern void kgdb_free_init_mem(void);
> #else /* ! CONFIG_KGDB */
> #define in_dbg_master() (0)
> #define dbg_late_init()
> static inline void kgdb_panic(const char *msg) {}
> +static inline void kgdb_free_init_mem(void) { }
> #endif /* ! CONFIG_KGDB */
> #endif /* _KGDB_H_ */
> diff --git a/init/main.c b/init/main.c
> index c68d784376ca..a446ca3d334e 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1417,6 +1417,7 @@ static int __ref kernel_init(void *unused)
> async_synchronize_full();
> kprobe_free_init_mem();
> ftrace_free_init_mem();
> + kgdb_free_init_mem();
> free_initmem();
> mark_readonly();
>
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index 229dd119f430..319381e95d1d 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -465,6 +465,17 @@ int dbg_remove_all_break(void)
> return 0;
> }
>
> +void kgdb_free_init_mem(void)
> +{
> + int i;
> +
> + /* Clear init memory breakpoints. */
> + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
> + if (init_section_contains((void *)kgdb_break[i].bpt_addr, 0))
> + kgdb_break[i].state = BP_UNDEFINED;
> + }
> +}
> +
> #ifdef CONFIG_KGDB_KDB
> void kdb_dump_stack_on_cpu(int cpu)
> {
> --
> 2.25.1
Re: [PATCH] kdb: Get rid of custom debug heap allocator
On Thu, Feb 25, 2021 at 04:52:58PM +0530, Sumit Garg wrote:
> Currently the only user for debug heap is kdbnearsym() which can be
> modified to rather ask the caller to supply a buffer for symbol name.
> So do that and modify kdbnearsym() callers to pass a symbol name buffer
> allocated from stack and hence remove custom debug heap allocator.
Is it really a good idea to increase stack usage this much? I thought
several architectures will take the debug exception on existing stacks
(and that these can nest with other exceptions).
The reason I'm concerned is that AFAICT the *purpose* of the current
heap is to minimise stack usage... and that this has the effect of
improving debugger robustness when we take exceptions on small shared
stacks.
The reason I called the heap redundant is that currently it also allows
us to have nested calls to kdbnearsym() whilst not consuming stack. In
this case, when I say nested I mean new calls to kdbnearsym() before the
previous caller has consumed the output rather than truely recursive
calls.
This is why I think the heap is pointless. In "normal" usage I don't
think there will never be a nested call to kdbnearsym() so I think a
single static buffer will suffice.
Technically speaking there is one way that kdbnearsym() can nest but I
think it is OK for that to be considered out-of-scope.
To explain...
It can nest is if we recursively enter the debugger! Recursive entry
should never happen, is pretty much untestable and, even if we tested
it, it is not a bug for an architeture to choose not to support it.
Nevertheless kgdb/kdb does include logic to handle this if an
architecture does make it as far are executing the trap. Note that
even if the architecture does somehow land in the debug trap there's
a strong chance the system is is too broken to resume (since we just
took an impossible trap). Therefore kdb will inhibit resume unless the
operator admits what they are doing won't work before trying to do it.
Therefore I think it is ok for namebuf to be statically allocated and
the only thing we need do for stability is ensure that kdbnearsym()
guarantees that namebuf[sizeof(namebuf)-1] == '\0' regardless of the
symbol length. Thus if by some miracle the system can resume after the
user has ignored the warning then kdb can't take a bad memory access
when it tries to print an overwritten symbol name. They see a few
garbage characters... but since they just told us to do something
crazy they should be expecting that.
Daniel.
PS The code to guarantee that if we read past the end of the string
we will still see a '\'0' before making an invalid memory access
should be well commented though... because its pretty nasty.
>
> This change has been tested using kgdbtest on arm64 which doesn't show
> any regressions.
>
> Suggested-by: Daniel Thompson
> Signed-off-by: Sumit Garg
> ---
> kernel/debug/kdb/kdb_debugger.c | 1 -
> kernel/debug/kdb/kdb_main.c | 6 +-
> kernel/debug/kdb/kdb_private.h | 7 +-
> kernel/debug/kdb/kdb_support.c | 294 +---
> 4 files changed, 11 insertions(+), 297 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_debugger.c b/kernel/debug/kdb/kdb_debugger.c
> index 0220afda3200..e91fc3e4edd5 100644
> --- a/kernel/debug/kdb/kdb_debugger.c
> +++ b/kernel/debug/kdb/kdb_debugger.c
> @@ -140,7 +140,6 @@ int kdb_stub(struct kgdb_state *ks)
>*/
> kdb_common_deinit_state();
> KDB_STATE_CLEAR(PAGER);
> - kdbnearsym_cleanup();
> if (error == KDB_CMD_KGDB) {
> if (KDB_STATE(DOING_KGDB))
> KDB_STATE_CLEAR(DOING_KGDB);
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index 9d69169582c6..ca525a3e0032 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -526,6 +526,7 @@ int kdbgetaddrarg(int argc, const char **argv, int
> *nextarg,
> char symbol = '\0';
> char *cp;
> kdb_symtab_t symtab;
> + char namebuf[KSYM_NAME_LEN];
>
> /*
>* If the enable flags prohibit both arbitrary memory access
> @@ -585,7 +586,7 @@ int kdbgetaddrarg(int argc, const char **argv, int
> *nextarg,
> }
>
> if (!found)
> - found = kdbnearsym(addr, );
> + found = kdbnearsym(addr, , namebuf);
>
> (*nextarg)++;
>
> @@ -1503,6 +1504,7 @@ static void kdb_md_line(const char *fmtstr, unsigned
> long addr,
> int i;
> int j;
> unsigned long word;
> + char namebuf[KSYM_NAME_LEN];
>
> memset(cbuf, '\0', sizeof(cbuf));
> if (phys)
> @@ -1518,7 +1520,7 @@ static void kdb_md_line(const char *fmtstr, unsigned
> long addr,
> break;
> kdb_printf(fmtstr,
Re: [PATCH V1 2/2] backlight: qcom-wled: Correct the sync_toggle sequence
On Wed, Feb 24, 2021 at 09:20:48AM +0530, Kiran Gunda wrote: > Currently the FSC SYNC_BIT and MOD_SYNC_BIT are toggled > from 1 to 0 to update the FSC and brightenss settings. > Change this sequence form 0 to 1 as per the hardware team > recommendation to update the FSC and brightness correctly. Again... this patch description feels somewhat rushed. A patch description is there to support code reviewer and to go on the version history to assist with future maintainance. They matter! Anyhow I don't recognise the "from 1 to 0" in the code since both before an after the change it goes "from 0 to 1" and "from 1 to 0" but in a different order. Doesn't the code actually currently implement "set then clear"? If so then, likewise the new code is adopting "clear then set". As with patch 1, the sync bits modified by wled3_sync_toggle singular or plural? Finally a description that is more sympathetic to the reviewer would be welcome. For example the following (if my guess is right and it is true) makes things much easier for the reviewer: "The sync takes place during a 0 to 1 transition of the sync bits so the hardware team recommends a clear-then-set approach in order to guarantee such a transition regardless of the previous register state". Daniel. > > Signed-off-by: Kiran Gunda > --- > drivers/video/backlight/qcom-wled.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/video/backlight/qcom-wled.c > b/drivers/video/backlight/qcom-wled.c > index aef52b9..19f83ac 100644 > --- a/drivers/video/backlight/qcom-wled.c > +++ b/drivers/video/backlight/qcom-wled.c > @@ -337,13 +337,13 @@ static int wled3_sync_toggle(struct wled *wled) > > rc = regmap_update_bits(wled->regmap, > wled->ctrl_addr + WLED3_SINK_REG_SYNC, > - mask, mask); > + mask, WLED3_SINK_REG_SYNC_CLEAR); > if (rc < 0) > return rc; > > rc = regmap_update_bits(wled->regmap, > wled->ctrl_addr + WLED3_SINK_REG_SYNC, > - mask, WLED3_SINK_REG_SYNC_CLEAR); > + mask, mask); > > return rc; > } > @@ -353,17 +353,17 @@ static int wled5_mod_sync_toggle(struct wled *wled) > int rc; > u8 val; > > - val = (wled->cfg.mod_sel == MOD_A) ? WLED5_SINK_REG_SYNC_MOD_A_BIT : > - WLED5_SINK_REG_SYNC_MOD_B_BIT; > rc = regmap_update_bits(wled->regmap, > wled->sink_addr + WLED5_SINK_REG_MOD_SYNC_BIT, > - WLED5_SINK_REG_SYNC_MASK, val); > + WLED5_SINK_REG_SYNC_MASK, 0); > if (rc < 0) > return rc; > > + val = (wled->cfg.mod_sel == MOD_A) ? WLED5_SINK_REG_SYNC_MOD_A_BIT : > + WLED5_SINK_REG_SYNC_MOD_B_BIT; > return regmap_update_bits(wled->regmap, > wled->sink_addr + WLED5_SINK_REG_MOD_SYNC_BIT, > - WLED5_SINK_REG_SYNC_MASK, 0); > + WLED5_SINK_REG_SYNC_MASK, val); > } > > static int wled_ovp_fault_status(struct wled *wled, bool *fault_set) > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
Re: [PATCH V1 1/2] backlight: qcom-wled: Fix FSC update issue for WLED5
On Wed, Feb 24, 2021 at 09:20:47AM +0530, Kiran Gunda wrote:
> Currently, for WLED5, after FSC register update MOD_SYNC_BIT
> is toggled instead of SYNC_BIT. MOD_SYNC_BIT has to be toggled
> after the brightness update and SYNC_BIT has to be toggled after
> FSC update for WLED5. Fix it.
Code looks fine but the description is a difficult to read (which makes
mining the history difficult).
Basically the descriptions here are very hard to read without the
context in PATCH 0/2. Since PATCH 0/2 won't enter the version history
that means these descriptions need to integrate some of the text from
what is currently PATCH 0/2.
I would expect this to be more like. It is basically joining together
text from PATCH 0 and PATCH 1 (I also switched to plural form for SYNC
bits... the code in the driver has mask generation based on the number
of strings, is that right?):
~~~
Currently, for WLED5, the FSC (Full scale current) setting is not
updated properly due to driver toggling the wrong register after an FSC
update.
On WLED5 we should only toggle the MOD_SYNC bit after a brightness
update. For an FSC update we need to toggle the SYNC bits instead.
Fix it by adopting the common wled3_sync_toggle() for WLED5 and
introducing new code to the brightness update path to
compensate.
~~~
Daniel.
>
> Signed-off-by: Kiran Gunda
> ---
> drivers/video/backlight/qcom-wled.c | 25 +++--
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/backlight/qcom-wled.c
> b/drivers/video/backlight/qcom-wled.c
> index 3bc7800..aef52b9 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -348,7 +348,7 @@ static int wled3_sync_toggle(struct wled *wled)
> return rc;
> }
>
> -static int wled5_sync_toggle(struct wled *wled)
> +static int wled5_mod_sync_toggle(struct wled *wled)
> {
> int rc;
> u8 val;
> @@ -445,10 +445,23 @@ static int wled_update_status(struct backlight_device
> *bl)
> goto unlock_mutex;
> }
>
> - rc = wled->wled_sync_toggle(wled);
> - if (rc < 0) {
> - dev_err(wled->dev, "wled sync failed rc:%d\n", rc);
> - goto unlock_mutex;
> + if (wled->version < 5) {
> + rc = wled->wled_sync_toggle(wled);
> + if (rc < 0) {
> + dev_err(wled->dev, "wled sync failed rc:%d\n",
> rc);
> + goto unlock_mutex;
> + }
> + } else {
> + /*
> + * For WLED5 toggling the MOD_SYNC_BIT updates the
> + * brightness
> + */
> + rc = wled5_mod_sync_toggle(wled);
> + if (rc < 0) {
> + dev_err(wled->dev, "wled mod sync failed
> rc:%d\n",
> + rc);
> + goto unlock_mutex;
> + }
> }
> }
>
> @@ -1459,7 +1472,7 @@ static int wled_configure(struct wled *wled)
> size = ARRAY_SIZE(wled5_opts);
> *cfg = wled5_config_defaults;
> wled->wled_set_brightness = wled5_set_brightness;
> - wled->wled_sync_toggle = wled5_sync_toggle;
> + wled->wled_sync_toggle = wled3_sync_toggle;
> wled->wled_cabc_config = wled5_cabc_config;
> wled->wled_ovp_delay = wled5_ovp_delay;
> wled->wled_auto_detection_required =
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
Re: [PATCH] kernel: debug: Handle breakpoints in kernel .init.text section
On Tue, Feb 23, 2021 at 02:33:50PM +0530, Sumit Garg wrote:
> Thanks Doug for your comments.
>
> On Tue, 23 Feb 2021 at 05:28, Doug Anderson wrote:
> > > To be clear there is still a very small window between call to
> > > free_initmem() and system_state = SYSTEM_RUNNING which can lead to
> > > removal of freed .init.text section breakpoints but I think we can live
> > > with that.
> >
> > I know kdb / kgdb tries to keep out of the way of the rest of the
> > system and so there's a bias to just try to infer the state of the
> > rest of the system, but this feels like a halfway solution when really
> > a cleaner solution really wouldn't intrude much on the main kernel.
> > It seems like it's at least worth asking if we can just add a call
> > like kgdb_drop_init_breakpoints() into main.c. Then we don't have to
> > try to guess the state...
Just for the record, +1. This would be a better approach.
> Sounds reasonable, will post RFC for this. I think we should call such
> function as kgdb_free_init_mem() in similar way as:
> - kprobe_free_init_mem()
> - ftrace_free_init_mem()
As is matching the names...
> @@ -378,8 +382,13 @@ int dbg_deactivate_sw_breakpoints(void)
> int i;
>
> for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
> - if (kgdb_break[i].state != BP_ACTIVE)
> + if (kgdb_break[i].state < BP_ACTIVE_INIT)
> + continue;
> + if (system_state >= SYSTEM_RUNNING &&
> + kgdb_break[i].state == BP_ACTIVE_INIT) {
> + kgdb_break[i].state = BP_UNDEFINED;
> continue;
> + }
> error = kgdb_arch_remove_breakpoint(_break[i]);
> if (error) {
> pr_info("BP remove failed: %lx\n",
>
> >
> > > + kgdb_break[i].state = BP_ACTIVE;
> > > + else
> > > + kgdb_break[i].state = BP_ACTIVE_INIT;
> >
> > I don't really see what the "BP_ACTIVE_INIT" state gets you. Why not
> > just leave it as "BP_ACTIVE" and put all the logic fully in
> > dbg_deactivate_sw_breakpoints()?
>
> Please see my response above.
>
> [which was]
> > "BP_ACTIVE_INIT" state is added specifically to handle this scenario
> > as to keep track of breakpoints that actually belong to the .init.text
> > section. And we should be able to again set breakpoints after free
> > since below change in this patch would mark them as "BP_UNDEFINED":
This answer does not say whether the BP_ACTIVE_INIT state needs to be
per-breakpoint state or whether we can infer it from the global state.
Changing the state of breakpoints in .init is a one-shot activity
whether it is triggered explicitly (e.g. kgdb_free_init_mem) or implicitly
(run the first time dbg_deactivate_sw_breakpoints is called with the system
state >= running).
As Doug has suggested it is quite possible to unify all the logic to
handle .init within a single function by running that function when the
state changes globally.
Daniel.
[GIT PULL] kgdb changes for v5.12
The following changes since commit 19c329f6808995b142b3966301f217c831e7cf31: Linux 5.11-rc4 (2021-01-17 16:37:05 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/danielt/linux.git/ tags/kgdb-5.12-rc1 for you to fetch changes up to f11e2bc682cc197e33bfd118178cadb61326dc0e: kgdb: Remove kgdb_schedule_breakpoint() (2021-02-11 10:51:56 +) kgdb patches for 5.12 Another fairly small set of changes of changes this cycle. The most significant functional change is a fix to better manage the flags when allocating memory. Additionally there is the removal of some unused code (which is slightly more dramatic than it sounds given it means there are now no tasklets in kgdb) together with a tidy up of the debug prints and some spelling corrections for the documentation. Signed-off-by: Daniel Thompson Daniel Thompson (1): kgdb: Remove kgdb_schedule_breakpoint() Lukas Bulwahn (1): kgdb: rectify kernel-doc for kgdb_unregister_io_module() Stephen Zhang (1): kdb: kdb_support: Fix debugging information problem Sumit Garg (1): kdb: Make memory allocations more robust wengjianfeng (1): kernel: debug: fix typo issue include/linux/kgdb.h | 1 - kernel/debug/debug_core.c | 28 +- kernel/debug/gdbstub.c | 4 ++-- kernel/debug/kdb/kdb_private.h | 12 +- kernel/debug/kdb/kdb_support.c | 53 -- 5 files changed, 34 insertions(+), 64 deletions(-)
Re: [PATCH v4] kdb: Simplify kdb commands registration
On Mon, Feb 22, 2021 at 06:33:18PM +0530, Sumit Garg wrote: > On Mon, 22 Feb 2021 at 17:35, Daniel Thompson > wrote: > > > > On Thu, Feb 18, 2021 at 05:39:58PM +0530, Sumit Garg wrote: > > > Simplify kdb commands registration via using linked list instead of > > > static array for commands storage. > > > > > > Signed-off-by: Sumit Garg > > > --- > > > > > > Changes in v4: > > > - Fix kdb commands memory allocation issue prior to slab being available > > > with an array of statically allocated commands. Now it works fine with > > > kgdbwait. > > > > I'm not sure this is the right approach. It's still faking dynamic usage > > when none of the callers at this stage of the boot actually are dynamic. > > > > Okay, as an alternative I came across dbg_kmalloc()/dbg_kfree() as well but > ... Last time I traced these functions I concluded that this heap can be removed if the symbol handling code is refactored a little. I'd be *seriously* reluctant to add any new callers... which I assume from your later comments you can live with ;-) . Daniel.
Re: [PATCH v4] kdb: Simplify kdb commands registration
On Thu, Feb 18, 2021 at 05:39:58PM +0530, Sumit Garg wrote:
> Simplify kdb commands registration via using linked list instead of
> static array for commands storage.
>
> Signed-off-by: Sumit Garg
> ---
>
> Changes in v4:
> - Fix kdb commands memory allocation issue prior to slab being available
> with an array of statically allocated commands. Now it works fine with
> kgdbwait.
I'm not sure this is the right approach. It's still faking dynamic usage
when none of the callers at this stage of the boot actually are dynamic.
Consider instead what would happen if there was a kdb_register_table() that
took a kdbtab_t pointer and an length and enqueued them to the new list.
The effect of this is that most of the existing kdb_register() and
kdb_register_flags() calls would become (self documenting) static
tables instead:
kdb_register_flags("md", kdb_md, "",
"Display Memory Contents, also mdWcN, e.g. md8c1", 1,
KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS);
...
Effectively becomes:
kdbtab_t maintab[] = {
{ .cmd_name = "md",
.cmd_func = mdb_md,
.cmd_usage = ",
.cmd_help = "Display Memory Contents, also mdWcN, e.g. md8c1",
.cmd_minlen = 1,
.cmd_flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
},
...
};
kdb_register_table(maintab, ARRAY_SIZE(maintab));
At that point the only users of kdb_register_flags() would be the macro
logic and that already relies on the slabs so it is OK to have dynamic
memory allocation for that.
Daniel.
PS It is also possible to switch the macro logic to simplify the
allocation by embedded a kdbtab_t into struct defcmd_set. That
would also even more tidy up of registration code... but that
could (and should) be in another patch so it doesn't all
have to land together.
> - Fix a misc checkpatch warning.
> - I have dropped Doug's review tag as I think this version includes a
> major fix that should be reviewed again.
>
> Changes in v3:
> - Remove redundant "if" check.
> - Pick up review tag from Doug.
>
> Changes in v2:
> - Remove redundant NULL check for "cmd_name".
> - Incorporate misc. comment.
>
> kernel/debug/kdb/kdb_main.c| 129
> ++---
> kernel/debug/kdb/kdb_private.h | 2 +
> 2 files changed, 47 insertions(+), 84 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> index 930ac1b..5215e04 100644
> --- a/kernel/debug/kdb/kdb_main.c
> +++ b/kernel/debug/kdb/kdb_main.c
> @@ -33,6 +33,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -84,15 +85,12 @@ static unsigned int kdb_continue_catastrophic =
> static unsigned int kdb_continue_catastrophic;
> #endif
>
> -/* kdb_commands describes the available commands. */
> -static kdbtab_t *kdb_commands;
> -#define KDB_BASE_CMD_MAX 50
> -static int kdb_max_commands = KDB_BASE_CMD_MAX;
> -static kdbtab_t kdb_base_commands[KDB_BASE_CMD_MAX];
> -#define for_each_kdbcmd(cmd, num)\
> - for ((cmd) = kdb_base_commands, (num) = 0; \
> - num < kdb_max_commands;\
> - num++, num == KDB_BASE_CMD_MAX ? cmd = kdb_commands : cmd++)
> +/* kdb_cmds_head describes the available commands. */
> +static LIST_HEAD(kdb_cmds_head);
> +
> +#define KDB_CMD_INIT_MAX 50
> +static int kdb_cmd_init_idx;
> +static kdbtab_t kdb_commands_init[KDB_CMD_INIT_MAX];
>
> typedef struct _kdbmsg {
> int km_diag;/* kdb diagnostic */
> @@ -921,7 +919,7 @@ int kdb_parse(const char *cmdstr)
> char *cp;
> char *cpp, quoted;
> kdbtab_t *tp;
> - int i, escaped, ignore_errors = 0, check_grep = 0;
> + int escaped, ignore_errors = 0, check_grep = 0;
>
> /*
>* First tokenize the command string.
> @@ -1011,25 +1009,17 @@ int kdb_parse(const char *cmdstr)
> ++argv[0];
> }
>
> - for_each_kdbcmd(tp, i) {
> - if (tp->cmd_name) {
> - /*
> - * If this command is allowed to be abbreviated,
> - * check to see if this is it.
> - */
> -
> - if (tp->cmd_minlen
> - && (strlen(argv[0]) <= tp->cmd_minlen)) {
> - if (strncmp(argv[0],
> - tp->cmd_name,
> - tp->cmd_minlen) == 0) {
> - break;
> - }
> - }
> + list_for_each_entry(tp, _cmds_head, list_node) {
> + /*
> + * If this command is allowed to be abbreviated,
> + * check to see if this is it.
> + */
> + if (tp->cmd_minlen && (strlen(argv[0]) <= tp->cmd_minlen) &&
> + (strncmp(argv[0],
Re: [PATCH 2/2] MIPS: make kgdb depend on FPU support
On Wed, Feb 10, 2021 at 03:15:10PM +0100, Maciej W. Rozycki wrote: > On Wed, 10 Feb 2021, Daniel Thompson wrote: > > > > NB if GDB sees a register padded out (FAOD it means all-x's rather than > > > a > > > hex string placed throughout the respective slot) in a `g' packet, then > > > it > > > will mark the register internally as "unavailable" and present it to the > > > receiver of the information as such rather than giving any specific > > > value. > > > I don't remember offhand what the syntax for the `G' packet is in that > > > case; possibly GDB just sends all-zeros, and in any case you can't make > > > GDB write any specific value to such a register via any user > > > interface. > > > > kgdb doesn't track register validity and adding would be a fairly big > > change. Everything internally (including some of the interactions with > > arch code) is based on updating a binary shadow of register state which > > is only bin2hex'ed just before transmitting a packet. > > I've had a peek and it doesn't appear to me it would be a big deal. > > We have `gdb_regs' defined as an array of longs. We'd just need a second > array for a register validity bitmap, which could for simplicity just have > a single bit per each byte of `gdb_regs'. It would then be updated in > `pt_regs_to_gdb_regs' according to the result of `dbg_get_reg' across the > number of bits given by `dbg_reg_def[i].size'. And then `kgdb_mem2hex' > would interpret the bitmap given as an extra argument accordingly. > > It looks to me like a couple of lines of extra code really. Agree, the core changes aren't too bad. I was more concerned about whether the validity bits would leak into the arch specific code via sleeping_thread_to_gdb_regs() and also noted the effort needed to review each architectures dbg_get_reg() implementation if we are going to react differently to it's return value. It is still not an infeasible amount of work though if someone does want to go in this direction. Daniel.
[PATCH] kgdb: Remove kgdb_schedule_breakpoint()
To the very best of my knowledge there has never been any in-tree
code that calls this function. It exists largely to support an
out-of-tree driver that provides kgdb-over-ethernet using the
netpoll API.
kgdboe has been out-of-tree for more than 10 years and I don't
recall any serious attempt to upstream it at any point in the last
five. At this stage it looks better to stop carrying this code in
the kernel and integrate the code into the out-of-tree driver
instead.
The long term trajectory for the kernel looks likely to include
effort to remove or reduce the use of tasklets (something that has
also been true for the last 10 years). Thus the main real reason
for this patch is to make explicit that the in-tree kgdb features
do not require tasklets.
Signed-off-by: Daniel Thompson
---
Notes:
During this cycle two developers have proposed tidying up the
DECLARE_TASKLET_OLD() in the debug core. Both threads ended with a
suggestion to remove kgdb_schedule_breakpoint() but I don't recall
seeing a follow up patch for either thread... so I wrote it myself.
include/linux/kgdb.h | 1 -
kernel/debug/debug_core.c | 26 --
2 files changed, 27 deletions(-)
diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
index 0d6cf64c8bb12..0444b44bd156d 100644
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -325,7 +325,6 @@ extern char *kgdb_mem2hex(char *mem, char *buf, int count);
extern int kgdb_hex2mem(char *buf, char *mem, int count);
extern int kgdb_isremovedbreak(unsigned long addr);
-extern void kgdb_schedule_breakpoint(void);
extern int kgdb_has_hit_break(unsigned long addr);
extern int
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index 7f22c1c0ffe80..b636d517c02c4 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -119,7 +119,6 @@ static DEFINE_RAW_SPINLOCK(dbg_slave_lock);
*/
static atomic_tmasters_in_kgdb;
static atomic_tslaves_in_kgdb;
-static atomic_tkgdb_break_tasklet_var;
atomic_t kgdb_setting_breakpoint;
struct task_struct *kgdb_usethread;
@@ -1084,31 +1083,6 @@ static void kgdb_unregister_callbacks(void)
}
}
-/*
- * There are times a tasklet needs to be used vs a compiled in
- * break point so as to cause an exception outside a kgdb I/O module,
- * such as is the case with kgdboe, where calling a breakpoint in the
- * I/O driver itself would be fatal.
- */
-static void kgdb_tasklet_bpt(unsigned long ing)
-{
- kgdb_breakpoint();
- atomic_set(_break_tasklet_var, 0);
-}
-
-static DECLARE_TASKLET_OLD(kgdb_tasklet_breakpoint, kgdb_tasklet_bpt);
-
-void kgdb_schedule_breakpoint(void)
-{
- if (atomic_read(_break_tasklet_var) ||
- atomic_read(_active) != -1 ||
- atomic_read(_setting_breakpoint))
- return;
- atomic_inc(_break_tasklet_var);
- tasklet_schedule(_tasklet_breakpoint);
-}
-EXPORT_SYMBOL_GPL(kgdb_schedule_breakpoint);
-
/**
* kgdb_register_io_module - register KGDB IO module
* @new_dbg_io_ops: the io ops vector
base-commit: 19c329f6808995b142b3966301f217c831e7cf31
prerequisite-patch-id: 6d9085a2ef51882c80a4f13264cd12a14dcceb54
prerequisite-patch-id: c0a2cb664281d00a6e32867896374a617aafb358
prerequisite-patch-id: 6bbcef7ce98747090ecb13fd3eda74a241e47249
prerequisite-patch-id: 8bf7c51993c06ff88809d49ed59cbace3d94604e
--
2.29.2
Re: [PATCH 2/2] MIPS: make kgdb depend on FPU support
On Wed, Feb 10, 2021 at 01:11:28PM +0100, Maciej W. Rozycki wrote: > On Wed, 10 Feb 2021, Daniel Thompson wrote: > > > > Wrapping the relevant parts of this file into #ifdef MIPS_FP_SUPPORT > > > would be as easy though and would qualify as a proper fix given that we > > > have no XML description support for the MIPS target (so we need to supply > > > the inexistent registers in the protocol; or maybe we can return NULL in > > > `dbg_get_reg' to get them padded out in the RSP packet, I haven't checked > > > if generic KGDB code supports this feature). > > > > Returning NULL should be fine. > > > > The generic code will cope OK. The values in the f.p. registers may > > act a little odd if gdb uses a 'G' packet to set them to non-zero values > > (since kgdb will cache the values gdb sent it) but the developer > > operating the debugger will probably figure out what is going on without > > too much pain. > > Ack, thanks! > > NB if GDB sees a register padded out (FAOD it means all-x's rather than a > hex string placed throughout the respective slot) in a `g' packet, then it > will mark the register internally as "unavailable" and present it to the > receiver of the information as such rather than giving any specific value. > I don't remember offhand what the syntax for the `G' packet is in that > case; possibly GDB just sends all-zeros, and in any case you can't make > GDB write any specific value to such a register via any user > interface. kgdb doesn't track register validity and adding would be a fairly big change. Everything internally (including some of the interactions with arch code) is based on updating a binary shadow of register state which is only bin2hex'ed just before transmitting a packet. It will simply default them to zero and update them on a 'G' packet. > The way the unavailability is shown depends on the interface used, i.e. > it will be different between the `info all-registers'/`info register $reg' > commands, and the `p $reg' command (or any expression involving `$reg'), > and the MI interface. But in any case it will be unambiguous. I guess this probably does create a technical protocol violation since kgdb will reject per-register read/write for register that its report says are zero rather then invalid. Daniel.
Re: [PATCH 1/4] driver core: Use subdir-ccflags-* to inherit debug flag
On Mon, Feb 08, 2021 at 09:09:20PM +0800, Yicong Yang wrote:
> On 2021/2/8 18:47, Greg KH wrote:
> > On Mon, Feb 08, 2021 at 06:44:52PM +0800, Yicong Yang wrote:
> >> On 2021/2/5 17:53, Greg KH wrote:
> >>> What does this offer in benefit of the existing way? What is it fixing?
> >>> Why do this "churn"?
> >>
> >> currently we have added ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG in the
> >> Makefile
> >> of driver/base and driver/base/power, but not in the subdirectory
> >> driver/base/firmware_loader. we cannot turn the debug on for subdirectory
> >> firmware_loader if we config DEBUG_DRIVER and there is no kconfig option
> >> for the it.
> >
> > Is that necessary? Does that directory need it?
>
> there are several debug prints in firmware_loader/main.c:
>
> ./main.c:207: pr_debug("%s: fw-%s fw_priv=%p\n", __func__, fw_name,
> fw_priv);
> ./main.c:245: pr_debug("batched request - sharing the same
> struct fw_priv and lookup for multiple requests\n");
>
Even if these are not in scope for CONFIG_DEBUG_DRVIER there is a
config option that would allow you to observe them without changing
any code (CONFIG_DYNAMIC_DEBUG).
Daniel.
Re: [PATCH 2/2] MIPS: make kgdb depend on FPU support
On Mon, Feb 08, 2021 at 06:03:08PM +0100, Maciej W. Rozycki wrote: > On Fri, 22 Jan 2021, Arnd Bergmann wrote: > > > From: Arnd Bergmann > > > > kgdb fails to build when the FPU support is disabled: > > > > arch/mips/kernel/kgdb.c: In function 'dbg_set_reg': > > arch/mips/kernel/kgdb.c:147:35: error: 'struct thread_struct' has no member > > named 'fpu' > > 147 |memcpy((void *)>thread.fpu.fcr31, mem, > > | ^ > > arch/mips/kernel/kgdb.c:155:34: error: 'struct thread_struct' has no member > > named 'fpu' > > 155 | memcpy((void *)>thread.fpu.fpr[fp_reg], mem, > > > > This is only relevant for CONFIG_EXPERT=y, so disallowing it > > in Kconfig is an easier workaround than fixing it properly. > > Wrapping the relevant parts of this file into #ifdef MIPS_FP_SUPPORT > would be as easy though and would qualify as a proper fix given that we > have no XML description support for the MIPS target (so we need to supply > the inexistent registers in the protocol; or maybe we can return NULL in > `dbg_get_reg' to get them padded out in the RSP packet, I haven't checked > if generic KGDB code supports this feature). Returning NULL should be fine. The generic code will cope OK. The values in the f.p. registers may act a little odd if gdb uses a 'G' packet to set them to non-zero values (since kgdb will cache the values gdb sent it) but the developer operating the debugger will probably figure out what is going on without too much pain. Daniel.
Re: [PATCH v3] kdb: Simplify kdb commands registration
On Mon, Feb 08, 2021 at 03:18:19PM +0530, Sumit Garg wrote:
> On Mon, 8 Feb 2021 at 15:13, Daniel Thompson
> wrote:
> >
> > On Fri, Jan 29, 2021 at 03:47:07PM +0530, Sumit Garg wrote:
> > > @@ -1011,25 +1005,17 @@ int kdb_parse(const char *cmdstr)
> > > ++argv[0];
> > > }
> > >
> > > - for_each_kdbcmd(tp, i) {
> > > - if (tp->cmd_name) {
> > > - /*
> > > - * If this command is allowed to be abbreviated,
> > > - * check to see if this is it.
> > > - */
> > > -
> > > - if (tp->cmd_minlen
> > > - && (strlen(argv[0]) <= tp->cmd_minlen)) {
> > > - if (strncmp(argv[0],
> > > - tp->cmd_name,
> > > - tp->cmd_minlen) == 0) {
> > > - break;
> > > - }
> > > - }
> > > -
> > > - if (strcmp(argv[0], tp->cmd_name) == 0)
> > > + list_for_each_entry(tp, _cmds_head, list_node) {
> > > + /*
> > > + * If this command is allowed to be abbreviated,
> > > + * check to see if this is it.
> > > + */
> > > + if (tp->cmd_minlen && (strlen(argv[0]) <= tp->cmd_minlen) &&
> > > + (strncmp(argv[0], tp->cmd_name, tp->cmd_minlen) == 0))
> > > break;
> >
> > Looks like you forgot to unindent this line.
> >
> > I will fix it up but... checkpatch would have found this.
> >
>
> Ah, I missed to run checkpatch on v3. Thanks for fixing this up.
Unfortunately, it's not just checkpatch. This patch also causes a
large number of test suite regressions. In particular it looks like
kgdbwait does not work with this patch applied.
The problem occurs on multiple architectures all with
close-to-defconfig kernels. However to share one specific
failure, x86_64_defconfig plus the following is not bootable:
../scripts/config --enable DEBUG_INFO --enable DEBUG_FS \
--enable KALLSYMS_ALL --enable MAGIC_SYSRQ --enable KGDB \
--enable KGDB_TESTS --enable KGDB_KDB --enable KDB_KEYBOARD \
--enable LKDTM
Try:
qemu-system-x86_64 \
-enable-kvm -m 1G -smp 2 -nographic
-kernel arch/x86/boot/bzImage \
-append "console=ttyS0,115200 kgdboc=ttyS0 kgdbwait"
Daniel.
Re: [PATCH v3] kdb: Simplify kdb commands registration
On Fri, Jan 29, 2021 at 03:47:07PM +0530, Sumit Garg wrote:
> @@ -1011,25 +1005,17 @@ int kdb_parse(const char *cmdstr)
> ++argv[0];
> }
>
> - for_each_kdbcmd(tp, i) {
> - if (tp->cmd_name) {
> - /*
> - * If this command is allowed to be abbreviated,
> - * check to see if this is it.
> - */
> -
> - if (tp->cmd_minlen
> - && (strlen(argv[0]) <= tp->cmd_minlen)) {
> - if (strncmp(argv[0],
> - tp->cmd_name,
> - tp->cmd_minlen) == 0) {
> - break;
> - }
> - }
> -
> - if (strcmp(argv[0], tp->cmd_name) == 0)
> + list_for_each_entry(tp, _cmds_head, list_node) {
> + /*
> + * If this command is allowed to be abbreviated,
> + * check to see if this is it.
> + */
> + if (tp->cmd_minlen && (strlen(argv[0]) <= tp->cmd_minlen) &&
> + (strncmp(argv[0], tp->cmd_name, tp->cmd_minlen) == 0))
> break;
Looks like you forgot to unindent this line.
I will fix it up but... checkpatch would have found this.
Daniel.
Re: [PATCH v2] kdb: Refactor env variables get/set code
On Thu, Feb 04, 2021 at 03:44:20PM +0530, Sumit Garg wrote:
> @@ -318,6 +318,65 @@ int kdbgetintenv(const char *match, int *value)
> }
>
> /*
> + * kdb_setenv() - Alter an existing environment variable or create a new one.
> + * @var: Name of the variable
> + * @val: Value of the variable
> + *
> + * Return: Zero on success, a kdb diagnostic on failure.
> + */
> +static int kdb_setenv(const char *var, const char *val)
> +{
> + int i;
> + char *ep;
> + size_t varlen, vallen;
> +
> + varlen = strlen(var);
> + vallen = strlen(val);
> + ep = kdballocenv(varlen + vallen + 2);
> + if (ep == (char *)0)
> + return KDB_ENVBUFFULL;
> +
> + sprintf(ep, "%s=%s", var, val);
> +
> + ep[varlen+vallen+1] = '\0';
What is this line for? It looks pointless to me.
I know it's copied from the original code but it doesn't look like the
sort of code you should want your name to appear next to in a git blame
;-) .
Daniel.
Re: [PATCH v4] kdb: kdb_support: Fix debugging information problem
On Thu, Feb 04, 2021 at 08:07:09PM +0800, Stephen Zhang wrote:
> There are several common patterns.
>
> 0:
> kdb_printf("...",...);
>
> which is the normal one.
>
> 1:
> kdb_printf("%s: "...,__func__,...)
>
> We could improve '1' to this :
>
> #define kdb_func_printf(format, args...) \
>kdb_printf("%s: " format, __func__, ## args)
>
> 2:
> if(KDB_DEBUG(AR))
> kdb_printf("%s "...,__func__,...);
>
> We could improve '2' to this :
> #define kdb_dbg_printf(mask, format, args...) \
>do { \
> if (KDB_DEBUG(mask)) \
> kdb_func_printf(format, ##
> args); \
This line is picked up by checkpatch as being overlong.
>} while (0)
>
> In additon, we changed the format code of size_t to %zu.
Should be `addition`.
> Signed-off-by: Stephen Zhang
It is arguable that there should be a Reviewed-by: from Doug here...
although given the big changes in v3 I don't think you were wrong
to drop it.
Nevertheless... given the implicit R-b ("when Daniel merges") in Doug's
comments on v3 I decided to reinstate it.
No action needed from you on this. I have fixed up all these issues
when I applied the patch. Thanks!
Daniel.
Re: [PATCH] kernel: debug: fix typo issue
On Wed, Feb 03, 2021 at 04:10:34PM +0800, samirweng1979 wrote: > From: wengjianfeng > > change 'regster' to 'register'. > > Signed-off-by: wengjianfeng Applied. Thanks.
Re: [PATCH] kgdb: rectify kernel-doc for kgdb_unregister_io_module()
On Mon, Jan 25, 2021 at 03:48:47PM +0100, Lukas Bulwahn wrote: > The command 'find ./kernel/debug/ | xargs ./scripts/kernel-doc -none' > reported a typo in the kernel-doc of kgdb_unregister_io_module(). > > Rectify the kernel-doc, such that no issues remain for ./kernel/debug/. > > Signed-off-by: Lukas Bulwahn Applied. Thanks.
Re: [PATCH] kernel: debug: fix typo issue
On Wed, Feb 03, 2021 at 07:36:09PM +0800, wengjianfeng wrote: > On Wed, 3 Feb 2021 11:23:59 + > Daniel Thompson wrote: > > > On Wed, Feb 03, 2021 at 04:10:34PM +0800, samirweng1979 wrote: > > > From: wengjianfeng > > > > > > change 'regster' to 'register'. > > > > > > Signed-off-by: wengjianfeng > > > > It looks like the Subject line might not be correct for this patch? > > > > Is it really the first time this patch has been circulated or should > > it have been tagged RESEND or v2? > >The patch was first sent on January 25, but nobody relply me, so I >just resend the patch, and the patch is the same as last time,does I >need add RESEND tag? thanks. Yes. When resending a patch adding a RESEND is a useful courtesy to let the maintainer know why they have received two copies of the same patch (and to make clear that the content is the same). Daniel.
Re: [PATCH] backlight: pcf50633: pdata may be a null pointer, null pointer dereference cause crash
On Tue, Feb 02, 2021 at 03:25:49PM -0600, FUZZ DR wrote: > Sorry about the missing description, I have a description at my local > commit. But the commit description disappeared when I used git send-email > to submit this patch. > > backlight: pcf50633: pdata may be a null pointer, null pointer dereference > causes crash > pdata has been checked at line 120 before dereference. However, it is used > without check at line 130. So just add the check, To be clear what your analyzer is reporting is a mismatch of programmer intent. In line 120 suggests a belief that pdata could be NULL whilst line 130 suggests a belief that pdata is never NULL. Your analyzer cannot not tell you which belief is incorrect, only that there is a mismatch. > It is better to write a default value to the register if the ramp_time has > a default value. Then it does not need to return -EINVAL. It will keep > consistent with the behavior at line 120. I disagree. You have assumed that line 120 is correct and that this code needs to support the case where pdata is NULL. However eleven years of history disprove this! This code is never called without valid platform data. Therefore we should fix the assumption on line 120 by making it clear that this code code expects non-NULL platform data. Given there are developers working to eliminate this kind of platform data structure (by adopting device properties instead) I don't want to make their life harder by implementing unused and untested special cases that they would have to reason about. Daniel. > > Daniel Thompson 于2021年2月2日周二 上午3:25写道： > > > On Mon, Feb 01, 2021 at 08:41:38AM -0600, Wenjia Zhao wrote: > > > Signed-off-by: Wenjia Zhao > > > > There should be a patch description here explaining why the patch > > is needed and how it works. > > > > > > > --- > > > drivers/video/backlight/pcf50633-backlight.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/video/backlight/pcf50633-backlight.c > > b/drivers/video/backlight/pcf50633-backlight.c > > > index 540dd338..43267af 100644 > > > --- a/drivers/video/backlight/pcf50633-backlight.c > > > +++ b/drivers/video/backlight/pcf50633-backlight.c > > > @@ -127,7 +127,8 @@ static int pcf50633_bl_probe(struct platform_device > > *pdev) > > > > > > platform_set_drvdata(pdev, pcf_bl); > > > > > > - pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM, > > pdata->ramp_time); > > > + if (pdata) > > > +pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM, > > pdata->ramp_time); > > > > Assuming you found this issue using a static analyzer then I think it > > might be better to if an "if (!pdata) return -EINVAL" further up the > > file instead. > > > > In other words it is better to "document" (via the return code) that the > > code does not support pdata == NULL than to add another untested code > > path. > > > > > > Daniel. > >
Re: [PATCH] kernel: debug: fix typo issue
On Wed, Feb 03, 2021 at 04:10:34PM +0800, samirweng1979 wrote:
> From: wengjianfeng
>
> change 'regster' to 'register'.
>
> Signed-off-by: wengjianfeng
It looks like the Subject line might not be correct for this patch?
Is it really the first time this patch has been circulated or should it
have been tagged RESEND or v2?
Daniel.
> ---
> kernel/debug/gdbstub.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
> index a77df59..e149a0a 100644
> --- a/kernel/debug/gdbstub.c
> +++ b/kernel/debug/gdbstub.c
> @@ -595,7 +595,7 @@ static char *gdb_hex_reg_helper(int regnum, char *out)
> dbg_reg_def[i].size);
> }
>
> -/* Handle the 'p' individual regster get */
> +/* Handle the 'p' individual register get */
> static void gdb_cmd_reg_get(struct kgdb_state *ks)
> {
> unsigned long regnum;
> @@ -610,7 +610,7 @@ static void gdb_cmd_reg_get(struct kgdb_state *ks)
> gdb_hex_reg_helper(regnum, remcom_out_buffer);
> }
>
> -/* Handle the 'P' individual regster set */
> +/* Handle the 'P' individual register set */
> static void gdb_cmd_reg_set(struct kgdb_state *ks)
> {
> unsigned long regnum;
> --
> 1.9.1
>
>
Re: [PATCH] backlight: pcf50633: pdata may be a null pointer, null pointer dereference cause crash
On Mon, Feb 01, 2021 at 08:41:38AM -0600, Wenjia Zhao wrote: > Signed-off-by: Wenjia Zhao There should be a patch description here explaining why the patch is needed and how it works. > --- > drivers/video/backlight/pcf50633-backlight.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/video/backlight/pcf50633-backlight.c > b/drivers/video/backlight/pcf50633-backlight.c > index 540dd338..43267af 100644 > --- a/drivers/video/backlight/pcf50633-backlight.c > +++ b/drivers/video/backlight/pcf50633-backlight.c > @@ -127,7 +127,8 @@ static int pcf50633_bl_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, pcf_bl); > > - pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM, pdata->ramp_time); > + if (pdata) > +pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM, pdata->ramp_time); Assuming you found this issue using a static analyzer then I think it might be better to if an "if (!pdata) return -EINVAL" further up the file instead. In other words it is better to "document" (via the return code) that the code does not support pdata == NULL than to add another untested code path. Daniel.
Re: [PATCH] kdb: Refactor env variables get/set code
On Mon, Jan 25, 2021 at 07:59:45PM +0530, Sumit Garg wrote:
> Move kdb environment related get/set APIs to a separate file in order
> to provide an abstraction for environment variables access and hence
> enhances code readability.
>
> Signed-off-by: Sumit Garg
> ---
> kernel/debug/kdb/Makefile | 2 +-
> kernel/debug/kdb/kdb_env.c | 229
> +
> kernel/debug/kdb/kdb_main.c| 201 +---
So... a couple of things bother me about this.
1. This patch mixes together real changes (new functions for example) and
code motion. That makes is needlessly difficult to review. Code
motion and changes should always be different patches.
2. I'm rather unconvinced by the premise that removing a continuous
block of functions from one file to another has a particularly big
impact on readabiity. The existing environment functions are not
scattered in many difference places so I think any gain from
moving them is lost (and then some) by the potential for painful
merge conflicts, especially if anything needs backporting.
I *think* I like the new functions (though I agree with Doug about the
naming) although it is hard to be entirely sure since they are tangled
in with the code motion.
Basically I'd rather see the patch without the code motion...
something that just extracts some of the ad-hoc environmental scans
into functions and puts the functions near the existing env
manipulation functions.
Daniel.
> kernel/debug/kdb/kdb_private.h | 3 +
> 4 files changed, 235 insertions(+), 200 deletions(-)
> create mode 100644 kernel/debug/kdb/kdb_env.c
>
> diff --git a/kernel/debug/kdb/Makefile b/kernel/debug/kdb/Makefile
> index efac857..b76aebe 100644
> --- a/kernel/debug/kdb/Makefile
> +++ b/kernel/debug/kdb/Makefile
> @@ -6,7 +6,7 @@
> # Copyright (c) 2009 Wind River Systems, Inc. All Rights Reserved.
> #
>
> -obj-y := kdb_io.o kdb_main.o kdb_support.o kdb_bt.o gen-kdb_cmds.o kdb_bp.o
> kdb_debugger.o
> +obj-y := kdb_io.o kdb_main.o kdb_support.o kdb_bt.o gen-kdb_cmds.o kdb_bp.o
> kdb_debugger.o kdb_env.o
> obj-$(CONFIG_KDB_KEYBOARD)+= kdb_keyboard.o
>
> clean-files := gen-kdb_cmds.c
> diff --git a/kernel/debug/kdb/kdb_env.c b/kernel/debug/kdb/kdb_env.c
> new file mode 100644
> index 000..33ab5e6
> --- /dev/null
> +++ b/kernel/debug/kdb/kdb_env.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Kernel Debugger Architecture Independent Environment Functions
> + *
> + * Copyright (c) 1999-2004 Silicon Graphics, Inc. All Rights Reserved.
> + * Copyright (c) 2009 Wind River Systems, Inc. All Rights Reserved.
> + * 03/02/13added new 2.5 kallsyms
> + */
> +
> +#include
> +#include
> +#include "kdb_private.h"
> +
> +/*
> + * Initial environment. This is all kept static and local to
> + * this file. We don't want to rely on the memory allocation
> + * mechanisms in the kernel, so we use a very limited allocate-only
> + * heap for new and altered environment variables. The entire
> + * environment is limited to a fixed number of entries (add more
> + * to __env[] if required) and a fixed amount of heap (add more to
> + * KDB_ENVBUFSIZE if required).
> + */
> +static char *__env[] = {
> +#if defined(CONFIG_SMP)
> + "PROMPT=[%d]kdb> ",
> +#else
> + "PROMPT=kdb> ",
> +#endif
> + "MOREPROMPT=more> ",
> + "RADIX=16",
> + "MDCOUNT=8",/* lines of md output */
> + KDB_PLATFORM_ENV,
> + "DTABCOUNT=30",
> + "NOSECT=1",
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> + (char *)0,
> +};
> +
> +static const int __nenv = ARRAY_SIZE(__env);
> +
> +/*
> + * kdbgetenv - This function will return the character string value of
> + * an environment variable.
> + * Parameters:
> + * match A character string representing an environment variable.
> + * Returns:
> + * NULLNo environment variable matches 'match'
> + * char* Pointer to string value of environment variable.
> + */
> +char *kdbgetenv(const char *match)
> +{
> + char **ep = __env;
> + int matchlen = strlen(match);
> + int i;
> +
> + for (i = 0; i < __nenv; i++) {
> + char *e = *ep++;
> +
> + if (!e)
> + continue;
> +
> + if ((strncmp(match, e, matchlen) == 0)
> + && ((e[matchlen] == '\0')
> +|| (e[matchlen] == '='))) {
> + char *cp = strchr(e, '=');
> +
> + return cp ? ++cp : "";
> + }
> + }
> + return NULL;
> +}
> +
> +/*
> + *
Re: [PATCH] kgdb: use new API for breakpoint tasklet
On Sat, Jan 23, 2021 at 07:42:37PM +0100, Emil Renner Berthing wrote:
> This converts the kgdb_tasklet_breakpoint to use the new API in
> commit 12cc923f1ccc ("tasklet: Introduce new initialization API")
>
> The new API changes the argument passed to the callback function, but
> fortunately the argument isn't used so it is straight forward to use
> DECLARE_TASKLET() rather than DECLARE_TASKLET_OLD().
This patch overlaps with a more ambitious patch from Davidlohr.
Perhaps you can join in with the other thread since the discussion
there is unresolved. See:
https://lkml.org/lkml/2021/1/14/1320
Daniel.
>
> Signed-off-by: Emil Renner Berthing
> ---
> kernel/debug/debug_core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index af6e8b4fb359..98d44c2bb0a4 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -1090,13 +1090,13 @@ static void kgdb_unregister_callbacks(void)
> * such as is the case with kgdboe, where calling a breakpoint in the
> * I/O driver itself would be fatal.
> */
> -static void kgdb_tasklet_bpt(unsigned long ing)
> +static void kgdb_tasklet_bpt(struct tasklet_struct *unused)
> {
> kgdb_breakpoint();
> atomic_set(_break_tasklet_var, 0);
> }
>
> -static DECLARE_TASKLET_OLD(kgdb_tasklet_breakpoint, kgdb_tasklet_bpt);
> +static DECLARE_TASKLET(kgdb_tasklet_breakpoint, kgdb_tasklet_bpt);
>
> void kgdb_schedule_breakpoint(void)
> {
> --
> 2.30.0
>
Re: [PATCH v3] kdb: Make memory allocations more robust
On Fri, Jan 22, 2021 at 09:25:44AM -0800, Doug Anderson wrote: > Hi, > > On Fri, Jan 22, 2021 at 3:06 AM Sumit Garg wrote: > > > > Currently kdb uses in_interrupt() to determine whether its library > > code has been called from the kgdb trap handler or from a saner calling > > context such as driver init. This approach is broken because > > in_interrupt() alone isn't able to determine kgdb trap handler entry from > > normal task context. This can happen during normal use of basic features > > such as breakpoints and can also be trivially reproduced using: > > echo g > /proc/sysrq-trigger > > I guess an alternative to your patch is to fully eliminate GFP_KDB. > It always strikes me as a sub-optimal design to choose between > GFP_ATOMIC and GFP_KERNEL like this. Presumably others must agree > because otherwise I'd expect that the overall kernel would have > something like "GFP_AUTOMATIC"? > > It doesn't feel like it'd be that hard to do something more explicit. > From a quick glance: > > * I think kdb_defcmd() and kdb_defcmd2() are always called in response > to a user typing something on the kdb command line. Those should > always be GFP_ATOMIC, right? No. I'm afraid not. The kdb parser is also used to execute kernel/debug/kdb/kdb_cmds as part of the kdb initialization. This initialization happens from the init calls rather than from the kgdb trap handler code. When I first looked at Sumit's patch I had a similar reaction to you but, whilst it is clearly it's not impossible to pass flags into the kdb parser and all its subcommands, I concluded that GFP_KDB is a better approach. BTW the reason I insisted on getting rid of the in_atomic() was to make it clear that GFP_KDB discriminates between exactly two calling contexts (normal and kgdb trap handler). I was didn't want any hints that imply GFP_KDB is a (broken) implementation of something like GFP_AUTOMATIC! Daniel.
Re: [PATCH v2] kdb: Make memory allocations more robust
On Fri, Jan 22, 2021 at 03:50:50PM +0530, Sumit Garg wrote: > Currently kdb uses in_interrupt() to determine whether it's library Looks like a good change just a few nitpicks with the description: s/it's/its/ > code has been called from the kgdb trap handler or from a saner calling > context such as driver init. This approach is broken because > in_interrupt() alone isn't able to determine kgdb trap handler entry via > normal task context such as [1]. Why footnote this and aren't breakpoints a far more natural reason to enter the debugger? The following will be clearer for backporting, etc: ... detmermine kgdb trap handler entry from normal task context. This can happen during normal use of basic features such as breakpoints and can also be trivially reproduced using: echo g > /proc/sysrq-trigger > > We can improve this by adding check for in_dbg_master() instead which > explicitly determines if we are running in debugger context. > > [1] $ echo g > /proc/sysrq-trigger > Cc: stable@ ? > Signed-off-by: Sumit Garg Daniel. > --- > > Changes in v2: > - Get rid of redundant in_atomic() check. > > kernel/debug/kdb/kdb_private.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h > index 7a4a181..344eb0d 100644 > --- a/kernel/debug/kdb/kdb_private.h > +++ b/kernel/debug/kdb/kdb_private.h > @@ -231,7 +231,7 @@ extern struct task_struct *kdb_curr_task(int); > > #define kdb_task_has_cpu(p) (task_curr(p)) > > -#define GFP_KDB (in_interrupt() ? GFP_ATOMIC : GFP_KERNEL) > +#define GFP_KDB (in_dbg_master() ? GFP_ATOMIC : GFP_KERNEL) > > extern void *debug_kmalloc(size_t size, gfp_t flags); > extern void debug_kfree(void *); > -- > 2.7.4 >
Re: [PATCH] kdb: Make memory allocations more robust
On Fri, Jan 22, 2021 at 03:08:31PM +0530, Sumit Garg wrote: > Currently kdb uses in_interrupt() to determine whether it's library > code has been called from the kgdb trap handler or from a saner calling > context such as driver init. This approach is broken because > in_interrupt() alone isn't able to determine kgdb trap handler entry via > normal task context such as [1]. > > We can improve this by adding check for in_dbg_master() which explicitly > determines if we are running in debugger context. Also, use in_atomic() > instead of in_interrupt() as the former is more appropriate to know atomic > context and moreover the later one is deprecated. Why do we need the in_atomic() here? Or put another way, why isn't in_dbg_master() sufficient? Daniel. > > [1] $ echo g > /proc/sysrq-trigger > > Signed-off-by: Sumit Garg > --- > kernel/debug/kdb/kdb_private.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h > index 7a4a181..7a9ebd9 100644 > --- a/kernel/debug/kdb/kdb_private.h > +++ b/kernel/debug/kdb/kdb_private.h > @@ -231,7 +231,7 @@ extern struct task_struct *kdb_curr_task(int); > > #define kdb_task_has_cpu(p) (task_curr(p)) > > -#define GFP_KDB (in_interrupt() ? GFP_ATOMIC : GFP_KERNEL) > +#define GFP_KDB (in_atomic() || in_dbg_master() ? GFP_ATOMIC : GFP_KERNEL) > > extern void *debug_kmalloc(size_t size, gfp_t flags); > extern void debug_kfree(void *); > -- > 2.7.4 >
Re: [PATCH] kgdb: Schedule breakpoints via workqueue
On Thu, Jan 14, 2021 at 04:13:44PM -0800, Davidlohr Bueso wrote:
> The original functionality was added back in:
>
> 1cee5e35f15 (kgdb: Add the ability to schedule a breakpoint via a tasklet)
>
> However tasklets have long been deprecated as being too heavy on
> the system by running in irq context - and this is not a performance
> critical path. If a higher priority process wants to run, it must
> wait for the tasklet to finish before doing so. Instead, generate
> the breakpoint exception in process context.
I don't agree that "this is not a performance critical path".
kgdb is a stop-the-world debugger: if the developer trying to understand
the system behaviour has commanded the system to halt then that is what
it should be doing. It should not be scheduling tasks that are not
necessary to bring the system a halt.
In other words this code is using tasklets *specifically* to benefit
from their weird calling context.
However I am aware the way the wind is blowing w.r.t. tasklets
and...
> Signed-off-by: Davidlohr Bueso
> ---
> Compile-tested only.
... this code can only ever be compile tested since AFAIK it has no
in-kernel callers.
There is a (still maintained) out-of-tree user that provides
kgdb-over-ethernet using the netpoll API. It must defer the stop to a
tasklet to avoid problems with netpoll running alongside the RX handler.
Whilst I have some sympathy, that code has been out-of-tree for more
than 10 years and I don't recall any serious attempt to upstream it at
any point in the last five.
So unless someone yells (convincingly) perhaps it's time to rip this
out and help prepare for a tasklet-free future?
Daniel.
>
> kernel/debug/debug_core.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index af6e8b4fb359..e1ff974c6b6f 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -119,7 +119,7 @@ static DEFINE_RAW_SPINLOCK(dbg_slave_lock);
> */
> static atomic_t masters_in_kgdb;
> static atomic_t slaves_in_kgdb;
> -static atomic_t kgdb_break_tasklet_var;
> +static atomic_t kgdb_break_work_var;
> atomic_t kgdb_setting_breakpoint;
>
> struct task_struct *kgdb_usethread;
> @@ -1085,27 +1085,27 @@ static void kgdb_unregister_callbacks(void)
> }
>
> /*
> - * There are times a tasklet needs to be used vs a compiled in
> + * There are times a workqueue needs to be used vs a compiled in
> * break point so as to cause an exception outside a kgdb I/O module,
> * such as is the case with kgdboe, where calling a breakpoint in the
> * I/O driver itself would be fatal.
> */
> -static void kgdb_tasklet_bpt(unsigned long ing)
> +static void kgdb_work_bpt(struct work_struct *unused)
> {
> kgdb_breakpoint();
> - atomic_set(_break_tasklet_var, 0);
> + atomic_set(_break_work_var, 0);
> }
>
> -static DECLARE_TASKLET_OLD(kgdb_tasklet_breakpoint, kgdb_tasklet_bpt);
> +static DECLARE_WORK(kgdb_async_breakpoint, kgdb_work_bpt);
>
> void kgdb_schedule_breakpoint(void)
> {
> - if (atomic_read(_break_tasklet_var) ||
> + if (atomic_read(_break_work_var) ||
> atomic_read(_active) != -1 ||
> atomic_read(_setting_breakpoint))
> return;
> - atomic_inc(_break_tasklet_var);
> - tasklet_schedule(_tasklet_breakpoint);
> + atomic_inc(_break_work_var);
> + schedule_work(_async_breakpoint);
> }
> EXPORT_SYMBOL_GPL(kgdb_schedule_breakpoint);
>
> --
> 2.26.2
>
Re: [PATCH] kgdbts: Passing ekgdbts to command line causes panic
Please avoid top posting. Threads on LKML are typically presented as
follows.
On Tue, Jan 05, 2021 at 11:21:38AM +0800, bodefang wrote:
> At 2021-01-04 19:28:54, "Daniel Thompson"
> wrote:
> >On Mon, Dec 28, 2020 at 09:58:58AM +0800, Defang Bo wrote:
> >> Similar to commit<1bd54d851f50>("kgdboc: Passing ekgdboc to command
> >> line causes panic"), kgdbts_option_setup does not check input
> >> argument before passing it to strlen. The argument would be a NULL
> >> pointer.
> >
> > Something seems to be missing here.
> >
> > The ekgdbts parameter mentioned in the subject line doesn't exist so
> > how can including it on the kernel command line could provoke a
> > panic.
> >
> > Please can you share the kernel boot arguments you used when you
> > tested this patch?
>
> I use static analysis tool to find these funcs are similar to the
> commit<1bd54d851f50>(kgdboc: Passing ekgdboc to command line causes
> panic), so it's just defensive, I haven't actually hitted this but
> there actually seems a problem here.
I don't see how a problem that occured when ekgdboc is parsed can occur
because this module does not parse ekgdbts! Are there really any
circumstances where kgdbts_option_setup() can be called with a NULL
argument?
Daniel.
Re: [PATCH] kgdbts: Passing ekgdbts to command line causes panic
On Mon, Dec 28, 2020 at 09:58:58AM +0800, Defang Bo wrote:
> Similar to commit<1bd54d851f50>("kgdboc: Passing ekgdboc to command line
> causes panic"),
> kgdbts_option_setup does not check input argument before passing it to strlen.
> The argument would be a NULL pointer.
Something seems to be missing here.
The ekgdbts parameter mentioned in the subject line doesn't exist so
how can including it on the kernel command line could provoke a panic.
Please can you share the kernel boot arguments you used when you tested
this patch?
Daniel.
> Signed-off-by: Defang Bo
> ---
> drivers/misc/kgdbts.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
> index 945701b..b077547 100644
> --- a/drivers/misc/kgdbts.c
> +++ b/drivers/misc/kgdbts.c
> @@ -1057,6 +1057,11 @@ static void kgdbts_run_tests(void)
>
> static int kgdbts_option_setup(char *opt)
> {
> + if (!opt) {
> + pr_err("kgdbts: config string not provided\n");
> + return -EINVAL;
> + }
> +
> if (strlen(opt) >= MAX_CONFIG_LEN) {
> printk(KERN_ERR "kgdbts: config string too long\n");
> return -ENOSPC;
> --
> 2.7.4
>
Re: [PATCH v2 -next] video: backlight: use DEFINE_MUTEX() for mutex lock
On Thu, Dec 24, 2020 at 09:26:01PM +0800, Zheng Yongjun wrote:
> mutex lock can be initialized automatically with DEFINE_MUTEX()
> rather than explicitly calling mutex_init().
>
> Signed-off-by: Zheng Yongjun
> ---
> drivers/video/backlight/backlight.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/video/backlight/backlight.c
> b/drivers/video/backlight/backlight.c
> index 537fe1b376ad..d7a09c422547 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -64,7 +64,7 @@
> */
>
> static struct list_head backlight_dev_list;
> -static struct mutex backlight_dev_list_mutex;
> +static DEFINE_MUTEX(backlight_dev_list_mutex);
> static struct blocking_notifier_head backlight_notifier;
Why do we want to convert one of these variables to use a
static initializers but leave the other two unchanged? Surely they
should all be changed.
Daniel.
>
> static const char *const backlight_types[] = {
> @@ -757,7 +757,6 @@ static int __init backlight_class_init(void)
> backlight_class->dev_groups = bl_device_groups;
> backlight_class->pm = _class_dev_pm_ops;
> INIT_LIST_HEAD(_dev_list);
> - mutex_init(_dev_list_mutex);
> BLOCKING_INIT_NOTIFIER_HEAD(_notifier);
>
> return 0;
> --
> 2.22.0
>
Re: [PATCH -next] video: backlight: use DEFINE_MUTEX (and mutex_init() had been too late)
On Wed, Dec 23, 2020 at 10:10:35PM +0800, Zheng Yongjun wrote:
> Signed-off-by: Zheng Yongjun
Can you explain the Subject for this patch in more detail?
If this patch is required to correct a bug then it looks to me like it
is incomplete.
Daniel.
> ---
> drivers/video/backlight/backlight.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/video/backlight/backlight.c
> b/drivers/video/backlight/backlight.c
> index 537fe1b376ad..d7a09c422547 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -64,7 +64,7 @@
> */
>
> static struct list_head backlight_dev_list;
> -static struct mutex backlight_dev_list_mutex;
> +static DEFINE_MUTEX(backlight_dev_list_mutex);
> static struct blocking_notifier_head backlight_notifier;
>
> static const char *const backlight_types[] = {
> @@ -757,7 +757,6 @@ static int __init backlight_class_init(void)
> backlight_class->dev_groups = bl_device_groups;
> backlight_class->pm = _class_dev_pm_ops;
> INIT_LIST_HEAD(_dev_list);
> - mutex_init(_dev_list_mutex);
> BLOCKING_INIT_NOTIFIER_HEAD(_notifier);
>
> return 0;
> --
> 2.22.0
>
Re: [PATCH -next] misc: use DIV_ROUND_UP macro to do calculation
On Tue, Dec 22, 2020 at 09:33:44PM +0800, Zheng Yongjun wrote: > Don't open-code DIV_ROUND_UP() kernel macro. > > Signed-off-by: Zheng Yongjun > --- > drivers/misc/kgdbts.c | 5 ++--- Arguably this patch should have kgdbts in the Subject line. > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c > index 945701bce553..cd086921a792 100644 > --- a/drivers/misc/kgdbts.c > +++ b/drivers/misc/kgdbts.c > @@ -139,9 +139,8 @@ static int restart_from_top_after_write; > static int sstep_state; > > /* Storage for the registers, in GDB format. */ > -static unsigned long kgdbts_gdb_regs[(NUMREGBYTES + > - sizeof(unsigned long) - 1) / > - sizeof(unsigned long)]; > +static unsigned long kgdbts_gdb_regs[DIV_ROUND_UP(NUMREGBYTES, > + sizeof(unsigned long)i)]; How was this patch tested? This code does not look like it is compilable. Daniel.
Re: [PATCH] x86/kgdb: Allow removal of early BPs
Hi Stefan
On Mon, Dec 14, 2020 at 03:13:12PM +0100, Stefan Saecherl wrote:
> The problem is that breakpoints that are set early (e.g. via kgdbwait)
> cannot be deleted after boot completed (to be precise after mark_rodata_ro
> ran).
>
> When setting a breakpoint early there are executable pages that are
> writable so the copy_to_kernel_nofault call in kgdb_arch_set_breakpoint
> succeeds and the breakpoint is saved as type BP_BREAKPOINT.
>
> Later in the boot write access to these pages is restricted. So when
> removing the breakpoint the copy_to_kernel_nofault call in
> kgdb_arch_remove_breakpoint is destined to fail and the breakpoint removal
> fails. So after copy_to_kernel_nofault failed try to text_poke_kgdb which
> can work around nonwriteability.
>
> One thing to consider when doing this is that code can go away during boot
> (e.g. .init.text). Previously kgdb_arch_remove_breakpoint handled this case
> gracefully by just having copy_to_kernel_nofault fail but if one then calls
> text_poke_kgdb the system dies due to the BUG_ON we moved out of
> __text_poke. To avoid this __text_poke now returns an error in case of a
> nonpresent code page and the error is handled at call site.
>
> Checkpatch complains about two uses of BUG_ON but the new code should not
> trigger BUG_ON in cases where the old didn't.
>
> Co-developed-by: Lorena Kretzschmar
> Signed-off-by: Lorena Kretzschmar
> Signed-off-by: Stefan Saecherl
I took this to be a gap in the kgdbtest suite so I added a couple of
tests that cover this area. Before this patch they failed now they
pass (at least they do for ARCH=x86).
I don't see any new failures either, so:
Tested-by: Daniel Thompson
Daniel.
> ---
> arch/x86/kernel/alternative.c | 16 +++
> arch/x86/kernel/kgdb.c| 54 ---
> 2 files changed, 48 insertions(+), 22 deletions(-)
>
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 2400ad62f330..0f145d837885 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -878,11 +878,9 @@ static void *__text_poke(void *addr, const void *opcode,
> size_t len)
> if (cross_page_boundary)
> pages[1] = virt_to_page(addr + PAGE_SIZE);
> }
> - /*
> - * If something went wrong, crash and burn since recovery paths are not
> - * implemented.
> - */
> - BUG_ON(!pages[0] || (cross_page_boundary && !pages[1]));
> +
> + if (!pages[0] || (cross_page_boundary && !pages[1]))
> + return ERR_PTR(-EFAULT);
>
> /*
>* Map the page without the global bit, as TLB flushing is done with
> @@ -976,7 +974,13 @@ void *text_poke(void *addr, const void *opcode, size_t
> len)
> {
> lockdep_assert_held(_mutex);
>
> - return __text_poke(addr, opcode, len);
> + addr = __text_poke(addr, opcode, len);
> + /*
> + * If something went wrong, crash and burn since recovery paths are not
> + * implemented.
> + */
> + BUG_ON(IS_ERR(addr));
> + return addr;
> }
>
> /**
> diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
> index ff7878df96b4..e98c9c43db7c 100644
> --- a/arch/x86/kernel/kgdb.c
> +++ b/arch/x86/kernel/kgdb.c
> @@ -731,6 +731,7 @@ void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long
> ip)
> int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
> {
> int err;
> + void *addr;
>
> bpt->type = BP_BREAKPOINT;
> err = copy_from_kernel_nofault(bpt->saved_instr, (char *)bpt->bpt_addr,
> @@ -747,8 +748,14 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
>*/
> if (mutex_is_locked(_mutex))
> return -EBUSY;
> - text_poke_kgdb((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
> -BREAK_INSTR_SIZE);
> +
> + addr = text_poke_kgdb((void *)bpt->bpt_addr,
> arch_kgdb_ops.gdb_bpt_instr,
> + BREAK_INSTR_SIZE);
> + /* This should never trigger because the above call to
> copy_from_kernel_nofault
> + * already succeeded.
> + */
> + BUG_ON(IS_ERR(addr));
> +
> bpt->type = BP_POKE_BREAKPOINT;
>
> return 0;
> @@ -756,21 +763,36 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
>
> int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
> {
> - if (bpt->type != BP_POKE_BREAKPOINT)
> - goto knl_write;
> - /*
> - * It is safe to call text_poke_kgdb() because normal kernel execution
> - * is stopped on all cores, so long as the text_mutex is not locked.
> - */
Re: [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
On Mon, Dec 14, 2020 at 10:40:55PM +0800, ChiYuan Huang wrote: > Hi, > > Daniel Thompson 於 2020年12月14日 週一 下午5:59寫道： > > > > Hi CY > > > > On Sat, Dec 12, 2020 at 12:33:43AM +0800, cy_huang wrote: > > > From: ChiYuan Huang > > > > > > Adds DT binding document for Richtek RT4831 backlight. > > > > > > Signed-off-by: ChiYuan Huang > > > > This patch got keyword filtered and brought to my attention > > but the rest of the series did not. > > > > If it was a backlight patch series you need to send it To: the > > all the backlight maintainers. > > > Yes, I'm waiting for mfd reviewing. > Due to mfd patch, I need to add backlight dt-binding patch prior to > backlight source code. > Or autobuild robot will said mfd dt-binding build fail from Rob. > That's why I send the backlight dt-binding prior to the source code. > > I still have backlight/regulator source code patch after mfd reviewing. > Do you want me to send all the patches without waiting for mfd reviewing? To some extent it's up to you. I think I would have shared all the pieces at once (although not it Lee, as mfd maintainer, had suggested otherwise). Daniel. > > > > Daniel. > > > > > > > --- > > > since v3 > > > - Move inlcude/dt-bindings/leds/rt4831-backlight.h from v3 mfd binding > > > patch to here. > > > - Add dual license tag in header and backlight binding document. > > > - Left backlight dt-binding example only. > > > --- > > > .../leds/backlight/richtek,rt4831-backlight.yaml | 76 > > > ++ > > > include/dt-bindings/leds/rt4831-backlight.h| 23 +++ > > > 2 files changed, 99 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > > > create mode 100644 include/dt-bindings/leds/rt4831-backlight.h > > > > > > diff --git > > > a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > > > > > > b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > > > new file mode 100644 > > > index ..f24c8d1 > > > --- /dev/null > > > +++ > > > b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml > > > @@ -0,0 +1,76 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: > > > http://devicetree.org/schemas/leds/backlight/richtek,rt4831-backlight.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Richtek RT4831 Backlight > > > + > > > +maintainers: > > > + - ChiYuan Huang > > > + > > > +description: | > > > + RT4831 is a mutifunctional device that can provide power to the LCD > > > display > > > + and LCD backlight. > > > + > > > + For the LCD backlight, it can provide four channel WLED driving > > > capability. > > > + Each channel driving current is up to 30mA > > > + > > > + Datasheet is available at > > > + https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.pdf > > > + > > > +properties: > > > + compatible: > > > +const: richtek,rt4831-backlight > > > + > > > + default-brightness: > > > +description: | > > > + The default brightness that applied to the system on start-up. > > > +$ref: /schemas/types.yaml#/definitions/uint32 > > > +minimum: 0 > > > +maximum: 2048 > > > + > > > + max-brightness: > > > +description: | > > > + The max brightness for the H/W limit > > > +$ref: /schemas/types.yaml#/definitions/uint32 > > > +minimum: 0 > > > +maximum: 2048 > > > + > > > + richtek,pwm-enable: > > > +description: | > > > + Specify the backlight dimming following by PWM duty or by SW > > > control. > > > +type: boolean > > > + > > > + richtek,bled-ovp-sel: > > > +description: | > > > + Backlight OVP level selection, currently support 17V/21V/25V/29V. > > > +$ref: /schemas/types.yaml#/definitions/uint8 > > > +default: 1 > > > +minimum: 0 > > > +maximum: 3 > > > + > > > + richtek,channel-use: > > > +description: | > > > + Backlight LED
Re: [PATCH -next] backlight: sky81452-backlight: convert comma to semicolon
On Mon, Dec 14, 2020 at 09:34:58PM +0800, Zheng Yongjun wrote: > Replace a comma between expression statements by a semicolon. > > Signed-off-by: Zheng Yongjun Weird! I guess it was harmless but still seriously weird ;-) Reviewed-by: Daniel Thompson Thanks! > --- > drivers/video/backlight/sky81452-backlight.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/video/backlight/sky81452-backlight.c > b/drivers/video/backlight/sky81452-backlight.c > index 8268ac43d54f..c95e0de7f4e7 100644 > --- a/drivers/video/backlight/sky81452-backlight.c > +++ b/drivers/video/backlight/sky81452-backlight.c > @@ -291,7 +291,7 @@ static int sky81452_bl_probe(struct platform_device *pdev) > } > > memset(, 0, sizeof(props)); > - props.max_brightness = SKY81452_MAX_BRIGHTNESS, > + props.max_brightness = SKY81452_MAX_BRIGHTNESS; > name = pdata->name ? pdata->name : SKY81452_DEFAULT_NAME; > bd = devm_backlight_device_register(dev, name, dev, regmap, > _bl_ops, ); > -- > 2.22.0 >
Re: [RFC HACK PATCH] PCI: dwc: layerscape: Hack around enumeration problems with Honeycomb LX2K
On Fri, Dec 11, 2020 at 05:05:58PM +, Daniel Thompson wrote:
> On Fri, Dec 11, 2020 at 08:37:40AM -0600, Rob Herring wrote:
> > On Fri, Dec 11, 2020 at 6:15 AM Daniel Thompson
> > > BTW I noticed many other pcie-designware drivers take advantage
> > > of a function called dw_pcie_wait_for_link() in their init paths...
> > > but my naive attempts to add it to the layerscape driver results
> > > in non-booting systems so I haven't embarrassed myself by including
> > > that in the patch!
> >
> > You need to look at what's pending for v5.11, because I reworked this
> > to be more unified. The ordering of init is also possibly changed. The
> > sequence is now like this:
> >
> > dw_pcie_setup_rc(pp);
> > dw_pcie_msi_init(pp);
> >
> > if (!dw_pcie_link_up(pci) && pci->ops->start_link) {
> > ret = pci->ops->start_link(pci);
> > if (ret)
> > goto err_free_msi;
> > }
> >
> > /* Ignore errors, the link may come up later */
> > dw_pcie_wait_for_link(pci);
>
> Thanks. That looks likely to fix it since IIUC dw_pcie_wait_for_link()
> will end up waiting somewhat like the double check I added to
> ls_pcie_link_up().
>
> I'll take a look at let you know.
Yes. These changes have fixed the enumeration problems for me.
I tested pci/next and I cherry picked your patch series onto v5.10 and
both are working well.
Given this fixes a bug for me, do you think there is any scope for me
to whittle down your series into patches for the stable kernels or am
I likely to find too many extra bits being pulled in?
Daniel.
Re: [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
Hi CY
On Sat, Dec 12, 2020 at 12:33:43AM +0800, cy_huang wrote:
> From: ChiYuan Huang
>
> Adds DT binding document for Richtek RT4831 backlight.
>
> Signed-off-by: ChiYuan Huang
This patch got keyword filtered and brought to my attention
but the rest of the series did not.
If it was a backlight patch series you need to send it To: the
all the backlight maintainers.
Daniel.
> ---
> since v3
> - Move inlcude/dt-bindings/leds/rt4831-backlight.h from v3 mfd binding patch
> to here.
> - Add dual license tag in header and backlight binding document.
> - Left backlight dt-binding example only.
> ---
> .../leds/backlight/richtek,rt4831-backlight.yaml | 76
> ++
> include/dt-bindings/leds/rt4831-backlight.h| 23 +++
> 2 files changed, 99 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> create mode 100644 include/dt-bindings/leds/rt4831-backlight.h
>
> diff --git
> a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
>
> b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> new file mode 100644
> index ..f24c8d1
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id:
> http://devicetree.org/schemas/leds/backlight/richtek,rt4831-backlight.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Richtek RT4831 Backlight
> +
> +maintainers:
> + - ChiYuan Huang
> +
> +description: |
> + RT4831 is a mutifunctional device that can provide power to the LCD display
> + and LCD backlight.
> +
> + For the LCD backlight, it can provide four channel WLED driving capability.
> + Each channel driving current is up to 30mA
> +
> + Datasheet is available at
> + https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.pdf
> +
> +properties:
> + compatible:
> +const: richtek,rt4831-backlight
> +
> + default-brightness:
> +description: |
> + The default brightness that applied to the system on start-up.
> +$ref: /schemas/types.yaml#/definitions/uint32
> +minimum: 0
> +maximum: 2048
> +
> + max-brightness:
> +description: |
> + The max brightness for the H/W limit
> +$ref: /schemas/types.yaml#/definitions/uint32
> +minimum: 0
> +maximum: 2048
> +
> + richtek,pwm-enable:
> +description: |
> + Specify the backlight dimming following by PWM duty or by SW control.
> +type: boolean
> +
> + richtek,bled-ovp-sel:
> +description: |
> + Backlight OVP level selection, currently support 17V/21V/25V/29V.
> +$ref: /schemas/types.yaml#/definitions/uint8
> +default: 1
> +minimum: 0
> +maximum: 3
> +
> + richtek,channel-use:
> +description: |
> + Backlight LED channel to be used.
> + BIT 0/1/2/3 is used to indicate led channel 1/2/3/4 enable or disable.
> +$ref: /schemas/types.yaml#/definitions/uint8
> +minimum: 1
> +maximum: 15
> +
> +required:
> + - compatible
> + - richtek,channel-use
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +#include
> +backlight {
> + compatible = "richtek,rt4831-backlight";
> + default-brightness = <1024>;
> + max-brightness = <2048>;
> + richtek,bled-ovp-sel = /bits/ 8 ;
> + richtek,channel-use = /bits/ 8 ;
> +};
> diff --git a/include/dt-bindings/leds/rt4831-backlight.h
> b/include/dt-bindings/leds/rt4831-backlight.h
> new file mode 100644
> index ..125c635
> --- /dev/null
> +++ b/include/dt-bindings/leds/rt4831-backlight.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * This header provides constants for rt4831 backlight bindings.
> + *
> + * Copyright (C) 2020, Richtek Technology Corp.
> + * Author: ChiYuan Huang
> + */
> +
> +#ifndef _DT_BINDINGS_RT4831_BACKLIGHT_H
> +#define _DT_BINDINGS_RT4831_BACKLIGHT_H
> +
> +#define RT4831_BLOVPLVL_17V 0
> +#define RT4831_BLOVPLVL_21V 1
> +#define RT4831_BLOVPLVL_25V 2
> +#define RT4831_BLOVPLVL_29V 3
> +
> +#define RT4831_BLED_CH1EN(1 << 0)
> +#define RT4831_BLED_CH2EN(1 << 1)
> +#define RT4831_BLED_CH3EN(1 << 2)
> +#define RT4831_BLED_CH4EN(1 << 3)
> +#define RT4831_BLED_ALLCHEN ((1 << 4) - 1)
> +
> +#endif /* _DT_BINDINGS_RT4831_BACKLIGHT_H */
> --
> 2.7.4
>
Re: [RFC HACK PATCH] PCI: dwc: layerscape: Hack around enumeration problems with Honeycomb LX2K
On Fri, Dec 11, 2020 at 08:37:40AM -0600, Rob Herring wrote:
> On Fri, Dec 11, 2020 at 6:15 AM Daniel Thompson
> wrote:
> >
> > I have been chasing down a problem enumerating an NVMe drive on a
> > Honeycomb LX2K (NXP LX2160A). Specifically the drive can only enumerate
> > successfully if the we are emitting lots of console messages via a UART.
> > If the system is booted with `quiet` set then enumeration fails.
>
> We really need to get away from work-arounds for device X on host Y. I
> suspect they are not limited to that combination only...
No objection on that. This patch was essentially sharing the result of
an investigation where I got stuck at the "now fix it properly" stage!
> How exactly does it fail to enumerate? There's a (racy) linkup check
> on config accesses, is it reporting link down and skipping config
> accesses?
In dmesg terms it looked like this:
--- nvme_borked_gpu_working.linted.dmesg2020-11-18 15:28:35.544118213
+
+++ nvme_working_gpu_working.linted.dmesg 2020-11-18 15:28:56.180076946
+
@@ -241,11 +241,19 @@
pci :00:00.0: reg 0x38: [mem 0x904800-0x9048ff pref]
pci :00:00.0: supports D1 D2
pci :00:00.0: PME# supported from D0 D1 D2 D3hot
+pci :01:00.0: [15b7:5009] type 00 class 0x010802
+pci :01:00.0: reg 0x10: [mem 0x904900-0x9049003fff 64bit]
+pci :01:00.0: reg 0x20: [mem 0x9049004000-0x90490040ff 64bit]
pci :00:00.0: BAR 1: assigned [mem 0x904000-0x9043ff]
pci :00:00.0: BAR 0: assigned [mem 0x904400-0x9044ff]
pci :00:00.0: BAR 6: assigned [mem 0x904500-0x9045ff pref]
+pci :00:00.0: BAR 14: assigned [mem 0x904600-0x90460f]
+pci :01:00.0: BAR 0: assigned [mem 0x904600-0x9046003fff 64bit]
+pci :01:00.0: BAR 4: assigned [mem 0x9046004000-0x90460040ff 64bit]
pci :00:00.0: PCI bridge to [bus 01-ff]
+pci :00:00.0: bridge window [mem 0x904600-0x90460f]
pci :00:00.0: Max Payload Size set to 256/ 256 (was 128), Max Read Rq
256
+pci :01:00.0: Max Payload Size set to 256/ 512 (was 128), Max Read Rq
256
layerscape-pcie 380.pcie: host bridge /soc/pcie@380 ranges:
layerscape-pcie 380.pcie: MEM 0xa04000..0xa07fff ->
0x004000
layerscape-pcie 380.pcie: PCI host bridge to bus 0001:00
... and be aware that the last three lines here are another PCIe
controller coming up just fine and it is about to detect the GPU
(which like the NVMe is also gen3) just fine whether or not we
add a short delay.
> > I guessed this would be due to the timing impact of printk-to-UART and
> > tried to find out where a delay could be added to provoke a successful
> > enumeration.
> >
> > This patch contains the results. The delay length (1ms) was selected by
> > binary searching downwards until the delay was not effective for these
> > devices (Honeycomb LX2K and a Western Digital WD Blue SN550).
> >
> > I have also included the workaround twice (conditionally compiled). The
> > first change is the *latest* possible code path that we can deploy a
> > delay whilst the second is the earliest place I could find.
> >
> > The summary is that the critical window were we are currently relying on
> > a call to the console UART code can "mend" the driver runs from calling
> > dw_pcie_setup_rc() in host init to just before we read the state in the
> > link up callback.
> >
> > Signed-off-by: Daniel Thompson
> > ---
> >
> > Notes:
> > This patch is RFC (and HACK) because I don't have much clue *why* this
> > patch works... merely that this is the smallest possible change I need
> > to replicate the current accidental printk() workaround. Perhaps one
> > could argue that RFC here stands for request-for-clue. All my
> > observations and changes here are empirical and I don't know how best to
> > turn them into something that is not a hack!
> >
> > BTW I noticed many other pcie-designware drivers take advantage
> > of a function called dw_pcie_wait_for_link() in their init paths...
> > but my naive attempts to add it to the layerscape driver results
> > in non-booting systems so I haven't embarrassed myself by including
> > that in the patch!
>
> You need to look at what's pending for v5.11, because I reworked this
> to be more unified. The ordering of init is also possibly changed. The
> sequence is now like this:
>
> dw_pcie_setup_rc(pp);
> dw_pcie_msi_init(pp);
>
> if (!dw_pcie_link_up(pci) && pci->ops->start_link) {
> ret = pci->ops->start_link(pci);
> if (ret)
> goto err_free_msi;
> }
>
> /* Ignore errors, the link may come up later */
> dw_pcie_wait_for_link(pci);
Thanks. That looks likely to fix it since IIUC dw_pcie_wait_for_link()
will end up waiting somewhat like the double check I added to
ls_pcie_link_up().
I'll take a look at let you know.
Daniel.
Re: [PATCH RESEND net-next 1/2] dpaa2-eth: send a scatter-gather FD instead of realloc-ing
On Fri, Dec 11, 2020 at 02:01:28PM +, Ioana Ciornei wrote: > On Thu, Dec 10, 2020 at 08:06:36PM +0200, Ioana Ciornei wrote: > > [Added also the netdev mailing list, I haven't heard of linux-netdev > > before but kept it] > > > > On Thu, Dec 10, 2020 at 05:31:56PM +, Daniel Thompson wrote: > > > Hi Ioana > > > > Hi Daniel, > > > > > > > > On Mon, Jun 29, 2020 at 06:47:11PM +, Ioana Ciornei wrote: > > > > Instead of realloc-ing the skb on the Tx path when the provided headroom > > > > is smaller than the HW requirements, create a Scatter/Gather frame > > > > descriptor with only one entry. > > > > > > > > Remove the '[drv] tx realloc frames' counter exposed previously through > > > > ethtool since it is no longer used. > > > > > > > > Signed-off-by: Ioana Ciornei > > > > --- > > > > > > I've been chasing down a networking regression on my LX2160A board > > > (Honeycomb LX2K based on CEx7 LX2160A COM) that first appeared in v5.9. > > > > > > It makes the board unreliable opening outbound connections meaning > > > things like `apt update` or `git fetch` often can't open the connection. > > > It does not happen all the time but is sufficient to make the boards > > > built-in networking useless for workstation use. > > > > > > The problem is strongly linked to warnings in the logs so I used the > > > warnings to bisect down to locate the cause of the regression and it > > > pinpointed this patch. I have confirmed that in both v5.9 and v5.10-rc7 > > > that reverting this patch (and fixing up the merge issues) fixes the > > > regression and the warnings stop appearing. > > > > > > A typical example of the warning is below (io-pgtable-arm.c:281 is an > > > error path that I guess would cause dma_map_page_attrs() to return > > > an error): > > > > > > [ 714.464927] WARNING: CPU: 13 PID: 0 at > > > drivers/iommu/io-pgtable-arm.c:281 __arm_lpae_map+0x2d4/0x30c > > > [ 714.464930] Modules linked in: snd_seq_dummy(E) snd_hrtimer(E) > > > snd_seq(E) snd_seq_device(E) snd_timer(E) snd(E) soundcore(E) bridge(E) > > > stp(E) llc(E) rfkill(E) caam_jr(E) crypto_engine(E) rng_core(E) > > > joydev(E) evdev(E) dpaa2_caam(E) caamhash_desc(E) caamalg_desc(E) > > > authenc(E) libdes(E) dpaa2_console(E) ofpart(E) caam(E) sg(E) error(E) > > > lm90(E) at24(E) spi_nor(E) mtd(E) sbsa_gwdt(E) qoriq_thermal(E) > > > layerscape_edac_mod(E) qoriq_cpufreq(E) drm(E) fuse(E) configfs(E) > > > ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) > > > mbcache(E) jbd2(E) hid_generic(E) usbhid(E) hid(E) dm_crypt(E) dm_mod(E) > > > sd_mod(E) fsl_dpaa2_ptp(E) ptp_qoriq(E) fsl_dpaa2_eth(E) > > > xhci_plat_hcd(E) xhci_hcd(E) usbcore(E) aes_ce_blk(E) crypto_simd(E) > > > cryptd(E) aes_ce_cipher(E) ghash_ce(E) gf128mul(E) at803x(E) libaes(E) > > > fsl_mc_dpio(E) pcs_lynx(E) rtc_pcf2127(E) sha2_ce(E) phylink(E) > > > xgmac_mdio(E) regmap_spi(E) of_mdio(E) sha256_arm64(E) > > > i2c_mux_pca954x(E) fixed_phy(E) i2c_mux(E) sha1_ce(E) ptp(E) libphy(E) > > > [ 714.465131] pps_core(E) ahci_qoriq(E) libahci_platform(E) nvme(E) > > > libahci(E) nvme_core(E) t10_pi(E) libata(E) crc_t10dif(E) > > > crct10dif_generic(E) crct10dif_common(E) dwc3(E) scsi_mod(E) udc_core(E) > > > roles(E) ulpi(E) sdhci_of_esdhc(E) sdhci_pltfm(E) sdhci(E) > > > spi_nxp_fspi(E) i2c_imx(E) fixed(E) > > > [ 714.465192] CPU: 13 PID: 0 Comm: swapper/13 Tainted: GW E > > > 5.10.0-rc7-1-gba98d13279ca #52 > > > [ 714.465196] Hardware name: SolidRun LX2160A Honeycomb (DT) > > > [ 714.465202] pstate: 6005 (nZCv daif -PAN -UAO -TCO BTYPE=--) > > > [ 714.465207] pc : __arm_lpae_map+0x2d4/0x30c > > > [ 714.465211] lr : __arm_lpae_map+0x114/0x30c > > > [ 714.465215] sp : 80001006b340 > > > [ 714.465219] x29: 80001006b340 x28: 002086538003 > > > [ 714.465227] x27: 0a20 x26: 1000 > > > [ 714.465236] x25: 0f44 x24: 0020adf8d000 > > > [ 714.465245] x23: 0001 x22: faeca000 > > > [ 714.465253] x21: 0003 x20: 19b60d64d200 > > > [ 714.465261] x19: 00ca x18: > > > [ 714.465270] x17: x16: cccb7cf3ca20 > > > [ 714.465278] x15: x14: > > > [ 714.465286] x13: 0003
[RFC HACK PATCH] PCI: dwc: layerscape: Hack around enumeration problems with Honeycomb LX2K
I have been chasing down a problem enumerating an NVMe drive on a
Honeycomb LX2K (NXP LX2160A). Specifically the drive can only enumerate
successfully if the we are emitting lots of console messages via a UART.
If the system is booted with `quiet` set then enumeration fails.
I guessed this would be due to the timing impact of printk-to-UART and
tried to find out where a delay could be added to provoke a successful
enumeration.
This patch contains the results. The delay length (1ms) was selected by
binary searching downwards until the delay was not effective for these
devices (Honeycomb LX2K and a Western Digital WD Blue SN550).
I have also included the workaround twice (conditionally compiled). The
first change is the *latest* possible code path that we can deploy a
delay whilst the second is the earliest place I could find.
The summary is that the critical window were we are currently relying on
a call to the console UART code can "mend" the driver runs from calling
dw_pcie_setup_rc() in host init to just before we read the state in the
link up callback.
Signed-off-by: Daniel Thompson
---
Notes:
This patch is RFC (and HACK) because I don't have much clue *why* this
patch works... merely that this is the smallest possible change I need
to replicate the current accidental printk() workaround. Perhaps one
could argue that RFC here stands for request-for-clue. All my
observations and changes here are empirical and I don't know how best to
turn them into something that is not a hack!
BTW I noticed many other pcie-designware drivers take advantage
of a function called dw_pcie_wait_for_link() in their init paths...
but my naive attempts to add it to the layerscape driver results
in non-booting systems so I haven't embarrassed myself by including
that in the patch!
drivers/pci/controller/dwc/pci-layerscape.c | 35 +
1 file changed, 35 insertions(+)
diff --git a/drivers/pci/controller/dwc/pci-layerscape.c
b/drivers/pci/controller/dwc/pci-layerscape.c
index f24f79a70d9a..c354904b90ef 100644
--- a/drivers/pci/controller/dwc/pci-layerscape.c
+++ b/drivers/pci/controller/dwc/pci-layerscape.c
@@ -22,6 +22,8 @@
#include "pcie-designware.h"
+#define WORKAROUND_LATEST_POSSIBLE
+
/* PEX1/2 Misc Ports Status Register */
#define SCFG_PEXMSCPORTSR(pex_idx) (0x94 + (pex_idx) * 4)
#define LTSSM_STATE_SHIFT 20
@@ -113,10 +115,31 @@ static int ls_pcie_link_up(struct dw_pcie *pci)
struct ls_pcie *pcie = to_ls_pcie(pci);
u32 state;
+ /*
+* Strictly speaking *this* (before the ioread32) is the latest
+* point a simple delay can be effective. If we move the delay
+* after the ioread32 then the NVMe does not enumerate.
+*
+* However this function appears to be frequently called so an
+* unconditional delay here causes noticeable delay at boot
+* time. Hence we implement the workaround by retrying the read
+* after a short delay if we think we might need to return false.
+*/
+
state = (ioread32(pcie->lut + pcie->drvdata->lut_dbg) >>
pcie->drvdata->ltssm_shift) &
LTSSM_STATE_MASK;
+#ifdef WORKAROUND_LATEST_POSSIBLE
+ if (state < LTSSM_PCIE_L0) {
+ /* see comment above */
+ mdelay(1);
+ state = (ioread32(pcie->lut + pcie->drvdata->lut_dbg) >>
+pcie->drvdata->ltssm_shift) &
+LTSSM_STATE_MASK;
+ }
+#endif
+
if (state < LTSSM_PCIE_L0)
return 0;
@@ -152,6 +175,18 @@ static int ls_pcie_host_init(struct pcie_port *pp)
dw_pcie_setup_rc(pp);
+#ifdef WORKAROUND_EARLIEST_POSSIBLE
+ /*
+* This is the earliest point the delay is effective.
+* If we move it before dw_pcie_setup_rc() then the
+* NVMe does not enumerate.
+*
+* 500us is too short to reliably work around the issue
+* hence adopting 1000us here.
+*/
+ mdelay(1);
+#endif
+
return 0;
}
base-commit: 0477e92881850d44910a7e94fc2c46f96faa131f
--
2.29.2
Re: [PATCH RESEND net-next 1/2] dpaa2-eth: send a scatter-gather FD instead of realloc-ing
Hi Ioana On Mon, Jun 29, 2020 at 06:47:11PM +, Ioana Ciornei wrote: > Instead of realloc-ing the skb on the Tx path when the provided headroom > is smaller than the HW requirements, create a Scatter/Gather frame > descriptor with only one entry. > > Remove the '[drv] tx realloc frames' counter exposed previously through > ethtool since it is no longer used. > > Signed-off-by: Ioana Ciornei > --- I've been chasing down a networking regression on my LX2160A board (Honeycomb LX2K based on CEx7 LX2160A COM) that first appeared in v5.9. It makes the board unreliable opening outbound connections meaning things like `apt update` or `git fetch` often can't open the connection. It does not happen all the time but is sufficient to make the boards built-in networking useless for workstation use. The problem is strongly linked to warnings in the logs so I used the warnings to bisect down to locate the cause of the regression and it pinpointed this patch. I have confirmed that in both v5.9 and v5.10-rc7 that reverting this patch (and fixing up the merge issues) fixes the regression and the warnings stop appearing. A typical example of the warning is below (io-pgtable-arm.c:281 is an error path that I guess would cause dma_map_page_attrs() to return an error): [ 714.464927] WARNING: CPU: 13 PID: 0 at drivers/iommu/io-pgtable-arm.c:281 __arm_lpae_map+0x2d4/0x30c [ 714.464930] Modules linked in: snd_seq_dummy(E) snd_hrtimer(E) snd_seq(E) snd_seq_device(E) snd_timer(E) snd(E) soundcore(E) bridge(E) stp(E) llc(E) rfkill(E) caam_jr(E) crypto_engine(E) rng_core(E) joydev(E) evdev(E) dpaa2_caam(E) caamhash_desc(E) caamalg_desc(E) authenc(E) libdes(E) dpaa2_console(E) ofpart(E) caam(E) sg(E) error(E) lm90(E) at24(E) spi_nor(E) mtd(E) sbsa_gwdt(E) qoriq_thermal(E) layerscape_edac_mod(E) qoriq_cpufreq(E) drm(E) fuse(E) configfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E) crc16(E) mbcache(E) jbd2(E) hid_generic(E) usbhid(E) hid(E) dm_crypt(E) dm_mod(E) sd_mod(E) fsl_dpaa2_ptp(E) ptp_qoriq(E) fsl_dpaa2_eth(E) xhci_plat_hcd(E) xhci_hcd(E) usbcore(E) aes_ce_blk(E) crypto_simd(E) cryptd(E) aes_ce_cipher(E) ghash_ce(E) gf128mul(E) at803x(E) libaes(E) fsl_mc_dpio(E) pcs_lynx(E) rtc_pcf2127(E) sha2_ce(E) phylink(E) xgmac_mdio(E) regmap_spi(E) of_mdio(E) sha256_arm64(E) i2c_mux_pca954x(E) fixed_phy(E) i2c_mux(E) sha1_ce(E) ptp(E) libphy(E) [ 714.465131] pps_core(E) ahci_qoriq(E) libahci_platform(E) nvme(E) libahci(E) nvme_core(E) t10_pi(E) libata(E) crc_t10dif(E) crct10dif_generic(E) crct10dif_common(E) dwc3(E) scsi_mod(E) udc_core(E) roles(E) ulpi(E) sdhci_of_esdhc(E) sdhci_pltfm(E) sdhci(E) spi_nxp_fspi(E) i2c_imx(E) fixed(E) [ 714.465192] CPU: 13 PID: 0 Comm: swapper/13 Tainted: GW E 5.10.0-rc7-1-gba98d13279ca #52 [ 714.465196] Hardware name: SolidRun LX2160A Honeycomb (DT) [ 714.465202] pstate: 6005 (nZCv daif -PAN -UAO -TCO BTYPE=--) [ 714.465207] pc : __arm_lpae_map+0x2d4/0x30c [ 714.465211] lr : __arm_lpae_map+0x114/0x30c [ 714.465215] sp : 80001006b340 [ 714.465219] x29: 80001006b340 x28: 002086538003 [ 714.465227] x27: 0a20 x26: 1000 [ 714.465236] x25: 0f44 x24: 0020adf8d000 [ 714.465245] x23: 0001 x22: faeca000 [ 714.465253] x21: 0003 x20: 19b60d64d200 [ 714.465261] x19: 00ca x18: [ 714.465270] x17: x16: cccb7cf3ca20 [ 714.465278] x15: x14: [ 714.465286] x13: 0003 x12: 0010 [ 714.465294] x11: x10: 0002 [ 714.465302] x9 : cccb7d5b6e78 x8 : 01ff [ 714.465311] x7 : 19b606538650 x6 : 19b606538000 [ 714.465319] x5 : 0009 x4 : 0f44 [ 714.465327] x3 : 1000 x2 : 0020adf8d000 [ 714.465335] x1 : 0002 x0 : 0003 [ 714.465343] Call trace: [ 714.465348] __arm_lpae_map+0x2d4/0x30c [ 714.465353] __arm_lpae_map+0x114/0x30c [ 714.465357] __arm_lpae_map+0x114/0x30c [ 714.465362] __arm_lpae_map+0x114/0x30c [ 714.465366] arm_lpae_map+0xf4/0x180 [ 714.465373] arm_smmu_map+0x4c/0xc0 [ 714.465379] __iommu_map+0x100/0x2bc [ 714.465385] iommu_map_atomic+0x20/0x30 [ 714.465391] __iommu_dma_map+0xb0/0x110 [ 714.465397] iommu_dma_map_page+0xb8/0x120 [ 714.465404] dma_map_page_attrs+0x1a8/0x210 [ 714.465413] __dpaa2_eth_tx+0x384/0xbd0 [fsl_dpaa2_eth] [ 714.465421] dpaa2_eth_tx+0x84/0x134 [fsl_dpaa2_eth] [ 714.465427] dev_hard_start_xmit+0x10c/0x2b0 [ 714.465433] sch_direct_xmit+0x1a0/0x550 [ 714.465438] __qdisc_run+0x140/0x670 [ 714.465443] __dev_queue_xmit+0x6c4/0xa74 [ 714.465449] dev_queue_xmit+0x20/0x2c [ 714.465463] br_dev_queue_push_xmit+0xc4/0x1a0 [bridge] [ 714.465476] br_forward_finish+0xdc/0xf0 [bridge] [ 714.465489] __br_forward+0x160/0x1c0 [bridge] [ 714.465502] br_forward+0x13c/0x160 [bridge] [
[PATCH] ARM: Kconfig: Select ARCH_HAVE_NMI_SAFE_CMPXCHG where possible
Currently ARCH_HAVE_NMI_SAFE_CMPXCHG is not set on Arm systems and this makes it impossible to enable features such as ftrace histogram triggers on Arm platforms. Most Arm systems are NMI safe simply because there is no NMI but this isn't universally true meaning we cannot set ARCH_HAVE_NMI_SAFE_CMPXCHG for all Arm devices. However the load/store exclusive implementation of cmpxchg is NMI-safe and this implementation is used ARMv6k and later. Let's select ARCH_HAVE_NMI_SAFE_CMPXCHG for these systems. Note that ARMv6 uses load/store exclusive for 32-bit cmpxchg but relies on interrupt masking for 8- and 16-bit operations. This patch is conservative and does not change behaviour for CPU_V6. Signed-off-by: Daniel Thompson --- arch/arm/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 002e0cf025f59..fd434c5958b62 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -24,6 +24,7 @@ config ARM select ARCH_HAS_TEARDOWN_DMA_OPS if MMU select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST select ARCH_HAVE_CUSTOM_GPIO_H + select ARCH_HAVE_NMI_SAFE_CMPXCHG if CPU_V7 || CPU_V7M || CPU_V6K select ARCH_HAS_GCOV_PROFILE_ALL select ARCH_KEEP_MEMBLOCK if HAVE_ARCH_PFN_VALID || KEXEC select ARCH_MIGHT_HAVE_PC_PARPORT base-commit: 0477e92881850d44910a7e94fc2c46f96faa131f -- 2.28.0
Re: [PATCH 0/5] drop unused BACKLIGHT_GENERIC option
On Mon, Nov 30, 2020 at 03:21:32PM +, Andrey Zhizhikin wrote:
> Since the removal of generic_bl driver from the source tree in commit
> 7ecdea4a0226 ("backlight: generic_bl: Remove this driver as it is
> unused") BACKLIGHT_GENERIC config option became obsolete as well and
> therefore subject to clean-up from all configuration files.
>
> This series introduces patches to address this removal, separated by
> architectures in the kernel tree.
>
> Andrey Zhizhikin (5):
> ARM: configs: drop unused BACKLIGHT_GENERIC option
> arm64: defconfig: drop unused BACKLIGHT_GENERIC option
> MIPS: configs: drop unused BACKLIGHT_GENERIC option
> parisc: configs: drop unused BACKLIGHT_GENERIC option
> powerpc/configs: drop unused BACKLIGHT_GENERIC option
Whole series:
Acked-by: Daniel Thompson
Daniel.
Re: [PATCH] Documentation: kgdb: Fix a typo
On Mon, Nov 16, 2020 at 05:42:47PM +0800, Tiezhu Yang wrote: > "to into" -> "into" > > Reported-by: Sergei Shtylyov > Signed-off-by: Tiezhu Yang Acked-by: Daniel Thompson > --- > Documentation/dev-tools/kgdb.rst | 7 +++ > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/Documentation/dev-tools/kgdb.rst > b/Documentation/dev-tools/kgdb.rst > index 77b688e..4345624 100644 > --- a/Documentation/dev-tools/kgdb.rst > +++ b/Documentation/dev-tools/kgdb.rst > @@ -63,10 +63,9 @@ will want to turn on ``CONFIG_DEBUG_INFO`` which is called > It is advised, but not required, that you turn on the > ``CONFIG_FRAME_POINTER`` kernel option which is called > :menuselection:`Compile > the kernel with frame pointers` in the config menu. This option inserts code > -to into the compiled executable which saves the frame information in > -registers or on the stack at different points which allows a debugger > -such as gdb to more accurately construct stack back traces while > -debugging the kernel. > +into the compiled executable which saves the frame information in registers > +or on the stack at different points which allows a debugger such as gdb to > +more accurately construct stack back traces while debugging the kernel. > > If the architecture that you are using supports the kernel option > ``CONFIG_STRICT_KERNEL_RWX``, you should consider turning it off. This > -- > 2.1.0 >
Re: [PATCH] Replaced hard coded function names in debug messages with __func__ macro.
On Tue, Nov 03, 2020 at 09:36:54PM +0100, Tabot Kevin wrote: > On Tue, Nov 03, 2020 at 10:04:40AM +0000, Daniel Thompson wrote: > > On Mon, Nov 02, 2020 at 01:15:56PM +0100, Tabot Kevin wrote: > > > On Mon, Nov 02, 2020 at 09:33:24AM +0000, Daniel Thompson wrote: > > > > > @@ -146,7 +146,7 @@ static int ov2680_g_bin_factor_x(struct > > > > > v4l2_subdev *sd, s32 *val) > > > > > struct ov2680_device *dev = to_ov2680_sensor(sd); > > > > > struct i2c_client *client = v4l2_get_subdevdata(sd); > > > > > > > > > > - dev_dbg(>dev, "ov2680_g_bin_factor_x\n"); > > > > > + dev_dbg(>dev, "%s\n", __func__); > > > > > > > > It might be better just to remove this sort of message. > > > > > > > > They are not "wrong wrong" but are they actually useful one a > > > > driver's basic functions work? Even where they are useful > > > > dynamic techniques (ftrace, tracepoints, etc) arguably provide a > > > > better way to support "did my function actually run" debug > > > > approaches anyway. > > > > > > Thank you very much for the response. So, should I just revert back to > > > the original all the changes in places where I replace hard coded > > > functions names with __func__? > > > > Personally I think it is better to remove them completely from the > > driver rather than revert to the original form. Naturally if Mauro or > > Sakari have strong views on this kind of printed message then you > > need to take that into account but, in general, messages like this > > add little or no value to the driver and can be removed. > > > I went through the code in an attempt to completely remove all "dev_dbg" > messages, The goal should not be to remove all dev_dbg() messages. I have only suggested removing dev_dbg() that print things that are not useful or redundantly duplicate what can be achieved with ftrace or tracepoints. Maybe that will remove the dev_dbg() messages and maybe it won't. That depends entirely what the function actually prints when executed. > but I noticed not only are there many "dev_dbg" messages, there > are also many such messages like (dev_info, dev_err, etc). Should I > remove them all too? The resulting patch will have your name on it rather than mine. That means it is you that must make the decisions here. Firstly you can review each message output to decide if it is useful. Only remove message whose output is not useful (same as for dev_dbg() ). If it is useful then you should think about whether the log level matches the importance of the message. For example, are the dev_err() message really covering error conditions? are there warnings for normal" conditions? is the dev_info() useful to someone who is not the driver author?). Daniel. > > > > > > > @@ -251,8 +251,8 @@ static long __ov2680_set_exposure(struct > > > > > v4l2_subdev *sd, int coarse_itg, > > > > > int ret, exp_val; > > > > > > > > > > dev_dbg(>dev, > > > > > - "+++__ov2680_set_exposure coarse_itg %d, gain %d, > > > > > digitgain %d++\n", > > > > > - coarse_itg, gain, digitgain); > > > > > + "+++%s coarse_itg %d, gain %d, digitgain %d++\n", > > > > > + __func__, coarse_itg, gain, digitgain); > > > > This case is a little less clear cut since the printed message does show > > some elements of internal state. However AFAICT this function just writes > > some state to the hardware so I still take the view that dynamic > > tools (I2C tracepoints for example) provide a better way to debug the > > driver. > > > > > > Daniel.
Re: [PATCH v3] MAINTAINERS: add Dan Murphy as TI LP8xxx drivers maintainer
On Tue, Nov 03, 2020 at 05:28:32PM +0100, Krzysztof Kozlowski wrote: > Milo Kim's email in TI bounces with permanent error (550: Invalid > recipient). Last email from him on LKML was in 2017. Move Milo Kim to > credits and add Dan Murphy from TI to look after: > - TI LP855x backlight driver, > - TI LP8727 charger driver, > - TI LP8788 MFD (ADC, LEDs, charger and regulator) drivers. > > Signed-off-by: Krzysztof Kozlowski > Cc: Dan Murphy > Acked-by: Dan Murphy > Acked-by: Jonathan Cameron > Acked-by: Sebastian Reichel > > --- > > Dear Lee, > > Could you take care about this patch? Just in case Lee wants it: Acked-by: Daniel Thompson Daniel.
Re: [PATCH] Replaced hard coded function names in debug messages with __func__ macro.
On Mon, Nov 02, 2020 at 01:15:56PM +0100, Tabot Kevin wrote: > On Mon, Nov 02, 2020 at 09:33:24AM +0000, Daniel Thompson wrote: > > On Sat, Oct 31, 2020 at 05:41:03PM +0100, Tabot Kevin wrote: > > > This patch fixes the following: > > > - Uses __func__ macro to print function names. > > > - Got rid of unnecessary braces around single line if statements. > > > - End of block comments on a seperate line. > > > - A spelling mistake of the word "on". > > > > > > Signed-off-by: Tabot Kevin > > > --- > > > drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 25 > > > +++--- > > > 1 file changed, 13 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c > > > b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c > > > index c907305..1396a33 100644 > > > --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c > > > +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c > > > @@ -146,7 +146,7 @@ static int ov2680_g_bin_factor_x(struct v4l2_subdev > > > *sd, s32 *val) > > > struct ov2680_device *dev = to_ov2680_sensor(sd); > > > struct i2c_client *client = v4l2_get_subdevdata(sd); > > > > > > - dev_dbg(>dev, "ov2680_g_bin_factor_x\n"); > > > + dev_dbg(>dev, "%s\n", __func__); > > > > It might be better just to remove this sort of message. > > > > They are not "wrong wrong" but are they actually useful one a > > driver's basic functions work? Even where they are useful > > dynamic techniques (ftrace, tracepoints, etc) arguably provide a > > better way to support "did my function actually run" debug > > approaches anyway. > > Thank you very much for the response. So, should I just revert back to > the original all the changes in places where I replace hard coded > functions names with __func__? [Responses on LKML should be quoted like this rather than top-posted] Personally I think it is better to remove them completely from the driver rather than revert to the original form. Naturally if Mauro or Sakari have strong views on this kind of printed message then you need to take that into account but, in general, messages like this add little or no value to the driver and can be removed. > > > @@ -251,8 +251,8 @@ static long __ov2680_set_exposure(struct v4l2_subdev > > > *sd, int coarse_itg, > > > int ret, exp_val; > > > > > > dev_dbg(>dev, > > > - "+++__ov2680_set_exposure coarse_itg %d, gain %d, digitgain > > > %d++\n", > > > - coarse_itg, gain, digitgain); > > > + "+++%s coarse_itg %d, gain %d, digitgain %d++\n", > > > + __func__, coarse_itg, gain, digitgain); This case is a little less clear cut since the printed message does show some elements of internal state. However AFAICT this function just writes some state to the hardware so I still take the view that dynamic tools (I2C tracepoints for example) provide a better way to debug the driver. Daniel.
Re: [PATCH] Replaced hard coded function names in debug messages with __func__ macro.
On Sat, Oct 31, 2020 at 05:41:03PM +0100, Tabot Kevin wrote:
> This patch fixes the following:
> - Uses __func__ macro to print function names.
> - Got rid of unnecessary braces around single line if statements.
> - End of block comments on a seperate line.
> - A spelling mistake of the word "on".
>
> Signed-off-by: Tabot Kevin
> ---
> drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 25
> +++---
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> index c907305..1396a33 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> @@ -146,7 +146,7 @@ static int ov2680_g_bin_factor_x(struct v4l2_subdev *sd,
> s32 *val)
> struct ov2680_device *dev = to_ov2680_sensor(sd);
> struct i2c_client *client = v4l2_get_subdevdata(sd);
>
> - dev_dbg(>dev, "ov2680_g_bin_factor_x\n");
> + dev_dbg(>dev, "%s\n", __func__);
It might be better just to remove this sort of message.
They are not "wrong wrong" but are they actually useful one a
driver's basic functions work? Even where they are useful
dynamic techniques (ftrace, tracepoints, etc) arguably provide a
better way to support "did my function actually run" debug
approaches anyway.
Daniel.
> *val = ov2680_res[dev->fmt_idx].bin_factor_x;
>
> return 0;
> @@ -158,7 +158,7 @@ static int ov2680_g_bin_factor_y(struct v4l2_subdev *sd,
> s32 *val)
> struct i2c_client *client = v4l2_get_subdevdata(sd);
>
> *val = ov2680_res[dev->fmt_idx].bin_factor_y;
> - dev_dbg(>dev, "ov2680_g_bin_factor_y\n");
> + dev_dbg(>dev, "%s\n", __func__);
> return 0;
> }
>
> @@ -173,7 +173,7 @@ static int ov2680_get_intg_factor(struct i2c_client
> *client,
> u16 reg_val;
> int ret;
>
> - dev_dbg(>dev, "ov2680_get_intg_factor\n");
> + dev_dbg(>dev, "%s\n", __func__);
> if (!info)
> return -EINVAL;
>
> @@ -251,8 +251,8 @@ static long __ov2680_set_exposure(struct v4l2_subdev *sd,
> int coarse_itg,
> int ret, exp_val;
>
> dev_dbg(>dev,
> - "+++__ov2680_set_exposure coarse_itg %d, gain %d, digitgain
> %d++\n",
> - coarse_itg, gain, digitgain);
> + "+++%s coarse_itg %d, gain %d, digitgain %d++\n",
> + __func__, coarse_itg, gain, digitgain);
>
> vts = ov2680_res[dev->fmt_idx].lines_per_frame;
>
> @@ -461,11 +461,11 @@ static int ov2680_v_flip(struct v4l2_subdev *sd, s32
> value)
> ret = ov2680_read_reg(client, 1, OV2680_FLIP_REG, );
> if (ret)
> return ret;
> - if (value) {
> + if (value)
> val |= OV2680_FLIP_MIRROR_BIT_ENABLE;
> - } else {
> + else
> val &= ~OV2680_FLIP_MIRROR_BIT_ENABLE;
> - }
> +
> ret = ov2680_write_reg(client, 1,
> OV2680_FLIP_REG, val);
> if (ret)
> @@ -731,7 +731,8 @@ static int gpio_ctrl(struct v4l2_subdev *sd, bool flag)
>* existing integrations often wire two (reset/power_down)
>* because that is the way other sensors work. There is no
>* way to tell how it is wired internally, so existing
> - * firmwares expose both and we drive them symmetrically. */
> + * firmwares expose both and we drive them symmetrically.
> + */
> if (flag) {
> ret = dev->platform_data->gpio0_ctrl(sd, 1);
> usleep_range(1, 15000);
> @@ -1060,9 +1061,9 @@ static int ov2680_s_stream(struct v4l2_subdev *sd, int
> enable)
>
> mutex_lock(>input_lock);
> if (enable)
> - dev_dbg(>dev, "ov2680_s_stream one\n");
> + dev_dbg(>dev, "%s on\n", __func__);
> else
> - dev_dbg(>dev, "ov2680_s_stream off\n");
> + dev_dbg(>dev, "%s off\n", __func__);
>
> ret = ov2680_write_reg(client, 1, OV2680_SW_STREAM,
> enable ? OV2680_START_STREAMING :
> @@ -1226,7 +1227,7 @@ static int ov2680_remove(struct i2c_client *client)
> struct v4l2_subdev *sd = i2c_get_clientdata(client);
> struct ov2680_device *dev = to_ov2680_sensor(sd);
>
> - dev_dbg(>dev, "ov2680_remove...\n");
> + dev_dbg(>dev, "%s...\n", __func__);
>
> dev->platform_data->csi_cfg(sd, 0);
>
> --
> 2.7.4
>
Re: [PATCH v6 6/7] kgdb: roundup: Allow runtime arch specific override
On Mon, Nov 02, 2020 at 11:48:53AM +0530, Sumit Garg wrote:
> On Thu, 29 Oct 2020 at 20:51, Daniel Thompson
> wrote:
> >
> > On Thu, Oct 29, 2020 at 08:26:26PM +0530, Sumit Garg wrote:
> > > Add a new API kgdb_arch_roundup_cpus() for a particular archichecture to
> > > override default kgdb roundup and if it detects at runtime to not support
> > > NMI roundup then it can fallback to default implementation using async
> > > SMP cross-calls.
> > >
> > > Currently such an architecture example is arm64 supporting pseudo NMIs
> > > feature which is only available on platforms which have support for GICv3
> > > or later version.
> > >
> > > Signed-off-by: Sumit Garg
> > > ---
> > > arch/powerpc/kernel/kgdb.c | 3 ++-
> > > arch/sparc/kernel/smp_64.c | 3 ++-
> > > arch/x86/kernel/kgdb.c | 6 --
> > > include/linux/kgdb.h | 5 +++--
> > > kernel/debug/debug_core.c | 10 +-
> > > 5 files changed, 20 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kernel/kgdb.c b/arch/powerpc/kernel/kgdb.c
> > > index 4090802..126575d 100644
> > > --- a/arch/powerpc/kernel/kgdb.c
> > > +++ b/arch/powerpc/kernel/kgdb.c
> > > @@ -125,9 +125,10 @@ static int kgdb_debugger_ipi(struct pt_regs *regs)
> > > }
> > >
> > > #ifdef CONFIG_SMP
> > > -void kgdb_roundup_cpus(void)
> > > +bool kgdb_arch_roundup_cpus(void)
> > > {
> > > smp_send_debugger_break();
> > > + return true;
> > > }
> > > #endif
> > >
> > > diff --git a/arch/sparc/kernel/smp_64.c b/arch/sparc/kernel/smp_64.c
> > > index e38d8bf..c459c83 100644
> > > --- a/arch/sparc/kernel/smp_64.c
> > > +++ b/arch/sparc/kernel/smp_64.c
> > > @@ -1014,9 +1014,10 @@ void flush_dcache_page_all(struct mm_struct *mm,
> > > struct page *page)
> > > }
> > >
> > > #ifdef CONFIG_KGDB
> > > -void kgdb_roundup_cpus(void)
> > > +bool kgdb_arch_roundup_cpus(void)
> > > {
> > > smp_cross_call(_kgdb_capture, 0, 0, 0);
> > > + return true;
> > > }
> > > #endif
> > >
> > > diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
> > > index ff7878d..1b756d9 100644
> > > --- a/arch/x86/kernel/kgdb.c
> > > +++ b/arch/x86/kernel/kgdb.c
> > > @@ -404,7 +404,8 @@ static void kgdb_disable_hw_debug(struct pt_regs
> > > *regs)
> > >
> > > #ifdef CONFIG_SMP
> > > /**
> > > - * kgdb_roundup_cpus - Get other CPUs into a holding pattern
> > > + * kgdb_arch_roundup_cpus - Get other CPUs into a holding pattern
> > > + *in an architectural specific manner
> > > *
> > > * On SMP systems, we need to get the attention of the other CPUs
> > > * and get them be in a known state. This should do what is needed
> > > @@ -414,9 +415,10 @@ static void kgdb_disable_hw_debug(struct pt_regs
> > > *regs)
> > > *
> > > * On non-SMP systems, this is not called.
> > > */
> > > -void kgdb_roundup_cpus(void)
> > > +bool kgdb_arch_roundup_cpus(void)
> > > {
> > > apic_send_IPI_allbutself(NMI_VECTOR);
> > > + return true;
> > > }
> > > #endif
> > >
> > > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> > > index 0d6cf64..f9db5b8 100644
> > > --- a/include/linux/kgdb.h
> > > +++ b/include/linux/kgdb.h
> > > @@ -200,7 +200,8 @@ kgdb_arch_handle_qxfer_pkt(char *remcom_in_buffer,
> > > extern void kgdb_call_nmi_hook(void *ignored);
> > >
> > > /**
> > > - * kgdb_roundup_cpus - Get other CPUs into a holding pattern
> > > + * kgdb_arch_roundup_cpus - Get other CPUs into a holding pattern
> > > + *in an architectural specific manner
> > > *
> > > * On SMP systems, we need to get the attention of the other CPUs
> > > * and get them into a known state. This should do what is needed
> > > @@ -210,7 +211,7 @@ extern void kgdb_call_nmi_hook(void *ignored);
> > > *
> > > * On non-SMP systems, this is not called.
> > > */
> > > -extern void kgdb_roundup_cpus(void);
> > > +extern bool kgdb_arch_roundup_cpus(void);
> > >
> > > /**
> > > * kgdb_arch_set_pc - Generic call back to the program counter
> > > diff
Re: [PATCH v6 7/7] arm64: kgdb: Roundup cpus using IPI as NMI
On Thu, Oct 29, 2020 at 04:22:34PM +, Daniel Thompson wrote:
> On Thu, Oct 29, 2020 at 08:26:27PM +0530, Sumit Garg wrote:
> > arm64 platforms with GICv3 or later supports pseudo NMIs which can be
> > leveraged to roundup CPUs which are stuck in hard lockup state with
> > interrupts disabled that wouldn't be possible with a normal IPI.
> >
> > So instead switch to roundup CPUs using IPI turned as NMI. And in
> > case a particular arm64 platform doesn't supports pseudo NMIs,
> > it will switch back to default kgdb CPUs roundup mechanism.
> >
> > Signed-off-by: Sumit Garg
> > ---
> > arch/arm64/include/asm/kgdb.h | 9 +
> > arch/arm64/kernel/ipi_nmi.c | 5 +
> > arch/arm64/kernel/kgdb.c | 35 +++
> > 3 files changed, 49 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
> > index 21fc85e..c3d2425 100644
> > --- a/arch/arm64/include/asm/kgdb.h
> > +++ b/arch/arm64/include/asm/kgdb.h
> > @@ -24,6 +24,15 @@ static inline void arch_kgdb_breakpoint(void)
> > extern void kgdb_handle_bus_error(void);
> > extern int kgdb_fault_expected;
> >
> > +#ifdef CONFIG_KGDB
> > +extern bool kgdb_ipi_nmicallback(int cpu, void *regs);
> > +#else
> > +static inline bool kgdb_ipi_nmicallback(int cpu, void *regs)
> > +{
> > + return false;
> > +}
> > +#endif
> > +
> > #endif /* !__ASSEMBLY__ */
> >
> > /*
> > diff --git a/arch/arm64/kernel/ipi_nmi.c b/arch/arm64/kernel/ipi_nmi.c
> > index 597dcf7..6ace182 100644
> > --- a/arch/arm64/kernel/ipi_nmi.c
> > +++ b/arch/arm64/kernel/ipi_nmi.c
> > @@ -8,6 +8,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -45,10 +46,14 @@ bool arch_trigger_cpumask_backtrace(const cpumask_t
> > *mask, bool exclude_self)
> > static irqreturn_t ipi_nmi_handler(int irq, void *data)
> > {
> > irqreturn_t ret = IRQ_NONE;
> > + unsigned int cpu = smp_processor_id();
> >
> > if (nmi_cpu_backtrace(get_irq_regs()))
> > ret = IRQ_HANDLED;
> >
> > + if (kgdb_ipi_nmicallback(cpu, get_irq_regs()))
> > + ret = IRQ_HANDLED;
> > +
> > return ret;
>
> It would be better to declare existing return value for
> kgdb_nmicallback() to be dangerously stupid and fix it so it returns an
> irqreturn_t (that's easy since most callers do not need to check the
> return value).
>
> Then this code simply becomes:
>
> return kgdb_nmicallback(cpu, get_irq_regs());
Actually, reflecting on this maybe it is better to keep kgdb_nmicallin()
and kgdb_nmicallback() aligned w.r.t. return codes (even if they are a
little unusual).
I'm still not sure why we'd keep kgdb_ipi_nmicallback() though.
kgdb_nmicallback() is intended to be called from arch code...
Daniel.
>
>
> > }
> >
> > diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> > index 1a157ca3..c26e710 100644
> > --- a/arch/arm64/kernel/kgdb.c
> > +++ b/arch/arm64/kernel/kgdb.c
> > @@ -17,6 +17,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> >
> > struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = {
> > @@ -353,3 +354,37 @@ int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
> > return aarch64_insn_write((void *)bpt->bpt_addr,
> > *(u32 *)bpt->saved_instr);
> > }
> > +
> > +bool kgdb_ipi_nmicallback(int cpu, void *regs)
> > +{
> > + if (atomic_read(_active) != -1) {
> > + kgdb_nmicallback(cpu, regs);
> > + return true;
> > + }
> > +
> > + return false;
> > +}
>
> I *really* don't like this function.
>
> If the return code of kgdb_nmicallback() is broken then fix it, don't
> just wrap it and invent a new criteria for the return code.
>
> To be honest I don't actually think the logic in kgdb_nmicallback() is
> broken. As mentioned above the return value has a weird definition (0
> for "handled it OK" and 1 for "nothing for me to do") but the logic to
> calculate the return code looks OK.
>
>
> > +
> > +static void kgdb_smp_callback(void *data)
> > +{
> > + unsigned int cpu = smp_processor_id();
> > +
> > + if (atomic_read(_active) != -1)
> > + kgdb_nmicallback(cpu, get_irq_regs());
> > +}
>
> This is Unused. I presume it is litter from a previous revision of the
> code and can be deleted?
>
>
> > +
> > +bool kgdb_arch_roundup_cpus(void)
> > +{
> > + struct cpumask mask;
> > +
> > + if (!arm64_supports_nmi())
> > + return false;
> > +
> > + cpumask_copy(, cpu_online_mask);
> > + cpumask_clear_cpu(raw_smp_processor_id(), );
> > + if (cpumask_empty())
> > + return false;
>
> Why do we need to fallback if there is no work to do? There will still
> be no work to do when we call the fallback.
>
>
> Daniel.
Re: [PATCH v6 7/7] arm64: kgdb: Roundup cpus using IPI as NMI
On Thu, Oct 29, 2020 at 08:26:27PM +0530, Sumit Garg wrote:
> arm64 platforms with GICv3 or later supports pseudo NMIs which can be
> leveraged to roundup CPUs which are stuck in hard lockup state with
> interrupts disabled that wouldn't be possible with a normal IPI.
>
> So instead switch to roundup CPUs using IPI turned as NMI. And in
> case a particular arm64 platform doesn't supports pseudo NMIs,
> it will switch back to default kgdb CPUs roundup mechanism.
>
> Signed-off-by: Sumit Garg
> ---
> arch/arm64/include/asm/kgdb.h | 9 +
> arch/arm64/kernel/ipi_nmi.c | 5 +
> arch/arm64/kernel/kgdb.c | 35 +++
> 3 files changed, 49 insertions(+)
>
> diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
> index 21fc85e..c3d2425 100644
> --- a/arch/arm64/include/asm/kgdb.h
> +++ b/arch/arm64/include/asm/kgdb.h
> @@ -24,6 +24,15 @@ static inline void arch_kgdb_breakpoint(void)
> extern void kgdb_handle_bus_error(void);
> extern int kgdb_fault_expected;
>
> +#ifdef CONFIG_KGDB
> +extern bool kgdb_ipi_nmicallback(int cpu, void *regs);
> +#else
> +static inline bool kgdb_ipi_nmicallback(int cpu, void *regs)
> +{
> + return false;
> +}
> +#endif
> +
> #endif /* !__ASSEMBLY__ */
>
> /*
> diff --git a/arch/arm64/kernel/ipi_nmi.c b/arch/arm64/kernel/ipi_nmi.c
> index 597dcf7..6ace182 100644
> --- a/arch/arm64/kernel/ipi_nmi.c
> +++ b/arch/arm64/kernel/ipi_nmi.c
> @@ -8,6 +8,7 @@
>
> #include
> #include
> +#include
> #include
> #include
>
> @@ -45,10 +46,14 @@ bool arch_trigger_cpumask_backtrace(const cpumask_t
> *mask, bool exclude_self)
> static irqreturn_t ipi_nmi_handler(int irq, void *data)
> {
> irqreturn_t ret = IRQ_NONE;
> + unsigned int cpu = smp_processor_id();
>
> if (nmi_cpu_backtrace(get_irq_regs()))
> ret = IRQ_HANDLED;
>
> + if (kgdb_ipi_nmicallback(cpu, get_irq_regs()))
> + ret = IRQ_HANDLED;
> +
> return ret;
It would be better to declare existing return value for
kgdb_nmicallback() to be dangerously stupid and fix it so it returns an
irqreturn_t (that's easy since most callers do not need to check the
return value).
Then this code simply becomes:
return kgdb_nmicallback(cpu, get_irq_regs());
> }
>
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index 1a157ca3..c26e710 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -17,6 +17,7 @@
>
> #include
> #include
> +#include
> #include
>
> struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = {
> @@ -353,3 +354,37 @@ int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
> return aarch64_insn_write((void *)bpt->bpt_addr,
> *(u32 *)bpt->saved_instr);
> }
> +
> +bool kgdb_ipi_nmicallback(int cpu, void *regs)
> +{
> + if (atomic_read(_active) != -1) {
> + kgdb_nmicallback(cpu, regs);
> + return true;
> + }
> +
> + return false;
> +}
I *really* don't like this function.
If the return code of kgdb_nmicallback() is broken then fix it, don't
just wrap it and invent a new criteria for the return code.
To be honest I don't actually think the logic in kgdb_nmicallback() is
broken. As mentioned above the return value has a weird definition (0
for "handled it OK" and 1 for "nothing for me to do") but the logic to
calculate the return code looks OK.
> +
> +static void kgdb_smp_callback(void *data)
> +{
> + unsigned int cpu = smp_processor_id();
> +
> + if (atomic_read(_active) != -1)
> + kgdb_nmicallback(cpu, get_irq_regs());
> +}
This is Unused. I presume it is litter from a previous revision of the
code and can be deleted?
> +
> +bool kgdb_arch_roundup_cpus(void)
> +{
> + struct cpumask mask;
> +
> + if (!arm64_supports_nmi())
> + return false;
> +
> + cpumask_copy(, cpu_online_mask);
> + cpumask_clear_cpu(raw_smp_processor_id(), );
> + if (cpumask_empty())
> + return false;
Why do we need to fallback if there is no work to do? There will still
be no work to do when we call the fallback.
Daniel.
Re: [PATCH v6 6/7] kgdb: roundup: Allow runtime arch specific override
On Thu, Oct 29, 2020 at 08:26:26PM +0530, Sumit Garg wrote:
> Add a new API kgdb_arch_roundup_cpus() for a particular archichecture to
> override default kgdb roundup and if it detects at runtime to not support
> NMI roundup then it can fallback to default implementation using async
> SMP cross-calls.
>
> Currently such an architecture example is arm64 supporting pseudo NMIs
> feature which is only available on platforms which have support for GICv3
> or later version.
>
> Signed-off-by: Sumit Garg
> ---
> arch/powerpc/kernel/kgdb.c | 3 ++-
> arch/sparc/kernel/smp_64.c | 3 ++-
> arch/x86/kernel/kgdb.c | 6 --
> include/linux/kgdb.h | 5 +++--
> kernel/debug/debug_core.c | 10 +-
> 5 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/kernel/kgdb.c b/arch/powerpc/kernel/kgdb.c
> index 4090802..126575d 100644
> --- a/arch/powerpc/kernel/kgdb.c
> +++ b/arch/powerpc/kernel/kgdb.c
> @@ -125,9 +125,10 @@ static int kgdb_debugger_ipi(struct pt_regs *regs)
> }
>
> #ifdef CONFIG_SMP
> -void kgdb_roundup_cpus(void)
> +bool kgdb_arch_roundup_cpus(void)
> {
> smp_send_debugger_break();
> + return true;
> }
> #endif
>
> diff --git a/arch/sparc/kernel/smp_64.c b/arch/sparc/kernel/smp_64.c
> index e38d8bf..c459c83 100644
> --- a/arch/sparc/kernel/smp_64.c
> +++ b/arch/sparc/kernel/smp_64.c
> @@ -1014,9 +1014,10 @@ void flush_dcache_page_all(struct mm_struct *mm,
> struct page *page)
> }
>
> #ifdef CONFIG_KGDB
> -void kgdb_roundup_cpus(void)
> +bool kgdb_arch_roundup_cpus(void)
> {
> smp_cross_call(_kgdb_capture, 0, 0, 0);
> + return true;
> }
> #endif
>
> diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
> index ff7878d..1b756d9 100644
> --- a/arch/x86/kernel/kgdb.c
> +++ b/arch/x86/kernel/kgdb.c
> @@ -404,7 +404,8 @@ static void kgdb_disable_hw_debug(struct pt_regs *regs)
>
> #ifdef CONFIG_SMP
> /**
> - * kgdb_roundup_cpus - Get other CPUs into a holding pattern
> + * kgdb_arch_roundup_cpus - Get other CPUs into a holding pattern
> + *in an architectural specific manner
> *
> * On SMP systems, we need to get the attention of the other CPUs
> * and get them be in a known state. This should do what is needed
> @@ -414,9 +415,10 @@ static void kgdb_disable_hw_debug(struct pt_regs *regs)
> *
> * On non-SMP systems, this is not called.
> */
> -void kgdb_roundup_cpus(void)
> +bool kgdb_arch_roundup_cpus(void)
> {
> apic_send_IPI_allbutself(NMI_VECTOR);
> + return true;
> }
> #endif
>
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> index 0d6cf64..f9db5b8 100644
> --- a/include/linux/kgdb.h
> +++ b/include/linux/kgdb.h
> @@ -200,7 +200,8 @@ kgdb_arch_handle_qxfer_pkt(char *remcom_in_buffer,
> extern void kgdb_call_nmi_hook(void *ignored);
>
> /**
> - * kgdb_roundup_cpus - Get other CPUs into a holding pattern
> + * kgdb_arch_roundup_cpus - Get other CPUs into a holding pattern
> + *in an architectural specific manner
> *
> * On SMP systems, we need to get the attention of the other CPUs
> * and get them into a known state. This should do what is needed
> @@ -210,7 +211,7 @@ extern void kgdb_call_nmi_hook(void *ignored);
> *
> * On non-SMP systems, this is not called.
> */
> -extern void kgdb_roundup_cpus(void);
> +extern bool kgdb_arch_roundup_cpus(void);
>
> /**
> * kgdb_arch_set_pc - Generic call back to the program counter
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index 1e75a89..27e401c 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -241,13 +241,21 @@ void __weak kgdb_call_nmi_hook(void *ignored)
> }
> NOKPROBE_SYMBOL(kgdb_call_nmi_hook);
>
> -void __weak kgdb_roundup_cpus(void)
> +bool __weak kgdb_arch_roundup_cpus(void)
> +{
> + return false;
Do we really need to introduce all these boolean return values just to
call a bit of library code when one of the architectures wants to
fallback to a generic implementation?
Why not something more like:
void kgdb_smp_call_nmi_hook(void)
{
/* original weak version of kgdb_roundup_cpus goes here */
}
void __weak kgdb_roundup_cpus(void)
{
kgdb_smp_call_nmi_hook();
}
arm64 can now simply call the new library function if a fallback is needed.
Note that same question applies to the backtrace changes...
Daniel.
> +}
> +
> +static void kgdb_roundup_cpus(void)
> {
> call_single_data_t *csd;
> int this_cpu = raw_smp_processor_id();
> int cpu;
> int ret;
>
> + if (kgdb_arch_roundup_cpus())
> + return;
> +
> for_each_online_cpu(cpu) {
> /* No need to roundup ourselves */
> if (cpu == this_cpu)
> --
> 2.7.4
>
Re: [PATCH v3 3/3] backlight: pwm_bl: Fix interpolation
On Wed, Oct 21, 2020 at 10:04:45PM -0700, Alexandru Stan wrote: > The previous behavior was a little unexpected, its properties/problems: > 1. It was designed to generate strictly increasing values (no repeats) > 2. It had quantization errors when calculating step size. Resulting in > unexpected jumps near the end of some segments. > > Example settings: > brightness-levels = <0 1 2 4 8 16 32 64 128 256>; > num-interpolated-steps = <16>; > > Whenever num-interpolated-steps was larger than the distance > between 2 consecutive brightness levels the table would get really > discontinuous. The slope of the interpolation would stick with > integers only and if it was 0 the whole line segment would get skipped. > > The distances between 1 2 4 and 8 would be 1 (property #1 fighting us), > and only starting with 16 it would start to interpolate properly. > > Property #1 is not enough. The goal here is more than just monotonically > increasing. We should still care about the shape of the curve. Repeated > points might be desired if we're in the part of the curve where we want > to go slow (aka slope near 0). > > Problem #2 is plainly a bug. Imagine if the 64 entry was 63 instead, > the calculated slope on the 32-63 segment will be almost half as it > should be. > > The most expected and simplest algorithm for interpolation is linear > interpolation, which would handle both problems. > Let's just implement that! > > Take pairs of points from the brightness-levels array and linearly > interpolate between them. On the X axis (what userspace sees) we'll > now have equally sized intervals (num-interpolated-steps sized, > as opposed to before where we were at the mercy of quantization). > > END INTERESTING. I guess this a copy 'n paste error from some internal log book? Better removed... but I won't lose sleep over it. > Signed-off-by: Alexandru Stan I've waited a bit to see how strong the feelings were w.r.t. getting rid of the division from the table initialization. It was something I was aware of during an earlier review but it was below my personal nitpicking threshold (which could be badly calibrated... hence waiting). However it's all been quiet so: Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH v3 1/3] ARM: dts: rockchip: veyron: Remove 0 point from brightness-levels
On Wed, Oct 21, 2020 at 10:04:43PM -0700, Alexandru Stan wrote:
> The extra 0 only adds one point in the userspace visible range,
> so this change is almost a noop with the current driver behavior.
>
> We don't need the 0% point, userspace seems to handle this just fine
> because it uses the bl_power property to turn off the display.
>
> Furthermore after adding "backlight: pwm_bl: Fix interpolation" patch,
> the backlight interpolation will work a little differently. So we need
> to preemptively remove the 0-3 segment since otherwise we would have a
> 252 long interpolation that would slowly go between 0 and 3, looking
> really bad in userspace. So it's almost a noop/cleanup now, but it will
> be required in the future.
>
> Signed-off-by: Alexandru Stan
Acked-by: Daniel Thompson
> ---
>
> arch/arm/boot/dts/rk3288-veyron-jaq.dts| 2 +-
> arch/arm/boot/dts/rk3288-veyron-minnie.dts | 2 +-
> arch/arm/boot/dts/rk3288-veyron-tiger.dts | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/boot/dts/rk3288-veyron-jaq.dts
> b/arch/arm/boot/dts/rk3288-veyron-jaq.dts
> index af77ab20586d..4a148cf1defc 100644
> --- a/arch/arm/boot/dts/rk3288-veyron-jaq.dts
> +++ b/arch/arm/boot/dts/rk3288-veyron-jaq.dts
> @@ -20,7 +20,7 @@ / {
>
> {
> /* Jaq panel PWM must be >= 3%, so start non-zero brightness at 8 */
> - brightness-levels = <0 8 255>;
> + brightness-levels = <8 255>;
> num-interpolated-steps = <247>;
> };
>
> diff --git a/arch/arm/boot/dts/rk3288-veyron-minnie.dts
> b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
> index f8b69e0a16a0..82fc6fba 100644
> --- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts
> +++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
> @@ -39,7 +39,7 @@ volum_up {
>
> {
> /* Minnie panel PWM must be >= 1%, so start non-zero brightness at 3 */
> - brightness-levels = <0 3 255>;
> + brightness-levels = <3 255>;
> num-interpolated-steps = <252>;
> };
>
> diff --git a/arch/arm/boot/dts/rk3288-veyron-tiger.dts
> b/arch/arm/boot/dts/rk3288-veyron-tiger.dts
> index 069f0c2c1fdf..52a84cbe7a90 100644
> --- a/arch/arm/boot/dts/rk3288-veyron-tiger.dts
> +++ b/arch/arm/boot/dts/rk3288-veyron-tiger.dts
> @@ -23,7 +23,7 @@ / {
>
> {
> /* Tiger panel PWM must be >= 1%, so start non-zero brightness at 3 */
> - brightness-levels = <0 3 255>;
> + brightness-levels = <3 255>;
> num-interpolated-steps = <252>;
> };
>
> --
> 2.28.0
Re: [PATCH v3 2/3] arm64: dts: qcom: trogdor: Add brightness-levels
On Wed, Oct 21, 2020 at 10:04:44PM -0700, Alexandru Stan wrote:
> We want userspace to represent the human perceived brightness.
> Since the led drivers and the leds themselves don't have a
> linear response to the value we give them in terms of perceived
> brightness, we'll bake the curve into the dts.
>
> The panel also doesn't have a good response under 5%, so we'll avoid
> sending it anything lower than that.
>
> Note: Ideally this patch should be coupled with the driver change from
> "backlight: pwm_bl: Fix interpolation", but it can work without it,
> without looking too ugly.
>
> Signed-off-by: Alexandru Stan
Acked-by: Daniel Thompson
> ---
>
> arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index bf875589d364..ccdabc6c4994 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -179,6 +179,15 @@ pp3300_fp_tp: pp3300-fp-tp-regulator {
> backlight: backlight {
> compatible = "pwm-backlight";
>
> + /* The panels don't seem to like anything below ~ 5% */
> + brightness-levels = <
> + 196 256 324 400 484 576 676 784 900 1024 1156 1296
> + 1444 1600 1764 1936 2116 2304 2500 2704 2916 3136
> + 3364 3600 3844 4096
> + >;
> + num-interpolated-steps = <64>;
> + default-brightness-level = <951>;
> +
> pwms = <_ec_pwm 1>;
> enable-gpios = < 12 GPIO_ACTIVE_HIGH>;
> power-supply = <_sys>;
> --
> 2.28.0
Re: drivers/video/backlight/ltv350qv.c:192:12: warning: stack frame size of 13472 bytes in function 'ltv350qv_power'
On Sun, Oct 25, 2020 at 12:17:08PM -0700, Andrew Morton wrote: > On Mon, 26 Oct 2020 02:15:37 +0800 kernel test robot wrote: > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > > master > > head: d76913908102044f14381df865bb74df17a538cb > > commit: cae9dc35ed9ff82a99754e51d57ff6c332e1f7e4 kasan: allow enabling > > stack tagging for tag-based mode > > date: 3 months ago > > config: arm64-randconfig-r005-20201026 (attached as .config) > > compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project > > 1c8371692dfe8245bc6690ff1262dcced4649d21) > > reproduce (this is a W=1 build): > > wget > > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O > > ~/bin/make.cross > > chmod +x ~/bin/make.cross > > # install arm64 cross compiling tool for clang build > > # apt-get install binutils-aarch64-linux-gnu > > # > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cae9dc35ed9ff82a99754e51d57ff6c332e1f7e4 > > git remote add linus > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > > git fetch --no-tags linus master > > git checkout cae9dc35ed9ff82a99754e51d57ff6c332e1f7e4 > > # save the attached .config to linux build tree > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross > > ARCH=arm64 > > > > If you fix the issue, kindly add following tag as appropriate > > Reported-by: kernel test robot > > > > All warnings (new ones prefixed by >>): > > > > >> drivers/video/backlight/ltv350qv.c:192:12: warning: stack frame size of > > >> 13472 bytes in function 'ltv350qv_power' [-Wframe-larger-than=] > > That's a lot of stack. > > >static int ltv350qv_power(struct ltv350qv *lcd, int power) > > ^ > >1 warning generated. > > > > vim +/ltv350qv_power +192 drivers/video/backlight/ltv350qv.c > > Odd - the code looks pretty normal. It is possible that your compiler > is (crazily) inlining ltv350qv_write_reg()? Certainly could be. Same config compiled with gcc-9 gives ltv350qv_write_reg() a stack usage that is large but not crazy: 768 bytes and dropping to 480 bytes with the sanitizers disabled. With the sanitizers enabled then even the cumulative stack usage of ltv350qv_power() through to ltv350qv_write_reg() is still less than 1k. Daniel.
Re: [PATCH AUTOSEL 5.4 31/80] kgdb: Make "kgdbcon" work properly with "kgdb_earlycon"
On Mon, Oct 26, 2020 at 07:54:27PM -0400, Sasha Levin wrote:
> From: Douglas Anderson
>
> [ Upstream commit b18b099e04f450cdc77bec72acefcde7042bd1f3 ]
>
> On my system the kernel processes the "kgdb_earlycon" parameter before
> the "kgdbcon" parameter. When we setup "kgdb_earlycon" we'll end up
> in kgdb_register_callbacks() and "kgdb_use_con" won't have been set
> yet so we'll never get around to starting "kgdbcon". Let's remedy
> this by detecting that the IO module was already registered when
> setting "kgdb_use_con" and registering the console then.
>
> As part of this, to avoid pre-declaring things, move the handling of
> the "kgdbcon" further down in the file.
>
> Signed-off-by: Douglas Anderson
> Link:
> https://lore.kernel.org/r/20200630151422.1.I4aa062751ff5e281f5116655c976dff545c09a46@changeid
> Signed-off-by: Daniel Thompson
> Signed-off-by: Sasha Levin
kgdb[oc]_earlycon was a new feature introduced in v5.8 so, based on the
summary above, this fix does not obviously apply to older kernels.
However after looking closely...
I think the issue described above would also occur if kgdbdbgp (an
incomprehensible sequence consonants that translates to "present
debugger via USB EHCI debug") were used in conjunction with kgdbcon
meaning backporting does make sense.
Daniel.
> ---
> kernel/debug/debug_core.c | 22 ++
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index f3225e53d..097ab02989f92 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -96,14 +96,6 @@ int dbg_switch_cpu;
> /* Use kdb or gdbserver mode */
> int dbg_kdb_mode = 1;
>
> -static int __init opt_kgdb_con(char *str)
> -{
> - kgdb_use_con = 1;
> - return 0;
> -}
> -
> -early_param("kgdbcon", opt_kgdb_con);
> -
> module_param(kgdb_use_con, int, 0644);
> module_param(kgdbreboot, int, 0644);
>
> @@ -876,6 +868,20 @@ static struct console kgdbcons = {
> .index = -1,
> };
>
> +static int __init opt_kgdb_con(char *str)
> +{
> + kgdb_use_con = 1;
> +
> + if (kgdb_io_module_registered && !kgdb_con_registered) {
> + register_console();
> + kgdb_con_registered = 1;
> + }
> +
> + return 0;
> +}
> +
> +early_param("kgdbcon", opt_kgdb_con);
> +
> #ifdef CONFIG_MAGIC_SYSRQ
> static void sysrq_handle_dbg(int key)
> {
> --
> 2.25.1
>
Re: [PATCH v5 1/5] arm64: Add framework to turn IPI as NMI
On Tue, Oct 20, 2020 at 04:52:43PM +0530, Sumit Garg wrote:
> On Tue, 20 Oct 2020 at 15:38, Marc Zyngier wrote:
> >
> > On 2020-10-20 07:43, Sumit Garg wrote:
> > > On Mon, 19 Oct 2020 at 17:07, Marc Zyngier wrote:
> >
> > [...]
> >
> > >> > +{
> > >> > + if (!ipi_desc)
> > >> > + return;
> > >> > +
> > >> > + if (is_nmi) {
> > >> > + if (!prepare_percpu_nmi(ipi_id))
> > >> > + enable_percpu_nmi(ipi_id, IRQ_TYPE_NONE);
> > >> > + } else {
> > >> > + enable_percpu_irq(ipi_id, IRQ_TYPE_NONE);
> > >>
> > >> I'm not keen on this. Normal IRQs can't reliably work, so why do you
> > >> even bother with this?
> > >
> > > Yeah I agree but we need to support existing functionality for kgdb
> > > roundup and sysrq backtrace using normal IRQs as well.
> >
> > When has this become a requirement? I don't really see the point in
> > implementing something that is known not to work.
> >
>
> For kgdb:
>
> Default implementation [1] uses smp_call_function_single_async() which
> in turn will invoke IPI as a normal IRQ to roundup CPUs.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/debug/debug_core.c#n244
>
> For sysrq backtrace:
>
> Default implementation [2] fallbacks to smp_call_function() (IPI as a
> normal IRQ) to print backtrace in case architecture doesn't provide
> arch_trigger_cpumask_backtrace() hook.
>
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/sysrq.c#n250
>
> So in general, IPI as a normal IRQ is still useful for debugging but
> it can't debug a core which is stuck in deadlock with interrupts
> disabled.
>
> And since we choose override default implementations for pseudo NMI
> support, we need to be backwards compatible for platforms which don't
> possess pseudo NMI support.
Do the fallback implementations require a new IPI? The fallbacks
could rely on existing mechanisms such as the smp_call_function
family.
Daniel.
[GIT PULL] kgdb changes for v5.10-rc1
The following changes since commit f75aef392f869018f78cfedf3c320a6b3fcfda6b: Linux 5.9-rc3 (2020-08-30 16:01:54 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/danielt/linux.git/ tags/kgdb-5.10-rc1 for you to fetch changes up to d081a6e353168f15e63eb9e9334757f20343319f: kdb: Fix pager search for multi-line strings (2020-10-01 14:44:08 +0100) kgdb patches for 5.10-rc1 A fairly modest set of changes for this cycle. Of particular note are an earlycon fix from Doug Anderson and my own changes to get kgdb/kdb to honour the kprobe blocklist. The later creates a safety rail that strongly encourages developers not to place breakpoints in, for example, arch specific trap handling code. Also included are a couple of small fixes and tweaks: an API update, eliminate a coverity dead code warning, improved handling of search during multi-line printk and a couple of typo corrections. Signed-off-by: Daniel Thompson Cengiz Can (1): kdb: remove unnecessary null check of dbg_io_ops Daniel Thompson (4): kgdb: Honour the kprobe blocklist when setting breakpoints kgdb: Add NOKPROBE labels on the trap handler functions kernel: debug: Centralize dbg_[de]activate_sw_breakpoints kdb: Fix pager search for multi-line strings Davidlohr Bueso (1): kdb: Use newer api for tasklist scanning Douglas Anderson (1): kgdb: Make "kgdbcon" work properly with "kgdb_earlycon" Youling Tang (1): kernel/debug: Fix spelling mistake in debug_core.c include/linux/kgdb.h| 18 kernel/debug/debug_core.c | 48 - kernel/debug/gdbstub.c | 5 ++--- kernel/debug/kdb/kdb_bp.c | 9 kernel/debug/kdb/kdb_bt.c | 4 ++-- kernel/debug/kdb/kdb_debugger.c | 2 -- kernel/debug/kdb/kdb_io.c | 22 +++ kernel/debug/kdb/kdb_main.c | 8 +++ kernel/debug/kdb/kdb_private.h | 4 lib/Kconfig.kgdb| 15 + 10 files changed, 101 insertions(+), 34 deletions(-)
Re: [PATCH v2 3/3] arm64: dts: qcom: trogdor: Add brightness-levels
On Wed, Oct 14, 2020 at 06:51:19AM -0700, Doug Anderson wrote:
> Hi,
>
> On Wed, Oct 14, 2020 at 4:33 AM Daniel Thompson
> wrote:
> >
> > On Tue, Oct 13, 2020 at 09:28:38AM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Tue, Oct 13, 2020 at 1:01 AM Alexandru Stan
> > > wrote:
> > > >
> > > > Now that we have better interpolation for the backlight
> > > > ("backlight: pwm_bl: Fix interpolation"), we can now add the curve to
> > > > the trogdor boards, being careful to crop the low end.
> > >
> > > Just to make it clear, the patch this depends on hasn't landed yet.
> > > Presumably it will land in the v5.10 timeframe? That means that
> > > without extra coordination this patch can target v5.11.
> >
> > You're talking about patch 1 from this set? Despite the title I view
> > the patch as changing policy (albeit one that does also fix some annoying
> > quantization errors at the same time) so it's not necessarily a
> > candidate for merging outside the merge window (I've not checked with
> > Lee but I think it likely the shutter is already down for features).
>
> Ugh, I'm off by one. :( Right. New features prepared for v5.10
> should already have been baking in linuxnext and merge requests have
> already started flowing towards Linus. After v5.10-rc1 then it'd just
> fixes and this doesn't really qualify. So the timing dictates that
> patch #1 will land in v5.11, not v5.10.
>
>
> > Moreover I'm not clear why there a dependency here that would stop the
> > changes landing in different trees.
>
> I haven't tried Alexandru's device tree patch without the associated
> code changes, but I guess I just assumed that it would make a really
> ugly (non)ideal backlight curve and we'd be better off with what we
> currently have (AKA no curve specified at all).
>
> Hrm, thinking about it, I guess the worst case is a slightly non-ideal
> backlight curve and it would be all good in the final v5.11 which
> would have the device tree and code changes, so you're right that both
> the code and device tree could target v5.11 without anything too
> bad...
Excellent. I'll try to remember this for v3 so I can get my Acked-by
versus Reviewed-by tags correct ;-) .
Daniel.
>
>
> > Daniel.
> >
> >
> > > > Signed-off-by: Alexandru Stan
> > > > ---
> > > >
> > > > arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 9 +
> > > > 1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > > > b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > > > index bf875589d364..ccdabc6c4994 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > > > @@ -179,6 +179,15 @@ pp3300_fp_tp: pp3300-fp-tp-regulator {
> > > > backlight: backlight {
> > > > compatible = "pwm-backlight";
> > > >
> > > > + /* The panels don't seem to like anything below ~ 5% */
> > > > + brightness-levels = <
> > > > + 196 256 324 400 484 576 676 784 900 1024 1156
> > > > 1296
> > > > + 1444 1600 1764 1936 2116 2304 2500 2704 2916
> > > > 3136
> > > > + 3364 3600 3844 4096
> > > > + >;
> > > > + num-interpolated-steps = <64>;
> > > > + default-brightness-level = <951>;
> > >
> > > I haven't done lots of digging here, but this matches what Alexandru
> > > and Matthias agreed upon for the downstream tree and seems sane.
> > > Thus:
> > >
> > > Reviewed-by: Douglas Anderson
Re: [PATCH v2 2/3] ARM: dts: rockchip: veyron: Remove 0 point from brightness-levels
On Tue, Oct 13, 2020 at 01:01:02AM -0700, Alexandru Stan wrote:
> After the "PWM backlight interpolation adjustments" patches, the
> backlight interpolation works a little differently. The way these
> dts files were working before was relying on a bug (IMHO).
>
> Remove the 0-3 range since otherwise we would have a 252 long
> interpolation that would slowly go between 0 and 3, looking really bad
> in userspace.
>
> We don't need the 0% point, userspace seems to handle this just fine
> because it uses the bl_power property to turn off the display.
>
> Signed-off-by: Alexandru Stan
Acked-by: Daniel Thompson
Note also shouldn't this be patch 1 of the set. AFAICT it makes sense
whether or not the interpolation algorithm is changed.
Daniel.
> ---
>
> arch/arm/boot/dts/rk3288-veyron-jaq.dts| 2 +-
> arch/arm/boot/dts/rk3288-veyron-minnie.dts | 2 +-
> arch/arm/boot/dts/rk3288-veyron-tiger.dts | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/boot/dts/rk3288-veyron-jaq.dts
> b/arch/arm/boot/dts/rk3288-veyron-jaq.dts
> index af77ab20586d..4a148cf1defc 100644
> --- a/arch/arm/boot/dts/rk3288-veyron-jaq.dts
> +++ b/arch/arm/boot/dts/rk3288-veyron-jaq.dts
> @@ -20,7 +20,7 @@ / {
>
> {
> /* Jaq panel PWM must be >= 3%, so start non-zero brightness at 8 */
> - brightness-levels = <0 8 255>;
> + brightness-levels = <8 255>;
> num-interpolated-steps = <247>;
> };
>
> diff --git a/arch/arm/boot/dts/rk3288-veyron-minnie.dts
> b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
> index f8b69e0a16a0..82fc6fba 100644
> --- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts
> +++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
> @@ -39,7 +39,7 @@ volum_up {
>
> {
> /* Minnie panel PWM must be >= 1%, so start non-zero brightness at 3 */
> - brightness-levels = <0 3 255>;
> + brightness-levels = <3 255>;
> num-interpolated-steps = <252>;
> };
>
> diff --git a/arch/arm/boot/dts/rk3288-veyron-tiger.dts
> b/arch/arm/boot/dts/rk3288-veyron-tiger.dts
> index 069f0c2c1fdf..52a84cbe7a90 100644
> --- a/arch/arm/boot/dts/rk3288-veyron-tiger.dts
> +++ b/arch/arm/boot/dts/rk3288-veyron-tiger.dts
> @@ -23,7 +23,7 @@ / {
>
> {
> /* Tiger panel PWM must be >= 1%, so start non-zero brightness at 3 */
> - brightness-levels = <0 3 255>;
> + brightness-levels = <3 255>;
> num-interpolated-steps = <252>;
> };
>
> --
> 2.28.0
>
Re: [PATCH v2 3/3] arm64: dts: qcom: trogdor: Add brightness-levels
On Tue, Oct 13, 2020 at 09:28:38AM -0700, Doug Anderson wrote:
> Hi,
>
> On Tue, Oct 13, 2020 at 1:01 AM Alexandru Stan wrote:
> >
> > Now that we have better interpolation for the backlight
> > ("backlight: pwm_bl: Fix interpolation"), we can now add the curve to
> > the trogdor boards, being careful to crop the low end.
>
> Just to make it clear, the patch this depends on hasn't landed yet.
> Presumably it will land in the v5.10 timeframe? That means that
> without extra coordination this patch can target v5.11.
You're talking about patch 1 from this set? Despite the title I view
the patch as changing policy (albeit one that does also fix some annoying
quantization errors at the same time) so it's not necessarily a
candidate for merging outside the merge window (I've not checked with
Lee but I think it likely the shutter is already down for features).
Moreover I'm not clear why there a dependency here that would stop the
changes landing in different trees.
Daniel.
> > Signed-off-by: Alexandru Stan
> > ---
> >
> > arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 9 +
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > index bf875589d364..ccdabc6c4994 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > @@ -179,6 +179,15 @@ pp3300_fp_tp: pp3300-fp-tp-regulator {
> > backlight: backlight {
> > compatible = "pwm-backlight";
> >
> > + /* The panels don't seem to like anything below ~ 5% */
> > + brightness-levels = <
> > + 196 256 324 400 484 576 676 784 900 1024 1156 1296
> > + 1444 1600 1764 1936 2116 2304 2500 2704 2916 3136
> > + 3364 3600 3844 4096
> > + >;
> > + num-interpolated-steps = <64>;
> > + default-brightness-level = <951>;
>
> I haven't done lots of digging here, but this matches what Alexandru
> and Matthias agreed upon for the downstream tree and seems sane.
> Thus:
>
> Reviewed-by: Douglas Anderson
Re: [PATCH v2 1/3] backlight: pwm_bl: Fix interpolation
On Tue, Oct 13, 2020 at 01:01:01AM -0700, Alexandru Stan wrote:
> Whenever num-interpolated-steps was larger than the distance
> between 2 consecutive brightness levels the table would get really
> discontinuous. The slope of the interpolation would stick with
> integers only and if it was 0 the whole line segment would get skipped.
>
> Example settings:
> brightness-levels = <0 1 2 4 8 16 32 64 128 256>;
> num-interpolated-steps = <16>;
>
> The distances between 1 2 4 and 8 would be 1, and only starting with 16
> it would start to interpolate properly.
Both comments a perilously close to nitpicking but enough that I wanted
to reply...
I'd suggest that the current behaviour as having two properties.
1. It was designed to generate strictly increasing tables (no repeated
values).
2. It's implementation contains quantization errors when calculating the
step size. This results in both the discards of some interpolated
steps you mentioned (it is possible to insert extra steps between 4
and 8 whilst retaining a strictly increasing table). It also
results in a potentially large undershoot when multiplying a step
size (64 interpolated steps and a gap of 127 is likely to get a visual
jump as we hop through 63 physical steps in one go).
#1 can is a policy that can be changed. #2 is a bug that could be fixed.
To be clear I don't object to generating a monotonically increasing
table but I'd prefer the policy change to be explicitly described in
the description.
> Let's change it so there's always interpolation happening, even if
> there's no enough points available (read: values in the table would
> appear more than once). This should match the expected behavior much
> more closely.
>
> Signed-off-by: Alexandru Stan
> ---
>
> drivers/video/backlight/pwm_bl.c | 70 ++--
> 1 file changed, 31 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/video/backlight/pwm_bl.c
> b/drivers/video/backlight/pwm_bl.c
> index dfc760830eb9..3e77f6b73fd9 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -230,8 +230,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
> struct platform_pwm_backlight_data *data)
> {
> struct device_node *node = dev->of_node;
> - unsigned int num_levels = 0;
> - unsigned int levels_count;
> + unsigned int num_levels;
> unsigned int num_steps = 0;
> struct property *prop;
> unsigned int *table;
> @@ -260,12 +259,11 @@ static int pwm_backlight_parse_dt(struct device *dev,
> if (!prop)
> return 0;
>
> - data->max_brightness = length / sizeof(u32);
> + num_levels = length / sizeof(u32);
>
> /* read brightness levels from DT property */
> - if (data->max_brightness > 0) {
> - size_t size = sizeof(*data->levels) * data->max_brightness;
> - unsigned int i, j, n = 0;
> + if (num_levels > 0) {
> + size_t size = sizeof(*data->levels) * num_levels;
>
> data->levels = devm_kzalloc(dev, size, GFP_KERNEL);
> if (!data->levels)
> @@ -273,7 +271,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
>
> ret = of_property_read_u32_array(node, "brightness-levels",
>data->levels,
> - data->max_brightness);
> + num_levels);
> if (ret < 0)
> return ret;
>
> @@ -298,7 +296,13 @@ static int pwm_backlight_parse_dt(struct device *dev,
>* between two points.
>*/
> if (num_steps) {
> - if (data->max_brightness < 2) {
> + unsigned int num_input_levels = num_levels;
> + unsigned int i;
> + u32 x1, x2, x, dx;
> + u32 y1, y2;
> + s64 dy;
> +
> + if (num_input_levels < 2) {
> dev_err(dev, "can't interpolate\n");
> return -EINVAL;
> }
> @@ -308,14 +312,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
>* taking in consideration the number of interpolated
>* steps between two levels.
>*/
> - for (i = 0; i < data->max_brightness - 1; i++) {
> - if ((data->levels[i + 1] - data->levels[i]) /
> -num_steps)
> - num_levels += num_steps;
> - else
> - num_levels++;
> - }
> - num_levels++;
> + num_levels = (num_input_levels - 1) * num_steps + 1;
>
Re: [PATCH v4 41/52] docs: kgdb.rst: fix :c:type: usages
On Wed, Sep 30, 2020 at 03:25:04PM +0200, Mauro Carvalho Chehab wrote: > Which Sphinx 3, :c:type: can't be used anymore for structs, > as this should be used only for typedefs. > > Rely on automarkup.py for struct references. > > This file has an special case, though: it uses the tag also > to point to an array. Let's use, instead, :c:expr: for such > purpose, as it should do the right thing. > > This should fix this warning: > > ./Documentation/dev-tools/kgdb.rst:875: WARNING: Unparseable C > cross-reference: 'kdb_poll_funcs[]' > Invalid C declaration: Expected end of definition. [error at 14] > kdb_poll_funcs[] > --^ > > Signed-off-by: Mauro Carvalho Chehab Acked-by: Daniel Thompson Daniel. > --- > Documentation/dev-tools/kgdb.rst | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/Documentation/dev-tools/kgdb.rst > b/Documentation/dev-tools/kgdb.rst > index c908ef4d3f04..77b688e6a254 100644 > --- a/Documentation/dev-tools/kgdb.rst > +++ b/Documentation/dev-tools/kgdb.rst > @@ -726,7 +726,7 @@ The kernel debugger is organized into a number of > components: > - contains an arch-specific trap catcher which invokes >kgdb_handle_exception() to start kgdb about doing its work > > - - translation to and from gdb specific packet format to :c:type:`pt_regs` > + - translation to and from gdb specific packet format to struct pt_regs > > - Registration and unregistration of architecture specific trap >hooks > @@ -846,7 +846,7 @@ invokes a callback in the serial core which in turn uses > the callback in > the UART driver. > > When using kgdboc with a UART, the UART driver must implement two > -callbacks in the :c:type:`struct uart_ops `. > +callbacks in the struct uart_ops. > Example from ``drivers/8250.c``:: > > > @@ -875,7 +875,7 @@ kernel when ``CONFIG_KDB_KEYBOARD=y`` is set in the > kernel configuration. > The core polled keyboard driver for PS/2 type keyboards is in > ``drivers/char/kdb_keyboard.c``. This driver is hooked into the debug core > when kgdboc populates the callback in the array called > -:c:type:`kdb_poll_funcs[]`. The kdb_get_kbd_char() is the top-level > +:c:expr:`kdb_poll_funcs[]`. The kdb_get_kbd_char() is the top-level > function which polls hardware for single character input. > > kgdboc and kms > -- > 2.26.2 >
Re: [PATCH v3 0/3] kgdb: Honour the kprobe blocklist when setting breakpoints
On Sun, Sep 27, 2020 at 10:15:28PM +0100, Daniel Thompson wrote:
> kgdb has traditionally adopted a no safety rails approach to breakpoint
> placement. If the debugger is commanded to place a breakpoint at an
> address then it will do so even if that breakpoint results in kgdb
> becoming inoperable.
>
> A stop-the-world debugger with memory peek/poke intrinsically provides
> its operator with the means to hose their system in all manner of
> exciting ways (not least because stopping-the-world is already a DoS
> attack ;-) ). Nevertheless the current no safety rail approach is
> difficult to defend, especially given kprobes can provide us with plenty
> of machinery to mark the parts of the kernel where breakpointing is
> discouraged.
>
> This patchset introduces some safety rails by using the existing kprobes
> infrastructure and ensures this will be enabled by default on
> architectures that implement kprobes. At present it does not cover
> absolutely all locations where breakpoints can cause trouble but it will
> block off several avenues, including the architecture specific parts
> that are handled by arch_within_kprobe_blacklist().
>
> v4:
> * Fixed KConfig dependencies for HONOUR_KPROBE_BLOCKLIST on kernels
> where MODULES=n
> * Add additional debug_core.c functions to the blocklist (thanks Doug)
> * Collected a few tags
Looks like I neglected to bump the version number in the subject.
For the avoidance of doubt, this comment is correct and the subject
line is broken.
Sorry!
Daniel.
>
> v3:
> * Dropped the single step blocklist checks. It is not proven that the
> code was actually reachable without triggering the catastrophic
> failure flag (which inhibits resume already).
> * Update patch description for ("kgdb: Add NOKPROBE labels...") and
> added symbols that are called during trap exit
> * Added a new patch to push the breakpoint activation later in the
> flow and ensure the I/O functions are not called with breakpoints
> activated.
>
> v2:
> * Reworked after initial RFC to make honouring the blocklist require
> CONFIG_KPROBES. It is now optional but the blocklist will be enabled
> by default for architectures that CONFIG_HAVE_KPROBES
>
> Daniel Thompson (3):
> kgdb: Honour the kprobe blocklist when setting breakpoints
> kgdb: Add NOKPROBE labels on the trap handler functions
> kernel: debug: Centralize dbg_[de]activate_sw_breakpoints
>
> include/linux/kgdb.h| 18 ++
> kernel/debug/debug_core.c | 22 ++
> kernel/debug/gdbstub.c | 1 -
> kernel/debug/kdb/kdb_bp.c | 9 +
> kernel/debug/kdb/kdb_debugger.c | 2 --
> lib/Kconfig.kgdb| 15 +++
> 6 files changed, 64 insertions(+), 3 deletions(-)
>
> --
> 2.25.4
>
[PATCH v3 1/3] kgdb: Honour the kprobe blocklist when setting breakpoints
Currently kgdb has absolutely no safety rails in place to discourage or
prevent a user from placing a breakpoint in dangerous places such as
the debugger's own trap entry/exit and other places where it is not safe
to take synchronous traps.
Introduce a new config symbol KGDB_HONOUR_BLOCKLIST and modify the
default implementation of kgdb_validate_break_address() so that we use
the kprobe blocklist to prohibit instrumentation of critical functions
if the config symbol is set. The config symbol dependencies are set to
ensure that the blocklist will be enabled by default if we enable KGDB
and are compiling for an architecture where we HAVE_KPROBES.
Suggested-by: Peter Zijlstra
Reviewed-by: Douglas Anderson
Reviewed-by: Masami Hiramatsu
Signed-off-by: Daniel Thompson
---
include/linux/kgdb.h | 18 ++
kernel/debug/debug_core.c | 4
kernel/debug/kdb/kdb_bp.c | 9 +
lib/Kconfig.kgdb | 15 +++
4 files changed, 46 insertions(+)
diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
index 477b8b7c908f..0d6cf64c8bb1 100644
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
#ifdef CONFIG_HAVE_ARCH_KGDB
#include
#endif
@@ -335,6 +336,23 @@ extern int kgdb_nmicallin(int cpu, int trapnr, void *regs,
int err_code,
atomic_t *snd_rdy);
extern void gdbstub_exit(int status);
+/*
+ * kgdb and kprobes both use the same (kprobe) blocklist (which makes sense
+ * given they are both typically hooked up to the same trap meaning on most
+ * architectures one cannot be used to debug the other)
+ *
+ * However on architectures where kprobes is not (yet) implemented we permit
+ * breakpoints everywhere rather than blocking everything by default.
+ */
+static inline bool kgdb_within_blocklist(unsigned long addr)
+{
+#ifdef CONFIG_KGDB_HONOUR_BLOCKLIST
+ return within_kprobe_blacklist(addr);
+#else
+ return false;
+#endif
+}
+
extern int kgdb_single_step;
extern atomic_tkgdb_active;
#define in_dbg_master() \
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index b16dbc1bf056..b1277728a835 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -188,6 +188,10 @@ int __weak kgdb_validate_break_address(unsigned long addr)
{
struct kgdb_bkpt tmp;
int err;
+
+ if (kgdb_within_blocklist(addr))
+ return -EINVAL;
+
/* Validate setting the breakpoint and then removing it. If the
* remove fails, the kernel needs to emit a bad message because we
* are deep trouble not being able to put things back the way we
diff --git a/kernel/debug/kdb/kdb_bp.c b/kernel/debug/kdb/kdb_bp.c
index d7ebb2c79cb8..ec4940146612 100644
--- a/kernel/debug/kdb/kdb_bp.c
+++ b/kernel/debug/kdb/kdb_bp.c
@@ -306,6 +306,15 @@ static int kdb_bp(int argc, const char **argv)
if (!template.bp_addr)
return KDB_BADINT;
+ /*
+* This check is redundant (since the breakpoint machinery should
+* be doing the same check during kdb_bp_install) but gives the
+* user immediate feedback.
+*/
+ diag = kgdb_validate_break_address(template.bp_addr);
+ if (diag)
+ return diag;
+
/*
* Find an empty bp structure to allocate
*/
diff --git a/lib/Kconfig.kgdb b/lib/Kconfig.kgdb
index 256f2486f9bd..05dae05b6cc9 100644
--- a/lib/Kconfig.kgdb
+++ b/lib/Kconfig.kgdb
@@ -24,6 +24,21 @@ menuconfig KGDB
if KGDB
+config KGDB_HONOUR_BLOCKLIST
+ bool "KGDB: use kprobe blocklist to prohibit unsafe breakpoints"
+ depends on HAVE_KPROBES
+ depends on MODULES
+ select KPROBES
+ default y
+ help
+ If set to Y the debug core will use the kprobe blocklist to
+ identify symbols where it is unsafe to set breakpoints.
+ In particular this disallows instrumentation of functions
+ called during debug trap handling and thus makes it very
+ difficult to inadvertently provoke recursive trap handling.
+
+ If unsure, say Y.
+
config KGDB_SERIAL_CONSOLE
tristate "KGDB: use kgdb over the serial console"
select CONSOLE_POLL
--
2.25.4
[PATCH v3 2/3] kgdb: Add NOKPROBE labels on the trap handler functions
Currently kgdb honours the kprobe blocklist but doesn't place its own
trap handling code on the list. Add labels to discourage attempting to
use kgdb to debug itself.
Not every functions that executes from the trap handler needs to be
marked up: relatively early in the trap handler execution (just after
we bring the other CPUs to a halt) all breakpoints are replaced with
the original opcodes. This patch marks up code in the debug_core that
executes between trap entry and the breakpoints being deactivated
and, also, code that executes between breakpoint activation and trap
exit.
To be clear these changes are not sufficient to make recursive trapping
impossible since cover all the library calls made during kgdb's
entry/exit logic. However going much further whilst we are sharing the
kprobe blocklist risks reducing the capabilities of kprobe and this
would be a bad trade off (especially so given kgdb's users are currently
conditioned to avoid recursive traps).
Signed-off-by: Daniel Thompson
---
kernel/debug/debug_core.c | 16
1 file changed, 16 insertions(+)
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index b1277728a835..faa1f99ce65a 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -177,12 +177,14 @@ int __weak kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
arch_kgdb_ops.gdb_bpt_instr, BREAK_INSTR_SIZE);
return err;
}
+NOKPROBE_SYMBOL(kgdb_arch_set_breakpoint);
int __weak kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
{
return copy_to_kernel_nofault((char *)bpt->bpt_addr,
(char *)bpt->saved_instr, BREAK_INSTR_SIZE);
}
+NOKPROBE_SYMBOL(kgdb_arch_remove_breakpoint);
int __weak kgdb_validate_break_address(unsigned long addr)
{
@@ -212,6 +214,7 @@ unsigned long __weak kgdb_arch_pc(int exception, struct
pt_regs *regs)
{
return instruction_pointer(regs);
}
+NOKPROBE_SYMBOL(kgdb_arch_pc);
int __weak kgdb_arch_init(void)
{
@@ -222,6 +225,7 @@ int __weak kgdb_skipexception(int exception, struct pt_regs
*regs)
{
return 0;
}
+NOKPROBE_SYMBOL(kgdb_skipexception);
#ifdef CONFIG_SMP
@@ -243,6 +247,7 @@ void __weak kgdb_call_nmi_hook(void *ignored)
*/
kgdb_nmicallback(raw_smp_processor_id(), get_irq_regs());
}
+NOKPROBE_SYMBOL(kgdb_call_nmi_hook);
void __weak kgdb_roundup_cpus(void)
{
@@ -276,6 +281,7 @@ void __weak kgdb_roundup_cpus(void)
kgdb_info[cpu].rounding_up = false;
}
}
+NOKPROBE_SYMBOL(kgdb_roundup_cpus);
#endif
@@ -302,6 +308,7 @@ static void kgdb_flush_swbreak_addr(unsigned long addr)
/* Force flush instruction cache if it was outside the mm */
flush_icache_range(addr, addr + BREAK_INSTR_SIZE);
}
+NOKPROBE_SYMBOL(kgdb_flush_swbreak_addr);
/*
* SW breakpoint management:
@@ -329,6 +336,7 @@ int dbg_activate_sw_breakpoints(void)
}
return ret;
}
+NOKPROBE_SYMBOL(dbg_activate_sw_breakpoints);
int dbg_set_sw_break(unsigned long addr)
{
@@ -392,6 +400,7 @@ int dbg_deactivate_sw_breakpoints(void)
}
return ret;
}
+NOKPROBE_SYMBOL(dbg_deactivate_sw_breakpoints);
int dbg_remove_sw_break(unsigned long addr)
{
@@ -513,6 +522,7 @@ static int kgdb_io_ready(int print_wait)
}
return 1;
}
+NOKPROBE_SYMBOL(kgdb_io_ready);
static int kgdb_reenter_check(struct kgdb_state *ks)
{
@@ -560,6 +570,7 @@ static int kgdb_reenter_check(struct kgdb_state *ks)
return 1;
}
+NOKPROBE_SYMBOL(kgdb_reenter_check);
static void dbg_touch_watchdogs(void)
{
@@ -567,6 +578,7 @@ static void dbg_touch_watchdogs(void)
clocksource_touch_watchdog();
rcu_cpu_stall_reset();
}
+NOKPROBE_SYMBOL(dbg_touch_watchdogs);
static int kgdb_cpu_enter(struct kgdb_state *ks, struct pt_regs *regs,
int exception_state)
@@ -798,6 +810,7 @@ static int kgdb_cpu_enter(struct kgdb_state *ks, struct
pt_regs *regs,
return kgdb_info[cpu].ret_state;
}
+NOKPROBE_SYMBOL(kgdb_cpu_enter);
/*
* kgdb_handle_exception() - main entry point from a kernel exception
@@ -842,6 +855,7 @@ kgdb_handle_exception(int evector, int signo, int ecode,
struct pt_regs *regs)
arch_kgdb_ops.enable_nmi(1);
return ret;
}
+NOKPROBE_SYMBOL(kgdb_handle_exception);
/*
* GDB places a breakpoint at this function to know dynamically loaded objects.
@@ -876,6 +890,7 @@ int kgdb_nmicallback(int cpu, void *regs)
#endif
return 1;
}
+NOKPROBE_SYMBOL(kgdb_nmicallback);
int kgdb_nmicallin(int cpu, int trapnr, void *regs, int err_code,
atomic_t *send_ready)
@@ -901,6 +916,7 @@ int kgdb_nmicallin(int cpu, int trapnr, void *regs, int
err_code,
#endif
return 1;
}
+NOKPROBE_SYMBOL(kgdb_nmicallin);
static void kgdb_console_write(struct console *co, const char *s,
unsigned count)
--
2.25.4
[PATCH v3 0/3] kgdb: Honour the kprobe blocklist when setting breakpoints
kgdb has traditionally adopted a no safety rails approach to breakpoint
placement. If the debugger is commanded to place a breakpoint at an
address then it will do so even if that breakpoint results in kgdb
becoming inoperable.
A stop-the-world debugger with memory peek/poke intrinsically provides
its operator with the means to hose their system in all manner of
exciting ways (not least because stopping-the-world is already a DoS
attack ;-) ). Nevertheless the current no safety rail approach is
difficult to defend, especially given kprobes can provide us with plenty
of machinery to mark the parts of the kernel where breakpointing is
discouraged.
This patchset introduces some safety rails by using the existing kprobes
infrastructure and ensures this will be enabled by default on
architectures that implement kprobes. At present it does not cover
absolutely all locations where breakpoints can cause trouble but it will
block off several avenues, including the architecture specific parts
that are handled by arch_within_kprobe_blacklist().
v4:
* Fixed KConfig dependencies for HONOUR_KPROBE_BLOCKLIST on kernels
where MODULES=n
* Add additional debug_core.c functions to the blocklist (thanks Doug)
* Collected a few tags
v3:
* Dropped the single step blocklist checks. It is not proven that the
code was actually reachable without triggering the catastrophic
failure flag (which inhibits resume already).
* Update patch description for ("kgdb: Add NOKPROBE labels...") and
added symbols that are called during trap exit
* Added a new patch to push the breakpoint activation later in the
flow and ensure the I/O functions are not called with breakpoints
activated.
v2:
* Reworked after initial RFC to make honouring the blocklist require
CONFIG_KPROBES. It is now optional but the blocklist will be enabled
by default for architectures that CONFIG_HAVE_KPROBES
Daniel Thompson (3):
kgdb: Honour the kprobe blocklist when setting breakpoints
kgdb: Add NOKPROBE labels on the trap handler functions
kernel: debug: Centralize dbg_[de]activate_sw_breakpoints
include/linux/kgdb.h| 18 ++
kernel/debug/debug_core.c | 22 ++
kernel/debug/gdbstub.c | 1 -
kernel/debug/kdb/kdb_bp.c | 9 +
kernel/debug/kdb/kdb_debugger.c | 2 --
lib/Kconfig.kgdb| 15 +++
6 files changed, 64 insertions(+), 3 deletions(-)
--
2.25.4
