| Message ID | 20230925230829.341-3-beaub@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tracing/user_events: Fix alignment issues for 32 on 64-bit and BE | expand |
Note, this doesn't seem to apply to my tree so I only added the first patch. I think this needs to go through Shuah's tree. -- Steve On Mon, 25 Sep 2023 23:08:29 +0000 Beau Belgrave <beaub@linux.microsoft.com> wrote: > The abi_test currently uses a long sized test value for enablement > checks. On LE this works fine, however, on BE this results in inaccurate > assert checks due to a bit being used and assuming it's value is the > same on both LE and BE. > > Use int type for 32-bit values and long type for 64-bit values to ensure > appropriate behavior on both LE and BE. > > Fixes: 60b1af8de8c1 ("tracing/user_events: Add ABI self-test") > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com> > --- > tools/testing/selftests/user_events/abi_test.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/tools/testing/selftests/user_events/abi_test.c b/tools/testing/selftests/user_events/abi_test.c > index 5125c42efe65..67af4c491c0c 100644 > --- a/tools/testing/selftests/user_events/abi_test.c > +++ b/tools/testing/selftests/user_events/abi_test.c > @@ -46,7 +46,7 @@ static int change_event(bool enable) > return ret; > } > > -static int reg_enable(long *enable, int size, int bit) > +static int reg_enable(void *enable, int size, int bit) > { > struct user_reg reg = {0}; > int fd = open(data_file, O_RDWR); > @@ -68,7 +68,7 @@ static int reg_enable(long *enable, int size, int bit) > return ret; > } > > -static int reg_disable(long *enable, int bit) > +static int reg_disable(void *enable, int bit) > { > struct user_unreg reg = {0}; > int fd = open(data_file, O_RDWR); > @@ -89,12 +89,14 @@ static int reg_disable(long *enable, int bit) > } > > FIXTURE(user) { > - long check; > + int check; > + long check_long; > }; > > FIXTURE_SETUP(user) { > change_event(false); > self->check = 0; > + self->check_long = 0; > } > > FIXTURE_TEARDOWN(user) { > @@ -131,9 +133,9 @@ TEST_F(user, bit_sizes) { > > #if BITS_PER_LONG == 8 > /* Allow 0-64 bits for 64-bit */ > - ASSERT_EQ(0, reg_enable(&self->check, sizeof(long), 63)); > - ASSERT_NE(0, reg_enable(&self->check, sizeof(long), 64)); > - ASSERT_EQ(0, reg_disable(&self->check, 63)); > + ASSERT_EQ(0, reg_enable(&self->check_long, sizeof(long), 63)); > + ASSERT_NE(0, reg_enable(&self->check_long, sizeof(long), 64)); > + ASSERT_EQ(0, reg_disable(&self->check_long, 63)); > #endif > > /* Disallowed sizes (everything beside 4 and 8) */ > @@ -195,7 +197,7 @@ static int clone_check(void *check) > for (i = 0; i < 10; ++i) { > usleep(100000); > > - if (*(long *)check) > + if (*(int *)check) > return 0; > } >
On 10/3/23 18:59, Steven Rostedt wrote: > > Note, this doesn't seem to apply to my tree so I only added the first > patch. I think this needs to go through Shuah's tree. > > -- Steve > > Yes. I sent a fix up for rc4 - I can pull these two patches into linux-kselftest next Steve! Does that work for you? thanks, -- Shuah
On Wed, 4 Oct 2023 09:10:52 -0600 Shuah Khan <skhan@linuxfoundation.org> wrote: > On 10/3/23 18:59, Steven Rostedt wrote: > > > > Note, this doesn't seem to apply to my tree so I only added the first > > patch. I think this needs to go through Shuah's tree. > > > > -- Steve > > > > > > Yes. I sent a fix up for rc4 - I can pull these two patches into > linux-kselftest next > > Steve! Does that work for you? > I applied the first patch to my tree, I think the second patch is fine to go separately through your tree. -- Steve
On 10/4/23 09:14, Steven Rostedt wrote: > On Wed, 4 Oct 2023 09:10:52 -0600 > Shuah Khan <skhan@linuxfoundation.org> wrote: > >> On 10/3/23 18:59, Steven Rostedt wrote: >>> >>> Note, this doesn't seem to apply to my tree so I only added the first >>> patch. I think this needs to go through Shuah's tree. >>> >>> -- Steve >>> >>> >> >> Yes. I sent a fix up for rc4 - I can pull these two patches into >> linux-kselftest next >> >> Steve! Does that work for you? >> > > I applied the first patch to my tree, I think the second patch is fine to go > separately through your tree. > Yes I will apply this to linux-kselftest fixes branch once my PR clears. thanks, -- Shuah
On 10/4/23 10:38, Shuah Khan wrote: > On 10/4/23 09:14, Steven Rostedt wrote: >> On Wed, 4 Oct 2023 09:10:52 -0600 >> Shuah Khan <skhan@linuxfoundation.org> wrote: >> >>> On 10/3/23 18:59, Steven Rostedt wrote: >>>> >>>> Note, this doesn't seem to apply to my tree so I only added the first >>>> patch. I think this needs to go through Shuah's tree. >>>> >>>> -- Steve >>>> >>> >>> Yes. I sent a fix up for rc4 - I can pull these two patches into >>> linux-kselftest next >>> >>> Steve! Does that work for you? >>> >> >> I applied the first patch to my tree, I think the second patch is fine to go >> separately through your tree. >> > > > Yes I will apply this to linux-kselftest fixes branch once my PR > clears. > Hmm. Which tree is this patch based on? This doesn't apply to linux-kselftest fixes - I thought this was based on top of fixes since I sent in a fix for Linux 6.6-rc4 for user_events Beau, Please rebase to the correct tree/branch and send v2 for this patch. thanks, -- Shuah
On Thu, 5 Oct 2023 08:48:14 -0600 Shuah Khan <skhan@linuxfoundation.org> wrote: > Hmm. Which tree is this patch based on? This doesn't apply to > linux-kselftest fixes - I thought this was based on top of fixes > since I sent in a fix for Linux 6.6-rc4 for user_events > > Beau, Please rebase to the correct tree/branch and send v2 for > this patch. Hmm, so this didn't apply to my tree nor yours. Beau, can you verify which tree this goes to? -- Steve
On Thu, Oct 05, 2023 at 11:08:15AM -0400, Steven Rostedt wrote: > On Thu, 5 Oct 2023 08:48:14 -0600 > Shuah Khan <skhan@linuxfoundation.org> wrote: > > > Hmm. Which tree is this patch based on? This doesn't apply to > > linux-kselftest fixes - I thought this was based on top of fixes > > since I sent in a fix for Linux 6.6-rc4 for user_events > > > > Beau, Please rebase to the correct tree/branch and send v2 for > > this patch. > > Hmm, so this didn't apply to my tree nor yours. > > Beau, can you verify which tree this goes to? > > -- Steve It was based on tracing/for-next. I'll get a v2 out rebased upon linux-kselftest, does that work? Thanks, -Beau
On Thu, 5 Oct 2023 09:52:30 -0700 Beau Belgrave <beaub@linux.microsoft.com> wrote: > It was based on tracing/for-next. > > I'll get a v2 out rebased upon linux-kselftest, does that work? Hmm, then it should have applied to my tree. I didn't look too deep. Can you see if it applies to: git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git trace/for-next ? Thanks, -- Steve
diff --git a/tools/testing/selftests/user_events/abi_test.c b/tools/testing/selftests/user_events/abi_test.c index 5125c42efe65..67af4c491c0c 100644 --- a/tools/testing/selftests/user_events/abi_test.c +++ b/tools/testing/selftests/user_events/abi_test.c @@ -46,7 +46,7 @@ static int change_event(bool enable) return ret; } -static int reg_enable(long *enable, int size, int bit) +static int reg_enable(void *enable, int size, int bit) { struct user_reg reg = {0}; int fd = open(data_file, O_RDWR); @@ -68,7 +68,7 @@ static int reg_enable(long *enable, int size, int bit) return ret; } -static int reg_disable(long *enable, int bit) +static int reg_disable(void *enable, int bit) { struct user_unreg reg = {0}; int fd = open(data_file, O_RDWR); @@ -89,12 +89,14 @@ static int reg_disable(long *enable, int bit) } FIXTURE(user) { - long check; + int check; + long check_long; }; FIXTURE_SETUP(user) { change_event(false); self->check = 0; + self->check_long = 0; } FIXTURE_TEARDOWN(user) { @@ -131,9 +133,9 @@ TEST_F(user, bit_sizes) { #if BITS_PER_LONG == 8 /* Allow 0-64 bits for 64-bit */ - ASSERT_EQ(0, reg_enable(&self->check, sizeof(long), 63)); - ASSERT_NE(0, reg_enable(&self->check, sizeof(long), 64)); - ASSERT_EQ(0, reg_disable(&self->check, 63)); + ASSERT_EQ(0, reg_enable(&self->check_long, sizeof(long), 63)); + ASSERT_NE(0, reg_enable(&self->check_long, sizeof(long), 64)); + ASSERT_EQ(0, reg_disable(&self->check_long, 63)); #endif /* Disallowed sizes (everything beside 4 and 8) */ @@ -195,7 +197,7 @@ static int clone_check(void *check) for (i = 0; i < 10; ++i) { usleep(100000); - if (*(long *)check) + if (*(int *)check) return 0; }
The abi_test currently uses a long sized test value for enablement checks. On LE this works fine, however, on BE this results in inaccurate assert checks due to a bit being used and assuming it's value is the same on both LE and BE. Use int type for 32-bit values and long type for 64-bit values to ensure appropriate behavior on both LE and BE. Fixes: 60b1af8de8c1 ("tracing/user_events: Add ABI self-test") Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com> --- tools/testing/selftests/user_events/abi_test.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)