On Sat, Apr 7, 2012 at 8:51 AM, Jiang Liu <liuj97@xxxxxxxxx> wrote:
> On 04/07/2012 07:23 AM, Bjorn Helgaas wrote:
>> On Fri, Apr 6, 2012 at 9:31 AM, Jiang Liu <liuj97@xxxxxxxxx> wrote:
>>> Function pci_get_dev_by_id() will hold a reference count on the pci device
>>> returned, so pci_dev_present() should release the corresponding reference
>>> count to avoid memory leakage.
>>>
>>> Signed-off-by: Jiang Liu <jiang.liu@xxxxxxxxxx>
>>> ---
>>> drivers/pci/search.c | 10 +++++-----
>>> 1 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
>>> index 9d75dc8..b572730 100644
>>> --- a/drivers/pci/search.c
>>> +++ b/drivers/pci/search.c
>>> @@ -338,13 +338,13 @@ int pci_dev_present(const struct pci_device_id *ids)
>>> WARN_ON(in_interrupt());
>>> while (ids->vendor || ids->subvendor || ids->class_mask) {
>>> found = pci_get_dev_by_id(ids, NULL);
>>> - if (found)
>>> - goto exit;
>>> + if (found) {
>>> + pci_dev_put(found);
>>> + return 1;
>>> + }
>>> ids++;
>>> }
>>> -exit:
>>> - if (found)
>>> - return 1;
>>> +
>>> return 0;
>>> }
>>> EXPORT_SYMBOL(pci_dev_present);
>>
>> This might be the right thing to do, but I'd like to understand what's
>> going on, so let's talk about it a bit first.
>>
>> I agree, there appears to be a leak here. Or at least, the fact that
>> we keep a reference when a device is found doesn't match the comment.
>> What problem are you seeing from this leak?
>>
>> There are not many callers, and most appear to be one-time things done
>> at boot, looking for built-in devices known to be defective. These
>> devices won't be removable, so the leak shouldn't be causing
>> hot-remove issues.
> I noticed this issue when reading code, no real issue disclosed yet.
> As you have pointed out, this interface is used for built-in devices only,
> there should be no real issue currently and the patch is just for purity.
>
>>
>> IMO, this is a bogus interface that leads to poor code, and I don't
>> want to encourage its use. For device defect workarounds, I think
>> it'd be better to use PCI quirks to catch the defective device. Some
>> chipset defects affect all downstream devices, and a quirk could make
>> the defect visible to all the drivers, not just the ones that use
>> pci_dev_present(). For example, look at tg3_write_reorder_chipsets
>> and tg3_dma_wait_state_chipsets. Those aren't for tg3 bugs, they're
>> for chipset bugs that might affect other devices, too. But right now,
>> that knowledge is buried in the tg3 driver.
> Thanks for pointing out the real issue behind this bogus interface.
> It would be better to solve this type of chip bugs in PCI core instead
> of in individual drivers.
I applied this to the -next branch, thanks.
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
