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
