rtc-cmos has two drivers, one PNP and one platform. When PNP has not
succeeded probing, platform is registered. However, it tries to
unregister both drivers unconditionally, instead of only unregistering
those that were successfully registered. Fix that with a boolean
variable for each driver indicating whether registering was successful.
Signed-off-by: Thadeu Lima de Souza Cascardo <[email protected]>
---
drivers/rtc/rtc-cmos.c | 23 ++++++++++++++++++-----
1 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 23e10b6..f7a4701 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -1174,23 +1174,34 @@ static struct platform_driver cmos_platform_driver = {
}
};
+#ifdef CONFIG_PNP
+static bool pnp_driver_registered;
+#endif
+static bool platform_driver_registered;
+
static int __init cmos_init(void)
{
int retval = 0;
#ifdef CONFIG_PNP
- pnp_register_driver(&cmos_pnp_driver);
+ retval = pnp_register_driver(&cmos_pnp_driver);
+ if (retval == 0)
+ pnp_driver_registered = true;
#endif
- if (!cmos_rtc.dev)
+ if (!cmos_rtc.dev) {
retval = platform_driver_probe(&cmos_platform_driver,
cmos_platform_probe);
+ if (retval == 0)
+ platform_driver_registered = true;
+ }
if (retval == 0)
return 0;
#ifdef CONFIG_PNP
- pnp_unregister_driver(&cmos_pnp_driver);
+ if (pnp_driver_registered)
+ pnp_unregister_driver(&cmos_pnp_driver);
#endif
return retval;
}
@@ -1199,9 +1210,11 @@ module_init(cmos_init);
static void __exit cmos_exit(void)
{
#ifdef CONFIG_PNP
- pnp_unregister_driver(&cmos_pnp_driver);
+ if (pnp_driver_registered)
+ pnp_unregister_driver(&cmos_pnp_driver);
#endif
- platform_driver_unregister(&cmos_platform_driver);
+ if (platform_driver_registered)
+ platform_driver_unregister(&cmos_platform_driver);
}
module_exit(cmos_exit);
--
1.6.3
On Sun, 5 Jul 2009 01:07:59 -0300
Thadeu Lima de Souza Cascardo <[email protected]> wrote:
> rtc-cmos has two drivers, one PNP and one platform. When PNP has not
> succeeded probing, platform is registered. However, it tries to
> unregister both drivers unconditionally, instead of only unregistering
> those that were successfully registered. Fix that with a boolean
> variable for each driver indicating whether registering was successful.
OK, thanks.
This came up a few weeks ago - the kernel was actually crashing deep
down in the driver core, when a not-registered device was unregistered.
I believe Kay was planning on making the driver core more robust, so
that crash shouldn't be happening any more. Kay, can you please confirm
that thsi got fixed?
But we still shouldn't be unregistering a not-registered driver, and
the patch looks to be a suitable way of preventing that.
Alternatively, we could declare that unregistering a not-registered
driver is a permissible thing for a subsytem to do. It doesn't sound
like a good idea though - we'd lose runtime checking opportunities.
drivers/rtc/rtc-cmos.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff -puN drivers/rtc/rtc-cmos.c~rtc-mark-if-rtc-cmos-drivers-were-successfully-registered drivers/rtc/rtc-cmos.c
--- a/drivers/rtc/rtc-cmos.c~rtc-mark-if-rtc-cmos-drivers-were-successfully-registered
+++ a/drivers/rtc/rtc-cmos.c
@@ -1174,23 +1174,34 @@ static struct platform_driver cmos_platf
}
};
+#ifdef CONFIG_PNP
+static bool pnp_driver_registered;
+#endif
+static bool platform_driver_registered;
+
static int __init cmos_init(void)
{
int retval = 0;
#ifdef CONFIG_PNP
- pnp_register_driver(&cmos_pnp_driver);
+ retval = pnp_register_driver(&cmos_pnp_driver);
+ if (retval == 0)
+ pnp_driver_registered = true;
#endif
- if (!cmos_rtc.dev)
+ if (!cmos_rtc.dev) {
retval = platform_driver_probe(&cmos_platform_driver,
cmos_platform_probe);
+ if (retval == 0)
+ platform_driver_registered = true;
+ }
if (retval == 0)
return 0;
#ifdef CONFIG_PNP
- pnp_unregister_driver(&cmos_pnp_driver);
+ if (pnp_driver_registered)
+ pnp_unregister_driver(&cmos_pnp_driver);
#endif
return retval;
}
@@ -1199,9 +1210,11 @@ module_init(cmos_init);
static void __exit cmos_exit(void)
{
#ifdef CONFIG_PNP
- pnp_unregister_driver(&cmos_pnp_driver);
+ if (pnp_driver_registered)
+ pnp_unregister_driver(&cmos_pnp_driver);
#endif
- platform_driver_unregister(&cmos_platform_driver);
+ if (platform_driver_registered)
+ platform_driver_unregister(&cmos_platform_driver);
}
module_exit(cmos_exit);
_
On Mon, Jul 20, 2009 at 22:19, Andrew Morton<[email protected]> wrote:
> On Sun, 5 Jul 2009 01:07:59 -0300
> Thadeu Lima de Souza Cascardo <[email protected]> wrote:
>
>> rtc-cmos has two drivers, one PNP and one platform. When PNP has not
>> succeeded probing, platform is registered. However, it tries to
>> unregister both drivers unconditionally, instead of only unregistering
>> those that were successfully registered. Fix that with a boolean
>> variable for each driver indicating whether registering was successful.
> This came up a few weeks ago - the kernel was actually crashing deep
> down in the driver core, when a not-registered device was unregistered.
>
> I believe Kay was planning on making the driver core more robust, so
> that crash shouldn't be happening any more. Kay, can you please confirm
> that thsi got fixed?
This is supposed to fix it:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5c8563d773c0e9f0ac2a552e84806decd98ce732
Thanks,
Kay
On Mon, Jul 20, 2009 at 10:32:06PM +0200, Kay Sievers wrote:
> On Mon, Jul 20, 2009 at 22:19, Andrew Morton<[email protected]> wrote:
> > On Sun, 5 Jul 2009 01:07:59 -0300
> > Thadeu Lima de Souza Cascardo <[email protected]> wrote:
> >
> >> rtc-cmos has two drivers, one PNP and one platform. When PNP has not
> >> succeeded probing, platform is registered. However, it tries to
> >> unregister both drivers unconditionally, instead of only unregistering
> >> those that were successfully registered. Fix that with a boolean
> >> variable for each driver indicating whether registering was successful.
>
> > This came up a few weeks ago - the kernel was actually crashing deep
> > down in the driver core, when a not-registered device was unregistered.
> >
> > I believe Kay was planning on making the driver core more robust, so
> > that crash shouldn't be happening any more. Kay, can you please confirm
> > that thsi got fixed?
>
> This is supposed to fix it:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5c8563d773c0e9f0ac2a552e84806decd98ce732
>
> Thanks,
> Kay
Yes, but instead of an oops, we get a warning when the driver could just
behave properly. That's the intention of this patch: do not warn the
user when there's nothing to warn about, as long as the driver behaves
itself. Doing an rmmod on a unused module should not give the user what
looks like an oops. I should start being more verbose with my commit
messages. :-D
My best and thanks for you work,
Thadeu Cascardo.
On Mon, Jul 20, 2009 at 22:57, Thadeu Lima de Souza
Cascardo<[email protected]> wrote:
> On Mon, Jul 20, 2009 at 10:32:06PM +0200, Kay Sievers wrote:
>> On Mon, Jul 20, 2009 at 22:19, Andrew Morton<[email protected]> wrote:
>> > I believe Kay was planning on making the driver core more robust, so
>> > that crash shouldn't be happening any more. Kay, can you please confirm
>> > that thsi got fixed?
>>
>> This is supposed to fix it:
>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5c8563d773c0e9f0ac2a552e84806decd98ce732
> Yes, but instead of an oops, we get a warning when the driver could just
> behave properly. That's the intention of this patch: do not warn the
> user when there's nothing to warn about, as long as the driver behaves
> itself.
Sure, it was not supposed to fix the rtc issue properly, it's just to
make it not oops. :)
Thanks,
Kay