Re: [PATCH 1/1] tty: serial: use dmaengine_desc_set_callback()

[Thierry Reding]

On Thu, Nov 05, 2026 at 08:02:34PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 05, 2026 at 05:21:06PM +0100, Krzysztof Kozlowski wrote:
> > On 05/11/2026 17:18, Andy Shevchenko wrote:
> > > On Thu, Nov 05, 2026 at 04:36:38PM +0100, Thierry Reding wrote:
> > >> On Thu, Nov 05, 2026 at 04:17:53PM +0200, Andy Shevchenko wrote:
> > >>> On Thu, Nov 05, 2026 at 01:28:46PM +0100, Thierry Reding wrote:
> > >>>> On Mon, Nov 02, 2026 at 12:54:48PM -0500, Frank.Li@xxxxxxxxxxx wrote:
> > >>>>>
> > >>>>> Replace open-coded assignments to callback and callback_param fields
> > >>>>> of struct dma_async_tx_descriptor with calls to the
> > >>>>> dmaengine_desc_set_callback() helper across serial drivers.
> > >>>>>
> > >>>>> No functional change intended. Prepare to make struct
> > >>>>> dma_async_tx_descriptor private to the DMA engine.
> > >>>>>
> > >>>>> Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > >>>>> ---
> > >>>>>  drivers/tty/serial/8250/8250_dma.c  | 6 ++----
> > >>>>>  drivers/tty/serial/8250/8250_mtk.c  | 3 +--
> > >>>>>  drivers/tty/serial/8250/8250_omap.c | 6 ++----
> > >>>>>  drivers/tty/serial/amba-pl011.c     | 6 ++----
> > >>>>>  drivers/tty/serial/atmel_serial.c   | 6 ++----
> > >>>>>  drivers/tty/serial/fsl_lpuart.c     | 6 ++----
> > >>>>>  drivers/tty/serial/imx.c            | 6 ++----
> > >>>>>  drivers/tty/serial/msm_serial.c     | 6 ++----
> > >>>>>  drivers/tty/serial/mxs-auart.c      | 6 ++----
> > >>>>>  drivers/tty/serial/pch_uart.c       | 6 ++----
> > >>>>>  drivers/tty/serial/samsung_tty.c    | 6 ++----
> > >>>>>  drivers/tty/serial/serial-tegra.c   | 6 ++----
> > >>>>>  drivers/tty/serial/sh-sci.c         | 9 +++------
> > >>>>>  drivers/tty/serial/sprd_serial.c    | 3 +--
> > >>>>>  drivers/tty/serial/stm32-usart.c    | 6 ++----
> > >>>>>  15 files changed, 29 insertions(+), 58 deletions(-)
> > >>>>
> > >>>> Reviewed-by: Thierry Reding <treding@xxxxxxxxxx>
> > >>>
> > >>> Krzysztof, now look at this case. I would argue that this is even less fit than
> > >>> # subpart(s) case, because it might imply anything. Again the problem is the
> > >>> bundle, and for such a case (when tons of independent changes are bundled in
> > >>> the same stuff, *every* Rb tag has to be explained on what has been reviewed!
> > >>
> > >> It may be a bit subtle, but the placement of the tag indicates that it
> > >> applies to the entirety of the changes. I didn't crop the list of
> > >> changed files, nor did I omit the summary.
> > > 
> > > This doesn't clarify the question. Wordless Rb tag on a bundles is an oddity.
> > 
> > No, it's perfectly normal and typical. I would not expect anything else,
> > because writing "LGTM" is pointless. We are experienced reviewers and we
> > do understand what Reviewer's statement of oversight means, thus giving
> > the tag is obvious, explicit and not odd.
> > 
> > > 
> > >> That bit of meta-data obviously isn't picked up into the tag line, but
> > >> I think the default meaning of Reviewed-by is that it applies to the
> > >> entire thing.
> > > 
> > > And I interpret it otherwise, because this is a set of independent changes
> > > bundled into a single patch!
> > 
> > It's not set of independent changes, but even if it was then it would
> > not matter. We review the code regardless how we actually judge the
> > atomicity of a patch.
> 
> And I confront exactly that! When the patch may be split it has to. This is
> seems the outcome of this discussion to me. From now on I think I will NAK
> any bundle patch until piece that I in charge to is split from the bundle.

I'd like to caution against this. There's effectively one change in this
patch and it converts all the drivers in the same way. This was probably
generated using a script or semantic patch. Requiring this to be split
up is going to make the author spend a lot of time creating 15 patches
instead of just one. They will have to write the same commit message
with tiny variations, which they'll most likely just use a script for
because it's tedious. Or they don't use a script and likely get at least
one commit message wrong. And then we're not reviewing a patch for
technical correctness anymore but waste a lot of time on nit-picking on
a subject or commit message. Each of these commit messages is not going
to add any new information either, so the only thing you really achieve
is to increase the signal-to-noise ratio.

That's completely absurd.

Also, reviewing 15 miniature patches is going to take much more time
that reviewing this monolithic one, so I suspect potential reviewers are
more likely to just stay away from it to begin with.

In the end you make life miserable for everyone. Well, except for
individual driver maintainers that want a quick and easy way to slap
their approval onto only the one driver change that they really care
about.

But there's already a way that they can do that. Acked-by can just mean
"I maintain this driver and I agree with the direction, but I haven't
bothered to do a full technical review". That's what most of these
"partial" Reviewed-bys are, so why not just use that existing tool?

And I'm saying all of this without judgement, because sometimes you do
not have the time to do a full review but you still want to lend your
support to ongoing work. I do that quite often myself.

But at other times I do want to look at something in more detail, or
just ended up looking thoroughly enough for it to count as a full review
and when I do I'll let people know with a Reviewed-by.

Thierry

Attachment: signature.asc
Description: PGP signature