Am 08.04.2024 um 10:36 hat Philippe Mathieu-Daudé geschrieben: > Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org> > --- > 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