Messages in this thread | | | From | Guenter Roeck <> | Subject | Re: [PATCH] net: 6pack: Fix tx timeout and slot time | Date | Thu, 9 Sep 2021 09:44:03 -0700 |
| |
On 9/9/21 8:27 AM, David Miller wrote: > From: Guenter Roeck <linux@roeck-us.net> > Date: Thu, 9 Sep 2021 07:53:29 -0700 > >> On 9/9/21 4:34 AM, David Miller wrote: >>> From: Guenter Roeck <linux@roeck-us.net> >>> Date: Wed, 8 Sep 2021 20:57:43 -0700 >>> >>>> tx timeout and slot time are currently specified in units of HZ. >>>> On Alpha, HZ is defined as 1024. When building alpha:allmodconfig, >>>> this results in the following error message. >>>> >>>> drivers/net/hamradio/6pack.c: In function 'sixpack_open': >>>> drivers/net/hamradio/6pack.c:71:41: error: >>>> unsigned conversion from 'int' to 'unsigned char' >>>> changes value from '256' to '0' >>>> >>>> In the 6PACK protocol, tx timeout is specified in units of 10 ms >>>> and transmitted over the wire. Defining a value dependent on HZ >>>> doesn't really make sense. Assume that the intent was to set tx >>>> timeout and slot time based on a HZ value of 100 and use constants >>>> instead of values depending on HZ for SIXP_TXDELAY and SIXP_SLOTTIME. >>>> >>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>>> --- >>>> No idea if this is correct or even makes sense. Compile tested only. >>> These are timer offsets so they have to me HZ based. Better to make >>> the >>> structure members unsigned long, I think. >>> >> >> Hmm, ok. Both tx_delay and slottime are updated in sp_encaps(), >> though, >> from data in the transmit buffer. The KISS protocol description states >> that the values are in units of 10ms; that is where my assumption >> came from. > > They are ms and must be converted using to HZ in order to use the values as timer offsets. > > The values are perfectly fine, the types used to store them need to be fixed. > >> Anyway, I am inclined to just mark the protocol as dependent on >> !ALPHA. Would you accept that ? > > No, fix this properly. Make the unsigfned char members be unsigned long. > > Why do you not want to fix it this way? >
All I want is to get alpha:allmodconfig to compile, nothing else, but at the same time I don't want to introduce new bugs.
If I make tx_delay unsigned long, it is still passed to encode_sixpack() as parameter, and that parameter is declared unsigned char and put into a byte sized buffer. If the value is 0x100, it will then be truncated to 0x0, only that is then done silently without generating a compile error. Sure, that fixes the compile error, but not the underlying issue. Maybe that truncation is perfectly fine, but I don't understand the code well enough to make that call.
Never mind, I'll let someone else who has a better understanding of the code deal with this.
Guenter
| |