linux-kernel - Re: [PATCH 1/5] spi: add spi_optimize

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: Tue, 13 Feb 2024 13:20:31 -0600
From: David Lechner <dlechner@...libre.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Mark Brown <broonie@...nel.org>, Martin Sperl <kernel@...tin.sperl.org>, 
	David Jander <david@...tonic.nl>, Jonathan Cameron <jic23@...nel.org>, 
	Michael Hennerich <michael.hennerich@...log.com>, Nuno Sá <nuno.sa@...log.com>, 
	Alain Volmat <alain.volmat@...s.st.com>, Maxime Coquelin <mcoquelin.stm32@...il.com>, 
	Alexandre Torgue <alexandre.torgue@...s.st.com>, linux-spi@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com, 
	linux-arm-kernel@...ts.infradead.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH 1/5] spi: add spi_optimize_message() APIs

On Tue, Feb 13, 2024 at 11:25 AM Jonathan Cameron
<Jonathan.Cameron@...wei.com> wrote:
>
>
> I thought about suggesting splitting this into an initial patch that just does
> the bits without the controller callbacks. Maybe it would work better that way
> with that introduced after the validate and splitting of transfers (so most
> of patches 1 and 2) as a patch 3 prior to the stm32 additions?

Unless anyone else feels the same way, I'm inclined to avoid the extra
work of splitting it up.


> > +static void __spi_unoptimize_message(struct spi_message *msg)
> > +{
> > +     struct spi_controller *ctlr = msg->spi->controller;
> > +
> > +     if (ctlr->unoptimize_message)
> > +             ctlr->unoptimize_message(msg);
> > +
> > +     msg->optimized = false;
> > +     msg->opt_state = NULL;
> > +}
>
> Seems misbalanced that this doesn't take a pre_optimized flag in but
> __spi_optimize does. I'd move handling that to outside the call in both cases.
>
>

Agreed.


> > @@ -4331,10 +4463,15 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message)
> >               return -ESHUTDOWN;
> >       }
> >
> > -     status = __spi_validate(spi, message);
> > -     if (status != 0)
> > +     status = spi_maybe_optimize_message(spi, message);
> > +     if (status)
> >               return status;
> >
> > +     /*
> > +      * NB: all return paths after this point must ensure that
> > +      * spi_finalize_current_message() is called to avoid leaking resources.
>
> I'm not sure a catch all like that makes sense. Not sufficient to call
> the finer grained spi_maybe_unoptimize_message()  ?

Hmm... this is my bias from a previous fix showing through. Maybe this
comment doesn't belong in this patch. The short answer to your
question is "it's complicated".