On Mon, 13 May 2024, Hans de Goede wrote:
> Hi,
>
> 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.
--
i.
