Re: [PATCH 1/4] hwmon: (f71805f) Use request_muxed_region for Super-IO accesses

Re: [PATCH 1/4] hwmon: (f71805f) Use request_muxed_region for Super-IO accesses

[John Garry]

On 04/04/2019 19:33, Guenter Roeck wrote:
> Super-IO accesses may fail on a system with no or unmapped LPC bus.
>
> Unable to handle kernel paging request at virtual address ffffffbffee0002e
> pgd = ffffffc1d68d4000
> [ffffffbffee0002e] *pgd=0000000000000000, *pud=0000000000000000
> Internal error: Oops: 94000046 [#1] PREEMPT SMP
> Modules linked in: f71805f(+) hwmon
> CPU: 3 PID: 1659 Comm: insmod Not tainted 4.5.0+ #88
> Hardware name: linux,dummy-virt (DT)
> task: ffffffc1f6665400 ti: ffffffc1d6418000 task.ti: ffffffc1d6418000
> PC is at f71805f_find+0x6c/0x358 [f71805f]
>
> Also, other drivers may attempt to access the LPC bus at the same time,
> resulting in undefined behavior.
>
> Use request_muxed_region() to ensure that IO access on the requested
> address space is supported, and to ensure that access by multiple
> drivers is synchronized.
>
> Fixes: e53004e20a58e ("hwmon: New f71805f driver")
> Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> Reported-by: John Garry <john.garry@huawei.com>
> Cc: John Garry <john.garry@huawei.com>

Acked-by: John Garry <john.garry@huawei.com>

> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/f71805f.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwmon/f71805f.c b/drivers/hwmon/f71805f.c
> index 73c681162653..623736d2a7c1 100644
> --- a/drivers/hwmon/f71805f.c
> +++ b/drivers/hwmon/f71805f.c
> @@ -96,17 +96,23 @@ superio_select(int base, int ld)
>  	outb(ld, base + 1);
>  }
>
> -static inline void
> +static inline int
>  superio_enter(int base)
>  {
> +	if (!request_muxed_region(base, 2, DRVNAME))
> +		return -EBUSY;
> +
>  	outb(0x87, base);
>  	outb(0x87, base);
> +
> +	return 0;
>  }
>
>  static inline void
>  superio_exit(int base)
>  {
>  	outb(0xaa, base);
> +	release_region(base, 2);
>  }
>
>  /*
> @@ -1561,7 +1567,7 @@ static int __init f71805f_device_add(unsigned short address,
>  static int __init f71805f_find(int sioaddr, unsigned short *address,
>  			       struct f71805f_sio_data *sio_data)
>  {
> -	int err = -ENODEV;
> +	int err;
>  	u16 devid;
>
>  	static const char * const names[] = {
> @@ -1569,8 +1575,11 @@ static int __init f71805f_find(int sioaddr, unsigned short *address,
>  		"F71872F/FG or F71806F/FG",
>  	};
>
> -	superio_enter(sioaddr);
> +	err = superio_enter(sioaddr);
> +	if (err)
> +		return err;
>
> +	err = -ENODEV;
>  	devid = superio_inw(sioaddr, SIO_REG_MANID);
>  	if (devid != SIO_FINTEK_ID)
>  		goto exit;
>

Re: [PATCH 2/4] hwmon: (pc87427) Use request_muxed_region for Super-IO accesses

[John Garry]

On 04/04/2019 19:33, Guenter Roeck wrote:
> Super-IO accesses may fail on a system with no or unmapped LPC bus.
>
> Also, other drivers may attempt to access the LPC bus at the same time,
> resulting in undefined behavior.
>
> Use request_muxed_region() to ensure that IO access on the requested
> address space is supported, and to ensure that access by multiple drivers
> is synchronized.
>
> Fixes: ba224e2c4f0a7 ("hwmon: New PC87427 hardware monitoring driver")
> Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> Reported-by: John Garry <john.garry@huawei.com>
> Cc: John Garry <john.garry@huawei.com>

Acked-by: John Garry <john.garry@huawei.com>

> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/pc87427.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/pc87427.c b/drivers/hwmon/pc87427.c
> index d1a3f2040c00..58eee8fa3e6d 100644
> --- a/drivers/hwmon/pc87427.c
> +++ b/drivers/hwmon/pc87427.c
> @@ -106,6 +106,13 @@ static const char *logdev_str[2] = { DRVNAME " FMC", DRVNAME " HMC" };
>  #define LD_IN		1
>  #define LD_TEMP		1
>
> +static inline int superio_enter(int sioaddr)
> +{
> +	if (!request_muxed_region(sioaddr, 2, DRVNAME))
> +		return -EBUSY;
> +	return 0;
> +}
> +
>  static inline void superio_outb(int sioaddr, int reg, int val)
>  {
>  	outb(reg, sioaddr);
> @@ -122,6 +129,7 @@ static inline void superio_exit(int sioaddr)
>  {
>  	outb(0x02, sioaddr);
>  	outb(0x02, sioaddr + 1);
> +	release_region(sioaddr, 2);
>  }
>
>  /*
> @@ -1195,7 +1203,11 @@ static int __init pc87427_find(int sioaddr, struct pc87427_sio_data *sio_data)
>  {
>  	u16 val;
>  	u8 cfg, cfg_b;
> -	int i, err = 0;
> +	int i, err;
> +
> +	err = superio_enter(sioaddr);
> +	if (err)
> +		return err;
>
>  	/* Identify device */
>  	val = force_id ? force_id : superio_inb(sioaddr, SIOREG_DEVID);
>

Re: [PATCH 3/4] hwmon: (smsc47b397) Use request_muxed_region for Super-IO accesses

[John Garry]

On 04/04/2019 19:33, Guenter Roeck wrote:
> Super-IO accesses may fail on a system with no or unmapped LPC bus.
>
> Also, other drivers may attempt to access the LPC bus at the same time,
> resulting in undefined behavior.
>
> Use request_muxed_region() to ensure that IO access on the requested
> address space is supported, and to ensure that access by multiple drivers
> is synchronized.
>
> Fixes: 8d5d45fb1468 ("I2C: Move hwmon drivers (2/3)")
> Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> Reported-by: John Garry <john.garry@huawei.com>

Acked-by: John Garry <john.garry@huawei.com>

> Cc: John Garry <john.garry@huawei.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/smsc47b397.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/smsc47b397.c b/drivers/hwmon/smsc47b397.c
> index c0775084dde0..60e193f2e970 100644
> --- a/drivers/hwmon/smsc47b397.c
> +++ b/drivers/hwmon/smsc47b397.c
> @@ -72,14 +72,19 @@ static inline void superio_select(int ld)
>  	superio_outb(0x07, ld);
>  }
>
> -static inline void superio_enter(void)
> +static inline int superio_enter(void)
>  {
> +	if (!request_muxed_region(REG, 2, DRVNAME))
> +		return -EBUSY;
> +
>  	outb(0x55, REG);
> +	return 0;
>  }
>
>  static inline void superio_exit(void)
>  {
>  	outb(0xAA, REG);
> +	release_region(REG, 2);
>  }
>
>  #define SUPERIO_REG_DEVID	0x20
> @@ -300,8 +305,12 @@ static int __init smsc47b397_find(void)
>  	u8 id, rev;
>  	char *name;
>  	unsigned short addr;
> +	int err;
> +
> +	err = superio_enter();
> +	if (err)
> +		return err;
>
> -	superio_enter();
>  	id = force_id ? force_id : superio_inb(SUPERIO_REG_DEVID);
>
>  	switch (id) {
>

Re: [PATCH 4/4] hwmon: (smsc47b397) Use request_muxed_region for Super-IO accesses

[John Garry]

On 04/04/2019 19:33, Guenter Roeck wrote:
> Super-IO accesses may fail on a system with no or unmapped LPC bus.
>
> Also, other drivers may attempt to access the LPC bus at the same time,
> resulting in undefined behavior.
>
> Use request_muxed_region() to ensure that IO access on the requested
> address space is supported, and to ensure that access by multiple drivers
> is synchronized.
>
> Fixes: 8d5d45fb1468 ("I2C: Move hwmon drivers (2/3)")
> Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> Reported-by: John Garry <john.garry@huawei.com>
> Cc: John Garry <john.garry@huawei.com>

Acked-by: John Garry <john.garry@huawei.com>

> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/smsc47m1.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/smsc47m1.c b/drivers/hwmon/smsc47m1.c
> index c7b6a425e2c0..0736ca6a3aee 100644
> --- a/drivers/hwmon/smsc47m1.c
> +++ b/drivers/hwmon/smsc47m1.c
> @@ -73,16 +73,21 @@ superio_inb(int reg)
>  /* logical device for fans is 0x0A */
>  #define superio_select() superio_outb(0x07, 0x0A)
>
> -static inline void
> +static inline int
>  superio_enter(void)
>  {
> +	if (!request_muxed_region(REG, 2, DRVNAME))
> +		return -EBUSY;
> +
>  	outb(0x55, REG);
> +	return 0;
>  }
>
>  static inline void
>  superio_exit(void)
>  {
>  	outb(0xAA, REG);
> +	release_region(REG, 2);
>  }
>
>  #define SUPERIO_REG_ACT		0x30
> @@ -531,8 +536,12 @@ static int __init smsc47m1_find(struct smsc47m1_sio_data *sio_data)
>  {
>  	u8 val;
>  	unsigned short addr;
> +	int err;
> +
> +	err = superio_enter();
> +	if (err)
> +		return err;
>
> -	superio_enter();
>  	val = force_id ? force_id : superio_inb(SUPERIO_REG_DEVID);
>
>  	/*
>