* Re: [PATCH net] geneve: use ipv6_mod_enabled to check if ipv6 is disabled
From: David Ahern @ 2019-02-25 16:57 UTC (permalink / raw)
To: Hangbin Liu, netdev; +Cc: Stefano Brivio, David S . Miller
In-Reply-To: <20190225041350.9913-1-liuhangbin@gmail.com>
On 2/24/19 9:13 PM, Hangbin Liu wrote:
> ipv6_mod_enabled() is more safe and gentle to check if ipv6 is disabled
> at running time.
>
> Fixes: c0a47e44c098 ("geneve: should not call rt6_lookup() when ipv6 was disabled")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> drivers/net/geneve.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 3377ac66a347..42c5e43ece68 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -1514,7 +1514,7 @@ static void geneve_link_config(struct net_device *dev,
> case AF_INET6: {
> struct rt6_info *rt;
>
> - if (!__in6_dev_get(dev))
> + if (!ipv6_mod_enabled())
> break;
>
> rt = rt6_lookup(geneve->net, &info->key.u.ipv6.dst, NULL, 0,
>
seems to me the existing check is more appropriate and relevant for the
code path: is ipv6 enabled on this device versus is ipv6 enabled at all.
^ permalink raw reply
* Re: [PATCH net] sit: use ipv6_mod_enabled to check if ipv6 is disabled
From: David Ahern @ 2019-02-25 16:57 UTC (permalink / raw)
To: Eric Dumazet, Hangbin Liu; +Cc: netdev, Stefano Brivio, David S . Miller
In-Reply-To: <1dd57d9d-fed2-67b8-ac28-7ef3681eeed2@gmail.com>
On 2/25/19 9:39 AM, Eric Dumazet wrote:
>
>
> On 02/25/2019 12:17 AM, Hangbin Liu wrote:
>> On Sun, Feb 24, 2019 at 08:24:51PM -0800, Eric Dumazet wrote:
>>>
>>>
>>> On 02/24/2019 08:12 PM, Hangbin Liu wrote:
>>>> ipv6_mod_enabled() is more safe and gentle to check if ipv6 is disabled
>>>> at running time.
>>>>
>>>
>>>
>>> Why is it better exactly ?
>>>
>>> IPv6 can be enabled on the host, but disabled per device
>>>
>>> /proc/sys/net/ipv6/conf/{name}/disable_ipv6
>>
>> Sorry, it looks I didn't make it clear in the commit description.
>> This issue only occurs when IPv6 is disabled at boot time as there is
>> no IPv6 route entry. Disable ipv6 on specific interface is not affected.
>> So check ipv6_mod_enabled() is enough and we don't need to worry about
>> the rcu_read_lock or the dev status.
>>
>> Should I update the commit description?
>
> Certainly. Are you telling us skb->dev could be NULL here ?
>
> Because rcu_read_lock() should already be asserted.
>
Same response as geneve. The existing check is more appropriate and
relevant for the code path: is ipv6 enabled on this device versus is
ipv6 enabled at all.
^ permalink raw reply
* Re: [PATCH 1/3] net: ethernet: add support for PCS and 2.5G speed
From: Florian Fainelli @ 2019-02-25 17:21 UTC (permalink / raw)
To: Parshuram Raju Thombare, nicolas.ferre@microchip.com,
davem@davemloft.net, netdev@vger.kernel.org, andrew@lunn.ch,
hkallweit1@gmail.com, linux-kernel@vger.kernel.org,
Rafal Ciepiela, Piotr Sroka, Jan Kotas
In-Reply-To: <CO2PR07MB2469FCDFEE81BE4D3ECE9C92C17A0@CO2PR07MB2469.namprd07.prod.outlook.com>
On 2/25/19 1:11 AM, Parshuram Raju Thombare wrote:
>> Le 2/22/19 à 12:12 PM, Parshuram Thombare a écrit :
>>> This patch add support for PCS (for SGMII interface) and 2.5Gbps MAC
>>> in Cadence ethernet controller driver.
>>
>> At a high level you don't seem to be making use of PHYLINK so which 2.5Gbps
>> interfaces do you actually support?
>>
>
> New ethernet controller have MAC which support 2.5G speed.
> Also there is addition of PCS and SGMII interface.
I should have asked this more clearly: have you tested with SFP modules
for instance? If you want to be able to reliably support 2500baseT
and/or 2500baseX with hot plugging of such modules, you need to
implement PHYLINK for that network driver, there is no other way around.
>
>>>
>>> Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
>>> ---
>>
>> [snip]
>>
>>> @@ -361,26 +361,50 @@ static int macb_mdio_write(struct mii_bus *bus, int
>> mii_id, int regnum,
>>> * macb_set_tx_clk() - Set a clock to a new frequency
>>> * @clk Pointer to the clock to change
>>> * @rate New frequency in Hz
>>> + * @interafce Phy interface
>>
>> Typo: @interface and this is an unrelated change.
>>
>>> * @dev Pointer to the struct net_device
>>> */
>>> -static void macb_set_tx_clk(struct clk *clk, int speed, struct
>>> net_device *dev)
>>> +static void macb_set_tx_clk(struct clk *clk, int speed,
>>> + phy_interface_t interface, struct net_device *dev)
>>> {
>>> long ferr, rate, rate_rounded;
>>>
>>> if (!clk)
>>> return;
>>>
>>> - switch (speed) {
>>> - case SPEED_10:
>>> + if (interface == PHY_INTERFACE_MODE_GMII ||
>>> + interface == PHY_INTERFACE_MODE_MII) {
>>> + switch (speed) {
>>> + case SPEED_10:> rate = 2500000;
>>
>> You need to add one tab to align rate and break.
>
> Do you mean a tab each for rate and break lines ?
> All switch statements are aligned at a tab. I am not sure how does case and rate got on same line.
It should look like this:
switch (cond) {
case cond1:
do_something();
break;
etc.
>
>>
>>> break;
>>> - case SPEED_100:
>>> + case SPEED_100:
>>> rate = 25000000;
>>> break;
>>> - case SPEED_1000:
>>> + case SPEED_1000:
>>> rate = 125000000;
>>> break;
>>> - default:
>>> + default:
>>> + return;
>>> + }
>>> + } else if (interface == PHY_INTERFACE_MODE_SGMII) {
>>> + switch (speed) {
>>> + case SPEED_10:
>>> + rate = 1250000;
>>> + break;
>>> + case SPEED_100:
>>> + rate = 12500000;
>>> + break;
>>> + case SPEED_1000:
>>> + rate = 125000000;
>>> + break;
>>> + case SPEED_2500:
>>> + rate = 312500000;
>>> + break;
>>> + default:
>>> + return;
>>
>> The indentation is broken here and you can greatly simplify this with a simple
>> function that returns speed * 1250 and does an initial check for unsupported
>> speeds.
>>
>
> I ran checkpatch.pl and all indentation issues were cleared. But I think having function
> is better option, I will make that change.
>
>>> + }
>>> + } else {
>>> return;
>>> }
>>>
>>> @@ -410,30 +434,49 @@ static void macb_handle_link_change(struct
>>> net_device *dev)
>>>
>>> spin_lock_irqsave(&bp->lock, flags);
>>>
>>> - if (phydev->link) {
>>> - if ((bp->speed != phydev->speed) ||
>>> - (bp->duplex != phydev->duplex)) {
>>> - u32 reg;
>>> -
>>> - reg = macb_readl(bp, NCFGR);
>>> - reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
>>> - if (macb_is_gem(bp))
>>> - reg &= ~GEM_BIT(GBE);
>>> + if (phydev->link && (bp->speed != phydev->speed ||
>>> + bp->duplex != phydev->duplex)) {
>>> + u32 reg;
>>>
>>> - if (phydev->duplex)
>>> - reg |= MACB_BIT(FD);
>>> + reg = macb_readl(bp, NCFGR);
>>> + reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
>>> + if (macb_is_gem(bp))
>>> + reg &= ~GEM_BIT(GBE);
>>> + if (phydev->duplex)
>>> + reg |= MACB_BIT(FD);
>>> + macb_or_gem_writel(bp, NCFGR, reg);
>>> +
>>> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII &&
>>> + (phydev->speed == SPEED_1000 ||
>>> + phydev->speed == SPEED_2500)) {
>>> + if (bp->caps & MACB_CAPS_TWO_PT_FIVE_GIG_SPEED) {
>>> + reg = gem_readl(bp, NCR) &
>>> + ~GEM_BIT(TWO_PT_FIVE_GIG);
>>> + gem_writel(bp, NCR, reg);
>>> + }
>>
>> If you are making correct use of the capabilities then there is no point in re-
>> checking them here. If you allowed the MAC to advertise 2.5Gbps then it is de-
>> facto SGMII capable.
>
> PHY_INTERFACE_MODE_SGMII is selected only on the basis of presence of PCS.
> This additional check is to make sure PHY also support 1G/2.5G.
My point is that you should never get to that function with
bp->phy_interface == PHY_INTERFACE_MODE_SGMII if you have done necessary
verifications at the time you connect to the PHY. If you let
PHY_INTERFACE_MODE_SGMII go through on a Cadence IP version that does
not have the MACB_CAPS_TWO_PT_FIVE_GIG_SPEED capability bit set, then
this is broken.
>
>>> + gem_writel(bp, NCFGR, GEM_BIT(GBE) |
>>> + gem_readl(bp, NCFGR));
>>> + if (bp->caps & MACB_CAPS_TWO_PT_FIVE_GIG_SPEED
>> &&
>>> + phydev->speed == SPEED_2500)
>>> + gem_writel(bp, NCR, gem_readl(bp, NCR) |
>>> + GEM_BIT(TWO_PT_FIVE_GIG));
>>> + } else if (phydev->speed == SPEED_1000) {
>>> + gem_writel(bp, NCFGR, GEM_BIT(GBE) |
>>> + gem_readl(bp, NCFGR));
>>> + } else {
>>> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
>> {
>>> + reg = gem_readl(bp, NCFGR);
>>> + reg &= ~(GEM_BIT(SGMIIEN) |
>> GEM_BIT(PCSSEL));
>>> + gem_writel(bp, NCFGR, reg);
>>> + }
>>> if (phydev->speed == SPEED_100)
>>> - reg |= MACB_BIT(SPD);
>>> - if (phydev->speed == SPEED_1000 &&
>>> - bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
>>> - reg |= GEM_BIT(GBE);
>>> -
>>> - macb_or_gem_writel(bp, NCFGR, reg);
>>> -
>>> - bp->speed = phydev->speed;
>>> - bp->duplex = phydev->duplex;
>>> - status_change = 1;
>>> + macb_writel(bp, NCFGR, MACB_BIT(SPD) |
>>> + macb_readl(bp, NCFGR));
>>> }
>>
>> There is a lot of repetition while setting the GBE bit which always set based on
>> speed == 1000 irrespective of the mode, so take that part out of the if () else if ()
>> else () clauses.
>>
>
> Ok, I will change it.
>
>>> +
>>> + bp->speed = phydev->speed;
>>> + bp->duplex = phydev->duplex;
>>> + status_change = 1;
>>> }
>>>
>>> if (phydev->link != bp->link) {
>>> @@ -453,7 +496,8 @@ static void macb_handle_link_change(struct net_device
>> *dev)
>>> /* Update the TX clock rate if and only if the link is
>>> * up and there has been a link change.
>>> */
>>> - macb_set_tx_clk(bp->tx_clk, phydev->speed, dev);
>>> + macb_set_tx_clk(bp->tx_clk, phydev->speed,
>>> + bp->phy_interface, dev);
>>>
>>> netif_carrier_on(dev);
>>> netdev_info(dev, "link up (%d/%s)\n", @@ -543,10
>> +587,16 @@ static
>>> int macb_mii_probe(struct net_device *dev)
>>> }
>>>
>>> /* mask with MAC supported features */
>>> - if (macb_is_gem(bp) && bp->caps &
>> MACB_CAPS_GIGABIT_MODE_AVAILABLE)
>>> - phy_set_max_speed(phydev, SPEED_1000);
>>> - else
>>> - phy_set_max_speed(phydev, SPEED_100);
>>> + if (macb_is_gem(bp)) {
>>
>> You have changed the previous logic that also checked for
>> MACB_CAPS_GIGABIT_MODE_AVAILABLE, why?
>
> My understanding is all GEM (ID >= 0x2) support GIGABIT mode.
> Was there any other reason for this check ?
Well, if anyone would know, it would be you, I don't work for Cadence
nor have access to the internal IP documentation.
>
>>> + linkmode_copy(phydev->supported, PHY_GBIT_FEATURES);
>>> + if (bp->caps & MACB_CAPS_TWO_PT_FIVE_GIG_SPEED)
>>> +
>> linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>>> + phydev->supported);
>>> + } else {
>>> + linkmode_copy(phydev->supported, PHY_BASIC_FEATURES);
>>> + }
>>> +
>>> + linkmode_copy(phydev->advertising, phydev->supported);
>>>
>>> if (bp->caps & MACB_CAPS_NO_GIGABIT_HALF)
>>> phy_remove_link_mode(phydev,
>>> @@ -2217,8 +2267,6 @@ static void macb_init_hw(struct macb *bp)
>>> macb_set_hwaddr(bp);
>>>
>>> config = macb_mdc_clk_div(bp);
>>> - if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
>>> - config |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
>>> config |= MACB_BF(RBOF, NET_IP_ALIGN); /* Make eth data
>> aligned */
>>> config |= MACB_BIT(PAE); /* PAuse Enable */
>>> config |= MACB_BIT(DRFCS); /* Discard Rx FCS */
>>> @@ -3255,6 +3303,23 @@ static void macb_configure_caps(struct macb *bp,
>>> dcfg = gem_readl(bp, DCFG1);
>>> if (GEM_BFEXT(IRQCOR, dcfg) == 0)
>>> bp->caps |= MACB_CAPS_ISR_CLEAR_ON_WRITE;
>>> + if (GEM_BFEXT(NO_PCS, dcfg) == 0)
>>> + bp->caps |= MACB_CAPS_PCS;
>>> + switch (MACB_BFEXT(IDNUM, macb_readl(bp, MID))) {
>>> + case MACB_GEM7016_IDNUM:
>>> + case MACB_GEM7017_IDNUM:
>>> + case MACB_GEM7017A_IDNUM:
>>> + case MACB_GEM7020_IDNUM:
>>> + case MACB_GEM7021_IDNUM:
>>> + case MACB_GEM7021A_IDNUM:
>>> + case MACB_GEM7022_IDNUM:
>>> + if (bp->caps & MACB_CAPS_PCS)
>>> + bp->caps |= MACB_CAPS_TWO_PT_FIVE_GIG_SPEED;
>>> + break;
>>> +
>>> + default:
>>> + break;
>>> + }
>>> dcfg = gem_readl(bp, DCFG2);
>>> if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF)))
>> == 0)
>>> bp->caps |= MACB_CAPS_FIFO_MODE;
>>> @@ -4110,7 +4175,28 @@ static int macb_probe(struct platform_device
>> *pdev)
>>> else
>>> bp->phy_interface = PHY_INTERFACE_MODE_MII;
>>> } else {
>>> + switch (err) {
>>> + case PHY_INTERFACE_MODE_SGMII:
>>> + if (bp->caps & MACB_CAPS_PCS) {
>>> + bp->phy_interface = PHY_INTERFACE_MODE_SGMII;
>>> + break;
>>> + }
>>
>> If SGMII was selected on a version of the IP that does not support it, then falling
>> back to GMII or MII does not sound correct, this is a hard error that must be
>> handled as such.
>> --
>> Florian
>
> My intention was to continue (instead of failing) with whatever functionality is available.
> Can we have some error message and continue with what is available ?
This is probably not going to help anyone, imagine you incorrectly
specified a 'phy-mode' property in the Device Tree and you have to dig
into the driver in the function that sets the clock rate to find out
what you just defaulted to 1Gbits/GMII for instance. This is not user
friendly at all and will increase the support burden on your end as
well, if SGMII is specified and the IP does not support it, detect it
and return an error, how that propagates, either as a probe failure or
the inability to open the network device, your call.
--
Florian
^ permalink raw reply
* Re: [PATCH] net: dsa: read mac address from DT for slave device
From: Florian Fainelli @ 2019-02-25 17:27 UTC (permalink / raw)
To: xiaofeis
Cc: Vinod Koul, David S. Miller, linux-arm-msm, Bjorn Andersson,
Andrew Lunn, Vivien Didelot, Niklas Cassel, netdev
In-Reply-To: <fc02f99d3a84ace86f6c2cb6411b4b33@codeaurora.org>
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
>
> 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;
>>>
--
Florian
^ permalink raw reply
* Re: [PATCH v2] net: dsa: fix a leaked reference by adding missing of_node_put
From: David Miller @ 2019-02-25 17:35 UTC (permalink / raw)
To: wen.yang99
Cc: andrew, vivien.didelot, f.fainelli, netdev, linux-kernel,
wang.yi59
In-Reply-To: <1551079339-30270-1-git-send-email-wen.yang99@zte.com.cn>
From: Wen Yang <wen.yang99@zte.com.cn>
Date: Mon, 25 Feb 2019 15:22:19 +0800
> The call to of_parse_phandle returns a node pointer with refcount
> incremented thus it must be explicitly decremented after the last
> usage.
>
> Detected by coccinelle with the following warnings:
> ./net/dsa/port.c:294:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 284, but without a corresponding object release within this function.
> ./net/dsa/dsa2.c:627:3-9: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 618, but without a corresponding object release within this function.
> ./net/dsa/dsa2.c:630:3-9: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 618, but without a corresponding object release within this function.
> ./net/dsa/dsa2.c:636:3-9: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 618, but without a corresponding object release within this function.
> ./net/dsa/dsa2.c:639:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 618, but without a corresponding object release within this function.
>
> Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
> Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>
> Reviewed-by: "David S. Miller" <davem@davemloft.net>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] tcp: remove unused parameter of tcp_sacktag_bsearch()
From: David Miller @ 2019-02-25 17:38 UTC (permalink / raw)
To: ap420073; +Cc: edumazet, netdev
In-Reply-To: <20190225094233.16231-1-ap420073@gmail.com>
From: Taehee Yoo <ap420073@gmail.com>
Date: Mon, 25 Feb 2019 18:42:33 +0900
> parameter state in the tcp_sacktag_bsearch() is not used.
> So, it can be removed.
>
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
Applied, thanks Taehee.
^ permalink raw reply
* Re: [PATCH net-next v2 5/7] net: sched: pie: add more cases to auto-tune alpha and beta
From: David Miller @ 2019-02-25 17:40 UTC (permalink / raw)
To: lesliemonis
Cc: jhs, netdev, dave, tahiliani, dhavaljkhandla26, hrishihiraskar,
bmanish15597, sdp.sachin
In-Reply-To: <20190225102051.12268-6-lesliemonis@gmail.com>
From: Leslie Monis <lesliemonis@gmail.com>
Date: Mon, 25 Feb 2019 15:50:49 +0530
> + power = 100;
> + while (q->vars.prob < MAX_PROB / (u64)power &&
You have to use div_u64() or similar when doing 64-bit divisons otherwise
you'll get build failures on 32-bit platforms.
^ permalink raw reply
* Re: [PATCH] tcp: clean up SOCK_DEBUG()
From: David Miller @ 2019-02-25 17:41 UTC (permalink / raw)
To: laoar.shao; +Cc: netdev, shaoyafang
In-Reply-To: <1551090828-31988-1-git-send-email-laoar.shao@gmail.com>
From: Yafang Shao <laoar.shao@gmail.com>
Date: Mon, 25 Feb 2019 18:33:48 +0800
> Per discussion with Daniel[1] and Eric[2], these SOCK_DEBUG() calles in
> TCP are not needed now.
> We'd better clean up it.
>
> [1] https://patchwork.ozlabs.org/patch/1035573/
> [2] https://patchwork.ozlabs.org/patch/1040533/
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net v3 0/2] selftests: pmtu: fix and increase coverage
From: Stefano Brivio @ 2019-02-25 17:46 UTC (permalink / raw)
To: Paolo Abeni; +Cc: netdev, David S. Miller, David Ahern, Sabrina Dubroca
In-Reply-To: <cover.1551106591.git.pabeni@redhat.com>
On Mon, 25 Feb 2019 16:08:35 +0100
Paolo Abeni <pabeni@redhat.com> wrote:
> This series includes a fixup for the pmtu.sh test script, related to IPv6
> address management, and adds coverage for the recently reported and fixed
> PMTU exception issue
>
> v2 -> v3:
> - more cleanups
>
> v1 -> v2:
> - several script cleanups
>
> Paolo Abeni (2):
> selftests: pmtu: disable DAD in all namespaces
> selftests: pmtu: add explicit tests for PMTU exceptions cleanup
Thanks for following up!
Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
--
Stefano
^ permalink raw reply
* Re: [PATCH net-next resend 1/2] net: dsa: mv88e6xxx: Default CMODE to 1000BaseX only on 6390X
From: David Miller @ 2019-02-25 17:47 UTC (permalink / raw)
To: marek.behun; +Cc: netdev, andrew, f.fainelli
In-Reply-To: <20190225113955.25700-2-marek.behun@nic.cz>
From: Marek Behún <marek.behun@nic.cz>
Date: Mon, 25 Feb 2019 12:39:54 +0100
> Commit 787799a9d555 sets the SERDES interfaces of 6390 and 6390X to
> 1000BaseX, but this is only needed on 6390X, since there are SERDES
> interfaces which can be used on lower ports on 6390.
>
> This commit fixes this by returning to previous behaviour on 6390.
> (Previous behaviour means that CMODE is not set at all if requested mode
> is NA).
>
> This is needed on Turris MOX, where the 88e6190 is connected to CPU in
> 2500BaseX mode.
>
> Fixes: 787799a9d555 ("net: dsa: mv88e6xxx: Default ports 9/10 6390X CMODE to 1000BaseX")
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next resend 2/2] net: dsa: mv88e6xxx: Fix phylink_validate for Topaz family
From: David Miller @ 2019-02-25 17:47 UTC (permalink / raw)
To: marek.behun; +Cc: netdev, andrew, f.fainelli
In-Reply-To: <20190225113955.25700-3-marek.behun@nic.cz>
From: Marek Behún <marek.behun@nic.cz>
Date: Mon, 25 Feb 2019 12:39:55 +0100
> The Topaz family should have different phylink_validate method from the
> Peridot, since on Topaz the port supporting 2500BaseX mode is port 5,
> not 9 and 10.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Applied.
^ permalink raw reply
* Re: [PATCH net-next v2 5/7] net: sched: pie: add more cases to auto-tune alpha and beta
From: Leslie Monis @ 2019-02-25 18:04 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20190225.094038.1156384775297802527.davem@davemloft.net>
On Mon, Feb 25, 2019 at 09:40:38AM -0800, David Miller wrote:
> From: Leslie Monis <lesliemonis@gmail.com>
> Date: Mon, 25 Feb 2019 15:50:49 +0530
>
> > + power = 100;
> > + while (q->vars.prob < MAX_PROB / (u64)power &&
>
> You have to use div_u64() or similar when doing 64-bit divisons otherwise
> you'll get build failures on 32-bit platforms.
Alright. Thanks for the feedback.
^ permalink raw reply
* Re: [PATCH net] net: sched: act_tunnel_key: fix NULL pointer dereference during init
From: David Miller @ 2019-02-25 18:16 UTC (permalink / raw)
To: vladbu; +Cc: netdev, dcaratti, jhs, xiyou.wangcong, jiri, wenxu, roid
In-Reply-To: <20190225152827.8741-1-vladbu@mellanox.com>
From: Vlad Buslov <vladbu@mellanox.com>
Date: Mon, 25 Feb 2019 17:28:27 +0200
> Metadata pointer is only initialized for action TCA_TUNNEL_KEY_ACT_SET, but
> it is unconditionally dereferenced in tunnel_key_init() error handler.
> Verify that metadata pointer is not NULL before dereferencing it in
> tunnel_key_init error handling code.
>
> Fixes: ee28bb56ac5b ("net/sched: fix memory leak in act_tunnel_key_init()")
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [PATCH net-next] net: sched: set dedicated tcf_walker flag when tp is empty
From: David Miller @ 2019-02-25 18:18 UTC (permalink / raw)
To: vladbu; +Cc: netdev, jhs, xiyou.wangcong, jiri
In-Reply-To: <20190225153831.10037-1-vladbu@mellanox.com>
From: Vlad Buslov <vladbu@mellanox.com>
Date: Mon, 25 Feb 2019 17:38:31 +0200
> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
> at least once is unreliable. Some classifiers set 'stop' flag on error
> before calling walker callback, other classifiers used to call it with NULL
> filter pointer when empty. In order to prevent further regressions, extend
> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
> tcf_walker->fn() implementation that is used to check if classifier has
> filters configured.
>
> Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent execution")
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next] net: sched: don't release block->lock when dumping chains
From: David Miller @ 2019-02-25 18:25 UTC (permalink / raw)
To: vladbu; +Cc: netdev, jhs, xiyou.wangcong, jiri
In-Reply-To: <20190225154544.10453-1-vladbu@mellanox.com>
From: Vlad Buslov <vladbu@mellanox.com>
Date: Mon, 25 Feb 2019 17:45:44 +0200
> Function tc_dump_chain() obtains and releases block->lock on each iteration
> of its inner loop that dumps all chains on block. Outputting chain template
> info is fast operation so locking/unlocking mutex multiple times is an
> overhead when lock is highly contested. Modify tc_dump_chain() to only
> obtain block->lock once and dump all chains without releasing it.
>
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
Applied, thanks Vlad.
^ permalink raw reply
* Re: [PATCH] tun: fix blocking read
From: David Miller @ 2019-02-25 18:25 UTC (permalink / raw)
To: eric.dumazet; +Cc: mail, netdev, linux-kernel
In-Reply-To: <b727b3bb-ec46-12bf-dd38-0363c9ca5854@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 25 Feb 2019 07:53:08 -0800
>
>
> On 02/24/2019 10:12 PM, David Miller wrote:
>> From: Timur Celik <mail@timurcelik.de>
>> Date: Sat, 23 Feb 2019 12:53:13 +0100
>>
>>> This patch moves setting of the current state into the loop. Otherwise
>>> the task may end up in a busy wait loop if none of the break conditions
>>> are met.
>>>
>>> Signed-off-by: Timur Celik <mail@timurcelik.de>
>>
>> Applied and queued up for -stable, thanks.
>>
>
> First part of the patch was matching the changelog, however
> the second change was not really needed, or should have used
>
> __set_current_state(TASK_RUNNING);
Timur, please followup.
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH net-next 3/4] e1000e: Fix -Wformat-truncation warnings
From: Neftin, Sasha @ 2019-02-25 18:26 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: open list, moderated list:INTEL ETHERNET DRIVERS, davem
In-Reply-To: <20190222040929.16657-4-f.fainelli@gmail.com>
On 22/02/2019 6:09, Florian Fainelli wrote:
> Provide precision hints to snprintf() since we know the destination
> buffer size of the RX/TX ring names are IFNAMSIZ + 5 - 1. This fixes the
> following warnings:
>
> drivers/net/ethernet/intel/e1000e/netdev.c: In function
> 'e1000_request_msix':
> drivers/net/ethernet/intel/e1000e/netdev.c:2109:13: warning: 'snprintf'
> output may be truncated before the last format character
> [-Wformat-truncation=]
> "%s-rx-0", netdev->name);
> ^
> drivers/net/ethernet/intel/e1000e/netdev.c:2107:3: note: 'snprintf'
> output between 6 and 21 bytes into a destination of size 20
> snprintf(adapter->rx_ring->name,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> sizeof(adapter->rx_ring->name) - 1,
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> "%s-rx-0", netdev->name);
> ~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/ethernet/intel/e1000e/netdev.c:2125:13: warning: 'snprintf'
> output may be truncated before the last format character
> [-Wformat-truncation=]
> "%s-tx-0", netdev->name);
> ^
> drivers/net/ethernet/intel/e1000e/netdev.c:2123:3: note: 'snprintf'
> output between 6 and 21 bytes into a destination of size 20
> snprintf(adapter->tx_ring->name,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> sizeof(adapter->tx_ring->name) - 1,
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> "%s-tx-0", netdev->name);
> ~~~~~~~~~~~~~~~~~~~~~~~~
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/net/ethernet/intel/e1000e/netdev.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 736fa51878f8..7acc61e4f645 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -2106,7 +2106,7 @@ static int e1000_request_msix(struct e1000_adapter *adapter)
> if (strlen(netdev->name) < (IFNAMSIZ - 5))
> snprintf(adapter->rx_ring->name,
> sizeof(adapter->rx_ring->name) - 1,
> - "%s-rx-0", netdev->name);
> + "%.14s-rx-0", netdev->name);
> else
> memcpy(adapter->rx_ring->name, netdev->name, IFNAMSIZ);
> err = request_irq(adapter->msix_entries[vector].vector,
> @@ -2122,7 +2122,7 @@ static int e1000_request_msix(struct e1000_adapter *adapter)
> if (strlen(netdev->name) < (IFNAMSIZ - 5))
> snprintf(adapter->tx_ring->name,
> sizeof(adapter->tx_ring->name) - 1,
> - "%s-tx-0", netdev->name);
> + "%.14s-tx-0", netdev->name);
> else
> memcpy(adapter->tx_ring->name, netdev->name, IFNAMSIZ);
> err = request_irq(adapter->msix_entries[vector].vector,
>
Acked-by: Sasha Neftin <sasha.neftin@intel.com>
^ permalink raw reply
* Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie
From: Lorenz Bauer @ 2019-02-25 18:26 UTC (permalink / raw)
To: Martin Lau
Cc: ast@kernel.org, daniel@iogearbox.net, netdev@vger.kernel.org,
linux-api@vger.kernel.org
In-Reply-To: <20190223004431.osqrn3u3lp7a6zww@kafai-mbp.dhcp.thefacebook.com>
On Sat, 23 Feb 2019 at 00:44, Martin Lau <kafai@fb.com> wrote:
>
> On Fri, Feb 22, 2019 at 09:50:55AM +0000, Lorenz Bauer wrote:
> > Using bpf_sk_lookup_tcp it's possible to ascertain whether a packet belongs
> > to a known connection. However, there is one corner case: no sockets are
> > created if SYN cookies are active. This means that the final ACK in the
> > 3WHS is misclassified.
> >
> > Using the helper, we can look up the listening socket via bpf_sk_lookup_tcp
> > and then check whether a packet is a valid SYN cookie ACK.
> >
> > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > ---
> > include/uapi/linux/bpf.h | 18 ++++++++++-
> > net/core/filter.c | 68 ++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 85 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index bcdd2474eee7..bc2af87e9621 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2359,6 +2359,21 @@ union bpf_attr {
> > * Return
> > * A **struct bpf_tcp_sock** pointer on success, or NULL in
> > * case of failure.
> > + *
> > + * int bpf_sk_check_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
> > + * Description
> > + * Check whether iph and th contain a valid SYN cookie ACK for
> > + * the listening socket in sk.
> > + *
> > + * iph points to the start of the IPv4 or IPv6 header, while
> > + * iph_len contains sizeof(struct iphdr) or sizeof(struct ip6hdr).
> > + *
> > + * th points to the start of the TCP header, while th_len contains
> > + * sizeof(struct tcphdr).
> > + *
> > + * Return
> > + * 0 if iph and th are a valid SYN cookie ACK, or a negative error
> > + * otherwise.
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -2457,7 +2472,8 @@ union bpf_attr {
> > FN(spin_lock), \
> > FN(spin_unlock), \
> > FN(sk_fullsock), \
> > - FN(tcp_sock),
> > + FN(tcp_sock), \
> > + FN(sk_check_syncookie),
> >
> > /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > * function eBPF program intends to call
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 85749f6ec789..9e68897cc7ed 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -5426,6 +5426,70 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = {
> > .arg1_type = ARG_PTR_TO_SOCK_COMMON,
> > };
> >
> > +BPF_CALL_5(bpf_sk_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
> s/bpf_sk_check_syncookie/bpf_tcp_check_syncookie/>
>
> > + struct tcphdr *, th, u32, th_len)
> > +{
> > +#if IS_ENABLED(CONFIG_SYN_COOKIES)
> nit. "#ifdef CONFIG_SYN_COOKIES" such that it is clear it is a bool kconfig.
>
> > + u32 cookie;
> > + int ret;
> > +
> > + if (unlikely(th_len < sizeof(*th)))
> > + return -EINVAL;
> > +
> > + /* sk_listener() allows TCP_NEW_SYN_RECV, which makes no sense here. */
> > + if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
> From the test program in patch 3, the "sk" here is obtained from
> bpf_sk_lookup_tcp() which does a sk_to_full_sk() before returning.
> AFAICT, meaning bpf_sk_lookup_tcp() will return the listening sk
> even if there is a request_sock. Does it make sense to check
> syncookie if there is already a request_sock?
No, that doesn't make a lot of sense. I hadn't realised that
sk_lookup_tcp only returns full sockets.
This means we need a way to detect that there is a request sock for a
given tuple.
* adding a reqsk_exists(tuple) helper means we have to pay the lookup cost twice
* drop the sk argument and do the necessary lookups in the helper
itself, but that also
wastes a call to __inet_lookup_listener
* skip sk_to_full_sk() in a helper and return RET_PTR_TO_SOCK_COMMON,
but that violates a bunch of assumptions (e.g. calling bpf_sk_release on them)
For context: ultimately we want use this to answer the question: does
this (encapsulated)
packet contain a payload destined to a local socket? Amongst the edge
cases we need to
handle are ICMP Packet Too Big messages and SYN cookies. A solution
would be to hide
all this in an "uber" helper that takes pointers to the L3 / L4
headers and returns a verdict,
but that seems a bit gross.
>
> > + return -EINVAL;
> > +
> > + if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
> Should tcp_synq_no_recent_overflow(tp) be checked also?
>
Yes, not sure how that slipped out.
> > + return -EINVAL;
> > +
> > + if (!th->ack || th->rst)
> How about th->syn?
>
Yes, I missed the fact that the callers in tcp_ipv{4,6}.c check this.
> > + return -ENOENT;
> > +
> > + cookie = ntohl(th->ack_seq) - 1;
> > +
> > + switch (sk->sk_family) {
> > + case AF_INET:
> > + if (unlikely(iph_len < sizeof(struct iphdr)))
> > + return -EINVAL;
> > +
> > + ret = __cookie_v4_check((struct iphdr *)iph, th, cookie);
> > + break;
> > +
> > +#if IS_ENABLED(CONFIG_IPV6)
> > + case AF_INET6:
> > + if (unlikely(iph_len < sizeof(struct ipv6hdr)))
> > + return -EINVAL;
> > +
> > + ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie);
> > + break;
> > +#endif /* CONFIG_IPV6 */
> > +
> > + default:
> > + return -EPROTONOSUPPORT;
> > + }
> > +
> > + if (ret > 0)
> > + return 0;
> > +
> > + return -ENOENT;
> > +#else
> > + return -ENOTSUP;
> > +#endif
> > +}
> > +
> > +static const struct bpf_func_proto bpf_sk_check_syncookie_proto = {
> > + .func = bpf_sk_check_syncookie,
> > + .gpl_only = true,
> > + .pkt_access = true,
> > + .ret_type = RET_INTEGER,
> > + .arg1_type = ARG_PTR_TO_SOCKET,
> I think it should be ARG_PTR_TO_TCP_SOCK
>
> > + .arg2_type = ARG_PTR_TO_MEM,
> > + .arg3_type = ARG_CONST_SIZE,
> > + .arg4_type = ARG_PTR_TO_MEM,
> > + .arg5_type = ARG_CONST_SIZE,
> > +};
> > +
> > #endif /* CONFIG_INET */
--
Lorenz Bauer | Systems Engineer
25 Lavington St., London SE1 0NZ
www.cloudflare.com
^ permalink raw reply
* Re: [PATCH net] geneve: use ipv6_mod_enabled to check if ipv6 is disabled
From: David Miller @ 2019-02-25 18:28 UTC (permalink / raw)
To: dsa; +Cc: liuhangbin, netdev, sbrivio
In-Reply-To: <00742c9b-9cf8-1025-2c3a-0684cd8f6c7f@cumulusnetworks.com>
From: David Ahern <dsa@cumulusnetworks.com>
Date: Mon, 25 Feb 2019 09:57:08 -0700
> On 2/24/19 9:13 PM, Hangbin Liu wrote:
>> ipv6_mod_enabled() is more safe and gentle to check if ipv6 is disabled
>> at running time.
>>
>> Fixes: c0a47e44c098 ("geneve: should not call rt6_lookup() when ipv6 was disabled")
>> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>> ---
>> drivers/net/geneve.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
>> index 3377ac66a347..42c5e43ece68 100644
>> --- a/drivers/net/geneve.c
>> +++ b/drivers/net/geneve.c
>> @@ -1514,7 +1514,7 @@ static void geneve_link_config(struct net_device *dev,
>> case AF_INET6: {
>> struct rt6_info *rt;
>>
>> - if (!__in6_dev_get(dev))
>> + if (!ipv6_mod_enabled())
>> break;
>>
>> rt = rt6_lookup(geneve->net, &info->key.u.ipv6.dst, NULL, 0,
>>
>
> seems to me the existing check is more appropriate and relevant for the
> code path: is ipv6 enabled on this device versus is ipv6 enabled at all.
Agreed.
^ permalink raw reply
* Re: [PATCH net-next v3 5/6] devlink: hold a reference to the netdevice around ethtool compat
From: Jakub Kicinski @ 2019-02-25 18:31 UTC (permalink / raw)
To: Jiri Pirko; +Cc: davem, mkubecek, andrew, f.fainelli, netdev, oss-drivers
In-Reply-To: <20190224110003.GI2759@nanopsycho.orion>
On Sun, 24 Feb 2019 12:00:03 +0100, Jiri Pirko wrote:
> Fri, Feb 22, 2019 at 11:07:56PM CET, jakub.kicinski@netronome.com wrote:
> >When ethtool is calling into devlink compat code make sure we have
> >a reference on the netdevice on which the operation was invoked.
> >
> >v3: move the hold/lock logic into devlink_compat_* functions (Florian)
> >
> >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> >---
> > net/core/devlink.c | 34 +++++++++++++++++++++++-----------
> > net/core/ethtool.c | 13 ++-----------
> > 2 files changed, 25 insertions(+), 22 deletions(-)
> >
> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >index a13055160be0..78c6ea1870ca 100644
> >--- a/net/core/devlink.c
> >+++ b/net/core/devlink.c
> >@@ -6430,27 +6430,39 @@ void devlink_compat_running_version(struct net_device *dev,
> > {
> > struct devlink *devlink;
> >
> >+ dev_hold(dev);
> >+ rtnl_unlock();
>
> Ha, I got it now. You rely on dev_hold to make sure the
> devlink instance does not dissappear. But until this patch, that is not
> guaranteed (or I'm missing it).
Yup, I think the expectation that drivers should free netdevs before
unregistering devlink holds today. But it may be a source of bugs :S
I can add take devlink_mutex, and check the devlink instance is
registered. Do you prefer an explicit ->registered field like port has,
or can I do this:
diff --git a/net/core/devlink.c b/net/core/devlink.c
index cefcc0f45d44..be39ad6a4e2e 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -5275,6 +5275,7 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size)
return NULL;
devlink->ops = ops;
devlink_net_set(devlink, &init_net);
+ INIT_LIST_HEAD(&devlink->list);
INIT_LIST_HEAD(&devlink->port_list);
INIT_LIST_HEAD(&devlink->sb_list);
INIT_LIST_HEAD_RCU(&devlink->dpipe_table_list);
@@ -5303,6 +5304,11 @@ int devlink_register(struct devlink *devlink, struct device *dev)
}
EXPORT_SYMBOL_GPL(devlink_register);
+static bool devlink_is_registered(struct devlink *devlink)
+{
+ return list_empty(&devlink->list);
+}
+
/**
* devlink_unregister - Unregister devlink instance
*
@@ -5312,7 +5318,7 @@ void devlink_unregister(struct devlink *devlink)
{
mutex_lock(&devlink_mutex);
devlink_notify(devlink, DEVLINK_CMD_DEL);
- list_del(&devlink->list);
+ list_del_init(&devlink->list);
mutex_unlock(&devlink_mutex);
}
EXPORT_SYMBOL_GPL(devlink_unregister);
?
^ permalink raw reply related
* Re: [PATCH net-next v3 6/6] devlink: add missing NULL checks for devlink ops
From: Jakub Kicinski @ 2019-02-25 18:32 UTC (permalink / raw)
To: Jiri Pirko; +Cc: davem, mkubecek, andrew, f.fainelli, netdev, oss-drivers
In-Reply-To: <20190224110319.GJ2759@nanopsycho.orion>
On Sun, 24 Feb 2019 12:03:19 +0100, Jiri Pirko wrote:
> Fri, Feb 22, 2019 at 11:07:57PM CET, jakub.kicinski@netronome.com wrote:
>> Commit 76726ccb7f46 ("devlink: add flash update command") and
>> commit 2d8dc5bbf4e7 ("devlink: Add support for reload")
>> access devlink ops without NULL-checking. Add the missing checks.
>> Note that all drivers currently implementing devlink pass non-NULL
>> ops, so this is not a problem.
>
> Wouldn't it be better to rather put WARN_ON&fail when driver calls
> devlink_alloc() with NULL ops and avoid these checks in the whole code?
Sounds good! Should I remove the existing ones?
^ permalink raw reply
* Re: [PATCH v1 net-next 2/4] net: dsa: microchip: add MIB counter reading support
From: Pavel Machek @ 2019-02-25 18:34 UTC (permalink / raw)
To: Joe Perches
Cc: Florian Fainelli, Tristram.Ha, andrew, sergio.paracuellos,
UNGLinuxDriver, netdev
In-Reply-To: <2cf3bf259bed9e44bd7e885850e45cb3a62e3779.camel@perches.com>
[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]
On Wed 2019-02-20 15:49:25, Joe Perches wrote:
> On Tue, 2019-02-12 at 19:51 -0800, Florian Fainelli wrote:
> > On February 12, 2019 6:39:49 PM PST, Tristram.Ha@microchip.com wrote:
> > > > > +static void ksz9477_freeze_mib(struct ksz_device *dev, int port,
> > > > > + bool freeze)
> > > > > +{
> > > > > + struct ksz_port *p = &dev->ports[port];
> > > > > + u32 val = freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;
> > > >
> > > > Reverse Christmas tree.
> > >
> > > There was a checkpatch.pl patch in 2016 that tried to check this, but
> > > it was never accepted?
>
> https://lore.kernel.org/patchwork/patch/732076/
>
> While I still think use of reverse christmas tree is misguided,
> there were some disagreements about what form it really is:
Agreed about "misguided".
Often there's more logical order, like "self" pointer first... which
this patch follows.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH net] net: socket: set sock->sk to NULL after calling proto_ops::release()
From: David Miller @ 2019-02-25 18:41 UTC (permalink / raw)
To: ebiggers
Cc: netdev, linux-kernel, maowenan, xiyou.wangcong, lorenzo,
penguin-kernel, viro
In-Reply-To: <20190221221356.173485-1-ebiggers@kernel.org>
From: Eric Biggers <ebiggers@kernel.org>
Date: Thu, 21 Feb 2019 14:13:56 -0800
> From: Eric Biggers <ebiggers@google.com>
>
> Commit 9060cb719e61 ("net: crypto set sk to NULL when af_alg_release.")
> fixed a use-after-free in sockfs_setattr() when an AF_ALG socket is
> closed concurrently with fchownat(). However, it ignored that many
> other proto_ops::release() methods don't set sock->sk to NULL and
> therefore allow the same use-after-free:
...
> Rather than fixing all these and relying on every socket type to get
> this right forever, just make __sock_release() set sock->sk to NULL
> itself after calling proto_ops::release().
>
> Reproducer that produces the KASAN splat when any of these socket types
> are configured into the kernel:
...
> Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.")
> Signed-off-by: Eric Biggers <ebiggers@google.com>
Applied and queued up for -stable, thanks Eric.
^ permalink raw reply
* [net-next 04/16] ice: sizeof(<type>) should be avoided
From: Jeff Kirsher @ 2019-02-25 18:42 UTC (permalink / raw)
To: davem
Cc: Bruce Allan, netdev, nhorman, sassmann, Anirudh Venkataramanan,
Andrew Bowers, Jeff Kirsher
In-Reply-To: <20190225184306.13505-1-jeffrey.t.kirsher@intel.com>
From: Bruce Allan <bruce.w.allan@intel.com>
With sizeof(), it is preferable to use the variable of type <type> instead
of sizeof(<type>).
There are multiple places where a temporary variable is used to hold a
'size' value which is then used for a subsequent alloc/memset. Get rid
of the temporary variable by calculating size as part of the alloc/memset
statement.
Also remove unnecessary type-cast.
Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/ice/ice_ethtool.c | 15 ++++++------
drivers/net/ethernet/intel/ice/ice_lib.c | 10 ++++----
drivers/net/ethernet/intel/ice/ice_main.c | 23 +++++++++----------
drivers/net/ethernet/intel/ice/ice_sched.c | 9 ++++----
drivers/net/ethernet/intel/ice/ice_switch.c | 2 +-
drivers/net/ethernet/intel/ice/ice_txrx.c | 24 ++++++++------------
6 files changed, 38 insertions(+), 45 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index a82f0202652d..a93daf660171 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -1084,7 +1084,7 @@ ice_get_settings_link_up(struct ethtool_link_ksettings *ks,
* current PHY type, get what is supported by the NVM and intersect
* them to get what is truly supported
*/
- memset(&cap_ksettings, 0, sizeof(struct ethtool_link_ksettings));
+ memset(&cap_ksettings, 0, sizeof(cap_ksettings));
ice_phy_type_to_ethtool(netdev, &cap_ksettings);
ethtool_intersect_link_masks(ks, &cap_ksettings);
@@ -1416,7 +1416,7 @@ ice_set_link_ksettings(struct net_device *netdev,
return -EOPNOTSUPP;
/* copy the ksettings to copy_ks to avoid modifying the original */
- memcpy(©_ks, ks, sizeof(struct ethtool_link_ksettings));
+ memcpy(©_ks, ks, sizeof(copy_ks));
/* save autoneg out of ksettings */
autoneg = copy_ks.base.autoneg;
@@ -1435,7 +1435,7 @@ ice_set_link_ksettings(struct net_device *netdev,
return -EINVAL;
/* get our own copy of the bits to check against */
- memset(&safe_ks, 0, sizeof(struct ethtool_link_ksettings));
+ memset(&safe_ks, 0, sizeof(safe_ks));
safe_ks.base.cmd = copy_ks.base.cmd;
safe_ks.base.link_mode_masks_nwords =
copy_ks.base.link_mode_masks_nwords;
@@ -1449,8 +1449,7 @@ ice_set_link_ksettings(struct net_device *netdev,
/* If copy_ks.base and safe_ks.base are not the same now, then they are
* trying to set something that we do not support.
*/
- if (memcmp(©_ks.base, &safe_ks.base,
- sizeof(struct ethtool_link_settings)))
+ if (memcmp(©_ks.base, &safe_ks.base, sizeof(copy_ks.base)))
return -EOPNOTSUPP;
while (test_and_set_bit(__ICE_CFG_BUSY, pf->state)) {
@@ -1474,7 +1473,7 @@ ice_set_link_ksettings(struct net_device *netdev,
}
/* Copy abilities to config in case autoneg is not set below */
- memset(&config, 0, sizeof(struct ice_aqc_set_phy_cfg_data));
+ memset(&config, 0, sizeof(config));
config.caps = abilities->caps & ~ICE_AQC_PHY_AN_MODE;
if (abilities->caps & ICE_AQC_PHY_AN_MODE)
config.caps |= ICE_AQ_PHY_ENA_AUTO_LINK_UPDT;
@@ -1668,7 +1667,7 @@ ice_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring)
vsi->tx_rings[0]->count, new_tx_cnt);
tx_rings = devm_kcalloc(&pf->pdev->dev, vsi->alloc_txq,
- sizeof(struct ice_ring), GFP_KERNEL);
+ sizeof(*tx_rings), GFP_KERNEL);
if (!tx_rings) {
err = -ENOMEM;
goto done;
@@ -1700,7 +1699,7 @@ ice_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring)
vsi->rx_rings[0]->count, new_rx_cnt);
rx_rings = devm_kcalloc(&pf->pdev->dev, vsi->alloc_rxq,
- sizeof(struct ice_ring), GFP_KERNEL);
+ sizeof(*rx_rings), GFP_KERNEL);
if (!rx_rings) {
err = -ENOMEM;
goto done;
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 27c3760ae5cb..82aae530d6c5 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -249,12 +249,12 @@ static int ice_vsi_alloc_arrays(struct ice_vsi *vsi, bool alloc_qvectors)
/* allocate memory for both Tx and Rx ring pointers */
vsi->tx_rings = devm_kcalloc(&pf->pdev->dev, vsi->alloc_txq,
- sizeof(struct ice_ring *), GFP_KERNEL);
+ sizeof(*vsi->tx_rings), GFP_KERNEL);
if (!vsi->tx_rings)
goto err_txrings;
vsi->rx_rings = devm_kcalloc(&pf->pdev->dev, vsi->alloc_rxq,
- sizeof(struct ice_ring *), GFP_KERNEL);
+ sizeof(*vsi->rx_rings), GFP_KERNEL);
if (!vsi->rx_rings)
goto err_rxrings;
@@ -262,7 +262,7 @@ static int ice_vsi_alloc_arrays(struct ice_vsi *vsi, bool alloc_qvectors)
/* allocate memory for q_vector pointers */
vsi->q_vectors = devm_kcalloc(&pf->pdev->dev,
vsi->num_q_vectors,
- sizeof(struct ice_q_vector *),
+ sizeof(*vsi->q_vectors),
GFP_KERNEL);
if (!vsi->q_vectors)
goto err_vectors;
@@ -355,7 +355,7 @@ void ice_vsi_delete(struct ice_vsi *vsi)
ctxt.vf_num = vsi->vf_id;
ctxt.vsi_num = vsi->vsi_num;
- memcpy(&ctxt.info, &vsi->info, sizeof(struct ice_aqc_vsi_props));
+ memcpy(&ctxt.info, &vsi->info, sizeof(ctxt.info));
status = ice_free_vsi(&pf->hw, vsi->idx, &ctxt, false, NULL);
if (status)
@@ -1620,7 +1620,7 @@ ice_vsi_cfg_txqs(struct ice_vsi *vsi, struct ice_ring **rings, int offset)
u16 buf_len, i, pf_q;
int err = 0, tc;
- buf_len = sizeof(struct ice_aqc_add_tx_qgrp);
+ buf_len = sizeof(*qg_buf);
qg_buf = devm_kzalloc(&pf->pdev->dev, buf_len, GFP_KERNEL);
if (!qg_buf)
return -ENOMEM;
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 9d266d754445..0731b8994958 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -1513,8 +1513,8 @@ static int ice_cfg_netdev(struct ice_vsi *vsi)
u8 mac_addr[ETH_ALEN];
int err;
- netdev = alloc_etherdev_mqs(sizeof(struct ice_netdev_priv),
- vsi->alloc_txq, vsi->alloc_rxq);
+ netdev = alloc_etherdev_mqs(sizeof(*np), vsi->alloc_txq,
+ vsi->alloc_rxq);
if (!netdev)
return -ENOMEM;
@@ -1867,7 +1867,7 @@ static int ice_ena_msix_range(struct ice_pf *pf)
v_left -= pf->num_lan_msix;
pf->msix_entries = devm_kcalloc(&pf->pdev->dev, v_budget,
- sizeof(struct msix_entry), GFP_KERNEL);
+ sizeof(*pf->msix_entries), GFP_KERNEL);
if (!pf->msix_entries) {
err = -ENOMEM;
@@ -1955,7 +1955,6 @@ static void ice_clear_interrupt_scheme(struct ice_pf *pf)
static int ice_init_interrupt_scheme(struct ice_pf *pf)
{
int vectors = 0, hw_vectors = 0;
- ssize_t size;
if (test_bit(ICE_FLAG_MSIX_ENA, pf->flags))
vectors = ice_ena_msix_range(pf);
@@ -1966,9 +1965,9 @@ static int ice_init_interrupt_scheme(struct ice_pf *pf)
return vectors;
/* set up vector assignment tracking */
- size = sizeof(struct ice_res_tracker) + (sizeof(u16) * vectors);
-
- pf->sw_irq_tracker = devm_kzalloc(&pf->pdev->dev, size, GFP_KERNEL);
+ pf->sw_irq_tracker =
+ devm_kzalloc(&pf->pdev->dev, sizeof(*pf->sw_irq_tracker) +
+ (sizeof(u16) * vectors), GFP_KERNEL);
if (!pf->sw_irq_tracker) {
ice_dis_msix(pf);
return -ENOMEM;
@@ -1980,9 +1979,9 @@ static int ice_init_interrupt_scheme(struct ice_pf *pf)
/* set up HW vector assignment tracking */
hw_vectors = pf->hw.func_caps.common_cap.num_msix_vectors;
- size = sizeof(struct ice_res_tracker) + (sizeof(u16) * hw_vectors);
-
- pf->hw_irq_tracker = devm_kzalloc(&pf->pdev->dev, size, GFP_KERNEL);
+ pf->hw_irq_tracker =
+ devm_kzalloc(&pf->pdev->dev, sizeof(*pf->hw_irq_tracker) +
+ (sizeof(u16) * hw_vectors), GFP_KERNEL);
if (!pf->hw_irq_tracker) {
ice_clear_interrupt_scheme(pf);
return -ENOMEM;
@@ -2116,7 +2115,7 @@ static int ice_probe(struct pci_dev *pdev,
}
pf->vsi = devm_kcalloc(&pdev->dev, pf->num_alloc_vsi,
- sizeof(struct ice_vsi *), GFP_KERNEL);
+ sizeof(*pf->vsi), GFP_KERNEL);
if (!pf->vsi) {
err = -ENOMEM;
goto err_init_pf_unroll;
@@ -2148,7 +2147,7 @@ static int ice_probe(struct pci_dev *pdev,
}
/* create switch struct for the switch element created by FW on boot */
- pf->first_sw = devm_kzalloc(&pdev->dev, sizeof(struct ice_sw),
+ pf->first_sw = devm_kzalloc(&pdev->dev, sizeof(*pf->first_sw),
GFP_KERNEL);
if (!pf->first_sw) {
err = -ENOMEM;
diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
index 81fb7d19c0fd..f355f59d8b64 100644
--- a/drivers/net/ethernet/intel/ice/ice_sched.c
+++ b/drivers/net/ethernet/intel/ice/ice_sched.c
@@ -1066,11 +1066,10 @@ enum ice_status ice_sched_query_res_alloc(struct ice_hw *hw)
hw->max_children[i] = le16_to_cpu(max_sibl);
}
- hw->layer_info = (struct ice_aqc_layer_props *)
- devm_kmemdup(ice_hw_to_dev(hw), buf->layer_props,
- (hw->num_tx_sched_layers *
- sizeof(*hw->layer_info)),
- GFP_KERNEL);
+ hw->layer_info = devm_kmemdup(ice_hw_to_dev(hw), buf->layer_props,
+ (hw->num_tx_sched_layers *
+ sizeof(*hw->layer_info)),
+ GFP_KERNEL);
if (!hw->layer_info) {
status = ICE_ERR_NO_MEMORY;
goto sched_query_out;
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
index 2e5693107fa4..d560a8aa5015 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.c
+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
@@ -98,7 +98,7 @@ enum ice_status ice_init_def_sw_recp(struct ice_hw *hw)
u8 i;
recps = devm_kcalloc(ice_hw_to_dev(hw), ICE_MAX_NUM_RECIPES,
- sizeof(struct ice_sw_recipe), GFP_KERNEL);
+ sizeof(*recps), GFP_KERNEL);
if (!recps)
return ICE_ERR_NO_MEMORY;
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
index 2357fcac996b..67d129bfe489 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
@@ -48,7 +48,6 @@ static struct netdev_queue *txring_txq(const struct ice_ring *ring)
*/
void ice_clean_tx_ring(struct ice_ring *tx_ring)
{
- unsigned long size;
u16 i;
/* ring already cleared, nothing to do */
@@ -59,8 +58,7 @@ void ice_clean_tx_ring(struct ice_ring *tx_ring)
for (i = 0; i < tx_ring->count; i++)
ice_unmap_and_free_tx_buf(tx_ring, &tx_ring->tx_buf[i]);
- size = sizeof(struct ice_tx_buf) * tx_ring->count;
- memset(tx_ring->tx_buf, 0, size);
+ memset(tx_ring->tx_buf, 0, sizeof(*tx_ring->tx_buf) * tx_ring->count);
/* Zero out the descriptor ring */
memset(tx_ring->desc, 0, tx_ring->size);
@@ -226,21 +224,21 @@ static bool ice_clean_tx_irq(struct ice_vsi *vsi, struct ice_ring *tx_ring,
int ice_setup_tx_ring(struct ice_ring *tx_ring)
{
struct device *dev = tx_ring->dev;
- int bi_size;
if (!dev)
return -ENOMEM;
/* warn if we are about to overwrite the pointer */
WARN_ON(tx_ring->tx_buf);
- bi_size = sizeof(struct ice_tx_buf) * tx_ring->count;
- tx_ring->tx_buf = devm_kzalloc(dev, bi_size, GFP_KERNEL);
+ tx_ring->tx_buf =
+ devm_kzalloc(dev, sizeof(*tx_ring->tx_buf) * tx_ring->count,
+ GFP_KERNEL);
if (!tx_ring->tx_buf)
return -ENOMEM;
/* round up to nearest 4K */
- tx_ring->size = tx_ring->count * sizeof(struct ice_tx_desc);
- tx_ring->size = ALIGN(tx_ring->size, 4096);
+ tx_ring->size = ALIGN(tx_ring->count * sizeof(struct ice_tx_desc),
+ 4096);
tx_ring->desc = dmam_alloc_coherent(dev, tx_ring->size, &tx_ring->dma,
GFP_KERNEL);
if (!tx_ring->desc) {
@@ -267,7 +265,6 @@ int ice_setup_tx_ring(struct ice_ring *tx_ring)
void ice_clean_rx_ring(struct ice_ring *rx_ring)
{
struct device *dev = rx_ring->dev;
- unsigned long size;
u16 i;
/* ring already cleared, nothing to do */
@@ -292,8 +289,7 @@ void ice_clean_rx_ring(struct ice_ring *rx_ring)
rx_buf->page_offset = 0;
}
- size = sizeof(struct ice_rx_buf) * rx_ring->count;
- memset(rx_ring->rx_buf, 0, size);
+ memset(rx_ring->rx_buf, 0, sizeof(*rx_ring->rx_buf) * rx_ring->count);
/* Zero out the descriptor ring */
memset(rx_ring->desc, 0, rx_ring->size);
@@ -331,15 +327,15 @@ void ice_free_rx_ring(struct ice_ring *rx_ring)
int ice_setup_rx_ring(struct ice_ring *rx_ring)
{
struct device *dev = rx_ring->dev;
- int bi_size;
if (!dev)
return -ENOMEM;
/* warn if we are about to overwrite the pointer */
WARN_ON(rx_ring->rx_buf);
- bi_size = sizeof(struct ice_rx_buf) * rx_ring->count;
- rx_ring->rx_buf = devm_kzalloc(dev, bi_size, GFP_KERNEL);
+ rx_ring->rx_buf =
+ devm_kzalloc(dev, sizeof(*rx_ring->rx_buf) * rx_ring->count,
+ GFP_KERNEL);
if (!rx_ring->rx_buf)
return -ENOMEM;
--
2.20.1
^ permalink raw reply related
* [net-next 05/16] ice: fix stack hogs from struct ice_vsi_ctx structures
From: Jeff Kirsher @ 2019-02-25 18:42 UTC (permalink / raw)
To: davem
Cc: Bruce Allan, netdev, nhorman, sassmann, Anirudh Venkataramanan,
Andrew Bowers, Jeff Kirsher
In-Reply-To: <20190225184306.13505-1-jeffrey.t.kirsher@intel.com>
From: Bruce Allan <bruce.w.allan@intel.com>
struct ice_vsi_ctx has gotten large enough that function local declarations
of it on the stack are causing stack hogs. Fix that by allocating the
structs on heap. Cleanup some formatting issues in the code around these
changes and fix incorrect data type uses of returned functions in a couple
places.
Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/ice/ice_lib.c | 98 ++++++++++++-------
drivers/net/ethernet/intel/ice/ice_main.c | 27 +++--
.../net/ethernet/intel/ice/ice_virtchnl_pf.c | 59 ++++++-----
3 files changed, 117 insertions(+), 67 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 82aae530d6c5..d93a0d5a783c 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -348,19 +348,25 @@ static int ice_get_free_slot(void *array, int size, int curr)
void ice_vsi_delete(struct ice_vsi *vsi)
{
struct ice_pf *pf = vsi->back;
- struct ice_vsi_ctx ctxt;
+ struct ice_vsi_ctx *ctxt;
enum ice_status status;
+ ctxt = devm_kzalloc(&pf->pdev->dev, sizeof(*ctxt), GFP_KERNEL);
+ if (!ctxt)
+ return;
+
if (vsi->type == ICE_VSI_VF)
- ctxt.vf_num = vsi->vf_id;
- ctxt.vsi_num = vsi->vsi_num;
+ ctxt->vf_num = vsi->vf_id;
+ ctxt->vsi_num = vsi->vsi_num;
- memcpy(&ctxt.info, &vsi->info, sizeof(ctxt.info));
+ memcpy(&ctxt->info, &vsi->info, sizeof(ctxt->info));
- status = ice_free_vsi(&pf->hw, vsi->idx, &ctxt, false, NULL);
+ status = ice_free_vsi(&pf->hw, vsi->idx, ctxt, false, NULL);
if (status)
dev_err(&pf->pdev->dev, "Failed to delete VSI %i in FW\n",
vsi->vsi_num);
+
+ devm_kfree(&pf->pdev->dev, ctxt);
}
/**
@@ -908,37 +914,41 @@ static void ice_set_rss_vsi_ctx(struct ice_vsi_ctx *ctxt, struct ice_vsi *vsi)
*/
static int ice_vsi_init(struct ice_vsi *vsi)
{
- struct ice_vsi_ctx ctxt = { 0 };
struct ice_pf *pf = vsi->back;
struct ice_hw *hw = &pf->hw;
+ struct ice_vsi_ctx *ctxt;
int ret = 0;
+ ctxt = devm_kzalloc(&pf->pdev->dev, sizeof(*ctxt), GFP_KERNEL);
+ if (!ctxt)
+ return -ENOMEM;
+
switch (vsi->type) {
case ICE_VSI_PF:
- ctxt.flags = ICE_AQ_VSI_TYPE_PF;
+ ctxt->flags = ICE_AQ_VSI_TYPE_PF;
break;
case ICE_VSI_VF:
- ctxt.flags = ICE_AQ_VSI_TYPE_VF;
+ ctxt->flags = ICE_AQ_VSI_TYPE_VF;
/* VF number here is the absolute VF number (0-255) */
- ctxt.vf_num = vsi->vf_id + hw->func_caps.vf_base_id;
+ ctxt->vf_num = vsi->vf_id + hw->func_caps.vf_base_id;
break;
default:
return -ENODEV;
}
- ice_set_dflt_vsi_ctx(&ctxt);
+ ice_set_dflt_vsi_ctx(ctxt);
/* if the switch is in VEB mode, allow VSI loopback */
if (vsi->vsw->bridge_mode == BRIDGE_MODE_VEB)
- ctxt.info.sw_flags |= ICE_AQ_VSI_SW_FLAG_ALLOW_LB;
+ ctxt->info.sw_flags |= ICE_AQ_VSI_SW_FLAG_ALLOW_LB;
/* Set LUT type and HASH type if RSS is enabled */
if (test_bit(ICE_FLAG_RSS_ENA, pf->flags))
- ice_set_rss_vsi_ctx(&ctxt, vsi);
+ ice_set_rss_vsi_ctx(ctxt, vsi);
- ctxt.info.sw_id = vsi->port_info->sw_id;
- ice_vsi_setup_q_map(vsi, &ctxt);
+ ctxt->info.sw_id = vsi->port_info->sw_id;
+ ice_vsi_setup_q_map(vsi, ctxt);
- ret = ice_add_vsi(hw, vsi->idx, &ctxt, NULL);
+ ret = ice_add_vsi(hw, vsi->idx, ctxt, NULL);
if (ret) {
dev_err(&pf->pdev->dev,
"Add VSI failed, err %d\n", ret);
@@ -946,11 +956,12 @@ static int ice_vsi_init(struct ice_vsi *vsi)
}
/* keep context for update VSI operations */
- vsi->info = ctxt.info;
+ vsi->info = ctxt->info;
/* record VSI number returned */
- vsi->vsi_num = ctxt.vsi_num;
+ vsi->vsi_num = ctxt->vsi_num;
+ devm_kfree(&pf->pdev->dev, ctxt);
return ret;
}
@@ -1823,26 +1834,34 @@ int ice_vsi_manage_vlan_insertion(struct ice_vsi *vsi)
{
struct device *dev = &vsi->back->pdev->dev;
struct ice_hw *hw = &vsi->back->hw;
- struct ice_vsi_ctx ctxt = { 0 };
+ struct ice_vsi_ctx *ctxt;
enum ice_status status;
+ int ret = 0;
+
+ ctxt = devm_kzalloc(dev, sizeof(*ctxt), GFP_KERNEL);
+ if (!ctxt)
+ return -ENOMEM;
/* Here we are configuring the VSI to let the driver add VLAN tags by
* setting vlan_flags to ICE_AQ_VSI_VLAN_MODE_ALL. The actual VLAN tag
* insertion happens in the Tx hot path, in ice_tx_map.
*/
- ctxt.info.vlan_flags = ICE_AQ_VSI_VLAN_MODE_ALL;
+ ctxt->info.vlan_flags = ICE_AQ_VSI_VLAN_MODE_ALL;
- ctxt.info.valid_sections = cpu_to_le16(ICE_AQ_VSI_PROP_VLAN_VALID);
+ ctxt->info.valid_sections = cpu_to_le16(ICE_AQ_VSI_PROP_VLAN_VALID);
- status = ice_update_vsi(hw, vsi->idx, &ctxt, NULL);
+ status = ice_update_vsi(hw, vsi->idx, ctxt, NULL);
if (status) {
dev_err(dev, "update VSI for VLAN insert failed, err %d aq_err %d\n",
status, hw->adminq.sq_last_status);
- return -EIO;
+ ret = -EIO;
+ goto out;
}
- vsi->info.vlan_flags = ctxt.info.vlan_flags;
- return 0;
+ vsi->info.vlan_flags = ctxt->info.vlan_flags;
+out:
+ devm_kfree(dev, ctxt);
+ return ret;
}
/**
@@ -1854,35 +1873,42 @@ int ice_vsi_manage_vlan_stripping(struct ice_vsi *vsi, bool ena)
{
struct device *dev = &vsi->back->pdev->dev;
struct ice_hw *hw = &vsi->back->hw;
- struct ice_vsi_ctx ctxt = { 0 };
+ struct ice_vsi_ctx *ctxt;
enum ice_status status;
+ int ret = 0;
+
+ ctxt = devm_kzalloc(dev, sizeof(*ctxt), GFP_KERNEL);
+ if (!ctxt)
+ return -ENOMEM;
/* Here we are configuring what the VSI should do with the VLAN tag in
* the Rx packet. We can either leave the tag in the packet or put it in
* the Rx descriptor.
*/
- if (ena) {
+ if (ena)
/* Strip VLAN tag from Rx packet and put it in the desc */
- ctxt.info.vlan_flags = ICE_AQ_VSI_VLAN_EMOD_STR_BOTH;
- } else {
+ ctxt->info.vlan_flags = ICE_AQ_VSI_VLAN_EMOD_STR_BOTH;
+ else
/* Disable stripping. Leave tag in packet */
- ctxt.info.vlan_flags = ICE_AQ_VSI_VLAN_EMOD_NOTHING;
- }
+ ctxt->info.vlan_flags = ICE_AQ_VSI_VLAN_EMOD_NOTHING;
/* Allow all packets untagged/tagged */
- ctxt.info.vlan_flags |= ICE_AQ_VSI_VLAN_MODE_ALL;
+ ctxt->info.vlan_flags |= ICE_AQ_VSI_VLAN_MODE_ALL;
- ctxt.info.valid_sections = cpu_to_le16(ICE_AQ_VSI_PROP_VLAN_VALID);
+ ctxt->info.valid_sections = cpu_to_le16(ICE_AQ_VSI_PROP_VLAN_VALID);
- status = ice_update_vsi(hw, vsi->idx, &ctxt, NULL);
+ status = ice_update_vsi(hw, vsi->idx, ctxt, NULL);
if (status) {
dev_err(dev, "update VSI for VLAN strip failed, ena = %d err %d aq_err %d\n",
ena, status, hw->adminq.sq_last_status);
- return -EIO;
+ ret = -EIO;
+ goto out;
}
- vsi->info.vlan_flags = ctxt.info.vlan_flags;
- return 0;
+ vsi->info.vlan_flags = ctxt->info.vlan_flags;
+out:
+ devm_kfree(dev, ctxt);
+ return ret;
}
/**
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 0731b8994958..aff348e42562 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3707,30 +3707,39 @@ static int ice_vsi_update_bridge_mode(struct ice_vsi *vsi, u16 bmode)
struct device *dev = &vsi->back->pdev->dev;
struct ice_aqc_vsi_props *vsi_props;
struct ice_hw *hw = &vsi->back->hw;
- struct ice_vsi_ctx ctxt = { 0 };
+ struct ice_vsi_ctx *ctxt;
enum ice_status status;
+ int ret = 0;
vsi_props = &vsi->info;
- ctxt.info = vsi->info;
+
+ ctxt = devm_kzalloc(dev, sizeof(*ctxt), GFP_KERNEL);
+ if (!ctxt)
+ return -ENOMEM;
+
+ ctxt->info = vsi->info;
if (bmode == BRIDGE_MODE_VEB)
/* change from VEPA to VEB mode */
- ctxt.info.sw_flags |= ICE_AQ_VSI_SW_FLAG_ALLOW_LB;
+ ctxt->info.sw_flags |= ICE_AQ_VSI_SW_FLAG_ALLOW_LB;
else
/* change from VEB to VEPA mode */
- ctxt.info.sw_flags &= ~ICE_AQ_VSI_SW_FLAG_ALLOW_LB;
- ctxt.info.valid_sections = cpu_to_le16(ICE_AQ_VSI_PROP_SW_VALID);
+ ctxt->info.sw_flags &= ~ICE_AQ_VSI_SW_FLAG_ALLOW_LB;
+ ctxt->info.valid_sections = cpu_to_le16(ICE_AQ_VSI_PROP_SW_VALID);
- status = ice_update_vsi(hw, vsi->idx, &ctxt, NULL);
+ status = ice_update_vsi(hw, vsi->idx, ctxt, NULL);
if (status) {
dev_err(dev, "update VSI for bridge mode failed, bmode = %d err %d aq_err %d\n",
bmode, status, hw->adminq.sq_last_status);
- return -EIO;
+ ret = -EIO;
+ goto out;
}
/* Update sw flags for book keeping */
- vsi_props->sw_flags = ctxt.info.sw_flags;
+ vsi_props->sw_flags = ctxt->info.sw_flags;
- return 0;
+out:
+ devm_kfree(dev, ctxt);
+ return ret;
}
/**
diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c
index 80b50e67cbef..79d793a76042 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c
@@ -345,25 +345,33 @@ static int ice_vsi_set_pvid(struct ice_vsi *vsi, u16 vid)
{
struct device *dev = &vsi->back->pdev->dev;
struct ice_hw *hw = &vsi->back->hw;
- struct ice_vsi_ctx ctxt = { 0 };
+ struct ice_vsi_ctx *ctxt;
enum ice_status status;
+ int ret = 0;
+
+ ctxt = devm_kzalloc(dev, sizeof(*ctxt), GFP_KERNEL);
+ if (!ctxt)
+ return -ENOMEM;
- ctxt.info.vlan_flags = ICE_AQ_VSI_VLAN_MODE_UNTAGGED |
- ICE_AQ_VSI_PVLAN_INSERT_PVID |
- ICE_AQ_VSI_VLAN_EMOD_STR;
- ctxt.info.pvid = cpu_to_le16(vid);
- ctxt.info.valid_sections = cpu_to_le16(ICE_AQ_VSI_PROP_VLAN_VALID);
+ ctxt->info.vlan_flags = (ICE_AQ_VSI_VLAN_MODE_UNTAGGED |
+ ICE_AQ_VSI_PVLAN_INSERT_PVID |
+ ICE_AQ_VSI_VLAN_EMOD_STR);
+ ctxt->info.pvid = cpu_to_le16(vid);
+ ctxt->info.valid_sections = cpu_to_le16(ICE_AQ_VSI_PROP_VLAN_VALID);
- status = ice_update_vsi(hw, vsi->idx, &ctxt, NULL);
+ status = ice_update_vsi(hw, vsi->idx, ctxt, NULL);
if (status) {
dev_info(dev, "update VSI for VLAN insert failed, err %d aq_err %d\n",
status, hw->adminq.sq_last_status);
- return -EIO;
+ ret = -EIO;
+ goto out;
}
- vsi->info.pvid = ctxt.info.pvid;
- vsi->info.vlan_flags = ctxt.info.vlan_flags;
- return 0;
+ vsi->info.pvid = ctxt->info.pvid;
+ vsi->info.vlan_flags = ctxt->info.vlan_flags;
+out:
+ devm_kfree(dev, ctxt);
+ return ret;
}
/**
@@ -2479,11 +2487,12 @@ int ice_get_vf_cfg(struct net_device *netdev, int vf_id,
int ice_set_vf_spoofchk(struct net_device *netdev, int vf_id, bool ena)
{
struct ice_netdev_priv *np = netdev_priv(netdev);
- struct ice_vsi_ctx ctx = { 0 };
struct ice_vsi *vsi = np->vsi;
struct ice_pf *pf = vsi->back;
+ struct ice_vsi_ctx *ctx;
+ enum ice_status status;
struct ice_vf *vf;
- int status;
+ int ret = 0;
/* validate the request */
if (vf_id >= pf->num_alloc_vfs) {
@@ -2503,25 +2512,31 @@ int ice_set_vf_spoofchk(struct net_device *netdev, int vf_id, bool ena)
return 0;
}
- ctx.info.valid_sections = cpu_to_le16(ICE_AQ_VSI_PROP_SECURITY_VALID);
+ ctx = devm_kzalloc(&pf->pdev->dev, sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ ctx->info.valid_sections = cpu_to_le16(ICE_AQ_VSI_PROP_SECURITY_VALID);
if (ena) {
- ctx.info.sec_flags |= ICE_AQ_VSI_SEC_FLAG_ENA_MAC_ANTI_SPOOF;
- ctx.info.sw_flags2 |= ICE_AQ_VSI_SW_FLAG_RX_PRUNE_EN_M;
+ ctx->info.sec_flags |= ICE_AQ_VSI_SEC_FLAG_ENA_MAC_ANTI_SPOOF;
+ ctx->info.sw_flags2 |= ICE_AQ_VSI_SW_FLAG_RX_PRUNE_EN_M;
}
- status = ice_update_vsi(&pf->hw, vsi->idx, &ctx, NULL);
+ status = ice_update_vsi(&pf->hw, vsi->idx, ctx, NULL);
if (status) {
dev_dbg(&pf->pdev->dev,
"Error %d, failed to update VSI* parameters\n", status);
- return -EIO;
+ ret = -EIO;
+ goto out;
}
vf->spoofchk = ena;
- vsi->info.sec_flags = ctx.info.sec_flags;
- vsi->info.sw_flags2 = ctx.info.sw_flags2;
-
- return status;
+ vsi->info.sec_flags = ctx->info.sec_flags;
+ vsi->info.sw_flags2 = ctx->info.sw_flags2;
+out:
+ devm_kfree(&pf->pdev->dev, ctx);
+ return ret;
}
/**
--
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