[i2c-tools] tools: Load the i2c-dev kernel module when needed
diff mbox series

Message ID 20191107140006.14413-1-olysonek@redhat.com
State New
Headers show
Series
  • [i2c-tools] tools: Load the i2c-dev kernel module when needed
Related show

Commit Message

Ondřej Lysoněk Nov. 7, 2019, 2 p.m. UTC
The i2c-dev kernel module is necessary to access I2C adapters from
userspace, so it's needed by all the I2C tools. The module is not
loaded automatically, so when e.g. i2cdetect is run without i2c-dev
loaded, it gives a false impression that no I2C adapters exist, which
is not very user-friendly:

 # i2cdetect -l
 # modprobe i2c-dev
 # i2cdetect -l
i2c-0	i2c       	i915 gmbus ssc                  	I2C adapter
i2c-1	i2c       	i915 gmbus vga                  	I2C adapter
i2c-2	i2c       	i915 gmbus panel                	I2C adapter
i2c-3	i2c       	i915 gmbus dpc                  	I2C adapter
i2c-4	i2c       	i915 gmbus dpb                  	I2C adapter
i2c-5	i2c       	i915 gmbus dpd                  	I2C adapter
i2c-6	i2c       	DPDDC-C                         	I2C adapter
i2c-7	i2c       	DPDDC-D                         	I2C adapter

This patch makes all the tools autoload i2c-dev when needed. The code
to load the module is compiled in only if libkmod is present on the
system.

This resolves the following Fedora bug:
https://bugzilla.redhat.com/show_bug.cgi?id=913203

The patch is based on a previous version of the patch by Michal Minar,
posted on linux-i2c some time ago:
https://marc.info/?l=linux-i2c&m=140195625915505&w=2

The patch is also inspired by the GPLv2+ modprobe from kmod.

Signed-off-by: Ondřej Lysoněk <olysonek@redhat.com>
---
 tools/Module.mk   | 11 +++++-
 tools/i2cbusses.c | 97 +++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 95 insertions(+), 13 deletions(-)

Comments

Jean Delvare Nov. 11, 2019, 7:03 p.m. UTC | #1
Hi Ondřej,

On Thu,  7 Nov 2019 15:00:06 +0100, Ondřej Lysoněk wrote:
> The i2c-dev kernel module is necessary to access I2C adapters from
> userspace, so it's needed by all the I2C tools. The module is not
> loaded automatically, so when e.g. i2cdetect is run without i2c-dev
> loaded, it gives a false impression that no I2C adapters exist, which
> is not very user-friendly:
> 
>  # i2cdetect -l
>  # modprobe i2c-dev
>  # i2cdetect -l
> i2c-0	i2c       	i915 gmbus ssc                  	I2C adapter
> i2c-1	i2c       	i915 gmbus vga                  	I2C adapter
> i2c-2	i2c       	i915 gmbus panel                	I2C adapter
> i2c-3	i2c       	i915 gmbus dpc                  	I2C adapter
> i2c-4	i2c       	i915 gmbus dpb                  	I2C adapter
> i2c-5	i2c       	i915 gmbus dpd                  	I2C adapter
> i2c-6	i2c       	DPDDC-C                         	I2C adapter
> i2c-7	i2c       	DPDDC-D                         	I2C adapter

Yeah, I agree, I meant to do this for a long time, actually I have a
proof-of-concept patch for this on my disk from 2006 :-(

> This patch makes all the tools autoload i2c-dev when needed. The code
> to load the module is compiled in only if libkmod is present on the
> system.

What is the added value of libkmod here? I mean, how is this any better
than:

	system("modprobe i2c-dev");

which is what my patch was doing?

> This resolves the following Fedora bug:
> https://bugzilla.redhat.com/show_bug.cgi?id=913203
> 
> The patch is based on a previous version of the patch by Michal Minar,
> posted on linux-i2c some time ago:
> https://marc.info/?l=linux-i2c&m=140195625915505&w=2
> 
> The patch is also inspired by the GPLv2+ modprobe from kmod.
> 
> Signed-off-by: Ondřej Lysoněk <olysonek@redhat.com>
> ---
>  tools/Module.mk   | 11 +++++-
>  tools/i2cbusses.c | 97 +++++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 95 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/Module.mk b/tools/Module.mk
> index 693102f..17ef8d4 100644
> --- a/tools/Module.mkles joues
> +++ b/tools/Module.mk
> @@ -12,10 +12,17 @@ TOOLS_DIR	:= tools
>  TOOLS_CFLAGS	:= -Wstrict-prototypes -Wshadow -Wpointer-arith -Wcast-qual \
>  		   -Wcast-align -Wwrite-strings -Wnested-externs -Winline \
>  		   -W -Wundef -Wmissing-prototypes -Iinclude
> +TOOLS_LDFLAGS	:=
> +
> +ifeq ($(shell pkg-config --exists libkmod && echo 1), 1)
> +TOOLS_CFLAGS	+= $(shell pkg-config --cflags libkmod) -DUSE_LIBKMOD
> +TOOLS_LDFLAGS	+= $(shell pkg-config --libs libkmod)
> +endif
> +
>  ifeq ($(USE_STATIC_LIB),1)
> -TOOLS_LDFLAGS	:= $(LIB_DIR)/$(LIB_STLIBNAME)
> +TOOLS_LDFLAGS	+= $(LIB_DIR)/$(LIB_STLIBNAME)
>  else
> -TOOLS_LDFLAGS	:= -L$(LIB_DIR) -li2c
> +TOOLS_LDFLAGS	+= -L$(LIB_DIR) -li2c
>  endif
>  
>  TOOLS_TARGETS	:= i2cdetect i2cdump i2cset i2cget i2ctransfer
> diff --git a/tools/i2cbusses.c b/tools/i2cbusses.c
> index b4f00ae..5f3ad42 100644
> --- a/tools/i2cbusses.c
> +++ b/tools/i2cbusses.c
> @@ -43,6 +43,14 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c-dev.h>
>  
> +#ifdef USE_LIBKMOD
> +#include <libkmod.h>
> +
> +#define I2C_DEV_MOD_NAME "i2c_dev"
> +#define MODULE_LOAD_ERR_MSG "Error: Failed to load required " \
> +                             I2C_DEV_MOD_NAME " kernel module: "
> +#endif
> +
>  enum adt { adt_dummy, adt_isa, adt_i2c, adt_smbus, adt_unknown };
>  
>  struct adap_type {
> @@ -63,6 +71,61 @@ static struct adap_type adap_types[5] = {
>  	  .algo		= "N/A", },
>  };
>  
> +
> +/**
> + * Try to load the i2c_dev kernel module.
> + * Returns 1 on success, 0 otherwise.
> + */
> +static int load_i2c_dev_module(int quiet) {
> +	int ret = 0;
> +#ifdef USE_LIBKMOD
> +	int flags = 0;
> +	struct kmod_ctx *ctx;
> +	struct kmod_list *l, *list = NULL;
> +
> +	ctx = kmod_new(NULL, NULL);
> +	if (!ctx) {
> +		if (!quiet)
> +			fprintf(stderr,
> +				MODULE_LOAD_ERR_MSG "kmod_new() failed\n");
> +		goto out;
> +	}
> +	if (kmod_module_new_from_lookup(ctx, I2C_DEV_MOD_NAME, &list) < 0
> +	    || list == NULL) {
> +		if (!quiet)
> +			fprintf(stderr, MODULE_LOAD_ERR_MSG
> +				I2C_DEV_MOD_NAME " module lookup failed\n");
> +		goto ctx_unref;
> +	}
> +
> +	flags |= KMOD_PROBE_APPLY_BLACKLIST_ALIAS_ONLY;
> +	kmod_list_foreach(l, list) {
> +		struct kmod_module *mod = kmod_module_get_module(l);
> +		int res;
> +		res = kmod_module_probe_insert_module(mod, flags, NULL, NULL,
> +		                                      NULL, NULL);
> +		ret = (res >= 0);
> +		if (res == -ENOENT && !quiet) {
> +			fprintf(stderr, MODULE_LOAD_ERR_MSG
> +			        "unknown symbol in module \"%s\", or unknown parameter (see dmesg)\n",
> +			        kmod_module_get_name(mod));
> +		} else if (res < 0 && !quiet) {
> +			fprintf(stderr, MODULE_LOAD_ERR_MSG "(module %s): %s\n",
> +			        kmod_module_get_name(mod), strerror(-res));
> +		}
> +		kmod_module_unref(mod);
> +	}
> +
> +	kmod_module_unref_list(list);
> +ctx_unref:
> +	kmod_unref(ctx);
> +out:
> +#else
> +	(void) quiet;
> +#endif
> +	return ret;
> +}
> +
>  static enum adt i2c_get_funcs(int i2cbus)
>  {
>  	unsigned long funcs;
> @@ -209,8 +272,12 @@ struct i2c_adap *gather_i2c_busses(void)
>  	   i2c-dev and what we really care about are the i2c-dev numbers.
>  	   Unfortunately the names are harder to get in i2c-dev */
>  	strcat(sysfs, "/class/i2c-dev");
> -	if(!(dir = opendir(sysfs)))
> -		goto done;
> +	if(!(dir = opendir(sysfs))) {
> +		if (!load_i2c_dev_module(0))
> +			goto done;
> +		if ((!(dir = opendir(sysfs))))
> +			goto done;
> +	}
>  	/* go through the busses */
>  	while ((de = readdir(dir)) != NULL) {
>  		if (!strcmp(de->d_name, "."))
> @@ -407,21 +474,29 @@ int parse_i2c_address(const char *address_arg, int all_addrs)
>  int open_i2c_dev(int i2cbus, char *filename, size_t size, int quiet)
>  {
>  	int file, len;
> +	int i;
>  
> -	len = snprintf(filename, size, "/dev/i2c/%d", i2cbus);
> -	if (len >= (int)size) {
> -		fprintf(stderr, "%s: path truncated\n", filename);
> -		return -EOVERFLOW;
> -	}
> -	file = open(filename, O_RDWR);
> -
> -	if (file < 0 && (errno == ENOENT || errno == ENOTDIR)) {
> -		len = snprintf(filename, size, "/dev/i2c-%d", i2cbus);
> +	for (i = 0; i < 2; ++i) {

Sorry, what are you doing here?

> +		len = snprintf(filename, size, "/dev/i2c/%d", i2cbus);
>  		if (len >= (int)size) {
>  			fprintf(stderr, "%s: path truncated\n", filename);
>  			return -EOVERFLOW;
>  		}
>  		file = open(filename, O_RDWR);
> +
> +		if (file < 0 && (errno == ENOENT || errno == ENOTDIR)) {
> +			len = snprintf(filename, size, "/dev/i2c-%d", i2cbus);
> +			if (len >= (int)size) {
> +				fprintf(stderr, "%s: path truncated\n", filename);
> +				return -EOVERFLOW;
> +			}
> +			file = open(filename, O_RDWR);
> +		}
> +
> +		if (file >= 0 || (errno != ENOENT && errno != ENOTDIR))
> +			break;
> +		if (i > 0 || !load_i2c_dev_module(quiet))
> +			break;
>  	}
>  
>  	if (file < 0 && !quiet) {
Ondřej Lysoněk Nov. 12, 2019, 9:05 a.m. UTC | #2
Hi Jean,

Jean Delvare <jdelvare@suse.de> writes:

> Hi Ondřej,
>
> On Thu,  7 Nov 2019 15:00:06 +0100, Ondřej Lysoněk wrote:
>> The i2c-dev kernel module is necessary to access I2C adapters from
>> userspace, so it's needed by all the I2C tools. The module is not
>> loaded automatically, so when e.g. i2cdetect is run without i2c-dev
>> loaded, it gives a false impression that no I2C adapters exist, which
>> is not very user-friendly:
>> 
>>  # i2cdetect -l
>>  # modprobe i2c-dev
>>  # i2cdetect -l
>> i2c-0	i2c       	i915 gmbus ssc                  	I2C adapter
>> i2c-1	i2c       	i915 gmbus vga                  	I2C adapter
>> i2c-2	i2c       	i915 gmbus panel                	I2C adapter
>> i2c-3	i2c       	i915 gmbus dpc                  	I2C adapter
>> i2c-4	i2c       	i915 gmbus dpb                  	I2C adapter
>> i2c-5	i2c       	i915 gmbus dpd                  	I2C adapter
>> i2c-6	i2c       	DPDDC-C                         	I2C adapter
>> i2c-7	i2c       	DPDDC-D                         	I2C adapter
>
> Yeah, I agree, I meant to do this for a long time, actually I have a
> proof-of-concept patch for this on my disk from 2006 :-(
>
>> This patch makes all the tools autoload i2c-dev when needed. The code
>> to load the module is compiled in only if libkmod is present on the
>> system.
>
> What is the added value of libkmod here? I mean, how is this any better
> than:
>
> 	system("modprobe i2c-dev");
>
> which is what my patch was doing?

Using libkmod should be faster as it avoids executing the shell and then
modprobe. I could use system("modprobe i2c-dev") in the #else branch
when libkmod is not available, though.

>
>> This resolves the following Fedora bug:
>> https://bugzilla.redhat.com/show_bug.cgi?id=913203
>> 
>> The patch is based on a previous version of the patch by Michal Minar,
>> posted on linux-i2c some time ago:
>> https://marc.info/?l=linux-i2c&m=140195625915505&w=2
>> 
>> The patch is also inspired by the GPLv2+ modprobe from kmod.
>> 
>> Signed-off-by: Ondřej Lysoněk <olysonek@redhat.com>
>> ---
>>  tools/Module.mk   | 11 +++++-
>>  tools/i2cbusses.c | 97 +++++++++++++++++++++++++++++++++++++++++------
>>  2 files changed, 95 insertions(+), 13 deletions(-)
>> 
>> diff --git a/tools/Module.mk b/tools/Module.mk
>> index 693102f..17ef8d4 100644
>> --- a/tools/Module.mkles joues
>> +++ b/tools/Module.mk
>> @@ -12,10 +12,17 @@ TOOLS_DIR	:= tools
>>  TOOLS_CFLAGS	:= -Wstrict-prototypes -Wshadow -Wpointer-arith -Wcast-qual \
>>  		   -Wcast-align -Wwrite-strings -Wnested-externs -Winline \
>>  		   -W -Wundef -Wmissing-prototypes -Iinclude
>> +TOOLS_LDFLAGS	:=
>> +
>> +ifeq ($(shell pkg-config --exists libkmod && echo 1), 1)
>> +TOOLS_CFLAGS	+= $(shell pkg-config --cflags libkmod) -DUSE_LIBKMOD
>> +TOOLS_LDFLAGS	+= $(shell pkg-config --libs libkmod)
>> +endif
>> +
>>  ifeq ($(USE_STATIC_LIB),1)
>> -TOOLS_LDFLAGS	:= $(LIB_DIR)/$(LIB_STLIBNAME)
>> +TOOLS_LDFLAGS	+= $(LIB_DIR)/$(LIB_STLIBNAME)
>>  else
>> -TOOLS_LDFLAGS	:= -L$(LIB_DIR) -li2c
>> +TOOLS_LDFLAGS	+= -L$(LIB_DIR) -li2c
>>  endif
>>  
>>  TOOLS_TARGETS	:= i2cdetect i2cdump i2cset i2cget i2ctransfer
>> diff --git a/tools/i2cbusses.c b/tools/i2cbusses.c
>> index b4f00ae..5f3ad42 100644
>> --- a/tools/i2cbusses.c
>> +++ b/tools/i2cbusses.c
>> @@ -43,6 +43,14 @@
>>  #include <linux/i2c.h>
>>  #include <linux/i2c-dev.h>
>>  
>> +#ifdef USE_LIBKMOD
>> +#include <libkmod.h>
>> +
>> +#define I2C_DEV_MOD_NAME "i2c_dev"
>> +#define MODULE_LOAD_ERR_MSG "Error: Failed to load required " \
>> +                             I2C_DEV_MOD_NAME " kernel module: "
>> +#endif
>> +
>>  enum adt { adt_dummy, adt_isa, adt_i2c, adt_smbus, adt_unknown };
>>  
>>  struct adap_type {
>> @@ -63,6 +71,61 @@ static struct adap_type adap_types[5] = {
>>  	  .algo		= "N/A", },
>>  };
>>  
>> +
>> +/**
>> + * Try to load the i2c_dev kernel module.
>> + * Returns 1 on success, 0 otherwise.
>> + */
>> +static int load_i2c_dev_module(int quiet) {
>> +	int ret = 0;
>> +#ifdef USE_LIBKMOD
>> +	int flags = 0;
>> +	struct kmod_ctx *ctx;
>> +	struct kmod_list *l, *list = NULL;
>> +
>> +	ctx = kmod_new(NULL, NULL);
>> +	if (!ctx) {
>> +		if (!quiet)
>> +			fprintf(stderr,
>> +				MODULE_LOAD_ERR_MSG "kmod_new() failed\n");
>> +		goto out;
>> +	}
>> +	if (kmod_module_new_from_lookup(ctx, I2C_DEV_MOD_NAME, &list) < 0
>> +	    || list == NULL) {
>> +		if (!quiet)
>> +			fprintf(stderr, MODULE_LOAD_ERR_MSG
>> +				I2C_DEV_MOD_NAME " module lookup failed\n");
>> +		goto ctx_unref;
>> +	}
>> +
>> +	flags |= KMOD_PROBE_APPLY_BLACKLIST_ALIAS_ONLY;
>> +	kmod_list_foreach(l, list) {
>> +		struct kmod_module *mod = kmod_module_get_module(l);
>> +		int res;
>> +		res = kmod_module_probe_insert_module(mod, flags, NULL, NULL,
>> +		                                      NULL, NULL);
>> +		ret = (res >= 0);
>> +		if (res == -ENOENT && !quiet) {
>> +			fprintf(stderr, MODULE_LOAD_ERR_MSG
>> +			        "unknown symbol in module \"%s\", or unknown parameter (see dmesg)\n",
>> +			        kmod_module_get_name(mod));
>> +		} else if (res < 0 && !quiet) {
>> +			fprintf(stderr, MODULE_LOAD_ERR_MSG "(module %s): %s\n",
>> +			        kmod_module_get_name(mod), strerror(-res));
>> +		}
>> +		kmod_module_unref(mod);
>> +	}
>> +
>> +	kmod_module_unref_list(list);
>> +ctx_unref:
>> +	kmod_unref(ctx);
>> +out:
>> +#else
>> +	(void) quiet;
>> +#endif
>> +	return ret;
>> +}
>> +
>>  static enum adt i2c_get_funcs(int i2cbus)
>>  {
>>  	unsigned long funcs;
>> @@ -209,8 +272,12 @@ struct i2c_adap *gather_i2c_busses(void)
>>  	   i2c-dev and what we really care about are the i2c-dev numbers.
>>  	   Unfortunately the names are harder to get in i2c-dev */
>>  	strcat(sysfs, "/class/i2c-dev");
>> -	if(!(dir = opendir(sysfs)))
>> -		goto done;
>> +	if(!(dir = opendir(sysfs))) {
>> +		if (!load_i2c_dev_module(0))
>> +			goto done;
>> +		if ((!(dir = opendir(sysfs))))
>> +			goto done;
>> +	}
>>  	/* go through the busses */
>>  	while ((de = readdir(dir)) != NULL) {
>>  		if (!strcmp(de->d_name, "."))
>> @@ -407,21 +474,29 @@ int parse_i2c_address(const char *address_arg, int all_addrs)
>>  int open_i2c_dev(int i2cbus, char *filename, size_t size, int quiet)
>>  {
>>  	int file, len;
>> +	int i;
>>  
>> -	len = snprintf(filename, size, "/dev/i2c/%d", i2cbus);
>> -	if (len >= (int)size) {
>> -		fprintf(stderr, "%s: path truncated\n", filename);
>> -		return -EOVERFLOW;
>> -	}
>> -	file = open(filename, O_RDWR);
>> -
>> -	if (file < 0 && (errno == ENOENT || errno == ENOTDIR)) {
>> -		len = snprintf(filename, size, "/dev/i2c-%d", i2cbus);
>> +	for (i = 0; i < 2; ++i) {
>
> Sorry, what are you doing here?

I'm attempting to load /dev/i2c/%d or /dev/i2c-%d. If none of them
exist, I load the i2c-dev module and try again. Perhaps it would be
clearer if I used goto instead of the for loop. Or move opening the
files to a separate function.

>
>> +		len = snprintf(filename, size, "/dev/i2c/%d", i2cbus);
>>  		if (len >= (int)size) {
>>  			fprintf(stderr, "%s: path truncated\n", filename);
>>  			return -EOVERFLOW;
>>  		}
>>  		file = open(filename, O_RDWR);
>> +
>> +		if (file < 0 && (errno == ENOENT || errno == ENOTDIR)) {
>> +			len = snprintf(filename, size, "/dev/i2c-%d", i2cbus);
>> +			if (len >= (int)size) {
>> +				fprintf(stderr, "%s: path truncated\n", filename);
>> +				return -EOVERFLOW;
>> +			}
>> +			file = open(filename, O_RDWR);
>> +		}
>> +
>> +		if (file >= 0 || (errno != ENOENT && errno != ENOTDIR))
>> +			break;
>> +		if (i > 0 || !load_i2c_dev_module(quiet))
>> +			break;
>>  	}
>>  
>>  	if (file < 0 && !quiet) {
>
>
> -- 
> Jean Delvare
> SUSE L3 Support

Thanks,
Ondra
Jean Delvare Nov. 12, 2019, 10:48 a.m. UTC | #3
On Tue, 2019-11-12 at 10:05 +0100, Ondřej Lysoněk wrote:
> Jean Delvare <jdelvare@suse.de> writes:
> > On Thu,  7 Nov 2019 15:00:06 +0100, Ondřej Lysoněk wrote:
> > > This patch makes all the tools autoload i2c-dev when needed. The code
> > > to load the module is compiled in only if libkmod is present on the
> > > system.
> > 
> > What is the added value of libkmod here? I mean, how is this any better
> > than:
> > 
> > 	system("modprobe i2c-dev");
> > 
> > which is what my patch was doing?
> 
> Using libkmod should be faster as it avoids executing the shell and then
> modprobe. I could use system("modprobe i2c-dev") in the #else branch
> when libkmod is not available, though.

Michal's patch was doing that, and I think it makes sense, as I'd
rather not depend on the presence of libkmod for the feature itself. I
would like to be able to document "the i2c-dev module will be loaded as
needed" without having to explain in which case this will actually work
and in which case it won't. Doesn't matter if it's slower without
libkmod - that does not need to be documented.

> > > This resolves the following Fedora bug:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=913203
> > > 
> > > The patch is based on a previous version of the patch by Michal Minar,
> > > posted on linux-i2c some time ago:
> > > https://marc.info/?l=linux-i2c&m=140195625915505&w=2
> > > 
> > > The patch is also inspired by the GPLv2+ modprobe from kmod.
> > > 
> > > Signed-off-by: Ondřej Lysoněk <olysonek@redhat.com>
> > > ---
> > >  tools/Module.mk   | 11 +++++-
> > >  tools/i2cbusses.c | 97 +++++++++++++++++++++++++++++++++++++++++------
> > >  2 files changed, 95 insertions(+), 13 deletions(-)
> > >  (...)
> > > +/**
> > > + * Try to load the i2c_dev kernel module.
> > > + * Returns 1 on success, 0 otherwise.
> > > + */

A rather unusual convention. Any reason why you opted for that rather
that the usual 0 on success / -errno on error?

> > > +static int load_i2c_dev_module(int quiet) {
> > > +	int ret = 0;
> > > +#ifdef USE_LIBKMOD
> > > +	int flags = 0;
> > > +	struct kmod_ctx *ctx;
> > > +	struct kmod_list *l, *list = NULL;
> > > +
> > > +	ctx = kmod_new(NULL, NULL);
> > > +	if (!ctx) {
> > > +		if (!quiet)
> > > +			fprintf(stderr,
> > > +				MODULE_LOAD_ERR_MSG "kmod_new() failed\n");
> > > +		goto out;
> > > +	}
> > > +	if (kmod_module_new_from_lookup(ctx, I2C_DEV_MOD_NAME, &list) < 0
> > > +	    || list == NULL) {
> > > +		if (!quiet)
> > > +			fprintf(stderr, MODULE_LOAD_ERR_MSG
> > > +				I2C_DEV_MOD_NAME " module lookup failed\n");
> > > +		goto ctx_unref;
> > > +	}
> > > +
> > > +	flags |= KMOD_PROBE_APPLY_BLACKLIST_ALIAS_ONLY;
> > > +	kmod_list_foreach(l, list) {

I must say I'm surprised this is a list, I thought module names had to
be unique?

> > > +		struct kmod_module *mod = kmod_module_get_module(l);
> > > +		int res;
> > > +		res = kmod_module_probe_insert_module(mod, flags, NULL, NULL,
> > > +		                                      NULL, NULL);
> > > +		ret = (res >= 0);

Mixing "ret" and "res" in the same function is asking for trouble. Do
you really need two variables?

> > > +		if (res == -ENOENT && !quiet) {
> > > +			fprintf(stderr, MODULE_LOAD_ERR_MSG
> > > +			        "unknown symbol in module \"%s\", or unknown parameter (see dmesg)\n",
> > > +			        kmod_module_get_name(mod));
> > > +		} else if (res < 0 && !quiet) {
> > > +			fprintf(stderr, MODULE_LOAD_ERR_MSG "(module %s): %s\n",
> > > +			        kmod_module_get_name(mod), strerror(-res));
> > > +		}
> > > +		kmod_module_unref(mod);

Are we guaranteed that the driver probing has completed at this point?
I mean, the kernel sends events for udev to process and the actual /dev
nodes are created by udev asynchronously as I understand it. Don't we
need an equivalent of "udevadm settle" here?

> > > +	}
> > > +
> > > +	kmod_module_unref_list(list);
> > > +ctx_unref:
> > > +	kmod_unref(ctx);
> > > +out:
> > > +#else
> > > +	(void) quiet;
> > > +#endif
> > > +	return ret;
> > > +}
> > > +
> > >  static enum adt i2c_get_funcs(int i2cbus)
> > >  {
> > >  	unsigned long funcs;
> > > @@ -209,8 +272,12 @@ struct i2c_adap *gather_i2c_busses(void)
> > >  	   i2c-dev and what we really care about are the i2c-dev numbers.
> > >  	   Unfortunately the names are harder to get in i2c-dev */
> > >  	strcat(sysfs, "/class/i2c-dev");
> > > -	if(!(dir = opendir(sysfs)))
> > > -		goto done;
> > > +	if(!(dir = opendir(sysfs))) {
> > > +		if (!load_i2c_dev_module(0))
> > > +			goto done;
> > > +		if ((!(dir = opendir(sysfs))))
> > > +			goto done;
> > > +	}
> > >  	/* go through the busses */
> > >  	while ((de = readdir(dir)) != NULL) {
> > >  		if (!strcmp(de->d_name, "."))
> > > @@ -407,21 +474,29 @@ int parse_i2c_address(const char *address_arg, int all_addrs)
> > >  int open_i2c_dev(int i2cbus, char *filename, size_t size, int quiet)
> > >  {
> > >  	int file, len;
> > > +	int i;
> > >  
> > > -	len = snprintf(filename, size, "/dev/i2c/%d", i2cbus);
> > > -	if (len >= (int)size) {
> > > -		fprintf(stderr, "%s: path truncated\n", filename);
> > > -		return -EOVERFLOW;
> > > -	}
> > > -	file = open(filename, O_RDWR);
> > > -
> > > -	if (file < 0 && (errno == ENOENT || errno == ENOTDIR)) {
> > > -		len = snprintf(filename, size, "/dev/i2c-%d", i2cbus);
> > > +	for (i = 0; i < 2; ++i) {
> > 
> > Sorry, what are you doing here?
> 
> I'm attempting to load /dev/i2c/%d or /dev/i2c-%d. If none of them
> exist, I load the i2c-dev module and try again. Perhaps it would be
> clearer if I used goto instead of the for loop. Or move opening the
> files to a separate function.

I think I would rename open_i2c_dev to static __open_i2c_dev or
similar, and then add (untested):

int open_i2c_dev(int i2cbus, char *filename, size_t size, int quiet)
{
	int file;

	file = __open_i2c_dev(i2cbus, filename, size, quiet);
	if (file < 0 && load_i2c_dev_module(quiet))
		file = __open_i2c_dev(i2cbus, filename, size, quiet);

	return file;
}

This avoids the extra indentation or the goto, and seems neater
overall. I don't think we care about the slight performance penalty.
What do you think?

Thanks,
Ondřej Lysoněk Nov. 12, 2019, 3:26 p.m. UTC | #4
Jean Delvare <jdelvare@suse.de> writes:

> On Tue, 2019-11-12 at 10:05 +0100, Ondřej Lysoněk wrote:
>> Jean Delvare <jdelvare@suse.de> writes:
>> > On Thu,  7 Nov 2019 15:00:06 +0100, Ondřej Lysoněk wrote:
>> > > This patch makes all the tools autoload i2c-dev when needed. The code
>> > > to load the module is compiled in only if libkmod is present on the
>> > > system.
>> > 
>> > What is the added value of libkmod here? I mean, how is this any better
>> > than:
>> > 
>> > 	system("modprobe i2c-dev");
>> > 
>> > which is what my patch was doing?
>> 
>> Using libkmod should be faster as it avoids executing the shell and then
>> modprobe. I could use system("modprobe i2c-dev") in the #else branch
>> when libkmod is not available, though.
>
> Michal's patch was doing that, and I think it makes sense, as I'd
> rather not depend on the presence of libkmod for the feature itself. I
> would like to be able to document "the i2c-dev module will be loaded as
> needed" without having to explain in which case this will actually work
> and in which case it won't. Doesn't matter if it's slower without
> libkmod - that does not need to be documented.

Sure. I thought there was a reason not to use system() so I removed it,
but I was wrong. I'll re-add it on the #else branch.

>> > > This resolves the following Fedora bug:
>> > > https://bugzilla.redhat.com/show_bug.cgi?id=913203
>> > > 
>> > > The patch is based on a previous version of the patch by Michal Minar,
>> > > posted on linux-i2c some time ago:
>> > > https://marc.info/?l=linux-i2c&m=140195625915505&w=2
>> > > 
>> > > The patch is also inspired by the GPLv2+ modprobe from kmod.
>> > > 
>> > > Signed-off-by: Ondřej Lysoněk <olysonek@redhat.com>
>> > > ---
>> > >  tools/Module.mk   | 11 +++++-
>> > >  tools/i2cbusses.c | 97 +++++++++++++++++++++++++++++++++++++++++------
>> > >  2 files changed, 95 insertions(+), 13 deletions(-)
>> > >  (...)
>> > > +/**
>> > > + * Try to load the i2c_dev kernel module.
>> > > + * Returns 1 on success, 0 otherwise.
>> > > + */
>
> A rather unusual convention. Any reason why you opted for that rather
> that the usual 0 on success / -errno on error?

No reason; this was taken from Michal's patch. I'll change it in v2.

>> > > +static int load_i2c_dev_module(int quiet) {
>> > > +	int ret = 0;
>> > > +#ifdef USE_LIBKMOD
>> > > +	int flags = 0;
>> > > +	struct kmod_ctx *ctx;
>> > > +	struct kmod_list *l, *list = NULL;
>> > > +
>> > > +	ctx = kmod_new(NULL, NULL);
>> > > +	if (!ctx) {
>> > > +		if (!quiet)
>> > > +			fprintf(stderr,
>> > > +				MODULE_LOAD_ERR_MSG "kmod_new() failed\n");
>> > > +		goto out;
>> > > +	}
>> > > +	if (kmod_module_new_from_lookup(ctx, I2C_DEV_MOD_NAME, &list) < 0
>> > > +	    || list == NULL) {
>> > > +		if (!quiet)
>> > > +			fprintf(stderr, MODULE_LOAD_ERR_MSG
>> > > +				I2C_DEV_MOD_NAME " module lookup failed\n");
>> > > +		goto ctx_unref;
>> > > +	}
>> > > +
>> > > +	flags |= KMOD_PROBE_APPLY_BLACKLIST_ALIAS_ONLY;
>> > > +	kmod_list_foreach(l, list) {
>
> I must say I'm surprised this is a list, I thought module names had to
> be unique?

I was surprised as well. My understanding is that you pass an alias to
kmod_module_new_from_lookup() rather than a module name. And there could
be aliases called e.g. 'i2c-dev' that point to some other modules. The
kmod_module_new_from_lookup() function resolves the aliases and puts all
the target modules to the list. So module names are probably unique, but
aliases need not be. I think.

modprobe uses the same approach.

>> > > +		struct kmod_module *mod = kmod_module_get_module(l);
>> > > +		int res;
>> > > +		res = kmod_module_probe_insert_module(mod, flags, NULL, NULL,
>> > > +		                                      NULL, NULL);
>> > > +		ret = (res >= 0);
>
> Mixing "ret" and "res" in the same function is asking for trouble. Do
> you really need two variables?

Good point. I'll resolve it somehow in v2.

>> > > +		if (res == -ENOENT && !quiet) {
>> > > +			fprintf(stderr, MODULE_LOAD_ERR_MSG
>> > > +			        "unknown symbol in module \"%s\", or unknown parameter (see dmesg)\n",
>> > > +			        kmod_module_get_name(mod));
>> > > +		} else if (res < 0 && !quiet) {
>> > > +			fprintf(stderr, MODULE_LOAD_ERR_MSG "(module %s): %s\n",
>> > > +			        kmod_module_get_name(mod), strerror(-res));
>> > > +		}
>> > > +		kmod_module_unref(mod);
>
> Are we guaranteed that the driver probing has completed at this point?
> I mean, the kernel sends events for udev to process and the actual /dev
> nodes are created by udev asynchronously as I understand it. Don't we
> need an equivalent of "udevadm settle" here?

To be honest, I'm not sure. I'll look into it.

>> > > +	}
>> > > +
>> > > +	kmod_module_unref_list(list);
>> > > +ctx_unref:
>> > > +	kmod_unref(ctx);
>> > > +out:
>> > > +#else
>> > > +	(void) quiet;
>> > > +#endif
>> > > +	return ret;
>> > > +}
>> > > +
>> > >  static enum adt i2c_get_funcs(int i2cbus)
>> > >  {
>> > >  	unsigned long funcs;
>> > > @@ -209,8 +272,12 @@ struct i2c_adap *gather_i2c_busses(void)
>> > >  	   i2c-dev and what we really care about are the i2c-dev numbers.
>> > >  	   Unfortunately the names are harder to get in i2c-dev */
>> > >  	strcat(sysfs, "/class/i2c-dev");
>> > > -	if(!(dir = opendir(sysfs)))
>> > > -		goto done;
>> > > +	if(!(dir = opendir(sysfs))) {
>> > > +		if (!load_i2c_dev_module(0))
>> > > +			goto done;
>> > > +		if ((!(dir = opendir(sysfs))))
>> > > +			goto done;
>> > > +	}
>> > >  	/* go through the busses */
>> > >  	while ((de = readdir(dir)) != NULL) {
>> > >  		if (!strcmp(de->d_name, "."))
>> > > @@ -407,21 +474,29 @@ int parse_i2c_address(const char *address_arg, int all_addrs)
>> > >  int open_i2c_dev(int i2cbus, char *filename, size_t size, int quiet)
>> > >  {
>> > >  	int file, len;
>> > > +	int i;
>> > >  
>> > > -	len = snprintf(filename, size, "/dev/i2c/%d", i2cbus);
>> > > -	if (len >= (int)size) {
>> > > -		fprintf(stderr, "%s: path truncated\n", filename);
>> > > -		return -EOVERFLOW;
>> > > -	}
>> > > -	file = open(filename, O_RDWR);
>> > > -
>> > > -	if (file < 0 && (errno == ENOENT || errno == ENOTDIR)) {
>> > > -		len = snprintf(filename, size, "/dev/i2c-%d", i2cbus);
>> > > +	for (i = 0; i < 2; ++i) {
>> > 
>> > Sorry, what are you doing here?
>> 
>> I'm attempting to load /dev/i2c/%d or /dev/i2c-%d. If none of them
>> exist, I load the i2c-dev module and try again. Perhaps it would be
>> clearer if I used goto instead of the for loop. Or move opening the
>> files to a separate function.
>
> I think I would rename open_i2c_dev to static __open_i2c_dev or
> similar, and then add (untested):
>
> int open_i2c_dev(int i2cbus, char *filename, size_t size, int quiet)
> {
> 	int file;
>
> 	file = __open_i2c_dev(i2cbus, filename, size, quiet);
> 	if (file < 0 && load_i2c_dev_module(quiet))
> 		file = __open_i2c_dev(i2cbus, filename, size, quiet);
>
> 	return file;
> }
>
> This avoids the extra indentation or the goto, and seems neater
> overall. I don't think we care about the slight performance penalty.
> What do you think?

Yes, I think that's the best solution. Will do.

> Thanks,
> -- 
> Jean Delvare
> SUSE L3 Support

Thanks for the review!

Ondra

Patch
diff mbox series

diff --git a/tools/Module.mk b/tools/Module.mk
index 693102f..17ef8d4 100644
--- a/tools/Module.mk
+++ b/tools/Module.mk
@@ -12,10 +12,17 @@  TOOLS_DIR	:= tools
 TOOLS_CFLAGS	:= -Wstrict-prototypes -Wshadow -Wpointer-arith -Wcast-qual \
 		   -Wcast-align -Wwrite-strings -Wnested-externs -Winline \
 		   -W -Wundef -Wmissing-prototypes -Iinclude
+TOOLS_LDFLAGS	:=
+
+ifeq ($(shell pkg-config --exists libkmod && echo 1), 1)
+TOOLS_CFLAGS	+= $(shell pkg-config --cflags libkmod) -DUSE_LIBKMOD
+TOOLS_LDFLAGS	+= $(shell pkg-config --libs libkmod)
+endif
+
 ifeq ($(USE_STATIC_LIB),1)
-TOOLS_LDFLAGS	:= $(LIB_DIR)/$(LIB_STLIBNAME)
+TOOLS_LDFLAGS	+= $(LIB_DIR)/$(LIB_STLIBNAME)
 else
-TOOLS_LDFLAGS	:= -L$(LIB_DIR) -li2c
+TOOLS_LDFLAGS	+= -L$(LIB_DIR) -li2c
 endif
 
 TOOLS_TARGETS	:= i2cdetect i2cdump i2cset i2cget i2ctransfer
diff --git a/tools/i2cbusses.c b/tools/i2cbusses.c
index b4f00ae..5f3ad42 100644
--- a/tools/i2cbusses.c
+++ b/tools/i2cbusses.c
@@ -43,6 +43,14 @@ 
 #include <linux/i2c.h>
 #include <linux/i2c-dev.h>
 
+#ifdef USE_LIBKMOD
+#include <libkmod.h>
+
+#define I2C_DEV_MOD_NAME "i2c_dev"
+#define MODULE_LOAD_ERR_MSG "Error: Failed to load required " \
+                             I2C_DEV_MOD_NAME " kernel module: "
+#endif
+
 enum adt { adt_dummy, adt_isa, adt_i2c, adt_smbus, adt_unknown };
 
 struct adap_type {
@@ -63,6 +71,61 @@  static struct adap_type adap_types[5] = {
 	  .algo		= "N/A", },
 };
 
+
+/**
+ * Try to load the i2c_dev kernel module.
+ * Returns 1 on success, 0 otherwise.
+ */
+static int load_i2c_dev_module(int quiet) {
+	int ret = 0;
+#ifdef USE_LIBKMOD
+	int flags = 0;
+	struct kmod_ctx *ctx;
+	struct kmod_list *l, *list = NULL;
+
+	ctx = kmod_new(NULL, NULL);
+	if (!ctx) {
+		if (!quiet)
+			fprintf(stderr,
+				MODULE_LOAD_ERR_MSG "kmod_new() failed\n");
+		goto out;
+	}
+	if (kmod_module_new_from_lookup(ctx, I2C_DEV_MOD_NAME, &list) < 0
+	    || list == NULL) {
+		if (!quiet)
+			fprintf(stderr, MODULE_LOAD_ERR_MSG
+				I2C_DEV_MOD_NAME " module lookup failed\n");
+		goto ctx_unref;
+	}
+
+	flags |= KMOD_PROBE_APPLY_BLACKLIST_ALIAS_ONLY;
+	kmod_list_foreach(l, list) {
+		struct kmod_module *mod = kmod_module_get_module(l);
+		int res;
+		res = kmod_module_probe_insert_module(mod, flags, NULL, NULL,
+		                                      NULL, NULL);
+		ret = (res >= 0);
+		if (res == -ENOENT && !quiet) {
+			fprintf(stderr, MODULE_LOAD_ERR_MSG
+			        "unknown symbol in module \"%s\", or unknown parameter (see dmesg)\n",
+			        kmod_module_get_name(mod));
+		} else if (res < 0 && !quiet) {
+			fprintf(stderr, MODULE_LOAD_ERR_MSG "(module %s): %s\n",
+			        kmod_module_get_name(mod), strerror(-res));
+		}
+		kmod_module_unref(mod);
+	}
+
+	kmod_module_unref_list(list);
+ctx_unref:
+	kmod_unref(ctx);
+out:
+#else
+	(void) quiet;
+#endif
+	return ret;
+}
+
 static enum adt i2c_get_funcs(int i2cbus)
 {
 	unsigned long funcs;
@@ -209,8 +272,12 @@  struct i2c_adap *gather_i2c_busses(void)
 	   i2c-dev and what we really care about are the i2c-dev numbers.
 	   Unfortunately the names are harder to get in i2c-dev */
 	strcat(sysfs, "/class/i2c-dev");
-	if(!(dir = opendir(sysfs)))
-		goto done;
+	if(!(dir = opendir(sysfs))) {
+		if (!load_i2c_dev_module(0))
+			goto done;
+		if ((!(dir = opendir(sysfs))))
+			goto done;
+	}
 	/* go through the busses */
 	while ((de = readdir(dir)) != NULL) {
 		if (!strcmp(de->d_name, "."))
@@ -407,21 +474,29 @@  int parse_i2c_address(const char *address_arg, int all_addrs)
 int open_i2c_dev(int i2cbus, char *filename, size_t size, int quiet)
 {
 	int file, len;
+	int i;
 
-	len = snprintf(filename, size, "/dev/i2c/%d", i2cbus);
-	if (len >= (int)size) {
-		fprintf(stderr, "%s: path truncated\n", filename);
-		return -EOVERFLOW;
-	}
-	file = open(filename, O_RDWR);
-
-	if (file < 0 && (errno == ENOENT || errno == ENOTDIR)) {
-		len = snprintf(filename, size, "/dev/i2c-%d", i2cbus);
+	for (i = 0; i < 2; ++i) {
+		len = snprintf(filename, size, "/dev/i2c/%d", i2cbus);
 		if (len >= (int)size) {
 			fprintf(stderr, "%s: path truncated\n", filename);
 			return -EOVERFLOW;
 		}
 		file = open(filename, O_RDWR);
+
+		if (file < 0 && (errno == ENOENT || errno == ENOTDIR)) {
+			len = snprintf(filename, size, "/dev/i2c-%d", i2cbus);
+			if (len >= (int)size) {
+				fprintf(stderr, "%s: path truncated\n", filename);
+				return -EOVERFLOW;
+			}
+			file = open(filename, O_RDWR);
+		}
+
+		if (file >= 0 || (errno != ENOENT && errno != ENOTDIR))
+			break;
+		if (i > 0 || !load_i2c_dev_module(quiet))
+			break;
 	}
 
 	if (file < 0 && !quiet) {