linux-kernel - Re: [PATCH 06/23] PCI: rescan with bus or bridge using callback method too

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 9 Mar 2012 10:22:11 -0700
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Yinghai Lu <yinghai@...nel.org>
Cc:	Jesse Barnes <jbarnes@...tuousgeek.org>, x86 <x86@...nel.org>,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 06/23] PCI: rescan with bus or bridge using callback
 method too

On Thu, Mar 8, 2012 at 11:53 PM, Yinghai Lu <yinghai@...nel.org> wrote:
> On Thu, Mar 8, 2012 at 4:56 PM, Bjorn Helgaas <bhelgaas@...gle.com> wrote:
>> On Tue, Mar 6, 2012 at 12:13 AM, Yinghai Lu <yinghai@...nel.org> wrote:
>>> Just like removal.
>>>
>>> Because We could add new bus under the bridges...
>>>
>>> Signed-off-by: Yinghai Lu <yinghai@...nel.org>
>>> ---
>>>  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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/