Re: [PATCH RESEND] mac802154: Fix uninit-value access in ieee802154_hdr_push_sechdr
Hi Zhang,
zhang_shur...@foxmail.com wrote on Sat, 2 Dec 2023 22:58:52 +0800:
> The syzkaller reported an issue:
Subject should start with [PATCH wpan]
>
> BUG: KMSAN: uninit-value in ieee802154_hdr_push_sechdr
> net/ieee802154/header_ops.c:54 [inline]
> BUG: KMSAN: uninit-value in ieee802154_hdr_push+0x971/0xb90
> net/ieee802154/header_ops.c:108
> ieee802154_hdr_push_sechdr net/ieee802154/header_ops.c:54 [inline]
> ieee802154_hdr_push+0x971/0xb90 net/ieee802154/header_ops.c:108
> ieee802154_header_create+0x9c0/0xc00 net/mac802154/iface.c:396
> wpan_dev_hard_header include/net/cfg802154.h:494 [inline]
> dgram_sendmsg+0xd1d/0x1500 net/ieee802154/socket.c:677
> ieee802154_sock_sendmsg+0x91/0xc0 net/ieee802154/socket.c:96
> sock_sendmsg_nosec net/socket.c:725 [inline]
> sock_sendmsg net/socket.c:748 [inline]
> sys_sendmsg+0x9c2/0xd60 net/socket.c:2494
> ___sys_sendmsg+0x28d/0x3c0 net/socket.c:2548
> __sys_sendmsg+0x225/0x3c0 net/socket.c:2577
> __compat_sys_sendmsg net/compat.c:346 [inline]
> __do_compat_sys_sendmsg net/compat.c:353 [inline]
> __se_compat_sys_sendmsg net/compat.c:350 [inline]
>
> We found hdr->key_id_mode is uninitialized in mac802154_set_header_security()
> which indicates hdr.fc.security_enabled should be 0. However, it is set to be
> cb->secen before.
> Later, ieee802154_hdr_push_sechdr is invoked, causing KMSAN complains
> uninit-value issue.
I am not too deeply involved in the security header but for me it feels
like your patch does the opposite of what's needed. We should maybe
initialize hdr->key_id_mode based on the value in cb->secen, no? (maybe
Alexander will have a better understanding than I have).
> Since mac802154_set_header_security() sets hdr.fc.security_enabled based on
> the variables
> ieee802154_sub_if_data *sdata and ieee802154_mac_cb *cb in a collaborative
> manner.
> Therefore, we should not set security_enabled prior to
> mac802154_set_header_security().
>
> Fixed it by removing the line that sets the hdr.fc.security_enabled.
>
> Syzkaller don't provide repro, and I provide a syz repro like:
> r0 = syz_init_net_socket$802154_dgram(0x24, 0x2, 0x0)
> setsockopt$WPAN_SECURITY(r0, 0x0, 0x1, &(0x7f00)=0x2, 0x4)
> setsockopt$WPAN_SECURITY(r0, 0x0, 0x1, &(0x7f80), 0x4)
> sendmsg$802154_dgram(r0, &(0x7f000100)={&(0x7f40)={0x24, @short},
> 0x14, &(0x7fc0)={0x0}}, 0x0)
>
> Fixes: 32edc40ae65c ("ieee802154: change _cb handling slightly")
> Signed-off-by: Zhang Shurong
> ---
This is a resend, a message in a shortlog to express why is welcome. If
it's a ping, then no need to resend.
> net/mac802154/iface.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
> index c0e2da5072be..c99b6e40a5db 100644
> --- a/net/mac802154/iface.c
> +++ b/net/mac802154/iface.c
> @@ -368,7 +368,6 @@ static int ieee802154_header_create(struct sk_buff *skb,
>
> memset(, 0, sizeof(hdr.fc));
> hdr.fc.type = cb->type;
> - hdr.fc.security_enabled = cb->secen;
> hdr.fc.ack_request = cb->ackreq;
> hdr.seq = atomic_inc_return(>ieee802154_ptr->dsn) & 0xFF;
>
Thanks,
Miquèl
Re: [PATCH] [v2] ieee802154: ca8210: Fix a potential UAF in ca8210_probe
Hi Dinghao,
dinghao@zju.edu.cn wrote on Tue, 26 Sep 2023 11:22:44 +0800:
> If of_clk_add_provider() fails in ca8210_register_ext_clock(),
> it calls clk_unregister() to release priv->clk and returns an
> error. However, the caller ca8210_probe() then calls ca8210_remove(),
> where priv->clk is freed again in ca8210_unregister_ext_clock(). In
> this case, a use-after-free may happen in the second time we call
> clk_unregister().
>
> Fix this by removing the first clk_unregister(). Also, priv->clk could
> be an error code on failure of clk_register_fixed_rate(). Use
> IS_ERR_OR_NULL to catch this case in ca8210_unregister_ext_clock().
>
> Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
Missing Cc stable, this needs to be backported.
> Signed-off-by: Dinghao Liu
> ---
>
> Changelog:
>
> v2: -Remove the first clk_unregister() instead of nulling priv->clk.
> ---
> drivers/net/ieee802154/ca8210.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
> index aebb19f1b3a4..b35c6f59bd1a 100644
> --- a/drivers/net/ieee802154/ca8210.c
> +++ b/drivers/net/ieee802154/ca8210.c
> @@ -2759,7 +2759,6 @@ static int ca8210_register_ext_clock(struct spi_device
> *spi)
> }
> ret = of_clk_add_provider(np, of_clk_src_simple_get, priv->clk);
> if (ret) {
> - clk_unregister(priv->clk);
> dev_crit(
> >dev,
> "Failed to register external clock as clock provider\n"
I was hoping you would simplify this function a bit more.
> @@ -2780,7 +2779,7 @@ static void ca8210_unregister_ext_clock(struct
> spi_device *spi)
> {
> struct ca8210_priv *priv = spi_get_drvdata(spi);
>
> - if (!priv->clk)
> + if (IS_ERR_OR_NULL(priv->clk))
> return
>
> of_clk_del_provider(spi->dev.of_node);
Alex, Stefan, who handles wpan and wpan/next this release?
Thanks,
Miquèl
Re: [PATCH] ieee802154: ca8210: Fix a potential UAF in ca8210_probe
Hi Dinghao,
dinghao@zju.edu.cn wrote on Mon, 25 Sep 2023 15:24:22 +0800:
> If of_clk_add_provider() fails in ca8210_register_ext_clock(),
> it calls clk_unregister() to release priv->clk and returns an
> error. However, the caller ca8210_probe() then calls ca8210_remove(),
> where priv->clk is freed again in ca8210_unregister_ext_clock(). In
> this case, a use-after-free may happen in the second time we call
> clk_unregister().
>
> Fix this by nulling priv->clk after the first clk_unregister(). Also
> refine the pointer checking in ca8210_unregister_ext_clock().
>
> Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
> Signed-off-by: Dinghao Liu
> ---
> drivers/net/ieee802154/ca8210.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
> index aebb19f1b3a4..1d545879c000 100644
> --- a/drivers/net/ieee802154/ca8210.c
> +++ b/drivers/net/ieee802154/ca8210.c
> @@ -2760,6 +2760,7 @@ static int ca8210_register_ext_clock(struct spi_device
> *spi)
> ret = of_clk_add_provider(np, of_clk_src_simple_get, priv->clk);
> if (ret) {
> clk_unregister(priv->clk);
> + priv->clk = NULL;
This function is a bit convoluted. You could just return the result of
of_clk_add_provider() (keep the printk's if you want, they don't seem
very useful) and let ca8210_unregister_ext_clock() do the cleanup.
> dev_crit(
> >dev,
> "Failed to register external clock as clock provider\n"
> @@ -2780,7 +2781,7 @@ static void ca8210_unregister_ext_clock(struct
> spi_device *spi)
> {
> struct ca8210_priv *priv = spi_get_drvdata(spi);
>
> - if (!priv->clk)
> + if (IS_ERR_OR_NULL(priv->clk))
Does not look useful as you are enforcing priv->clock to be valid or
NULL, it cannot be an error code.
Thanks,
Miquèl
Re: [PATCH] mtd: rawnand: fix an error code in nand_setup_interface()
Hi Dan,
Dan Carpenter wrote on Sat, 17 Apr 2021
13:24:26 +0300:
> On Fri, Apr 16, 2021 at 05:00:40PM +0200, Miquel Raynal wrote:
> > Hi Dan,
> >
> > Dan Carpenter wrote on Wed, 14 Apr 2021
> > 08:56:33 +0300:
> >
> > > We should return an error code if the timing mode is not acknowledged
> > > by the NAND chip.
> >
> > This truly is questionable (and I am not yet decided whether the answer
> > should be yes or no).
> >
> > Returning an error here would produce the entire boot sequence to fail,
> > even though the NAND chip would work in mode 0.
> >
> > Not returning an error would print the below warning (so the
> > user/developer is warned) and continue the boot with the slowest
> > timing interface.
> >
> > Honestly I would be more in favor of letting things as they are
> > because I don't think this may be considered as a buggy situation, but I
> > am open to discussion.
> >
>
> If we decided that the original code is correct then one way to silence
> the warning would be to do:
>
> if (tmode_param[0] != chip->best_interface_config->timings.mode) {
> pr_warn("timing mode %d not acknowledged by the NAND chip\n",
> chip->best_interface_config->timings.mode);
> ret = 0;
> goto err_reset_chip;
> }
>
> Setting "ret = 0;" right before the goto makes the code look more
> intentional to human readers as well.
Absolutely right. Let's got for it then.
Cheers,
Miquèl
Re: [PATCH] mtd: core: Constify buf in mtd_write_user_prot_reg()
Hi Tudor,
Tudor Ambarus wrote on Sat, 3 Apr 2021
09:09:31 +0300:
> The write buffer comes from user and should be const.
> Constify write buffer in mtd core and across all _write_user_prot_reg()
> users. cfi_cmdset_{0001, 0002} and onenand_base will pay the cost of an
> explicit cast to discard the const qualifier since the beginning, since
> they are using an otp_op_t function prototype that is used for both reads
> and writes. mtd_dataflash and SPI NOR will benefit of the const buffer
> because they are using different paths for writes and reads.
>
> Signed-off-by: Tudor Ambarus
Applied on top of mtd/next after the merge of all our internal PR's.
Thanks,
Miquèl
Re: [PATCH] mtd: rawnand: fix an error code in nand_setup_interface()
Hi Dan,
Dan Carpenter wrote on Wed, 14 Apr 2021
08:56:33 +0300:
> We should return an error code if the timing mode is not acknowledged
> by the NAND chip.
This truly is questionable (and I am not yet decided whether the answer
should be yes or no).
Returning an error here would produce the entire boot sequence to fail,
even though the NAND chip would work in mode 0.
Not returning an error would print the below warning (so the
user/developer is warned) and continue the boot with the slowest
timing interface.
Honestly I would be more in favor of letting things as they are
because I don't think this may be considered as a buggy situation, but I
am open to discussion.
> Fixes: 415ae78ffb5d ("mtd: rawnand: check ONFI timings have been acked by the
> chip")
> Signed-off-by: Dan Carpenter
> ---
> From static analysis. Not tested.
>
> drivers/mtd/nand/raw/nand_base.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c
> b/drivers/mtd/nand/raw/nand_base.c
> index fb072c95..d83c0503f96f 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -880,6 +880,7 @@ static int nand_setup_interface(struct nand_chip *chip,
> int chipnr)
> if (tmode_param[0] != chip->best_interface_config->timings.mode) {
> pr_warn("timing mode %d not acknowledged by the NAND chip\n",
> chip->best_interface_config->timings.mode);
> + ret = -EINVAL;
> goto err_reset_chip;
> }
>
Thanks,
Miquèl
Re: [PATCH v2] mtd: spinand: add support for Foresee FS35ND01G-S1Y2
Hi Daniel, Daniel Palmer wrote on Wed, 7 Apr 2021 21:01:01 +0900: > Hi Miquel, > > On Wed, 7 Apr 2021 at 17:02, Miquel Raynal wrote: > > You may look at micron_8_ecc_get_status() helper to guide you. But > > IMHO, if there are 0-3 bf, you should probably assume there were 3 bf > > and return 3, if there were 4, return 4, if it's uncorrectable return > > -EBADMSG otherwise -EINVAL. > > Understood. > > > We should verify that this does not mess with UBI wear leveling > > though. Please check that returning 3-bit errors no matter the > > actual number of flipped bits does not lead UBI to move the data away > > (I think it's fine but we need to be sure otherwise the implementation > > proposal is not valid). > > Ok. I'm not sure how to check that yet but I'll look into it. > You can probably check the threshold in sysfs (/sys/class/mtd/mtdX/*threshold*). Thanks, Miquèl
Re: [PATCH V2] mtd: phram: Fix error return code in phram_setup()
Hi Yu,
Yu Kuai wrote on Thu, 8 Apr 2021 21:38:12 +0800:
> Return a negative error code from the error handling case instead
> of 0, as done elsewhere in this function.
>
> Reported-by: Hulk Robot
> Signed-off-by: Yu Kuai
> ---
> drivers/mtd/devices/phram.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index 5b04ae6c3057..6ed6c51fac69 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -270,6 +270,7 @@ static int phram_setup(const char *val)
> if (len == 0 || erasesize == 0 || erasesize > len
> || erasesize > UINT_MAX || rem) {
> parse_err("illegal erasesize or len\n");
> + ret = -EINVAL;
> goto error;
> }
>
Actually I don't know why but I overlooked the change. I thought you
were removing the ret = line, sorry about that. Anyway, I prefer
the new wording so I'll apply the v2.
Thanks,
Miquèl
Re: [PATCH 3/3] mtd: phram: Fix error return code in phram_setup()
Hi Yu,
Yu Kuai wrote on Thu, 8 Apr 2021 19:15:14 +0800:
> Fix to return a negative error code from the error handling
> case instead of 0, as done elsewhere in this function.
>
> Reported-by: Hulk Robot
> Signed-off-by: Yu Kuai
> ---
> drivers/mtd/devices/phram.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index 5b04ae6c3057..6ed6c51fac69 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -270,6 +270,7 @@ static int phram_setup(const char *val)
> if (len == 0 || erasesize == 0 || erasesize > len
> || erasesize > UINT_MAX || rem) {
> parse_err("illegal erasesize or len\n");
> + ret = -EINVAL;
> goto error;
> }
>
It looks like you're doing the opposite of what you say.
Thanks,
Miquèl
Re: [PATCH] mtd: add OTP (one-time-programmable) erase ioctl
Hello, Michael Walle wrote on Thu, 08 Apr 2021 08:55:42 +0200: > Hi Tudor, > > Am 2021-04-08 07:51, schrieb tudor.amba...@microchip.com: > > Would you please resend this patch, together with the mtd-utils > > and the SPI NOR patch in a single patch set? You'll help us all > > having all in a single place. > > This has already been picked-up: > https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/commit/?h=mtd/next=e3c1f1c92d6ede3cfa09d6a103d3d1c1ef645e35 > > Although, I didn't receive an email notice. > > -michael Sometimes the notifications are not triggered when there is a conflict when applying the patch from patchwork directly. I usually answer manually in this case but I might have forgotten. About the patch, I felt it was good enough for merging, and I want to avoid applying such patches right before freezing our branches. Hence, I tend to be more aggressive earlier in the release cycles because I hate when my patches get delayed infinitely. The other side is a more careful approach when -rc6 gets tagged so that I can drop anything which would be crazily broken before our -next branches are stalled, leading for an useless public revert. Of course, I am fully open to removing this patch from -next if you ever feel it was too early and will happily get rid of it for this release: we can move the patch for the next release if you agree on this (especially since it touches the ABI). Cheers, Miquèl
Re: [PATCH v11 1/4] dt-bindings: mtd: Convert Qcom NANDc binding to YAML
On Fri, 2021-04-02 at 15:01:25 UTC, Manivannan Sadhasivam wrote: > Convert Qcom NANDc devicetree binding to YAML. > > Signed-off-by: Manivannan Sadhasivam > Reviewed-by: Rob Herring Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
Re: [PATCH v11 2/4] dt-bindings: mtd: Add a property to declare secure regions in NAND chips
On Fri, 2021-04-02 at 15:01:26 UTC, Manivannan Sadhasivam wrote: > On a typical end product, a vendor may choose to secure some regions in > the NAND memory which are supposed to stay intact between FW upgrades. > The access to those regions will be blocked by a secure element like > Trustzone. So the normal world software like Linux kernel should not > touch these regions (including reading). > > So let's add a property for declaring such secure regions so that the > drivers can skip touching them. > > Reviewed-by: Rob Herring > Signed-off-by: Manivannan Sadhasivam Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
Re: [PATCH v11 3/4] mtd: rawnand: Add support for secure regions in NAND memory
On Fri, 2021-04-02 at 15:01:27 UTC, Manivannan Sadhasivam wrote: > On a typical end product, a vendor may choose to secure some regions in > the NAND memory which are supposed to stay intact between FW upgrades. > The access to those regions will be blocked by a secure element like > Trustzone. So the normal world software like Linux kernel should not > touch these regions (including reading). > > The regions are declared using a NAND chip DT property, > "secure-regions". So let's make use of this property in the raw NAND > core and skip access to the secure regions present in a system. > > Signed-off-by: Manivannan Sadhasivam Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
Re: [PATCH v11 4/4] mtd: rawnand: qcom: Add missing nand_cleanup() in error path
On Fri, 2021-04-02 at 15:01:28 UTC, Manivannan Sadhasivam wrote: > Add missing nand_cleanup() in the alloc_bam_transaction() error path > to cleanup the resources properly. > > Signed-off-by: Manivannan Sadhasivam Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
Re: [PATCH] mtd: rawnand: qcom: Use dma_mapping_error() for error check
On Mon, 2021-04-05 at 05:09:12 UTC, Manivannan Sadhasivam wrote: > dma_mapping_error() should be used for checking the error value of > dma_map_resource() API. > > Signed-off-by: Manivannan Sadhasivam Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
Re: [PATCH] mtd: nand: gpmi: Fix a double free in gpmi_nand_init
On Sat, 2021-04-03 at 06:09:05 UTC, Lv Yunlong wrote:
> If the callee gpmi_alloc_dma_buffer() failed to alloc memory for
> this->raw_buffer, gpmi_free_dma_buffer() will be called to free
> this->auxiliary_virt. But this->auxiliary_virt is still a non-NULL
> and valid ptr.
>
> Then gpmi_alloc_dma_buffer() returns err and gpmi_free_dma_buffer()
> is called again to free this->auxiliary_virt in err_out. This causes
> a double free.
>
> As gpmi_free_dma_buffer() has already called in gpmi_alloc_dma_buffer's
> error path, so it should return err directly instead of releasing the dma
> buffer again.
>
> Fixes: 4d02423e9afe6 ("mtd: nand: gpmi: Fix gpmi_nand_init() error path")
> Signed-off-by: Lv Yunlong
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git
nand/next, thanks.
Miquel
Re: [PATCH v2 2/2] Fix the issue for clearing status process
Hi Yoshio,
Yoshio Furuyama wrote on Tue, 6 Apr
2021 10:47:26 +0900:
Could you add "mtd: nand: bbt:" as prefix for the title (same for the
other patch, even though you're not the original author).
> In the unlikely event of bad block,
> it should update its block status to BBT,
> In this case, there are 2 kind of issue for handling
> a) Mark bad block status to BBT: It was fixed by Patric's patch
> b) Clear status to BBT: I posted patch for this issue
>
> Patch:
> Issue of handing BBT (Bad Block Table) for
> some particular blocks (Ex:10, 11)
> Updating status is, first clear status, second set bad block status.
> Patrick's patch is only fixed the issue for setting status process,
> so this patch fix the clearing status process.
This commit message is not clearly describing the situation, could you
please reword it?
>
> Signed-off-by: Yoshio Furuyama
> ---
> drivers/mtd/nand/bbt.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/bbt.c b/drivers/mtd/nand/bbt.c
> index 64af6898131d..0780896eaafe 100644
> --- a/drivers/mtd/nand/bbt.c
> +++ b/drivers/mtd/nand/bbt.c
> @@ -112,11 +112,13 @@ int nanddev_bbt_set_block_status(struct nand_device
> *nand, unsigned int entry,
>((entry * bits_per_block) / BITS_PER_LONG);
> unsigned int offs = (entry * bits_per_block) % BITS_PER_LONG;
> unsigned long val = status & GENMASK(bits_per_block - 1, 0);
> + unsigned long shift = ((bits_per_block + offs <= BITS_PER_LONG) ?
> + (offs + bits_per_block - 1) :
> (BITS_PER_LONG - 1));
Given the fact that we do arithmetic operations (&, |) on an unsigned
long value I don't think the operation tampers with the next entry in
the pos array.
I'm fine fixing it but I don't think this implementation works. It is
fine if offs is 29 or 30 but not if it is 31 (assuming 32-bits
arithmetic, it's the same for the 64-bit case).
>
> if (entry >= nanddev_neraseblocks(nand))
> return -ERANGE;
>
> - pos[0] &= ~GENMASK(offs + bits_per_block - 1, offs);
Would something like the following work?
pos[0] &= ~GENMASK(MIN(offs + bits_per_block - 1, BITS_PER_LONG - 1),
offs)
Again, I am not convinced it is worth darkening the logic unless I am
not understanding it correctly.
> + pos[0] &= ~GENMASK(shift, offs);
> pos[0] |= val << offs;
>
> if (bits_per_block + offs > BITS_PER_LONG) {
Thanks,
Miquèl
Re: [PATCH v2] mtd: spinand: add support for Foresee FS35ND01G-S1Y2
Hi Daniel, Daniel Palmer wrote on Fri, 26 Mar 2021 23:09:28 +0900: > Hi Miquel, > > Sorry for the constant pestering on this.. > > On Tue, 23 Mar 2021 at 23:06, Miquel Raynal wrote: > > > # nandbiterrs -i /dev/mtd1 > > > incremental biterrors test > > > Successfully corrected 0 bit errors per subpage > > > Inserted biterror @ 0/5 > > > Read reported 4 corrected bit errors > > > ECC failure, invalid data despite read success > > > > This is not a valid behavior. There is something wrong with the way ECC > > status is read/retrieved. The read should indeed report 4 corrected bit > > errors, but then the data should be valid. Here it means that the > > introduced error appears corrected but in fact is not. > > We need to understand what status are available and write the > > appropriate vendor code. > > I looked at the datasheet again and the encoding for ecc status seems > a bit funky. > The chip seems to have only two bits for ECC status with this encoding: > 0 - Read success with 0-3 flipped bits. > 1 - Read success with 4 flipped bits. > 2 - Uncorrectable. > 3 - Reserved. > Very nice information. > This looks almost the same as the Winbond chips with 0 and 1 being successes > but with no totally error free status. > > Anyhow, if 4 bits were corrected is returned for 1 then nandbiterrs > reports "ECC failure, invalid data despite read success". > If I return 3 bit errors for 0 and -EBADMSG for anything else > nandbiterrs doesn't complain anymore but is this right?: You may look at micron_8_ecc_get_status() helper to guide you. But IMHO, if there are 0-3 bf, you should probably assume there were 3 bf and return 3, if there were 4, return 4, if it's uncorrectable return -EBADMSG otherwise -EINVAL. We should verify that this does not mess with UBI wear leveling though. Please check that returning 3-bit errors no matter the actual number of flipped bits does not lead UBI to move the data away (I think it's fine but we need to be sure otherwise the implementation proposal is not valid). Thanks, Miquèl
Re: [PATCH] mtd: rawnand: nand_bbt: Skip bad blocks when searching for the BBT in NAND
On Thu, 2021-03-25 at 10:23:37 UTC, Stefan Riedmueller wrote: > The blocks containing the bad block table can become bad as well. So > make sure to skip any blocks that are marked bad when searching for the > bad block table. > > Otherwise in very rare cases where two BBT blocks wear out it might > happen that an obsolete BBT is used instead of a newer available > version. > > Signed-off-by: Stefan Riedmueller Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
Re: [PATCH] mtd: require write permissions for locking and badblock ioctls
On Wed, 2021-03-03 at 15:57:35 UTC, Michael Walle wrote:
> MEMLOCK, MEMUNLOCK and OTPLOCK modify protection bits. Thus require
> write permission. Depending on the hardware MEMLOCK might even be
> write-once, e.g. for SPI-NOR flashes with their WP# tied to GND. OTPLOCK
> is always write-once.
>
> MEMSETBADBLOCK modifies the bad block table.
>
> Fixes: f7e6b19bc764 ("mtd: properly check all write ioctls for permissions")
> Signed-off-by: Michael Walle
> Reviewed-by: Greg Kroah-Hartman
> Acked-by: Rafał Miłecki
> Acked-by: Richard Weinberger
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git
mtd/next, thanks.
Miquel
Re: [PATCH v7 1/3] mtd: core: add nvmem-cells compatible to parse mtd as nvmem cells
On Fri, 2021-03-12 at 06:28:19 UTC, Ansuel Smith wrote: > Partitions that contains the nvmem-cells compatible will register > their direct subonodes as nvmem cells and the node will be treated as a > nvmem provider. > > Signed-off-by: Ansuel Smith > Tested-by: Rafał Miłecki Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH v7 2/3] devicetree: nvmem: nvmem: drop $nodename restriction
On Fri, 2021-03-12 at 06:28:20 UTC, Ansuel Smith wrote: > Drop $nodename restriction as now mtd partition can also be used as > nvmem provider. > > Signed-off-by: Ansuel Smith > Reviewed-by: Rob Herring Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH v7 3/3] dt-bindings: mtd: Document use of nvmem-cells compatible
On Fri, 2021-03-12 at 06:28:21 UTC, Ansuel Smith wrote: > Document nvmem-cells compatible used to treat mtd partitions as a > nvmem provider. > > Signed-off-by: Ansuel Smith > Reviewed-by: Rob Herring Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH] include: linux: mtd: Remove duplicate include of nand.h
On Tue, 2021-03-23 at 03:17:37 UTC, Wan Jiabing wrote: > linux/mtd/nand.h has been included at line 17. > So we remove the duplicate one at line 21. > > Signed-off-by: Wan Jiabing Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH] mtd: rawnand: atmel: Update ecc_stats.corrected counter
On Mon, 2021-03-22 at 15:07:14 UTC, Tudor Ambarus wrote:
> From: "Kai Stuhlemmer (ebee Engineering)"
>
> Update MTD ECC statistics with the number of corrected bits.
>
> Fixes: f88fc122cc34 ("mtd: nand: Cleanup/rework the atmel_nand driver")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Kai Stuhlemmer (ebee Engineering)
> Signed-off-by: Tudor Ambarus
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git
nand/next, thanks.
Miquel
Re: [PATCH 1/2] mtd: rawnand: brcmnand: read/write oob during EDU transfer
On Thu, 2021-03-11 at 17:09:08 UTC, Kamal Dasu wrote: > Added support to read/write oob during EDU transfers. > > Signed-off-by: Kamal Dasu Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
Re: [PATCH 2/2] mtd: rawnand: brcmnand: move to polling in pio mode on oops write
On Thu, 2021-03-11 at 17:09:09 UTC, Kamal Dasu wrote: > This change makes sure that Broadcom NAND driver moves to interrupt > polling on the first brcmnand_write() call. > > Signed-off-by: Kamal Dasu Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
Re: [PATCH] mtd:rawnand: remove duplicate include in rawnand.h
On Sat, 2021-03-13 at 10:57:02 UTC, menglong8.d...@gmail.com wrote: > From: Zhang Yunkai > > 'linux/mtd/nand.h' included in 'rawnand.h' is duplicated. > It is also included in the 17th line. > > Signed-off-by: Zhang Yunkai Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
Re: [PATCH -next] mtd: rawnand: rockchip: Use flexible-array member instead of zero-length array
On Tue, 2021-03-23 at 13:11:37 UTC, Zou Wei wrote: > Suppresses the following coccinelle warning: > > drivers/mtd/nand/raw/rockchip-nand-controller.c:162:4-8: WARNING use > flexible-array member instead > > Signed-off-by: Zou Wei Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
Re: [PATCH v2 mtd/fixes] mtd: spinand: core: add missing MODULE_DEVICE_TABLE()
On Tue, 2021-03-23 at 17:37:19 UTC, Alexander Lobakin wrote:
> The module misses MODULE_DEVICE_TABLE() for both SPI and OF ID tables
> and thus never autoloads on ID matches.
> Add the missing declarations.
> Present since day-0 of spinand framework introduction.
>
> Fixes: 7529df465248 ("mtd: nand: Add core infrastructure to support SPI
> NANDs")
> Cc: sta...@vger.kernel.org # 4.19+
> Signed-off-by: Alexander Lobakin
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git
nand/next, thanks.
Miquel
Re: [PATCH v8 3/3] mtd: rawnand: Add support for secure regions in NAND memory
Hi Manivannan,
Manivannan Sadhasivam wrote on Tue,
23 Mar 2021 13:09:30 +0530:
> On a typical end product, a vendor may choose to secure some regions in
> the NAND memory which are supposed to stay intact between FW upgrades.
> The access to those regions will be blocked by a secure element like
> Trustzone. So the normal world software like Linux kernel should not
> touch these regions (including reading).
>
> The regions are declared using a NAND chip DT property,
> "secure-regions". So let's make use of this property in the raw NAND
> core and skip access to the secure regions present in a system.
>
> Signed-off-by: Manivannan Sadhasivam
> ---
> drivers/mtd/nand/raw/nand_base.c | 105 +++
> include/linux/mtd/rawnand.h | 14 +
> 2 files changed, 119 insertions(+)
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c
> b/drivers/mtd/nand/raw/nand_base.c
> index c33fa1b1847f..2a990219f498 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -278,11 +278,46 @@ static int nand_block_bad(struct nand_chip *chip,
> loff_t ofs)
> return 0;
> }
>
> +/**
> + * nand_check_secure_region() - Check if the region is secured
> + * @chip: NAND chip object
> + * @offset: Offset of the region to check
> + * @size: Size of the region to check
> + *
> + * Checks if the region is secured by comparing the offset and size with the
> + * list of secure regions obtained from DT. Returns -EIO if the region is
> + * secured else 0.
> + */
> +static int nand_check_secure_region(struct nand_chip *chip, loff_t offset,
> u64 size)
I think I would prefer a boolean return value here, with a rename:
static bool nand_region_is_secured() or
nand_region_is_accessible/reachable/whatever()
then something lik:
if (nand_region_is_secured())
return -EIO;
> +{
> + int i;
> +
> + /* Skip touching the secure regions if present */
> + for (i = 0; i < chip->nr_secure_regions; i++) {
> + const struct nand_secure_region *region =
> >secure_regions[i];
> +
> + if (offset + size < region->offset ||
> + offset >= region->offset + region->size)
I think as-is the condition does not work.
Let's assume we want to check the region { .offset = 1, size = 1 } and
the region { .offset = 2, size = 1 } is reserved. This is:
if ((1 + 1 < 2) /* false */ ||
(1 >= 2 + 1) /* false */)
continue;
return -EIO; /* EIO is returned while the area is valid
*/
> + continue;
> +
Perhaps a dev_dbg() entry here would make sense.
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
[...]
> +static int of_get_nand_secure_regions(struct nand_chip *chip)
> +{
> + struct device_node *dn = nand_get_flash_node(chip);
> + struct property *prop;
> + int length, nr_elem, i, j;
> +
> + prop = of_find_property(dn, "secure-regions", );
> + if (prop) {
I generally prefer the below logic:
if (!prop)
return 0;
Then you earn an indentation level.
> + nr_elem = length / sizeof(u64);
of_property_count_elems_of_size() ?
> + chip->nr_secure_regions = nr_elem / 2;
> +
> + chip->secure_regions = kcalloc(nr_elem,
> sizeof(*chip->secure_regions), GFP_KERNEL);
IIRC ->secure_regions is a structure with lengths and offset, so you
don't want to allocate nr_elem but nr_secure_regions number of
items here.
> + if (!chip->secure_regions)
> + return -ENOMEM;
> +
> + for (i = 0, j = 0; i < chip->nr_secure_regions; i++, j += 2) {
> + of_property_read_u64_index(dn, "secure-regions", j,
> +
> >secure_regions[i].offset);
> + of_property_read_u64_index(dn, "secure-regions", j + 1,
> +
> >secure_regions[i].size);
> + }
> + }
> +
> + return 0;
> +}
> +
> static int rawnand_dt_init(struct nand_chip *chip)
> {
> struct nand_device *nand = mtd_to_nanddev(nand_to_mtd(chip));
> struct device_node *dn = nand_get_flash_node(chip);
> + int ret;
>
> if (!dn)
> return 0;
> @@ -5015,6 +5107,16 @@ static int rawnand_dt_init(struct nand_chip *chip)
> of_get_nand_ecc_user_config(nand);
> of_get_nand_ecc_legacy_user_config(chip);
>
> + /*
> + * Look for secure regions in the NAND chip. These regions are supposed
> + * to be protected by a secure element like Trustzone. So the read/write
> + * accesses to these regions will be blocked in the runtime by this
> + * driver.
> + */
> + ret = of_get_nand_secure_regions(chip);
> + if (!ret)
> + return ret;
I think we can do this initialization pretty much when we want in the
init process as long as
Re: [PATCH v2] mtd: spinand: add support for Foresee FS35ND01G-S1Y2
Hi Daniel, Daniel Palmer wrote on Tue, 23 Mar 2021 20:47:10 +0900: > Hi Miquel, > > On Tue, 23 Mar 2021 at 19:32, Miquel Raynal wrote: > > You can run nandbiterrs -i /dev/mtdX > > > > You'll see if there is ECC correction or not (and its level). > > These are results I get for both of the nandbiterrs tests. > > # nandbiterrs -i /dev/mtd1 > incremental biterrors test > Successfully corrected 0 bit errors per subpage > Inserted biterror @ 0/5 > Read reported 4 corrected bit errors > ECC failure, invalid data despite read success This is not a valid behavior. There is something wrong with the way ECC status is read/retrieved. The read should indeed report 4 corrected bit errors, but then the data should be valid. Here it means that the introduced error appears corrected but in fact is not. We need to understand what status are available and write the appropriate vendor code. Thanks, Miquèl
Re: [PATCH v2] mtd: spinand: add support for Foresee FS35ND01G-S1Y2
Hi Daniel, Daniel Palmer wrote on Tue, 23 Mar 2021 18:33:54 +0900: > Hi Miquel, > > On Tue, 23 Mar 2021 at 03:32, Miquel Raynal wrote: > > > I think this shows that the datasheet is right in that the complete 64 > > > bytes of "spare area" is usable. > > > I have no idea where it puts the ECC though. :) > > > > Argh, I don't like when hardware tries to be smart. > > I'm sort of worried that there just isn't any ECC :D You can run nandbiterrs -i /dev/mtdX You'll see if there is ECC correction or not (and its level). Thanks, Miquèl
Re: [PATCH v2] mtd: spinand: add support for Foresee FS35ND01G-S1Y2
Hi Daniel, Daniel Palmer wrote on Mon, 22 Mar 2021 21:44:40 +0900: > Hi Miquel, > > Sorry for the resend. Gmail randomly switched to HTML email so the > original version seems to have bounced. > > On Mon, 15 Feb 2021 at 20:16, Miquel Raynal wrote: > > > "2. Spare area 800H to 83FH is all available for user. > > > ECC parity codes are programmed in > > > additional space and not user accessible." > > > > > > It would seem that the pages are actually bigger than 2K + 64 or there > > > is some other place they keep the ECC. > > > Or both datasheets are lying. Somewhere else in the datasheets it says > > > that writes to the ECC area will be ignored but that doesn't make a > > > lot of sense if the ECC area isn't user accessible in the first place. > > > > > > I didn't think about it at the time but I can take a dump of the OOB > > > area of my FS35ND01G-S1Y2 to confirm it's all 0xff except for any > > > factory marked bad blocks. > > > > I see. Can you please try the following: > > > > nandwrite -o /dev/mtdx /dev/zero > > nanddump -ol1 /dev/mtdx > > If the entire area is effectively free to be used, you should see 0's > > everywhere. Otherwise you should have ff's somewhere. > > Sorry I didn't follow up sooner on this. I needed to order another of > this flash chip to test with as I couldn't destroy the data on the one > I have. > > Anyhow: > > Erased the page with flash erase (I'm forcing it to erase bad blocks > here as I mess up the marker, I have a hack to allow erasing bad > blocks..) > Everything is 0xFF for that page. > > # flash_erase -N /dev/mtd1 0 1 > Erasing 128 Kibyte @ 0 -- 100 % complete > # nanddump --bb=dumpbad -n -l2048 -o -c -s 0x0 /dev/mtd1 > Block size 131072, page size 2048, OOB size 64 > Dumping data starting at 0x and ending at 0x0800... > 0x: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > || > > 0x07f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > || > OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > || > OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > || > OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > || > OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > || > > Write zeros into the page and OOB. > Get all zeros back including the OOB. > > # nandwrite -o /dev/mtd1 /dev/zero > Writing data to block 0 at offset 0x0 > Bad block at 0, 1 block(s) will be skipped > Writing data to block 1 at offset 0x2 > # nandwrite -N -o /dev/mtd1 /dev/zero > Writing data to block 0 at offset 0x0 > # nanddump --bb=dumpbad -n -l2048 -o -c -s 0x0 /dev/mtd1 > Block size 131072, page size 2048, OOB size 64 > Dumping data starting at 0x and ending at 0x0800... > 0x: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > || > ... > 0x07f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > || > OOB Data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > || > OOB Data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > || > OOB Data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > || > OOB Data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > || > > Erase the page again and writing random junk into it. > Seeing random junk everywhere including the OOB. > > # flash_erase -N /dev/mtd1 0 1 > Erasing 128 Kibyte @ 0 -- 100 % complete > # nandwrite -N -o /dev/mtd1 /dev/urandom > Writing data to [ 230.506260] random: nandwrite: uninitialized > urandom read (2048 bytes read) > block 0 at offse[ 230.514705] random: nandwrite: uninitialized > urandom read (64 bytes read) > t 0x0 > # nanddump --bb=dumpbad -n -l2048 -o -c -s 0x0 /dev/mtd1 > Block size 131072, page size 2048, OOB size 64 > Dumping data starting at 0x and ending at 0x0800... > 0x: 5e 24 bd 5f d9 c6 ce c5 b1 85 52 4d 27 94 c9 98 > |^$._..RM'...| > ... > 0x07f0: fa 9f 7f 7d ce 99 33 88 d6 9f 99 7d 84 e7 0c 4d > |...}..3}...M| > OOB Data: b1 81 07 6a 8d 47 8b ed 89 88 ac 62 e8 ae 48 54 > |...j.G.b..HT| > OOB Data: 7d b2 ea 73 f3 29 ba 65 e6 45 cb 8b 1a c6 5b dc > |}..s.).e.E[.| > OOB Data: b2 2e 77 56 e0 e1 04 59 86 31 7a e5 bd 43 f9 48 > |..wV...Y.1z..C.H| > OOB Data: 52 05 b2 f1 65 64 59 22 79 50 ec 89 55 6b 6e 23 > |R...edY"yP..Ukn#| > > I think this shows that the datasheet is right in that the complete 64 > bytes of "spare area" is usable. > I have no idea where it puts the ECC though. :) Argh, I don't like when hardware tries to be smart. Ok then let's declare no ECC bytes in the OOB layout, I guess it's the best thing to do... Thanks for checking btw! I don't recall the state of the patch which triggered this discussion, so I guess it's a good time to respin. Cheers, Miquèl
Re: [PATCH v1] mtd: nand: Fix BBT update issue
Hi Yoshio,
+ Patrick
Yoshio Furuyama wrote on Tue, 16 Feb
2021 09:37:55 +0900:
> Fixed issue of manages BBT (Bad Block Table).
> It didn't mark correctly when a specific block was bad block.
>
> This issue occurs when the bad block mark (3-bit chunk) is
> crosses over 32 bit (e.g. Block10, Block21...) unit.
>
Thanks for the patch and sorry for the very long wait period, I wanted
to understand better the issue but I didn't had the time to do it.
Would you mind having a look at Patrick's fix merged in U-Boot a year
ago:
commit 06fc4573b9d0878dd1d3b302884601263fe6e85f
Author: Doyle, Patrick
Date: Wed Jul 15 14:46:34 2020 +
Fix corner case in bad block table handling.
In the unlikely event that both blocks 10 and 11 are marked as bad (on a
32 bit machine), then the process of marking block 10 as bad stomps on
cached entry for block 11. There are (of course) other examples.
Signed-off-by: Patrick Doyle
Reviewed-by: Richard Weinberger
diff --git a/drivers/mtd/nand/bbt.c b/drivers/mtd/nand/bbt.c
index 84d60b86521..294daee7b22 100644
--- a/drivers/mtd/nand/bbt.c
+++ b/drivers/mtd/nand/bbt.c
@@ -127,7 +127,7 @@ int nanddev_bbt_set_block_status(struct nand_device *nand,
unsigned int entry,
unsigned int rbits = bits_per_block + offs - BITS_PER_LONG;
pos[1] &= ~GENMASK(rbits - 1, 0);
- pos[1] |= val >> rbits;
+ pos[1] |= val >> (bits_per_block - rbits);
}
return 0;
It looks both patches fix different errors but I wonder if merging Patrick's
patch first would not make more sense.
Ideally, could you please send a series with the two patches, perhaps
Patrick's patch first, then yours, adding a Fixes and Cc: stable tags
to both?
> Signed-off-by: Yoshio Furuyama
> ---
> drivers/mtd/nand/bbt.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/bbt.c b/drivers/mtd/nand/bbt.c
> index 044adf913854..979c47e61381 100644
> --- a/drivers/mtd/nand/bbt.c
> +++ b/drivers/mtd/nand/bbt.c
> @@ -112,18 +112,20 @@ int nanddev_bbt_set_block_status(struct nand_device
> *nand, unsigned int entry,
>((entry * bits_per_block) / BITS_PER_LONG);
> unsigned int offs = (entry * bits_per_block) % BITS_PER_LONG;
> unsigned long val = status & GENMASK(bits_per_block - 1, 0);
> + unsigned long shift = ((bits_per_block + offs <= BITS_PER_LONG) ?
> + (offs + bits_per_block - 1) :
> (BITS_PER_LONG - 1));
>
> if (entry >= nanddev_neraseblocks(nand))
> return -ERANGE;
>
> - pos[0] &= ~GENMASK(offs + bits_per_block - 1, offs);
> + pos[0] &= ~GENMASK(shift, offs);
> pos[0] |= val << offs;
>
> if (bits_per_block + offs > BITS_PER_LONG) {
> unsigned int rbits = bits_per_block + offs - BITS_PER_LONG;
>
> pos[1] &= ~GENMASK(rbits - 1, 0);
> - pos[1] |= val >> rbits;
> + pos[1] |= (val >> (BITS_PER_LONG - offs));
> }
>
> return 0;
Thanks,
Miquèl
Re: [PATCH v5 0/3] Add support for secure regions in NAND
Hi Manivannan, Manivannan Sadhasivam wrote on Wed, 17 Mar 2021 17:55:10 +0530: > On a typical end product, a vendor may choose to secure some regions in > the NAND memory which are supposed to stay intact between FW upgrades. > The access to those regions will be blocked by a secure element like > Trustzone. So the normal world software like Linux kernel should not > touch these regions (including reading). > > So this series adds a property for declaring such secure regions in DT > so that the driver can skip touching them. While at it, the Qcom NANDc > DT binding is also converted to YAML format. > > Thanks, > Mani > > Changes in v5: > > * Switched to "uint64-matrix" as suggested by Rob > * Moved the whole logic from qcom driver to nand core as suggested by Boris I'm really thinking about a nand-wide property now. Do you think it makes sense to move the helper to the NAND core (instead of the raw NAND core)? I'm fine only using it in the raw NAND core though. Also, can I request a global s/sec/secure/ update? I find the "sec" abbreviation unclear and I think we have more than enough cryptic names :-) Thanks, Miquèl
Re: [PATCH] dt-bindings: i3c: Fix silvaco,i3c-master-v1 compatible string
Rob Herring wrote on Thu, 11 Mar 2021 16:40:56 -0700:
> The example for the Silvaco I3C master doesn't match the schema's
> compatible string. Fix it.
>
> Cc: Miquel Raynal
> Cc: Conor Culhane
> Cc: Alexandre Belloni
> Cc: linux-...@lists.infradead.org
> Signed-off-by: Rob Herring
> ---
> Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
> b/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
> index adb5165505aa..62f3ca66274f 100644
> --- a/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
> +++ b/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
> @@ -49,7 +49,7 @@ additionalProperties: true
> examples:
>- |
> i3c-master@a000 {
> -compatible = "silvaco,i3c-master";
> +compatible = "silvaco,i3c-master-v1";
> clocks = <_clk 71>, <>, <>;
> clock-names = "pclk", "fast_clk", "slow_clk";
> interrupt-parent = <>;
Reviewed-by: Miquel Raynal
Re: [PATCH -next] mtd: parsers: ofpart: make symbol 'bcm4908_partitions_quirks' static
On Thu, 2021-03-04 at 06:46:00 UTC, 'Wei Yongjun wrote:
> From: Wei Yongjun
>
> The sparse tool complains as follows:
>
> drivers/mtd/parsers/ofpart_core.c:25:32: warning:
> symbol 'bcm4908_partitions_quirks' was not declared. Should it be static?
>
> This symbol is not used outside of ofpart_core.c, so this
> commit marks it static.
>
> Fixes: 457da931b608 ("mtd: parsers: ofpart: support BCM4908 fixed partitions")
> Reported-by: Hulk Robot
> Signed-off-by: Wei Yongjun
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git
mtd/next, thanks.
Miquel
Re: [PATCH RESEND][next] mtd: onenand: Fix fall-through warnings for Clang
On Fri, 2021-03-05 at 08:23:56 UTC, "Gustavo A. R. Silva" wrote: > In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning > by explicitly adding a break statement instead of letting the code fall > through to the next case. > > Link: https://github.com/KSPP/linux/issues/115 > Signed-off-by: Gustavo A. R. Silva Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH RESEND][next] mtd: rawnand: fsmc: Fix fall-through warnings for Clang
On Fri, 2021-03-05 at 08:25:59 UTC, "Gustavo A. R. Silva" wrote: > In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning > by explicitly adding a break statement instead of letting the code fall > through to the next case. > > Link: https://github.com/KSPP/linux/issues/115 > Signed-off-by: Gustavo A. R. Silva Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH] mtd: maps: fix error return code of physmap_flash_remove()
On Mon, 2021-03-08 at 03:44:46 UTC, Jia-Ju Bai wrote:
> When platform_get_drvdata() returns NULL to info, no error return code
> of physmap_flash_remove() is assigned.
> To fix this bug, err is assigned with -EINVAL in this case
>
> Fixes: 73566edf9b91 ("[MTD] Convert physmap to platform driver")
> Reported-by: TOTE Robot
> Signed-off-by: Jia-Ju Bai
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git
mtd/next, thanks.
Miquel
Re: [PATCH RESEND][next] mtd: cfi: Fix fall-through warnings for Clang
On Fri, 2021-03-05 at 08:19:33 UTC, "Gustavo A. R. Silva" wrote: > In preparation to enable -Wimplicit-fallthrough for Clang, fix multiple > warnings by explicitly adding multiple break statements and a return > instead of letting the code fall through to the next case. > > Link: https://github.com/KSPP/linux/issues/115 > Signed-off-by: Gustavo A. R. Silva > Acked-by: Vignesh Raghavendra Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH RESEND][next] mtd: mtdchar: Fix fall-through warnings for Clang
On Fri, 2021-03-05 at 08:22:24 UTC, "Gustavo A. R. Silva" wrote: > In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning > by explicitly adding a break statement instead of letting the code fall > through to the next case. > > Link: https://github.com/KSPP/linux/issues/115 > Signed-off-by: Gustavo A. R. Silva Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH v3][next] mtd: rawnand: stm32_fmc2: Fix fall-through warnings for Clang
On Fri, 2021-03-05 at 08:29:53 UTC, "Gustavo A. R. Silva" wrote: > In preparation to enable -Wimplicit-fallthrough for Clang, fix a couple > of warnings by explicitly adding a couple of break statements instead > of letting the code fall through to the next case. > > Link: https://github.com/KSPP/linux/issues/115 > Signed-off-by: Gustavo A. R. Silva Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH next v4 02/15] mtd: mtdoops: synchronize kmsg_dumper
Hello, John Ogness wrote on Wed, 3 Mar 2021 11:15:15 +0100: > The kmsg_dumper can be called from any context and CPU, possibly > from multiple CPUs simultaneously. Since the writing of the buffer > can occur from a later scheduled work queue, the oops buffer must > be protected against simultaneous dumping. > > Use an atomic bit to mark when the buffer is protected. Release the > protection in between setting the buffer and the actual writing in > order for a possible panic (immediate write) to be written during > the scheduling of a previous oops (delayed write). > > An atomic bit (rather than a spinlock) was chosen so that no > scheduling or preemption side-effects would be introduced. The MTD > kmsg_dumper may dump directly or it may be delayed (via scheduled > work). Depending on the context, different MTD callbacks are used. > For example, mtd_write() expects to be called in a non-atomic > context and may take a mutex. > > Signed-off-by: John Ogness > Reviewed-by: Petr Mladek Reviewed-by: Miquel Raynal Thanks, Miquèl
Re: drivers/mtd/parsers/qcomsmempart.c:109 parse_qcomsmem_part() warn: passing zero to 'PTR_ERR'
Hello,
Dan Carpenter wrote on Wed, 3 Mar 2021
08:49:18 +0300:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> master
> head: 7a7fd0de4a9804299793e564a555a49c1fc924cb
> commit: 803eb124e1a64e42888542c3444bfe6dac412c7f mtd: parsers: Add Qcom SMEM
> parser
> config: nds32-randconfig-m031-20210302 (attached as .config)
> compiler: nds32le-linux-gcc (GCC) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot
> Reported-by: Dan Carpenter
>
> smatch warnings:
> drivers/mtd/parsers/qcomsmempart.c:109 parse_qcomsmem_part() warn: passing
> zero to 'PTR_ERR'
>
> vim +/PTR_ERR +109 drivers/mtd/parsers/qcomsmempart.c
>
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 57 static int
> parse_qcomsmem_part(struct mtd_info *mtd,
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 58
>const struct mtd_partition **pparts,
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 59
>struct mtd_part_parser_data *data)
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 60 {
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 61 struct
> smem_flash_pentry *pentry;
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 62 struct
> smem_flash_ptable *ptable;
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 63 size_t len =
> SMEM_FLASH_PTABLE_HDR_LEN;
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 64 struct
> mtd_partition *parts;
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 65 int ret, i,
> numparts;
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 66 char *name, *c;
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 67
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 68
> pr_debug("Parsing partition table info from SMEM\n");
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 69 ptable =
> qcom_smem_get(SMEM_APPS, SMEM_AARM_PARTITION_TABLE, );
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 70 if
> (IS_ERR(ptable)) {
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 71
> pr_err("Error reading partition table header\n");
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 72 return
> PTR_ERR(ptable);
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 73 }
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 74
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 75 /* Verify
> ptable magic */
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 76 if
> (le32_to_cpu(ptable->magic1) != SMEM_FLASH_PART_MAGIC1 ||
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 77
> le32_to_cpu(ptable->magic2) != SMEM_FLASH_PART_MAGIC2) {
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 78
> pr_err("Partition table magic verification failed\n");
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 79 return
> -EINVAL;
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 80 }
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 81
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 82 /* Ensure that
> # of partitions is less than the max we have allocated */
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 83 numparts =
> le32_to_cpu(ptable->numparts);
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 84 if (numparts >
> SMEM_FLASH_PTABLE_MAX_PARTS_V4) {
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 85
> pr_err("Partition numbers exceed the max limit\n");
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 86 return
> -EINVAL;
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 87 }
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 88
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 89 /* Find out
> length of partition data based on table version */
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 90 if
> (le32_to_cpu(ptable->version) <= SMEM_FLASH_PTABLE_V3) {
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 91 len =
> SMEM_FLASH_PTABLE_HDR_LEN + SMEM_FLASH_PTABLE_MAX_PARTS_V3 *
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 92
> sizeof(struct smem_flash_pentry);
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 93 } else if
> (le32_to_cpu(ptable->version) == SMEM_FLASH_PTABLE_V4) {
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 94 len =
> SMEM_FLASH_PTABLE_HDR_LEN + SMEM_FLASH_PTABLE_MAX_PARTS_V4 *
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 95
> sizeof(struct smem_flash_pentry);
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 96 } else {
> 803eb124e1a64e Manivannan Sadhasivam 2021-01-04 97
> pr_err("Unknown ptable version (%d)", le32_to_cpu(ptable->version));
> 803eb124e1a64e
Re: [PATCH][next] mtd: nand: Fix error handling in nand_prog_page_op
Hi Colin,
Colin King wrote on Wed, 3 Mar 2021
09:42:46 +:
> From: Colin Ian King
>
> The less than zero comparison with status is always false because status
> is a u8. Fix this by using ret as the return check for the call to
> chip->legacy.waitfunc() and checking on this and assigning status to ret
> if it is OK.
>
> Addresses-Coverity: ("Unsigned compared against 0")
> Fixes: 52f67def97f1 ("mtd: nand: fix error handling in nand_prog_page_op()
> #1")
> Signed-off-by: Colin Ian King
Thanks for the fix, but this has been handled just an hour ago :)
Cheers,
Miquèl
Re: [PATCH 0/2] Handle probe defer properly in MTD core
Hi Manivannan, Manivannan Sadhasivam wrote on Tue, 2 Mar 2021 18:57:55 +0530: > Hello, > > These two patches aims at fixing the -EPROBE_DEFER handling in the MTD > core and also in the Qcom nand driver. The "qcomsmem" parser depends on > the QCOM_SMEM driver to parse the partitions defined in the shared > memory. Due to the DT layout, the SMEM driver might probe after the NAND > driver. In that case, the -EPROBE_DEFER returned by qcom_smem_get() in > the parser will fail to propagate till the driver core. So this will > result in the partitions not getting parsed even after the SMEM driver is > available. > > So fix this issue by handling the -EPROBE_DEFER error properly in both > MTD core and in the Qcom nand driver. This issue is observed on Qcom SDX55 > based Telit FN980 EVB and in SDX55-MTP. Applied manually on top of nand/next as infradead.org is dead at the moment and the patches were not collected by patchwork. Thanks, Miquèl
Re: [PATCH mtd/next 2/8] mtd: ftl: Use module_mtd_blktrans to register driver
On Sat, 2021-02-13 at 16:45:54 UTC, Dejin Zheng wrote: > Removing some boilerplate by using module_mtd_blktrans instead of calling > register and unregister in the otherwise empty init/exit functions. > > Signed-off-by: Dejin Zheng Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH 4/5] mtd: rawnand: qcom: Add helper to configure location register
On Tue, 2021-02-23 at 19:39:00 UTC, Md Sadre Alam wrote: > Create a nandc_set_read_loc() helper to abstract the > configuration of the location register. > > QPIC v2 onwards features a separate location register > for the last codeword, so introducing this extra helper > which will simplify the addition of QPIC v2 support. > > Signed-off-by: Md Sadre Alam Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
Re: [PATCH] mtd: rawnand: fsmc: Fix error code in fsmc_nand_probe()
On Mon, 2021-02-15 at 15:58:49 UTC, Dan Carpenter wrote:
> If dma_request_channel() fails then the probe fails and it should
> return a negative error code, but currently it returns success.
>
> fixes: 4774fb0a48aa ("mtd: nand/fsmc: Add DMA support")
> Signed-off-by: Dan Carpenter
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git
nand/next, thanks.
Miquel
Re: [PATCH mtd/next 1/8] mtd: Add helper macro for register_mtd_blktrans boilerplate
On Sat, 2021-02-13 at 16:45:53 UTC, Dejin Zheng wrote: > This patch introduces the module_mtd_blktrans macro which is a convenience > macro for mtd blktrans modules similar to module_platform_driver. > It is intended to be used by drivers which init/exit section does nothing > but register/unregister the mtd blktrans driver. By using this macro it is > possible to eliminate a few lines of boilerplate code per mtd blktrans > driver. > > Signed-off-by: Dejin Zheng Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH] mtd: physmap: physmap-bt1-rom: Fix unintentional stack access
On Fri, 2021-02-12 at 10:40:22 UTC, "Gustavo A. R. Silva" wrote:
> Cast to (char *) in order to avoid unintentionally accessing
> the stack.
>
> Notice that data is of type u32, so any increment to
> will be in the order of 4-byte chunks, and this piece of code
> is actually intended to be a byte offset.
>
> Fixes: b3e79e7682e0 ("mtd: physmap: Add Baikal-T1 physically mapped ROM
> support")
> Addresses-Coverity-ID: 1497765 ("Out-of-bounds access")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva
> Acked-by: Serge Semin
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git
mtd/next, thanks.
Miquel
Re: [PATCH] mtd: mtdcore: constify name param in mtd_bdi_init
On Thu, 2021-02-25 at 14:33:29 UTC, Tomas Winkler wrote: > The bdi name is not modified by the function, it should be const. > > Signed-off-by: Tomas Winkler Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH 2/2] mtd: char: Get rid of Big MTD Lock
On Wed, 2021-02-17 at 21:18:45 UTC, Alexander A Sverdlin wrote: > From: Alexander Sverdlin > > Get rid of central chrdev MTD lock, which prevents simultaneous operations > on completely independent physical MTD chips. Replace it with newly > introduced per-master mutex. > > Signed-off-by: Alexander Sverdlin Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH mtd/next 3/8] mtd: inftlcore: Use module_mtd_blktrans to register driver
On Sat, 2021-02-13 at 16:45:55 UTC, Dejin Zheng wrote: > Removing some boilerplate by using module_mtd_blktrans instead of calling > register and unregister in the otherwise empty init/exit functions. > > Signed-off-by: Dejin Zheng Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH mtd/next 6/8] mtd: mtdswap: Use module_mtd_blktrans to register driver
On Sat, 2021-02-13 at 16:45:58 UTC, Dejin Zheng wrote: > Removing some boilerplate by using module_mtd_blktrans instead of calling > register and unregister in the otherwise empty init/exit functions. > > Signed-off-by: Dejin Zheng Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH mtd/next 4/8] mtd: mtdblock: Use module_mtd_blktrans to register driver
On Sat, 2021-02-13 at 16:45:56 UTC, Dejin Zheng wrote: > Removing some boilerplate by using module_mtd_blktrans instead of calling > register and unregister in the otherwise empty init/exit functions. > > Signed-off-by: Dejin Zheng Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH mtd/next 7/8] mtd: nftlcore: Use module_mtd_blktrans to register driver
On Sat, 2021-02-13 at 16:45:59 UTC, Dejin Zheng wrote: > Removing some boilerplate by using module_mtd_blktrans instead of calling > register and unregister in the otherwise empty init/exit functions. > > Signed-off-by: Dejin Zheng Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH mtd/next 8/8] mtd: rfd_ftl: Use module_mtd_blktrans to register driver
On Sat, 2021-02-13 at 16:46:00 UTC, Dejin Zheng wrote: > Removing some boilerplate by using module_mtd_blktrans instead of calling > register and unregister in the otherwise empty init/exit functions. > > Signed-off-by: Dejin Zheng Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH mtd/next 5/8] mtd: mtdblock_ro: Use module_mtd_blktrans to register driver
On Sat, 2021-02-13 at 16:45:57 UTC, Dejin Zheng wrote: > Removing some boilerplate by using module_mtd_blktrans instead of calling > register and unregister in the otherwise empty init/exit functions. > > Signed-off-by: Dejin Zheng Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH 1/2] mtd: char: Drop mtd_mutex usage from mtdchar_open()
On Wed, 2021-02-17 at 21:18:44 UTC, Alexander A Sverdlin wrote: > From: Alexander Sverdlin > > It looks unnecessary in the function, remove it from the function > having in mind to remove it completely. > > Signed-off-by: Alexander Sverdlin Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH v2 8/9] mtd/drivers/nand: Use HZ macros
Hi Daniel, Daniel Lezcano wrote on Tue, 2 Mar 2021 18:03:12 +0100: > On 02/03/2021 17:31, Miquel Raynal wrote: > > On Wed, 2021-02-24 at 14:42:18 UTC, Daniel Lezcano wrote: > >> HZ unit conversion macros are available in units.h, use them and > >> remove the duplicate definition. > >> > >> Signed-off-by: Daniel Lezcano > > > > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git > > nand/next, thanks. > > Actually, I was expecting to have it merged through linux-pm as this > patch depends on 1/9 > > > No problem, I just removed it from my tree. However in this case please fix the subject prefix to "mtd: rawnand: intel:". With this nit fixed, Acked-by: Miquel Raynal Thanks, Miquèl
Re: [PATCH v2 8/9] mtd/drivers/nand: Use HZ macros
On Wed, 2021-02-24 at 14:42:18 UTC, Daniel Lezcano wrote: > HZ unit conversion macros are available in units.h, use them and > remove the duplicate definition. > > Signed-off-by: Daniel Lezcano Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
Re: [PATCH v2] nand: brcmnand: fix OOB R/W with Hamming ECC
On Wed, 2021-02-24 at 08:02:10 UTC,
=?utf-8?q?=C3=81lvaro_Fern=C3=A1ndez_Rojas?= wrote:
> Hamming ECC doesn't cover the OOB data, so reading or writing OOB shall
> always be done without ECC enabled.
> This is a problem when adding JFFS2 cleanmarkers to erased blocks. If JFFS2
> clenmarkers are added to the OOB with ECC enabled, OOB bytes will be changed
> from ff ff ff to 00 00 00, reporting incorrect ECC errors.
>
> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB
> NAND controller")
> Signed-off-by: Álvaro Fernández Rojas
> Acked-by: Brian Norris
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git
nand/next, thanks.
Miquel
Re: [PATCH] mtd: rawnand: qcom: Update register macro name for 0x2c offset
On Sat, 2021-01-30 at 20:07:16 UTC, Md Sadre Alam wrote: > This change will remove unused register name macro NAND_DEV1_ECC_CFG. > Since this register was only available in QPIC version 1.4.20 ipq40xx > and it was not used. In QPIC version 1.5 on wards this register got > removed.In QPIC version 2.0 0x2c offset is updated with register > NAND_AUTO_STATUS_EN So adding this register macro NAND_AUTO_STATUS_EN > with offset 0x2c. > > Signed-off-by: Md Sadre Alam > Reviewed-by: Manivannan Sadhasivam Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
Re: [PATCH 3/5] mtd: rawnand: qcom: Rename parameter name in macro
On Tue, 2021-02-23 at 19:38:59 UTC, Md Sadre Alam wrote: > Rename the parameters of the nandc_set_read_loc() macro > to avoid the confusion between is_last_read_loc which > is last location in a read code word and last_cw which > is last code word of a page data. > > Signed-off-by: Md Sadre Alam Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
Re: [PATCH 2/5] mtd: rawnand: qcom: Add helper to check last code word
On Tue, 2021-02-23 at 19:38:58 UTC, Md Sadre Alam wrote: > Add the qcom_nandc_is_last_cw() helper which checks if > the input cw index is the last one or not. > > Signed-off-by: Md Sadre Alam Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
Re: [PATCH 1/5] mtd: rawnand: qcom: Convert nandc to chip in Read/Write helper
On Tue, 2021-02-23 at 19:38:57 UTC, Md Sadre Alam wrote: > This change will convert nandc to chip in Read/Write helper, this > change is needed because if we wnated to access number of steps > in Read/Write helper then we need to get the chip->ecc.steps, > currentlly its not possible.After this change we can directly > acces chip->ecc.steps in Read/Write helper. > > Signed-off-by: Md Sadre Alam Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks. Miquel
Re: [PATCH][next] i3c: master: svc: remove redundant assignment to cmd->read_len
Colin King wrote on Wed, 24 Feb 2021
15:13:49 +:
> From: Colin Ian King
>
> The assignment of xfer_len to cmd->read_len appears to be redundant
> as the next statement re-assigns the value 0 to it. Clean up the
> code by removing the redundant first assignment.
>
> Addresses-Coverity: ("Unused value")
> Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver")
> Signed-off-by: Colin Ian King
> ---
> drivers/i3c/master/svc-i3c-master.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/i3c/master/svc-i3c-master.c
> b/drivers/i3c/master/svc-i3c-master.c
> index 8d990696676e..1f6ba4221817 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -1124,7 +1124,6 @@ static int svc_i3c_master_send_direct_ccc_cmd(struct
> svc_i3c_master *master,
> cmd->in = NULL;
> cmd->out = >id;
> cmd->len = 1;
> - cmd->read_len = xfer_len;
> cmd->read_len = 0;
> cmd->continued = true;
>
Reviewed-by: Miquel Raynal
Thanks,
Miquèl
Re: [RFC PATCH] mtd: add OTP (one-time-programmable) erase ioctl
Hi Michael,
Michael Walle wrote on Tue, 2 Mar 2021 12:09:27
+0100:
> This may sound like a contradiction but some SPI-NOR flashes really
> support erasing their OTP region until it is finally locked. Having the
> possibility to erase an OTP region might come in handy during
> development.
>
> The ioctl argument follows the OTPLOCK style.
>
> Signed-off-by: Michael Walle
> ---
> OTP support for SPI-NOR flashes may be merged soon:
> https://lore.kernel.org/linux-mtd/20210216162807.13509-1-mich...@walle.cc/
>
> Tudor suggested to add support for the OTP erase operation most SPI-NOR
> flashes have:
> https://lore.kernel.org/linux-mtd/d4f74b1b-fa1b-97ec-858c-d807fe1f9...@microchip.com/
>
> Therefore, this is an RFC to get some feedback on the MTD side, once this
> is finished, I can post a patch for mtd-utils. Then we'll have a foundation
> to add the support to SPI-NOR.
>
> drivers/mtd/mtdchar.c | 7 ++-
> drivers/mtd/mtdcore.c | 12
> include/linux/mtd/mtd.h| 3 +++
> include/uapi/mtd/mtd-abi.h | 2 ++
> 4 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 323035d4f2d0..da423dd031ae 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -661,6 +661,7 @@ static int mtdchar_ioctl(struct file *file, u_int cmd,
> u_long arg)
> case OTPGETREGIONCOUNT:
> case OTPGETREGIONINFO:
> case OTPLOCK:
> + case OTPERASE:
> case ECCGETLAYOUT:
> case ECCGETSTATS:
> case MTDFILEMODE:
> @@ -938,6 +939,7 @@ static int mtdchar_ioctl(struct file *file, u_int cmd,
> u_long arg)
> }
>
> case OTPLOCK:
> + case OTPERASE:
> {
> struct otp_info oinfo;
>
> @@ -945,7 +947,10 @@ static int mtdchar_ioctl(struct file *file, u_int cmd,
> u_long arg)
> return -EINVAL;
> if (copy_from_user(, argp, sizeof(oinfo)))
> return -EFAULT;
> - ret = mtd_lock_user_prot_reg(mtd, oinfo.start, oinfo.length);
> + if (cmd == OTPLOCK)
> + ret = mtd_lock_user_prot_reg(mtd, oinfo.start,
> oinfo.length);
> + else
> + ret = mtd_erase_user_prot_reg(mtd, oinfo.start,
> oinfo.length);
> break;
> }
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 2d6423d89a17..d844d718ef77 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1918,6 +1918,18 @@ int mtd_lock_user_prot_reg(struct mtd_info *mtd,
> loff_t from, size_t len)
> }
> EXPORT_SYMBOL_GPL(mtd_lock_user_prot_reg);
>
> +int mtd_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len)
> +{
> + struct mtd_info *master = mtd_get_master(mtd);
> +
> + if (!master->_erase_user_prot_reg)
> + return -EOPNOTSUPP;
> + if (!len)
> + return 0;
Should we add a sanity check enforcing that we don't try to access
beyond the end of the OTP area?
> + return master->_erase_user_prot_reg(master, from, len);
> +}
> +EXPORT_SYMBOL_GPL(mtd_erase_user_prot_reg);
> +
> /* Chip-supported device locking */
> int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> {
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 157357ec1441..734ad7a8c353 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -336,6 +336,8 @@ struct mtd_info {
>size_t len, size_t *retlen, u_char *buf);
> int (*_lock_user_prot_reg) (struct mtd_info *mtd, loff_t from,
> size_t len);
> + int (*_erase_user_prot_reg) (struct mtd_info *mtd, loff_t from,
> + size_t len);
> int (*_writev) (struct mtd_info *mtd, const struct kvec *vecs,
> unsigned long count, loff_t to, size_t *retlen);
> void (*_sync) (struct mtd_info *mtd);
> @@ -517,6 +519,7 @@ int mtd_read_user_prot_reg(struct mtd_info *mtd, loff_t
> from, size_t len,
> int mtd_write_user_prot_reg(struct mtd_info *mtd, loff_t to, size_t len,
> size_t *retlen, u_char *buf);
> int mtd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len);
> +int mtd_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len);
>
> int mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
> unsigned long count, loff_t to, size_t *retlen);
> diff --git a/include/uapi/mtd/mtd-abi.h b/include/uapi/mtd/mtd-abi.h
> index 65b9db936557..242015f60d10 100644
> --- a/include/uapi/mtd/mtd-abi.h
> +++ b/include/uapi/mtd/mtd-abi.h
> @@ -205,6 +205,8 @@ struct otp_info {
> * without OOB, e.g., NOR flash.
> */
> #define MEMWRITE _IOWR('M', 24, struct mtd_write_req)
> +/* Erase a given range of user data (must be in mode
> %MTD_FILE_MODE_OTP_USER) */
> +#define OTPERASE _IOR('M', 25, struct otp_info)
>
> /*
>
Re: [PATCH v2] nand: brcmnand: fix OOB R/W with Hamming ECC
Hi Álvaro,
Álvaro Fernández Rojas wrote on Thu, 25 Feb 2021
08:54:09 +0100:
> Hi Miquel,
>
> > El 25 feb 2021, a las 8:48, Miquel Raynal
> > escribió:
> >
> > Hi Álvaro,
> >
> > Brian Norris wrote on Wed, 24 Feb 2021
> > 13:01:13 -0800:
> >
> >> On Wed, Feb 24, 2021 at 12:02 AM Álvaro Fernández Rojas
> >> wrote:
> >>> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom
> >>> STB NAND controller")
> >>
> >> FWIW, I could believe this was broken. We weren't testing Hamming ECC
> >> (nor JFFS2) at the time, so it could easily have obvious bugs like
> >> this.
> >
> > Right, you should probably limit the backport to the time when raw
> > accessors got introduced/fixed.
>
> What do you mean?
> Those accessors have been there since the first commit
> (27c5b17cd1b10564fa36f8f51e4b4b41436ecc32):
> https://github.com/torvalds/linux/blob/27c5b17cd1b10564fa36f8f51e4b4b41436ecc32/drivers/mtd/nand/brcmnand/brcmnand.c#L1896-L1899
I misunderstood Brian's answer. This commit is not that old and looks
legit.
Re: [PATCH v2] nand: brcmnand: fix OOB R/W with Hamming ECC
Hi Álvaro,
Brian Norris wrote on Wed, 24 Feb 2021
13:01:13 -0800:
> On Wed, Feb 24, 2021 at 12:02 AM Álvaro Fernández Rojas
> wrote:
> > Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB
> > NAND controller")
>
> FWIW, I could believe this was broken. We weren't testing Hamming ECC
> (nor JFFS2) at the time, so it could easily have obvious bugs like
> this.
Right, you should probably limit the backport to the time when raw
accessors got introduced/fixed.
Thanks,
Miquèl
Re: [PATCH v2 3/3] mtd: rawnand: qcom: Add support for secure regions in NAND memory
Hi Manivannan,
Manivannan Sadhasivam wrote on Thu,
25 Feb 2021 09:41:29 +0530:
> On a typical end product, a vendor may choose to secure some regions in
> the NAND memory which are supposed to stay intact between FW upgrades.
> The access to those regions will be blocked by a secure element like
> Trustzone. So the normal world software like Linux kernel should not
> touch these regions (including reading).
>
> The regions are declared using a NAND chip DT property,
> "nand-secure-regions". So let's make use of this property and skip
> access to the secure regions present in a system.
>
> Signed-off-by: Manivannan Sadhasivam
> ---
[...]
> config_nand_page_write(nandc);
> @@ -2830,7 +2865,8 @@ static int qcom_nand_host_init_and_register(struct
> qcom_nand_controller *nandc,
> struct nand_chip *chip = >chip;
> struct mtd_info *mtd = nand_to_mtd(chip);
> struct device *dev = nandc->dev;
> - int ret;
> + struct property *prop;
> + int ret, length, nr_elem;
>
> ret = of_property_read_u32(dn, "reg", >cs);
> if (ret) {
> @@ -2886,6 +2922,24 @@ static int qcom_nand_host_init_and_register(struct
> qcom_nand_controller *nandc,
> }
> }
>
> + /*
> + * Look for secure regions in the NAND chip. These regions are supposed
> + * to be protected by a secure element like Trustzone. So the read/write
> + * accesses to these regions will be blocked in the runtime by this
> + * driver.
> + */
> + prop = of_find_property(dn, "nand-secure-regions", );
I'm not sure the nand- prefix on this property is needed here, but
whatever.
> + if (prop) {
> + nr_elem = length / sizeof(u32);
> + host->nr_sec_regions = nr_elem / 2;
> +
> + host->sec_regions = devm_kcalloc(dev, nr_elem, sizeof(u32),
> GFP_KERNEL);
> + if (!host->sec_regions)
> + return -ENOMEM;
> +
> + of_property_read_u32_array(dn, "nand-secure-regions",
> host->sec_regions, nr_elem);
> + }
> +
I would move this before nand_scan().
If you don't, you should bail out with a nand_cleanup() upon error.
> ret = mtd_device_parse_register(mtd, probes, NULL, NULL, 0);
> if (ret)
> nand_cleanup(chip);
Otherwise lgtm.
Thanks,
Miquèl
Re: [PATCH] mtd: rawnand: qcom: update last code word register
Hello, mda...@codeaurora.org wrote on Wed, 24 Feb 2021 22:00:05 +0530: > On 2021-02-24 12:18, Miquel Raynal wrote: > > Hello, > > > > mda...@codeaurora.org wrote on Wed, 24 Feb 2021 10:09:48 +0530: > > > >> On 2021-02-24 01:13, mda...@codeaurora.org wrote: > >> > On 2021-02-23 22:04, Miquel Raynal wrote: > >> >> Hello, > >> >> >> Md Sadre Alam wrote on Tue, 23 Feb 2021 > >> >> 01:34:27 +0530: > >> >> >>> From QPIC version 2.0 onwards new register got added to read last > >> >> >>a new > >> >> >>> codeword. This change will add the READ_LOCATION_LAST_CW_n > >> >> >>> register. > >> >> >> Add support for this READ_LOCATION_LAST_CW_n register. > >> >> >>> >>> For first three code word READ_LOCATION_n register will be > >> >>> use.For last code word READ_LOCATION_LAST_CW_n register will be > >> >>> use. > >> >> >> " > >> >> In the case of QPIC v2, codewords 0, 1 and 2 will be accessed through > >> >> READ_LOCATION_n, while codeword 3 will be accessed through > >> >> READ_LOCATION_LAST_CW_n. > >> >> " > >> >> >> When I read my own sentence, I feel that there is something wrong. > >> >> If there are only 4 codewords, I guess a QPIC v2 is able to use > >> >> READ_LOCATION_3 or READ_LOCATION_LAST_CW_0 interchangeably. Isn't it? > >> >> >> I guess the point of having these "last_cw_n" registers is to > >> >> >> support > >> >> up to 8 codewords, am I wrong? If this the case, the current patch > >> >> completely fails doing that I don't get the point of such change. > >> > > >> > This register is only use to read last code word. > >> > > >> > I have address all the comments from all the previous sub sequent > >> > patches and pushed > >> > all patches in only one series. > >> > > >> > Please check. > >> >> The registers READ_LOCATION & READ_LOCATION_LAST are not associated > >> >> >> with number of code words. > >> These two registers are used to access the location inside a code >> > >> word. > > > > Ok. Can you please explain what is a location then? Or point me to a > > datasheet that explains it. > >The location is the position inside a code word. > > > > > Bottom line question: why having READ_LOCATION_0, _1,... an > > READ_LOCATION_LAST_0, _1, etc? > > READ_LOCATION_0, _1,... are used to extract multiple chunks from a code > word. > > e.g If we wanted to extract first 100 bytes from a code word then (0...99) > READ_LOCATION_0 will be configured. > if we wanted to extract next 100 bytes (100...199) then READ_LOCATION_1 > will be configured. > > same way for last code word READ_LOCATION_LAST_0, _1, will be used. > Nice explanation, and thanks for the below figures. So I guess there is some kind of "small SRAM" that is directly addressable perhaps? I think I'm fine with your series now. Just a small nit: next time you send a series, please update the version number "[PATCH v6]" (automatically added with the -v6 parameter in git-format-patch). But no need to resend just for that. Thanks, Miquèl
Re: [PATCH] nand: brcmnand: fix OOB R/W with Hamming ECC
Hi Álvaro, Álvaro Fernández Rojas wrote on Wed, 24 Feb 2021 08:16:58 +0100: > Hi Florian, > > > El 24 feb 2021, a las 4:46, Florian Fainelli > > escribió: > > > > > > > > On 2/22/2021 12:16 PM, Álvaro Fernández Rojas wrote: > >> Hamming ECC doesn't cover the OOB data, so reading or writing OOB shall > >> always be done without ECC enabled. > >> This is a problem when adding JFFS2 cleanmarkers to erased blocks. If JFFS2 > >> clenmarkers are added to the OOB with ECC enabled, OOB bytes will be > >> changed > >> from ff ff ff to 00 00 00, reporting incorrect ECC errors. > >> > >> Signed-off-by: Álvaro Fernández Rojas > > > > Should there be a Fixes: tag provided here for back porting to stable trees? > > I think so, but the fixed commit would be the first one, right? > 27c5b17cd1b10564fa36f8f51e4b4b41436ecc32 Yep, shouldn't be a problem. Thanks, Miquèl
Re: [PATCH] mtd: rawnand: qcom: update last code word register
Hello, mda...@codeaurora.org wrote on Wed, 24 Feb 2021 10:09:48 +0530: > On 2021-02-24 01:13, mda...@codeaurora.org wrote: > > On 2021-02-23 22:04, Miquel Raynal wrote: > >> Hello, > >> >> Md Sadre Alam wrote on Tue, 23 Feb 2021 > >> 01:34:27 +0530: > >> >>> From QPIC version 2.0 onwards new register got added to read last > >> >>a new > >> >>> codeword. This change will add the READ_LOCATION_LAST_CW_n register. > >> >> Add support for this READ_LOCATION_LAST_CW_n register. > >> >>> >>> For first three code word READ_LOCATION_n register will be > >>> use.For last code word READ_LOCATION_LAST_CW_n register will be > >>> use. > >> >> " > >> In the case of QPIC v2, codewords 0, 1 and 2 will be accessed through > >> READ_LOCATION_n, while codeword 3 will be accessed through > >> READ_LOCATION_LAST_CW_n. > >> " > >> >> When I read my own sentence, I feel that there is something wrong. > >> If there are only 4 codewords, I guess a QPIC v2 is able to use > >> READ_LOCATION_3 or READ_LOCATION_LAST_CW_0 interchangeably. Isn't it? > >> >> I guess the point of having these "last_cw_n" registers is to support > >> up to 8 codewords, am I wrong? If this the case, the current patch > >> completely fails doing that I don't get the point of such change. > > > > This register is only use to read last code word. > > > > I have address all the comments from all the previous sub sequent > > patches and pushed > > all patches in only one series. > > > > Please check. > > The registers READ_LOCATION & READ_LOCATION_LAST are not associated with > number of code words. > These two registers are used to access the location inside a code word. Ok. Can you please explain what is a location then? Or point me to a datasheet that explains it. Bottom line question: why having READ_LOCATION_0, _1,... an READ_LOCATION_LAST_0, _1, etc? > So whether we are having 4 code words > or 8 code words it doesn't matter. If we wanted access the location within > normal code word we have to > use READ_LOCATION register and if we wanted to access location in last code > word then we have to use > READ_LOCATION_LAST. > > > >> >>> Signed-off-by: Md Sadre Alam Thanks, Miquèl
Re: [PATCH 2/3] dt-bindings: mtd: Add a property to declare secure regions in Qcom NANDc
Hi Manivannan, Manivannan Sadhasivam wrote on Tue, 23 Feb 2021 23:15:46 +0530: > Hi Miquel, > > On Tue, Feb 23, 2021 at 05:49:22PM +0100, Miquel Raynal wrote: > > Hi Manivannan, > > > > Manivannan Sadhasivam wrote on Mon, > > 22 Feb 2021 17:32:58 +0530: > > > > > On a typical end product, a vendor may choose to secure some regions in > > > the NAND memory which are supposed to stay intact between FW upgrades. > > > The access to those regions will be blocked by a secure element like > > > Trustzone. So the normal world software like Linux kernel should not > > > touch these regions (including reading). > > > > > > So let's add a property for declaring such secure regions so that the > > > driver can skip touching them. > > > > > > Signed-off-by: Manivannan Sadhasivam > > > --- > > > Documentation/devicetree/bindings/mtd/qcom,nandc.yaml | 7 +++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml > > > b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml > > > index 84ad7ff30121..7500e20da9c1 100644 > > > --- a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml > > > +++ b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml > > > @@ -48,6 +48,13 @@ patternProperties: > > > enum: > > >- 512 > > > > > > + qcom,secure-regions: > > > +$ref: /schemas/types.yaml#/definitions/uint32-array > > > +description: > > > + Regions in the NAND memory which are protected using a secure > > > element > > > + like Trustzone. This property contains the start address and > > > size of > > > + the secure regions present (optional). > > > > What does this "(optional)" means? If you mean the property is optional > > then it should be described accordingly in the yaml file, or am I > > missing something? > > > > IIUC, if a property is not listed under "required" section then it is > optional. But I've added the quote here to just make it explicit. Absolutely, I was just wondering why you mentioned it here. I don't think it's useful, you can drop it. > > > I wonder if it wouldn't be better to make this a NAND chip node > > property. I don't think a qcom prefix is needed as potentially many > > other SoCs might have the same "feature". > > > > I'm fine adding support for it in the qcom driver only though. > > > > Hmm, sounds good to me. > > Thanks, > Mani > > > > + > > > allOf: > > >- $ref: "nand-controller.yaml#" > > > > > > > Thanks, > > Miquèl Thanks, Miquèl
Re: [PATCH 2/3] dt-bindings: mtd: Add a property to declare secure regions in Qcom NANDc
Hi Manivannan, Manivannan Sadhasivam wrote on Mon, 22 Feb 2021 17:32:58 +0530: > On a typical end product, a vendor may choose to secure some regions in > the NAND memory which are supposed to stay intact between FW upgrades. > The access to those regions will be blocked by a secure element like > Trustzone. So the normal world software like Linux kernel should not > touch these regions (including reading). > > So let's add a property for declaring such secure regions so that the > driver can skip touching them. > > Signed-off-by: Manivannan Sadhasivam > --- > Documentation/devicetree/bindings/mtd/qcom,nandc.yaml | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml > b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml > index 84ad7ff30121..7500e20da9c1 100644 > --- a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml > +++ b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml > @@ -48,6 +48,13 @@ patternProperties: > enum: >- 512 > > + qcom,secure-regions: > +$ref: /schemas/types.yaml#/definitions/uint32-array > +description: > + Regions in the NAND memory which are protected using a secure > element > + like Trustzone. This property contains the start address and size > of > + the secure regions present (optional). What does this "(optional)" means? If you mean the property is optional then it should be described accordingly in the yaml file, or am I missing something? I wonder if it wouldn't be better to make this a NAND chip node property. I don't think a qcom prefix is needed as potentially many other SoCs might have the same "feature". I'm fine adding support for it in the qcom driver only though. > + > allOf: >- $ref: "nand-controller.yaml#" > Thanks, Miquèl
Re: [PATCH] mtd: rawnand: qcom: update last code word register
Hello,
Md Sadre Alam wrote on Tue, 23 Feb 2021
01:34:27 +0530:
> From QPIC version 2.0 onwards new register got added to read last
a new
> codeword. This change will add the READ_LOCATION_LAST_CW_n register.
Add support for this READ_LOCATION_LAST_CW_n register.
>
> For first three code word READ_LOCATION_n register will be
> use.For last code word READ_LOCATION_LAST_CW_n register will be
> use.
"
In the case of QPIC v2, codewords 0, 1 and 2 will be accessed through
READ_LOCATION_n, while codeword 3 will be accessed through
READ_LOCATION_LAST_CW_n.
"
When I read my own sentence, I feel that there is something wrong.
If there are only 4 codewords, I guess a QPIC v2 is able to use
READ_LOCATION_3 or READ_LOCATION_LAST_CW_0 interchangeably. Isn't it?
I guess the point of having these "last_cw_n" registers is to support
up to 8 codewords, am I wrong? If this the case, the current patch
completely fails doing that I don't get the point of such change.
> Signed-off-by: Md Sadre Alam
> ---
[...]
> /* helper to configure address register values */
> @@ -700,8 +727,9 @@ static void set_address(struct qcom_nand_host *host, u16
> column, int page)
> *
> * @num_cw: number of steps for the read/write operation
> * @read:read or write operation
> + * @cw : which code word
> */
> -static void update_rw_regs(struct qcom_nand_host *host, int num_cw, bool
> read)
> +static void update_rw_regs(struct qcom_nand_host *host, int num_cw, bool
> read, int cw)
> {
> struct nand_chip *chip = >chip;
> struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> @@ -740,7 +768,7 @@ static void update_rw_regs(struct qcom_nand_host *host,
> int num_cw, bool read)
> nandc_set_reg(nandc, NAND_EXEC_CMD, 1);
>
> if (read)
> - nandc_set_read_loc(chip, 0, 0, 0, host->use_ecc ?
> + nandc_set_read_loc(chip, cw, 0, 0, host->use_ecc ?
> host->cw_data : host->cw_size, 1);
> }
>
> @@ -,18 +1139,34 @@ static void config_nand_page_read(struct nand_chip
> *chip)
> NAND_ERASED_CW_SET | NAND_BAM_NEXT_SGL);
> }
>
> +/* helper to check which location register should be use for this
/*
* Check which location...
> + * code word. NAND_READ_LOCATION or NAND_READ_LOCATION_LAST_CW
> + */
> +static bool config_loc_last_reg(struct nand_chip *chip, int cw)
> +{
> + struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> + struct nand_ecc_ctrl *ecc = >ecc;
> +
> + if (nandc->props->qpic_v2 && qcom_nandc_is_last_cw(ecc, cw))
> + return true;
Not sure this is really useful, it's probably better to drop this
helper and just use...
> +
> + return false;
> +}
> /*
> * Helper to prepare DMA descriptors for configuring registers
> * before reading each codeword in NAND page.
> */
> static void
> -config_nand_cw_read(struct nand_chip *chip, bool use_ecc)
> +config_nand_cw_read(struct nand_chip *chip, bool use_ecc, int cw)
> {
> struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> + int reg = NAND_READ_LOCATION_0;
> +
> + if (config_loc_last_reg(chip, cw))
... if (nandc->props->qpic_v2 && qcom_nandc_is_lastcw()) here.
> + reg = NAND_READ_LOCATION_LAST_CW_0;
>
> if (nandc->props->is_bam)
> - write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
> - NAND_BAM_NEXT_SGL);
> + write_reg_dma(nandc, reg, 4, NAND_BAM_NEXT_SGL);
>
> write_reg_dma(nandc, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL);
> write_reg_dma(nandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL);
> @@ -1142,12 +1186,12 @@ config_nand_cw_read(struct nand_chip *chip, bool
> use_ecc)
Thanks,
Miquèl
Re: [PATCH] mtd: rawnand: qcom: Add helper to configure location register
Hello,
Md Sadre Alam wrote on Mon, 22 Feb 2021
20:35:43 +0530:
> This change will add helper nandc_set_read_loc() to configure
> location register value. QPIC V2 on wards there is separate
> location register to read the last code word. This helper
> will use to configure location register for last code word
> as well.
>
"
Create a nandc_set_read_loc() helper to abstract the configuration of
the location register.
QPIC v2 onwards features a separate location register for the last
codeword, so introducing this extra helper will simplify the addition of
QPIC v2 support.
"
> Signed-off-by: Md Sadre Alam
> ---
> drivers/mtd/nand/raw/qcom_nandc.c | 38 ++
> 1 file changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c
> b/drivers/mtd/nand/raw/qcom_nandc.c
> index bfefb4e..82d083ad 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -181,8 +181,8 @@
> #define ECC_BCH_4BITBIT(2)
> #define ECC_BCH_8BITBIT(3)
>
> -#define nandc_set_read_loc(nandc, reg, cw_offset, read_size,
> is_last_read_loc) \
> -nandc_set_reg(nandc, NAND_READ_LOCATION_##reg, \
> +#define nandc_set_read_loc_first(nandc, reg, cw_offset, read_size,
> is_last_read_loc) \
Please do the s/nandc/chip/ renaming in patch 1.
> +nandc_set_reg(nandc, reg,\
> ((cw_offset) << READ_LOCATION_OFFSET) | \
> ((read_size) << READ_LOCATION_SIZE) | \
> ((is_last_read_loc) << READ_LOCATION_LAST))
> @@ -667,6 +667,20 @@ static bool qcom_nandc_is_last_cw(struct nand_ecc_ctrl
> *ecc, int cw)
> return cw == (ecc->steps - 1);
> }
>
> +/* helper to configure location register values */
> +static void nandc_set_read_loc(struct nand_chip *chip, int cw, int reg,
> +int cw_offset, int read_size, int
> is_last_read_loc)
> +{
> + struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> +
> + int reg_base = NAND_READ_LOCATION_0;
> +
> + reg_base += reg * 4;
> +
> + return nandc_set_read_loc_first(nandc, reg_base, cw_offset,
> + read_size, is_last_read_loc);
> +}
> +
> /* helper to configure address register values */
> static void set_address(struct qcom_nand_host *host, u16 column, int page)
> {
> @@ -726,7 +740,7 @@ static void update_rw_regs(struct qcom_nand_host *host,
> int num_cw, bool read)
> nandc_set_reg(nandc, NAND_EXEC_CMD, 1);
>
> if (read)
> - nandc_set_read_loc(nandc, 0, 0, host->use_ecc ?
> + nandc_set_read_loc(chip, 0, 0, 0, host->use_ecc ?
> host->cw_data : host->cw_size, 1);
> }
>
> @@ -1221,7 +1235,7 @@ static int nandc_param(struct qcom_nand_host *host)
> nandc_set_reg(nandc, NAND_DEV_CMD_VLD_RESTORE, nandc->vld);
> }
>
> - nandc_set_read_loc(nandc, 0, 0, 512, 1);
> + nandc_set_read_loc(chip, 0, 0, 0, 512, 1);
>
> if (!nandc->props->qpic_v2) {
> write_reg_dma(nandc, NAND_DEV_CMD_VLD, 1, 0);
> @@ -1649,16 +1663,16 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd, struct
> nand_chip *chip,
> }
>
> if (nandc->props->is_bam) {
> - nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0);
> + nandc_set_read_loc(chip, cw, 0, read_loc, data_size1, 0);
> read_loc += data_size1;
>
> - nandc_set_read_loc(nandc, 1, read_loc, oob_size1, 0);
> + nandc_set_read_loc(chip, cw, 1, read_loc, oob_size1, 0);
> read_loc += oob_size1;
>
> - nandc_set_read_loc(nandc, 2, read_loc, data_size2, 0);
> + nandc_set_read_loc(chip, cw, 2, read_loc, data_size2, 0);
> read_loc += data_size2;
>
> - nandc_set_read_loc(nandc, 3, read_loc, oob_size2, 1);
> + nandc_set_read_loc(chip, cw, 3, read_loc, oob_size2, 1);
> }
>
> config_nand_cw_read(chip, false);
> @@ -1889,13 +1903,13 @@ static int read_page_ecc(struct qcom_nand_host *host,
> u8 *data_buf,
>
> if (nandc->props->is_bam) {
> if (data_buf && oob_buf) {
> - nandc_set_read_loc(nandc, 0, 0, data_size, 0);
> - nandc_set_read_loc(nandc, 1, data_size,
> + nandc_set_read_loc(chip, i, 0, 0, data_size, 0);
> + nandc_set_read_loc(chip, i, 1, data_size,
> oob_size, 1);
> } else if (data_buf) {
> - nandc_set_read_loc(nandc, 0, 0, data_size, 1);
> + nandc_set_read_loc(chip, i, 0, 0, data_size, 1);
> } else {
> - nandc_set_read_loc(nandc, 0, data_size,
> +
Re: [PATCH] mtd: rawnand: qcom: Rename parameter name in macro
Hello, Md Sadre Alam wrote on Mon, 22 Feb 2021 13:05:42 +0530: > This change will rename parameter name in macro > nandc_set_read_loc().renamed parameter names are > cw_offset, read_size, is_last_read_loc. > Sinc in QPIC V2 on-wards there is separate location > register to read last code word, so to just differnciate > b/w is_last_read_loc from last_cw this change needed. " Rename the parameters of the nandc_set_read_loc() macro to avoid the confusion between is_last_read_loc which and which . > > Signed-off-by: Md Sadre Alam > --- > drivers/mtd/nand/raw/qcom_nandc.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c > b/drivers/mtd/nand/raw/qcom_nandc.c > index 4189a7f..bfefb4e 100644 > --- a/drivers/mtd/nand/raw/qcom_nandc.c > +++ b/drivers/mtd/nand/raw/qcom_nandc.c > @@ -181,11 +181,11 @@ > #define ECC_BCH_4BITBIT(2) > #define ECC_BCH_8BITBIT(3) > > -#define nandc_set_read_loc(nandc, reg, offset, size, is_last)\ > +#define nandc_set_read_loc(nandc, reg, cw_offset, read_size, > is_last_read_loc) \ > nandc_set_reg(nandc, NAND_READ_LOCATION_##reg, \ > - ((offset) << READ_LOCATION_OFFSET) | \ > - ((size) << READ_LOCATION_SIZE) | \ > - ((is_last) << READ_LOCATION_LAST)) > + ((cw_offset) << READ_LOCATION_OFFSET) | \ > + ((read_size) << READ_LOCATION_SIZE) | \ > + ((is_last_read_loc) << READ_LOCATION_LAST)) > > /* > * Returns the actual register address for all NAND_DEV_ registers Thanks, Miquèl
Re: [PATCH] mtd: rawnand: qcom: Add helper to check last code word
Hello,
Md Sadre Alam wrote on Mon, 22 Feb 2021
11:54:55 +0530:
> This change will add helper qcom_nandc_is_last_cw()
Use the imperative form, something like:
"
Add the qcom_nandc_is_last_cw() helper which checks if the input cw
index is the last one or not.
"
> which will check for last code word and return true for
> last code word and false for other code word.
>
> Signed-off-by: Md Sadre Alam
> ---
> drivers/mtd/nand/raw/qcom_nandc.c | 24 +++-
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c
> b/drivers/mtd/nand/raw/qcom_nandc.c
> index ae8870ec..4189a7f 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -661,6 +661,12 @@ static void nandc_set_reg(struct qcom_nand_controller
> *nandc, int offset,
> *reg = cpu_to_le32(val);
> }
>
> +/* Helper to check the code word, whether it is last cw or not */
> +static bool qcom_nandc_is_last_cw(struct nand_ecc_ctrl *ecc, int cw)
> +{
> + return cw == (ecc->steps - 1);
> +}
> +
> /* helper to configure address register values */
> static void set_address(struct qcom_nand_host *host, u16 column, int page)
> {
> @@ -1632,7 +1638,7 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd, struct
> nand_chip *chip,
> data_size1 = mtd->writesize - host->cw_size * (ecc->steps - 1);
> oob_size1 = host->bbm_size;
>
> - if (cw == (ecc->steps - 1)) {
> + if (qcom_nandc_is_last_cw(ecc, cw)) {
> data_size2 = ecc->size - data_size1 -
>((ecc->steps - 1) * 4);
> oob_size2 = (ecc->steps * 4) + host->ecc_bytes_hw +
> @@ -1713,7 +1719,7 @@ check_for_erased_page(struct qcom_nand_host *host, u8
> *data_buf,
> }
>
> for_each_set_bit(cw, _cws, ecc->steps) {
> - if (cw == (ecc->steps - 1)) {
> + if (qcom_nandc_is_last_cw(ecc, cw)) {
> data_size = ecc->size - ((ecc->steps - 1) * 4);
> oob_size = (ecc->steps * 4) + host->ecc_bytes_hw;
> } else {
> @@ -1773,7 +1779,7 @@ static int parse_read_errors(struct qcom_nand_host
> *host, u8 *data_buf,
> u32 flash, buffer, erased_cw;
> int data_len, oob_len;
>
> - if (i == (ecc->steps - 1)) {
> + if (qcom_nandc_is_last_cw(ecc, i)) {
> data_len = ecc->size - ((ecc->steps - 1) << 2);
> oob_len = ecc->steps << 2;
> } else {
> @@ -1872,7 +1878,7 @@ static int read_page_ecc(struct qcom_nand_host *host,
> u8 *data_buf,
> for (i = 0; i < ecc->steps; i++) {
> int data_size, oob_size;
>
> - if (i == (ecc->steps - 1)) {
> + if (qcom_nandc_is_last_cw(ecc, i)) {
> data_size = ecc->size - ((ecc->steps - 1) << 2);
> oob_size = (ecc->steps << 2) + host->ecc_bytes_hw +
> host->spare_bytes;
> @@ -2051,7 +2057,7 @@ static int qcom_nandc_write_page(struct nand_chip
> *chip, const uint8_t *buf,
> for (i = 0; i < ecc->steps; i++) {
> int data_size, oob_size;
>
> - if (i == (ecc->steps - 1)) {
> + if (qcom_nandc_is_last_cw(ecc, i)) {
> data_size = ecc->size - ((ecc->steps - 1) << 2);
> oob_size = (ecc->steps << 2) + host->ecc_bytes_hw +
> host->spare_bytes;
> @@ -2068,10 +2074,10 @@ static int qcom_nandc_write_page(struct nand_chip
> *chip, const uint8_t *buf,
>* when ECC is enabled, we don't really need to write anything
>* to oob for the first n - 1 codewords since these oob regions
>* just contain ECC bytes that's written by the controller
> - * itself. For the last codeword, we skip the bbm positions and
> - * write to the free oob area.
> + * itself. For the last codeword, we skip the bbm positions and
> write
> + * to the free oob area.
Not related change, please drop.
>*/
> - if (i == (ecc->steps - 1)) {
> + if (qcom_nandc_is_last_cw(ecc, i)) {
> oob_buf += host->bbm_size;
>
> write_data_dma(nandc, FLASH_BUF_ACC + data_size,
> @@ -2126,7 +2132,7 @@ static int qcom_nandc_write_page_raw(struct nand_chip
> *chip,
> data_size1 = mtd->writesize - host->cw_size * (ecc->steps - 1);
> oob_size1 = host->bbm_size;
>
> - if (i == (ecc->steps - 1)) {
> + if (qcom_nandc_is_last_cw(ecc, i)) {
> data_size2 = ecc->size - data_size1 -
>((ecc->steps - 1) << 2);
> oob_size2 = (ecc->steps << 2) + host->ecc_bytes_hw +
Thanks,
Miquèl
Re: [PATCH] mtd: rawnand: qcom: Convert nandc to chip in Read/Write helper
Hello, Md Sadre Alam wrote on Mon, 22 Feb 2021 01:55:01 +0530: > This change will convert nandc to chip in Read/Write helper, this > change is needed because if we wnated to access number of steps > in Read/Write helper then we need to get the chip->ecc.steps, > currentlly its not possible.After this change we can directly > acces chip->ecc.steps in Read/Write helper. > > Signed-off-by: Md Sadre Alam Thanks for splitting your series, I think it's much easier to review and contains much less imprecise changes. I have a few minor comments (see the following e-mails), please address them and then please send all your patches a single series, not like 6+ independent patches. I'll then require someone to test it if we are good I'll merge it. Thanks, Miquèl
Re: [PATCH] mtd: rawnand: qcom: update last code word register
Hello,
> >> >> +/* helper to configure location register values */
> >> +static void nandc_set_read_loc(struct nand_chip *chip, int cw, int >> reg,
> >> + int offset, int size, int is_last)
> >
> > You know cw, you have access to chip->ecc.steps, so you can derive by
> > yourself if is_last is set or not. No need to forward it through
> > function calls.
>
>
>This "is_last" is not for last code word, it will indicate the Location
> register "NAND_READ_LOCATION_n" last bit.
Ok, I've mixed two things. Let's keep this boolean as it is for now and
just do the minimum changes to support the LOCATION_LAST_cw registers.
Nevertheless, can't you calculate is_last from nandc_set_read_loc() ?
I also think a bit of renaming (in a different patch) would be welcome
to avoid such confusions.
Just to be clear: I think you should take a step back, and try to
simplify a bit this driver. I understand you know every character by
heart but with an external eye it's not that easy to understand what
you want to do and why:
- write small commits with a single, atomic change
- try to reduce the number of parameters when it is possible
- try to use meaningful names (is_last vs. LAST_CW)
- try to avoid extra indentation level when possible
[...]
> >> @@ -1094,11 +1144,19 @@ static void
config_nand_page_read(struct
>> qcom_nand_controller *nandc)
> >> * before reading each codeword in NAND page.
> >> */
> >> static void
> >> -config_nand_cw_read(struct qcom_nand_controller *nandc, bool use_ecc)
> >> +config_nand_cw_read(struct nand_chip *chip, bool use_ecc, int cw)
> >> {
> >> - if (nandc->props->is_bam)
> >> - write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
> >> -NAND_BAM_NEXT_SGL);
> >> + struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> >> + struct nand_ecc_ctrl *ecc = >ecc;
> >> +
> >> + if (nandc->props->is_bam) {
> >> + if (nandc->props->qpic_v2 && cw == (ecc->steps - 1))
> >> + write_reg_dma(nandc, NAND_READ_LOCATION_LAST_CW_0, 4,
> >> +NAND_BAM_NEXT_SGL);
> >> + else
> >> + write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
> >> +NAND_BAM_NEXT_SGL);
> >> + }
> >
> > Same here, I am pretty sure we can abstract the complexity.
> >
> Here I did this because , i need pointer to struct nand_ecc_ctrl structure
> to access ecc->steps for CW comparison for last code word. cw ==
> (ecc->steps - 1)
>
> So i think no separate patch needed for conversion of nanc-->chip.
> Please let me know if still separate patch needed for nanc-->chip
> conversion.
I was talking about the extra indentation level.
the "qpic_v2 && cv == ..." condition can be checked by write_reg_dma
directly.
You could even introduce a helper returning the boolean value of which
register should be used.
Regarding the use of nand_chip instead of nandc, if there are too many
changes involved, I prefer a separate patch.
>
> >> >> write_reg_dma(nandc, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL);
> >>write_reg_dma(nandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL);
> >> @@ -1117,11 +1175,11 @@ config_nand_cw_read(struct >> qcom_nand_controller
> >> *nandc, bool use_ecc)
> >> * single codeword in page
> >> */
> >> static void
> >> -config_nand_single_cw_page_read(struct qcom_nand_controller *nandc,
> >> - bool use_ecc)
> >> +config_nand_single_cw_page_read(struct nand_chip *chip,
> >> + bool use_ecc, int cw)
> >> {
> >> - config_nand_page_read(nandc);
> >> - config_nand_cw_read(nandc, use_ecc);
> >> + config_nand_page_read(chip);
> >> + config_nand_cw_read(chip, use_ecc, cw);
> >> }
> >> >> /*
> >> @@ -1205,7 +1263,7 @@ static int nandc_param(struct qcom_nand_host >>
> >> *host)
> >>nandc_set_reg(nandc, NAND_DEV_CMD_VLD_RESTORE, nandc->vld);
> >>}
> >> >> - nandc_set_read_loc(nandc, 0, 0, 512, 1);
> >> + nandc_set_read_loc(chip, 0, 0, 0, 512, 1);
> >> >> if (!nandc->props->qpic_v2) {
> >>write_reg_dma(nandc, NAND_DEV_CMD_VLD, 1, 0);
> >> @@ -1215,7 +1273,7 @@ static int nandc_param(struct qcom_nand_host >>
> >> *host)
> >>nandc->buf_count = 512;
> >>memset(nandc->data_buffer, 0xff, nandc->buf_count);
> >> >> - config_nand_single_cw_page_read(nandc, false);
> >> + config_nand_single_cw_page_read(chip, false, 0);
> >> >> read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer,
> >> nandc->buf_count, 0);
> >> @@ -1617,7 +1675,7 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd, >>
> >> struct nand_chip *chip,
> >>clear_bam_transaction(nandc);
> >>set_address(host, host->cw_size * cw, page);
> >>update_rw_regs(host, 1, true);
> >> - config_nand_page_read(nandc);
> >> + config_nand_page_read(chip);
> >> >> data_size1 = mtd->writesize -
Re: [PATCH] mtd: rawnand: qcom: update last code word register
Hello,
Md Sadre Alam wrote on Tue, 16 Feb 2021
00:46:42 +0530:
> From QPIC version 2.0 onwards new register got added to
> read last codeword. This change will add the READ_LOCATION_LAST_CW_n
> register.
>
> For first three code word READ_LOCATION_n register will be
> use.For last code word READ_LOCATION_LAST_CW_n register will be
> use.
>
> Signed-off-by: Md Sadre Alam
> ---
> [V6]
It is also very important that you mark the version in the subject:
[PATCH v6] mtd: rawnand: etc
Otherwise it is difficult to keep track on the changes.
> * Updated write_reg_dma() function in "v6"
> * Removed extra indentation level in read_page_ecc() to read last code word
> in "v6"
> * Removed boolean check in config_nand_cw_read() in "v6"
> drivers/mtd/nand/raw/qcom_nandc.c | 118
> --
> 1 file changed, 87 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c
> b/drivers/mtd/nand/raw/qcom_nandc.c
> index 667e4bf..bae352f 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -48,6 +48,10 @@
> #define NAND_READ_LOCATION_10xf24
> #define NAND_READ_LOCATION_20xf28
> #define NAND_READ_LOCATION_30xf2c
> +#define NAND_READ_LOCATION_LAST_CW_00xf40
> +#define NAND_READ_LOCATION_LAST_CW_10xf44
> +#define NAND_READ_LOCATION_LAST_CW_20xf48
> +#define NAND_READ_LOCATION_LAST_CW_30xf4c
>
> /* dummy register offsets, used by write_reg_dma */
> #define NAND_DEV_CMD1_RESTORE 0xdead
> @@ -181,8 +185,14 @@
> #define ECC_BCH_4BITBIT(2)
> #define ECC_BCH_8BITBIT(3)
>
> -#define nandc_set_read_loc(nandc, reg, offset, size, is_last)\
> -nandc_set_reg(nandc, NAND_READ_LOCATION_##reg, \
> +#define nandc_set_read_loc_first(nandc, reg, offset, size, is_last) \
> +nandc_set_reg(nandc, reg,\
> + ((offset) << READ_LOCATION_OFFSET) | \
> + ((size) << READ_LOCATION_SIZE) | \
> + ((is_last) << READ_LOCATION_LAST))
> +
> +#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last) \
> +nandc_set_reg(nandc, reg,\
> ((offset) << READ_LOCATION_OFFSET) | \
> ((size) << READ_LOCATION_SIZE) | \
> ((is_last) << READ_LOCATION_LAST))
> @@ -316,6 +326,10 @@ struct nandc_regs {
> __le32 read_location1;
> __le32 read_location2;
> __le32 read_location3;
> + __le32 read_location_last0;
> + __le32 read_location_last1;
> + __le32 read_location_last2;
> + __le32 read_location_last3;
>
> __le32 erased_cw_detect_cfg_clr;
> __le32 erased_cw_detect_cfg_set;
> @@ -644,6 +658,14 @@ static __le32 *offset_to_nandc_reg(struct nandc_regs
> *regs, int offset)
> return >read_location2;
> case NAND_READ_LOCATION_3:
> return >read_location3;
> + case NAND_READ_LOCATION_LAST_CW_0:
> + return >read_location_last0;
> + case NAND_READ_LOCATION_LAST_CW_1:
> + return >read_location_last1;
> + case NAND_READ_LOCATION_LAST_CW_2:
> + return >read_location_last2;
> + case NAND_READ_LOCATION_LAST_CW_3:
> + return >read_location_last3;
> default:
> return NULL;
> }
> @@ -661,6 +683,26 @@ static void nandc_set_reg(struct qcom_nand_controller
> *nandc, int offset,
> *reg = cpu_to_le32(val);
> }
>
> +/* helper to configure location register values */
> +static void nandc_set_read_loc(struct nand_chip *chip, int cw, int reg,
> +int offset, int size, int is_last)
You know cw, you have access to chip->ecc.steps, so you can derive by
yourself if is_last is set or not. No need to forward it through
function calls.
> +{
> + struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> + struct nand_ecc_ctrl *ecc = >ecc;
> +
> + int loc = NAND_READ_LOCATION_0;
> +
> + if (nandc->props->qpic_v2 && cw == (ecc->steps - 1))
> + loc = NAND_READ_LOCATION_LAST_CW_0;
> +
> + loc += reg * 4;
> +
> + if (nandc->props->qpic_v2 && cw == (ecc->steps - 1))
Just compute is_last a single time at the top of the helper and use it.
> + return nandc_set_read_loc_last(nandc, loc, offset, size,
> is_last);
> + else
> + return nandc_set_read_loc_first(nandc, loc, offset, size,
> is_last);
> +}
> +
> /* helper to configure address register values */
> static void set_address(struct qcom_nand_host *host, u16 column, int page)
> {
> @@ -685,6 +727,7 @@ static void update_rw_regs(struct qcom_nand_host *host,
> int num_cw, bool read)
> {
> struct nand_chip *chip = >chip;
> struct qcom_nand_controller *nandc =
Re: [PATCH v2] mtd: spinand: add support for Foresee FS35ND01G-S1Y2
Hi Daniel,
Daniel Palmer wrote on Mon, 15 Feb 2021 19:53:13
+0900:
> Hi Miquel,
>
> On Mon, 15 Feb 2021 at 19:24, Miquel Raynal wrote:
> >
> > Can you please add a changelog here when you send a new version of a
> > patch?
>
> Sorry, I was going to add a cover letter but elsewhere got told that
> one isn't needed for a single patch..
A cover letter is useful when there are many patches, or when there is
some context that is important to remember.
But a changelog should always be added when you change something
between two versions. And the changelog can be located below the three
dashes ("---") without being part of the final commit message, it does
not need to be in a separate cover letter.
> Basically I changed FS35ND01G to FS35ND01G-S1Y2 as that's the proper
> part number for the chip I have and there seem to be a few variations
> of this.
> Aside from that I fixed up the hex numbers to be uppercase and added
> the oob layout callbacks.
>
> > > +static int fs35nd01g_s1y2_ooblayout_free(struct mtd_info *mtd, int
> > > section,
> > > + struct mtd_oob_region *region)
> > > +{
> > > + if (section > 3)
> > > + return -ERANGE;
> > > +
> > > + /*
> > > + * No ECC data is stored in the accessible OOB so the full 16 bytes
> > > + * of each spare region is available to the user. Apparently also
> > > + * covered by the internal ECC.
> >
> > How is this even possible? ECC must be stored somewhere, maybe it is
> > not possible to retrieve it but I guess you cannot use the 32 bytes of
> > OOB for user data. Can you please verify that?
>
> This worried me too as I could not find the OOB layout anywhere.
> They simply list there being 4 512 byte main areas and then 4 16 byte
> spare areas. The only other note is that the first byte of spare0 is
> used for the bad block marker.
>
> I contacted Longsys but they didn't get back to me.
> So what I did here was I started googling strings within the datasheet
> to find other chips that are probably the same IP inside and I found
> the FM25G01.
> It's datasheet shares a lot of the same text and the flash layout
> diagrams etc are the same.
> It has the same table for the flash layout. 4 512 byte areas and 4 16
> byte spare areas. It has the same note for the bad block marker and
> then one additional note:
>
> "2. Spare area 800H to 83FH is all available for user.
> ECC parity codes are programmed in
> additional space and not user accessible."
>
> It would seem that the pages are actually bigger than 2K + 64 or there
> is some other place they keep the ECC.
> Or both datasheets are lying. Somewhere else in the datasheets it says
> that writes to the ECC area will be ignored but that doesn't make a
> lot of sense if the ECC area isn't user accessible in the first place.
>
> I didn't think about it at the time but I can take a dump of the OOB
> area of my FS35ND01G-S1Y2 to confirm it's all 0xff except for any
> factory marked bad blocks.
I see. Can you please try the following:
nandwrite -o /dev/mtdx /dev/zero
nanddump -ol1 /dev/mtdx
If the entire area is effectively free to be used, you should see 0's
everywhere. Otherwise you should have ff's somewhere.
Thanks,
Miquèl
Re: [PATCH v2] mtd: spinand: add support for Foresee FS35ND01G-S1Y2
Hi Daniel,
Daniel Palmer wrote on Sat, 13 Feb 2021 18:57:24
+0900:
> Add support for the Foresee FS35ND01G-S1Y2 manufactured by Longsys.
>
> Signed-off-by: Daniel Palmer
>
> Link:
> https://datasheet.lcsc.com/szlcsc/2008121142_FORESEE-FS35ND01G-S1Y2QWFI000_C719495.pdf
> ---
Can you please add a changelog here when you send a new version of a
patch?
> drivers/mtd/nand/spi/Makefile | 2 +-
> drivers/mtd/nand/spi/core.c| 1 +
> drivers/mtd/nand/spi/longsys.c | 86 ++
> include/linux/mtd/spinand.h| 1 +
> 4 files changed, 89 insertions(+), 1 deletion(-)
> create mode 100644 drivers/mtd/nand/spi/longsys.c
>
> diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
> index 9662b9c1d5a9..1d6819022e43 100644
> --- a/drivers/mtd/nand/spi/Makefile
> +++ b/drivers/mtd/nand/spi/Makefile
> @@ -1,3 +1,3 @@
> # SPDX-License-Identifier: GPL-2.0
> -spinand-objs := core.o gigadevice.o macronix.o micron.o paragon.o toshiba.o
> winbond.o
> +spinand-objs := core.o gigadevice.o longsys.o macronix.o micron.o paragon.o
> toshiba.o winbond.o
> obj-$(CONFIG_MTD_SPI_NAND) += spinand.o
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 61d932c1b718..8c36f0f6b1c9 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -864,6 +864,7 @@ static const struct nand_ops spinand_ops = {
>
> static const struct spinand_manufacturer *spinand_manufacturers[] = {
> _spinand_manufacturer,
> + _spinand_manufacturer,
> _spinand_manufacturer,
> _spinand_manufacturer,
> _spinand_manufacturer,
> diff --git a/drivers/mtd/nand/spi/longsys.c b/drivers/mtd/nand/spi/longsys.c
> new file mode 100644
> index ..418bd5a1f20d
> --- /dev/null
> +++ b/drivers/mtd/nand/spi/longsys.c
> @@ -0,0 +1,86 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 Daniel Palmer
> + *
> + */
> +
> +#include
> +#include
> +#include
> +
> +#define SPINAND_MFR_LONGSYS 0xCD
> +
> +static SPINAND_OP_VARIANTS(read_cache_variants,
> + SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
> + SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
> +
> +static SPINAND_OP_VARIANTS(write_cache_variants,
> + SPINAND_PROG_LOAD(true, 0, NULL, 0));
> +
> +static SPINAND_OP_VARIANTS(update_cache_variants,
> + SPINAND_PROG_LOAD(false, 0, NULL, 0));
> +
> +static int fs35nd01g_s1y2_ooblayout_ecc(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *region)
> +{
> + if (section > 3)
> + return -ERANGE;
> +
> + /* ECC is not user accessible */
> + region->offset = 0;
> + region->length = 0;
> +
> + return 0;
> +}
> +
> +static int fs35nd01g_s1y2_ooblayout_free(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *region)
> +{
> + if (section > 3)
> + return -ERANGE;
> +
> + /*
> + * No ECC data is stored in the accessible OOB so the full 16 bytes
> + * of each spare region is available to the user. Apparently also
> + * covered by the internal ECC.
How is this even possible? ECC must be stored somewhere, maybe it is
not possible to retrieve it but I guess you cannot use the 32 bytes of
OOB for user data. Can you please verify that?
> + */
> + if (section) {
> + region->offset = 16 * section;
> + region->length = 16;
> + } else {
> + /* First byte in spare0 area is used for bad block marker */
> + region->offset = 1;
> + region->length = 15;
> + }
> +
> + return 0;
> +}
> +
> +static const struct mtd_ooblayout_ops fs35nd01g_s1y2_ooblayout = {
> + .ecc = fs35nd01g_s1y2_ooblayout_ecc,
> + .free = fs35nd01g_s1y2_ooblayout_free,
> +};
> +
> +static const struct spinand_info longsys_spinand_table[] = {
> + SPINAND_INFO("FS35ND01G-S1Y2",
> + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xEA, 0x11),
> + NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1),
> + NAND_ECCREQ(4, 512),
> + SPINAND_INFO_OP_VARIANTS(_cache_variants,
> + _cache_variants,
> + _cache_variants),
> + SPINAND_HAS_QE_BIT,
> + SPINAND_ECCINFO(_s1y2_ooblayout,
> + NULL)),
> +};
> +
> +static const struct spinand_manufacturer_ops longsys_spinand_manuf_ops = {
> +};
> +
> +const struct spinand_manufacturer longsys_spinand_manufacturer = {
> + .id = SPINAND_MFR_LONGSYS,
> + .name = "Longsys",
> + .chips = longsys_spinand_table,
> + .nchips = ARRAY_SIZE(longsys_spinand_table),
> + .ops = _spinand_manuf_ops,
> +};
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index
Re: [PATCH] mtd: rawnand: qcom: update last code word register
Hello,
Md Sadre Alam wrote on Mon, 15 Feb 2021
02:47:31 +0530:
> From QPIC version 2.0 onwards new register got added to
> read last codeword. This change will add the READ_LOCATION_LAST_CW_n
> register.
>
> For first three code word READ_LOCATION_n register will be
> use.For last code word READ_LOCATION_LAST_CW_n register will be
> use.
>
> Signed-off-by: Md Sadre Alam
> ---
> [V5]
> * Added helper function to update location register value.
Please don't forget the "v5" in the message object.
> /*
> @@ -1094,11 +1141,16 @@ static void config_nand_page_read(struct
> qcom_nand_controller *nandc)
> * before reading each codeword in NAND page.
> */
> static void
> -config_nand_cw_read(struct qcom_nand_controller *nandc, bool use_ecc)
> +config_nand_cw_read(struct qcom_nand_controller *nandc, bool use_ecc, bool
> last_cw)
> {
> - if (nandc->props->is_bam)
> - write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
> - NAND_BAM_NEXT_SGL);
> + if (nandc->props->is_bam) {
> + if (nandc->props->qpic_v2 && last_cw)
> + write_reg_dma(nandc, NAND_READ_LOCATION_LAST_CW_0, 4,
> + NAND_BAM_NEXT_SGL);
> + else
> + write_reg_dma(nandc, NAND_READ_LOCATION_0, 4,
> + NAND_BAM_NEXT_SGL);
I guess write_reg_dma should be updated as well.
[...]
>
> - config_nand_cw_read(nandc, false);
> + config_nand_cw_read(nandc, false, cw == ecc->steps - 1 ? true : false);
>
> read_data_dma(nandc, reg_off, data_buf, data_size1, 0);
> reg_off += data_size1;
> @@ -1873,18 +1938,31 @@ static int read_page_ecc(struct qcom_nand_host *host,
> u8 *data_buf,
>
> if (nandc->props->is_bam) {
> if (data_buf && oob_buf) {
> - nandc_set_read_loc(nandc, 0, 0, data_size, 0);
> - nandc_set_read_loc(nandc, 1, data_size,
> -oob_size, 1);
> + if (nandc->props->qpic_v2 && i == (ecc->steps -
> 1)) {
I would like the helper to handle this condition. I would prefer to
avoid yet an extra indentation level.
> + nandc_set_read_loc(chip, i, 0, 0,
> data_size, 0);
> + nandc_set_read_loc(chip, i, 1,
> data_size,
> +oob_size, 1);
> + } else {
> + nandc_set_read_loc(chip, i, 0, 0,
> data_size, 0);
> + nandc_set_read_loc(chip, i, 1,
> data_size,
> +oob_size, 1);
> + }
> } else if (data_buf) {
> - nandc_set_read_loc(nandc, 0, 0, data_size, 1);
> + if (nandc->props->qpic_v2 && i == (ecc->steps -
> 1))
> + nandc_set_read_loc(chip, i, 0, 0,
> data_size, 1);
> + else
> + nandc_set_read_loc(chip, i, 0, 0,
> data_size, 1);
> } else {
> - nandc_set_read_loc(nandc, 0, data_size,
> -oob_size, 1);
> + if (nandc->props->qpic_v2 && i == (ecc->steps -
> 1))
> + nandc_set_read_loc(chip, i, 0,
> data_size,
> +oob_size, 1);
> + else
> + nandc_set_read_loc(chip, i, 0,
> data_size,
> +oob_size, 1);
> }
> }
>
> - config_nand_cw_read(nandc, true);
> + config_nand_cw_read(nandc, true, i == ecc->steps - 1 ? true :
> false);
i == (ecc->steps - 1)
is already a boolean, you don't need
"? true : false"
>
> if (data_buf)
> read_data_dma(nandc, FLASH_BUF_ACC, data_buf,
> @@ -1946,7 +2024,7 @@ static int copy_last_cw(struct qcom_nand_host *host,
> int page)
> set_address(host, host->cw_size * (ecc->steps - 1), page);
> update_rw_regs(host, 1, true);
>
> - config_nand_single_cw_page_read(nandc, host->use_ecc);
> + config_nand_single_cw_page_read(nandc, host->use_ecc, true);
Maybe it's best to just forward the codeword and let the code that
needs to know if it is the last one or not do the comparison.
>
> read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer, size, 0);
>
Thanks,
Miquèl
Re: [PATCH] mtd: physmap: physmap-bt1-rom: Fix unintentional stack access
Hi Gustavo, "Gustavo A. R. Silva" wrote on Fri, 12 Feb 2021 08:45:33 -0600: > On 2/12/21 08:12, Miquel Raynal wrote: > > Hi Gustavo, > > > > "Gustavo A. R. Silva" wrote on Fri, 12 Feb 2021 > > 04:40:22 -0600: > > > >> Cast to (char *) in order to avoid unintentionally accessing > >> the stack. > >> > >> Notice that data is of type u32, so any increment to > >> will be in the order of 4-byte chunks, and this piece of code > >> is actually intended to be a byte offset. > > > > I don't have the same reading. I don't say that Coverity report is > > wrong, but let's discuss this a bit further. > > > > Given that is of type u32 *, you say that " + shift" > > produces increments of 4-bytes, ie. we would access " + 4 * > > shift"? Because I don't think this is the case (again, I may be wrong). > > Yep; this is pointer arithmetic. If you have an object ptr of type u32 *: > > u32 *ptr; > > and let's say it points to address 100. If you increment it by one: > > ptr++ > > ptr will now point to address 104, not to 101. > > Now, if instead, you first cast ptr to 'char *' and increment it by 1, > then it will point to address 101. Yep, I got confused with the proper addition compared to dereferencing. Patch looks legitimate. Thanks, Miquèl
Re: [PATCH] mtd: physmap: physmap-bt1-rom: Fix unintentional stack access
Hi Gustavo,
"Gustavo A. R. Silva" wrote on Fri, 12 Feb 2021
04:40:22 -0600:
> Cast to (char *) in order to avoid unintentionally accessing
> the stack.
>
> Notice that data is of type u32, so any increment to
> will be in the order of 4-byte chunks, and this piece of code
> is actually intended to be a byte offset.
I don't have the same reading. I don't say that Coverity report is
wrong, but let's discuss this a bit further.
Given that is of type u32 *, you say that " + shift"
produces increments of 4-bytes, ie. we would access " + 4 *
shift"? Because I don't think this is the case (again, I may be wrong).
I'm sure this would be the case if we dereferenced data like
data[shift] though, but in this case I don't see what this cast is
fixing. Can you enlighten me?
Could the out-of-bounds warning come from the fact that shift
might be bigger than the data array spread?
> Fixes: b3e79e7682e0 ("mtd: physmap: Add Baikal-T1 physically mapped ROM
> support")
> Addresses-Coverity-ID: 1497765 ("Out-of-bounds access")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/mtd/maps/physmap-bt1-rom.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/maps/physmap-bt1-rom.c
> b/drivers/mtd/maps/physmap-bt1-rom.c
> index a35450002284..58782cfaf71c 100644
> --- a/drivers/mtd/maps/physmap-bt1-rom.c
> +++ b/drivers/mtd/maps/physmap-bt1-rom.c
> @@ -79,7 +79,7 @@ static void __xipram bt1_rom_map_copy_from(struct map_info
> *map,
> if (shift) {
> chunk = min_t(ssize_t, 4 - shift, len);
> data = readl_relaxed(src - shift);
> - memcpy(to, + shift, chunk);
> + memcpy(to, (char *) + shift, chunk);
> src += chunk;
> to += chunk;
> len -= chunk;
Thanks,
Miquèl
Re: [PATCH V4] mtd: rawnand: qcom: update last code word register
Hello,
mda...@codeaurora.org wrote on Fri, 12 Feb 2021 01:00:47 +0530:
> On 2021-02-11 19:37, Miquel Raynal wrote:
> > Hello,
> >
> > Manivannan Sadhasivam wrote on Wed,
> > 10 Feb 2021 14:31:44 +0530:
> >
> >> On Fri, Jan 29, 2021 at 03:09:19AM +0530, Md Sadre Alam wrote:
> >> > From QPIC version 2.0 onwards new register got added to
> >> > read last codeword. This change will add the READ_LOCATION_LAST_CW_n
> >> > register.
> >> >
> >> > For first three code word READ_LOCATION_n register will be
> >> > use.For last code word READ_LOCATION_LAST_CW_n register will be
> >> > use.
> >
> > Sorry for the late notice, I think the patch is fine but if you don't
> > mind I would like to propose a small change that should simplify your
> > patch a lot, see below.
> >
> >> >
> >> > Signed-off-by: Md Sadre Alam
> >> >> Reviewed-by: Manivannan Sadhasivam
> >> >> Thanks,
> >> Mani
> >> >> > ---
> >> > [V4]
> >> > * Modified condition for nandc_set_read_loc_last() in
> >> > qcom_nandc_read_cw_raw().
> >> > * Added one additional argument "last_cw" to the function
> >> > config_nand_cw_read()
> >> >to handle last code word condition.
> >> > * Changed total number of last code word register
> >> > "NAND_READ_LOCATION_LAST_CW_0" to 4
> >> >while doing code word configuration.
> >> > drivers/mtd/nand/raw/qcom_nandc.c | 110
> >> > +-
> >> > 1 file changed, 84 insertions(+), 26 deletions(-)
> >> >
> >> > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c
> >> > b/drivers/mtd/nand/raw/qcom_nandc.c
> >> > index 667e4bf..9484be8 100644
> >> > --- a/drivers/mtd/nand/raw/qcom_nandc.c
> >> > +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> >> > @@ -48,6 +48,10 @@
> >> > #define NAND_READ_LOCATION_10xf24
> >> > #define NAND_READ_LOCATION_20xf28
> >> > #define NAND_READ_LOCATION_30xf2c
> >> > +#define NAND_READ_LOCATION_LAST_CW_00xf40
> >> > +#define NAND_READ_LOCATION_LAST_CW_10xf44
> >> > +#define NAND_READ_LOCATION_LAST_CW_20xf48
> >> > +#define NAND_READ_LOCATION_LAST_CW_30xf4c
> >> >
> >> > /* dummy register offsets, used by write_reg_dma */
> >> > #define NAND_DEV_CMD1_RESTORE 0xdead
> >> > @@ -187,6 +191,12 @@ nandc_set_reg(nandc, NAND_READ_LOCATION_##reg,
> >> > \
> >> >((size) << READ_LOCATION_SIZE) | \
> >> >((is_last) << READ_LOCATION_LAST))
> >> >
> >> > +#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last)
> >> > \
> >> > +nandc_set_reg(nandc, NAND_READ_LOCATION_LAST_CW_##reg,
> >> > \
> >> > + ((offset) << READ_LOCATION_OFFSET) | \
> >> > + ((size) << READ_LOCATION_SIZE) | \
> >> > + ((is_last) << READ_LOCATION_LAST))
> >> > +
> >
> > You could rename the macro nandc_set_read_loc() into
> > nandc_set_read_loc_first() or anything else that make sense, then have
> > a helper which does:
> >
> > nandc_set_read_loc()
> > {
> > if (condition for first)
> > return nandc_set_read_loc_first();
> > else
> > return nandc_set_read_loc_last();
> > }
> >
>
>Yes this is more precise way & simplify the patch a lot.
>But for this i have to change these two macro as a function.
>
>nandc_set_read_loc() & nandc_set_read_loc_last().
>
>Since for last code word register we are using Token Pasting Operator##.
>
>So if i am implementing like the below.
>
>/* helper to configure location register values */
>static void nandc_set_read_loc(struct qcom_nand_controller *nandc, int reg,
>int offset, int size, int is_last, bool last_cw)
>{
>if (last_cw)
>return nandc_set_read_loc_last(nandc, reg, offset, size,
> is_last);
>else
>return nandc_set_read_loc_first(nandc, reg, offset, size,
> is_last);
> }
>
>So here for macro expansion reg should be a value not a variable else it
> will be expended like
>NAND_READ_LOCATION_LAST_CW_reg instead of
> NAND_READ_LOCATION_LAST_CW_0,1,2,3 etc.
I know it involves a little bit more computation but I wonder if using
funcs instead of macros here would not be nicer? Perhaps something like:
loc = is_last ? NAND_READ_LOCATION /* 0xf20 */ :
NAND_READ_LOCATION_LAST /* 0xf40 */;
loc += reg * 2;
> the call for nandc_set_read_loc() as nandc_set_read_loc(nandc, 0, read_loc,
> data_size1, 0, true); ---> for last code word.
> nandc_set_read_loc(nandc, 0, read_loc, data_size1, 0, false); ---> for
> first three code wrod.
I think it's best to forward 'cw' as a parameter and do the
computation of is_last locally.
> So is this ok for you to convert these two macro into function ?
>
> > And in the rest of your patch you won't have to touch anything else.
> >
> > Thanks,
> > Miquèl
Thanks,
Miquèl
Re: [PATCH V4] mtd: rawnand: qcom: update last code word register
Hello,
Manivannan Sadhasivam wrote on Wed,
10 Feb 2021 14:31:44 +0530:
> On Fri, Jan 29, 2021 at 03:09:19AM +0530, Md Sadre Alam wrote:
> > From QPIC version 2.0 onwards new register got added to
> > read last codeword. This change will add the READ_LOCATION_LAST_CW_n
> > register.
> >
> > For first three code word READ_LOCATION_n register will be
> > use.For last code word READ_LOCATION_LAST_CW_n register will be
> > use.
Sorry for the late notice, I think the patch is fine but if you don't
mind I would like to propose a small change that should simplify your
patch a lot, see below.
> >
> > Signed-off-by: Md Sadre Alam
>
> Reviewed-by: Manivannan Sadhasivam
>
> Thanks,
> Mani
>
> > ---
> > [V4]
> > * Modified condition for nandc_set_read_loc_last() in
> > qcom_nandc_read_cw_raw().
> > * Added one additional argument "last_cw" to the function
> > config_nand_cw_read()
> >to handle last code word condition.
> > * Changed total number of last code word register
> > "NAND_READ_LOCATION_LAST_CW_0" to 4
> >while doing code word configuration.
> > drivers/mtd/nand/raw/qcom_nandc.c | 110
> > +-
> > 1 file changed, 84 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c
> > b/drivers/mtd/nand/raw/qcom_nandc.c
> > index 667e4bf..9484be8 100644
> > --- a/drivers/mtd/nand/raw/qcom_nandc.c
> > +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> > @@ -48,6 +48,10 @@
> > #defineNAND_READ_LOCATION_10xf24
> > #defineNAND_READ_LOCATION_20xf28
> > #defineNAND_READ_LOCATION_30xf2c
> > +#defineNAND_READ_LOCATION_LAST_CW_00xf40
> > +#defineNAND_READ_LOCATION_LAST_CW_10xf44
> > +#defineNAND_READ_LOCATION_LAST_CW_20xf48
> > +#defineNAND_READ_LOCATION_LAST_CW_30xf4c
> >
> > /* dummy register offsets, used by write_reg_dma */
> > #defineNAND_DEV_CMD1_RESTORE 0xdead
> > @@ -187,6 +191,12 @@ nandc_set_reg(nandc, NAND_READ_LOCATION_##reg,
> > \
> > ((size) << READ_LOCATION_SIZE) | \
> > ((is_last) << READ_LOCATION_LAST))
> >
> > +#define nandc_set_read_loc_last(nandc, reg, offset, size, is_last) \
> > +nandc_set_reg(nandc, NAND_READ_LOCATION_LAST_CW_##reg,
> > \
> > + ((offset) << READ_LOCATION_OFFSET) | \
> > + ((size) << READ_LOCATION_SIZE) | \
> > + ((is_last) << READ_LOCATION_LAST))
> > +
You could rename the macro nandc_set_read_loc() into
nandc_set_read_loc_first() or anything else that make sense, then have
a helper which does:
nandc_set_read_loc()
{
if (condition for first)
return nandc_set_read_loc_first();
else
return nandc_set_read_loc_last();
}
And in the rest of your patch you won't have to touch anything else.
Thanks,
Miquèl
Re: [PATCH 0/8] MUSE: Userspace backed MTD v3
Hi Richard, Richard Weinberger wrote on Wed, 10 Feb 2021 12:23:53 +0100 (CET): > Miquel, > > - Ursprüngliche Mail - > >> Does in-band and OOB data need to be handled together? > > > > Short answer: yes. > > > >> If so, then two requests is not a good option. > > > > More detailed answer: > > > > There is a type of MTD device (NAND devices) which are composed, for > > each page, of X in-band bytes plus Y out-of-band metadata bytes. > > > > Accessing either the in-band data, or the out-of-band data, or both at > > the same time are all valid use cases. > > > > * Read operation details: > > From a hardware point of view, the out-of-band data is (almost) > > always retrieved when the in-band data is read because it contains > > meta-data used to correct eventual bitflips. In this case, if both > > areas are requested, it is highly non-efficient to do two requests, > > that's why the MTD core allows to do both at the same time. > > * Write operation details: > > Even worse, in the write case, you *must* write both at the same > > time. It is physically impossible to do one after the other (still > > with actual hardware, of course). > > > > That is why it is preferable that MUSE will be able to access both in > > a single request. > > By single request we meant FUSE op-codes. The NAND simulator in Userspace > will see just one call. My plan is to abstract it in libfuse. If libfuse abstracts it, as long as MTD only sees a single request I'm fine :) Thanks, Miquèl
Re: [PATCH 0/8] MUSE: Userspace backed MTD v3
Hi Miklos, Miklos Szeredi wrote on Wed, 10 Feb 2021 11:16:45 +0100: > On Tue, Feb 9, 2021 at 10:39 PM Richard Weinberger wrote: > > > > Miklos, > > > > - Ursprüngliche Mail - > > > If you look at fuse_do_ioctl() it does variable length input and > > > output at the same time. I guess you need something similar to that. > > > > I'm not sure whether I understand correctly. > > > > In MUSE one use case would be attaching two distinct (variable length) > > buffers to a > > single FUSE request, in both directions. > > If I read fuse_do_ioctl() correctly, it attaches always a single buffer per > > request > > but does multiple requests. > > Right. > > > In MUSE we cold go the same path and issue up to two requests. > > One for in-band and optionally a second one for the out-of-band data. > > Hmmm? > > Does in-band and OOB data need to be handled together? Short answer: yes. > If so, then two requests is not a good option. More detailed answer: There is a type of MTD device (NAND devices) which are composed, for each page, of X in-band bytes plus Y out-of-band metadata bytes. Accessing either the in-band data, or the out-of-band data, or both at the same time are all valid use cases. * Read operation details: From a hardware point of view, the out-of-band data is (almost) always retrieved when the in-band data is read because it contains meta-data used to correct eventual bitflips. In this case, if both areas are requested, it is highly non-efficient to do two requests, that's why the MTD core allows to do both at the same time. * Write operation details: Even worse, in the write case, you *must* write both at the same time. It is physically impossible to do one after the other (still with actual hardware, of course). That is why it is preferable that MUSE will be able to access both in a single request. Thanks, Miquèl
Re: [PATCH] mtd: rawnand: qcom: Update register macro name for 0x2c offset
Hello, mda...@codeaurora.org wrote on Fri, 05 Feb 2021 23:26:33 +0530: > On 2021-01-31 01:37, Md Sadre Alam wrote: > > This change will remove unused register name macro NAND_DEV1_ECC_CFG. > > Since this register was only available in QPIC version 1.4.20 ipq40xx > > and it was not used. In QPIC version 1.5 on wards this register got > > removed.In QPIC version 2.0 0x2c offset is updated with register > > NAND_AUTO_STATUS_EN So adding this register macro NAND_AUTO_STATUS_EN > > with offset 0x2c. > > > > Signed-off-by: Md Sadre Alam > >Ping! Is any additional info needed for this patch ? The patch is fine but we are at -rc6, the NAND PR has already been sent, I don't plan to add more patches for this release. I will apply new patches at v5.12-rc1. Thanks, Miquèl
Re: [PATCH] drivers: mtd: Better word replace a not so good word in the file mtd_blkdevs.c
Hi Bhaskar, Bhaskar Chowdhury wrote on Fri, 5 Feb 2021 19:06:39 +0530: > On 14:26 Fri 05 Feb 2021, Miquel Raynal wrote: > >Hi Bhaskar, > > > >Bhaskar Chowdhury wrote on Fri, 5 Feb 2021 > >18:11:51 +0530: > > > >> s/fucking/invite/ > >> > >> > >> Signed-off-by: Bhaskar Chowdhury > >> --- > >> drivers/mtd/mtd_blkdevs.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c > >> index fb8e12d590a1..756a0995e474 100644 > >> --- a/drivers/mtd/mtd_blkdevs.c > >> +++ b/drivers/mtd/mtd_blkdevs.c > >> @@ -523,7 +523,7 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *t > r) > >>int ret; > >> > >>/* Register the notifier if/when the first device type is > >> - registered, to prevent the link/init ordering from fucking > >> + registered, to prevent the link/init ordering from invite > >> us over. */ > > > >invite us over? > > > >I'm not a native English speaker but this does not bring any value to > >my ears. Worse, I don't even get the point. Better rewrite the comment > >entirely than just swapping "fucking" with a random word, no? > > > Got your point , and I do understand "fuck" and "fucking" is so strong word > that it is really difficult to replace with something else. > > But..but you suggestion to rewrite the comments sounds good Miquel ...wish I > could have that much time in hand to replace every sentence having that word > ...all the patched sent with that word replaces...please step forward and > make > and send patches . > > Today I sent 10 patches replacing that word(weather that make sense or not) > you can see those in the ML ...please pick up as you wish and send patches. There are currently 21 uses of "fuck[ing]". It's not a mountain to climb. Nor a race. > I was simply replacing the word ..never bother about the meaning it > conveys..doesn't matter really. Are you kidding? What is the purpose of a comment if no one understands it after a blind change? > > so... >
Re: [PATCH] drivers: mtd: Better word replace a not so good word in the file mtd_blkdevs.c
Hi Bhaskar, Bhaskar Chowdhury wrote on Fri, 5 Feb 2021 18:11:51 +0530: > s/fucking/invite/ > > > Signed-off-by: Bhaskar Chowdhury > --- > drivers/mtd/mtd_blkdevs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c > index fb8e12d590a1..756a0995e474 100644 > --- a/drivers/mtd/mtd_blkdevs.c > +++ b/drivers/mtd/mtd_blkdevs.c > @@ -523,7 +523,7 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr) > int ret; > > /* Register the notifier if/when the first device type is > -registered, to prevent the link/init ordering from fucking > +registered, to prevent the link/init ordering from invite > us over. */ invite us over? I'm not a native English speaker but this does not bring any value to my ears. Worse, I don't even get the point. Better rewrite the comment entirely than just swapping "fucking" with a random word, no? Thanks, Miquèl
Re: [PATCH 1/2] spi: spi-mem: add spi_mem_dtr_supports_op()
Hi Pratyush, Pratyush Yadav wrote on Thu, 4 Feb 2021 19:42:17 +0530: > spi_mem_default_supports_op() rejects DTR ops by default to ensure that > the controller drivers that haven't been updated with DTR support > continue to reject them. It also makes sure that controllers that don't > support DTR mode at all (which is most of them at the moment) also > reject them. > > This means that controller drivers that want to support DTR mode can't > use spi_mem_default_supports_op(). Driver authors have to roll their own > supports_op() function and mimic the buswidth checks. See > spi-cadence-quadspi.c for example. Or even worse, driver authors might > skip it completely or get it wrong. > > Add spi_mem_dtr_supports_op(). It provides a basic sanity check for DTR > ops and performs the buswidth requirement check. Move the logic for > checking buswidth in spi_mem_default_supports_op() to a separate > function so the logic is not repeated twice. > > Signed-off-by: Pratyush Yadav I am not a SPI-NOR expert but for what I know this approach looks good to me. Let's see what other maintainers think. Reviewed-by: Miquel Raynal Thanks, Miquèl
Re: [PATCH] mtd: rawnand: Do not check for bad block if bbt is unavailable
Hi Boris, Boris Brezillon wrote on Thu, 4 Feb 2021 10:27:38 +0100: > On Thu, 4 Feb 2021 10:04:08 +0100 > Miquel Raynal wrote: > > > Hi Boris, > > > > Boris Brezillon wrote on Thu, 4 Feb > > 2021 09:59:45 +0100: > > > > > On Thu, 4 Feb 2021 14:22:21 +0530 > > > Manivannan Sadhasivam wrote: > > > > > > > On Thu, Feb 04, 2021 at 09:13:36AM +0100, Miquel Raynal wrote: > > > > > Hi Manivannan, > > > > > > > > > > Manivannan Sadhasivam wrote on Wed, > > > > > 03 Feb 2021 17:11:31 +0530: > > > > > > > > > > > On 3 February 2021 4:54:22 PM IST, Boris Brezillon > > > > > > wrote: > > > > > > >On Wed, 03 Feb 2021 16:22:42 +0530 > > > > > > >Manivannan Sadhasivam wrote: > > > > > > > > > > > > > >> On 3 February 2021 3:49:14 PM IST, Boris Brezillon > > > > > > > wrote: > > > > > > >> >On Wed, 03 Feb 2021 15:42:02 +0530 > > > > > > >> >Manivannan Sadhasivam wrote: > > > > > > >> > > > > > > > >> >> >> > > > > > > >> >> >> I got more information from the vendor, Telit. The access > > > > > > >> >> >> to > > > > > > >the > > > > > > >> >3rd > > > > > > >> >> >partition is protected by Trustzone and any access in non > > > > > > >> >> > > > > > > > >privileged > > > > > > >> >> >mode (where Linux kernel runs) causes kernel panic and the > > > > > > >> >> >device > > > > > > >> >> >reboots. > > > > > > >> > > > > > > > >> >Out of curiosity, is it a per-CS-line thing or is this section > > > > > > >> >protected on all CS? > > > > > > >> > > > > > > > >> > > > > > > >> Sorry, I didn't get your question. > > > > > > > > > > > > > >The qcom controller can handle several chips, each connected > > > > > > >through a > > > > > > >different CS (chip-select) line, right? I'm wondering if the > > > > > > >firmware > > > > > > >running in secure mode has the ability to block access for a > > > > > > >specific > > > > > > >CS line or if all CS lines have the same constraint. That will > > > > > > >impact > > > > > > >the way you describe it in your DT (in one case the secure-region > > > > > > >property should be under the controller node, in the other case it > > > > > > >should be under the NAND chip node). > > > > > > > > > > > > Right. I believe the implementation is common to all NAND chips so > > > > > > the property should be in the controller node. > > > > > > > > > > Looks weird: do you mean that each of the chips will have a secure > > > > > area? > > > > > > > > I way I said is, the "secure-region" property will be present in the > > > > controller > > > > node and not in the NAND chip node since this is not related to the > > > > device > > > > functionality. > > > > > > > > But for referencing the NAND device, the property can have the phandle > > > > as below: > > > > > > > > secure-region = < 0x>; > > > > > > My question was really what happens from a functional PoV. If you have > > > per-chip protection at the FW level, this property should be under the > > > NAND node. OTH, if the FW doesn't look at the selected chip before > > > blocking the access, it should be at the controller level. So, you > > > really have to understand what the secure FW does. > > > > I'm not so sure actually, that's why I like the phandle to nand0 -> in > > any case it's not a property of the NAND chip itself, it's kind of a > > host constraint, so I don't get why the property should be at the > > NAND node level? > > I would argue that we already have plenty of NAND properties that > encode things controlled by the host (ECC, partitions, HW randomizer, > boot device, and all kind of controller specific stuff) :P. Having > the props under the NAND node makes it clear what those things are > applied to, and it's also easier to parse for the driver (you already > have to parse each node to get the reg property anyway). Fair points. > > Also, we should probably support several secure regions (which could be > > a way to express the fact that the FW does not look at the CS)? > > Sure, the secure-region should probably be renamed secure-regions, even > if it's defined at the NAND chip level. Absolutely. Thanks, Miquèl
