| Message ID | 20210909035743.1247042-1-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: 6pack: Fix tx timeout and slot time | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 11 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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. Thanks.
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. Command Function Comments 0 Data frame The rest of the frame is data to be sent on the HDLC channel. 1 TXDELAY The next byte is the transmitter keyup delay in 10 ms units. The default start-up value is 50 (i.e., 500 ms). 2 P The next byte is the persistence parameter, p, scaled to the range 0 - 255 with the following formula: P = p * 256 - 1 The default value is P = 63 (i.e., p = 0.25). 3 SlotTime The next byte is the slot interval in 10 ms units. The default is 10 (i.e., 100ms). But then slottime is indeed used with jiffies, which is odd. tx_delay is used (before it is updated) in encode_sixpack() and added to a character buffer. I thought that was the value sent on the wire (which would again be supposed to be in 10ms units). I don't see where else it is used, but I may be missing it. That means though it can not easily be changed to anything but unsigned char. Anyway, I am inclined to just mark the protocol as dependent on !ALPHA. Would you accept that ? Thanks, Guenter
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? Thank you.
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
diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c index 8fe8887d506a..6192244b304a 100644 --- a/drivers/net/hamradio/6pack.c +++ b/drivers/net/hamradio/6pack.c @@ -68,9 +68,9 @@ #define SIXP_DAMA_OFF 0 /* default level 2 parameters */ -#define SIXP_TXDELAY (HZ/4) /* in 1 s */ +#define SIXP_TXDELAY 25 /* 250 ms */ #define SIXP_PERSIST 50 /* in 256ths */ -#define SIXP_SLOTTIME (HZ/10) /* in 1 s */ +#define SIXP_SLOTTIME 10 /* 100 ms */ #define SIXP_INIT_RESYNC_TIMEOUT (3*HZ/2) /* in 1 s */ #define SIXP_RESYNC_TIMEOUT 5*HZ /* in 1 s */
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. drivers/net/hamradio/6pack.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)