| Message ID | 20221018202734.140489-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: rt2x00: use explicitly signed type for clamping | expand |
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
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c index cbbb1a4849cf..e233ef9892b3 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c +++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c @@ -4035,23 +4035,23 @@ static void rt2800_iq_calibrate(struct rt2x00_dev *rt2x00dev, int channel) rt2800_bbp_write(rt2x00dev, 159, cal != 0xff ? cal : 0); } -static char rt2800_txpower_to_dev(struct rt2x00_dev *rt2x00dev, +static s8 rt2800_txpower_to_dev(struct rt2x00_dev *rt2x00dev, unsigned int channel, - char txpower) + s8 txpower) { if (rt2x00_rt(rt2x00dev, RT3593) || rt2x00_rt(rt2x00dev, RT3883)) txpower = rt2x00_get_field8(txpower, EEPROM_TXPOWER_ALC); if (channel <= 14) - return clamp_t(char, txpower, MIN_G_TXPOWER, MAX_G_TXPOWER); + return clamp_t(s8, txpower, MIN_G_TXPOWER, MAX_G_TXPOWER); if (rt2x00_rt(rt2x00dev, RT3593) || rt2x00_rt(rt2x00dev, RT3883)) - return clamp_t(char, txpower, MIN_A_TXPOWER_3593, + return clamp_t(s8, txpower, MIN_A_TXPOWER_3593, MAX_A_TXPOWER_3593); else - return clamp_t(char, txpower, MIN_A_TXPOWER, MAX_A_TXPOWER); + return clamp_t(s8, txpower, MIN_A_TXPOWER, MAX_A_TXPOWER); } static void rt3883_bbp_adjust(struct rt2x00_dev *rt2x00dev,
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(-)