| Message ID | 20210914091415.17918-9-jslaby@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | [01/16] tty: unexport tty_ldisc_release | expand |
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
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
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
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 --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);
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(-)