diff mbox series

kgdb: Remove kgdb_schedule_breakpoint()

Message ID 20210210142525.2876648-1-daniel.thompson@linaro.org
State Accepted
Commit f11e2bc682cc197e33bfd118178cadb61326dc0e
Headers show
Series kgdb: Remove kgdb_schedule_breakpoint() | expand

Commit Message

Daniel Thompson Feb. 10, 2021, 2:25 p.m. UTC 
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 <daniel.thompson@linaro.org>

---

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(-)


base-commit: 19c329f6808995b142b3966301f217c831e7cf31
prerequisite-patch-id: 6d9085a2ef51882c80a4f13264cd12a14dcceb54
prerequisite-patch-id: c0a2cb664281d00a6e32867896374a617aafb358
prerequisite-patch-id: 6bbcef7ce98747090ecb13fd3eda74a241e47249
prerequisite-patch-id: 8bf7c51993c06ff88809d49ed59cbace3d94604e
--
2.29.2

Comments

Doug Anderson Feb. 10, 2021, 3:57 p.m. UTC | #1 
Hi,

On Wed, Feb 10, 2021 at 6:25 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>

> 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 <daniel.thompson@linaro.org>

> ---

>

> 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(-)


Reviewed-by: Douglas Anderson <dianders@chromium.org>
Davidlohr Bueso Feb. 10, 2021, 7:47 p.m. UTC | #2 
On Wed, 10 Feb 2021, Daniel Thompson wrote:

>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.


I'm happy to see another user gone, I missed that it was even an option
to remove this altogether. Yeah so in general I started sending random
patches to get rid of some tasklets after seeing the recent extentions
in 12cc923f1cc (tasklet: Introduce new initialization API), which is
really the wrong way to go imo. Some driver maintainers/authors push
back in the name of performance (albeit tasklets provide no guarantees
because of ksoftirqd, for example), some don't care as much. There are
also constantly new users being added (despite the explicit deprecation
of the api) defering through tasklets, which makes me wonder if the tasklet
removal is anything but a pipe dream.

Acked-by: Davidlohr Bueso <dbueso@suse.de>

>

>Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

>---

>

>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.


Thanks,
Davidlohr
Jason Wessel Feb. 10, 2021, 9:57 p.m. UTC | #3 
On 2/10/21 8:25 AM, Daniel Thompson wrote:
> 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.



There was another out of tree user of this, but I don't know if
this is still applicable today.  The scenario is around the
ability to use a character sequence when kgdboc is active
on the console such as <control-c>, to cause a break point, vs
using a hardware break over a tty (because not all hardware supported this).

I could send the original patch that implements this along, but
I question if it is needed given the devices out there.  The reason
the original patch existed at all was to deal with some pick serial
hardware.

---- original out of tree patch header ----

Subject: [PATCH] kgdboc, tty: Add the rx polling call back capability

The idea is to allow kgdboc to intercept a <contorol-c> or any other
character of preference to cause breakpoint interrupt which will start
the kgdb interface running on the controlling terminal where the
character was typed.

The default behavior of kgdboc changes such that the control-c will
always generate an entry to kgdb unless the "n" option is used in the
kgdb configuration line. IE: kgdboc=ttyS0,n,115200

In order to make use of the new API, a low level serial driver must
check to see if it should execute the callback function for each
character that it processes.  This is similar to the approach used
with the NET_POLL API's rx_hook.

The only changes to the tty layer introduced by this patch are:
   * Add poll_rx_cb() call back for the low level driver
   * Move the poll_init() into kgdboc and out of tty_find_polling_driv()
   * change poll_init() to accept the rx callback parameter
---
  Documentation/DocBook/kgdb.tmpl  |   46 ++++++++++++++++++++++---
  drivers/tty/serial/kgdboc.c      |   70 ++++++++++++++++++++++++++++++++++++++-
  drivers/tty/serial/serial_core.c |   23 ++++++++++++
  drivers/tty/tty_io.c             |    9 +----
  include/linux/serial_core.h      |    3 +
  include/linux/tty_driver.h       |    3 +
  6 files changed, 139 insertions(+), 15 deletions(-)


-------------------------------------------



> 

> 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.


Because it has no in tree users, it absolutely makes the most
sense to purge this function.

Acked-by: Jason Wessel <jason.wessel@windriver.com>


> 

> 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 <daniel.thompson@linaro.org>

> ---

> 

> 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_t			masters_in_kgdb;

>   static atomic_t			slaves_in_kgdb;

> -static atomic_t			kgdb_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(&kgdb_break_tasklet_var, 0);

> -}

> -

> -static DECLARE_TASKLET_OLD(kgdb_tasklet_breakpoint, kgdb_tasklet_bpt);

> -

> -void kgdb_schedule_breakpoint(void)

> -{

> -	if (atomic_read(&kgdb_break_tasklet_var) ||

> -		atomic_read(&kgdb_active) != -1 ||

> -		atomic_read(&kgdb_setting_breakpoint))

> -		return;

> -	atomic_inc(&kgdb_break_tasklet_var);

> -	tasklet_schedule(&kgdb_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

>
diff mbox series

Patch

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_t			masters_in_kgdb;
 static atomic_t			slaves_in_kgdb;
-static atomic_t			kgdb_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(&kgdb_break_tasklet_var, 0);
-}
-
-static DECLARE_TASKLET_OLD(kgdb_tasklet_breakpoint, kgdb_tasklet_bpt);
-
-void kgdb_schedule_breakpoint(void)
-{
-	if (atomic_read(&kgdb_break_tasklet_var) ||
-		atomic_read(&kgdb_active) != -1 ||
-		atomic_read(&kgdb_setting_breakpoint))
-		return;
-	atomic_inc(&kgdb_break_tasklet_var);
-	tasklet_schedule(&kgdb_tasklet_breakpoint);
-}
-EXPORT_SYMBOL_GPL(kgdb_schedule_breakpoint);
-
 /**
  *	kgdb_register_io_module - register KGDB IO module
  *	@new_dbg_io_ops: the io ops vector