From: Martin Schiller <ms@dev.tdt.de>
To: Jakub Kicinski <kubakici@wp.pl>
Cc: khc@pm.waw.pl, davem@davemloft.net, linux-x25@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] wan/hdlc_x25: make lapb params configurable
Date: Tue, 14 Jan 2020 06:37:03 +0100 [thread overview]
Message-ID: <83f60f76a0cf602c73361ccdb34cc640@dev.tdt.de> (raw)
In-Reply-To: <20200113055316.4e811276@cakuba>
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 <ms@dev.tdt.de>
>
>> 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 */
next prev parent reply other threads:[~2020-01-14 5:37 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-13 12:45 [PATCH 1/2] wan/hdlc_x25: make lapb params configurable Martin Schiller
2020-01-13 12:45 ` [PATCH 2/2] wan/hdlc_x25: fix skb handling Martin Schiller
2020-01-13 13:44 ` Jakub Kicinski
2020-01-14 8:10 ` Martin Schiller
2020-01-13 13:53 ` [PATCH 1/2] wan/hdlc_x25: make lapb params configurable Jakub Kicinski
2020-01-14 5:37 ` Martin Schiller [this message]
2020-01-14 12:51 ` Jakub Kicinski
2020-01-14 13:33 ` Martin Schiller
2020-01-14 13:45 ` Jakub Kicinski
2020-03-26 6:08 ` Krzysztof Hałasa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=83f60f76a0cf602c73361ccdb34cc640@dev.tdt.de \
--to=ms@dev.tdt.de \
--cc=davem@davemloft.net \
--cc=khc@pm.waw.pl \
--cc=kubakici@wp.pl \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-x25@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).