* Re: [PATCH net 0/9] several fixups for virtio-net XDP
From: Jason Wang @ 2016-12-26 2:39 UTC (permalink / raw)
To: John Fastabend, mst, virtualization, netdev, linux-kernel
Cc: john.r.fastabend
In-Reply-To: <585D5A71.4010106@gmail.com>
On 2016年12月24日 01:10, John Fastabend wrote:
> On 16-12-23 06:37 AM, Jason Wang wrote:
>> Merry Xmas and a Happy New year to all:
>>
>> This series tries to fixes several issues for virtio-net XDP which
>> could be categorized into several parts:
>>
>> - fix several issues during XDP linearizing
>> - allow csumed packet to work for XDP_PASS
>> - make EWMA rxbuf size estimation works for XDP
>> - forbid XDP when GUEST_UFO is support
>> - remove big packet XDP support
>> - add XDP support or small buffer
>>
>> Please see individual patches for details.
>>
>> Thanks
>>
>> Jason Wang (9):
>> virtio-net: remove the warning before XDP linearizing
>> virtio-net: correctly xmit linearized page on XDP_TX
>> virtio-net: fix page miscount during XDP linearizing
>> virtio-net: correctly handle XDP_PASS for linearized packets
>> virtio-net: unbreak csumed packets for XDP_PASS
>> virtio-net: make rx buf size estimation works for XDP
>> virtio-net: forbid XDP when VIRTIO_NET_F_GUEST_UFO is support
>> virtio-net: remove big packet XDP codes
>> virtio-net: XDP support for small buffers
>>
>> drivers/net/virtio_net.c | 172 ++++++++++++++++++++++++++++-------------------
>> 1 file changed, 102 insertions(+), 70 deletions(-)
>>
> Thanks a lot Jason. The last piece that is needed is support to
> complete XDP support is to get the adjust_head part correct. I'll
> send out a patch in a bit but will need to merge it on top of this
> set.
>
> .John
Yes, glad to see the your patch.
Thanks.
^ permalink raw reply
* Re: [PATCH net 7/9] virtio-net: forbid XDP when VIRTIO_NET_F_GUEST_UFO is support
From: Jason Wang @ 2016-12-26 2:38 UTC (permalink / raw)
To: John Fastabend, mst, virtualization, netdev, linux-kernel
Cc: john.r.fastabend
In-Reply-To: <585D4C5E.5050908@gmail.com>
On 2016年12月24日 00:10, John Fastabend wrote:
> On 16-12-23 08:02 AM, John Fastabend wrote:
>> On 16-12-23 06:37 AM, Jason Wang wrote:
>>> When VIRTIO_NET_F_GUEST_UFO is negotiated, host could still send UFO
>>> packet that exceeds a single page which could not be handled
>>> correctly by XDP. So this patch forbids setting XDP when GUEST_UFO is
>>> supported. While at it, forbid XDP for ECN (which comes only from GRO)
>>> too to prevent user from misconfiguration.
>>>
> Is sending packets greater than single page though normal in this case?
Yes, when NETIF_F_UFO was enabled for tap, it won't segment UFO packet
and will send it directly to guest. (This could be reproduced with
UDP_STREAM between two guests or host to guest).
Thanks
> I don't have any need to support big packet mode other than MST asked
> for it. And I wasn't seeing this in my tests. MTU is capped at 4k - hdr
> when XDP is enabled.
>
> .John
>
>>> Cc: John Fastabend <john.r.fastabend@intel.com>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> ---
>>> drivers/net/virtio_net.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 77ae358..c1f66d8 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -1684,7 +1684,9 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
>>> int i, err;
>>>
>>> if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
>>> - virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6)) {
>>> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
>>> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
>>> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO)) {
>>> netdev_warn(dev, "can't set XDP while host is implementing LRO, disable LRO first\n");
>>> return -EOPNOTSUPP;
>>> }
>>>
>> Acked-by: John Fastabend <john.r.fastabend@intel.com>
>>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net 4/9] virtio-net: correctly handle XDP_PASS for linearized packets
From: Jason Wang @ 2016-12-26 2:34 UTC (permalink / raw)
To: John Fastabend, mst, virtualization, netdev, linux-kernel
Cc: john.r.fastabend
In-Reply-To: <585D4966.9060605@gmail.com>
On 2016年12月23日 23:57, John Fastabend wrote:
> On 16-12-23 06:37 AM, Jason Wang wrote:
>> When XDP_PASS were determined for linearized packets, we try to get
>> new buffers in the virtqueue and build skbs from them. This is wrong,
>> we should create skbs based on existed buffers instead. Fixing them by
>> creating skb based on xdp_page.
>>
>> With this patch "ping 192.168.100.4 -s 3900 -M do" works for XDP_PASS.
>>
>> Cc: John Fastabend <john.r.fastabend@intel.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> drivers/net/virtio_net.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 58ad40e..470293e 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -578,8 +578,14 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> act = do_xdp_prog(vi, rq, xdp_prog, xdp_page, offset, len);
>> switch (act) {
>> case XDP_PASS:
>> - if (unlikely(xdp_page != page))
>> - __free_pages(xdp_page, 0);
>> + /* We can only create skb based on xdp_page. */
>> + if (unlikely(xdp_page != page)) {
>> + rcu_read_unlock();
>> + put_page(page);
>> + head_skb = page_to_skb(vi, rq, xdp_page,
>> + 0, len, PAGE_SIZE);
>> + return head_skb;
>> + }
>> break;
>> case XDP_TX:
>> if (unlikely(xdp_page != page))
>>
> Great thanks. This was likely working before because of the memory
> leak fixed in 3/9.
Looks not, without this and 3/9 the code will try to get buffers and
build skb for a new packet instead of existed buffers.
Thanks
>
> Acked-by: John Fastabend <john.r.fastabend@intel.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net 3/9] virtio-net: fix page miscount during XDP linearizing
From: Jason Wang @ 2016-12-26 2:30 UTC (permalink / raw)
To: John Fastabend, mst, virtualization, netdev, linux-kernel
Cc: john.r.fastabend
In-Reply-To: <585D48A7.3040701@gmail.com>
On 2016年12月23日 23:54, John Fastabend wrote:
> On 16-12-23 06:37 AM, Jason Wang wrote:
>> We don't put page during linearizing, the would cause leaking when
>> xmit through XDP_TX or the packet exceeds PAGE_SIZE. Fix them by
>> put page accordingly. Also decrease the number of buffers during
>> linearizing to make sure caller can free buffers correctly when packet
>> exceeds PAGE_SIZE. With this patch, we won't get OOM after linearize
>> huge number of packets.
>>
>> Cc: John Fastabend <john.r.fastabend@intel.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
> Thanks! looks good. By the way do you happen to have any actual
> configuration where this path is hit? I obviously didn't test this
> very long other than a quick test with my hacked vhost driver.
>
> Acked-by: John Fastabend <john.r.fastabend@intel.com>
Yes, I have. Just increase the MTU above 1500 for both virtio and tap
and produce some traffic with size which will lead underestimated of rxbuf.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
From: Pali Rohár @ 2016-12-25 20:46 UTC (permalink / raw)
To: Arend Van Spriel
Cc: Ming Lei, Luis R. Rodriguez, Greg Kroah-Hartman, Kalle Valo,
David Gnedt, Michal Kazior, Daniel Wagner, Tony Lindgren,
Sebastian Reichel, Pavel Machek, Ivaylo Dimitrov, Aaro Koskinen,
Grazvydas Ignotas, linux-kernel, linux-wireless, netdev
In-Reply-To: <e728586e-80bf-c9e0-0063-71da4c4aba85@broadcom.com>
[-- Attachment #1: Type: Text/Plain, Size: 5551 bytes --]
On Sunday 25 December 2016 21:15:40 Arend Van Spriel wrote:
> On 24-12-2016 17:52, Pali Rohár wrote:
> > NVS calibration data for wl1251 are model specific. Every one
> > device with wl1251 chip has different and calibrated in factory.
> >
> > Not all wl1251 chips have own EEPROM where are calibration data
> > stored. And in that case there is no "standard" place. Every
> > device has stored them on different place (some in rootfs file,
> > some in dedicated nand partition, some in another proprietary
> > structure).
> >
> > Kernel wl1251 driver cannot support every one different storage
> > decided by device manufacture so it will use
> > request_firmware_prefer_user() call for loading NVS calibration
> > data and userspace helper will be responsible to prepare correct
> > data.
>
> Responding to this patch as it provides a lot of context to discuss.
> As you might have gathered from earlier discussions I am not a fan
> of using user-space helper. I can agree that the kernel driver,
> wl1251 in this case, should be agnostic to platform specific details
> regarding storage solutions and the firmware api should hide that.
> However, it seems your only solution is adding user-space to the mix
> and changing the api towards that. Can we solve it without
> user-space help?
Without userspace helper it means that userspace helper code must be
integrated into kernel.
So what is userspace helper doing?
1) Read MAC address from CAL
2) Read NVS data from CAL
3) Modify MAC address in memory NVS data (new for this patch series)
4) Modify in memory NVS data if we in FCC country
Checking for country is done via dbus call to either Maemo cellular
daemon or alternatively via REGDOMAIN in /etc/default/crda. I have plan
to use ofono (instead Maemo cellular daemon) too...
Currently we are using closed Nokia proprietary CAL library.
Steps 1) and 2) needs closed library, step 4) needs dbus call.
In current state I do not see way to integrate it into kernel. And
because wl1251 currently uses request_firmware() to load those nvs data
I think it is still the best way how to handle it...
And IIRC there was already discussion about Nokia CAL parser in kernel
and it was declined.
> The firmware_class already supports a number of path prefixes it
> traverses looking for the requested firmware. So I was thinking about
> adding a hashtable in which a platform driver can add firmware which
> are stored in the hashtable using the hashed firmware name. Upon a
> firmware request from the driver we could check the hashtable before
> traversing the path prefixes on VFS. The obvious problem is that the
> request may come before the firmware is added to the hashtable. Just
> wanted to pitch the idea first and hear what others think about it
> and maybe someone has a nice solution for this problem. Fingers
> crossed :-p
>
> > In case userspace helper fails request_firmware_prefer_user() still
> > try to load data file directly from VFS as fallback mechanism.
> >
> > On Nokia N900 device which has wl1251 chip, NVS calibration data
> > are stored in CAL nand partition. CAL is proprietary Nokia
> > key/value format for nand devices.
>
> With the firmware hashtable api on N900 a platform driver could
> interpret the CAL data in the nand partition and provide it through
> the firmware_class.
>
> > With this patch it is finally possible to load correct model
> > specific NVS calibration data for Nokia N900.
>
> But on other devices that use wl1251, but for instance have no
> userspace helper the request to userspace will fail (after 60 sec?)
> and try VFS after that. Maybe not so nice.
Currently support for those devices is broken (like for N900) as without
proper NVS data they do not work correctly...
> You should consider other device configurations. Not just N900.
I do not have any other wl1251 devices. I know that pandora has wl1251
too, but it has wl1251 with eeprom where is stored NVS. And in this case
request_firmware() is not used there.
> Regards,
> Arend
>
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> >
> > drivers/net/wireless/ti/wl1251/Kconfig | 1 +
> > drivers/net/wireless/ti/wl1251/main.c | 2 +-
> > 2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/ti/wl1251/Kconfig
> > b/drivers/net/wireless/ti/wl1251/Kconfig index 7142ccf..affe154
> > 100644
> > --- a/drivers/net/wireless/ti/wl1251/Kconfig
> > +++ b/drivers/net/wireless/ti/wl1251/Kconfig
> > @@ -2,6 +2,7 @@ config WL1251
> >
> > tristate "TI wl1251 driver support"
> > depends on MAC80211
> > select FW_LOADER
> >
> > + select FW_LOADER_USER_HELPER
> >
> > select CRC7
> > ---help---
> >
> > This will enable TI wl1251 driver support. The drivers make
> >
> > diff --git a/drivers/net/wireless/ti/wl1251/main.c
> > b/drivers/net/wireless/ti/wl1251/main.c index 208f062..24f8866
> > 100644
> > --- a/drivers/net/wireless/ti/wl1251/main.c
> > +++ b/drivers/net/wireless/ti/wl1251/main.c
> > @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
> >
> > struct device *dev = wiphy_dev(wl->hw->wiphy);
> > int ret;
> >
> > - ret = request_firmware(&fw, WL1251_NVS_NAME, dev);
> > + ret = request_firmware_prefer_user(&fw, WL1251_NVS_NAME, dev);
> >
> > if (ret < 0) {
> >
> > wl1251_error("could not get nvs file: %d", ret);
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
From: Arend Van Spriel @ 2016-12-25 20:15 UTC (permalink / raw)
To: Pali Rohár, Ming Lei, Luis R. Rodriguez, Greg Kroah-Hartman,
Kalle Valo, David Gnedt, Michal Kazior, Daniel Wagner,
Tony Lindgren, Sebastian Reichel, Pavel Machek, Ivaylo Dimitrov,
Aaro Koskinen, Grazvydas Ignotas
Cc: linux-kernel, linux-wireless, netdev
In-Reply-To: <1482598381-16513-3-git-send-email-pali.rohar@gmail.com>
On 24-12-2016 17:52, Pali Rohár wrote:
> NVS calibration data for wl1251 are model specific. Every one device with
> wl1251 chip has different and calibrated in factory.
>
> Not all wl1251 chips have own EEPROM where are calibration data stored. And
> in that case there is no "standard" place. Every device has stored them on
> different place (some in rootfs file, some in dedicated nand partition,
> some in another proprietary structure).
>
> Kernel wl1251 driver cannot support every one different storage decided by
> device manufacture so it will use request_firmware_prefer_user() call for
> loading NVS calibration data and userspace helper will be responsible to
> prepare correct data.
Responding to this patch as it provides a lot of context to discuss. As
you might have gathered from earlier discussions I am not a fan of using
user-space helper. I can agree that the kernel driver, wl1251 in this
case, should be agnostic to platform specific details regarding storage
solutions and the firmware api should hide that. However, it seems your
only solution is adding user-space to the mix and changing the api
towards that. Can we solve it without user-space help?
The firmware_class already supports a number of path prefixes it
traverses looking for the requested firmware. So I was thinking about
adding a hashtable in which a platform driver can add firmware which are
stored in the hashtable using the hashed firmware name. Upon a firmware
request from the driver we could check the hashtable before traversing
the path prefixes on VFS. The obvious problem is that the request may
come before the firmware is added to the hashtable. Just wanted to pitch
the idea first and hear what others think about it and maybe someone has
a nice solution for this problem. Fingers crossed :-p
> In case userspace helper fails request_firmware_prefer_user() still try to
> load data file directly from VFS as fallback mechanism.
>
> On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
> in CAL nand partition. CAL is proprietary Nokia key/value format for nand
> devices.
With the firmware hashtable api on N900 a platform driver could
interpret the CAL data in the nand partition and provide it through the
firmware_class.
> With this patch it is finally possible to load correct model specific NVS
> calibration data for Nokia N900.
But on other devices that use wl1251, but for instance have no userspace
helper the request to userspace will fail (after 60 sec?) and try VFS
after that. Maybe not so nice. You should consider other device
configurations. Not just N900.
Regards,
Arend
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
> drivers/net/wireless/ti/wl1251/Kconfig | 1 +
> drivers/net/wireless/ti/wl1251/main.c | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ti/wl1251/Kconfig b/drivers/net/wireless/ti/wl1251/Kconfig
> index 7142ccf..affe154 100644
> --- a/drivers/net/wireless/ti/wl1251/Kconfig
> +++ b/drivers/net/wireless/ti/wl1251/Kconfig
> @@ -2,6 +2,7 @@ config WL1251
> tristate "TI wl1251 driver support"
> depends on MAC80211
> select FW_LOADER
> + select FW_LOADER_USER_HELPER
> select CRC7
> ---help---
> This will enable TI wl1251 driver support. The drivers make
> diff --git a/drivers/net/wireless/ti/wl1251/main.c b/drivers/net/wireless/ti/wl1251/main.c
> index 208f062..24f8866 100644
> --- a/drivers/net/wireless/ti/wl1251/main.c
> +++ b/drivers/net/wireless/ti/wl1251/main.c
> @@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
> struct device *dev = wiphy_dev(wl->hw->wiphy);
> int ret;
>
> - ret = request_firmware(&fw, WL1251_NVS_NAME, dev);
> + ret = request_firmware_prefer_user(&fw, WL1251_NVS_NAME, dev);
>
> if (ret < 0) {
> wl1251_error("could not get nvs file: %d", ret);
>
^ permalink raw reply
* RE: [PATCH v3 net-next] bnx2x: ethtool -x support for rss_key
From: Mintz, Yuval @ 2016-12-25 11:36 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
In-Reply-To: <1482342498.8944.62.camel@edumazet-glaptop3.roam.corp.google.com>
+ if (key) {
> + WARN_ON_ONCE(bnx2x_get_rxfh_key_size(dev) !=
> T_ETH_RSS_KEY * 4);
> + bnx2x_get_rss_key(&bp->rss_conf_obj, key);
> + }
This doesn’t work VFs [the PF has the RSS configuration object in their
case; They don't have it], which is fine as 'key' should never be set for
them [since you're adding bnx2x_get_rxfh_key_size() to ethtool ops
only for PFs]. But this probably still worth a comment, though.
> - memcpy(rss.rss_key, rss_tlv->rss_key, sizeof(rss_tlv->rss_key));
> + memcpy(&vf->rss_conf_obj.rss_key, rss_tlv->rss_key,
> +sizeof(rss_tlv->rss_key));
> rss.rss_obj = &vf->rss_conf_obj;
> rss.rss_result_mask = rss_tlv->rss_result_mask;
The change you've applied in bnx2x_setup_rss() should affect here
as well, meaning the PF would copy the parameters into the PF's RSS
configuration object belonging to the VF from the parameter.
This change would cause the PF to configure the VF's RSS key
as all-zeros [as parameters were initially zeroed].
^ permalink raw reply
* RE: [PATCH iproute2 v3 4/4] ifstat: Add "sw only" extended statistics to ifstat
From: Nogah Frankel @ 2016-12-25 11:23 UTC (permalink / raw)
To: Roopa Prabhu
Cc: netdev@vger.kernel.org, stephen@networkplumber.org,
roszenrami@gmail.com, Or Gerlitz, Jiri Pirko, Elad Raz,
Yotam Gigi, Ido Schimmel
In-Reply-To: <585C413B.9050905@cumulusnetworks.com>
> -----Original Message-----
> From: Roopa Prabhu [mailto:roopa@cumulusnetworks.com]
> Sent: Thursday, December 22, 2016 11:10 PM
> To: Nogah Frankel <nogahf@mellanox.com>
> Cc: netdev@vger.kernel.org; stephen@networkplumber.org; roszenrami@gmail.com; Or
> Gerlitz <ogerlitz@mellanox.com>; Jiri Pirko <jiri@mellanox.com>; Elad Raz
> <eladr@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Ido Schimmel
> <idosch@mellanox.com>
> Subject: Re: [PATCH iproute2 v3 4/4] ifstat: Add "sw only" extended statistics to ifstat
>
> On 12/22/16, 8:23 AM, Nogah Frankel wrote:
> > Add support for extended statistics of SW only type, for counting only the
> > packets that went via the cpu. (useful for systems with forward
> > offloading). It reads it from filter type IFLA_STATS_LINK_OFFLOAD_XSTATS
> > and sub type IFLA_OFFLOAD_XSTATS_CPU_HIT.
> >
> > It is under the name 'software'
> > (or any shorten of it as 'soft' or simply 's')
> >
> > For example:
> > ifstat -x s
> >
> >
> Nogah, can we keep the option names closer to the attribute names ?
> That would avoid some confusion and help with the follow-up stats.
>
> ifstat -x offload cpu
> or
> ifstat -x cpu
>
> for others it would be:
>
> ifstat -x link [vlan|igmp]
> ifstat -x vlan
> ifstat -x igmp
> ifstat -x lacp
>
> and so on...
>
> thanks!
Sure, I will change it.
^ permalink raw reply
* RE: [PATCH iproute2 v3 2/4] ifstat: Add extended statistics to ifstat
From: Nogah Frankel @ 2016-12-25 10:25 UTC (permalink / raw)
To: Stephen Hemminger
Cc: netdev@vger.kernel.org, roopa@cumulusnetworks.com,
roszenrami@gmail.com, Or Gerlitz, Jiri Pirko, Elad Raz,
Yotam Gigi, Ido Schimmel
In-Reply-To: <20161222105919.08b8738b@xeon-e3>
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, December 22, 2016 8:59 PM
> To: Nogah Frankel <nogahf@mellanox.com>
> Cc: netdev@vger.kernel.org; roopa@cumulusnetworks.com; roszenrami@gmail.com; Or
> Gerlitz <ogerlitz@mellanox.com>; Jiri Pirko <jiri@mellanox.com>; Elad Raz
> <eladr@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Ido Schimmel
> <idosch@mellanox.com>
> Subject: Re: [PATCH iproute2 v3 2/4] ifstat: Add extended statistics to ifstat
>
> On Thu, 22 Dec 2016 18:23:13 +0200
> Nogah Frankel <nogahf@mellanox.com> wrote:
> On Thu, 22 Dec 2016 18:23:13 +0200
> Nogah Frankel <nogahf@mellanox.com> wrote:
>
> > }
> > @@ -691,18 +804,22 @@ static const struct option longopts[] = {
> > { "interval", 1, 0, 't' },
> > { "version", 0, 0, 'V' },
> > { "zeros", 0, 0, 'z' },
> > + { "extended", 1, 0, 'x'},
> > { 0 }
> > };
> >
> > +
> > int main(int argc, char *argv[])
>
> You let extra whitespace changes creep in.
>
>
> > + case 'x':
> > + is_extended = true;
> > + memset(stats_type, 0, 64);
> > + strncpy(stats_type, optarg, 63);
> > + break;
>
> This seems like doing this either the paranoid or hard way.
> Why not:
> const char *stats_type = NULL;
> ...
>
> case 'x':
> stats_type = optarg;
> break;
> ...
> if (stats_type)
> snprintf(hist_name, sizeof(hist_name),
> "%s/.%s_ifstat.u%d", P_tmpdir, stats_type,
> getuid());
> else
> snprintf(hist_name, sizeof(hist_name),
> "%s/.ifstat.u%d", P_tmpdir, getuid());
>
>
> Since:
> 1) optarg points to area in argv that is persistent (avoid copy)
> 2) don't need is_extended flag value then
>
> Please cleanup and resubmit.
>
>
I will.
Thank you.
^ permalink raw reply
* Re: [PATCH v3 2/2] drivers: net: ethernet: 3com: fix return value
From: Sergei Shtylyov @ 2016-12-25 10:07 UTC (permalink / raw)
To: Thomas Preisner
Cc: dave, netdev, linux-kernel, linux-kernel, milan.stephan+linux
In-Reply-To: <1482625822-19658-3-git-send-email-thomas.preisner+linux@fau.de>
Hello!
On 12/25/2016 3:30 AM, Thomas Preisner wrote:
> In some cases the return value of a failing function is not being used
> and the function typhoon_init_one() returns another negative error
> code instead.
>
> Signed-off-by: Thomas Preisner <thomas.preisner+linux@fau.de>
> Signed-off-by: Milan Stephan <milan.stephan+linux@fau.de>
> ---
> drivers/net/ethernet/3com/typhoon.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
In addition to what DaveM said, your choise of the subject prefixes is too
wide -- it would seem that you're fixing all 3com drivers, while you're only
fixing typhoon. That "typhoon:" alone would have been an appropriate prefix.
MBR, Sergei
^ permalink raw reply
* Re: [PATCH] net: ethtool: don't require CAP_NET_ADMIN for ETHTOOL_GLINKSETTINGS
From: Vincent Bernat @ 2016-12-25 7:44 UTC (permalink / raw)
To: Miroslav Lichvar; +Cc: netdev, David Decotigny
In-Reply-To: <20161124095506.25791-1-mlichvar@redhat.com>
❦ 24 novembre 2016 10:55 +0100, Miroslav Lichvar <mlichvar@redhat.com> :
> The ETHTOOL_GLINKSETTINGS command is deprecating the ETHTOOL_GSET
> command and likewise it shouldn't require the CAP_NET_ADMIN
> capability.
Could this patch be pushed to stable branches too?
--
Each module should do one thing well.
- The Elements of Programming Style (Kernighan & Plauger)
^ permalink raw reply
* [PATCH v2] ipv4: Namespaceify tcp_tw_reuse knob
From: Haishuang Yan @ 2016-12-25 6:33 UTC (permalink / raw)
To: David S. Miller, Alexey Kuznetsov, James Morris, Patrick McHardy,
Nikolay Borisov
Cc: netdev, linux-kernel, Haishuang Yan
Different namespaces might have different requirements to reuse
TIME-WAIT sockets for new connections. This might be required in
cases where different namespace applications are in place which
require TIME_WAIT socket connections to be reduced independently
of the host.
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
---
Changes in v2:
- Make the commit message more clearer.
---
include/net/netns/ipv4.h | 1 +
include/net/tcp.h | 1 -
net/ipv4/sysctl_net_ipv4.c | 14 +++++++-------
net/ipv4/tcp_ipv4.c | 4 ++--
4 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index f0cf5a1..0378e88 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -110,6 +110,7 @@ struct netns_ipv4 {
int sysctl_tcp_orphan_retries;
int sysctl_tcp_fin_timeout;
unsigned int sysctl_tcp_notsent_lowat;
+ int sysctl_tcp_tw_reuse;
int sysctl_igmp_max_memberships;
int sysctl_igmp_max_msf;
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 207147b..6061963 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -252,7 +252,6 @@
extern int sysctl_tcp_rmem[3];
extern int sysctl_tcp_app_win;
extern int sysctl_tcp_adv_win_scale;
-extern int sysctl_tcp_tw_reuse;
extern int sysctl_tcp_frto;
extern int sysctl_tcp_low_latency;
extern int sysctl_tcp_nometrics_save;
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 80bc36b..22cbd61 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -433,13 +433,6 @@ static int proc_tcp_fastopen_key(struct ctl_table *ctl, int write,
.extra2 = &tcp_adv_win_scale_max,
},
{
- .procname = "tcp_tw_reuse",
- .data = &sysctl_tcp_tw_reuse,
- .maxlen = sizeof(int),
- .mode = 0644,
- .proc_handler = proc_dointvec
- },
- {
.procname = "tcp_frto",
.data = &sysctl_tcp_frto,
.maxlen = sizeof(int),
@@ -960,6 +953,13 @@ static int proc_tcp_fastopen_key(struct ctl_table *ctl, int write,
.mode = 0644,
.proc_handler = proc_dointvec,
},
+ {
+ .procname = "tcp_tw_reuse",
+ .data = &init_net.ipv4.sysctl_tcp_tw_reuse,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec
+ },
#ifdef CONFIG_IP_ROUTE_MULTIPATH
{
.procname = "fib_multipath_use_neigh",
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 30d81f5..fe9da4f 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -84,7 +84,6 @@
#include <crypto/hash.h>
#include <linux/scatterlist.h>
-int sysctl_tcp_tw_reuse __read_mostly;
int sysctl_tcp_low_latency __read_mostly;
#ifdef CONFIG_TCP_MD5SIG
@@ -120,7 +119,7 @@ int tcp_twsk_unique(struct sock *sk, struct sock *sktw, void *twp)
and use initial timestamp retrieved from peer table.
*/
if (tcptw->tw_ts_recent_stamp &&
- (!twp || (sysctl_tcp_tw_reuse &&
+ (!twp || (sock_net(sk)->ipv4.sysctl_tcp_tw_reuse &&
get_seconds() - tcptw->tw_ts_recent_stamp > 1))) {
tp->write_seq = tcptw->tw_snd_nxt + 65535 + 2;
if (tp->write_seq == 0)
@@ -2456,6 +2455,7 @@ static int __net_init tcp_sk_init(struct net *net)
net->ipv4.sysctl_tcp_orphan_retries = 0;
net->ipv4.sysctl_tcp_fin_timeout = TCP_FIN_TIMEOUT;
net->ipv4.sysctl_tcp_notsent_lowat = UINT_MAX;
+ net->ipv4.sysctl_tcp_tw_reuse = 0;
return 0;
fail:
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] ipv4: Namespaceify tcp_tw_reuse knob
From: David Miller @ 2016-12-25 2:07 UTC (permalink / raw)
To: yanhaishuang; +Cc: kuznet, jmorris, kaber, netdev, linux-kernel
In-Reply-To: <1482583387-79777-1-git-send-email-yanhaishuang@cmss.chinamobile.com>
From: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
Date: Sat, 24 Dec 2016 20:43:07 +0800
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
You need to provide something more than an empty commit message.
Instead, the commit message must explain why this particular
sysctl should be considered for namespacification and what
the implications, both good and bad, are for such a change.
^ permalink raw reply
* Re: [PATCH v3 1/2] drivers: net: ethernet: 3com: fix return value
From: David Miller @ 2016-12-25 0:56 UTC (permalink / raw)
To: thomas.preisner+linux
Cc: sergei.shtylyov, dave, netdev, linux-kernel, linux-kernel,
milan.stephan+linux
In-Reply-To: <1482625822-19658-2-git-send-email-thomas.preisner+linux@fau.de>
It is never, ever, appropriate to use the same exact Subject: line
text for two different changes.
Someone looking at "git shortlog" has no way to know what is different
between the two changes.
You must put care and time into constructing Subject: lines because
this text is critical for data mining and analysis done by both humans
and machines.
^ permalink raw reply
* [PATCH v3 2/2] drivers: net: ethernet: 3com: fix return value
From: Thomas Preisner @ 2016-12-25 0:30 UTC (permalink / raw)
To: sergei.shtylyov
Cc: dave, netdev, linux-kernel, linux-kernel, milan.stephan+linux,
thomas.preisner+linux
In-Reply-To: <1482625822-19658-1-git-send-email-thomas.preisner+linux@fau.de>
In some cases the return value of a failing function is not being used
and the function typhoon_init_one() returns another negative error
code instead.
Signed-off-by: Thomas Preisner <thomas.preisner+linux@fau.de>
Signed-off-by: Milan Stephan <milan.stephan+linux@fau.de>
---
drivers/net/ethernet/3com/typhoon.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/3com/typhoon.c b/drivers/net/ethernet/3com/typhoon.c
index c88b88a..8821a24 100644
--- a/drivers/net/ethernet/3com/typhoon.c
+++ b/drivers/net/ethernet/3com/typhoon.c
@@ -2370,9 +2370,9 @@ typhoon_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
* 4) Get the hardware address.
* 5) Put the card to sleep.
*/
- if (typhoon_reset(ioaddr, WaitSleep) < 0) {
+ err = typhoon_reset(ioaddr, WaitSleep);
+ if (err < 0) {
err_msg = "could not reset 3XP";
- err = -EIO;
goto error_out_dma;
}
@@ -2386,16 +2386,16 @@ typhoon_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
typhoon_init_interface(tp);
typhoon_init_rings(tp);
- if(typhoon_boot_3XP(tp, TYPHOON_STATUS_WAITING_FOR_HOST) < 0) {
+ err = typhoon_boot_3XP(tp, TYPHOON_STATUS_WAITING_FOR_HOST);
+ if (err < 0) {
err_msg = "cannot boot 3XP sleep image";
- err = -EIO;
goto error_out_reset;
}
INIT_COMMAND_WITH_RESPONSE(&xp_cmd, TYPHOON_CMD_READ_MAC_ADDRESS);
- if(typhoon_issue_command(tp, 1, &xp_cmd, 1, xp_resp) < 0) {
+ err = typhoon_issue_command(tp, 1, &xp_cmd, 1, xp_resp);
+ if (err < 0) {
err_msg = "cannot read MAC address";
- err = -EIO;
goto error_out_reset;
}
@@ -2430,9 +2430,9 @@ typhoon_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
if(xp_resp[0].numDesc != 0)
tp->capabilities |= TYPHOON_WAKEUP_NEEDS_RESET;
- if(typhoon_sleep(tp, PCI_D3hot, 0) < 0) {
+ err = typhoon_sleep(tp, PCI_D3hot, 0);
+ if (err < 0) {
err_msg = "cannot put adapter to sleep";
- err = -EIO;
goto error_out_reset;
}
--
2.7.4
^ permalink raw reply related
* [PATCH v3 1/2] drivers: net: ethernet: 3com: fix return value
From: Thomas Preisner @ 2016-12-25 0:30 UTC (permalink / raw)
To: sergei.shtylyov
Cc: dave, netdev, linux-kernel, linux-kernel, milan.stephan+linux,
thomas.preisner+linux
In-Reply-To: <1482625822-19658-1-git-send-email-thomas.preisner+linux@fau.de>
In a few cases the err-variable is not set to a negative error code if a
function call fails and thus 0 is returned instead.
It may be better to set err to the appropriate negative error code
before returning.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=188841
Reported-by: Pan Bian <bianpan2016@163.com>
Signed-off-by: Thomas Preisner <thomas.preisner+linux@fau.de>
Signed-off-by: Milan Stephan <milan.stephan+linux@fau.de>
---
drivers/net/ethernet/3com/typhoon.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/3com/typhoon.c b/drivers/net/ethernet/3com/typhoon.c
index a0cacbe..c88b88a 100644
--- a/drivers/net/ethernet/3com/typhoon.c
+++ b/drivers/net/ethernet/3com/typhoon.c
@@ -2404,6 +2404,7 @@ typhoon_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
- if(!is_valid_ether_addr(dev->dev_addr)) {
+ if (!is_valid_ether_addr(dev->dev_addr)) {
err_msg = "Could not obtain valid ethernet address, aborting";
+ err = -EIO;
goto error_out_reset;
}
@@ -2411,7 +2412,8 @@ typhoon_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
* later when we print out the version reported.
*/
INIT_COMMAND_WITH_RESPONSE(&xp_cmd, TYPHOON_CMD_READ_VERSIONS);
- if(typhoon_issue_command(tp, 1, &xp_cmd, 3, xp_resp) < 0) {
+ err = typhoon_issue_command(tp, 1, &xp_cmd, 3, xp_resp);
+ if (err < 0) {
err_msg = "Could not get Sleep Image version";
goto error_out_reset;
}
@@ -2453,7 +2455,8 @@ typhoon_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
dev->features = dev->hw_features |
NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_RXCSUM;
- if(register_netdev(dev) < 0) {
+ err = register_netdev(dev);
+ if (err < 0) {
err_msg = "unable to register netdev";
goto error_out_reset;
}
--
2.7.4
^ permalink raw reply related
* Re: Re: [PATCH v2 1/2] drivers: net: ethernet: 3com: fix return value
From: Thomas Preisner @ 2016-12-25 0:30 UTC (permalink / raw)
To: sergei.shtylyov
Cc: dave, netdev, linux-kernel, linux-kernel, milan.stephan+linux,
thomas.preisner+linux
In-Reply-To: <4200be74-f7e7-db6b-a258-8fd178fef369@cogentembedded.com>
Hello.
On Sat, 2016-12-24 at 20:06 +0100, Sergei Shtylyov wrote:
>Hello!
>
>On 12/24/2016 03:02 PM, Thomas Preisner wrote:
>
>> In a few cases the err-variable is not set to a negative error code if a
>> function call fails and thus 0 is returned instead.
>> It may be better to set err to the appropriate negative error code
>> before returning.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=188841
>>
>> Reported-by: Pan Bian <bianpan2016@163.com>
>> Signed-off-by: Thomas Preisner <thomas.preisner+linux@fau.de>
>> Signed-off-by: Milan Stephan <milan.stephan+linux@fau.de>
>> ---
>> drivers/net/ethernet/3com/typhoon.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/3com/typhoon.c b/drivers/net/ethernet/3com/typhoon.c
>> index a0cacbe..c88b88a 100644
>> --- a/drivers/net/ethernet/3com/typhoon.c
>> +++ b/drivers/net/ethernet/3com/typhoon.c
>[...]
>> @@ -2411,7 +2412,8 @@ typhoon_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>> * later when we print out the version reported.
>> */
>> INIT_COMMAND_WITH_RESPONSE(&xp_cmd, TYPHOON_CMD_READ_VERSIONS);
>> - if(typhoon_issue_command(tp, 1, &xp_cmd, 3, xp_resp) < 0) {
>> + err = typhoon_issue_command(tp, 1, &xp_cmd, 3, xp_resp);
>> + if(err < 0) {
>
> Need a space between *if* and (. Run your patches thru
>scripts/checkpatch.pl before posting, please.
Those spaces were actually left out purposely: The file in question (typhoon.c)
is missing those spaces between the statements (if, for, while) and the
following opening bracket pretty much always (except 2-3 times) and we figured
that it might be better to keep the coding style consistent since this might
aswell have been intended by the original author.
>
>> err_msg = "Could not get Sleep Image version";
>> goto error_out_reset;
>> }
>> @@ -2453,7 +2455,8 @@ typhoon_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>> dev->features = dev->hw_features |
>> NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_RXCSUM;
>>
>> - if(register_netdev(dev) < 0) {
>> + err = register_netdev(dev);
>> + if(err < 0) {
>
> Same here.
>
>[...]
>
>MBR, Sergei
But of course we can provide you with a patchset including those spaces.
With Regards,
Milan and Thomas
^ permalink raw reply
* Re: [PATCHv2 1/5] sh_eth: add generic wake-on-lan support via magic packet
From: Sergei Shtylyov @ 2016-12-24 21:53 UTC (permalink / raw)
To: Geert Uytterhoeven, Niklas Söderlund
Cc: Simon Horman, netdev@vger.kernel.org, Linux-Renesas
In-Reply-To: <CAMuHMdXVVS_eLozvDnEz67_Fwg7jGDK0YEGh84rS+EAm4RFnag@mail.gmail.com>
Hello!
On 12/19/2016 08:11 PM, Geert Uytterhoeven wrote:
>>>> One quirk needed for WoL is that the module clock needs to be prevented
>>>> from being switched off by Runtime PM. To keep the clock alive the
>>>
>>> I tried to find the code in question and failed, getting muddled in the
>>> RPM maze. Could you point at this code for my education? :-)
>>
>> In my investigation I observed this (simplified) call graph with regards
>> to clocks for suspend:
>>
>> pm_suspend
There's a long list of the calls skipped here. :-)
>> pm_clk_suspend
>> clk_disable
>> clk_core_disable
>> cpg_mstp_clock_disable
>>
>> The interesting function here are clk_core_disable(). In that function a
>> 'enable_count' for each clock is decremented and the clock is only
>> turned of if the count reaches zero, hence cpg_mstp_clock_disable() are
>> only called if the counter reaches 0. At runtime the enable_count can be
>> displayed by examining /sys/kernel/debug/clk/clk_summary.
Well, this is not new to me... it's more interesting how we get there... :-)
[...]
>>>> usage count of the clock. Then when Runtime PM decreases the clock usage
>>>> count it won't reach 0 and be switched off.
>>>
>>> You mean it does this even though we don't call pr_runtime_put_sync()
>>> as done in sh_eth_close()?
>>
>> Yes.
>>
>> I had a look at the pm_runtime_* functions in include/linux/pm_runtime.h
>> and drivers/base/power/runtime.c and could not find any clock handling.
>> Maybe they only deal with power domains?
>
> There should be a generic way to prevent a device from being suspended.
Indeed.
> This will make sure the module clock is not disabled, and the power domain
> (if applicable) is not powered down.
I've just bumped into <linux/pm_wakeirq.h>, it looks promising...
[...]
> Gr{oetje,eeting}s,
>
> Geert
MBR, Sergei
^ permalink raw reply
* Re: [PATCH net] net, sched: fix soft lockup in tc_classify
From: Daniel Borkmann @ 2016-12-24 21:03 UTC (permalink / raw)
To: Cong Wang
Cc: David Miller, Shahar Klein, Or Gerlitz, Roi Dayan, Jiri Pirko,
John Fastabend, Linux Kernel Network Developers
In-Reply-To: <CAM_iQpX2X-WHbf1VxfQzh_-YUEqk=o6B+uYfYhj_45jJGaFSfQ@mail.gmail.com>
On 12/24/2016 08:34 AM, Cong Wang wrote:
> On Thu, Dec 22, 2016 at 4:26 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 12/22/2016 08:05 PM, Cong Wang wrote:
>>> On Wed, Dec 21, 2016 at 1:07 PM, Daniel Borkmann <daniel@iogearbox.net>
>>> wrote:
>>>>
>>>> Ok, you mean for net. In that case I prefer the smaller sized fix to be
>>>> honest. It also covers everything from the point where we fetch the chain
>>>> via cops->tcf_chain() to the end of the function, which is where most of
>>>> the complexity resides, and only the two mentioned commits do the relock,
>>>
>>> I really wish the problem is only about relocking, but look at the code,
>>> the deeper reason why we have this bug is the complexity of the logic
>>> inside tc_ctl_tfilter(): 1) the replay logic is hard, we have to make it
>>> idempotent; 2) the request logic itself is hard, because of tc filter
>>> design
>>> and implementation.
>>>
>>> This is why I worry more than just relocking.
>>
>> But do you have a concrete 2nd issue/bug you're seeing? It rather sounds to
>> me your argument is more about fear of complexity on tc framework itself.
>> I agree it's complex, and tc_ctl_tfilter() is quite big in itself, where it
>> would be good to reduce it's complexity into smaller pieces. But it's not
>> really related to the fix itself, reducing complexity requires significantly
>> more and deeper work on the code. We can rework tc_ctl_tfilter() in net-next
>> to try to simplify it, sure, but I don't get why we have to discuss so much
>> on this matter in this context, really.
>
> Thanks for ignoring my point 1) above... You are dragging the discussion
> further.
I don't think so. The analysis and patch I proposed provides an explanation
of how we get into the seen endless loop, it provides a logical fix for it,
which has been reviewed by others and it has been tested extensively that it
resolves the issue, which was easily reproducible for the reporter and that
after the fix it never occurred again. The delta is absolutely simple and
really low risk. Given this function has not much changed over time, also
distros could pick it up that have a much older base kernel than current
stable ones. This initiated follow-up discussion we're having here in general
is dragging the focus away for everyone, and quite frankly I'm getting tired
of discussing it. I have stated my preferences, you have stated yours, and
we're only repeating ourselves in circles which isn't helpful in any way,
the discussion is not about some concrete bug in the logic to fix anymore
(otherwise please name it). Hence my proposal that everything else can wait
and be done in net-next.
^ permalink raw reply
* Re: [RFC PATCH 4.10 3/6] bpf: Use SHA256 instead of SHA1 for bpf digests
From: Daniel Borkmann @ 2016-12-24 19:59 UTC (permalink / raw)
To: Andy Lutomirski, Netdev, LKML, Linux Crypto Mailing List
Cc: Jason A. Donenfeld, Hannes Frederic Sowa, Alexei Starovoitov,
Eric Dumazet, Eric Biggers, Tom Herbert, David S. Miller,
Alexei Starovoitov
In-Reply-To: <bbfd69226fd74391045bafc695bba9a46cacca85.1482545792.git.luto@kernel.org>
On 12/24/2016 03:22 AM, Andy Lutomirski wrote:
> BPF digests are intended to be used to avoid reloading programs that
> are already loaded. For use cases (CRIU?) where untrusted programs
> are involved, intentional hash collisions could cause the wrong BPF
> program to execute. Additionally, if BPF digests are ever used
> in-kernel to skip verification, a hash collision could give privilege
> escalation directly.
Just for the record, digests will never ever be used to skip the
verification step, so I don't know why this idea even comes up
here (?) or is part of the changelog? As this will never be done
anyway, rather drop that part so we can avoid confusion on this?
Wrt untrusted programs, I don't see much of a use on this facility
in general for them. Something like a tail call map would quite
likely only be private to the application. And again, I really doubt
we'll have something like user namespace support in the foreseeable
future. Anyway, that said, I don't really have a big issue if you
want to switch to sha256, though.
> SHA1 is no longer considered adequately collision-resistant (see, for
> example, all the major browsers dropping support for SHA1
> certificates). Use SHA256 instead.
>
> I moved the digest field to keep all of the bpf program metadata in
> the same cache line.
>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 1/2] drivers: net: ethernet: 3com: fix return value
From: Sergei Shtylyov @ 2016-12-24 19:06 UTC (permalink / raw)
To: Thomas Preisner, dave
Cc: netdev, linux-kernel, linux-kernel, milan.stephan+linux
In-Reply-To: <1482580958-15406-2-git-send-email-thomas.preisner+linux@fau.de>
Hello!
On 12/24/2016 03:02 PM, Thomas Preisner wrote:
> In a few cases the err-variable is not set to a negative error code if a
> function call fails and thus 0 is returned instead.
> It may be better to set err to the appropriate negative error code
> before returning.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=188841
>
> Reported-by: Pan Bian <bianpan2016@163.com>
> Signed-off-by: Thomas Preisner <thomas.preisner+linux@fau.de>
> Signed-off-by: Milan Stephan <milan.stephan+linux@fau.de>
> ---
> drivers/net/ethernet/3com/typhoon.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/3com/typhoon.c b/drivers/net/ethernet/3com/typhoon.c
> index a0cacbe..c88b88a 100644
> --- a/drivers/net/ethernet/3com/typhoon.c
> +++ b/drivers/net/ethernet/3com/typhoon.c
[...]
> @@ -2411,7 +2412,8 @@ typhoon_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> * later when we print out the version reported.
> */
> INIT_COMMAND_WITH_RESPONSE(&xp_cmd, TYPHOON_CMD_READ_VERSIONS);
> - if(typhoon_issue_command(tp, 1, &xp_cmd, 3, xp_resp) < 0) {
> + err = typhoon_issue_command(tp, 1, &xp_cmd, 3, xp_resp);
> + if(err < 0) {
Need a space between *if* and (. Run your patches thru
scripts/checkpatch.pl before posting, please.
> err_msg = "Could not get Sleep Image version";
> goto error_out_reset;
> }
> @@ -2453,7 +2455,8 @@ typhoon_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> dev->features = dev->hw_features |
> NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_RXCSUM;
>
> - if(register_netdev(dev) < 0) {
> + err = register_netdev(dev);
> + if(err < 0) {
Same here.
[...]
MBR, Sergei
^ permalink raw reply
* Re: [PATCH 6/6] wl1251: Set generated MAC address back to NVS data
From: Pali Rohár @ 2016-12-24 18:44 UTC (permalink / raw)
To: Pavel Machek
Cc: Ming Lei, Luis R. Rodriguez, Greg Kroah-Hartman, Kalle Valo,
David Gnedt, Michal Kazior, Daniel Wagner, Tony Lindgren,
Sebastian Reichel, Ivaylo Dimitrov, Aaro Koskinen,
Grazvydas Ignotas, linux-kernel, linux-wireless, netdev
In-Reply-To: <20161224181730.GD13590@amd>
[-- Attachment #1: Type: Text/Plain, Size: 1277 bytes --]
On Saturday 24 December 2016 19:17:30 Pavel Machek wrote:
> Hi!
>
> > In case there is no valid MAC address kernel generates random one.
> > This patch propagate this generated MAC address back to NVS data
> > which will be uploaded to wl1251 chip. So HW would have same MAC
> > address as linux kernel uses.
> >
> > return 0;
> >
> > }
> >
> > +static int wl1251_write_nvs_mac(struct wl1251 *wl)
> > +{
>
> The name is quite confusing, this sounds like writing into
> non-volatile storage.
>
> > + int i;
> > +
> > + if (wl->nvs_len < 0x24)
> > + return -ENODATA;
> > +
> > + /* length is 2 and data address is 0x546c (mask is 0xfffe) */
>
> You don't actually check for the mask.
It is quite complicated. { 0x6d, 0x54 } (= 0x546d) in data represent
address 0x546c and content are data. You need to apply mask 0xfffe for
0x546d and you get address where data will be written (so 0x546c).
> > + if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b]
> > != 0x54) + return -EINVAL;
>
> You have two copies of these. Does it make sense to move it to helper
> function?
I'm thinking if checks is really needed. But probably moving it to
separate function is good idea.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH 5/6] wl1251: Parse and use MAC address from supplied NVS data
From: Pali Rohár @ 2016-12-24 18:40 UTC (permalink / raw)
To: Pavel Machek
Cc: Ming Lei, Luis R. Rodriguez, Greg Kroah-Hartman, Kalle Valo,
David Gnedt, Michal Kazior, Daniel Wagner, Tony Lindgren,
Sebastian Reichel, Ivaylo Dimitrov, Aaro Koskinen,
Grazvydas Ignotas, linux-kernel, linux-wireless, netdev
In-Reply-To: <20161224181421.GC13590@amd>
[-- Attachment #1: Type: Text/Plain, Size: 663 bytes --]
On Saturday 24 December 2016 19:14:21 Pavel Machek wrote:
> On Sat 2016-12-24 17:53:00, Pali Rohár wrote:
> > @@ -1581,10 +1598,16 @@ int wl1251_init_ieee80211(struct wl1251
> > *wl)
> >
> > wl->hw->queues = 4;
> >
> > + if (wl->nvs == NULL && !wl->use_eeprom) {
> > + ret = wl1251_fetch_nvs(wl);
> > + if (ret < 0)
> > + goto out;
> > + }
>
> Is goto out here good idea? IMNSHO it is copy&paste bug, it should
> just proceed with generating random address.
No, goto is correct here. wl1251 cannot be initialized without NVS data.
And when fetching (from userspace) fails it is fatal error.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH 4/6] wl1251: Generate random MAC address only if driver does not have valid
From: Pali Rohár @ 2016-12-24 18:38 UTC (permalink / raw)
To: Pavel Machek
Cc: Ming Lei, Luis R. Rodriguez, Greg Kroah-Hartman, Kalle Valo,
David Gnedt, Michal Kazior, Daniel Wagner, Tony Lindgren,
Sebastian Reichel, Ivaylo Dimitrov, Aaro Koskinen,
Grazvydas Ignotas, linux-kernel, linux-wireless, netdev
In-Reply-To: <20161224180854.GB13590@amd>
[-- Attachment #1: Type: Text/Plain, Size: 1251 bytes --]
On Saturday 24 December 2016 19:08:54 Pavel Machek wrote:
> On Sat 2016-12-24 17:52:59, Pali Rohár wrote:
> > Before this patch driver generated random MAC address every time
> > when was doing initialization. And after that random MAC address
> > could be overwritten with fixed one if provided.
>
> Before this patch, driver generated random MAC address every time it
> was initialized. After that random MAC address could be overwritten
> with fixed one, if provided.
>
> > This patch changes order. First it tries to read fixed MAC address
> > and if it fails then driver generates random MAC address.
>
> I don't quite get where the advantage is supposed to be. Is it that
> "use_eeprom" is set, but reading fails?
Random bytes are read from kernel only if random MAC address is needed.
And in wl->mac_addr is always either invalid address or permanenent mac
address which will be used. Without patch in wl->mac_addr can be random
temporary address for some time...
> The only case where this helps is if wl1251_read_eeprom_mac()
> succeeds but reads invalid address.
>
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>
> Acked-by: Pavel Machek <pavel@ucw.cz>
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH 6/6] wl1251: Set generated MAC address back to NVS data
From: Pavel Machek @ 2016-12-24 18:17 UTC (permalink / raw)
To: Pali Rohár
Cc: Ming Lei, Luis R. Rodriguez, Greg Kroah-Hartman, Kalle Valo,
David Gnedt, Michal Kazior, Daniel Wagner, Tony Lindgren,
Sebastian Reichel, Ivaylo Dimitrov, Aaro Koskinen,
Grazvydas Ignotas, linux-kernel, linux-wireless, netdev
In-Reply-To: <1482598381-16513-7-git-send-email-pali.rohar@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 960 bytes --]
Hi!
> In case there is no valid MAC address kernel generates random one. This
> patch propagate this generated MAC address back to NVS data which will be
> uploaded to wl1251 chip. So HW would have same MAC address as linux kernel
> uses.
> return 0;
> }
>
> +static int wl1251_write_nvs_mac(struct wl1251 *wl)
> +{
The name is quite confusing, this sounds like writing into
non-volatile storage.
> + int i;
> +
> + if (wl->nvs_len < 0x24)
> + return -ENODATA;
> +
> + /* length is 2 and data address is 0x546c (mask is 0xfffe) */
You don't actually check for the mask.
> + if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b] != 0x54)
> + return -EINVAL;
You have two copies of these. Does it make sense to move it to helper
function?
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ 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