* Re: [PATCH] qmi_wwan: support modify usbnet's rx_urb_size
From: Daniele Palmas @ 2020-08-03 10:33 UTC (permalink / raw)
To: Greg KH
Cc: yzc666, Bjørn Mork, David Miller, kuba, netdev, linux-usb,
carl
In-Reply-To: <20200803094931.GD635660@kroah.com>
Il giorno lun 3 ago 2020 alle ore 11:49 Greg KH
<gregkh@linuxfoundation.org> ha scritto:
>
> On Mon, Aug 03, 2020 at 10:26:18AM +0200, Daniele Palmas wrote:
> > Hi Greg,
> >
> > Il giorno lun 3 ago 2020 alle ore 10:18 Greg KH
> > <gregkh@linuxfoundation.org> ha scritto:
> > >
> > > On Mon, Aug 03, 2020 at 02:51:05PM +0800, yzc666@netease.com wrote:
> > > > From: carl <carl.yin@quectel.com>
> > > >
> > > > When QMUX enabled, the 'dl-datagram-max-size' can be 4KB/16KB/31KB depend on QUALCOMM's chipsets.
> > > > User can set 'dl-datagram-max-size' by 'QMI_WDA_SET_DATA_FORMAT'.
> > > > The usbnet's rx_urb_size must lager than or equal to the 'dl-datagram-max-size'.
> > > > This patch allow user to modify usbnet's rx_urb_size by next command.
> > > >
> > > > echo 4096 > /sys/class/net/wwan0/qmi/rx_urb_size
> > > >
> > > > Next commnds show how to set and query 'dl-datagram-max-size' by qmicli
> > > > # qmicli -d /dev/cdc-wdm1 --wda-set-data-format="link-layer-protocol=raw-ip, ul-protocol=qmap,
> > > > dl-protocol=qmap, dl-max-datagrams=32, dl-datagram-max-size=31744, ep-type=hsusb, ep-iface-number=4"
> > > > [/dev/cdc-wdm1] Successfully set data format
> > > > QoS flow header: no
> > > > Link layer protocol: 'raw-ip'
> > > > Uplink data aggregation protocol: 'qmap'
> > > > Downlink data aggregation protocol: 'qmap'
> > > > NDP signature: '0'
> > > > Downlink data aggregation max datagrams: '10'
> > > > Downlink data aggregation max size: '4096'
> > > >
> > > > # qmicli -d /dev/cdc-wdm1 --wda-get-data-format
> > > > [/dev/cdc-wdm1] Successfully got data format
> > > > QoS flow header: no
> > > > Link layer protocol: 'raw-ip'
> > > > Uplink data aggregation protocol: 'qmap'
> > > > Downlink data aggregation protocol: 'qmap'
> > > > NDP signature: '0'
> > > > Downlink data aggregation max datagrams: '10'
> > > > Downlink data aggregation max size: '4096'
> > > >
> > > > Signed-off-by: carl <carl.yin@quectel.com>
> > > > ---
> > > > drivers/net/usb/qmi_wwan.c | 39 ++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 39 insertions(+)
> > > >
> > > > diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> > > > index 07c42c0719f5b..8ea57fd99ae43 100644
> > > > --- a/drivers/net/usb/qmi_wwan.c
> > > > +++ b/drivers/net/usb/qmi_wwan.c
> > > > @@ -400,6 +400,44 @@ static ssize_t raw_ip_store(struct device *d, struct device_attribute *attr, co
> > > > return ret;
> > > > }
> > > >
> > > > +static ssize_t rx_urb_size_show(struct device *d, struct device_attribute *attr, char *buf)
> > > > +{
> > > > + struct usbnet *dev = netdev_priv(to_net_dev(d));
> > > > +
> > > > + return sprintf(buf, "%zd\n", dev->rx_urb_size);
> > >
> > > Why do you care about this?
> > >
> > > > +}
> > > > +
> > > > +static ssize_t rx_urb_size_store(struct device *d, struct device_attribute *attr,
> > > > + const char *buf, size_t len)
> > > > +{
> > > > + struct usbnet *dev = netdev_priv(to_net_dev(d));
> > > > + u32 rx_urb_size;
> > > > + int ret;
> > > > +
> > > > + if (kstrtou32(buf, 0, &rx_urb_size))
> > > > + return -EINVAL;
> > > > +
> > > > + /* no change? */
> > > > + if (rx_urb_size == dev->rx_urb_size)
> > > > + return len;
> > > > +
> > > > + if (!rtnl_trylock())
> > > > + return restart_syscall();
> > > > +
> > > > + /* we don't want to modify a running netdev */
> > > > + if (netif_running(dev->net)) {
> > > > + netdev_err(dev->net, "Cannot change a running device\n");
> > > > + ret = -EBUSY;
> > > > + goto err;
> > > > + }
> > > > +
> > > > + dev->rx_urb_size = rx_urb_size;
> > > > + ret = len;
> > > > +err:
> > > > + rtnl_unlock();
> > > > + return ret;
> > > > +}
> > > > +
> > > > static ssize_t add_mux_show(struct device *d, struct device_attribute *attr, char *buf)
> > > > {
> > > > struct net_device *dev = to_net_dev(d);
> > > > @@ -505,6 +543,7 @@ static DEVICE_ATTR_RW(add_mux);
> > > > static DEVICE_ATTR_RW(del_mux);
> > > >
> > > > static struct attribute *qmi_wwan_sysfs_attrs[] = {
> > > > + &dev_attr_rx_urb_size.attr,
> > >
> > > You added a driver-specific sysfs file and did not document in in
> > > Documentation/ABI? That's not ok, sorry, please fix up.
> > >
> > > Actually, no, this all should be done "automatically", do not change the
> > > urb size on the fly. Change it at probe time based on the device you
> > > are using, do not force userspace to "know" what to do here, as it will
> > > not know that at all.
> > >
> >
> > the problem with doing at probe time is that rx_urb_size is not fixed,
> > but depends on the configuration done at the userspace level with
> > QMI_WDA_SET_DATA_FORMAT, so the userspace knows that.
>
> Where does QMI_WDA_SET_DATA_FORMAT come from?
>
This is a request of Qualcomm proprietary protocol used, among other
things, to configure data aggregation for modems. There's an open
source userspace implementation in the libqmi project
(https://cgit.freedesktop.org/libqmi/tree/data/qmi-service-wda.json)
> And the commit log says that this "depends on the chipset being used",
> so why don't you know that at probe time, does the chipset change? :)
>
Me too does not understand this, I let the author explain...
> > Currently there's a workaround for setting rx_urb_size i.e. changing
> > the network interface MTU: this is fine for most uses with qmap, but
> > there's the limitation that certain values (multiple of the endpoint
> > size) are not allowed.
>
> Why not just set it really high to start with? That should not affect
> any older devices, as the urb size does not matter. The only thing is
> if it is too small that things can not move as fast as they might be
> able to.
>
Yes, this was proposed in the past by Bjørn
(https://lists.freedesktop.org/archives/libqmi-devel/2020-February/003221.html),
but I was not sure about issues with old modems.
Now I understand that there are no such issues, then I agree this is
the simplest solution: I've seen modems requiring as much as 49152,
but usually the default for qmap is <= 16384.
And, by the way, increasing the rx urb size is required also in
non-qmap mode, since the current value leads to babbling issues with
some chipsets (mine
https://www.spinics.net/lists/linux-usb/msg198025.html and Paul's
https://lists.freedesktop.org/archives/libqmi-devel/2020-February/003217.html),
so I think we should definitely increase this also for non-qmap mode.
Bjørn, what do you think?
Thanks,
Daniele
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH net v2 0/2] netfilter: conntrack: Fix CT offload timeout on heavily loaded systems
From: Pablo Neira Ayuso @ 2020-08-03 10:33 UTC (permalink / raw)
To: Roi Dayan; +Cc: netdev, Paul Blakey, Oz Shlomo, Marcelo Ricardo Leitner
In-Reply-To: <20200803073305.702079-1-roid@mellanox.com>
On Mon, Aug 03, 2020 at 10:33:03AM +0300, Roi Dayan wrote:
> On heavily loaded systems the GC can take time to go over all existing
> conns and reset their timeout. At that time other calls like from
> nf_conntrack_in() can call of nf_ct_is_expired() and see the conn as
> expired. To fix this when we set the offload bit we should also reset
> the timeout instead of counting on GC to finish first iteration over
> all conns before the initial timeout.
>
> First commit is to expose the function that updates the timeout.
> Second commit is to use it from flow_offload_add().
Series applied to nf.git, thanks.
^ permalink raw reply
* Re: [PATCH] qmi_wwan: support modify usbnet's rx_urb_size
From: Bjørn Mork @ 2020-08-03 10:32 UTC (permalink / raw)
To: Daniele Palmas
Cc: Greg KH, yzc666, David Miller, kuba, netdev, linux-usb, carl
In-Reply-To: <CAGRyCJE-BF=0tWakreObGv-skahDp-ui8zP1TPPkX48sMFk4-w@mail.gmail.com>
Daniele Palmas <dnlplm@gmail.com> writes:
> Il giorno lun 3 ago 2020 alle ore 10:18 Greg KH
> <gregkh@linuxfoundation.org> ha scritto:
>
>> Actually, no, this all should be done "automatically", do not change the
>> urb size on the fly. Change it at probe time based on the device you
>> are using, do not force userspace to "know" what to do here, as it will
>> not know that at all.
>>
>
> the problem with doing at probe time is that rx_urb_size is not fixed,
> but depends on the configuration done at the userspace level with
> QMI_WDA_SET_DATA_FORMAT, so the userspace knows that.
Yes, but the driver "will know" (or "may assume") this based on the
QMI_WWAN_FLAG_MUX flag, as long as we are using the driver internal
(de)muxing. We should be able to automatically set a sane rx_urb_size
value based on this?
Not sure if the rmnet driver currently can be used on top of qmi_wwan?
That will obviously need some other workaround.
> Currently there's a workaround for setting rx_urb_size i.e. changing
> the network interface MTU: this is fine for most uses with qmap, but
> there's the limitation that certain values (multiple of the endpoint
> size) are not allowed.
And this also requires an additional setup step for user/userspace,
which we should try to avoid if possible.
I'm all for a fully automatic solution. I don't think rx_urb_size
should be directly configurable. And it it were, then it should be
implemented in the usbnet framework. It is not a qmi_wwan specific
attribute.
Bjørn
^ permalink raw reply
* 答复: [PATCH] qmi_wwan: support modify usbnet's rx_urb_size
From: Carl Yin(殷张成) @ 2020-08-03 10:35 UTC (permalink / raw)
To: Greg KH, Daniele Palmas
Cc: yzc666@netease.com, Bjørn Mork, David Miller,
kuba@kernel.org, netdev@vger.kernel.org, linux-usb
In-Reply-To: <20200803094931.GD635660@kroah.com>
Hi Greg:
I am carl, software engineer from a Chinese company ' Quectel Wireless Solutions Co., Ltd'.
Chinese name is Yinzhangcheng. So English name carl.yin.
My company’s products are LTE/5G modules base on QUALCOMM's chipset, like MDM9607/MDM9640/SDX24/SDX55...etc.
On 2020-08-03，"Greg KH" <gregkh@linuxfoundation.org> wrote：
>On Mon, Aug 03, 2020 at 10:26:18AM +0200, Daniele Palmas wrote:
> Hi Greg,
>
> Il giorno lun 3 ago 2020 alle ore 10:18 Greg KH
> <gregkh@linuxfoundation.org> ha scritto:
> >
> > On Mon, Aug 03, 2020 at 02:51:05PM +0800, yzc666@netease.com wrote:
> > > From: carl <carl.yin@quectel.com>
> > >
> > > When QMUX enabled, the 'dl-datagram-max-size' can be 4KB/16KB/31KB depend on QUALCOMM's chipsets.
> > > User can set 'dl-datagram-max-size' by 'QMI_WDA_SET_DATA_FORMAT'.
> > > The usbnet's rx_urb_size must lager than or equal to the 'dl-datagram-max-size'.
> > > This patch allow user to modify usbnet's rx_urb_size by next command.
> > >
> > > echo 4096 > /sys/class/net/wwan0/qmi/rx_urb_size
> > >
> > > Next commnds show how to set and query 'dl-datagram-max-size' by qmicli
> > > # qmicli -d /dev/cdc-wdm1 --wda-set-data-format="link-layer-protocol=raw-ip, ul-protocol=qmap,
> > > dl-protocol=qmap, dl-max-datagrams=32, dl-datagram-max-size=31744, ep-type=hsusb, ep-iface-number=4"
> > > [/dev/cdc-wdm1] Successfully set data format
> > > QoS flow header: no
> > > Link layer protocol: 'raw-ip'
> > > Uplink data aggregation protocol: 'qmap'
> > > Downlink data aggregation protocol: 'qmap'
> > > NDP signature: '0'
> > > Downlink data aggregation max datagrams: '10'
> > > Downlink data aggregation max size: '4096'
> > >
> > > # qmicli -d /dev/cdc-wdm1 --wda-get-data-format
> > > [/dev/cdc-wdm1] Successfully got data format
> > > QoS flow header: no
> > > Link layer protocol: 'raw-ip'
> > > Uplink data aggregation protocol: 'qmap'
> > > Downlink data aggregation protocol: 'qmap'
> > > NDP signature: '0'
> > > Downlink data aggregation max datagrams: '10'
> > > Downlink data aggregation max size: '4096'
> > >
> > > Signed-off-by: carl <carl.yin@quectel.com>
> > > ---
> > > drivers/net/usb/qmi_wwan.c | 39
> > > ++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 39 insertions(+)
> > >
> > > diff --git a/drivers/net/usb/qmi_wwan.c
> > > b/drivers/net/usb/qmi_wwan.c index 07c42c0719f5b..8ea57fd99ae43
> > > 100644
> > > --- a/drivers/net/usb/qmi_wwan.c
> > > +++ b/drivers/net/usb/qmi_wwan.c
> > > @@ -400,6 +400,44 @@ static ssize_t raw_ip_store(struct device *d, struct device_attribute *attr, co
> > > return ret;
> > > }
> > >
> > > +static ssize_t rx_urb_size_show(struct device *d, struct
> > > +device_attribute *attr, char *buf) {
> > > + struct usbnet *dev = netdev_priv(to_net_dev(d));
> > > +
> > > + return sprintf(buf, "%zd\n", dev->rx_urb_size);
> >
> > Why do you care about this?
> >
> > > +}
> > > +
> > > +static ssize_t rx_urb_size_store(struct device *d, struct device_attribute *attr,
> > > + const char *buf, size_t len) {
> > > + struct usbnet *dev = netdev_priv(to_net_dev(d));
> > > + u32 rx_urb_size;
> > > + int ret;
> > > +
> > > + if (kstrtou32(buf, 0, &rx_urb_size))
> > > + return -EINVAL;
> > > +
> > > + /* no change? */
> > > + if (rx_urb_size == dev->rx_urb_size)
> > > + return len;
> > > +
> > > + if (!rtnl_trylock())
> > > + return restart_syscall();
> > > +
> > > + /* we don't want to modify a running netdev */
> > > + if (netif_running(dev->net)) {
> > > + netdev_err(dev->net, "Cannot change a running device\n");
> > > + ret = -EBUSY;
> > > + goto err;
> > > + }
> > > +
> > > + dev->rx_urb_size = rx_urb_size;
> > > + ret = len;
> > > +err:
> > > + rtnl_unlock();
> > > + return ret;
> > > +}
> > > +
> > > static ssize_t add_mux_show(struct device *d, struct
> > > device_attribute *attr, char *buf) {
> > > struct net_device *dev = to_net_dev(d); @@ -505,6 +543,7 @@
> > > static DEVICE_ATTR_RW(add_mux); static DEVICE_ATTR_RW(del_mux);
> > >
> > > static struct attribute *qmi_wwan_sysfs_attrs[] = {
> > > + &dev_attr_rx_urb_size.attr,
> >
> > You added a driver-specific sysfs file and did not document in in
> > Documentation/ABI? That's not ok, sorry, please fix up.
> >
> > Actually, no, this all should be done "automatically", do not change
> > the urb size on the fly. Change it at probe time based on the
> > device you are using, do not force userspace to "know" what to do
> > here, as it will not know that at all.
> >
>
> the problem with doing at probe time is that rx_urb_size is not fixed,
> but depends on the configuration done at the userspace level with
> QMI_WDA_SET_DATA_FORMAT, so the userspace knows that.
>
> Where does QMI_WDA_SET_DATA_FORMAT come from?
>
QMI_WDA_SET_DATA_FORMAT is from QUALLCOM's QMI protocol.
Like USB NET MBIM protocol, ' QMI protocol ' also can transfer multiple IP Packets in one URB.
The benefit is reduce USB interrupts, so can reduce CPU loading.
QUALLCOM call it as QMAP, full name is QUALCOMM Multiplexing and Aggregation.
The means of 'dl-datagram-max-size' in QMAP is 'Maximum size in bytes of a single aggregated packet allowed on downlink."
For example, MDM9607 support up to 4KB, SDX20 support up to 16KB, SDX55 support up to 31KB.
The 'dl-datagram-max-size' can by support is depend the chipset.
But the userspace can use ' QMI_WDA_SET_DATA_FORMAT' to Negotiates an new size(less than or equal to 'dl-datagram-max-size').
Most of case, the userspace will use the max 'dl-datagram-max-size', for benefit of 'reduce CPU loding'.
> And the commit log says that this "depends on the chipset being used", so why don't you know that at probe time, does the chipset change? :)
>
> > Currently there's a workaround for setting rx_urb_size i.e. changing
> > the network interface MTU: this is fine for most uses with qmap, but
> > there's the limitation that certain values (multiple of the endpoint
> > size) are not allowed.
>
> Why not just set it really high to start with? That should not affect any older devices, as the urb size does not matter. The only thing is if it is too small that things can not move as fast as they might be able to.
>
Hi Daniele: I also understand the "limitation", do you mean need to send "USB ZERO length PACKET'.
The function of MTU is not same as rx_urb_size, for example we can set rx_urb_size to 31KB, but MTU is still 1500B.
> thanks,
>
> greg k-h
^ permalink raw reply
* RE: [net-next 1/5] ice: add the virtchnl handler for AdminQ command
From: Wang, Haiyue @ 2020-08-03 10:39 UTC (permalink / raw)
To: Jakub Kicinski, Tom Herbert
Cc: Venkataramanan, Anirudh, davem@davemloft.net, nhorman@redhat.com,
sassmann@redhat.com, Bowers, AndrewX, Kirsher, Jeffrey T,
netdev@vger.kernel.org, Nguyen, Anthony L, Lu, Nannan,
Liang, Cunming
In-Reply-To: <20200727160406.4d2bc1c8@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, July 28, 2020 07:04
> To: Tom Herbert <tom@herbertland.com>
> Cc: Venkataramanan, Anirudh <anirudh.venkataramanan@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> davem@davemloft.net; nhorman@redhat.com; sassmann@redhat.com; Bowers, AndrewX
> <andrewx.bowers@intel.com>; Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; netdev@vger.kernel.org;
> Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Lu, Nannan <nannan.lu@intel.com>
> Subject: Re: [net-next 1/5] ice: add the virtchnl handler for AdminQ command
>
> In this case, I'm guessing, Intel can reuse RTE flow -> AQ code written
> to run on PFs on the special VF.
>
> This community has selected switchdev + flower for programming flows.
> I believe implementing flower offloads would solve your use case, and
> at the same time be most beneficial to the netdev community.
Jakub,
Thanks, I deep into the switchdev, it is kernel software bridge for hardware
offload, and each port is registered with register_netdev. So this solution
is not suitable for current case: VF can be assigned to VMs.
BR,
Haiyue
^ permalink raw reply
* Re: [PATCH] qmi_wwan: support modify usbnet's rx_urb_size
From: Bjørn Mork @ 2020-08-03 10:40 UTC (permalink / raw)
To: Daniele Palmas
Cc: Greg KH, yzc666, David Miller, kuba, netdev, linux-usb, carl
In-Reply-To: <CAGRyCJER3J4BkLohcPumdKUkQ9g39YsjERac5CSrY2-8jj+N7A@mail.gmail.com>
Daniele Palmas <dnlplm@gmail.com> writes:
> Il giorno lun 3 ago 2020 alle ore 11:49 Greg KH
> <gregkh@linuxfoundation.org> ha scritto:
>>
>> Where does QMI_WDA_SET_DATA_FORMAT come from?
>>
>
> This is a request of Qualcomm proprietary protocol used, among other
> things, to configure data aggregation for modems. There's an open
> source userspace implementation in the libqmi project
> (https://cgit.freedesktop.org/libqmi/tree/data/qmi-service-wda.json)
>
>> And the commit log says that this "depends on the chipset being used",
>> so why don't you know that at probe time, does the chipset change? :)
>>
>
> Me too does not understand this, I let the author explain...
>
>> > Currently there's a workaround for setting rx_urb_size i.e. changing
>> > the network interface MTU: this is fine for most uses with qmap, but
>> > there's the limitation that certain values (multiple of the endpoint
>> > size) are not allowed.
>>
>> Why not just set it really high to start with? That should not affect
>> any older devices, as the urb size does not matter. The only thing is
>> if it is too small that things can not move as fast as they might be
>> able to.
>>
>
> Yes, this was proposed in the past by Bjørn
> (https://lists.freedesktop.org/archives/libqmi-devel/2020-February/003221.html),
> but I was not sure about issues with old modems.
Ah, right. Forgot about that.
> Now I understand that there are no such issues, then I agree this is
> the simplest solution: I've seen modems requiring as much as 49152,
> but usually the default for qmap is <= 16384.
>
> And, by the way, increasing the rx urb size is required also in
> non-qmap mode, since the current value leads to babbling issues with
> some chipsets (mine
> https://www.spinics.net/lists/linux-usb/msg198025.html and Paul's
> https://lists.freedesktop.org/archives/libqmi-devel/2020-February/003217.html),
> so I think we should definitely increase this also for non-qmap mode.
>
> Bjørn, what do you think?
I think we have good enough reasons to increase the rx urb size by
default. Let's try.
Bjørn
^ permalink raw reply
* Re: [PATCH net v2 1/2] netfilter: conntrack: Move nf_ct_offload_timeout to header file
From: Pablo Neira Ayuso @ 2020-08-03 11:03 UTC (permalink / raw)
To: Roi Dayan; +Cc: netdev, Paul Blakey, Oz Shlomo, Marcelo Ricardo Leitner
In-Reply-To: <20200803073305.702079-2-roid@mellanox.com>
On Mon, Aug 03, 2020 at 10:33:04AM +0300, Roi Dayan wrote:
> To be used by callers from other modules.
>
> Signed-off-by: Roi Dayan <roid@mellanox.com>
> Reviewed-by: Oz Shlomo <ozsh@mellanox.com>
> ---
> include/net/netfilter/nf_conntrack.h | 12 ++++++++++++
> net/netfilter/nf_conntrack_core.c | 12 ------------
> 2 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
> index 90690e37a56f..8481819ff632 100644
> --- a/include/net/netfilter/nf_conntrack.h
> +++ b/include/net/netfilter/nf_conntrack.h
> @@ -279,6 +279,18 @@ static inline bool nf_ct_should_gc(const struct nf_conn *ct)
> !nf_ct_is_dying(ct);
> }
>
> +#define DAY (86400 * HZ)
> +
> +/* Set an arbitrary timeout large enough not to ever expire, this save
> + * us a check for the IPS_OFFLOAD_BIT from the packet path via
> + * nf_ct_is_expired().
> + */
> +static inline void nf_ct_offload_timeout(struct nf_conn *ct)
> +{
> + if (nf_ct_expires(ct) < DAY / 2)
> + ct->timeout = nfct_time_stamp + DAY;
> +}
> +
> struct kernel_param;
>
For the record: I have renamed DAY to NF_CT_DAY to avoid a possible
symbol name clash. No need to resend, I applied this small change
before applying.
^ permalink raw reply
* PMTUD broken inside network namespace with multipath routing
From: mastertheknife @ 2020-08-03 11:14 UTC (permalink / raw)
To: netdev
Hi,
I have observed that PMTUD (Path MTU discovery) is broken using
multipath routing inside a network namespace. This breaks TCP, because
it keeps trying to send oversized packets.
Observed on kernel 5.4.44, other kernels weren't tested. However i
went through net/ipv4/route.c and haven't spotted changes in this
area, so i believe this bug is still there.
Host test with multipath routing:
---------------------------------
root@host1:~# ip route add 192.168.247.100/32 dev vmbr2 nexthop via
192.168.252.250 dev vmbr2 nexthop via 192.168.252.252 dev vmbr2
root@host1:~# ip route | grep -A2 192.168.247.100
192.168.247.100
nexthop via 192.168.252.250 dev vmbr2 weight 1
nexthop via 192.168.252.252 dev vmbr2 weight 1
root@host1:~# ping -M do -s 1380 192.168.247.100
PING 192.168.247.100 (192.168.247.100) 1380(1408) bytes of data.
From 192.168.252.252 icmp_seq=1 Frag needed and DF set (mtu = 1406)
ping: local error: Message too long, mtu=1406
ping: local error: Message too long, mtu=1406
ping: local error: Message too long, mtu=1406
ping: local error: Message too long, mtu=1406
^C
--- 192.168.247.100 ping statistics ---
5 packets transmitted, 0 received, +5 errors, 100% packet loss, time 80ms
root@host1:~# ip route get 192.168.247.100
192.168.247.100 via 192.168.252.250 dev vmbr2 src 192.168.252.15 uid 0
cache expires 583sec mtu 1406
LXC container inside that host with multipath routing:
------------------------------------------------------
[root@lxctest ~]# ip route add 192.168.247.100/32 dev eth0 nexthop via
192.168.252.250 dev eth0 nexthop via 192.168.252.252 dev eth0
[root@lxctest ~]# ip route
default via 192.168.252.100 dev eth0 proto static metric 100
192.168.247.100
nexthop via 192.168.252.250 dev eth0 weight 1
nexthop via 192.168.252.252 dev eth0 weight 1
192.168.252.0/24 dev eth0 proto kernel scope link src 192.168.252.207 metric 100
[root@lxctest ~]# ping -M do -s 1380 192.168.247.100
PING 192.168.247.100 (192.168.247.100) 1380(1408) bytes of data.
From 192.168.252.252 icmp_seq=1 Frag needed and DF set (mtu = 1406)
From 192.168.252.252 icmp_seq=2 Frag needed and DF set (mtu = 1406)
From 192.168.252.252 icmp_seq=3 Frag needed and DF set (mtu = 1406)
From 192.168.252.252 icmp_seq=4 Frag needed and DF set (mtu = 1406)
[root@lxctest ~]# ip route get 192.168.247.100
192.168.247.100 via 192.168.252.252 dev eth0 src 192.168.252.207 uid 0
cache
LXC container inside that host with regular routing:
----------------------------------------------------
[root@lxctest ~]# ip route add 192.168.247.100/32 via 192.168.252.252 dev eth0
[root@lxctest ~]# ip route
default via 192.168.252.100 dev eth0 proto static metric 100
192.168.247.100 via 192.168.252.252 dev eth0
192.168.252.0/24 dev eth0 proto kernel scope link src 192.168.252.207 metric 100
[root@lxctest ~]# ping -M do -s 1380 192.168.247.100
PING 192.168.247.100 (192.168.247.100) 1380(1408) bytes of data.
From 192.168.252.252 icmp_seq=1 Frag needed and DF set (mtu = 1406)
ping: local error: Message too long, mtu=1406
ping: local error: Message too long, mtu=1406
ping: local error: Message too long, mtu=1406
^C
--- 192.168.247.100 ping statistics ---
4 packets transmitted, 0 received, +4 errors, 100% packet loss, time 82ms
[root@lxctest ~]# ip route get 192.168.247.100
192.168.247.100 via 192.168.252.252 dev eth0 src 192.168.252.207 uid 0
cache expires 591sec mtu 1406
What seems to be happening, is that when multipath routing is used
inside LXC (or any network namespace), the kernel doesn't generate a
routing exception to force the lower MTU.
I believe this is a bug inside the kernel.
Kfir Itzhak
^ permalink raw reply
* Re: [PATCH v3 iproute2-next] devlink: Add board.serial_number to info subcommand.
From: Jiri Pirko @ 2020-08-03 11:22 UTC (permalink / raw)
To: Vasundhara Volam
Cc: netdev, dsahern, stephen, davem, jiri, kuba, michael.chan
In-Reply-To: <20200731104643.35726-1-vasundhara-v.volam@broadcom.com>
Fri, Jul 31, 2020 at 12:46:43PM CEST, vasundhara-v.volam@broadcom.com wrote:
>Add support for reading board serial_number to devlink info
>subcommand. Example:
>
>$ devlink dev info pci/0000:af:00.0 -jp
>{
> "info": {
> "pci/0000:af:00.0": {
> "driver": "bnxt_en",
> "serial_number": "00-10-18-FF-FE-AD-1A-00",
> "board.serial_number": "433551F+172300000",
> "versions": {
> "fixed": {
> "board.id": "7339763 Rev 0.",
> "asic.id": "16D7",
> "asic.rev": "1"
> },
> "running": {
> "fw": "216.1.216.0",
> "fw.psid": "0.0.0",
> "fw.mgmt": "216.1.192.0",
> "fw.mgmt.api": "1.10.1",
> "fw.ncsi": "0.0.0.0",
> "fw.roce": "216.1.16.0"
> }
> }
> }
> }
>}
>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Looks fine.
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
^ permalink raw reply
* RE: [PATCH 2/2] net: phy: Associate device node with fixed PHY
From: Madalin Bucur (OSS) @ 2020-08-03 11:45 UTC (permalink / raw)
To: Russell King - ARM Linux admin, Madalin Bucur (OSS)
Cc: Andrew Lunn, Vikas Singh, f.fainelli@gmail.com,
hkallweit1@gmail.com, netdev@vger.kernel.org,
Calvin Johnson (OSS), kuldip dwivedi, Vikas Singh
In-Reply-To: <20200803090716.GL1551@shell.armlinux.org.uk>
> -----Original Message-----
> From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Sent: 03 August 2020 12:07
> To: Madalin Bucur (OSS) <madalin.bucur@oss.nxp.com>
> Cc: Andrew Lunn <andrew@lunn.ch>; Vikas Singh
> <vikas.singh@puresoftware.com>; f.fainelli@gmail.com; hkallweit1@gmail.com;
> netdev@vger.kernel.org; Calvin Johnson (OSS) <calvin.johnson@oss.nxp.com>;
> kuldip dwivedi <kuldip.dwivedi@puresoftware.com>; Vikas Singh
> <vikas.singh@nxp.com>
> Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed PHY
>
> On Mon, Aug 03, 2020 at 08:33:19AM +0000, Madalin Bucur (OSS) wrote:
> > > -----Original Message-----
> > > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> > > Behalf Of Andrew Lunn
> > > Sent: 01 August 2020 18:11
> > > To: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > > Cc: Vikas Singh <vikas.singh@puresoftware.com>; f.fainelli@gmail.com;
> > > hkallweit1@gmail.com; netdev@vger.kernel.org; Calvin Johnson (OSS)
> > > <calvin.johnson@oss.nxp.com>; kuldip dwivedi
> > > <kuldip.dwivedi@puresoftware.com>; Madalin Bucur (OSS)
> > > <madalin.bucur@oss.nxp.com>; Vikas Singh <vikas.singh@nxp.com>
> > > Subject: Re: [PATCH 2/2] net: phy: Associate device node with fixed
> PHY
> > >
> > > On Sat, Aug 01, 2020 at 10:41:32AM +0100, Russell King - ARM Linux
> admin
> > > wrote:
> > > > On Sat, Aug 01, 2020 at 09:52:52AM +0530, Vikas Singh wrote:
> > > > > Hi Andrew,
> > > > >
> > > > > Please refer to the "fman" node under
> > > > > linux/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
> > > > > I have two 10G ethernet interfaces out of which one is of fixed-
> link.
> > > >
> > > > Please do not top post.
> > > >
> > > > How does XGMII (which is a 10G only interface) work at 1G speed? Is
> > > > what is in DT itself a hack because fixed-phy doesn't support 10G
> > > > modes?
> > >
> > > My gut feeling is there is some hack going on here, which is why i'm
> > > being persistent at trying to understand what is actually going on
> > > here.
> >
> > Hi Andrew,
> >
> > That platform used 1G fixed link there since there was no support for
> > 10G fixed link at the time. PHYlib could have tolerated 10G speed there
> > With a one-liner.
>
> That statement is false. It is not a "one liner". fixed-phy exposes
> the settings to userspace as a Clause 22 PHY register set, and the
> Clause 22 register set does not support 10G. So, a "one liner" would
> just be yet another hack. Adding Clause 45 PHY emulation support
> would be a huge task.
>
> > I understand that PHYLink is working to describe this
> > Better, but it was not there at that time. Adding the dependency on
> > PHYLink was not desirable as most of the users for the DPAA 1 platforms
> > were targeting kernels before the PHYLink introduction (and last I've
> > looked, it's still under development, with unstable APIs so we'll
> > take a look at this later, when it settles).
>
> I think you need to read Documentation/process/stable-api-nonsense.rst
> particularly the section "Stable Kernel Source Interfaces".
>
> phylink is going to be under development for quite some time to come
> as requirements evolve. For example, when support for QSFP interfaces
> is eventually worked out, I suspect there will need to be some further
> changes to the driver interface. This is completely normal.
>
> Now, as to the stability of the phylink API to drivers, it has in fact
> been very stable - it has only changed over the course of this year to
> support split PCS, a necessary step for DPAA2 and a few others. It has
> been around in mainline for two years, and has been around much longer
> than that, and during that time it has been in mainline, the MAC facing
> interface has not changed until recently.
>
> So, I find your claim to be quite unreasonable.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
I see you agree that there were and there will be many changes for a while,
It's not a complaint, I know hot it works, it's just a decision based on
required effort vs features offered vs user requirements. Lately it's been
time consuming to try to fix things in this area.
Regards
Madalin
^ permalink raw reply
* [PATCH ethtool 0/7] compiler warnings cleanup, part 1
From: Michal Kubecek @ 2020-08-03 11:57 UTC (permalink / raw)
To: netdev
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2151 bytes --]
Maciej Żenczykowski recently cleaned up many "unused parameter" compiler
warnings but some new occurences appeared since (mostly in netlink code).
This series gets rid of all currently found "unused parameter" warnings and
also one zero length array access warning (gcc10). There are still some
compiler warnings left (signed/unsigned comparison and missing struct field
initializers); these will be handled in next cycle as the fixes are more
intrusive.
This series should not affect resulting code; checked by comparing
resulting binary against unpatched source.
Michal Kubecek (7):
rename maybe_unused macro to __maybe_unused
cable_test: clean up unused parameters
igc: mark unused callback parameter
netlink: mark unused callback parameter
netlink: mark unused parameters of bitset walker callbacks
netlink: mark unused parameters of parser callbacks
ioctl: avoid zero length array warning in get_stringset()
amd8111e.c | 2 +-
at76c50x-usb.c | 2 +-
de2104x.c | 4 ++--
dsa.c | 2 +-
e100.c | 2 +-
e1000.c | 2 +-
et131x.c | 2 +-
ethtool.c | 10 ++++++----
fec.c | 2 +-
fec_8xx.c | 2 +-
fjes.c | 2 +-
ibm_emac.c | 2 +-
igb.c | 2 +-
igc.c | 3 ++-
internal.h | 2 +-
ixgb.c | 2 +-
ixgbe.c | 2 +-
ixgbevf.c | 2 +-
lan78xx.c | 2 +-
marvell.c | 4 ++--
natsemi.c | 4 ++--
netlink/cable_test.c | 21 ++++++++-------------
netlink/netlink.c | 2 +-
netlink/parser.c | 28 ++++++++++++++++------------
netlink/pause.c | 3 ++-
netlink/privflags.c | 2 +-
netlink/settings.c | 9 ++++++---
netlink/tsinfo.c | 2 +-
realtek.c | 2 +-
sfc.c | 3 ++-
smsc911x.c | 2 +-
stmmac.c | 4 ++--
tg3.c | 4 ++--
tse.c | 2 +-
vioc.c | 2 +-
vmxnet3.c | 2 +-
36 files changed, 76 insertions(+), 69 deletions(-)
--
2.28.0
^ permalink raw reply
* [PATCH ethtool 1/7] rename maybe_unused macro to __maybe_unused
From: Michal Kubecek @ 2020-08-03 11:57 UTC (permalink / raw)
To: netdev
In-Reply-To: <cover.1596451857.git.mkubecek@suse.cz>
This makes the code consistent with kernel and also makes it a bit more
apparent that it is an attribute.
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
amd8111e.c | 2 +-
at76c50x-usb.c | 2 +-
de2104x.c | 4 ++--
dsa.c | 2 +-
e100.c | 2 +-
e1000.c | 2 +-
et131x.c | 2 +-
ethtool.c | 6 +++---
fec.c | 2 +-
fec_8xx.c | 2 +-
fjes.c | 2 +-
ibm_emac.c | 2 +-
igb.c | 2 +-
internal.h | 2 +-
ixgb.c | 2 +-
ixgbe.c | 2 +-
ixgbevf.c | 2 +-
lan78xx.c | 2 +-
marvell.c | 4 ++--
natsemi.c | 4 ++--
realtek.c | 2 +-
sfc.c | 3 ++-
smsc911x.c | 2 +-
stmmac.c | 4 ++--
tg3.c | 4 ++--
tse.c | 2 +-
vioc.c | 2 +-
vmxnet3.c | 2 +-
28 files changed, 36 insertions(+), 35 deletions(-)
diff --git a/amd8111e.c b/amd8111e.c
index 5a056b3f84ca..175516bd2904 100644
--- a/amd8111e.c
+++ b/amd8111e.c
@@ -152,7 +152,7 @@ typedef enum {
#define PHY_SPEED_100 0x3
-int amd8111e_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int amd8111e_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
diff --git a/at76c50x-usb.c b/at76c50x-usb.c
index 0121e9886b65..fad41bf5fe25 100644
--- a/at76c50x-usb.c
+++ b/at76c50x-usb.c
@@ -12,7 +12,7 @@ static char *hw_versions[] = {
" 505AMX",
};
-int at76c50x_usb_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int at76c50x_usb_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
u8 version = (u8)(regs->version >> 24);
diff --git a/de2104x.c b/de2104x.c
index cc03533d1548..190422fb2249 100644
--- a/de2104x.c
+++ b/de2104x.c
@@ -111,7 +111,7 @@ print_rx_missed(u32 csr8)
}
}
-static void de21040_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+static void de21040_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
u32 tmp, v, *data = (u32 *)regs->data;
@@ -417,7 +417,7 @@ static void de21040_dump_regs(struct ethtool_drvinfo *info maybe_unused,
v & (1<<0) ? " Jabber disable\n" : "");
}
-static void de21041_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+static void de21041_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
u32 tmp, v, *data = (u32 *)regs->data;
diff --git a/dsa.c b/dsa.c
index 0071769861c3..65502a899194 100644
--- a/dsa.c
+++ b/dsa.c
@@ -870,7 +870,7 @@ static int dsa_mv88e6xxx_dump_regs(struct ethtool_regs *regs)
#undef FIELD
#undef REG
-int dsa_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int dsa_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
/* DSA per-driver register dump */
diff --git a/e100.c b/e100.c
index 540ae3544faf..fd4bd031a4a6 100644
--- a/e100.c
+++ b/e100.c
@@ -36,7 +36,7 @@
#define CU_CMD 0x00F0
#define RU_CMD 0x0007
-int e100_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int e100_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
u32 *regs_buff = (u32 *)regs->data;
diff --git a/e1000.c b/e1000.c
index 91e5bc14afe5..dbd6eb55a92c 100644
--- a/e1000.c
+++ b/e1000.c
@@ -363,7 +363,7 @@ static enum e1000_mac_type e1000_get_mac_type(u16 device_id)
return mac_type;
}
-int e1000_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int e1000_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
u32 *regs_buff = (u32 *)regs->data;
diff --git a/et131x.c b/et131x.c
index 1b0607177e6d..a23f7a27091c 100644
--- a/et131x.c
+++ b/et131x.c
@@ -2,7 +2,7 @@
#include <string.h>
#include "internal.h"
-int et131x_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int et131x_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
u8 version = (u8)(regs->version >> 24);
diff --git a/ethtool.c b/ethtool.c
index 1b99ac91dcbf..0f312bdae2bb 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -366,7 +366,7 @@ static int rxflow_str_to_type(const char *str)
return flow_type;
}
-static int do_version(struct cmd_context *ctx maybe_unused)
+static int do_version(struct cmd_context *ctx __maybe_unused)
{
fprintf(stdout,
PACKAGE " version " VERSION
@@ -1106,7 +1106,7 @@ nested:
}
static int dump_eeprom(int geeprom_dump_raw,
- struct ethtool_drvinfo *info maybe_unused,
+ struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_eeprom *ee)
{
if (geeprom_dump_raw) {
@@ -5708,7 +5708,7 @@ static const struct option args[] = {
{}
};
-static int show_usage(struct cmd_context *ctx maybe_unused)
+static int show_usage(struct cmd_context *ctx __maybe_unused)
{
int i;
diff --git a/fec.c b/fec.c
index 22bc09f982a0..9cb4f8b1d4e1 100644
--- a/fec.c
+++ b/fec.c
@@ -194,7 +194,7 @@ static void fec_dump_reg_v2(int reg, u32 val)
#undef FIELD
#undef REG
-int fec_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int fec_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
const u32 *data = (u32 *)regs->data;
diff --git a/fec_8xx.c b/fec_8xx.c
index 02ecaef84364..63352fca36b8 100644
--- a/fec_8xx.c
+++ b/fec_8xx.c
@@ -47,7 +47,7 @@ struct fec {
(unsigned long)(offsetof(struct fec, x)), \
#x, f->x)
-int fec_8xx_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int fec_8xx_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
struct fec *f = (struct fec *)regs->data;
diff --git a/fjes.c b/fjes.c
index 4c5f6bc70843..05bd24511fb7 100644
--- a/fjes.c
+++ b/fjes.c
@@ -2,7 +2,7 @@
#include <stdio.h>
#include "internal.h"
-int fjes_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int fjes_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
u32 *regs_buff = (u32 *)regs->data;
diff --git a/ibm_emac.c b/ibm_emac.c
index 3259c175a43a..ea01d56f609c 100644
--- a/ibm_emac.c
+++ b/ibm_emac.c
@@ -314,7 +314,7 @@ static void *print_tah_regs(void *buf)
return p + 1;
}
-int ibm_emac_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int ibm_emac_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
struct emac_ethtool_regs_hdr *hdr =
diff --git a/igb.c b/igb.c
index 89b5cdb5d689..f358f53b74e0 100644
--- a/igb.c
+++ b/igb.c
@@ -88,7 +88,7 @@
#define E1000_TCTL_RTLC 0x01000000 /* Re-transmit on late collision */
#define E1000_TCTL_NRTU 0x02000000 /* No Re-transmit on underrun */
-int igb_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int igb_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
u32 *regs_buff = (u32 *)regs->data;
diff --git a/internal.h b/internal.h
index 1c6689a29c81..8ae1efab5b5c 100644
--- a/internal.h
+++ b/internal.h
@@ -26,7 +26,7 @@
#include "json_writer.h"
#include "json_print.h"
-#define maybe_unused __attribute__((__unused__))
+#define __maybe_unused __attribute__((__unused__))
/* internal for netlink interface */
#ifdef ETHTOOL_ENABLE_NETLINK
diff --git a/ixgb.c b/ixgb.c
index 7c16c6e76d44..8aec9a9d2258 100644
--- a/ixgb.c
+++ b/ixgb.c
@@ -38,7 +38,7 @@
#define IXGB_RAH_ASEL_SRC 0x00010000
#define IXGB_RAH_AV 0x80000000
-int ixgb_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int ixgb_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
u32 *regs_buff = (u32 *)regs->data;
diff --git a/ixgbe.c b/ixgbe.c
index 9754b2ad078f..6d509c87f357 100644
--- a/ixgbe.c
+++ b/ixgbe.c
@@ -168,7 +168,7 @@ ixgbe_get_mac_type(u16 device_id)
}
int
-ixgbe_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+ixgbe_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
u32 *regs_buff = (u32 *)regs->data;
diff --git a/ixgbevf.c b/ixgbevf.c
index 265e0bf740b3..91c2b2cd4f3c 100644
--- a/ixgbevf.c
+++ b/ixgbevf.c
@@ -3,7 +3,7 @@
#include "internal.h"
int
-ixgbevf_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+ixgbevf_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
u32 *regs_buff = (u32 *)regs->data;
diff --git a/lan78xx.c b/lan78xx.c
index 46ade1c417f9..75ee0487d5fc 100644
--- a/lan78xx.c
+++ b/lan78xx.c
@@ -2,7 +2,7 @@
#include <string.h>
#include "internal.h"
-int lan78xx_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int lan78xx_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
unsigned int *lan78xx_reg = (unsigned int *)regs->data;
diff --git a/marvell.c b/marvell.c
index 9e5440d73c12..8afb150327a3 100644
--- a/marvell.c
+++ b/marvell.c
@@ -259,7 +259,7 @@ static void dump_control(u8 *r)
printf("General Purpose I/O 0x%08X\n", *(u32 *) (r + 0x15c));
}
-int skge_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int skge_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
const u32 *r = (const u32 *) regs->data;
@@ -380,7 +380,7 @@ static void dump_prefetch(const char *name, const void *r)
}
}
-int sky2_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int sky2_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
const u16 *r16 = (const u16 *) regs->data;
diff --git a/natsemi.c b/natsemi.c
index ce82c426c178..0af465959cbc 100644
--- a/natsemi.c
+++ b/natsemi.c
@@ -323,7 +323,7 @@ static void __print_intr(int d, int intr, const char *name,
} while (0)
int
-natsemi_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+natsemi_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
u32 *data = (u32 *)regs->data;
@@ -964,7 +964,7 @@ natsemi_dump_regs(struct ethtool_drvinfo *info maybe_unused,
}
int
-natsemi_dump_eeprom(struct ethtool_drvinfo *info maybe_unused,
+natsemi_dump_eeprom(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_eeprom *ee)
{
int i;
diff --git a/realtek.c b/realtek.c
index d10cfd41dc95..ee0c6119dafa 100644
--- a/realtek.c
+++ b/realtek.c
@@ -241,7 +241,7 @@ print_intr_bits(u16 mask)
}
int
-realtek_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+realtek_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
u32 *data = (u32 *) regs->data;
diff --git a/sfc.c b/sfc.c
index f56243d449ec..340800ee0fa0 100644
--- a/sfc.c
+++ b/sfc.c
@@ -3890,7 +3890,8 @@ print_complex_table(unsigned revision, const struct efx_nic_reg_table *table,
}
int
-sfc_dump_regs(struct ethtool_drvinfo *info maybe_unused, struct ethtool_regs *regs)
+sfc_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
+ struct ethtool_regs *regs)
{
const struct efx_nic_reg *reg;
const struct efx_nic_reg_table *table;
diff --git a/smsc911x.c b/smsc911x.c
index bafee21485cf..b64350460451 100644
--- a/smsc911x.c
+++ b/smsc911x.c
@@ -2,7 +2,7 @@
#include <string.h>
#include "internal.h"
-int smsc911x_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int smsc911x_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
unsigned int *smsc_reg = (unsigned int *)regs->data;
diff --git a/stmmac.c b/stmmac.c
index 98d905835e16..58471200cd80 100644
--- a/stmmac.c
+++ b/stmmac.c
@@ -18,7 +18,7 @@
#define GMAC_REG_NUM 55
#define GMAC_DMA_REG_NUM 23
-int st_mac100_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int st_mac100_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
int i;
@@ -51,7 +51,7 @@ int st_mac100_dump_regs(struct ethtool_drvinfo *info maybe_unused,
return 0;
}
-int st_gmac_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int st_gmac_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
int i;
diff --git a/tg3.c b/tg3.c
index 8698391f63ea..ac73b33ae4e3 100644
--- a/tg3.c
+++ b/tg3.c
@@ -4,7 +4,7 @@
#define TG3_MAGIC 0x669955aa
-int tg3_dump_eeprom(struct ethtool_drvinfo *info maybe_unused,
+int tg3_dump_eeprom(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_eeprom *ee)
{
int i;
@@ -23,7 +23,7 @@ int tg3_dump_eeprom(struct ethtool_drvinfo *info maybe_unused,
return 0;
}
-int tg3_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int tg3_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
int i;
diff --git a/tse.c b/tse.c
index e5241ee4c98e..fb00d218ab8a 100644
--- a/tse.c
+++ b/tse.c
@@ -25,7 +25,7 @@ bitset(u32 val, int bit)
return 0;
}
-int altera_tse_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int altera_tse_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
int i;
diff --git a/vioc.c b/vioc.c
index ef163ab499f0..c04a6dc092f9 100644
--- a/vioc.c
+++ b/vioc.c
@@ -11,7 +11,7 @@ struct regs_line {
#define VIOC_REGS_LINE_SIZE sizeof(struct regs_line)
-int vioc_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+int vioc_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
unsigned int i;
diff --git a/vmxnet3.c b/vmxnet3.c
index c97214511e93..68726825a8ca 100644
--- a/vmxnet3.c
+++ b/vmxnet3.c
@@ -3,7 +3,7 @@
#include "internal.h"
int
-vmxnet3_dump_regs(struct ethtool_drvinfo *info maybe_unused,
+vmxnet3_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
struct ethtool_regs *regs)
{
u32 *regs_buff = (u32 *)regs->data;
--
2.28.0
^ permalink raw reply related
* [PATCH ethtool 2/7] cable_test: clean up unused parameters
From: Michal Kubecek @ 2020-08-03 11:57 UTC (permalink / raw)
To: netdev
In-Reply-To: <cover.1596451857.git.mkubecek@suse.cz>
Functions nl_cable_test_ntf_attr() and nl_cable_test_tdr_ntf_attr() do not
use nlctx parameter and as they are not callbacks with fixed signature, we
can simply drop it. Once we do, the same is true for cable_test_ntf_nest()
and cable_test_tdr_ntf_nest().
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
netlink/cable_test.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/netlink/cable_test.c b/netlink/cable_test.c
index c2b9c97d1239..d39b7d82efb0 100644
--- a/netlink/cable_test.c
+++ b/netlink/cable_test.c
@@ -88,8 +88,7 @@ static char *nl_pair2txt(uint8_t pair)
}
}
-static int nl_cable_test_ntf_attr(struct nlattr *evattr,
- struct nl_context *nlctx)
+static int nl_cable_test_ntf_attr(struct nlattr *evattr)
{
unsigned int cm;
uint16_t code;
@@ -122,14 +121,13 @@ static int nl_cable_test_ntf_attr(struct nlattr *evattr,
return 0;
}
-static void cable_test_ntf_nest(const struct nlattr *nest,
- struct nl_context *nlctx)
+static void cable_test_ntf_nest(const struct nlattr *nest)
{
struct nlattr *pos;
int ret;
mnl_attr_for_each_nested(pos, nest) {
- ret = nl_cable_test_ntf_attr(pos, nlctx);
+ ret = nl_cable_test_ntf_attr(pos);
if (ret < 0)
return;
}
@@ -180,7 +178,7 @@ static int cable_test_ntf_stop_cb(const struct nlmsghdr *nlhdr, void *data)
}
if (tb[ETHTOOL_A_CABLE_TEST_NTF_NEST])
- cable_test_ntf_nest(tb[ETHTOOL_A_CABLE_TEST_NTF_NEST], nlctx);
+ cable_test_ntf_nest(tb[ETHTOOL_A_CABLE_TEST_NTF_NEST]);
if (status == ETHTOOL_A_CABLE_TEST_NTF_STATUS_COMPLETED) {
if (ctctx)
@@ -339,8 +337,7 @@ static int nl_get_cable_test_tdr_step(const struct nlattr *nest,
return 0;
}
-static int nl_cable_test_tdr_ntf_attr(struct nlattr *evattr,
- struct nl_context *nlctx)
+static int nl_cable_test_tdr_ntf_attr(struct nlattr *evattr)
{
uint32_t first, last, step;
uint8_t pair;
@@ -391,14 +388,13 @@ static int nl_cable_test_tdr_ntf_attr(struct nlattr *evattr,
return 0;
}
-static void cable_test_tdr_ntf_nest(const struct nlattr *nest,
- struct nl_context *nlctx)
+static void cable_test_tdr_ntf_nest(const struct nlattr *nest)
{
struct nlattr *pos;
int ret;
mnl_attr_for_each_nested(pos, nest) {
- ret = nl_cable_test_tdr_ntf_attr(pos, nlctx);
+ ret = nl_cable_test_tdr_ntf_attr(pos);
if (ret < 0)
return;
}
@@ -450,8 +446,7 @@ int cable_test_tdr_ntf_stop_cb(const struct nlmsghdr *nlhdr, void *data)
}
if (tb[ETHTOOL_A_CABLE_TEST_TDR_NTF_NEST])
- cable_test_tdr_ntf_nest(tb[ETHTOOL_A_CABLE_TEST_TDR_NTF_NEST],
- nlctx);
+ cable_test_tdr_ntf_nest(tb[ETHTOOL_A_CABLE_TEST_TDR_NTF_NEST]);
if (status == ETHTOOL_A_CABLE_TEST_NTF_STATUS_COMPLETED) {
if (ctctx)
--
2.28.0
^ permalink raw reply related
* [PATCH ethtool 3/7] igc: mark unused callback parameter
From: Michal Kubecek @ 2020-08-03 11:57 UTC (permalink / raw)
To: netdev
In-Reply-To: <cover.1596451857.git.mkubecek@suse.cz>
Mark info parameter of igc_dump_regs() as unused to get rid of gcc warning.
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
igc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/igc.c b/igc.c
index 2c4abcef1e15..1550ac0c1c2b 100644
--- a/igc.c
+++ b/igc.c
@@ -94,7 +94,8 @@ static const char *bit_to_prio(u32 val)
return val ? "low" : "high";
}
-int igc_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs)
+int igc_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
+ struct ethtool_regs *regs)
{
u32 reg;
int offset, i;
--
2.28.0
^ permalink raw reply related
* [PATCH ethtool 4/7] netlink: mark unused callback parameter
From: Michal Kubecek @ 2020-08-03 11:57 UTC (permalink / raw)
To: netdev
In-Reply-To: <cover.1596451857.git.mkubecek@suse.cz>
Function nomsg_reply_cb() is used as a callback for mnl_cb_run() but it
does not use its data parameter; mark it as unused to get rid of compiler
warning.
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
netlink/netlink.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/netlink/netlink.c b/netlink/netlink.c
index 17b7788600d0..76b6e825b1d0 100644
--- a/netlink/netlink.c
+++ b/netlink/netlink.c
@@ -16,7 +16,7 @@
/* Used as reply callback for requests where no reply is expected (e.g. most
* "set" type commands)
*/
-int nomsg_reply_cb(const struct nlmsghdr *nlhdr, void *data)
+int nomsg_reply_cb(const struct nlmsghdr *nlhdr, void *data __maybe_unused)
{
const struct genlmsghdr *ghdr = (const struct genlmsghdr *)(nlhdr + 1);
--
2.28.0
^ permalink raw reply related
* [PATCH ethtool 5/7] netlink: mark unused parameters of bitset walker callbacks
From: Michal Kubecek @ 2020-08-03 11:57 UTC (permalink / raw)
To: netdev
In-Reply-To: <cover.1596451857.git.mkubecek@suse.cz>
Some callbacks used with walk_bitset() do not use all parameters passed to
them. Mark unused parameters explicitly to get rid of compiler warnings.
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
netlink/pause.c | 3 ++-
netlink/privflags.c | 2 +-
netlink/settings.c | 9 ++++++---
netlink/tsinfo.c | 2 +-
4 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/netlink/pause.c b/netlink/pause.c
index 48215d29aa34..7b6b3a1d2c10 100644
--- a/netlink/pause.c
+++ b/netlink/pause.c
@@ -21,7 +21,8 @@ struct pause_autoneg_status {
bool asym_pause;
};
-static void pause_autoneg_walker(unsigned int idx, const char *name, bool val,
+static void pause_autoneg_walker(unsigned int idx,
+ const char *name __maybe_unused, bool val,
void *data)
{
struct pause_autoneg_status *status = data;
diff --git a/netlink/privflags.c b/netlink/privflags.c
index a06cd6d88d9d..299ccdc21581 100644
--- a/netlink/privflags.c
+++ b/netlink/privflags.c
@@ -19,7 +19,7 @@
/* PRIVFLAGS_GET */
static void privflags_maxlen_walk_cb(unsigned int idx, const char *name,
- bool val, void *data)
+ bool val __maybe_unused, void *data)
{
unsigned int *maxlen = data;
unsigned int len, n;
diff --git a/netlink/settings.c b/netlink/settings.c
index 726259d83702..17ef000ed812 100644
--- a/netlink/settings.c
+++ b/netlink/settings.c
@@ -656,7 +656,8 @@ int linkstate_reply_cb(const struct nlmsghdr *nlhdr, void *data)
return MNL_CB_OK;
}
-void wol_modes_cb(unsigned int idx, const char *name, bool val, void *data)
+void wol_modes_cb(unsigned int idx, const char *name __maybe_unused, bool val,
+ void *data)
{
struct ethtool_wolinfo *wol = data;
@@ -704,7 +705,8 @@ int wol_reply_cb(const struct nlmsghdr *nlhdr, void *data)
return MNL_CB_OK;
}
-void msgmask_cb(unsigned int idx, const char *name, bool val, void *data)
+void msgmask_cb(unsigned int idx, const char *name __maybe_unused, bool val,
+ void *data)
{
u32 *msg_mask = data;
@@ -714,7 +716,8 @@ void msgmask_cb(unsigned int idx, const char *name, bool val, void *data)
*msg_mask |= (1U << idx);
}
-void msgmask_cb2(unsigned int idx, const char *name, bool val, void *data)
+void msgmask_cb2(unsigned int idx __maybe_unused, const char *name,
+ bool val, void *data __maybe_unused)
{
if (val)
printf(" %s", name);
diff --git a/netlink/tsinfo.c b/netlink/tsinfo.c
index 03ce91cd4314..c6571ffc16ff 100644
--- a/netlink/tsinfo.c
+++ b/netlink/tsinfo.c
@@ -16,7 +16,7 @@
/* TSINFO_GET */
static void tsinfo_dump_cb(unsigned int idx, const char *name, bool val,
- void *data)
+ void *data __maybe_unused)
{
if (!val)
return;
--
2.28.0
^ permalink raw reply related
* [PATCH ethtool 6/7] netlink: mark unused parameters of parser callbacks
From: Michal Kubecek @ 2020-08-03 11:57 UTC (permalink / raw)
To: netdev
In-Reply-To: <cover.1596451857.git.mkubecek@suse.cz>
Some calbacks used with nl_parser() do not use all parameters passed to
them. Mark unused parameters explicitly to get rid of compiler warnings.
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
netlink/parser.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/netlink/parser.c b/netlink/parser.c
index f152a8268f0b..395bd5743af9 100644
--- a/netlink/parser.c
+++ b/netlink/parser.c
@@ -155,8 +155,9 @@ static int lookup_u8(const char *arg, uint8_t *result,
/* Parser handler for a flag. Expects a name (with no additional argument),
* generates NLA_FLAG or sets a bool (if the name was present).
*/
-int nl_parse_flag(struct nl_context *nlctx, uint16_t type, const void *data,
- struct nl_msg_buff *msgbuff, void *dest)
+int nl_parse_flag(struct nl_context *nlctx __maybe_unused, uint16_t type,
+ const void *data __maybe_unused, struct nl_msg_buff *msgbuff,
+ void *dest)
{
if (dest)
*(bool *)dest = true;
@@ -166,7 +167,8 @@ int nl_parse_flag(struct nl_context *nlctx, uint16_t type, const void *data,
/* Parser handler for null terminated string. Expects a string argument,
* generates NLA_NUL_STRING or fills const char *
*/
-int nl_parse_string(struct nl_context *nlctx, uint16_t type, const void *data,
+int nl_parse_string(struct nl_context *nlctx, uint16_t type,
+ const void *data __maybe_unused,
struct nl_msg_buff *msgbuff, void *dest)
{
const char *arg = *nlctx->argp;
@@ -183,8 +185,8 @@ int nl_parse_string(struct nl_context *nlctx, uint16_t type, const void *data,
* (may use 0x prefix), generates NLA_U32 or fills an uint32_t.
*/
int nl_parse_direct_u32(struct nl_context *nlctx, uint16_t type,
- const void *data, struct nl_msg_buff *msgbuff,
- void *dest)
+ const void *data __maybe_unused,
+ struct nl_msg_buff *msgbuff, void *dest)
{
const char *arg = *nlctx->argp;
uint32_t val;
@@ -207,8 +209,8 @@ int nl_parse_direct_u32(struct nl_context *nlctx, uint16_t type,
* (may use 0x prefix), generates NLA_U32 or fills an uint32_t.
*/
int nl_parse_direct_u8(struct nl_context *nlctx, uint16_t type,
- const void *data, struct nl_msg_buff *msgbuff,
- void *dest)
+ const void *data __maybe_unused,
+ struct nl_msg_buff *msgbuff, void *dest)
{
const char *arg = *nlctx->argp;
uint8_t val;
@@ -231,8 +233,8 @@ int nl_parse_direct_u8(struct nl_context *nlctx, uint16_t type,
* NLA_U32 or fills an uint32_t.
*/
int nl_parse_direct_m2cm(struct nl_context *nlctx, uint16_t type,
- const void *data, struct nl_msg_buff *msgbuff,
- void *dest)
+ const void *data __maybe_unused,
+ struct nl_msg_buff *msgbuff, void *dest)
{
const char *arg = *nlctx->argp;
float meters;
@@ -256,7 +258,8 @@ int nl_parse_direct_m2cm(struct nl_context *nlctx, uint16_t type,
/* Parser handler for (tri-state) bool. Expects "name on|off", generates
* NLA_U8 which is 1 for "on" and 0 for "off".
*/
-int nl_parse_u8bool(struct nl_context *nlctx, uint16_t type, const void *data,
+int nl_parse_u8bool(struct nl_context *nlctx, uint16_t type,
+ const void *data __maybe_unused,
struct nl_msg_buff *msgbuff, void *dest)
{
const char *arg = *nlctx->argp;
@@ -463,8 +466,9 @@ err:
* error_parser_params (error message, return value and number of extra
* arguments to skip).
*/
-int nl_parse_error(struct nl_context *nlctx, uint16_t type, const void *data,
- struct nl_msg_buff *msgbuff, void *dest)
+int nl_parse_error(struct nl_context *nlctx, uint16_t type __maybe_unused,
+ const void *data, struct nl_msg_buff *msgbuff __maybe_unused,
+ void *dest __maybe_unused)
{
const struct error_parser_data *parser_data = data;
unsigned int skip = parser_data->extra_args;
--
2.28.0
^ permalink raw reply related
* [PATCH ethtool 7/7] ioctl: avoid zero length array warning in get_stringset()
From: Michal Kubecek @ 2020-08-03 11:57 UTC (permalink / raw)
To: netdev
In-Reply-To: <cover.1596451857.git.mkubecek@suse.cz>
Starting with gcc10, gcc issues a warning about accessing elements of
zero leghth arrays. This is usually fixed by using C99 variable length
arrays but struct ethtool_sset_info is part of kernel UAPI so use an
auxiliary pointer instead.
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
ethtool.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/ethtool.c b/ethtool.c
index 0f312bdae2bb..c4ad186cd390 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -1632,7 +1632,9 @@ get_stringset(struct cmd_context *ctx, enum ethtool_stringset set_id,
sset_info.hdr.reserved = 0;
sset_info.hdr.sset_mask = 1ULL << set_id;
if (send_ioctl(ctx, &sset_info) == 0) {
- len = sset_info.hdr.sset_mask ? sset_info.hdr.data[0] : 0;
+ const u32 *sset_lengths = sset_info.hdr.data;
+
+ len = sset_info.hdr.sset_mask ? sset_lengths[0] : 0;
} else if (errno == EOPNOTSUPP && drvinfo_offset != 0) {
/* Fallback for old kernel versions */
drvinfo.cmd = ETHTOOL_GDRVINFO;
--
2.28.0
^ permalink raw reply related
* Re: [PATCH bpf-next v3 00/29] bpf: switch to memcg-based memory accounting
From: Daniel Borkmann @ 2020-08-03 12:05 UTC (permalink / raw)
To: Roman Gushchin, bpf; +Cc: netdev, Alexei Starovoitov, kernel-team, linux-kernel
In-Reply-To: <20200730212310.2609108-1-guro@fb.com>
On 7/30/20 11:22 PM, Roman Gushchin wrote:
> Currently bpf is using the memlock rlimit for the memory accounting.
> This approach has its downsides and over time has created a significant
> amount of problems:
>
> 1) The limit is per-user, but because most bpf operations are performed
> as root, the limit has a little value.
>
> 2) It's hard to come up with a specific maximum value. Especially because
> the counter is shared with non-bpf users (e.g. memlock() users).
> Any specific value is either too low and creates false failures
> or too high and useless.
>
> 3) Charging is not connected to the actual memory allocation. Bpf code
> should manually calculate the estimated cost and precharge the counter,
> and then take care of uncharging, including all fail paths.
> It adds to the code complexity and makes it easy to leak a charge.
>
> 4) There is no simple way of getting the current value of the counter.
> We've used drgn for it, but it's far from being convenient.
>
> 5) Cryptic -EPERM is returned on exceeding the limit. Libbpf even had
> a function to "explain" this case for users.
>
> In order to overcome these problems let's switch to the memcg-based
> memory accounting of bpf objects. With the recent addition of the percpu
> memory accounting, now it's possible to provide a comprehensive accounting
> of memory used by bpf programs and maps.
>
> This approach has the following advantages:
> 1) The limit is per-cgroup and hierarchical. It's way more flexible and allows
> a better control over memory usage by different workloads.
>
> 2) The actual memory consumption is taken into account. It happens automatically
> on the allocation time if __GFP_ACCOUNT flags is passed. Uncharging is also
> performed automatically on releasing the memory. So the code on the bpf side
> becomes simpler and safer.
>
> 3) There is a simple way to get the current value and statistics.
>
> The patchset consists of the following parts:
> 1) memcg-based accounting for various bpf objects: progs and maps
> 2) removal of the rlimit-based accounting
> 3) removal of rlimit adjustments in userspace samples
The diff stat looks nice & agree that rlimit sucks, but I'm missing how this is set
is supposed to work reliably, at least I currently fail to see it. Elaborating on this
in more depth especially for the case of unprivileged users should be a /fundamental/
part of the commit message.
Lets take an example: unprivileged user adds a max sized hashtable to one of its
programs, and configures the map that it will perform runtime allocation. The load
succeeds as it doesn't surpass the limits set for the current memcg. Kernel then
processes packets from softirq. Given the runtime allocations, we end up mischarging
to whoever ended up triggering __do_softirq(). If, for example, ksoftirq thread, then
it's probably reasonable to assume that this might not be accounted e.g. limits are
not imposed on the root cgroup. If so we would probably need to drag the context of
/where/ this must be charged to __memcg_kmem_charge_page() to do it reliably. Otherwise
how do you protect unprivileged users to OOM the machine?
Similarly, what happens to unprivileged users if kmemcg was not configured into the
kernel or has been disabled?
Thanks,
Daniel
^ permalink raw reply
* 答复: [PATCH] qmi_wwan: support modify usbnet's rx_urb_size
From: Carl Yin(殷张成) @ 2020-08-03 12:08 UTC (permalink / raw)
To: Bjørn Mork, Daniele Palmas
Cc: Greg KH, yzc666@netease.com, David Miller, kuba@kernel.org,
netdev@vger.kernel.org, linux-usb
In-Reply-To: <87r1so9fzl.fsf@miraculix.mork.no>
bjorn@mork.no writes:
> Daniele Palmas <dnlplm@gmail.com> writes:
> > Il giorno lun 3 ago 2020 alle ore 10:18 Greg KH
> > <gregkh@linuxfoundation.org> ha scritto:
> >
> >> Actually, no, this all should be done "automatically", do not change
> >> the urb size on the fly. Change it at probe time based on the device
> >> you are using, do not force userspace to "know" what to do here, as
> >> it will not know that at all.
> >>
> >
> > the problem with doing at probe time is that rx_urb_size is not fixed,
> > but depends on the configuration done at the userspace level with
> > QMI_WDA_SET_DATA_FORMAT, so the userspace knows that.
>
> Yes, but the driver "will know" (or "may assume") this based on the
> QMI_WWAN_FLAG_MUX flag, as long as we are using the driver internal
> (de)muxing. We should be able to automatically set a sane rx_urb_size value
> based on this?
>
> Not sure if the rmnet driver currently can be used on top of qmi_wwan?
> That will obviously need some other workaround.
>
> > Currently there's a workaround for setting rx_urb_size i.e. changing
> > the network interface MTU: this is fine for most uses with qmap, but
> > there's the limitation that certain values (multiple of the endpoint
> > size) are not allowed.
>
> And this also requires an additional setup step for user/userspace, which we
> should try to avoid if possible.
>
> I'm all for a fully automatic solution. I don't think rx_urb_size should be directly
> configurable. And it it were, then it should be implemented in the usbnet
> framework. It is not a qmi_wwan specific attribute.
Hi Bjørn,
You can check cdc_ncm.c.
cdc ncm driver set 'rx_urb_size' on driver probe time, and the value read from' cdc_ncm_bind_common() 's USB_CDC_GET_NTB_FORMAT '.
and also allow the userspace to modify 'rx_urb_size' by ' /sys/class/net/wwan0/cdc_ncm/rx_max'.
>
>
> Bjørn
^ permalink raw reply
* pull-request: mac80211-next 2020-08-03
From: Johannes Berg @ 2020-08-03 12:15 UTC (permalink / raw)
To: netdev; +Cc: linux-wireless
Hi Dave,
Not sure you'll still take a few stragglers, but if you're
going to make a last pass then having a few more things in
would be nice. One (the while -> if) is something I'd even
send as a bugfix later, the rest are just nice to have. :)
Please pull (if you still want) and let me know if there's
any problem.
Thanks,
johannes
The following changes since commit bd0b33b24897ba9ddad221e8ac5b6f0e38a2e004:
Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net (2020-08-02 01:02:12 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git tags/mac80211-next-for-davem-2020-08-03
for you to fetch changes up to 0b91111fb1a164fedbb68a9263afafd10ffa6ec3:
mac80211: Do not report beacon loss if beacon filtering enabled (2020-08-03 13:02:06 +0200)
----------------------------------------------------------------
A few more changes, notably:
* handle new SAE (WPA3 authentication) status codes in the correct way
* fix a while that should be an if instead, avoiding infinite loops
* handle beacon filtering changing better
----------------------------------------------------------------
Johannes Berg (1):
mac80211: fix misplaced while instead of if
John Crispin (1):
mac8211: fix struct initialisation
Jouni Malinen (1):
mac80211: Handle special status codes in SAE commit
Loic Poulain (1):
mac80211: Do not report beacon loss if beacon filtering enabled
Miaohe Lin (2):
mac80211: use eth_zero_addr() to clear mac address
nl80211: use eth_zero_addr() to clear mac address
include/linux/ieee80211.h | 2 ++
net/mac80211/agg-rx.c | 2 +-
net/mac80211/mlme.c | 8 +++++++-
net/mac80211/sta_info.c | 2 +-
net/mac80211/trace.h | 3 ++-
net/wireless/nl80211.c | 3 +--
6 files changed, 14 insertions(+), 6 deletions(-)
^ permalink raw reply
* Re: [PATCH net-next RFC 00/13] Add devlink reload level option
From: Moshe Shemesh @ 2020-08-03 12:17 UTC (permalink / raw)
To: Vasundhara Volam, Jacob Keller
Cc: David S. Miller, Jiri Pirko, Netdev, open list
In-Reply-To: <CAACQVJo+bAr_k=LjgdTKbOxFEkpbYAsaWbkSDjUepgO7_XQfNA@mail.gmail.com>
On 8/3/2020 1:24 PM, Vasundhara Volam wrote:
> On Tue, Jul 28, 2020 at 10:13 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
>>
>>
>> On 7/27/2020 10:25 PM, Vasundhara Volam wrote:
>>> On Mon, Jul 27, 2020 at 4:36 PM Moshe Shemesh <moshe@mellanox.com> wrote:
>>>> Introduce new option on devlink reload API to enable the user to select the
>>>> reload level required. Complete support for all levels in mlx5.
>>>> The following reload levels are supported:
>>>> driver: Driver entities re-instantiation only.
>>>> fw_reset: Firmware reset and driver entities re-instantiation.
>>> The Name is a little confusing. I think it should be renamed to
>>> fw_live_reset (in which both firmware and driver entities are
>>> re-instantiated). For only fw_reset, the driver should not undergo
>>> reset (it requires a driver reload for firmware to undergo reset).
>>>
>> So, I think the differentiation here is that "live_patch" doesn't reset
>> anything.
> This seems similar to flashing the firmware and does not reset anything.
The live patch is activating fw change without reset.
It is not suitable for any fw change but fw gaps which don't require reset.
I can query the fw to check if the pending image change is suitable or
require fw reset.
>>>> fw_live_patch: Firmware live patching only.
>>> This level is not clear. Is this similar to flashing??
>>>
>>> Also I have a basic query. The reload command is split into
>>> reload_up/reload_down handlers (Please correct me if this behaviour is
>>> changed with this patchset). What if the vendor specific driver does
>>> not support up/down and needs only a single handler to fire a firmware
>>> reset or firmware live reset command?
>> In the "reload_down" handler, they would trigger the appropriate reset,
>> and quiesce anything that needs to be done. Then on reload up, it would
>> restore and bring up anything quiesced in the first stage.
> Yes, I got the "reload_down" and "reload_up". Similar to the device
> "remove" and "re-probe" respectively.
>
> But our requirement is a similar "ethtool reset" command, where
> ethtool calls a single callback in driver and driver just sends a
> firmware command for doing the reset. Once firmware receives the
> command, it will initiate the reset of driver and firmware entities
> asynchronously.
It is similar to mlx5 case here for fw_reset. The driver triggers the fw
command to reset and all PFs drivers gets events to handle and do
re-initialization. To fit it to the devlink reload_down and reload_up,
I wait for the event handler to complete and it stops at driver unload
to have the driver up by devlink reload_up. See patch 8 in this patchset.
^ permalink raw reply
* Re: [Linux-kernel-mentees] [PATCH net] net/smc: Prevent kernel-infoleak in __smc_diag_dump()
From: Ursula Braun @ 2020-08-03 12:18 UTC (permalink / raw)
To: Peilin Ye, Karsten Graul
Cc: David S. Miller, Jakub Kicinski, Dan Carpenter, Arnd Bergmann,
Greg Kroah-Hartman, linux-s390, netdev, linux-kernel-mentees,
linux-kernel
In-Reply-To: <20200801194440.246747-1-yepeilin.cs@gmail.com>
On 8/1/20 9:44 PM, Peilin Ye wrote:
> __smc_diag_dump() is potentially copying uninitialized kernel stack memory
> into socket buffers, since the compiler may leave a 4-byte hole near the
> beginning of `struct smcd_diag_dmbinfo`. Fix it by initializing `dinfo`
> with memset().
>
> Cc: stable@vger.kernel.org
> Fixes: 4b1b7d3b30a6 ("net/smc: add SMC-D diag support")
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com>
> ---
> Reference: https://lwn.net/Articles/417989/
>
> $ pahole -C "smcd_diag_dmbinfo" net/smc/smc_diag.o
> struct smcd_diag_dmbinfo {
> __u32 linkid; /* 0 4 */
>
> /* XXX 4 bytes hole, try to pack */
>
> __u64 peer_gid __attribute__((__aligned__(8))); /* 8 8 */
> __u64 my_gid __attribute__((__aligned__(8))); /* 16 8 */
> __u64 token __attribute__((__aligned__(8))); /* 24 8 */
> __u64 peer_token __attribute__((__aligned__(8))); /* 32 8 */
>
> /* size: 40, cachelines: 1, members: 5 */
> /* sum members: 36, holes: 1, sum holes: 4 */
> /* forced alignments: 4, forced holes: 1, sum forced holes: 4 */
> /* last cacheline: 40 bytes */
> } __attribute__((__aligned__(8)));
> $ _
>
Thanks, patch is added to our local library and will be part of our
next shipment of smc patches for the net-tree.
Regards, Ursula
> net/smc/smc_diag.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
> index e1f64f4ba236..da9ba6d1679b 100644
> --- a/net/smc/smc_diag.c
> +++ b/net/smc/smc_diag.c
> @@ -170,13 +170,15 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
> (req->diag_ext & (1 << (SMC_DIAG_DMBINFO - 1))) &&
> !list_empty(&smc->conn.lgr->list)) {
> struct smc_connection *conn = &smc->conn;
> - struct smcd_diag_dmbinfo dinfo = {
> - .linkid = *((u32 *)conn->lgr->id),
> - .peer_gid = conn->lgr->peer_gid,
> - .my_gid = conn->lgr->smcd->local_gid,
> - .token = conn->rmb_desc->token,
> - .peer_token = conn->peer_token
> - };
> + struct smcd_diag_dmbinfo dinfo;
> +
> + memset(&dinfo, 0, sizeof(dinfo));
> +
> + dinfo.linkid = *((u32 *)conn->lgr->id);
> + dinfo.peer_gid = conn->lgr->peer_gid;
> + dinfo.my_gid = conn->lgr->smcd->local_gid;
> + dinfo.token = conn->rmb_desc->token;
> + dinfo.peer_token = conn->peer_token;
>
> if (nla_put(skb, SMC_DIAG_DMBINFO, sizeof(dinfo), &dinfo) < 0)
> goto errout;
>
^ permalink raw reply
* [PATCH 5.7 095/120] iwlwifi: fix crash in iwl_dbg_tlv_alloc_trigger
From: Greg Kroah-Hartman @ 2020-08-03 12:19 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Jiri Slaby, Dieter Nützel,
Johannes Berg, Emmanuel Grumbach, Luca Coelho,
Intel Linux Wireless, Kalle Valo, David S. Miller, Jakub Kicinski,
linux-wireless, netdev, Sasha Levin
In-Reply-To: <20200803121902.860751811@linuxfoundation.org>
From: Jiri Slaby <jslaby@suse.cz>
[ Upstream commit ea0cca61d628662e4a1b26c77c7646f9a0257069 ]
The tlv passed to iwl_dbg_tlv_alloc_trigger comes from a loaded firmware
file. The memory can be marked as read-only as firmware could be
shared. In anyway, writing to this memory is not expected. So,
iwl_dbg_tlv_alloc_trigger can crash now:
BUG: unable to handle page fault for address: ffffae2c01bfa794
PF: supervisor write access in kernel mode
PF: error_code(0x0003) - permissions violation
PGD 107d51067 P4D 107d51067 PUD 107d52067 PMD 659ad2067 PTE 8000000662298161
CPU: 2 PID: 161 Comm: kworker/2:1 Not tainted 5.7.0-3.gad96a07-default #1 openSUSE Tumbleweed (unreleased)
RIP: 0010:iwl_dbg_tlv_alloc_trigger+0x25/0x60 [iwlwifi]
Code: eb f2 0f 1f 00 66 66 66 66 90 83 7e 04 33 48 89 f8 44 8b 46 10 48 89 f7 76 40 41 8d 50 ff 83 fa 19 77 23 8b 56 20 85 d2 75 07 <c7> 46 20 ff ff ff ff 4b 8d 14 40 48 c1 e2 04 48 8d b4 10 00 05 00
RSP: 0018:ffffae2c00417ce8 EFLAGS: 00010246
RAX: ffff8f0522334018 RBX: ffff8f0522334018 RCX: ffffffffc0fc26c0
RDX: 0000000000000000 RSI: ffffae2c01bfa774 RDI: ffffae2c01bfa774
RBP: 0000000000000000 R08: 0000000000000004 R09: 0000000000000001
R10: 0000000000000034 R11: ffffae2c01bfa77c R12: ffff8f0522334230
R13: 0000000001000009 R14: ffff8f0523fdbc00 R15: ffff8f051f395800
FS: 0000000000000000(0000) GS:ffff8f0527c80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffae2c01bfa794 CR3: 0000000389eba000 CR4: 00000000000006e0
Call Trace:
iwl_dbg_tlv_alloc+0x79/0x120 [iwlwifi]
iwl_parse_tlv_firmware.isra.0+0x57d/0x1550 [iwlwifi]
iwl_req_fw_callback+0x3f8/0x6a0 [iwlwifi]
request_firmware_work_func+0x47/0x90
process_one_work+0x1e3/0x3b0
worker_thread+0x46/0x340
kthread+0x115/0x140
ret_from_fork+0x1f/0x40
As can be seen, write bit is not set in the PTE. Read of
trig->occurrences succeeds in iwl_dbg_tlv_alloc_trigger, but
trig->occurrences = cpu_to_le32(-1); fails there, obviously.
This is likely because we (at SUSE) use compressed firmware and that is
marked as RO after decompression (see fw_map_paged_buf).
Fix it by creating a temporary buffer in case we need to change the
memory.
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Reported-by: Dieter Nützel <Dieter@nuetzel-hh.de>
Tested-by: Dieter Nützel <Dieter@nuetzel-hh.de>
Cc: Johannes Berg <johannes.berg@intel.com>
Cc: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Cc: Luca Coelho <luciano.coelho@intel.com>
Cc: Intel Linux Wireless <linuxwifi@intel.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
Link: https://lore.kernel.org/r/20200612073800.27742-1-jslaby@suse.cz
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
index bf2f00b892140..85b132a77787d 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
@@ -263,6 +263,8 @@ static int iwl_dbg_tlv_alloc_trigger(struct iwl_trans *trans,
{
struct iwl_fw_ini_trigger_tlv *trig = (void *)tlv->data;
u32 tp = le32_to_cpu(trig->time_point);
+ struct iwl_ucode_tlv *dup = NULL;
+ int ret;
if (le32_to_cpu(tlv->length) < sizeof(*trig))
return -EINVAL;
@@ -275,10 +277,20 @@ static int iwl_dbg_tlv_alloc_trigger(struct iwl_trans *trans,
return -EINVAL;
}
- if (!le32_to_cpu(trig->occurrences))
+ if (!le32_to_cpu(trig->occurrences)) {
+ dup = kmemdup(tlv, sizeof(*tlv) + le32_to_cpu(tlv->length),
+ GFP_KERNEL);
+ if (!dup)
+ return -ENOMEM;
+ trig = (void *)dup->data;
trig->occurrences = cpu_to_le32(-1);
+ tlv = dup;
+ }
+
+ ret = iwl_dbg_tlv_add(tlv, &trans->dbg.time_point[tp].trig_list);
+ kfree(dup);
- return iwl_dbg_tlv_add(tlv, &trans->dbg.time_point[tp].trig_list);
+ return ret;
}
static int (*dbg_tlv_alloc[])(struct iwl_trans *trans,
--
2.25.1
^ permalink raw reply related
* Re: [PATCH net-next RFC 00/13] Add devlink reload level option
From: Vasundhara Volam @ 2020-08-03 12:47 UTC (permalink / raw)
To: Moshe Shemesh
Cc: Jacob Keller, David S. Miller, Jiri Pirko, Netdev, open list
In-Reply-To: <7a9c315f-fa29-7bd5-31be-3748b8841b29@mellanox.com>
On Mon, Aug 3, 2020 at 5:47 PM Moshe Shemesh <moshe@mellanox.com> wrote:
>
>
> On 8/3/2020 1:24 PM, Vasundhara Volam wrote:
> > On Tue, Jul 28, 2020 at 10:13 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
> >>
> >>
> >> On 7/27/2020 10:25 PM, Vasundhara Volam wrote:
> >>> On Mon, Jul 27, 2020 at 4:36 PM Moshe Shemesh <moshe@mellanox.com> wrote:
> >>>> Introduce new option on devlink reload API to enable the user to select the
> >>>> reload level required. Complete support for all levels in mlx5.
> >>>> The following reload levels are supported:
> >>>> driver: Driver entities re-instantiation only.
> >>>> fw_reset: Firmware reset and driver entities re-instantiation.
> >>> The Name is a little confusing. I think it should be renamed to
> >>> fw_live_reset (in which both firmware and driver entities are
> >>> re-instantiated). For only fw_reset, the driver should not undergo
> >>> reset (it requires a driver reload for firmware to undergo reset).
> >>>
> >> So, I think the differentiation here is that "live_patch" doesn't reset
> >> anything.
> > This seems similar to flashing the firmware and does not reset anything.
>
>
> The live patch is activating fw change without reset.
>
> It is not suitable for any fw change but fw gaps which don't require reset.
>
> I can query the fw to check if the pending image change is suitable or
> require fw reset.
Okay.
>
> >>>> fw_live_patch: Firmware live patching only.
> >>> This level is not clear. Is this similar to flashing??
> >>>
> >>> Also I have a basic query. The reload command is split into
> >>> reload_up/reload_down handlers (Please correct me if this behaviour is
> >>> changed with this patchset). What if the vendor specific driver does
> >>> not support up/down and needs only a single handler to fire a firmware
> >>> reset or firmware live reset command?
> >> In the "reload_down" handler, they would trigger the appropriate reset,
> >> and quiesce anything that needs to be done. Then on reload up, it would
> >> restore and bring up anything quiesced in the first stage.
> > Yes, I got the "reload_down" and "reload_up". Similar to the device
> > "remove" and "re-probe" respectively.
> >
> > But our requirement is a similar "ethtool reset" command, where
> > ethtool calls a single callback in driver and driver just sends a
> > firmware command for doing the reset. Once firmware receives the
> > command, it will initiate the reset of driver and firmware entities
> > asynchronously.
>
>
> It is similar to mlx5 case here for fw_reset. The driver triggers the fw
> command to reset and all PFs drivers gets events to handle and do
> re-initialization. To fit it to the devlink reload_down and reload_up,
> I wait for the event handler to complete and it stops at driver unload
> to have the driver up by devlink reload_up. See patch 8 in this patchset.
>
Yes, I see reload_down is triggering the reset. In our driver, after
triggering the reset through a firmware command, reset is done in
another context as the driver initiates the reset only after receiving
an ASYNC event from the firmware.
Probably, we have to use reload_down() to send firmware command to
trigger reset and do nothing in reload_up. And returning from reload
does not mean that reset is complete as it is done in another context
and the driver notifies the health reporter once the reset is
complete. devlink framework may have to allow drivers to implement
reload_down only to look more clean or call reload_up only if the
driver notifies the devlink once reset is completed from another
context. Please suggest.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox