[PATCH v2 0/3] Make all it87 drivers SMP safe.

[PATCH v2 0/3] Make all it87 drivers SMP safe.

[Nat Gurumoorthy]

There are 3 different drivers that touch the it87 hardware registers.
The 3 drivers have been written independently and access the it87 hardware
registers assuming they are the only driver accessing it. This change
attempts to serialize access to the hardware by defining a global spinlock
it87_io_lock in a file it87_lock.c. This lock has to be acquired by each
of the it87 drivers before it can access the hardware. We have defined
a new Kconfig option IT87_LOCK. When it is selected it87_lock.c is compiled
into the kernel thereby making the lock global and accessable to the it87
drivers which are typically built as loadable modules. All the it87 drivers
select IT87_LOCK to compile the lock into the kernel.
The routines accessing the hardware are being called from module init,
open, ioctl and module exit routines and hence it is sufficient to use
calls to spin_lock and spin_unlock to acquire and release the locks. For
the same reasons it87_wdt.c has extensive changes to remove calls to
spin_lock_irqsave and spin_unlock_irqrestore. The lock is now acquired
in superio_enter and released in superio_exit. This is now identical
to the code in drivers/hwmon/it87.c and drivers/watchdog/it8712f_wdt.c.
Added __acquire and __release annotations wherever needed.

01 - Adds the relevant lock files.
 drivers/watchdog/Kconfig
 drivers/watchdog/Makefile
 drivers/watchdog/it87_lock.c
 include/linux/it87_lock.h

02 - Adds changes to watchdog drivers to use the new lock.
 drivers/watchdog/it8712f_wdt.c
 drivers/watchdog/it87_wdt.c

03 - Adds changes to hwmon driver to use the new lock.
 drivers/hwmon/Kconfig
 drivers/hwmon/it87.c


 drivers/hwmon/Kconfig          |    1 +
 drivers/hwmon/it87.c           |   14 ++++++++++++-
 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 ++++++---------------------------------
 include/linux/it87_lock.h      |   28 ++++++++++++++++++++++++++
 8 files changed, 96 insertions(+), 41 deletions(-)
 create mode 100644 drivers/watchdog/it87_lock.c
 create mode 100644 include/linux/it87_lock.h

Signed-off-by: Nat Gurumoorthy <natg@google.com>

Re: [PATCH v2 0/3] Make all it87 drivers SMP safe.

[Guenter Roeck]

On Thu, Apr 07, 2011 at 05:26:00PM -0400, Nat Gurumoorthy wrote:
> There are 3 different drivers that touch the it87 hardware registers.
> The 3 drivers have been written independently and access the it87 hardware
> registers assuming they are the only driver accessing it. This change
> attempts to serialize access to the hardware by defining a global spinlock
> it87_io_lock in a file it87_lock.c. This lock has to be acquired by each
> of the it87 drivers before it can access the hardware. We have defined
> a new Kconfig option IT87_LOCK. When it is selected it87_lock.c is compiled
> into the kernel thereby making the lock global and accessable to the it87
> drivers which are typically built as loadable modules. All the it87 drivers
> select IT87_LOCK to compile the lock into the kernel.
> The routines accessing the hardware are being called from module init,
> open, ioctl and module exit routines and hence it is sufficient to use
> calls to spin_lock and spin_unlock to acquire and release the locks. For
> the same reasons it87_wdt.c has extensive changes to remove calls to
> spin_lock_irqsave and spin_unlock_irqrestore. The lock is now acquired
> in superio_enter and released in superio_exit. This is now identical
> to the code in drivers/hwmon/it87.c and drivers/watchdog/it8712f_wdt.c.
> Added __acquire and __release annotations wherever needed.
> 
> 01 - Adds the relevant lock files.
>  drivers/watchdog/Kconfig
>  drivers/watchdog/Makefile
>  drivers/watchdog/it87_lock.c
>  include/linux/it87_lock.h
> 
Did you consider naming this file include/linux/it87.h as suggested ?
I thought that was a goodd idea.

When you send out new versions of your patch set, it would be prudent
to list the patch version, as well as the changes made compared to previous
versions of your patch. See Documents/SubmittingPatches, rule #2.

Thanks,
Guenter

Re: [PATCH v2 0/3] Make all it87 drivers SMP safe.

[Natarajan Gurumoorthy]

Guenter,
      Comments are below

On Thu, Apr 7, 2011 at 5:37 PM, Guenter Roeck
<guenter.roeck@ericsson.com> wrote:
>
> Did you consider naming this file include/linux/it87.h as suggested ?
> I thought that was a goodd idea.
>
This does seem to be a good idea. I had some other thoughts about
where to place the
it87_lock.c file. One thought was to move the lock into the
drivers/mfd directory and
completely decouple the lock from the watchdog or the hwmon
directories. The mfd/Kconfig
would contain the IT87_LOCK.

> When you send out new versions of your patch set, it would be prudent
> to list the patch version, as well as the changes made compared to previous
I am new at this. Exactly where do you list the patch version. I put
v2 in the subject line.
The only difference between the 2 patch sets was that each of the patches has a
more verbose body section explaining the contents of the patch and
each of the sub patch
Subject line reflected what was happening in that sub patch. I also
made sure I had the
"In-Reply-To" entry in the patches.

Where in the patch do you discuss the changes made with respect to the
previous patch.

When I send out the next set of patches do I have to send out the
entire patch set marked as v3.

> versions of your patch. See Documents/SubmittingPatches, rule #2.
>
> Thanks,
> Guenter



--
Regards
Nat Gurumoorthy AB6SJ

Re: [PATCH v2 0/3] Make all it87 drivers SMP safe.

[Guenter Roeck]

On Fri, Apr 08, 2011 at 12:58:05AM -0400, Natarajan Gurumoorthy wrote:
> Guenter,
>       Comments are below
> 
> On Thu, Apr 7, 2011 at 5:37 PM, Guenter Roeck
> <guenter.roeck@ericsson.com> wrote:
> >
> > Did you consider naming this file include/linux/it87.h as suggested ?
> > I thought that was a goodd idea.
> >
> This does seem to be a good idea. I had some other thoughts about
> where to place the
> it87_lock.c file. One thought was to move the lock into the
> drivers/mfd directory and
> completely decouple the lock from the watchdog or the hwmon
> directories. The mfd/Kconfig
> would contain the IT87_LOCK.
> 
Might make sense, but I would suggest to solicit feedback from others before
you do that. Specifically you'll need some guidance from the watchdog maintainer
and possibly from the mfd maintainer if your current solution is acceptable
or if the mfd solution would be better (ie more likely to be accepted).

If you move the lock handling to mfd, you probably want to name the file it87.c,
or maybe it87-core.c, to ensure that other functionality can be added to it
without having to rename the file.

> > When you send out new versions of your patch set, it would be prudent
> > to list the patch version, as well as the changes made compared to previous
> I am new at this. Exactly where do you list the patch version. I put
> v2 in the subject line.

That was ok.

> The only difference between the 2 patch sets was that each of the patches has a
> more verbose body section explaining the contents of the patch and
> each of the sub patch
> Subject line reflected what was happening in that sub patch. I also
> made sure I had the
> "In-Reply-To" entry in the patches.
> 
This somehow didn't work. If you look at the kernel.org mailing list, it
did not recognize your series of patches as a single thread, but lists
it as independent posts.

You might want to play with that by sending series of patches to yourself 
and check if your mailer recognizes it as one thread.

> Where in the patch do you discuss the changes made with respect to the
> previous patch.
> 
See SubmittingPatches, #15. Add version information (ie what changed in which version)
after the "---" marker line, such as 

---
v2: Changed subject line to be meaningful, added description

One hint - the first comment line in each of your patches is a bit odd and should
not really be there. Keep in mind that everything up to the "---" line will end up
in the commit log unless the maintainer takes it out. The rest of the explanations
looked ok to me.

> When I send out the next set of patches do I have to send out the
> entire patch set marked as v3.
> 
If you change the header file name and/or location, each of the patches will be affected,
so each of the patches should get a new version number. If you don't change anything,
don't resubmit (yet).

Thanks,
Guenter