On Fri, 6 Nov 2020 at 18:12, Pavel Pisa <p...@cmp.felk.cvut.cz> wrote:
>
> Hello Peter,
>
> this one is a little problematic. I understand that you want
> to have clean code and no warnings reports from coverity.
>
> On Friday 06 of November 2020 18:11:51 Peter Maydell wrote:
> > @@ -271,10 +266,7 @@ static void ctucan_send_ready_buffers(CtuCanCoreState
> > *s) if (buff2tx_idx == -1) {
> > break;
> > }
> > - buff_st_mask = 0xf << (buff2tx_idx * 4);
> > - buff_st = (s->tx_status.u32 >> (buff2tx_idx * 4)) & 0xf;
>
> There I would kept extracted state in the variable. Actual model is really
> simplified to real hardware. Tx succeeds in zero time.
>
> > int_stat.u32 = 0;
> > - buff_st = TXT_RDY;
>
> This is why the TXT_RDY state immediately changes to TXT_TOK. It works well
> for actual simple CAN subsystem implementation. But if we want to implement
> priorization of messages on emulated bus and even simulate real bus latency
> by delay to state change and interrut delivery, then we need to proceed
> through TXT_RDY state. If it is a problem for release, that your want to have
> coverity clean source tree, then please left the line as a comment there
> or use some trick with
> (void)buff_st;
>
> Or if you prefer, use
>
> + s->tx_status.u32 = deposit32(s->tx_status.u32,
> + buff2tx_idx * 4, 4, TXT_RDY);
>
> if it silent the coverity.
I was going to put a comment in v2 of this patch series to
document that this is where the status goes to TXT_RDY,
but looking at the code, at this point the buffer status field
is *already* TXT_RDY -- the preceding loop does not allow
us to get to this point for an entry which is in any other
state. So while I agree with your suggestion that it's worth
having at least a documentation comment to indicate where the
state is changing, I think there is no intermediate state
transition to document here.
thanks
-- PMM
