On Tue, Jul 14, 2015 at 09:11:54PM -0500, Bjorn Helgaas wrote:
>On Tue, Jul 14, 2015 at 9:02 PM, Wei Yang <weiyang@xxxxxxxxxxxxxxxxxx> wrote:
>> On Tue, Jul 14, 2015 at 05:00:21PM -0500, Bjorn Helgaas wrote:
>>>On Tue, Jun 30, 2015 at 09:16:44AM +0800, Wei Yang wrote:
>>>> The return value of the pci_find_(ext_)capability is the position of this
>>>> Cap. After previous two patches clean up, the position returned is an
>>>> unsigned value. Only 0 indicates the Cap is not presented.
>>>>
>>>> This patch consolidates the form of check from (pos <= 0)to (!pos).
>>>>
>>>> Signed-off-by: Wei Yang <weiyang@xxxxxxxxxxxxxxxxxx>
>>>
>>>Applied to pci/misc with changelog as below.
>>>
>>>It seems pretty clear to me that pci_find_capability() returns either 0 or
>>>a u8 value. pci_find_ext_capability() does return an int. It looks to me
>>>like it can never be negative, but if you wanted it to be even more clear,
>>>you could easily change just pci_find_next_ext_capability() to use a u16
>>>for "pos". That would be very simple and wouldn't change any interfaces.
>>
>> pci_find_capability() will return either 0 or a u8 value, while in the code
>> the return value is an "int" type. So for the first sight, it may not that
>> immediate. The same as pci_find_ext_capability().
>>
>> This is the reason for patch 2/3. The purpose is to make the return type
>> reflect the value it will return.
>>
>> Patch 3 does exactly what you said, use a u16 for "pos" in
>> pci_find_next_ext_capability().
>
>Yes. Patch 3 contains this hunk:
>
>@@ -272,11 +272,11 @@ EXPORT_SYMBOL(pci_bus_find_capability);
> * not support it. Some capabilities can occur several times, e.g., the
> * vendor-specific capability, and this provides a way to find them all.
> */
>-int pci_find_next_ext_capability(struct pci_dev *dev, int start, int cap)
>+u16 pci_find_next_ext_capability(struct pci_dev *dev, int start, u16 cap)
> {
> u32 header;
> int ttl;
>- int pos = PCI_CFG_SPACE_SIZE;
>+ u16 pos = PCI_CFG_SPACE_SIZE;
>
> /* minimum 8 bytes per capability */
> ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
>
>I think the second piece (changing "pos" from int to u16) is enough to
>make it easy to analyze, and it only affects this function, so that
>seems obviously fine.
>
>The first piece (changing the return type) changes the signature of an
>exported interface. That is more work, e.g., it's more of a hassle
>for distros to backport if they want to preserve interfaces, so it's
>not quite as obvious that it's worthwhile.
OK, I got your concern.
>
>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
--
Richard Yang
Help you, Help me
--
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
