linux-kernel - Re: [PATCH v2 4/8] drm/mipi-dsi: Introduce mipi_dsi_*_write_seq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87y18w2n6h.fsf@intel.com>
Date: Mon, 29 Apr 2024 18:39:18 +0300
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Doug Anderson <dianders@...omium.org>, neil.armstrong@...aro.org
Cc: dri-devel@...ts.freedesktop.org, Linus Walleij
 <linus.walleij@...aro.org>, lvzhaoxiong@...qin.corp-partner.google.com,
 Hsin-Yi Wang <hsinyi@...gle.com>, Javier Martinez Canillas
 <javierm@...hat.com>, Joel Selvaraj <jo@...amily.in>, Dmitry Baryshkov
 <dmitry.baryshkov@...aro.org>, Cong Yang
 <yangcong5@...qin.corp-partner.google.com>, Sam Ravnborg
 <sam@...nborg.org>, Daniel Vetter <daniel@...ll.ch>, David Airlie
 <airlied@...il.com>, Maarten Lankhorst
 <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
 Thomas Zimmermann <tzimmermann@...e.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/8] drm/mipi-dsi: Introduce
 mipi_dsi_*_write_seq_multi()

On Mon, 29 Apr 2024, Doug Anderson <dianders@...omium.org> wrote:
> Hi,
>
> On Mon, Apr 29, 2024 at 2:38 AM Neil Armstrong
> <neil.armstrong@...aro.org> wrote:
>>
>> > +/**
>> > + * struct mipi_dsi_multi_context - Context to call multiple MIPI DSI funcs in a row
>> > + * @dsi: Pointer to the MIPI DSI device
>> > + * @accum_err: Storage for the accumulated error over the multiple calls. Init
>> > + *   to 0. If a function encounters an error then the error code will be
>> > + *   stored here. If you call a function and this points to a non-zero
>> > + *   value then the function will be a noop. This allows calling a function
>> > + *   many times in a row and just checking the error at the end to see if
>> > + *   any of them failed.
>> > + */
>> > +
>> > +struct mipi_dsi_multi_context {
>> > +     struct mipi_dsi_device *dsi;
>> > +     int accum_err;
>> > +};
>>
>> I like the design, but having a context struct seems over-engineered while we could pass
>> a single int over without encapsulating it with mipi_dsi_multi_context.
>>
>> void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_multi_context *ctx,
>>                                      const void *data, size_t len);
>> vs
>> void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_device *dsi, int *accum_err,
>>                                      const void *data, size_t len);
>>
>> is the same, and it avoids having to declare a mipi_dsi_multi_context and set ctx->dsi,
>> and I'll find it much easier to migrate, just add a &ret and make sure ret is initialized to 0.
>
> Yeah, I had the same reaction when Jani suggested the context style
> [1] and I actually coded it up exactly as you suggest above. I then
> changed my mind and went with the context. My motivation was that when
> I tested it I found that using the context produced smaller code.
> Specifically, from the description of this patch we see we end up
> with:
>
> Total: Before=10651, After=9663, chg -9.28%
>
> ...when I didn't have the context and I had the accum_err then instead
> of getting ~9% smaller I believe it actually got ~0.5% bigger. This
> makes some sense as the caller has to pass 4 parameters to each call
> instead of 3.
>
> It's not a giant size difference but it was at least some motivation
> that helped push me in this direction. I'd also say that when I looked
> at the code in the end the style grew on me. It's really not too
> terrible to have one line in your functions that looks like:
>
> struct mipi_dsi_multi_context ctx = { .dsi = boe->dsi };
>
> ...and if that becomes the canonical way to do it then it's really
> hard to accidentally forget to initialize the error value. With the
> other API it's _very_ easy to forget to initialize the error value and
> the compiler won't yell at you. It also makes it very obvious to the
> caller that this function is doing something a little different than
> most Linux APIs with that error return.
>
> So I guess I'd say that I ended up being pretty happy with the
> "context" even if it does feel a little over engineered and I'd argue
> to keep it that way. That being said, if you feel strongly about it
> then we can perhaps get others to chime in to see which style they
> prefer? Let me know what you think.
>
>
> [1] https://lore.kernel.org/r/8734r85tcf.fsf@intel.com

FWIW, I don't feel strongly about this, and I could be persuaded either
way, but I've got this gut feeling that an extensible context parameter
might be benefitial future proofing in this case.

BR,
Jani.


-- 
Jani Nikula, Intel