hw/net/can: Add missing fallthrough statements

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

Posted by Thomas Huth 3 years, 9 months ago

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. */
-- 
2.18.1

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

Posted by Laurent Vivier 3 years, 9 months ago

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>

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

Posted by Pavel Pisa 3 years, 9 months ago

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>

By the way, we have prepared CAN FD support for QEMU,
the CAN core update and device model to emulate
our open-source/design/hardware CTU CAN FD IP core

  https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core

QEMU emulation

  https://gitlab.fel.cvut.cz/canbus/qemu-canbus/-/commits/charvj10-canfd

I hope to find time to add patch to document CAN support to CAN FD
extension and send whole series this week. Stay tuned, please.

Best wishes,
 
                Pavel

-- 

                Pavel Pisa
    phone:      +420 603531357
    e-mail:     pisa@cmp.felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://dce.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/

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

Posted by Laurent Vivier 3 years, 7 months ago

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