from:"jeffy"

[RFC PATCH v12 3/5] mwifiex: Disable wakeup irq handling for pcie

2017-12-25 Thread Jeffy Chen

We are going to handle the wakeup irq in the pci core.

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

Changes in v13: None
Changes in v12: None
Changes in v11: None
Changes in v10: None
Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v3: None
Changes in v2: None

 drivers/net/wireless/marvell/mwifiex/main.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index a96bd7e653bf..3cc3403b977a 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1567,6 +1567,10 @@ static void mwifiex_probe_of(struct mwifiex_adapter 
*adapter)
goto err_exit;
 
adapter->dt_node = dev->of_node;
+
+   if (adapter->iface_type != MWIFIEX_PCIE)
+   goto err_exit;
+
adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
if (!adapter->irq_wakeup) {
dev_dbg(dev, "fail to parse irq_wakeup from device tree\n");
-- 
2.11.0

[RFC PATCH v12 0/5] PCI: rockchip: Move PCIe WAKE# handling into pci core

2017-12-25 Thread Jeffy Chen


Currently we are handling wake irq in mrvl wifi driver. Move it into
pci core.

Tested on my chromebook bob(with cros 4.4 kernel and mrvl wifi).


Changes in v13:
Fix compiler error reported by kbuild test robot <fengguang...@intel.com>

Changes in v12:
Only add irq definitions for PCI devices and rewrite the commit message.
Enable the wake irq in noirq stage to avoid possible irq storm.

Changes in v11:
Address Brian's comments.
Only support 1-per-device PCIe WAKE# pin as suggested.
Move to pcie port as Brian suggested.

Changes in v10:
Use device_set_wakeup_capable() instead of device_set_wakeup_enable(),
since dedicated wakeirq will be lost in device_set_wakeup_enable(false).

Changes in v9:
Add section for PCI devices and rewrite the commit message.
Fix check error in .cleanup().
Move dedicated wakeirq setup to setup() callback and use
device_set_wakeup_enable() to enable/disable.
Rewrite the commit message.

Changes in v8:
Add optional "pci", and rewrite commit message.
Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.
Rewrite the commit message.

Changes in v7:
Move PCIE_WAKE handling into pci core.

Changes in v6:
Fix device_init_wake error handling, and add some comments.

Changes in v5:
Move to pci.txt
Rebase.
Use "wakeup" instead of "wake"

Changes in v3:
Fix error handling.

Changes in v2:
Use dev_pm_set_dedicated_wake_irq.

Jeffy Chen (5):
  dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
  of/irq: Adjust of_pci_irq parsing for multiple interrupts
  mwifiex: Disable wakeup irq handling for pcie
  PCI / PM: Add support for the PCIe WAKE# signal for OF
  arm64: dts: rockchip: Move PCIe WAKE# irq to pcie port for Gru

 Documentation/devicetree/bindings/pci/pci.txt | 10 
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi  | 11 ++--
 drivers/net/wireless/marvell/mwifiex/main.c   |  4 ++
 drivers/of/of_pci_irq.c   | 71 +++--
 drivers/pci/Makefile  |  1 +
 drivers/pci/pci-driver.c  | 10 
 drivers/pci/pci-of.c  | 75 +++
 include/linux/of_pci.h|  9 
 8 files changed, 183 insertions(+), 8 deletions(-)
 create mode 100644 drivers/pci/pci-of.c

-- 
2.11.0

[RFC PATCH v12 3/5] mwifiex: Disable wakeup irq handling for pcie

2017-12-25 Thread Jeffy Chen

We are going to handle the wakeup irq in the pci core.

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

Changes in v12: None
Changes in v11: None
Changes in v10: None
Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v3: None
Changes in v2: None

 drivers/net/wireless/marvell/mwifiex/main.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index a96bd7e653bf..3cc3403b977a 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1567,6 +1567,10 @@ static void mwifiex_probe_of(struct mwifiex_adapter 
*adapter)
goto err_exit;
 
adapter->dt_node = dev->of_node;
+
+   if (adapter->iface_type != MWIFIEX_PCIE)
+   goto err_exit;
+
adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
if (!adapter->irq_wakeup) {
dev_dbg(dev, "fail to parse irq_wakeup from device tree\n");
-- 
2.11.0

[RFC PATCH v12 0/5] PCI: rockchip: Move PCIe WAKE# handling into pci core

2017-12-25 Thread Jeffy Chen


Currently we are handling wake irq in mrvl wifi driver. Move it into
pci core.

Tested on my chromebook bob(with cros 4.4 kernel and mrvl wifi).


Changes in v12:
Enable the wake irq in noirq stage to avoid possible irq storm.

Changes in v11:
Only add irq definitions for PCI devices and rewrite the commit message.
Address Brian's comments.
Only support 1-per-device PCIe WAKE# pin as suggested.
Move to pcie port as Brian suggested.

Changes in v10:
Use device_set_wakeup_capable() instead of device_set_wakeup_enable(),
since dedicated wakeirq will be lost in device_set_wakeup_enable(false).

Changes in v9:
Add section for PCI devices and rewrite the commit message.
Fix check error in .cleanup().
Move dedicated wakeirq setup to setup() callback and use
device_set_wakeup_enable() to enable/disable.
Rewrite the commit message.

Changes in v8:
Add optional "pci", and rewrite commit message.
Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.
Rewrite the commit message.

Changes in v7:
Move PCIE_WAKE handling into pci core.

Changes in v6:
Fix device_init_wake error handling, and add some comments.

Changes in v5:
Move to pci.txt
Rebase.
Use "wakeup" instead of "wake"

Changes in v3:
Fix error handling.

Changes in v2:
Use dev_pm_set_dedicated_wake_irq.

Jeffy Chen (5):
  dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
  of/irq: Adjust of_pci_irq parsing for multiple interrupts
  mwifiex: Disable wakeup irq handling for pcie
  PCI / PM: Add support for the PCIe WAKE# signal for OF
  arm64: dts: rockchip: Move PCIe WAKE# irq to pcie port for Gru

 Documentation/devicetree/bindings/pci/pci.txt | 10 
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi  | 11 ++--
 drivers/net/wireless/marvell/mwifiex/main.c   |  4 ++
 drivers/of/of_pci_irq.c   | 74 --
 drivers/pci/Makefile  |  1 +
 drivers/pci/pci-driver.c  | 10 
 drivers/pci/pci-of.c  | 75 +++
 include/linux/of_pci.h|  9 
 8 files changed, 186 insertions(+), 8 deletions(-)
 create mode 100644 drivers/pci/pci-of.c

-- 
2.11.0

[RFC PATCH v11 3/5] mwifiex: Disable wakeup irq handling for pcie

2017-12-25 Thread Jeffy Chen

We are going to handle the wakeup irq in the pci core.

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

Changes in v11: None
Changes in v10: None
Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v3: None
Changes in v2: None

 drivers/net/wireless/marvell/mwifiex/main.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index a96bd7e653bf..3cc3403b977a 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1567,6 +1567,10 @@ static void mwifiex_probe_of(struct mwifiex_adapter 
*adapter)
goto err_exit;
 
adapter->dt_node = dev->of_node;
+
+   if (adapter->iface_type != MWIFIEX_PCIE)
+   goto err_exit;
+
adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
if (!adapter->irq_wakeup) {
dev_dbg(dev, "fail to parse irq_wakeup from device tree\n");
-- 
2.11.0

[RFC PATCH v11 0/5] PCI: rockchip: Move PCIe WAKE# handling into pci core

2017-12-25 Thread Jeffy Chen


Currently we are handling wake irq in mrvl wifi driver. Move it into
pci core.

Tested on my chromebook bob(with cros 4.4 kernel and mrvl wifi).


Changes in v11:
Only add irq definitions for PCI devices and rewrite the commit message.
Address Brian's comments.
Only support 1-per-device PCIe WAKE# pin as suggested.
Move to pcie port as Brian suggested.

Changes in v10:
Use device_set_wakeup_capable() instead of device_set_wakeup_enable(),
since dedicated wakeirq will be lost in device_set_wakeup_enable(false).

Changes in v9:
Add section for PCI devices and rewrite the commit message.
Fix check error in .cleanup().
Move dedicated wakeirq setup to setup() callback and use
device_set_wakeup_enable() to enable/disable.
Rewrite the commit message.

Changes in v8:
Add optional "pci", and rewrite commit message.
Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.
Rewrite the commit message.

Changes in v7:
Move PCIE_WAKE handling into pci core.

Changes in v6:
Fix device_init_wake error handling, and add some comments.

Changes in v5:
Move to pci.txt
Rebase.
Use "wakeup" instead of "wake"

Changes in v3:
Fix error handling.

Changes in v2:
Use dev_pm_set_dedicated_wake_irq.

Jeffy Chen (5):
  dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
  of/irq: Adjust of_pci_irq parsing for multiple interrupts
  mwifiex: Disable wakeup irq handling for pcie
  PCI / PM: Add support for the PCIe WAKE# signal for OF
  arm64: dts: rockchip: Move PCIe WAKE# irq to pcie port for Gru

 Documentation/devicetree/bindings/pci/pci.txt | 10 
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi  | 11 +++--
 drivers/net/wireless/marvell/mwifiex/main.c   |  4 ++
 drivers/of/of_pci_irq.c   | 71 +--
 drivers/pci/pci-driver.c  | 10 
 include/linux/of_pci.h|  9 
 6 files changed, 107 insertions(+), 8 deletions(-)

-- 
2.11.0

Re: [RFC PATCH v10 0/7] PCI: rockchip: Move PCIe WAKE# handling into pci core

2017-10-29 Thread jeffy


Hi Rafael,

thanks for your reply.

On 10/28/2017 05:07 PM, Rafael J. Wysocki wrote:

Overall, I don't quite like the direction this is going into, but I need to
have a deeper look.  Which may take some time, so please bear with me.


ok, i'll wait for your comments, thanks :)


Thanks,
Rafael

[RFC PATCH v10 3/7] mwifiex: Disable wakeup irq handling for pcie

2017-10-27 Thread Jeffy Chen

We are going to handle the wakeup irq in the pci core.

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

Changes in v10: None
Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v3: None
Changes in v2: None

 drivers/net/wireless/marvell/mwifiex/main.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index ee40b739b289..ba081c16f85c 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1568,6 +1568,10 @@ static void mwifiex_probe_of(struct mwifiex_adapter 
*adapter)
goto err_exit;
 
adapter->dt_node = dev->of_node;
+
+   if (adapter->iface_type != MWIFIEX_PCIE)
+   goto err_exit;
+
adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
if (!adapter->irq_wakeup) {
dev_dbg(dev, "fail to parse irq_wakeup from device tree\n");
-- 
2.11.0

[RFC PATCH v10 0/7] PCI: rockchip: Move PCIe WAKE# handling into pci core

2017-10-27 Thread Jeffy Chen


Currently we are handling wake irq in mrvl wifi driver. Move it into
pci core.

Tested on my chromebook bob(with cros 4.4 kernel and mrvl wifi).


Changes in v10:
Use device_set_wakeup_capable() instead of device_set_wakeup_enable(),
since dedicated wakeirq will be lost in device_set_wakeup_enable(false).

Changes in v9:
Add section for PCI devices and rewrite the commit message.
Rewrite the commit message.
Fix check error in .cleanup().
Move dedicated wakeirq setup to setup() callback and use
device_set_wakeup_enable() to enable/disable.

Changes in v8:
Add optional "pci", and rewrite commit message.
Rewrite the commit message.
Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.

Changes in v7:
Move PCIE_WAKE handling into pci core.

Changes in v6:
Fix device_init_wake error handling, and add some comments.

Changes in v5:
Move to pci.txt
Use "wakeup" instead of "wake"
Rebase.

Changes in v3:
Fix error handling.

Changes in v2:
Use dev_pm_set_dedicated_wake_irq.

Jeffy Chen (7):
  dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
  of/irq: Adjust of_pci_irq parsing for multiple interrupts
  mwifiex: Disable wakeup irq handling for pcie
  arm64: dts: rockchip: Move PCIe WAKE# irq to pcie driver for Gru
  PCI: Make pci_platform_pm_ops's callbacks optional
  PCI / PM: Move acpi wakeup code to pci core
  PCI / PM: Add support for the PCIe WAKE# signal for OF

 Documentation/devicetree/bindings/pci/pci.txt |   8 ++
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi  |  15 +--
 drivers/net/wireless/marvell/mwifiex/main.c   |   4 +
 drivers/of/of_pci_irq.c   |  13 ++-
 drivers/pci/Makefile  |   2 +-
 drivers/pci/pci-acpi.c| 121 
 drivers/pci/pci-driver.c  |   9 ++
 drivers/pci/pci-of.c  | 127 ++
 drivers/pci/pci.c | 112 +++
 drivers/pci/pci.h |  31 +--
 drivers/pci/probe.c   |  12 ++-
 drivers/pci/remove.c  |   2 +
 include/linux/pci.h   |   2 +
 13 files changed, 361 insertions(+), 97 deletions(-)
 create mode 100644 drivers/pci/pci-of.c

-- 
2.11.0

[RFC PATCH v9 3/7] mwifiex: Disable wakeup irq handling for pcie

2017-10-27 Thread Jeffy Chen

We are going to handle the wakeup irq in the pci core.

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v3: None
Changes in v2: None

 drivers/net/wireless/marvell/mwifiex/main.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index ee40b739b289..ba081c16f85c 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1568,6 +1568,10 @@ static void mwifiex_probe_of(struct mwifiex_adapter 
*adapter)
goto err_exit;
 
adapter->dt_node = dev->of_node;
+
+   if (adapter->iface_type != MWIFIEX_PCIE)
+   goto err_exit;
+
adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
if (!adapter->irq_wakeup) {
dev_dbg(dev, "fail to parse irq_wakeup from device tree\n");
-- 
2.11.0

[RFC PATCH v9 0/7] PCI: rockchip: Move PCIe WAKE# handling into pci core

2017-10-27 Thread Jeffy Chen


Currently we are handling wake irq in mrvl wifi driver. Move it into
pci core.

Tested on my chromebook bob(with cros 4.4 kernel and mrvl wifi).


Changes in v9:
Add section for PCI devices and rewrite the commit message.
Rewrite the commit message.
Fix check error in .cleanup().
Move dedicated wakeirq setup to setup() callback and use
device_set_wakeup_enable() to enable/disable.

Changes in v8:
Add optional "pci", and rewrite commit message.
Rewrite the commit message.
Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.

Changes in v7:
Move PCIE_WAKE handling into pci core.

Changes in v6:
Fix device_init_wake error handling, and add some comments.

Changes in v5:
Move to pci.txt
Use "wakeup" instead of "wake"
Rebase.

Changes in v3:
Fix error handling.

Changes in v2:
Use dev_pm_set_dedicated_wake_irq.

Jeffy Chen (7):
  dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
  of/irq: Adjust of_pci_irq parsing for multiple interrupts
  mwifiex: Disable wakeup irq handling for pcie
  arm64: dts: rockchip: Move PCIe WAKE# irq to pcie driver for Gru
  PCI: Make pci_platform_pm_ops's callbacks optional
  PCI / PM: Move acpi wakeup code to pci core
  PCI / PM: Add support for the PCIe WAKE# signal for OF

 Documentation/devicetree/bindings/pci/pci.txt |   8 ++
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi  |  15 +--
 drivers/net/wireless/marvell/mwifiex/main.c   |   4 +
 drivers/of/of_pci_irq.c   |  13 ++-
 drivers/pci/Makefile  |   2 +-
 drivers/pci/pci-acpi.c| 121 -
 drivers/pci/pci-driver.c  |   9 ++
 drivers/pci/pci-of.c  | 126 ++
 drivers/pci/pci.c | 112 +++
 drivers/pci/pci.h |  31 +--
 drivers/pci/probe.c   |  12 ++-
 drivers/pci/remove.c  |   2 +
 include/linux/pci.h   |   2 +
 13 files changed, 360 insertions(+), 97 deletions(-)
 create mode 100644 drivers/pci/pci-of.c

-- 
2.11.0

[RFC PATCH v8 0/7] PCI: rockchip: Move PCIe WAKE# handling into pci core

2017-10-26 Thread Jeffy Chen


Currently we are handling wake irq in mrvl wifi driver. Move it into
pci core.

Tested on my chromebook bob(with cros 4.4 kernel and mrvl wifi).


Changes in v8:
Add optional "pci", and rewrite commit message.
Rewrite the commit message.
Add pci-of.c and use platform_pm_ops to handle the PCIe WAKE# signal.

Changes in v7:
Move PCIE_WAKE handling into pci core.

Changes in v6:
Fix device_init_wake error handling, and add some comments.

Changes in v5:
Move to pci.txt
Use "wakeup" instead of "wake"
Rebase.

Changes in v3:
Fix error handling.

Changes in v2:
Use dev_pm_set_dedicated_wake_irq.

Jeffy Chen (7):
  dt-bindings: PCI: Add definition of PCIe WAKE# irq and PCI irq
  mwifiex: Disable wakeup irq handling for pcie
  arm64: dts: rockchip: Handle PCIe WAKE# signal in pcie driver for Gru
  of/irq: Adjust of pci irq parsing for multiple interrupts
  PCI: Make pci_platform_pm_ops's callbacks optional
  PCI / PM: Move acpi wakeup code to pci core
  PCI / PM: Add support for the PCIe WAKE# signal for OF

 Documentation/devicetree/bindings/pci/pci.txt |   3 +
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi  |  15 +--
 drivers/net/wireless/marvell/mwifiex/main.c   |   4 +
 drivers/of/of_pci_irq.c   |  13 ++-
 drivers/pci/Makefile  |   2 +-
 drivers/pci/pci-acpi.c| 121 +++
 drivers/pci/pci-driver.c  |   9 ++
 drivers/pci/pci-of.c  | 136 ++
 drivers/pci/pci.c | 112 +
 drivers/pci/pci.h |  31 --
 drivers/pci/probe.c   |  12 ++-
 drivers/pci/remove.c  |   2 +
 include/linux/pci.h   |   2 +
 13 files changed, 365 insertions(+), 97 deletions(-)
 create mode 100644 drivers/pci/pci-of.c

-- 
2.11.0

[RFC PATCH v8 2/7] mwifiex: Disable wakeup irq handling for pcie

2017-10-26 Thread Jeffy Chen

We are going to handle the wakeup irq in the pci core.

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v3: None
Changes in v2: None

 drivers/net/wireless/marvell/mwifiex/main.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index ee40b739b289..ba081c16f85c 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1568,6 +1568,10 @@ static void mwifiex_probe_of(struct mwifiex_adapter 
*adapter)
goto err_exit;
 
adapter->dt_node = dev->of_node;
+
+   if (adapter->iface_type != MWIFIEX_PCIE)
+   goto err_exit;
+
adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
if (!adapter->irq_wakeup) {
dev_dbg(dev, "fail to parse irq_wakeup from device tree\n");
-- 
2.11.0

Re: [PATCH] mwifiex: uninit wakeup info when failed to add card

2017-07-06 Thread jeffy


Hi brian,

On 07/06/2017 02:08 AM, Brian Norris wrote:

On Mon, Jul 03, 2017 at 03:54:30PM +0800, Jeffy Chen wrote:

We inited wakeup info at the beginning of mwifiex_add_card, so we need
to uninit it in the error handling.

It's much the same as what we did in:
36908c4 mwifiex: uninit wakeup info when removing device


Yeah, I noticed I hadn't covered all the error cases, so your change is
part of the fix. But you're not covering the error paths when the
firmware doesn't load correctly -- this happens asynchronously to
mwifiex_add_card(). i.e., you need to fixup the error paths in
_mwifiex_fw_dpc() too.

right, so we need to uninit it before every mwifiex_free_adapter :)


Brian


Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>

---

  drivers/net/wireless/marvell/mwifiex/main.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index f2600b8..17d2cbe 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1655,6 +1655,8 @@ mwifiex_add_card(void *card, struct completion *fw_done,
mwifiex_shutdown_drv(adapter);
}
  err_kmalloc:
+   if (adapter->irq_wakeup >= 0)
+   device_init_wakeup(adapter->dev, false);
mwifiex_free_adapter(adapter);

  err_init_sw:
--
2.1.4

[PATCH v2] mwifiex: uninit wakeup info in the error handling

2017-07-06 Thread Jeffy Chen

We inited wakeup info at the beginning of mwifiex_add_card, so we need
to uninit it in the error handling.

It's much the same as what we did in:
36908c4 mwifiex: uninit wakeup info when removing device

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>

---

Changes in v2:
Uninit wakeup when _mwifiex_fw_dpc failed too.

 drivers/net/wireless/marvell/mwifiex/main.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index f2600b8..097a899 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -665,8 +665,11 @@ static int _mwifiex_fw_dpc(const struct firmware 
*firmware, void *context)
release_firmware(adapter->firmware);
adapter->firmware = NULL;
}
-   if (init_failed)
+   if (init_failed) {
+   if (adapter->irq_wakeup >= 0)
+   device_init_wakeup(adapter->dev, false);
mwifiex_free_adapter(adapter);
+   }
/* Tell all current and future waiters we're finished */
complete_all(fw_done);
 
@@ -1655,6 +1658,8 @@ mwifiex_add_card(void *card, struct completion *fw_done,
mwifiex_shutdown_drv(adapter);
}
 err_kmalloc:
+   if (adapter->irq_wakeup >= 0)
+   device_init_wakeup(adapter->dev, false);
mwifiex_free_adapter(adapter);
 
 err_init_sw:
-- 
2.1.4

[PATCH] mwifiex: uninit wakeup info when failed to add card

2017-07-03 Thread Jeffy Chen

We inited wakeup info at the beginning of mwifiex_add_card, so we need
to uninit it in the error handling.

It's much the same as what we did in:
36908c4 mwifiex: uninit wakeup info when removing device

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>

---

 drivers/net/wireless/marvell/mwifiex/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index f2600b8..17d2cbe 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1655,6 +1655,8 @@ mwifiex_add_card(void *card, struct completion *fw_done,
mwifiex_shutdown_drv(adapter);
}
 err_kmalloc:
+   if (adapter->irq_wakeup >= 0)
+   device_init_wakeup(adapter->dev, false);
mwifiex_free_adapter(adapter);
 
 err_init_sw:
-- 
2.1.4

Re: Bluetooth: might sleep error in hidp_session_thread

2017-06-27 Thread jeffy


Hi Rohit,

On 06/24/2017 02:00 AM, Rohit Vaswani wrote:

I don't have a way to reply back to the older message; but you can use by 
tested-by for the below patch and re-send:


ok, i've resent it, thanks for your test by~

and for replying to an old message, i've figured it out when i tried to 
do that recently:


1/ download the mbox file from the patchwork 
link(https://patchwork.kernel.org/patch/***)

2/ rename it to .mbox
3/ import into thunerbird(using ImportExportTools)

hope that helps ;)


For patch: [v4,3/3] Bluetooth: hidp: fix possible might sleep error in 
hidp_session_thread

Tested-by: Rohit Vaswani <rvasw...@nvidia.com>

-Rohit

-Original Message-
From: jeffy [mailto:jeffy.c...@rock-chips.com]
Sent: Friday, June 23, 2017 05:39
To: Rohit Vaswani; linux-blueto...@vger.kernel.org
Cc: Brian Norris; Douglas Anderson; Johan Hedberg; Peter Hurley; Johan Hedberg; 
netdev@vger.kernel.org; linux-ker...@vger.kernel.org; David S. Miller; Marcel 
Holtmann; Gustavo Padovan
Subject: Re: Bluetooth: might sleep error in hidp_session_thread

Hi Rohit,

Thanx for your reply, and sorry to the delay, somehow your mail was marked as 
spam by the mail server :(

On 06/13/2017 02:31 AM, Rohit Vaswani wrote:

Hi Jeffy,

I was looking into the patch from Jeffy Chen from February 14 2017  :
   [v4,3/3] Bluetooth: hidp: fix possible might sleep error in
hidp_session_thread: https://patchwork.kernel.org/patch/9570931/

We faced a similar issue and this patch seems to fix the problem in our 
preliminary test.
I am trying to check if there was a reason this wasn't merged earlier ?


hmm, i'm not sure why, but please feel free to add your test-by~


Thanks,
Rohit

nvpublic

[RESEND PATCH v4 2/3] Bluetooth: cmtp: fix possible might sleep error in cmtp_session

2017-06-27 Thread Jeffy Chen

It looks like cmtp_session has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
Reviewed-by: Brian Norris <briannor...@chromium.org>
Reviewed-by: AL Yu-Chen Cho <a...@suse.com>

---

Changes in v4: None
Changes in v2:
Remove unnecessary memory barrier before wake_up_* functions.

 net/bluetooth/cmtp/core.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c
index 9e59b66..1152ce3 100644
--- a/net/bluetooth/cmtp/core.c
+++ b/net/bluetooth/cmtp/core.c
@@ -280,16 +280,16 @@ static int cmtp_session(void *arg)
struct cmtp_session *session = arg;
struct sock *sk = session->sock->sk;
struct sk_buff *skb;
-   wait_queue_t wait;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
BT_DBG("session %p", session);
 
set_user_nice(current, -15);
 
-   init_waitqueue_entry(, current);
add_wait_queue(sk_sleep(sk), );
while (1) {
-   set_current_state(TASK_INTERRUPTIBLE);
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
 
if (atomic_read(>terminate))
break;
@@ -306,9 +306,8 @@ static int cmtp_session(void *arg)
 
cmtp_process_transmit(session);
 
-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
-   __set_current_state(TASK_RUNNING);
remove_wait_queue(sk_sleep(sk), );
 
down_write(_session_sem);
@@ -393,7 +392,7 @@ int cmtp_add_connection(struct cmtp_connadd_req *req, 
struct socket *sock)
err = cmtp_attach_device(session);
if (err < 0) {
atomic_inc(>terminate);
-   wake_up_process(session->task);
+   wake_up_interruptible(sk_sleep(session->sock->sk));
up_write(_session_sem);
return err;
}
@@ -431,7 +430,11 @@ int cmtp_del_connection(struct cmtp_conndel_req *req)
 
/* Stop session thread */
atomic_inc(>terminate);
-   wake_up_process(session->task);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
+
+   wake_up_interruptible(sk_sleep(session->sock->sk));
} else
err = -ENOENT;
 
-- 
2.1.4

[RESEND PATCH v4 3/3] Bluetooth: hidp: fix possible might sleep error in hidp_session_thread

2017-06-27 Thread Jeffy Chen

It looks like hidp_session_thread has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
Tested-by: AL Yu-Chen Cho <a...@suse.com>
Tested-by: Rohit Vaswani <rvasw...@nvidia.com>

---

Changes in v4:
1/ Make hidp_session_wake_function static.
2/ Remove unnecessary default_wake_function.

Changes in v2:
1/ Fix could not wake up by wake attempts on original wait queues.
2/ Remove unnecessary memory barrier before wake_up_* functions.

 net/bluetooth/hidp/core.c | 33 ++---
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 0bec458..1fc0764 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -36,6 +36,7 @@
 #define VERSION "1.2"
 
 static DECLARE_RWSEM(hidp_session_sem);
+static DECLARE_WAIT_QUEUE_HEAD(hidp_session_wq);
 static LIST_HEAD(hidp_session_list);
 
 static unsigned char hidp_keycode[256] = {
@@ -1068,12 +1069,12 @@ static int hidp_session_start_sync(struct hidp_session 
*session)
  * Wake up session thread and notify it to stop. This is asynchronous and
  * returns immediately. Call this whenever a runtime error occurs and you want
  * the session to stop.
- * Note: wake_up_process() performs any necessary memory-barriers for us.
+ * Note: wake_up_interruptible() performs any necessary memory-barriers for us.
  */
 static void hidp_session_terminate(struct hidp_session *session)
 {
atomic_inc(>terminate);
-   wake_up_process(session->task);
+   wake_up_interruptible(_session_wq);
 }
 
 /*
@@ -1180,7 +1181,9 @@ static void hidp_session_run(struct hidp_session *session)
struct sock *ctrl_sk = session->ctrl_sock->sk;
struct sock *intr_sk = session->intr_sock->sk;
struct sk_buff *skb;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
+   add_wait_queue(_session_wq, );
for (;;) {
/*
 * This thread can be woken up two ways:
@@ -1188,12 +1191,10 @@ static void hidp_session_run(struct hidp_session 
*session)
 *session->terminate flag and wakes this thread up.
 *  - Via modifying the socket state of ctrl/intr_sock. This
 *thread is woken up by ->sk_state_changed().
-*
-* Note: set_current_state() performs any necessary
-* memory-barriers for us.
 */
-   set_current_state(TASK_INTERRUPTIBLE);
 
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
if (atomic_read(>terminate))
break;
 
@@ -1227,11 +1228,22 @@ static void hidp_session_run(struct hidp_session 
*session)
hidp_process_transmit(session, >ctrl_transmit,
  session->ctrl_sock);
 
-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
+   remove_wait_queue(_session_wq, );
 
atomic_inc(>terminate);
-   set_current_state(TASK_RUNNING);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
+}
+
+static int hidp_session_wake_function(wait_queue_t *wait,
+ unsigned int mode,
+ int sync, void *key)
+{
+   wake_up_interruptible(_session_wq);
+   return false;
 }
 
 /*
@@ -1244,7 +1256,8 @@ static void hidp_session_run(struct hidp_session *session)
 static int hidp_session_thread(void *arg)
 {
struct hidp_session *session = arg;
-   wait_queue_t ctrl_wait, intr_wait;
+   DEFINE_WAIT_FUNC(ctrl_wait, hidp_session_wake_function);
+   DEFINE_WAIT_FUNC(intr_wait, hidp_session_wake_function);
 
BT_DBG("session %p", session);
 
@@ -1254,8 +1267,6 @@ static int hidp_session_thread(void *arg)
set_user_nice(current, -15);
hidp_set_timer(session);
 
-   init_waitqueue_entry(_wait, current);
-   init_waitqueue_entry(_wait, current);
add_wait_queue(sk_sleep(session->ctrl_sock->sk), _wait);
add_wait_queue(sk_sleep(session->intr_sock->sk), _wait);
/* This memory barrier is paired with wq_has_sleeper(). See
-- 
2.1.4

[RESEND PATCH v4 1/3] Bluetooth: bnep: fix possible might sleep error in bnep_session

2017-06-27 Thread Jeffy Chen

It looks like bnep_session has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
Reviewed-by: Brian Norris <briannor...@chromium.org>
Reviewed-by: AL Yu-Chen Cho <a...@suse.com>
---

Changes in v4: None
Changes in v2: None

 net/bluetooth/bnep/core.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index fbf251f..4d6b94d 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -484,16 +484,16 @@ static int bnep_session(void *arg)
struct net_device *dev = s->dev;
struct sock *sk = s->sock->sk;
struct sk_buff *skb;
-   wait_queue_t wait;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
BT_DBG("");
 
set_user_nice(current, -15);
 
-   init_waitqueue_entry(, current);
add_wait_queue(sk_sleep(sk), );
while (1) {
-   set_current_state(TASK_INTERRUPTIBLE);
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
 
if (atomic_read(>terminate))
break;
@@ -515,9 +515,8 @@ static int bnep_session(void *arg)
break;
netif_wake_queue(dev);
 
-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
-   __set_current_state(TASK_RUNNING);
remove_wait_queue(sk_sleep(sk), );
 
/* Cleanup session */
@@ -666,7 +665,7 @@ int bnep_del_connection(struct bnep_conndel_req *req)
s = __bnep_get_session(req->dst);
if (s) {
atomic_inc(>terminate);
-   wake_up_process(s->task);
+   wake_up_interruptible(sk_sleep(s->sock->sk));
} else
err = -ENOENT;
 
-- 
2.1.4

Re: Bluetooth: might sleep error in hidp_session_thread

2017-06-23 Thread jeffy


Hi Rohit,

Thanx for your reply, and sorry to the delay, somehow your mail was 
marked as spam by the mail server :(


On 06/13/2017 02:31 AM, Rohit Vaswani wrote:

Hi Jeffy,

I was looking into the patch from Jeffy Chen from February 14 2017  :
  [v4,3/3] Bluetooth: hidp: fix possible might sleep error in 
hidp_session_thread: https://patchwork.kernel.org/patch/9570931/

We faced a similar issue and this patch seems to fix the problem in our 
preliminary test.
I am trying to check if there was a reason this wasn't merged earlier ?


hmm, i'm not sure why, but please feel free to add your test-by~


Thanks,
Rohit

nvpublic

Re: [Patch net] ipv6: only call ip6_route_dev_notify() once for NETDEV_UNREGISTER

2017-06-20 Thread jeffy


Hi Cong Wang,

oh, oops, i did misread.

also, Tested-by: Jeffy Chen <jeffy.c...@rock-chips.com>

On 06/21/2017 11:01 AM, jeffy wrote:

Hi Cong Wang,

i don't know much about net core, maybe i'm misreading the code...but

On 06/21/2017 02:42 AM, Cong Wang wrote:

In commit 242d3a49a2a1 ("ipv6: reorder ip6_route_dev_notifier after
ipv6_dev_notf")
I assumed NETDEV_REGISTER and NETDEV_UNREGISTER are paired,
unfortunately, as reported by jeffy, netdev_wait_allrefs()
could rebroadcast NETDEV_UNREGISTER event until all refs are
gone.

We have to add an additional check to avoid this corner case.
For netdev_wait_allrefs() dev->reg_state is NETREG_UNREGISTERED,
for dev_change_net_namespace(), dev->reg_state is
NETREG_REGISTERED. So check for dev->reg_state != NETREG_UNREGISTERED.


i saw we are calling NETDEV_REGISTER in these cases:
1/ register_netdevice_notifier:
the paired unregister would be:
a) normal unregister: rollback_registered_many
b) the error path:
register_netdevice_notifier->rollback jump label

2/ register_netdevice:
the paired unregister would both be rollback_registered_many for
normal/error cases

3/ dev_change_net_namespace:
the paired unregister is the one right before the register notify

i think we are handling all register notifies, but only unregister
notify from rollback_registered_many now?

you are checking for NETREG_UNREGISTERED, so only filter out the 
unregistered after rollback_registered_many, so that indeed covers all 
cases :)




Fixes: 242d3a49a2a1 ("ipv6: reorder ip6_route_dev_notifier after
ipv6_dev_notf")
Reported-by: jeffy <jeffy.c...@rock-chips.com>
Cc: David Ahern <dsah...@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangc...@gmail.com>
---
  net/ipv6/route.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 7cebd95..322bd62 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3722,7 +3722,11 @@ static int ip6_route_dev_notify(struct
notifier_block *this,
  net->ipv6.ip6_blk_hole_entry->dst.dev = dev;
  net->ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(dev);
  #endif
- } else if (event == NETDEV_UNREGISTER) {
+ } else if (event == NETDEV_UNREGISTER &&
+dev->reg_state != NETREG_UNREGISTERED) {
+/* NETDEV_UNREGISTER could be fired for multiple times by
+ * netdev_wait_allrefs(). Make sure we only call this once.
+ */
  in6_dev_put(net->ipv6.ip6_null_entry->rt6i_idev);
  #ifdef CONFIG_IPV6_MULTIPLE_TABLES
  in6_dev_put(net->ipv6.ip6_prohibit_entry->rt6i_idev);

Re: [Patch net] ipv6: only call ip6_route_dev_notify() once for NETDEV_UNREGISTER

2017-06-20 Thread jeffy


Hi Cong Wang,

i don't know much about net core, maybe i'm misreading the code...but

On 06/21/2017 02:42 AM, Cong Wang wrote:

In commit 242d3a49a2a1 ("ipv6: reorder ip6_route_dev_notifier after 
ipv6_dev_notf")
I assumed NETDEV_REGISTER and NETDEV_UNREGISTER are paired,
unfortunately, as reported by jeffy, netdev_wait_allrefs()
could rebroadcast NETDEV_UNREGISTER event until all refs are
gone.

We have to add an additional check to avoid this corner case.
For netdev_wait_allrefs() dev->reg_state is NETREG_UNREGISTERED,
for dev_change_net_namespace(), dev->reg_state is
NETREG_REGISTERED. So check for dev->reg_state != NETREG_UNREGISTERED.


i saw we are calling NETDEV_REGISTER in these cases:
1/ register_netdevice_notifier:
the paired unregister would be:
a) normal unregister: rollback_registered_many
b) the error path:
register_netdevice_notifier->rollback jump label

2/ register_netdevice:
the paired unregister would both be rollback_registered_many for 
normal/error cases


3/ dev_change_net_namespace:
the paired unregister is the one right before the register notify

i think we are handling all register notifies, but only unregister 
notify from rollback_registered_many now?





Fixes: 242d3a49a2a1 ("ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf")
Reported-by: jeffy <jeffy.c...@rock-chips.com>
Cc: David Ahern <dsah...@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangc...@gmail.com>
---
  net/ipv6/route.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 7cebd95..322bd62 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3722,7 +3722,11 @@ static int ip6_route_dev_notify(struct notifier_block 
*this,
net->ipv6.ip6_blk_hole_entry->dst.dev = dev;
net->ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(dev);
  #endif
-} else if (event == NETDEV_UNREGISTER) {
+} else if (event == NETDEV_UNREGISTER &&
+   dev->reg_state != NETREG_UNREGISTERED) {
+   /* NETDEV_UNREGISTER could be fired for multiple times by
+* netdev_wait_allrefs(). Make sure we only call this once.
+*/
in6_dev_put(net->ipv6.ip6_null_entry->rt6i_idev);
  #ifdef CONFIG_IPV6_MULTIPLE_TABLES
in6_dev_put(net->ipv6.ip6_prohibit_entry->rt6i_idev);

Re: [net,v2] ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf

2017-06-20 Thread jeffy


Hi Cong Wang,

On 06/20/2017 12:54 PM, Cong Wang wrote:

Hello,

On Mon, Jun 19, 2017 at 8:15 PM, jeffy <jeffy.c...@rock-chips.com> wrote:

but actually they are not guaranteed to be paired:

the netdev_run_todo(see the first dump stack above) would call
netdev_wait_allrefs to rebroadcast unregister notification multiple times,
unless timed out or all of the "struct net_device"'s refs released:

  * This is called when unregistering network devices.
  *
  * Any protocol or device that holds a reference should register
  * for netdevice notification, and cleanup and put back the
  * reference if they receive an UNREGISTER event.
  * We can get stuck here if buggy protocols don't correctly
  * call dev_put.
  */
static void netdev_wait_allrefs(struct net_device *dev)
{
...
 while (refcnt != 0) {
 if (time_after(jiffies, rebroadcast_time + 1 * HZ)) {
 rtnl_lock();

 /* Rebroadcast unregister notification */
 call_netdevice_notifiers(NETDEV_UNREGISTER, dev);

 __rtnl_unlock();
 rcu_barrier();
 rtnl_lock();


call_netdevice_notifiers(NETDEV_UNREGISTER_FINAL, dev);


Interesting, I didn't notice this corner-case, because normally
we would hit the one in rollback_registered_many(). Probably
we need to add a check

if (dev->reg_state == NETREG_UNREGISTERING)

in ip6_route_dev_notify(). Can you give it a try?

the NETREG_UNREGISTERING check works for my test:)

but i saw dev_change_net_namespace also call NETDEV_UNREGISTER & 
NETDEV_REGISTER:


int dev_change_net_namespace(struct net_device *dev, struct net *net, 
const char *pat)

{
...
/* Don't allow namespace local devices to be moved. */
err = -EINVAL;
if (dev->features & NETIF_F_NETNS_LOCAL)
goto out;

/* Ensure the device has been registrered */
if (dev->reg_state != NETREG_REGISTERED)
goto out;
...
/* Notify protocols, that we are about to destroy
 * this device. They should clean all the things.
 *
 * Note that dev->reg_state stays at NETREG_REGISTERED.
 * This is wanted because this way 8021q and macvlan know
 * the device is just moving and can keep their slaves up.
 */
call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
...
/* Notify protocols, that a new device appeared. */
call_netdevice_notifiers(NETDEV_REGISTER, dev);


maybe we should also add a check for NETDEV_REGISTER event? maybe:
if (dev->reg_state != NETREG_REGISTERED)




I guess we probably need to revise other NETDEV_UNREGISTER
handlers too.

I will send a patch tomorrow.

sounds great~


Thanks!

Re: [net,v2] ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf

2017-06-19 Thread jeffy


Hi guys,

i hit some warnings when testing this patch on my local 4.4 kernel(arm64 
chromebook) with KASAN & SLUB_DEBUG:


[9.919374] BUG: KASAN: use-after-free in 
ip6_route_dev_notify+0x194/0x2bc at addr ffc0c9d4e480

[9.928469] Read of size 4 by task kworker/u12:3/124
[9.933463] 
=

[9.941686] BUG kmalloc-1024 (Not tainted): kasan: bad access detected
...
[   10.741337] Workqueue: netns cleanup_net
[   10.745300] Call trace:
[   10.747776] [] dump_backtrace+0x0/0x200
[   10.753203] [] show_stack+0x24/0x30
[   10.758284] [] dump_stack+0xa8/0xcc
[   10.763364] [] print_trailer+0x158/0x168
[   10.768877] [] object_err+0x4c/0x5c
[   10.773956] [] kasan_report+0x338/0x4ec
[   10.779383] [] __asan_load4+0x7c/0x84
[   10.784640] [] ip6_route_dev_notify+0x194/0x2bc
[   10.790763] [] notifier_call_chain+0x78/0xc0
[   10.796625] [] raw_notifier_call_chain+0x3c/0x4c
[   10.802835] [] call_netdevice_notifiers_info+0x8c/0x9c
[   10.809564] [] call_netdevice_notifiers+0x9c/0xcc
[   10.815859] [] netdev_run_todo+0x224/0x3f0
[   10.821547] [] rtnl_unlock+0x14/0x1c
[   10.826716] [] default_device_exit_batch+0x258/0x2a0
[   10.833269] [] ops_exit_list+0x74/0xdc
[   10.838608] [] cleanup_net+0x290/0x400


and also this:
[   11.607268] BUG kmalloc-1024 (Tainted: GB  ): Poison 
overwritten
[   11.607270] 
-
[   11.607274] INFO: 0xffc0c9d4e478-0xffc0c9d4e478. First byte 
0x67 instead of 0x6b

...
[   11.607679] [] print_trailer+0x158/0x168
[   11.607683] [] check_bytes_and_report+0xd8/0x13c
[   11.607688] [] check_object+0x134/0x230
[   11.607692] [] alloc_debug_processing+0x104/0x178
[   11.607697] [] ___slab_alloc.constprop.26+0x2ec/0x434
[   11.607702] [] 
__slab_alloc.isra.23.constprop.25+0x48/0x5c

[   11.607707] [] __kmalloc_track_caller+0x12c/0x338



it looks like the "struct inet6_dev" been touched after freed, and 
refcnt changed(0xffc0c9d4e478, 376 bytes of struct inet6_dev) when 
reusing this memory.





i think the problem would be we are assuming NETDEV_REGISTER and 
NETDEV_UNREGISTER be paired in ip6_route_dev_notify:


> +  if (event == NETDEV_REGISTER) {
>net->ipv6.ip6_null_entry->dst.dev = dev;
>net->ipv6.ip6_null_entry->rt6i_idev = in6_dev_get(dev);
>   #ifdef CONFIG_IPV6_MULTIPLE_TABLES
> @@ -3718,6 +3721,12 @@ static int ip6_route_dev_notify(struct 
notifier_block *this,

>net->ipv6.ip6_blk_hole_entry->dst.dev = dev;
>net->ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(dev);
>   #endif
> +   } else if (event == NETDEV_UNREGISTER) {
> +  in6_dev_put(net->ipv6.ip6_null_entry->rt6i_idev);
> +#ifdef CONFIG_IPV6_MULTIPLE_TABLES
> +  in6_dev_put(net->ipv6.ip6_prohibit_entry->rt6i_idev);
> +  in6_dev_put(net->ipv6.ip6_blk_hole_entry->rt6i_idev);
> +#endif
>}

but actually they are not guaranteed to be paired:

the netdev_run_todo(see the first dump stack above) would call 
netdev_wait_allrefs to rebroadcast unregister notification multiple 
times, unless timed out or all of the "struct net_device"'s refs released:


 * This is called when unregistering network devices.
 *
 * Any protocol or device that holds a reference should register
 * for netdevice notification, and cleanup and put back the
 * reference if they receive an UNREGISTER event.
 * We can get stuck here if buggy protocols don't correctly
 * call dev_put.
 */
static void netdev_wait_allrefs(struct net_device *dev)
{
...
while (refcnt != 0) {
if (time_after(jiffies, rebroadcast_time + 1 * HZ)) {
rtnl_lock();

/* Rebroadcast unregister notification */
call_netdevice_notifiers(NETDEV_UNREGISTER, dev);

__rtnl_unlock();
rcu_barrier();
rtnl_lock();


call_netdevice_notifiers(NETDEV_UNREGISTER_FINAL, dev);



On 05/05/2017 01:36 AM, WANG Cong wrote:

For each netns (except init_net), we initialize its null entry
in 3 places:

1) The template itself, as we use kmemdup()
2) Code around dst_init_metrics() in ip6_route_net_init()
3) ip6_route_dev_notify(), which is supposed to initialize it after
loopback registers

Unfortunately the last one still happens in a wrong order because
we expect to initialize net->ipv6.ip6_null_entry->rt6i_idev to
net->loopback_dev's idev, so we have to do that after we add
idev to it. However, this notifier has priority == 0 same as
ipv6_dev_notf, and ipv6_dev_notf is registered after
ip6_route_dev_notifier so it is called actually after
ip6_route_dev_notifier.

Fix it by picking a smaller priority for ip6_route_dev_notifier.
Also, we have to release the refcnt accordingly when unregistering
loopback_dev because device exit functions are called before subsys
exit functions.

Cc: David

Re: [PATCH 3/3] mwifiex: wake system up when receives a wake irq

2017-02-24 Thread jeffy


Hi Kalle,

On 02/24/2017 07:01 PM, Kalle Valo wrote:

Jeffy Chen <jeffy.c...@rock-chips.com> writes:


Currrently we are disabling this wake irq after receiving it. If this
happens before we finish suspend and the pm event check is disabled,
the system will continue suspending, and this irq would not work again.

We may need to abort system suspend to avoid that.

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>

I only see patch 3 in patchwork. Where are patches 1 and 2?




the other two are much like this one, but for bluetooth :)

please check:

https://patchwork.kernel.org/patch/9589455 New  [1/3] btusb: 
wake system up when receives a wake irq
https://patchwork.kernel.org/patch/9589453 New  [2/3] btmrvl: 
wake system up when receives a wake irq
https://patchwork.kernel.org/patch/9589457 New  [3/3] mwifiex: 
wake system up when receives a wake irq




(sorry, this is a resent mail, because the last one was rejected due to 
wrong format)

[PATCH 3/3] mwifiex: wake system up when receives a wake irq

2017-02-23 Thread Jeffy Chen

Currrently we are disabling this wake irq after receiving it. If this
happens before we finish suspend and the pm event check is disabled,
the system will continue suspending, and this irq would not work again.

We may need to abort system suspend to avoid that.

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

 drivers/net/wireless/marvell/mwifiex/main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index 5ebca1d..30f4994 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -17,6 +17,8 @@
  * this warranty disclaimer.
  */
 
+#include 
+
 #include "main.h"
 #include "wmm.h"
 #include "cfg80211.h"
@@ -1509,6 +1511,7 @@ static irqreturn_t mwifiex_irq_wakeup_handler(int irq, 
void *priv)
 
/* Notify PM core we are wakeup source */
pm_wakeup_event(adapter->dev, 0);
+   pm_system_wakeup();
 
return IRQ_HANDLED;
 }
-- 
2.1.4

Re: [PATCH v3 3/3] Bluetooth: hidp: fix possible might sleep error in hidp_session_thread

2017-02-13 Thread jeffy


Hi Dmitry,

On 02/14/2017 05:27 AM, Dmitry Torokhov wrote:

Hi Jeffy,

On Sun, Feb 12, 2017 at 8:12 PM, Jeffy Chen <jeffy.c...@rock-chips.com> wrote:

It looks like hidp_session_thread has same pattern as the issue reported in
old rfcomm:

 while (1) {
 set_current_state(TASK_INTERRUPTIBLE);
 if (condition)
 break;
 // may call might_sleep here
 schedule();
 }
 __set_current_state(TASK_RUNNING);

Which fixed at:
 dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
1/ Fix could not wake up by wake attempts on original wait queues.
2/ Remove unnecessary memory barrier before wake_up_* functions.

---

Changes in v3: None
Changes in v2: None

  net/bluetooth/hidp/core.c | 33 ++---
  1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 0bec458..076bc50 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -36,6 +36,7 @@
  #define VERSION "1.2"

  static DECLARE_RWSEM(hidp_session_sem);
+static DECLARE_WAIT_QUEUE_HEAD(hidp_session_wq);
  static LIST_HEAD(hidp_session_list);

  static unsigned char hidp_keycode[256] = {
@@ -1068,12 +1069,12 @@ static int hidp_session_start_sync(struct hidp_session 
*session)
   * Wake up session thread and notify it to stop. This is asynchronous and
   * returns immediately. Call this whenever a runtime error occurs and you want
   * the session to stop.
- * Note: wake_up_process() performs any necessary memory-barriers for us.
+ * Note: wake_up_interruptible() performs any necessary memory-barriers for us.
   */
  static void hidp_session_terminate(struct hidp_session *session)
  {
 atomic_inc(>terminate);
-   wake_up_process(session->task);
+   wake_up_interruptible(_session_wq);
  }

  /*
@@ -1180,7 +1181,9 @@ static void hidp_session_run(struct hidp_session *session)
 struct sock *ctrl_sk = session->ctrl_sock->sk;
 struct sock *intr_sk = session->intr_sock->sk;
 struct sk_buff *skb;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);

+   add_wait_queue(_session_wq, );
 for (;;) {
 /*
  * This thread can be woken up two ways:
@@ -1188,12 +1191,10 @@ static void hidp_session_run(struct hidp_session 
*session)
  *session->terminate flag and wakes this thread up.
  *  - Via modifying the socket state of ctrl/intr_sock. This
  *thread is woken up by ->sk_state_changed().
-*
-* Note: set_current_state() performs any necessary
-* memory-barriers for us.
  */
-   set_current_state(TASK_INTERRUPTIBLE);

+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
 if (atomic_read(>terminate))
 break;

@@ -1227,11 +1228,22 @@ static void hidp_session_run(struct hidp_session 
*session)
 hidp_process_transmit(session, >ctrl_transmit,
   session->ctrl_sock);

-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
 }
+   remove_wait_queue(_session_wq, );

 atomic_inc(>terminate);
-   set_current_state(TASK_RUNNING);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
+}
+
+int hidp_session_wake_function(wait_queue_t *wait, unsigned int mode,
+  int sync, void *key)

static?

Oops, sure.



+{
+   wake_up_interruptible(_session_wq);
+
+   return default_wake_function(wait, mode, sync, key);

I do not think you need to call default_wake_function() here: the
process is waiting on hidp_session_wq, so the wakeup above is what
will wake up the session. By the time you get to call
default_wake_function() task will already be in wrong state. I think
we should simply return "false" instead of calling
default_wake_function().

Ok, thanx.



  }

  /*
@@ -1244,7 +1256,8 @@ static void hidp_session_run(struct hidp_session *session)
  static int hidp_session_thread(void *arg)
  {
 struct hidp_session *session = arg;
-   wait_queue_t ctrl_wait, intr_wait;
+   DEFINE_WAIT_FUNC(ctrl_wait, hidp_session_wake_function);
+   DEFINE_WAIT_FUNC(intr_wait, hidp_session_wake_function);

 BT_DBG("session %p", session);

@@ -1254,8 +1267,6 @@ static int hidp_session_thread(void *arg)
 set_user_nice(current, -15);
 hidp_set_timer(session);

-   init_waitqueue_entry(_wait, current);
-   init_waitqueue_entry(_wait, current);

[PATCH v4 3/3] Bluetooth: hidp: fix possible might sleep error in hidp_session_thread

2017-02-13 Thread Jeffy Chen

It looks like hidp_session_thread has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
1/ Fix could not wake up by wake attempts on original wait queues.
2/ Remove unnecessary memory barrier before wake_up_* functions.

1/ Make hidp_session_wake_function static.
2/ Remove unnecessary default_wake_function.

---

Changes in v3: None
Changes in v2: None

 net/bluetooth/hidp/core.c | 33 ++---
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 0bec458..1fc0764 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -36,6 +36,7 @@
 #define VERSION "1.2"
 
 static DECLARE_RWSEM(hidp_session_sem);
+static DECLARE_WAIT_QUEUE_HEAD(hidp_session_wq);
 static LIST_HEAD(hidp_session_list);
 
 static unsigned char hidp_keycode[256] = {
@@ -1068,12 +1069,12 @@ static int hidp_session_start_sync(struct hidp_session 
*session)
  * Wake up session thread and notify it to stop. This is asynchronous and
  * returns immediately. Call this whenever a runtime error occurs and you want
  * the session to stop.
- * Note: wake_up_process() performs any necessary memory-barriers for us.
+ * Note: wake_up_interruptible() performs any necessary memory-barriers for us.
  */
 static void hidp_session_terminate(struct hidp_session *session)
 {
atomic_inc(>terminate);
-   wake_up_process(session->task);
+   wake_up_interruptible(_session_wq);
 }
 
 /*
@@ -1180,7 +1181,9 @@ static void hidp_session_run(struct hidp_session *session)
struct sock *ctrl_sk = session->ctrl_sock->sk;
struct sock *intr_sk = session->intr_sock->sk;
struct sk_buff *skb;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
+   add_wait_queue(_session_wq, );
for (;;) {
/*
 * This thread can be woken up two ways:
@@ -1188,12 +1191,10 @@ static void hidp_session_run(struct hidp_session 
*session)
 *session->terminate flag and wakes this thread up.
 *  - Via modifying the socket state of ctrl/intr_sock. This
 *thread is woken up by ->sk_state_changed().
-*
-* Note: set_current_state() performs any necessary
-* memory-barriers for us.
 */
-   set_current_state(TASK_INTERRUPTIBLE);
 
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
if (atomic_read(>terminate))
break;
 
@@ -1227,11 +1228,22 @@ static void hidp_session_run(struct hidp_session 
*session)
hidp_process_transmit(session, >ctrl_transmit,
  session->ctrl_sock);
 
-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
+   remove_wait_queue(_session_wq, );
 
atomic_inc(>terminate);
-   set_current_state(TASK_RUNNING);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
+}
+
+static int hidp_session_wake_function(wait_queue_t *wait,
+ unsigned int mode,
+ int sync, void *key)
+{
+   wake_up_interruptible(_session_wq);
+   return false;
 }
 
 /*
@@ -1244,7 +1256,8 @@ static void hidp_session_run(struct hidp_session *session)
 static int hidp_session_thread(void *arg)
 {
struct hidp_session *session = arg;
-   wait_queue_t ctrl_wait, intr_wait;
+   DEFINE_WAIT_FUNC(ctrl_wait, hidp_session_wake_function);
+   DEFINE_WAIT_FUNC(intr_wait, hidp_session_wake_function);
 
BT_DBG("session %p", session);
 
@@ -1254,8 +1267,6 @@ static int hidp_session_thread(void *arg)
set_user_nice(current, -15);
hidp_set_timer(session);
 
-   init_waitqueue_entry(_wait, current);
-   init_waitqueue_entry(_wait, current);
add_wait_queue(sk_sleep(session->ctrl_sock->sk), _wait);
add_wait_queue(sk_sleep(session->intr_sock->sk), _wait);
/* This memory barrier is paired with wq_has_sleeper(). See
-- 
2.1.4

[PATCH v4 2/3] Bluetooth: cmtp: fix possible might sleep error in cmtp_session

2017-02-13 Thread Jeffy Chen

It looks like cmtp_session has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
Reviewed-by: Brian Norris <briannor...@chromium.org>
Remove unnecessary memory barrier before wake_up_* functions.

---

Changes in v3:
Add brian's Reviewed-by.

Changes in v2: None

 net/bluetooth/cmtp/core.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c
index 9e59b66..1152ce3 100644
--- a/net/bluetooth/cmtp/core.c
+++ b/net/bluetooth/cmtp/core.c
@@ -280,16 +280,16 @@ static int cmtp_session(void *arg)
struct cmtp_session *session = arg;
struct sock *sk = session->sock->sk;
struct sk_buff *skb;
-   wait_queue_t wait;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
BT_DBG("session %p", session);
 
set_user_nice(current, -15);
 
-   init_waitqueue_entry(, current);
add_wait_queue(sk_sleep(sk), );
while (1) {
-   set_current_state(TASK_INTERRUPTIBLE);
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
 
if (atomic_read(>terminate))
break;
@@ -306,9 +306,8 @@ static int cmtp_session(void *arg)
 
cmtp_process_transmit(session);
 
-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
-   __set_current_state(TASK_RUNNING);
remove_wait_queue(sk_sleep(sk), );
 
down_write(_session_sem);
@@ -393,7 +392,7 @@ int cmtp_add_connection(struct cmtp_connadd_req *req, 
struct socket *sock)
err = cmtp_attach_device(session);
if (err < 0) {
atomic_inc(>terminate);
-   wake_up_process(session->task);
+   wake_up_interruptible(sk_sleep(session->sock->sk));
up_write(_session_sem);
return err;
}
@@ -431,7 +430,11 @@ int cmtp_del_connection(struct cmtp_conndel_req *req)
 
/* Stop session thread */
atomic_inc(>terminate);
-   wake_up_process(session->task);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
+
+   wake_up_interruptible(sk_sleep(session->sock->sk));
} else
err = -ENOENT;
 
-- 
2.1.4

[PATCH v4 1/3] Bluetooth: bnep: fix possible might sleep error in bnep_session

2017-02-13 Thread Jeffy Chen

It looks like bnep_session has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
Reviewed-by: Brian Norris <briannor...@chromium.org>
---

Changes in v3:
Add brian's Reviewed-by.

Changes in v2:
Remove unnecessary memory barrier before wake_up_* functions.

 net/bluetooth/bnep/core.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index fbf251f..4d6b94d 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -484,16 +484,16 @@ static int bnep_session(void *arg)
struct net_device *dev = s->dev;
struct sock *sk = s->sock->sk;
struct sk_buff *skb;
-   wait_queue_t wait;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
BT_DBG("");
 
set_user_nice(current, -15);
 
-   init_waitqueue_entry(, current);
add_wait_queue(sk_sleep(sk), );
while (1) {
-   set_current_state(TASK_INTERRUPTIBLE);
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
 
if (atomic_read(>terminate))
break;
@@ -515,9 +515,8 @@ static int bnep_session(void *arg)
break;
netif_wake_queue(dev);
 
-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
-   __set_current_state(TASK_RUNNING);
remove_wait_queue(sk_sleep(sk), );
 
/* Cleanup session */
@@ -666,7 +665,7 @@ int bnep_del_connection(struct bnep_conndel_req *req)
s = __bnep_get_session(req->dst);
if (s) {
atomic_inc(>terminate);
-   wake_up_process(s->task);
+   wake_up_interruptible(sk_sleep(s->sock->sk));
} else
err = -ENOENT;
 
-- 
2.1.4

Re: [PATCH 3/3] Bluetooth: hidp: fix possible might sleep error in hidp_session_thread

2017-02-12 Thread jeffy


Hi brian,

On 02/11/2017 09:26 AM, Brian Norris wrote:

Hi Jeffy,

I'm really not an expert on bluetooth or HIDP, but I can't bring myself
to say that this is correct. I still think you have a problem.

On Tue, Jan 24, 2017 at 12:07:51PM +0800, Jeffy Chen wrote:

It looks like hidp_session_thread has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

  net/bluetooth/hidp/core.c | 23 +++
  1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 0bec458..43d6e6a 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -36,6 +36,7 @@
  #define VERSION "1.2"
  
  static DECLARE_RWSEM(hidp_session_sem);

+static DECLARE_WAIT_QUEUE_HEAD(hidp_session_wq);
  static LIST_HEAD(hidp_session_list);
  
  static unsigned char hidp_keycode[256] = {

@@ -1068,12 +1069,15 @@ static int hidp_session_start_sync(struct hidp_session 
*session)
   * Wake up session thread and notify it to stop. This is asynchronous and
   * returns immediately. Call this whenever a runtime error occurs and you want
   * the session to stop.
- * Note: wake_up_process() performs any necessary memory-barriers for us.
   */
  static void hidp_session_terminate(struct hidp_session *session)
  {
atomic_inc(>terminate);
-   wake_up_process(session->task);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
+
+   wake_up_interruptible(_session_wq);

So, you're adding a whole new wait queue here.


  }
  
  /*

@@ -1180,7 +1184,9 @@ static void hidp_session_run(struct hidp_session *session)
struct sock *ctrl_sk = session->ctrl_sock->sk;
struct sock *intr_sk = session->intr_sock->sk;
struct sk_buff *skb;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
  
+	add_wait_queue(_session_wq, );

for (;;) {
/*
 * This thread can be woken up two ways:
@@ -1188,12 +1194,10 @@ static void hidp_session_run(struct hidp_session 
*session)
 *session->terminate flag and wakes this thread up.
 *  - Via modifying the socket state of ctrl/intr_sock. This
 *thread is woken up by ->sk_state_changed().
-*
-* Note: set_current_state() performs any necessary
-* memory-barriers for us.
 */
-   set_current_state(TASK_INTERRUPTIBLE);
  
+		/* Ensure session->terminate is updated */

+   smp_mb__before_atomic();
if (atomic_read(>terminate))
break;
  
@@ -1227,11 +1231,14 @@ static void hidp_session_run(struct hidp_session *session)

hidp_process_transmit(session, >ctrl_transmit,
  session->ctrl_sock);
  
-		schedule();

+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);

And you're waiting on it here.

But you're already on two other wait queues (hidp_session_thread()). So
the nice WQ_FLAG_WOKEN handling will only happen if you get woken via
the new hidp_session_wq queue. But what about the other two? Seems like
again you might have a race condition that would lead you to
(temporarily, at least?) missing a wake-up attempt.

Thanx for point that out.


I'm not really sure what the best way to resolve this would be. My best
guess would be to either consolidate the use of these wait queues, or
lese roll a version of wait_woken() to handle 2 or more wait heads...

Am I wrong? I easily could be.

Brian


}
+   remove_wait_queue(_session_wq, );
  
  	atomic_inc(>terminate);

-   set_current_state(TASK_RUNNING);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
  }
  
  /*

--
2.1.4

Re: [PATCH 2/3] Bluetooth: cmtp: fix possible might sleep error in cmtp_session

2017-02-12 Thread jeffy


Hi brian,

On 02/11/2017 09:43 AM, Brian Norris wrote:

Hi,

On Tue, Jan 24, 2017 at 12:07:50PM +0800, Jeffy Chen wrote:

It looks like cmtp_session has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

  net/bluetooth/cmtp/core.c | 21 ++---
  1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c
index 9e59b66..6b03f2b 100644
--- a/net/bluetooth/cmtp/core.c
+++ b/net/bluetooth/cmtp/core.c
@@ -280,16 +280,16 @@ static int cmtp_session(void *arg)
struct cmtp_session *session = arg;
struct sock *sk = session->sock->sk;
struct sk_buff *skb;
-   wait_queue_t wait;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
  
  	BT_DBG("session %p", session);
  
  	set_user_nice(current, -15);
  
-	init_waitqueue_entry(, current);

add_wait_queue(sk_sleep(sk), );
while (1) {
-   set_current_state(TASK_INTERRUPTIBLE);
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
  
  		if (atomic_read(>terminate))

break;
@@ -306,9 +306,8 @@ static int cmtp_session(void *arg)
  
  		cmtp_process_transmit(session);
  
-		schedule();

+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
-   __set_current_state(TASK_RUNNING);
remove_wait_queue(sk_sleep(sk), );
  
  	down_write(_session_sem);

@@ -393,7 +392,11 @@ int cmtp_add_connection(struct cmtp_connadd_req *req, 
struct socket *sock)
err = cmtp_attach_device(session);
if (err < 0) {
atomic_inc(>terminate);
-   wake_up_process(session->task);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
+

Same comment about the barrier.

Done, there are barriers in wake functions indeed, thanx!



+   wake_up_interruptible(sk_sleep(session->sock->sk));
up_write(_session_sem);
return err;
}
@@ -431,7 +434,11 @@ int cmtp_del_connection(struct cmtp_conndel_req *req)
  
  		/* Stop session thread */

atomic_inc(>terminate);
-   wake_up_process(session->task);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();

And again.

But otherwise I think this looks OK, again with the caveat that I don't
know Bluetooth/CMTP that well:

Reviewed-by: Brian Norris <briannor...@chromium.org>


+
+   wake_up_interruptible(sk_sleep(session->sock->sk));
} else
err = -ENOENT;
  
--

2.1.4

Re: [PATCH 1/3] Bluetooth: bnep: fix possible might sleep error in bnep_session

2017-02-12 Thread jeffy


Hi brian,

On 02/11/2017 09:40 AM, Brian Norris wrote:

Hi,

On Tue, Jan 24, 2017 at 12:07:49PM +0800, Jeffy Chen wrote:

It looks like bnep_session has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

  net/bluetooth/bnep/core.c | 15 +--
  1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index fbf251f..da04d51 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -484,16 +484,16 @@ static int bnep_session(void *arg)
struct net_device *dev = s->dev;
struct sock *sk = s->sock->sk;
struct sk_buff *skb;
-   wait_queue_t wait;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
  
  	BT_DBG("");
  
  	set_user_nice(current, -15);
  
-	init_waitqueue_entry(, current);

add_wait_queue(sk_sleep(sk), );
while (1) {
-   set_current_state(TASK_INTERRUPTIBLE);
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
  
  		if (atomic_read(>terminate))

break;
@@ -515,9 +515,8 @@ static int bnep_session(void *arg)
break;
netif_wake_queue(dev);
  
-		schedule();

+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
-   __set_current_state(TASK_RUNNING);
remove_wait_queue(sk_sleep(sk), );
  
  	/* Cleanup session */

@@ -666,7 +665,11 @@ int bnep_del_connection(struct bnep_conndel_req *req)
s = __bnep_get_session(req->dst);
if (s) {
atomic_inc(>terminate);
-   wake_up_process(s->task);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
+

__wake_up() suggests:

  * It may be assumed that this function implies a write memory barrier before
  * changing the task state if and only if any tasks are woken up.

so the above barrier is probably unnecessary. I'm not so sure about the
one before atomic_read(); seems fine.

Got it, thanx!


Other than that, I this looks ok:

Reviewed-by: Brian Norris <briannor...@chromium.or>

But I haven't been testing BNEP.

Brian


+   wake_up_interruptible(sk_sleep(s->sock->sk));
} else
err = -ENOENT;
  
--

2.1.4

[PATCH v3 3/3] Bluetooth: hidp: fix possible might sleep error in hidp_session_thread

2017-02-12 Thread Jeffy Chen

It looks like hidp_session_thread has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
1/ Fix could not wake up by wake attempts on original wait queues.
2/ Remove unnecessary memory barrier before wake_up_* functions.

---

Changes in v3: None
Changes in v2: None

 net/bluetooth/hidp/core.c | 33 ++---
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 0bec458..076bc50 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -36,6 +36,7 @@
 #define VERSION "1.2"
 
 static DECLARE_RWSEM(hidp_session_sem);
+static DECLARE_WAIT_QUEUE_HEAD(hidp_session_wq);
 static LIST_HEAD(hidp_session_list);
 
 static unsigned char hidp_keycode[256] = {
@@ -1068,12 +1069,12 @@ static int hidp_session_start_sync(struct hidp_session 
*session)
  * Wake up session thread and notify it to stop. This is asynchronous and
  * returns immediately. Call this whenever a runtime error occurs and you want
  * the session to stop.
- * Note: wake_up_process() performs any necessary memory-barriers for us.
+ * Note: wake_up_interruptible() performs any necessary memory-barriers for us.
  */
 static void hidp_session_terminate(struct hidp_session *session)
 {
atomic_inc(>terminate);
-   wake_up_process(session->task);
+   wake_up_interruptible(_session_wq);
 }
 
 /*
@@ -1180,7 +1181,9 @@ static void hidp_session_run(struct hidp_session *session)
struct sock *ctrl_sk = session->ctrl_sock->sk;
struct sock *intr_sk = session->intr_sock->sk;
struct sk_buff *skb;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
+   add_wait_queue(_session_wq, );
for (;;) {
/*
 * This thread can be woken up two ways:
@@ -1188,12 +1191,10 @@ static void hidp_session_run(struct hidp_session 
*session)
 *session->terminate flag and wakes this thread up.
 *  - Via modifying the socket state of ctrl/intr_sock. This
 *thread is woken up by ->sk_state_changed().
-*
-* Note: set_current_state() performs any necessary
-* memory-barriers for us.
 */
-   set_current_state(TASK_INTERRUPTIBLE);
 
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
if (atomic_read(>terminate))
break;
 
@@ -1227,11 +1228,22 @@ static void hidp_session_run(struct hidp_session 
*session)
hidp_process_transmit(session, >ctrl_transmit,
  session->ctrl_sock);
 
-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
+   remove_wait_queue(_session_wq, );
 
atomic_inc(>terminate);
-   set_current_state(TASK_RUNNING);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
+}
+
+int hidp_session_wake_function(wait_queue_t *wait, unsigned int mode,
+  int sync, void *key)
+{
+   wake_up_interruptible(_session_wq);
+
+   return default_wake_function(wait, mode, sync, key);
 }
 
 /*
@@ -1244,7 +1256,8 @@ static void hidp_session_run(struct hidp_session *session)
 static int hidp_session_thread(void *arg)
 {
struct hidp_session *session = arg;
-   wait_queue_t ctrl_wait, intr_wait;
+   DEFINE_WAIT_FUNC(ctrl_wait, hidp_session_wake_function);
+   DEFINE_WAIT_FUNC(intr_wait, hidp_session_wake_function);
 
BT_DBG("session %p", session);
 
@@ -1254,8 +1267,6 @@ static int hidp_session_thread(void *arg)
set_user_nice(current, -15);
hidp_set_timer(session);
 
-   init_waitqueue_entry(_wait, current);
-   init_waitqueue_entry(_wait, current);
add_wait_queue(sk_sleep(session->ctrl_sock->sk), _wait);
add_wait_queue(sk_sleep(session->intr_sock->sk), _wait);
/* This memory barrier is paired with wq_has_sleeper(). See
-- 
2.1.4

[PATCH v3 1/3] Bluetooth: bnep: fix possible might sleep error in bnep_session

2017-02-12 Thread Jeffy Chen

It looks like bnep_session has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
Reviewed-by: Brian Norris <briannor...@chromium.org>
---

Changes in v3:
Add brian's Reviewed-by.

Changes in v2:
Remove unnecessary memory barrier before wake_up_* functions.

 net/bluetooth/bnep/core.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index fbf251f..4d6b94d 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -484,16 +484,16 @@ static int bnep_session(void *arg)
struct net_device *dev = s->dev;
struct sock *sk = s->sock->sk;
struct sk_buff *skb;
-   wait_queue_t wait;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
BT_DBG("");
 
set_user_nice(current, -15);
 
-   init_waitqueue_entry(, current);
add_wait_queue(sk_sleep(sk), );
while (1) {
-   set_current_state(TASK_INTERRUPTIBLE);
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
 
if (atomic_read(>terminate))
break;
@@ -515,9 +515,8 @@ static int bnep_session(void *arg)
break;
netif_wake_queue(dev);
 
-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
-   __set_current_state(TASK_RUNNING);
remove_wait_queue(sk_sleep(sk), );
 
/* Cleanup session */
@@ -666,7 +665,7 @@ int bnep_del_connection(struct bnep_conndel_req *req)
s = __bnep_get_session(req->dst);
if (s) {
atomic_inc(>terminate);
-   wake_up_process(s->task);
+   wake_up_interruptible(sk_sleep(s->sock->sk));
} else
err = -ENOENT;
 
-- 
2.1.4

[PATCH v3 2/3] Bluetooth: cmtp: fix possible might sleep error in cmtp_session

2017-02-12 Thread Jeffy Chen

It looks like cmtp_session has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
Reviewed-by: Brian Norris <briannor...@chromium.org>
Remove unnecessary memory barrier before wake_up_* functions.

---

Changes in v3:
Add brian's Reviewed-by.

Changes in v2: None

 net/bluetooth/cmtp/core.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c
index 9e59b66..1152ce3 100644
--- a/net/bluetooth/cmtp/core.c
+++ b/net/bluetooth/cmtp/core.c
@@ -280,16 +280,16 @@ static int cmtp_session(void *arg)
struct cmtp_session *session = arg;
struct sock *sk = session->sock->sk;
struct sk_buff *skb;
-   wait_queue_t wait;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
BT_DBG("session %p", session);
 
set_user_nice(current, -15);
 
-   init_waitqueue_entry(, current);
add_wait_queue(sk_sleep(sk), );
while (1) {
-   set_current_state(TASK_INTERRUPTIBLE);
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
 
if (atomic_read(>terminate))
break;
@@ -306,9 +306,8 @@ static int cmtp_session(void *arg)
 
cmtp_process_transmit(session);
 
-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
-   __set_current_state(TASK_RUNNING);
remove_wait_queue(sk_sleep(sk), );
 
down_write(_session_sem);
@@ -393,7 +392,7 @@ int cmtp_add_connection(struct cmtp_connadd_req *req, 
struct socket *sock)
err = cmtp_attach_device(session);
if (err < 0) {
atomic_inc(>terminate);
-   wake_up_process(session->task);
+   wake_up_interruptible(sk_sleep(session->sock->sk));
up_write(_session_sem);
return err;
}
@@ -431,7 +430,11 @@ int cmtp_del_connection(struct cmtp_conndel_req *req)
 
/* Stop session thread */
atomic_inc(>terminate);
-   wake_up_process(session->task);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
+
+   wake_up_interruptible(sk_sleep(session->sock->sk));
} else
err = -ENOENT;
 
-- 
2.1.4

[PATCH v2 3/3] Bluetooth: hidp: fix possible might sleep error in hidp_session_thread

2017-02-12 Thread Jeffy Chen

It looks like hidp_session_thread has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
1/ Fix could not wake up by wake attempts on original wait queues.
2/ Remove unnecessary memory barrier before wake_up_* functions.

---

Changes in v2: None

 net/bluetooth/hidp/core.c | 33 ++---
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 0bec458..076bc50 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -36,6 +36,7 @@
 #define VERSION "1.2"
 
 static DECLARE_RWSEM(hidp_session_sem);
+static DECLARE_WAIT_QUEUE_HEAD(hidp_session_wq);
 static LIST_HEAD(hidp_session_list);
 
 static unsigned char hidp_keycode[256] = {
@@ -1068,12 +1069,12 @@ static int hidp_session_start_sync(struct hidp_session 
*session)
  * Wake up session thread and notify it to stop. This is asynchronous and
  * returns immediately. Call this whenever a runtime error occurs and you want
  * the session to stop.
- * Note: wake_up_process() performs any necessary memory-barriers for us.
+ * Note: wake_up_interruptible() performs any necessary memory-barriers for us.
  */
 static void hidp_session_terminate(struct hidp_session *session)
 {
atomic_inc(>terminate);
-   wake_up_process(session->task);
+   wake_up_interruptible(_session_wq);
 }
 
 /*
@@ -1180,7 +1181,9 @@ static void hidp_session_run(struct hidp_session *session)
struct sock *ctrl_sk = session->ctrl_sock->sk;
struct sock *intr_sk = session->intr_sock->sk;
struct sk_buff *skb;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
+   add_wait_queue(_session_wq, );
for (;;) {
/*
 * This thread can be woken up two ways:
@@ -1188,12 +1191,10 @@ static void hidp_session_run(struct hidp_session 
*session)
 *session->terminate flag and wakes this thread up.
 *  - Via modifying the socket state of ctrl/intr_sock. This
 *thread is woken up by ->sk_state_changed().
-*
-* Note: set_current_state() performs any necessary
-* memory-barriers for us.
 */
-   set_current_state(TASK_INTERRUPTIBLE);
 
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
if (atomic_read(>terminate))
break;
 
@@ -1227,11 +1228,22 @@ static void hidp_session_run(struct hidp_session 
*session)
hidp_process_transmit(session, >ctrl_transmit,
  session->ctrl_sock);
 
-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
+   remove_wait_queue(_session_wq, );
 
atomic_inc(>terminate);
-   set_current_state(TASK_RUNNING);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
+}
+
+int hidp_session_wake_function(wait_queue_t *wait, unsigned int mode,
+  int sync, void *key)
+{
+   wake_up_interruptible(_session_wq);
+
+   return default_wake_function(wait, mode, sync, key);
 }
 
 /*
@@ -1244,7 +1256,8 @@ static void hidp_session_run(struct hidp_session *session)
 static int hidp_session_thread(void *arg)
 {
struct hidp_session *session = arg;
-   wait_queue_t ctrl_wait, intr_wait;
+   DEFINE_WAIT_FUNC(ctrl_wait, hidp_session_wake_function);
+   DEFINE_WAIT_FUNC(intr_wait, hidp_session_wake_function);
 
BT_DBG("session %p", session);
 
@@ -1254,8 +1267,6 @@ static int hidp_session_thread(void *arg)
set_user_nice(current, -15);
hidp_set_timer(session);
 
-   init_waitqueue_entry(_wait, current);
-   init_waitqueue_entry(_wait, current);
add_wait_queue(sk_sleep(session->ctrl_sock->sk), _wait);
add_wait_queue(sk_sleep(session->intr_sock->sk), _wait);
/* This memory barrier is paired with wq_has_sleeper(). See
-- 
2.1.4

[PATCH v2 1/3] Bluetooth: bnep: fix possible might sleep error in bnep_session

2017-02-12 Thread Jeffy Chen

It looks like bnep_session has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

Changes in v2:
Remove unnecessary memory barrier before wake_up_* functions.

 net/bluetooth/bnep/core.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index fbf251f..4d6b94d 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -484,16 +484,16 @@ static int bnep_session(void *arg)
struct net_device *dev = s->dev;
struct sock *sk = s->sock->sk;
struct sk_buff *skb;
-   wait_queue_t wait;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
BT_DBG("");
 
set_user_nice(current, -15);
 
-   init_waitqueue_entry(, current);
add_wait_queue(sk_sleep(sk), );
while (1) {
-   set_current_state(TASK_INTERRUPTIBLE);
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
 
if (atomic_read(>terminate))
break;
@@ -515,9 +515,8 @@ static int bnep_session(void *arg)
break;
netif_wake_queue(dev);
 
-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
-   __set_current_state(TASK_RUNNING);
remove_wait_queue(sk_sleep(sk), );
 
/* Cleanup session */
@@ -666,7 +665,7 @@ int bnep_del_connection(struct bnep_conndel_req *req)
s = __bnep_get_session(req->dst);
if (s) {
atomic_inc(>terminate);
-   wake_up_process(s->task);
+   wake_up_interruptible(sk_sleep(s->sock->sk));
} else
err = -ENOENT;
 
-- 
2.1.4

[PATCH v2 2/3] Bluetooth: cmtp: fix possible might sleep error in cmtp_session

2017-02-12 Thread Jeffy Chen

It looks like cmtp_session has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
Remove unnecessary memory barrier before wake_up_* functions.

---

Changes in v2: None

 net/bluetooth/cmtp/core.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c
index 9e59b66..1152ce3 100644
--- a/net/bluetooth/cmtp/core.c
+++ b/net/bluetooth/cmtp/core.c
@@ -280,16 +280,16 @@ static int cmtp_session(void *arg)
struct cmtp_session *session = arg;
struct sock *sk = session->sock->sk;
struct sk_buff *skb;
-   wait_queue_t wait;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
BT_DBG("session %p", session);
 
set_user_nice(current, -15);
 
-   init_waitqueue_entry(, current);
add_wait_queue(sk_sleep(sk), );
while (1) {
-   set_current_state(TASK_INTERRUPTIBLE);
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
 
if (atomic_read(>terminate))
break;
@@ -306,9 +306,8 @@ static int cmtp_session(void *arg)
 
cmtp_process_transmit(session);
 
-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
-   __set_current_state(TASK_RUNNING);
remove_wait_queue(sk_sleep(sk), );
 
down_write(_session_sem);
@@ -393,7 +392,7 @@ int cmtp_add_connection(struct cmtp_connadd_req *req, 
struct socket *sock)
err = cmtp_attach_device(session);
if (err < 0) {
atomic_inc(>terminate);
-   wake_up_process(session->task);
+   wake_up_interruptible(sk_sleep(session->sock->sk));
up_write(_session_sem);
return err;
}
@@ -431,7 +430,11 @@ int cmtp_del_connection(struct cmtp_conndel_req *req)
 
/* Stop session thread */
atomic_inc(>terminate);
-   wake_up_process(session->task);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
+
+   wake_up_interruptible(sk_sleep(session->sock->sk));
} else
err = -ENOENT;
 
-- 
2.1.4

Re: [PATCH] Bluetooth: hidp: might sleep error in hidp_session_thread

2017-01-23 Thread jeffy


Hi brian,

On 01/24/2017 10:31 AM, Brian Norris wrote:

Hi Jeffy,

On Fri, Jan 20, 2017 at 09:52:08PM +0800, Jeffy Chen wrote:

[   39.044329] do not call blocking ops when !TASK_RUNNING; state=1 set
at [] hidp_session_thread+0x110/0x568 [hidp]
...
[   40.159664] Call trace:
[   40.162122] [] __might_sleep+0x64/0x90
[   40.167443] [] lock_sock_nested+0x30/0x78
[   40.173047] [] l2cap_sock_sendmsg+0x90/0xf0
[bluetooth]
[   40.179842] [] sock_sendmsg+0x4c/0x68
[   40.185072] [] kernel_sendmsg+0x54/0x68
[   40.190477] [] hidp_send_frame+0x78/0xa0 [hidp]
[   40.196574] [] hidp_process_transmit+0x44/0x98
[hidp]
[   40.203191] [] hidp_session_thread+0x364/0x568
[hidp]

Am I crazy, or are several other protocols broken like this too? I see a
similar structure in net/bluetooth/bnep/core.c and
net/bluetooth/cmtp/core.c, at least, each of which also call
kernel_sendmsg(), which might be an l2cap socket (...I think? I'm not a
bluetooth expert really).
Thanx, uploaded a new serial of patchset, which contains hidp & cmtp & 
bnep:9534023/9534025/9534027



Following (https://lwn.net/Articles/628628/).

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

  net/bluetooth/hidp/core.c | 15 +--
  1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 0bec458..bfd3fb8 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -1180,7 +1180,9 @@ static void hidp_session_run(struct hidp_session *session)
struct sock *ctrl_sk = session->ctrl_sock->sk;
struct sock *intr_sk = session->intr_sock->sk;
struct sk_buff *skb;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
  
+	add_wait_queue(sk_sleep(intr_sk), );

for (;;) {
/*
 * This thread can be woken up two ways:
@@ -1188,12 +1190,10 @@ static void hidp_session_run(struct hidp_session 
*session)
 *session->terminate flag and wakes this thread up.
 *  - Via modifying the socket state of ctrl/intr_sock. This
 *thread is woken up by ->sk_state_changed().
-*
-* Note: set_current_state() performs any necessary
-* memory-barriers for us.
 */
-   set_current_state(TASK_INTERRUPTIBLE);
  
+		/* Ensure session->terminate is updated */

+   smp_mb__before_atomic();
if (atomic_read(>terminate))
break;
  
@@ -1227,11 +1227,14 @@ static void hidp_session_run(struct hidp_session *session)

hidp_process_transmit(session, >ctrl_transmit,
  session->ctrl_sock);
  
-		schedule();

+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);

I think this looks mostly good, except what about the
hidp_session_terminate() condition? In that case, you're running
wake_up_process() -- which won't set WQ_FLAG_WOKEN for us. So what
happens if we see a hidp_session_terminate() call in between the check
for the ->terminate count, but before we call wait_woken()? IIUC, I
think we'll just ignore the call and keep waiting for the next wake
signal.

I think you might need to rework hidp_session_terminate() too, to
actually target the wait queue and not just the processes.

IIUC, of course. I could be wrong...

Ok, that make sense, thanx for point that out.


Brian


}
+   remove_wait_queue(sk_sleep(intr_sk), );
  
  	atomic_inc(>terminate);

-   set_current_state(TASK_RUNNING);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
  }
  
  /*

--
2.1.4

[PATCH 2/3] Bluetooth: cmtp: fix possible might sleep error in cmtp_session

2017-01-23 Thread Jeffy Chen

It looks like cmtp_session has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

 net/bluetooth/cmtp/core.c | 21 ++---
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c
index 9e59b66..6b03f2b 100644
--- a/net/bluetooth/cmtp/core.c
+++ b/net/bluetooth/cmtp/core.c
@@ -280,16 +280,16 @@ static int cmtp_session(void *arg)
struct cmtp_session *session = arg;
struct sock *sk = session->sock->sk;
struct sk_buff *skb;
-   wait_queue_t wait;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
BT_DBG("session %p", session);
 
set_user_nice(current, -15);
 
-   init_waitqueue_entry(, current);
add_wait_queue(sk_sleep(sk), );
while (1) {
-   set_current_state(TASK_INTERRUPTIBLE);
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
 
if (atomic_read(>terminate))
break;
@@ -306,9 +306,8 @@ static int cmtp_session(void *arg)
 
cmtp_process_transmit(session);
 
-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
-   __set_current_state(TASK_RUNNING);
remove_wait_queue(sk_sleep(sk), );
 
down_write(_session_sem);
@@ -393,7 +392,11 @@ int cmtp_add_connection(struct cmtp_connadd_req *req, 
struct socket *sock)
err = cmtp_attach_device(session);
if (err < 0) {
atomic_inc(>terminate);
-   wake_up_process(session->task);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
+
+   wake_up_interruptible(sk_sleep(session->sock->sk));
up_write(_session_sem);
return err;
}
@@ -431,7 +434,11 @@ int cmtp_del_connection(struct cmtp_conndel_req *req)
 
/* Stop session thread */
atomic_inc(>terminate);
-   wake_up_process(session->task);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
+
+   wake_up_interruptible(sk_sleep(session->sock->sk));
} else
err = -ENOENT;
 
-- 
2.1.4

[PATCH 1/3] Bluetooth: bnep: fix possible might sleep error in bnep_session

2017-01-23 Thread Jeffy Chen

It looks like bnep_session has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

 net/bluetooth/bnep/core.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index fbf251f..da04d51 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -484,16 +484,16 @@ static int bnep_session(void *arg)
struct net_device *dev = s->dev;
struct sock *sk = s->sock->sk;
struct sk_buff *skb;
-   wait_queue_t wait;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
BT_DBG("");
 
set_user_nice(current, -15);
 
-   init_waitqueue_entry(, current);
add_wait_queue(sk_sleep(sk), );
while (1) {
-   set_current_state(TASK_INTERRUPTIBLE);
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
 
if (atomic_read(>terminate))
break;
@@ -515,9 +515,8 @@ static int bnep_session(void *arg)
break;
netif_wake_queue(dev);
 
-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
-   __set_current_state(TASK_RUNNING);
remove_wait_queue(sk_sleep(sk), );
 
/* Cleanup session */
@@ -666,7 +665,11 @@ int bnep_del_connection(struct bnep_conndel_req *req)
s = __bnep_get_session(req->dst);
if (s) {
atomic_inc(>terminate);
-   wake_up_process(s->task);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
+
+   wake_up_interruptible(sk_sleep(s->sock->sk));
} else
err = -ENOENT;
 
-- 
2.1.4

[PATCH 3/3] Bluetooth: hidp: fix possible might sleep error in hidp_session_thread

2017-01-23 Thread Jeffy Chen

It looks like hidp_session_thread has same pattern as the issue reported in
old rfcomm:

while (1) {
set_current_state(TASK_INTERRUPTIBLE);
if (condition)
break;
// may call might_sleep here
schedule();
}
__set_current_state(TASK_RUNNING);

Which fixed at:
dfb2fae Bluetooth: Fix nested sleeps

So let's fix it at the same way, also follow the suggestion of:
https://lwn.net/Articles/628628/

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

 net/bluetooth/hidp/core.c | 23 +++
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 0bec458..43d6e6a 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -36,6 +36,7 @@
 #define VERSION "1.2"
 
 static DECLARE_RWSEM(hidp_session_sem);
+static DECLARE_WAIT_QUEUE_HEAD(hidp_session_wq);
 static LIST_HEAD(hidp_session_list);
 
 static unsigned char hidp_keycode[256] = {
@@ -1068,12 +1069,15 @@ static int hidp_session_start_sync(struct hidp_session 
*session)
  * Wake up session thread and notify it to stop. This is asynchronous and
  * returns immediately. Call this whenever a runtime error occurs and you want
  * the session to stop.
- * Note: wake_up_process() performs any necessary memory-barriers for us.
  */
 static void hidp_session_terminate(struct hidp_session *session)
 {
atomic_inc(>terminate);
-   wake_up_process(session->task);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
+
+   wake_up_interruptible(_session_wq);
 }
 
 /*
@@ -1180,7 +1184,9 @@ static void hidp_session_run(struct hidp_session *session)
struct sock *ctrl_sk = session->ctrl_sock->sk;
struct sock *intr_sk = session->intr_sock->sk;
struct sk_buff *skb;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
+   add_wait_queue(_session_wq, );
for (;;) {
/*
 * This thread can be woken up two ways:
@@ -1188,12 +1194,10 @@ static void hidp_session_run(struct hidp_session 
*session)
 *session->terminate flag and wakes this thread up.
 *  - Via modifying the socket state of ctrl/intr_sock. This
 *thread is woken up by ->sk_state_changed().
-*
-* Note: set_current_state() performs any necessary
-* memory-barriers for us.
 */
-   set_current_state(TASK_INTERRUPTIBLE);
 
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
if (atomic_read(>terminate))
break;
 
@@ -1227,11 +1231,14 @@ static void hidp_session_run(struct hidp_session 
*session)
hidp_process_transmit(session, >ctrl_transmit,
  session->ctrl_sock);
 
-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
+   remove_wait_queue(_session_wq, );
 
atomic_inc(>terminate);
-   set_current_state(TASK_RUNNING);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
 }
 
 /*
-- 
2.1.4

[PATCH] Bluetooth: hidp: might sleep error in hidp_session_thread

2017-01-20 Thread Jeffy Chen

[   39.044329] do not call blocking ops when !TASK_RUNNING; state=1 set
at [] hidp_session_thread+0x110/0x568 [hidp]
...
[   40.159664] Call trace:
[   40.162122] [] __might_sleep+0x64/0x90
[   40.167443] [] lock_sock_nested+0x30/0x78
[   40.173047] [] l2cap_sock_sendmsg+0x90/0xf0
[bluetooth]
[   40.179842] [] sock_sendmsg+0x4c/0x68
[   40.185072] [] kernel_sendmsg+0x54/0x68
[   40.190477] [] hidp_send_frame+0x78/0xa0 [hidp]
[   40.196574] [] hidp_process_transmit+0x44/0x98
[hidp]
[   40.203191] [] hidp_session_thread+0x364/0x568
[hidp]

Following (https://lwn.net/Articles/628628/).

Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

 net/bluetooth/hidp/core.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 0bec458..bfd3fb8 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -1180,7 +1180,9 @@ static void hidp_session_run(struct hidp_session *session)
struct sock *ctrl_sk = session->ctrl_sock->sk;
struct sock *intr_sk = session->intr_sock->sk;
struct sk_buff *skb;
+   DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
+   add_wait_queue(sk_sleep(intr_sk), );
for (;;) {
/*
 * This thread can be woken up two ways:
@@ -1188,12 +1190,10 @@ static void hidp_session_run(struct hidp_session 
*session)
 *session->terminate flag and wakes this thread up.
 *  - Via modifying the socket state of ctrl/intr_sock. This
 *thread is woken up by ->sk_state_changed().
-*
-* Note: set_current_state() performs any necessary
-* memory-barriers for us.
 */
-   set_current_state(TASK_INTERRUPTIBLE);
 
+   /* Ensure session->terminate is updated */
+   smp_mb__before_atomic();
if (atomic_read(>terminate))
break;
 
@@ -1227,11 +1227,14 @@ static void hidp_session_run(struct hidp_session 
*session)
hidp_process_transmit(session, >ctrl_transmit,
  session->ctrl_sock);
 
-   schedule();
+   wait_woken(, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
}
+   remove_wait_queue(sk_sleep(intr_sk), );
 
atomic_inc(>terminate);
-   set_current_state(TASK_RUNNING);
+
+   /* Ensure session->terminate is updated */
+   smp_mb__after_atomic();
 }
 
 /*
-- 
2.1.4

45 matches

Mail list logo