On 2012/11/12 15:17, Kaneshige, Kenji wrote:
>> @@ -94,6 +93,8 @@ static int init_slot(struct controller *ctrl)
>> struct hotplug_slot_info *info = NULL;
>> struct hotplug_slot_ops *ops = NULL;
>> char name[SLOT_NAME_SIZE];
>> + char *buffer;
>> + int len;
>> int retval = -ENOMEM;
>>
>> hotplug = kzalloc(sizeof(*hotplug), GFP_KERNEL);
>> @@ -135,6 +136,19 @@ static int init_slot(struct controller *ctrl)
>> if (retval)
>> ctrl_err(ctrl,
>> "pci_hp_register failed with error %d\n", retval);
>
> I think it's natural to go to out: here if retval != 0.
> I guess you intentionally didn't do that because you might want to do
> workqueue cleanup in pciehp_release_ctrl() code path. But it is confusing
> and hard to understand.
Hi Kaneshige,
You are right, go to out here is better.
> In the previous patch, you created the work queue in pcie_init_slot().
> I think it's better. Maybe the reason you moved it from pcie_init_slot()
> to init_slot() was that you needed the slot name by pci_hp_register().
>
> How about using physical slot number, which is same as pci slot name by
> pci_hp_register() on the normal platform, for the workqueue name instead?
>
I think use physical slot number is ok, so what about workqueue name like slot(physical slot number) format ?
root 45808 0.0 0.0 0 0 ? S< 16:25 0:00 [slot(0)]
root 45815 0.0 0.0 0 0 ? S< 16:25 0:00 [slot(4)]
root 45836 0.0 0.0 0 0 ? S< 16:25 0:00 [slot(5)]
root 45840 0.0 0.0 0 0 ? S< 16:25 0:00 [slot(3)]
root 45842 0.0 0.0 0 0 ? S< 16:25 0:00 [slot(7)]
root 45844 0.0 0.0 0 0 ? S< 16:25 0:00 [slot(228)]
root 45846 0.0 0.0 0 0 ? S< 16:25 0:00 [slot(232)]
root 45848 0.0 0.0 0 0 ? S< 16:25 0:00 [slot(233)]
root 45850 0.0 0.0 0 0 ? S< 16:25 0:00 [slot(240)]
root 45852 0.0 0.0 0 0 ? S< 16:25 0:00 [slot(241)]
root 45854 0.0 0.0 0 0 ? S< 16:25 0:00 [slot(244)]
root 45856 0.0 0.0 0 0 ? S< 16:25 0:00 [slot(245)]
root 45879 0.0 0.0 4416 2304 pts/0 S+ 16:25 0:00 grep slot
Thanks!
Yijing
>> +
>> + len = strlen(slot_name(slot)) + 16;
>> + buffer = kzalloc(len, GFP_KERNEL);
>> + if (!buffer) {
>> + retval = -ENOMEM;
>> + goto out;
>> + }
>> + snprintf(buffer, len, "pciehp_slot(%s)", slot_name(slot));
>> + slot->wq = create_singlethread_workqueue(buffer);
>> + if (!slot->wq)
>> + retval = -ENOMEM;
>> +
>> + kfree(buffer);
>> out:
>> if (retval) {
>> kfree(ops);
>
> Regards,
> Kenji Kaneshige
>
>
> .
>
--
Thanks!
Yijing
--
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
