Jean Delvare <jdelvare@xxxxxxx> writes:
> On Tue, 2019-11-12 at 10:05 +0100, Ondřej Lysoněk wrote:
>> Jean Delvare <jdelvare@xxxxxxx> 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@xxxxxxxxxx>
>> > > ---
>> > > 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
