search

berarma / new-lg4ff

Experimental Logitech force feedback module for Linux
GNU General Public License v2.0
298 stars 18 forks source link

Fix regressions in 32bit systems #97

Open berarma opened 3 weeks ago

berarma commented 3 weeks ago

See #96.

zakk4223 commented 3 weeks ago

That seems to work on the arm.

However, what's the best way to test if it broke this: 2b865afe016b1a25a7a0ff921f94f81f3d1c2969 ? Seems like the same bug will arise

berarma commented 3 weeks ago

I don't understand the question. If this PR works, I guess the issue will be multiplying unsigned with signed values.

You can write a small test program doing the multiplication and showing the reult combining different types, signed and unsigned.

zakk4223 commented 3 weeks ago

I'm saying that previously, you'd fixed 32-bit issues by casting both of those values to (int). Then you changed it in the commit I referenced to only cast the parameters[0].level to (long). I assume you did this because long is wider than int, but that's not the case on 32bit arm, they are the same size.

Casting to s32 is no different than casting to int, so whatever bug you fixed with the change to long cast will be reintroduced with this change.

berarma commented 3 weeks ago

I'm saying that previously, you'd fixed 32-bit issues by casting both of those values to (int). Then you changed it in the commit I referenced to only cast the parameters[0].level to (long). I assume you did this because long is wider than int, but that's not the case on 32bit arm, they are the same size.

Casting to s32 is no different than casting to int, so whatever bug you fixed with the change to long cast will be reintroduced with this change.

Aha, I get it. My intention when casting to a long was to be sure that at least a 32bit type was used. In retrospect, this wasn't the best way to do it since long can be different sizes on different architectures. The macro s32 is safer because it guarantees the cast to a 32bit type.

Although most probably this wasn't the issue. Instead multiplying a 32bit signed value with a 32bit unsigned value and storing it in a 32bit signed variable was. I could have casted both to long to the same effect, but I'd rather use s32 this time.

I'll test this PR on my 64bit system, and if it works correctly here and in your 32bit system, I think it's a safer cast than it was.

berarma commented 2 weeks ago

This isn't working well for me on a 64bits system. I'll take a closer look and see if there's really a need for 64bits types for this calculation.

berarma commented 2 weeks ago

I've reviewed the code and a 64bit cast is definitely needed. One question though about the proposed solution in https://github.com/berarma/new-lg4ff/issues/96#issuecomment-2155839482, why use div_s64 instead of relying on the compiler to do the division?

zakk4223 commented 2 weeks ago

Compiler will detect 64-bit divide on 32-bit arch and insert a helper function (for gcc it comes from libgcc). Kernel isn't linked with the compiler library so you will get a link error.

berarma commented 2 weeks ago

I think I've finally nailed it down. It was a case of me badly fixing 32bits support, then breaking it again trying to fix it properly.

Your solution of using 64bits operations was good I guess, as it correctly pointed to the real problem, but I've preferred to avoid using 64bits arithmetic for performance reasons. I've scaled down the gain from 16bits to 8bits so that it all fits into 32bits integers. It shouldn't matter since the hardware only has 255 force levels anyway.

If you could test it before merging it so I'm sure this is really fixed now. I've added some comments so hopefully this doesn't bite me again.

Thank you for your help.

zakk4223 commented 1 week ago

The (s64) cast (line 886 to 888) will still cause the compiler to generate a 64bit divide. Linking fails with unknown symbol due to the ldivmod inserted by the compiler.

berarma commented 1 week ago

I'll need to do more tests. Apparently, this isn't enough to make it work with 32bits operations.

I'll be away from the wheel for most of the summer so it might take a while before I can do more tests. Please, use your own fix in the mean time.