* Kenji Kaneshige <[email protected]>:
> Kenji Kaneshige wrote:
>> Hi Alex-san,
>>
>> Here is one comment, though I have not finished reviewing/testing
>> your patches yet (sorry for the delay).
>>
>> Alex Chiang wrote:
>>
>> (snip.)
>>
>>> diff --git a/drivers/pci/hotplug/pci_hotplug_core.c
>>> b/drivers/pci/hotplug/pci_hotplug_core.c
>>> index 3e37d63..46802dc 100644
>>> --- a/drivers/pci/hotplug/pci_hotplug_core.c
>>> +++ b/drivers/pci/hotplug/pci_hotplug_core.c
>>> @@ -570,39 +570,32 @@ int pci_hp_register(struct hotplug_slot *slot,
>>> struct pci_bus *bus, int slot_nr,
>>> return -EINVAL;
>>> }
>>> - /* Check if we have already registered a slot with the same
>>> name. */
>>> - if (get_slot_from_name(name))
>>> - return -EEXIST;
>>> -
>>> /*
>>> - * No problems if we call this interface from both ACPI_PCI_SLOT
>>> - * driver and call it here again. If we've already created the
>>> - * pci_slot, the interface will simply bump the refcount.
>>> + * Look for existing slot. If we find it, and it was created by a
>>> + * slot detection driver (ie, doesn't have a ->hotplug()) then we
>>> + * allow the hotplug driver calling us to rename the slot if
>>> desired.
>>> + *
>>> + * Otherwise, create the slot and carry on with life.
>>> */
>>> - pci_slot = pci_create_slot(bus, slot_nr, name);
>>> - if (IS_ERR(pci_slot))
>>> - return PTR_ERR(pci_slot);
>>> -
>>> - if (pci_slot->hotplug) {
>>> - dbg("%s: already claimed\n", __func__);
>>> - pci_destroy_slot(pci_slot);
>>> - return -EBUSY;
>>> + pci_slot = pci_get_pci_slot(bus, slot_nr);
>>
>> The pci_get_pci_slot() function refers pci_bus->slots list, so it
>> should be called with pci_bus_sem semaphore held as pci_create_slot()
>> does, or pci_bus_sem semaphore should be held by pci_get_pci_slot()
>> itself.
Yes, I've changed pci_get_pci_slot() to acquire the pci_bus_sem
semaphore.
Thank you for pointing this out.
It will be fixed in v4 of this patch series, which I will send
out after I receive the rest of your review comments.
Thanks!
/ac
Alex-san,
Alex Chiang wrote:
> * Kenji Kaneshige <[email protected]>:
>> Kenji Kaneshige wrote:
>>> Hi Alex-san,
>>>
>>> Here is one comment, though I have not finished reviewing/testing
>>> your patches yet (sorry for the delay).
>>>
>>> Alex Chiang wrote:
>>>
>>> (snip.)
>>>
>>>> diff --git a/drivers/pci/hotplug/pci_hotplug_core.c
>>>> b/drivers/pci/hotplug/pci_hotplug_core.c
>>>> index 3e37d63..46802dc 100644
>>>> --- a/drivers/pci/hotplug/pci_hotplug_core.c
>>>> +++ b/drivers/pci/hotplug/pci_hotplug_core.c
>>>> @@ -570,39 +570,32 @@ int pci_hp_register(struct hotplug_slot *slot,
>>>> struct pci_bus *bus, int slot_nr,
>>>> return -EINVAL;
>>>> }
>>>> - /* Check if we have already registered a slot with the same
>>>> name. */
>>>> - if (get_slot_from_name(name))
>>>> - return -EEXIST;
>>>> -
>>>> /*
>>>> - * No problems if we call this interface from both ACPI_PCI_SLOT
>>>> - * driver and call it here again. If we've already created the
>>>> - * pci_slot, the interface will simply bump the refcount.
>>>> + * Look for existing slot. If we find it, and it was created by a
>>>> + * slot detection driver (ie, doesn't have a ->hotplug()) then we
>>>> + * allow the hotplug driver calling us to rename the slot if
>>>> desired.
>>>> + *
>>>> + * Otherwise, create the slot and carry on with life.
>>>> */
>>>> - pci_slot = pci_create_slot(bus, slot_nr, name);
>>>> - if (IS_ERR(pci_slot))
>>>> - return PTR_ERR(pci_slot);
>>>> -
>>>> - if (pci_slot->hotplug) {
>>>> - dbg("%s: already claimed\n", __func__);
>>>> - pci_destroy_slot(pci_slot);
>>>> - return -EBUSY;
>>>> + pci_slot = pci_get_pci_slot(bus, slot_nr);
>>> The pci_get_pci_slot() function refers pci_bus->slots list, so it
>>> should be called with pci_bus_sem semaphore held as pci_create_slot()
>>> does, or pci_bus_sem semaphore should be held by pci_get_pci_slot()
>>> itself.
>
> Yes, I've changed pci_get_pci_slot() to acquire the pci_bus_sem
> semaphore.
>
> Thank you for pointing this out.
>
> It will be fixed in v4 of this patch series, which I will send
> out after I receive the rest of your review comments.
>
I noticed that changing pci_get_pci_slot() to acquire the pci_bus_sem
might be not enough. If slot was created between pci_get_pci_slot() and
pci_create_slot() by another thread in the following code, something
wrong would happen I think.
pci_slot = pci_get_pci_slot(bus, slot_nr);
if (pci_slot) {
if (pci_slot->hotplug) {
result = -EBUSY;
goto err;
}
if (strcmp(kobject_name(&pci_slot->kobj), name))
if ((result = pci_rename_slot(pci_slot, name)))
goto err;
} else {
pci_slot = pci_create_slot(bus, slot_nr, name);
if ((result = IS_ERR(pci_slot)))
goto out;
}
I've finished reviewing and testing your patches. The rest of your
patch looks good to me. Of course, we must not forget the comment
from Taku Izumi.
Thanks,
Kenji Kaneshige
Hi Kenji-san,
* Kenji Kaneshige <[email protected]>:
> Alex Chiang wrote:
>> * Kenji Kaneshige <[email protected]>:
>>> Kenji Kaneshige wrote:
>>>> Hi Alex-san,
>>>>
>>>> Here is one comment, though I have not finished reviewing/testing
>>>> your patches yet (sorry for the delay).
>>>>
>>>> Alex Chiang wrote:
>>>>
>>>> (snip.)
>>>>
>>>>> diff --git a/drivers/pci/hotplug/pci_hotplug_core.c
>>>>> b/drivers/pci/hotplug/pci_hotplug_core.c
>>>>> index 3e37d63..46802dc 100644
>>>>> --- a/drivers/pci/hotplug/pci_hotplug_core.c
>>>>> +++ b/drivers/pci/hotplug/pci_hotplug_core.c
>>>>> @@ -570,39 +570,32 @@ int pci_hp_register(struct hotplug_slot
>>>>> *slot, struct pci_bus *bus, int slot_nr,
>>>>> return -EINVAL;
>>>>> }
>>>>> - /* Check if we have already registered a slot with the same
>>>>> name. */
>>>>> - if (get_slot_from_name(name))
>>>>> - return -EEXIST;
>>>>> -
>>>>> /*
>>>>> - * No problems if we call this interface from both ACPI_PCI_SLOT
>>>>> - * driver and call it here again. If we've already created the
>>>>> - * pci_slot, the interface will simply bump the refcount.
>>>>> + * Look for existing slot. If we find it, and it was created by a
>>>>> + * slot detection driver (ie, doesn't have a ->hotplug()) then we
>>>>> + * allow the hotplug driver calling us to rename the slot if
>>>>> desired.
>>>>> + *
>>>>> + * Otherwise, create the slot and carry on with life.
>>>>> */
>>>>> - pci_slot = pci_create_slot(bus, slot_nr, name);
>>>>> - if (IS_ERR(pci_slot))
>>>>> - return PTR_ERR(pci_slot);
>>>>> -
>>>>> - if (pci_slot->hotplug) {
>>>>> - dbg("%s: already claimed\n", __func__);
>>>>> - pci_destroy_slot(pci_slot);
>>>>> - return -EBUSY;
>>>>> + pci_slot = pci_get_pci_slot(bus, slot_nr);
>>>> The pci_get_pci_slot() function refers pci_bus->slots list, so it
>>>> should be called with pci_bus_sem semaphore held as pci_create_slot()
>>>> does, or pci_bus_sem semaphore should be held by pci_get_pci_slot()
>>>> itself.
>>
>> Yes, I've changed pci_get_pci_slot() to acquire the pci_bus_sem
>> semaphore.
>>
>> Thank you for pointing this out.
>>
>> It will be fixed in v4 of this patch series, which I will send
>> out after I receive the rest of your review comments.
>>
>
> I noticed that changing pci_get_pci_slot() to acquire the pci_bus_sem
> might be not enough. If slot was created between pci_get_pci_slot() and
> pci_create_slot() by another thread in the following code, something
> wrong would happen I think.
>
> pci_slot = pci_get_pci_slot(bus, slot_nr);
> if (pci_slot) {
> if (pci_slot->hotplug) {
> result = -EBUSY;
> goto err;
> }
>
> if (strcmp(kobject_name(&pci_slot->kobj), name))
> if ((result = pci_rename_slot(pci_slot, name)))
> goto err;
> } else {
> pci_slot = pci_create_slot(bus, slot_nr, name);
> if ((result = IS_ERR(pci_slot)))
> goto out;
> }
I'm sorry, I don't think I see the problem you are pointing out.
If pci_get_pci_slot() finds a pci_slot, we do not modify
pci_bus->slots any further, so even if a new slot is created, it
shouldn't affect the pci_slot that we already found.
I must be missing something, but I don't know what. Would you
mind explaining what you had in mind with your comment?
> I've finished reviewing and testing your patches. The rest of your
> patch looks good to me. Of course, we must not forget the comment
> from Taku Izumi.
Yes, I've modified my patch series to take into account the bugs
that Taku-san found.
Thank you both for your reviews.
/ac
Alex Chiang wrote:
> Hi Kenji-san,
>
> * Kenji Kaneshige <[email protected]>:
>> Alex Chiang wrote:
>>> * Kenji Kaneshige <[email protected]>:
>>>> Kenji Kaneshige wrote:
>>>>> Hi Alex-san,
>>>>>
>>>>> Here is one comment, though I have not finished reviewing/testing
>>>>> your patches yet (sorry for the delay).
>>>>>
>>>>> Alex Chiang wrote:
>>>>>
>>>>> (snip.)
>>>>>
>>>>>> diff --git a/drivers/pci/hotplug/pci_hotplug_core.c
>>>>>> b/drivers/pci/hotplug/pci_hotplug_core.c
>>>>>> index 3e37d63..46802dc 100644
>>>>>> --- a/drivers/pci/hotplug/pci_hotplug_core.c
>>>>>> +++ b/drivers/pci/hotplug/pci_hotplug_core.c
>>>>>> @@ -570,39 +570,32 @@ int pci_hp_register(struct hotplug_slot
>>>>>> *slot, struct pci_bus *bus, int slot_nr,
>>>>>> return -EINVAL;
>>>>>> }
>>>>>> - /* Check if we have already registered a slot with the same
>>>>>> name. */
>>>>>> - if (get_slot_from_name(name))
>>>>>> - return -EEXIST;
>>>>>> -
>>>>>> /*
>>>>>> - * No problems if we call this interface from both ACPI_PCI_SLOT
>>>>>> - * driver and call it here again. If we've already created the
>>>>>> - * pci_slot, the interface will simply bump the refcount.
>>>>>> + * Look for existing slot. If we find it, and it was created by a
>>>>>> + * slot detection driver (ie, doesn't have a ->hotplug()) then we
>>>>>> + * allow the hotplug driver calling us to rename the slot if
>>>>>> desired.
>>>>>> + *
>>>>>> + * Otherwise, create the slot and carry on with life.
>>>>>> */
>>>>>> - pci_slot = pci_create_slot(bus, slot_nr, name);
>>>>>> - if (IS_ERR(pci_slot))
>>>>>> - return PTR_ERR(pci_slot);
>>>>>> -
>>>>>> - if (pci_slot->hotplug) {
>>>>>> - dbg("%s: already claimed\n", __func__);
>>>>>> - pci_destroy_slot(pci_slot);
>>>>>> - return -EBUSY;
>>>>>> + pci_slot = pci_get_pci_slot(bus, slot_nr);
>>>>> The pci_get_pci_slot() function refers pci_bus->slots list, so it
>>>>> should be called with pci_bus_sem semaphore held as pci_create_slot()
>>>>> does, or pci_bus_sem semaphore should be held by pci_get_pci_slot()
>>>>> itself.
>>> Yes, I've changed pci_get_pci_slot() to acquire the pci_bus_sem
>>> semaphore.
>>>
>>> Thank you for pointing this out.
>>>
>>> It will be fixed in v4 of this patch series, which I will send
>>> out after I receive the rest of your review comments.
>>>
>> I noticed that changing pci_get_pci_slot() to acquire the pci_bus_sem
>> might be not enough. If slot was created between pci_get_pci_slot() and
>> pci_create_slot() by another thread in the following code, something
>> wrong would happen I think.
>>
>> pci_slot = pci_get_pci_slot(bus, slot_nr);
>> if (pci_slot) {
>> if (pci_slot->hotplug) {
>> result = -EBUSY;
>> goto err;
>> }
>>
>> if (strcmp(kobject_name(&pci_slot->kobj), name))
>> if ((result = pci_rename_slot(pci_slot, name)))
>> goto err;
>> } else {
>> pci_slot = pci_create_slot(bus, slot_nr, name);
>> if ((result = IS_ERR(pci_slot)))
>> goto out;
>> }
>
> I'm sorry, I don't think I see the problem you are pointing out.
>
> If pci_get_pci_slot() finds a pci_slot, we do not modify
> pci_bus->slots any further, so even if a new slot is created, it
> shouldn't affect the pci_slot that we already found.
>
> I must be missing something, but I don't know what. Would you
> mind explaining what you had in mind with your comment?
>
Here is one of the example of the problem I imagine.
When pci_get_pci_slot() does NOT find a pci_slot, pci_hp_register()
calls pci_create_slot(). If another hotplug driver creates slot
between pci_get_pci_slot() and pci_create_slot(), hotplug member of
pci_slot returned from pci_create_slot() is not NULL in the following
code.
>> pci_slot = pci_create_slot(bus, slot_nr, name);
>> if ((result = IS_ERR(pci_slot)))
>> goto out;
In this case, we should return with -EBUSY. But because there is no
check to see if pci->slot is NULL, pci_slot->hotplug will be overwritten
with the new value.
Thanks,
Kenji Kaneshige