On Thu, 25 Apr 2024, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> 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);
No reason you couldn't do error logging here
if (*accum_ret)
dev_err(...)
> }
>
> ...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.
I like this option the best, for the simple reason that the caller side
is aware of what's going on, there's no magic control flow happening,
and they can add error handling in the middle if they so choose.
I don't find this unintuitive, but if it helps, you could conceivably
add a context parameter:
struct mipi_dsi_seq_context context = {
.dsi = dsi,
};
mipi_dsi_dcs_write_seq(&context, HX83102_SETSPCCMD, 0xcd);
...
if (context.ret)
...
And even have further control in the context whether to log or keep
going or whatever.
I don't think the efficiency in error cases is a problem worth thinking
about, but you could address that by turning this into a macro so
there's no extra calls on errors.
BR,
Jani.
>
> ---
>
> 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
--
Jani Nikula, Intel
