* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Marc Zyngier @ 2019-02-27 10:02 UTC (permalink / raw)
To: Brian Norris
Cc: Amitkumar Karwar, Enric Balletbo i Serra, Ganapathi Bhat,
Heiko Stuebner, Kalle Valo, Nishant Sarmukadam, Rob Herring,
Xinming Hu, David S. Miller, devicetree, linux-arm-kernel,
linux-kernel, linux-rockchip, linux-wireless, netdev, linux-pm,
Jeffy Chen, Rafael J. Wysocki, Tony Lindgren, Lorenzo Pieralisi
In-Reply-To: <20190226232822.GA174696@google.com>
+ Lorenzo
Hi Brian,
On 26/02/2019 23:28, Brian Norris wrote:
> + others
>
> Hi Marc,
>
> Thanks for the series. I have a few bits of history to add to this, and
> some comments.
>
> On Sun, Feb 24, 2019 at 02:04:22PM +0000, Marc Zyngier wrote:
>> For quite some time, I wondered why the PCI mwifiex device built in my
>> Chromebook was unable to use the good old legacy interrupts. But as MSIs
>> were working fine, I never really bothered investigating. I finally had a
>> look, and the result isn't very pretty.
>>
>> On this machine (rk3399-based kevin), the wake-up interrupt is described as
>> such:
>>
>> &pci_rootport {
>> mvl_wifi: wifi@0,0 {
>> compatible = "pci1b4b,2b42";
>> reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
>> 0x83010000 0x0 0x00100000 0x0 0x00100000>;
>> interrupt-parent = <&gpio0>;
>> interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
>> pinctrl-names = "default";
>> pinctrl-0 = <&wlan_host_wake_l>;
>> wakeup-source;
>> };
>> };
>>
>> Note how the interrupt is part of the properties directly attached to the
>> PCI node. And yet, this interrupt has nothing to do with a PCI legacy
>> interrupt, as it is attached to the wake-up widget that bypasses the PCIe RC
>> altogether (Yay for the broken design!). This is in total violation of the
>> IEEE Std 1275-1994 spec[1], which clearly documents that such interrupt
>> specifiers describe the PCI device interrupts, and must obey the
>> INT-{A,B,C,D} mapping. Oops!
>
> You're not the first person to notice this. All the motivations are not
> necessarily painted clearly in their cover letter, but here are some
> previous attempts at solving this problem:
>
> [RFC PATCH v11 0/5] PCI: rockchip: Move PCIe WAKE# handling into pci core
> https://lkml.kernel.org/lkml/20171225114742.18920-1-jeffy.chen@rock-chips.com/
> http://lkml.kernel.org/lkml/20171226023646.17722-1-jeffy.chen@rock-chips.com/
>
> As you can see by the 12th iteration, it wasn't left unsolved for lack
> of trying...
I wasn't aware of this. That's definitely a better approach than my
hack, and I would really like this to be revived.
>
> Frankly, if a proper DT replacement to the admittedly bad binding isn't
> agreed upon quickly, I'd be very happy to just have WAKE# support
> removed from the DTS for now, and the existing mwifiex binding should
> just be removed. (Wake-on-WiFi was never properly vetted on these
> platforms anyway.) It mostly serves to just cause problems like you've
> noticed.
Agreed. If there is no actual use for this, and that we can build a case
for a better solution, let's remove the wakeup support from the Gru DT
(it is invalid anyway), and bring it back if and when we get the right
level of support.
[...]
> One problem Rockchip authors were also trying to resolve here is that
> PCIe WAKE# handling should not really be something the PCI device driver
> has to handle directly. Despite your complaints about not using in-band
> TLP wakeup, a separate WAKE# pin is in fact a documented part of the
> PCIe standard, and it so happens that the Rockchip RC does not support
> handling TLPs in S3, if you want to have decent power consumption. (Your
> "bad hardware" complaints could justifiably fall here, I suppose.)
>
> Additionally, I've had pushback from PCI driver authors/maintainers on
> adding more special handling for this sort of interrupt property (not
> the binding specifically, but just the concept of handling WAKE# in the
> driver), as they claim this should be handled by the system firmware,
> when they set the appropriate wakeup flags, which filter down to
> __pci_enable_wake() -> platform_pci_set_wakeup(). That's how x86 systems
> do it (note: I know for a fact that many have a very similar
> architecture -- WAKE# is not routed to the RC, because, why does it need
> to? and they *don't* use TLP wakeup either -- they just hide it in
> firmware better), and it Just Works.
Even on an arm64 platform, there is no reason why a wakeup interrupt
couldn't be handled by FW rather than the OS. It just need to be wired
to the right spot (so that it generates a secure interrupt that can be
handled by FW).
> So, we basically concluded that we should standardize on a way to
> describe WAKE# interrupts such that PCI drivers don't have to deal with
> it at all, and the PCI core can do it for us. 12 revisions later
> and...we still never agreed on a good device tree binding for this.
Is the DT binding the only problem? Do we have an agreement for the core
code?
> IOW, maybe your wake-up sub-node is the best way to side-step the
> problems of conflicting with the OF PCI spec. But I'd still really like
> to avoid parsing it in mwifiex, if at all possible.
Honestly, my solution is just a terrible hack. I wasn't aware that this
was a more general problem, and I'd love it to be addressed in the core
PCI code.
> (We'd still be left with the marvell,wakeup-pin propery to parse
> specifically in mwifiex, which sadly has to exist because....well,
> Samsung decided to do chip-on-board, and then they failed to use the
> correct pin on Marvell's side when wiring up WAKE#. Sigh.)
Oh well...
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Marc Zyngier @ 2019-02-27 9:27 UTC (permalink / raw)
To: Brian Norris
Cc: Ard Biesheuvel, Amitkumar Karwar, Enric Balletbo i Serra,
Ganapathi Bhat, Heiko Stuebner, Kalle Valo, Nishant Sarmukadam,
Rob Herring, Xinming Hu, Devicetree List,
<netdev@vger.kernel.org>,
<linux-wireless@vger.kernel.org>, Linux Kernel Mailing List,
linux-rockchip, David S. Miller, linux-arm-kernel, linux-pci,
linux-pm, Jeffy Chen
In-Reply-To: <20190226234422.GD174696@google.com>
On 26/02/2019 23:44, Brian Norris wrote:
> Hi,
>
> On Tue, Feb 26, 2019 at 05:14:00PM +0000, Marc Zyngier wrote:
>> On 26/02/2019 16:21, Ard Biesheuvel wrote:
>>> On Mon, 25 Feb 2019 at 15:53, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> It outlines one thing: If you have to interpret per-device PCI
>>>> properties from DT, you're in for serious trouble. I should get some
>>>> better HW.
>>>>
>>>
>>> Yeah, it obviously makes no sense at all for the interrupt parent of a
>>> PCI device to deviate from the host bridge's interrupt parent, and
>>> it's quite unfortunate that we can't simply ban it now that the cat is
>>> out of the bag already.
>>>
>>> Arguably, the wake up widget is not part of the PCI device, but I have
>>> no opinion as to whether it is better modeling it as a sub device as
>>> you are proposing or as an entirely separate device referenced via a
>>> phandle.
>>
>> It is not that clear. The widget seems to be an integral part of the
>> device, as it is the same basic IP that is used for SDIO and USB.
>
> It's not really a widget specific to this IP. It's just a GPIO. It so
> happens that both SDIO and PCIe designs have wanted to use a GPIO for
> wakeup, as many other devices do. (Note: it's not just cheap ARM
> devices; pulling up some Intel Chromebook designs, I see the exact same
> WAKE# GPIO on their PCIe WiFi as well.)
Arghh! If I can't even point people to an Intel design as an example of
something done halfway right, we're screwed! ;-)
>
>> It looks like the good old pre-PCI-2.2 days, where you had to have a
>> separate cable between your network card and the base-board for the
>> wake-up interrupt to be delivered. Starting with PCI-2.2, the bus can
>> carry the signal just fine. With PCIe, it should just be an interrupt
>> TLP sent to the RC, but that's obviously not within the capabilities of
>> the HW.
>
> You should search the PCI Express specification for WAKE#. There is a
> clearly-documented "side-band wake" feature that is part of the
> standard, as an alternative to in-band TLP wakeup. While you claim this
> is an ancient thing, it in fact still in use on many systems -- it's
> just usually abstracted better by ACPI firmware, whereas the dirty
> laundry is aired a bit more on a Device Tree system. And we got it
> wrong.
I stand corrected. I was really hoping for these side-band wires to be a
thing of the past, but clearly the world hasn't moved as quickly as I hoped.
>> Anyway, it'd be good if the Marvell people could chime in and let us
>> know how they'd prefer to handle this.
>
> I'm not sure this is really a Marvell-specific problem. (Well, except
> for the marvell,wakeup-pin silliness, which is somewhat orthogonal.) In
> fact, if we cared a little more about Wake-on-WiFi, we'd be trying to
> support the same (out-of-band WAKE#) with other WiFi drivers.
I'd definitely like to see this standardized. It would give us more
coverage, and prevent everyone from doing their own thing. But as you
mention, WoW doesn't seem to get much traction.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* Re: [PATCH net] selftests: fixes for UDP GRO
From: Paolo Abeni @ 2019-02-27 9:26 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, David S. Miller, Willem de Bruijn
In-Reply-To: <CAF=yD-LeXUVU26iX-di5vfdDvrfuuAcJmyGGpUGN5FHX0fi0Wg@mail.gmail.com>
Hi,
On Tue, 2019-02-26 at 13:38 -0500, Willem de Bruijn wrote:
> On Tue, Feb 26, 2019 at 9:28 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > The current implementation for UDP GRO tests is racy: the receiver
> > may flush the RX queue while the sending is still transmitting and
> > incorrectly report RX errors, with a wrong number of packet received.
> >
> > Add explicit timeouts to the receiver for both connection activation
> > (first packet received for UDP) and reception completion, so that
> > in the above critical scenario the receiver will wait for the
> > transfer completion.
> >
> > Fixes: 3327a9c46352 ("selftests: add functionals test for UDP GRO")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>
> Acked-by: Willem de Bruijn <willemb@google.com>
Thanks for reviewing.
> ---
>
> This is because of that "force termination after the second poll()"?
Yes, exactly.
> Could perhaps also just extend do_recv to wait while (!interrupted &&
> tnow < tstart + 2000) and avoid the explicit arguments.
I thought about that, but then UDP GRO self-tests would take a bit more
and the binary helper would be less re-usable for future test cases.
Cheers,
Paolo
^ permalink raw reply
* Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read
From: Harini Katakam @ 2019-02-27 9:05 UTC (permalink / raw)
To: Michal Simek
Cc: Paul Kocialkowski, Andrew Lunn, Florian Fainelli, netdev,
linux-arm-kernel, linux-kernel, David S . Miller,
Thomas Petazzoni, Heiner Kallweit
In-Reply-To: <8bb813fb-102b-00c9-fb6f-a3e928965051@xilinx.com>
Hi Andrew, Paul,
On Wed, Feb 27, 2019 at 2:15 PM Michal Simek <michal.simek@xilinx.com> wrote:
>
> On 21. 02. 19 12:03, Michal Simek wrote:
> > On 21. 02. 19 11:24, Paul Kocialkowski wrote:
> >> Hi,
> >>
> >> On Wed, 2019-02-20 at 07:58 +0100, Michal Simek wrote:
> >>> Hi,
> >>>
> >>> On 19. 02. 19 18:25, Andrew Lunn wrote:
> >>>>> Thanks for the suggestion! So I had a closer look at that driver to try
> >>>>> and see what could go wrong and it looks like I found a few things
> >>>>> there.
> >>>>
> >>>> Hi Paul
> >>>>
> >>>> Yes, this driver has issues. If i remember correctly, it got merged
> >>>> while i was on vacation. I pointed out a few issues, but the authors
> >>>> never responded. Feel free to fix it up.
> >>>
Sorry for this - I've synced up with the author and got the comments from the
time this driver was upstreamed. I'll try to address those and Paul's
suggestions
going forward.
> >>> Will be good to know who was that person.
> >>>
> >>> I can't do much this week with this because responsible person for this
> >>> driver is out of office this week. That's why please give us some time
> >>> to get back to this.
> >>
> >> Understood. I think we need to start a discussion about how the general
> >> design of this driver can be improved.
> >>
> >> In particular, I wonder if it could work better to make this driver a
> >> PHY driver that just redirects all its ops to the actual PHY driver,
> >> except for read_status where it should also add some code.
Thanks, I'm looking into this option and also a way to expose the correct
interface mode setting as you mentioned below. I'll get back before the
end of the week. Please do let me know if you have any further suggestions.
Regards,
Harini
> >
> > I didn't take a look at Linux driver but it should work in a way that it
> > checks description (more below) and then wait for attached phy to do its
> > work and on the way back just setup this bridge based on that.
> >
> >> Maybe we could also manage to expose a RGMII PHY mode to the actual PHY
> >> this way. Currently, the PHY mode has to be set to GMII for the MAC to
> >> be configured correctly, but the PHY also gets this information while
> >> it should be told that RGMII is in use. This doesn't seem to play a big
> >> role in PHY configuration though, but it's still inadequate.
> >>
> >> What do you think?
<snip>
^ permalink raw reply
* [PATCH net] bnxt_en: Drop oversize TX packets to prevent errors.
From: Michael Chan @ 2019-02-27 8:58 UTC (permalink / raw)
To: davem; +Cc: netdev
There have been reports of oversize UDP packets being sent to the
driver to be transmitted, causing error conditions. The issue is
likely caused by the dst of the SKB switching between 'lo' with
64K MTU and the hardware device with a smaller MTU. Patches are
being proposed by Mahesh Bandewar <maheshb@google.com> to fix the
issue.
In the meantime, add a quick length check in the driver to prevent
the error. The driver uses the TX packet size as index to look up an
array to setup the TX BD. The array is large enough to support all MTU
sizes supported by the driver. The oversize TX packet causes the
driver to index beyond the array and put garbage values into the
TX BD. Add a simple check to prevent this.
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
David, I think this should be queued for stable as well.
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index d95730c..803f799 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -500,6 +500,12 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
}
length >>= 9;
+ if (unlikely(length >= ARRAY_SIZE(bnxt_lhint_arr))) {
+ dev_warn_ratelimited(&pdev->dev, "Dropped oversize %d bytes TX packet.\n",
+ skb->len);
+ i = 0;
+ goto tx_dma_error;
+ }
flags |= bnxt_lhint_arr[length];
txbd->tx_bd_len_flags_type = cpu_to_le32(flags);
--
2.5.1
^ permalink raw reply related
* Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read
From: Michal Simek @ 2019-02-27 8:43 UTC (permalink / raw)
To: Michal Simek, Paul Kocialkowski, Andrew Lunn, Harini Katakam
Cc: Florian Fainelli, netdev, linux-arm-kernel, linux-kernel,
David S . Miller, Thomas Petazzoni, Heiner Kallweit
In-Reply-To: <9cb2f7a8-a8cf-ef80-d260-cc67c072b5c5@xilinx.com>
On 21. 02. 19 12:03, Michal Simek wrote:
> On 21. 02. 19 11:24, Paul Kocialkowski wrote:
>> Hi,
>>
>> On Wed, 2019-02-20 at 07:58 +0100, Michal Simek wrote:
>>> Hi,
>>>
>>> On 19. 02. 19 18:25, Andrew Lunn wrote:
>>>>> Thanks for the suggestion! So I had a closer look at that driver to try
>>>>> and see what could go wrong and it looks like I found a few things
>>>>> there.
>>>>
>>>> Hi Paul
>>>>
>>>> Yes, this driver has issues. If i remember correctly, it got merged
>>>> while i was on vacation. I pointed out a few issues, but the authors
>>>> never responded. Feel free to fix it up.
>>>
>>> Will be good to know who was that person.
>>>
>>> I can't do much this week with this because responsible person for this
>>> driver is out of office this week. That's why please give us some time
>>> to get back to this.
>>
>> Understood. I think we need to start a discussion about how the general
>> design of this driver can be improved.
>>
>> In particular, I wonder if it could work better to make this driver a
>> PHY driver that just redirects all its ops to the actual PHY driver,
>> except for read_status where it should also add some code.
>
> I didn't take a look at Linux driver but it should work in a way that it
> checks description (more below) and then wait for attached phy to do its
> work and on the way back just setup this bridge based on that.
>
>> Maybe we could also manage to expose a RGMII PHY mode to the actual PHY
>> this way. Currently, the PHY mode has to be set to GMII for the MAC to
>> be configured correctly, but the PHY also gets this information while
>> it should be told that RGMII is in use. This doesn't seem to play a big
>> role in PHY configuration though, but it's still inadequate.
>>
>> What do you think?
>
> I stop the driver to be applied to u-boot because exactly this
> gmii/rgmii configuration. There is a need that mac is configured to gmii
> and this needs to be checked but to phy rgmii needs to be exposed.
> I was trying to find out hardware design and board where I can do some
> experiments but didn't find out. That's why I need to wait for
> colleagues to point me to that.
Harini: Can you please respond this thread?
Thanks,
Michal
^ permalink raw reply
* Re: Handling an Extra Signal at PHY Reset
From: Paul Kocialkowski @ 2019-02-27 8:19 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, Heiner Kallweit, netdev, Thomas Petazzoni,
Mylène Josserand
In-Reply-To: <20190221140458.GF5653@lunn.ch>
Hi Andrew,
On Thu, 2019-02-21 at 15:04 +0100, Andrew Lunn wrote:
> > I can also confirm that it does not prevent contacting the PHY on the
> > MDIO bus, contrary to what I have stated previously.
>
> O.K, so wrong voltage does not matter, you can still probe the PHY.
>
> Ignore the switch. Use a pin hog to set the GPIO to the disabled
> state. And set the voltage using the PHY register during probe().
Thanks a lot for your suggestion! I have tried it out and it appears to
work just as well as before.
Although the CONFIG pin state is still sampled at PHY reset, having it
connected to the PTP clock instead of the LED pin does not seem to
change the address we're getting.
We'll stick with that solution, since it's the least invasive one.
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
^ permalink raw reply
* [PATCH v2 net] ipv4: Add ICMPv6 support when parse route ipproto
From: Hangbin Liu @ 2019-02-27 8:15 UTC (permalink / raw)
To: netdev; +Cc: David Ahern, Roopa Prabhu, davem, Sabrina Dubroca, Hangbin Liu
In-Reply-To: <20190225074700.7316-1-liuhangbin@gmail.com>
For ip rules, we need to use 'ipproto ipv6-icmp' to match ICMPv6 headers.
But for ip -6 route, currently we only support tcp, udp and icmp.
Add ICMPv6 support so we can match ipv6-icmp rules for route lookup.
v2: As David Ahern and Sabrina Dubroca suggested, Add an argument to
rtm_getroute_parse_ip_proto() to handle ICMP/ICMPv6 with different family.
Reported-by: Jianlin Shi <jishi@redhat.com>
Fixes: eacb9384a3fe ("ipv6: support sport, dport and ip_proto in RTM_GETROUTE")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
include/net/ip.h | 2 +-
net/ipv4/netlink.c | 17 +++++++++++++----
net/ipv4/route.c | 2 +-
net/ipv6/route.c | 3 ++-
4 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/include/net/ip.h b/include/net/ip.h
index 8866bfce6121..80300e8a6280 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -716,7 +716,7 @@ extern int sysctl_icmp_msgs_burst;
int ip_misc_proc_init(void);
#endif
-int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto,
+int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto, u8 family,
struct netlink_ext_ack *extack);
#endif /* _IP_H */
diff --git a/net/ipv4/netlink.c b/net/ipv4/netlink.c
index f86bb4f06609..d8e3a1fb8e82 100644
--- a/net/ipv4/netlink.c
+++ b/net/ipv4/netlink.c
@@ -3,9 +3,10 @@
#include <linux/types.h>
#include <net/net_namespace.h>
#include <net/netlink.h>
+#include <linux/in6.h>
#include <net/ip.h>
-int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto,
+int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto, u8 family,
struct netlink_ext_ack *extack)
{
*ip_proto = nla_get_u8(attr);
@@ -13,11 +14,19 @@ int rtm_getroute_parse_ip_proto(struct nlattr *attr, u8 *ip_proto,
switch (*ip_proto) {
case IPPROTO_TCP:
case IPPROTO_UDP:
+ return 0;
case IPPROTO_ICMP:
+ if (family != AF_INET)
+ break;
+ return 0;
+#if IS_ENABLED(CONFIG_IPV6)
+ case IPPROTO_ICMPV6:
+ if (family != AF_INET6)
+ break;
return 0;
- default:
- NL_SET_ERR_MSG(extack, "Unsupported ip proto");
- return -EOPNOTSUPP;
+#endif
}
+ NL_SET_ERR_MSG(extack, "Unsupported ip proto");
+ return -EOPNOTSUPP;
}
EXPORT_SYMBOL_GPL(rtm_getroute_parse_ip_proto);
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 5163b64f8fb3..7bb9128c8363 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2803,7 +2803,7 @@ static int inet_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
if (tb[RTA_IP_PROTO]) {
err = rtm_getroute_parse_ip_proto(tb[RTA_IP_PROTO],
- &ip_proto, extack);
+ &ip_proto, AF_INET, extack);
if (err)
return err;
}
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ce15dc4ccbfa..6f949aa5b64f 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4889,7 +4889,8 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
if (tb[RTA_IP_PROTO]) {
err = rtm_getroute_parse_ip_proto(tb[RTA_IP_PROTO],
- &fl6.flowi6_proto, extack);
+ &fl6.flowi6_proto, AF_INET6,
+ extack);
if (err)
goto errout;
}
--
2.19.2
^ permalink raw reply related
* Re: [PATCH net-next v2] net: sched: act_tunnel_key: fix metadata handling
From: Roi Dayan @ 2019-02-27 8:14 UTC (permalink / raw)
To: Vlad Buslov, netdev@vger.kernel.org, dcaratti@redhat.com
Cc: jhs@mojatatu.com, xiyou.wangcong@gmail.com, jiri@resnulli.us,
davem@davemloft.net, wenxu@ucloud.cn
In-Reply-To: <20190225153014.8885-1-vladbu@mellanox.com>
On 25/02/2019 17:30, Vlad Buslov wrote:
> Tunnel key action params->tcft_enc_metadata is only set when action is
> TCA_TUNNEL_KEY_ACT_SET. However, metadata pointer is incorrectly
> dereferenced during tunnel key init and release without verifying that
> action is if correct type, which causes NULL pointer dereference. Metadata
> tunnel dst_cache is also leaked on action overwrite.
>
> Fix metadata handling:
> - Verify that metadata pointer is not NULL before dereferencing it in
> tunnel_key_init error handling code.
> - Move dst_cache destroy code into tunnel_key_release_params() function
> that is called in both action overwrite and release cases (fixes resource
> leak) and verifies that actions has correct type before dereferencing
> metadata pointer (fixes NULL pointer dereference).
>
> Oops with KASAN enabled during tdc tests execution:
>
> [ 261.080482] ==================================================================
> [ 261.088049] BUG: KASAN: null-ptr-deref in dst_cache_destroy+0x21/0xa0
> [ 261.094613] Read of size 8 at addr 00000000000000b0 by task tc/2976
> [ 261.102524] CPU: 14 PID: 2976 Comm: tc Not tainted 5.0.0-rc7+ #157
> [ 261.108844] Hardware name: Supermicro SYS-2028TP-DECR/X10DRT-P, BIOS 2.0b 03/30/2017
> [ 261.116726] Call Trace:
> [ 261.119234] dump_stack+0x9a/0xeb
> [ 261.122625] ? dst_cache_destroy+0x21/0xa0
> [ 261.126818] ? dst_cache_destroy+0x21/0xa0
> [ 261.131004] kasan_report+0x176/0x192
> [ 261.134752] ? idr_get_next+0xd0/0x120
> [ 261.138578] ? dst_cache_destroy+0x21/0xa0
> [ 261.142768] dst_cache_destroy+0x21/0xa0
> [ 261.146799] tunnel_key_release+0x3a/0x50 [act_tunnel_key]
> [ 261.152392] tcf_action_cleanup+0x2c/0xc0
> [ 261.156490] tcf_generic_walker+0x4c2/0x5c0
> [ 261.160794] ? tcf_action_dump_1+0x390/0x390
> [ 261.165163] ? tunnel_key_walker+0x5/0x1a0 [act_tunnel_key]
> [ 261.170865] ? tunnel_key_walker+0xe9/0x1a0 [act_tunnel_key]
> [ 261.176641] tca_action_gd+0x600/0xa40
> [ 261.180482] ? tca_get_fill.constprop.17+0x200/0x200
> [ 261.185548] ? __lock_acquire+0x588/0x1d20
> [ 261.189741] ? __lock_acquire+0x588/0x1d20
> [ 261.193922] ? mark_held_locks+0x90/0x90
> [ 261.197944] ? mark_held_locks+0x90/0x90
> [ 261.202018] ? __nla_parse+0xfe/0x190
> [ 261.205774] tc_ctl_action+0x218/0x230
> [ 261.209614] ? tcf_action_add+0x230/0x230
> [ 261.213726] rtnetlink_rcv_msg+0x3a5/0x600
> [ 261.217910] ? lock_downgrade+0x2d0/0x2d0
> [ 261.222006] ? validate_linkmsg+0x400/0x400
> [ 261.226278] ? find_held_lock+0x6d/0xd0
> [ 261.230200] ? match_held_lock+0x1b/0x210
> [ 261.234296] ? validate_linkmsg+0x400/0x400
> [ 261.238567] netlink_rcv_skb+0xc7/0x1f0
> [ 261.242489] ? netlink_ack+0x470/0x470
> [ 261.246319] ? netlink_deliver_tap+0x1f3/0x5a0
> [ 261.250874] netlink_unicast+0x2ae/0x350
> [ 261.254884] ? netlink_attachskb+0x340/0x340
> [ 261.261647] ? _copy_from_iter_full+0xdd/0x380
> [ 261.268576] ? __virt_addr_valid+0xb6/0xf0
> [ 261.275227] ? __check_object_size+0x159/0x240
> [ 261.282184] netlink_sendmsg+0x4d3/0x630
> [ 261.288572] ? netlink_unicast+0x350/0x350
> [ 261.295132] ? netlink_unicast+0x350/0x350
> [ 261.301608] sock_sendmsg+0x6d/0x80
> [ 261.307467] ___sys_sendmsg+0x48e/0x540
> [ 261.313633] ? copy_msghdr_from_user+0x210/0x210
> [ 261.320545] ? save_stack+0x89/0xb0
> [ 261.326289] ? __lock_acquire+0x588/0x1d20
> [ 261.332605] ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 261.340063] ? mark_held_locks+0x90/0x90
> [ 261.346162] ? do_filp_open+0x138/0x1d0
> [ 261.352108] ? may_open_dev+0x50/0x50
> [ 261.357897] ? match_held_lock+0x1b/0x210
> [ 261.364016] ? __fget_light+0xa6/0xe0
> [ 261.369840] ? __sys_sendmsg+0xd2/0x150
> [ 261.375814] __sys_sendmsg+0xd2/0x150
> [ 261.381610] ? __ia32_sys_shutdown+0x30/0x30
> [ 261.388026] ? lock_downgrade+0x2d0/0x2d0
> [ 261.394182] ? mark_held_locks+0x1c/0x90
> [ 261.400230] ? do_syscall_64+0x1e/0x280
> [ 261.406172] do_syscall_64+0x78/0x280
> [ 261.411932] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 261.419103] RIP: 0033:0x7f28e91a8b87
> [ 261.424791] Code: 64 89 02 48 c7 c0 ff ff ff ff eb b9 0f 1f 80 00 00 00 00 8b 05 6a 2b 2c 00 48 63 d2 48 63 ff 85 c0 75 18 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 59 f3 c3 0f 1f 80 00 00 00 00 53 48 89 f3 48
> [ 261.448226] RSP: 002b:00007ffdc5c4e2d8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> [ 261.458183] RAX: ffffffffffffffda RBX: 000000005c73c202 RCX: 00007f28e91a8b87
> [ 261.467728] RDX: 0000000000000000 RSI: 00007ffdc5c4e340 RDI: 0000000000000003
> [ 261.477342] RBP: 0000000000000000 R08: 0000000000000001 R09: 000000000000000c
> [ 261.486970] R10: 000000000000000c R11: 0000000000000246 R12: 0000000000000001
> [ 261.496599] R13: 000000000067b4e0 R14: 00007ffdc5c5248c R15: 00007ffdc5c52480
> [ 261.506281] ==================================================================
> [ 261.516076] Disabling lock debugging due to kernel taint
> [ 261.523979] BUG: unable to handle kernel NULL pointer dereference at 00000000000000b0
> [ 261.534413] #PF error: [normal kernel read fault]
> [ 261.541730] PGD 8000000317400067 P4D 8000000317400067 PUD 316878067 PMD 0
> [ 261.551294] Oops: 0000 [#1] SMP KASAN PTI
> [ 261.557985] CPU: 14 PID: 2976 Comm: tc Tainted: G B 5.0.0-rc7+ #157
> [ 261.568306] Hardware name: Supermicro SYS-2028TP-DECR/X10DRT-P, BIOS 2.0b 03/30/2017
> [ 261.578874] RIP: 0010:dst_cache_destroy+0x21/0xa0
> [ 261.586413] Code: f4 ff ff ff eb f6 0f 1f 00 0f 1f 44 00 00 41 56 41 55 49 c7 c6 60 fe 35 af 41 54 55 49 89 fc 53 bd ff ff ff ff e8 ef 98 73 ff <49> 83 3c 24 00 75 35 eb 6c 4c 63 ed e8 de 98 73 ff 4a 8d 3c ed 40
> [ 261.611247] RSP: 0018:ffff888316447160 EFLAGS: 00010282
> [ 261.619564] RAX: 0000000000000000 RBX: ffff88835b3e2f00 RCX: ffffffffad1c5071
> [ 261.629862] RDX: 0000000000000003 RSI: dffffc0000000000 RDI: 0000000000000297
> [ 261.640149] RBP: 00000000ffffffff R08: fffffbfff5dd4e89 R09: fffffbfff5dd4e89
> [ 261.650467] R10: 0000000000000001 R11: fffffbfff5dd4e88 R12: 00000000000000b0
> [ 261.660785] R13: ffff8883267a10c0 R14: ffffffffaf35fe60 R15: 0000000000000001
> [ 261.671110] FS: 00007f28ea3e6400(0000) GS:ffff888364200000(0000) knlGS:0000000000000000
> [ 261.682447] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 261.691491] CR2: 00000000000000b0 CR3: 00000003178ae004 CR4: 00000000001606e0
> [ 261.701283] Call Trace:
> [ 261.706374] tunnel_key_release+0x3a/0x50 [act_tunnel_key]
> [ 261.714522] tcf_action_cleanup+0x2c/0xc0
> [ 261.721208] tcf_generic_walker+0x4c2/0x5c0
> [ 261.728074] ? tcf_action_dump_1+0x390/0x390
> [ 261.734996] ? tunnel_key_walker+0x5/0x1a0 [act_tunnel_key]
> [ 261.743247] ? tunnel_key_walker+0xe9/0x1a0 [act_tunnel_key]
> [ 261.751557] tca_action_gd+0x600/0xa40
> [ 261.757991] ? tca_get_fill.constprop.17+0x200/0x200
> [ 261.765644] ? __lock_acquire+0x588/0x1d20
> [ 261.772461] ? __lock_acquire+0x588/0x1d20
> [ 261.779266] ? mark_held_locks+0x90/0x90
> [ 261.785880] ? mark_held_locks+0x90/0x90
> [ 261.792470] ? __nla_parse+0xfe/0x190
> [ 261.798738] tc_ctl_action+0x218/0x230
> [ 261.805145] ? tcf_action_add+0x230/0x230
> [ 261.811760] rtnetlink_rcv_msg+0x3a5/0x600
> [ 261.818564] ? lock_downgrade+0x2d0/0x2d0
> [ 261.825433] ? validate_linkmsg+0x400/0x400
> [ 261.832256] ? find_held_lock+0x6d/0xd0
> [ 261.838624] ? match_held_lock+0x1b/0x210
> [ 261.845142] ? validate_linkmsg+0x400/0x400
> [ 261.851729] netlink_rcv_skb+0xc7/0x1f0
> [ 261.857976] ? netlink_ack+0x470/0x470
> [ 261.864132] ? netlink_deliver_tap+0x1f3/0x5a0
> [ 261.870969] netlink_unicast+0x2ae/0x350
> [ 261.877294] ? netlink_attachskb+0x340/0x340
> [ 261.883962] ? _copy_from_iter_full+0xdd/0x380
> [ 261.890750] ? __virt_addr_valid+0xb6/0xf0
> [ 261.897188] ? __check_object_size+0x159/0x240
> [ 261.903928] netlink_sendmsg+0x4d3/0x630
> [ 261.910112] ? netlink_unicast+0x350/0x350
> [ 261.916410] ? netlink_unicast+0x350/0x350
> [ 261.922656] sock_sendmsg+0x6d/0x80
> [ 261.928257] ___sys_sendmsg+0x48e/0x540
> [ 261.934183] ? copy_msghdr_from_user+0x210/0x210
> [ 261.940865] ? save_stack+0x89/0xb0
> [ 261.946355] ? __lock_acquire+0x588/0x1d20
> [ 261.952358] ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 261.959468] ? mark_held_locks+0x90/0x90
> [ 261.965248] ? do_filp_open+0x138/0x1d0
> [ 261.970910] ? may_open_dev+0x50/0x50
> [ 261.976386] ? match_held_lock+0x1b/0x210
> [ 261.982210] ? __fget_light+0xa6/0xe0
> [ 261.987648] ? __sys_sendmsg+0xd2/0x150
> [ 261.993263] __sys_sendmsg+0xd2/0x150
> [ 261.998613] ? __ia32_sys_shutdown+0x30/0x30
> [ 262.004555] ? lock_downgrade+0x2d0/0x2d0
> [ 262.010236] ? mark_held_locks+0x1c/0x90
> [ 262.015758] ? do_syscall_64+0x1e/0x280
> [ 262.021234] do_syscall_64+0x78/0x280
> [ 262.026500] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 262.033207] RIP: 0033:0x7f28e91a8b87
> [ 262.038421] Code: 64 89 02 48 c7 c0 ff ff ff ff eb b9 0f 1f 80 00 00 00 00 8b 05 6a 2b 2c 00 48 63 d2 48 63 ff 85 c0 75 18 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 59 f3 c3 0f 1f 80 00 00 00 00 53 48 89 f3 48
> [ 262.060708] RSP: 002b:00007ffdc5c4e2d8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> [ 262.070112] RAX: ffffffffffffffda RBX: 000000005c73c202 RCX: 00007f28e91a8b87
> [ 262.079087] RDX: 0000000000000000 RSI: 00007ffdc5c4e340 RDI: 0000000000000003
> [ 262.088122] RBP: 0000000000000000 R08: 0000000000000001 R09: 000000000000000c
> [ 262.097157] R10: 000000000000000c R11: 0000000000000246 R12: 0000000000000001
> [ 262.106207] R13: 000000000067b4e0 R14: 00007ffdc5c5248c R15: 00007ffdc5c52480
> [ 262.115271] Modules linked in: act_tunnel_key act_skbmod act_simple act_connmark nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 act_csum libcrc32c act_meta_skbtcindex act_meta_skbprio act_meta_mark act_ife ife act_police act_sample psample act_gact veth nfsv3 nfs_acl nfs lockd grace fscache bridge stp llc intel_rapl sb_edac mlx5_ib x86_pkg_temp_thermal sunrpc intel_powerclamp coretemp ib_uverbs kvm_intel ib_core kvm irqbypass mlx5_core crct10dif_pclmul crc32_pclmul crc32c_intel igb ghash_clmulni_intel intel_cstate mlxfw iTCO_wdt devlink intel_uncore iTCO_vendor_support ipmi_ssif ptp mei_me intel_rapl_perf ioatdma joydev pps_core ses mei i2c_i801 pcspkr enclosure lpc_ich dca wmi ipmi_si ipmi_devintf ipmi_msghandler acpi_pad acpi_power_meter pcc_cpufreq ast i2c_algo_bit drm_kms_helper ttm drm mpt3sas raid_class scsi_transport_sas
> [ 262.204393] CR2: 00000000000000b0
> [ 262.210390] ---[ end trace 2e41d786f2c7901a ]---
> [ 262.226790] RIP: 0010:dst_cache_destroy+0x21/0xa0
> [ 262.234083] Code: f4 ff ff ff eb f6 0f 1f 00 0f 1f 44 00 00 41 56 41 55 49 c7 c6 60 fe 35 af 41 54 55 49 89 fc 53 bd ff ff ff ff e8 ef 98 73 ff <49> 83 3c 24 00 75 35 eb 6c 4c 63 ed e8 de 98 73 ff 4a 8d 3c ed 40
> [ 262.258311] RSP: 0018:ffff888316447160 EFLAGS: 00010282
> [ 262.266304] RAX: 0000000000000000 RBX: ffff88835b3e2f00 RCX: ffffffffad1c5071
> [ 262.276251] RDX: 0000000000000003 RSI: dffffc0000000000 RDI: 0000000000000297
> [ 262.286208] RBP: 00000000ffffffff R08: fffffbfff5dd4e89 R09: fffffbfff5dd4e89
> [ 262.296183] R10: 0000000000000001 R11: fffffbfff5dd4e88 R12: 00000000000000b0
> [ 262.306157] R13: ffff8883267a10c0 R14: ffffffffaf35fe60 R15: 0000000000000001
> [ 262.316139] FS: 00007f28ea3e6400(0000) GS:ffff888364200000(0000) knlGS:0000000000000000
> [ 262.327146] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 262.335815] CR2: 00000000000000b0 CR3: 00000003178ae004 CR4: 00000000001606e0
>
> Fixes: 41411e2fd6b8 ("net/sched: act_tunnel_key: Add dst_cache support")
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> ---
> Changes from V1 to V2:
> - Extract metadata->dst error handler fix into standalone patch that targets
> net.
>
> net/sched/act_tunnel_key.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
> index 3404af72b4c1..fc2b884b170c 100644
> --- a/net/sched/act_tunnel_key.c
> +++ b/net/sched/act_tunnel_key.c
> @@ -201,8 +201,14 @@ static void tunnel_key_release_params(struct tcf_tunnel_key_params *p)
> {
> if (!p)
> return;
> - if (p->tcft_action == TCA_TUNNEL_KEY_ACT_SET)
> + if (p->tcft_action == TCA_TUNNEL_KEY_ACT_SET) {
> +#ifdef CONFIG_DST_CACHE
> + struct ip_tunnel_info *info = &p->tcft_enc_metadata->u.tun_info;
> +
> + dst_cache_destroy(&info->dst_cache);
> +#endif
> dst_release(&p->tcft_enc_metadata->dst);
> + }
> kfree_rcu(p, rcu);
> }
>
> @@ -384,7 +390,8 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
>
> release_dst_cache:
> #ifdef CONFIG_DST_CACHE
> - dst_cache_destroy(&metadata->u.tun_info.dst_cache);
> + if (metadata)
> + dst_cache_destroy(&metadata->u.tun_info.dst_cache);
> #endif
> release_tun_meta:
> dst_release(&metadata->dst);
> @@ -401,15 +408,8 @@ static void tunnel_key_release(struct tc_action *a)
> {
> struct tcf_tunnel_key *t = to_tunnel_key(a);
> struct tcf_tunnel_key_params *params;
> -#ifdef CONFIG_DST_CACHE
> - struct ip_tunnel_info *info;
> -#endif
>
> params = rcu_dereference_protected(t->params, 1);
> -#ifdef CONFIG_DST_CACHE
> - info = ¶ms->tcft_enc_metadata->u.tun_info;
> - dst_cache_destroy(&info->dst_cache);
> -#endif
> tunnel_key_release_params(params);
> }
>
>
Reviewed-by: Roi Dayan <roid@mellanox.com>
^ permalink raw reply
* Re: AF_XDP design flaws
From: Björn Töpel @ 2019-02-27 8:08 UTC (permalink / raw)
To: John Fastabend
Cc: Maxim Mikityanskiy, netdev@vger.kernel.org, Björn Töpel,
Magnus Karlsson, David S. Miller, Tariq Toukan, Saeed Mahameed,
Eran Ben Elisha
In-Reply-To: <b48e282f-8405-8974-8b71-df15f4bda8ab@gmail.com>
On 2019-02-26 17:41, John Fastabend wrote:
> On 2/26/19 6:49 AM, Maxim Mikityanskiy wrote:
>> Hi everyone,
>>
Hi! It's exciting to see more vendors looking into AF_XDP. ARM was
involved early on, and now Mellanox. :-) It's really important that
the AF_XDP model is a good fit for all drivers/vendors! Thanks for
looking into this.
>> I would like to discuss some design flaws of AF_XDP socket (XSK) implementation
>> in kernel. At the moment I don't see a way to work around them without changing
>> the API, so I would like to make sure that I'm not missing anything and to
>> suggest and discuss some possible improvements that can be made.
>>
>> The issues I describe below are caused by the fact that the driver depends on
>> the application doing some things, and if the application is
>> slow/buggy/malicious, the driver is forced to busy poll because of the lack of a
>> notification mechanism from the application side. I will refer to the i40e
>> driver implementation a lot, as it is the first implementation of AF_XDP, but
>> the issues are general and affect any driver. I already considered trying to fix
>> it on driver level, but it doesn't seem possible, so it looks like the behavior
>> and implementation of AF_XDP in the kernel has to be changed.
>>
>> RX side busy polling
>> ====================
>>
>> On the RX side, the driver expects the application to put some descriptors in
>> the Fill Ring. There is no way for the application to notify the driver that
>> there are more Fill Ring descriptors to take, so the driver is forced to busy
>> poll the Fill Ring if it gets empty. E.g., the i40e driver does it in NAPI poll:
>>
>> int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
>> {
>> ...
>> failure = failure ||
>> !i40e_alloc_rx_buffers_fast_zc(rx_ring,
>> cleaned_count);
>> ...
>> return failure ? budget : (int)total_rx_packets;
>> }
>>
>> Basically, it means that if there are no descriptors in the Fill Ring, NAPI will
>> never stop, draining CPU.
>>
>> Possible cases when it happens
>> ------------------------------
>>
>> 1. The application is slow, it received some frames in the RX Ring, and it is
>> still handling the data, so it has no free frames to put to the Fill Ring.
>>
>> 2. The application is malicious, it opens an XSK and puts no frames to the Fill
>> Ring. It can be used as a local DoS attack.
>>
>> 3. The application is buggy and stops filling the Fill Ring for whatever reason
>> (deadlock, waiting for another blocking operation, other bugs).
>>
>> Although loading an XDP program requires root access, the DoS attack can be
>> targeted to setups that already use XDP, i.e. an XDP program is already loaded.
>> Even under root, userspace applications should not be able to disrupt system
>> stability by just calling normal APIs without an intention to destroy the
>> system, and here it happens in case 1.
> I believe this is by design. If packets per second is your top priority
> (at the expense of power, cpu, etc.) this is the behavior you might
> want. To resolve your points if you don't trust the application it
> should be isolated to a queue pair and cores so the impact is known and
> managed.
>
Correct, and I agree with John here. AF_XDP is a raw interface, and
needs to be managed.
> That said having a flag to back-off seems like a good idea. But should
> be doable as a feature without breaking current API.
>
>> Possible way to solve the issue
>> -------------------------------
>>
>> When the driver can't take new Fill Ring frames, it shouldn't busy poll.
>> Instead, it signals the failure to the application (e.g., with POLLERR), and
>> after that it's up to the application to restart polling (e.g., by calling
>> sendto()) after refilling the Fill Ring. The issue with this approach is that it
>> changes the API, so we either have to deal with it or to introduce some API
>> version field.
> See above. I like the idea here though.
>
The uapi doesn't mandate *how* the driver should behave. The rational
for the aggressive i40e fill ring polling (when the ring is empty) is
to minimize the latency. The "fill ring is empty" behavior might be
different on different drivers.
I see a number of different alternatives to the current i40e way of
busy-polling, all ranging from watchdog timers to actual HW
implementations where writing to the tail pointer of the fill ring
kicks a door bell.
That being said, I agree with previous speakers; Being able to set a
policy/driver behavior from userland is a good idea. This can be added
without breaking the existing model.
Max, any input to what this interface should look like from your
perspective?
>> TX side getting stuck
>> =====================
>>
>> On the TX side, there is the Completion Ring that the application has to clean.
>> If it doesn't, the i40e driver stops taking descriptors from the TX Ring. If the
>> application finally completes something, the driver can go on transmitting.
>> However, it would require busy polling the Completion Ring (just like with the
>> Fill Ring on the RX side). i40e doesn't do it, instead, it relies on the
>> application to kick the TX by calling sendto(). The issue is that poll() doesn't
>> return POLLOUT in this case, because the TX Ring is full, so the application
>> will never call sendto(), and the ring is stuck forever (or at least until
>> something triggers NAPI).
>>
>> Possible way to solve the issue
>> -------------------------------
>>
>> When the driver can't reserve a descriptor in the Completion Ring, it should
>> signal the failure to the application (e.g., with POLLERR). The application
>> shouldn't call sendto() every time it sees that the number of not completed
>> frames is greater than zero (like xdpsock sample does). Instead, the application
>> should kick the TX only when it wants to flush the ring, and, in addition, after
>> resolving the cause for POLLERR, i.e. after handling Completion Ring entries.
>> The API will also have to change with this approach.
>>
> +1 seems to me this can be done without breaking existing API though.
>
Keep in mind that the xdpsock application is sample/toy, and should
not be seen as the API documentation.
I don't see that the API need to be changed. AF_XDP requires that the
fill ring has entries, in order to receive data. Analogous, the
completion ring needs to managed by the application to send frames.
Are you suggesting that using poll() et al should be mandatory? Again,
take me through your send scenario, step by step.
To me, the userland application has all the knobs to determine whether
the Tx ring can be flushed or not.
>> Triggering NAPI on a different CPU core
>> =======================================
>>
>> .ndo_xsk_async_xmit runs on a random CPU core, so, to preserve CPU affinity,
>> i40e triggers an interrupt to schedule NAPI, instead of calling napi_schedule
>> directly. Scheduling NAPI on the correct CPU is what would every driver do, I
>> guess, but currently it has to be implemented differently in every driver, and
>> it relies on hardware features (the ability to trigger an IRQ).
> Ideally the application would be pinned to a core and the traffic
> steered to that core using ethtool/tc. Yes it requires a bit of mgmt on
> the platform but I think this is needed for best pps numbers.
>
>> I suggest introducing a kernel API that would allow triggering NAPI on a given
>> CPU. A brief look shows that something like smp_call_function_single_async can
>> be used. Advantages:
> Assuming you want to avoid pinning cores/traffic for some reason. Could
> this be done with some combination of cpumap + af_xdp? I haven't thought
> too much about it though. Maybe Bjorn has some ideas.
>
For the Intel drivers, the NAPI is associated to a certain
interrupt. This does not change with AF_XDP, so ndo_xsk_async_xmit
simply kicks the NAPI to run on the CPU bound defined by the irq
affinity.
I'm all open for additionally kernel APIs, and I'm happy to hack the
i40e internals as well. Again, it must be really simple to add
XDP/AF_XDP-ZC support for other vendor.
Ideally, what I would like is a generic way of associating netdev
queues (think ethtools-channels) with NAPI contexts. E.g. "queues
1,2,3 on NAPI foo, and I,II,III on NAPI bar, and the NAPIs should run
on on the baz cgroup". This is a much bigger task and outside the
AF_XDP scope. It ties in to the whole "threaded NAPIs and netdev
queues as a first class kernel object" discussion. ;-)
Magnus is working on proper busy-poll() (i.e. that the poll() syscall
executes the NAPI context). With that solution the user application
can select which core it wants to execute the poll() on -- and the
NAPI will be executed from the poll().
>> 1. It lifts the hardware requirement to be able to raise an interrupt on demand.
>>
>> 2. It would allow to move common code to the kernel (.ndo_xsk_async_xmit).
>>
>> 3. It is also useful in the situation where CPU affinity changes while being in
>> NAPI poll. Currently, i40e and mlx5e try to stop NAPI polling by returning
>> a value less than budget if CPU affinity changes. However, there are cases
>> (e.g., NAPIF_STATE_MISSED) when NAPI will be rescheduled on a wrong CPU. It's a
>> race between the interrupt, which will move NAPI to the correct CPU, and
>> __napi_schedule from a wrong CPU. Having an API to schedule NAPI on a given CPU
>> will benefit both mlx5e and i40e, because when this situation happens, it kills
>> the performance.
> How would we know what core to trigger NAPI on?
>
>> I would be happy to hear your thoughts about these issues.
> At least the first two observations seem incrementally solvable to me
> and would be nice improvements. I assume the last case can be resolved
> by pinning cores/traffic but for the general case perhaps it is useful.
>
Good summary, John. Really good input Max, and addressable -- but API
changes as far as I see it is not required.
(I'm out-of-office this week, skiing. So, I'll be away from the lists
until Monday!)
Cheers,
Björn
>> Thanks,
>> Max
>>
^ permalink raw reply
* [PATCH V2] samples: bpf: fix: broken sample regarding removed function
From: Daniel T. Lee @ 2019-02-27 7:52 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov; +Cc: netdev
Currently, running sample "task_fd_query" and "tracex3" occurs the
following error. On kernel v5.0-rc* this sample will be unavailable
due to the removal of function 'blk_start_request' at commit "a1ce35f".
(function removed, as "Single Queue IO scheduler" no longer exists)
$ sudo ./task_fd_query
failed to create kprobe 'blk_start_request' error 'No such file or
directory'
This commit will change the function 'blk_start_request' to
'blk_mq_start_request' to fix the broken sample.
Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
V2 : typo fixed
---
samples/bpf/task_fd_query_kern.c | 2 +-
samples/bpf/task_fd_query_user.c | 2 +-
samples/bpf/tracex3_kern.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/samples/bpf/task_fd_query_kern.c b/samples/bpf/task_fd_query_kern.c
index f4b0a9ea674d..5f1b2cdababd 100644
--- a/samples/bpf/task_fd_query_kern.c
+++ b/samples/bpf/task_fd_query_kern.c
@@ -4,7 +4,7 @@
#include <uapi/linux/bpf.h>
#include "bpf_helpers.h"
-SEC("kprobe/blk_start_request")
+SEC("kprobe/blk_mq_start_request")
int bpf_prog1(struct pt_regs *ctx)
{
return 0;
diff --git a/samples/bpf/task_fd_query_user.c b/samples/bpf/task_fd_query_user.c
index 8381d792f138..a6e6c76e50c9 100644
--- a/samples/bpf/task_fd_query_user.c
+++ b/samples/bpf/task_fd_query_user.c
@@ -311,7 +311,7 @@ int main(int argc, char **argv)
}
/* test two functions in the corresponding *_kern.c file */
- CHECK_AND_RET(test_debug_fs_kprobe(0, "blk_start_request",
+ CHECK_AND_RET(test_debug_fs_kprobe(0, "blk_mq_start_request",
BPF_FD_TYPE_KPROBE));
CHECK_AND_RET(test_debug_fs_kprobe(1, "blk_account_io_completion",
BPF_FD_TYPE_KRETPROBE));
diff --git a/samples/bpf/tracex3_kern.c b/samples/bpf/tracex3_kern.c
index 9974c3d7c18b..4378492a970a 100644
--- a/samples/bpf/tracex3_kern.c
+++ b/samples/bpf/tracex3_kern.c
@@ -20,7 +20,7 @@ struct bpf_map_def SEC("maps") my_map = {
/* kprobe is NOT a stable ABI. If kernel internals change this bpf+kprobe
* example will no longer be meaningful
*/
-SEC("kprobe/blk_start_request")
+SEC("kprobe/blk_mq_start_request")
int bpf_prog1(struct pt_regs *ctx)
{
long rq = PT_REGS_PARM1(ctx);
--
2.17.1
^ permalink raw reply related
* Re: [RFC v1 12/19] RDMA/irdma: Implement device supported verb APIs
From: Gal Pressman @ 2019-02-27 7:31 UTC (permalink / raw)
To: Saleem, Shiraz, dledford@redhat.com, jgg@ziepe.ca,
davem@davemloft.net
Cc: linux-rdma@vger.kernel.org, netdev@vger.kernel.org,
Ismail, Mustafa, Kirsher, Jeffrey T, Yossi Leybovich
In-Reply-To: <9DD61F30A802C4429A01CA4200E302A7A5A4FB85@fmsmsx124.amr.corp.intel.com>
On 26-Feb-19 23:09, Saleem, Shiraz wrote:
>> Subject: Re: [RFC v1 12/19] RDMA/irdma: Implement device supported verb APIs
>>
>> On 15-Feb-19 19:10, Shiraz Saleem wrote:
>>> /**
>>> * irdma_dealloc_ucontext - deallocate the user context data structure
>>> * @context: user context created during alloc */ static int
>>> irdma_dealloc_ucontext(struct ib_ucontext *context) {
>>> struct irdma_ucontext *ucontext = to_ucontext(context);
>>> unsigned long flags;
>>>
>>> spin_lock_irqsave(&ucontext->cq_reg_mem_list_lock, flags);
>>> if (!list_empty(&ucontext->cq_reg_mem_list)) {
>>> spin_unlock_irqrestore(&ucontext->cq_reg_mem_list_lock, flags);
>>> return -EBUSY;
>>> }
>>> spin_unlock_irqrestore(&ucontext->cq_reg_mem_list_lock, flags);
>>>
>>> spin_lock_irqsave(&ucontext->qp_reg_mem_list_lock, flags);
>>> if (!list_empty(&ucontext->qp_reg_mem_list)) {
>>> spin_unlock_irqrestore(&ucontext->qp_reg_mem_list_lock, flags);
>>> return -EBUSY;
>>
>> Drivers are not permitted to fail dealloc_ucontext.
>
> This is fixed in RFC v1 submission. Maybe this was pasted from the v0 ver?
>
> [..]
>
>>> +/**
>>> + * irdma_alloc_pd - allocate protection domain
>>> + * @pd: PD pointer
>>> + * @context: user context created during alloc
>>> + * @udata: user data
>>> + */
>>> +static int irdma_alloc_pd(struct ib_pd *pd,
>>> + struct ib_ucontext *context,
>>> + struct ib_udata *udata)
>>> +{
>>> + struct irdma_pd *iwpd = to_iwpd(pd);
>>> + struct irdma_device *iwdev = to_iwdev(pd->device);
>>> + struct irdma_sc_dev *dev = &iwdev->rf->sc_dev;
>>> + struct irdma_pci_f *rf = iwdev->rf;
>>> + struct irdma_alloc_pd_resp uresp = {};
>>> + struct irdma_sc_pd *sc_pd;
>>> + struct irdma_ucontext *ucontext;
>>> + u32 pd_id = 0;
>>> + int err;
>>> +
>>> + if (iwdev->closing)
>>> + return -ENODEV;
>>> +
>>> + err = irdma_alloc_rsrc(rf, rf->allocated_pds, rf->max_pd, &pd_id,
>>> + &rf->next_pd);
>>> + if (err)
>>> + return err;
>>> +
>>> + sc_pd = &iwpd->sc_pd;
>>> + if (context) {
>>
>> I think this should be 'if (udata)', this applies to many other places in this driver.
>
> That’s right. Will fix it.
>
>>
>>> + ucontext = to_ucontext(context);
>>> + dev->iw_pd_ops->pd_init(dev, sc_pd, pd_id, ucontext->abi_ver);
>>> + uresp.pd_id = pd_id;
>>> + if (ib_copy_to_udata(udata, &uresp, sizeof(uresp))) {
>>> + err = -EFAULT;
>>> + goto error;
>>> + }
>>> + } else {
>>> + dev->iw_pd_ops->pd_init(dev, sc_pd, pd_id, -1);
>>> + }
>>> +
>>> + irdma_add_pdusecount(iwpd);
>>> +
>>> + return 0;
>>> +error:
>>> + irdma_free_rsrc(rf, rf->allocated_pds, pd_id);
>>> +
>>> + return err;
>>> +}
>>> +/**
>>> + * irdma_create_qp - create qp
>>> + * @ibpd: ptr of pd
>>> + * @init_attr: attributes for qp
>>> + * @udata: user data for create qp
>>> + */
>>> +static struct ib_qp *irdma_create_qp(struct ib_pd *ibpd,
>>> + struct ib_qp_init_attr *init_attr,
>>> + struct ib_udata *udata)
>>> +{
>>> + struct irdma_pd *iwpd = to_iwpd(ibpd);
>>> + struct irdma_device *iwdev = to_iwdev(ibpd->device);
>>> + struct irdma_pci_f *rf = iwdev->rf;
>>> + struct irdma_cqp *iwcqp = &rf->cqp;
>>> + struct irdma_qp *iwqp;
>>> + struct irdma_ucontext *ucontext;
>>> + struct irdma_create_qp_req req;
>>> + struct irdma_create_qp_resp uresp = {};
>>> + struct i40iw_create_qp_resp uresp_gen1 = {};
>>> + u32 qp_num = 0;
>>> + void *mem;
>>> + enum irdma_status_code ret;
>>> + int err_code = 0;
>>> + int sq_size;
>>> + int rq_size;
>>> + struct irdma_sc_qp *qp;
>>> + struct irdma_sc_dev *dev = &rf->sc_dev;
>>> + struct irdma_qp_init_info init_info = {};
>>> + struct irdma_create_qp_info *qp_info;
>>> + struct irdma_cqp_request *cqp_request;
>>> + struct cqp_cmds_info *cqp_info;
>>> + struct irdma_qp_host_ctx_info *ctx_info;
>>> + struct irdma_iwarp_offload_info *iwarp_info;
>>> + struct irdma_roce_offload_info *roce_info;
>>> + struct irdma_udp_offload_info *udp_info;
>>> + unsigned long flags;
>>> +
>>> + if (iwdev->closing)
>>> + return ERR_PTR(-ENODEV);
>>> +
>>> + if (init_attr->create_flags)
>>> + return ERR_PTR(-EINVAL);
>>> +
>>> + if (init_attr->cap.max_inline_data > dev->hw_attrs.max_hw_inline)
>>> + init_attr->cap.max_inline_data = dev->hw_attrs.max_hw_inline;
>>> +
>>> + if (init_attr->cap.max_send_sge > dev->hw_attrs.max_hw_wq_frags)
>>> + init_attr->cap.max_send_sge = dev-
>>> hw_attrs.max_hw_wq_frags;
>>> +
>>> + if (init_attr->cap.max_recv_sge > dev->hw_attrs.max_hw_wq_frags)
>>> + init_attr->cap.max_recv_sge = dev->hw_attrs.max_hw_wq_frags;
>>
>> AFAIK, you can change the requested values to be greater than or equal to the
>> values requested. I don't think you can change them to something smaller.
>
> Hmm...This is a sanity check to make sure we don’t exceed the device supported values.
> But we should fail the call.
>
> [..]
>
>>> + mem = kzalloc(sizeof(*iwqp), GFP_KERNEL);
>>> + if (!mem)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + iwqp = (struct irdma_qp *)mem;
>>> + iwqp->allocated_buf = mem;
>>
>> 'allocated_buf' feels redundant. Why is iwqp not sufficient?
>
> I agree.
> [..]
>
>>> + if (udata) {
>>> + err_code = ib_copy_from_udata(&req, udata, sizeof(req));
>>
>> Perhaps ib_copy_from_udata(&req, udata, min(sizeof(req), udata->inlen)?
>> Applies to other call sites of ib_copy_from/to_udata as well.
>>
>
> It’s a good idea.
>
>>> + * irdma_query - query qp attributes
>>> + * @ibqp: qp pointer
>>> + * @attr: attributes pointer
>>> + * @attr_mask: Not used
>>> + * @init_attr: qp attributes to return */ static int
>>> +irdma_query_qp(struct ib_qp *ibqp,
>>> + struct ib_qp_attr *attr,
>>> + int attr_mask,
>>> + struct ib_qp_init_attr *init_attr) {
>>> + struct irdma_qp *iwqp = to_iwqp(ibqp);
>>> + struct irdma_sc_qp *qp = &iwqp->sc_qp;
>>> +
>>> + attr->qp_state = iwqp->ibqp_state;
>>> + attr->cur_qp_state = iwqp->ibqp_state;
>>> + attr->qp_access_flags = 0;
>>> + attr->cap.max_send_wr = qp->qp_uk.sq_size - 1;
>>> + attr->cap.max_recv_wr = qp->qp_uk.rq_size - 1;
>>
>> Why -1?
>
> It's reserved for HW. But the equation should be
> (sqdepth - I40IW_SQ_RSVD) >> sqshift.
>
> [....]
>>
>>> + attr->cap.max_inline_data = qp->qp_uk.max_inline_data;
>>> + attr->cap.max_send_sge = qp->qp_uk.max_sq_frag_cnt;
>>> + attr->cap.max_recv_sge = qp->qp_uk.max_rq_frag_cnt;
>>> + attr->qkey = iwqp->roce_info.qkey;
>>> +
>>> + init_attr->event_handler = iwqp->ibqp.event_handler;
>>> + init_attr->qp_context = iwqp->ibqp.qp_context;
>>> + init_attr->send_cq = iwqp->ibqp.send_cq;
>>> + init_attr->recv_cq = iwqp->ibqp.recv_cq;
>>> + init_attr->srq = iwqp->ibqp.srq;
>>> + init_attr->cap = attr->cap;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * irdma_destroy_cq - destroy cq
>>> + * @ib_cq: cq pointer
>>> + */
>>> +static int irdma_destroy_cq(struct ib_cq *ib_cq) {
>>> + struct irdma_cq *iwcq;
>>> + struct irdma_device *iwdev;
>>> + struct irdma_sc_cq *cq;
>>> +
>>> + if (!ib_cq) {
>>> + irdma_pr_err("ib_cq == NULL\n");
>>> + return 0;
>>> + }
>>
>> Is this really needed? Which caller can pass NULL pointer?
>
> Not needed.
>
>>> +
>>> +/**
>>> + * board_id_show
>>> + */
>>> +static ssize_t board_id_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + return sprintf(buf, "%.*s\n", 32, "IRDMA Board ID");
>>
>> That doesn't add much information.
>
> Will fix.
>
>>
>>> +}
>>> +
>>> +static DEVICE_ATTR_RO(hw_rev);
>>> +static DEVICE_ATTR_RO(hca_type);
>>> +static DEVICE_ATTR_RO(board_id);
>>> +
>>> +static struct attribute *irdma_dev_attributes[] = {
>>> + &dev_attr_hw_rev.attr,
>>> + &dev_attr_hca_type.attr,
>>> + &dev_attr_board_id.attr,
>>> + NULL
>>> +};
>>> +
>>> +static const struct attribute_group irdma_attr_group = {
>>> + .attrs = irdma_dev_attributes,
>>> +};
>>> +
>>> +/**
>>> + * irdma_modify_port Modify port properties
>>> + * @ibdev: device pointer from stack
>>> + * @port: port number
>>> + * @port_modify_mask: mask for port modifications
>>> + * @props: port properties
>>> + */
>>> +static int irdma_modify_port(struct ib_device *ibdev,
>>> + u8 port,
>>> + int port_modify_mask,
>>> + struct ib_port_modify *props) {
>>> + return 0;
>>> +}
>>
>> Same question as disacossiate_ucontext.
>
> This was likely added during early dev. and can be removed.
>
>>
>>> +
>>> +/**
>>> + * irdma_query_gid_roce - Query port GID for Roce
>>> + * @ibdev: device pointer from stack
>>> + * @port: port number
>>> + * @index: Entry index
>>> + * @gid: Global ID
>>> + */
>>> +static int irdma_query_gid_roce(struct ib_device *ibdev,
>>> + u8 port,
>>> + int index,
>>> + union ib_gid *gid)
>>> +{
>>> + int ret;
>>> +
>>> + ret = rdma_query_gid(ibdev, port, index, gid);
>>> + if (ret == -EAGAIN) {
>>
>> I can't see a path where rdma_query_gid returns -EAGAIN.
>
> This function can be removed now. It's only applicable to non-Roce providers.
>
>>
>>> + memcpy(gid, &zgid, sizeof(*gid));
>>> + return 0;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>
>>> +/**
>>> + * irdma_create_ah - create address handle
>>> + * @ibpd: ptr to protection domain
>>> + * @ah_attr: address handle attributes
>>
>> 'ah_attr' -> 'attr', missing flags and udata.
>
> Will fix all these hits in the driver.
>
> [..]
>>> + */
>>> +static int irdma_destroy_ah(struct ib_ah *ibah, u32 flags) {
>>> + struct irdma_device *iwdev = to_iwdev(ibah->device);
>>> + struct irdma_ah *ah = to_iwah(ibah);
>>> + int err;
>>> +
>>> + if (!ah->sc_ah.ah_info.ah_valid)
>>> + return -EINVAL;
>>> +
>>> + err = irdma_ah_cqp_op(iwdev->rf, &ah->sc_ah,
>> IRDMA_OP_AH_DESTROY,
>>> + flags & RDMA_DESTROY_AH_SLEEPABLE,
>>> + irdma_destroy_ah_cb, ah);
>>> + if (!err)
>>> + return 0;
>>
>> Why are the rest of the cleanups only in case of error?
>
> On success, the cleanup is done in the callback, irdma_destroy_ah_cb.
>
> [...]
>
>
>>> +static __be64 irdma_mac_to_guid(struct net_device *ndev) {
>>> + unsigned char *mac = ndev->dev_addr;
>>> + __be64 guid;
>>> + unsigned char *dst = (unsigned char *)&guid;
>>> +
>>> + dst[0] = mac[0] ^ 2;
>>> + dst[1] = mac[1];
>>> + dst[2] = mac[2];
>>> + dst[3] = 0xff;
>>> + dst[4] = 0xfe;
>>> + dst[5] = mac[3];
>>> + dst[6] = mac[4];
>>> + dst[7] = mac[5];
>>> +
>>> + return guid;
>>> +}
>>
>> There's a variant of this function in irdma, bnxt_re, ocrdma and qedr.
>> Maybe it's time to provide it in common code?
>
> Agreed.
>
Other than that:
Reviewed-by: Gal Pressman <galpress@amazon.com>
^ permalink raw reply
* Re: [RFC v1 11/19] RDMA/irdma: Add PBLE resource manager
From: Leon Romanovsky @ 2019-02-27 6:58 UTC (permalink / raw)
To: Shiraz Saleem
Cc: dledford, jgg, davem, linux-rdma, netdev, mustafa.ismail,
jeffrey.t.kirsher
In-Reply-To: <20190215171107.6464-12-shiraz.saleem@intel.com>
[-- Attachment #1: Type: text/plain, Size: 5284 bytes --]
On Fri, Feb 15, 2019 at 11:10:58AM -0600, Shiraz Saleem wrote:
> From: Mustafa Ismail <mustafa.ismail@intel.com>
>
> Implement a Physical Buffer List Entry (PBLE) resource manager
> to manage a pool of PBLE HMC resource objects.
>
> Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com>
> Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
> ---
> drivers/infiniband/hw/irdma/pble.c | 520 +++++++++++++++++++++++++++++++++++++
> drivers/infiniband/hw/irdma/pble.h | 135 ++++++++++
> 2 files changed, 655 insertions(+)
> create mode 100644 drivers/infiniband/hw/irdma/pble.c
> create mode 100644 drivers/infiniband/hw/irdma/pble.h
>
> diff --git a/drivers/infiniband/hw/irdma/pble.c b/drivers/infiniband/hw/irdma/pble.c
> new file mode 100644
> index 0000000..66fab69
> --- /dev/null
> +++ b/drivers/infiniband/hw/irdma/pble.c
> @@ -0,0 +1,520 @@
> +// SPDX-License-Identifier: GPL-2.0 or Linux-OpenIB
> +/* Copyright (c) 2019, Intel Corporation. */
> +
> +#include "osdep.h"
> +#include "status.h"
> +#include "hmc.h"
> +#include "defs.h"
> +#include "type.h"
> +#include "protos.h"
> +#include "pble.h"
> +
> +static enum irdma_status_code add_pble_prm(struct irdma_hmc_pble_rsrc *pble_rsrc);
> +
> +/**
> + * irdma_destroy_pble_prm - destroy prm during module unload
> + * @pble_rsrc: pble resources
> + */
> +void irdma_destroy_pble_prm(struct irdma_hmc_pble_rsrc *pble_rsrc)
> +{
> + struct irdma_sc_dev *dev = pble_rsrc->dev;
> + struct irdma_chunk *chunk;
> + struct irdma_pble_prm *pinfo = &pble_rsrc->pinfo;
> +
> + while (!list_empty(&pinfo->clist)) {
> + chunk = (struct irdma_chunk *)pinfo->clist.next;
> + list_del(&chunk->list);
> + if (chunk->type == PBLE_SD_PAGED)
> + irdma_pble_free_paged_mem(chunk);
> + if (chunk->bitmapbuf)
> + irdma_free_virt_mem(dev->hw, &chunk->bitmapmem);
> + irdma_free_virt_mem(dev->hw, &chunk->chunkmem);
> + }
> +}
> +
> +/**
> + * irdma_hmc_init_pble - Initialize pble resources during module load
> + * @dev: irdma_sc_dev struct
> + * @pble_rsrc: pble resources
> + */
> +enum irdma_status_code
> +irdma_hmc_init_pble(struct irdma_sc_dev *dev,
> + struct irdma_hmc_pble_rsrc *pble_rsrc)
> +{
> + struct irdma_hmc_info *hmc_info;
> + u32 fpm_idx = 0;
> + enum irdma_status_code status = 0;
> +
> + hmc_info = dev->hmc_info;
> + pble_rsrc->dev = dev;
> + pble_rsrc->fpm_base_addr = hmc_info->hmc_obj[IRDMA_HMC_IW_PBLE].base;
> + /* Start pble' on 4k boundary */
> + if (pble_rsrc->fpm_base_addr & 0xfff)
> + fpm_idx = (PAGE_SIZE - (pble_rsrc->fpm_base_addr & 0xfff)) >> 3;
> + pble_rsrc->unallocated_pble =
> + hmc_info->hmc_obj[IRDMA_HMC_IW_PBLE].cnt - fpm_idx;
> + pble_rsrc->next_fpm_addr = pble_rsrc->fpm_base_addr + (fpm_idx << 3);
> + pble_rsrc->pinfo.pble_shift = PBLE_SHIFT;
> +
> + spin_lock_init(&pble_rsrc->pinfo.prm_lock);
> + INIT_LIST_HEAD(&pble_rsrc->pinfo.clist);
> + if (add_pble_prm(pble_rsrc)) {
> + irdma_destroy_pble_prm(pble_rsrc);
> + status = IRDMA_ERR_NO_MEMORY;
> + }
> +
> + return status;
> +}
> +
> +/**
> + * get_sd_pd_idx - Returns sd index, pd index and rel_pd_idx from fpm address
> + * @ pble_rsrc: structure containing fpm address
> + * @ idx: where to return indexes
> + */
> +static void get_sd_pd_idx(struct irdma_hmc_pble_rsrc *pble_rsrc,
> + struct sd_pd_idx *idx)
> +{
> + idx->sd_idx = (u32)(pble_rsrc->next_fpm_addr) /
> + IRDMA_HMC_DIRECT_BP_SIZE;
> + idx->pd_idx = (u32)(pble_rsrc->next_fpm_addr) / IRDMA_HMC_PAGED_BP_SIZE;
> + idx->rel_pd_idx = (idx->pd_idx % IRDMA_HMC_PD_CNT_IN_SD);
The amount of type-casting in this driver is astonishing. It will be
better to declare all types to be aligned from the beginning.
> +}
> +
> +/**
> + * add_sd_direct - add sd direct for pble
> + * @pble_rsrc: pble resource ptr
> + * @info: page info for sd
> + */
> +static enum irdma_status_code
> +add_sd_direct(struct irdma_hmc_pble_rsrc *pble_rsrc,
> + struct irdma_add_page_info *info)
> +{
> + struct irdma_sc_dev *dev = pble_rsrc->dev;
> + enum irdma_status_code ret_code = 0;
> + struct sd_pd_idx *idx = &info->idx;
> + struct irdma_chunk *chunk = info->chunk;
> + struct irdma_hmc_info *hmc_info = info->hmc_info;
> + struct irdma_hmc_sd_entry *sd_entry = info->sd_entry;
> + u32 offset = 0;
> +
> + if (!sd_entry->valid) {
> + ret_code = irdma_add_sd_table_entry(dev->hw, hmc_info,
> + info->idx.sd_idx,
> + IRDMA_SD_TYPE_DIRECT,
> + IRDMA_HMC_DIRECT_BP_SIZE);
> + if (ret_code)
> + return ret_code;
> +
> + chunk->type = PBLE_SD_CONTIGOUS;
> + }
> +
> + offset = idx->rel_pd_idx << HMC_PAGED_BP_SHIFT;
> + chunk->size = info->pages << HMC_PAGED_BP_SHIFT;
> + chunk->vaddr = (u64)((u8 *)sd_entry->u.bp.addr.va + offset);
> + chunk->fpm_addr = pble_rsrc->next_fpm_addr;
> + irdma_debug(dev, IRDMA_DEBUG_PBLE,
> + "chunk_size[%lld] = 0x%llx vaddr=0x%llx fpm_addr = %llx\n",
> + chunk->size, chunk->size, chunk->vaddr, chunk->fpm_addr);
> +
> + return 0;
> +}
> +
> +/**
> + * fpm_to_idx - given fpm address, get pble index
> + * @pble_rsrc: pble resource management
> + * @addr: fpm address for index
> + */
> +static u32 fpm_to_idx(struct irdma_hmc_pble_rsrc *pble_rsrc, u64 addr)
> +{
> + u64 idx;
> +
> + idx = (addr - (pble_rsrc->fpm_base_addr)) >> 3;
> +
> + return (u32)idx;
lower_32_bits()
Thanks
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH] net: dsa: read mac address from DT for slave device
From: xiaofeis @ 2019-02-27 6:57 UTC (permalink / raw)
To: Florian Fainelli
Cc: Vinod Koul, David S. Miller, linux-arm-msm, Bjorn Andersson,
Andrew Lunn, Vivien Didelot, Niklas Cassel, netdev
In-Reply-To: <9962d7a13bb25c88cc42b3f4dd68cee7@codeaurora.org>
On 2019-02-27 10:04, xiaofeis@codeaurora.org wrote:
> On 2019-02-26 15:45, xiaofeis@codeaurora.org wrote:
>> On 2019-02-26 01:27, Florian Fainelli wrote:
>>> On 2/25/19 5:28 AM, xiaofeis@codeaurora.org wrote:
>>>> Hi Florian
>>>>
>>>> We have two slave DSA interfaces, wan0 and lan0, one is for wan
>>>> port,
>>>> and the other is for lan port. Customer has it's mac address pool,
>>>> they
>>>> want
>>>> to assign the mac address from the pool on wan0, lan0, and other
>>>> interfaces like
>>>> wifi, bt. Coreboot/uboot will populate it to the DTS node, so the
>>>> driver
>>>> can
>>>> get it from it's node. For DSA slave interface, it already has it's
>>>> own
>>>> DTS node, it's
>>>> easy to just add one porperty "local-mac-address" there for the
>>>> usage in
>>>> DSA driver.
>>>>
>>>> If not use DSA framework, normally we will use eth0.x and eth0.y for
>>>> wan
>>>> and lan.
>>>> On this case, customer usually also assign the MAC address on these
>>>> logical interface
>>>> from it's pool.
>>>
>>> OK, but this is not necessary per my previous explanation: the CPU
>>> <=>
>>> WAN pipe is a separate broadcast domain (otherwise it is a security
>>> hole
>>> since you exposing LAN machines to the outside world), and so there
>>> is
>>> no need for a separate MAC address. It might be convenient to have
>>> one,
>>> especially for the provider, if they run a management software (e.g.:
>>> TR69), but it is not required per-se.
>>>
>>> Let me ask a secondary question here, how many Ethernet MACs connect
>>> to
>>> the switch in this configuration? Is there one that is supposed to be
>>> assigned all LAN traffic and one that is supposed to be assigned all
>>> WAN
>>> traffic? If so, then what you are doing makes even less
>>>
>>
>> Only one MAC connected to switch cpu port, both lan0 and wan0 are on
>> the top of
>> same interface(eth0).
>>
> Customer doesn't care about the MAC controller's MAC address, just
> leave it as the driver
> randomly generated. They just want to assign the MAC address on wan
> and lan DSA logical
> interface.
>
> Many customer doesn't use DSA, for example, they use eth0.1/eth0.2 for
> lan/wan with one MAC controller.
> They configure switch wan port in vlan2 group, and lan port in vlan1
> group, they usually assign mac address
> on the logical interface(eth0.1ð0.2), i think this is similar with
> DSA slave interfaces.
>
Unlike vlan logical interface, dsa port already has it's dts node, so
it's
easy and natural to add the mac address property for DSA driver use.
On DSA configuration, we won't assign ip on mater eth0. The mac DA from
wan port will be
the wan0's mac address, the mac DA from lan port will be the lan0's mac
address, they are
separate. I think the this is in general on some customer's deploy.
>>>>
>>>> On 2019-02-22 23:43, Florian Fainelli wrote:
>>>>> On 2/22/19 4:58 AM, Vinod Koul wrote:
>>>>>> From: Xiaofei Shen <xiaofeis@codeaurora.org>
>>>>>>
>>>>>> Before creating a slave netdevice, get the mac address from DTS
>>>>>> and
>>>>>> apply in case it is valid.
>>>>>
>>>>> Can you explain your use case in details?
>>>>>
>>>>> Assigning a MAC address to a network device that represents a
>>>>> switch
>>>>> port does not quite make sense in general. The switch port is
>>>>> really
>>>>> representing one end of a pipe, so one side you have stations and
>>>>> on the
>>>>> other side, you have the CPU/management Ethernet MAC controller's
>>>>> MAC
>>>>> address which constitutes a station as well. The DSA slave network
>>>>> devices are just software constructs meant to steer traffic towards
>>>>> specific ports of the switch, but they are all from the perpsective
>>>>> of
>>>>> traffic reaching the CPU Port in the first place, therefore traffic
>>>>> that
>>>>> is generally a known unicast Ethernet frame with the CPU's MAC
>>>>> address
>>>>> as MAC DA (and of course all types of unknown MC, management
>>>>> traffic
>>>>> etc.)
>>>>>
>>>>> By default, DSA switch need to come up in a configuration where all
>>>>> ports (except CPU/management) must be strictly separate from every
>>>>> other
>>>>> port such that we can achieve what a standalone Ethernet NIC would
>>>>> do.
>>>>> This works because all ports are isolated from one another, so
>>>>> there is
>>>>> no cross talk and so having the same MAC address (the one from the
>>>>> CPU)
>>>>> on the DSA slave network devices just works, each port is a
>>>>> separate
>>>>> broadcast domain.
>>>>>
>>>>> Once you start bridging one or ore ports, the bridge root port will
>>>>> have
>>>>> a MAC address, most likely the one the CPU/management Ethernet MAC,
>>>>> but
>>>>> similarly, this is not an issue and that's exactly how a software
>>>>> bridge
>>>>> would work as well.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Xiaofei Shen <xiaofeis@codeaurora.org>
>>>>>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
>>>>>> ---
>>>>>> include/net/dsa.h | 1 +
>>>>>> net/dsa/dsa2.c | 1 +
>>>>>> net/dsa/slave.c | 5 ++++-
>>>>>> 3 files changed, 6 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>>>>>> index b3eefe8e18fd..aa24ce756679 100644
>>>>>> --- a/include/net/dsa.h
>>>>>> +++ b/include/net/dsa.h
>>>>>> @@ -198,6 +198,7 @@ struct dsa_port {
>>>>>> unsigned int index;
>>>>>> const char *name;
>>>>>> const struct dsa_port *cpu_dp;
>>>>>> + const char *mac;
>>>>>> struct device_node *dn;
>>>>>> unsigned int ageing_time;
>>>>>> u8 stp_state;
>>>>>> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
>>>>>> index a1917025e155..afb7d9fa42f6 100644
>>>>>> --- a/net/dsa/dsa2.c
>>>>>> +++ b/net/dsa/dsa2.c
>>>>>> @@ -261,6 +261,7 @@ static int dsa_port_setup(struct dsa_port *dp)
>>>>>> int err = 0;
>>>>>>
>>>>>> memset(&dp->devlink_port, 0, sizeof(dp->devlink_port));
>>>>>> + dp->mac = of_get_mac_address(dp->dn);
>>>>>>
>>>>>> if (dp->type != DSA_PORT_TYPE_UNUSED)
>>>>>> err = devlink_port_register(ds->devlink,
>>>>>> &dp->devlink_port,
>>>>>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>>>>>> index a3fcc1d01615..8e64c4e947c6 100644
>>>>>> --- a/net/dsa/slave.c
>>>>>> +++ b/net/dsa/slave.c
>>>>>> @@ -1308,7 +1308,10 @@ int dsa_slave_create(struct dsa_port *port)
>>>>>> slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
>>>>>> slave_dev->hw_features |= NETIF_F_HW_TC;
>>>>>> slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
>>>>>> - eth_hw_addr_inherit(slave_dev, master);
>>>>>> + if (port->mac && is_valid_ether_addr(port->mac))
>>>>>> + ether_addr_copy(slave_dev->dev_addr, port->mac);
>>>>>> + else
>>>>>> + eth_hw_addr_inherit(slave_dev, master);
>>>>>> slave_dev->priv_flags |= IFF_NO_QUEUE;
>>>>>> slave_dev->netdev_ops = &dsa_slave_netdev_ops;
>>>>>> slave_dev->switchdev_ops = &dsa_slave_switchdev_ops;
>>>>>>
^ permalink raw reply
* Re: [PATCH internal pre-review] bpf: add missing entries to bpf_helpers.h
From: Martin Lau @ 2019-02-27 6:52 UTC (permalink / raw)
To: Willem de Bruijn
Cc: netdev@vger.kernel.org, sdf@google.com, soheil@google.com,
Willem de Bruijn
In-Reply-To: <20190226225553.214360-1-willemdebruijn.kernel@gmail.com>
On Tue, Feb 26, 2019 at 05:55:53PM -0500, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
> tools/testing/selftests/bpf/bpf_helpers.h | 29 +++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> index d9999f1ed1d2a..fcd11839903c7 100644
> --- a/tools/testing/selftests/bpf/bpf_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> @@ -232,6 +232,35 @@ static int (*bpf_skb_change_head)(void *, int len, int flags) =
> (void *) BPF_FUNC_skb_change_head;
> static int (*bpf_skb_pull_data)(void *, int len) =
> (void *) BPF_FUNC_skb_pull_data;
> +static unsigned long long (*bpf_get_cgroup_classid)(void *ctx) =
> + (void *) BPF_FUNC_get_cgroup_classid;
> +static unsigned long long (*bpf_get_route_realm)(void *ctx) =
> + (void *) BPF_FUNC_get_route_realm;
> +static int (*bpf_skb_change_proto)(void *ctx, __be16 proto, __u64 flags) =
> + (void *) BPF_FUNC_skb_change_proto;
> +static int (*bpf_skb_change_type)(void *ctx, __u32 type) =
> + (void *) BPF_FUNC_skb_change_type;
> +static unsigned long long (*bpf_get_hash_recalc)(void *ctx) =
> + (void *) BPF_FUNC_get_hash_recalc;
> +static unsigned long long (*bpf_get_current_task)(void *ctx) =
> + (void *) BPF_FUNC_get_current_task;
> +static int (*bpf_skb_change_tail)(void *ctx, __u32 len, __u64 flags) =
> + (void *) BPF_FUNC_skb_change_tail;
> +static long long (*bpf_csum_update)(void *ctx, __u32 csum) =
> + (void *) BPF_FUNC_csum_update;
> +static void (*bpf_set_hash_invalid)(void *ctx) =
> + (void *) BPF_FUNC_set_hash_invalid;
> +static int (*bpf_get_numa_node_id)(void) =
> + (void *) BPF_FUNC_get_numa_node_id;
> +static int (*bpf_probe_read_str)(void *ctx, int size, const void *unsafe_ptr) =
> + (void *) BPF_FUNC_probe_read_str;
> +static unsigned int (*bpf_get_socket_uid)(void *ctx) =
> + (void *) BPF_FUNC_get_socket_uid;
> +static unsigned int (*bpf_set_hash)(void *ctx, __u32 hash) =
> + (void *) BPF_FUNC_set_hash;
> +static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
> + unsigned long long flags) =
> + (void *) BPF_FUNC_skb_adjust_room;
>
> /* Scan the ARCH passed in from ARCH env variable (see Makefile) */
> #if defined(__TARGET_ARCH_x86)
> --
> 2.21.0.rc2.261.ga7da99ff1b-goog
>
LGTM. Please respin with a commit message.
Acked-by: Martin KaFai Lau <kafai@fb.com>
^ permalink raw reply
* Re: [RFC v1 13/19] RDMA/irdma: Add RoCEv2 UD OP support
From: Leon Romanovsky @ 2019-02-27 6:50 UTC (permalink / raw)
To: Shiraz Saleem
Cc: dledford, jgg, davem, linux-rdma, netdev, mustafa.ismail,
jeffrey.t.kirsher
In-Reply-To: <20190215171107.6464-14-shiraz.saleem@intel.com>
[-- Attachment #1: Type: text/plain, Size: 526 bytes --]
On Fri, Feb 15, 2019 at 11:11:00AM -0600, Shiraz Saleem wrote:
> From: Mustafa Ismail <mustafa.ismail@intel.com>
>
> Add the header, data structures and functions
> to populate the WQE descriptors and issue the
> Control QP commands that support RoCEv2 UD operations.
>
> Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com>
> Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
> ---
<...>
> +#define IRDMA_UDA_QPSQ_IMMDATA_M \
> + ((u64)0xffffffffffffffffULL << IRDMA_UDA_QPSQ_IMMDATA_S)
Both u64 and ULL?
Thanks
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 3/3] net: dsa: microchip: add other KSZ9477 switch variants
From: kbuild test robot @ 2019-02-27 6:47 UTC (permalink / raw)
To: Tristram.Ha
Cc: kbuild-all, Andrew Lunn, Florian Fainelli, Pavel Machek,
Tristram Ha, UNGLinuxDriver, netdev
In-Reply-To: <1551224265-9304-4-git-send-email-Tristram.Ha@microchip.com>
[-- Attachment #1: Type: text/plain, Size: 4368 bytes --]
Hi Tristram,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Tristram-Ha-microchip-com/net-dsa-microchip-add-KSZ9893-switch-support/20190227-141333
config: i386-randconfig-x009-201908 (attached as .config)
compiler: gcc-8 (Debian 8.2.0-20) 8.2.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/net/dsa/microchip/ksz9477.c: In function 'ksz9477_switch_detect':
>> drivers/net/dsa/microchip/ksz9477.c:1516:8: error: implicit declaration of function 'of_modalias_node'; did you mean 'of_match_node'? [-Werror=implicit-function-declaration]
if (!of_modalias_node(dev->dev->of_node, name, sizeof(name))) {
^~~~~~~~~~~~~~~~
of_match_node
cc1: some warnings being treated as errors
vim +1516 drivers/net/dsa/microchip/ksz9477.c
1433
1434 static int ksz9477_switch_detect(struct ksz_device *dev)
1435 {
1436 int chip = -1;
1437 u8 data8;
1438 u8 id_hi;
1439 u8 id_lo;
1440 u32 id32;
1441 int ret;
1442
1443 /* turn off SPI DO Edge select */
1444 ret = ksz_read8(dev, REG_SW_GLOBAL_SERIAL_CTRL_0, &data8);
1445 if (ret)
1446 return ret;
1447
1448 data8 &= ~SPI_AUTO_EDGE_DETECTION;
1449 ret = ksz_write8(dev, REG_SW_GLOBAL_SERIAL_CTRL_0, data8);
1450 if (ret)
1451 return ret;
1452
1453 /* read chip id */
1454 ret = ksz_read32(dev, REG_CHIP_ID0__1, &id32);
1455 if (ret)
1456 return ret;
1457 ret = ksz_read8(dev, REG_GLOBAL_OPTIONS, &data8);
1458 if (ret)
1459 return ret;
1460
1461 /* Number of ports can be reduced depending on chip. */
1462 dev->mib_port_cnt = TOTAL_PORT_NUM;
1463 dev->phy_port_cnt = 5;
1464
1465 /* Default capability is gigabit capable. */
1466 dev->features = GBIT_SUPPORT;
1467
1468 id_hi = (u8)(id32 >> 16);
1469 id_lo = (u8)(id32 >> 8);
1470 if ((id_lo & 0xf) == 3) {
1471 /* Chip is from KSZ9893 design. */
1472 dev->features |= IS_9893;
1473
1474 /* Chip does not support gigabit. */
1475 if (data8 & SW_QW_ABLE)
1476 dev->features &= ~GBIT_SUPPORT;
1477 dev->mib_port_cnt = 3;
1478 dev->phy_port_cnt = 2;
1479 if (!(data8 & SW_AVB_ABLE))
1480 chip = KSZ9893_SW_CHIP;
1481 else if (data8 & SW_QW_ABLE)
1482 chip = KSZ8563_SW_CHIP;
1483 else
1484 chip = KSZ9563_SW_CHIP;
1485 } else {
1486 /* Chip uses new XMII register definitions. */
1487 dev->features |= NEW_XMII;
1488
1489 /* Chip does not support gigabit. */
1490 if (!(data8 & SW_GIGABIT_ABLE))
1491 dev->features &= ~GBIT_SUPPORT;
1492 if ((id_lo & 0xf) == 6)
1493 dev->mib_port_cnt = 6;
1494 if (id_hi == FAMILY_ID_94)
1495 chip = KSZ9477_SW_CHIP;
1496 else if (id_hi == FAMILY_ID_98 && id_lo == CHIP_ID_97)
1497 chip = KSZ9897_SW_CHIP;
1498 else if (id_hi == FAMILY_ID_98 && id_lo == CHIP_ID_96)
1499 chip = KSZ9896_SW_CHIP;
1500 else if (id_hi == FAMILY_ID_95 && id_lo == CHIP_ID_67)
1501 chip = KSZ9567_SW_CHIP;
1502 else if (id_hi == FAMILY_ID_85 && id_lo == CHIP_ID_67)
1503 chip = KSZ8567_SW_CHIP;
1504 if (id_lo == CHIP_ID_67) {
1505 id_hi = FAMILY_ID_98;
1506 id_lo = CHIP_ID_97;
1507 } else if (id_lo == CHIP_ID_66) {
1508 id_hi = FAMILY_ID_98;
1509 id_lo = CHIP_ID_96;
1510 }
1511 }
1512 if (dev->dev->of_node) {
1513 char name[80];
1514
1515 /* KSZ8565 chip can only be set through device tree. */
> 1516 if (!of_modalias_node(dev->dev->of_node, name, sizeof(name))) {
1517 if (!strcmp(name, "ksz8565")) {
1518 chip = KSZ8565_SW_CHIP;
1519 id_hi = FAMILY_ID_98;
1520 id_lo = 0x95;
1521 }
1522 }
1523 }
1524
1525 /* Change chip id to known ones so it can be matched against them. */
1526 id32 = (id_hi << 16) | (id_lo << 8);
1527
1528 dev->chip_id = id32;
1529
1530 /* Update switch device name to matched chip. */
1531 if (chip >= 0)
1532 dev->name = ksz9477_chip_names[chip];
1533
1534 return 0;
1535 }
1536
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31591 bytes --]
^ permalink raw reply
* [PATCH iproute2-next] rdma: Add the prefix for driver attributes
From: Leon Romanovsky @ 2019-02-27 6:41 UTC (permalink / raw)
To: David Ahern, Lijun Ou, Steve Wise
Cc: Leon Romanovsky, netdev, RDMA mailing list, Stephen Hemminger
From: Leon Romanovsky <leonro@mellanox.com>
There is a need to distinguish between driver vs. general exposed
attributes. The most common use case is to expose some internal
garbage under extremely common and sexy name, e.g. pi, ci e.t.c
In order to achieve that, we will add "drv_" prefix to all strings
which were received through RDMA_NLDEV_ATTR_DRIVER_* attributes.
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>a
---
rdma/utils.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/rdma/utils.c b/rdma/utils.c
index 6bc14cd5..1f6bf330 100644
--- a/rdma/utils.c
+++ b/rdma/utils.c
@@ -829,27 +829,37 @@ static int print_driver_entry(struct rd *rd, struct nlattr *key_attr,
struct nlattr *val_attr,
enum rdma_nldev_print_type print_type)
{
- const char *key_str = mnl_attr_get_str(key_attr);
int attr_type = nla_type(val_attr);
+ int ret = -EINVAL;
+ char *key_str;
+
+ if (asprintf(&key_str, "drv_%s", mnl_attr_get_str(key_attr)) == -1)
+ return -ENOMEM;
switch (attr_type) {
case RDMA_NLDEV_ATTR_DRIVER_STRING:
- return print_driver_string(rd, key_str,
- mnl_attr_get_str(val_attr));
+ ret = print_driver_string(rd, key_str,
+ mnl_attr_get_str(val_attr));
+ break;
case RDMA_NLDEV_ATTR_DRIVER_S32:
- return print_driver_s32(rd, key_str,
- mnl_attr_get_u32(val_attr), print_type);
+ ret = print_driver_s32(rd, key_str, mnl_attr_get_u32(val_attr),
+ print_type);
+ break;
case RDMA_NLDEV_ATTR_DRIVER_U32:
- return print_driver_u32(rd, key_str,
- mnl_attr_get_u32(val_attr), print_type);
+ ret = print_driver_u32(rd, key_str, mnl_attr_get_u32(val_attr),
+ print_type);
+ break;
case RDMA_NLDEV_ATTR_DRIVER_S64:
- return print_driver_s64(rd, key_str,
- mnl_attr_get_u64(val_attr), print_type);
+ ret = print_driver_s64(rd, key_str, mnl_attr_get_u64(val_attr),
+ print_type);
+ break;
case RDMA_NLDEV_ATTR_DRIVER_U64:
- return print_driver_u64(rd, key_str,
- mnl_attr_get_u64(val_attr), print_type);
+ ret = print_driver_u64(rd, key_str, mnl_attr_get_u64(val_attr),
+ print_type);
+ break;
}
- return -EINVAL;
+ free(key_str);
+ return ret;
}
void print_driver_table(struct rd *rd, struct nlattr *tb)
--
2.19.1
^ permalink raw reply related
* Re: [PATCH] samples: bpf: fix: broken sample regarding removed function
From: Martin Lau @ 2019-02-27 6:40 UTC (permalink / raw)
To: Daniel T. Lee; +Cc: Daniel Borkmann, Alexei Starovoitov, netdev@vger.kernel.org
In-Reply-To: <20190227030559.19672-1-danieltimlee@gmail.com>
On Tue, Feb 26, 2019 at 10:05:59PM -0500, Daniel T. Lee wrote:
> Currently, running sample "task_fd_query" and "tracex3" occurs the
> following error. On kernel v5.0-rc* this sample will be unavailable
> due to the removal of function 'blk_start_request' at commit "a1ce35f".
> (function removed, as "Single Queue IO scheduler" no longer exists)
>
> $ sudo ./task_fd_query
> failed to create kprobe 'blk_start_request' error 'No such file or
> directory'
>
> This commit will change the function 'blk_start_request' to
> 'blk_mq_start_request' to fix the broken sample.
>
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> ---
> samples/bpf/task_fd_query_kern.c | 2 +-
> samples/bpf/task_fd_query_user.c | 2 +-
> samples/bpf/tracex3_kern.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/samples/bpf/task_fd_query_kern.c b/samples/bpf/task_fd_query_kern.c
> index f4b0a9ea674d..5f1b2cdababd 100644
> --- a/samples/bpf/task_fd_query_kern.c
> +++ b/samples/bpf/task_fd_query_kern.c
> @@ -4,7 +4,7 @@
> #include <uapi/linux/bpf.h>
> #include "bpf_helpers.h"
>
> -SEC("kprobe/blk_start_request")
> +SEC("kprobe/blk_mq_start_requset")
I suspect it does not run. Have you tested it?
There is a typo:
blk_mq_start_requSEt
blk_mq_start_requESt
^ permalink raw reply
* Re: [PATCH bpf-next 1/3] bpf: add bpf_progenyof helper
From: Martin Lau @ 2019-02-27 6:26 UTC (permalink / raw)
To: Javier Honduvilla Coto; +Cc: netdev@vger.kernel.org, Yonghong Song, Kernel Team
In-Reply-To: <20190226223651.3166820-2-javierhonduco@fb.com>
On Tue, Feb 26, 2019 at 02:36:49PM -0800, Javier Honduvilla Coto wrote:
> This patch adds the bpf_progenyof helper which receives a PID and returns
What is progenof?
> 1 if the process currently being executed is in the process hierarchy
> including itself or 0 if not.
>
> This is very useful in tracing programs when we want to filter by a
> given PID and all the children it might spawn. The current workarounds
> most people implement for this purpose have issues:
>
> - Attaching to process spawning syscalls and dynamically add those PIDs
> to some bpf map that would be used to filter is cumbersome and
> potentially racy.
> - Unrolling some loop to perform what this helper is doing consumes lots
> of instructions. That and the impossibility to jump backwards makes it
> really hard to be correct in really large process chains.
>
> Signed-off-by: Javier Honduvilla Coto <javierhonduco@fb.com>
> ---
> include/linux/bpf.h | 1 +
> include/uapi/linux/bpf.h | 3 ++-
> kernel/bpf/core.c | 1 +
> kernel/bpf/helpers.c | 29 +++++++++++++++++++++++++++++
> kernel/trace/bpf_trace.c | 2 ++
> 5 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index de18227b3d95..447395ba202b 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -921,6 +921,7 @@ extern const struct bpf_func_proto bpf_sk_redirect_map_proto;
> extern const struct bpf_func_proto bpf_spin_lock_proto;
> extern const struct bpf_func_proto bpf_spin_unlock_proto;
> extern const struct bpf_func_proto bpf_get_local_storage_proto;
> +extern const struct bpf_func_proto bpf_progenyof_proto;
It seems only used in bpf_trace.c. Does it have to be here?
>
> /* Shared helpers among cBPF and eBPF. */
> void bpf_user_rnd_init_once(void);
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index bcdd2474eee7..804e4218eb28 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2457,7 +2457,8 @@ union bpf_attr {
> FN(spin_lock), \
> FN(spin_unlock), \
> FN(sk_fullsock), \
> - FN(tcp_sock),
> + FN(tcp_sock), \
> + FN(progenyof),
Please add doc like other helpers do.
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> * function eBPF program intends to call
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index ef88b167959d..69e209fbd128 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -2015,6 +2015,7 @@ const struct bpf_func_proto bpf_get_current_uid_gid_proto __weak;
> const struct bpf_func_proto bpf_get_current_comm_proto __weak;
> const struct bpf_func_proto bpf_get_current_cgroup_id_proto __weak;
> const struct bpf_func_proto bpf_get_local_storage_proto __weak;
> +const struct bpf_func_proto bpf_progenyof_proto __weak;
>
> const struct bpf_func_proto * __weak bpf_get_trace_printk_proto(void)
> {
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index a411fc17d265..3899787e8dbf 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -18,6 +18,7 @@
> #include <linux/sched.h>
> #include <linux/uidgid.h>
> #include <linux/filter.h>
> +#include <linux/init_task.h>
>
> /* If kernel subsystem is allowing eBPF programs to call this function,
> * inside its own verifier_ops->get_func_proto() callback it should return
> @@ -364,3 +365,31 @@ const struct bpf_func_proto bpf_get_local_storage_proto = {
> };
> #endif
> #endif
> +
> +BPF_CALL_1(bpf_progenyof, int, pid)
> +{
> + int result = 0;
> + struct task_struct *task = current;
> +
> + if (unlikely(!task))
hmm.... Could current be NULL?
> + return -EINVAL;
> +
> + rcu_read_lock();
> + while (task != &init_task) {
I don't know the details of init_task, so qq:
Could the passed in "pid" be the init_task->pid?
If possible, what is the expected "result"?
> + if (task->pid == pid) {
> + result = 1;
> + break;
> + }
> + task = rcu_dereference(task->real_parent);
> + }
> + rcu_read_unlock();
> +
> + return result;
> +}
> +
> +const struct bpf_func_proto bpf_progenyof_proto = {
> + .func = bpf_progenyof,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_ANYTHING,
> +};
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index f1a86a0d881d..8602ae83c799 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -600,6 +600,8 @@ tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_get_prandom_u32_proto;
> case BPF_FUNC_probe_read_str:
> return &bpf_probe_read_str_proto;
> + case BPF_FUNC_progenyof:
> + return &bpf_progenyof_proto;
> #ifdef CONFIG_CGROUPS
> case BPF_FUNC_get_current_cgroup_id:
> return &bpf_get_current_cgroup_id_proto;
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH 3/8] tlan: use pci_zalloc instead of pci_alloc
From: Joe Perches @ 2019-02-27 6:22 UTC (permalink / raw)
To: Robert Eshleman
Cc: Tariq Toukan, David S. Miller, Samuel Chessman, netdev,
linux-rdma, linux-kernel
In-Reply-To: <51125a3daac49f7b5bb360a422663aee26e6776b.1551246708.git.bobbyeshleman@gmail.com>
On Tue, 2019-02-26 at 22:09 -0800, Robert Eshleman wrote:
> This patch replaces a pci_alloc_consistent and memset(,0) call
> with a single call to pci_zalloc_consistent.
[]
> diff --git a/drivers/net/ethernet/ti/tlan.c b/drivers/net/ethernet/ti/tlan.c
[]
> @@ -845,17 +845,16 @@ static int tlan_init(struct net_device *dev)
>
> dma_size = (TLAN_NUM_RX_LISTS + TLAN_NUM_TX_LISTS)
> * (sizeof(struct tlan_list));
> - priv->dma_storage = pci_alloc_consistent(priv->pci_dev,
> - dma_size,
> - &priv->dma_storage_dma);
> + priv->dma_storage = pci_zalloc_consistent(priv->pci_dev,
> + dma_size,
> + &priv->dma_storage_dma);
> priv->dma_size = dma_size;
>
> - if (priv->dma_storage == NULL) {
> + if (!priv->dma_storage) {
> pr_err("Could not allocate lists and buffers for %s\n",
> dev->name);
unrelated trivia:
This pr_err (and likely others in this file)
could be replace by netdev_err
^ permalink raw reply
* [PATCH 2/8] net/mlxsw: use pci_zalloc_consistent instead of pci_alloc_consistent
From: Robert Eshleman @ 2019-02-27 6:09 UTC (permalink / raw)
To: bobbyeshleman
Cc: Tariq Toukan, David S. Miller, Jiri Pirko, Ido Schimmel, netdev,
linux-rdma, linux-kernel
In-Reply-To: <cover.1551246708.git.bobbyeshleman@gmail.com>
This patch replaces a call to pci_alloc_consistent and then
memset(0,) with a single call to pci_zalloc_consistent.
Signed-off-by: Robert Eshleman <bobbyeshleman@gmail.com>
---
drivers/net/ethernet/mellanox/mlxsw/pci.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/pci.c b/drivers/net/ethernet/mellanox/mlxsw/pci.c
index a2321fe8d6a0..388f349573f3 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/pci.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/pci.c
@@ -830,12 +830,11 @@ static int mlxsw_pci_queue_init(struct mlxsw_pci *mlxsw_pci, char *mbox,
tasklet_init(&q->tasklet, q_ops->tasklet, (unsigned long) q);
mem_item->size = MLXSW_PCI_AQ_SIZE;
- mem_item->buf = pci_alloc_consistent(mlxsw_pci->pdev,
- mem_item->size,
- &mem_item->mapaddr);
+ mem_item->buf = pci_zalloc_consistent(mlxsw_pci->pdev,
+ mem_item->size,
+ &mem_item->mapaddr);
if (!mem_item->buf)
return -ENOMEM;
- memset(mem_item->buf, 0, mem_item->size);
q->elem_info = kcalloc(q->count, sizeof(*q->elem_info), GFP_KERNEL);
if (!q->elem_info) {
--
2.20.1
^ permalink raw reply related
* [PATCH 3/8] tlan: use pci_zalloc instead of pci_alloc
From: Robert Eshleman @ 2019-02-27 6:09 UTC (permalink / raw)
To: bobbyeshleman
Cc: Tariq Toukan, David S. Miller, Samuel Chessman, netdev,
linux-rdma, linux-kernel
In-Reply-To: <cover.1551246708.git.bobbyeshleman@gmail.com>
This patch replaces a pci_alloc_consistent and memset(,0) call
with a single call to pci_zalloc_consistent.
Signed-off-by: Robert Eshleman <bobbyeshleman@gmail.com>
---
drivers/net/ethernet/ti/tlan.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/ti/tlan.c b/drivers/net/ethernet/ti/tlan.c
index b4ab1a5f6cd0..e1f7e71c3b21 100644
--- a/drivers/net/ethernet/ti/tlan.c
+++ b/drivers/net/ethernet/ti/tlan.c
@@ -845,17 +845,16 @@ static int tlan_init(struct net_device *dev)
dma_size = (TLAN_NUM_RX_LISTS + TLAN_NUM_TX_LISTS)
* (sizeof(struct tlan_list));
- priv->dma_storage = pci_alloc_consistent(priv->pci_dev,
- dma_size,
- &priv->dma_storage_dma);
+ priv->dma_storage = pci_zalloc_consistent(priv->pci_dev,
+ dma_size,
+ &priv->dma_storage_dma);
priv->dma_size = dma_size;
- if (priv->dma_storage == NULL) {
+ if (!priv->dma_storage) {
pr_err("Could not allocate lists and buffers for %s\n",
dev->name);
return -ENOMEM;
}
- memset(priv->dma_storage, 0, dma_size);
priv->rx_list = (struct tlan_list *)
ALIGN((unsigned long)priv->dma_storage, 8);
priv->rx_list_dma = ALIGN(priv->dma_storage_dma, 8);
--
2.20.1
^ permalink raw reply related
* [PATCH 5/8] at12: use pci_zalloc instead of pci_alloc
From: Robert Eshleman @ 2019-02-27 6:09 UTC (permalink / raw)
To: bobbyeshleman
Cc: Jay Cliburn, Chris Snook, David S. Miller, Tariq Toukan, netdev,
linux-kernel, linux-rdma
In-Reply-To: <cover.1551246708.git.bobbyeshleman@gmail.com>
This patch replaces a pci_alloc and memset(,0) call
with a single call to pci_zalloc.
Signed-off-by: Robert Eshleman <bobbyeshleman@gmail.com>
---
drivers/net/ethernet/atheros/atlx/atl2.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/atheros/atlx/atl2.c b/drivers/net/ethernet/atheros/atlx/atl2.c
index bb41becb6609..a145c2d1b1d2 100644
--- a/drivers/net/ethernet/atheros/atlx/atl2.c
+++ b/drivers/net/ethernet/atheros/atlx/atl2.c
@@ -300,11 +300,10 @@ static s32 atl2_setup_ring_resources(struct atl2_adapter *adapter)
adapter->txs_ring_size * 4 + 7 + /* dword align */
adapter->rxd_ring_size * 1536 + 127; /* 128bytes align */
- adapter->ring_vir_addr = pci_alloc_consistent(pdev, size,
- &adapter->ring_dma);
+ adapter->ring_vir_addr = pci_zalloc_consistent(pdev, size,
+ &adapter->ring_dma);
if (!adapter->ring_vir_addr)
return -ENOMEM;
- memset(adapter->ring_vir_addr, 0, adapter->ring_size);
/* Init TXD Ring */
adapter->txd_dma = adapter->ring_dma ;
--
2.20.1
^ permalink raw reply related
* [PATCH 6/8] netxen: remove unnecessary memset(,0) calls
From: Robert Eshleman @ 2019-02-27 6:09 UTC (permalink / raw)
To: bobbyeshleman
Cc: Tariq Toukan, David S. Miller, Manish Chopra, Rahul Verma,
GR-Linux-NIC-Dev, netdev, linux-rdma, linux-kernel
In-Reply-To: <cover.1551246708.git.bobbyeshleman@gmail.com>
This patch reduces calls to memset(,0) by replacing memset(,0)
calls that only zero-out newly declared structs with simply
assigning those structs to zero structs.
It also removes a pci_alloc_consistent call followed by a memset(,0)
call by simply using pci_zalloc_consistent.
Signed-off-by: Robert Eshleman <bobbyeshleman@gmail.com>
---
.../ethernet/qlogic/netxen/netxen_nic_ctx.c | 36 +++++++------------
1 file changed, 13 insertions(+), 23 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_ctx.c b/drivers/net/ethernet/qlogic/netxen/netxen_nic_ctx.c
index 7503aa222392..f2010c032361 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_ctx.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_ctx.c
@@ -99,8 +99,7 @@ netxen_issue_cmd(struct netxen_adapter *adapter, struct netxen_cmd_args *cmd)
static int
netxen_get_minidump_template_size(struct netxen_adapter *adapter)
{
- struct netxen_cmd_args cmd;
- memset(&cmd, 0, sizeof(cmd));
+ struct netxen_cmd_args cmd = {0};
cmd.req.cmd = NX_CDRP_CMD_TEMP_SIZE;
memset(&cmd.rsp, 1, sizeof(struct _cdrp_cmd));
netxen_issue_cmd(adapter, &cmd);
@@ -120,7 +119,7 @@ netxen_get_minidump_template(struct netxen_adapter *adapter)
dma_addr_t md_template_addr;
void *addr;
u32 size;
- struct netxen_cmd_args cmd;
+ struct netxen_cmd_args cmd = {0};
size = adapter->mdump.md_template_size;
if (size == 0) {
@@ -135,7 +134,6 @@ netxen_get_minidump_template(struct netxen_adapter *adapter)
return -ENOMEM;
}
- memset(&cmd, 0, sizeof(cmd));
memset(&cmd.rsp, 1, sizeof(struct _cdrp_cmd));
cmd.req.cmd = NX_CDRP_CMD_GET_TEMP_HDR;
cmd.req.arg1 = LSD(md_template_addr);
@@ -233,9 +231,8 @@ nx_fw_cmd_set_mtu(struct netxen_adapter *adapter, int mtu)
{
u32 rcode = NX_RCODE_SUCCESS;
struct netxen_recv_context *recv_ctx = &adapter->recv_ctx;
- struct netxen_cmd_args cmd;
+ struct netxen_cmd_args cmd = {0};
- memset(&cmd, 0, sizeof(cmd));
cmd.req.cmd = NX_CDRP_CMD_SET_MTU;
cmd.req.arg1 = recv_ctx->context_id;
cmd.req.arg2 = mtu;
@@ -254,9 +251,8 @@ int
nx_fw_cmd_set_gbe_port(struct netxen_adapter *adapter,
u32 speed, u32 duplex, u32 autoneg)
{
- struct netxen_cmd_args cmd;
+ struct netxen_cmd_args cmd = {0};
- memset(&cmd, 0, sizeof(cmd));
cmd.req.cmd = NX_CDRP_CMD_CONFIG_GBE_PORT;
cmd.req.arg1 = speed;
cmd.req.arg2 = duplex;
@@ -276,7 +272,7 @@ nx_fw_cmd_create_rx_ctx(struct netxen_adapter *adapter)
nx_cardrsp_sds_ring_t *prsp_sds;
struct nx_host_rds_ring *rds_ring;
struct nx_host_sds_ring *sds_ring;
- struct netxen_cmd_args cmd;
+ struct netxen_cmd_args cmd = {0};
dma_addr_t hostrq_phys_addr, cardrsp_phys_addr;
u64 phys_addr;
@@ -359,7 +355,6 @@ nx_fw_cmd_create_rx_ctx(struct netxen_adapter *adapter)
}
phys_addr = hostrq_phys_addr;
- memset(&cmd, 0, sizeof(cmd));
cmd.req.arg1 = (u32)(phys_addr >> 32);
cmd.req.arg2 = (u32)(phys_addr & 0xffffffff);
cmd.req.arg3 = rq_size;
@@ -413,9 +408,8 @@ static void
nx_fw_cmd_destroy_rx_ctx(struct netxen_adapter *adapter)
{
struct netxen_recv_context *recv_ctx = &adapter->recv_ctx;
- struct netxen_cmd_args cmd;
+ struct netxen_cmd_args cmd = {0};
- memset(&cmd, 0, sizeof(cmd));
cmd.req.arg1 = recv_ctx->context_id;
cmd.req.arg2 = NX_DESTROY_CTX_RESET;
cmd.req.arg3 = 0;
@@ -520,9 +514,8 @@ nx_fw_cmd_create_tx_ctx(struct netxen_adapter *adapter)
static void
nx_fw_cmd_destroy_tx_ctx(struct netxen_adapter *adapter)
{
- struct netxen_cmd_args cmd;
+ struct netxen_cmd_args cmd = {0};
- memset(&cmd, 0, sizeof(cmd));
cmd.req.arg1 = adapter->tx_context_id;
cmd.req.arg2 = NX_DESTROY_CTX_RESET;
cmd.req.arg3 = 0;
@@ -538,9 +531,8 @@ int
nx_fw_cmd_query_phy(struct netxen_adapter *adapter, u32 reg, u32 *val)
{
u32 rcode;
- struct netxen_cmd_args cmd;
+ struct netxen_cmd_args cmd = {0};
- memset(&cmd, 0, sizeof(cmd));
cmd.req.arg1 = reg;
cmd.req.arg2 = 0;
cmd.req.arg3 = 0;
@@ -561,9 +553,8 @@ int
nx_fw_cmd_set_phy(struct netxen_adapter *adapter, u32 reg, u32 val)
{
u32 rcode;
- struct netxen_cmd_args cmd;
+ struct netxen_cmd_args cmd = {0};
- memset(&cmd, 0, sizeof(cmd));
cmd.req.arg1 = reg;
cmd.req.arg2 = val;
cmd.req.arg3 = 0;
@@ -763,15 +754,14 @@ int netxen_alloc_hw_resources(struct netxen_adapter *adapter)
recv_ctx = &adapter->recv_ctx;
tx_ring = adapter->tx_ring;
- addr = pci_alloc_consistent(pdev,
- sizeof(struct netxen_ring_ctx) + sizeof(uint32_t),
- &recv_ctx->phys_addr);
- if (addr == NULL) {
+ addr = pci_zalloc_consistent(pdev,
+ sizeof(struct netxen_ring_ctx) + sizeof(uint32_t),
+ &recv_ctx->phys_addr);
+ if (!addr) {
dev_err(&pdev->dev, "failed to allocate hw context\n");
return -ENOMEM;
}
- memset(addr, 0, sizeof(struct netxen_ring_ctx));
recv_ctx->hwctx = addr;
recv_ctx->hwctx->ctx_id = cpu_to_le32(port);
recv_ctx->hwctx->cmd_consumer_offset =
--
2.20.1
^ permalink raw reply related
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