Skip to content

ntb_hw_switchtec: Make partition index larger than 31 workable#67

Merged
wesleywesley merged 1 commit intodevelfrom
fix_ffs_for_64bit
May 16, 2019
Merged

ntb_hw_switchtec: Make partition index larger than 31 workable#67
wesleywesley merged 1 commit intodevelfrom
fix_ffs_for_64bit

Conversation

@wesleywesley
Copy link
Copy Markdown
Contributor

Switchtec could support as mush as 48 partitions, but ffs & fls are
for 32 bit argument, in case of partition index larger than 31, the
current code could not parse the peer partition index correctly.

Change to the 64 bit version __ffs & __fls accordlingly to fix this
bug.

Signed-off-by: Wesley Sheng wesley.sheng@microchip.com

Copy link
Copy Markdown
Collaborator

@lsgunth lsgunth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good, but you should be using __ffs64(), not __ffs(). The latter will still be 32bits on non-64bit platforms.

The title of the commit should reflect that it's fixing a bug. "Make something workable" is very awkward english.

"Fix bug with more than 32 partitions caused by integer ffs"

Or something like that.

@wesleywesley
Copy link
Copy Markdown
Contributor Author

@lsgunth
Thx for comments.

I found the ffs/fls are defined on /include/asm-generic/bitops
ffs/fls are only for 32 bits input; 0 is an acceptable input; fls(0) = 0, fls(1) = 1, fls(0x80000000) = 32
fls64 are same as fls, but for 64-bit. on non-64bit platforms it still work correctly.

__ffs/__fls
With different to its 32bit version on:
undefined if no bit exists, so code should chec against 0 first.
And the output are from 0 to 63/31, instead of the 1 to 64/32.
Yes, but they are still 32bit on non-64bit platforms. (Only work for 64bit when "BITS_PER_LONG == 64
")

__ffs64 was defined on /include/linux/bitops.h
On 64 bit arches this is a synomyn for __ffs and the result is not defined if no bits are set.
Will change the patch from __ffs to __ffs64.

One question:
__fls is also exist the issue: still be 32bits on non-64bit platforms.
Should it change to fls64, but the code is weird for both __ffs64 and fls64 coexist, they have different on the least(0 or 1)/most(63 or 64) significant bit.

Regard,
Wesley

@kelvin-cao
Copy link
Copy Markdown
Collaborator

kelvin-cao commented May 13, 2019

We can have a helper function like this to make input zero valid.

static inline int ffs_64(u64 word)
{
       int result = __ffs64(word) + 1;
       return word ? result : 0;
}

And for the fls function, we can make another similar function fls_64() with u64 data as the input (refer to __ffs64() in the kernel).

@lsgunth
Copy link
Copy Markdown
Collaborator

lsgunth commented May 13, 2019

If you make helper functions such as those, it's going to be a pain to upstream as we'd really need to do it correctly and put them in the common code.

Just use the ones that are available and their associated semantics. __ffs64() and fls64().

@wesleywesley wesleywesley requested a review from lsgunth May 15, 2019 02:41
Switchtec could support as mush as 48 partitions, but ffs & fls are
for 32 bit argument, in case of partition index larger than 31, the
current code could not parse the peer partition index correctly.
Change to the 64 bit version __ffs64 & fls64 accordingly to fix this
bug.

Signed-off-by: Wesley Sheng <wesley.sheng@microchip.com>
Copy link
Copy Markdown
Collaborator

@lsgunth lsgunth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@wesleywesley wesleywesley merged commit 1edd82f into devel May 16, 2019
@kelvin-cao kelvin-cao deleted the fix_ffs_for_64bit branch August 12, 2020 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants