[PATCH v2] parport: register driver later

[PATCH v2] parport: register driver later

[Sudip Mukherjee]

If the parport bus is not yet registered and any device using parallel
port tries to register with the bus we get a stackdump with a message
of Kernel bug.

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Cc: <stable@vger.kernel.org> # 4.2+
Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
---

Hi Ross,
Can you please test this patch in your setup. This is a respin of the
previous patch in another way.

 drivers/parport/share.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/parport/share.c b/drivers/parport/share.c
index 3308427..c1eba80 100644
--- a/drivers/parport/share.c
+++ b/drivers/parport/share.c
@@ -135,9 +135,18 @@ static struct bus_type parport_bus_type = {
 	.probe = parport_probe,
 };
 
+static bool is_registered;
+
 int parport_bus_init(void)
 {
-	return bus_register(&parport_bus_type);
+	int ret;
+
+	ret = bus_register(&parport_bus_type);
+	if (ret)
+		return ret;
+
+	is_registered = true;
+	return 0;
 }
 
 void parport_bus_exit(void)
@@ -273,6 +282,9 @@ int __parport_register_driver(struct parport_driver *drv, struct module *owner,
 		/* using device model */
 		int ret;
 
+		if (!is_registered)
+			return -EAGAIN;
+
 		/* initialize common driver fields */
 		drv->driver.name = drv->name;
 		drv->driver.bus = &parport_bus_type;
-- 
1.9.1

Re: [PATCH v2] parport: register driver later

[Ross Zwisler]

On Sun, Mar 06, 2016 at 08:40:10PM +0530, Sudip Mukherjee wrote:
> If the parport bus is not yet registered and any device using parallel
> port tries to register with the bus we get a stackdump with a message
> of Kernel bug.
> 
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> Cc: <stable@vger.kernel.org> # 4.2+
> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
> ---
> 
> Hi Ross,
> Can you please test this patch in your setup. This is a respin of the
> previous patch in another way.

Yep, this also solves the issue for me.

Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>

>  drivers/parport/share.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/parport/share.c b/drivers/parport/share.c
> index 3308427..c1eba80 100644
> --- a/drivers/parport/share.c
> +++ b/drivers/parport/share.c
> @@ -135,9 +135,18 @@ static struct bus_type parport_bus_type = {
>  	.probe = parport_probe,
>  };
>  
> +static bool is_registered;
> +
>  int parport_bus_init(void)
>  {
> -	return bus_register(&parport_bus_type);
> +	int ret;
> +
> +	ret = bus_register(&parport_bus_type);
> +	if (ret)
> +		return ret;
> +
> +	is_registered = true;
> +	return 0;
>  }
>  
>  void parport_bus_exit(void)
> @@ -273,6 +282,9 @@ int __parport_register_driver(struct parport_driver *drv, struct module *owner,
>  		/* using device model */
>  		int ret;
>  
> +		if (!is_registered)
> +			return -EAGAIN;
> +
>  		/* initialize common driver fields */
>  		drv->driver.name = drv->name;
>  		drv->driver.bus = &parport_bus_type;
> -- 
> 1.9.1

Re: [PATCH v2] parport: register driver later

[Sudip Mukherjee]

On Mon, Mar 07, 2016 at 10:32:55AM -0700, Ross Zwisler wrote:
> On Sun, Mar 06, 2016 at 08:40:10PM +0530, Sudip Mukherjee wrote:
> > If the parport bus is not yet registered and any device using parallel
> > port tries to register with the bus we get a stackdump with a message
> > of Kernel bug.
> > 
> > Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> > Cc: <stable@vger.kernel.org> # 4.2+
> > Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
> > ---
> > 
> > Hi Ross,
> > Can you please test this patch in your setup. This is a respin of the
> > previous patch in another way.
> 
> Yep, this also solves the issue for me.
> 
> Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>

Hi Greg,
If this patch is ok, can we please have it in v4.6 .
Anyway, the problem patch which this patch tried to fix has already
been reverted by Linus - 
1701f680407c ("Revert "ppdev: use new parport device model"") but we still
can have problem with the other devices that use parport.

BTW, I know you are busy, but in these situations where I need to have
the fix urgently in the tree, is there any other way to solve the purpose?
I feel it was incompetency on my part where Linus had to interfere and
revert a patch even though the fix was already posted.

regards
sudip

Re: [PATCH v2] parport: register driver later

[Greg KH]

On Tue, Apr 05, 2016 at 06:26:08AM +0100, Sudip Mukherjee wrote:
> On Mon, Mar 07, 2016 at 10:32:55AM -0700, Ross Zwisler wrote:
> > On Sun, Mar 06, 2016 at 08:40:10PM +0530, Sudip Mukherjee wrote:
> > > If the parport bus is not yet registered and any device using parallel
> > > port tries to register with the bus we get a stackdump with a message
> > > of Kernel bug.
> > > 
> > > Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> > > Cc: <stable@vger.kernel.org> # 4.2+
> > > Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
> > > ---
> > > 
> > > Hi Ross,
> > > Can you please test this patch in your setup. This is a respin of the
> > > previous patch in another way.
> > 
> > Yep, this also solves the issue for me.
> > 
> > Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> 
> Hi Greg,
> If this patch is ok, can we please have it in v4.6 .
> Anyway, the problem patch which this patch tried to fix has already
> been reverted by Linus - 
> 1701f680407c ("Revert "ppdev: use new parport device model"") but we still
> can have problem with the other devices that use parport.
> 
> BTW, I know you are busy, but in these situations where I need to have
> the fix urgently in the tree, is there any other way to solve the purpose?
> I feel it was incompetency on my part where Linus had to interfere and
> revert a patch even though the fix was already posted.

A bit better commit message here would have caused me to notice it.
Something like "Revert a broken patch because it crashes all of our
machines without it!!!" would be a hint it needed to go in :)

I think the lack of parport hardware around seems to have caused a total
lack of testing this code path while it was in linux-next and in my
local testing, sorry about that, it should have been caught a lot
earlier.

thanks,

greg k-h

Re: [PATCH v2] parport: register driver later

[Sudip Mukherjee]

On Tuesday 05 April 2016 06:28 PM, Greg KH wrote:
> On Tue, Apr 05, 2016 at 06:26:08AM +0100, Sudip Mukherjee wrote:
>> On Mon, Mar 07, 2016 at 10:32:55AM -0700, Ross Zwisler wrote:
>>> On Sun, Mar 06, 2016 at 08:40:10PM +0530, Sudip Mukherjee wrote:
>>>> If the parport bus is not yet registered and any device using parallel
>>>> port tries to register with the bus we get a stackdump with a message
>>>> of Kernel bug.
>>>>
>>>> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
>>>> Cc: <stable@vger.kernel.org> # 4.2+
>>>> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
>>>> ---
>>>>
>>>> Hi Ross,
>>>> Can you please test this patch in your setup. This is a respin of the
>>>> previous patch in another way.
>>>
>>> Yep, this also solves the issue for me.
>>>
>>> Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>
>>
>> Hi Greg,
>> If this patch is ok, can we please have it in v4.6 .
>> Anyway, the problem patch which this patch tried to fix has already
>> been reverted by Linus -
>> 1701f680407c ("Revert "ppdev: use new parport device model"") but we still
>> can have problem with the other devices that use parport.
>>
>> BTW, I know you are busy, but in these situations where I need to have
>> the fix urgently in the tree, is there any other way to solve the purpose?
>> I feel it was incompetency on my part where Linus had to interfere and
>> revert a patch even though the fix was already posted.
>
> A bit better commit message here would have caused me to notice it.
> Something like "Revert a broken patch because it crashes all of our
> machines without it!!!" would be a hint it needed to go in :)

Well. its actually my fault. Previously I used to ping and remind you if 
there is something urgent which needed to go in before your tree closes. 
But this time I got busy with the job change and traveling and 
everything was a mess on my side.
So, now that the ppdev patch has been reverted by Linus, what do you 
suggest that we do? I will say, to have this patch as a temporary fix 
(for other devices) while I work on the deferred probe for parport which 
will solve the problem.

regards
sudip

Re: [PATCH v2] parport: register driver later

[Greg KH]

On Tue, Apr 05, 2016 at 06:47:36PM +0530, Sudip Mukherjee wrote:
> On Tuesday 05 April 2016 06:28 PM, Greg KH wrote:
> > On Tue, Apr 05, 2016 at 06:26:08AM +0100, Sudip Mukherjee wrote:
> > > On Mon, Mar 07, 2016 at 10:32:55AM -0700, Ross Zwisler wrote:
> > > > On Sun, Mar 06, 2016 at 08:40:10PM +0530, Sudip Mukherjee wrote:
> > > > > If the parport bus is not yet registered and any device using parallel
> > > > > port tries to register with the bus we get a stackdump with a message
> > > > > of Kernel bug.
> > > > > 
> > > > > Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> > > > > Cc: <stable@vger.kernel.org> # 4.2+
> > > > > Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
> > > > > ---
> > > > > 
> > > > > Hi Ross,
> > > > > Can you please test this patch in your setup. This is a respin of the
> > > > > previous patch in another way.
> > > > 
> > > > Yep, this also solves the issue for me.
> > > > 
> > > > Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > > 
> > > Hi Greg,
> > > If this patch is ok, can we please have it in v4.6 .
> > > Anyway, the problem patch which this patch tried to fix has already
> > > been reverted by Linus -
> > > 1701f680407c ("Revert "ppdev: use new parport device model"") but we still
> > > can have problem with the other devices that use parport.
> > > 
> > > BTW, I know you are busy, but in these situations where I need to have
> > > the fix urgently in the tree, is there any other way to solve the purpose?
> > > I feel it was incompetency on my part where Linus had to interfere and
> > > revert a patch even though the fix was already posted.
> > 
> > A bit better commit message here would have caused me to notice it.
> > Something like "Revert a broken patch because it crashes all of our
> > machines without it!!!" would be a hint it needed to go in :)
> 
> Well. its actually my fault. Previously I used to ping and remind you if
> there is something urgent which needed to go in before your tree closes. But
> this time I got busy with the job change and traveling and everything was a
> mess on my side.
> So, now that the ppdev patch has been reverted by Linus, what do you suggest
> that we do? I will say, to have this patch as a temporary fix (for other
> devices) while I work on the deferred probe for parport which will solve the
> problem.

Work on deferred probe, you should have time before the 4.7-rc1 merge
window happens.

thanks,

greg k-h

Re: [PATCH v2] parport: register driver later

[Linus Torvalds]

On Tue, Apr 5, 2016 at 5:58 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> A bit better commit message here would have caused me to notice it.
> Something like "Revert a broken patch because it crashes all of our
> machines without it!!!" would be a hint it needed to go in :)

Didn't my revert of e7223f1860 fix this?

> I think the lack of parport hardware around seems to have caused a total
> lack of testing this code path while it was in linux-next and in my
> local testing, sorry about that, it should have been caught a lot
> earlier.

Well, the original 0day kernel test robot report was from the
linux-next days, back in February. I reverted the commit that the
kernel test robot indicated was the point where the actual trouble
started two weeks ago (commit 1701f680407c).

I was hoping that would be it. Have there been reports since that I
haven't seen?

                Linus

Re: [PATCH v2] parport: register driver later

[Greg KH]

On Tue, Apr 05, 2016 at 06:44:12AM -0700, Linus Torvalds wrote:
> On Tue, Apr 5, 2016 at 5:58 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > A bit better commit message here would have caused me to notice it.
> > Something like "Revert a broken patch because it crashes all of our
> > machines without it!!!" would be a hint it needed to go in :)
> 
> Didn't my revert of e7223f1860 fix this?

Yes.

> > I think the lack of parport hardware around seems to have caused a total
> > lack of testing this code path while it was in linux-next and in my
> > local testing, sorry about that, it should have been caught a lot
> > earlier.
> 
> Well, the original 0day kernel test robot report was from the
> linux-next days, back in February. I reverted the commit that the
> kernel test robot indicated was the point where the actual trouble
> started two weeks ago (commit 1701f680407c).
> 
> I was hoping that would be it. Have there been reports since that I
> haven't seen?

Not that I have seen.  But I missed the 0day report so I might not be
the best judge here...

thanks,

greg k-h

Re: [PATCH v2] parport: register driver later

[Linus Torvalds]

On Tue, Apr 5, 2016 at 6:47 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>> I was hoping that would be it. Have there been reports since that I
>> haven't seen?
>
> Not that I have seen.

Ok. So as far as I can see we can ignore this for now (at at least be
no worse off than we used to be).

I didn't check *why* ppdev_init happens before the parport driver has
been initialized,

I get the feeling that the trivial fix would be to just make a new
"parport_init/exit()" pair that just does the parport_bus_init(). And
mark that as "subsys_initcall()" so that it gets done before the
individual drivers. No need to even be fancy about deferred probing
etc, just a simple "ppdev depends on parport" chain.

But I didn't look into the actual sequence of events, so it's entirely
possible I'm missing something.

                 Linus

Re: [PATCH v2] parport: register driver later

[Sudip Mukherjee]

On Mon, Apr 04, 2016 at 11:13:12PM -0700, Linus Torvalds wrote:
> On Mar 6, 2016 7:10 AM, "Sudip Mukherjee" <sudipm.mukherjee@gmail.com>
> wrote:
> 
> >
> > Hi Ross,
> > Can you please test this patch in your setup. This is a respin of the
> > previous patch in another way.
> 
> This seems to have the exact same problem as the previous solution: it
> avoids the oops by simply failing the parport registration.
> 
> Yes, that gets rid of the oops, but it also means the driver that tries to
> register with the parport subsystem doesn't actually get registered.
> 
> What am I missing? Why would that
> 
>    return -EAGAIN;
> 
> be OK, when it historically just worked?

Hi Linus,
It really is an honour that you have taken time to reply to my mail.

It historically used to work because it never used the device model. In
device model we will get a stackdump if some device tries to register
with a bus when the bus itself is not yet registered.

If pardev tried to register with parallel port before parallel port
has actually registered then parport code used to call
get_lowlevel_driver() and even after that if portlist was empty then
pardev used to get NULL when it tried parport_find_number() and it
used to print a warning about "no associated port" and exit. There was
no stackdump.

This fix is actually not ok. This is a temporary fix and the actual fix
should be to implement deffered probe. And I have mentioned that in
https://lkml.org/lkml/2016/3/4/306
"We should actually have some deferred probe here. But considering that
you will be closing your trees soon so a quick fix to solve the problem
for now. We will revisit this when we remove the old api (hopefully v4.7)."


regards
sudip