From: Guenter Roeck <guenter.roeck@ericsson.com>
To: Natarajan Gurumoorthy <natg@google.com>
Cc: Jean Delvare <khali@linux-fr.org>,
Wim Van Sebroeck <wim@iguana.be>,
Mike Waychison <mikew@google.com>,
"lm-sensors@lm-sensors.org" <lm-sensors@lm-sensors.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-watchdog@vger.kernel.org" <linux-watchdog@vger.kernel.org>
Subject: Re: [PATCH v6 1/2] Use "request_muxed_region" in it87 watchdog drivers
Date: Thu, 14 Apr 2011 19:40:05 -0700 [thread overview]
Message-ID: <20110415024005.GA21642@ericsson.com> (raw)
In-Reply-To: <BANLkTin6M0R4d+HMovbRwqthSHowUEKFow@mail.gmail.com>
On Thu, Apr 14, 2011 at 09:18:57PM -0400, Natarajan Gurumoorthy wrote:
> On Thu, Apr 14, 2011 at 3:49 PM, Guenter Roeck
> <guenter.roeck@ericsson.com> wrote:
> > This is way too complicated. Just return an error if
> > request_muxed_region fails, like all other callers of
> > request_muxed_region do.
> >
> Guenter,
> This maybe a little complicated but it is a solution that will
> correctly deal with the following scenario:
> Hardware: it8712f
> cpu: Multicore
> kernel: smp
> Modules being loaded during initialization: it8712f_wtd and w83697hf
>
> The problem is one of the cores is halfway through initializing
> it8712f_wdt and another core is just starting to run the
> "w83697hf_check_wdt" which is going to call "request_region" to
> reserve 0x2e 0x2f region. Just after that call on the other core
> it8712f_wdt_init calls it8712f_wdt_disable which calls superio_enter
> which will call "request_muxed_region" for the exact same region. This
> is guaranteed to fail because as of now w83697hf is one of the many
> non compliant drivers who are calling "request_region". The w83697hf
> will do the probe and conclude there is no w83697hf chip in the system
> and abort rest of the driver initialization. Meanwhile the it8712f_wtd
> driver initialization will fail. This will be one of those
> intermittent failures that make most kernel device driver writers
> prematurely bald :-).
>
> The way I have reworked the driver will survive the above scenario.
>
Unless I am missing something, the problem is that there is another driver
calling request_region() instead of request_muxed_region().
w83697hf_check_wdt() (and other drivers doing the same) should call
request_muxed_region(). Otherwise, the same problem could happen with the
caller(s) of request_region() - those calls could fail as well.
Plus, other callers of request_muxed_reason() not implementing
your retry code could end up with the same problem, ie with spurious failures.
I think we will have to find a solution which does not require retries
when calling request_muxed_region(). On the other side, others know
this code much better than I do, so maybe someone has a better solution.
Thanks,
Guenter
WARNING: multiple messages have this Message-ID (diff)
From: Guenter Roeck <guenter.roeck@ericsson.com>
To: Natarajan Gurumoorthy <natg@google.com>
Cc: Jean Delvare <khali@linux-fr.org>,
Wim Van Sebroeck <wim@iguana.be>,
Mike Waychison <mikew@google.com>,
"lm-sensors@lm-sensors.org" <lm-sensors@lm-sensors.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-watchdog@vger.kernel.org" <linux-watchdog@vger.kernel.org>
Subject: Re: [lm-sensors] [PATCH v6 1/2] Use "request_muxed_region" in it87
Date: Fri, 15 Apr 2011 02:40:05 +0000 [thread overview]
Message-ID: <20110415024005.GA21642@ericsson.com> (raw)
In-Reply-To: <BANLkTin6M0R4d+HMovbRwqthSHowUEKFow@mail.gmail.com>
On Thu, Apr 14, 2011 at 09:18:57PM -0400, Natarajan Gurumoorthy wrote:
> On Thu, Apr 14, 2011 at 3:49 PM, Guenter Roeck
> <guenter.roeck@ericsson.com> wrote:
> > This is way too complicated. Just return an error if
> > request_muxed_region fails, like all other callers of
> > request_muxed_region do.
> >
> Guenter,
> This maybe a little complicated but it is a solution that will
> correctly deal with the following scenario:
> Hardware: it8712f
> cpu: Multicore
> kernel: smp
> Modules being loaded during initialization: it8712f_wtd and w83697hf
>
> The problem is one of the cores is halfway through initializing
> it8712f_wdt and another core is just starting to run the
> "w83697hf_check_wdt" which is going to call "request_region" to
> reserve 0x2e 0x2f region. Just after that call on the other core
> it8712f_wdt_init calls it8712f_wdt_disable which calls superio_enter
> which will call "request_muxed_region" for the exact same region. This
> is guaranteed to fail because as of now w83697hf is one of the many
> non compliant drivers who are calling "request_region". The w83697hf
> will do the probe and conclude there is no w83697hf chip in the system
> and abort rest of the driver initialization. Meanwhile the it8712f_wtd
> driver initialization will fail. This will be one of those
> intermittent failures that make most kernel device driver writers
> prematurely bald :-).
>
> The way I have reworked the driver will survive the above scenario.
>
Unless I am missing something, the problem is that there is another driver
calling request_region() instead of request_muxed_region().
w83697hf_check_wdt() (and other drivers doing the same) should call
request_muxed_region(). Otherwise, the same problem could happen with the
caller(s) of request_region() - those calls could fail as well.
Plus, other callers of request_muxed_reason() not implementing
your retry code could end up with the same problem, ie with spurious failures.
I think we will have to find a solution which does not require retries
when calling request_muxed_region(). On the other side, others know
this code much better than I do, so maybe someone has a better solution.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
next prev parent reply other threads:[~2011-04-15 2:40 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-14 21:36 [PATCH v6 0/2] Make all it87 drivers SMP safe Nat Gurumoorthy
2011-04-14 21:36 ` [lm-sensors] " Nat Gurumoorthy
2011-04-14 21:37 ` [PATCH v6 1/2] Use "request_muxed_region" in it87 watchdog drivers Nat Gurumoorthy
2011-04-14 21:37 ` [lm-sensors] [PATCH v6 1/2] Use "request_muxed_region" in it87 Nat Gurumoorthy
2011-04-14 22:49 ` [PATCH v6 1/2] Use "request_muxed_region" in it87 watchdog drivers Guenter Roeck
2011-04-14 22:49 ` [lm-sensors] [PATCH v6 1/2] Use "request_muxed_region" in it87 Guenter Roeck
2011-04-15 1:18 ` [PATCH v6 1/2] Use "request_muxed_region" in it87 watchdog drivers Natarajan Gurumoorthy
2011-04-15 1:18 ` [lm-sensors] [PATCH v6 1/2] Use "request_muxed_region" in it87 Natarajan Gurumoorthy
2011-04-15 2:40 ` Guenter Roeck [this message]
2011-04-15 2:40 ` Guenter Roeck
2011-04-15 3:14 ` [PATCH v6 1/2] Use "request_muxed_region" in it87 watchdog drivers Natarajan Gurumoorthy
2011-04-15 3:14 ` [lm-sensors] [PATCH v6 1/2] Use "request_muxed_region" in it87 Natarajan Gurumoorthy
2011-04-15 5:58 ` [PATCH v6 1/2] Use "request_muxed_region" in it87 watchdog drivers Guenter Roeck
2011-04-15 5:58 ` [lm-sensors] [PATCH v6 1/2] Use "request_muxed_region" in it87 Guenter Roeck
2011-04-15 8:46 ` [PATCH v6 1/2] Use "request_muxed_region" in it87 watchdog drivers Natarajan Gurumoorthy
2011-04-15 8:46 ` [lm-sensors] [PATCH v6 1/2] Use "request_muxed_region" in it87 Natarajan Gurumoorthy
2011-04-15 11:05 ` [PATCH v6 1/2] Use "request_muxed_region" in it87 watchdog drivers Guenter Roeck
2011-04-15 11:05 ` [lm-sensors] [PATCH v6 1/2] Use "request_muxed_region" in it87 Guenter Roeck
2011-04-18 7:33 ` [lm-sensors] [PATCH v6 1/2] Use "request_muxed_region" in it87 watchdog drivers Hans de Goede
2011-04-18 7:33 ` [lm-sensors] [PATCH v6 1/2] Use "request_muxed_region" in it87 Hans de Goede
2011-04-14 21:38 ` [PATCH v6 2/2] Use "request_muxed_region" in it87 hwmon drivers Nat Gurumoorthy
2011-04-14 21:38 ` [lm-sensors] [PATCH v6 2/2] Use "request_muxed_region" in it87 Nat Gurumoorthy
2011-04-15 21:07 ` [PATCH v6 0/2] Make all it87 drivers SMP safe Jarod Wilson
2011-04-15 21:07 ` [lm-sensors] " Jarod Wilson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20110415024005.GA21642@ericsson.com \
--to=guenter.roeck@ericsson.com \
--cc=khali@linux-fr.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=lm-sensors@lm-sensors.org \
--cc=mikew@google.com \
--cc=natg@google.com \
--cc=wim@iguana.be \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.