On Mon, 13 May 2024, Hans de Goede wrote: > On 5/13/24 3:34 PM, Ilpo Järvinen wrote: > > On Mon, 13 May 2024, Hans de Goede wrote: > >> On 5/13/24 3:14 PM, Ilpo Järvinen wrote: > >>> On Mon, 13 May 2024, Hans de Goede wrote: > >>>> On 5/13/24 2:12 PM, Ilpo Järvinen wrote: > >>>>> On Mon, 13 May 2024, Hans de Goede wrote: > >>>>>> On 5/13/24 10:34 AM, Ilpo Järvinen wrote: > >>>>>>> On Sun, 12 May 2024, Hans de Goede wrote: > >>> > >>>>>>>> + > >>>>>>>> + dell_bl->resp_idx++; > >>>>>>>> + if (dell_bl->resp_idx < dell_bl->resp_len) > >>>>>>>> + continue; > >>>>>>>> + > >>>>>>>> + csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); > >>>>>>>> + if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { > >>>>>>>> + dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", > >>>>>>>> + dell_bl->resp[dell_bl->resp_len - 1], csum); > >>>>>>>> + dell_bl->status = -EIO; > >>>>>>>> + goto wakeup; > >>>>>>>> + } > >>>>>>> > >>>>>>> Why is the checksum calculation and check inside the loop?? > >>>>>> > >>>>>> The loop iterates over received bytes, which may contain extra data > >>>>>> after the response, the: > >>>>>> > >>>>>> dell_bl->resp_idx++; > >>>>>> if (dell_bl->resp_idx < dell_bl->resp_len) > >>>>>> continue; > >>>>>> > >>>>>> continues looping until we have received all the expected bytes. So here, past this > >>>>>> check, we are are at the point where we have a complete response and then we verify it. > >>>>>> > >>>>>> And on successful verification wake-up any waiters. > >>>>> > >>>>> So effectively you want to terminate the loop on two conditions here: > >>>>> > >>>>> a) dell_bl->resp_idx == dell_bl->resp_len (complete frame) > >>>>> a) if i == len (not yet received a full frame) > >>>>> > >>>>> Why not code those rather than the current goto & continue madness? > >>>>> > >>>>> Then, after the loop, you can test: > >>>>> > >>>>> if (dell_bl->resp_idx == dell_bl->resp_len) { > >>>>> // calc checksum, etc. > >>>>> } > >>>>> > >>>>> ? > >>>> > >>>> Ok, I've added the following change for v3: > >>>> > >>>> diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c > >>>> index bf5b12efcb19..66d8c6ddcb83 100644 > >>>> --- a/drivers/platform/x86/dell/dell-uart-backlight.c > >>>> +++ b/drivers/platform/x86/dell/dell-uart-backlight.c > >>>> @@ -87,6 +87,7 @@ static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl, > >>>> dell_bl->status = -EBUSY; > >>>> dell_bl->resp = resp; > >>>> dell_bl->resp_idx = 0; > >>>> + dell_bl->resp_len = -1; /* Invalid / unset */ > >>>> dell_bl->resp_max_len = resp_max_len; > >>>> dell_bl->pending_cmd = cmd[1]; > >>>> > >>>> @@ -219,7 +219,7 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, > >>>> return len; > >>>> } > >>>> > >>>> - for (i = 0; i < len; i++) { > >>>> + for (i = 0; i < len && dell_bl->resp_idx != dell_bl->resp_len; i++, dell_bl->resp_idx++) { > >>>> dell_bl->resp[dell_bl->resp_idx] = data[i]; > >>>> > >>>> switch (dell_bl->resp_idx) { > >>>> @@ -228,46 +228,41 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, > >>>> if (dell_bl->resp_len < MIN_RESP_LEN) { > >>>> dev_err(dell_bl->dev, "Response length too small %d < %d\n", > >>>> dell_bl->resp_len, MIN_RESP_LEN); > >>>> - dell_bl->status = -EIO; > >>>> - goto wakeup; > >>>> + goto error; > >>>> } > >>>> > >>>> if (dell_bl->resp_len > dell_bl->resp_max_len) { > >>>> dev_err(dell_bl->dev, "Response length too big %d > %d\n", > >>>> dell_bl->resp_len, dell_bl->resp_max_len); > >>>> - dell_bl->status = -EIO; > >>>> - goto wakeup; > >>>> + goto error; > >>>> } > >>>> break; > >>>> case RESP_CMD: /* CMD byte */ > >>>> if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) { > >>>> dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", > >>>> dell_bl->resp[RESP_CMD], dell_bl->pending_cmd); > >>>> - dell_bl->status = -EIO; > >>>> - goto wakeup; > >>>> + goto error; > >>>> } > >>>> break; > >>>> } > >>>> + } > >>>> > >>>> - dell_bl->resp_idx++; > >>>> - if (dell_bl->resp_idx < dell_bl->resp_len) > >>>> - continue; > >>>> - > >>>> + if (dell_bl->resp_idx == dell_bl->resp_len) { > >>>> csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); > >>>> if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { > >>>> dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", > >>>> dell_bl->resp[dell_bl->resp_len - 1], csum); > >>>> - dell_bl->status = -EIO; > >>>> - goto wakeup; > >>>> + goto error; > >>>> } > >>>> - > >>>> dell_bl->status = 0; /* Success */ > >>>> - goto wakeup; > >>>> + wake_up(&dell_bl->wait_queue); > >>>> + return i; > >>>> } > >>>> > >>>> return len; > >>>> > >>>> -wakeup: > >>>> +error: > >>>> + dell_bl->status = -EIO; > >>>> wake_up(&dell_bl->wait_queue); > >>>> return i + 1; > >>>> } > >>> > >>> Thanks, this is way easier to follow. > >> > >> I'm glad you like it. > >> > >> There is a little bug in this version though, the goto error on the checksum fail > >> case returns i + i, which should be i in that case, I'll just drop the goto there and > >> instead always use the return i already present at the end of the > >> "if (dell_bl->resp_idx == dell_bl->resp_len) { }" block. > > > > It could have been solved more logically incrementing i and resp_idx here: > > > > dell_bl->resp[dell_bl->resp_idx] = data[i]; > > dell_bl->resp_idx++; > > i++; > > > > so that the inconsistent state is eliminated. > > > > I also realized (I know I was the one who suggested it) that reverse logic > > would be better for the incomplete frame check: > > > > if (dell_bl->resp_idx < dell_bl->resp_len) > > return len; > > > > // checksum logic... > > > > Perhaps the success and error return paths could then be merged too. > > Interesting suggestion, I also realized that the 2 response-length checks are a > range check so I've folded those together. So here is what I have now for v3, > note that the i++ is now done when copying data over: > > i = 0; > while (i < len && dell_bl->resp_idx != dell_bl->resp_len) { > dell_bl->resp[dell_bl->resp_idx] = data[i++]; > > switch (dell_bl->resp_idx) { > case RESP_LEN: /* Length byte */ > dell_bl->resp_len = dell_bl->resp[RESP_LEN]; > if (!in_range(dell_bl->resp_len, MIN_RESP_LEN, dell_bl->resp_max_len)) { in_range() takes start and len, not start and end. I really hate that helper because it has that trap and would often require "+ min" to be added. > dev_err(dell_bl->dev, "Response length %d out if range %d - %d\n", > dell_bl->resp_len, MIN_RESP_LEN, dell_bl->resp_max_len); > dell_bl->status = -EIO; > goto wakeup; > } > break; > case RESP_CMD: /* CMD byte */ > if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) { > dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", > dell_bl->resp[RESP_CMD], dell_bl->pending_cmd); > dell_bl->status = -EIO; > goto wakeup; > } > break; > } > dell_bl->resp_idx++; Good, I didn't realize the switch used the index. -- i. > } > > if (dell_bl->resp_idx != dell_bl->resp_len) > return len; /* Response not complete yet */ > > csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); > if (dell_bl->resp[dell_bl->resp_len - 1] == csum) { > dell_bl->status = 0; /* Success */ > } else { > dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", > dell_bl->resp[dell_bl->resp_len - 1], csum); > dell_bl->status = -EIO; > } > wakeup: > wake_up(&dell_bl->wait_queue); > return i; > } > > Regards, > > Hans > >