On Thu, May 02, 2019 at 04:40:41PM +0200, Greg Kroah-Hartman wrote: > On Thu, May 02, 2019 at 09:28:37AM -0500, Gustavo A. R. Silva wrote: > > >>>> @@ -1813,6 +1813,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial, > > >>>> } > > >>>> /* Else, drop through */ > > >>>> } > > >>>> + /* Fall through */ > > >>>> case EXPECT_DATA: /* Expect data */ > > >>> > > >>> Looks like you forgot to take the original review feedback you got into > > >>> account: > > >>> > > >>> https://lkml.kernel.org/r/87k1zf4k24.fsf@xxxxxxxxxxxxxxxxx > > >>> > > >> > > >> Oh, the thing is that the fall-through comments have to be placed at > > >> the very bottom of the case. Also, based on that feedback, this time > > >> I left the "Else, drop through" comment in place, so people can be > > >> informed that such fall-through is conditional. > > >> > > >> What do you think about this: > > >> > > >> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c > > >> index 4ca31c0e4174..52f27fc82563 100644 > > >> --- a/drivers/usb/serial/io_edgeport.c > > >> +++ b/drivers/usb/serial/io_edgeport.c > > >> @@ -1751,7 +1751,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial, > > >> edge_serial->rxState = EXPECT_HDR2; > > >> break; > > >> } > > >> - /* otherwise, drop on through */ > > >> + /* Fall through - otherwise, drop on through */ > > >> case EXPECT_HDR2: > > >> edge_serial->rxHeader2 = *buffer; > > >> ++buffer; > > >> @@ -1813,6 +1813,11 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial, > > >> } > > >> /* Else, drop through */ > > >> } > > >> + /* Beware that, currently, there are at least three > > >> + * break statements in this case block, so the > > >> + * fall-through marked below is NOT unconditional. > > >> + */ > > >> + /* Fall through */ > > >> case EXPECT_DATA: /* Expect data */ > > >> if (bufferLength < edge_serial->rxBytesRemaining) { > > >> rxLen = bufferLength; > > > > > > It's better than v2, but I thought you said you were gonna look into > > > restructuring the code to maintain (or even improve) readability? > > > > > > > At first, I thought about that, but now I don't think that's realistic. > > I'd turn the if-else into a switch, and based on the history of feedback > > on this patch, we will end up having the same complains about the break > > statements in that new switch and the possibility of a fall-through to > > case EXPECT_DATA. At the end I would still have to add a comment explaining > > that the last fall-through mark in unconditional. > > I love it how no one is blaming the original author of this code (i.e. > me...) > > Let me see if I can fix it up to be more "sane", this is my fault. Thanks, that'd be great. I haven't looked at it myself in a long time, but judging from the old thread it did not seem impossible at least. Getting rid of some of that deep nesting would be good either way. :) Johan
