[RFC PATCH 0/2] Fix system crash for accessing unmapped IO port regions

[RFC PATCH 0/2] Fix system crash for accessing unmapped IO port regions

[John Garry]

It was reported some time ago that systems will crash if a driver attempts
to access IO port addresses when the PCI IO port region has not been
mapped [1].

More recently, a similar crash was seen where the system PCI host probe
fails, and the IPMI driver crashes the system while attempting to do some
IO port accesses [2].

This (incomplete) patchset attempts to keep the kernel alive in such
situations, by rejecting IO port resource requests until PCI IO port
regions have been mapped (in a pci_remap_iospace() call).

Currently the PCI IO port region is initialized to the full range,
{0, IO_SPACE_LIMIT}. As such, any IO port region requests would not fail
because of PCI IO port regions not being mapped.

This patchset looks to remedy this issue by ensuring IO port requests are
made to direct children of ioport_resource (PCI host IO port regions),
similar to Arnd's solution, in [1]:

"I see that ioport_resource gets initialized to the {0, IO_SPACE_LIMIT}
range. If we could change it so that pci_remap_iospace() hooks up
to ioport_resource and extends it whenever something gets mapped
there up to IO_SPACE_LIMIT, we can change the default range to
{0,0}, which would fail for any request_region call before the
first pci_remap_iospace."

I didn't use this solution exactly, as I thought that it may cause
problems if we later wanted to remove PCI host IO port regions.

There is another separate issue that many drivers fail to request IO port
region, prior to access. This patchset fixes the f71805f driver as an
example.

There are others drivers which need to be fixed up to do the same.

1. https://www.spinics.net/lists/linux-pci/msg49821.html
2. https://www.spinics.net/lists/arm-kernel/msg694702.html

John Garry (2):
  resource: Request IO port regions from children of ioport_resource
  hwmon: (f71805f): Use request_region() in f71805f_init()

 drivers/hwmon/f71805f.c | 13 ++++++++++++-
 include/linux/ioport.h  |  6 +++++-
 kernel/resource.c       | 19 +++++++++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)

-- 
2.17.1

[RFC PATCH 1/2] resource: Request IO port regions from children of ioport_resource

[John Garry]

Currently when we request an IO port region, the request is made directly
to the top resource, ioport_resource.

There is an issue here, in that drivers may request an IO port region even
if the IO port region has not even been mapped in (in pci_remap_iospace()).

This may lead to crashes when the PCI host has not enumerated prior to
accessing the IO port region, as below:

root@(none)$ insmod f71805f.ko
[   32.264016] Unable to handle kernel paging request at virtual address ffff7dfffee0002e
[   32.279936] Mem abort info:
[   32.285536]   ESR = 0x96000046
[   32.291657]   Exception class = DABT (current EL), IL = 32 bits
[   32.303542]   SET = 0, FnV = 0
[   32.309661]   EA = 0, S1PTW = 0
[   32.315957] Data abort info:
[   32.321728]   ISV = 0, ISS = 0x00000046
[   32.329420]   CM = 0, WnR = 1
[   32.335367] swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____)
[   32.349174] [ffff7dfffee0002e] pgd=0000000001392003, pud=0000000001393003, pmd=0000000000000000
[   32.366652] Internal error: Oops: 96000046 [#1] PREEMPT SMP
[   32.377835] Modules linked in: f71805f(+)
[   32.385876] CPU: 4 PID: 2681 Comm: insmod Not tainted 5.0.0-00003-ga6247cc0f8f2 #52
[   32.401252] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018
[   32.419597] pstate: 80000005 (Nzcv daif -PAN -UAO)
[   32.429213] pc : logic_outb+0x54/0xb8
[   32.436556] lr : f71805f_find+0x2c/0x1b8 [f71805f]
[   32.446167] sp : ffff0000256bba90
[   32.452808] x29: ffff0000256bba90 x28: ffff000008b544d0
[   32.463468] x27: ffff0000256bbdf0 x26: 0000000000000100
[   32.474127] x25: 000000000000002c x24: ffff000011396000
[   32.484787] x23: ffff0000256bbb3e x22: ffff0000256bbb40
[   32.495446] x21: ffff000008b591b8 x20: 0000000000000087
[   32.506106] x19: 000000000000002e x18: ffffffffffffffff
[   32.516765] x17: 0000000000000000 x16: 0000000000000000
[   32.527424] x15: 0000000000000400 x14: 0000000000000400
[   32.538084] x13: 0000000000001832 x12: 0000000000000000
[   32.548743] x11: ffff801ffbf08ac0 x10: 0000801fead02000
[   32.559403] x9 : ffff801ffbffe840 x8 : 0000000000000001
[   32.570062] x7 : 0000000000210d00 x6 : 0000000000000000
[   32.580721] x5 : ffff801fb3eac380 x4 : ffff801ffbef4b20
[   32.591381] x3 : 0000000000ffbffe x2 : ffff0000256bbb40
[   32.602040] x1 : ffff7dfffee0002e x0 : ffff7dfffee00000
[   32.612701] Process insmod (pid: 2681, stack limit = 0x(____ptrval____))
[   32.626155] Call trace:
[   32.631050]  logic_outb+0x54/0xb8
[   32.637693]  f71805f_find+0x2c/0x1b8 [f71805f]
[   32.646607]  f71805f_init+0x38/0xe48 [f71805f]
[   32.655521]  do_one_initcall+0x5c/0x178
[   32.663212]  do_init_module+0x58/0x1b0
[   32.670728]  load_module+0x1dc8/0x2178
[   32.678243]  __se_sys_init_module+0x14c/0x1e8
[   32.686981]  __arm64_sys_init_module+0x18/0x20
[   32.695894]  el0_svc_common+0x60/0x100
[   32.703409]  el0_svc_handler+0x2c/0x80
[   32.710924]  el0_svc+0x8/0xc
[   32.716693] Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034)
[   32.728925] ---[ end trace ddb5e493ee686685 ]---
Segmentation fault
root@(none)$

This issue was originally reported in [1].

This patch changes the functionality of request_region() to request a
region of the direct children of the top ioport_resource.

In this, if the IO port region has not been mapped for a particular IO
region, a suitable child region will not exist, and, as such,
request_region() calls will fail.

A side note: there are many drivers in the kernel which fail to even call
request_region() prior to IO port accesses, and they also need to be fixed
(to call request_region() as appropriate).

[1] https://www.spinics.net/lists/linux-pci/msg49821.html

Signed-off-by: John Garry <john.garry@huawei.com>
---
 include/linux/ioport.h |  6 +++++-
 kernel/resource.c      | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index da0ebaec25f0..cf40e1ed8211 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -217,7 +217,7 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2)
 
 
 /* Convenience shorthand with allocation */
-#define request_region(start,n,name)		__request_region(&ioport_resource, (start), (n), (name), 0)
+#define request_region(start,n,name)		__request_region_from_children(&ioport_resource, (start), (n), (name), 0)
 #define request_muxed_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
 #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
 #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)
@@ -230,6 +230,10 @@ extern struct resource * __request_region(struct resource *,
 					resource_size_t n,
 					const char *name, int flags);
 
+extern struct resource * __request_region_from_children(struct resource *,
+							resource_size_t start,
+							resource_size_t n,
+							const char *name, int flags);
 /* Compatibility cruft */
 #define release_region(start,n)	__release_region(&ioport_resource, (start), (n))
 #define release_mem_region(start,n)	__release_region(&iomem_resource, (start), (n))
diff --git a/kernel/resource.c b/kernel/resource.c
index 915c02e8e5dd..4a4656bd6051 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1098,6 +1098,25 @@ resource_size_t resource_alignment(struct resource *res)
 
 static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait);
 
+struct resource * __request_region_from_children(struct resource *parent,
+						 resource_size_t start,
+						 resource_size_t n,
+						 const char *name, int flags)
+{
+	struct resource *child;
+
+	/* TODO - add locking */
+	for (child = parent->child; child; child = child->sibling) {
+		struct resource *res = __request_region(child, start, n, name, flags);
+
+		if (res)
+			return res;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(__request_region_from_children);
+
 /**
  * __request_region - create a new busy resource region
  * @parent: parent resource descriptor
-- 
2.17.1

Re: [RFC PATCH 1/2] resource: Request IO port regions from children of ioport_resource

[Guenter Roeck]

On Fri, Mar 15, 2019 at 12:55:15AM +0800, John Garry wrote:
> Currently when we request an IO port region, the request is made directly
> to the top resource, ioport_resource.
> 
> There is an issue here, in that drivers may request an IO port region even
> if the IO port region has not even been mapped in (in pci_remap_iospace()).
> 
> This may lead to crashes when the PCI host has not enumerated prior to
> accessing the IO port region, as below:
> 
> root@(none)$ insmod f71805f.ko
> [   32.264016] Unable to handle kernel paging request at virtual address ffff7dfffee0002e
> [   32.279936] Mem abort info:
> [   32.285536]   ESR = 0x96000046
> [   32.291657]   Exception class = DABT (current EL), IL = 32 bits
> [   32.303542]   SET = 0, FnV = 0
> [   32.309661]   EA = 0, S1PTW = 0
> [   32.315957] Data abort info:
> [   32.321728]   ISV = 0, ISS = 0x00000046
> [   32.329420]   CM = 0, WnR = 1
> [   32.335367] swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____)
> [   32.349174] [ffff7dfffee0002e] pgd=0000000001392003, pud=0000000001393003, pmd=0000000000000000
> [   32.366652] Internal error: Oops: 96000046 [#1] PREEMPT SMP
> [   32.377835] Modules linked in: f71805f(+)
> [   32.385876] CPU: 4 PID: 2681 Comm: insmod Not tainted 5.0.0-00003-ga6247cc0f8f2 #52
> [   32.401252] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018
> [   32.419597] pstate: 80000005 (Nzcv daif -PAN -UAO)
> [   32.429213] pc : logic_outb+0x54/0xb8
> [   32.436556] lr : f71805f_find+0x2c/0x1b8 [f71805f]
> [   32.446167] sp : ffff0000256bba90
> [   32.452808] x29: ffff0000256bba90 x28: ffff000008b544d0
> [   32.463468] x27: ffff0000256bbdf0 x26: 0000000000000100
> [   32.474127] x25: 000000000000002c x24: ffff000011396000
> [   32.484787] x23: ffff0000256bbb3e x22: ffff0000256bbb40
> [   32.495446] x21: ffff000008b591b8 x20: 0000000000000087
> [   32.506106] x19: 000000000000002e x18: ffffffffffffffff
> [   32.516765] x17: 0000000000000000 x16: 0000000000000000
> [   32.527424] x15: 0000000000000400 x14: 0000000000000400
> [   32.538084] x13: 0000000000001832 x12: 0000000000000000
> [   32.548743] x11: ffff801ffbf08ac0 x10: 0000801fead02000
> [   32.559403] x9 : ffff801ffbffe840 x8 : 0000000000000001
> [   32.570062] x7 : 0000000000210d00 x6 : 0000000000000000
> [   32.580721] x5 : ffff801fb3eac380 x4 : ffff801ffbef4b20
> [   32.591381] x3 : 0000000000ffbffe x2 : ffff0000256bbb40
> [   32.602040] x1 : ffff7dfffee0002e x0 : ffff7dfffee00000
> [   32.612701] Process insmod (pid: 2681, stack limit = 0x(____ptrval____))
> [   32.626155] Call trace:
> [   32.631050]  logic_outb+0x54/0xb8
> [   32.637693]  f71805f_find+0x2c/0x1b8 [f71805f]
> [   32.646607]  f71805f_init+0x38/0xe48 [f71805f]
> [   32.655521]  do_one_initcall+0x5c/0x178
> [   32.663212]  do_init_module+0x58/0x1b0
> [   32.670728]  load_module+0x1dc8/0x2178
> [   32.678243]  __se_sys_init_module+0x14c/0x1e8
> [   32.686981]  __arm64_sys_init_module+0x18/0x20
> [   32.695894]  el0_svc_common+0x60/0x100
> [   32.703409]  el0_svc_handler+0x2c/0x80
> [   32.710924]  el0_svc+0x8/0xc
> [   32.716693] Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034)
> [   32.728925] ---[ end trace ddb5e493ee686685 ]---
> Segmentation fault
> root@(none)$
> 
> This issue was originally reported in [1].
> 
> This patch changes the functionality of request_region() to request a
> region of the direct children of the top ioport_resource.
> 
> In this, if the IO port region has not been mapped for a particular IO
> region, a suitable child region will not exist, and, as such,
> request_region() calls will fail.
> 
> A side note: there are many drivers in the kernel which fail to even call
> request_region() prior to IO port accesses, and they also need to be fixed
> (to call request_region() as appropriate).
> 

Isn't that orthogonal to this problem ?

> [1] https://www.spinics.net/lists/linux-pci/msg49821.html
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  include/linux/ioport.h |  6 +++++-
>  kernel/resource.c      | 19 +++++++++++++++++++
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index da0ebaec25f0..cf40e1ed8211 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -217,7 +217,7 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2)
>  
>  
>  /* Convenience shorthand with allocation */
> -#define request_region(start,n,name)		__request_region(&ioport_resource, (start), (n), (name), 0)
> +#define request_region(start,n,name)		__request_region_from_children(&ioport_resource, (start), (n), (name), 0)
>  #define request_muxed_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
>  #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
>  #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)

I can't comment on the merits of the patch (though I am a bit concerned
about its impact and potential source of regressions), but I don't really
understand why it would only apply to request_region() and not to
request_muxed_region(). Is this an oversight or on purpose ?

Guenter

Re: [RFC PATCH 1/2] resource: Request IO port regions from children of ioport_resource

[John Garry]

On 14/03/2019 17:12, Guenter Roeck wrote:
> On Fri, Mar 15, 2019 at 12:55:15AM +0800, John Garry wrote:
>> Currently when we request an IO port region, the request is made directly
>> to the top resource, ioport_resource.
>>
>> There is an issue here, in that drivers may request an IO port region even
>> if the IO port region has not even been mapped in (in pci_remap_iospace()).
>>
>> This may lead to crashes when the PCI host has not enumerated prior to
>> accessing the IO port region, as below:
>>
>> root@(none)$ insmod f71805f.ko
>> [   32.264016] Unable to handle kernel paging request at virtual address ffff7dfffee0002e
>> [   32.279936] Mem abort info:
>> [   32.285536]   ESR = 0x96000046
>> [   32.291657]   Exception class = DABT (current EL), IL = 32 bits
>> [   32.303542]   SET = 0, FnV = 0
>> [   32.309661]   EA = 0, S1PTW = 0
>> [   32.315957] Data abort info:
>> [   32.321728]   ISV = 0, ISS = 0x00000046
>> [   32.329420]   CM = 0, WnR = 1
>> [   32.335367] swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____)
>> [   32.349174] [ffff7dfffee0002e] pgd=0000000001392003, pud=0000000001393003, pmd=0000000000000000
>> [   32.366652] Internal error: Oops: 96000046 [#1] PREEMPT SMP
>> [   32.377835] Modules linked in: f71805f(+)
>> [   32.385876] CPU: 4 PID: 2681 Comm: insmod Not tainted 5.0.0-00003-ga6247cc0f8f2 #52
>> [   32.401252] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018
>> [   32.419597] pstate: 80000005 (Nzcv daif -PAN -UAO)
>> [   32.429213] pc : logic_outb+0x54/0xb8
>> [   32.436556] lr : f71805f_find+0x2c/0x1b8 [f71805f]
>> [   32.446167] sp : ffff0000256bba90
>> [   32.452808] x29: ffff0000256bba90 x28: ffff000008b544d0
>> [   32.463468] x27: ffff0000256bbdf0 x26: 0000000000000100
>> [   32.474127] x25: 000000000000002c x24: ffff000011396000
>> [   32.484787] x23: ffff0000256bbb3e x22: ffff0000256bbb40
>> [   32.495446] x21: ffff000008b591b8 x20: 0000000000000087
>> [   32.506106] x19: 000000000000002e x18: ffffffffffffffff
>> [   32.516765] x17: 0000000000000000 x16: 0000000000000000
>> [   32.527424] x15: 0000000000000400 x14: 0000000000000400
>> [   32.538084] x13: 0000000000001832 x12: 0000000000000000
>> [   32.548743] x11: ffff801ffbf08ac0 x10: 0000801fead02000
>> [   32.559403] x9 : ffff801ffbffe840 x8 : 0000000000000001
>> [   32.570062] x7 : 0000000000210d00 x6 : 0000000000000000
>> [   32.580721] x5 : ffff801fb3eac380 x4 : ffff801ffbef4b20
>> [   32.591381] x3 : 0000000000ffbffe x2 : ffff0000256bbb40
>> [   32.602040] x1 : ffff7dfffee0002e x0 : ffff7dfffee00000
>> [   32.612701] Process insmod (pid: 2681, stack limit = 0x(____ptrval____))
>> [   32.626155] Call trace:
>> [   32.631050]  logic_outb+0x54/0xb8
>> [   32.637693]  f71805f_find+0x2c/0x1b8 [f71805f]
>> [   32.646607]  f71805f_init+0x38/0xe48 [f71805f]
>> [   32.655521]  do_one_initcall+0x5c/0x178
>> [   32.663212]  do_init_module+0x58/0x1b0
>> [   32.670728]  load_module+0x1dc8/0x2178
>> [   32.678243]  __se_sys_init_module+0x14c/0x1e8
>> [   32.686981]  __arm64_sys_init_module+0x18/0x20
>> [   32.695894]  el0_svc_common+0x60/0x100
>> [   32.703409]  el0_svc_handler+0x2c/0x80
>> [   32.710924]  el0_svc+0x8/0xc
>> [   32.716693] Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034)
>> [   32.728925] ---[ end trace ddb5e493ee686685 ]---
>> Segmentation fault
>> root@(none)$
>>
>> This issue was originally reported in [1].
>>
>> This patch changes the functionality of request_region() to request a
>> region of the direct children of the top ioport_resource.
>>
>> In this, if the IO port region has not been mapped for a particular IO
>> region, a suitable child region will not exist, and, as such,
>> request_region() calls will fail.
>>
>> A side note: there are many drivers in the kernel which fail to even call
>> request_region() prior to IO port accesses, and they also need to be fixed
>> (to call request_region() as appropriate).
>>

Hi Guenter,

>
> Isn't that orthogonal to this problem ?

Yes, it is really. I only added the f71805f driver fixup to illustrate 
the problem in resource.c and the potential system crash, and how this 
patch would fix it.

>
>> [1] https://www.spinics.net/lists/linux-pci/msg49821.html
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
>>  include/linux/ioport.h |  6 +++++-
>>  kernel/resource.c      | 19 +++++++++++++++++++
>>  2 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>> index da0ebaec25f0..cf40e1ed8211 100644
>> --- a/include/linux/ioport.h
>> +++ b/include/linux/ioport.h
>> @@ -217,7 +217,7 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2)
>>
>>
>>  /* Convenience shorthand with allocation */
>> -#define request_region(start,n,name)		__request_region(&ioport_resource, (start), (n), (name), 0)
>> +#define request_region(start,n,name)		__request_region_from_children(&ioport_resource, (start), (n), (name), 0)
>>  #define request_muxed_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
>>  #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
>>  #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)
>
> I can't comment on the merits of the patch (though I am a bit concerned
> about its impact and potential source of regressions),but I don't really
> understand why it would only apply to request_region() and not to
> request_muxed_region(). Is this an oversight or on purpose ?

It's an oversight, which would be fixed.

Thanks,
John

>
> Guenter
>
> .
>

[RFC PATCH 2/2] hwmon: (f71805f): Use request_region() in f71805f_init()

[John Garry]

Currently the driver does not call request_region() prior to
accessing IO port regions in f71805f_init(), so add it.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/hwmon/f71805f.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/f71805f.c b/drivers/hwmon/f71805f.c
index 73c681162653..910082c7f184 100644
--- a/drivers/hwmon/f71805f.c
+++ b/drivers/hwmon/f71805f.c
@@ -1617,10 +1617,21 @@ static int __init f71805f_init(void)
 	int err;
 	unsigned short address;
 	struct f71805f_sio_data sio_data;
+	struct resource *res;
+	size_t size = 0x4e - 0x2e + SIO_REG_ADDR + 4;
+
+	/* Request the whole 0x2e - 0x4e region */
+	res = request_region(0x2e, size, "f71805f");
+	if (!res)
+		return -EBUSY;
 
 	if (f71805f_find(0x2e, &address, &sio_data)
-	 && f71805f_find(0x4e, &address, &sio_data))
+	 && f71805f_find(0x4e, &address, &sio_data)) {
+		release_region(0x2e, size);
 		return -ENODEV;
+	}
+
+	release_region(0x2e, size);
 
 	err = platform_driver_register(&f71805f_driver);
 	if (err)
-- 
2.17.1

Re: [RFC PATCH 2/2] hwmon: (f71805f): Use request_region() in f71805f_init()

[Guenter Roeck]

On Fri, Mar 15, 2019 at 12:55:16AM +0800, John Garry wrote:
> Currently the driver does not call request_region() prior to
> accessing IO port regions in f71805f_init(), so add it.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/hwmon/f71805f.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/f71805f.c b/drivers/hwmon/f71805f.c
> index 73c681162653..910082c7f184 100644
> --- a/drivers/hwmon/f71805f.c
> +++ b/drivers/hwmon/f71805f.c
> @@ -1617,10 +1617,21 @@ static int __init f71805f_init(void)
>  	int err;
>  	unsigned short address;
>  	struct f71805f_sio_data sio_data;
> +	struct resource *res;
> +	size_t size = 0x4e - 0x2e + SIO_REG_ADDR + 4;
> +
> +	/* Request the whole 0x2e - 0x4e region */
> +	res = request_region(0x2e, size, "f71805f");

request_muxed_region() would be a better choice here since it doesn't
bail out immediately if the region is temporarily busy.

Thanks,
Guenter

> +	if (!res)
> +		return -EBUSY;
>  
>  	if (f71805f_find(0x2e, &address, &sio_data)
> -	 && f71805f_find(0x4e, &address, &sio_data))
> +	 && f71805f_find(0x4e, &address, &sio_data)) {
> +		release_region(0x2e, size);
>  		return -ENODEV;
> +	}
> +
> +	release_region(0x2e, size);
>  
>  	err = platform_driver_register(&f71805f_driver);
>  	if (err)
> -- 
> 2.17.1
> 

Re: [RFC PATCH 2/2] hwmon: (f71805f): Use request_region() in f71805f_init()

[John Garry]

On 14/03/2019 17:05, Guenter Roeck wrote:
> On Fri, Mar 15, 2019 at 12:55:16AM +0800, John Garry wrote:
>> Currently the driver does not call request_region() prior to
>> accessing IO port regions in f71805f_init(), so add it.
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
>>  drivers/hwmon/f71805f.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwmon/f71805f.c b/drivers/hwmon/f71805f.c
>> index 73c681162653..910082c7f184 100644
>> --- a/drivers/hwmon/f71805f.c
>> +++ b/drivers/hwmon/f71805f.c
>> @@ -1617,10 +1617,21 @@ static int __init f71805f_init(void)
>>  	int err;
>>  	unsigned short address;
>>  	struct f71805f_sio_data sio_data;
>> +	struct resource *res;
>> +	size_t size = 0x4e - 0x2e + SIO_REG_ADDR + 4;
>> +
>> +	/* Request the whole 0x2e - 0x4e region */
>> +	res = request_region(0x2e, size, "f71805f");
>

Hi Guenter,

> request_muxed_region() would be a better choice here since it doesn't
> bail out immediately if the region is temporarily busy.

Are you saying that this region could be busy due to another driver 
accessing this same region (probably for the same purpose)?

If so, that would be reasonable.

Thanks,
John

>
> Thanks,
> Guenter
>
>> +	if (!res)
>> +		return -EBUSY;
>>
>>  	if (f71805f_find(0x2e, &address, &sio_data)
>> -	 && f71805f_find(0x4e, &address, &sio_data))
>> +	 && f71805f_find(0x4e, &address, &sio_data)) {
>> +		release_region(0x2e, size);
>>  		return -ENODEV;
>> +	}
>> +
>> +	release_region(0x2e, size);
>>
>>  	err = platform_driver_register(&f71805f_driver);
>>  	if (err)
>> --
>> 2.17.1
>>
>
> .
>

Re: [RFC PATCH 2/2] hwmon: (f71805f): Use request_region() in f71805f_init()

[Guenter Roeck]

On Thu, Mar 14, 2019 at 05:21:32PM +0000, John Garry wrote:
> On 14/03/2019 17:05, Guenter Roeck wrote:
> >On Fri, Mar 15, 2019 at 12:55:16AM +0800, John Garry wrote:
> >>Currently the driver does not call request_region() prior to
> >>accessing IO port regions in f71805f_init(), so add it.
> >>
> >>Signed-off-by: John Garry <john.garry@huawei.com>
> >>---
> >> drivers/hwmon/f71805f.c | 13 ++++++++++++-
> >> 1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/hwmon/f71805f.c b/drivers/hwmon/f71805f.c
> >>index 73c681162653..910082c7f184 100644
> >>--- a/drivers/hwmon/f71805f.c
> >>+++ b/drivers/hwmon/f71805f.c
> >>@@ -1617,10 +1617,21 @@ static int __init f71805f_init(void)
> >> 	int err;
> >> 	unsigned short address;
> >> 	struct f71805f_sio_data sio_data;
> >>+	struct resource *res;
> >>+	size_t size = 0x4e - 0x2e + SIO_REG_ADDR + 4;
> >>+
> >>+	/* Request the whole 0x2e - 0x4e region */
> >>+	res = request_region(0x2e, size, "f71805f");
> >
> 
> Hi Guenter,
> 
> >request_muxed_region() would be a better choice here since it doesn't
> >bail out immediately if the region is temporarily busy.
> 
> Are you saying that this region could be busy due to another driver
> accessing this same region (probably for the same purpose)?
> 
Depends on the definition of "same reason". To access the chip's
Super-IO configuration registers, yes, that can happen. I don't
see it used in the kernel today, but examples would be someone
adding support for the chips supported by this driver to gpio-f7188x,
or adding support for the F71872 watchdog to the f71808e_wdt driver.

Guenter

Re: [RFC PATCH 2/2] hwmon: (f71805f): Use request_region() in f71805f_init()

[John Garry]

On 14/03/2019 17:44, Guenter Roeck wrote:
> On Thu, Mar 14, 2019 at 05:21:32PM +0000, John Garry wrote:
>> On 14/03/2019 17:05, Guenter Roeck wrote:
>>> On Fri, Mar 15, 2019 at 12:55:16AM +0800, John Garry wrote:
>>>> Currently the driver does not call request_region() prior to
>>>> accessing IO port regions in f71805f_init(), so add it.
>>>>
>>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>>> ---
>>>> drivers/hwmon/f71805f.c | 13 ++++++++++++-
>>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/hwmon/f71805f.c b/drivers/hwmon/f71805f.c
>>>> index 73c681162653..910082c7f184 100644
>>>> --- a/drivers/hwmon/f71805f.c
>>>> +++ b/drivers/hwmon/f71805f.c
>>>> @@ -1617,10 +1617,21 @@ static int __init f71805f_init(void)
>>>> 	int err;
>>>> 	unsigned short address;
>>>> 	struct f71805f_sio_data sio_data;
>>>> +	struct resource *res;
>>>> +	size_t size = 0x4e - 0x2e + SIO_REG_ADDR + 4;
>>>> +
>>>> +	/* Request the whole 0x2e - 0x4e region */
>>>> +	res = request_region(0x2e, size, "f71805f");
>>>
>>
>> Hi Guenter,
>>
>>> request_muxed_region() would be a better choice here since it doesn't
>>> bail out immediately if the region is temporarily busy.
>>
>> Are you saying that this region could be busy due to another driver
>> accessing this same region (probably for the same purpose)?
>>

Hi Guenter,

> Depends on the definition of "same reason". To access the chip's
> Super-IO configuration registers, yes, that can happen. I don't
> see it used in the kernel today, but examples would be someone
> adding support for the chips supported by this driver to gpio-f7188x,
> or adding support for the F71872 watchdog to the f71808e_wdt driver.

OK, understood.

In terms of fixing these up, as mentioned @ 
https://www.spinics.net/lists/linux-pci/msg49843.html, there are other 
modules which have this issue. I asked Kefang today, and here's his list:

f71805f.ko
f71882fg.ko
pc87427.ko
sch56xx-common.ko
smsc47b397.ko
smsc47m1.ko

So maybe it's better to send separate patch(sets) for 1/2 and 2/2+other 
hwmon fixes - let me know your preference.

Thanks,
John

>
> Guenter
>
> .
>

Re: [RFC PATCH 2/2] hwmon: (f71805f): Use request_region() in f71805f_init()

[Guenter Roeck]

On 3/15/19 4:20 AM, John Garry wrote:
> On 14/03/2019 17:44, Guenter Roeck wrote:
>> On Thu, Mar 14, 2019 at 05:21:32PM +0000, John Garry wrote:
>>> On 14/03/2019 17:05, Guenter Roeck wrote:
>>>> On Fri, Mar 15, 2019 at 12:55:16AM +0800, John Garry wrote:
>>>>> Currently the driver does not call request_region() prior to
>>>>> accessing IO port regions in f71805f_init(), so add it.
>>>>>
>>>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>>>> ---
>>>>> drivers/hwmon/f71805f.c | 13 ++++++++++++-
>>>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/hwmon/f71805f.c b/drivers/hwmon/f71805f.c
>>>>> index 73c681162653..910082c7f184 100644
>>>>> --- a/drivers/hwmon/f71805f.c
>>>>> +++ b/drivers/hwmon/f71805f.c
>>>>> @@ -1617,10 +1617,21 @@ static int __init f71805f_init(void)
>>>>>     int err;
>>>>>     unsigned short address;
>>>>>     struct f71805f_sio_data sio_data;
>>>>> +    struct resource *res;
>>>>> +    size_t size = 0x4e - 0x2e + SIO_REG_ADDR + 4;
>>>>> +
>>>>> +    /* Request the whole 0x2e - 0x4e region */
>>>>> +    res = request_region(0x2e, size, "f71805f");
>>>>
>>>
>>> Hi Guenter,
>>>
>>>> request_muxed_region() would be a better choice here since it doesn't
>>>> bail out immediately if the region is temporarily busy.
>>>
>>> Are you saying that this region could be busy due to another driver
>>> accessing this same region (probably for the same purpose)?
>>>
> 
> Hi Guenter,
> 
>> Depends on the definition of "same reason". To access the chip's
>> Super-IO configuration registers, yes, that can happen. I don't
>> see it used in the kernel today, but examples would be someone
>> adding support for the chips supported by this driver to gpio-f7188x,
>> or adding support for the F71872 watchdog to the f71808e_wdt driver.
> 
> OK, understood.
> 
> In terms of fixing these up, as mentioned @ https://www.spinics.net/lists/linux-pci/msg49843.html, there are other modules which have this issue. I asked Kefang today, and here's his list:
> 
> f71805f.ko
> f71882fg.ko
> pc87427.ko
> sch56xx-common.ko
> smsc47b397.ko
> smsc47m1.ko
> 
> So maybe it's better to send separate patch(sets) for 1/2 and 2/2+other hwmon fixes - let me know your preference.
> 

One patch per file, please.

Thanks,
Guenter

> Thanks,
> John
> 
>>
>> Guenter
>>
>> .
>>
> 
> 
>