Re: [PATCH 06/23] PCI: rescan with bus or bridge using callbackmethod too

[Bjorn Helgaas]

On Thu, Mar 8, 2012 at 11:53 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
> On Thu, Mar 8, 2012 at 4:56 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
>> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
>>> Just like removal.
>>>
>>> Because We could add new bus under the bridges...
>>>
>>> Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
>>> ---
>>>  drivers/pci/pci-sysfs.c |   43 +++++++++++++++++++++++++++++++------------
>>>  1 files changed, 31 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>>> index 2049b2f..1794508 100644
>>> --- a/drivers/pci/pci-sysfs.c
>>> +++ b/drivers/pci/pci-sysfs.c
>>> @@ -325,21 +325,31 @@ dev_rescan_store(struct device *dev, struct device_attribute *attr,
>>>        return count;
>>>  }
>>>
>>> +static void bridge_rescan_callback(struct device *dev)
>>> +{
>>> +       struct pci_dev *pdev = to_pci_dev(dev);
>>> +
>>> +       mutex_lock(&pci_remove_rescan_mutex);
>>> +       pci_rescan_bus_bridge_resize(pdev);
>>> +       mutex_unlock(&pci_remove_rescan_mutex);
>>> +}
>>> +
>>>  static ssize_t
>>>  dev_bridge_rescan_store(struct device *dev, struct device_attribute *attr,
>>>                 const char *buf, size_t count)
>>>  {
>>> +       int ret = 0;
>>>        unsigned long val;
>>> -       struct pci_dev *pdev = to_pci_dev(dev);
>>>
>>>        if (kstrtoul(buf, 0, &val) < 0)
>>>                return -EINVAL;
>>>
>>> -       if (val) {
>>> -               mutex_lock(&pci_remove_rescan_mutex);
>>> -               pci_rescan_bus_bridge_resize(pdev);
>>> -               mutex_unlock(&pci_remove_rescan_mutex);
>>> -       }
>>> +       if (val)
>>> +               ret = device_schedule_callback(dev, bridge_rescan_callback);
>>> +
>>> +       if (ret)
>>> +               count = ret;
>>> +
>>>        return count;
>>
>> I see that you copied this style from remove_store(), but it's ugly
>> and hard to follow.  I think this would be more readable:
>>
>>    if (!val)
>>        return count;
>>
>>    err = device_schedule_callback(dev, bridge_rescan_callback);
>>    if (err)
>>        return err;
>>
>>    return count;
>>
>
> like
>
>        if (!val)
>                return count;
>
>        ret = device_schedule_callback(dev, bus_rescan_callback);
>
>        if (ret)
>                return ret;
>
>        return count;
>
> that will have three return instead of one.
>
> both should be ok.

Why did you just repeat my proposal, changing "err" to "ret"?  I
missed the point you're trying to make.  The name "err" gives an
important clue that device_schedule_callback() returns 0 or errno.
That's essential to understanding the code.

Your original patch has one "return."  Mine proposal has three.  All
that means is that the reader of your patch has to manage more stuff
in her head to understand the code.  That's not a benefit!

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/