diff mbox series

[16/16] tty: drop tty_flip_buffer_push

Message ID 20210914091415.17918-9-jslaby@suse.cz
State New
Headers show
Series [01/16] tty: unexport tty_ldisc_release | expand

Commit Message

Jiri Slaby Sept. 14, 2021, 9:14 a.m. UTC
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.

One less exported function.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/tty_buffer.c | 17 -----------------
 include/linux/tty_flip.h |  1 -
 2 files changed, 18 deletions(-)

Comments

Johan Hovold Sept. 16, 2021, 10:03 a.m. UTC | #1
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.


Did you consider inlining tty_flip_buffer_push() or unexporting
tty_schedule_flip() instead?

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.

Also, can you please start adding cover letters to your series to
provide an overview of what it is you're trying to accomplish?

Johan
Jiri Slaby Sept. 22, 2021, 6:57 a.m. UTC | #2
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.

> 

> Did you consider inlining tty_flip_buffer_push() or unexporting

> tty_schedule_flip() instead?


Yes -- I see no reason for two functions doing the very same thing. It's 
only confusing.

> 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.

> 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.

thanks,
-- 
js
suse labs
Johan Hovold Sept. 23, 2021, 8:32 a.m. UTC | #3
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
Jiri Slaby Nov. 18, 2021, 7:54 a.m. UTC | #4
Friendly ping Johan, Greg: any opinions on the tty_schedule_flip vs 
tty_flip_buffer_push case -- which one should I keep?

I would like to move forward with these as I have a lot kernel-doc 
writings pending and depending on this patch (be it "drop 
tty_flip_buffer_push" or "drop tty_schedule_flip").

Thanks.

On 22. 09. 21, 8:57, 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.
>>
>> Did you consider inlining tty_flip_buffer_push() or unexporting
>> tty_schedule_flip() instead?
> 
> Yes -- I see no reason for two functions doing the very same thing. It's 
> only confusing.
> 
>> 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.
diff mbox series

Patch

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 635d0af229b7..19b44639c464 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -550,23 +550,6 @@  static void flush_to_ldisc(struct work_struct *work)
 
 }
 
-/**
- *	tty_flip_buffer_push	-	terminal
- *	@port: tty port to push
- *
- *	Queue a push of the terminal flip buffers to the line discipline.
- *	Can be called from IRQ/atomic context.
- *
- *	In the event of the queue being busy for flipping the work will be
- *	held off and retried later.
- */
-
-void tty_flip_buffer_push(struct tty_port *port)
-{
-	tty_schedule_flip(port);
-}
-EXPORT_SYMBOL(tty_flip_buffer_push);
-
 /**
  *	tty_buffer_init		-	prepare a tty buffer structure
  *	@port: tty port to initialise
diff --git a/include/linux/tty_flip.h b/include/linux/tty_flip.h
index 9916acb5de49..7efef54df932 100644
--- a/include/linux/tty_flip.h
+++ b/include/linux/tty_flip.h
@@ -16,7 +16,6 @@  int tty_insert_flip_string_fixed_flag(struct tty_port *port,
 		const unsigned char *chars, char flag, size_t size);
 int tty_prepare_flip_string(struct tty_port *port, unsigned char **chars,
 		size_t size);
-void tty_flip_buffer_push(struct tty_port *port);
 void tty_schedule_flip(struct tty_port *port);
 int __tty_insert_flip_char(struct tty_port *port, unsigned char ch, char flag);