[PATCH] TTY:vt: convert remain take_over_console's users to do_take_over_console

[PATCH] TTY:vt: convert remain take_over_console's users to do_take_over_console

[Wang YanQing]

Impact:
1:convert all remain take_over_console to do_take_over_console
2:update take_over_console to do_take_over_console in comment

Commit dc9641895abb ("vt: delete unneeded functions
register_con_driver|take_over_console") delete take_over_console,
but forget to convert remain take_over_console's users to new API
do_take_over_console, this patch fix it.

Signed-off-by: Wang YanQing <udknight@gmail.com>
---
 Sorry for my mistake, I believe I do the full
 kernel source find|grep, but the reality is I 
 forget  take_over_console.
 
 I will send the patch to update DOC tonight.
 arch/alpha/kernel/console.c             |  4 +++-
 arch/alpha/kernel/process.c             |  4 +++-
 arch/mips/pci/pci-bcm1480.c             |  4 +++-
 arch/mips/pci/pci-sb1250.c              |  4 +++-
 arch/parisc/kernel/setup.c              |  2 +-
 drivers/tty/vt/vt.c                     |  2 +-
 drivers/usb/misc/sisusbvga/sisusb_con.c | 18 +++++++++++-------
 drivers/video/console/fbcon.c           |  2 +-
 drivers/video/console/mdacon.c          |  8 ++++++--
 drivers/video/console/newport_con.c     |  9 ++++++---
 drivers/video/console/sticon.c          |  6 +++++-
 11 files changed, 43 insertions(+), 20 deletions(-)

diff --git a/arch/alpha/kernel/console.c b/arch/alpha/kernel/console.c
index da711e3..6a61dee 100644
--- a/arch/alpha/kernel/console.c
+++ b/arch/alpha/kernel/console.c
@@ -61,7 +61,9 @@ locate_and_init_vga(void *(*sel_func)(void *, void *))
 
 	/* Set the VGA hose and init the new console. */
 	pci_vga_hose = hose;
-	take_over_console(&vga_con, 0, MAX_NR_CONSOLES-1, 1);
+	console_lock();
+	do_take_over_console(&vga_con, 0, MAX_NR_CONSOLES-1, 1);
+	console_unlock();
 }
 
 void __init
diff --git a/arch/alpha/kernel/process.c b/arch/alpha/kernel/process.c
index ab80a80..f2360a7 100644
--- a/arch/alpha/kernel/process.c
+++ b/arch/alpha/kernel/process.c
@@ -117,7 +117,9 @@ common_shutdown_1(void *generic_ptr)
 		if (in_interrupt())
 			irq_exit();
 		/* This has the effect of resetting the VGA video origin.  */
-		take_over_console(&dummy_con, 0, MAX_NR_CONSOLES-1, 1);
+		console_lock();
+		do_take_over_console(&dummy_con, 0, MAX_NR_CONSOLES-1, 1);
+		console_unlock();
 #endif
 		pci_restore_srm_config();
 		set_hae(srm_hae);
diff --git a/arch/mips/pci/pci-bcm1480.c b/arch/mips/pci/pci-bcm1480.c
index e2e69e1..44dd5aa 100644
--- a/arch/mips/pci/pci-bcm1480.c
+++ b/arch/mips/pci/pci-bcm1480.c
@@ -257,7 +257,9 @@ static int __init bcm1480_pcibios_init(void)
 	register_pci_controller(&bcm1480_controller);
 
 #ifdef CONFIG_VGA_CONSOLE
-	take_over_console(&vga_con, 0, MAX_NR_CONSOLES-1, 1);
+	console_lock();
+	do_take_over_console(&vga_con, 0, MAX_NR_CONSOLES-1, 1);
+	console_unlock();
 #endif
 	return 0;
 }
diff --git a/arch/mips/pci/pci-sb1250.c b/arch/mips/pci/pci-sb1250.c
index cdefcc4..fc634ae 100644
--- a/arch/mips/pci/pci-sb1250.c
+++ b/arch/mips/pci/pci-sb1250.c
@@ -283,7 +283,9 @@ static int __init sb1250_pcibios_init(void)
 	register_pci_controller(&sb1250_controller);
 
 #ifdef CONFIG_VGA_CONSOLE
-	take_over_console(&vga_con, 0, MAX_NR_CONSOLES - 1, 1);
+	console_lock();
+	do_take_over_console(&vga_con, 0, MAX_NR_CONSOLES - 1, 1);
+	console_unlock();
 #endif
 	return 0;
 }
diff --git a/arch/parisc/kernel/setup.c b/arch/parisc/kernel/setup.c
index 76b63e7..60c1ae6 100644
--- a/arch/parisc/kernel/setup.c
+++ b/arch/parisc/kernel/setup.c
@@ -155,7 +155,7 @@ void __init setup_arch(char **cmdline_p)
 #endif
 
 #if defined(CONFIG_VT) && defined(CONFIG_DUMMY_CONSOLE)
-	conswitchp = &dummy_con;	/* we use take_over_console() later ! */
+	conswitchp = &dummy_con;	/* we use do_take_over_console() later ! */
 #endif
 
 }
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 405e1c9..d4ba2ba 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3592,7 +3592,7 @@ EXPORT_SYMBOL_GPL(do_unregister_con_driver);
  *	when a driver wants to take over some existing consoles
  *	and become default driver for newly opened ones.
  *
- *	take_over_console is basically a register followed by unbind
+ *	do_take_over_console is basically a register followed by unbind
  */
 int do_take_over_console(const struct consw *csw, int first, int last, int deflt)
 {
diff --git a/drivers/usb/misc/sisusbvga/sisusb_con.c b/drivers/usb/misc/sisusbvga/sisusb_con.c
index 411e605..a638c4e 100644
--- a/drivers/usb/misc/sisusbvga/sisusb_con.c
+++ b/drivers/usb/misc/sisusbvga/sisusb_con.c
@@ -208,7 +208,7 @@ sisusbcon_init(struct vc_data *c, int init)
 	struct sisusb_usb_data *sisusb;
 	int cols, rows;
 
-	/* This is called by take_over_console(),
+	/* This is called by do_take_over_console(),
 	 * ie by us/under our control. It is
 	 * only called after text mode and fonts
 	 * are set up/restored.
@@ -273,7 +273,7 @@ sisusbcon_deinit(struct vc_data *c)
 	struct sisusb_usb_data *sisusb;
 	int i;
 
-	/* This is called by take_over_console()
+	/* This is called by do_take_over_console()
 	 * and others, ie not under our control.
 	 */
 
@@ -1490,8 +1490,9 @@ sisusb_console_init(struct sisusb_usb_data *sisusb, int first, int last)
 	mutex_unlock(&sisusb->lock);
 
 	/* Now grab the desired console(s) */
-	ret = take_over_console(&sisusb_con, first - 1, last - 1, 0);
-
+	console_lock();
+	ret = do_take_over_console(&sisusb_con, first - 1, last - 1, 0);
+	console_unlock();
 	if (!ret)
 		sisusb->haveconsole = 1;
 	else {
@@ -1535,11 +1536,14 @@ sisusb_console_exit(struct sisusb_usb_data *sisusb)
 
 	if (sisusb->haveconsole) {
 		for (i = 0; i < MAX_NR_CONSOLES; i++)
-			if (sisusb->havethisconsole[i])
-				take_over_console(&sisusb_dummy_con, i, i, 0);
+			if (sisusb->havethisconsole[i]) {
+				console_lock();
+				do_take_over_console(&sisusb_dummy_con, i, i, 0);
+				console_unlock();
 				/* At this point, con_deinit for all our
-				 * consoles is executed by take_over_console().
+				 * consoles is executed by do_take_over_console().
 				 */
+			}
 		sisusb->haveconsole = 0;
 	}
 
diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index a09c667..d55b337 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -873,7 +873,7 @@ static int set_con2fb_map(int unit, int newidx, int user)
 /*
  *  Low Level Operations
  */
-/* NOTE: fbcon cannot be __init: it may be called from take_over_console later */
+/* NOTE: fbcon cannot be __init: it may be called from do_take_over_console later */
 static int var_to_display(struct display *disp,
 			  struct fb_var_screeninfo *var,
 			  struct fb_info *info)
diff --git a/drivers/video/console/mdacon.c b/drivers/video/console/mdacon.c
index 0b67866..296e945 100644
--- a/drivers/video/console/mdacon.c
+++ b/drivers/video/console/mdacon.c
@@ -585,10 +585,14 @@ static const struct consw mda_con = {
 
 int __init mda_console_init(void)
 {
+	int err;
+
 	if (mda_first_vc > mda_last_vc)
 		return 1;
-
-	return take_over_console(&mda_con, mda_first_vc-1, mda_last_vc-1, 0);
+	console_lock();
+	err = do_take_over_console(&mda_con, mda_first_vc-1, mda_last_vc-1, 0);
+	console_unlock();
+	return err;
 }
 
 static void __exit mda_console_exit(void)
diff --git a/drivers/video/console/newport_con.c b/drivers/video/console/newport_con.c
index b05afd0..a6ab929 100644
--- a/drivers/video/console/newport_con.c
+++ b/drivers/video/console/newport_con.c
@@ -297,7 +297,7 @@ static void newport_exit(void)
 		newport_set_def_font(i, NULL);
 }
 
-/* Can't be __init, take_over_console may call it later */
+/* Can't be __init, do_take_over_console may call it later */
 static const char *newport_startup(void)
 {
 	int i;
@@ -746,6 +746,7 @@ static int newport_probe(struct gio_device *dev,
 			 const struct gio_device_id *id)
 {
 	unsigned long newport_addr;
+	int err;
 
 	if (!dev->resource.start)
 		return -EINVAL;
@@ -759,8 +760,10 @@ static int newport_probe(struct gio_device *dev,
 
 	npregs = (struct newport_regs *)/* ioremap cannot fail */
 		ioremap(newport_addr, sizeof(struct newport_regs));
-
-	return take_over_console(&newport_con, 0, MAX_NR_CONSOLES - 1, 1);
+	console_lock();
+	err = do_take_over_console(&newport_con, 0, MAX_NR_CONSOLES - 1, 1);
+	console_unlock();
+	return err;
 }
 
 static void newport_remove(struct gio_device *dev)
diff --git a/drivers/video/console/sticon.c b/drivers/video/console/sticon.c
index 491c1c1..5f65ca3 100644
--- a/drivers/video/console/sticon.c
+++ b/drivers/video/console/sticon.c
@@ -372,6 +372,7 @@ static const struct consw sti_con = {
 
 static int __init sticonsole_init(void)
 {
+    int err;
     /* already initialized ? */
     if (sticon_sti)
 	 return 0;
@@ -382,7 +383,10 @@ static int __init sticonsole_init(void)
 
     if (conswitchp == &dummy_con) {
 	printk(KERN_INFO "sticon: Initializing STI text console.\n");
-	return take_over_console(&sti_con, 0, MAX_NR_CONSOLES - 1, 1);
+	console_lock();
+	err = do_take_over_console(&sti_con, 0, MAX_NR_CONSOLES - 1, 1);
+	console_unlock();
+	return err;
     }
     return 0;
 }
-- 
1.7.12.4.dirty

Re: [PATCH] TTY:vt: convert remain take_over_console's users to do_take_over_console

[Peter Hurley]

On 05/21/2013 01:15 AM, Wang YanQing wrote:
> Impact:
> 1:convert all remain take_over_console to do_take_over_console
> 2:update take_over_console to do_take_over_console in comment
>
> Commit dc9641895abb ("vt: delete unneeded functions
> register_con_driver|take_over_console") delete take_over_console,
> but forget to convert remain take_over_console's users to new API
> do_take_over_console, this patch fix it.
>
> Signed-off-by: Wang YanQing <udknight@gmail.com>
> ---
>   Sorry for my mistake, I believe I do the full
>   kernel source find|grep, but the reality is I
>   forget  take_over_console.

I would rather revert dc9641895abb which purported to delete
_unneeded_ functions than have this. Obviously the functions
were needed.

Regards,
Peter Hurley

Re: [PATCH] TTY:vt: convert remain take_over_console's users to do_take_over_console

[Wang YanQing]

On Tue, May 21, 2013 at 09:10:33AM -0400, Peter Hurley wrote:
> I would rather revert dc9641895abb which purported to delete
> _unneeded_ functions than have this. Obviously the functions
> were needed.
> 

Hi Peter, this series patches' goal is to reduce codes'
redundance and function duplication. But if we keep take_over_console, 
then we have to rewrite it as a trivial wrapper over do_take_over_console,
or we have to keep bind_con_driver and register_con_driver, and this
will bring use codes' redundance.

And if we rewrite take_over_console as a wrapper over
do_take_over_console, it is so trivial, delete it and let kernel
use the unified version of APIs will simplify the APIs.

Thanks.

Re: [PATCH] TTY:vt: convert remain take_over_console's users to do_take_over_console

[Peter Hurley]

On 05/21/2013 10:42 AM, Wang YanQing wrote:
> On Tue, May 21, 2013 at 09:10:33AM -0400, Peter Hurley wrote:
>> I would rather revert dc9641895abb which purported to delete
>> _unneeded_ functions than have this. Obviously the functions
>> were needed.
>>
>
> Hi Peter, this series patches' goal is to reduce codes'
> redundance and function duplication. But if we keep take_over_console,
> then we have to rewrite it as a trivial wrapper over do_take_over_console,
> or we have to keep bind_con_driver and register_con_driver, and this
> will bring use codes' redundance.
>
> And if we rewrite take_over_console as a wrapper over
> do_take_over_console, it is so trivial, delete it and let kernel
> use the unified version of APIs will simplify the APIs.

Except now you're spreading the brokenness that is console_lock()
over many more source files than the single-use case of
do_take_over_console().

The actual interface is take_over_console(); the _workaround_ is
exposing do_take_over_console() for fbcon to wrap.

Regards,
Peter Hurley

Re: [PATCH] TTY:vt: convert remain take_over_console's users to do_take_over_console

[Wang YanQing]

On Tue, May 21, 2013 at 11:48:58AM -0400, Peter Hurley wrote:
> On 05/21/2013 10:42 AM, Wang YanQing wrote:
> > On Tue, May 21, 2013 at 09:10:33AM -0400, Peter Hurley wrote:
> >> I would rather revert dc9641895abb which purported to delete
> >> _unneeded_ functions than have this. Obviously the functions
> >> were needed.
> >>
> >
> > Hi Peter, this series patches' goal is to reduce codes'
> > redundance and function duplication. But if we keep take_over_console,
> > then we have to rewrite it as a trivial wrapper over do_take_over_console,
> > or we have to keep bind_con_driver and register_con_driver, and this
> > will bring use codes' redundance.
> >
> > And if we rewrite take_over_console as a wrapper over
> > do_take_over_console, it is so trivial, delete it and let kernel
> > use the unified version of APIs will simplify the APIs.
> 
> Except now you're spreading the brokenness that is console_lock()
> over many more source files than the single-use case of
> do_take_over_console().

> The actual interface is take_over_console(); the _workaround_ is
> exposing do_take_over_console() for fbcon to wrap.

This _workaround_ willn't work, take_over_console will hold console_lock internal,
but do_take_over_console need caller hold console_lock, then we can't rewrite 
do_take_over_console as a wrap base on take_over_console. 

But the reverse is ok. So if we have to do it, then the actual interface 
is do_take_over_console, and the "_workaround_" is exposing take_over_console 
as a wrap base on do_take_over_console.

Re: [PATCH] TTY:vt: convert remain take_over_console's users to do_take_over_console

[Wang YanQing]

On Wed, May 22, 2013 at 12:18:49AM +0800, Wang YanQing wrote:
> > Except now you're spreading the brokenness that is console_lock()
> > over many more source files than the single-use case of
> > do_take_over_console().
> 
> > The actual interface is take_over_console(); the _workaround_ is
> > exposing do_take_over_console() for fbcon to wrap.
> 
> This _workaround_ willn't work, take_over_console will hold console_lock internal,
> but do_take_over_console need caller hold console_lock, then we can't rewrite 
> do_take_over_console as a wrap base on take_over_console. 
> 
> But the reverse is ok. So if we have to do it, then the actual interface 
> is do_take_over_console, and the "_workaround_" is exposing take_over_console 
> as a wrap base on do_take_over_console.

But if we do this, then we have two version functions do the same thing except
caller/callee hold lock, I can't see much sense to have them at the same time.

Thanks.

Re: [PATCH] TTY:vt: convert remain take_over_console's users to do_take_over_console

[Peter Hurley]

On 05/21/2013 12:29 PM, Wang YanQing wrote:
> On Wed, May 22, 2013 at 12:18:49AM +0800, Wang YanQing wrote:
>>> Except now you're spreading the brokenness that is console_lock()
>>> over many more source files than the single-use case of
>>> do_take_over_console().
>>
>>> The actual interface is take_over_console(); the _workaround_ is
>>> exposing do_take_over_console() for fbcon to wrap.
>>
>> This _workaround_ willn't work, take_over_console will hold console_lock internal,
>> but do_take_over_console need caller hold console_lock, then we can't rewrite
>> do_take_over_console as a wrap base on take_over_console.

The workaround I'm referring to is commit 50e244cc793 which
exposed do_take_over_console() as an interface to band aid the lock
problems.

I'm ok with take_over_console() being a lock wrapper around
do_take_over_console(), if you are trying to preserve your
other changes.

Regards,
Peter Hurley

Re: [PATCH] TTY:vt: convert remain take_over_console's users to do_take_over_console

[Pavel Machek]

On Tue 2013-05-21 13:15:12, Wang YanQing wrote:
> Impact:
> 1:convert all remain take_over_console to do_take_over_console

This is step backwards.

> --- a/arch/alpha/kernel/console.c
> +++ b/arch/alpha/kernel/console.c
> @@ -61,7 +61,9 @@ locate_and_init_vga(void *(*sel_func)(void *, void *))
>  
>  	/* Set the VGA hose and init the new console. */
>  	pci_vga_hose = hose;
> -	take_over_console(&vga_con, 0, MAX_NR_CONSOLES-1, 1);
> +	console_lock();
> +	do_take_over_console(&vga_con, 0, MAX_NR_CONSOLES-1, 1);
> +	console_unlock();
>  }

Original was better.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] TTY:vt: convert remain take_over_console's users to do_take_over_console

[Wang YanQing]

On Tue, Jun 04, 2013 at 10:13:18PM +0200, Pavel Machek wrote:
> On Tue 2013-05-21 13:15:12, Wang YanQing wrote:
> > Impact:
> > 1:convert all remain take_over_console to do_take_over_console
> 
> This is step backwards.

What is step backwards? do_take_over_console appear MUCH MUCH later in kernel
than take_over_console, do_take_over_console is the new API, I can't understand
what is step backwards.

> > --- a/arch/alpha/kernel/console.c
> > +++ b/arch/alpha/kernel/console.c
> > @@ -61,7 +61,9 @@ locate_and_init_vga(void *(*sel_func)(void *, void *))
> >  
> >  	/* Set the VGA hose and init the new console. */
> >  	pci_vga_hose = hose;
> > -	take_over_console(&vga_con, 0, MAX_NR_CONSOLES-1, 1);
> > +	console_lock();
> > +	do_take_over_console(&vga_con, 0, MAX_NR_CONSOLES-1, 1);
> > +	console_unlock();
> >  }
> 
> Original was better.

Except reduce some console_lock/unlock scatter scattered in kernel, I 
can't see the "BETTER", and it is not a BIG problem for the benefit to
unify the API.

Thanks.

Re: [PATCH] TTY:vt: convert remain take_over_console's users to do_take_over_console

[Pavel Machek]

On Thu 2013-06-06 09:23:13, Wang YanQing wrote:
> On Tue, Jun 04, 2013 at 10:13:18PM +0200, Pavel Machek wrote:
> > On Tue 2013-05-21 13:15:12, Wang YanQing wrote:
> > > Impact:
> > > 1:convert all remain take_over_console to do_take_over_console
> > 
> > This is step backwards.
> 
> What is step backwards? do_take_over_console appear MUCH MUCH later in kernel
> than take_over_console, do_take_over_console is the new API, I can't understand
> what is step backwards.

"do_*" is internal api, "*" is external api. You sprinkle internal api
all over the place.

> > > --- a/arch/alpha/kernel/console.c
> > > +++ b/arch/alpha/kernel/console.c
> > > @@ -61,7 +61,9 @@ locate_and_init_vga(void *(*sel_func)(void *, void *))
> > >  
> > >  	/* Set the VGA hose and init the new console. */
> > >  	pci_vga_hose = hose;
> > > -	take_over_console(&vga_con, 0, MAX_NR_CONSOLES-1, 1);
> > > +	console_lock();
> > > +	do_take_over_console(&vga_con, 0, MAX_NR_CONSOLES-1, 1);
> > > +	console_unlock();
> > >  }
> > 
> > Original was better.
> 
> Except reduce some console_lock/unlock scatter scattered in kernel, I 
> can't see the "BETTER", and it is not a BIG problem for the benefit to
> unify the API.

You replaced "calling exported function" with "calling
internal-sounding function and adding locking too".


									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] TTY:vt: convert remain take_over_console's users to do_take_over_console

[Wang YanQing]

On Thu, Jun 06, 2013 at 01:47:46PM +0200, Pavel Machek wrote:
> On Thu 2013-06-06 09:23:13, Wang YanQing wrote:
> > On Tue, Jun 04, 2013 at 10:13:18PM +0200, Pavel Machek wrote:
> > > On Tue 2013-05-21 13:15:12, Wang YanQing wrote:
> > > > Impact:
> > > > 1:convert all remain take_over_console to do_take_over_console
> > > 
> > > This is step backwards.
> > 
> > What is step backwards? do_take_over_console appear MUCH MUCH later in kernel
> > than take_over_console, do_take_over_console is the new API, I can't understand
> > what is step backwards.
> 
> "do_*" is internal api, "*" is external api. You sprinkle internal api
> all over the place.
> 

internal vs external? No, they only have one difference, callee vs caller hold
the console lock.

> > > > --- a/arch/alpha/kernel/console.c
> > > > +++ b/arch/alpha/kernel/console.c
> > > > @@ -61,7 +61,9 @@ locate_and_init_vga(void *(*sel_func)(void *, void *))
> > > >  
> > > >  	/* Set the VGA hose and init the new console. */
> > > >  	pci_vga_hose = hose;
> > > > -	take_over_console(&vga_con, 0, MAX_NR_CONSOLES-1, 1);
> > > > +	console_lock();
> > > > +	do_take_over_console(&vga_con, 0, MAX_NR_CONSOLES-1, 1);
> > > > +	console_unlock();
> > > >  }
> > > 
> > > Original was better.
> > 
> > Except reduce some console_lock/unlock scatter scattered in kernel, I 
> > can't see the "BETTER", and it is not a BIG problem for the benefit to
> > unify the API.
> 
> You replaced "calling exported function" with "calling
> internal-sounding function and adding locking too".

do_take_over_console is also exported by:

EXPORT_SYMBOL_GPL(do_take_over_console)