Re: [PATCH] Make all it87 drivers SMP safe.

[Natarajan Gurumoorthy <natg@google.com>]

Guenter,
      Thank you for spotting the fact the everything goes south if you
disable "watchdog". I am working on a solution. Looks like the ideal
place to store it87_io_lock.c will be drivers/misc and the IT87_LOCK
config will be placed before the MISC_DEVICES entry in
drivers/misc/Kconfig file. This will be similar to the
SENSORS_LIS3LV02D entry in that Kconfig file.

      Now going back to the partitioning do I send this change out as
a multi patch set consisting of 4 parts something as below:

patch 0 has a description

patch 1 has only the lock and related files
        drivers/misc/Kconfig
        drivers/misc/Makefile
        include/linux/it87_lock.h
        drivers/misc/it87_lock.c

patch 2 has drivers/watchdog changes
        drivers/watchdog/Kconfig
        drivers/watchdog/it8712f_wdt.c
        drivers/watchdog/it87_wdt.c

patch 3 has drives/hwmon changes
        drivers/hwmon/Kconfig
        drivers/hwmon/it87.c

Regards
Nat

On Tue, Apr 5, 2011 at 5:43 PM, Guenter Roeck
<guenter.roeck@ericsson.com> wrote:
>
> On Tue, Apr 05, 2011 at 08:13:50PM -0400, Natarajan Gurumoorthy wrote:
> > Guenter,
> >       How would you partition it out? Are you suggesting that we do
> > the following:
> >
> > Patch1:
> >    drivers/hwmon/Kconfig          |    1 +
> >    drivers/hwmon/it87.c           |   14 ++++++++++++-
> >
> > Patch2:
> >    drivers/watchdog/Kconfig       |   12 +++++++++++
> >    drivers/watchdog/Makefile      |    1 +
> >    drivers/watchdog/it8712f_wdt.c |   10 ++++----
> >    drivers/watchdog/it87_lock.c   |   27 +++++++++++++++++++++++++
> >    drivers/watchdog/it87_wdt.c    |   42 ++++++---------------------------------
> >
> > Patch3:
> >    include/linux/it87_lock.h      |   28 ++++++++++++++++++++++++++
> >
> No, not really. The include file is part of the locking code, and the sequence is wrong.
>
> I personally would introduce the lock in the 1st patch.
> This would affect
>        drivers/watchdog/it87_lock.c
>        include/linux/it87_lock.h
>        drivers/watchdog/Makefile
>        drivers/watchdog/Kconfig
>
> The second patch would update the watchdog driver, affecting
>        drivers/watchdog/Kconfig
>        drivers/watchdog/it8712f_wdt.c
>
> and the last patch would update the hwmon driver.
>        drivers/hwmon/Kconfig
>        drivers/hwmon/it87.c
>
> Others may argue that patch 1 and 2 (introducing the lock and updating
> the watchdog driver) should be in a single patch, since the lock alone
> does not do anything without being used. This is a matter of opinion
> and really depends on the maintainer of the watchdog subsystem.
>
> Note that your patch has practical problems. If I disable WATCHDOG but enable
> the IT87 hwmon driver, I get:
>
> warning: (SENSORS_IT87) selects IT87_LOCK which has unmet direct dependencies (WATCHDOG)
>
> during configuration, and undefined references to it87_io_lock when linking.
> So it looks like you might want to consider moving the locking code to a location
> outside the watchdog code.
>
> Thanks,
> Guenter



--
Regards
Nat Gurumoorthy AB6SJ