From: Hans de Goede <hdegoede@redhat.com>
To: Andy Shevchenko <andy@kernel.org>
Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
"AceLan Kao" <acelan.kao@canonical.com>,
"Roman Bogoyev" <roman@computercheck.com.au>,
"Kai Heng Feng" <kai.heng.feng@canonical.com>,
platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH v2 1/2] platform/x86: Add new Dell UART backlight driver
Date: Mon, 13 May 2024 17:33:10 +0200 [thread overview]
Message-ID: <b1880f73-c8f5-4076-bb62-6c9ea620e99e@redhat.com> (raw)
In-Reply-To: <ZkIvdZ9Xhs-HiZn0@smile.fi.intel.com>
Hi,
On 5/13/24 5:19 PM, Andy Shevchenko wrote:
> On Mon, May 13, 2024 at 03:18:10PM +0200, Hans de Goede wrote:
>> On 5/13/24 2:58 PM, Andy Shevchenko wrote:
>>> On Mon, May 13, 2024 at 01:15:50PM +0200, Hans de Goede wrote:
>
> ...
>
>>>> +static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl,
>>>> + const u8 *cmd, int cmd_len,
>>>> + u8 *resp, int resp_max_len)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ret = mutex_lock_killable(&dell_bl->mutex);
>>>
>>> Can't be called via cleanup.h?
>>
>> I prefer to have the locking explicit rather then use cleanup.h .
>
> Hmm... interesting, so you push-back the cleanup.h usage?
I'm in favor of the guard(mutex)(&smne_mutex); syntax, but this
is a mutex_lock_killable() for which that does not work AFAIK.
So in this case AFAICT we would need to use the cleanup stuff manually
and in that case I believe that in that case just sticking with
the current code is better.
>
> <snip>
>
>>>> + 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;
>>>
>>> No default case?
>>
>> Nope, this checks the validity of the first 2 bytes / the header. The data is not checked.
>
> Why not
>
> default:
> /* We do not check the data */
> break;
>
> ?
TBH I don't see any added value in adding that.
>
> ...
>
>>>> + dev_dbg(dev, "Firmware version: %.*s\n", resp[RESP_LEN] - 3, resp + RESP_DATA);
>>>
>>> I would be on the safest side, i.e. not trusting that it will be NUL-terminated
>>> string, hence something like %*pE?
>>
>> Right, this is why the existing dev_dbg() already passes a precision and we do
>> want to actually stop if there is a 0 there, which %pE does not do.
>
> I'm talking about the opposite, when it might go over the boundary.
AFAIK the way the precision is used in the current code limits things to at max the boundary,
stopping earlier if a 0 is encountered earlier.
Regards,
Hans
next prev parent reply other threads:[~2024-05-13 15:33 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-13 11:15 [PATCH v2 0/2] platform/x86: Add new Dell UART backlight driver Hans de Goede
2024-05-13 11:15 ` [PATCH v2 1/2] " Hans de Goede
2024-05-13 12:58 ` Andy Shevchenko
2024-05-13 13:18 ` Hans de Goede
2024-05-13 15:19 ` Andy Shevchenko
2024-05-13 15:33 ` Hans de Goede [this message]
2024-05-13 15:44 ` Andy Shevchenko
2024-05-13 15:45 ` Andy Shevchenko
2024-05-13 15:54 ` Ilpo Järvinen
2024-05-13 16:04 ` Andy Shevchenko
2024-05-13 11:15 ` [PATCH v2 2/2] tools arch x86: Add dell-uart-backlight-emulator Hans de Goede
2024-05-13 12:46 ` Andy Shevchenko
2024-05-13 13:25 ` 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=b1880f73-c8f5-4076-bb62-6c9ea620e99e@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 \
--cc=roman@computercheck.com.au \
/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).