Re: [PATCH-for-9.0? 1/3] hw/block/nand: Factor nand_load_iolen() method out
Am 08.04.2024 um 10:36 hat Philippe Mathieu-Daudé geschrieben:
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/block/nand.c | 32 +++-
> 1 file changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/hw/block/nand.c b/hw/block/nand.c
> index d1435f2207..6fa9038bb5 100644
> --- a/hw/block/nand.c
> +++ b/hw/block/nand.c
> @@ -243,9 +243,25 @@ static inline void nand_pushio_byte(NANDFlashState *s,
> uint8_t value)
> }
> }
>
> +/*
> + * nand_load_block: Load block containing (s->addr + @offset).
> + * Returns length of data available at @offset in this block.
> + */
> +static int nand_load_block(NANDFlashState *s, int offset)
> +{
> +int iolen;
> +
> +s->blk_load(s, s->addr, offset);
Wouldn't it make more sense for @offset to be unsigned, like in
nand_command() before this patch?
I think the values are guaranteed to be small enough to fit in either
signed or unsigned, but we never check for < 0 (maybe that should be
done in your patch to s->blk_load() anyway).
> +iolen = (1 << s->page_shift) - offset;
This is not new, but I'm confused. Can this legitimately be negative?
offset seems to be limited to (1 << s->addr_shift) + s->offset in
practice, but addr_shift > page_shift for NAND_PAGE_SIZE == 2048.
After patch 3, offset is implicitly limited to NAND_PAGE_SIZE + OOB_SIZE
because we return early if s->blk_load() fails. I wonder if it wouldn't
be better to catch this in the callers already and only assert in
s->blk_load().
Anyway, after patch 3 iolen can only temporarily become negative here...
> +if (s->gnd) {
> +iolen += 1 << s->oob_shift;
...before it becomes non-negative again here.
> +}
> +return iolen;
> +}
So none of this makes the code technically incorrect after applying the
full series, but let someone modify it who doesn't understand these
intricacies and I think chances are that it will fall apart.
Kevin
Re: [PATCH-for-9.0? 1/3] hw/block/nand: Factor nand_load_iolen() method out
On 4/7/24 22:36, Philippe Mathieu-Daudé wrote: Signed-off-by: Philippe Mathieu-Daudé --- hw/block/nand.c | 32 +++- 1 file changed, 19 insertions(+), 13 deletions(-) Reviewed-by: Richard Henderson r~
[PATCH-for-9.0? 1/3] hw/block/nand: Factor nand_load_iolen() method out
Signed-off-by: Philippe Mathieu-Daudé
---
hw/block/nand.c | 32 +++-
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/hw/block/nand.c b/hw/block/nand.c
index d1435f2207..6fa9038bb5 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -243,9 +243,25 @@ static inline void nand_pushio_byte(NANDFlashState *s,
uint8_t value)
}
}
+/*
+ * nand_load_block: Load block containing (s->addr + @offset).
+ * Returns length of data available at @offset in this block.
+ */
+static int nand_load_block(NANDFlashState *s, int offset)
+{
+int iolen;
+
+s->blk_load(s, s->addr, offset);
+
+iolen = (1 << s->page_shift) - offset;
+if (s->gnd) {
+iolen += 1 << s->oob_shift;
+}
+return iolen;
+}
+
static void nand_command(NANDFlashState *s)
{
-unsigned int offset;
switch (s->cmd) {
case NAND_CMD_READ0:
s->iolen = 0;
@@ -271,12 +287,7 @@ static void nand_command(NANDFlashState *s)
case NAND_CMD_NOSERIALREAD2:
if (!(nand_flash_ids[s->chip_id].options & NAND_SAMSUNG_LP))
break;
-offset = s->addr & ((1 << s->addr_shift) - 1);
-s->blk_load(s, s->addr, offset);
-if (s->gnd)
-s->iolen = (1 << s->page_shift) - offset;
-else
-s->iolen = (1 << s->page_shift) + (1 << s->oob_shift) - offset;
+s->iolen = nand_load_block(s, s->addr & ((1 << s->addr_shift) - 1));
break;
case NAND_CMD_RESET:
@@ -597,12 +608,7 @@ uint32_t nand_getio(DeviceState *dev)
if (!s->iolen && s->cmd == NAND_CMD_READ0) {
offset = (int) (s->addr & ((1 << s->addr_shift) - 1)) + s->offset;
s->offset = 0;
-
-s->blk_load(s, s->addr, offset);
-if (s->gnd)
-s->iolen = (1 << s->page_shift) - offset;
-else
-s->iolen = (1 << s->page_shift) + (1 << s->oob_shift) - offset;
+s->iolen = nand_load_block(s, offset);
}
if (s->ce || s->iolen <= 0) {
--
2.41.0
