diff mbox series

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

Message ID 1552582516-70855-3-git-send-email-john.garry@huawei.com (mailing list archive)
State Superseded, archived
Headers show
Series Fix system crash for accessing unmapped IO port regions | expand

Commit Message

John Garry March 14, 2019, 4:55 p.m. UTC
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(-)

Comments

Guenter Roeck March 14, 2019, 5:05 p.m. UTC | #1
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
>
John Garry March 14, 2019, 5:21 p.m. UTC | #2
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
>>
>
> .
>
Guenter Roeck March 14, 2019, 5:44 p.m. UTC | #3
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
John Garry March 15, 2019, 11:20 a.m. UTC | #4
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
>
> .
>
Guenter Roeck March 15, 2019, 12:58 p.m. UTC | #5
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
>>
>> .
>>
> 
> 
>
diff mbox series

Patch

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)