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?
<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;
?
...
> >> + 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.
--
With Best Regards,
Andy Shevchenko
