From: Hans de Goede <hdegoede@redhat.com>
To: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: Andy Shevchenko <andy@kernel.org>,
AceLan Kao <acelan.kao@canonical.com>,
Kai-Heng Feng <kai.heng.feng@canonical.com>,
platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver
Date: Mon, 13 May 2024 16:39:31 +0200 [thread overview]
Message-ID: <b310285a-e01a-4e2f-8118-84500837c6ca@redhat.com> (raw)
In-Reply-To: <06498232-5aeb-6e63-6775-2447a8c60542@linux.intel.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
next prev parent reply other threads:[~2024-05-13 14:39 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-12 16:23 [PATCH 0/2] platform/x86: Add new Dell UART backlight driver Hans de Goede
2024-05-12 16:23 ` [PATCH 1/2] " Hans de Goede
2024-05-12 19:35 ` Andy Shevchenko
2024-05-13 10:01 ` Hans de Goede
2024-05-13 12:28 ` Andy Shevchenko
2024-05-13 12:36 ` Ilpo Järvinen
2024-05-13 8:34 ` Ilpo Järvinen
2024-05-13 9:55 ` Hans de Goede
2024-05-13 12:12 ` Ilpo Järvinen
2024-05-13 12:14 ` Ilpo Järvinen
2024-05-13 13:07 ` Hans de Goede
2024-05-13 13:14 ` Ilpo Järvinen
2024-05-13 13:21 ` Hans de Goede
2024-05-13 13:34 ` Ilpo Järvinen
2024-05-13 14:23 ` Hans de Goede
2024-05-13 14:36 ` Ilpo Järvinen
2024-05-13 14:39 ` Hans de Goede [this message]
2024-05-12 16:23 ` [PATCH 2/2] tools arch x86: Add dell-uart-backlight-emulator Hans de Goede
2024-05-12 19:32 ` Andy Shevchenko
2024-05-12 19:47 ` Hans de Goede
2024-05-13 11:03 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b310285a-e01a-4e2f-8118-84500837c6ca@redhat.com \
--to=hdegoede@redhat.com \
--cc=acelan.kao@canonical.com \
--cc=andy@kernel.org \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=kai.heng.feng@canonical.com \
--cc=platform-driver-x86@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).