Re: [PATCH] hw/net/can: Add missing fallthrough statements

[Laurent Vivier]

Le 14/07/2020 à 00:09, Pavel Pisa a écrit :
> Hello Laurent and others,
> 
> On Monday 06 of July 2020 18:35:50 Laurent Vivier wrote:
>> Le 30/06/2020 à 09:55, Thomas Huth a écrit :
>>> Add fallthrough annotations to be able to compile the code without
>>> warnings when using -Wimplicit-fallthrough in our CFLAGS. Looking
>>> at the code, it seems like the fallthrough is indeed intended here,
>>> so the comments should be appropriate.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  hw/net/can/can_sja1000.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/hw/net/can/can_sja1000.c b/hw/net/can/can_sja1000.c
>>> index ea915a023a..299932998a 100644
>>> --- a/hw/net/can/can_sja1000.c
>>> +++ b/hw/net/can/can_sja1000.c
>>> @@ -523,6 +523,7 @@ void can_sja_mem_write(CanSJA1000State *s, hwaddr
>>> addr, uint64_t val, break;
>>>          case 16: /* RX frame information addr16-28. */
>>>              s->status_pel |= (1 << 5); /* Set transmit status. */
>>> +            /* fallthrough */
>>>          case 17 ... 28:
>>>              if (s->mode & 0x01) { /* Reset mode */
>>>                  if (addr < 24) {
>>> @@ -620,6 +621,7 @@ void can_sja_mem_write(CanSJA1000State *s, hwaddr
>>> addr, uint64_t val, break;
>>>          case 10:
>>>              s->status_bas |= (1 << 5); /* Set transmit status. */
>>> +            /* fallthrough */
>>>          case 11 ... 19:
>>>              if ((s->control & 0x01) == 0) { /* Operation mode */
>>>                  s->tx_buff[addr - 10] = val; /* Store to TX buffer
>>> directly. */
>>
>> cc: Pavel Pisa <pisa@cmp.felk.cvut.cz>
>>
>> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> 
> The fallthrough is intentional for sure but I have gone
> through datasheet and checked why the status bit is set
> there and my conclusion is that to mimic real HW the status
> bit should not be set there. In the fact, it should be set
> and immediately (in a future delayed) reset after SJA_CMR
> transmit request write. This would mimic real hardware
> more closely. May it be I send patch in future when more
> of our developed CAN support is added to QEMU. The status
> bit behavior has no influence on actual Linux SJA1000 driver
> anyway.
> 
> So for now, I confirm that adding  /* fallthrough */ is correct
> step forward.
> 
> Reviewed-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
> 

Applied to my trivial-patches branch.

Thanks,
Laurent