Hi all,
I'm taking a look into the following piece of code in drivers/misc/sgi-xp/xpc_uv.c:
561 case XPC_ACTIVATE_MQ_MSG_CHCTL_OPENCOMPLETE_UV: {
562 struct xpc_activate_mq_msg_chctl_opencomplete_uv *msg;
563
564 if (!part_setup)
565 break;
566
567 msg = container_of(msg_hdr, struct
568 xpc_activate_mq_msg_chctl_opencomplete_uv, hdr);
569 spin_lock_irqsave(&part->chctl_lock, irq_flags);
570 part->chctl.flags[msg->ch_number] |= XPC_CHCTL_OPENCOMPLETE;
571 spin_unlock_irqrestore(&part->chctl_lock, irq_flags);
572
573 xpc_wakeup_channel_mgr(part);
574 }
and I'm trying to figure out if the following warning is due to a missing break
at the end of the case, or if this is just a false positive and a /* fall through */
annotation should be added:
drivers/misc/sgi-xp/xpc_uv.c: In function ‘xpc_handle_activate_mq_msg_uv’:
drivers/misc/sgi-xp/xpc_uv.c:573:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
xpc_wakeup_channel_mgr(part);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/misc/sgi-xp/xpc_uv.c:575:2: note: here
case XPC_ACTIVATE_MQ_MSG_MARK_ENGAGED_UV:
^~~~
The piece of code above was introduced by the following commit in 2009:
efdd06ed181a88a11e612238c1ac04668e665395
The cases are pretty similar, and the fact that this code was introduced
in the middle of the switch statement and not at the end or the beginning,
leads me to believe that this is a false positive. On the other hand,
all the other cases end with a break or a return but this one. So, I
better ask your opinions about this.
Thanks
--
Gustavo
Hi all,
Friendly ping:
Who can provide some feedback on this?
Thanks
--
Gustavo
On 2/26/19 10:24 PM, Gustavo A. R. Silva wrote:
> Hi all,
>
> I'm taking a look into the following piece of code in drivers/misc/sgi-xp/xpc_uv.c:
>
> 561 case XPC_ACTIVATE_MQ_MSG_CHCTL_OPENCOMPLETE_UV: {
> 562 struct xpc_activate_mq_msg_chctl_opencomplete_uv *msg;
> 563
> 564 if (!part_setup)
> 565 break;
> 566
> 567 msg = container_of(msg_hdr, struct
> 568 xpc_activate_mq_msg_chctl_opencomplete_uv, hdr);
> 569 spin_lock_irqsave(&part->chctl_lock, irq_flags);
> 570 part->chctl.flags[msg->ch_number] |= XPC_CHCTL_OPENCOMPLETE;
> 571 spin_unlock_irqrestore(&part->chctl_lock, irq_flags);
> 572
> 573 xpc_wakeup_channel_mgr(part);
> 574 }
>
> and I'm trying to figure out if the following warning is due to a missing break
> at the end of the case, or if this is just a false positive and a /* fall through */
> annotation should be added:
>
> drivers/misc/sgi-xp/xpc_uv.c: In function ‘xpc_handle_activate_mq_msg_uv’:
> drivers/misc/sgi-xp/xpc_uv.c:573:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
> xpc_wakeup_channel_mgr(part);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/misc/sgi-xp/xpc_uv.c:575:2: note: here
> case XPC_ACTIVATE_MQ_MSG_MARK_ENGAGED_UV:
> ^~~~
>
> The piece of code above was introduced by the following commit in 2009:
>
> efdd06ed181a88a11e612238c1ac04668e665395
>
> The cases are pretty similar, and the fact that this code was introduced
> in the middle of the switch statement and not at the end or the beginning,
> leads me to believe that this is a false positive. On the other hand,
> all the other cases end with a break or a return but this one. So, I
> better ask your opinions about this.
>
> Thanks
> --
> Gustavo
>
I am sorry for my delayed response. I missed the earlier email.
On Wed, Mar 20, 2019 at 9:37 AM Gustavo A. R. Silva
<[email protected]> wrote:
>
...
> > I'm taking a look into the following piece of code in drivers/misc/sgi-xp/xpc_uv.c:
...
> > and I'm trying to figure out if the following warning is due to a missing break
> > at the end of the case, or if this is just a false positive and a /* fall through */
> > annotation should be added:
The fall-through is by design. The protocol previously had a windows
of failure where a connection
could be in the process of being established and a failure could be
detected prior to the
handling of the establishment message. I added the new open complete
message and leveraged
the fall-through to mark the connection established.
Please let me know if you do not intend to submit a patch for this.
Thank you,
Robin Holt
On 3/20/19 10:23 AM, Robin Holt wrote:
> I am sorry for my delayed response. I missed the earlier email.
>
> On Wed, Mar 20, 2019 at 9:37 AM Gustavo A. R. Silva
> <[email protected]> wrote:
>>
> ...
>>> I'm taking a look into the following piece of code in drivers/misc/sgi-xp/xpc_uv.c:
> ...
>>> and I'm trying to figure out if the following warning is due to a missing break
>>> at the end of the case, or if this is just a false positive and a /* fall through */
>>> annotation should be added:
>
> The fall-through is by design. The protocol previously had a windows
> of failure where a connection
> could be in the process of being established and a failure could be
> detected prior to the
> handling of the establishment message. I added the new open complete
> message and leveraged
> the fall-through to mark the connection established.
>
Great. I see now.
> Please let me know if you do not intend to submit a patch for this.
>
I will send a patch to add the fall-through comment and fix the following
warning:
drivers/misc/sgi-xp/xpc_uv.c: In function ‘xpc_handle_activate_mq_msg_uv’:
drivers/misc/sgi-xp/xpc_uv.c:573:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
xpc_wakeup_channel_mgr(part);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/misc/sgi-xp/xpc_uv.c:575:2: note: here
case XPC_ACTIVATE_MQ_MSG_MARK_ENGAGED_UV:
^~~~
Notice that this is part of the ongoing efforts to enable -Wimplicit-fallthrough.
Thanks, Robin.
--
Gustavo