Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754425AbaG3AFt (ORCPT ); Tue, 29 Jul 2014 20:05:49 -0400 Received: from cantor2.suse.de ([195.135.220.15]:55128 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750702AbaG3AFr (ORCPT ); Tue, 29 Jul 2014 20:05:47 -0400 Date: Wed, 30 Jul 2014 02:05:41 +0200 From: "Luis R. Rodriguez" To: Greg KH Cc: Santosh Rastapur , Hariprasad S , Takashi Iwai , "linux-kernel@vger.kernel.org" , Tetsuo Handa , Joseph Salisbury , Kay Sievers , One Thousand Gnomes , Tim Gardner , Pierre Fersing , Andrew Morton , Oleg Nesterov , Benjamin Poirier , Nagalakshmi Nandigama , Praveen Krishnamoorthy , Sreekanth Reddy , Abhijit Mahajan , MPT-FusionLinux.pdl@avagotech.com, Linux SCSI List , "netdev@vger.kernel.org" Subject: Re: [PATCH v2 2/4] driver core: enable drivers to use deferred probe from init Message-ID: <20140730000541.GR21930@wotan.suse.de> References: <1406572110-26823-1-git-send-email-mcgrof@do-not-panic.com> <1406572110-26823-3-git-send-email-mcgrof@do-not-panic.com> <20140728185538.GB17609@kroah.com> <20140728234644.GA18729@kroah.com> <20140729003501.GA17020@kroah.com> <20140729231422.GA14494@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140729231422.GA14494@kroah.com> User-Agent: Mutt/1.5.17 (2007-11-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > #include > #include /* for mdelay */ > +#include > > #include > #include > @@ -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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/