diff mbox

[v5,0/1] Fix CONFIG_USB=y && CONFIG_MODULES not set

Message ID 20221201211630.101541-1-allenwebb@google.com
State New
Headers show

Commit Message

Allen Webb Dec. 1, 2022, 9:16 p.m. UTC 
# Patch Review Discussion

Luis Chamberlain <mcgrof@kernel.org> would prefer something generic and
mentioned kmod already has builtin.alias.bin.

Something generic would be preferable, but I don't see a path to a
generic solution without handling the nuances of each subsystem. The
problem I am trying to solve is to expose the match id's for builtin
kernel modules. The aliases associated with match ids are not included
in builtin.alias.bin, so work would be needed to handle them (ideally
files2alias.c would be used since it already handles each subsystem).
I have an experimental patch (https://crrev.com/c/3840672) that
generates buildin.alias at kernel build time which I could upload
separately if that is preferred. Note that approach would involve
adding a new file to the kernel packaging requirements.

I have some concern that builtin.alias.bin might be used in cases where
the match-id-based aliases are not desired. There are much more
match-id-based aliases than regular aliases.

One advantage of the sysfs approach is there wouldn't be a reliance on
the buildin.alias file being in sync with the running kernel. The
output of sysfs would reflect the match ids for the currently running
kernel and modules.

# Patch change history

I have included incremental diffs to make the between patch-version
changes clear.

## Changes made in v5 - this version

* Fix a build issue when CONFIG_MODULES is not set, but CONFIG_USB=y.
* Add this cover letter



Allen Webb (1):
  modules: add modalias file to sysfs for modules.

 drivers/base/Makefile          |   2 +-
 drivers/base/base.h            |   8 +
 drivers/base/bus.c             |  42 ++++++
 drivers/base/mod_devicetable.c | 257 +++++++++++++++++++++++++++++++++
 drivers/usb/core/driver.c      |   2 +
 include/linux/device/bus.h     |   8 +
 include/linux/module.h         |   1 +
 kernel/module/internal.h       |   2 +
 kernel/module/sysfs.c          |  88 +++++++++++
 kernel/params.c                |   7 +
 10 files changed, 416 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/mod_devicetable.c

Comments

Greg Kroah-Hartman Dec. 2, 2022, 12:46 p.m. UTC | #1 
On Thu, Dec 01, 2022 at 03:16:29PM -0600, Allen Webb wrote:
> # Patch Review Discussion
> 
> Luis Chamberlain <mcgrof@kernel.org> would prefer something generic and
> mentioned kmod already has builtin.alias.bin.

Wait, how can 0/1 have a patch in it?

Take some time and read the mailing list for hundreds of examples of how
people submit patch series and how they document the changes in them and
why they should be accepted.  I think you are making this much harder
than it has to be.

even more confused,

greg k-h
diff mbox

Patch

diff --git a/drivers/base/base.h b/drivers/base/base.h
index beaa252c04388..fec56271104fa 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -173,7 +173,7 @@  static inline void module_add_driver(struct module *mod,
 static inline void module_remove_driver(struct device_driver *drv) { }
 #endif

-#if defined(CONFIG_SYSFS)
+#if defined(CONFIG_SYSFS) && defined(CONFIG_MODULES)
 ssize_t usb_drv_to_modalias(struct device_driver *drv, char *buf,
 			    size_t count);
 #else


## Changes made in v4

* Fix some issues reported by checkpatch.pl
* Add an explanitory comment for the ADD macro
* Fix a build issue when CONFIG_MODULES is not set.

diff --git a/drivers/base/mod_devicetable.c b/drivers/base/mod_devicetable.c
index f1d3de9f111c4..d7f198aad430f 100644
--- a/drivers/base/mod_devicetable.c
+++ b/drivers/base/mod_devicetable.c
@@ -12,24 +12,33 @@ 
 #include "base.h"
 #include "../usb/core/usb.h"

+/* Helper macro to add a modalias field to the string buffer associated with
+ * a match id.
+ *
+ * Note that:
+ *   + len should be a ssize_t and is modified in the macro
+ *   + sep should be a string literal and is concatenated as part of a format
+ *     string
+ *   + field is the struct field of the match id
+ */
 #define ADD(buf, count, len, sep, cond, field)				\
-do {									\
+do {				                                        \
+	char *buf_ = buf;						\
+	size_t count_ = count;                                          \
 	if (cond)							\
-		(len) += scnprintf(&(buf)[len],				\
-			(count) - (len),				\
+		(len) += scnprintf(&buf_[len],				\
+			count_ - (len),					\
 			sizeof(field) == 1 ? (sep "%02X") :		\
 			sizeof(field) == 2 ? (sep "%04X") :		\
 			sizeof(field) == 4 ? (sep "%08X") : "",		\
 			(field));					\
 	else								\
-		(len) += scnprintf(&(buf)[len], (count) - (len), (sep "*")); \
+		(len) += scnprintf(&buf_[len], count_ - (len), (sep "*")); \
 } while (0)

 #ifdef CONFIG_USB
 /* USB related modaliases can be split because of device number matching, so
  * this function handles individual modaliases for one segment of the range.
- *
- *
  */
 static ssize_t usb_id_to_modalias(const struct usb_device_id *id,
 				  unsigned int bcdDevice_initial,
@@ -134,8 +143,7 @@  static unsigned int incbcd(unsigned int *bcd,
 	return init;
 }

-/* Print the modaliases for the specified struct usb_device_id.
- */
+/* Print the modaliases for the specified struct usb_device_id. */
 static ssize_t usb_id_to_modalias_multi(const struct usb_device_id *id,
 					const char *mod_name, char *buf,
 					size_t count)
diff --git a/kernel/params.c b/kernel/params.c
index 111024196361a..b7fd5313a3118 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -13,7 +13,12 @@ 
 #include <linux/slab.h>
 #include <linux/ctype.h>
 #include <linux/security.h>
+
+#ifdef CONFIG_MODULES
 #include "module/internal.h"
+#else
+static inline void add_modalias_attr(struct module_kobject *mk) {}
+#endif /* !CONFIG_MODULES */

 #ifdef CONFIG_SYSFS
 /* Protects all built-in parameters, modules use their own param_lock */


## Changes made in v3

* Fixed more build errors found by the Intel bot
  * uint64_t division isn't always available so use `do_div` instead.
* Removed extra print statements that are not needed in the modalias
  output.

diff --git a/drivers/base/mod_devicetable.c b/drivers/base/mod_devicetable.c
index b04442207668c..f1d3de9f111c4 100644
--- a/drivers/base/mod_devicetable.c
+++ b/drivers/base/mod_devicetable.c
@@ -101,7 +101,7 @@  static unsigned int incbcd(unsigned int *bcd,
 			   size_t chars)
 {
 	unsigned int init = *bcd, i, j;
-	unsigned long long c, dec = 0;
+	unsigned long long c, dec = 0, div;

 	/* If bcd is not in BCD format, just increment */
 	if (max > 0x9) {
@@ -126,7 +126,9 @@  static unsigned int incbcd(unsigned int *bcd,
 	for (i = 0 ; i < chars ; i++) {
 		for (c = 1, j = 0 ; j < i ; j++)
 			c = c * 10;
-		c = (dec / c) % 10;
+		div = dec;
+		(void)do_div(div, c); /* div = div / c */
+		c = do_div(div, 10); /* c = div % 10; div = div / 10 */
 		*bcd += c << (i << 2);
 	}
 	return init;
diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
index e80bfa4639765..651c677c4ab96 100644
--- a/kernel/module/sysfs.c
+++ b/kernel/module/sysfs.c
@@ -272,9 +272,6 @@  static int print_modalias_for_drv(struct device_driver *drv, void *p)
 			return len;
 		s->len += len;
 	}
-
-	s->len += scnprintf(&s->buf[s->len], s->count - s->len, "driver %s\n",
-			    drv->name);
 	return 0;
 }

@@ -299,15 +296,6 @@  static ssize_t module_modalias_read(struct file *filp, struct kobject *kobj,
 	if (error)
 		return error;

-	if (mk->mod)
-		state.len += scnprintf(&buf[state.len], count - state.len,
-				       "modalias %s %s\n", kobject_name(kobj),
-				       mk->mod->name);
-	else
-		state.len += scnprintf(&buf[state.len], count - state.len,
-				       "modalias %s NULL\n",
-				       kobject_name(kobj));
-
 	/*
 	 * The caller checked the pos and count against our size.
 	 */


## Changes made in v2

Fixed build errors found by the Intel bot.
* `ENOSUP` is not always available so use `EINVAL` instead
* Include the header that declares `usb_drv_to_modalias` where it is defined
* Only implement `usb_drv_to_modalias` for `CONFIG_USB=y`

diff --git a/drivers/base/base.h b/drivers/base/base.h
index b3dec55fc6e87..beaa252c04388 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -178,7 +178,7 @@  ssize_t usb_drv_to_modalias(struct device_driver *drv, char *buf,
 			    size_t count);
 #else
 static inline ssize_t usb_drv_to_modalias(struct device_driver *drv, char *buf,
-					  size_t count) { return -ENOSUP; }
+					  size_t count) { return -EINVAL; }
 #endif

 #ifdef CONFIG_DEVTMPFS
diff --git a/drivers/base/mod_devicetable.c b/drivers/base/mod_devicetable.c
index da8d524cdf57a..b04442207668c 100644
--- a/drivers/base/mod_devicetable.c
+++ b/drivers/base/mod_devicetable.c
@@ -9,6 +9,7 @@ 
 #include <linux/device.h>
 #include <linux/usb.h>

+#include "base.h"
 #include "../usb/core/usb.h"

 #define ADD(buf, count, len, sep, cond, field)				\
@@ -24,6 +25,7 @@  do {									\
 		(len) += scnprintf(&(buf)[len], (count) - (len), (sep "*")); \
 } while (0)

+#ifdef CONFIG_USB
 /* USB related modaliases can be split because of device number matching, so
  * this function handles individual modaliases for one segment of the range.
  *
@@ -239,3 +241,7 @@  ssize_t usb_drv_to_modalias(struct device_driver *drv, char *buf,
 	}
 	return len;
 }
+#else
+inline ssize_t usb_drv_to_modalias(struct device_driver *drv, char *buf,
+				   size_t count){ return 0; }
+#endif