Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init

[Luis R. Rodriguez]

On Tue, Jul 29, 2014 at 04:14:22PM -0700, Greg KH wrote:
> On Mon, Jul 28, 2014 at 06:13:43PM -0700, Luis R. Rodriguez wrote:
> > >> > Why not just put the initial "register the device" in a single-shot
> > >> > workqueue or thread or something like that so that modprobe returns
> > >> > instantly back with a success and all is fine?
> > >>
> > >> That surely is possible but why not a general solution for such kludges?
> > >
> > > Because the driver should be fixed.  How hard would it be to do what I
> > > just suggested here, 20 lines added at most?
> > 
> > I appreciate the feedback, but don't look at me, I'd happy to go nutty
> > on ripping things apart from hairy drivers, however Chelsie folks
> > expressed concerns over major rework on the driver... so even if we
> > did have something I expect things to take a while to bake / gain
> > confidence from others.
> 
> "rework"?  Heh, here's a patch that adds 10 lines to the mptsas driver
> that should also work for any other driver as well.  Why not just do
> this instead?

That's not a rework, that's a kludge, doing something similar for other
drivers would be replicating kludges, the deferred probe use was trying
to generalize a kludge with 3 lines of code. But I'm no not yet convinced
its the best solution now.

> > This also just addresses this *one* Ethernet driver, there was that
> > SCSI driver that created the original report -- Canonical merged
> > Joseph's fix as a work around,
> 
> What fix was that?

https://launchpadlibrarian.net/169714201/kthread-Do-not-leave-kthread_create-immediately.patch

It was reviewed and Oleg preferred the timeout instead be reviewed
on systemd devel mailing list. That didn't go anywhere but today Hannes
posted a patch and that got merged. Its still not solving all issues
though as it lets us override the timeout value, systems / drivers
can still crash without a long timeout.

> > there is no general solution for this yet, and again with that work
> > around you won't find which drivers run into this issue.
> 
> Great, fix them as they are found, that's fine. 

Are we really OK in letting distributions have to deal with crashes
because of this new driver 30 second timeout ? I think warning about
it would be better and more friendlier, no? What gains do we have to
kill the damn thing?

> Again, don't add stuff
> to the driver core to paper over crappy drivers, I'm not going to take
> that type of change.  I _only_ took the defer binding code as there was
> no other way for an individual driver to deal with things if it's
> resources were not present yet due to binding order in the system.

Ok but its a bit unfair to force killing drivers over a new driver 30 second
timeout requirement for a change that was made implicitly through a series
of collateral changes.

> So, anyone care to test the patch below on a system that has this
> problem to let me know if it would work or not?  Odds are, this should
> be a workqueue, to make it cleaner, but a kthread is just so easy to
> use...
> 
> thanks,
> 
> greg k-h
> 
> diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
> index 711fcb5cec87..4fd4f36a2d9e 100644
> --- a/drivers/message/fusion/mptsas.c
> +++ b/drivers/message/fusion/mptsas.c
> @@ -51,6 +51,7 @@
>  #include <linux/jiffies.h>
>  #include <linux/workqueue.h>
>  #include <linux/delay.h>	/* for mdelay */
> +#include <linux/kthread.h>
>  
>  #include <scsi/scsi.h>
>  #include <scsi/scsi_cmnd.h>
> @@ -5393,8 +5394,7 @@ static struct pci_driver mptsas_driver = {
>  #endif
>  };
>  
> -static int __init
> -mptsas_init(void)
> +static int mptsas_real_init(void *data)
>  {
>  	int error;
>  
> @@ -5429,9 +5429,19 @@ mptsas_init(void)
>  	return error;
>  }
>  
> +static struct task_struct *init_thread;
> +
> +static int __init
> +mptsas_init(void)
> +{
> +	init_thread = kthread_run(mptsas_real_init, NULL, "mptsas_init");
> +	return 0;
> +}
> +
>  static void __exit
>  mptsas_exit(void)
>  {
> +	kthread_stop(init_thread);
>  	pci_unregister_driver(&mptsas_driver);
>  	sas_release_transport(mptsas_transport_template);

So we're OK to see these kludges sprinkled over the kernel instead of
genrealizing somethiing for them in the meantime?

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/