Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Hans de Goede <hdegoede@redhat.com>]

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