Hi,
On Thu, Apr 25, 2024 at 1:19 AM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
>
> > @@ -279,6 +281,8 @@ enum mipi_dsi_dcs_tear_mode {
> >
> > ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,
> > const void *data, size_t len);
> > +ssize_t mipi_dsi_dcs_write_buffer_chatty(struct mipi_dsi_device *dsi,
> > + const void *data, size_t len);
> > ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
> > const void *data, size_t len);
> > ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dsi, u8 cmd, void *data,
> > @@ -317,14 +321,10 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
> > #define mipi_dsi_generic_write_seq(dsi, seq...) \
> > do { \
> > static const u8 d[] = { seq }; \
> > - struct device *dev = &dsi->dev; \
> > int ret; \
> > - ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d)); \
> > - if (ret < 0) { \
> > - dev_err_ratelimited(dev, "transmit data failed: %d\n", \
> > - ret); \
> > + ret = mipi_dsi_generic_write_chatty(dsi, d, ARRAY_SIZE(d)); \
> > + if (ret < 0) \
> > return ret; \
> > - } \
> > } while (0)
>
> The one thing that I've always disliked about these macros (even if I've
> never actually used them myself) is that they hide control flow from the
> caller, i.e. return directly. You don't see that in the code, it's not
> documented, and if you wanted to do better error handling yourself,
> you're out of luck.
Yeah, I agree that it's not the cleanest. That being said, it is
existing code and making the existing code less bloated seems worth
doing.
I'd also say that it feels worth it to have _some_ solution so that
the caller doesn't need to write error handling after every single cmd
sent. If we get rid of / discourage these macros that's either going
to end us up with ugly/verbose code or it's going to encourage people
to totally skip error handling. IMO neither of those are wonderful
solutions.
While thinking about this there were a few ideas I came up with. None
of them are amazing, but probably they are better than the hidden
"return" like this. Perhaps we could mark the current function as
"deprecated" and pick one of these depending on what others opinions
are:
1. Use "goto" and force the caller to give a goto target for error handling.
This is based on an idea that Dmitry came up with, but made a little
more explicit. Example usage:
int ret;
ret = 0;
mipi_dsi_dcs_write_seq_goto(dsi, &ret, HX83102_SETSPCCMD, 0xcd,
some_cmd_failed);
mipi_dsi_dcs_write_seq_goto(dsi, &ret, HX83102_SETMIPI, 0x84,
some_cmd_failed);
mipi_dsi_dcs_write_seq_goto(dsi, &ret, HX83102_SETSPCCMD, 0x3f,
some_cmd_failed);
mipi_dsi_dcs_write_seq_goto(dsi, &ret, HX83102_SETVDC, 0x1b, 0x04,
some_cmd_failed);
...
some_cmd_failed:
pr_err("Commands failed to write: %d", ret);
return ret;
}
One downside here is that you can't easily tell which command failed
to put it in the error message. A variant of this idea (1a?) could be
to hoist the print back into the write command. I'd want to pick one
or the other. I guess my preference would be to hoist the print into
the write command and if someone really doesn't want the print then
they call mipi_dsi_dcs_write_buffer() directly.
---
2. Accept that a slightly less efficient handling of the error case
and perhaps a less intuitive API, but avoid the goto.
Essentially you could pass in "ret" and have the function be a no-op
if an error is already present. Something like this:
void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_device *dsi,
const void *data, size_t len, int *accum_ret)
{
if (*accum_ret)
return;
*accum_ret = mipi_dsi_dcs_write_buffer(dsi, data, len);
}
...and then the caller:
int ret;
ret = 0;
mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETSPCCMD, 0xcd, &ret);
mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETMIPI, 0x84, &ret);
mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETSPCCMD, 0x3f, &ret);
mipi_dsi_dcs_write_seq_multi(dsi, HX83102_SETVDC, 0x1b, 0x04, &ret);
if (ret)
goto some_cmd_failed;
This has similar properties to solution #1.
---
3. Accept that callers don't want to error handling but just need a print.
I'm not 100% sure we want to encourage this. On the one hand it's
unlikely anyone is really going to be able to reliably recover super
properly from an error midway through a big long command sequence. On
the other hand, this means we can't pass the error back to the caller.
In theory the caller _could_ try to handle errors by resetting / power
cycling things, so that's a real downside.
Example usage:
mipi_dsi_dcs_write_seq_chatty(dsi, HX83102_SETSPCCMD, 0xcd);
mipi_dsi_dcs_write_seq_chatty(dsi, HX83102_SETMIPI, 0x84);
mipi_dsi_dcs_write_seq_chatty(dsi, HX83102_SETSPCCMD, 0x3f);
mipi_dsi_dcs_write_seq_chatty(dsi, HX83102_SETVDC, 0x1b, 0x04);
---
I think I'd lean towards #1a (user passes goto label and we include
the error print in the helper), but I'd personally be happy with any
of #1 or #2. I don't love #3.
> Be that as it may, the combo of ratelimited error printing and return on
> errors does not make much sense to me. If there's something to print,
> you bail out, that's it. I suspect we never hit the ratelimit.
Yeah, I'm in favor of removing the ratelimit.
> You might even want to try *only* changing the ratelimited printing to a
> regular error message, and see if the compiler can combine the logging
> to a single exit point in the callers. Ratelimited it obviously can't
> because every single one of them is unique.
It wasn't quite as good. Comparing the "after" solution (AKA applying
$SUBJECT patch) vs. _not_ taking $SUBJECT patch and instead changing
dev_err_ratelimited() to dev_err().
$ scripts/bloat-o-meter \
.../after/panel-novatek-nt36672e.ko \
.../noratelimit/panel-novatek-nt36672e.ko
add/remove: 0/0 grow/shrink: 1/0 up/down: 3404/0 (3404)
Function old new delta
nt36672e_1080x2408_60hz_init 7260 10664 +3404
Total: Before=11669, After=15073, chg +29.17%
...so $SUBJECT patch is still better.
---
Where does that leave us? IMO:
a) If others agree, we should land $SUBJECT patch. It doesn't change
the behavior at all and gives big savings. It adds an extra function
hop, but presumably the fact that we have to fetch _a lot_ less stuff
from RAM might mean we still get better performance (likely it doesn't
matter anyway since this is not hotpath code).
b) Atop this patch, we should consider changing dev_err_ratelimited()
to dev_err(). It doesn't seem to make lots of sense to me to ratelimit
this error.
c) Atop this patch, we should consider making the two existing macros
"deprecated" in favor of a new macro that makes the control flow more
obvious.
How does that sound to folks?
-Doug
