[PATCH] parport: daisy: do not try to load lowlevel driver

[PATCH] parport: daisy: do not try to load lowlevel driver

[Sudip Mukherjee]

Some distros like Suse has an alias for "parport_lowlevel" and that
alias points to "parport_pc". Now when the parport bus registers, it
also initialises the daisy driver as the daisy driver is needed to
check the port when the port is first found. Due to the new device
model daisy driver will now try to find the parallel ports while trying
to register its driver so that it can bind with them. Now, since daisy
driver is loaded while parport bus is initialising the list of parport
is still empty and it tries to load the lowlevel driver, which has an
alias set to parport_pc, now causes a deadlock.

We do not need to search for ports and bind the initial list of ports
to daisy driver as daisy driver is always the first driver to use the
new found parport and we know when the parport bus is registering the
list of parport will always be empty. So, proceed with the daisy_drv
registration even if the list of parport is empty.

Fixes: 1aec4211204d ("parport: daisy: use new parport device model")
Reported-by: Michal Kubecek <mkubecek@suse.cz>
Reported-by: Steven Rostedt <rostedt@goodmis.org>
Tested-by: Michal Kubecek <mkubecek@suse.cz>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
---
 drivers/parport/share.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/parport/share.c b/drivers/parport/share.c
index 0171b8dbcdcd..f87948fbfc34 100644
--- a/drivers/parport/share.c
+++ b/drivers/parport/share.c
@@ -274,7 +274,7 @@ static int port_check(struct device *dev, void *dev_drv)
 int __parport_register_driver(struct parport_driver *drv, struct module *owner,
 			      const char *mod_name)
 {
-	if (list_empty(&portlist))
+	if (list_empty(&portlist) && strcmp(drv->name, "daisy_drv"))
 		get_lowlevel_driver();
 
 	if (drv->devmodel) {
-- 
2.11.0

Re: [PATCH] parport: daisy: do not try to load lowlevel driver

[Linus Torvalds]

On Mon, Mar 25, 2019 at 2:13 PM Sudip Mukherjee
<sudipm.mukherjee@gmail.com> wrote:
>
> We do not need to search for ports and bind the initial list of ports
> to daisy driver as daisy driver is always the first driver to use the
> new found parport and we know when the parport bus is registering the
> list of parport will always be empty. So, proceed with the daisy_drv
> registration even if the list of parport is empty.

This is completely hacky and senseless.

The problem as far as I can tell is that the daisy driver shouldn't
register early at all, and shouldn't be a subsys initcall. It should
just be a driver initcall, and happen naturally after the parport
subsystem has been initialized.

This patch just makes the code even *less* understandable.

I'm going to revert that problematic commit.

If you can get it working without that incorrect and senseless tie-in
of the daisy driver registration with the regular partport init
sequence, we can revisit this.

                      Linus

Re: [PATCH] parport: daisy: do not try to load lowlevel driver

[Steven Rostedt]

On Mon, 25 Mar 2019 21:13:25 +0000
Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote:

> Some distros like Suse has an alias for "parport_lowlevel" and that
> alias points to "parport_pc". Now when the parport bus registers, it
> also initialises the daisy driver as the daisy driver is needed to
> check the port when the port is first found. Due to the new device
> model daisy driver will now try to find the parallel ports while trying
> to register its driver so that it can bind with them. Now, since daisy
> driver is loaded while parport bus is initialising the list of parport
> is still empty and it tries to load the lowlevel driver, which has an
> alias set to parport_pc, now causes a deadlock.
> 
> We do not need to search for ports and bind the initial list of ports
> to daisy driver as daisy driver is always the first driver to use the
> new found parport and we know when the parport bus is registering the
> list of parport will always be empty. So, proceed with the daisy_drv
> registration even if the list of parport is empty.
> 
> Fixes: 1aec4211204d ("parport: daisy: use new parport device model")
> Reported-by: Michal Kubecek <mkubecek@suse.cz>
> Reported-by: Steven Rostedt <rostedt@goodmis.org>
> Tested-by: Michal Kubecek <mkubecek@suse.cz>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>

Tested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

Re: [PATCH] parport: daisy: do not try to load lowlevel driver

[Sudip Mukherjee]

Hi Linus,

On Mon, Mar 25, 2019 at 9:49 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Mar 25, 2019 at 2:13 PM Sudip Mukherjee
> <sudipm.mukherjee@gmail.com> wrote:
> >
> > We do not need to search for ports and bind the initial list of ports
> > to daisy driver as daisy driver is always the first driver to use the
> > new found parport and we know when the parport bus is registering the
> > list of parport will always be empty. So, proceed with the daisy_drv
> > registration even if the list of parport is empty.
>
> This is completely hacky and senseless.

I admit its hacky. :(

>
> The problem as far as I can tell is that the daisy driver shouldn't
> register early at all, and shouldn't be a subsys initcall. It should
> just be a driver initcall, and happen naturally after the parport
> subsystem has been initialized.

yes, but the daisy driver has to be the first one to register even
before any of the ports have actually registered and then it will try
to load the lowlevel driver. In x86, parport_pc will register the port
and then announce it. When the port is announced daisy_driver should
check that port and this happens even before the port is added to the
list. So, we will always be in this situation that when daisy driver
registers the port list is empty.

>
> This patch just makes the code even *less* understandable.
>
> I'm going to revert that problematic commit.
>
> If you can get it working without that incorrect and senseless tie-in
> of the daisy driver registration with the regular partport init
> sequence, we can revisit this.

I will find a better way to do this.


-- 
Regards
Sudip