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

Luis R. Rodriguez
2014-07-30T00:05:49Z

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