Driver should call pci_disable_device() if it returns from
moxa_pci_probe() with error.

Meanwhile, the driver calls pci_enable_device() in
moxa_pci_probe(), but never calls pci_disable_device() during removal.

Signed-off-by: ruanjinjie <ruanjinjie@huawei.com>
---
 drivers/tty/moxa.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/moxa.c b/drivers/tty/moxa.c
index 35b6fddf0341..9174b64ea2db 100644
--- a/drivers/tty/moxa.c
+++ b/drivers/tty/moxa.c
@@ -1239,7 +1239,7 @@ static int moxa_pci_probe(struct pci_dev *pdev,
 	retval = pci_enable_device(pdev);
 	if (retval) {
 		dev_err(&pdev->dev, "can't enable pci device\n");
-		goto err;
+		return retval;
 	}
 
 	for (i = 0; i < MAX_BOARDS; i++)
@@ -1300,6 +1300,7 @@ static int moxa_pci_probe(struct pci_dev *pdev,
 err_reg:
 	pci_release_region(pdev, 2);
 err:
+	pci_disable_device(pdev);
 	return retval;
 }
 
@@ -1310,6 +1311,7 @@ static void moxa_pci_remove(struct pci_dev *pdev)
 	moxa_board_deinit(brd);
 
 	pci_release_region(pdev, 2);
+	pci_disable_device(pdev);
 }
 
 static struct pci_driver moxa_pci_driver = {
-- 
2.25.1

On Fri, Sep 23, 2022 at 05:25:30PM +0800, ruanjinjie wrote:
> Driver should call pci_disable_device() if it returns from
> moxa_pci_probe() with error.

Why?

That is not a normal thing to do, as you can disable other PCI devices
attached to it, right?

> Meanwhile, the driver calls pci_enable_device() in
> moxa_pci_probe(), but never calls pci_disable_device() during removal.

And is that really a problem?

> 
> Signed-off-by: ruanjinjie <ruanjinjie@huawei.com>
> ---
>  drivers/tty/moxa.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Do you have this hardware to test with?

How did you find this issue?

thanks,

greg k-h

On 2022/9/23 17:56, Greg KH wrote:
> On Fri, Sep 23, 2022 at 05:25:30PM +0800, ruanjinjie wrote:
>> Driver should call pci_disable_device() if it returns from
>> moxa_pci_probe() with error.
> 
> Why?
> 
> That is not a normal thing to do, as you can disable other PCI devices
> attached to it, right?
> 
>> Meanwhile, the driver calls pci_enable_device() in
>> moxa_pci_probe(), but never calls pci_disable_device() during removal.
> 
> And is that really a problem?
> 
>>
>> Signed-off-by: ruanjinjie <ruanjinjie@huawei.com>
>> ---
>>  drivers/tty/moxa.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Do you have this hardware to test with?
> 
> How did you find this issue?
> 

We use static analysis via coccinelle to find the above issue. The
command we use is below:

spatch -I include -timeout 60 -very_quiet -sp_file
pci_disable_device_missing.cocci drivers/tty/moxa.c

> thanks,
> 
> greg k-h

On Fri, Sep 23, 2022 at 06:12:03PM +0800, Ruan Jinjie wrote:
> 
> 
> On 2022/9/23 17:56, Greg KH wrote:
> > On Fri, Sep 23, 2022 at 05:25:30PM +0800, ruanjinjie wrote:
> >> Driver should call pci_disable_device() if it returns from
> >> moxa_pci_probe() with error.
> > 
> > Why?
> > 
> > That is not a normal thing to do, as you can disable other PCI devices
> > attached to it, right?
> > 
> >> Meanwhile, the driver calls pci_enable_device() in
> >> moxa_pci_probe(), but never calls pci_disable_device() during removal.
> > 
> > And is that really a problem?
> > 
> >>
> >> Signed-off-by: ruanjinjie <ruanjinjie@huawei.com>
> >> ---
> >>  drivers/tty/moxa.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > Do you have this hardware to test with?
> > 
> > How did you find this issue?
> > 
> 
> We use static analysis via coccinelle to find the above issue. The
> command we use is below:
> 
> spatch -I include -timeout 60 -very_quiet -sp_file
> pci_disable_device_missing.cocci drivers/tty/moxa.c

Please test with real hardware and look up what pci_disable_device()
does.

greg k-h

On 2022/9/23 19:25, Greg KH wrote:
> On Fri, Sep 23, 2022 at 06:12:03PM +0800, Ruan Jinjie wrote:
>>
>>
>> On 2022/9/23 17:56, Greg KH wrote:
>>> On Fri, Sep 23, 2022 at 05:25:30PM +0800, ruanjinjie wrote:
>>>> Driver should call pci_disable_device() if it returns from
>>>> moxa_pci_probe() with error.
>>>
>>> Why?
>>>
>>> That is not a normal thing to do, as you can disable other PCI devices
>>> attached to it, right?

I think pci_enable_device and pci_disable_device should be paired. If
there are other PCI devices, they will not be disabled because they are
disabled only when reference counting enable_cnt is reduced to 0.

When these devices are initialized, pci_enable_device is called and
reference counting enable_cnt is incremented. Therefore, when the
current device invokes pci_disable_device, other devices are working
normally and have not invoke pci_disable_device. As a result, the value
of enable_cnt is not 0, and the device will not be really disabled.
There's no problem with that.

On the other hand, if there are no other PCI devices attached to it, and
this driver do not call pci_disable_device() if it returns from
moxa_pci_probe() with error or during removal, the device will never be
disabled.

>>>
>>>> Meanwhile, the driver calls pci_enable_device() in
>>>> moxa_pci_probe(), but never calls pci_disable_device() during removal.
>>>
>>> And is that really a problem?
>>>
>>>>
>>>> Signed-off-by: ruanjinjie <ruanjinjie@huawei.com>
>>>> ---
>>>>  drivers/tty/moxa.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> Do you have this hardware to test with?
>>>
>>> How did you find this issue?
>>>
>>
>> We use static analysis via coccinelle to find the above issue. The
>> command we use is below:
>>
>> spatch -I include -timeout 60 -very_quiet -sp_file
>> pci_disable_device_missing.cocci drivers/tty/moxa.c
> 
> Please test with real hardware and look up what pci_disable_device()
> does.
> 
> greg k-h

On 2022/9/23 19:25, Greg KH wrote:
> On Fri, Sep 23, 2022 at 06:12:03PM +0800, Ruan Jinjie wrote:
>>
>>
>> On 2022/9/23 17:56, Greg KH wrote:
>>> On Fri, Sep 23, 2022 at 05:25:30PM +0800, ruanjinjie wrote:
>>>> Driver should call pci_disable_device() if it returns from
>>>> moxa_pci_probe() with error.
>>>
>>> Why?
>>>
>>> That is not a normal thing to do, as you can disable other PCI devices
>>> attached to it, right?
>>>
>>>> Meanwhile, the driver calls pci_enable_device() in
>>>> moxa_pci_probe(), but never calls pci_disable_device() during removal.
>>>
>>> And is that really a problem?
>>>
>>>>
>>>> Signed-off-by: ruanjinjie <ruanjinjie@huawei.com>
>>>> ---
>>>>  drivers/tty/moxa.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> Do you have this hardware to test with?
>>>
>>> How did you find this issue?
>>>
>>
>> We use static analysis via coccinelle to find the above issue. The
>> command we use is below:
>>
>> spatch -I include -timeout 60 -very_quiet -sp_file
>> pci_disable_device_missing.cocci drivers/tty/moxa.c
> 
> Please test with real hardware and look up what pci_disable_device()
> does.

Thank you very much for your sincere advice! we will deeply experiment
with this issue.

> 
> greg k-h