* Re: [pull request][net-next 0/3] Mellanox, mlx5 GRE tunnel offloads
From: Tom Herbert @ 2017-09-06 19:01 UTC (permalink / raw)
To: Alexander Duyck
Cc: Hannes Frederic Sowa, Saeed Mahameed, Saeed Mahameed,
David S. Miller, Linux Netdev List
In-Reply-To: <CAKgT0UeK0njpy1unJUV+=-JGOHTEZCJu0mNd-UuX8aO2G2zKmQ@mail.gmail.com>
On Wed, Sep 6, 2017 at 10:43 AM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Wed, Sep 6, 2017 at 9:17 AM, Tom Herbert <tom@herbertland.com> wrote:
>> On Tue, Sep 5, 2017 at 8:06 PM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> On Tue, Sep 5, 2017 at 2:13 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>>> The situation with encapsulation is even more complicated:
>>>>>
>>>>> We are basically only interested in the UDP/vxlan/Ethernet/IP/UDP
>>>>> constellation. If we do the fragmentation inside the vxlan tunnel and
>>>>> carry over the skb hash to all resulting UDP/vxlan packets source ports,
>>>>> we are fine and reordering on the receiver NIC won't happen in this
>>>>> case. If the fragmentation happens on the outer UDP header, this will
>>>>> result in reordering of the inner L2 flow. Unfortunately this depends on
>>>>> how the vxlan tunnel was set up, how other devices do that and (I
>>>>> believe so) on the kernel version.
>>>>>
>>>> This really isn't that complicated. The assumption that an IP network
>>>> always delivers packets in order is simply wrong. The inventors of
>>>> VXLAN must have know full well that when you use IP, packets can and
>>>> eventually will be delivered out of order. This isn't just because of
>>>> fragmentation, there are many other reasons that packets can be
>>>> delivered OOO. This also must have been known when IP/GRE and any
>>>> other protocol that carries L2 over IP was invented. If OOO is an
>>>> issue for these protocols then they need to be fixed-- this is not a
>>>> concern with IP protocol nor the stack.
>>>>
>>>> Tom
>>>
>>> As far as a little background on the original patch I believe the
>>> issue that was fixed by the patch was a video streaming application
>>> that was sending/receiving a mix of fragmented and non-fragmented
>>> packets. Receiving them out of order due to the fragmentation was
>>> causing issues with stutters in the video and so we ended up disabling
>>> UDP by default in the NICs listed. We decided to go that way as UDP
>>> RSS was viewed as a performance optimization, while the out-of-order
>>> problems were viewed as a functionality issue.
>>>
>> Hi Alex,
>>
>> Thanks for the details! Were you able to find the root cause for this?
>> In particular, it would be interesting to know if it is the kernel or
>> device that introduced the jitter, or if it's the application that
>> doesn't handle OOO well...
>>
>> Tom
>
> It is hard to say since my memory of the events from 7 years ago is
> pretty vague at this point, but I'm pretty sure it was the
> application. Basically getting the frames out of order was causing
> them to have to drop video data if I recall correctly.
>
Oh, I didn't notice that patch was from 2010. Maybe the application
has been fixed by now! :-)
Perhaps, it's time to try to turn UDP hashing on again by default?
Even if NICs aren't doing this, there are network devices that are
looking at UDP for ECMP and that doesn't seem to be causing widespread
problems currently...
Tom
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Mason @ 2017-09-06 19:14 UTC (permalink / raw)
To: David Daney, Florian Fainelli
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard
In-Reply-To: <ff070239-28b7-d41b-8abe-c9f810561372@caviumnetworks.com>
On 06/09/2017 20:00, David Daney wrote:
> On 08/31/2017 11:29 AM, Florian Fainelli wrote:
>> On 08/31/2017 11:12 AM, Mason wrote:
>>> On 31/08/2017 19:53, Florian Fainelli wrote:
>>>> On 08/31/2017 10:49 AM, Mason wrote:
>>>>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>>>>> And the race is between phy_detach() setting phydev->attached_dev = NULL
>>>>>> and phy_state_machine() running in PHY_HALTED state and calling
>>>>>> netif_carrier_off().
>>>>>
>>>>> I must be missing something.
>>>>> (Since a thread cannot race against itself.)
>>>>>
>>>>> phy_disconnect calls phy_stop_machine which
>>>>> 1) stops the work queue from running in a separate thread
>>>>> 2) calls phy_state_machine *synchronously*
>>>>> which runs the PHY_HALTED case with everything well-defined
>>>>> end of phy_stop_machine
>>>>>
>>>>> phy_disconnect only then calls phy_detach()
>>>>> which makes future calls of phy_state_machine perilous.
>>>>>
>>>>> This all happens in the same thread, so I'm not yet
>>>>> seeing where the race happens?
>>>>
>>>> The race is as described in David's earlier email, so let's recap:
>>>>
>>>> Thread 1 Thread 2
>>>> phy_disconnect()
>>>> phy_stop_interrupts()
>>>> phy_stop_machine()
>>>> phy_state_machine()
>>>> -> queue_delayed_work()
>>>> phy_detach()
>>>> phy_state_machine()
>>>> -> netif_carrier_off()
>>>>
>>>> If phy_detach() finishes earlier than the workqueue had a chance to be
>>>> scheduled and process PHY_HALTED again, then we trigger the NULL pointer
>>>> de-reference.
>>>>
>>>> workqueues are not tasklets, the CPU scheduling them gets no guarantee
>>>> they will run on the same CPU.
>>>
>>> Something does not add up.
>>>
>>> The synchronous call to phy_state_machine() does:
>>>
>>> case PHY_HALTED:
>>> if (phydev->link) {
>>> phydev->link = 0;
>>> netif_carrier_off(phydev->attached_dev);
>>> phy_adjust_link(phydev);
>>> do_suspend = true;
>>> }
>>>
>>> then sets phydev->link = 0; therefore subsequent calls to
>>> phy_state_machin() will be no-op.
>>
>> Actually you are right, once phydev->link is set to 0 these would become
>> no-ops. Still scratching my head as to what happens for David then...
>>
>>>
>>> Also, queue_delayed_work() is only called in polling mode.
>>> David stated that he's using interrupt mode.
>
> Did you see what I wrote?
>
> phy_disconnect() calls phy_stop_interrupts() which puts it into polling
> mode. So the polling work gets queued unconditionally.
I did address that remark in
https://www.mail-archive.com/netdev@vger.kernel.org/msg186336.html
int phy_stop_interrupts(struct phy_device *phydev)
{
int err = phy_disable_interrupts(phydev);
if (err)
phy_error(phydev);
free_irq(phydev->irq, phydev);
/* If work indeed has been cancelled, disable_irq() will have
* been left unbalanced from phy_interrupt() and enable_irq()
* has to be called so that other devices on the line work.
*/
while (atomic_dec_return(&phydev->irq_disable) >= 0)
enable_irq(phydev->irq);
return err;
}
Which part of this function changes phydev->irq to PHY_POLL?
Perhaps phydev->drv->config_intr?
What PHY are you using?
Regards.
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Florian Fainelli @ 2017-09-06 19:28 UTC (permalink / raw)
To: Mason
Cc: Marc Gonzalez, David Daney, netdev, Geert Uytterhoeven,
David Miller, Andrew Lunn, Mans Rullgard, Thibaud Cornic
In-Reply-To: <927413e9-4f1f-963c-2d3a-5a88de2eac9e@free.fr>
On 09/06/2017 07:55 AM, Mason wrote:
> On 31/08/2017 21:18, Florian Fainelli wrote:
>
>> On 08/31/2017 12:09 PM, Mason wrote:
>>
>>> 1) nb8800_link_reconfigure() calls phy_print_status()
>>> which prints the "Link down" and "Link up" messages
>>> to the console. With the patch reverted, nothing is
>>> printed when the link goes down, and the result is
>>> random when the link comes up. Sometimes, we get
>>> down + up, sometimes just up.
>>
>> Nothing printed when you bring down the network interface as a result of
>> not signaling the link down, there is a small nuance here.
>
> Let me first focus on the "Link down" message.
>
> Do you agree that such a message should be printed when the
> link goes down, not when the link comes up?
The question is not so much about printing the message rather than a)
having the adjust_link callback be called and b) having this particular
callback correctly determine if a "change" has occurred, but let's focus
on the notification too. Printing the message is a consequence of these
two conditions and that's what matters.
There is not unfortunately a hard and fast answer it's clearly a
philosophical problem here.
The link is not physically down, the cable is still plugged so
generating a link down even is not necessarily correct. It would be
convenient for network manager programs, just like it is for network
drivers to treat it as such because that allows them to act like if the
cable was unplugged, which may be a good way to perform a number of
actions including but not limited to: entering a low power state,
re-initialization parts of the Ethernet MAC that need it (DMA, etc.,
etc.). That does not appear to be an absolute requirement for most, if
not all drivers since it changed after 3.4 and no one did bat an eye
about it.
Upon bringing the interface back up again, same thing, if the cable was
not disconnected should we just generate a link UP event, and if we do
that, are we going to confuse any network manager application?
Generating a link transition DOWN -> UP is certainly helpful for any
network application in that they do not need to keep any state just like
it clearly indicates a change was detected.
>
> Perhaps the issue is that the 2 following cases need to be
> handled differently:
> A) operator sets link down on the command-line
This is already handled differently since when you administratively
bring down an interface you call ndo_stop() which will be doing a
phy_stop() + phy_disconnect() which result in stopping the PHY state
machine and disconnecting from the PHY.
> B) asynchronous event makes link go down (peer is dead, cable is cut, etc)
>
> In B) the PHY state machine keeps on running, and eventually
> calls adjust_link()
Correct.
>
> In A) the driver calls phy_stop() and phy_disconnect() and
> therefore adjust_link() will not be called?
That is the current behavior (after the revert) and we can always change
it if deemed necessary, problem is, this broke for two people (one still
being discussed as of right now), so at this point I am very wary of
making any changes without more testing. I really need to get one of
these PHY interrupts wired to one of my boards or create a software
model of such a configuration before accepting new changes in that area.
Thank you
--
Florian
^ permalink raw reply
* [wireless-testsing2:master 4/5] drivers/net//ethernet/marvell/mvpp2.c:7618:2: warning: passing argument 4 of 'mvpp2_port_copy_mac_addr' from incompatible pointer type
From: kbuild test robot @ 2017-09-06 19:30 UTC (permalink / raw)
To: David S. Miller; +Cc: kbuild-all, netdev, Bob Copeland
[-- Attachment #1: Type: text/plain, Size: 16010 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-testing.git master
head: d17be7f7503bf9492198a242779a68af93fd92de
commit: 96c03618031bae5e9068b16f9e437b79f98f6482 [4/5] Merge remote-tracking branch 'mac80211-next/master'
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 96c03618031bae5e9068b16f9e437b79f98f6482
# save the attached .config to linux build tree
make.cross ARCH=xtensa
All warnings (new ones prefixed by >>):
drivers/net//ethernet/marvell/mvpp2.c: In function 'mvpp2_port_probe':
>> drivers/net//ethernet/marvell/mvpp2.c:7618:2: warning: passing argument 4 of 'mvpp2_port_copy_mac_addr' from incompatible pointer type
mvpp2_port_copy_mac_addr(dev, priv, port_node, &mac_from);
^
drivers/net//ethernet/marvell/mvpp2.c:7468:13: note: expected 'char **' but argument is of type 'const char **'
static void mvpp2_port_copy_mac_addr(struct net_device *dev, struct mvpp2 *priv,
^
drivers/net//ethernet/marvell/mvpp2.c:7509:7: warning: unused variable 'hw_mac_addr' [-Wunused-variable]
char hw_mac_addr[ETH_ALEN] = {0};
^
drivers/net//ethernet/marvell/mvpp2.c:7507:14: warning: unused variable 'dt_mac_addr' [-Wunused-variable]
const char *dt_mac_addr;
^
vim +/mvpp2_port_copy_mac_addr +7618 drivers/net//ethernet/marvell/mvpp2.c
3ba8c81e1 Antoine Tenart 2017-09-02 7495
3f518509d Marcin Wojtas 2014-07-10 7496 /* Ports initialization */
3f518509d Marcin Wojtas 2014-07-10 7497 static int mvpp2_port_probe(struct platform_device *pdev,
3f518509d Marcin Wojtas 2014-07-10 7498 struct device_node *port_node,
59b9a31ed Thomas Petazzoni 2017-03-07 7499 struct mvpp2 *priv)
3f518509d Marcin Wojtas 2014-07-10 7500 {
3f518509d Marcin Wojtas 2014-07-10 7501 struct device_node *phy_node;
542897d98 Antoine Tenart 2017-08-30 7502 struct phy *comphy;
3f518509d Marcin Wojtas 2014-07-10 7503 struct mvpp2_port *port;
edc660fa0 Marcin Wojtas 2015-08-06 7504 struct mvpp2_port_pcpu *port_pcpu;
3f518509d Marcin Wojtas 2014-07-10 7505 struct net_device *dev;
3f518509d Marcin Wojtas 2014-07-10 7506 struct resource *res;
3f518509d Marcin Wojtas 2014-07-10 7507 const char *dt_mac_addr;
96c036180 Bob Copeland 2017-09-04 7508 const char *mac_from = "";
4c2286826 Antoine Tenart 2017-08-25 7509 char hw_mac_addr[ETH_ALEN] = {0};
09f839755 Thomas Petazzoni 2017-08-03 7510 unsigned int ntxqs, nrxqs;
213f428f5 Thomas Petazzoni 2017-08-03 7511 bool has_tx_irqs;
3f518509d Marcin Wojtas 2014-07-10 7512 u32 id;
3f518509d Marcin Wojtas 2014-07-10 7513 int features;
3f518509d Marcin Wojtas 2014-07-10 7514 int phy_mode;
edc660fa0 Marcin Wojtas 2015-08-06 7515 int err, i, cpu;
3f518509d Marcin Wojtas 2014-07-10 7516
213f428f5 Thomas Petazzoni 2017-08-03 7517 has_tx_irqs = mvpp2_port_has_tx_irqs(priv, port_node);
213f428f5 Thomas Petazzoni 2017-08-03 7518
213f428f5 Thomas Petazzoni 2017-08-03 7519 if (!has_tx_irqs)
213f428f5 Thomas Petazzoni 2017-08-03 7520 queue_mode = MVPP2_QDIST_SINGLE_MODE;
213f428f5 Thomas Petazzoni 2017-08-03 7521
09f839755 Thomas Petazzoni 2017-08-03 7522 ntxqs = MVPP2_MAX_TXQ;
213f428f5 Thomas Petazzoni 2017-08-03 7523 if (priv->hw_version == MVPP22 && queue_mode == MVPP2_QDIST_MULTI_MODE)
213f428f5 Thomas Petazzoni 2017-08-03 7524 nrxqs = MVPP2_DEFAULT_RXQ * num_possible_cpus();
213f428f5 Thomas Petazzoni 2017-08-03 7525 else
09f839755 Thomas Petazzoni 2017-08-03 7526 nrxqs = MVPP2_DEFAULT_RXQ;
09f839755 Thomas Petazzoni 2017-08-03 7527
09f839755 Thomas Petazzoni 2017-08-03 7528 dev = alloc_etherdev_mqs(sizeof(*port), ntxqs, nrxqs);
3f518509d Marcin Wojtas 2014-07-10 7529 if (!dev)
3f518509d Marcin Wojtas 2014-07-10 7530 return -ENOMEM;
3f518509d Marcin Wojtas 2014-07-10 7531
3f518509d Marcin Wojtas 2014-07-10 7532 phy_node = of_parse_phandle(port_node, "phy", 0);
3f518509d Marcin Wojtas 2014-07-10 7533 phy_mode = of_get_phy_mode(port_node);
3f518509d Marcin Wojtas 2014-07-10 7534 if (phy_mode < 0) {
3f518509d Marcin Wojtas 2014-07-10 7535 dev_err(&pdev->dev, "incorrect phy mode\n");
3f518509d Marcin Wojtas 2014-07-10 7536 err = phy_mode;
3f518509d Marcin Wojtas 2014-07-10 7537 goto err_free_netdev;
3f518509d Marcin Wojtas 2014-07-10 7538 }
3f518509d Marcin Wojtas 2014-07-10 7539
542897d98 Antoine Tenart 2017-08-30 7540 comphy = devm_of_phy_get(&pdev->dev, port_node, NULL);
542897d98 Antoine Tenart 2017-08-30 7541 if (IS_ERR(comphy)) {
542897d98 Antoine Tenart 2017-08-30 7542 if (PTR_ERR(comphy) == -EPROBE_DEFER) {
542897d98 Antoine Tenart 2017-08-30 7543 err = -EPROBE_DEFER;
542897d98 Antoine Tenart 2017-08-30 7544 goto err_free_netdev;
542897d98 Antoine Tenart 2017-08-30 7545 }
542897d98 Antoine Tenart 2017-08-30 7546 comphy = NULL;
542897d98 Antoine Tenart 2017-08-30 7547 }
542897d98 Antoine Tenart 2017-08-30 7548
3f518509d Marcin Wojtas 2014-07-10 7549 if (of_property_read_u32(port_node, "port-id", &id)) {
3f518509d Marcin Wojtas 2014-07-10 7550 err = -EINVAL;
3f518509d Marcin Wojtas 2014-07-10 7551 dev_err(&pdev->dev, "missing port-id value\n");
3f518509d Marcin Wojtas 2014-07-10 7552 goto err_free_netdev;
3f518509d Marcin Wojtas 2014-07-10 7553 }
3f518509d Marcin Wojtas 2014-07-10 7554
3f518509d Marcin Wojtas 2014-07-10 7555 dev->tx_queue_len = MVPP2_MAX_TXD;
3f518509d Marcin Wojtas 2014-07-10 7556 dev->watchdog_timeo = 5 * HZ;
3f518509d Marcin Wojtas 2014-07-10 7557 dev->netdev_ops = &mvpp2_netdev_ops;
3f518509d Marcin Wojtas 2014-07-10 7558 dev->ethtool_ops = &mvpp2_eth_tool_ops;
3f518509d Marcin Wojtas 2014-07-10 7559
3f518509d Marcin Wojtas 2014-07-10 7560 port = netdev_priv(dev);
591f4cfab Thomas Petazzoni 2017-08-03 7561 port->dev = dev;
09f839755 Thomas Petazzoni 2017-08-03 7562 port->ntxqs = ntxqs;
09f839755 Thomas Petazzoni 2017-08-03 7563 port->nrxqs = nrxqs;
213f428f5 Thomas Petazzoni 2017-08-03 7564 port->priv = priv;
213f428f5 Thomas Petazzoni 2017-08-03 7565 port->has_tx_irqs = has_tx_irqs;
3f518509d Marcin Wojtas 2014-07-10 7566
591f4cfab Thomas Petazzoni 2017-08-03 7567 err = mvpp2_queue_vectors_init(port, port_node);
591f4cfab Thomas Petazzoni 2017-08-03 7568 if (err)
3f518509d Marcin Wojtas 2014-07-10 7569 goto err_free_netdev;
3f518509d Marcin Wojtas 2014-07-10 7570
fd3651b2a Antoine Tenart 2017-09-01 7571 port->link_irq = of_irq_get_byname(port_node, "link");
fd3651b2a Antoine Tenart 2017-09-01 7572 if (port->link_irq == -EPROBE_DEFER) {
fd3651b2a Antoine Tenart 2017-09-01 7573 err = -EPROBE_DEFER;
fd3651b2a Antoine Tenart 2017-09-01 7574 goto err_deinit_qvecs;
fd3651b2a Antoine Tenart 2017-09-01 7575 }
fd3651b2a Antoine Tenart 2017-09-01 7576 if (port->link_irq <= 0)
fd3651b2a Antoine Tenart 2017-09-01 7577 /* the link irq is optional */
fd3651b2a Antoine Tenart 2017-09-01 7578 port->link_irq = 0;
fd3651b2a Antoine Tenart 2017-09-01 7579
3f518509d Marcin Wojtas 2014-07-10 7580 if (of_property_read_bool(port_node, "marvell,loopback"))
3f518509d Marcin Wojtas 2014-07-10 7581 port->flags |= MVPP2_F_LOOPBACK;
3f518509d Marcin Wojtas 2014-07-10 7582
3f518509d Marcin Wojtas 2014-07-10 7583 port->id = id;
59b9a31ed Thomas Petazzoni 2017-03-07 7584 if (priv->hw_version == MVPP21)
09f839755 Thomas Petazzoni 2017-08-03 7585 port->first_rxq = port->id * port->nrxqs;
59b9a31ed Thomas Petazzoni 2017-03-07 7586 else
59b9a31ed Thomas Petazzoni 2017-03-07 7587 port->first_rxq = port->id * priv->max_port_rxqs;
59b9a31ed Thomas Petazzoni 2017-03-07 7588
3f518509d Marcin Wojtas 2014-07-10 7589 port->phy_node = phy_node;
3f518509d Marcin Wojtas 2014-07-10 7590 port->phy_interface = phy_mode;
542897d98 Antoine Tenart 2017-08-30 7591 port->comphy = comphy;
3f518509d Marcin Wojtas 2014-07-10 7592
a786841df Thomas Petazzoni 2017-03-07 7593 if (priv->hw_version == MVPP21) {
a786841df Thomas Petazzoni 2017-03-07 7594 res = platform_get_resource(pdev, IORESOURCE_MEM, 2 + id);
3f518509d Marcin Wojtas 2014-07-10 7595 port->base = devm_ioremap_resource(&pdev->dev, res);
3f518509d Marcin Wojtas 2014-07-10 7596 if (IS_ERR(port->base)) {
3f518509d Marcin Wojtas 2014-07-10 7597 err = PTR_ERR(port->base);
fd3651b2a Antoine Tenart 2017-09-01 7598 goto err_free_irq;
3f518509d Marcin Wojtas 2014-07-10 7599 }
a786841df Thomas Petazzoni 2017-03-07 7600 } else {
a786841df Thomas Petazzoni 2017-03-07 7601 if (of_property_read_u32(port_node, "gop-port-id",
a786841df Thomas Petazzoni 2017-03-07 7602 &port->gop_id)) {
a786841df Thomas Petazzoni 2017-03-07 7603 err = -EINVAL;
a786841df Thomas Petazzoni 2017-03-07 7604 dev_err(&pdev->dev, "missing gop-port-id value\n");
591f4cfab Thomas Petazzoni 2017-08-03 7605 goto err_deinit_qvecs;
a786841df Thomas Petazzoni 2017-03-07 7606 }
a786841df Thomas Petazzoni 2017-03-07 7607
a786841df Thomas Petazzoni 2017-03-07 7608 port->base = priv->iface_base + MVPP22_GMAC_BASE(port->gop_id);
a786841df Thomas Petazzoni 2017-03-07 7609 }
3f518509d Marcin Wojtas 2014-07-10 7610
3f518509d Marcin Wojtas 2014-07-10 7611 /* Alloc per-cpu stats */
3f518509d Marcin Wojtas 2014-07-10 7612 port->stats = netdev_alloc_pcpu_stats(struct mvpp2_pcpu_stats);
3f518509d Marcin Wojtas 2014-07-10 7613 if (!port->stats) {
3f518509d Marcin Wojtas 2014-07-10 7614 err = -ENOMEM;
fd3651b2a Antoine Tenart 2017-09-01 7615 goto err_free_irq;
3f518509d Marcin Wojtas 2014-07-10 7616 }
3f518509d Marcin Wojtas 2014-07-10 7617
3ba8c81e1 Antoine Tenart 2017-09-02 @7618 mvpp2_port_copy_mac_addr(dev, priv, port_node, &mac_from);
3f518509d Marcin Wojtas 2014-07-10 7619
3f518509d Marcin Wojtas 2014-07-10 7620 port->tx_ring_size = MVPP2_MAX_TXD;
3f518509d Marcin Wojtas 2014-07-10 7621 port->rx_ring_size = MVPP2_MAX_RXD;
3f518509d Marcin Wojtas 2014-07-10 7622 SET_NETDEV_DEV(dev, &pdev->dev);
3f518509d Marcin Wojtas 2014-07-10 7623
3f518509d Marcin Wojtas 2014-07-10 7624 err = mvpp2_port_init(port);
3f518509d Marcin Wojtas 2014-07-10 7625 if (err < 0) {
3f518509d Marcin Wojtas 2014-07-10 7626 dev_err(&pdev->dev, "failed to init port %d\n", id);
3f518509d Marcin Wojtas 2014-07-10 7627 goto err_free_stats;
3f518509d Marcin Wojtas 2014-07-10 7628 }
269758214 Thomas Petazzoni 2017-03-07 7629
269758214 Thomas Petazzoni 2017-03-07 7630 mvpp2_port_periodic_xon_disable(port);
269758214 Thomas Petazzoni 2017-03-07 7631
269758214 Thomas Petazzoni 2017-03-07 7632 if (priv->hw_version == MVPP21)
269758214 Thomas Petazzoni 2017-03-07 7633 mvpp2_port_fc_adv_enable(port);
269758214 Thomas Petazzoni 2017-03-07 7634
269758214 Thomas Petazzoni 2017-03-07 7635 mvpp2_port_reset(port);
3f518509d Marcin Wojtas 2014-07-10 7636
edc660fa0 Marcin Wojtas 2015-08-06 7637 port->pcpu = alloc_percpu(struct mvpp2_port_pcpu);
edc660fa0 Marcin Wojtas 2015-08-06 7638 if (!port->pcpu) {
edc660fa0 Marcin Wojtas 2015-08-06 7639 err = -ENOMEM;
edc660fa0 Marcin Wojtas 2015-08-06 7640 goto err_free_txq_pcpu;
edc660fa0 Marcin Wojtas 2015-08-06 7641 }
edc660fa0 Marcin Wojtas 2015-08-06 7642
213f428f5 Thomas Petazzoni 2017-08-03 7643 if (!port->has_tx_irqs) {
edc660fa0 Marcin Wojtas 2015-08-06 7644 for_each_present_cpu(cpu) {
edc660fa0 Marcin Wojtas 2015-08-06 7645 port_pcpu = per_cpu_ptr(port->pcpu, cpu);
edc660fa0 Marcin Wojtas 2015-08-06 7646
edc660fa0 Marcin Wojtas 2015-08-06 7647 hrtimer_init(&port_pcpu->tx_done_timer, CLOCK_MONOTONIC,
edc660fa0 Marcin Wojtas 2015-08-06 7648 HRTIMER_MODE_REL_PINNED);
edc660fa0 Marcin Wojtas 2015-08-06 7649 port_pcpu->tx_done_timer.function = mvpp2_hr_timer_cb;
edc660fa0 Marcin Wojtas 2015-08-06 7650 port_pcpu->timer_scheduled = false;
edc660fa0 Marcin Wojtas 2015-08-06 7651
213f428f5 Thomas Petazzoni 2017-08-03 7652 tasklet_init(&port_pcpu->tx_done_tasklet,
213f428f5 Thomas Petazzoni 2017-08-03 7653 mvpp2_tx_proc_cb,
edc660fa0 Marcin Wojtas 2015-08-06 7654 (unsigned long)dev);
edc660fa0 Marcin Wojtas 2015-08-06 7655 }
213f428f5 Thomas Petazzoni 2017-08-03 7656 }
edc660fa0 Marcin Wojtas 2015-08-06 7657
186cd4d4e Antoine Tenart 2017-08-23 7658 features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
3f518509d Marcin Wojtas 2014-07-10 7659 dev->features = features | NETIF_F_RXCSUM;
3f518509d Marcin Wojtas 2014-07-10 7660 dev->hw_features |= features | NETIF_F_RXCSUM | NETIF_F_GRO;
3f518509d Marcin Wojtas 2014-07-10 7661 dev->vlan_features |= features;
3f518509d Marcin Wojtas 2014-07-10 7662
5777987e0 Jarod Wilson 2016-10-17 7663 /* MTU range: 68 - 9676 */
5777987e0 Jarod Wilson 2016-10-17 7664 dev->min_mtu = ETH_MIN_MTU;
5777987e0 Jarod Wilson 2016-10-17 7665 /* 9676 == 9700 - 20 and rounding to 8 */
5777987e0 Jarod Wilson 2016-10-17 7666 dev->max_mtu = 9676;
5777987e0 Jarod Wilson 2016-10-17 7667
3f518509d Marcin Wojtas 2014-07-10 7668 err = register_netdev(dev);
3f518509d Marcin Wojtas 2014-07-10 7669 if (err < 0) {
3f518509d Marcin Wojtas 2014-07-10 7670 dev_err(&pdev->dev, "failed to register netdev\n");
edc660fa0 Marcin Wojtas 2015-08-06 7671 goto err_free_port_pcpu;
3f518509d Marcin Wojtas 2014-07-10 7672 }
3f518509d Marcin Wojtas 2014-07-10 7673 netdev_info(dev, "Using %s mac address %pM\n", mac_from, dev->dev_addr);
3f518509d Marcin Wojtas 2014-07-10 7674
3f518509d Marcin Wojtas 2014-07-10 7675 priv->port_list[id] = port;
3f518509d Marcin Wojtas 2014-07-10 7676 return 0;
3f518509d Marcin Wojtas 2014-07-10 7677
edc660fa0 Marcin Wojtas 2015-08-06 7678 err_free_port_pcpu:
edc660fa0 Marcin Wojtas 2015-08-06 7679 free_percpu(port->pcpu);
3f518509d Marcin Wojtas 2014-07-10 7680 err_free_txq_pcpu:
09f839755 Thomas Petazzoni 2017-08-03 7681 for (i = 0; i < port->ntxqs; i++)
3f518509d Marcin Wojtas 2014-07-10 7682 free_percpu(port->txqs[i]->pcpu);
3f518509d Marcin Wojtas 2014-07-10 7683 err_free_stats:
3f518509d Marcin Wojtas 2014-07-10 7684 free_percpu(port->stats);
fd3651b2a Antoine Tenart 2017-09-01 7685 err_free_irq:
fd3651b2a Antoine Tenart 2017-09-01 7686 if (port->link_irq)
fd3651b2a Antoine Tenart 2017-09-01 7687 irq_dispose_mapping(port->link_irq);
591f4cfab Thomas Petazzoni 2017-08-03 7688 err_deinit_qvecs:
591f4cfab Thomas Petazzoni 2017-08-03 7689 mvpp2_queue_vectors_deinit(port);
3f518509d Marcin Wojtas 2014-07-10 7690 err_free_netdev:
ccb80393c Peter Chen 2016-08-01 7691 of_node_put(phy_node);
3f518509d Marcin Wojtas 2014-07-10 7692 free_netdev(dev);
3f518509d Marcin Wojtas 2014-07-10 7693 return err;
3f518509d Marcin Wojtas 2014-07-10 7694 }
3f518509d Marcin Wojtas 2014-07-10 7695
:::::: The code at line 7618 was first introduced by commit
:::::: 3ba8c81e15c11fc396d0b5d11adaf9db6ed39533 net: mvpp2: move the mac retrieval/copy logic into its own function
:::::: TO: Antoine Tenart <antoine.tenart@free-electrons.com>
:::::: CC: David S. Miller <davem@davemloft.net>
---
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: 51119 bytes --]
^ permalink raw reply
* RE: [PATCH] vsock: only load vmci transport on VMware hypervisor by default
From: Dexuan Cui @ 2017-09-06 19:39 UTC (permalink / raw)
To: Jorgen S. Hansen, Stefan Hajnoczi
Cc: Michal Kubecek, joe@perches.com, olaf@aepfle.de,
Stephen Hemminger, jasowang@redhat.com, netdev@vger.kernel.org,
Haiyang Zhang, Dave Scott, apw@canonical.com,
linux-kernel@vger.kernel.org, Vitaly Kuznetsov, Rolf Neugebauer,
gregkh@linuxfoundation.org, Marcelo Cerri,
devel@linuxdriverproject.org, Asias He, davem@davemloft.net,
George Zhang, Dan Carpenter
In-Reply-To: <8E47FDFD-809F-4EE9-9068-65BE50BE8BEB@vmware.com>
> From: Jorgen S. Hansen [mailto:jhansen@vmware.com]
> Sent: Wednesday, September 6, 2017 7:11 AM
>> ...
> > I'm currently working on NFS over AF_VSOCK and sock_diag support (for
> > ss(8) and netstat-like tools).
> >
> > Multi-transport support is lower priority for me at the moment. I'm
> > happy to review patches though. If there is no progress on this by the
> > end of the year then I will have time to work on it.
> >
>
> I’ll try to find time to write a more coherent proposal in the coming weeks,
> and we can discuss that.
>
> Jorgen
Thank you!
Thanks,
-- Dexuan
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Florian Fainelli @ 2017-09-06 19:42 UTC (permalink / raw)
To: Mason, Andrew Lunn
Cc: Marc Gonzalez, David Daney, netdev, Geert Uytterhoeven,
David Miller, Mans Rullgard, Thibaud Cornic
In-Reply-To: <730292be-affa-c19d-75ab-edba367788e8@free.fr>
On 09/06/2017 08:51 AM, Mason wrote:
> On 31/08/2017 21:18, Florian Fainelli wrote:
>
>> On 08/31/2017 12:09 PM, Mason wrote:
>>
>>> On 31/08/2017 19:03, Florian Fainelli wrote:
>>>
>>>> On 08/31/2017 05:29 AM, Marc Gonzalez wrote:
>>>>
>>>>> On 31/08/2017 02:49, Florian Fainelli wrote:
>>>>>
>>>>>> The original motivation for this change originated from Marc Gonzalez
>>>>>> indicating that his network driver did not have its adjust_link
>>>>>> callback
>>>>>> executing with phydev->link = 0 while he was expecting it.
>>>>>
>>>>> I expect the core to call phy_adjust_link() for link changes.
>>>>> This used to work back in 3.4 and was broken somewhere along
>>>>> the way.
>>>>
>>>> If that was working correctly in 3.4 surely we can look at the diff and
>>>> figure out what changed, even maybe find the offending commit, can you
>>>> do that?
>>>
>>> Bisecting would a be a huge pain because my platform was
>>> not upstream until v4.4
>>
>> Then just diff the file and try to pinpoint which commit may have
>> changed that?
>
> Running 'ip link set eth0 down' on the command-line.
>
> In v3.4 => adjust_link() callback is called
> In v4.5 => adjust_link() callback is NOT called
>
> $ git log --oneline --no-merges v3.4..v4.5 drivers/net/phy/phy.c | wc -l
> 59
>
> I'm not sure what "just diff the file" entails.
git log -p --no-merges v3.4..v4.5 drivers/net/phy/{phy,phy_device.c} and
see what would seem remotely sensible to what you are observing.
> I can't move 3.4 up, nor move 4.5 down.
You can always copy the PHYLIB files at any given commit back into an
older tree, or vice versa because it is largely self contained with
little to no dependencies on other headers/files/facilities etc. This is
not convenient I agree, but it's a poor man's way of determining what
changed within PHYLIB that results in what you are seeing.
AFAICT you could use QEMU with the versatile board that has smsc911x as
an Ethernet adapter which is PHYLIB compliant which may be used to
pinpoint which commit start changing this behavior. It's long, it's painful.
> I'm not even sure the problem comes from drivers/net/phy/phy.c
> to be honest.
If that's the case then I am not sure what else we can do.
--
Florian
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Florian Fainelli @ 2017-09-06 19:54 UTC (permalink / raw)
To: Andrew Lunn
Cc: Roopa Prabhu, netdev, Vivien Didelot, Woojung.Huh, jbe, sean.wang,
john
In-Reply-To: <20170906164217.GE15315@lunn.ch>
On 09/06/2017 09:42 AM, Andrew Lunn wrote:
>>> On the switch asics we work with, the driver has information if the
>>> packet was
>>> forwarded in hardware. This is per packet reason code telling why the
>>> CPU is seeing the packet.
>>> The driver can use this information to reset skb->offload_fwd_mark to
>>> allow software forward.
>
>> I am not positive this is universally available across different
>> switch vendors.
>
> It is not universally available. We cannot rely on it being available
> with switches supported by DSA.
>
> We have a few choices:
>
> 1) We assume anything the switch forwards to the CPU has also been
> sent out whatever ports of the switch it needs to. Set
> offload_fwd_mark.
>
> 2) We assume anything the switch forwards to the CPU has not gone
> anywhere else, and the bridge needs to send it out whatever ports
> it thinks. Don't set offload_fwd_mark.
>
> 3) We define some rules about what packets the switch should handle,
> and then do some deep packet inspection to decide if
> offload_fwd_mark should be set or not.
>
> I don't see 3) being possible. We are dealing with a fixed silicon
> data path, not something which is fully programmable.
>
> So it is down to 1) or 2). I've been assuming 1), but maybe we need to
> discuss that as well.
At the very least we should probably move the skb->offload_fwd_mark
setting down into the individual taggers since they should be in a
better position to set it or not based on the switch device they are
driving, this should address, on a per-switch basis whether 2) or 3)
applies to a given switch.
That being said, I have a feeling that the Marvell switches behave a
tiny bit differently than others in that they do not flood broadcast by
default in a given L2 domain.
On b53/bcm_sf2 there is the ability to disable the reception of
broadcast frames on the management/CPU port, and while there is the
ability to configure which ports should be flooded in case of
unicast/multicast lookup failures, I don't see anything for Broadcast,
so I am assuming this will get forwarded by default. Will test with your
patch set later on time permitting.
--
Florian
^ permalink raw reply
* Re: [net-next:master 491/511] xt_hashlimit.c:undefined reference to `__aeabi_uldivmod'
From: Arnd Bergmann @ 2017-09-06 19:57 UTC (permalink / raw)
To: kbuild test robot; +Cc: kbuild-all, Networking
In-Reply-To: <201709062318.dBly2cas%fengguang.wu@intel.com>
On Wed, Sep 6, 2017 at 5:27 PM, kbuild test robot
<fengguang.wu@intel.com> wrote:
> Hi Arnd,
>
> It's probably a bug fix that unveils the link errors.
Correct.
> net/netfilter/xt_hashlimit.o: In function `hashlimit_mt_common':
>>> xt_hashlimit.c:(.text+0x1f68): undefined reference to `__aeabi_uldivmod'
I actually wrote a patch for that yesterday, but it seems I forgot to submit it.
I've sent it now.
Arnd
^ permalink raw reply
* [PATCH] netfilter: xt_hashlimit: avoid 64-bit division
From: Arnd Bergmann @ 2017-09-06 19:57 UTC (permalink / raw)
To: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
David S. Miller
Cc: Arnd Bergmann, Vishwanath Pai, Josh Hunt, netfilter-devel,
coreteam, netdev, linux-kernel
64-bit division is expensive on 32-bit architectures, and
requires a special function call to avoid a link error like:
net/netfilter/xt_hashlimit.o: In function `hashlimit_mt_common':
xt_hashlimit.c:(.text+0x1328): undefined reference to `__aeabi_uldivmod'
In the case of hashlimit_mt_common, we don't actually need a
64-bit operation, we can simply rewrite the function slightly
to make that clear to the compiler.
Fixes: bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
net/netfilter/xt_hashlimit.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 10d48234f5f4..50b53d86eef5 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -531,7 +531,10 @@ static u64 user2rate_bytes(u64 user)
{
u64 r;
- r = user ? 0xFFFFFFFFULL / user : 0xFFFFFFFFULL;
+ if (user > 0xFFFFFFFFULL)
+ return 0;
+
+ r = user ? 0xFFFFFFFFULL / (u32)user : 0xFFFFFFFFULL;
r = (r - 1) << 4;
return r;
}
--
2.9.0
^ permalink raw reply related
* Re: [PATCH] netfilter: xt_hashlimit: avoid 64-bit division
From: Vishwanath Pai @ 2017-09-06 20:22 UTC (permalink / raw)
To: Arnd Bergmann, Pablo Neira Ayuso, Jozsef Kadlecsik,
Florian Westphal, David S. Miller
Cc: Josh Hunt, netfilter-devel, coreteam, netdev, linux-kernel
In-Reply-To: <20170906195825.3715290-1-arnd@arndb.de>
On 09/06/2017 03:57 PM, Arnd Bergmann wrote:
> 64-bit division is expensive on 32-bit architectures, and
> requires a special function call to avoid a link error like:
>
> net/netfilter/xt_hashlimit.o: In function `hashlimit_mt_common':
> xt_hashlimit.c:(.text+0x1328): undefined reference to `__aeabi_uldivmod'
>
> In the case of hashlimit_mt_common, we don't actually need a
> 64-bit operation, we can simply rewrite the function slightly
> to make that clear to the compiler.
>
> Fixes: bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> net/netfilter/xt_hashlimit.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> index 10d48234f5f4..50b53d86eef5 100644
> --- a/net/netfilter/xt_hashlimit.c
> +++ b/net/netfilter/xt_hashlimit.c
> @@ -531,7 +531,10 @@ static u64 user2rate_bytes(u64 user)
> {
> u64 r;
>
> - r = user ? 0xFFFFFFFFULL / user : 0xFFFFFFFFULL;
> + if (user > 0xFFFFFFFFULL)
> + return 0;
> +
> + r = user ? 0xFFFFFFFFULL / (u32)user : 0xFFFFFFFFULL;
> r = (r - 1) << 4;
> return r;
> }
>
I have submitted another patch to fix this:
https://patchwork.ozlabs.org/patch/809881/
We have seen this problem before, I was careful not to introduce this
again in the new patch but clearly I overlooked this particular line :(
In the other cases we fixed it by replacing division with div64_u64().
-Vishwanath
^ permalink raw reply
* [PATCH 13/14] netfilter/ipvs: Use %pS printk format for direct addresses
From: Helge Deller @ 2017-09-06 20:28 UTC (permalink / raw)
To: linux-kernel
Cc: Sergey Senozhatsky, Petr Mladek, Andrew Morton, Wensong Zhang,
netdev, lvs-devel, netfilter-devel
In-Reply-To: <1504729681-3504-1-git-send-email-deller@gmx.de>
The debug and error printk functions in ipvs uses wrongly the %pF instead of
the %pS printk format specifier for printing symbols for the address returned
by _builtin_return_address(0). Fix it for the ia64, ppc64 and parisc64
architectures.
Signed-off-by: Helge Deller <deller@gmx.de>
Cc: Wensong Zhang <wensong@linux-vs.org>
Cc: netdev@vger.kernel.org
Cc: lvs-devel@vger.kernel.org
Cc: netfilter-devel@vger.kernel.org
---
net/netfilter/ipvs/ip_vs_conn.c | 2 +-
net/netfilter/ipvs/ip_vs_ctl.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
index 3d2ac71a..f73561c 100644
--- a/net/netfilter/ipvs/ip_vs_conn.c
+++ b/net/netfilter/ipvs/ip_vs_conn.c
@@ -185,7 +185,7 @@ static inline int ip_vs_conn_hash(struct ip_vs_conn *cp)
hlist_add_head_rcu(&cp->c_list, &ip_vs_conn_tab[hash]);
ret = 1;
} else {
- pr_err("%s(): request for already hashed, called from %pF\n",
+ pr_err("%s(): request for already hashed, called from %pS\n",
__func__, __builtin_return_address(0));
ret = 0;
}
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
index 1fa3c23..88fc58a 100644
--- a/net/netfilter/ipvs/ip_vs_ctl.c
+++ b/net/netfilter/ipvs/ip_vs_ctl.c
@@ -300,7 +300,7 @@ static int ip_vs_svc_hash(struct ip_vs_service *svc)
unsigned int hash;
if (svc->flags & IP_VS_SVC_F_HASHED) {
- pr_err("%s(): request for already hashed, called from %pF\n",
+ pr_err("%s(): request for already hashed, called from %pS\n",
__func__, __builtin_return_address(0));
return 0;
}
@@ -334,7 +334,7 @@ static int ip_vs_svc_hash(struct ip_vs_service *svc)
static int ip_vs_svc_unhash(struct ip_vs_service *svc)
{
if (!(svc->flags & IP_VS_SVC_F_HASHED)) {
- pr_err("%s(): request for unhash flagged, called from %pF\n",
+ pr_err("%s(): request for unhash flagged, called from %pS\n",
__func__, __builtin_return_address(0));
return 0;
}
--
2.1.0
^ permalink raw reply related
* Re: [patch net] net: sched: fix memleak for chain zero
From: Jiri Pirko @ 2017-09-06 20:33 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim,
Jakub Kicinski, mlxsw
In-Reply-To: <CAM_iQpWZJ9VVD66h6RMNofobKyoFyztx2jYqBEHuE6_iAu7PNQ@mail.gmail.com>
Wed, Sep 06, 2017 at 07:40:02PM CEST, xiyou.wangcong@gmail.com wrote:
>On Wed, Sep 6, 2017 at 4:14 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> There's a memleak happening for chain 0. The thing is, chain 0 needs to
>> be always present, not created on demand. Therefore tcf_block_get upon
>> creation of block calls the tcf_chain_create function directly. The
>> chain is created with refcnt == 1, which is not correct in this case and
>> causes the memleak. So move the refcnt increment into tcf_chain_get
>> function even for the case when chain needs to be created.
>>
>
>Your approach could work but you just make the code even
>uglier than it is now:
>
>1. The current code is already ugly for special-casing chain 0:
>
> if (--chain->refcnt == 0 && !chain->filter_chain && chain->index != 0)
> tcf_chain_destroy(chain);
>
>2. With your patch, chain 0 has a different _initial_ refcnt with others.
No. Initial refcnt is the same. ! for every action that holds the chain.
So actually, it returns it back where it should be.
>
>3. Allowing an object (chain 0) exists with refcnt==0
So? That is for every chain that does not have goto_chain action
pointing at. Please read the code.
>
>Compare it with my patch:
>
>1. No special-case for chain 0, the above ugly part is removed
>
>2. Every chain is equal and created with refcnt==1
>
>3. Any chain with refcnt==0 is destroyed
^ permalink raw reply
* Re: [PATCH] netfilter: xt_hashlimit: avoid 64-bit division
From: Arnd Bergmann @ 2017-09-06 20:48 UTC (permalink / raw)
To: Vishwanath Pai
Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
David S. Miller, Josh Hunt, netfilter-devel, coreteam, Networking,
Linux Kernel Mailing List
In-Reply-To: <7791dee1-64a9-dcaa-c5d9-0b04a44bc526@akamai.com>
On Wed, Sep 6, 2017 at 10:22 PM, Vishwanath Pai <vpai@akamai.com> wrote:
> On 09/06/2017 03:57 PM, Arnd Bergmann wrote:
>> 64-bit division is expensive on 32-bit architectures, and
>> requires a special function call to avoid a link error like:
>>
>> net/netfilter/xt_hashlimit.o: In function `hashlimit_mt_common':
>> xt_hashlimit.c:(.text+0x1328): undefined reference to `__aeabi_uldivmod'
>>
>> In the case of hashlimit_mt_common, we don't actually need a
>> 64-bit operation, we can simply rewrite the function slightly
>> to make that clear to the compiler.
>>
>> Fixes: bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> net/netfilter/xt_hashlimit.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
>> index 10d48234f5f4..50b53d86eef5 100644
>> --- a/net/netfilter/xt_hashlimit.c
>> +++ b/net/netfilter/xt_hashlimit.c
>> @@ -531,7 +531,10 @@ static u64 user2rate_bytes(u64 user)
>> {
>> u64 r;
>>
>> - r = user ? 0xFFFFFFFFULL / user : 0xFFFFFFFFULL;
>> + if (user > 0xFFFFFFFFULL)
>> + return 0;
>> +
>> + r = user ? 0xFFFFFFFFULL / (u32)user : 0xFFFFFFFFULL;
>> r = (r - 1) << 4;
>> return r;
>> }
>>
>
> I have submitted another patch to fix this:
> https://patchwork.ozlabs.org/patch/809881/
>
> We have seen this problem before, I was careful not to introduce this
> again in the new patch but clearly I overlooked this particular line :(
>
> In the other cases we fixed it by replacing division with div64_u64().
div64_u64() seems needlessly expensive here since the dividend
is known to be a 32-bit number. I guess the function is not called
frequently though, so it doesn't matter much.
Arnd
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: David Daney @ 2017-09-06 20:49 UTC (permalink / raw)
To: Florian Fainelli, Mason
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard
In-Reply-To: <572f49fd-f623-f064-a551-e243c57cef7f@gmail.com>
On 09/06/2017 11:59 AM, Florian Fainelli wrote:
> On 09/06/2017 11:00 AM, David Daney wrote:
>> On 08/31/2017 11:29 AM, Florian Fainelli wrote:
>>> On 08/31/2017 11:12 AM, Mason wrote:
>>>> On 31/08/2017 19:53, Florian Fainelli wrote:
>>>>> On 08/31/2017 10:49 AM, Mason wrote:
>>>>>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>>>>>> And the race is between phy_detach() setting phydev->attached_dev
>>>>>>> = NULL
>>>>>>> and phy_state_machine() running in PHY_HALTED state and calling
>>>>>>> netif_carrier_off().
>>>>>>
>>>>>> I must be missing something.
>>>>>> (Since a thread cannot race against itself.)
>>>>>>
>>>>>> phy_disconnect calls phy_stop_machine which
>>>>>> 1) stops the work queue from running in a separate thread
>>>>>> 2) calls phy_state_machine *synchronously*
>>>>>> which runs the PHY_HALTED case with everything well-defined
>>>>>> end of phy_stop_machine
>>>>>>
>>>>>> phy_disconnect only then calls phy_detach()
>>>>>> which makes future calls of phy_state_machine perilous.
>>>>>>
>>>>>> This all happens in the same thread, so I'm not yet
>>>>>> seeing where the race happens?
>>>>>
>>>>> The race is as described in David's earlier email, so let's recap:
>>>>>
>>>>> Thread 1 Thread 2
>>>>> phy_disconnect()
>>>>> phy_stop_interrupts()
>>>>> phy_stop_machine()
>>>>> phy_state_machine()
>>>>> -> queue_delayed_work()
>>>>> phy_detach()
>>>>> phy_state_machine()
>>>>> -> netif_carrier_off()
>>>>>
>>>>> If phy_detach() finishes earlier than the workqueue had a chance to be
>>>>> scheduled and process PHY_HALTED again, then we trigger the NULL
>>>>> pointer
>>>>> de-reference.
>>>>>
>>>>> workqueues are not tasklets, the CPU scheduling them gets no guarantee
>>>>> they will run on the same CPU.
>>>>
>>>> Something does not add up.
>>>>
>>>> The synchronous call to phy_state_machine() does:
>>>>
>>>> case PHY_HALTED:
>>>> if (phydev->link) {
>>>> phydev->link = 0;
>>>> netif_carrier_off(phydev->attached_dev);
>>>> phy_adjust_link(phydev);
>>>> do_suspend = true;
>>>> }
>>>>
>>>> then sets phydev->link = 0; therefore subsequent calls to
>>>> phy_state_machin() will be no-op.
>>>
>>> Actually you are right, once phydev->link is set to 0 these would become
>>> no-ops. Still scratching my head as to what happens for David then...
>>>
>>>>
>>>> Also, queue_delayed_work() is only called in polling mode.
>>>> David stated that he's using interrupt mode.
>>
>> Did you see what I wrote?
>
> Still not following, see below.
>
>>
>> phy_disconnect() calls phy_stop_interrupts() which puts it into polling
>> mode. So the polling work gets queued unconditionally.
>
> What part of phy_stop_interrupts() is responsible for changing
> phydev->irq to PHY_POLL? free_irq() cannot touch phydev->irq otherwise
> subsequent request_irq() calls won't work anymore.
> phy_disable_interrupts() only calls back into the PHY driver to
> acknowledge and clear interrupts.
>
> If we were using a PHY with PHY_POLL, as Marc said, the first
> synchronous call to phy_state_machine() would have acted on PHY_HALTED
> and even if we incorrectly keep re-scheduling the state machine from
> PHY_HALTED to PHY_HALTED the second time around nothing can happen.
>
> What are we missing here?
>
OK, I am now as confused as you guys are. I will go back and get an
ftrace log out of the failure.
David.
^ permalink raw reply
* [PATCH v2 net-next 0/2] hv_netvsc: sub channel initialization fixes
From: Stephen Hemminger @ 2017-09-06 20:53 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
One serious deadlock, and one minor optimization.
Stephen Hemminger (2):
hv_netvsc: fix deadlock on hotplug
hv_netvsc: avoid unnecessary wakeups on subchannel creation
drivers/net/hyperv/hyperv_net.h | 3 +
drivers/net/hyperv/netvsc.c | 3 +
drivers/net/hyperv/netvsc_drv.c | 11 +---
drivers/net/hyperv/rndis_filter.c | 126 ++++++++++++++++++++++++++------------
4 files changed, 96 insertions(+), 47 deletions(-)
--
2.11.0
^ permalink raw reply
* [PATCH v2 net-next 1/2] hv_netvsc: fix deadlock on hotplug
From: Stephen Hemminger @ 2017-09-06 20:53 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
In-Reply-To: <20170906205306.10541-1-sthemmin@microsoft.com>
When a virtual device is added dynamically (via host console), then
the vmbus sends an offer message for the primary channel. The processing
of this message for networking causes the network device to then
initialize the sub channels.
The problem is that setting up the sub channels needs to wait until
the subsequent subchannel offers have been processed. These offers
come in on the same ring buffer and work queue as where the primary
offer is being processed; leading to a deadlock.
This did not happen in older kernels, because the sub channel waiting
logic was broken (it wasn't really waiting).
The solution is to do the sub channel setup in its own work queue
context that is scheduled by the primary channel setup; and then
happens later.
Fixes: 732e49850c5e ("netvsc: fix race on sub channel creation")
Reported-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
v2 - fix module removal race with new work queue
drivers/net/hyperv/hyperv_net.h | 3 +
drivers/net/hyperv/netvsc.c | 3 +
drivers/net/hyperv/netvsc_drv.c | 11 +---
drivers/net/hyperv/rndis_filter.c | 122 ++++++++++++++++++++++++++------------
4 files changed, 94 insertions(+), 45 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index ec546da86683..d98cdfb1536b 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -204,6 +204,8 @@ int netvsc_recv_callback(struct net_device *net,
const struct ndis_pkt_8021q_info *vlan);
void netvsc_channel_cb(void *context);
int netvsc_poll(struct napi_struct *napi, int budget);
+
+void rndis_set_subchannel(struct work_struct *w);
bool rndis_filter_opened(const struct netvsc_device *nvdev);
int rndis_filter_open(struct netvsc_device *nvdev);
int rndis_filter_close(struct netvsc_device *nvdev);
@@ -782,6 +784,7 @@ struct netvsc_device {
u32 num_chn;
atomic_t open_chn;
+ struct work_struct subchan_work;
wait_queue_head_t subchan_open;
struct rndis_device *extension;
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 0062b802676f..a5511b7326af 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -81,6 +81,7 @@ static struct netvsc_device *alloc_net_device(void)
init_completion(&net_device->channel_init_wait);
init_waitqueue_head(&net_device->subchan_open);
+ INIT_WORK(&net_device->subchan_work, rndis_set_subchannel);
return net_device;
}
@@ -557,6 +558,8 @@ void netvsc_device_remove(struct hv_device *device)
= rtnl_dereference(net_device_ctx->nvdev);
int i;
+ cancel_work_sync(&net_device->subchan_work);
+
netvsc_disconnect_vsp(device);
RCU_INIT_POINTER(net_device_ctx->nvdev, NULL);
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 165ba4b3b423..c538a4f15f3b 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -853,10 +853,7 @@ static int netvsc_set_channels(struct net_device *net,
rndis_filter_device_remove(dev, nvdev);
nvdev = rndis_filter_device_add(dev, &device_info);
- if (!IS_ERR(nvdev)) {
- netif_set_real_num_tx_queues(net, nvdev->num_chn);
- netif_set_real_num_rx_queues(net, nvdev->num_chn);
- } else {
+ if (IS_ERR(nvdev)) {
ret = PTR_ERR(nvdev);
device_info.num_chn = orig;
nvdev = rndis_filter_device_add(dev, &device_info);
@@ -1954,9 +1951,6 @@ static int netvsc_probe(struct hv_device *dev,
NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX;
net->vlan_features = net->features;
- netif_set_real_num_tx_queues(net, nvdev->num_chn);
- netif_set_real_num_rx_queues(net, nvdev->num_chn);
-
netdev_lockdep_set_classes(net);
/* MTU range: 68 - 1500 or 65521 */
@@ -2012,9 +2006,10 @@ static int netvsc_remove(struct hv_device *dev)
if (vf_netdev)
netvsc_unregister_vf(vf_netdev);
+ unregister_netdevice(net);
+
rndis_filter_device_remove(dev,
rtnl_dereference(ndev_ctx->nvdev));
- unregister_netdevice(net);
rtnl_unlock();
hv_set_drvdata(dev, NULL);
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index 69c40b8fccc3..731bc7cc6f43 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -1039,8 +1039,6 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
/* Set the channel before opening.*/
nvchan->channel = new_sc;
- netif_napi_add(ndev, &nvchan->napi,
- netvsc_poll, NAPI_POLL_WEIGHT);
ret = vmbus_open(new_sc, nvscdev->ring_size * PAGE_SIZE,
nvscdev->ring_size * PAGE_SIZE, NULL, 0,
@@ -1048,12 +1046,88 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
if (ret == 0)
napi_enable(&nvchan->napi);
else
- netif_napi_del(&nvchan->napi);
+ netdev_notice(ndev, "sub channel open failed: %d\n", ret);
atomic_inc(&nvscdev->open_chn);
wake_up(&nvscdev->subchan_open);
}
+/* Open sub-channels after completing the handling of the device probe.
+ * This breaks overlap of processing the host message for the
+ * new primary channel with the initialization of sub-channels.
+ */
+void rndis_set_subchannel(struct work_struct *w)
+{
+ struct netvsc_device *nvdev
+ = container_of(w, struct netvsc_device, subchan_work);
+ struct nvsp_message *init_packet = &nvdev->channel_init_pkt;
+ struct net_device_context *ndev_ctx;
+ struct rndis_device *rdev;
+ struct net_device *ndev;
+ struct hv_device *hv_dev;
+ int i, ret;
+
+ if (!rtnl_trylock()) {
+ schedule_work(w);
+ return;
+ }
+
+ rdev = nvdev->extension;
+ if (!rdev)
+ goto unlock; /* device was removed */
+
+ ndev = rdev->ndev;
+ ndev_ctx = netdev_priv(ndev);
+ hv_dev = ndev_ctx->device_ctx;
+
+ memset(init_packet, 0, sizeof(struct nvsp_message));
+ init_packet->hdr.msg_type = NVSP_MSG5_TYPE_SUBCHANNEL;
+ init_packet->msg.v5_msg.subchn_req.op = NVSP_SUBCHANNEL_ALLOCATE;
+ init_packet->msg.v5_msg.subchn_req.num_subchannels =
+ nvdev->num_chn - 1;
+ ret = vmbus_sendpacket(hv_dev->channel, init_packet,
+ sizeof(struct nvsp_message),
+ (unsigned long)init_packet,
+ VM_PKT_DATA_INBAND,
+ VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+ if (ret) {
+ netdev_err(ndev, "sub channel allocate send failed: %d\n", ret);
+ goto failed;
+ }
+
+ wait_for_completion(&nvdev->channel_init_wait);
+ if (init_packet->msg.v5_msg.subchn_comp.status != NVSP_STAT_SUCCESS) {
+ netdev_err(ndev, "sub channel request failed\n");
+ goto failed;
+ }
+
+ nvdev->num_chn = 1 +
+ init_packet->msg.v5_msg.subchn_comp.num_subchannels;
+
+ /* wait for all sub channels to open */
+ wait_event(nvdev->subchan_open,
+ atomic_read(&nvdev->open_chn) == nvdev->num_chn);
+
+ /* ignore failues from setting rss parameters, still have channels */
+ rndis_filter_set_rss_param(rdev, netvsc_hash_key);
+
+ netif_set_real_num_tx_queues(ndev, nvdev->num_chn);
+ netif_set_real_num_rx_queues(ndev, nvdev->num_chn);
+
+ rtnl_unlock();
+ return;
+
+failed:
+ /* fallback to only primary channel */
+ for (i = 1; i < nvdev->num_chn; i++)
+ netif_napi_del(&nvdev->chan_table[i].napi);
+
+ nvdev->max_chn = 1;
+ nvdev->num_chn = 1;
+unlock:
+ rtnl_unlock();
+}
+
struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
struct netvsc_device_info *device_info)
{
@@ -1063,7 +1137,6 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
struct rndis_device *rndis_device;
struct ndis_offload hwcaps;
struct ndis_offload_params offloads;
- struct nvsp_message *init_packet;
struct ndis_recv_scale_cap rsscap;
u32 rsscap_size = sizeof(struct ndis_recv_scale_cap);
unsigned int gso_max_size = GSO_MAX_SIZE;
@@ -1215,9 +1288,7 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
net_device->num_chn);
atomic_set(&net_device->open_chn, 1);
-
- if (net_device->num_chn == 1)
- return net_device;
+ vmbus_set_sc_create_callback(dev->channel, netvsc_sc_open);
for (i = 1; i < net_device->num_chn; i++) {
ret = netvsc_alloc_recv_comp_ring(net_device, i);
@@ -1228,38 +1299,15 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
}
}
- vmbus_set_sc_create_callback(dev->channel, netvsc_sc_open);
+ for (i = 1; i < net_device->num_chn; i++)
+ netif_napi_add(net, &net_device->chan_table[i].napi,
+ netvsc_poll, NAPI_POLL_WEIGHT);
- init_packet = &net_device->channel_init_pkt;
- memset(init_packet, 0, sizeof(struct nvsp_message));
- init_packet->hdr.msg_type = NVSP_MSG5_TYPE_SUBCHANNEL;
- init_packet->msg.v5_msg.subchn_req.op = NVSP_SUBCHANNEL_ALLOCATE;
- init_packet->msg.v5_msg.subchn_req.num_subchannels =
- net_device->num_chn - 1;
- ret = vmbus_sendpacket(dev->channel, init_packet,
- sizeof(struct nvsp_message),
- (unsigned long)init_packet,
- VM_PKT_DATA_INBAND,
- VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
- if (ret)
- goto out;
-
- wait_for_completion(&net_device->channel_init_wait);
- if (init_packet->msg.v5_msg.subchn_comp.status != NVSP_STAT_SUCCESS) {
- ret = -ENODEV;
- goto out;
- }
+ if (net_device->num_chn > 1)
+ schedule_work(&net_device->subchan_work);
- net_device->num_chn = 1 +
- init_packet->msg.v5_msg.subchn_comp.num_subchannels;
-
- /* wait for all sub channels to open */
- wait_event(net_device->subchan_open,
- atomic_read(&net_device->open_chn) == net_device->num_chn);
-
- /* ignore failues from setting rss parameters, still have channels */
- rndis_filter_set_rss_param(rndis_device, netvsc_hash_key);
out:
+ /* if unavailable, just proceed with one queue */
if (ret) {
net_device->max_chn = 1;
net_device->num_chn = 1;
@@ -1280,10 +1328,10 @@ void rndis_filter_device_remove(struct hv_device *dev,
/* Halt and release the rndis device */
rndis_filter_halt_device(rndis_dev);
- kfree(rndis_dev);
net_dev->extension = NULL;
netvsc_device_remove(dev);
+ kfree(rndis_dev);
}
int rndis_filter_open(struct netvsc_device *nvdev)
--
2.11.0
^ permalink raw reply related
* [PATCH v2 net-next 2/2] hv_netvsc: avoid unnecessary wakeups on subchannel creation
From: Stephen Hemminger @ 2017-09-06 20:53 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
In-Reply-To: <20170906205306.10541-1-sthemmin@microsoft.com>
Only need to wakeup the initiator after all sub-channels
are opened.
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
drivers/net/hyperv/rndis_filter.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index 731bc7cc6f43..065b204d8e17 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -1048,8 +1048,8 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
else
netdev_notice(ndev, "sub channel open failed: %d\n", ret);
- atomic_inc(&nvscdev->open_chn);
- wake_up(&nvscdev->subchan_open);
+ if (atomic_inc_return(&nvscdev->open_chn) == nvscdev->num_chn)
+ wake_up(&nvscdev->subchan_open);
}
/* Open sub-channels after completing the handling of the device probe.
--
2.11.0
^ permalink raw reply related
* [PATCH net] hv_netvsc: fix deadlock on hotplug
From: Stephen Hemminger @ 2017-09-06 21:11 UTC (permalink / raw)
To: davem; +Cc: haiyangz, netdev, Stephen Hemminger
When a virtual device is added dynamically (via host console), then
the vmbus sends an offer message for the primary channel. The processing
of this message for networking causes the network device to then
initialize the sub channels.
The problem is that setting up the sub channels needs to wait until
the subsequent subchannel offers have been processed. These offers
come in on the same ring buffer and work queue as where the primary
offer is being processed; leading to a deadlock.
This did not happen in older kernels, because the sub channel waiting
logic was broken (it wasn't really waiting).
The solution is to do the sub channel setup in its own work queue
context that is scheduled by the primary channel setup; and then
happens later.
Fixes: 732e49850c5e ("netvsc: fix race on sub channel creation")
Reported-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
This is version for 4.13 (linux-net)
drivers/net/hyperv/hyperv_net.h | 3 +
drivers/net/hyperv/netvsc.c | 3 +
drivers/net/hyperv/netvsc_drv.c | 18 +-----
drivers/net/hyperv/rndis_filter.c | 118 +++++++++++++++++++++++++++-----------
4 files changed, 94 insertions(+), 48 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 12cc64bfcff8..b82119be14e9 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -200,6 +200,8 @@ int netvsc_recv_callback(struct net_device *net,
const struct ndis_pkt_8021q_info *vlan);
void netvsc_channel_cb(void *context);
int netvsc_poll(struct napi_struct *napi, int budget);
+
+void rndis_set_subchannel(struct work_struct *w);
int rndis_filter_open(struct netvsc_device *nvdev);
int rndis_filter_close(struct netvsc_device *nvdev);
int rndis_filter_device_add(struct hv_device *dev,
@@ -766,6 +768,7 @@ struct netvsc_device {
u32 num_chn;
atomic_t open_chn;
+ struct work_struct subchan_work;
wait_queue_head_t subchan_open;
struct rndis_device *extension;
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index d18c3326a1f7..76ef14cf55a1 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -79,6 +79,7 @@ static struct netvsc_device *alloc_net_device(void)
net_device->pkt_align = RNDIS_PKT_ALIGN_DEFAULT;
init_completion(&net_device->channel_init_wait);
init_waitqueue_head(&net_device->subchan_open);
+ INIT_WORK(&net_device->subchan_work, rndis_set_subchannel);
return net_device;
}
@@ -553,6 +554,8 @@ void netvsc_device_remove(struct hv_device *device)
struct netvsc_device *net_device = net_device_ctx->nvdev;
int i;
+ cancel_work_sync(&net_device->subchan_work);
+
netvsc_disconnect_vsp(device);
RCU_INIT_POINTER(net_device_ctx->nvdev, NULL);
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index d91cbc6c3ca4..9c8dd86a663e 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -740,24 +740,13 @@ static int netvsc_set_queues(struct net_device *net, struct hv_device *dev,
u32 num_chn)
{
struct netvsc_device_info device_info;
- int ret;
memset(&device_info, 0, sizeof(device_info));
device_info.num_chn = num_chn;
device_info.ring_size = ring_size;
device_info.max_num_vrss_chns = num_chn;
- ret = rndis_filter_device_add(dev, &device_info);
- if (ret)
- return ret;
-
- ret = netif_set_real_num_tx_queues(net, num_chn);
- if (ret)
- return ret;
-
- ret = netif_set_real_num_rx_queues(net, num_chn);
-
- return ret;
+ return rndis_filter_device_add(dev, &device_info);
}
static int netvsc_set_channels(struct net_device *net,
@@ -1573,8 +1562,6 @@ static int netvsc_probe(struct hv_device *dev,
/* RCU not necessary here, device not registered */
nvdev = net_device_ctx->nvdev;
- netif_set_real_num_tx_queues(net, nvdev->num_chn);
- netif_set_real_num_rx_queues(net, nvdev->num_chn);
/* MTU range: 68 - 1500 or 65521 */
net->min_mtu = NETVSC_MTU_MIN;
@@ -1616,11 +1603,10 @@ static int netvsc_remove(struct hv_device *dev)
* removed. Also blocks mtu and channel changes.
*/
rtnl_lock();
+ unregister_netdevice(net);
rndis_filter_device_remove(dev, ndev_ctx->nvdev);
rtnl_unlock();
- unregister_netdev(net);
-
hv_set_drvdata(dev, NULL);
free_netdev(net);
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index d6308ffda53e..015a8e2b733c 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -1037,8 +1037,6 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
/* Set the channel before opening.*/
nvchan->channel = new_sc;
- netif_napi_add(ndev, &nvchan->napi,
- netvsc_poll, NAPI_POLL_WEIGHT);
ret = vmbus_open(new_sc, nvscdev->ring_size * PAGE_SIZE,
nvscdev->ring_size * PAGE_SIZE, NULL, 0,
@@ -1046,12 +1044,90 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
if (ret == 0)
napi_enable(&nvchan->napi);
else
- netif_napi_del(&nvchan->napi);
+ netdev_notice(ndev, "sub channel open failed: %d\n", ret);
atomic_inc(&nvscdev->open_chn);
wake_up(&nvscdev->subchan_open);
}
+
+/* Open sub-channels after completing the handling of the device probe.
+ * This breaks overlap of processing the host message for the
+ * new primary channel with the initialization of sub-channels.
+ */
+void rndis_set_subchannel(struct work_struct *w)
+{
+ struct netvsc_device *nvdev
+ = container_of(w, struct netvsc_device, subchan_work);
+ struct nvsp_message *init_packet = &nvdev->channel_init_pkt;
+ struct net_device_context *ndev_ctx;
+ struct rndis_device *rdev;
+ struct net_device *ndev;
+ struct hv_device *hv_dev;
+ int i, ret;
+
+ if (!rtnl_trylock()) {
+ schedule_work(w);
+ return;
+ }
+
+ rdev = nvdev->extension;
+ if (!rdev)
+ goto unlock; /* device was removed */
+
+ ndev = rdev->ndev;
+ ndev_ctx = netdev_priv(ndev);
+ hv_dev = ndev_ctx->device_ctx;
+
+ memset(init_packet, 0, sizeof(struct nvsp_message));
+ init_packet->hdr.msg_type = NVSP_MSG5_TYPE_SUBCHANNEL;
+ init_packet->msg.v5_msg.subchn_req.op = NVSP_SUBCHANNEL_ALLOCATE;
+ init_packet->msg.v5_msg.subchn_req.num_subchannels =
+ nvdev->num_chn - 1;
+ ret = vmbus_sendpacket(hv_dev->channel, init_packet,
+ sizeof(struct nvsp_message),
+ (unsigned long)init_packet,
+ VM_PKT_DATA_INBAND,
+ VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+ if (ret) {
+ netdev_err(ndev, "sub channel allocate send failed: %d\n", ret);
+ goto failed;
+ }
+
+ wait_for_completion(&nvdev->channel_init_wait);
+ if (init_packet->msg.v5_msg.subchn_comp.status != NVSP_STAT_SUCCESS) {
+ netdev_err(ndev, "sub channel request failed\n");
+ goto failed;
+ }
+
+ nvdev->num_chn = 1 +
+ init_packet->msg.v5_msg.subchn_comp.num_subchannels;
+
+ /* wait for all sub channels to open */
+ wait_event(nvdev->subchan_open,
+ atomic_read(&nvdev->open_chn) == nvdev->num_chn);
+
+ /* ignore failues from setting rss parameters, still have channels */
+ rndis_filter_set_rss_param(rdev, netvsc_hash_key,
+ nvdev->num_chn);
+
+ netif_set_real_num_tx_queues(ndev, nvdev->num_chn);
+ netif_set_real_num_rx_queues(ndev, nvdev->num_chn);
+
+ rtnl_unlock();
+ return;
+
+failed:
+ /* fallback to only primary channel */
+ for (i = 1; i < nvdev->num_chn; i++)
+ netif_napi_del(&nvdev->chan_table[i].napi);
+
+ nvdev->max_chn = 1;
+ nvdev->num_chn = 1;
+unlock:
+ rtnl_unlock();
+}
+
int rndis_filter_device_add(struct hv_device *dev,
struct netvsc_device_info *device_info)
{
@@ -1061,7 +1137,6 @@ int rndis_filter_device_add(struct hv_device *dev,
struct rndis_device *rndis_device;
struct ndis_offload hwcaps;
struct ndis_offload_params offloads;
- struct nvsp_message *init_packet;
struct ndis_recv_scale_cap rsscap;
u32 rsscap_size = sizeof(struct ndis_recv_scale_cap);
unsigned int gso_max_size = GSO_MAX_SIZE;
@@ -1226,37 +1301,16 @@ int rndis_filter_device_add(struct hv_device *dev,
vmbus_set_sc_create_callback(dev->channel, netvsc_sc_open);
- init_packet = &net_device->channel_init_pkt;
- memset(init_packet, 0, sizeof(struct nvsp_message));
- init_packet->hdr.msg_type = NVSP_MSG5_TYPE_SUBCHANNEL;
- init_packet->msg.v5_msg.subchn_req.op = NVSP_SUBCHANNEL_ALLOCATE;
- init_packet->msg.v5_msg.subchn_req.num_subchannels =
- net_device->num_chn - 1;
- ret = vmbus_sendpacket(dev->channel, init_packet,
- sizeof(struct nvsp_message),
- (unsigned long)init_packet,
- VM_PKT_DATA_INBAND,
- VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
- if (ret)
- goto out;
- wait_for_completion(&net_device->channel_init_wait);
- if (init_packet->msg.v5_msg.subchn_comp.status != NVSP_STAT_SUCCESS) {
- ret = -ENODEV;
- goto out;
- }
+ for (i = 1; i < net_device->num_chn; i++)
+ netif_napi_add(net, &net_device->chan_table[i].napi,
+ netvsc_poll, NAPI_POLL_WEIGHT);
- net_device->num_chn = 1 +
- init_packet->msg.v5_msg.subchn_comp.num_subchannels;
-
- /* wait for all sub channels to open */
- wait_event(net_device->subchan_open,
- atomic_read(&net_device->open_chn) == net_device->num_chn);
+ if (net_device->num_chn > 1)
+ schedule_work(&net_device->subchan_work);
- /* ignore failues from setting rss parameters, still have channels */
- rndis_filter_set_rss_param(rndis_device, netvsc_hash_key,
- net_device->num_chn);
out:
+ /* if unavailable, just proceed with one queue */
if (ret) {
net_device->max_chn = 1;
net_device->num_chn = 1;
@@ -1277,10 +1331,10 @@ void rndis_filter_device_remove(struct hv_device *dev,
/* Halt and release the rndis device */
rndis_filter_halt_device(rndis_dev);
- kfree(rndis_dev);
net_dev->extension = NULL;
netvsc_device_remove(dev);
+ kfree(rndis_dev);
}
int rndis_filter_open(struct netvsc_device *nvdev)
--
2.11.0
^ permalink raw reply related
* Re: Fwd: DA850-evm MAC Address is random
From: Adam Ford @ 2017-09-06 21:41 UTC (permalink / raw)
To: Sekhar Nori; +Cc: Tony Lindgren, Grygorii Strashko, linux-omap, netdev
In-Reply-To: <1865772b-482e-042e-7c66-d33290f0d8fb@ti.com>
On Mon, Sep 4, 2017 at 11:42 PM, Sekhar Nori <nsekhar@ti.com> wrote:
> Hi Adam,
>
> On Wednesday 30 August 2017 11:08 AM, Sekhar Nori wrote:
>>> I wonder if U-Boot isn't pushing something to Linux because it doesn't
>>> appear to be running some of the da850 specific code even when I run
>>> linux-next. Can you tell me what verision of U-Boot you're using?
>>> Other than using davinci_all_defconfig, did you change the
>>> configuration at all?
>
>> I am using U-Boot 2017.01. Yes, the kernel was built using
>> davinci_all_defconfig and no other config change. Before booting kernel,
>> can you confirm that ethaddr is set in U-Boot environment? This is what
>> fdt_fixup_ethernet() reads to fixup the FDT before boot.
>>
>> Here is my complete boot log with environment variable dump.
>>
>> http://pastebin.ubuntu.com/25430265/
>
> Were you able to get rid of the random mac address problem?
Not yet. I haven't been able to rebuild Arago using TI's instructions
on the Wiki. I am not sure if it's a dependency issue or something
else. When I run Linux 4.13 using Buildroot as the rootfs, it does
not appear to run da850_evm_m25p80_notify_add(). I am going to
investigate whether or not da850_evm_init() is getting called. I was
wondering if you had some insight as to what calls that function? It
looks like it's defined as part of MACHINE_START(DAVINCI_DA850_EVM,
"DaVinci DA850/OMAP-L138/AM18x EVM"), but I don't know how it gets
called.
thanks
adam
>
> Thanks,
> Sekhar
^ permalink raw reply
* Re: [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode
From: Kosuke Tatsukawa @ 2017-09-06 22:45 UTC (permalink / raw)
To: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Reinis Rozitis
In-Reply-To: <17EC94B0A072C34B8DCF0D30AD16044A0296FEF5@BPXM09GP.gisp.nec.co.jp>
Reinis Rozitis reported that tlb_dynamic_lb was still 0 in balance-alb
mode even when using linux-4.12.10 kernels, which includes this patch.
It turned out that my previous patch only fixed the case when
balance-alb mode was specified as bonding module parameter, and not when
balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
common usage). In the latter case, tlb_dynamic_lb was set up according
to the default mode of the bonding interface, which happens to be
balance-rr.
I'll send another patch to address this case.
In the meantime, a workaround for this issue is to specify balance-tlb
mode as a module parameter for bonding. This will change the value of
tlb_balance_lb to 1, and make balance-alb work like pre-4.12 kernels.
Best regards.
> balance-alb mode used to have transmit dynamic load balancing feature
> enabled by default. However, transmit dynamic load balancing no longer
> works in balance-alb after commit 8b426dc54cf4 ("bonding: remove
> hardcoded value").
>
> Both balance-tlb and balance-alb use the function bond_do_alb_xmit() to
> send packets. This function uses the parameter tlb_dynamic_lb.
> tlb_dynamic_lb used to have the default value of 1 for balance-alb, but
> now the value is set to 0 except in balance-tlb.
>
> Re-enable transmit dyanmic load balancing by initializing tlb_dynamic_lb
> for balance-alb similar to balance-tlb.
>
> Signed-off-by: Kosuke Tatsukawa <tatsu@ab.jp.nec.com>
> Cc: stable@vger.kernel.org
> ---
> drivers/net/bonding/bond_main.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 14ff622..181839d 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4596,7 +4596,7 @@ static int bond_check_params(struct bond_params *params)
> }
> ad_user_port_key = valptr->value;
>
> - if (bond_mode == BOND_MODE_TLB) {
> + if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) {
> bond_opt_initstr(&newval, "default");
> valptr = bond_opt_parse(bond_opt_get(BOND_OPT_TLB_DYNAMIC_LB),
> &newval);
---
Kosuke TATSUKAWA | 1st Platform Software Division
| NEC Solution Innovators
| tatsu@ab.jp.nec.com
^ permalink raw reply
* [PATCH] net: bonding: Fix transmit load balancing in balance-alb mode if specified by sysfs
From: Kosuke Tatsukawa @ 2017-09-06 22:47 UTC (permalink / raw)
To: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Reinis Rozitis
Commit cbf5ecb30560 ("net: bonding: Fix transmit load balancing in
balance-alb mode") tried to fix transmit dynamic load balancing in
balance-alb mode, which wasn't working after commit 8b426dc54cf4
("bonding: remove hardcoded value").
It turned out that my previous patch only fixed the case when
balance-alb was specified as bonding module parameter, and not when
balance-alb mode was set using /sys/class/net/*/bonding/mode (the most
common usage). In the latter case, tlb_dynamic_lb was set up according
to the default mode of the bonding interface, which happens to be
balance-rr.
This additional patch addresses this issue by setting up tlb_dynamic_lb
to 1 if "mode" is set to balance-alb through the sysfs interface.
I didn't add code to change tlb_balance_lb back to the default value for
other modes, because "mode" is usually set up only once during
initialization, and it's not worthwhile to change the static variable
bonding_defaults in bond_main.c to a global variable just for this
purpose.
Commit 8b426dc54cf4 also changes the value of tlb_dynamic_lb for
balance-tlb mode if it is set up using the sysfs interface. I didn't
change that behavior, because the value of tlb_balance_lb can be changed
using the sysfs interface for balance-tlb, and I didn't like changing
the default value back and forth for balance-tlb.
As for balance-alb, /sys/class/net/*/bonding/tlb_balance_lb cannot be
written to. However, I think balance-alb with tlb_dynamic_lb set to 0
is not an intended usage, so there is little use making it writable at
this moment.
Fixes: 8b426dc54cf4 ("bonding: remove hardcoded value")
Reported-by: Reinis Rozitis <r@roze.lv>
Signed-off-by: Kosuke Tatsukawa <tatsu@ab.jp.nec.com>
Cc: stable@vger.kernel.org # v4.12+
---
drivers/net/bonding/bond_options.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index a12d603..5931aa2 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -754,6 +754,9 @@ static int bond_option_mode_set(struct bonding *bond,
bond->params.miimon);
}
+ if (newval->value == BOND_MODE_ALB)
+ bond->params.tlb_dynamic_lb = 1;
+
/* don't cache arp_validate between modes */
bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
bond->params.mode = newval->value;
^ permalink raw reply related
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: David Daney @ 2017-09-06 22:51 UTC (permalink / raw)
To: David Daney, Florian Fainelli, Mason
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard
In-Reply-To: <6890a27f-e87e-62c1-a676-e5ddf968adb6@caviumnetworks.com>
On 09/06/2017 01:49 PM, David Daney wrote:
> On 09/06/2017 11:59 AM, Florian Fainelli wrote:
>> On 09/06/2017 11:00 AM, David Daney wrote:
>>> On 08/31/2017 11:29 AM, Florian Fainelli wrote:
>>>> On 08/31/2017 11:12 AM, Mason wrote:
>>>>> On 31/08/2017 19:53, Florian Fainelli wrote:
>>>>>> On 08/31/2017 10:49 AM, Mason wrote:
>>>>>>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>>>>>>> And the race is between phy_detach() setting phydev->attached_dev
>>>>>>>> = NULL
>>>>>>>> and phy_state_machine() running in PHY_HALTED state and calling
>>>>>>>> netif_carrier_off().
>>>>>>>
>>>>>>> I must be missing something.
>>>>>>> (Since a thread cannot race against itself.)
>>>>>>>
>>>>>>> phy_disconnect calls phy_stop_machine which
>>>>>>> 1) stops the work queue from running in a separate thread
>>>>>>> 2) calls phy_state_machine *synchronously*
>>>>>>> which runs the PHY_HALTED case with everything well-defined
>>>>>>> end of phy_stop_machine
>>>>>>>
>>>>>>> phy_disconnect only then calls phy_detach()
>>>>>>> which makes future calls of phy_state_machine perilous.
>>>>>>>
>>>>>>> This all happens in the same thread, so I'm not yet
>>>>>>> seeing where the race happens?
>>>>>>
>>>>>> The race is as described in David's earlier email, so let's recap:
>>>>>>
>>>>>> Thread 1 Thread 2
>>>>>> phy_disconnect()
>>>>>> phy_stop_interrupts()
>>>>>> phy_stop_machine()
>>>>>> phy_state_machine()
>>>>>> -> queue_delayed_work()
>>>>>> phy_detach()
>>>>>> phy_state_machine()
>>>>>> -> netif_carrier_off()
>>>>>>
>>>>>> If phy_detach() finishes earlier than the workqueue had a chance
>>>>>> to be
>>>>>> scheduled and process PHY_HALTED again, then we trigger the NULL
>>>>>> pointer
>>>>>> de-reference.
>>>>>>
>>>>>> workqueues are not tasklets, the CPU scheduling them gets no
>>>>>> guarantee
>>>>>> they will run on the same CPU.
>>>>>
>>>>> Something does not add up.
>>>>>
>>>>> The synchronous call to phy_state_machine() does:
>>>>>
>>>>> case PHY_HALTED:
>>>>> if (phydev->link) {
>>>>> phydev->link = 0;
>>>>> netif_carrier_off(phydev->attached_dev);
>>>>> phy_adjust_link(phydev);
>>>>> do_suspend = true;
>>>>> }
>>>>>
>>>>> then sets phydev->link = 0; therefore subsequent calls to
>>>>> phy_state_machin() will be no-op.
>>>>
>>>> Actually you are right, once phydev->link is set to 0 these would
>>>> become
>>>> no-ops. Still scratching my head as to what happens for David then...
>>>>
>>>>>
>>>>> Also, queue_delayed_work() is only called in polling mode.
>>>>> David stated that he's using interrupt mode.
>>>
>>> Did you see what I wrote?
>>
>> Still not following, see below.
>>
>>>
>>> phy_disconnect() calls phy_stop_interrupts() which puts it into polling
>>> mode. So the polling work gets queued unconditionally.
>>
>> What part of phy_stop_interrupts() is responsible for changing
>> phydev->irq to PHY_POLL? free_irq() cannot touch phydev->irq otherwise
>> subsequent request_irq() calls won't work anymore.
>> phy_disable_interrupts() only calls back into the PHY driver to
>> acknowledge and clear interrupts.
>>
>> If we were using a PHY with PHY_POLL, as Marc said, the first
>> synchronous call to phy_state_machine() would have acted on PHY_HALTED
>> and even if we incorrectly keep re-scheduling the state machine from
>> PHY_HALTED to PHY_HALTED the second time around nothing can happen.
>>
>> What are we missing here?
>>
>
> OK, I am now as confused as you guys are. I will go back and get an
> ftrace log out of the failure.
>
OK, let's forget about the PHY_HALTED discussion.
Consider instead the case of a Marvell phy with no interrupts connected
on a v4.9.43 kernel, single CPU:
0) | phy_disconnect() {
0) | phy_stop_machine() {
0) | cancel_delayed_work_sync() {
0) + 23.986 us | } /* cancel_delayed_work_sync */
0) | phy_state_machine() {
0) | phy_start_aneg_priv() {
0) | marvell_config_aneg() {
0) ! 240.538 us | } /* marvell_config_aneg */
0) ! 244.971 us | } /* phy_start_aneg_priv */
0) | queue_delayed_work_on() {
0) + 18.016 us | } /* queue_delayed_work_on */
0) ! 268.184 us | } /* phy_state_machine */
0) ! 297.394 us | } /* phy_stop_machine */
0) | phy_detach() {
0) | phy_suspend() {
0) | phy_ethtool_get_wol() {
0) 0.677 us | } /* phy_ethtool_get_wol */
0) | genphy_suspend() {
0) + 71.250 us | } /* genphy_suspend */
0) + 74.197 us | } /* phy_suspend */
0) + 80.302 us | } /* phy_detach */
0) ! 380.072 us | } /* phy_disconnect */
.
.
.
0) | process_one_work() {
0) | find_worker_executing_work() {
0) 0.688 us | } /* find_worker_executing_work */
0) | set_work_pool_and_clear_pending() {
0) 0.734 us | } /* set_work_pool_and_clear_pending */
0) | phy_state_machine() {
0) | genphy_read_status() {
0) ! 205.721 us | } /* genphy_read_status */
0) | netif_carrier_off() {
0) | do_page_fault() {
The do_page_fault() at the end indicates the NULL pointer dereference.
That added call to phy_state_machine() turns the polling back on
unconditionally for a phy that should be disconnected. How is that correct?
David.
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Florian Fainelli @ 2017-09-06 23:14 UTC (permalink / raw)
To: David Daney, David Daney, Mason
Cc: Marc Gonzalez, netdev, Geert Uytterhoeven, David Miller,
Andrew Lunn, Mans Rullgard
In-Reply-To: <4a65e53c-f13b-9cc3-bffa-f2f2aae423b9@gmail.com>
On 09/06/2017 03:51 PM, David Daney wrote:
> On 09/06/2017 01:49 PM, David Daney wrote:
>> On 09/06/2017 11:59 AM, Florian Fainelli wrote:
>>> On 09/06/2017 11:00 AM, David Daney wrote:
>>>> On 08/31/2017 11:29 AM, Florian Fainelli wrote:
>>>>> On 08/31/2017 11:12 AM, Mason wrote:
>>>>>> On 31/08/2017 19:53, Florian Fainelli wrote:
>>>>>>> On 08/31/2017 10:49 AM, Mason wrote:
>>>>>>>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>>>>>>>> And the race is between phy_detach() setting phydev->attached_dev
>>>>>>>>> = NULL
>>>>>>>>> and phy_state_machine() running in PHY_HALTED state and calling
>>>>>>>>> netif_carrier_off().
>>>>>>>>
>>>>>>>> I must be missing something.
>>>>>>>> (Since a thread cannot race against itself.)
>>>>>>>>
>>>>>>>> phy_disconnect calls phy_stop_machine which
>>>>>>>> 1) stops the work queue from running in a separate thread
>>>>>>>> 2) calls phy_state_machine *synchronously*
>>>>>>>> which runs the PHY_HALTED case with everything well-defined
>>>>>>>> end of phy_stop_machine
>>>>>>>>
>>>>>>>> phy_disconnect only then calls phy_detach()
>>>>>>>> which makes future calls of phy_state_machine perilous.
>>>>>>>>
>>>>>>>> This all happens in the same thread, so I'm not yet
>>>>>>>> seeing where the race happens?
>>>>>>>
>>>>>>> The race is as described in David's earlier email, so let's recap:
>>>>>>>
>>>>>>> Thread 1 Thread 2
>>>>>>> phy_disconnect()
>>>>>>> phy_stop_interrupts()
>>>>>>> phy_stop_machine()
>>>>>>> phy_state_machine()
>>>>>>> -> queue_delayed_work()
>>>>>>> phy_detach()
>>>>>>> phy_state_machine()
>>>>>>> -> netif_carrier_off()
>>>>>>>
>>>>>>> If phy_detach() finishes earlier than the workqueue had a chance
>>>>>>> to be
>>>>>>> scheduled and process PHY_HALTED again, then we trigger the NULL
>>>>>>> pointer
>>>>>>> de-reference.
>>>>>>>
>>>>>>> workqueues are not tasklets, the CPU scheduling them gets no
>>>>>>> guarantee
>>>>>>> they will run on the same CPU.
>>>>>>
>>>>>> Something does not add up.
>>>>>>
>>>>>> The synchronous call to phy_state_machine() does:
>>>>>>
>>>>>> case PHY_HALTED:
>>>>>> if (phydev->link) {
>>>>>> phydev->link = 0;
>>>>>> netif_carrier_off(phydev->attached_dev);
>>>>>> phy_adjust_link(phydev);
>>>>>> do_suspend = true;
>>>>>> }
>>>>>>
>>>>>> then sets phydev->link = 0; therefore subsequent calls to
>>>>>> phy_state_machin() will be no-op.
>>>>>
>>>>> Actually you are right, once phydev->link is set to 0 these would
>>>>> become
>>>>> no-ops. Still scratching my head as to what happens for David then...
>>>>>
>>>>>>
>>>>>> Also, queue_delayed_work() is only called in polling mode.
>>>>>> David stated that he's using interrupt mode.
>>>>
>>>> Did you see what I wrote?
>>>
>>> Still not following, see below.
>>>
>>>>
>>>> phy_disconnect() calls phy_stop_interrupts() which puts it into polling
>>>> mode. So the polling work gets queued unconditionally.
>>>
>>> What part of phy_stop_interrupts() is responsible for changing
>>> phydev->irq to PHY_POLL? free_irq() cannot touch phydev->irq otherwise
>>> subsequent request_irq() calls won't work anymore.
>>> phy_disable_interrupts() only calls back into the PHY driver to
>>> acknowledge and clear interrupts.
>>>
>>> If we were using a PHY with PHY_POLL, as Marc said, the first
>>> synchronous call to phy_state_machine() would have acted on PHY_HALTED
>>> and even if we incorrectly keep re-scheduling the state machine from
>>> PHY_HALTED to PHY_HALTED the second time around nothing can happen.
>>>
>>> What are we missing here?
>>>
>>
>> OK, I am now as confused as you guys are. I will go back and get an
>> ftrace log out of the failure.
>>
> OK, let's forget about the PHY_HALTED discussion.
>
>
> Consider instead the case of a Marvell phy with no interrupts connected
> on a v4.9.43 kernel, single CPU:
>
>
> 0) | phy_disconnect() {
> 0) | phy_stop_machine() {
> 0) | cancel_delayed_work_sync() {
> 0) + 23.986 us | } /* cancel_delayed_work_sync */
> 0) | phy_state_machine() {
> 0) | phy_start_aneg_priv() {
Thanks for providing the trace, I think I have an idea of what's going
on, see below.
> 0) | marvell_config_aneg() {
> 0) ! 240.538 us | } /* marvell_config_aneg */
> 0) ! 244.971 us | } /* phy_start_aneg_priv */
> 0) | queue_delayed_work_on() {
> 0) + 18.016 us | } /* queue_delayed_work_on */
> 0) ! 268.184 us | } /* phy_state_machine */
> 0) ! 297.394 us | } /* phy_stop_machine */
> 0) | phy_detach() {
> 0) | phy_suspend() {
> 0) | phy_ethtool_get_wol() {
> 0) 0.677 us | } /* phy_ethtool_get_wol */
> 0) | genphy_suspend() {
> 0) + 71.250 us | } /* genphy_suspend */
> 0) + 74.197 us | } /* phy_suspend */
> 0) + 80.302 us | } /* phy_detach */
> 0) ! 380.072 us | } /* phy_disconnect */
> .
> .
> .
> 0) | process_one_work() {
> 0) | find_worker_executing_work() {
> 0) 0.688 us | } /* find_worker_executing_work */
> 0) | set_work_pool_and_clear_pending() {
> 0) 0.734 us | } /* set_work_pool_and_clear_pending */
> 0) | phy_state_machine() {
> 0) | genphy_read_status() {
> 0) ! 205.721 us | } /* genphy_read_status */
> 0) | netif_carrier_off() {
> 0) | do_page_fault() {
>
>
> The do_page_fault() at the end indicates the NULL pointer dereference.
>
> That added call to phy_state_machine() turns the polling back on
> unconditionally for a phy that should be disconnected. How is that
> correct?
It is not fundamentally correct and I don't think there was any
objection to that to begin with. In fact there is a bug/inefficiency
here in that if we have entered the PHY state machine with PHY_HALTED we
should not re-schedule it period, only applicable to PHY_POLL cases
*and* properly calling phy_stop() followed by phy_disconnect().
What I now think is happening in your case is the following:
phy_stop() was not called, so nothing does set phydev->state to
PHY_HALTED in the first place so we have:
phy_disconnect()
-> phy_stop_machine()
-> cancel_delayed_work_sync() OK
phydev->state is probably RUNNING so we have:
-> phydev->state = PHY_UP
phy_state_machine() is called synchronously
-> PHY_UP -> needs_aneg = true
-> phy_restart_aneg()
-> queue_delayed_work_sync()
-> phydev->adjust_link = NULL
-> phy_deatch() -> boom
Can you confirm whether the driver you are using does call phy_stop()
prior to phy_disconnect()? If that is the case then this whole theory
falls apart, if not, then this needs fixing in both the driver and PHYLIB.
Thanks
--
Florian
^ permalink raw reply
* Re: [GIT] Networking
From: Linus Torvalds @ 2017-09-06 23:27 UTC (permalink / raw)
To: David Miller, Johannes Berg, Emmanuel Grumbach, Luca Coelho,
Kalle Valo
Cc: Andrew Morton, Network Development, Linux Kernel Mailing List,
Intel Linux Wireless, Linux Wireless List
In-Reply-To: <20170905.214143.826912481689443792.davem@davemloft.net>
This pull request completely breaks Intel wireless for me.
This is my trusty old XPS 13 (9350), using Intel Wireless 8260 (rev 3a).
That remains a very standard Intel machine with absolutely zero odd
things going on.
The firmware is iwlwifi-8000C-28.ucode from
iwl7260-firmware-25.30.13.0-75.fc26.noarch, and the kernel reports
iwlwifi 0000:3a:00.0: loaded firmware version 27.455470.0 op_mode iwlmvm
the thing starts acting badly with this:
iwlwifi 0000:3a:00.0: FW Error notification: type 0x00000000 cmd_id 0x04
iwlwifi 0000:3a:00.0: FW Error notification: seq 0x0000 service 0x00000004
iwlwifi 0000:3a:00.0: FW Error notification: timestamp 0x 5D84
iwlwifi 0000:3a:00.0: Microcode SW error detected. Restarting 0x2000000.
iwlwifi 0000:3a:00.0: Start IWL Error Log Dump:
iwlwifi 0000:3a:00.0: Status: 0x00000100, count: 6
iwlwifi 0000:3a:00.0: Loaded firmware version: 27.455470.0
iwlwifi 0000:3a:00.0: 0x00000038 | BAD_COMMAND
iwlwifi 0000:3a:00.0: 0x00A002F0 | trm_hw_status0
...
iwlwifi 0000:3a:00.0: 0x00000000 | isr status reg
ieee80211 phy0: Hardware restart was requested
iwlwifi 0000:3a:00.0: FW error in SYNC CMD MAC_CONTEXT_CMD
CPU: 2 PID: 993 Comm: NetworkManager Not tainted 4.13.0-06466-g80cee03bf1d6 #4
Hardware name: Dell Inc. XPS 13 9350/09JHRY, BIOS 1.4.17 05/10/2017
Call Trace:
dump_stack+0x4d/0x70
iwl_trans_pcie_send_hcmd+0x4e7/0x530 [iwlwifi]
? wait_woken+0x80/0x80
iwl_trans_send_cmd+0x5c/0xc0 [iwlwifi]
iwl_mvm_send_cmd+0x32/0x90 [iwlmvm]
iwl_mvm_send_cmd_pdu+0x58/0x80 [iwlmvm]
iwl_mvm_mac_ctxt_send_cmd+0x2a/0x60 [iwlmvm]
? iwl_mvm_mac_ctxt_send_cmd+0x2a/0x60 [iwlmvm]
iwl_mvm_mac_ctxt_cmd_sta+0x140/0x1e0 [iwlmvm]
iwl_mvm_mac_ctx_send+0x2d/0x60 [iwlmvm]
iwl_mvm_mac_ctxt_add+0x43/0xc0 [iwlmvm]
iwl_mvm_mac_add_interface+0x139/0x2b0 [iwlmvm]
? iwl_led_brightness_set+0x1f/0x30 [iwlmvm]
drv_add_interface+0x4a/0x120 [mac80211]
ieee80211_do_open+0x33d/0x820 [mac80211]
ieee80211_open+0x52/0x60 [mac80211]
__dev_open+0xae/0x120
__dev_change_flags+0x17b/0x1c0
dev_change_flags+0x29/0x60
do_setlink+0x2f7/0xe60
? __nla_put+0x20/0x30
? _raw_read_unlock_bh+0x20/0x30
? inet6_fill_ifla6_attrs+0x4be/0x4e0
? __kmalloc_node_track_caller+0x35/0x2b0
? nla_parse+0x35/0x100
rtnl_newlink+0x5d2/0x8f0
? __netlink_sendskb+0x3b/0x60
? security_capset+0x40/0x80
? ns_capable_common+0x68/0x80
? ns_capable+0x13/0x20
rtnetlink_rcv_msg+0x1f9/0x280
? rtnl_calcit.isra.26+0x110/0x110
netlink_rcv_skb+0x8e/0x130
rtnetlink_rcv+0x15/0x20
netlink_unicast+0x18b/0x220
netlink_sendmsg+0x2ad/0x3a0
sock_sendmsg+0x38/0x50
___sys_sendmsg+0x269/0x2c0
? addrconf_sysctl_forward+0x114/0x280
? dev_forward_change+0x140/0x140
? sysctl_head_finish.part.22+0x32/0x40
? lockref_put_or_lock+0x5e/0x80
? dput.part.22+0x13e/0x1c0
? mntput+0x24/0x40
__sys_sendmsg+0x54/0x90
? __sys_sendmsg+0x54/0x90
SyS_sendmsg+0x12/0x20
entry_SYSCALL_64_fastpath+0x13/0x94
RIP: 0033:0x7ff1f9933134
RSP: 002b:00007ffe7419b460 EFLAGS: 00000293 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 000055604b6d82b9 RCX: 00007ff1f9933134
RDX: 0000000000000000 RSI: 00007ffe7419b4b0 RDI: 0000000000000007
RBP: 00007ffe7419b940 R08: 0000000000000000 R09: 000055604d16b400
R10: 00007ff1f7cf8b38 R11: 0000000000000293 R12: 0000000000000001
R13: 0000000000000001 R14: 00007ffe7419b670 R15: 000055604b9515a0
iwlwifi 0000:3a:00.0: Failed to send MAC context (action:1): -5
and it doesn't get any better from there. The next error seems to be
Timeout waiting for hardware access (CSR_GP_CNTRL 0x08000008)
------------[ cut here ]------------
WARNING: CPU: 3 PID: 1075 at
drivers/net/wireless/intel/iwlwifi/pcie/trans.c:1874
iwl_trans_pcie_grab_nic_access+0xdf/0xf0 [iwlwifi]
and it will continue with those microcode failure errors and various
other warnigns about how nothing is working.
And no, nothing works. A lot of log output, no actual network access..
Linus
^ permalink raw reply
* Re: [GIT] Networking
From: David Miller @ 2017-09-06 23:31 UTC (permalink / raw)
To: torvalds
Cc: johannes.berg, emmanuel.grumbach, luciano.coelho, kvalo, akpm,
netdev, linux-kernel, linuxwifi, linux-wireless
In-Reply-To: <CA+55aFw0KNTKFrRHk5hthDGTGgL9BGt9G=x_m9Tz7m_-pN+NoA@mail.gmail.com>
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 6 Sep 2017 16:27:15 -0700
> This pull request completely breaks Intel wireless for me.
>
> This is my trusty old XPS 13 (9350), using Intel Wireless 8260 (rev 3a).
>
> That remains a very standard Intel machine with absolutely zero odd
> things going on.
>
> The firmware is iwlwifi-8000C-28.ucode from
> iwl7260-firmware-25.30.13.0-75.fc26.noarch, and the kernel reports
...
Johannes and other Intel folks please look into this.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox