Hi,
On 5/13/24 4:36 PM, Ilpo Järvinen wrote:
> 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.
Thank you for caching that.
I'll just switch to open coding the range check then for v3.
Regards,
Hans
