wifi: rt2x00: use explicitly signed type for clamping

  On some platforms, `char` is unsigned, which makes casting -7 to char
overflow, which in turn makes the clamping operation bogus. Instead,
deal with an explicit `s8` type, so that the comparison is always
signed, and return an s8 result from the function as well. Note that
this function's result is assigned to a `short`, which is always signed.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Stanislaw Gruszka <stf_xl@wp.pl>
Cc: Helmut Schaa <helmut.schaa@googlemail.com>
Cc: Kalle Valo <kvalo@kernel.org>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
  

On Tue, Oct 18, 2022 at 02:27:34PM -0600, Jason A. Donenfeld wrote:
> On some platforms, `char` is unsigned, which makes casting -7 to char
> overflow, which in turn makes the clamping operation bogus. Instead,
> deal with an explicit `s8` type, so that the comparison is always
> signed, and return an s8 result from the function as well. Note that
> this function's result is assigned to a `short`, which is always signed.

Why not to use short? See my patch I just sent.
  

On Tue, Oct 18, 2022 at 11:40:54PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 18, 2022 at 02:27:34PM -0600, Jason A. Donenfeld wrote:
> > On some platforms, `char` is unsigned, which makes casting -7 to char
> > overflow, which in turn makes the clamping operation bogus. Instead,
> > deal with an explicit `s8` type, so that the comparison is always
> > signed, and return an s8 result from the function as well. Note that
> > this function's result is assigned to a `short`, which is always signed.
> 
> Why not to use short? See my patch I just sent.

Trying to have the most minimal change here that doesn't rock the boat.
I'm not out to rewrite the driver. I don't know the original author's
rationales. This patch here is correct and will generate the same code
as before on architectures where it wasn't broken.

However, if you want your "change the codegen" patch to be taken
seriously, you should probably send it to the wireless maintainers like
this one, and they can decide. Personally, I don't really care either
way.

Jason
  

On Tue, Oct 18, 2022 at 02:52:43PM -0600, Jason A. Donenfeld wrote:
> On Tue, Oct 18, 2022 at 11:40:54PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 18, 2022 at 02:27:34PM -0600, Jason A. Donenfeld wrote:
> > > On some platforms, `char` is unsigned, which makes casting -7 to char
> > > overflow, which in turn makes the clamping operation bogus. Instead,
> > > deal with an explicit `s8` type, so that the comparison is always
> > > signed, and return an s8 result from the function as well. Note that
> > > this function's result is assigned to a `short`, which is always signed.
> > 
> > Why not to use short? See my patch I just sent.
> 
> Trying to have the most minimal change here that doesn't rock the boat.
> I'm not out to rewrite the driver. I don't know the original author's
> rationales. This patch here is correct and will generate the same code
> as before on architectures where it wasn't broken.
> 
> However, if you want your "change the codegen" patch to be taken
> seriously, you should probably send it to the wireless maintainers like
> this one, and they can decide. Personally, I don't really care either
> way.

I have checked the code paths there and I found no evidence that short can't be
used. That's why my patch.

Okay, I will formally send it to the corresponding maintainers.

But if they want, they can always download this thread using `b4` tool and at
least comment on it.
  

On Tue, Oct 18, 2022 at 2:57 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Oct 18, 2022 at 02:52:43PM -0600, Jason A. Donenfeld wrote:
> > On Tue, Oct 18, 2022 at 11:40:54PM +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 18, 2022 at 02:27:34PM -0600, Jason A. Donenfeld wrote:
> > > > On some platforms, `char` is unsigned, which makes casting -7 to char
> > > > overflow, which in turn makes the clamping operation bogus. Instead,
> > > > deal with an explicit `s8` type, so that the comparison is always
> > > > signed, and return an s8 result from the function as well. Note that
> > > > this function's result is assigned to a `short`, which is always signed.
> > >
> > > Why not to use short? See my patch I just sent.
> >
> > Trying to have the most minimal change here that doesn't rock the boat.
> > I'm not out to rewrite the driver. I don't know the original author's
> > rationales. This patch here is correct and will generate the same code
> > as before on architectures where it wasn't broken.
> >
> > However, if you want your "change the codegen" patch to be taken
> > seriously, you should probably send it to the wireless maintainers like
> > this one, and they can decide. Personally, I don't really care either
> > way.
>
> I have checked the code paths there and I found no evidence that short can't be
> used. That's why my patch.

Do you have a rationale why you want to change codegen?

Jason
  

On Tue, 18 Oct 2022 14:27:34 -0600 "Jason A. Donenfeld" <Jason@zx2c4.com> wrote:

> On some platforms, `char` is unsigned, which makes casting -7 to char
> overflow, which in turn makes the clamping operation bogus. Instead,
> deal with an explicit `s8` type, so that the comparison is always
> signed, and return an s8 result from the function as well. Note that
> this function's result is assigned to a `short`, which is always signed.

Thanks.  I'll grab this for now to make -next happier.  Stephen will
tell us when the patch (or one like it) appears via the wireless tree.
  

On Tue, Oct 18, 2022 at 02:58:30PM -0600, Jason A. Donenfeld wrote:
> On Tue, Oct 18, 2022 at 2:57 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Oct 18, 2022 at 02:52:43PM -0600, Jason A. Donenfeld wrote:
> > > On Tue, Oct 18, 2022 at 11:40:54PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Oct 18, 2022 at 02:27:34PM -0600, Jason A. Donenfeld wrote:
> > > > > On some platforms, `char` is unsigned, which makes casting -7 to char
> > > > > overflow, which in turn makes the clamping operation bogus. Instead,
> > > > > deal with an explicit `s8` type, so that the comparison is always
> > > > > signed, and return an s8 result from the function as well. Note that
> > > > > this function's result is assigned to a `short`, which is always signed.
> > > >
> > > > Why not to use short? See my patch I just sent.
> > >
> > > Trying to have the most minimal change here that doesn't rock the boat.
> > > I'm not out to rewrite the driver. I don't know the original author's
> > > rationales. This patch here is correct and will generate the same code
> > > as before on architectures where it wasn't broken.
> > >
> > > However, if you want your "change the codegen" patch to be taken
> > > seriously, you should probably send it to the wireless maintainers like
> > > this one, and they can decide. Personally, I don't really care either
> > > way.
> >
> > I have checked the code paths there and I found no evidence that short can't be
> > used. That's why my patch.
> 
> Do you have a rationale why you want to change codegen?

It's not a hot path as far as I understand and keeping data types aligned seems
to me worth it even if codegen is changed. IS it so awful with short?
  

From: Andy Shevchenko
> Sent: 18 October 2022 22:10
....
> It's not a hot path as far as I understand and keeping data types aligned seems
> to me worth it even if codegen is changed. IS it so awful with short?

(without looking at the generated code)
Why is it even short, what is wrong with int?
It is extremely unlikely that the code requires any
calculation results be masked to 8 (or 16) bits, but
using s8 or s16 requires the compiler emit code to mask
any calculated values.

Remember, all C arithmetic requires promoting the values
to (at least) int.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On Tue, Oct 18, 2022 at 02:27:34PM -0600, Jason A. Donenfeld wrote:
> On some platforms, `char` is unsigned, which makes casting -7 to char
> overflow, which in turn makes the clamping operation bogus. Instead,
> deal with an explicit `s8` type, so that the comparison is always
> signed, and return an s8 result from the function as well. Note that
> this function's result is assigned to a `short`, which is always signed.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Stanislaw Gruszka <stf_xl@wp.pl>
> Cc: Helmut Schaa <helmut.schaa@googlemail.com>
> Cc: Kalle Valo <kvalo@kernel.org>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

I prefer s8 just because is shorter name than short :-)

Acked-by: Stanislaw Gruszka <stf_xl@wp.pl>
  

On Wed, Oct 19, 2022 at 10:52:19AM +0200, Stanislaw Gruszka wrote:
> On Tue, Oct 18, 2022 at 02:27:34PM -0600, Jason A. Donenfeld wrote:
> > On some platforms, `char` is unsigned, which makes casting -7 to char
> > overflow, which in turn makes the clamping operation bogus. Instead,
> > deal with an explicit `s8` type, so that the comparison is always
> > signed, and return an s8 result from the function as well. Note that
> > this function's result is assigned to a `short`, which is always signed.
> > 
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Stanislaw Gruszka <stf_xl@wp.pl>
> > Cc: Helmut Schaa <helmut.schaa@googlemail.com>
> > Cc: Kalle Valo <kvalo@kernel.org>
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> 
> I prefer s8 just because is shorter name than short :-)

Shouldn't the corresponding data structure type be fixed accordingly?
  

On Wed, Oct 19, 2022 at 02:04:57PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 19, 2022 at 10:52:19AM +0200, Stanislaw Gruszka wrote:
> > On Tue, Oct 18, 2022 at 02:27:34PM -0600, Jason A. Donenfeld wrote:
> > > On some platforms, `char` is unsigned, which makes casting -7 to char
> > > overflow, which in turn makes the clamping operation bogus. Instead,
> > > deal with an explicit `s8` type, so that the comparison is always
> > > signed, and return an s8 result from the function as well. Note that
> > > this function's result is assigned to a `short`, which is always signed.
> > > 
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Cc: Stanislaw Gruszka <stf_xl@wp.pl>
> > > Cc: Helmut Schaa <helmut.schaa@googlemail.com>
> > > Cc: Kalle Valo <kvalo@kernel.org>
> > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > 
> > I prefer s8 just because is shorter name than short :-)
> 
> Shouldn't the corresponding data structure type be fixed accordingly?

We can change types of channel_info default_power* fields in rt2x00.h,
but I'm a bit reluctant to do so, as I'm afraid this could change
actual power values sent to the hardware and will require careful
verification.

Regards
Stanislaw