Re: [PATCH -next] tty: moxa: add missing pci_disable_device()

[Ruan Jinjie]

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@xxxxxxxxxx>
>>>> ---
>>>>  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