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: 5107 bytes --]

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

?

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


-- 
 i.