linux-kernel - Re: [PATCH v2] axs_nand - add driver for NAND controller used on Synopsys AXS dev boards

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:	Fri, 4 Apr 2014 11:09:34 -0300
From:	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>
To:	Alexey Brodkin <Alexey.Brodkin@...opsys.com>
Cc:	linux-mtd@...ts.infradead.org,
	Vineet Gupta <Vineet.Gupta1@...opsys.com>,
	Brian Norris <computersforpeace@...il.com>,
	Grant Likely <grant.likely@...aro.org>,
	David Woodhouse <dwmw2@...radead.org>,
	Francois Bedard <Francois.Bedard@...opsys.com>,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2] axs_nand - add driver for NAND controller used on
 Synopsys AXS dev boards

On Apr 04, Alexey Brodkin wrote:
> Signed-off-by: Alexey Brodkin <abrodkin@...opsys.com>
> 

Maybe it would be nice adding some driver description here, so the commit
log actually says something useful about the commit.

[..]
> Reviewed-by: Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>
> 
> +/**
> + * axs_flag_wait_and_reset - Waits until requested flag in INT_STATUS register
> + *              is set by HW and resets it by writing "1" in INT_CLR_STATUS.
> + * @host:	Pointer to private data structure.
> + * @flag:	Bit/flag offset in INT_STATUS register
> + */
> +static void axs_flag_wait_and_reset(struct axs_nand_host *host, int flag)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < AXS_FLAG_WAIT_DELAY * 100; i++) {
> +		unsigned int status = reg_get(host, INT_STATUS);
> +
> +		if (status & (1 << flag)) {
> +			reg_set(host, INT_CLR_STATUS, 1 << flag);
> +			return;
> +		}
> +
> +		udelay(10);
> +	}
> +
> +	/*
> +	 * Since we cannot report this problem any further than
> +	 * axs_nand_{write|read}_buf() letting user know there's a problem.
> +	 */
> +	dev_err(host->dev, "Waited too long (%d s.) for flag/bit %d\n",
> +		AXS_FLAG_WAIT_DELAY, flag);
> +}

Hm... I'm not sure the above is really true.

The NAND core uses the replaceable chip->waitfunc callback to check the
status of issued commands. See for instance:

static int nand_write_oob_std(struct mtd_info *mtd, struct nand_chip *chip,
                              int page)
{
        int status = 0;
        const uint8_t *buf = chip->oob_poi;
        int length = mtd->oobsize;

        chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize, page);
        chip->write_buf(mtd, buf, length);
        /* Send command to program the OOB data */
        chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);

        status = chip->waitfunc(mtd, chip);

        return status & NAND_STATUS_FAIL ? -EIO : 0;
}

On the other side, if you are clearing the flags in axs_flag_wait_and_reset()
it might be a bit hard for you to get this right.

IOW, I'm not saying you *must* do this, but instead suggesting that you take
a look at waitfunc() and see if it helps report a proper error in the
read/write path.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
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/