Re: [PATCH v3 2/2] drivers: net: ethernet: 3com: fix return value
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 Signed-off-by: Milan Stephan --- 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
RE: [PATCH iproute2 v3 2/4] ifstat: Add extended statistics to ifstat
> -Original Message-
> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Thursday, December 22, 2016 8:59 PM
> To: Nogah Frankel
> Cc: netdev@vger.kernel.org; ro...@cumulusnetworks.com; roszenr...@gmail.com;
> Or
> Gerlitz ; Jiri Pirko ; Elad Raz
> ; Yotam Gigi ; Ido Schimmel
>
> Subject: Re: [PATCH iproute2 v3 2/4] ifstat: Add extended statistics to ifstat
>
> On Thu, 22 Dec 2016 18:23:13 +0200
> Nogah Frankel wrote:
> On Thu, 22 Dec 2016 18:23:13 +0200
> Nogah Frankel 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.
RE: [PATCH iproute2 v3 4/4] ifstat: Add "sw only" extended statistics to ifstat
> -Original Message- > From: Roopa Prabhu [mailto:ro...@cumulusnetworks.com] > Sent: Thursday, December 22, 2016 11:10 PM > To: Nogah Frankel > Cc: netdev@vger.kernel.org; step...@networkplumber.org; roszenr...@gmail.com; > Or > Gerlitz ; Jiri Pirko ; Elad Raz > ; Yotam Gigi ; Ido Schimmel > > 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.
RE: [PATCH v3 net-next] bnx2x: ethtool -x support for rss_key
+ 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].
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
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
> ---
> 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);
>
Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data
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 > > --- > > > > 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/ma
Re: [PATCH net 3/9] virtio-net: fix page miscount during XDP linearizing
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 Signed-off-by: Jason Wang --- 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 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
Re: [PATCH net 4/9] virtio-net: correctly handle XDP_PASS for linearized packets
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
Signed-off-by: Jason Wang
---
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
Re: [PATCH net 7/9] virtio-net: forbid XDP when VIRTIO_NET_F_GUEST_UFO is support
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
Signed-off-by: Jason Wang
---
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
Re: [PATCH net 0/9] several fixups for virtio-net XDP
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.
Re: 4.9.0-rc8: tg3 dead after resume
On Mon, Dec 12, 2016 at 3:53 PM, Siva Reddy Kallam wrote: > On Fri, Dec 9, 2016 at 7:59 PM, Billy Shuman wrote: >> On Thu, Dec 8, 2016 at 4:03 AM, Siva Reddy Kallam >> wrote: >>> On Thu, Dec 8, 2016 at 12:14 AM, Billy Shuman wrote: On Wed, Dec 7, 2016 at 12:37 PM, Michael Chan wrote: > On Wed, Dec 7, 2016 at 7:20 AM, Billy Shuman wrote: >> After resume on 4.9.0-rc8 tg3 is dead. >> >> In logs I see: >> kernel: tg3 :44:00.0: phy probe failed, err -19 >> kernel: tg3 :44:00.0: Problem fetching invariants of chip, aborting > > -19 is -ENODEV which means tg3 cannot read the PHY ID. > > If it's a true suspend/resume operation, the driver does not have to > go through probe during resume. Please explain how you do > suspend/resume. > Sorry my previous message was accidentally sent to early. I used systemd (systemctl suspend) to suspend. >>> We need more information to proceed further. >>> Without suspend, Are you able to use the tg3 port? >> >> Yes the port works fine without suspend. > OK >> >>> Which Broadcom card are you having in laptop? >> >> The nic is a NetXtreme BCM57762 Gigabit Ethernet PCIe in a thunderbolt3 dock. >> > OK >>> Please provide complete tg3 specific logs in dmesg. >>> >> >> [ 32.084010] tg3.c:v3.137 (May 11, 2014) >> [ 32.124695] tg3 :44:00.0 eth0: Tigon3 [partno(BCM957762) rev >> 57766001] (PCI Express) MAC address 98:e7:f4:8b:13:19 >> [ 32.124698] tg3 :44:00.0 eth0: attached PHY is 57765 >> (10/100/1000Base-T Ethernet) (WireSpeed[1], EEE[1]) >> [ 32.124699] tg3 :44:00.0 eth0: RXcsums[1] LinkChgREG[0] >> MIirq[0] ASF[0] TSOcap[1] >> [ 32.124700] tg3 :44:00.0 eth0: dma_rwctrl[0001] dma_mask[64-bit] >> [ 32.219764] tg3 :44:00.0 enp68s0: renamed from eth0 >> [ 36.219245] tg3 :44:00.0 enp68s0: Link is up at 1000 Mbps, full duplex >> [ 36.219250] tg3 :44:00.0 enp68s0: Flow control is on for TX and on >> for RX >> [ 36.219251] tg3 :44:00.0 enp68s0: EEE is disabled >> >> after resume >> [ 92.292838] tg3 :44:00.0 enp68s0: No firmware running >> [ 93.521744] tg3 :44:00.0: tg3_abort_hw timed out, >> TX_MODE_ENABLE will not clear MAC_TX_MODE= >> [ 106.704655] tg3 :44:00.0 enp68s0: Link is down >> [ 108.370356] tg3 :44:00.0: tg3_abort_hw timed out, >> TX_MODE_ENABLE will not clear MAC_TX_MODE= >> >> after rmmod, modprobe >> [ 570.933636] tg3 :44:00.0: tg3_abort_hw timed out, >> TX_MODE_ENABLE will not clear MAC_TX_MODE= >> [ 604.847215] tg3.c:v3.137 (May 11, 2014) >> [ 605.010075] tg3 :44:00.0: phy probe failed, err -19 >> [ 605.010077] tg3 :44:00.0: Problem fetching invariants of chip, >> aborting >> >> >> >> > We will try to reproduce and update you on this. We are unable to reproduce this issue with Ubuntu 16.10 (K4.8.0-22) kernel. We are in the process of verifying with 4.9.0-rc8 kernel and let you know the feedback. Can you please let us know the make/model of your laptop and procedure followed to enable tg3 driver? > Did this work before? There has been very few changes to tg3 recently. > This is a new laptop for me, but the same behavior is seen on 4.4.36 and 4.8.12. >> >> rmmod and modprobe does not fix the problem only a reboot resolves the >> issue. >> >> Billy
Re: [RFC PATCH 4.10 1/6] crypto/sha256: Refactor the API so it can be used without shash
On Sat, Dec 24, 2016 at 09:57:53AM -0800, Andy Lutomirski wrote: > > I actually do use incremental hashing later on. BPF currently > vmallocs() a big temporary buffer just so it can fill it and hash it. > I change it to hash as it goes. How much data is this supposed to hash on average? If it's a large amount then perhaps using the existing crypto API would be a better option than adding this. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
