On Mon, 13 May 2024, Hans de Goede wrote:
> Hi Ilpo,
>
> Thank you for the review.
>
> On 5/13/24 10:34 AM, Ilpo Järvinen wrote:
> > On Sun, 12 May 2024, Hans de Goede wrote:
> >> + u8 resp[3];
> >> +
> >> + set_brightness[2] = brightness;
> >> + set_brightness[3] = dell_uart_checksum(set_brightness, 3);
> >
> > Also, couldn't these be accessed through a struct to eliminate most of the
> > magic indexes?
>
> With the checksum at the end, this would require a VLA in the middle
> of the struct (get_version return buffer contains more then 1 dat byte)
> We could treat the checksum as an extra data byte, but then we are
> mixing struct usage for some fields + using an array of bytes
> approach for the data + checksum. For consistency I prefer to just
> stick with one approach which means using the array of bytes approach
> for everything.
Ok.
> >> + const u8 get_brightness[] = { 0x6A, 0x0C, 0x89 };
> >> + u8 resp[4];
> >> + int ret;
> >> +
> >> + ret = dell_uart_bl_command(dell_bl, get_brightness, ARRAY_SIZE(get_brightness),
> >> + resp, ARRAY_SIZE(resp));
> >> + if (ret)
> >> + return ret;
> >> +
> >> + if (resp[0] != 4) {
> >
> > sizeof(resp)
>
> Ack.
>
> > but isn't this already checked when reading it??
>
> No dell_uart_bl_receive() checks that the response will fit in the supplied
> buffer and that it has a valid checksum but the controller may send a
> response smaller then the passed in buffer and it will actually do this for
> the get_version command.
Ah, I see now that it checked against constant rather than the actual
value.
> >> + dev_warn(dell_bl->dev, "Bytes received out of band, dropping them.\n");
> >> + return len;
> >> + }
> >> +
> >> + for (i = 0; i < len; i++) {
> >> + dell_bl->resp[dell_bl->resp_idx] = data[i];
> >> +
> >> + switch (dell_bl->resp_idx) {
> >> + case 0: /* Length byte */
> >> + dell_bl->resp_len = dell_bl->resp[0];
> >> + if (dell_bl->resp_len < DELL_BL_MIN_RESP_SIZE) {
> >> + dev_err(dell_bl->dev, "Response length too small %d < %d\n",
> >> + dell_bl->resp_len, DELL_BL_MIN_RESP_SIZE);
> >> + dell_bl->status = -EIO;
> >> + goto wakeup;
> >> + } else 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;
> >> + }
> >> + break;
> >> + case 1: /* CMD byte */
> >> + if (dell_bl->resp[1] != dell_bl->pending_cmd) {
> >> + dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n",
> >> + dell_bl->resp[1], dell_bl->pending_cmd);
> >> + dell_bl->status = -EIO;
> >> + goto wakeup;
> >> + }
> >> + break;
> >> + }
> >> +
> >> + 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.
}
?
> >> + dell_bl->status = 1; /* Success */
> >> + goto wakeup;
> >
> > Huh? Now I'm totally lost how the control flow is supposed to go in this
> > function. Can you rethink this loop so it actual makes sense and doesn't
> > misuse gotos like this?
>
> This is the receive() callback from the UART the loop consumes bytes received
> by the UART. The gotos stop consuming bytes in 2 cases:
>
> 1. An error (unexpected data) is encountered.
> 2. A complete frame has been successfully received.
>
> The checking of the checksum + goto wakeup at the end of the loop is for 2.
>
> The:
>
> return len;
>
> after the loop indicates to the UART / tty-layer that all passed data
> has been consumed and this path gets hit when the driver needs to wait
> for more data because the response is not complete yet.
>
> >> +
> >> +wakeup:
> >> + wake_up(&dell_bl->wait_queue);
> >> + return i + 1;
> >> +}
--
i.
