[Skiboot] [PATCH 3/5] libflash/mbox-flash: Update to V2 of the protocol

[Suraj Jitindar Singh]

On Tue, 2017-05-02 at 09:33 +0200, Benjamin Herrenschmidt wrote:
> On Mon, 2017-04-24 at 19:14 +1000, Cyril Bur wrote:
> > 
> > +       if (mbox_flash->version == 1) {
> > +               msg_put_u16(msg, 0, pos >> mbox_flash->shift);
> > +               msg_put_u32(msg, 2, len);
> > +       } else {
> > +               uint64_t window_pos = (pos - mbox_flash-
> > >write.cur_pos);
> > +               uint64_t window_len = len + (window_pos &
> > mbox_flash_mask(mbox_flash));
> > +
> > +               msg_put_u16(msg, 0, window_pos >> mbox_flash-
> > >shift);
> > +               msg_put_u16(msg, 2, ALIGN_UP(window_len, 1 <<
> > mbox_flash->shift) >> mbox_flash->shift);
> > +       }
> > +
> Those alignments are gross and I think not quite right.

I think this is technically write because window_len will be len (2k) +
window_pos (3k) which will give us 5k, then aligned up will give us 8k
-> 2 blocks.

However I agree this could be structured a lot better to make it
clearer and that something like what you suggest below would make this
easier to follow.

> 
> Let's say your block size is 4k. window_pos is 3k and window_len is
> 2k
> 
> Intuitively, you see that you need to mark both block 0 and 1 dirty
> we
> straddle two blocks.
> 
> However, your code will align window_len to 4k, which shifted means 1
> so you'll only mark 1 block.
> 
> What you need to do (and it's cleaner than this constant shifting
> around) is to calculate the start and end (not len)
> 
> Then align them.
> 
> Then you can do end-start
> 
> So in my example, start becomes aligned to 0, end which is 5k becomes
> aligned to 8k, so your substraction now gives you 2 blocks.
> 
> Cheers,
> Ben.
>