* [PATCH net-next 4/5] net: dsa: mv88e6xxx: Support multiple MDIO busses
From: Andrew Lunn @ 2017-01-24 13:53 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Vivien Didelot, Florian Fainelli, Andrew Lunn
In-Reply-To: <1485266031-4980-1-git-send-email-andrew@lunn.ch>
The mv88e6390 has multiple MDIO busses. Generalize the parsing of the
device tree to support multiple mdio nodes. The external mdio bus has
a compatible strings to indicate it is external.
Keep a linked list of busses, placing the external mdio bus at the
tail of the list. When within the driver an mdio bus is needed,
e.g. for EEE or SERDES, use the head of the list which should be the
internal bus.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
.../devicetree/bindings/net/dsa/marvell.txt | 41 +++++++-
drivers/net/dsa/mv88e6xxx/chip.c | 110 +++++++++++++++------
drivers/net/dsa/mv88e6xxx/mv88e6xxx.h | 10 +-
3 files changed, 126 insertions(+), 35 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/dsa/marvell.txt b/Documentation/devicetree/bindings/net/dsa/marvell.txt
index b3dd6b40e0de..86a05563c8ae 100644
--- a/Documentation/devicetree/bindings/net/dsa/marvell.txt
+++ b/Documentation/devicetree/bindings/net/dsa/marvell.txt
@@ -26,8 +26,12 @@ Optional properties:
- interrupt-controller : Indicates the switch is itself an interrupt
controller. This is used for the PHY interrupts.
#interrupt-cells = <2> : Controller uses two cells, number and flag
-- mdio : container of PHY and devices on the switches MDIO
- bus
+- mdio : Container of PHY and devices on the switches MDIO
+ bus.
+- mdio? : Container of PHYs and devices on the external MDIO
+ bus. The node must contains a compatible string of
+ "marvell,mv88e6xxx-mdio-external"
+
Example:
mdio {
@@ -53,3 +57,36 @@ Example:
};
};
};
+
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ interrupt-parent = <&gpio0>;
+ interrupts = <27 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+
+ switch0: switch@0 {
+ compatible = "marvell,mv88e6390";
+ reg = <0>;
+ reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;
+ };
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ switch1phy0: switch1phy0@0 {
+ reg = <0>;
+ interrupt-parent = <&switch0>;
+ interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+ };
+ };
+
+ mdio1 {
+ compatible = "marvell,mv88e6xxx-mdio-external";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ switch1phy9: switch1phy0@9 {
+ reg = <9>;
+ };
+ };
+ };
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index b0fd1432f4f3..5668e778ed1d 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -236,16 +236,29 @@ static int mv88e6165_phy_write(struct mv88e6xxx_chip *chip,
return mv88e6xxx_write(chip, addr, reg, val);
}
+static struct mii_bus *mv88e6xxx_default_mdio_bus(struct mv88e6xxx_chip *chip)
+{
+ struct mv88e6xxx_mdio_bus *mdio_bus;
+
+ mdio_bus = list_first_entry(&chip->mdios, struct mv88e6xxx_mdio_bus,
+ list);
+ if (!mdio_bus)
+ return NULL;
+
+ return mdio_bus->bus;
+}
+
static int mv88e6xxx_phy_read(struct mv88e6xxx_chip *chip, int phy,
int reg, u16 *val)
{
int addr = phy; /* PHY devices addresses start at 0x0 */
- struct mii_bus *bus = chip->mdio_bus;
+ struct mii_bus *bus;
- if (!chip->info->ops->phy_read)
+ bus = mv88e6xxx_default_mdio_bus(chip);
+ if (!bus)
return -EOPNOTSUPP;
- if (!bus)
+ if (!chip->info->ops->phy_read)
return -EOPNOTSUPP;
return chip->info->ops->phy_read(chip, bus, addr, reg, val);
@@ -255,12 +268,13 @@ static int mv88e6xxx_phy_write(struct mv88e6xxx_chip *chip, int phy,
int reg, u16 val)
{
int addr = phy; /* PHY devices addresses start at 0x0 */
- struct mii_bus *bus = chip->mdio_bus;
+ struct mii_bus *bus;
- if (!chip->info->ops->phy_write)
+ bus = mv88e6xxx_default_mdio_bus(chip);
+ if (!bus)
return -EOPNOTSUPP;
- if (!bus)
+ if (!chip->info->ops->phy_write)
return -EOPNOTSUPP;
return chip->info->ops->phy_write(chip, bus, addr, reg, val);
@@ -2845,7 +2859,7 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
int i;
chip->ds = ds;
- ds->slave_mii_bus = chip->mdio_bus;
+ ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
mutex_lock(&chip->reg_lock);
@@ -2940,22 +2954,23 @@ static int mv88e6xxx_mdio_write(struct mii_bus *bus, int phy, int reg, u16 val)
}
static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip,
- struct device_node *np)
+ struct device_node *np,
+ bool external)
{
static int index;
struct mv88e6xxx_mdio_bus *mdio_bus;
struct mii_bus *bus;
int err;
- if (np)
- chip->mdio_np = of_get_child_by_name(np, "mdio");
-
bus = devm_mdiobus_alloc_size(chip->dev, sizeof(*mdio_bus));
if (!bus)
return -ENOMEM;
mdio_bus = bus->priv;
+ mdio_bus->bus = bus;
mdio_bus->chip = chip;
+ INIT_LIST_HEAD(&mdio_bus->list);
+ mdio_bus->external = external;
if (np) {
bus->name = np->full_name;
@@ -2969,34 +2984,72 @@ static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip,
bus->write = mv88e6xxx_mdio_write;
bus->parent = chip->dev;
- if (chip->mdio_np)
- err = of_mdiobus_register(bus, chip->mdio_np);
+ if (np)
+ err = of_mdiobus_register(bus, np);
else
err = mdiobus_register(bus);
if (err) {
dev_err(chip->dev, "Cannot register MDIO bus (%d)\n", err);
- goto out;
+ return err;
}
- chip->mdio_bus = bus;
+
+ if (external)
+ list_add_tail(&mdio_bus->list, &chip->mdios);
+ else
+ list_add(&mdio_bus->list, &chip->mdios);
return 0;
+}
-out:
- if (chip->mdio_np)
- of_node_put(chip->mdio_np);
+static const struct of_device_id mv88e6xxx_mdio_external_match[] = {
+ { .compatible = "marvell,mv88e6xxx-mdio-external",
+ .data = (void *)true },
+ { },
+};
- return err;
+static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip,
+ struct device_node *np)
+{
+ const struct of_device_id *match;
+ struct device_node *child;
+ int err;
+
+ /* Always register one mdio bus for the internal/default mdio
+ * bus. This maybe represented in the device tree, but is
+ * optional.
+ */
+ child = of_get_child_by_name(np, "mdio");
+ err = mv88e6xxx_mdio_register(chip, child, false);
+ if (err)
+ return err;
+
+ /* Walk the device tree, and see if there are any other nodes
+ * which say they are compatible with the external mdio
+ * bus.
+ */
+ for_each_available_child_of_node(np, child) {
+ match = of_match_node(mv88e6xxx_mdio_external_match, child);
+ if (match) {
+ err = mv88e6xxx_mdio_register(chip, child, true);
+ if (err)
+ return err;
+ }
+ }
+
+ return 0;
}
-static void mv88e6xxx_mdio_unregister(struct mv88e6xxx_chip *chip)
+static void mv88e6xxx_mdios_unregister(struct mv88e6xxx_chip *chip)
{
- struct mii_bus *bus = chip->mdio_bus;
+ struct mv88e6xxx_mdio_bus *mdio_bus;
+ struct mii_bus *bus;
- mdiobus_unregister(bus);
+ list_for_each_entry(mdio_bus, &chip->mdios, list) {
+ bus = mdio_bus->bus;
- if (chip->mdio_np)
- of_node_put(chip->mdio_np);
+ mdiobus_unregister(bus);
+ }
}
static int mv88e6xxx_get_eeprom_len(struct dsa_switch *ds)
@@ -4123,6 +4176,7 @@ static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
chip->dev = dev;
mutex_init(&chip->reg_lock);
+ INIT_LIST_HEAD(&chip->mdios);
return chip;
}
@@ -4197,7 +4251,7 @@ static const char *mv88e6xxx_drv_probe(struct device *dsa_dev,
mv88e6xxx_phy_init(chip);
- err = mv88e6xxx_mdio_register(chip, NULL);
+ err = mv88e6xxx_mdios_register(chip, NULL);
if (err)
goto free;
@@ -4398,7 +4452,7 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
}
}
- err = mv88e6xxx_mdio_register(chip, np);
+ err = mv88e6xxx_mdios_register(chip, np);
if (err)
goto out_g2_irq;
@@ -4409,7 +4463,7 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
return 0;
out_mdio:
- mv88e6xxx_mdio_unregister(chip);
+ mv88e6xxx_mdios_unregister(chip);
out_g2_irq:
if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_G2_INT) && chip->irq > 0)
mv88e6xxx_g2_irq_free(chip);
@@ -4430,7 +4484,7 @@ static void mv88e6xxx_remove(struct mdio_device *mdiodev)
mv88e6xxx_phy_destroy(chip);
mv88e6xxx_unregister_switch(chip);
- mv88e6xxx_mdio_unregister(chip);
+ mv88e6xxx_mdios_unregister(chip);
if (chip->irq > 0) {
if (mv88e6xxx_has(chip, MV88E6XXX_FLAG_G2_INT))
diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
index 6f7ddb594809..7d24add45e74 100644
--- a/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx.h
@@ -730,11 +730,8 @@ struct mv88e6xxx_chip {
/* set to size of eeprom if supported by the switch */
int eeprom_len;
- /* Device node for the MDIO bus */
- struct device_node *mdio_np;
-
- /* And the MDIO bus itself */
- struct mii_bus *mdio_bus;
+ /* List of mdio busses */
+ struct list_head mdios;
/* There can be two interrupt controllers, which are chained
* off a GPIO as interrupt source
@@ -751,7 +748,10 @@ struct mv88e6xxx_bus_ops {
};
struct mv88e6xxx_mdio_bus {
+ struct mii_bus *bus;
struct mv88e6xxx_chip *chip;
+ struct list_head list;
+ bool external;
};
struct mv88e6xxx_ops {
--
2.11.0
^ permalink raw reply related
* [PATCH net 3/3] alx: work around hardware bug in interrupt fallback path
From: Tobias Regnery @ 2017-01-24 13:34 UTC (permalink / raw)
To: netdev, jcliburn, chris.snook; +Cc: davem, Tobias Regnery
In-Reply-To: <cover.1485263833.git.tobias.regnery@gmail.com>
If requesting msi-x interrupts fails in alx_request_irq we fall back to
a single tx queue and msi or legacy interrupts.
Currently the adapter stops working in this case and we get tx watchdog
timeouts. For reasons unknown the adapter gets confused when we load the
dma adresses to the chip in alx_init_ring_ptrs twice: the first time with
multiple queues and the second time in the fallback case with a single
queue.
To fix this move the the call to alx_reinit_rings (which calls
alx_init_ring_ptrs) after alx_request_irq. At this time it is clear how
much tx queues we have and which dma addresses we use.
Fixes: d768319cd427 ("alx: enable multiple tx queues")
Signed-off-by: Tobias Regnery <tobias.regnery@gmail.com>
---
drivers/net/ethernet/atheros/alx/main.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/atheros/alx/main.c b/drivers/net/ethernet/atheros/alx/main.c
index 75cbd46e429d..7dcc907a449d 100644
--- a/drivers/net/ethernet/atheros/alx/main.c
+++ b/drivers/net/ethernet/atheros/alx/main.c
@@ -685,8 +685,6 @@ static int alx_alloc_rings(struct alx_priv *alx)
return -ENOMEM;
}
- alx_reinit_rings(alx);
-
return 0;
}
@@ -1242,6 +1240,12 @@ static int __alx_open(struct alx_priv *alx, bool resume)
if (err)
goto out_free_rings;
+ /* must be called after alx_request_irq because the chip stops working
+ * if we copy the dma addresses in alx_init_ring_ptrs twice when
+ * requesting msi-x interrupts failed
+ */
+ alx_reinit_rings(alx);
+
netif_set_real_num_tx_queues(alx->dev, alx->num_txq);
netif_set_real_num_rx_queues(alx->dev, alx->num_rxq);
--
2.9.3
^ permalink raw reply related
* [PATCH net 1/3] alx: fix wrong condition to free descriptor memory
From: Tobias Regnery @ 2017-01-24 13:34 UTC (permalink / raw)
To: netdev, jcliburn, chris.snook; +Cc: davem, Tobias Regnery
In-Reply-To: <cover.1485263833.git.tobias.regnery@gmail.com>
The condition to free the descriptor memory is wrong, we want to free the
memory if it is set and not if it is unset. Invert the test to fix this
issue.
Fixes: b0999223f224b ("alx: add ability to allocate and free alx_napi structures")
Signed-off-by: Tobias Regnery <tobias.regnery@gmail.com>
---
drivers/net/ethernet/atheros/alx/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/atheros/alx/main.c b/drivers/net/ethernet/atheros/alx/main.c
index c8f525574d68..765306bd78c2 100644
--- a/drivers/net/ethernet/atheros/alx/main.c
+++ b/drivers/net/ethernet/atheros/alx/main.c
@@ -703,7 +703,7 @@ static void alx_free_rings(struct alx_priv *alx)
if (alx->qnapi[0] && alx->qnapi[0]->rxq)
kfree(alx->qnapi[0]->rxq->bufs);
- if (!alx->descmem.virt)
+ if (alx->descmem.virt)
dma_free_coherent(&alx->hw.pdev->dev,
alx->descmem.size,
alx->descmem.virt,
--
2.9.3
^ permalink raw reply related
* [PATCH net 2/3] alx: fix fallback to msi or legacy interrupts
From: Tobias Regnery @ 2017-01-24 13:34 UTC (permalink / raw)
To: netdev, jcliburn, chris.snook; +Cc: davem, Tobias Regnery
In-Reply-To: <cover.1485263833.git.tobias.regnery@gmail.com>
If requesting msi-x interrupts fails we should fall back to msi or
legacy interrupts. However alx_realloc_ressources don't call
alx_init_intr, so we fail to set the right number of tx queues.
This results in watchdog timeouts and a nonfunctional adapter.
Fixes: d768319cd427 ("alx: enable multiple tx queues")
Signed-off-by: Tobias Regnery <tobias.regnery@gmail.com>
---
drivers/net/ethernet/atheros/alx/main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/atheros/alx/main.c b/drivers/net/ethernet/atheros/alx/main.c
index 765306bd78c2..75cbd46e429d 100644
--- a/drivers/net/ethernet/atheros/alx/main.c
+++ b/drivers/net/ethernet/atheros/alx/main.c
@@ -984,6 +984,7 @@ static int alx_realloc_resources(struct alx_priv *alx)
alx_free_rings(alx);
alx_free_napis(alx);
alx_disable_advanced_intr(alx);
+ alx_init_intr(alx, false);
err = alx_alloc_napis(alx);
if (err)
--
2.9.3
^ permalink raw reply related
* [PATCH net 0/3] alx: fix fallout from multi queue conversion
From: Tobias Regnery @ 2017-01-24 13:34 UTC (permalink / raw)
To: netdev, jcliburn, chris.snook; +Cc: davem, Tobias Regnery
Here are 3 fixes for the multi queue conversion in v4.10.
The first patch fixes a wrong condition in an if statement.
Patches 2 and 3 fixes regressions in the corner case when requesting msi-x
interrupts fails and we fall back to msi or legacy interrupts.
Tobias Regnery (3):
alx: fix wrong condition to free descriptor memory
alx: fix fallback to msi or legacy interrupts
alx: work around hardware bug in interrupt fallback path
drivers/net/ethernet/atheros/alx/main.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
--
2.9.3
^ permalink raw reply
* Re: [PATCH] net: ethernet: mvneta: add support for 2.5G DRSGMII mode
From: Andrew Lunn @ 2017-01-24 13:19 UTC (permalink / raw)
To: Jan Lübbe
Cc: netdev, devicetree, davem, Rob Herring, Mark Rutland,
Thomas Petazzoni, Florian Fainelli
In-Reply-To: <1485246858.5508.47.camel@pengutronix.de>
> It works without any other changes. ;)
>
> Initially I looked at adding a SPEED_2500, but I wasn't sure which code
> would need to handle the new value. Would the path from the ethtool
> ioctl to the driver be enough (mvneta_ethtool_set_link_ksettings() as
> you said)?
You have this connected to an FPGA? Do you have a traditional PHY in
the FPGA? Or are you implementing something more like an Ethernet
switch?
I don't know this driver too well, but what often happens is the PHY
performs autoneg and the phylib then calls the adjust_link function to
set the MAC to what has been negotiated. So if you have a PHY attached
which can do 2.5Ghz, you might be seeing calls to adjust_link with
SPEED_2500.
What does ethtool <devname> show?
Thanks
Andrew
^ permalink raw reply
* Re: [PATCHv5 net-next 0/5] sctp: add sender-side procedures for stream reconf asoc reset and add streams
From: Marcelo Ricardo Leitner @ 2017-01-24 13:11 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, Neil Horman, Vlad Yasevich, davem
In-Reply-To: <20170123184217.GB3781@localhost.localdomain>
On Mon, Jan 23, 2017 at 04:42:17PM -0200, Marcelo Ricardo Leitner wrote:
> On Sat, Jan 21, 2017 at 06:24:21PM +0800, Xin Long wrote:
> > Patch 3/5 is to implement sender-side procedures for the SSN/TSN Reset
> > Request Parameter described in rfc6525 section 5.1.4, patch 2/5 is
> > ahead of it to define a function to make the request chunk for it.
> >
> > Patch 5/5 is to implement sender-side procedures for the Add Incoming
> > and Outgoing Streams Request Parameter Request Parameter described in
> > rfc6525 section 5.1.5 and 5.1.6, patch 4/5 is ahead of it to define a
> > function to make the request chunk for it.
> >
> > Patch 1/5 is a fix to recover streams states when it fails to send
> > request.
> >
> > v1->v2:
> > - put these into a smaller group.
> > - rename some temporary variables in the codes.
> > - rename the titles of the commits and improve some changelogs.
> > v2->v3:
> > - re-split the patchset and make sure it has no dead codes for review.
> > - move some codes into stream.c from socket.c.
> > v3->v4:
> > - add one more patch to fix a send reset stream request issue.
> > - doing actual work only when request is sent successfully.
> > - reduce some indents in sctp_send_add_streams.
> > v4->v5:
> > - close streams before sending request and recover them when sending
> > fails in patch 1/5 and patch 3/5
>
> To make it clear, the notes on v3 patchset also apply to this one, which
> are:
> - usage of krealloc()
> - the removal of __packed attribute
- the int change too.
>
> So there should be a v6 of it soon.
>
> Thanks
>
> >
> > Xin Long (5):
> > sctp: streams should be recovered when it fails to send request.
> > sctp: add support for generating stream reconf ssn/tsn reset request
> > chunk
> > sctp: implement sender-side procedures for SSN/TSN Reset Request
> > Parameter
> > sctp: add support for generating stream reconf add incoming/outgoing
> > streams request chunk
> > sctp: implement sender-side procedures for Add Incoming/Outgoing
> > Streams Request Parameter
> >
> > include/linux/sctp.h | 12 ++++
> > include/net/sctp/sctp.h | 3 +
> > include/net/sctp/sm.h | 5 ++
> > include/uapi/linux/sctp.h | 8 +++
> > net/sctp/sm_make_chunk.c | 75 +++++++++++++++++++++++++
> > net/sctp/socket.c | 58 +++++++++++++++++++
> > net/sctp/stream.c | 138 +++++++++++++++++++++++++++++++++++++++++++++-
> > 7 files changed, 298 insertions(+), 1 deletion(-)
> >
> > --
> > 2.1.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams Request Parameter
From: 'Marcelo Ricardo Leitner' @ 2017-01-24 13:10 UTC (permalink / raw)
To: David Laight
Cc: 'Xin Long', network dev, linux-sctp@vger.kernel.org,
Neil Horman, Vlad Yasevich, davem@davemloft.net
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DB026CAC6@AcuExch.aculab.com>
On Tue, Jan 24, 2017 at 12:34:06PM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner
> > Sent: 23 January 2017 18:48
> > On Mon, Jan 23, 2017 at 11:25:56AM +0000, David Laight wrote:
> > > From: Xin Long
> > > > Sent: 19 January 2017 17:19
> > > > This patch is to implement Sender-Side Procedures for the Add
> > > > Outgoing and Incoming Streams Request Parameter described in
> > > > rfc6525 section 5.1.5-5.1.6.
> > > >
> > > > It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> > > > 6.3.4 for users.
> > > ...
> > > > + out = params->sas_outstrms;
> > > > + in = params->sas_instrms;
> > > > +
> > > > + if (!out && !in)
> > > > + goto out;
> > > > +
> > > > + if (out) {
> > > > + __u16 nums = stream->outcnt + out;
> > >
> > > Make nums 'unsigned int', the code will be smaller and you can
> > > use the value for the overflow check.
> >
> > Smaller as in to avoid the sum below?
> >
> > >
> > > > + /* Check for overflow, can't use nums here */
> > > > + if (stream->outcnt + out > SCTP_MAX_STREAM)
> > > > + goto out;
>
> No, smaller as in not requiring the compiler to add instructions
> to mask (or worse sign extend) the result of the arithmetic expression
> to less than the number of bits in an 'int' when the result of the
> expression is to be kept in a register.
>
> The x86 is about the only modern cpu that has 8 and 16 bit arithmetic.
> For everything else you really don't want to do arithmetic on char
> and short unless you really want the wrapping to happen.
Okay thanks.
Marcelo
^ permalink raw reply
* Re: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams Request Parameter
From: 'Marcelo Ricardo Leitner' @ 2017-01-24 13:08 UTC (permalink / raw)
To: David Laight
Cc: Neil Horman, 'Xin Long', network dev,
linux-sctp@vger.kernel.org, Vlad Yasevich, davem@davemloft.net
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DB026CAD7@AcuExch.aculab.com>
On Tue, Jan 24, 2017 at 12:35:39PM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner
> > Sent: 23 January 2017 16:03
> ...
> > > > Does kcalloc() zero the entire area, or just the length you ask for?
> > > > If the latter you need to zero the rest here.
> > > Better still, just use krealloc. You still need to zero out any space beyond
> > > the old length, but it will make the code shorter, and avoid the need for
> > > additional temporary variables.
> >
> > Seems if we pass gfp | __GFP_ZERO to krealloc it will end up zeroing the
> > slab for us before doing the memcpy.
> > I didn't follow all paths but in slab_alloc_node it will end up calling:
> > if (unlikely(gfpflags & __GFP_ZERO) && object)
> > memset(object, 0, s->object_size);
> > So I would expect that other paths also do it.
>
> You probably don't want krealloc() zeroing all of the new area.
Yep, agreed.
Marcelo
^ permalink raw reply
* Re: [patch net-next] sctp: fix some debug output
From: Marcelo Ricardo Leitner @ 2017-01-24 13:00 UTC (permalink / raw)
To: Dan Carpenter
Cc: Vlad Yasevich, Xin Long, Neil Horman, David S. Miller, linux-sctp,
netdev, kernel-janitors, Colin King
In-Reply-To: <20170124105007.GF4201@mwanda>
On Tue, Jan 24, 2017 at 01:50:07PM +0300, Dan Carpenter wrote:
> On Tue, Jan 24, 2017 at 07:14:11AM -0200, Marcelo Ricardo Leitner wrote:
> > On Tue, Jan 24, 2017 at 12:05:40PM +0300, Dan Carpenter wrote:
> > > We added SCTP_EVENT_TIMEOUT_RECONF but we didn't update this array so
> > > it causes an off-by-one read overflow.
> > >
> > > Fixes: 7b9438de0cd4 ("sctp: add stream reconf timer")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > Weird, seems your patch is missing the --- marker here.
>
> Are there tools that require the --- marker? I normally leave it out
> when it's not required.
Yeah I don't think it's required. It's usually there to split it between
changelog and non-changelog stuff like the diffstat output, which is not
there too, so it should be okay.
Marcelo
^ permalink raw reply
* [PATCH net] net: free ip_vs_dest structs when refcnt=0
From: David Windsor @ 2017-01-24 12:48 UTC (permalink / raw)
To: netdev, kernel-hardening
Cc: keescook, elena.reshetova, ja, dwindsor, ishkamiel
Currently, the ip_vs_dest cache frees ip_vs_dest objects when their reference
count becomes < 0. Aside from not being semantically sound, this is problematic
for the new type refcount_t, which will be introduced shortly in a separate patch.
refcount_t is the new kernel type for holding reference counts, and provides
overflow protection and a constrained interface relative to atomic_t (the type
currently being used for kernel reference counts).
Per Juilan Anastasov: "The problem is that dest_trash currently holds deleted
dests (unlinked from RCU lists) with refcnt=0." Changing dest_trash to hold
dest with refcnt=1 will allow us to free ip_vs_dest structs when their refcnt=0,
in ip_vs_dest_put_and_free().
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
include/net/ip_vs.h | 2 +-
net/netfilter/ipvs/ip_vs_ctl.c | 4 +---
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
index cd6018a..a3e78ad 100644
--- a/include/net/ip_vs.h
+++ b/include/net/ip_vs.h
@@ -1421,7 +1421,7 @@ static inline void ip_vs_dest_put(struct ip_vs_dest *dest)
static inline void ip_vs_dest_put_and_free(struct ip_vs_dest *dest)
{
- if (atomic_dec_return(&dest->refcnt) < 0)
+ if (atomic_dec_and_test(&dest->refcnt))
kfree(dest);
}
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
index 55e0169..6b5492e 100644
--- a/net/netfilter/ipvs/ip_vs_ctl.c
+++ b/net/netfilter/ipvs/ip_vs_ctl.c
@@ -711,7 +711,6 @@ ip_vs_trash_get_dest(struct ip_vs_service *svc, int dest_af,
dest->vport == svc->port))) {
/* HIT */
list_del(&dest->t_list);
- ip_vs_dest_hold(dest);
goto out;
}
}
@@ -1084,7 +1083,6 @@ static void __ip_vs_del_dest(struct netns_ipvs *ipvs, struct ip_vs_dest *dest,
list_add(&dest->t_list, &ipvs->dest_trash);
dest->idle_start = 0;
spin_unlock_bh(&ipvs->dest_trash_lock);
- ip_vs_dest_put(dest);
}
@@ -1160,7 +1158,7 @@ static void ip_vs_dest_trash_expire(unsigned long data)
spin_lock(&ipvs->dest_trash_lock);
list_for_each_entry_safe(dest, next, &ipvs->dest_trash, t_list) {
- if (atomic_read(&dest->refcnt) > 0)
+ if (atomic_read(&dest->refcnt) > 1)
continue;
if (dest->idle_start) {
if (time_before(now, dest->idle_start +
--
2.7.4
^ permalink raw reply related
* RE: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams Request Parameter
From: David Laight @ 2017-01-24 12:35 UTC (permalink / raw)
To: 'Marcelo Ricardo Leitner', Neil Horman
Cc: 'Xin Long', network dev, linux-sctp@vger.kernel.org,
Vlad Yasevich, davem@davemloft.net
In-Reply-To: <20170123160234.GY3781@localhost.localdomain>
From: Marcelo Ricardo Leitner
> Sent: 23 January 2017 16:03
...
> > > Does kcalloc() zero the entire area, or just the length you ask for?
> > > If the latter you need to zero the rest here.
> > Better still, just use krealloc. You still need to zero out any space beyond
> > the old length, but it will make the code shorter, and avoid the need for
> > additional temporary variables.
>
> Seems if we pass gfp | __GFP_ZERO to krealloc it will end up zeroing the
> slab for us before doing the memcpy.
> I didn't follow all paths but in slab_alloc_node it will end up calling:
> if (unlikely(gfpflags & __GFP_ZERO) && object)
> memset(object, 0, s->object_size);
> So I would expect that other paths also do it.
You probably don't want krealloc() zeroing all of the new area.
David
^ permalink raw reply
* RE: [PATCHv3 net-next 4/4] sctp: implement sender-side procedures for Add Incoming/Outgoing Streams Request Parameter
From: David Laight @ 2017-01-24 12:34 UTC (permalink / raw)
To: 'Marcelo Ricardo Leitner'
Cc: 'Xin Long', network dev, linux-sctp@vger.kernel.org,
Neil Horman, Vlad Yasevich, davem@davemloft.net
In-Reply-To: <20170123184744.GC3781@localhost.localdomain>
From: Marcelo Ricardo Leitner
> Sent: 23 January 2017 18:48
> On Mon, Jan 23, 2017 at 11:25:56AM +0000, David Laight wrote:
> > From: Xin Long
> > > Sent: 19 January 2017 17:19
> > > This patch is to implement Sender-Side Procedures for the Add
> > > Outgoing and Incoming Streams Request Parameter described in
> > > rfc6525 section 5.1.5-5.1.6.
> > >
> > > It is also to add sockopt SCTP_ADD_STREAMS in rfc6525 section
> > > 6.3.4 for users.
> > ...
> > > + out = params->sas_outstrms;
> > > + in = params->sas_instrms;
> > > +
> > > + if (!out && !in)
> > > + goto out;
> > > +
> > > + if (out) {
> > > + __u16 nums = stream->outcnt + out;
> >
> > Make nums 'unsigned int', the code will be smaller and you can
> > use the value for the overflow check.
>
> Smaller as in to avoid the sum below?
>
> >
> > > + /* Check for overflow, can't use nums here */
> > > + if (stream->outcnt + out > SCTP_MAX_STREAM)
> > > + goto out;
No, smaller as in not requiring the compiler to add instructions
to mask (or worse sign extend) the result of the arithmetic expression
to less than the number of bits in an 'int' when the result of the
expression is to be kept in a register.
The x86 is about the only modern cpu that has 8 and 16 bit arithmetic.
For everything else you really don't want to do arithmetic on char
and short unless you really want the wrapping to happen.
David
^ permalink raw reply
* Re: [PATCH][V3][net-next] net: sctp: fix array overrun read on sctp_timer_tbl
From: Neil Horman @ 2017-01-24 12:22 UTC (permalink / raw)
To: Colin King
Cc: Vlad Yasevich, David S . Miller, linux-sctp, netdev, linux-kernel
In-Reply-To: <20170124092554.27019-1-colin.king@canonical.com>
On Tue, Jan 24, 2017 at 09:25:54AM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Table sctp_timer_tbl is missing a TIMEOUT_RECONF string so
> add this in. Also compare timeout with the size of the array
> sctp_timer_tbl rather than SCTP_EVENT_TIMEOUT_MAX. Also add
> a build time check that SCTP_EVENT_TIMEOUT_MAX is correct
> so we don't ever get this kind of mismatch between the table
> and SCTP_EVENT_TIMEOUT_MAX in the future.
>
> Kudos to Marcelo Ricardo Leitner for spotting the missing string
> and suggesting the build time sanity check.
>
> Fixes CoverityScan CID#1397639 ("Out-of-bounds read")
>
> Fixes: 7b9438de0cd4 ("sctp: add stream reconf timer")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> net/sctp/debug.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/sctp/debug.c b/net/sctp/debug.c
> index 95d7b15..2e47eb2 100644
> --- a/net/sctp/debug.c
> +++ b/net/sctp/debug.c
> @@ -159,6 +159,7 @@ static const char *const sctp_timer_tbl[] = {
> "TIMEOUT_T4_RTO",
> "TIMEOUT_T5_SHUTDOWN_GUARD",
> "TIMEOUT_HEARTBEAT",
> + "TIMEOUT_RECONF",
> "TIMEOUT_SACK",
> "TIMEOUT_AUTOCLOSE",
> };
> @@ -166,7 +167,9 @@ static const char *const sctp_timer_tbl[] = {
> /* Lookup timer debug name. */
> const char *sctp_tname(const sctp_subtype_t id)
> {
> - if (id.timeout <= SCTP_EVENT_TIMEOUT_MAX)
> + BUILD_BUG_ON(SCTP_EVENT_TIMEOUT_MAX + 1 != ARRAY_SIZE(sctp_timer_tbl));
> +
> + if (id.timeout < ARRAY_SIZE(sctp_timer_tbl))
> return sctp_timer_tbl[id.timeout];
> return "unknown_timer";
> }
> --
> 2.10.2
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH for bnxt_re V4 20/21] RDMA/bnxt_re: Add QP event handling
From: Leon Romanovsky @ 2017-01-24 12:20 UTC (permalink / raw)
To: Selvin Xavier
Cc: dledford, linux-rdma, netdev, michael.chan, Eddie Wai,
Devesh Sharma, Somnath Kotur, Sriharsha Basavapatna
In-Reply-To: <1482320530-5344-21-git-send-email-selvin.xavier@broadcom.com>
[-- Attachment #1: Type: text/plain, Size: 3143 bytes --]
On Wed, Dec 21, 2016 at 03:42:09AM -0800, Selvin Xavier wrote:
> Implements callback handler for processing Async events related to a QP.
> This patch also implements the control path command completion handling.
>
> v3: Removes unwanted braces
>
> Signed-off-by: Eddie Wai <eddie.wai@broadcom.com>
> Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com>
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> ---
> drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 47 ++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
> diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> index 9144b5a..a000397 100644
> --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> @@ -257,6 +257,44 @@ static int bnxt_qplib_process_func_event(struct bnxt_qplib_rcfw *rcfw,
> return 0;
> }
>
> +static int bnxt_qplib_process_qp_event(struct bnxt_qplib_rcfw *rcfw,
> + struct creq_qp_event *qp_event)
> +{
> + struct bnxt_qplib_crsq *crsq = &rcfw->crsq;
> + struct bnxt_qplib_hwq *cmdq = &rcfw->cmdq;
> + struct bnxt_qplib_crsqe *crsqe;
> + u16 cbit, cookie, blocked = 0;
> + unsigned long flags;
> + u32 sw_cons;
> +
> + switch (qp_event->event) {
> + case CREQ_QP_EVENT_EVENT_QP_ERROR_NOTIFICATION:
> + break;
it looks like if( ... ) return 0;
> + default:
> + /* Command Response */
> + spin_lock_irqsave(&cmdq->lock, flags);
> + sw_cons = HWQ_CMP(crsq->cons, crsq);
> + crsqe = &crsq->crsq[sw_cons];
> + crsq->cons++;
> + memcpy(&crsqe->qp_event, qp_event, sizeof(crsqe->qp_event));
> +
> + cookie = le16_to_cpu(crsqe->qp_event.cookie);
> + blocked = cookie & RCFW_CMD_IS_BLOCKING;
> + cookie &= RCFW_MAX_COOKIE_VALUE;
> + cbit = cookie % RCFW_MAX_OUTSTANDING_CMD;
> + if (!test_and_clear_bit(cbit, rcfw->cmdq_bitmap))
> + dev_warn(&rcfw->pdev->dev,
> + "QPLIB: CMD bit %d was not requested", cbit);
> +
> + cmdq->cons += crsqe->req_size;
> + spin_unlock_irqrestore(&cmdq->lock, flags);
> + if (!blocked)
> + wake_up(&rcfw->waitq);
> + break;
> + }
> + return 0;
> +}
> +
> /* SP - CREQ Completion handlers */
> static void bnxt_qplib_service_creq(unsigned long data)
> {
> @@ -280,6 +318,15 @@ static void bnxt_qplib_service_creq(unsigned long data)
> type = creqe->type & CREQ_BASE_TYPE_MASK;
> switch (type) {
> case CREQ_BASE_TYPE_QP_EVENT:
> + if (!bnxt_qplib_process_qp_event
> + (rcfw, (struct creq_qp_event *)creqe))
> + rcfw->creq_qp_event_processed++;
> + else {
> + dev_warn(&rcfw->pdev->dev, "QPLIB: crsqe with");
> + dev_warn(&rcfw->pdev->dev,
> + "QPLIB: type = 0x%x not handled",
> + type);
> + }
> break;
> case CREQ_BASE_TYPE_FUNC_EVENT:
> if (!bnxt_qplib_process_func_event
> --
> 2.5.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH for bnxt_re V4 17/21] RDMA/bnxt_re: Handling dispatching of events to IB stack
From: Leon Romanovsky @ 2017-01-24 12:18 UTC (permalink / raw)
To: Selvin Xavier
Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
michael.chan-dY08KVG/lbpWk0Htik3J/w, Eddie Wai, Devesh Sharma,
Somnath Kotur, Sriharsha Basavapatna
In-Reply-To: <1482320530-5344-18-git-send-email-selvin.xavier-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4310 bytes --]
On Wed, Dec 21, 2016 at 03:42:06AM -0800, Selvin Xavier wrote:
> This patch implements events dispatching to the IB stack
> based on NETDEV events received.
>
> v2: Removed cleanup of the resources during driver unload since
> we are calling unregister_netdevice_notifier first in the exit.
>
> v3: Fixes cocci warnings and some sparse warnings
>
> Signed-off-by: Eddie Wai <eddie.wai-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Devesh Sharma <devesh.sharma-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Somnath Kotur <somnath.kotur-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Selvin Xavier <selvin.xavier-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> ---
> drivers/infiniband/hw/bnxt_re/main.c | 65 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 65 insertions(+)
>
> diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> index ab0b35a..bd13414 100644
> --- a/drivers/infiniband/hw/bnxt_re/main.c
> +++ b/drivers/infiniband/hw/bnxt_re/main.c
> @@ -729,6 +729,60 @@ static int bnxt_re_alloc_res(struct bnxt_re_dev *rdev)
> return rc;
> }
>
> +static void bnxt_re_dispatch_event(struct ib_device *ibdev, struct ib_qp *qp,
> + u8 port_num, enum ib_event_type event)
> +{
> + struct ib_event ib_event;
> +
> + ib_event.device = ibdev;
> + if (qp)
> + ib_event.element.qp = qp;
> + else
> + ib_event.element.port_num = port_num;
> + ib_event.event = event;
> + ib_dispatch_event(&ib_event);
> +}
> +
> +static bool bnxt_re_is_qp1_or_shadow_qp(struct bnxt_re_dev *rdev,
> + struct bnxt_re_qp *qp)
> +{
> + return (qp->ib_qp.qp_type == IB_QPT_GSI) || (qp == rdev->qp1_sqp);
> +}
> +
> +static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev, bool qp_wait)
> +{
> + int mask = IB_QP_STATE, qp_count, count = 1;
> + struct ib_qp_attr qp_attr;
> + struct bnxt_re_qp *qp;
> +
> + qp_attr.qp_state = IB_QPS_ERR;
> + mutex_lock(&rdev->qp_lock);
> + list_for_each_entry(qp, &rdev->qp_list, list) {
> + /* Modify the state of all QPs except QP1/Shadow QP */
> + if (!bnxt_re_is_qp1_or_shadow_qp(rdev, qp)) {
> + if (qp->qplib_qp.state !=
> + CMDQ_MODIFY_QP_NEW_STATE_RESET &&
> + qp->qplib_qp.state !=
> + CMDQ_MODIFY_QP_NEW_STATE_ERR) {
> + bnxt_re_dispatch_event(&rdev->ibdev, &qp->ib_qp,
> + 1, IB_EVENT_QP_FATAL);
> + bnxt_re_modify_qp(&qp->ib_qp, &qp_attr, mask,
> + NULL);
> + }
> + }
> + }
> +
> + mutex_unlock(&rdev->qp_lock);
> + if (qp_wait) {
All callers to this function in this patch set qp_wait to be false.
Do you have in following patches qp_wait == true?
I'm curious because of your msleep below.
> + /* Give the application some time to clean up */
> + do {
> + qp_count = atomic_read(&rdev->qp_count);
> + msleep(100);
> + } while ((qp_count != atomic_read(&rdev->qp_count)) &&
> + count--);
> + }
> +}
> +
> static void bnxt_re_ib_unreg(struct bnxt_re_dev *rdev, bool lock_wait)
> {
> int i, rc;
> @@ -888,6 +942,9 @@ static int bnxt_re_ib_reg(struct bnxt_re_dev *rdev)
> }
> }
> set_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, &rdev->flags);
> + bnxt_re_dispatch_event(&rdev->ibdev, NULL, 1, IB_EVENT_PORT_ACTIVE);
> + bnxt_re_dispatch_event(&rdev->ibdev, NULL, 1, IB_EVENT_GID_CHANGE);
> +
> return 0;
> free_sctx:
> bnxt_re_net_stats_ctx_free(rdev, rdev->qplib_ctx.stats.fw_id, true);
> @@ -967,10 +1024,18 @@ static void bnxt_re_task(struct work_struct *work)
> "Failed to register with IB: %#x", rc);
> break;
> case NETDEV_UP:
> + bnxt_re_dispatch_event(&rdev->ibdev, NULL, 1,
> + IB_EVENT_PORT_ACTIVE);
> break;
> case NETDEV_DOWN:
> + bnxt_re_dev_stop(rdev, false);
> break;
> case NETDEV_CHANGE:
> + if (!netif_carrier_ok(rdev->netdev))
> + bnxt_re_dev_stop(rdev, false);
> + else if (netif_carrier_ok(rdev->netdev))
> + bnxt_re_dispatch_event(&rdev->ibdev, NULL, 1,
> + IB_EVENT_PORT_ACTIVE);
> break;
> default:
> break;
> --
> 2.5.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH net-next v7 1/1] net sched actions: Add support for user cookies
From: Jiri Pirko @ 2017-01-24 12:15 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: davem, netdev, jiri, paulb, john.fastabend, simon.horman, mrv,
hadarh, ogerlitz, roid, xiyou.wangcong, daniel
In-Reply-To: <1485259361-16860-1-git-send-email-jhs@emojatatu.com>
Tue, Jan 24, 2017 at 01:02:41PM CET, jhs@mojatatu.com wrote:
>From: Jamal Hadi Salim <jhs@mojatatu.com>
>
>Introduce optional 128-bit action cookie.
>Like all other cookie schemes in the networking world (eg in protocols
>like http or existing kernel fib protocol field, etc) the idea is to save
>user state that when retrieved serves as a correlator. The kernel
>_should not_ intepret it. The user can store whatever they wish in the
>128 bits.
>
>Sample exercise(showing variable length use of cookie)
>
>.. create an accept action with cookie a1b2c3d4
>sudo $TC actions add action ok index 1 cookie a1b2c3d4
>
>.. dump all gact actions..
>sudo $TC -s actions ls action gact
>
> action order 0: gact action pass
> random type none pass val 0
> index 1 ref 1 bind 0 installed 5 sec used 5 sec
> Action statistics:
> Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
> cookie a1b2c3d4
>
>.. bind the accept action to a filter..
>sudo $TC filter add dev lo parent ffff: protocol ip prio 1 \
>u32 match ip dst 127.0.0.1/32 flowid 1:1 action gact index 1
>
>... send some traffic..
>$ ping 127.0.0.1 -c 3
>PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data.
>64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.020 ms
>64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.027 ms
>64 bytes from 127.0.0.1: icmp_seq=3 ttl=64 time=0.038 ms
>
>--- 127.0.0.1 ping statistics ---
>3 packets transmitted, 3 received, 0% packet loss, time 2109ms
>rtt min/avg/max/mdev = 0.020/0.028/0.038/0.008 ms 1
>
>... show some stats
>$ sudo $TC -s actions get action gact index 1
>
> action order 1: gact action pass
> random type none pass val 0
> index 1 ref 2 bind 1 installed 204 sec used 5 sec
> Action statistics:
> Sent 12168 bytes 164 pkt (dropped 0, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
> cookie a1b2c3d4
>
>.. try longer cookie...
>$ sudo $TC actions replace action ok index 1 cookie 1234567890abcdef
>.. dump..
>$ sudo $TC -s actions ls action gact
>
> action order 1: gact action pass
> random type none pass val 0
> index 1 ref 2 bind 1 installed 204 sec used 5 sec
> Action statistics:
> Sent 12168 bytes 164 pkt (dropped 0, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
> cookie 1234567890abcdef
>
>Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>---
>Changes in v7:
> -put guard around freeing cookie (caught by Simon)
> -separate out the creation of the cookie (suggested by Simon)
>
>Changes in v6:
> - fix mem leak caught by Florian
>
>Changes in V5:
> - kill the stylistic changes
> - Adopt a new structure with length-valuepointer representation
> - rename some things
>
>Changes in v4:
> - move stylistic changes out into a separate patch
> (and add more stylistic changes)
>
>Changes in v3:
> - use TC_ prefix for the max size
> - move the cookie struct so visible only to kernel
> - remove unneeded void * cast
>
>Changes in V2:
> -move from a union to a length-value representation
>
> include/net/act_api.h | 1 +
> include/net/pkt_cls.h | 8 ++++++++
> include/uapi/linux/pkt_cls.h | 3 +++
> net/sched/act_api.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 57 insertions(+)
>
>diff --git a/include/net/act_api.h b/include/net/act_api.h
>index 1d71644..cfa2ae3 100644
>--- a/include/net/act_api.h
>+++ b/include/net/act_api.h
>@@ -41,6 +41,7 @@ struct tc_action {
> struct rcu_head tcfa_rcu;
> struct gnet_stats_basic_cpu __percpu *cpu_bstats;
> struct gnet_stats_queue __percpu *cpu_qstats;
>+ struct tc_cookie *act_cookie;
> };
> #define tcf_head common.tcfa_head
> #define tcf_index common.tcfa_index
>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>index f0a0514..b43077e 100644
>--- a/include/net/pkt_cls.h
>+++ b/include/net/pkt_cls.h
>@@ -515,4 +515,12 @@ struct tc_cls_bpf_offload {
> u32 gen_flags;
> };
>
>+
>+/* This structure holds cookie structure that is passed from user
>+ * to the kernel for actions and classifiers
>+ */
>+struct tc_cookie {
>+ u8 *data;
>+ u32 len;
>+};
> #endif
>diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>index fd373eb..345551e 100644
>--- a/include/uapi/linux/pkt_cls.h
>+++ b/include/uapi/linux/pkt_cls.h
>@@ -4,6 +4,8 @@
> #include <linux/types.h>
> #include <linux/pkt_sched.h>
>
>+#define TC_COOKIE_MAX_SIZE 16
>+
> /* Action attributes */
> enum {
> TCA_ACT_UNSPEC,
>@@ -12,6 +14,7 @@ enum {
> TCA_ACT_INDEX,
> TCA_ACT_STATS,
> TCA_ACT_PAD,
>+ TCA_ACT_COOKIE,
> __TCA_ACT_MAX
> };
>
>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>index cd08df9..3c5e29b 100644
>--- a/net/sched/act_api.c
>+++ b/net/sched/act_api.c
>@@ -24,6 +24,7 @@
> #include <net/net_namespace.h>
> #include <net/sock.h>
> #include <net/sch_generic.h>
>+#include <net/pkt_cls.h>
> #include <net/act_api.h>
> #include <net/netlink.h>
>
>@@ -33,6 +34,12 @@ static void free_tcf(struct rcu_head *head)
>
> free_percpu(p->cpu_bstats);
> free_percpu(p->cpu_qstats);
>+
>+ if (p->act_cookie) {
>+ kfree(p->act_cookie->data);
>+ kfree(p->act_cookie);
>+ }
>+
> kfree(p);
> }
>
>@@ -475,6 +482,12 @@ int tcf_action_destroy(struct list_head *actions, int bind)
> goto nla_put_failure;
> if (tcf_action_copy_stats(skb, a, 0))
> goto nla_put_failure;
>+ if (a->act_cookie) {
>+ if (nla_put(skb, TCA_ACT_COOKIE, a->act_cookie->len,
>+ a->act_cookie->data))
>+ goto nla_put_failure;
>+ }
>+
> nest = nla_nest_start(skb, TCA_OPTIONS);
> if (nest == NULL)
> goto nla_put_failure;
>@@ -516,6 +529,22 @@ int tcf_action_dump(struct sk_buff *skb, struct list_head *actions,
> return err;
> }
>
>+int nla_memdup_cookie(struct tc_action *a, struct nlattr **tb)
>+{
>+ a->act_cookie = kzalloc(sizeof(*a->act_cookie), GFP_KERNEL);
>+ if (!a->act_cookie)
>+ return -ENOMEM;
>+
>+ a->act_cookie->data = nla_memdup(tb[TCA_ACT_COOKIE], GFP_KERNEL);
you can do just:
size_t len = nla_len(tb[TCA_ACT_COOKIE];
a->act_cookie = kzalloc(sizeof(*a->act_cookie) + len, GFP_KERNEL);
if (!a->act_cookie)
return -ENOMEM;
memcpy(a->act_cookie->data, nla_data(tb[TCA_ACT_COOKIE], len));
a->act_cookie->len = len;
return 0;
Really see no need to alloc 2 chunks instead of one. But as you like.
>+ if (!a->act_cookie->data) {
>+ kfree(a->act_cookie);
>+ return -ENOMEM;
>+ }
>+ a->act_cookie->len = nla_len(tb[TCA_ACT_COOKIE]);
>+
>+ return 0;
>+}
>+
> struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
> struct nlattr *est, char *name, int ovr,
> int bind)
>@@ -575,6 +604,22 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
> if (err < 0)
> goto err_mod;
>
>+ if (tb[TCA_ACT_COOKIE]) {
>+ int cklen = nla_len(tb[TCA_ACT_COOKIE]);
>+
>+ if (cklen > TC_COOKIE_MAX_SIZE) {
>+ err = -EINVAL;
>+ tcf_hash_release(a, bind);
>+ goto err_mod;
>+ }
>+
>+ err = nla_memdup_cookie(a, tb);
>+ if (err < 0) {
You can do just "if (err)", but anyway:
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
>+ tcf_hash_release(a, bind);
>+ goto err_mod;
>+ }
>+ }
>+
> /* module count goes up only when brand new policy is created
> * if it exists and is only bound to in a_o->init() then
> * ACT_P_CREATED is not returned (a zero is).
>--
>1.9.1
>
^ permalink raw reply
* Re: [PATCH 2/3] ath10k: use dma_zalloc_coherent()
From: Valo, Kalle @ 2017-01-24 12:13 UTC (permalink / raw)
To: Joe Perches
Cc: Srinivas Kandagatla, ath10k@lists.infradead.org,
linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <1485235529.12563.39.camel@perches.com>
Joe Perches <joe@perches.com> writes:
> On Tue, 2017-01-24 at 05:18 +0000, Valo, Kalle wrote:
>> Joe Perches <joe@perches.com> writes:
>>
>> > On Mon, 2017-01-23 at 15:04 +0000, Srinivas Kandagatla wrote:
>> > > use dma_zalloc_coherent() instead of dma_alloc_coherent and memset().
>> >
>> > []
>> > > diff --git a/drivers/net/wireless/ath/ath10k/pci.c
>> > > b/drivers/net/wireless/ath/ath10k/pci.c
>> >
>> > []
>> > > @@ -896,7 +896,7 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data,
>> > > */
>> > > alloc_nbytes = min_t(unsigned int, nbytes, DIAG_TRANSFER_LIMIT);
>> > >
>> > > - data_buf = (unsigned char *)dma_alloc_coherent(ar->dev,
>> > > + data_buf = (unsigned char *)dma_zalloc_coherent(ar->dev,
>> > > alloc_nbytes,
>> > > &ce_data_base,
>> > > GFP_ATOMIC);
>> >
>> > trivia:
>> >
>> > Nicer to realign arguments and remove the unnecessary cast.
>> >
>> > Perhaps:
>> >
>> > data_buf = dma_zalloc_coherent(ar->dev, alloc_nbytes, &ce_data_base,
>> > GFP_ATOMIC);
>>
>> Sure, but that should be in a separate patch.
>
> I don't think so, trivial patches can be combined.
>
> It's also nicer to realign all modified multiline
> arguments when performing these changes.
>
> Coccinelle generally does it automatically.
A matter of preference really. I prefer keeping style and functional
changes in separate patches, keeps the review simple. And style changes
can hide bugs.
--
Kalle Valo
^ permalink raw reply
* [PATCH net-next v7 1/1] net sched actions: Add support for user cookies
From: Jamal Hadi Salim @ 2017-01-24 12:02 UTC (permalink / raw)
To: davem
Cc: netdev, jiri, paulb, john.fastabend, simon.horman, mrv, hadarh,
ogerlitz, roid, xiyou.wangcong, daniel, Jamal Hadi Salim
From: Jamal Hadi Salim <jhs@mojatatu.com>
Introduce optional 128-bit action cookie.
Like all other cookie schemes in the networking world (eg in protocols
like http or existing kernel fib protocol field, etc) the idea is to save
user state that when retrieved serves as a correlator. The kernel
_should not_ intepret it. The user can store whatever they wish in the
128 bits.
Sample exercise(showing variable length use of cookie)
.. create an accept action with cookie a1b2c3d4
sudo $TC actions add action ok index 1 cookie a1b2c3d4
.. dump all gact actions..
sudo $TC -s actions ls action gact
action order 0: gact action pass
random type none pass val 0
index 1 ref 1 bind 0 installed 5 sec used 5 sec
Action statistics:
Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
cookie a1b2c3d4
.. bind the accept action to a filter..
sudo $TC filter add dev lo parent ffff: protocol ip prio 1 \
u32 match ip dst 127.0.0.1/32 flowid 1:1 action gact index 1
... send some traffic..
$ ping 127.0.0.1 -c 3
PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data.
64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.020 ms
64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.027 ms
64 bytes from 127.0.0.1: icmp_seq=3 ttl=64 time=0.038 ms
--- 127.0.0.1 ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2109ms
rtt min/avg/max/mdev = 0.020/0.028/0.038/0.008 ms 1
... show some stats
$ sudo $TC -s actions get action gact index 1
action order 1: gact action pass
random type none pass val 0
index 1 ref 2 bind 1 installed 204 sec used 5 sec
Action statistics:
Sent 12168 bytes 164 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
cookie a1b2c3d4
.. try longer cookie...
$ sudo $TC actions replace action ok index 1 cookie 1234567890abcdef
.. dump..
$ sudo $TC -s actions ls action gact
action order 1: gact action pass
random type none pass val 0
index 1 ref 2 bind 1 installed 204 sec used 5 sec
Action statistics:
Sent 12168 bytes 164 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
cookie 1234567890abcdef
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
Changes in v7:
-put guard around freeing cookie (caught by Simon)
-separate out the creation of the cookie (suggested by Simon)
Changes in v6:
- fix mem leak caught by Florian
Changes in V5:
- kill the stylistic changes
- Adopt a new structure with length-valuepointer representation
- rename some things
Changes in v4:
- move stylistic changes out into a separate patch
(and add more stylistic changes)
Changes in v3:
- use TC_ prefix for the max size
- move the cookie struct so visible only to kernel
- remove unneeded void * cast
Changes in V2:
-move from a union to a length-value representation
include/net/act_api.h | 1 +
include/net/pkt_cls.h | 8 ++++++++
include/uapi/linux/pkt_cls.h | 3 +++
net/sched/act_api.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 57 insertions(+)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 1d71644..cfa2ae3 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -41,6 +41,7 @@ struct tc_action {
struct rcu_head tcfa_rcu;
struct gnet_stats_basic_cpu __percpu *cpu_bstats;
struct gnet_stats_queue __percpu *cpu_qstats;
+ struct tc_cookie *act_cookie;
};
#define tcf_head common.tcfa_head
#define tcf_index common.tcfa_index
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index f0a0514..b43077e 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -515,4 +515,12 @@ struct tc_cls_bpf_offload {
u32 gen_flags;
};
+
+/* This structure holds cookie structure that is passed from user
+ * to the kernel for actions and classifiers
+ */
+struct tc_cookie {
+ u8 *data;
+ u32 len;
+};
#endif
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index fd373eb..345551e 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -4,6 +4,8 @@
#include <linux/types.h>
#include <linux/pkt_sched.h>
+#define TC_COOKIE_MAX_SIZE 16
+
/* Action attributes */
enum {
TCA_ACT_UNSPEC,
@@ -12,6 +14,7 @@ enum {
TCA_ACT_INDEX,
TCA_ACT_STATS,
TCA_ACT_PAD,
+ TCA_ACT_COOKIE,
__TCA_ACT_MAX
};
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index cd08df9..3c5e29b 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -24,6 +24,7 @@
#include <net/net_namespace.h>
#include <net/sock.h>
#include <net/sch_generic.h>
+#include <net/pkt_cls.h>
#include <net/act_api.h>
#include <net/netlink.h>
@@ -33,6 +34,12 @@ static void free_tcf(struct rcu_head *head)
free_percpu(p->cpu_bstats);
free_percpu(p->cpu_qstats);
+
+ if (p->act_cookie) {
+ kfree(p->act_cookie->data);
+ kfree(p->act_cookie);
+ }
+
kfree(p);
}
@@ -475,6 +482,12 @@ int tcf_action_destroy(struct list_head *actions, int bind)
goto nla_put_failure;
if (tcf_action_copy_stats(skb, a, 0))
goto nla_put_failure;
+ if (a->act_cookie) {
+ if (nla_put(skb, TCA_ACT_COOKIE, a->act_cookie->len,
+ a->act_cookie->data))
+ goto nla_put_failure;
+ }
+
nest = nla_nest_start(skb, TCA_OPTIONS);
if (nest == NULL)
goto nla_put_failure;
@@ -516,6 +529,22 @@ int tcf_action_dump(struct sk_buff *skb, struct list_head *actions,
return err;
}
+int nla_memdup_cookie(struct tc_action *a, struct nlattr **tb)
+{
+ a->act_cookie = kzalloc(sizeof(*a->act_cookie), GFP_KERNEL);
+ if (!a->act_cookie)
+ return -ENOMEM;
+
+ a->act_cookie->data = nla_memdup(tb[TCA_ACT_COOKIE], GFP_KERNEL);
+ if (!a->act_cookie->data) {
+ kfree(a->act_cookie);
+ return -ENOMEM;
+ }
+ a->act_cookie->len = nla_len(tb[TCA_ACT_COOKIE]);
+
+ return 0;
+}
+
struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
struct nlattr *est, char *name, int ovr,
int bind)
@@ -575,6 +604,22 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
if (err < 0)
goto err_mod;
+ if (tb[TCA_ACT_COOKIE]) {
+ int cklen = nla_len(tb[TCA_ACT_COOKIE]);
+
+ if (cklen > TC_COOKIE_MAX_SIZE) {
+ err = -EINVAL;
+ tcf_hash_release(a, bind);
+ goto err_mod;
+ }
+
+ err = nla_memdup_cookie(a, tb);
+ if (err < 0) {
+ tcf_hash_release(a, bind);
+ goto err_mod;
+ }
+ }
+
/* module count goes up only when brand new policy is created
* if it exists and is only bound to in a_o->init() then
* ACT_P_CREATED is not returned (a zero is).
--
1.9.1
^ permalink raw reply related
* [PATCH net v1 1/6] tipc: fix nametbl_lock soft lockup at node/link events
From: Parthasarathy Bhuvaragan @ 2017-01-24 12:00 UTC (permalink / raw)
To: netdev; +Cc: tipc-discussion, jon.maloy, ying.xue
In-Reply-To: <1485259248-21812-1-git-send-email-parthasarathy.bhuvaragan@ericsson.com>
We trigger a soft lockup as we grab nametbl_lock twice if the node
has a pending node up/down or link up/down event while:
- we process an incoming named message in tipc_named_rcv() and
perform an tipc_update_nametbl().
- we have pending backlog items in the name distributor queue
during a nametable update using tipc_nametbl_publish() or
tipc_nametbl_withdraw().
The following are the call chain associated:
tipc_named_rcv() Grabs nametbl_lock
tipc_update_nametbl() (publish/withdraw)
tipc_node_subscribe()/unsubscribe()
tipc_node_write_unlock()
<< lockup occurs if an outstanding node/link event
exits, as we grabs nametbl_lock again >>
tipc_nametbl_withdraw() Grab nametbl_lock
tipc_named_process_backlog()
tipc_update_nametbl()
<< rest as above >>
The function tipc_node_write_unlock(), in addition to releasing the
lock processes the outstanding node/link up/down events. To do this,
we need to grab the nametbl_lock again leading to the lockup.
In this commit we fix the soft lockup by introducing a fast variant of
node_unlock(), where we just release the lock. We adapt the
node_subscribe()/node_unsubscribe() to use the fast variants.
Reported-and-Tested-by: John Thompson <thompa.atl@gmail.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
Acked-by: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
---
net/tipc/node.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/net/tipc/node.c b/net/tipc/node.c
index 9d2f4c2b08ab..27753325e06e 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -263,6 +263,11 @@ static void tipc_node_write_lock(struct tipc_node *n)
write_lock_bh(&n->lock);
}
+static void tipc_node_write_unlock_fast(struct tipc_node *n)
+{
+ write_unlock_bh(&n->lock);
+}
+
static void tipc_node_write_unlock(struct tipc_node *n)
{
struct net *net = n->net;
@@ -417,7 +422,7 @@ void tipc_node_subscribe(struct net *net, struct list_head *subscr, u32 addr)
}
tipc_node_write_lock(n);
list_add_tail(subscr, &n->publ_list);
- tipc_node_write_unlock(n);
+ tipc_node_write_unlock_fast(n);
tipc_node_put(n);
}
@@ -435,7 +440,7 @@ void tipc_node_unsubscribe(struct net *net, struct list_head *subscr, u32 addr)
}
tipc_node_write_lock(n);
list_del_init(subscr);
- tipc_node_write_unlock(n);
+ tipc_node_write_unlock_fast(n);
tipc_node_put(n);
}
--
2.1.4
^ permalink raw reply related
* [PATCH net v1 2/6] tipc: add subscription refcount to avoid invalid delete
From: Parthasarathy Bhuvaragan @ 2017-01-24 12:00 UTC (permalink / raw)
To: netdev; +Cc: tipc-discussion, jon.maloy, ying.xue
In-Reply-To: <1485259248-21812-1-git-send-email-parthasarathy.bhuvaragan@ericsson.com>
Until now, the subscribers keep track of the subscriptions using
reference count at subscriber level. At subscription cancel or
subscriber delete, we delete the subscription only if the timer
was pending for the subscription. This approach is incorrect as:
1. del_timer() is not SMP safe, if on CPU0 the check for pending
timer returns true but CPU1 might schedule the timer callback
thereby deleting the subscription. Thus when CPU0 is scheduled,
it deletes an invalid subscription.
2. We export tipc_subscrp_report_overlap(), which accesses the
subscription pointer multiple times. Meanwhile the subscription
timer can expire thereby freeing the subscription and we might
continue to access the subscription pointer leading to memory
violations.
In this commit, we introduce subscription refcount to avoid deleting
an invalid subscription.
Reported-and-Tested-by: John Thompson <thompa.atl@gmail.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
Acked-by: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
---
net/tipc/subscr.c | 124 ++++++++++++++++++++++++++++++------------------------
net/tipc/subscr.h | 1 +
2 files changed, 71 insertions(+), 54 deletions(-)
diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
index 0dd02244e21d..9d94e65d0894 100644
--- a/net/tipc/subscr.c
+++ b/net/tipc/subscr.c
@@ -54,6 +54,8 @@ struct tipc_subscriber {
static void tipc_subscrp_delete(struct tipc_subscription *sub);
static void tipc_subscrb_put(struct tipc_subscriber *subscriber);
+static void tipc_subscrp_put(struct tipc_subscription *subscription);
+static void tipc_subscrp_get(struct tipc_subscription *subscription);
/**
* htohl - convert value to endianness used by destination
@@ -123,6 +125,7 @@ void tipc_subscrp_report_overlap(struct tipc_subscription *sub, u32 found_lower,
{
struct tipc_name_seq seq;
+ tipc_subscrp_get(sub);
tipc_subscrp_convert_seq(&sub->evt.s.seq, sub->swap, &seq);
if (!tipc_subscrp_check_overlap(&seq, found_lower, found_upper))
return;
@@ -132,30 +135,23 @@ void tipc_subscrp_report_overlap(struct tipc_subscription *sub, u32 found_lower,
tipc_subscrp_send_event(sub, found_lower, found_upper, event, port_ref,
node);
+ tipc_subscrp_put(sub);
}
static void tipc_subscrp_timeout(unsigned long data)
{
struct tipc_subscription *sub = (struct tipc_subscription *)data;
- struct tipc_subscriber *subscriber = sub->subscriber;
/* Notify subscriber of timeout */
tipc_subscrp_send_event(sub, sub->evt.s.seq.lower, sub->evt.s.seq.upper,
TIPC_SUBSCR_TIMEOUT, 0, 0);
- spin_lock_bh(&subscriber->lock);
- tipc_subscrp_delete(sub);
- spin_unlock_bh(&subscriber->lock);
-
- tipc_subscrb_put(subscriber);
+ tipc_subscrp_put(sub);
}
static void tipc_subscrb_kref_release(struct kref *kref)
{
- struct tipc_subscriber *subcriber = container_of(kref,
- struct tipc_subscriber, kref);
-
- kfree(subcriber);
+ kfree(container_of(kref,struct tipc_subscriber, kref));
}
static void tipc_subscrb_put(struct tipc_subscriber *subscriber)
@@ -168,6 +164,59 @@ static void tipc_subscrb_get(struct tipc_subscriber *subscriber)
kref_get(&subscriber->kref);
}
+static void tipc_subscrp_kref_release(struct kref *kref)
+{
+ struct tipc_subscription *sub = container_of(kref,
+ struct tipc_subscription,
+ kref);
+ struct tipc_net *tn = net_generic(sub->net, tipc_net_id);
+ struct tipc_subscriber *subscriber = sub->subscriber;
+
+ spin_lock_bh(&subscriber->lock);
+ tipc_nametbl_unsubscribe(sub);
+ list_del(&sub->subscrp_list);
+ atomic_dec(&tn->subscription_count);
+ spin_unlock_bh(&subscriber->lock);
+ kfree(sub);
+ tipc_subscrb_put(subscriber);
+}
+
+static void tipc_subscrp_put(struct tipc_subscription *subscription)
+{
+ kref_put(&subscription->kref, tipc_subscrp_kref_release);
+}
+
+static void tipc_subscrp_get(struct tipc_subscription *subscription)
+{
+ kref_get(&subscription->kref);
+}
+
+/* tipc_subscrb_subscrp_delete - delete a specific subscription or all
+ * subscriptions for a given subscriber.
+ */
+static void tipc_subscrb_subscrp_delete(struct tipc_subscriber *subscriber,
+ struct tipc_subscr *s)
+{
+ struct list_head *subscription_list = &subscriber->subscrp_list;
+ struct tipc_subscription *sub, *temp;
+
+ spin_lock_bh(&subscriber->lock);
+ list_for_each_entry_safe(sub, temp, subscription_list, subscrp_list) {
+ if (s && memcmp(s, &sub->evt.s, sizeof(struct tipc_subscr)))
+ continue;
+
+ tipc_subscrp_get(sub);
+ spin_unlock_bh(&subscriber->lock);
+ tipc_subscrp_delete(sub);
+ tipc_subscrp_put(sub);
+ spin_lock_bh(&subscriber->lock);
+
+ if (s)
+ break;
+ }
+ spin_unlock_bh(&subscriber->lock);
+}
+
static struct tipc_subscriber *tipc_subscrb_create(int conid)
{
struct tipc_subscriber *subscriber;
@@ -177,8 +226,8 @@ static struct tipc_subscriber *tipc_subscrb_create(int conid)
pr_warn("Subscriber rejected, no memory\n");
return NULL;
}
- kref_init(&subscriber->kref);
INIT_LIST_HEAD(&subscriber->subscrp_list);
+ kref_init(&subscriber->kref);
subscriber->conid = conid;
spin_lock_init(&subscriber->lock);
@@ -187,55 +236,22 @@ static struct tipc_subscriber *tipc_subscrb_create(int conid)
static void tipc_subscrb_delete(struct tipc_subscriber *subscriber)
{
- struct tipc_subscription *sub, *temp;
- u32 timeout;
-
- spin_lock_bh(&subscriber->lock);
- /* Destroy any existing subscriptions for subscriber */
- list_for_each_entry_safe(sub, temp, &subscriber->subscrp_list,
- subscrp_list) {
- timeout = htohl(sub->evt.s.timeout, sub->swap);
- if ((timeout == TIPC_WAIT_FOREVER) || del_timer(&sub->timer)) {
- tipc_subscrp_delete(sub);
- tipc_subscrb_put(subscriber);
- }
- }
- spin_unlock_bh(&subscriber->lock);
-
+ tipc_subscrb_subscrp_delete(subscriber, NULL);
tipc_subscrb_put(subscriber);
}
static void tipc_subscrp_delete(struct tipc_subscription *sub)
{
- struct tipc_net *tn = net_generic(sub->net, tipc_net_id);
+ u32 timeout = htohl(sub->evt.s.timeout, sub->swap);
- tipc_nametbl_unsubscribe(sub);
- list_del(&sub->subscrp_list);
- kfree(sub);
- atomic_dec(&tn->subscription_count);
+ if (timeout == TIPC_WAIT_FOREVER || del_timer(&sub->timer))
+ tipc_subscrp_put(sub);
}
static void tipc_subscrp_cancel(struct tipc_subscr *s,
struct tipc_subscriber *subscriber)
{
- struct tipc_subscription *sub, *temp;
- u32 timeout;
-
- spin_lock_bh(&subscriber->lock);
- /* Find first matching subscription, exit if not found */
- list_for_each_entry_safe(sub, temp, &subscriber->subscrp_list,
- subscrp_list) {
- if (!memcmp(s, &sub->evt.s, sizeof(struct tipc_subscr))) {
- timeout = htohl(sub->evt.s.timeout, sub->swap);
- if ((timeout == TIPC_WAIT_FOREVER) ||
- del_timer(&sub->timer)) {
- tipc_subscrp_delete(sub);
- tipc_subscrb_put(subscriber);
- }
- break;
- }
- }
- spin_unlock_bh(&subscriber->lock);
+ tipc_subscrb_subscrp_delete(subscriber, s);
}
static struct tipc_subscription *tipc_subscrp_create(struct net *net,
@@ -272,6 +288,7 @@ static struct tipc_subscription *tipc_subscrp_create(struct net *net,
sub->swap = swap;
memcpy(&sub->evt.s, s, sizeof(*s));
atomic_inc(&tn->subscription_count);
+ kref_init(&sub->kref);
return sub;
}
@@ -288,17 +305,16 @@ static void tipc_subscrp_subscribe(struct net *net, struct tipc_subscr *s,
spin_lock_bh(&subscriber->lock);
list_add(&sub->subscrp_list, &subscriber->subscrp_list);
- tipc_subscrb_get(subscriber);
sub->subscriber = subscriber;
tipc_nametbl_subscribe(sub);
+ tipc_subscrb_get(subscriber);
spin_unlock_bh(&subscriber->lock);
+ setup_timer(&sub->timer, tipc_subscrp_timeout, (unsigned long)sub);
timeout = htohl(sub->evt.s.timeout, swap);
- if (timeout == TIPC_WAIT_FOREVER)
- return;
- setup_timer(&sub->timer, tipc_subscrp_timeout, (unsigned long)sub);
- mod_timer(&sub->timer, jiffies + msecs_to_jiffies(timeout));
+ if (timeout != TIPC_WAIT_FOREVER)
+ mod_timer(&sub->timer, jiffies + msecs_to_jiffies(timeout));
}
/* Handle one termination request for the subscriber */
diff --git a/net/tipc/subscr.h b/net/tipc/subscr.h
index be60103082c9..ffdc214c117a 100644
--- a/net/tipc/subscr.h
+++ b/net/tipc/subscr.h
@@ -57,6 +57,7 @@ struct tipc_subscriber;
* @evt: template for events generated by subscription
*/
struct tipc_subscription {
+ struct kref kref;
struct tipc_subscriber *subscriber;
struct net *net;
struct timer_list timer;
--
2.1.4
^ permalink raw reply related
* [PATCH net v1 0/6] topology server fixes for nametable soft lockup
From: Parthasarathy Bhuvaragan @ 2017-01-24 12:00 UTC (permalink / raw)
To: netdev; +Cc: tipc-discussion, jon.maloy, ying.xue
In this series, we revert the commit 333f796235a527 ("tipc: fix a
race condition leading to subscriber refcnt bug") and provide an
alternate solution to fix the race conditions in commits 2-4.
We have to do this as the above commit introduced a nametbl soft
lockup at module exit as described by patch#4.
Parthasarathy Bhuvaragan (6):
tipc: fix nametbl_lock soft lockup at node/link events
tipc: add subscription refcount to avoid invalid delete
tipc: fix connection refcount error
tipc: fix nametbl_lock soft lockup at module exit
tipc: ignore requests when the connection state is not CONNECTED
tipc: fix cleanup at module unload
net/tipc/node.c | 9 +++-
net/tipc/server.c | 48 +++++++++------------
net/tipc/subscr.c | 124 ++++++++++++++++++++++++++++++------------------------
net/tipc/subscr.h | 1 +
4 files changed, 99 insertions(+), 83 deletions(-)
--
2.1.4
^ permalink raw reply
* [PATCH net v1 6/6] tipc: fix cleanup at module unload
From: Parthasarathy Bhuvaragan @ 2017-01-24 12:00 UTC (permalink / raw)
To: netdev; +Cc: jon.maloy, tipc-discussion
In-Reply-To: <1485259248-21812-1-git-send-email-parthasarathy.bhuvaragan@ericsson.com>
In tipc_server_stop(), we iterate over the connections with limiting
factor as server's idr_in_use. We ignore the fact that this variable
is decremented in tipc_close_conn(), leading to premature exit.
In this commit, we iterate until the we have no connections left.
Acked-by: Ying Xue <ying.xue@windriver.com>
Acked-by: Jon Maloy <jon.maloy@ericsson.com>
Tested-by: John Thompson <thompa.atl@gmail.com>
Signed-off-by: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
---
net/tipc/server.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/net/tipc/server.c b/net/tipc/server.c
index 04ff441b8065..3cd6402e812c 100644
--- a/net/tipc/server.c
+++ b/net/tipc/server.c
@@ -619,14 +619,12 @@ int tipc_server_start(struct tipc_server *s)
void tipc_server_stop(struct tipc_server *s)
{
struct tipc_conn *con;
- int total = 0;
int id;
spin_lock_bh(&s->idr_lock);
- for (id = 0; total < s->idr_in_use; id++) {
+ for (id = 0; s->idr_in_use; id++) {
con = idr_find(&s->conn_idr, id);
if (con) {
- total++;
spin_unlock_bh(&s->idr_lock);
tipc_close_conn(con);
spin_lock_bh(&s->idr_lock);
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
^ permalink raw reply related
* [PATCH net v1 5/6] tipc: ignore requests when the connection state is not CONNECTED
From: Parthasarathy Bhuvaragan @ 2017-01-24 12:00 UTC (permalink / raw)
To: netdev; +Cc: jon.maloy, tipc-discussion
In-Reply-To: <1485259248-21812-1-git-send-email-parthasarathy.bhuvaragan@ericsson.com>
In tipc_conn_sendmsg(), we first queue the request to the outqueue
followed by the connection state check. If the connection is not
connected, we should not queue this message.
In this commit, we reject the messages if the connection state is
not CF_CONNECTED.
Acked-by: Ying Xue <ying.xue@windriver.com>
Acked-by: Jon Maloy <jon.maloy@ericsson.com>
Tested-by: John Thompson <thompa.atl@gmail.com>
Signed-off-by: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
---
net/tipc/server.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/net/tipc/server.c b/net/tipc/server.c
index 826cde2c401e..04ff441b8065 100644
--- a/net/tipc/server.c
+++ b/net/tipc/server.c
@@ -453,6 +453,11 @@ int tipc_conn_sendmsg(struct tipc_server *s, int conid,
if (!con)
return -EINVAL;
+ if (!test_bit(CF_CONNECTED, &con->flags)) {
+ conn_put(con);
+ return 0;
+ }
+
e = tipc_alloc_entry(data, len);
if (!e) {
conn_put(con);
@@ -466,12 +471,8 @@ int tipc_conn_sendmsg(struct tipc_server *s, int conid,
list_add_tail(&e->list, &con->outqueue);
spin_unlock_bh(&con->outqueue_lock);
- if (test_bit(CF_CONNECTED, &con->flags)) {
- if (!queue_work(s->send_wq, &con->swork))
- conn_put(con);
- } else {
+ if (!queue_work(s->send_wq, &con->swork))
conn_put(con);
- }
return 0;
}
@@ -495,7 +496,7 @@ static void tipc_send_to_sock(struct tipc_conn *con)
int ret;
spin_lock_bh(&con->outqueue_lock);
- while (1) {
+ while (test_bit(CF_CONNECTED, &con->flags)) {
e = list_entry(con->outqueue.next, struct outqueue_entry,
list);
if ((struct list_head *) e == &con->outqueue)
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
^ permalink raw reply related
* [PATCH net v1 4/6] tipc: fix nametbl_lock soft lockup at module exit
From: Parthasarathy Bhuvaragan @ 2017-01-24 12:00 UTC (permalink / raw)
To: netdev; +Cc: jon.maloy, tipc-discussion
In-Reply-To: <1485259248-21812-1-git-send-email-parthasarathy.bhuvaragan@ericsson.com>
Commit 333f796235a527 ("tipc: fix a race condition leading to
subscriber refcnt bug") reveals a soft lockup while acquiring
nametbl_lock.
Before commit 333f796235a527, we call tipc_conn_shutdown() from
tipc_close_conn() in the context of tipc_topsrv_stop(). In that
context, we are allowed to grab the nametbl_lock.
Commit 333f796235a527, moved tipc_conn_release (renamed from
tipc_conn_shutdown) to the connection refcount cleanup. This allows
either tipc_nametbl_withdraw() or tipc_topsrv_stop() to the cleanup.
Since tipc_exit_net() first calls tipc_topsrv_stop() and then
tipc_nametble_withdraw() increases the chances for the later to
perform the connection cleanup.
The soft lockup occurs in the call chain of tipc_nametbl_withdraw(),
when it performs the tipc_conn_kref_release() as it tries to grab
nametbl_lock again while holding it already.
tipc_nametbl_withdraw() grabs nametbl_lock
tipc_nametbl_remove_publ()
tipc_subscrp_report_overlap()
tipc_subscrp_send_event()
tipc_conn_sendmsg()
<< if (con->flags != CF_CONNECTED) we do conn_put(),
triggering the cleanup as refcount=0. >>
tipc_conn_kref_release
tipc_sock_release
tipc_conn_release
tipc_subscrb_delete
tipc_subscrp_delete
tipc_nametbl_unsubscribe << Soft Lockup >>
The previous changes in this series fixes the race conditions fixed
by commit 333f796235a527. Hence we can now revert the commit.
Fixes: 333f796235a52727 ("tipc: fix a race condition leading to subscriber refcnt bug")
Reported-and-Tested-by: John Thompson <thompa.atl@gmail.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
Acked-by: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
---
net/tipc/server.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/net/tipc/server.c b/net/tipc/server.c
index 2e803601aa99..826cde2c401e 100644
--- a/net/tipc/server.c
+++ b/net/tipc/server.c
@@ -86,7 +86,6 @@ struct outqueue_entry {
static void tipc_recv_work(struct work_struct *work);
static void tipc_send_work(struct work_struct *work);
static void tipc_clean_outqueues(struct tipc_conn *con);
-static void tipc_sock_release(struct tipc_conn *con);
static void tipc_conn_kref_release(struct kref *kref)
{
@@ -104,7 +103,6 @@ static void tipc_conn_kref_release(struct kref *kref)
}
saddr->scope = -TIPC_NODE_SCOPE;
kernel_bind(sock, (struct sockaddr *)saddr, sizeof(*saddr));
- tipc_sock_release(con);
sock_release(sock);
con->sock = NULL;
@@ -194,19 +192,15 @@ static void tipc_unregister_callbacks(struct tipc_conn *con)
write_unlock_bh(&sk->sk_callback_lock);
}
-static void tipc_sock_release(struct tipc_conn *con)
+static void tipc_close_conn(struct tipc_conn *con)
{
struct tipc_server *s = con->server;
- if (con->conid)
- s->tipc_conn_release(con->conid, con->usr_data);
-
- tipc_unregister_callbacks(con);
-}
-
-static void tipc_close_conn(struct tipc_conn *con)
-{
if (test_and_clear_bit(CF_CONNECTED, &con->flags)) {
+ tipc_unregister_callbacks(con);
+
+ if (con->conid)
+ s->tipc_conn_release(con->conid, con->usr_data);
/* We shouldn't flush pending works as we may be in the
* thread. In fact the races with pending rx/tx work structs
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
^ 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