Hi,
On 5/13/24 2:12 PM, Ilpo Järvinen wrote:
> 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.
> }
>
> ?
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;
}
Regards,
Hans
>
>>>> + 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;
>>>> +}
>
>
