linux-kernel - Re: [PATCH 1/1] drivers/misc: at2x: use devm_kzalloc() to make cleanup paths simpler

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] [day] [month] [year] [list]

Date:	Tue, 28 May 2013 23:10:39 +0300
From:	Andy Shevchenko <andy.shevchenko@...il.com>
To:	Nikolay Balandin <n.a.balandin@...il.com>
Cc:	Wolfram Sang <wsa@...-dreams.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Bill Pemberton <wfp5p@...ginia.edu>,
	Jingoo Han <jg1.han@...sung.com>,
	Alexandre Pereira da Silva <aletes.xgr@...il.com>,
	Nikolay Balandin <nbalandin@....rtsoft.ru>,
	linux-i2c@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] drivers/misc: at2x: use devm_kzalloc() to make
 cleanup paths simpler

Thanks for updated version.
Please, find few comments below.

On Tue, May 28, 2013 at 10:03 PM, Nikolay Balandin
<n.a.balandin@...il.com> wrote:
> From: Nikolay Balandin <nbalandin@....rtsoft.ru>

Next time you may use --subject-prefix to specify the version of the
patch. This one in particular is "PATCH v2".

I'm pretty sure Greg or any maintainer will ask you to split your
patch to two: one per driver.

> Use devm_kzalloc() instead of kzalloc. Get rid of the "goto" statements
> and useless dev_dbg() calls when driver probe fails.

I think in the subject line is better to write something like
"at24: convert to use devm_kzalloc".
For at25 accordingly.

> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c

struct i2c_device_id *id)
>                 } else if (i2c_check_functionality(client->adapter,
>                                 I2C_FUNC_SMBUS_READ_BYTE_DATA)) {
>                         use_smbus = I2C_SMBUS_BYTE_DATA;
> -               } else {
> -                       err = -EPFNOSUPPORT;
> -                       goto err_out;
> -               }
> +               } else
> +                       return -EPFNOSUPPORT;

It's good to keep the style here untouched, because it follows CodingStyle.
thus
} else {
 return ...
}

> @@ -596,11 +589,11 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>                         at24->write_max = write_max;
>
>                         /* buffer (data + address at the beginning) */
> -                       at24->writebuf = kmalloc(write_max + 2, GFP_KERNEL);
> -                       if (!at24->writebuf) {
> -                               err = -ENOMEM;
> -                               goto err_struct;
> -                       }
> +                       at24->writebuf = devm_kzalloc(&client->dev,
> +                               write_max + 2, GFP_KERNEL);
> +

Redundant empty line

> +                       if (!at24->writebuf)
> +                               return -ENOMEM;
>                 } else {
>                         dev_warn(&client->dev,
>                                 "cannot write due to controller restrictions.");
> @@ -647,12 +640,6 @@ err_clients:
>         for (i = 1; i < num_addresses; i++)
>                 if (at24->client[i])
>                         i2c_unregister_device(at24->client[i]);
> -

No need to remove this empty line.

> @@ -666,9 +653,6 @@ static int at24_remove(struct i2c_client *client)
>
>         for (i = 1; i < at24->num_addresses; i++)
>                 i2c_unregister_device(at24->client[i]);
> -

Ditto.

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