linux-kernel - Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 31 Aug 2017 18:45:11 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Andrey Smirnov <andrew.smirnov@...il.com>
Cc:     linux-kernel@...r.kernel.org, cphealy@...il.com,
        Lucas Stach <l.stach@...gutronix.de>,
        Nikita Yushchenko <nikita.yoush@...entembedded.com>,
        Lee Jones <lee.jones@...aro.org>, Pavel Machek <pavel@....cz>
Subject: Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory
 Processor

On Mon, Aug 28, 2017 at 09:31:30AM -0700, Andrey Smirnov wrote:
> +static int rave_sp_debugfs_create(struct rave_sp *sp)
> +{
> +	struct dentry *file;
> +
> +	sp->debugfs = debugfs_create_dir("rave", NULL);
> +	if (!sp->debugfs)
> +		return -ENOMEM;

Why do you care about the return value of the debugfs function?  Hint,
you don't...

You should never care if debugfs failed, was built in, or succeeded,
your code should just do the same thing always.  Any value returned from
a debugfs call can just be passed to another one, regardless of the
value returned from the first one.

So here, just save it, and don't check it.

> +	file = debugfs_create_file("i2c_device_status", 0444,
> +				   sp->debugfs, sp,
> +				   &rave_sp_i2c_device_status);
> +	if (!file)
> +		goto error;

Nope, you don't care, just make the call.

> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, part_number_firmware);
> +	if (!file)
> +		goto error;

Same for all of these.

> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, part_number_bootloader);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, copper_rev_deb);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, copper_rev_rmb);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, copper_mod_deb);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, copper_mod_rmb);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, silicon_devrev);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, silicon_devid);
> +	if (!file)
> +		goto error;
> +
> +	return 0;

And your function can not fail, no need for this to return anything.

> +error:
> +	debugfs_remove_recursive(sp->debugfs);
> +	return -ENOMEM;
> +}
> +
> +static void rave_sp_degugfs_release(struct device *dev, void *res)
> +{
> +	struct rave_sp *sp = *(struct rave_sp **)res;
> +
> +	debugfs_remove_recursive(sp->debugfs);
> +}
> +
> +static int devm_rave_sp_debugfs_create(struct rave_sp *sp)
> +{
> +	struct rave_sp **rcsp;
> +	struct device *dev = &sp->serdev->dev;
> +	int ret;
> +
> +	rcsp = devres_alloc(rave_sp_degugfs_release, sizeof(*rcsp), GFP_KERNEL);
> +	if (!rcsp)
> +		return -ENOMEM;
> +
> +	ret = rave_sp_debugfs_create(sp);
> +	if (!ret) {
> +		*rcsp = sp;
> +		devres_add(dev, rcsp);
> +	} else {
> +		devres_free(rcsp);
> +	}

You should not care what debugfs is doing, if it is working or not.  So
no need to check here either.

debugfs was written to make it dirt-simple to use, I don't know why
people keep trying to put some error handling around it :)

thanks,

greg k-h