linux-kernel - Re: [PATCH v1 24/43] mtd: nand: add support for ts72xx

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZHugqa697eGqHnr4@surfacebook>
Date:   Sat, 3 Jun 2023 23:20:57 +0300
From:   andy.shevchenko@...il.com
To:     Nikita Shubin <nikita.shubin@...uefel.me>
Cc:     Alexander Sverdlin <alexander.sverdlin@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        Linus Walleij <linus.walleij@...aro.org>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Christophe Kerello <christophe.kerello@...s.st.com>,
        Jean Delvare <jdelvare@...e.de>,
        Liang Yang <liang.yang@...ogic.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Michael Peters <mpeters@...eddedts.com>,
        Kris Bahnsen <kris@...eddedts.com>,
        linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org
Subject: Re: [PATCH v1 24/43] mtd: nand: add support for ts72xx

Thu, Jun 01, 2023 at 08:45:29AM +0300, Nikita Shubin kirjoitti:
> Technologic Systems has it's own nand controller implementation in CPLD.
> 
> This patch adds support for TS-72XX boards family.

Use imperative mode, this is documented in the Submitting Patches,

...

> +/*
> + * Technologic Systems TS72xx NAND controller driver
> + *
> + * Copyright (C) 2023 Nikita Shubin <nikita.shubin@...uefel.me>
> + *
> + * derived: plat_nand.c

Derived from:

> + *  Author: Vitaly Wool <vitalywool@...il.com>
> + */

...

> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>

+ Blank line?

> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/platnand.h>

...

> +		bits = readb(addr) & ~0x07;

GENMASK()?

...

> +	addr += (1 << TS72XX_NAND_BUSY_ADDR_LINE);

BIT() ?

...

> +	return !!(readb(addr) & 0x20);

BIT() ?

...

> +	struct ts72xx_nand_data *data;
> +	struct mtd_info *mtd;
> +	int err = 0;

Redundant assignment.

> +	/* Allocate memory for the device structure (and zero it) */
> +	data = devm_kzalloc(&pdev->dev, sizeof(struct ts72xx_nand_data),

sizeof(*data) and make it a single line.

> +			    GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;

...

> +	nand_set_flash_node(&data->chip, pdev->dev.of_node);

Hmm... wondering why this uses OF node instead of fwnode... But okay, this is
question to the subsystem maintaners.


> +	err = mtd_device_parse_register(mtd, NULL, NULL,
> +					NULL, 0);

There is plenty of space on the previous line.

> +

Redundant blank line.

> +	if (!err)
> +		return err;
> +
> +	nand_cleanup(&data->chip);
> +
> +	return 0;

This seems at least weird and rather broken.
To me it looks like

	if (err) {
		nand_cleanup(&data->chip);
		return err;
	}

	return 0;

has to be here.

> +}

...

> +	ret = mtd_device_unregister(nand_to_mtd(chip));
> +	WARN_ON(ret);

WARN_ON()?! Why?

> +	nand_cleanup(chip);
> +}

-- 
With Best Regards,
Andy Shevchenko