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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Mon, 29 Apr 2024 07:13:08 -0700
From: Doug Anderson <dianders@...omium.org>
To: Sam Ravnborg <sam@...nborg.org>
Cc: dri-devel@...ts.freedesktop.org, Linus Walleij <linus.walleij@...aro.org>, 
	lvzhaoxiong@...qin.corp-partner.google.com, 
	Jani Nikula <jani.nikula@...ux.intel.com>, Hsin-Yi Wang <hsinyi@...gle.com>, 
	Javier Martinez Canillas <javierm@...hat.com>, Neil Armstrong <neil.armstrong@...aro.org>, 
	Joel Selvaraj <jo@...amily.in>, Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, 
	Cong Yang <yangcong5@...qin.corp-partner.google.com>, 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()

Hi,

On Fri, Apr 26, 2024 at 11:33 PM Sam Ravnborg <sam@...nborg.org> wrote:
>
> > ---
> > Right now this patch introduces two new functions in
> > drm_mipi_dsi.c. Alternatively we could have changed the prototype of
> > the "chatty" functions and made the deprecated macros adapt to the new
> > prototype. While this sounds nice, it bloated callers of the
> > deprecated functioin a bit because it caused the compiler to emit less
> > optimal code. It doesn't seem terrible to add two more functions, so I
> > went that way.
> The concern here is when it will be cleaned up. As a minimum please
> consider adding an item to todo.rst that details what should be done
> to get rid of the now obsolete chatty functions so someone can pick it
> up.

Sure, I can add something to do TODO. Do folks agree that's the
preferred way to do things? A few thoughts I had:

1. In theory it could be useful to keep _both_ the "chatty" and
"multi" variants of the functions. In at least a few places the
"multi" variant was awkward. The resulting auo_kd101n80_45na_init()
[1] looked awkward. If I was writing just this function I would have
chosen an API more like the "chatty" one, but since I was trying to
make all the init functions similar I kept them all using the "multi"
API. Does it make sense to keep both long term?

[1] https://lore.kernel.org/all/20240426165839.v2.7.Ib5030ab5cd41b4e08b1958bd7e51571725723008@changeid/

2. Going completely the opposite direction, we could also not bother
saving as much space today and _force_ everyone to move to the new
"multi" API to get the full space savings.


So I guess three options: a) leave my patches the way they are and add
a TODO, b) Keep the "chatty" variants long term and remove my
"after-the-cut", or c) Don't get as much space savings today but don't
introduce a new exported function. Opinions?


> > @@ -792,6 +792,34 @@ int mipi_dsi_generic_write_chatty(struct mipi_dsi_device *dsi,
> >  }
> >  EXPORT_SYMBOL(mipi_dsi_generic_write_chatty);
> >
> > +/**
> > + * mipi_dsi_generic_write_multi() - mipi_dsi_generic_write_chatty() w/ accum_err
> > + * @ctx: Context for multiple DSI transactions
> > + * @payload: buffer containing the payload
> > + * @size: size of payload buffer
> > + *
> > + * Like mipi_dsi_generic_write_chatty() but deals with errors in a way that
> > + * makes it convenient to make several calls in a row.
> > + */
> > +void mipi_dsi_generic_write_multi(struct mipi_dsi_multi_context *ctx,
> > +                               const void *payload, size_t size)
> > +{
> > +     struct mipi_dsi_device *dsi = ctx->dsi;
> > +     struct device *dev = &dsi->dev;
> > +     ssize_t ret;
> > +
> > +     if (ctx->accum_err)
> > +             return;
> > +
> > +     ret = mipi_dsi_generic_write(dsi, payload, size);
> > +     if (ret < 0) {
> > +             ctx->accum_err = ret;
> > +             dev_err_ratelimited(dev, "sending generic data %*ph failed: %d\n",
> > +                                 (int)size, payload, ctx->accum_err);
> > +     }
> I see no point in using ratelimited and then change it in the next
> patch.

Sure, I'll re-order patches.


> > @@ -197,6 +197,22 @@ struct mipi_dsi_device {
> >       struct drm_dsc_config *dsc;
> >  };
> >
> > +/**
> > + * 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;
> > +};
> Inline comments is - I think - preferred.

Sure, I'll update in the next version.