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

[Guenter Roeck <guenter.roeck@ericsson.com>]

On Wed, Apr 06, 2011 at 05:13:16PM -0400, Nat Gurumoorthy wrote:
> 03 - Adds changes to hwmon driver to use the new lock.
> 
> Signed-off-by: Nat Gurumoorthy <natg@google.com>
> 
Content looks ok, but two comments:

1) When sending a series of patches, send subsequent ones as reply to the first patch.
   "git send-email" does this for you, as does quilt.
2) The headline of each patch should describe what is done in that patch.
   The description above doesn't mean anything without knowing the context.

Also see Documentation/SubmittingPatches, item #2 and #15.

Thanks,
Guenter

> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 297bc9a..afe671a 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -436,6 +436,7 @@ config SENSORS_IBMPEX
>  config SENSORS_IT87
>  	tristate "ITE IT87xx and compatibles"
>  	select HWMON_VID
> +	select IT87_LOCK
>  	help
>  	  If you say yes here you get support for ITE IT8705F, IT8712F,
>  	  IT8716F, IT8718F, IT8720F, IT8721F, IT8726F and IT8758E sensor
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 316b648..bc32535 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -55,6 +55,8 @@
>  #include <linux/dmi.h>
>  #include <linux/acpi.h>
>  #include <linux/io.h>
> +#include <linux/spinlock.h>
> +#include <linux/it87_lock.h>
>  
>  #define DRVNAME "it87"
>  
> @@ -110,7 +112,9 @@ superio_select(int ldn)
>  
>  static inline void
>  superio_enter(void)
> +__acquires(&it87_io_lock)
>  {
> +	spin_lock(&it87_io_lock);
>  	outb(0x87, REG);
>  	outb(0x01, REG);
>  	outb(0x55, REG);
> @@ -119,9 +123,11 @@ superio_enter(void)
>  
>  static inline void
>  superio_exit(void)
> +__releases(&it87_io_lock)
>  {
>  	outb(0x02, REG);
>  	outb(0x02, VAL);
> +	spin_unlock(&it87_io_lock);
>  }
>  
>  /* Logical device 4 registers */
> @@ -1899,8 +1905,12 @@ static int __devexit it87_remove(struct platform_device *pdev)
>     would slow down the IT87 access and should not be necessary. */
>  static int it87_read_value(struct it87_data *data, u8 reg)
>  {
> +	int ret;
> +	spin_lock(&it87_io_lock);
>  	outb_p(reg, data->addr + IT87_ADDR_REG_OFFSET);
> -	return inb_p(data->addr + IT87_DATA_REG_OFFSET);
> +	ret = inb_p(data->addr + IT87_DATA_REG_OFFSET);
> +	spin_unlock(&it87_io_lock);
> +	return ret;
>  }
>  
>  /* Must be called with data->update_lock held, except during initialization.
> @@ -1908,8 +1918,10 @@ static int it87_read_value(struct it87_data *data, u8 reg)
>     would slow down the IT87 access and should not be necessary. */
>  static void it87_write_value(struct it87_data *data, u8 reg, u8 value)
>  {
> +	spin_lock(&it87_io_lock);
>  	outb_p(reg, data->addr + IT87_ADDR_REG_OFFSET);
>  	outb_p(value, data->addr + IT87_DATA_REG_OFFSET);
> +	spin_unlock(&it87_io_lock);
>  }
>  
>  /* Return 1 if and only if the PWM interface is safe to use */
> -- 
>