Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1266728

Re: [PATCH] mtd: phram: error handling

Show all headers | View raw

On Tue, Nov 10, 2015 at 10:45:55AM -0800, Joe Perches wrote:
> On Tue, 2015-11-10 at 10:39 -0800, Brian Norris wrote:
> > On Tue, Nov 10, 2015 at 10:33:07AM -0800, Joe Perches wrote:
> > > Expand parse_err macro with hidden flow in-place.
> > > Remove the now unused parse_err macro.
> []
> > I think -EINVAL makes more sense than 1. That
> > could be a subsequent patch, I suppose.
> 
> That means you have to trace all the callers
> to verify that converting 1 to -22 is acceptable.

It's fairly simple. Module initialization and module parameter calls
both *should* follow 0/negative error conventions. For module init, see
in kernel/module.c:

        /* Start the module */
        if (mod->init != NULL)
                ret = do_one_initcall(mod->init);
        if (ret < 0) {
                goto fail_free_freeinit;
        }
        if (ret > 0) {
                pr_warn("%s: '%s'->init suspiciously returned %d, it should "
                        "follow 0/-E convention\n"
                        "%s: loading module anyway...\n",
                        __func__, mod->name, ret, __func__);
                dump_stack();
        }

and in include/linux/moduleparam.h:

struct kernel_param_ops {
...
        /* Returns 0, or -errno.  arg is in kp->arg. */
        int (*set)(const char *val, const struct kernel_param *kp);
...
};

And for built-in modules, the return code is ignored (see
do_initcall_level()).

So I think the only question is whether we should actually be reporting
these errors on module insertion and on the module parameter call. I'd
say "definitely" to the latter and "yes" to the former, since the init
function already handles the case of an empty input (so the module can
be loaded with a blank command line without tripping on an -EINVAL
parameter check).

IOW, to use -EINVAL would be to actually enforce the error handling that
was intended in the first place. But that should be done in a separate
patch, and with an actual tester (since I doubt either you or Saurabh
are testing this driver).

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH] mtd: phram: error handling Saurabh Sengar <saurabh.truth@gmail.com> - 2015-11-08 09:50 +0100
  Re: [PATCH] mtd: phram: error handling Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-08 22:30 +0100
    [PATCH v2] mtd: phram: error handling Saurabh Sengar <saurabh.truth@gmail.com> - 2015-11-09 07:30 +0100
      Re: [PATCH v2] mtd: phram: error handling Brian Norris <computersforpeace@gmail.com> - 2015-11-10 19:30 +0100
        [PATCH] mtd: phram: error handling Joe Perches <joe@perches.com> - 2015-11-10 19:40 +0100
          Re: [PATCH] mtd: phram: error handling Joe Perches <joe@perches.com> - 2015-11-10 19:50 +0100
            Re: [PATCH] mtd: phram: error handling Brian Norris <computersforpeace@gmail.com> - 2015-11-10 20:10 +0100
              [PATCH] mtd: phram: error handling Saurabh Sengar <saurabh.truth@gmail.com> - 2015-11-10 20:30 +0100
                Re: [PATCH] mtd: phram: error handling Brian Norris <computersforpeace@gmail.com> - 2015-11-10 20:50 +0100
                [PATCH] mtd: phram: error handling Saurabh Sengar <saurabh.truth@gmail.com> - 2015-11-11 09:30 +0100
                Re: [PATCH] mtd: phram: error handling Brian Norris <computersforpeace@gmail.com> - 2015-11-11 20:50 +0100
                [PATCH] mtd: phram: error handling Saurabh Sengar <saurabh.truth@gmail.com> - 2015-11-12 07:30 +0100
          Re: [PATCH] mtd: phram: error handling Brian Norris <computersforpeace@gmail.com> - 2015-11-10 19:50 +0100

csiph-web