Re: [PATCH 16/16] tty: drop tty_flip_buffer_push

[Johan Hovold]

On Wed, Sep 22, 2021 at 08:57:17AM +0200, Jiri Slaby wrote:
> On 16. 09. 21, 12:03, Johan Hovold wrote:
> > On Tue, Sep 14, 2021 at 11:14:15AM +0200, Jiri Slaby wrote:
> >> Since commit a9c3f68f3cd8d (tty: Fix low_latency BUG) in 2014,
> >> tty_flip_buffer_push() is only a wrapper to tty_schedule_flip(). All
> >> users were converted, so remove tty_flip_buffer_push() completely.

> > The name tty_flip_buffer_push() is arguable more descriptive since the
> > work may already be running and is also less tied to the implementation.
> > 
> > The ratio of drivers using tty_flip_buffer_push() over
> > tty_schedule_flip() is also something like 186 to 15 so that would
> > amount to a lot less churn too.
> 
> OK, I can do either way. I chose this path as tty_schedule_flip was a 
> wrapper to tty_flip_buffer_push. In any case, I wouldn't take the number 
> of changed drivers as a measure. But if it makes more sense for people 
> regarding the naming, I will "flip" the two flips.

I'd prefer that. The name tty_flip_buffer_push() is more descriptive and
we've been using it in virtually all tty drivers since 1997. No need to
force people to relearn the pattern of tty insert + push.

(Also note that the kernel doc for tty_schedule_flip() says "push
characters to ldisc", while the name more reflects how the tty buffering
was originally implemented.)
 
> > Also, can you please start adding cover letters to your series to
> > provide an overview of what it is you're trying to accomplish?
> 
> I am not a fan of cover letters as they are not Cced to people who are 
> Cced in separate patches. So what would you like to see in the letter? 
> This series are just a random cleanup and IMO there is not much more to 
> be said except what is in their commit logs.

Even if all it said was "random cleanups" it would still be worth having
as I'd know that this is just Jiri moving code about and not necessarily
something that needs deeper review.

In this case the driver commit messages only said that "[w]e are going
to remove [tty_flip_buffer_push()]" without any real explanation why
that was chosen over the alternatives. I had to look up the series on
lore to look for more details. But since there's no cover letter I end
up having to skim the entire series only to come up mostly empty handed
as the final patch just added "One less exported function" as some sort
of motivation.

Let's not make it harder for reviewers. If you're sending random cleanup
patches then say so. And if they can be grouped into a few classes as
seemed to be the case here then having those outlined is also helpful.

Johan