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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 8029 bytes --]

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.

> 				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++;

Good, I didn't realize the switch used the index.

-- 
 i.

> 	}
> 
> 	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
> 
>