Hi,
On 5/13/24 3:34 PM, Ilpo Järvinen wrote:
> 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.
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)) {
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++;
}
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
