[PATCH 0/4] Remove #ifdef CONFIG_* from uapi headers (2023 edition)

[PATCH 0/4] Remove #ifdef CONFIG_* from uapi headers (2023 edition)

[Thomas Huth]

uapi headers should not use the kernel-internal CONFIG switches.
Palmer Dabbelt sent some patches to clean this up a couple of years
ago, but unfortunately some of those patches never got merged.
So here's a rebased version of those patches - since they are rather
trivial, I hope it's OK for everybody if they could go through Arnd's
"generic include/asm header files" branch?

Palmer Dabbelt (4):
  Move COMPAT_ATM_ADDPARTY to net/atm/svc.c
  Move ep_take_care_of_epollwakeup() to fs/eventpoll.c
  Move bp_type_idx to include/linux/hw_breakpoint.h
  Move USE_WCACHING to drivers/block/pktcdvd.c

 drivers/block/pktcdvd.c                  | 11 +++++++++++
 fs/eventpoll.c                           | 13 +++++++++++++
 include/linux/hw_breakpoint.h            | 10 ++++++++++
 include/uapi/linux/atmdev.h              |  4 ----
 include/uapi/linux/eventpoll.h           | 12 ------------
 include/uapi/linux/hw_breakpoint.h       | 10 ----------
 include/uapi/linux/pktcdvd.h             | 11 -----------
 net/atm/svc.c                            |  5 +++++
 tools/include/uapi/linux/hw_breakpoint.h | 10 ----------
 9 files changed, 39 insertions(+), 47 deletions(-)

-- 
2.31.1

[PATCH 1/4] Move COMPAT_ATM_ADDPARTY to net/atm/svc.c

[Thomas Huth]

From: Palmer Dabbelt <palmer@dabbelt.com>

This used to be behind an #ifdef COMPAT_COMPAT, so most of userspace
wouldn't have seen the definition before.  Unfortunately this header
file became visible to userspace, so the definition has instead been
moved to net/atm/svc.c (the only user).

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
Reviewed-by: Andrew Waterman <waterman@eecs.berkeley.edu>
Reviewed-by: Albert Ou <aou@eecs.berkeley.edu>
Message-Id: <1447119071-19392-4-git-send-email-palmer@dabbelt.com>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/uapi/linux/atmdev.h | 4 ----
 net/atm/svc.c               | 5 +++++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/atmdev.h b/include/uapi/linux/atmdev.h
index a5c15cf23bd7..20b0215084fc 100644
--- a/include/uapi/linux/atmdev.h
+++ b/include/uapi/linux/atmdev.h
@@ -101,10 +101,6 @@ struct atm_dev_stats {
 					/* use backend to make new if */
 #define ATM_ADDPARTY  	_IOW('a', ATMIOC_SPECIAL+4,struct atm_iobuf)
  					/* add party to p2mp call */
-#ifdef CONFIG_COMPAT
-/* It actually takes struct sockaddr_atmsvc, not struct atm_iobuf */
-#define COMPAT_ATM_ADDPARTY  	_IOW('a', ATMIOC_SPECIAL+4,struct compat_atm_iobuf)
-#endif
 #define ATM_DROPPARTY 	_IOW('a', ATMIOC_SPECIAL+5,int)
 					/* drop party from p2mp call */
 
diff --git a/net/atm/svc.c b/net/atm/svc.c
index 4a02bcaad279..d83556d8beb9 100644
--- a/net/atm/svc.c
+++ b/net/atm/svc.c
@@ -28,6 +28,11 @@
 #include "signaling.h"
 #include "addr.h"
 
+#ifdef CONFIG_COMPAT
+/* It actually takes struct sockaddr_atmsvc, not struct atm_iobuf */
+#define COMPAT_ATM_ADDPARTY _IOW('a', ATMIOC_SPECIAL + 4, struct compat_atm_iobuf)
+#endif
+
 static int svc_create(struct net *net, struct socket *sock, int protocol,
 		      int kern);
 
-- 
2.31.1

Re: [PATCH 1/4] Move COMPAT_ATM_ADDPARTY to net/atm/svc.c

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/4] Move ep_take_care_of_epollwakeup() to fs/eventpoll.c

[Thomas Huth]

From: Palmer Dabbelt <palmer@dabbelt.com>

This doesn't make any sense to expose to userspace, so it's been moved
to the one user.  This was introduced by commit 95f19f658ce1 ("epoll:
drop EPOLLWAKEUP if PM_SLEEP is disabled").

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
Reviewed-by: Andrew Waterman <waterman@eecs.berkeley.edu>
Reviewed-by: Albert Ou <aou@eecs.berkeley.edu>
Message-Id: <1447119071-19392-7-git-send-email-palmer@dabbelt.com>
[thuth: Rebased to fix contextual conflicts]
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 fs/eventpoll.c                 | 13 +++++++++++++
 include/uapi/linux/eventpoll.h | 12 ------------
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 64659b110973..e2a5d2cc9051 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -2042,6 +2042,19 @@ SYSCALL_DEFINE1(epoll_create, int, size)
 	return do_epoll_create(0);
 }
 
+#ifdef CONFIG_PM_SLEEP
+static inline void ep_take_care_of_epollwakeup(struct epoll_event *epev)
+{
+	if ((epev->events & EPOLLWAKEUP) && !capable(CAP_BLOCK_SUSPEND))
+		epev->events &= ~EPOLLWAKEUP;
+}
+#else
+static inline void ep_take_care_of_epollwakeup(struct epoll_event *epev)
+{
+	epev->events &= ~EPOLLWAKEUP;
+}
+#endif
+
 static inline int epoll_mutex_lock(struct mutex *mutex, int depth,
 				   bool nonblock)
 {
diff --git a/include/uapi/linux/eventpoll.h b/include/uapi/linux/eventpoll.h
index e687658843b1..cfbcc4cc49ac 100644
--- a/include/uapi/linux/eventpoll.h
+++ b/include/uapi/linux/eventpoll.h
@@ -85,16 +85,4 @@ struct epoll_event {
 	__u64 data;
 } EPOLL_PACKED;
 
-#ifdef CONFIG_PM_SLEEP
-static inline void ep_take_care_of_epollwakeup(struct epoll_event *epev)
-{
-	if ((epev->events & EPOLLWAKEUP) && !capable(CAP_BLOCK_SUSPEND))
-		epev->events &= ~EPOLLWAKEUP;
-}
-#else
-static inline void ep_take_care_of_epollwakeup(struct epoll_event *epev)
-{
-	epev->events &= ~EPOLLWAKEUP;
-}
-#endif
 #endif /* _UAPI_LINUX_EVENTPOLL_H */
-- 
2.31.1

Re: [PATCH 2/4] Move ep_take_care_of_epollwakeup() to fs/eventpoll.c

[Arnd Bergmann]

On Fri, Feb 17, 2023, at 21:22, Thomas Huth wrote:
> From: Palmer Dabbelt <palmer@dabbelt.com>
>
> This doesn't make any sense to expose to userspace, so it's been moved
> to the one user.  This was introduced by commit 95f19f658ce1 ("epoll:
> drop EPOLLWAKEUP if PM_SLEEP is disabled").
>
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> Reviewed-by: Andrew Waterman <waterman@eecs.berkeley.edu>
> Reviewed-by: Albert Ou <aou@eecs.berkeley.edu>
> Message-Id: <1447119071-19392-7-git-send-email-palmer@dabbelt.com>
> [thuth: Rebased to fix contextual conflicts]
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 2/4] Move ep_take_care_of_epollwakeup() to fs/eventpoll.c

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 3/4] Move bp_type_idx to include/linux/hw_breakpoint.h

[Thomas Huth]

From: Palmer Dabbelt <palmer@dabbelt.com>

This has a "#ifdef CONFIG_*" that used to be exposed to userspace.

The names in here are so generic that I don't think it's a good idea
to expose them to userspace (or even the rest of the kernel).  There are
multiple in-kernel users, so it's been moved to a kernel header file.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
Reviewed-by: Andrew Waterman <waterman@eecs.berkeley.edu>
Reviewed-by: Albert Ou <aou@eecs.berkeley.edu>
Message-Id: <1447119071-19392-10-git-send-email-palmer@dabbelt.com>
[thuth: Remove it also from tools/include/uapi/linux/hw_breakpoint.h]
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/linux/hw_breakpoint.h            | 10 ++++++++++
 include/uapi/linux/hw_breakpoint.h       | 10 ----------
 tools/include/uapi/linux/hw_breakpoint.h | 10 ----------
 3 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/include/linux/hw_breakpoint.h b/include/linux/hw_breakpoint.h
index f319bd26b030..7fbb45911273 100644
--- a/include/linux/hw_breakpoint.h
+++ b/include/linux/hw_breakpoint.h
@@ -7,6 +7,16 @@
 
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 
+enum bp_type_idx {
+	TYPE_INST	= 0,
+#if defined(CONFIG_HAVE_MIXED_BREAKPOINTS_REGS)
+	TYPE_DATA	= 0,
+#else
+	TYPE_DATA	= 1,
+#endif
+	TYPE_MAX
+};
+
 extern int __init init_hw_breakpoint(void);
 
 static inline void hw_breakpoint_init(struct perf_event_attr *attr)
diff --git a/include/uapi/linux/hw_breakpoint.h b/include/uapi/linux/hw_breakpoint.h
index 965e4d8606d8..1575d3ca6f0d 100644
--- a/include/uapi/linux/hw_breakpoint.h
+++ b/include/uapi/linux/hw_breakpoint.h
@@ -22,14 +22,4 @@ enum {
 	HW_BREAKPOINT_INVALID   = HW_BREAKPOINT_RW | HW_BREAKPOINT_X,
 };
 
-enum bp_type_idx {
-	TYPE_INST 	= 0,
-#ifdef CONFIG_HAVE_MIXED_BREAKPOINTS_REGS
-	TYPE_DATA	= 0,
-#else
-	TYPE_DATA	= 1,
-#endif
-	TYPE_MAX
-};
-
 #endif /* _UAPI_LINUX_HW_BREAKPOINT_H */
diff --git a/tools/include/uapi/linux/hw_breakpoint.h b/tools/include/uapi/linux/hw_breakpoint.h
index 965e4d8606d8..1575d3ca6f0d 100644
--- a/tools/include/uapi/linux/hw_breakpoint.h
+++ b/tools/include/uapi/linux/hw_breakpoint.h
@@ -22,14 +22,4 @@ enum {
 	HW_BREAKPOINT_INVALID   = HW_BREAKPOINT_RW | HW_BREAKPOINT_X,
 };
 
-enum bp_type_idx {
-	TYPE_INST 	= 0,
-#ifdef CONFIG_HAVE_MIXED_BREAKPOINTS_REGS
-	TYPE_DATA	= 0,
-#else
-	TYPE_DATA	= 1,
-#endif
-	TYPE_MAX
-};
-
 #endif /* _UAPI_LINUX_HW_BREAKPOINT_H */
-- 
2.31.1

Re: [PATCH 3/4] Move bp_type_idx to include/linux/hw_breakpoint.h

[Arnd Bergmann]

On Fri, Feb 17, 2023, at 21:23, Thomas Huth wrote:
> From: Palmer Dabbelt <palmer@dabbelt.com>
>
> This has a "#ifdef CONFIG_*" that used to be exposed to userspace.
>
> The names in here are so generic that I don't think it's a good idea
> to expose them to userspace (or even the rest of the kernel).  There are
> multiple in-kernel users, so it's been moved to a kernel header file.
>
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> Reviewed-by: Andrew Waterman <waterman@eecs.berkeley.edu>
> Reviewed-by: Albert Ou <aou@eecs.berkeley.edu>
> Message-Id: <1447119071-19392-10-git-send-email-palmer@dabbelt.com>
> [thuth: Remove it also from tools/include/uapi/linux/hw_breakpoint.h]
> Signed-off-by: Thomas Huth <thuth@redhat.com>

It took me a while to understand this code enough to be confident this
is the right solution. Note that CONFIG_HAVE_MIXED_BREAKPOINTS_REGS is
purely dependent on the architecture and could be replaced with something
that checks for x86||sh but it should be safe to assume that the
enum should never have been part of the uapi header.

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

      Arnd

Re: [PATCH 3/4] Move bp_type_idx to include/linux/hw_breakpoint.h

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 4/4] Move USE_WCACHING to drivers/block/pktcdvd.c

[Thomas Huth]

From: Palmer Dabbelt <palmer@dabbelt.com>

I don't think this was ever intended to be exposed to userspace, but
it did require an "#ifdef CONFIG_*".  Since the name is kind of
generic and was only used in one place, I've moved the definition to
the one user.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
Reviewed-by: Andrew Waterman <waterman@eecs.berkeley.edu>
Reviewed-by: Albert Ou <aou@eecs.berkeley.edu>
Message-Id: <1447119071-19392-11-git-send-email-palmer@dabbelt.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 drivers/block/pktcdvd.c      | 11 +++++++++++
 include/uapi/linux/pktcdvd.h | 11 -----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 2f1a92509271..1d5ec8d9d1a5 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -90,6 +90,17 @@ do {									\
 
 #define MAX_SPEED 0xffff
 
+/*
+ * use drive write caching -- we need deferred error handling to be
+ * able to successfully recover with this option (drive will return good
+ * status as soon as the cdb is validated).
+ */
+#if defined(CONFIG_CDROM_PKTCDVD_WCACHE)
+#define USE_WCACHING		1
+#else
+#define USE_WCACHING		0
+#endif
+
 static DEFINE_MUTEX(pktcdvd_mutex);
 static struct pktcdvd_device *pkt_devs[MAX_WRITERS];
 static struct proc_dir_entry *pkt_proc;
diff --git a/include/uapi/linux/pktcdvd.h b/include/uapi/linux/pktcdvd.h
index 9cbb55d21c94..6a5552dfd6af 100644
--- a/include/uapi/linux/pktcdvd.h
+++ b/include/uapi/linux/pktcdvd.h
@@ -29,17 +29,6 @@
  */
 #define PACKET_WAIT_TIME	(HZ * 5 / 1000)
 
-/*
- * use drive write caching -- we need deferred error handling to be
- * able to successfully recover with this option (drive will return good
- * status as soon as the cdb is validated).
- */
-#if defined(CONFIG_CDROM_PKTCDVD_WCACHE)
-#define USE_WCACHING		1
-#else
-#define USE_WCACHING		0
-#endif
-
 /*
  * No user-servicable parts beyond this point ->
  */
-- 
2.31.1

Re: [PATCH 4/4] Move USE_WCACHING to drivers/block/pktcdvd.c

[Arnd Bergmann]

On Fri, Feb 17, 2023, at 21:23, Thomas Huth wrote:
> From: Palmer Dabbelt <palmer@dabbelt.com>
>
> I don't think this was ever intended to be exposed to userspace, but
> it did require an "#ifdef CONFIG_*".  Since the name is kind of
> generic and was only used in one place, I've moved the definition to
> the one user.
>
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> Reviewed-by: Andrew Waterman <waterman@eecs.berkeley.edu>
> Reviewed-by: Albert Ou <aou@eecs.berkeley.edu>
> Message-Id: <1447119071-19392-11-git-send-email-palmer@dabbelt.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  drivers/block/pktcdvd.c      | 11 +++++++++++
>  include/uapi/linux/pktcdvd.h | 11 -----------
>  2 files changed, 11 insertions(+), 11 deletions(-)

I'm fairly sure there are more bits in uapi/linux/pktcdvd.h that should
be in drivers/block/pktcdvd.c instead, along with all of
include/linux/pktcdvd.h, but this change is obvious and safe by itself,
so 

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 4/4] Move USE_WCACHING to drivers/block/pktcdvd.c

[Christoph Hellwig]

On Fri, Feb 17, 2023 at 09:23:01PM +0100, Thomas Huth wrote:
> From: Palmer Dabbelt <palmer@dabbelt.com>
> 
> I don't think this was ever intended to be exposed to userspace, but
> it did require an "#ifdef CONFIG_*".  Since the name is kind of
> generic and was only used in one place, I've moved the definition to
> the one user.

I'd just remove USE_WCACHING entirel with something like:

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 2f1a92509271c4..5ae2a80db2c341 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -1869,12 +1869,12 @@ static noinline_for_stack int pkt_probe_settings(struct pktcdvd_device *pd)
 /*
  * enable/disable write caching on drive
  */
-static noinline_for_stack int pkt_write_caching(struct pktcdvd_device *pd,
-						int set)
+static noinline_for_stack int pkt_write_caching(struct pktcdvd_device *pd)
 {
 	struct packet_command cgc;
 	struct scsi_sense_hdr sshdr;
 	unsigned char buf[64];
+	bool set = IS_ENABLED(CONFIG_CDROM_PKTCDVD_WCACHE);
 	int ret;
 
 	init_cdrom_command(&cgc, buf, sizeof(buf), CGC_DATA_READ);
@@ -1890,7 +1890,12 @@ static noinline_for_stack int pkt_write_caching(struct pktcdvd_device *pd,
 	if (ret)
 		return ret;
 
-	buf[pd->mode_offset + 10] |= (!!set << 2);
+	/*
+	 * use drive write caching -- we need deferred error handling to be
+	 * able to successfully recover with this option (drive will return good
+	 * status as soon as the cdb is validated).
+	 */
+	buf[pd->mode_offset + 10] |= (set << 2);
 
 	cgc.buflen = cgc.cmd[8] = 2 + ((buf[0] << 8) | (buf[1] & 0xff));
 	ret = pkt_mode_select(pd, &cgc);
@@ -2085,7 +2090,7 @@ static int pkt_open_write(struct pktcdvd_device *pd)
 		return -EIO;
 	}
 
-	pkt_write_caching(pd, USE_WCACHING);
+	pkt_write_caching(pd);
 
 	ret = pkt_get_max_speed(pd, &write_speed);
 	if (ret)
diff --git a/include/uapi/linux/pktcdvd.h b/include/uapi/linux/pktcdvd.h
index 9cbb55d21c94af..6a5552dfd6af4e 100644
--- a/include/uapi/linux/pktcdvd.h
+++ b/include/uapi/linux/pktcdvd.h
@@ -29,17 +29,6 @@
  */
 #define PACKET_WAIT_TIME	(HZ * 5 / 1000)
 
-/*
- * use drive write caching -- we need deferred error handling to be
- * able to successfully recover with this option (drive will return good
- * status as soon as the cdb is validated).
- */
-#if defined(CONFIG_CDROM_PKTCDVD_WCACHE)
-#define USE_WCACHING		1
-#else
-#define USE_WCACHING		0
-#endif
-
 /*
  * No user-servicable parts beyond this point ->
  */

Re: [PATCH 0/4] Remove #ifdef CONFIG_* from uapi headers (2023 edition)

[Arnd Bergmann]

On Fri, Feb 17, 2023, at 21:22, Thomas Huth wrote:
> uapi headers should not use the kernel-internal CONFIG switches.
> Palmer Dabbelt sent some patches to clean this up a couple of years
> ago, but unfortunately some of those patches never got merged.
> So here's a rebased version of those patches - since they are rather
> trivial, I hope it's OK for everybody if they could go through Arnd's
> "generic include/asm header files" branch?

I just sent the pull request for the merge window an will be out
of office for most of next week. The patches all look good and
I provided a Reviewed-by: tag in case someone else wants to pick
up some or all of them. Otherwise I can apply them when I get
back.

    Arnd