[PATCH] net: 6pack: Fix tx timeout and slot time

[PATCH] net: 6pack: Fix tx timeout and slot time

[Guenter Roeck]

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(-)

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 */
 
-- 
2.33.0

Re: [PATCH] net: 6pack: Fix tx timeout and slot time

[David Miller]

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.

Re: [PATCH] net: 6pack: Fix tx timeout and slot time

[Guenter Roeck]

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

Re: [PATCH] net: 6pack: Fix tx timeout and slot time

[David Miller]

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.

Re: [PATCH] net: 6pack: Fix tx timeout and slot time

[Guenter Roeck]

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