| Message ID | 20170424091411.2151-5-cyril.bur@au1.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, 2017-04-24 at 19:14 +1000, Cyril Bur wrote: > Version two of the mbox-flash protocol defines a new command: > MARK_WRITE_ERASED. > > This command provides a simple way to mark a region of flash as all > 0xff > without the need to go and write all 0xff. This is an optimisation as > there is no need for an erase before a write, it is the > responsibility of > the BMC to deal with the flash correctly, however in v1 it was > ambiguous > what a client should do if the flash should be erased but not > actually > written to. This allows of a optimal path to resolve this problem. nak-do-not-apply-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> Yeah so this is wrong in the same way your dirty function was before you fixed it. I would recommend doing what I recommended in the previous patch which is reuse the dirty function to avoid bugs like this being duplicated in the future... > > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> > --- > libflash/mbox-flash.c | 64 > ++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 61 insertions(+), 3 deletions(-) > > diff --git a/libflash/mbox-flash.c b/libflash/mbox-flash.c > index 3dfcebe9..5d29215c 100644 > --- a/libflash/mbox-flash.c > +++ b/libflash/mbox-flash.c > @@ -761,7 +761,64 @@ out: > return rc; > } > > -static int mbox_flash_erase(struct blocklevel_device *bl __unused, > +static int mbox_flash_erase_v2(struct blocklevel_device *bl, > uint64_t pos, > + uint64_t len) > +{ > + uint64_t size; > + struct bmc_mbox_msg *msg; > + struct mbox_flash_data *mbox_flash; > + > + mbox_flash = container_of(bl, struct mbox_flash_data, bl); > + > + prlog(PR_TRACE, "Flash erase at 0x%08x for 0x%08x\n", (u32) > pos, (u32) len); > + while (len > 0) { > + int rc; > + > + /* Move window and get a new size to erase */ > + rc = mbox_window_move(mbox_flash, &mbox_flash- > >write, > + MBOX_C_CREATE_WRITE_WINDOW, > pos, len, &size); > + if (rc) > + return rc; > + > + msg = msg_alloc(mbox_flash, > MBOX_C_MARK_WRITE_ERASED); > + if (!msg) > + return FLASH_ERR_MALLOC_FAILED; > + > + msg_put_u16(msg, 0, pos >> mbox_flash->shift); Are you sure? Isn't pos here a flash index and we want a window index for the argument... > + msg_put_u16(msg, 2, len >> mbox_flash->shift); Are you sure? What if len is less than block_size... (Also shouldn't we be using size anyway) > + rc = msg_send(mbox_flash, msg); > + if (rc) { > + prlog(PR_ERR, "Failed to enqueue/send BMC > MBOX message\n"); > + msg_free_memory(msg); > + return rc; > + } > + > + rc = wait_for_bmc(mbox_flash, MBOX_DEFAULT_TIMEOUT); > + if (rc) { > + prlog(PR_ERR, "Error waiting for BMC\n"); > + msg_free_memory(msg); > + return rc; > + } > + > + msg_free_memory(msg); > + > + /* > + * Flush directly, don't mark that region dirty > otherwise it > + * isn't clear if a write happened there or not > + */ > + > + rc = mbox_flash_flush(mbox_flash); Same as previous patch, can we just flush after we exit the while loop? > + if (rc) > + return rc; > + > + len -= size; > + pos += size; > + } > + > + return 0; > +} > + > +static int mbox_flash_erase_v1(struct blocklevel_device *bl > __unused, > uint64_t pos __unused, uint64_t len __unused) > { > /* > @@ -859,7 +916,7 @@ static int protocol_init(struct mbox_flash_data > *mbox_flash) > /* Assume V2 */ > mbox_flash->bl.read = &mbox_flash_read; > mbox_flash->bl.write = &mbox_flash_write; > - mbox_flash->bl.erase = &mbox_flash_erase; > + mbox_flash->bl.erase = &mbox_flash_erase_v2; > mbox_flash->bl.get_info = &mbox_flash_get_info; > > /* Assume V2 */ > @@ -913,6 +970,7 @@ static int protocol_init(struct mbox_flash_data > *mbox_flash) > > prlog(PR_INFO, "Detected mbox protocol version %d\n", > mbox_flash->version); > if (mbox_flash->version == 1) { > + mbox_flash->bl.erase = &mbox_flash_erase_v1; > /* Not all handlers differ, update those which do */ > mbox_flash->handlers[MBOX_C_GET_MBOX_INFO] = > &mbox_flash_do_get_mbox_info; > mbox_flash->handlers[MBOX_C_GET_FLASH_INFO] = > &mbox_flash_do_get_flash_info_v1; > @@ -956,7 +1014,7 @@ int mbox_flash_init(struct blocklevel_device > **bl) > /* Assume V2 */ > mbox_flash->bl.read = &mbox_flash_read; > mbox_flash->bl.write = &mbox_flash_write; > - mbox_flash->bl.erase = &mbox_flash_erase; > + mbox_flash->bl.erase = &mbox_flash_erase_v2; > mbox_flash->bl.get_info = &mbox_flash_get_info; > > if (bmc_mbox_get_attn_reg() & MBOX_ATTN_BMC_REBOOT)
On Mon, 2017-04-24 at 19:14 +1000, Cyril Bur wrote: > + msg_put_u16(msg, 0, pos >> mbox_flash->shift); > + msg_put_u16(msg, 2, len >> mbox_flash->shift); > + rc = msg_send(mbox_flash, msg); > + if (rc) { Either you do proper alignment as for mark dirty or you verify that the inputs are aligned and error out if not with a warning. Cheers, Ben.
On Tue, 2017-05-02 at 14:28 +1000, Suraj Jitindar Singh wrote: > On Mon, 2017-04-24 at 19:14 +1000, Cyril Bur wrote: > > Version two of the mbox-flash protocol defines a new command: > > MARK_WRITE_ERASED. > > > > This command provides a simple way to mark a region of flash as all > > 0xff > > without the need to go and write all 0xff. This is an optimisation as > > there is no need for an erase before a write, it is the > > responsibility of > > the BMC to deal with the flash correctly, however in v1 it was > > ambiguous > > what a client should do if the flash should be erased but not > > actually > > written to. This allows of a optimal path to resolve this problem. > > nak-do-not-apply-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> > > Yeah so this is wrong in the same way your dirty function was before > you fixed it. > I would recommend doing what I recommended in the previous patch which > is reuse the dirty function to avoid bugs like this being duplicated in > the future... > Yeah I'll combine the two or rather, pull out the interesting bits. Looks like patch 5/5 will need to grow! Thanks. > > > > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> > > --- > > libflash/mbox-flash.c | 64 > > ++++++++++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 61 insertions(+), 3 deletions(-) > > > > diff --git a/libflash/mbox-flash.c b/libflash/mbox-flash.c > > index 3dfcebe9..5d29215c 100644 > > --- a/libflash/mbox-flash.c > > +++ b/libflash/mbox-flash.c > > @@ -761,7 +761,64 @@ out: > > return rc; > > } > > > > -static int mbox_flash_erase(struct blocklevel_device *bl __unused, > > +static int mbox_flash_erase_v2(struct blocklevel_device *bl, > > uint64_t pos, > > + uint64_t len) > > +{ > > + uint64_t size; > > + struct bmc_mbox_msg *msg; > > + struct mbox_flash_data *mbox_flash; > > + > > + mbox_flash = container_of(bl, struct mbox_flash_data, bl); > > + > > + prlog(PR_TRACE, "Flash erase at 0x%08x for 0x%08x\n", (u32) > > pos, (u32) len); > > + while (len > 0) { > > + int rc; > > + > > + /* Move window and get a new size to erase */ > > + rc = mbox_window_move(mbox_flash, &mbox_flash- > > > write, > > > > + MBOX_C_CREATE_WRITE_WINDOW, > > pos, len, &size); > > + if (rc) > > + return rc; > > + > > + msg = msg_alloc(mbox_flash, > > MBOX_C_MARK_WRITE_ERASED); > > + if (!msg) > > + return FLASH_ERR_MALLOC_FAILED; > > + > > + msg_put_u16(msg, 0, pos >> mbox_flash->shift); > > Are you sure? Isn't pos here a flash index and we want a window index > for the argument... Correct, this should be as per the dirty logic > > > + msg_put_u16(msg, 2, len >> mbox_flash->shift); > > Are you sure? What if len is less than block_size... (Also shouldn't we > be using size anyway) > Yeah I think this patch got neglected ... > > + rc = msg_send(mbox_flash, msg); > > + if (rc) { > > + prlog(PR_ERR, "Failed to enqueue/send BMC > > MBOX message\n"); > > + msg_free_memory(msg); > > + return rc; > > + } > > + > > + rc = wait_for_bmc(mbox_flash, MBOX_DEFAULT_TIMEOUT); > > + if (rc) { > > + prlog(PR_ERR, "Error waiting for BMC\n"); > > + msg_free_memory(msg); > > + return rc; > > + } > > + > > + msg_free_memory(msg); > > + > > + /* > > + * Flush directly, don't mark that region dirty > > otherwise it > > + * isn't clear if a write happened there or not > > + */ > > + > > + rc = mbox_flash_flush(mbox_flash); > > Same as previous patch, can we just flush after we exit the while loop? > Not as per my understanding of the spec. > > + if (rc) > > + return rc; > > + > > + len -= size; > > + pos += size; > > + } > > + > > + return 0; > > +} > > + > > +static int mbox_flash_erase_v1(struct blocklevel_device *bl > > __unused, > > uint64_t pos __unused, uint64_t len __unused) > > { > > /* > > @@ -859,7 +916,7 @@ static int protocol_init(struct mbox_flash_data > > *mbox_flash) > > /* Assume V2 */ > > mbox_flash->bl.read = &mbox_flash_read; > > mbox_flash->bl.write = &mbox_flash_write; > > - mbox_flash->bl.erase = &mbox_flash_erase; > > + mbox_flash->bl.erase = &mbox_flash_erase_v2; > > mbox_flash->bl.get_info = &mbox_flash_get_info; > > > > /* Assume V2 */ > > @@ -913,6 +970,7 @@ static int protocol_init(struct mbox_flash_data > > *mbox_flash) > > > > prlog(PR_INFO, "Detected mbox protocol version %d\n", > > mbox_flash->version); > > if (mbox_flash->version == 1) { > > + mbox_flash->bl.erase = &mbox_flash_erase_v1; > > /* Not all handlers differ, update those which do */ > > mbox_flash->handlers[MBOX_C_GET_MBOX_INFO] = > > &mbox_flash_do_get_mbox_info; > > mbox_flash->handlers[MBOX_C_GET_FLASH_INFO] = > > &mbox_flash_do_get_flash_info_v1; > > @@ -956,7 +1014,7 @@ int mbox_flash_init(struct blocklevel_device > > **bl) > > /* Assume V2 */ > > mbox_flash->bl.read = &mbox_flash_read; > > mbox_flash->bl.write = &mbox_flash_write; > > - mbox_flash->bl.erase = &mbox_flash_erase; > > + mbox_flash->bl.erase = &mbox_flash_erase_v2; > > mbox_flash->bl.get_info = &mbox_flash_get_info; > > > > if (bmc_mbox_get_attn_reg() & MBOX_ATTN_BMC_REBOOT) > >
diff --git a/libflash/mbox-flash.c b/libflash/mbox-flash.c index 3dfcebe9..5d29215c 100644 --- a/libflash/mbox-flash.c +++ b/libflash/mbox-flash.c @@ -761,7 +761,64 @@ out: return rc; } -static int mbox_flash_erase(struct blocklevel_device *bl __unused, +static int mbox_flash_erase_v2(struct blocklevel_device *bl, uint64_t pos, + uint64_t len) +{ + uint64_t size; + struct bmc_mbox_msg *msg; + struct mbox_flash_data *mbox_flash; + + mbox_flash = container_of(bl, struct mbox_flash_data, bl); + + prlog(PR_TRACE, "Flash erase at 0x%08x for 0x%08x\n", (u32) pos, (u32) len); + while (len > 0) { + int rc; + + /* Move window and get a new size to erase */ + rc = mbox_window_move(mbox_flash, &mbox_flash->write, + MBOX_C_CREATE_WRITE_WINDOW, pos, len, &size); + if (rc) + return rc; + + msg = msg_alloc(mbox_flash, MBOX_C_MARK_WRITE_ERASED); + if (!msg) + return FLASH_ERR_MALLOC_FAILED; + + msg_put_u16(msg, 0, pos >> mbox_flash->shift); + msg_put_u16(msg, 2, len >> mbox_flash->shift); + rc = msg_send(mbox_flash, msg); + if (rc) { + prlog(PR_ERR, "Failed to enqueue/send BMC MBOX message\n"); + msg_free_memory(msg); + return rc; + } + + rc = wait_for_bmc(mbox_flash, MBOX_DEFAULT_TIMEOUT); + if (rc) { + prlog(PR_ERR, "Error waiting for BMC\n"); + msg_free_memory(msg); + return rc; + } + + msg_free_memory(msg); + + /* + * Flush directly, don't mark that region dirty otherwise it + * isn't clear if a write happened there or not + */ + + rc = mbox_flash_flush(mbox_flash); + if (rc) + return rc; + + len -= size; + pos += size; + } + + return 0; +} + +static int mbox_flash_erase_v1(struct blocklevel_device *bl __unused, uint64_t pos __unused, uint64_t len __unused) { /* @@ -859,7 +916,7 @@ static int protocol_init(struct mbox_flash_data *mbox_flash) /* Assume V2 */ mbox_flash->bl.read = &mbox_flash_read; mbox_flash->bl.write = &mbox_flash_write; - mbox_flash->bl.erase = &mbox_flash_erase; + mbox_flash->bl.erase = &mbox_flash_erase_v2; mbox_flash->bl.get_info = &mbox_flash_get_info; /* Assume V2 */ @@ -913,6 +970,7 @@ static int protocol_init(struct mbox_flash_data *mbox_flash) prlog(PR_INFO, "Detected mbox protocol version %d\n", mbox_flash->version); if (mbox_flash->version == 1) { + mbox_flash->bl.erase = &mbox_flash_erase_v1; /* Not all handlers differ, update those which do */ mbox_flash->handlers[MBOX_C_GET_MBOX_INFO] = &mbox_flash_do_get_mbox_info; mbox_flash->handlers[MBOX_C_GET_FLASH_INFO] = &mbox_flash_do_get_flash_info_v1; @@ -956,7 +1014,7 @@ int mbox_flash_init(struct blocklevel_device **bl) /* Assume V2 */ mbox_flash->bl.read = &mbox_flash_read; mbox_flash->bl.write = &mbox_flash_write; - mbox_flash->bl.erase = &mbox_flash_erase; + mbox_flash->bl.erase = &mbox_flash_erase_v2; mbox_flash->bl.get_info = &mbox_flash_get_info; if (bmc_mbox_get_attn_reg() & MBOX_ATTN_BMC_REBOOT)
Version two of the mbox-flash protocol defines a new command: MARK_WRITE_ERASED. This command provides a simple way to mark a region of flash as all 0xff without the need to go and write all 0xff. This is an optimisation as there is no need for an erase before a write, it is the responsibility of the BMC to deal with the flash correctly, however in v1 it was ambiguous what a client should do if the flash should be erased but not actually written to. This allows of a optimal path to resolve this problem. Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> --- libflash/mbox-flash.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 61 insertions(+), 3 deletions(-)