This enables you to configure mode (DTE/DCE), Modulo, Window, T1, T2, N2 via
sethdlc (which needs to be patched as well).
Signed-off-by: Martin Schiller <[email protected]>
---
drivers/net/wan/hdlc_x25.c | 67 ++++++++++++++++++++++++++++++++-
include/uapi/linux/hdlc/ioctl.h | 11 +++++-
include/uapi/linux/if.h | 1 +
3 files changed, 76 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index 5643675ff724..b28051eba736 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -21,8 +21,17 @@
#include <linux/skbuff.h>
#include <net/x25device.h>
+struct x25_state {
+ x25_hdlc_proto settings;
+};
+
static int x25_ioctl(struct net_device *dev, struct ifreq *ifr);
+static inline struct x25_state* state(hdlc_device *hdlc)
+{
+ return (struct x25_state *)hdlc->state;
+}
+
/* These functions are callbacks called by LAPB layer */
static void x25_connect_disconnect(struct net_device *dev, int reason, int code)
@@ -132,6 +141,8 @@ static netdev_tx_t x25_xmit(struct sk_buff *skb, struct net_device *dev)
static int x25_open(struct net_device *dev)
{
int result;
+ hdlc_device *hdlc = dev_to_hdlc(dev);
+ struct lapb_parms_struct params;
static const struct lapb_register_struct cb = {
.connect_confirmation = x25_connected,
.connect_indication = x25_connected,
@@ -144,6 +155,26 @@ static int x25_open(struct net_device *dev)
result = lapb_register(dev, &cb);
if (result != LAPB_OK)
return result;
+
+ result = lapb_getparms(dev, ¶ms);
+ if (result != LAPB_OK)
+ return result;
+
+ if (state(hdlc)->settings.dce)
+ params.mode = params.mode | LAPB_DCE;
+
+ if (state(hdlc)->settings.modulo == 128)
+ params.mode = params.mode | LAPB_EXTENDED;
+
+ params.window = state(hdlc)->settings.window;
+ params.t1 = state(hdlc)->settings.t1;
+ params.t2 = state(hdlc)->settings.t2;
+ params.n2 = state(hdlc)->settings.n2;
+
+ result = lapb_setparms(dev, ¶ms);
+ if (result != LAPB_OK)
+ return result;
+
return 0;
}
@@ -186,6 +217,9 @@ static struct hdlc_proto proto = {
static int x25_ioctl(struct net_device *dev, struct ifreq *ifr)
{
+ x25_hdlc_proto __user *x25_s = ifr->ifr_settings.ifs_ifsu.x25;
+ const size_t size = sizeof(x25_hdlc_proto);
+ x25_hdlc_proto new_settings;
hdlc_device *hdlc = dev_to_hdlc(dev);
int result;
@@ -194,7 +228,13 @@ static int x25_ioctl(struct net_device *dev, struct ifreq *ifr)
if (dev_to_hdlc(dev)->proto != &proto)
return -EINVAL;
ifr->ifr_settings.type = IF_PROTO_X25;
- return 0; /* return protocol only, no settable parameters */
+ if (ifr->ifr_settings.size < size) {
+ ifr->ifr_settings.size = size; /* data size wanted */
+ return -ENOBUFS;
+ }
+ if (copy_to_user(x25_s, &state(hdlc)->settings, size))
+ return -EFAULT;
+ return 0;
case IF_PROTO_X25:
if (!capable(CAP_NET_ADMIN))
@@ -203,12 +243,35 @@ static int x25_ioctl(struct net_device *dev, struct ifreq *ifr)
if (dev->flags & IFF_UP)
return -EBUSY;
+ if (copy_from_user(&new_settings, x25_s, size))
+ return -EFAULT;
+
+ if ((new_settings.dce != 0 &&
+ new_settings.dce != 1) ||
+ (new_settings.modulo != 8 &&
+ new_settings.modulo != 128) ||
+ new_settings.window < 1 ||
+ (new_settings.modulo == 8 &&
+ new_settings.window > 7) ||
+ (new_settings.modulo == 128 &&
+ new_settings.window > 127) ||
+ new_settings.t1 < 1 ||
+ new_settings.t1 > 255 ||
+ new_settings.t2 < 1 ||
+ new_settings.t2 > 255 ||
+ new_settings.n2 < 1 ||
+ new_settings.n2 > 255)
+ return -EINVAL;
+
result=hdlc->attach(dev, ENCODING_NRZ,PARITY_CRC16_PR1_CCITT);
if (result)
return result;
- if ((result = attach_hdlc_protocol(dev, &proto, 0)))
+ if ((result = attach_hdlc_protocol(dev, &proto,
+ sizeof(struct x25_state))))
return result;
+
+ memcpy(&state(hdlc)->settings, &new_settings, size);
dev->type = ARPHRD_X25;
call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, dev);
netif_dormant_off(dev);
diff --git a/include/uapi/linux/hdlc/ioctl.h b/include/uapi/linux/hdlc/ioctl.h
index 0fe4238e8246..3656ce8b8af0 100644
--- a/include/uapi/linux/hdlc/ioctl.h
+++ b/include/uapi/linux/hdlc/ioctl.h
@@ -3,7 +3,7 @@
#define __HDLC_IOCTL_H__
-#define GENERIC_HDLC_VERSION 4 /* For synchronization with sethdlc utility */
+#define GENERIC_HDLC_VERSION 5 /* For synchronization with sethdlc utility */
#define CLOCK_DEFAULT 0 /* Default setting */
#define CLOCK_EXT 1 /* External TX and RX clock - DTE */
@@ -79,6 +79,15 @@ typedef struct {
unsigned int timeout;
} cisco_proto;
+typedef struct {
+ unsigned short dce; /* 1 for DCE (network side) operation */
+ unsigned int modulo; /* modulo (8 = basic / 128 = extended) */
+ unsigned int window; /* frame window size */
+ unsigned int t1; /* timeout t1 */
+ unsigned int t2; /* timeout t2 */
+ unsigned int n2; /* frame retry counter */
+} x25_hdlc_proto;
+
/* PPP doesn't need any info now - supply length = 0 to ioctl */
#endif /* __ASSEMBLY__ */
diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
index 4bf33344aab1..be714cd8c826 100644
--- a/include/uapi/linux/if.h
+++ b/include/uapi/linux/if.h
@@ -213,6 +213,7 @@ struct if_settings {
fr_proto __user *fr;
fr_proto_pvc __user *fr_pvc;
fr_proto_pvc_info __user *fr_pvc_info;
+ x25_hdlc_proto __user *x25;
/* interface settings */
sync_serial_settings __user *sync;
--
2.20.1
o call skb_reset_network_header() before hdlc->xmit()
o change skb proto to HDLC (0x0019) before hdlc->xmit()
o call dev_queue_xmit_nit() before hdlc->xmit()
This changes make it possible to trace (tcpdump) outgoing layer2
(ETH_P_HDLC) packets
o use a copy of the skb for lapb_data_request() in x25_xmit()
This fixes the problem, that tracing layer3 (ETH_P_X25) packets
results in a malformed first byte of the packets.
Signed-off-by: Martin Schiller <[email protected]>
---
drivers/net/wan/hdlc_x25.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index b28051eba736..434e5263eddf 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -72,6 +72,7 @@ static int x25_data_indication(struct net_device *dev, struct sk_buff *skb)
unsigned char *ptr;
skb_push(skb, 1);
+ skb_reset_network_header(skb);
if (skb_cow(skb, 1))
return NET_RX_DROP;
@@ -88,6 +89,9 @@ static int x25_data_indication(struct net_device *dev, struct sk_buff *skb)
static void x25_data_transmit(struct net_device *dev, struct sk_buff *skb)
{
hdlc_device *hdlc = dev_to_hdlc(dev);
+ skb_reset_network_header(skb);
+ skb->protocol = hdlc_type_trans(skb, dev);
+ dev_queue_xmit_nit(skb, dev);
hdlc->xmit(skb, dev); /* Ignore return value :-( */
}
@@ -95,16 +99,19 @@ static void x25_data_transmit(struct net_device *dev, struct sk_buff *skb)
static netdev_tx_t x25_xmit(struct sk_buff *skb, struct net_device *dev)
{
+ struct sk_buff *skbn;
int result;
/* X.25 to LAPB */
switch (skb->data[0]) {
case X25_IFACE_DATA: /* Data to be transmitted */
- skb_pull(skb, 1);
- if ((result = lapb_data_request(dev, skb)) != LAPB_OK)
- dev_kfree_skb(skb);
- return NETDEV_TX_OK;
+ skbn = skb_copy(skb, GFP_ATOMIC);
+ skb_pull(skbn, 1);
+ skb_reset_network_header(skbn);
+ if ((result = lapb_data_request(dev, skbn)) != LAPB_OK)
+ dev_kfree_skb(skbn);
+ break;
case X25_IFACE_CONNECT:
if ((result = lapb_connect_request(dev))!= LAPB_OK) {
--
2.20.1
On Mon, 13 Jan 2020 13:45:51 +0100, Martin Schiller wrote:
> o call skb_reset_network_header() before hdlc->xmit()
> o change skb proto to HDLC (0x0019) before hdlc->xmit()
> o call dev_queue_xmit_nit() before hdlc->xmit()
>
> This changes make it possible to trace (tcpdump) outgoing layer2
> (ETH_P_HDLC) packets
>
> o use a copy of the skb for lapb_data_request() in x25_xmit()
It's not clear to me why
> This fixes the problem, that tracing layer3 (ETH_P_X25) packets
> results in a malformed first byte of the packets.
>
> Signed-off-by: Martin Schiller <[email protected]>
> ---
> drivers/net/wan/hdlc_x25.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
> index b28051eba736..434e5263eddf 100644
> --- a/drivers/net/wan/hdlc_x25.c
> +++ b/drivers/net/wan/hdlc_x25.c
> @@ -72,6 +72,7 @@ static int x25_data_indication(struct net_device *dev, struct sk_buff *skb)
> unsigned char *ptr;
>
> skb_push(skb, 1);
> + skb_reset_network_header(skb);
>
> if (skb_cow(skb, 1))
This skb_cow() here is for the next handler down to have a 1 byte of
headroom guaranteed? It'd seem more natural to have skb_cow before the
push.. not that it's related to your patch.
> return NET_RX_DROP;
> @@ -88,6 +89,9 @@ static int x25_data_indication(struct net_device *dev, struct sk_buff *skb)
> static void x25_data_transmit(struct net_device *dev, struct sk_buff *skb)
> {
> hdlc_device *hdlc = dev_to_hdlc(dev);
Please insert a new line after the variable declaration since you're
touching this one.
> + skb_reset_network_header(skb);
> + skb->protocol = hdlc_type_trans(skb, dev);
> + dev_queue_xmit_nit(skb, dev);
> hdlc->xmit(skb, dev); /* Ignore return value :-( */
> }
>
On Mon, 13 Jan 2020 13:45:50 +0100, Martin Schiller wrote:
> This enables you to configure mode (DTE/DCE), Modulo, Window, T1, T2, N2 via
> sethdlc (which needs to be patched as well).
>
> Signed-off-by: Martin Schiller <[email protected]>
> diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
> index 5643675ff724..b28051eba736 100644
> --- a/drivers/net/wan/hdlc_x25.c
> +++ b/drivers/net/wan/hdlc_x25.c
> @@ -21,8 +21,17 @@
> #include <linux/skbuff.h>
> #include <net/x25device.h>
>
> +struct x25_state {
> + x25_hdlc_proto settings;
> +};
> +
> static int x25_ioctl(struct net_device *dev, struct ifreq *ifr);
>
> +static inline struct x25_state* state(hdlc_device *hdlc)
Please no more static inlines in source files. Compiler will know what
to do.
> +{
> + return (struct x25_state *)hdlc->state;
> +}
> +
> /* These functions are callbacks called by LAPB layer */
>
> static void x25_connect_disconnect(struct net_device *dev, int reason, int code)
> @@ -186,6 +217,9 @@ static struct hdlc_proto proto = {
>
> static int x25_ioctl(struct net_device *dev, struct ifreq *ifr)
> {
> + x25_hdlc_proto __user *x25_s = ifr->ifr_settings.ifs_ifsu.x25;
> + const size_t size = sizeof(x25_hdlc_proto);
> + x25_hdlc_proto new_settings;
> hdlc_device *hdlc = dev_to_hdlc(dev);
> int result;
>
> @@ -194,7 +228,13 @@ static int x25_ioctl(struct net_device *dev, struct ifreq *ifr)
> if (dev_to_hdlc(dev)->proto != &proto)
> return -EINVAL;
> ifr->ifr_settings.type = IF_PROTO_X25;
> - return 0; /* return protocol only, no settable parameters */
> + if (ifr->ifr_settings.size < size) {
> + ifr->ifr_settings.size = size; /* data size wanted */
> + return -ENOBUFS;
> + }
> + if (copy_to_user(x25_s, &state(hdlc)->settings, size))
> + return -EFAULT;
> + return 0;
>
> case IF_PROTO_X25:
> if (!capable(CAP_NET_ADMIN))
> @@ -203,12 +243,35 @@ static int x25_ioctl(struct net_device *dev, struct ifreq *ifr)
> if (dev->flags & IFF_UP)
> return -EBUSY;
>
> + if (copy_from_user(&new_settings, x25_s, size))
> + return -EFAULT;
> +
> + if ((new_settings.dce != 0 &&
> + new_settings.dce != 1) ||
> + (new_settings.modulo != 8 &&
> + new_settings.modulo != 128) ||
> + new_settings.window < 1 ||
> + (new_settings.modulo == 8 &&
> + new_settings.window > 7) ||
> + (new_settings.modulo == 128 &&
> + new_settings.window > 127) ||
> + new_settings.t1 < 1 ||
> + new_settings.t1 > 255 ||
> + new_settings.t2 < 1 ||
> + new_settings.t2 > 255 ||
> + new_settings.n2 < 1 ||
> + new_settings.n2 > 255)
> + return -EINVAL;
> +
> result=hdlc->attach(dev, ENCODING_NRZ,PARITY_CRC16_PR1_CCITT);
> if (result)
> return result;
>
> - if ((result = attach_hdlc_protocol(dev, &proto, 0)))
> + if ((result = attach_hdlc_protocol(dev, &proto,
> + sizeof(struct x25_state))))
> return result;
> +
> + memcpy(&state(hdlc)->settings, &new_settings, size);
> dev->type = ARPHRD_X25;
> call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, dev);
> netif_dormant_off(dev);
> diff --git a/include/uapi/linux/hdlc/ioctl.h b/include/uapi/linux/hdlc/ioctl.h
> index 0fe4238e8246..3656ce8b8af0 100644
> --- a/include/uapi/linux/hdlc/ioctl.h
> +++ b/include/uapi/linux/hdlc/ioctl.h
> @@ -3,7 +3,7 @@
> #define __HDLC_IOCTL_H__
>
>
> -#define GENERIC_HDLC_VERSION 4 /* For synchronization with sethdlc utility */
> +#define GENERIC_HDLC_VERSION 5 /* For synchronization with sethdlc utility */
What's the backward compatibility story in this code?
The IOCTL handling at least looks like it may start returning errors
to existing user space which could have expected the parameters to
IF_PROTO_X25 (other than just ifr_settings.type) to be ignored.
> #define CLOCK_DEFAULT 0 /* Default setting */
> #define CLOCK_EXT 1 /* External TX and RX clock - DTE */
On 2020-01-13 14:53, Jakub Kicinski wrote:
> On Mon, 13 Jan 2020 13:45:50 +0100, Martin Schiller wrote:
>> This enables you to configure mode (DTE/DCE), Modulo, Window, T1, T2,
>> N2 via
>> sethdlc (which needs to be patched as well).
>>
>> Signed-off-by: Martin Schiller <[email protected]>
>
>> diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
>> index 5643675ff724..b28051eba736 100644
>> --- a/drivers/net/wan/hdlc_x25.c
>> +++ b/drivers/net/wan/hdlc_x25.c
>> @@ -21,8 +21,17 @@
>> #include <linux/skbuff.h>
>> #include <net/x25device.h>
>>
>> +struct x25_state {
>> + x25_hdlc_proto settings;
>> +};
>> +
>> static int x25_ioctl(struct net_device *dev, struct ifreq *ifr);
>>
>> +static inline struct x25_state* state(hdlc_device *hdlc)
>
> Please no more static inlines in source files. Compiler will know what
> to do.
OK, i will remove the "inline". I've just used it, because it's also
in the hdlc_fr.c and hdlc_cisco.c code.
>
>> +{
>> + return (struct x25_state *)hdlc->state;
>> +}
>> +
>> /* These functions are callbacks called by LAPB layer */
>>
>> static void x25_connect_disconnect(struct net_device *dev, int
>> reason, int code)
>
>> @@ -186,6 +217,9 @@ static struct hdlc_proto proto = {
>>
>> static int x25_ioctl(struct net_device *dev, struct ifreq *ifr)
>> {
>> + x25_hdlc_proto __user *x25_s = ifr->ifr_settings.ifs_ifsu.x25;
>> + const size_t size = sizeof(x25_hdlc_proto);
>> + x25_hdlc_proto new_settings;
>> hdlc_device *hdlc = dev_to_hdlc(dev);
>> int result;
>>
>> @@ -194,7 +228,13 @@ static int x25_ioctl(struct net_device *dev,
>> struct ifreq *ifr)
>> if (dev_to_hdlc(dev)->proto != &proto)
>> return -EINVAL;
>> ifr->ifr_settings.type = IF_PROTO_X25;
>> - return 0; /* return protocol only, no settable parameters */
>> + if (ifr->ifr_settings.size < size) {
>> + ifr->ifr_settings.size = size; /* data size wanted */
>> + return -ENOBUFS;
>> + }
>> + if (copy_to_user(x25_s, &state(hdlc)->settings, size))
>> + return -EFAULT;
>> + return 0;
>>
>> case IF_PROTO_X25:
>> if (!capable(CAP_NET_ADMIN))
>> @@ -203,12 +243,35 @@ static int x25_ioctl(struct net_device *dev,
>> struct ifreq *ifr)
>> if (dev->flags & IFF_UP)
>> return -EBUSY;
>>
>> + if (copy_from_user(&new_settings, x25_s, size))
>> + return -EFAULT;
>> +
>> + if ((new_settings.dce != 0 &&
>> + new_settings.dce != 1) ||
>> + (new_settings.modulo != 8 &&
>> + new_settings.modulo != 128) ||
>> + new_settings.window < 1 ||
>> + (new_settings.modulo == 8 &&
>> + new_settings.window > 7) ||
>> + (new_settings.modulo == 128 &&
>> + new_settings.window > 127) ||
>> + new_settings.t1 < 1 ||
>> + new_settings.t1 > 255 ||
>> + new_settings.t2 < 1 ||
>> + new_settings.t2 > 255 ||
>> + new_settings.n2 < 1 ||
>> + new_settings.n2 > 255)
>> + return -EINVAL;
>> +
>> result=hdlc->attach(dev, ENCODING_NRZ,PARITY_CRC16_PR1_CCITT);
>> if (result)
>> return result;
>>
>> - if ((result = attach_hdlc_protocol(dev, &proto, 0)))
>> + if ((result = attach_hdlc_protocol(dev, &proto,
>> + sizeof(struct x25_state))))
>> return result;
>> +
>> + memcpy(&state(hdlc)->settings, &new_settings, size);
>> dev->type = ARPHRD_X25;
>> call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, dev);
>> netif_dormant_off(dev);
>> diff --git a/include/uapi/linux/hdlc/ioctl.h
>> b/include/uapi/linux/hdlc/ioctl.h
>> index 0fe4238e8246..3656ce8b8af0 100644
>> --- a/include/uapi/linux/hdlc/ioctl.h
>> +++ b/include/uapi/linux/hdlc/ioctl.h
>> @@ -3,7 +3,7 @@
>> #define __HDLC_IOCTL_H__
>>
>>
>> -#define GENERIC_HDLC_VERSION 4 /* For synchronization with sethdlc
>> utility */
>> +#define GENERIC_HDLC_VERSION 5 /* For synchronization with sethdlc
>> utility */
>
> What's the backward compatibility story in this code?
Well, I thought I have to increment the version to keep the kernel code
and the sethdlc utility in sync (like the comment says).
>
> The IOCTL handling at least looks like it may start returning errors
> to existing user space which could have expected the parameters to
> IF_PROTO_X25 (other than just ifr_settings.type) to be ignored.
I could also try to implement it without incrementing the version by
looking at ifr_settings.size and using the former defaults if the size
doesn't match.
>
>> #define CLOCK_DEFAULT 0 /* Default setting */
>> #define CLOCK_EXT 1 /* External TX and RX clock - DTE */
On 2020-01-13 14:44, Jakub Kicinski wrote:
> On Mon, 13 Jan 2020 13:45:51 +0100, Martin Schiller wrote:
>> o call skb_reset_network_header() before hdlc->xmit()
>> o change skb proto to HDLC (0x0019) before hdlc->xmit()
>> o call dev_queue_xmit_nit() before hdlc->xmit()
>>
>> This changes make it possible to trace (tcpdump) outgoing layer2
>> (ETH_P_HDLC) packets
>>
>> o use a copy of the skb for lapb_data_request() in x25_xmit()
>
> It's not clear to me why
Well, this patch is ported form an older environment which is based on
linux-3.4. I can't reproduce the misbehavior with actual version, so I
will drop this part of the patch.
>
>> This fixes the problem, that tracing layer3 (ETH_P_X25) packets
>> results in a malformed first byte of the packets.
>>
>> Signed-off-by: Martin Schiller <[email protected]>
>> ---
>> drivers/net/wan/hdlc_x25.c | 15 +++++++++++----
>> 1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
>> index b28051eba736..434e5263eddf 100644
>> --- a/drivers/net/wan/hdlc_x25.c
>> +++ b/drivers/net/wan/hdlc_x25.c
>> @@ -72,6 +72,7 @@ static int x25_data_indication(struct net_device
>> *dev, struct sk_buff *skb)
>> unsigned char *ptr;
>>
>> skb_push(skb, 1);
>> + skb_reset_network_header(skb);
>>
>> if (skb_cow(skb, 1))
>
> This skb_cow() here is for the next handler down to have a 1 byte of
> headroom guaranteed? It'd seem more natural to have skb_cow before the
> push.. not that it's related to your patch.
Thanks for the hint. I will move the skb_cow() before the skb_push().
>
>> return NET_RX_DROP;
>> @@ -88,6 +89,9 @@ static int x25_data_indication(struct net_device
>> *dev, struct sk_buff *skb)
>> static void x25_data_transmit(struct net_device *dev, struct sk_buff
>> *skb)
>> {
>> hdlc_device *hdlc = dev_to_hdlc(dev);
>
> Please insert a new line after the variable declaration since you're
> touching this one.
OK, will do.
>
>> + skb_reset_network_header(skb);
>> + skb->protocol = hdlc_type_trans(skb, dev);
I will also insert an "if (dev_nit_active(dev))" here.
>> + dev_queue_xmit_nit(skb, dev);
>> hdlc->xmit(skb, dev); /* Ignore return value :-( */
>> }
>>
On Tue, 14 Jan 2020 06:37:03 +0100, Martin Schiller wrote:
> >> diff --git a/include/uapi/linux/hdlc/ioctl.h
> >> b/include/uapi/linux/hdlc/ioctl.h
> >> index 0fe4238e8246..3656ce8b8af0 100644
> >> --- a/include/uapi/linux/hdlc/ioctl.h
> >> +++ b/include/uapi/linux/hdlc/ioctl.h
> >> @@ -3,7 +3,7 @@
> >> #define __HDLC_IOCTL_H__
> >>
> >>
> >> -#define GENERIC_HDLC_VERSION 4 /* For synchronization with sethdlc
> >> utility */
> >> +#define GENERIC_HDLC_VERSION 5 /* For synchronization with sethdlc
> >> utility */
> >
> > What's the backward compatibility story in this code?
>
> Well, I thought I have to increment the version to keep the kernel code
> and the sethdlc utility in sync (like the comment says).
Perhaps I chose the wrong place for asking this question, IOCTL code
was my real worry. I don't think this version number is validated so
I think bumping it shouldn't break anything?
> > The IOCTL handling at least looks like it may start returning errors
> > to existing user space which could have expected the parameters to
> > IF_PROTO_X25 (other than just ifr_settings.type) to be ignored.
>
> I could also try to implement it without incrementing the version by
> looking at ifr_settings.size and using the former defaults if the size
> doesn't match.
Sounds good, thank you!
On 2020-01-14 13:51, Jakub Kicinski wrote:
> On Tue, 14 Jan 2020 06:37:03 +0100, Martin Schiller wrote:
>> >> diff --git a/include/uapi/linux/hdlc/ioctl.h
>> >> b/include/uapi/linux/hdlc/ioctl.h
>> >> index 0fe4238e8246..3656ce8b8af0 100644
>> >> --- a/include/uapi/linux/hdlc/ioctl.h
>> >> +++ b/include/uapi/linux/hdlc/ioctl.h
>> >> @@ -3,7 +3,7 @@
>> >> #define __HDLC_IOCTL_H__
>> >>
>> >>
>> >> -#define GENERIC_HDLC_VERSION 4 /* For synchronization with sethdlc
>> >> utility */
>> >> +#define GENERIC_HDLC_VERSION 5 /* For synchronization with sethdlc
>> >> utility */
>> >
>> > What's the backward compatibility story in this code?
>>
>> Well, I thought I have to increment the version to keep the kernel
>> code
>> and the sethdlc utility in sync (like the comment says).
>
> Perhaps I chose the wrong place for asking this question, IOCTL code
> was my real worry. I don't think this version number is validated so
> I think bumping it shouldn't break anything?
sethdlc validates the GENERIC_HDLC_VERSION at compile time.
https://mirrors.edge.kernel.org/pub/linux/utils/net/hdlc/
Another question:
Where do I have to send my patch for sethdlc to?
>
>> > The IOCTL handling at least looks like it may start returning errors
>> > to existing user space which could have expected the parameters to
>> > IF_PROTO_X25 (other than just ifr_settings.type) to be ignored.
>>
>> I could also try to implement it without incrementing the version by
>> looking at ifr_settings.size and using the former defaults if the size
>> doesn't match.
>
> Sounds good, thank you!
OK, I will send a v2 of the patch.
On Tue, 14 Jan 2020 14:33:51 +0100, Martin Schiller wrote:
> On 2020-01-14 13:51, Jakub Kicinski wrote:
> > On Tue, 14 Jan 2020 06:37:03 +0100, Martin Schiller wrote:
> >> >> diff --git a/include/uapi/linux/hdlc/ioctl.h
> >> >> b/include/uapi/linux/hdlc/ioctl.h
> >> >> index 0fe4238e8246..3656ce8b8af0 100644
> >> >> --- a/include/uapi/linux/hdlc/ioctl.h
> >> >> +++ b/include/uapi/linux/hdlc/ioctl.h
> >> >> @@ -3,7 +3,7 @@
> >> >> #define __HDLC_IOCTL_H__
> >> >>
> >> >>
> >> >> -#define GENERIC_HDLC_VERSION 4 /* For synchronization with sethdlc
> >> >> utility */
> >> >> +#define GENERIC_HDLC_VERSION 5 /* For synchronization with sethdlc
> >> >> utility */
> >> >
> >> > What's the backward compatibility story in this code?
> >>
> >> Well, I thought I have to increment the version to keep the kernel
> >> code
> >> and the sethdlc utility in sync (like the comment says).
> >
> > Perhaps I chose the wrong place for asking this question, IOCTL code
> > was my real worry. I don't think this version number is validated so
> > I think bumping it shouldn't break anything?
>
> sethdlc validates the GENERIC_HDLC_VERSION at compile time.
>
> https://mirrors.edge.kernel.org/pub/linux/utils/net/hdlc/
Aw, okay, best not to bump it then.
> Another question:
> Where do I have to send my patch for sethdlc to?
No idea :)
Hi,
I overlooked this mail, sorry.
Martin Schiller <[email protected]> writes:
> sethdlc validates the GENERIC_HDLC_VERSION at compile time.
>
> https://mirrors.edge.kernel.org/pub/linux/utils/net/hdlc/
>
> Another question:
> Where do I have to send my patch for sethdlc to?
I guess you have to put your own version somewhere. I don't have access
to hdlc directory at kernel.org anymore, this stuff (sync serial with
HDLC) is no longer in use around here.
--
Krzysztof Halasa
ŁUKASIEWICZ Research Network
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland