From: Richard Weinberger <richard@nod.at>
To: Ben Shelton <ben.shelton@ni.com>
Cc: punnaiah.choudary.kalluri@xilinx.com, dwmw2@infradead.org,
computersforpeace@gmail.com, linux-mtd@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] mtd: nand: Add on-die ECC support
Date: Tue, 28 Apr 2015 00:19:16 +0200 [thread overview]
Message-ID: <553EB5E4.3050309@nod.at> (raw)
In-Reply-To: <20150427213558.GA22780@bshelton-desktop>
Am 27.04.2015 um 23:35 schrieb Ben Shelton:
> I tested this against the latest version of the PL353 NAND driver that Punnaiah
> has been working to upstream (copying her on this message). With a few changes
> to that driver, I got it most of the way through initialization with on-die ECC
> enabled, but it segfaults here with a null pointer dereference because the
> PL353 driver does not implement chip->cmd_ctrl. Instead, it implements a
> custom override of cmd->cmdfunc that does not call cmd_ctrl. Looking through
> the other in-tree NAND drivers, it looks like most of them do implement
> cmd_ctrl, but quite a few of them do not (e.g. au1550nd, denali, docg4).
>
> What do you think would be the best way to handle this? It seems like this gap
> could be bridged from either side -- either the PL353 driver could implement
> cmd_ctrl, at least as a stub version that provides the expected behavior in
> this case; or the on-die framework could break this out into a callback
> function with a default implementation that the driver could override to
> perform this behavior in the manner of its choosing.
Oh, I thought every driver has to implement that function. ;-\
But you're right there is a corner case.
What we could do is just using chip->cmdfunc() with a custom NAND command.
i.e. chip->cmdfunc(mtd, NAND_CMD_READMODE, -1, -1);
Gerhard Sittig tried to introduce such a command some time ago:
http://lists.infradead.org/pipermail/linux-mtd/2014-April/053115.html
Maybe Brian can bring some light into that too...
> When I build this without CONFIG_MTD_NAND_ECC_ON_DIE enabled, I get the
> following warning here:
>
> In file included from drivers/mtd/nand/nand_base.c:46:0:
> include/linux/mtd/nand_ondie.h: In function 'nand_read_subpage_on_die':
> include/linux/mtd/nand_ondie.h:28:1: warning: no return statement in function returning non-void [-Wreturn-type]
> include/linux/mtd/nand_ondie.h: In function 'nand_read_page_on_die':
> include/linux/mtd/nand_ondie.h:34:1: warning: no return statement in function returning non-void [-Wreturn-type]
>
> Perhaps return an error code here, even though you'll never get past the BUG()?
What gcc is this?
gcc 4.8 here does not warn, I thought it is smart enough that this function does never
return. Can it be that your .config has CONFIG_BUG=n?
Anyway, this functions clearly needs a return statement. :)
Thanks,
//richard
WARNING: multiple messages have this Message-ID (diff)
From: Richard Weinberger <richard@nod.at>
To: Ben Shelton <ben.shelton@ni.com>
Cc: linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
dwmw2@infradead.org, computersforpeace@gmail.com,
punnaiah.choudary.kalluri@xilinx.com
Subject: Re: [PATCH 1/3] mtd: nand: Add on-die ECC support
Date: Tue, 28 Apr 2015 00:19:16 +0200 [thread overview]
Message-ID: <553EB5E4.3050309@nod.at> (raw)
In-Reply-To: <20150427213558.GA22780@bshelton-desktop>
Am 27.04.2015 um 23:35 schrieb Ben Shelton:
> I tested this against the latest version of the PL353 NAND driver that Punnaiah
> has been working to upstream (copying her on this message). With a few changes
> to that driver, I got it most of the way through initialization with on-die ECC
> enabled, but it segfaults here with a null pointer dereference because the
> PL353 driver does not implement chip->cmd_ctrl. Instead, it implements a
> custom override of cmd->cmdfunc that does not call cmd_ctrl. Looking through
> the other in-tree NAND drivers, it looks like most of them do implement
> cmd_ctrl, but quite a few of them do not (e.g. au1550nd, denali, docg4).
>
> What do you think would be the best way to handle this? It seems like this gap
> could be bridged from either side -- either the PL353 driver could implement
> cmd_ctrl, at least as a stub version that provides the expected behavior in
> this case; or the on-die framework could break this out into a callback
> function with a default implementation that the driver could override to
> perform this behavior in the manner of its choosing.
Oh, I thought every driver has to implement that function. ;-\
But you're right there is a corner case.
What we could do is just using chip->cmdfunc() with a custom NAND command.
i.e. chip->cmdfunc(mtd, NAND_CMD_READMODE, -1, -1);
Gerhard Sittig tried to introduce such a command some time ago:
http://lists.infradead.org/pipermail/linux-mtd/2014-April/053115.html
Maybe Brian can bring some light into that too...
> When I build this without CONFIG_MTD_NAND_ECC_ON_DIE enabled, I get the
> following warning here:
>
> In file included from drivers/mtd/nand/nand_base.c:46:0:
> include/linux/mtd/nand_ondie.h: In function 'nand_read_subpage_on_die':
> include/linux/mtd/nand_ondie.h:28:1: warning: no return statement in function returning non-void [-Wreturn-type]
> include/linux/mtd/nand_ondie.h: In function 'nand_read_page_on_die':
> include/linux/mtd/nand_ondie.h:34:1: warning: no return statement in function returning non-void [-Wreturn-type]
>
> Perhaps return an error code here, even though you'll never get past the BUG()?
What gcc is this?
gcc 4.8 here does not warn, I thought it is smart enough that this function does never
return. Can it be that your .config has CONFIG_BUG=n?
Anyway, this functions clearly needs a return statement. :)
Thanks,
//richard
next prev parent reply other threads:[~2015-04-27 22:19 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-25 14:02 [RFC] On-die ECC support Richard Weinberger
2015-03-25 14:02 ` Richard Weinberger
2015-03-25 14:02 ` [PATCH 1/3] mtd: nand: Add on-die " Richard Weinberger
2015-03-25 14:02 ` Richard Weinberger
2015-03-25 20:39 ` Paul Bolle
2015-03-25 20:39 ` Paul Bolle
2015-04-27 21:35 ` Ben Shelton
2015-04-27 21:35 ` Ben Shelton
2015-04-27 22:19 ` Richard Weinberger [this message]
2015-04-27 22:19 ` Richard Weinberger
2015-04-27 22:36 ` Ben Shelton
2015-04-27 22:36 ` Ben Shelton
2015-04-27 22:42 ` Richard Weinberger
2015-04-27 22:42 ` Richard Weinberger
2015-04-27 22:53 ` Brian Norris
2015-04-27 22:53 ` Brian Norris
2015-04-27 22:57 ` Richard Weinberger
2015-04-27 22:57 ` Richard Weinberger
2015-04-27 23:10 ` Brian Norris
2015-04-27 23:10 ` Brian Norris
2015-04-27 23:15 ` Richard Weinberger
2015-04-27 23:15 ` Richard Weinberger
2015-04-27 23:19 ` Brian Norris
2015-04-27 23:19 ` Brian Norris
2015-04-27 23:23 ` Brian Norris
2015-04-27 23:23 ` Brian Norris
2015-04-28 2:48 ` punnaiah choudary kalluri
2015-04-28 2:48 ` punnaiah choudary kalluri
2015-04-28 3:22 ` Brian Norris
2015-04-28 3:22 ` Brian Norris
2015-04-28 3:44 ` punnaiah choudary kalluri
2015-04-28 3:44 ` punnaiah choudary kalluri
2015-04-28 14:03 ` Josh Cartwright
2015-04-28 14:03 ` Josh Cartwright
2015-04-28 16:19 ` punnaiah choudary kalluri
2015-04-28 16:19 ` punnaiah choudary kalluri
2015-05-08 21:26 ` Ben Shelton
2015-05-08 21:26 ` Ben Shelton
2015-05-08 21:39 ` Brian Norris
2015-05-08 21:39 ` Brian Norris
2015-05-08 21:43 ` Richard Weinberger
2015-05-08 21:43 ` Richard Weinberger
2015-04-28 3:15 ` punnaiah choudary kalluri
2015-04-28 3:15 ` punnaiah choudary kalluri
2015-03-25 14:02 ` [PATCH 2/3] mtd: nand: Add support for raw access when using on-die ECC Richard Weinberger
2015-03-25 14:02 ` Richard Weinberger
2015-03-25 14:02 ` [PATCH 3/3] mtd: nand: Wire up on-die ECC support Richard Weinberger
2015-03-25 14:02 ` Richard Weinberger
2015-04-21 12:31 ` [RFC] On-die " Richard Weinberger
2015-04-21 12:31 ` Richard Weinberger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=553EB5E4.3050309@nod.at \
--to=richard@nod.at \
--cc=ben.shelton@ni.com \
--cc=computersforpeace@gmail.com \
--cc=dwmw2@infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=punnaiah.choudary.kalluri@xilinx.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.