Re: [PATCH v2 6/7] dt-bindings: net: realtek: Add property to configure LED mode

[Matthias Kaehlcke]

Hi Florian,

On Wed, Jul 10, 2019 at 09:28:39AM -0700, Florian Fainelli wrote:
> On 7/10/19 8:55 AM, Rob Herring wrote:
> > On Wed, Jul 3, 2019 at 5:23 PM Matthias Kaehlcke  wrote:
> >>
> >> Hi Florian,
> >>
> >> On Wed, Jul 03, 2019 at 02:37:47PM -0700, Florian Fainelli wrote:
> >>> On 7/3/19 12:37 PM, Matthias Kaehlcke wrote:
> >>>> The LED behavior of some Realtek PHYs is configurable. Add the
> >>>> property 'realtek,led-modes' to specify the configuration of the
> >>>> LEDs.
> >>>>
> >>>> Signed-off-by: Matthias Kaehlcke 
> >>>> ---
> >>>> Changes in v2:
> >>>> - patch added to the series
> >>>> ---
> >>>>  .../devicetree/bindings/net/realtek.txt |  9 +
> >>>>  include/dt-bindings/net/realtek.h   | 17 +
> >>>>  2 files changed, 26 insertions(+)
> >>>>  create mode 100644 include/dt-bindings/net/realtek.h
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/net/realtek.txt 
> >>>> b/Documentation/devicetree/bindings/net/realtek.txt
> >>>> index 71d386c78269..40b0d6f9ee21 100644
> >>>> --- a/Documentation/devicetree/bindings/net/realtek.txt
> >>>> +++ b/Documentation/devicetree/bindings/net/realtek.txt
> >>>> @@ -9,6 +9,12 @@ Optional properties:
> >>>>
> >>>> SSC is only available on some Realtek PHYs (e.g. RTL8211E).
> >>>>
> >>>> +- realtek,led-modes: LED mode configuration.
> >>>> +
> >>>> +   A 0..3 element vector, with each element configuring the operating
> >>>> +   mode of an LED. Omitted LEDs are turned off. Allowed values are
> >>>> +   defined in "include/dt-bindings/net/realtek.h".
> >>>
> >>> This should probably be made more general and we should define LED modes
> >>> that makes sense regardless of the PHY device, introduce a set of
> >>> generic functions for validating and then add new function pointer for
> >>> setting the LED configuration to the PHY driver. This would allow to be
> >>> more future proof where each PHY driver could expose standard LEDs class
> >>> devices to user-space, and it would also allow facilities like: ethtool
> >>> -p to plug into that.
> >>>
> >>> Right now, each driver invents its own way of configuring LEDs, that
> >>> does not scale, and there is not really a good reason for that other
> >>> than reviewing drivers in isolation and therefore making it harder to
> >>> extract the commonality. Yes, I realize that since you are the latest
> >>> person submitting something in that area, you are being selected :)
> > 
> > I agree.
> > 
> >> I see the merit of your proposal to come up with a generic mechanism
> >> to configure Ethernet LEDs, however I can't justify spending much of
> >> my work time on this. If it is deemed useful I'm happy to send another
> >> version of the current patchset that addresses the reviewer's comments,
> >> but if the implementation of a generic LED configuration interface is
> >> a requirement I will have to abandon at least the LED configuration
> >> part of this series.
> > 
> > Can you at least define a common binding for this. Maybe that's just
> > removing 'realtek'. While the kernel side can evolve to a common
> > infrastructure, the DT bindings can't.
> 
> That would be a great start, and that is actually what I had in mind
> (should have been more specific), I was not going to have you Matthias
> do the grand slam and convert all this LED configuration into the LEDs
> class etc. that would not be fair.
> 
> It seems to be that we can fairly easily agree on a common binding for
> LED configuration, I would define something along those lines to be
> flexible:
> 
> phy-led-configuration = ;
> 
> where LED_NUM_MASK is one of:
> 
> 0 -> link
> 1 -> activity
> 2 -> speed

I don't understand this proposal completely. Is LED_NUM_MASK actually
a mask/set (potentially containing multiple LEDs) or is it "one of"
the LEDs?

Are you suggesting to assign each LED a specific role (link, activity,
speed)?

Could you maybe post a specific example involving multiple LEDs?

Thanks

Matthias

> that way you can define single/dual/triple LED configurations by
> updating the bitmask.
> 
> LED_CFG_MASK is one of:
> 
> 0 -> LED_CFG_10
> 1 -> LED_CFG_100
> 2 -> LED_CFG_1000
> 
> (let's assume 1Gbps or less for now)
> 
> or this can be combined in a single cell with a left shift.
> 
> Andrew, Heiner, do you see that approach working correctly and scaling
> appropriately?

[PATCH v4 1/3] Bluetooth: Add quirk for reading BD_ADDR from fwnode property

[Matthias Kaehlcke]

Add HCI_QUIRK_USE_BDADDR_PROPERTY to allow controllers to retrieve
the public Bluetooth address from the firmware node property
'local-bd-address'. If quirk is set and the property does not exist
or is invalid the controller is marked as unconfigured.

Signed-off-by: Matthias Kaehlcke 
Reviewed-by: Balakrishna Godavarthi 
Tested-by: Balakrishna Godavarthi 
---
Changes in v4:
- none

Changes in v3:
- return no value from hci_dev_get_bd_addr_from_property() since
  currently nobody uses it anyway
- use bacpy() in hci_dev_get_bd_addr_from_property()
- return -EADDRNOTAVAIL if no BD_ADDR is configured or if the driver
  has no ->set_bdaddr

Changes in v2:
- added check for return value of ->setup()
- only read BD_ADDR from the property if it isn't assigned yet. This
  is needed to support configuration from user space
- refactored the branch of the new quirk to get rid of 'bd_addr_set'
- added 'Reviewed-by: Balakrishna Godavarthi ' tag
---
 include/net/bluetooth/hci.h | 12 +++
 net/bluetooth/hci_core.c| 43 +
 net/bluetooth/mgmt.c|  6 --
 3 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c36dc1e20556a..fbba43e9bef5b 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -158,6 +158,18 @@ enum {
 */
HCI_QUIRK_INVALID_BDADDR,
 
+   /* When this quirk is set, the public Bluetooth address
+* initially reported by HCI Read BD Address command
+* is considered invalid. The public BD Address can be
+* specified in the fwnode property 'local-bd-address'.
+* If this property does not exist or is invalid controller
+* configuration is required before this device can be used.
+*
+* This quirk can be set before hci_register_dev is called or
+* during the hdev->setup vendor callback.
+*/
+   HCI_QUIRK_USE_BDADDR_PROPERTY,
+
/* When this quirk is set, the duplicate filtering during
 * scanning is based on Bluetooth devices addresses. To allow
 * RSSI based updates, restart scanning if needed.
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 26e3d36aee298..d6b2540ba7f8b 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -1355,6 +1356,32 @@ int hci_inquiry(void __user *arg)
return err;
 }
 
+/**
+ * hci_dev_get_bd_addr_from_property - Get the Bluetooth Device Address
+ *(BD_ADDR) for a HCI device from
+ *a firmware node property.
+ * @hdev:  The HCI device
+ *
+ * Search the firmware node for 'local-bd-address'.
+ *
+ * All-zero BD addresses are rejected, because those could be properties
+ * that exist in the firmware tables, but were not updated by the firmware. For
+ * example, the DTS could define 'local-bd-address', with zero BD addresses.
+ */
+static void hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
+{
+   struct fwnode_handle *fwnode = dev_fwnode(hdev->dev.parent);
+   bdaddr_t ba;
+   int ret;
+
+   ret = fwnode_property_read_u8_array(fwnode, "local-bd-address",
+   (u8 *)&ba, sizeof(ba));
+   if (ret < 0 || !bacmp(&ba, BDADDR_ANY))
+   return;
+
+   bacpy(&hdev->public_addr, &ba);
+}
+
 static int hci_dev_do_open(struct hci_dev *hdev)
 {
int ret = 0;
@@ -1422,6 +1449,22 @@ static int hci_dev_do_open(struct hci_dev *hdev)
if (hdev->setup)
ret = hdev->setup(hdev);
 
+   if (ret)
+   goto setup_failed;
+
+   if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) {
+   if (!bacmp(&hdev->public_addr, BDADDR_ANY))
+   hci_dev_get_bd_addr_from_property(hdev);
+
+   if (bacmp(&hdev->public_addr, BDADDR_ANY) &&
+   hdev->set_bdaddr)
+   ret = hdev->set_bdaddr(hdev,
+  &hdev->public_addr);
+   else
+   ret = -EADDRNOTAVAIL;
+   }
+
+setup_failed:
/* The transport driver can set these quirks before
 * creating the HCI device or in its setup callback.
 *
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index ccce954f81468..fae84353d030f 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -551,7 +551,8 @@ static bool is_configured(struct hci_dev *hdev)
!hci_dev_test_flag(hdev, HCI_EXT_CONFIGURED))
 

[PATCH v4 2/3] Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY

[Matthias Kaehlcke]

Use the HCI_QUIRK_USE_BDADDR_PROPERTY quirk to let the HCI
core handle the reading of 'local-bd-address'. With this there
is no need to set HCI_QUIRK_INVALID_BDADDR, the case of a
non-existing or invalid fwnode property is handled by the core
code.

Signed-off-by: Matthias Kaehlcke 
Reviewed-by: Balakrishna Godavarthi 
---
Changes in v4:
- btqcomsmd_setup(): removed unused variables 'err' and 'btq'

Changes in v3:
- none

Changes in v2:
- removed now unused field 'bdaddr' from struct btqcomsmd
- added 'Reviewed-by: Balakrishna Godavarthi ' tag
---
 drivers/bluetooth/btqcomsmd.c | 31 +++
 1 file changed, 3 insertions(+), 28 deletions(-)

diff --git a/drivers/bluetooth/btqcomsmd.c b/drivers/bluetooth/btqcomsmd.c
index 7df3eed1ef5e9..e0d4c6f1d3ab6 100644
--- a/drivers/bluetooth/btqcomsmd.c
+++ b/drivers/bluetooth/btqcomsmd.c
@@ -28,7 +28,6 @@
 struct btqcomsmd {
struct hci_dev *hdev;
 
-   bdaddr_t bdaddr;
struct rpmsg_endpoint *acl_channel;
struct rpmsg_endpoint *cmd_channel;
 };
@@ -116,32 +115,17 @@ static int btqcomsmd_close(struct hci_dev *hdev)
 
 static int btqcomsmd_setup(struct hci_dev *hdev)
 {
-   struct btqcomsmd *btq = hci_get_drvdata(hdev);
struct sk_buff *skb;
-   int err;
 
skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
if (IS_ERR(skb))
return PTR_ERR(skb);
kfree_skb(skb);
 
-   /* Devices do not have persistent storage for BD address. If no
-* BD address has been retrieved during probe, mark the device
-* as having an invalid BD address.
-*/
-   if (!bacmp(&btq->bdaddr, BDADDR_ANY)) {
-   set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
-   return 0;
-   }
-
-   /* When setting a configured BD address fails, mark the device
-* as having an invalid BD address.
+   /* Devices do not have persistent storage for BD address. Retrieve
+* it from the firmware node property.
 */
-   err = qca_set_bdaddr_rome(hdev, &btq->bdaddr);
-   if (err) {
-   set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
-   return 0;
-   }
+   set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
 
return 0;
 }
@@ -169,15 +153,6 @@ static int btqcomsmd_probe(struct platform_device *pdev)
if (IS_ERR(btq->cmd_channel))
return PTR_ERR(btq->cmd_channel);
 
-   /* The local-bd-address property is usually injected by the
-* bootloader which has access to the allocated BD address.
-*/
-   if (!of_property_read_u8_array(pdev->dev.of_node, "local-bd-address",
-  (u8 *)&btq->bdaddr, sizeof(bdaddr_t))) {
-   dev_info(&pdev->dev, "BD address %pMR retrieved from 
device-tree",
-&btq->bdaddr);
-   }
-
hdev = hci_alloc_dev();
if (!hdev)
return -ENOMEM;
-- 
2.21.0.rc0.258.g878e2cd30e-goog

[PATCH v4 0/3] Add quirk for reading BD_ADDR from fwnode property

[Matthias Kaehlcke]

On some systems the Bluetooth Device Address (BD_ADDR) isn't stored
on the Bluetooth chip itself. One way to configure the address is
through the device tree (patched in by the bootloader). The btqcomsmd
driver is an example, it can read the address from the DT property
'local-bd-address'.

To avoid redundant open-coded reading of 'local-bd-address' and error
handling this series adds the quirk HCI_QUIRK_USE_BDADDR_PROPERTY to
retrieve the BD address of a device from the DT and adapts the
btqcomsmd and hci_qca drivers to use this quirk.

Matthias Kaehlcke (3):
  Bluetooth: Add quirk for reading BD_ADDR from fwnode property
  Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY
  Bluetooth: hci_qca: Set HCI_QUIRK_USE_BDADDR_PROPERTY for wcn3990

 drivers/bluetooth/btqcomsmd.c | 31 +++--
 drivers/bluetooth/hci_qca.c   |  1 +
 include/net/bluetooth/hci.h   | 12 ++
 net/bluetooth/hci_core.c  | 43 +++
 net/bluetooth/mgmt.c  |  6 +++--
 5 files changed, 63 insertions(+), 30 deletions(-)

-- 
2.21.0.rc0.258.g878e2cd30e-goog

[PATCH v4 3/3] Bluetooth: hci_qca: Set HCI_QUIRK_USE_BDADDR_PROPERTY for wcn3990

[Matthias Kaehlcke]

Set quirk for wcn3990 to read BD_ADDR from a firmware node property.

Signed-off-by: Matthias Kaehlcke 
Tested-by: Balakrishna Godavarthi 
---
Changes in v4:
-none

Changes in v3:
- none

Changes in v2:
- patch added to the series
---
 drivers/bluetooth/hci_qca.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 5e03504c4e0ca..26efc2ef98d9a 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1192,6 +1192,7 @@ static int qca_setup(struct hci_uart *hu)
 * setup for every hci up.
 */
set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
+   set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
hu->hdev->shutdown = qca_power_off;
ret = qca_wcn3990_init(hu);
if (ret)
-- 
2.21.0.rc0.258.g878e2cd30e-goog

Re: [PATCH RESEND 0/3] Add quirk for reading BD_ADDR from fwnode property

[Matthias Kaehlcke]

Hi Marcel,

On Mon, Feb 18, 2019 at 11:48:21AM +0100, Marcel Holtmann wrote:
> Hi Matthias,
> 
> > [initial post: https://lore.kernel.org/patchwork/cover/1028184/]
> > 
> > On some systems the Bluetooth Device Address (BD_ADDR) isn't stored
> > on the Bluetooth chip itself. One way to configure the address is
> > through the device tree (patched in by the bootloader). The btqcomsmd
> > driver is an example, it can read the address from the DT property
> > 'local-bd-address'.
> > 
> > To avoid redundant open-coded reading of 'local-bd-address' and error
> > handling this series adds the quirk HCI_QUIRK_USE_BDADDR_PROPERTY to
> > retrieve the BD address of a device from the DT and adapts the
> > btqcomsmd and hci_qca drivers to use this quirk.
> > 
> > Matthias Kaehlcke (3):
> >  Bluetooth: Add quirk for reading BD_ADDR from fwnode property
> >  Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY
> >  Bluetooth: hci_qca: Set HCI_QUIRK_USE_BDADDR_PROPERTY for wcn3990
> > 
> > drivers/bluetooth/btqcomsmd.c | 29 +++
> > drivers/bluetooth/hci_qca.c   |  1 +
> > include/net/bluetooth/hci.h   | 12 ++
> > net/bluetooth/hci_core.c  | 43 +++
> > net/bluetooth/mgmt.c  |  6 +++--
> > 5 files changed, 63 insertions(+), 28 deletions(-)
> 
> I am getting compiler warnings when trying to apply this set:
> 
>   CC  drivers/bluetooth/btqcomsmd.o
> drivers/bluetooth/btqcomsmd.c: In function ‘btqcomsmd_setup’:
> drivers/bluetooth/btqcomsmd.c:120:6: warning: unused variable ‘err’ 
> [-Wunused-variable]
>   int err;
>   ^~~
> drivers/bluetooth/btqcomsmd.c:118:20: warning: unused variable ‘btq’ 
> [-Wunused-variable]
>   struct btqcomsmd *btq = hci_get_drvdata(hdev);
> ^~~

Sorry, I missed that the variables aren't used anymore. I'll send a
new version soon.

Thanks

Matthias

[PATCH RESEND 0/3] Add quirk for reading BD_ADDR from fwnode property

[Matthias Kaehlcke]

[initial post: https://lore.kernel.org/patchwork/cover/1028184/]

On some systems the Bluetooth Device Address (BD_ADDR) isn't stored
on the Bluetooth chip itself. One way to configure the address is
through the device tree (patched in by the bootloader). The btqcomsmd
driver is an example, it can read the address from the DT property
'local-bd-address'.

To avoid redundant open-coded reading of 'local-bd-address' and error
handling this series adds the quirk HCI_QUIRK_USE_BDADDR_PROPERTY to
retrieve the BD address of a device from the DT and adapts the
btqcomsmd and hci_qca drivers to use this quirk.

Matthias Kaehlcke (3):
  Bluetooth: Add quirk for reading BD_ADDR from fwnode property
  Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY
  Bluetooth: hci_qca: Set HCI_QUIRK_USE_BDADDR_PROPERTY for wcn3990

 drivers/bluetooth/btqcomsmd.c | 29 +++
 drivers/bluetooth/hci_qca.c   |  1 +
 include/net/bluetooth/hci.h   | 12 ++
 net/bluetooth/hci_core.c  | 43 +++
 net/bluetooth/mgmt.c  |  6 +++--
 5 files changed, 63 insertions(+), 28 deletions(-)

-- 
2.20.1.495.gaa96b0ce6b-goog

[PATCH RESEND 2/3] Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY

[Matthias Kaehlcke]

Use the HCI_QUIRK_USE_BDADDR_PROPERTY quirk to let the HCI
core handle the reading of 'local-bd-address'. With this there
is no need to set HCI_QUIRK_INVALID_BDADDR, the case of a
non-existing or invalid fwnode property is handled by the core
code.

Signed-off-by: Matthias Kaehlcke 
Reviewed-by: Balakrishna Godavarthi 
---
Changes in v3:
- none

Changes in v2:
- removed now unused field 'bdaddr' from struct btqcomsmd
- added 'Reviewed-by: Balakrishna Godavarthi ' tag
---
 drivers/bluetooth/btqcomsmd.c | 29 +++--
 1 file changed, 3 insertions(+), 26 deletions(-)

diff --git a/drivers/bluetooth/btqcomsmd.c b/drivers/bluetooth/btqcomsmd.c
index 7df3eed1ef5e9..b3020fab6c8e3 100644
--- a/drivers/bluetooth/btqcomsmd.c
+++ b/drivers/bluetooth/btqcomsmd.c
@@ -28,7 +28,6 @@
 struct btqcomsmd {
struct hci_dev *hdev;
 
-   bdaddr_t bdaddr;
struct rpmsg_endpoint *acl_channel;
struct rpmsg_endpoint *cmd_channel;
 };
@@ -125,23 +124,10 @@ static int btqcomsmd_setup(struct hci_dev *hdev)
return PTR_ERR(skb);
kfree_skb(skb);
 
-   /* Devices do not have persistent storage for BD address. If no
-* BD address has been retrieved during probe, mark the device
-* as having an invalid BD address.
+   /* Devices do not have persistent storage for BD address. Retrieve
+* it from the firmware node property.
 */
-   if (!bacmp(&btq->bdaddr, BDADDR_ANY)) {
-   set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
-   return 0;
-   }
-
-   /* When setting a configured BD address fails, mark the device
-* as having an invalid BD address.
-*/
-   err = qca_set_bdaddr_rome(hdev, &btq->bdaddr);
-   if (err) {
-   set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
-   return 0;
-   }
+   set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
 
return 0;
 }
@@ -169,15 +155,6 @@ static int btqcomsmd_probe(struct platform_device *pdev)
if (IS_ERR(btq->cmd_channel))
return PTR_ERR(btq->cmd_channel);
 
-   /* The local-bd-address property is usually injected by the
-* bootloader which has access to the allocated BD address.
-*/
-   if (!of_property_read_u8_array(pdev->dev.of_node, "local-bd-address",
-  (u8 *)&btq->bdaddr, sizeof(bdaddr_t))) {
-   dev_info(&pdev->dev, "BD address %pMR retrieved from 
device-tree",
-&btq->bdaddr);
-   }
-
hdev = hci_alloc_dev();
if (!hdev)
return -ENOMEM;
-- 
2.20.1.495.gaa96b0ce6b-goog

[PATCH RESEND 1/3] Bluetooth: Add quirk for reading BD_ADDR from fwnode property

[Matthias Kaehlcke]

Add HCI_QUIRK_USE_BDADDR_PROPERTY to allow controllers to retrieve
the public Bluetooth address from the firmware node property
'local-bd-address'. If quirk is set and the property does not exist
or is invalid the controller is marked as unconfigured.

Signed-off-by: Matthias Kaehlcke 
Reviewed-by: Balakrishna Godavarthi 
Tested-by: Balakrishna Godavarthi 
---
Changes in v3:
- return no value from hci_dev_get_bd_addr_from_property() since
  currently nobody uses it anyway
- use bacpy() in hci_dev_get_bd_addr_from_property()
- return -EADDRNOTAVAIL if no BD_ADDR is configured or if the driver
  has no ->set_bdaddr

Changes in v2:
- added check for return value of ->setup()
- only read BD_ADDR from the property if it isn't assigned yet. This
  is needed to support configuration from user space
- refactored the branch of the new quirk to get rid of 'bd_addr_set'
- added 'Reviewed-by: Balakrishna Godavarthi ' tag
---
 include/net/bluetooth/hci.h | 12 +++
 net/bluetooth/hci_core.c| 43 +
 net/bluetooth/mgmt.c|  6 --
 3 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c36dc1e20556a..fbba43e9bef5b 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -158,6 +158,18 @@ enum {
 */
HCI_QUIRK_INVALID_BDADDR,
 
+   /* When this quirk is set, the public Bluetooth address
+* initially reported by HCI Read BD Address command
+* is considered invalid. The public BD Address can be
+* specified in the fwnode property 'local-bd-address'.
+* If this property does not exist or is invalid controller
+* configuration is required before this device can be used.
+*
+* This quirk can be set before hci_register_dev is called or
+* during the hdev->setup vendor callback.
+*/
+   HCI_QUIRK_USE_BDADDR_PROPERTY,
+
/* When this quirk is set, the duplicate filtering during
 * scanning is based on Bluetooth devices addresses. To allow
 * RSSI based updates, restart scanning if needed.
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7352fe85674be..3ccfa351b87bb 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -1355,6 +1356,32 @@ int hci_inquiry(void __user *arg)
return err;
 }
 
+/**
+ * hci_dev_get_bd_addr_from_property - Get the Bluetooth Device Address
+ *(BD_ADDR) for a HCI device from
+ *a firmware node property.
+ * @hdev:  The HCI device
+ *
+ * Search the firmware node for 'local-bd-address'.
+ *
+ * All-zero BD addresses are rejected, because those could be properties
+ * that exist in the firmware tables, but were not updated by the firmware. For
+ * example, the DTS could define 'local-bd-address', with zero BD addresses.
+ */
+static void hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
+{
+   struct fwnode_handle *fwnode = dev_fwnode(hdev->dev.parent);
+   bdaddr_t ba;
+   int ret;
+
+   ret = fwnode_property_read_u8_array(fwnode, "local-bd-address",
+   (u8 *)&ba, sizeof(ba));
+   if (ret < 0 || !bacmp(&ba, BDADDR_ANY))
+   return;
+
+   bacpy(&hdev->public_addr, &ba);
+}
+
 static int hci_dev_do_open(struct hci_dev *hdev)
 {
int ret = 0;
@@ -1422,6 +1449,22 @@ static int hci_dev_do_open(struct hci_dev *hdev)
if (hdev->setup)
ret = hdev->setup(hdev);
 
+   if (ret)
+   goto setup_failed;
+
+   if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) {
+   if (!bacmp(&hdev->public_addr, BDADDR_ANY))
+   hci_dev_get_bd_addr_from_property(hdev);
+
+   if (bacmp(&hdev->public_addr, BDADDR_ANY) &&
+   hdev->set_bdaddr)
+   ret = hdev->set_bdaddr(hdev,
+  &hdev->public_addr);
+   else
+   ret = -EADDRNOTAVAIL;
+   }
+
+setup_failed:
/* The transport driver can set these quirks before
 * creating the HCI device or in its setup callback.
 *
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index ccce954f81468..fae84353d030f 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -551,7 +551,8 @@ static bool is_configured(struct hci_dev *hdev)
!hci_dev_test_flag(hdev, HCI_EXT_CONFIGURED))
return fals

[PATCH RESEND 3/3] Bluetooth: hci_qca: Set HCI_QUIRK_USE_BDADDR_PROPERTY for wcn3990

[Matthias Kaehlcke]

Set quirk for wcn3990 to read BD_ADDR from a firmware node property.

Signed-off-by: Matthias Kaehlcke 
Tested-by: Balakrishna Godavarthi 
---
Changes in v3:
- none

Changes in v2:
- patch added to the series
---
 drivers/bluetooth/hci_qca.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index f036c8f98ea33..0535833caa52c 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1193,6 +1193,7 @@ static int qca_setup(struct hci_uart *hu)
 * setup for every hci up.
 */
set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
+   set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
hu->hdev->shutdown = qca_power_off;
ret = qca_wcn3990_init(hu);
if (ret)
-- 
2.20.1.495.gaa96b0ce6b-goog

Re: [PATCH v3 1/3] Bluetooth: Add quirk for reading BD_ADDR from fwnode property

[Matthias Kaehlcke]

On Fri, Dec 28, 2018 at 02:05:44PM -0800, Matthias Kaehlcke wrote:
> Add HCI_QUIRK_USE_BDADDR_PROPERTY to allow controllers to retrieve
> the public Bluetooth address from the firmware node property
> 'local-bd-address'. If quirk is set and the property does not exist
> or is invalid the controller is marked as unconfigured.
> 
> Signed-off-by: Matthias Kaehlcke 
> Reviewed-by: Balakrishna Godavarthi 
> Tested-by: Balakrishna Godavarthi 
> ---
> Changes in v3:
> - return no value from hci_dev_get_bd_addr_from_property() since
>   currently nobody uses it anyway
> - use bacpy() in hci_dev_get_bd_addr_from_property()
> - return -EADDRNOTAVAIL if no BD_ADDR is configured or if the driver
>   has no ->set_bdaddr
> 
> Changes in v2:
> - added check for return value of ->setup()
> - only read BD_ADDR from the property if it isn't assigned yet. This
>   is needed to support configuration from user space
> - refactored the branch of the new quirk to get rid of 'bd_addr_set'
> - added 'Reviewed-by: Balakrishna Godavarthi ' tag
> ---
>  include/net/bluetooth/hci.h | 12 +++
>  net/bluetooth/hci_core.c| 43 +
>  net/bluetooth/mgmt.c|  6 --
>  3 files changed, 59 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index c36dc1e20556a..fbba43e9bef5b 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -158,6 +158,18 @@ enum {
>*/
>   HCI_QUIRK_INVALID_BDADDR,
>  
> + /* When this quirk is set, the public Bluetooth address
> +  * initially reported by HCI Read BD Address command
> +  * is considered invalid. The public BD Address can be
> +  * specified in the fwnode property 'local-bd-address'.
> +  * If this property does not exist or is invalid controller
> +  * configuration is required before this device can be used.
> +  *
> +  * This quirk can be set before hci_register_dev is called or
> +  * during the hdev->setup vendor callback.
> +  */
> + HCI_QUIRK_USE_BDADDR_PROPERTY,
> +
>   /* When this quirk is set, the duplicate filtering during
>* scanning is based on Bluetooth devices addresses. To allow
>* RSSI based updates, restart scanning if needed.
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 7352fe85674be..3ccfa351b87bb 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include 
> @@ -1355,6 +1356,32 @@ int hci_inquiry(void __user *arg)
>   return err;
>  }
>  
> +/**
> + * hci_dev_get_bd_addr_from_property - Get the Bluetooth Device Address
> + *  (BD_ADDR) for a HCI device from
> + *  a firmware node property.
> + * @hdev:The HCI device
> + *
> + * Search the firmware node for 'local-bd-address'.
> + *
> + * All-zero BD addresses are rejected, because those could be properties
> + * that exist in the firmware tables, but were not updated by the firmware. 
> For
> + * example, the DTS could define 'local-bd-address', with zero BD addresses.
> + */
> +static void hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
> +{
> + struct fwnode_handle *fwnode = dev_fwnode(hdev->dev.parent);
> + bdaddr_t ba;
> + int ret;
> +
> + ret = fwnode_property_read_u8_array(fwnode, "local-bd-address",
> + (u8 *)&ba, sizeof(ba));
> + if (ret < 0 || !bacmp(&ba, BDADDR_ANY))
> + return;
> +
> + bacpy(&hdev->public_addr, &ba);
> +}
> +
>  static int hci_dev_do_open(struct hci_dev *hdev)
>  {
>   int ret = 0;
> @@ -1422,6 +1449,22 @@ static int hci_dev_do_open(struct hci_dev *hdev)
>   if (hdev->setup)
>   ret = hdev->setup(hdev);
>  
> + if (ret)
> + goto setup_failed;
> +
> + if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) {
> + if (!bacmp(&hdev->public_addr, BDADDR_ANY))
> + hci_dev_get_bd_addr_from_property(hdev);
> +
> + if (bacmp(&hdev->public_addr, BDADDR_ANY) &&
> + hdev->set_bdaddr)
> + ret = hdev->set_bdaddr(hdev,
> +&hdev->public_addr);
> +

[PATCH v2] Bluetooth: Fix locking in bt_accept_enqueue() for BH context

[Matthias Kaehlcke]

With commit e16337622016 ("Bluetooth: Handle bt_accept_enqueue() socket
atomically") lock_sock[_nested]() is used to acquire the socket lock
before manipulating the socket. lock_sock[_nested]() may block, which
is problematic since bt_accept_enqueue() can be called in bottom half
context (e.g. from rfcomm_connect_ind()):

[] __might_sleep+0x4c/0x80
[] lock_sock_nested+0x24/0x58
[] bt_accept_enqueue+0x48/0xd4 [bluetooth]
[] rfcomm_connect_ind+0x190/0x218 [rfcomm]

Add a parameter to bt_accept_enqueue() to indicate whether the
function is called from BH context, and acquire the socket lock
with bh_lock_sock_nested() if that's the case.

Also adapt all callers of bt_accept_enqueue() to pass the new
parameter:

- l2cap_sock_new_connection_cb()
  - uses lock_sock() to lock the parent socket => process context

- rfcomm_connect_ind()
  - acquires the parent socket lock with bh_lock_sock() => BH
context

- __sco_chan_add()
  - called from sco_chan_add(), which is called from sco_connect().
parent is NULL, hence bt_accept_enqueue() isn't called in this
code path and we can ignore it
  - also called from sco_conn_ready(). uses bh_lock_sock() to acquire
the parent lock => BH context

Fixes: e16337622016 ("Bluetooth: Handle bt_accept_enqueue() socket atomically")
Signed-off-by: Matthias Kaehlcke 
---
Changes in v2:
- use parameter in bt_accept_enqueue() to decide which lock to
  acquire and adapt all callers
- updated commit message
---
 include/net/bluetooth/bluetooth.h |  2 +-
 net/bluetooth/af_bluetooth.c  | 16 +---
 net/bluetooth/l2cap_sock.c|  2 +-
 net/bluetooth/rfcomm/sock.c   |  2 +-
 net/bluetooth/sco.c   |  2 +-
 5 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h 
b/include/net/bluetooth/bluetooth.h
index ec9d6bc658559..fabee6db0abb7 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -276,7 +276,7 @@ int  bt_sock_ioctl(struct socket *sock, unsigned int cmd, 
unsigned long arg);
 int  bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);
 int  bt_sock_wait_ready(struct sock *sk, unsigned long flags);
 
-void bt_accept_enqueue(struct sock *parent, struct sock *sk);
+void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh);
 void bt_accept_unlink(struct sock *sk);
 struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock);
 
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index deacc52d7ff18..8d12198eaa949 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -154,15 +154,25 @@ void bt_sock_unlink(struct bt_sock_list *l, struct sock 
*sk)
 }
 EXPORT_SYMBOL(bt_sock_unlink);
 
-void bt_accept_enqueue(struct sock *parent, struct sock *sk)
+void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh)
 {
BT_DBG("parent %p, sk %p", parent, sk);
 
sock_hold(sk);
-   lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
+
+   if (bh)
+   bh_lock_sock_nested(sk);
+   else
+   lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
+
list_add_tail(&bt_sk(sk)->accept_q, &bt_sk(parent)->accept_q);
bt_sk(sk)->parent = parent;
-   release_sock(sk);
+
+   if (bh)
+   bh_unlock_sock(sk);
+   else
+   release_sock(sk);
+
parent->sk_ack_backlog++;
 }
 EXPORT_SYMBOL(bt_accept_enqueue);
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 686bdc6b35b03..a3a2cd55e23a9 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1252,7 +1252,7 @@ static struct l2cap_chan 
*l2cap_sock_new_connection_cb(struct l2cap_chan *chan)
 
l2cap_sock_init(sk, parent);
 
-   bt_accept_enqueue(parent, sk);
+   bt_accept_enqueue(parent, sk, false);
 
release_sock(parent);
 
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index aa0db1d1bd9b4..b1f49fcc04780 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -988,7 +988,7 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 
channel, struct rfcomm_dlc *
rfcomm_pi(sk)->channel = channel;
 
sk->sk_state = BT_CONFIG;
-   bt_accept_enqueue(parent, sk);
+   bt_accept_enqueue(parent, sk, true);
 
/* Accept connection and return socket DLC */
*d = rfcomm_pi(sk)->dlc;
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 529b38996d8bc..9a580999ca57e 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -193,7 +193,7 @@ static void __sco_chan_add(struct sco_conn *conn, struct 
sock *sk,
conn->sk = sk;
 
if (parent)
-   bt_accept_enqueue(parent, sk);
+   bt_accept_enqueue(parent, sk, true);
 }
 
 static int sco_chan_add(struct sco_conn *conn, struct sock *sk,
-- 
2.20.1.415.g653613c723-goog

[PATCH v3 0/3] Add quirk for reading BD_ADDR from fwnode property

[Matthias Kaehlcke]

On some systems the Bluetooth Device Address (BD_ADDR) isn't stored
on the Bluetooth chip itself. One way to configure the address is
through the device tree (patched in by the bootloader). The btqcomsmd
driver is an example, it can read the address from the DT property
'local-bd-address'.

To avoid redundant open-coded reading of 'local-bd-address' and error
handling this series adds the quirk HCI_QUIRK_USE_BDADDR_PROPERTY to
retrieve the BD address of a device from the DT and adapts the
btqcomsmd and hci_qca drivers to use this quirk.

Matthias Kaehlcke (3):
  Bluetooth: Add quirk for reading BD_ADDR from fwnode property
  Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY
  Bluetooth: hci_qca: Set HCI_QUIRK_USE_BDADDR_PROPERTY for wcn3990

 drivers/bluetooth/btqcomsmd.c | 29 +++
 drivers/bluetooth/hci_qca.c   |  1 +
 include/net/bluetooth/hci.h   | 12 ++
 net/bluetooth/hci_core.c  | 43 +++
 net/bluetooth/mgmt.c  |  6 +++--
 5 files changed, 63 insertions(+), 28 deletions(-)

-- 
2.20.1.415.g653613c723-goog

[PATCH v3 1/3] Bluetooth: Add quirk for reading BD_ADDR from fwnode property

[Matthias Kaehlcke]

Add HCI_QUIRK_USE_BDADDR_PROPERTY to allow controllers to retrieve
the public Bluetooth address from the firmware node property
'local-bd-address'. If quirk is set and the property does not exist
or is invalid the controller is marked as unconfigured.

Signed-off-by: Matthias Kaehlcke 
Reviewed-by: Balakrishna Godavarthi 
Tested-by: Balakrishna Godavarthi 
---
Changes in v3:
- return no value from hci_dev_get_bd_addr_from_property() since
  currently nobody uses it anyway
- use bacpy() in hci_dev_get_bd_addr_from_property()
- return -EADDRNOTAVAIL if no BD_ADDR is configured or if the driver
  has no ->set_bdaddr

Changes in v2:
- added check for return value of ->setup()
- only read BD_ADDR from the property if it isn't assigned yet. This
  is needed to support configuration from user space
- refactored the branch of the new quirk to get rid of 'bd_addr_set'
- added 'Reviewed-by: Balakrishna Godavarthi ' tag
---
 include/net/bluetooth/hci.h | 12 +++
 net/bluetooth/hci_core.c| 43 +
 net/bluetooth/mgmt.c|  6 --
 3 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c36dc1e20556a..fbba43e9bef5b 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -158,6 +158,18 @@ enum {
 */
HCI_QUIRK_INVALID_BDADDR,
 
+   /* When this quirk is set, the public Bluetooth address
+* initially reported by HCI Read BD Address command
+* is considered invalid. The public BD Address can be
+* specified in the fwnode property 'local-bd-address'.
+* If this property does not exist or is invalid controller
+* configuration is required before this device can be used.
+*
+* This quirk can be set before hci_register_dev is called or
+* during the hdev->setup vendor callback.
+*/
+   HCI_QUIRK_USE_BDADDR_PROPERTY,
+
/* When this quirk is set, the duplicate filtering during
 * scanning is based on Bluetooth devices addresses. To allow
 * RSSI based updates, restart scanning if needed.
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7352fe85674be..3ccfa351b87bb 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -1355,6 +1356,32 @@ int hci_inquiry(void __user *arg)
return err;
 }
 
+/**
+ * hci_dev_get_bd_addr_from_property - Get the Bluetooth Device Address
+ *(BD_ADDR) for a HCI device from
+ *a firmware node property.
+ * @hdev:  The HCI device
+ *
+ * Search the firmware node for 'local-bd-address'.
+ *
+ * All-zero BD addresses are rejected, because those could be properties
+ * that exist in the firmware tables, but were not updated by the firmware. For
+ * example, the DTS could define 'local-bd-address', with zero BD addresses.
+ */
+static void hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
+{
+   struct fwnode_handle *fwnode = dev_fwnode(hdev->dev.parent);
+   bdaddr_t ba;
+   int ret;
+
+   ret = fwnode_property_read_u8_array(fwnode, "local-bd-address",
+   (u8 *)&ba, sizeof(ba));
+   if (ret < 0 || !bacmp(&ba, BDADDR_ANY))
+   return;
+
+   bacpy(&hdev->public_addr, &ba);
+}
+
 static int hci_dev_do_open(struct hci_dev *hdev)
 {
int ret = 0;
@@ -1422,6 +1449,22 @@ static int hci_dev_do_open(struct hci_dev *hdev)
if (hdev->setup)
ret = hdev->setup(hdev);
 
+   if (ret)
+   goto setup_failed;
+
+   if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) {
+   if (!bacmp(&hdev->public_addr, BDADDR_ANY))
+   hci_dev_get_bd_addr_from_property(hdev);
+
+   if (bacmp(&hdev->public_addr, BDADDR_ANY) &&
+   hdev->set_bdaddr)
+   ret = hdev->set_bdaddr(hdev,
+  &hdev->public_addr);
+   else
+   ret = -EADDRNOTAVAIL;
+   }
+
+setup_failed:
/* The transport driver can set these quirks before
 * creating the HCI device or in its setup callback.
 *
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index ccce954f81468..fae84353d030f 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -551,7 +551,8 @@ static bool is_configured(struct hci_dev *hdev)
!hci_dev_test_flag(hdev, HCI_EXT_CONFIGURED))
return fals

[PATCH v3 2/3] Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY

[Matthias Kaehlcke]

Use the HCI_QUIRK_USE_BDADDR_PROPERTY quirk to let the HCI
core handle the reading of 'local-bd-address'. With this there
is no need to set HCI_QUIRK_INVALID_BDADDR, the case of a
non-existing or invalid fwnode property is handled by the core
code.

Signed-off-by: Matthias Kaehlcke 
Reviewed-by: Balakrishna Godavarthi 
---
Changes in v3:
- none

Changes in v2:
- removed now unused field 'bdaddr' from struct btqcomsmd
- added 'Reviewed-by: Balakrishna Godavarthi ' tag
---
 drivers/bluetooth/btqcomsmd.c | 29 +++--
 1 file changed, 3 insertions(+), 26 deletions(-)

diff --git a/drivers/bluetooth/btqcomsmd.c b/drivers/bluetooth/btqcomsmd.c
index 7df3eed1ef5e9..b3020fab6c8e3 100644
--- a/drivers/bluetooth/btqcomsmd.c
+++ b/drivers/bluetooth/btqcomsmd.c
@@ -28,7 +28,6 @@
 struct btqcomsmd {
struct hci_dev *hdev;
 
-   bdaddr_t bdaddr;
struct rpmsg_endpoint *acl_channel;
struct rpmsg_endpoint *cmd_channel;
 };
@@ -125,23 +124,10 @@ static int btqcomsmd_setup(struct hci_dev *hdev)
return PTR_ERR(skb);
kfree_skb(skb);
 
-   /* Devices do not have persistent storage for BD address. If no
-* BD address has been retrieved during probe, mark the device
-* as having an invalid BD address.
+   /* Devices do not have persistent storage for BD address. Retrieve
+* it from the firmware node property.
 */
-   if (!bacmp(&btq->bdaddr, BDADDR_ANY)) {
-   set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
-   return 0;
-   }
-
-   /* When setting a configured BD address fails, mark the device
-* as having an invalid BD address.
-*/
-   err = qca_set_bdaddr_rome(hdev, &btq->bdaddr);
-   if (err) {
-   set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
-   return 0;
-   }
+   set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
 
return 0;
 }
@@ -169,15 +155,6 @@ static int btqcomsmd_probe(struct platform_device *pdev)
if (IS_ERR(btq->cmd_channel))
return PTR_ERR(btq->cmd_channel);
 
-   /* The local-bd-address property is usually injected by the
-* bootloader which has access to the allocated BD address.
-*/
-   if (!of_property_read_u8_array(pdev->dev.of_node, "local-bd-address",
-  (u8 *)&btq->bdaddr, sizeof(bdaddr_t))) {
-   dev_info(&pdev->dev, "BD address %pMR retrieved from 
device-tree",
-&btq->bdaddr);
-   }
-
hdev = hci_alloc_dev();
if (!hdev)
return -ENOMEM;
-- 
2.20.1.415.g653613c723-goog

[PATCH v3 3/3] Bluetooth: hci_qca: Set HCI_QUIRK_USE_BDADDR_PROPERTY for wcn3990

[Matthias Kaehlcke]

Set quirk for wcn3990 to read BD_ADDR from a firmware node property.

Signed-off-by: Matthias Kaehlcke 
Tested-by: Balakrishna Godavarthi 
---
Changes in v3:
- none

Changes in v2:
- patch added to the series

tested with https://lore.kernel.org/patchwork/patch/1003830
("Bluetooth: hci_qca: Add helper to set device address.")
---
 drivers/bluetooth/hci_qca.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index f036c8f98ea33..0535833caa52c 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1193,6 +1193,7 @@ static int qca_setup(struct hci_uart *hu)
 * setup for every hci up.
 */
set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
+   set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
hu->hdev->shutdown = qca_power_off;
ret = qca_wcn3990_init(hu);
if (ret)
-- 
2.20.1.415.g653613c723-goog

Re: [PATCH RESEND v2 1/3] Bluetooth: Add quirk for reading BD_ADDR from fwnode property

[Matthias Kaehlcke]

Hi Marcel,

thanks for the review!

On Wed, Dec 19, 2018 at 03:22:12PM +0100, Marcel Holtmann wrote:

> > Add HCI_QUIRK_USE_BDADDR_PROPERTY to allow controllers to retrieve
> > the public Bluetooth address from the firmware node property
> > 'local-bd-address'. If quirk is set and the property does not exist
> > or is invalid the controller is marked as unconfigured.
> > 
> > Signed-off-by: Matthias Kaehlcke 
> > Reviewed-by: Balakrishna Godavarthi 
> > Tested-by: Balakrishna Godavarthi 
> > ---
> > Changes in v2:
> > - added check for return value of ->setup()
> > - only read BD_ADDR from the property if it isn't assigned yet. This
> >  is needed to support configuration from user space
> > - refactored the branch of the new quirk to get rid of 'bd_addr_set'
> > - added 'Reviewed-by: Balakrishna Godavarthi ' tag
> > ---
> > include/net/bluetooth/hci.h | 12 ++
> > net/bluetooth/hci_core.c| 45 +
> > net/bluetooth/mgmt.c|  6 +++--
> > 3 files changed, 61 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index c36dc1e20556a..fbba43e9bef5b 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -158,6 +158,18 @@ enum {
> >  */
> > HCI_QUIRK_INVALID_BDADDR,
> > 
> > +   /* When this quirk is set, the public Bluetooth address
> > +* initially reported by HCI Read BD Address command
> > +* is considered invalid. The public BD Address can be
> > +* specified in the fwnode property 'local-bd-address'.
> > +* If this property does not exist or is invalid controller
> > +* configuration is required before this device can be used.
> > +*
> > +* This quirk can be set before hci_register_dev is called or
> > +* during the hdev->setup vendor callback.
> > +*/
> > +   HCI_QUIRK_USE_BDADDR_PROPERTY,
> > +
> > /* When this quirk is set, the duplicate filtering during
> >  * scanning is based on Bluetooth devices addresses. To allow
> >  * RSSI based updates, restart scanning if needed.
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 7352fe85674be..d4149005a661e 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -30,6 +30,7 @@
> > #include 
> > #include 
> > #include 
> > +#include 
> > #include 
> > 
> > #include 
> > @@ -1355,6 +1356,36 @@ int hci_inquiry(void __user *arg)
> > return err;
> > }
> > 
> > +/**
> > + * hci_dev_get_bd_addr_from_property - Get the Bluetooth Device Address
> > + *(BD_ADDR) for a HCI device from
> > + *a firmware node property.
> > + * @hdev:  The HCI device
> > + *
> > + * Search the firmware node for 'local-bd-address'.
> > + *
> > + * All-zero BD addresses are rejected, because those could be properties
> > + * that exist in the firmware tables, but were not updated by the 
> > firmware. For
> > + * example, the DTS could define 'local-bd-address', with zero BD 
> > addresses.
> > + */
> > +static int hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
> > +{
> > +   struct fwnode_handle *fwnode = dev_fwnode(hdev->dev.parent);
> > +   bdaddr_t ba;
> > +   int ret;
> > +
> > +   ret = fwnode_property_read_u8_array(fwnode, "local-bd-address",
> > +   (u8 *)&ba, sizeof(ba));
> > +   if (ret < 0)
> > +   return ret;
> > +   if (!bacmp(&ba, BDADDR_ANY))
> > +   return -ENODATA;
> > +
> > +   hdev->public_addr = ba;
> 
> this needs to use bacpy btw.

will change

> > +
> > +   return 0;
> > +}
> 
> Make this void since the return value is actually not used right now.

ok

> > +
> > static int hci_dev_do_open(struct hci_dev *hdev)
> > {
> > int ret = 0;
> > @@ -1422,6 +1453,20 @@ static int hci_dev_do_open(struct hci_dev *hdev)
> > if (hdev->setup)
> > ret = hdev->setup(hdev);
> > 
> > +   if (ret)
> > +   goto setup_failed;
> > +
> > +   if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) {
> > +   if (!bacmp(&hdev->public_addr, BDADDR_ANY))
> > +

[PATCH RESEND v2 2/3] Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY

[Matthias Kaehlcke]

Use the HCI_QUIRK_USE_BDADDR_PROPERTY quirk to let the HCI
core handle the reading of 'local-bd-address'. With this there
is no need to set HCI_QUIRK_INVALID_BDADDR, the case of a
non-existing or invalid fwnode property is handled by the core
code.

Signed-off-by: Matthias Kaehlcke 
Reviewed-by: Balakrishna Godavarthi 
---
I couldn't actually test the changes in this driver since I
don't have a device with this controller. Could someone
from Qualcomm help with this?

Changes in v2:
- removed now unused field 'bdaddr' from struct btqcomsmd
- added 'Reviewed-by: Balakrishna Godavarthi ' tag
---
 drivers/bluetooth/btqcomsmd.c | 29 +++--
 1 file changed, 3 insertions(+), 26 deletions(-)

diff --git a/drivers/bluetooth/btqcomsmd.c b/drivers/bluetooth/btqcomsmd.c
index 7df3eed1ef5e9..b3020fab6c8e3 100644
--- a/drivers/bluetooth/btqcomsmd.c
+++ b/drivers/bluetooth/btqcomsmd.c
@@ -28,7 +28,6 @@
 struct btqcomsmd {
struct hci_dev *hdev;
 
-   bdaddr_t bdaddr;
struct rpmsg_endpoint *acl_channel;
struct rpmsg_endpoint *cmd_channel;
 };
@@ -125,23 +124,10 @@ static int btqcomsmd_setup(struct hci_dev *hdev)
return PTR_ERR(skb);
kfree_skb(skb);
 
-   /* Devices do not have persistent storage for BD address. If no
-* BD address has been retrieved during probe, mark the device
-* as having an invalid BD address.
+   /* Devices do not have persistent storage for BD address. Retrieve
+* it from the firmware node property.
 */
-   if (!bacmp(&btq->bdaddr, BDADDR_ANY)) {
-   set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
-   return 0;
-   }
-
-   /* When setting a configured BD address fails, mark the device
-* as having an invalid BD address.
-*/
-   err = qca_set_bdaddr_rome(hdev, &btq->bdaddr);
-   if (err) {
-   set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
-   return 0;
-   }
+   set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
 
return 0;
 }
@@ -169,15 +155,6 @@ static int btqcomsmd_probe(struct platform_device *pdev)
if (IS_ERR(btq->cmd_channel))
return PTR_ERR(btq->cmd_channel);
 
-   /* The local-bd-address property is usually injected by the
-* bootloader which has access to the allocated BD address.
-*/
-   if (!of_property_read_u8_array(pdev->dev.of_node, "local-bd-address",
-  (u8 *)&btq->bdaddr, sizeof(bdaddr_t))) {
-   dev_info(&pdev->dev, "BD address %pMR retrieved from 
device-tree",
-&btq->bdaddr);
-   }
-
hdev = hci_alloc_dev();
if (!hdev)
return -ENOMEM;
-- 
2.20.0.rc1.387.gf8505762e3-goog

[PATCH RESEND v2 3/3] Bluetooth: hci_qca: Set HCI_QUIRK_USE_BDADDR_PROPERTY for wcn3990

[Matthias Kaehlcke]

Set quirk for wcn3990 to read BD_ADDR from a firmware node property.

Signed-off-by: Matthias Kaehlcke 
Tested-by: Balakrishna Godavarthi 
---
Changes in v2:
- patch added to the series

tested with https://lore.kernel.org/patchwork/patch/1003830
("Bluetooth: hci_qca: Add helper to set device address.")
---
 drivers/bluetooth/hci_qca.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index f036c8f98ea33..0535833caa52c 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1193,6 +1193,7 @@ static int qca_setup(struct hci_uart *hu)
 * setup for every hci up.
 */
set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
+   set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
hu->hdev->shutdown = qca_power_off;
ret = qca_wcn3990_init(hu);
if (ret)
-- 
2.20.0.rc1.387.gf8505762e3-goog

[PATCH RESEND v2 0/3] Add quirk for reading BD_ADDR from fwnode property

[Matthias Kaehlcke]

On some systems the Bluetooth Device Address (BD_ADDR) isn't stored
on the Bluetooth chip itself. One way to configure the address is
through the device tree (patched in by the bootloader). The btqcomsmd
driver is an example, it can read the address from the DT property
'local-bd-address'.

To avoid redundant open-coded reading of 'local-bd-address' and error
handling this series adds the quirk HCI_QUIRK_USE_BDADDR_PROPERTY to
retrieve the BD address of a device from the DT and adapts the
btqcomsmd and hci_qca drivers to use this quirk.

First post: https://lore.kernel.org/patchwork/project/lkml/list/?series=370982

Matthias Kaehlcke (3):
  Bluetooth: Add quirk for reading BD_ADDR from fwnode property
  Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY
  Bluetooth: hci_qca: Set HCI_QUIRK_USE_BDADDR_PROPERTY for wcn3990

 drivers/bluetooth/btqcomsmd.c | 29 +++---
 drivers/bluetooth/hci_qca.c   |  1 +
 include/net/bluetooth/hci.h   | 12 ++
 net/bluetooth/hci_core.c  | 45 +++
 net/bluetooth/mgmt.c  |  6 +++--
 5 files changed, 65 insertions(+), 28 deletions(-)

-- 
2.20.0.rc1.387.gf8505762e3-goog

[PATCH RESEND v2 1/3] Bluetooth: Add quirk for reading BD_ADDR from fwnode property

[Matthias Kaehlcke]

Add HCI_QUIRK_USE_BDADDR_PROPERTY to allow controllers to retrieve
the public Bluetooth address from the firmware node property
'local-bd-address'. If quirk is set and the property does not exist
or is invalid the controller is marked as unconfigured.

Signed-off-by: Matthias Kaehlcke 
Reviewed-by: Balakrishna Godavarthi 
Tested-by: Balakrishna Godavarthi 
---
Changes in v2:
- added check for return value of ->setup()
- only read BD_ADDR from the property if it isn't assigned yet. This
  is needed to support configuration from user space
- refactored the branch of the new quirk to get rid of 'bd_addr_set'
- added 'Reviewed-by: Balakrishna Godavarthi ' tag
---
 include/net/bluetooth/hci.h | 12 ++
 net/bluetooth/hci_core.c| 45 +
 net/bluetooth/mgmt.c|  6 +++--
 3 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c36dc1e20556a..fbba43e9bef5b 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -158,6 +158,18 @@ enum {
 */
HCI_QUIRK_INVALID_BDADDR,
 
+   /* When this quirk is set, the public Bluetooth address
+* initially reported by HCI Read BD Address command
+* is considered invalid. The public BD Address can be
+* specified in the fwnode property 'local-bd-address'.
+* If this property does not exist or is invalid controller
+* configuration is required before this device can be used.
+*
+* This quirk can be set before hci_register_dev is called or
+* during the hdev->setup vendor callback.
+*/
+   HCI_QUIRK_USE_BDADDR_PROPERTY,
+
/* When this quirk is set, the duplicate filtering during
 * scanning is based on Bluetooth devices addresses. To allow
 * RSSI based updates, restart scanning if needed.
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7352fe85674be..d4149005a661e 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -1355,6 +1356,36 @@ int hci_inquiry(void __user *arg)
return err;
 }
 
+/**
+ * hci_dev_get_bd_addr_from_property - Get the Bluetooth Device Address
+ *(BD_ADDR) for a HCI device from
+ *a firmware node property.
+ * @hdev:  The HCI device
+ *
+ * Search the firmware node for 'local-bd-address'.
+ *
+ * All-zero BD addresses are rejected, because those could be properties
+ * that exist in the firmware tables, but were not updated by the firmware. For
+ * example, the DTS could define 'local-bd-address', with zero BD addresses.
+ */
+static int hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
+{
+   struct fwnode_handle *fwnode = dev_fwnode(hdev->dev.parent);
+   bdaddr_t ba;
+   int ret;
+
+   ret = fwnode_property_read_u8_array(fwnode, "local-bd-address",
+   (u8 *)&ba, sizeof(ba));
+   if (ret < 0)
+   return ret;
+   if (!bacmp(&ba, BDADDR_ANY))
+   return -ENODATA;
+
+   hdev->public_addr = ba;
+
+   return 0;
+}
+
 static int hci_dev_do_open(struct hci_dev *hdev)
 {
int ret = 0;
@@ -1422,6 +1453,20 @@ static int hci_dev_do_open(struct hci_dev *hdev)
if (hdev->setup)
ret = hdev->setup(hdev);
 
+   if (ret)
+   goto setup_failed;
+
+   if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) {
+   if (!bacmp(&hdev->public_addr, BDADDR_ANY))
+   hci_dev_get_bd_addr_from_property(hdev);
+
+   if (!bacmp(&hdev->public_addr, BDADDR_ANY) ||
+   !hdev->set_bdaddr ||
+   hdev->set_bdaddr(hdev, &hdev->public_addr))
+   hci_dev_set_flag(hdev, HCI_UNCONFIGURED);
+   }
+
+setup_failed:
/* The transport driver can set these quirks before
 * creating the HCI device or in its setup callback.
 *
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index ccce954f81468..fae84353d030f 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -551,7 +551,8 @@ static bool is_configured(struct hci_dev *hdev)
!hci_dev_test_flag(hdev, HCI_EXT_CONFIGURED))
return false;
 
-   if (test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks) &&
+   if ((test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks) ||
+test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) &&
!bacmp(&hdev->public_

[PATCH v2] rtlwifi: rtl8192cu: Remove variable self-assignment in rf.c

[Matthias Kaehlcke]

In _rtl92c_get_txpower_writeval_by_regulatory() the variable writeVal
is assigned to itself in an if ... else statement, apparently only to
document that the branch condition is handled and that a previously read
value should be returned unmodified. The self-assignment causes clang to
raise the following warning:

drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c:304:13:
  error: explicitly assigning value of variable of type 'u32'
(aka 'unsigned int') to itself [-Werror,-Wself-assign]
  writeVal = writeVal;

Delete the branch with the self-assignment.

Signed-off-by: Matthias Kaehlcke 
---
Changes in v2:
- Delete the 'else if' branch entirely

 drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c 
b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c
index 9cff6bc4049c..cf551785eb08 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c
@@ -299,9 +299,6 @@ static void 
_rtl92c_get_txpower_writeval_by_regulatory(struct ieee80211_hw *hw,
writeVal = 0x;
if (rtlpriv->dm.dynamic_txhighpower_lvl == TXHIGHPWRLEVEL_BT1)
writeVal = writeVal - 0x06060606;
-   else if (rtlpriv->dm.dynamic_txhighpower_lvl ==
-TXHIGHPWRLEVEL_BT2)
-   writeVal = writeVal;
*(p_outwriteval + rf) = writeVal;
}
 }
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH] rtlwifi: rtl8192cu: Remove variable self-assignment in rf.c

[Matthias Kaehlcke]

El Wed, Feb 07, 2018 at 02:35:59PM -0600 Larry Finger ha dit:

> On 02/07/2018 02:26 PM, Matthias Kaehlcke wrote:
> > In _rtl92c_get_txpower_writeval_by_regulatory() the variable writeVal
> > is assigned to itself in an if ... else statement, apparently only to
> > document that the branch condition is handled and that a previously read
> > value should be returned unmodified. The self-assignment causes clang to
> > raise the following warning:
> > 
> > drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c:304:13:
> >error: explicitly assigning value of variable of type 'u32'
> >  (aka 'unsigned int') to itself [-Werror,-Wself-assign]
> >writeVal = writeVal;
> > 
> > Replace the self-assignment with a semicolon, which still serves to
> > document the 'handling' of the branch condition.
> > 
> > Signed-off-by: Matthias Kaehlcke 
> > ---
> >   drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c 
> > b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c
> > index 9cff6bc4049c..4db92496c122 100644
> > --- a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c
> > +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c
> > @@ -301,7 +301,7 @@ static void 
> > _rtl92c_get_txpower_writeval_by_regulatory(struct ieee80211_hw *hw,
> > writeVal = writeVal - 0x06060606;
> > else if (rtlpriv->dm.dynamic_txhighpower_lvl ==
> >  TXHIGHPWRLEVEL_BT2)
> > -   writeVal = writeVal;
> > +   ;
> > *(p_outwriteval + rf) = writeVal;
> > }
> >   }
> > 
> 
> As the branch condition does nothing, why not remove it and save the
> compiler's optimizer a bit of work? The code looks strange, but it matches
> the rest of Realtek's USB drivers.

Sure, I am happy to change it to whatever the authors/maintainers prefer.

I'll wait a bit before respinning for if others feel strongly about
keeping the branch.

[PATCH] rtlwifi: rtl8192cu: Remove variable self-assignment in rf.c

[Matthias Kaehlcke]

In _rtl92c_get_txpower_writeval_by_regulatory() the variable writeVal
is assigned to itself in an if ... else statement, apparently only to
document that the branch condition is handled and that a previously read
value should be returned unmodified. The self-assignment causes clang to
raise the following warning:

drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c:304:13:
  error: explicitly assigning value of variable of type 'u32'
(aka 'unsigned int') to itself [-Werror,-Wself-assign]
  writeVal = writeVal;

Replace the self-assignment with a semicolon, which still serves to
document the 'handling' of the branch condition.

Signed-off-by: Matthias Kaehlcke 
---
 drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c 
b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c
index 9cff6bc4049c..4db92496c122 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/rf.c
@@ -301,7 +301,7 @@ static void 
_rtl92c_get_txpower_writeval_by_regulatory(struct ieee80211_hw *hw,
writeVal = writeVal - 0x06060606;
else if (rtlpriv->dm.dynamic_txhighpower_lvl ==
 TXHIGHPWRLEVEL_BT2)
-   writeVal = writeVal;
+   ;
*(p_outwriteval + rf) = writeVal;
}
 }
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH] wimax/i2400m: Remove VLAIS

[Matthias Kaehlcke]

El Mon, Oct 09, 2017 at 12:41:53PM -0700 Matthias Kaehlcke ha dit:

> From: Behan Webster 
> 
> Convert Variable Length Array in Struct (VLAIS) to valid C by converting
> local struct definition to use a flexible array. The structure is only
> used to define a cast of a buffer so the size of the struct is not used
> to allocate storage.
> 
> Signed-off-by: Behan Webster 
> Signed-off-by: Mark Charebois 
> Suggested-by: Arnd Bergmann 
> Signed-off-by: Matthias Kaehlcke 
> ---
>  drivers/net/wimax/i2400m/fw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wimax/i2400m/fw.c b/drivers/net/wimax/i2400m/fw.c
> index c9c711dcd0e6..a89b5685e68b 100644
> --- a/drivers/net/wimax/i2400m/fw.c
> +++ b/drivers/net/wimax/i2400m/fw.c
> @@ -652,7 +652,7 @@ static int i2400m_download_chunk(struct i2400m *i2400m, 
> const void *chunk,
>   struct device *dev = i2400m_dev(i2400m);
>   struct {
>   struct i2400m_bootrom_header cmd;
> - u8 cmd_payload[chunk_len];
> + u8 cmd_payload[];
>   } __packed *buf;
>   struct i2400m_bootrom_header ack;

ping

any comments on this?

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Matthias Kaehlcke]

El Tue, Oct 24, 2017 at 10:03:56AM -0700 Jakub Kicinski ha dit:

> On Tue, 24 Oct 2017 09:56:03 -0700, Matthias Kaehlcke wrote:
> > > @@ -561,6 +568,6 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned 
> > > int speed)
> > >   */
> > >  int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes)
> > >  {
> > > - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
> > > + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
> > > lanes, NSP_ETH_CTRL_SET_LANES);
> > >  }  
> > 
> > Do you plan to send this as an official patch?
> 
> Sorry...  Dirk is prepping a larger series for this area of code and it
> got stuck there a bit :S

No prob, just wanted to make sure it didn't slip under the radar.

>  He says it's coming any day now...

Great, thanks!

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Matthias Kaehlcke]

Hi Jakub,

El Wed, Oct 04, 2017 at 11:17:24AM -0700 Jakub Kicinski ha dit:

> On Wed, 4 Oct 2017 10:42:42 -0700, Manoj Gupta wrote:
> > Hi Jakub,
> > 
> > I had discussed about supporting this code with some clang developers.
> > However, the consensus was this code relies on a specific GCC optimizer
> > behavior and Clang does not share the same behavior by design.
> 
> Hm.  I find surprising that constant propagation to inlined functions
> is considered something GCC-specific rather than obvious/basic.
> 
> > Note that even GCC can't compile this code at -O0.
> 
> Yes, that makes me feel uncomfortable...  Could you try this?
> 
> ---8<---
> 
> diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c 
> b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> index f6f7c085f8e0..47251396fcae 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> @@ -469,10 +469,10 @@ int nfp_eth_set_configured(struct nfp_cpp *cpp, 
> unsigned int idx, bool configed)
>   return nfp_eth_config_commit_end(nsp);
>  }
>  
> -/* Force inline, FIELD_* macroes require masks to be compilation-time known 
> */
> -static __always_inline int
> +static int
>  nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
> -const u64 mask, unsigned int val, const u64 ctrl_bit)
> +const u64 mask, const unsigned int shift,
> +unsigned int val, const u64 ctrl_bit)
>  {
>   union eth_table_entry *entries = nfp_nsp_config_entries(nsp);
>   unsigned int idx = nfp_nsp_config_idx(nsp);
> @@ -489,11 +489,11 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned 
> int raw_idx,
>  
>   /* Check if we are already in requested state */
>   reg = le64_to_cpu(entries[idx].raw[raw_idx]);
> - if (val == FIELD_GET(mask, reg))
> + if (val == (reg & mask) >> shift)
>   return 0;
>  
>   reg &= ~mask;
> - reg |= FIELD_PREP(mask, val);
> + reg |= (val << shift) & mask;
>   entries[idx].raw[raw_idx] = cpu_to_le64(reg);
>  
>   entries[idx].control |= cpu_to_le64(ctrl_bit);
> @@ -503,6 +503,13 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int 
> raw_idx,
>   return 0;
>  }
>  
> +#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit)\
> + ({  \
> + __BF_FIELD_CHECK(mask, 0ULL, val, "NFP_ETH_SET_BIT_CONFIG: "); \
> + nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask), \
> +val, ctrl_bit);  \
> + })
> +
>  /**
>   * __nfp_eth_set_aneg() - set PHY autonegotiation control bit
>   * @nsp: NFP NSP handle returned from nfp_eth_config_start()
> @@ -515,7 +522,7 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int 
> raw_idx,
>   */
>  int __nfp_eth_set_aneg(struct nfp_nsp *nsp, enum nfp_eth_aneg mode)
>  {
> - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE,
> + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE,
> NSP_ETH_STATE_ANEG, mode,
> NSP_ETH_CTRL_SET_ANEG);
>  }
> @@ -544,7 +551,7 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int 
> speed)
>   return -EINVAL;
>   }
>  
> - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE,
> + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE,
> NSP_ETH_STATE_RATE, rate,
> NSP_ETH_CTRL_SET_RATE);
>  }
> @@ -561,6 +568,6 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int 
> speed)
>   */
>  int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes)
>  {
> - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
> + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
> lanes, NSP_ETH_CTRL_SET_LANES);
>  }

Do you plan to send this as an official patch?

Thanks

Matthias

[PATCH] wimax/i2400m: Remove VLAIS

[Matthias Kaehlcke]

From: Behan Webster 

Convert Variable Length Array in Struct (VLAIS) to valid C by converting
local struct definition to use a flexible array. The structure is only
used to define a cast of a buffer so the size of the struct is not used
to allocate storage.

Signed-off-by: Behan Webster 
Signed-off-by: Mark Charebois 
Suggested-by: Arnd Bergmann 
Signed-off-by: Matthias Kaehlcke 
---
 drivers/net/wimax/i2400m/fw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wimax/i2400m/fw.c b/drivers/net/wimax/i2400m/fw.c
index c9c711dcd0e6..a89b5685e68b 100644
--- a/drivers/net/wimax/i2400m/fw.c
+++ b/drivers/net/wimax/i2400m/fw.c
@@ -652,7 +652,7 @@ static int i2400m_download_chunk(struct i2400m *i2400m, 
const void *chunk,
struct device *dev = i2400m_dev(i2400m);
struct {
struct i2400m_bootrom_header cmd;
-   u8 cmd_payload[chunk_len];
+   u8 cmd_payload[];
} __packed *buf;
struct i2400m_bootrom_header ack;
 
-- 
2.14.2.920.gcf0c67979c-goog

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Matthias Kaehlcke]

El Wed, Oct 04, 2017 at 07:13:26PM -0700 Manoj Gupta ha dit:

> On Wed, Oct 4, 2017 at 7:06 PM, Jakub Kicinski
>  wrote:
> > On Wed, 4 Oct 2017 18:50:04 -0700, Manoj Gupta wrote:
> >> On Wed, Oct 4, 2017 at 5:56 PM, Jakub Kicinski wrote:
> >> > On Wed, 4 Oct 2017 17:38:22 -0700, Manoj Gupta wrote:
> >> >> On Wed, Oct 4, 2017 at 4:25 PM, Jakub Kicinski wrote:
> >> >> > On Wed, 4 Oct 2017 16:16:49 -0700, Matthias Kaehlcke wrote:
> >> >> >> > > Thanks for the suggestion. This seems a viable alternative if 
> >> >> >> > > David
> >> >> >> > > and the NFP owners can live without the extra checking provided 
> >> >> >> > > by
> >> >> >> > > __BF_FIELD_CHECK.
> >> >> >> >
> >> >> >> > The reason the __BF_FIELD_CHECK refuses to compile non-constant 
> >> >> >> > masks
> >> >> >> > is that it will require runtime ffs on the mask, which is 
> >> >> >> > potentially
> >> >> >> > costly.  I would also feel quite stupid adding those macros to the 
> >> >> >> > nfp
> >> >> >> > driver, given that I specifically created the bitfield.h header to 
> >> >> >> > not
> >> >> >> > have to reimplement these in every driver I write/maintain.
> >> >> >>
> >> >> >> That make sense, thanks for providing more context.
> >> >> >>
> >> >> >> > Can you please test the patch I provided in the other reply?
> >> >> >>
> >> >> >> With this patch there are no errors when building the kernel with
> >> >> >> clang.
> >> >> >
> >> >> > Cool, thanks for checking!  I will run it through full tests and queue
> >> >> > for upstreaming :)
> >> >>
> >> >> Just to let you know, using __BF_FIELD_CHECK macro will not Link with
> >> >> -O0 (GCC or Clang)  since references to __compiletime_assert_xxx will
> >> >> not be cleaned up.
> >> >
> >> > Do you mean the current nfp_eth_set_bit_config() will not work with -O0
> >> > on either complier, or any use of __BF_FIELD_CHECK() will not compile
> >> > with -O0?
> >>
> >> Any use of __BF_FIELD_CHECK. The code will compile but not link since
> >> calls to compiletime_assert_xxx (added by compiletime_assert
> >> macro) will not be removed in -O0.
> >
> > Why would that be, it's just a macro?  Does it by extension mean any
> > use of BUILD_BUG_ON_MSG() will not compile with -O0?
> 
> You have to look at the the code added once the macro is expanded :).
> Please look at implementation of compiletime_assert at
> http://elixir.free-electrons.com/linux/v4.12.14/source/include/linux/compiler.h#L507
> It creates a call to __compiler_assert_xxx inside a loop which is not
> cleaned up in -O0.

I just saw that v4.14 will have a fix for that:

commit c03567a8e8d5cf2aaca40e605c48f319dc2ead57
Author: Joe Stringer 
Date:   Thu Aug 31 16:15:33 2017 -0700

include/linux/compiler.h: don't perform compiletime_assert with -O0


Obviously this means that the checks aren't performed, however that
shouldn't be an issue since AFAIK the kernel doesn't officially
support -O0 builds in the first place.

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Matthias Kaehlcke]

Hi Jakub,

El Wed, Oct 04, 2017 at 03:22:03PM -0700 Jakub Kicinski ha dit:

> On Wed, 4 Oct 2017 11:49:57 -0700, Matthias Kaehlcke wrote:
> > Hi Joe,
> > 
> > El Wed, Oct 04, 2017 at 11:07:19AM -0700 Joe Perches ha dit:
> > 
> > > On Tue, 2017-10-03 at 13:05 -0700, Matthias Kaehlcke wrote:  
> > > > nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to
> > > > identify the 'mask' parameter as known to be constant at compile time,
> > > > which is required to use the FIELD_GET() macro.
> > > > 
> > > > The forced inlining does the trick for gcc, but for kernel builds with
> > > > clang it results in undefined symbols:  
> > > 
> > > Can't you use local different FIELD_PREP/FIELD_GET macros
> > > with a different name without the BUILD_BUG tests?
> > > 
> > > i.e.:
> > > 
> > > #define NFP_FIELD_PREP(_mask, _val)   \
> > > ({\
> > >   ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask);   \
> > > })
> > > 
> > > #define NFP_FIELD_GET(_mask, _reg)\
> > > ({\
> > >   (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
> > > })
> > > 
> > > Then the __always_inline can be removed from
> > > nfp_eth_set_bit_config too.  
> > 
> > Thanks for the suggestion. This seems a viable alternative if David
> > and the NFP owners can live without the extra checking provided by
> > __BF_FIELD_CHECK.
> 
> The reason the __BF_FIELD_CHECK refuses to compile non-constant masks
> is that it will require runtime ffs on the mask, which is potentially
> costly.  I would also feel quite stupid adding those macros to the nfp
> driver, given that I specifically created the bitfield.h header to not
> have to reimplement these in every driver I write/maintain.

That make sense, thanks for providing more context.

> Can you please test the patch I provided in the other reply?

With this patch there are no errors when building the kernel with
clang.

Thanks!

Matthias

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Matthias Kaehlcke]

Hi Joe,

El Wed, Oct 04, 2017 at 11:07:19AM -0700 Joe Perches ha dit:

> On Tue, 2017-10-03 at 13:05 -0700, Matthias Kaehlcke wrote:
> > nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to
> > identify the 'mask' parameter as known to be constant at compile time,
> > which is required to use the FIELD_GET() macro.
> > 
> > The forced inlining does the trick for gcc, but for kernel builds with
> > clang it results in undefined symbols:
> 
> Can't you use local different FIELD_PREP/FIELD_GET macros
> with a different name without the BUILD_BUG tests?
> 
> i.e.:
> 
> #define NFP_FIELD_PREP(_mask, _val)   \
> ({\
>   ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask);   \
> })
> 
> #define NFP_FIELD_GET(_mask, _reg)\
> ({\
>   (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
> })
> 
> Then the __always_inline can be removed from
> nfp_eth_set_bit_config too.

Thanks for the suggestion. This seems a viable alternative if David
and the NFP owners can live without the extra checking provided by
__BF_FIELD_CHECK.

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Matthias Kaehlcke]

El Wed, Oct 04, 2017 at 11:17:24AM -0700 Jakub Kicinski ha dit:

> On Wed, 4 Oct 2017 10:42:42 -0700, Manoj Gupta wrote:
> > Hi Jakub,
> > 
> > I had discussed about supporting this code with some clang developers.
> > However, the consensus was this code relies on a specific GCC optimizer
> > behavior and Clang does not share the same behavior by design.
> 
> Hm.  I find surprising that constant propagation to inlined functions
> is considered something GCC-specific rather than obvious/basic.
> 
> > Note that even GCC can't compile this code at -O0.
> 
> Yes, that makes me feel uncomfortable...  Could you try this?

The kernel as a whole does not build with -O0, so I couldn't try
that. I know Manoj (who isn't a kernel dev) isolated the
compiletime_assert macros and encountered the same 'undefined
reference' errors with gcc -O0 that we are seeing with clang.
This is due to:

 "if you use it in an inlined function and pass an argument of the
 function as the argument to the built-in, GCC never returns 1 when
 you call the inline function with a string constant or compound
 literal (see Compound Literals) and does not return 1 when you pass a
 constant numeric value to the inline function unless you specify the
 -O option."

https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gcc/Other-Builtins.html

> diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c 
> b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> index f6f7c085f8e0..47251396fcae 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> @@ -469,10 +469,10 @@ int nfp_eth_set_configured(struct nfp_cpp *cpp, 
> unsigned int idx, bool configed)
>   return nfp_eth_config_commit_end(nsp);
>  }
>  
> -/* Force inline, FIELD_* macroes require masks to be compilation-time known 
> */
> -static __always_inline int
> +static int
>  nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
> -const u64 mask, unsigned int val, const u64 ctrl_bit)
> +const u64 mask, const unsigned int shift,
> +unsigned int val, const u64 ctrl_bit)
>  {
>   union eth_table_entry *entries = nfp_nsp_config_entries(nsp);
>   unsigned int idx = nfp_nsp_config_idx(nsp);
> @@ -489,11 +489,11 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned 
> int raw_idx,
>  
>   /* Check if we are already in requested state */
>   reg = le64_to_cpu(entries[idx].raw[raw_idx]);
> - if (val == FIELD_GET(mask, reg))
> + if (val == (reg & mask) >> shift)
>   return 0;
>  
>   reg &= ~mask;
> - reg |= FIELD_PREP(mask, val);
> + reg |= (val << shift) & mask;
>   entries[idx].raw[raw_idx] = cpu_to_le64(reg);
>  
>   entries[idx].control |= cpu_to_le64(ctrl_bit);
> @@ -503,6 +503,13 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int 
> raw_idx,
>   return 0;
>  }
>  
> +#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit)\
> + ({  \
> + __BF_FIELD_CHECK(mask, 0ULL, val, "NFP_ETH_SET_BIT_CONFIG: "); \
> + nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask), \
> +val, ctrl_bit);  \
> + })
> +
>  /**
>   * __nfp_eth_set_aneg() - set PHY autonegotiation control bit
>   * @nsp: NFP NSP handle returned from nfp_eth_config_start()
> @@ -515,7 +522,7 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int 
> raw_idx,
>   */
>  int __nfp_eth_set_aneg(struct nfp_nsp *nsp, enum nfp_eth_aneg mode)
>  {
> - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE,
> + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE,
> NSP_ETH_STATE_ANEG, mode,
> NSP_ETH_CTRL_SET_ANEG);
>  }
> @@ -544,7 +551,7 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int 
> speed)
>   return -EINVAL;
>   }
>  
> - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE,
> + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE,
> NSP_ETH_STATE_RATE, rate,
> NSP_ETH_CTRL_SET_RATE);
>  }
> @@ -561,6 +568,6 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int 
> speed)
>   */
>  int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes)
>  {
> - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
> + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
> lanes, NSP_ETH_CTRL_SET_LANES);
>  }

Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Matthias Kaehlcke]

El Tue, Oct 03, 2017 at 02:50:00PM -0700 Jakub Kicinski ha dit:

> On Tue,  3 Oct 2017 13:05:46 -0700, Matthias Kaehlcke wrote:
> > nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to
> > identify the 'mask' parameter as known to be constant at compile time,
> > which is required to use the FIELD_GET() macro.
> > 
> > The forced inlining does the trick for gcc, but for kernel builds with
> > clang it results in undefined symbols:
> > 
> > drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.o: In function
> >   `__nfp_eth_set_aneg':
> > drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c:(.text+0x787):
> >   undefined reference to `__compiletime_assert_492'
> > drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c:(.text+0x7b1):
> >   undefined reference to `__compiletime_assert_496'
> > 
> > These __compiletime_assert_xyx() calls would have been optimized away if
> > the compiler had seen 'mask' as a constant.
> > 
> > Convert nfp_eth_set_bit_config() into a macro, which allows both gcc and
> > clang to identify 'mask' as a compile time constant.
> > 
> > Signed-off-by: Matthias Kaehlcke 
> 
> :(
> 
> Is there no chance of fixing the constant propagation in the compiler?

LLVM developers are reluctant and would like us kernel folks to
evaluate possible alternatives for the affected code first:
https://bugs.llvm.org/show_bug.cgi?id=4898

Given that this doesn't seem to be a widespread issue in the kernel
personally I would consider the conversion to a macro in this case an
acceptable solution, though it is definitely ugly. However I'm not the
owner of the driver or the subsystem, so my opinion doesn't really
carry much weight here ;-)

Any ideas about other, less ugly alternatives?

Matthias

[PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

[Matthias Kaehlcke]

nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to
identify the 'mask' parameter as known to be constant at compile time,
which is required to use the FIELD_GET() macro.

The forced inlining does the trick for gcc, but for kernel builds with
clang it results in undefined symbols:

drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.o: In function
  `__nfp_eth_set_aneg':
drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c:(.text+0x787):
  undefined reference to `__compiletime_assert_492'
drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c:(.text+0x7b1):
  undefined reference to `__compiletime_assert_496'

These __compiletime_assert_xyx() calls would have been optimized away if
the compiler had seen 'mask' as a constant.

Convert nfp_eth_set_bit_config() into a macro, which allows both gcc and
clang to identify 'mask' as a compile time constant.

Signed-off-by: Matthias Kaehlcke 
---
I am aware that a lengthy macro is not a pretty solution, I'm open for
better suggestions.

Note: The patch has been build-tested only since I don't have any NFP
hardware.

 .../ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c   | 67 +++---
 1 file changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c 
b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
index f6f7c085f8e0..e9c635867918 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
@@ -469,39 +469,40 @@ int nfp_eth_set_configured(struct nfp_cpp *cpp, unsigned 
int idx, bool configed)
return nfp_eth_config_commit_end(nsp);
 }
 
-/* Force inline, FIELD_* macroes require masks to be compilation-time known */
-static __always_inline int
-nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
-  const u64 mask, unsigned int val, const u64 ctrl_bit)
-{
-   union eth_table_entry *entries = nfp_nsp_config_entries(nsp);
-   unsigned int idx = nfp_nsp_config_idx(nsp);
-   u64 reg;
-
-   /* Note: set features were added in ABI 0.14 but the error
-*   codes were initially not populated correctly.
-*/
-   if (nfp_nsp_get_abi_ver_minor(nsp) < 17) {
-   nfp_err(nfp_nsp_cpp(nsp),
-   "set operations not supported, please update flash\n");
-   return -EOPNOTSUPP;
-   }
-
-   /* Check if we are already in requested state */
-   reg = le64_to_cpu(entries[idx].raw[raw_idx]);
-   if (val == FIELD_GET(mask, reg))
-   return 0;
-
-   reg &= ~mask;
-   reg |= FIELD_PREP(mask, val);
-   entries[idx].raw[raw_idx] = cpu_to_le64(reg);
-
-   entries[idx].control |= cpu_to_le64(ctrl_bit);
-
-   nfp_nsp_config_set_modified(nsp, true);
-
-   return 0;
-}
+#define nfp_eth_set_bit_config(nsp, raw_idx, mask, val, ctrl_bit)  \
+({ \
+   union eth_table_entry *entries = nfp_nsp_config_entries(nsp);   \
+   unsigned int idx = nfp_nsp_config_idx(nsp); \
+   u64 reg;\
+   int rc; \
+   \
+   /* Note: set features were added in ABI 0.14 but the error */   \
+   /*   codes were initially not populated correctly. */   \
+   if (nfp_nsp_get_abi_ver_minor(nsp) < 17) {  \
+   nfp_err(nfp_nsp_cpp(nsp),   \
+   "set operations not supported, please update flash\n"); 
\
+   rc = -EOPNOTSUPP;   \
+   goto out;   \
+   }   \
+   \
+   rc = 0; \
+   \
+   /* Check if we are already in requested state */\
+   reg = le64_to_cpu(entries[idx].raw[raw_idx]);   \
+   if (val == FIELD_GET(mask, reg))\
+   goto out;   \
+   \
+   reg &= ~mask;   \
+   reg |= FIELD_PREP(mask, val);   \
+   entries[idx].raw[raw_idx] = cpu_to_le64(reg);   \
+

[PATCH] netpoll: Fix device name check in netpoll_setup()

[Matthias Kaehlcke]

Apparently netpoll_setup() assumes that netpoll.dev_name is a pointer
when checking if the device name is set:

if (np->dev_name) {
  ...

However the field is a character array, therefore the condition always
yields true. Check instead whether the first byte of the array has a
non-zero value.

Signed-off-by: Matthias Kaehlcke 
---
 net/core/netpoll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 8357f164c660..912731bed7b7 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -666,7 +666,7 @@ int netpoll_setup(struct netpoll *np)
int err;
 
rtnl_lock();
-   if (np->dev_name) {
+   if (np->dev_name[0]) {
struct net *net = current->nsproxy->net_ns;
ndev = __dev_get_by_name(net, np->dev_name);
}
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH] net: jme: Remove unused functions

[Matthias Kaehlcke]

The functions jme_restart_tx_engine(), jme_pause_rx() and
jme_resume_rx() are not used. Removing them fixes the following warnings
when building with clang:

drivers/net/ethernet/jme.c:694:1: error: unused function
'jme_restart_tx_engine' [-Werror,-Wunused-function]

drivers/net/ethernet/jme.c:2393:20: error: unused function
'jme_pause_rx' [-Werror,-Wunused-function]

drivers/net/ethernet/jme.c:2406:20: error: unused function
'jme_resume_rx' [-Werror,-Wunused-function]

Signed-off-by: Matthias Kaehlcke 
---
 drivers/net/ethernet/jme.c | 42 --
 1 file changed, 42 deletions(-)

diff --git a/drivers/net/ethernet/jme.c b/drivers/net/ethernet/jme.c
index f580b49e6b67..0e5083a48937 100644
--- a/drivers/net/ethernet/jme.c
+++ b/drivers/net/ethernet/jme.c
@@ -691,17 +691,6 @@ jme_enable_tx_engine(struct jme_adapter *jme)
 }
 
 static inline void
-jme_restart_tx_engine(struct jme_adapter *jme)
-{
-   /*
-* Restart TX Engine
-*/
-   jwrite32(jme, JME_TXCS, jme->reg_txcs |
-   TXCS_SELECT_QUEUE0 |
-   TXCS_ENABLE);
-}
-
-static inline void
 jme_disable_tx_engine(struct jme_adapter *jme)
 {
int i;
@@ -2382,37 +2371,6 @@ jme_tx_timeout(struct net_device *netdev)
jme_reset_link(jme);
 }
 
-static inline void jme_pause_rx(struct jme_adapter *jme)
-{
-   atomic_dec(&jme->link_changing);
-
-   jme_set_rx_pcc(jme, PCC_OFF);
-   if (test_bit(JME_FLAG_POLL, &jme->flags)) {
-   JME_NAPI_DISABLE(jme);
-   } else {
-   tasklet_disable(&jme->rxclean_task);
-   tasklet_disable(&jme->rxempty_task);
-   }
-}
-
-static inline void jme_resume_rx(struct jme_adapter *jme)
-{
-   struct dynpcc_info *dpi = &(jme->dpi);
-
-   if (test_bit(JME_FLAG_POLL, &jme->flags)) {
-   JME_NAPI_ENABLE(jme);
-   } else {
-   tasklet_enable(&jme->rxclean_task);
-   tasklet_enable(&jme->rxempty_task);
-   }
-   dpi->cur= PCC_P1;
-   dpi->attempt= PCC_P1;
-   dpi->cnt= 0;
-   jme_set_rx_pcc(jme, PCC_P1);
-
-   atomic_inc(&jme->link_changing);
-}
-
 static void
 jme_get_drvinfo(struct net_device *netdev,
 struct ethtool_drvinfo *info)
-- 
2.13.0.219.gdb65acc882-goog

[PATCH] net1080: Remove unused function nc_dump_ttl()

[Matthias Kaehlcke]

The function is not used, removing it fixes the following warning when
building with clang:

drivers/net/usb/net1080.c:271:20: error: unused function
'nc_dump_ttl' [-Werror,-Wunused-function]

Also remove the definition of TTL_THIS, which is only used in
nc_dump_ttl()

Signed-off-by: Matthias Kaehlcke 
---
 drivers/net/usb/net1080.c | 9 -
 1 file changed, 9 deletions(-)

diff --git a/drivers/net/usb/net1080.c b/drivers/net/usb/net1080.c
index 4cbdb1307f3e..3202c19df83d 100644
--- a/drivers/net/usb/net1080.c
+++ b/drivers/net/usb/net1080.c
@@ -264,17 +264,9 @@ static inline void nc_dump_status(struct usbnet *dev, u16 
status)
  * TTL register
  */
 
-#defineTTL_THIS(ttl)   (0x00ff & ttl)
 #defineTTL_OTHER(ttl)  (0x00ff & (ttl >> 8))
 #define MK_TTL(this,other) ((u16)(((other)<<8)|(0x00ff&(this
 
-static inline void nc_dump_ttl(struct usbnet *dev, u16 ttl)
-{
-   netif_dbg(dev, link, dev->net, "net1080 %s-%s ttl 0x%x this = %d, other 
= %d\n",
- dev->udev->bus->bus_name, dev->udev->devpath,
- ttl, TTL_THIS(ttl), TTL_OTHER(ttl));
-}
-
 /*-*/
 
 static int net1080_reset(struct usbnet *dev)
@@ -308,7 +300,6 @@ static int net1080_reset(struct usbnet *dev)
goto done;
}
ttl = vp;
-   // nc_dump_ttl(dev, ttl);
 
nc_register_write(dev, REG_TTL,
MK_TTL(NC_READ_TTL_MS, TTL_OTHER(ttl)) );
-- 
2.13.0.303.g4ebf302169-goog

[PATCH] r8152: Remove unused function usb_ocp_read()

[Matthias Kaehlcke]

The function is not used, removing it fixes the following warning when
building with clang:

drivers/net/usb/r8152.c:825:5: error: unused function 'usb_ocp_read'
[-Werror,-Wunused-function]

Signed-off-by: Matthias Kaehlcke 
---
 drivers/net/usb/r8152.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ddc62cb69be8..e902df9595b9 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -841,12 +841,6 @@ int pla_ocp_write(struct r8152 *tp, u16 index, u16 byteen, 
u16 size, void *data)
 }
 
 static inline
-int usb_ocp_read(struct r8152 *tp, u16 index, u16 size, void *data)
-{
-   return generic_ocp_read(tp, index, size, data, MCU_TYPE_USB);
-}
-
-static inline
 int usb_ocp_write(struct r8152 *tp, u16 index, u16 byteen, u16 size, void 
*data)
 {
return generic_ocp_write(tp, index, byteen, size, data, MCU_TYPE_USB);
-- 
2.13.0.303.g4ebf302169-goog

Re: [PATCH] net1080: Mark nc_dump_ttl() as __maybe_unused

[Matthias Kaehlcke]

Hi David,

El Thu, May 18, 2017 at 10:48:08AM -0400 David Miller ha dit:

> From: Matthias Kaehlcke 
> Date: Wed, 17 May 2017 15:17:08 -0700
> 
> > The function is not used, but it looks useful for debugging. Adding the
> > attribute fixes the following clang warning:
> > 
> > drivers/net/usb/net1080.c:271:20: error: unused function
> > 'nc_dump_ttl' [-Werror,-Wunused-function]
> > 
> > Signed-off-by: Matthias Kaehlcke 
> 
> For this and the r8152 patch, I definitely prefer that the function is
> removed.
> 
> If someone needs them, they can pull it out of the GIT history.

Thanks for you comments, I'll send out updated patches soon.

[PATCH] r8152: Mark usb_ocp_read() as __maybe_unused

[Matthias Kaehlcke]

The function is not used, but is probably kept around for debugging and
symmetry with usb_ocp_write(). Adding the attribute fixes the following
clang warning:

drivers/net/usb/r8152.c:825:5: error: unused function 'usb_ocp_read'
[-Werror,-Wunused-function]

Signed-off-by: Matthias Kaehlcke 
---
 drivers/net/usb/r8152.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ddc62cb69be8..12776230ab96 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -840,7 +840,7 @@ int pla_ocp_write(struct r8152 *tp, u16 index, u16 byteen, 
u16 size, void *data)
return generic_ocp_write(tp, index, byteen, size, data, MCU_TYPE_PLA);
 }
 
-static inline
+static inline __maybe_unused
 int usb_ocp_read(struct r8152 *tp, u16 index, u16 size, void *data)
 {
return generic_ocp_read(tp, index, size, data, MCU_TYPE_USB);
-- 
2.13.0.303.g4ebf302169-goog

[PATCH] net1080: Mark nc_dump_ttl() as __maybe_unused

[Matthias Kaehlcke]

The function is not used, but it looks useful for debugging. Adding the
attribute fixes the following clang warning:

drivers/net/usb/net1080.c:271:20: error: unused function
'nc_dump_ttl' [-Werror,-Wunused-function]

Signed-off-by: Matthias Kaehlcke 
---
 drivers/net/usb/net1080.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/net1080.c b/drivers/net/usb/net1080.c
index 4cbdb1307f3e..7ade2119f462 100644
--- a/drivers/net/usb/net1080.c
+++ b/drivers/net/usb/net1080.c
@@ -268,7 +268,7 @@ static inline void nc_dump_status(struct usbnet *dev, u16 
status)
 #defineTTL_OTHER(ttl)  (0x00ff & (ttl >> 8))
 #define MK_TTL(this,other) ((u16)(((other)<<8)|(0x00ff&(this
 
-static inline void nc_dump_ttl(struct usbnet *dev, u16 ttl)
+static inline void __maybe_unused nc_dump_ttl(struct usbnet *dev, u16 ttl)
 {
netif_dbg(dev, link, dev->net, "net1080 %s-%s ttl 0x%x this = %d, other 
= %d\n",
  dev->udev->bus->bus_name, dev->udev->devpath,
-- 
2.13.0.303.g4ebf302169-goog

Re: [PATCH] netfilter: ctnetlink: Make some parameters integer to avoid enum mismatch

[Matthias Kaehlcke]

El Wed, Apr 19, 2017 at 11:39:20AM -0700 Matthias Kaehlcke ha dit:

> Not all parameters passed to ctnetlink_parse_tuple() and
> ctnetlink_exp_dump_tuple() match the enum type in the signatures of these
> functions. Since this is intended change the argument type of to be an int
> value.
> 
> Signed-off-by: Matthias Kaehlcke 
> ---

Ping, any comments on this patch?

Thanks

Matthias

Re: [PATCH] netfilter: ctnetlink: Make some parameters integer to avoid enum mismatch

[Matthias Kaehlcke]

El Wed, Apr 19, 2017 at 12:41:10PM -0700 Joe Perches ha dit:

> On Wed, 2017-04-19 at 11:39 -0700, Matthias Kaehlcke wrote:
> > Not all parameters passed to ctnetlink_parse_tuple() and
> > ctnetlink_exp_dump_tuple() match the enum type in the signatures of these
> > functions.
> 
> Maybe that should be changed/fixed.

Please see the previous discussion at
http://www.spinics.net/lists/netfilter-devel/msg47540.html

> > Since this is intended change the argument type of to be an int
> > value.
> 
> u32 is not int, it's unsigned int

I would argue that an unsigned int is an int(eger) and considered it
an unnecessary detail for the commit message to be explicit. I can
change it if others deem it incorrect.

Cheers

Matthias

> > Signed-off-by: Matthias Kaehlcke 
> > ---
> >  net/netfilter/nf_conntrack_netlink.c | 7 +++
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/netfilter/nf_conntrack_netlink.c 
> > b/net/netfilter/nf_conntrack_netlink.c
> > index dc7dfd68fafe..775eb5d9165b 100644
> > --- a/net/netfilter/nf_conntrack_netlink.c
> > +++ b/net/netfilter/nf_conntrack_netlink.c
> > @@ -1006,9 +1006,8 @@ static const struct nla_policy 
> > tuple_nla_policy[CTA_TUPLE_MAX+1] = {
> >  
> >  static int
> >  ctnetlink_parse_tuple(const struct nlattr * const cda[],
> > - struct nf_conntrack_tuple *tuple,
> > - enum ctattr_type type, u_int8_t l3num,
> > - struct nf_conntrack_zone *zone)
> > + struct nf_conntrack_tuple *tuple, u32 type,
> > + u_int8_t l3num, struct nf_conntrack_zone *zone)
> >  {
> > struct nlattr *tb[CTA_TUPLE_MAX+1];
> > int err;
> > @@ -2443,7 +2442,7 @@ static struct nfnl_ct_hook ctnetlink_glue_hook = {
> >  
> >  static int ctnetlink_exp_dump_tuple(struct sk_buff *skb,
> > const struct nf_conntrack_tuple *tuple,
> > -   enum ctattr_expect type)
> > +   u32 type)
> >  {
> > struct nlattr *nest_parms;
> >  

[PATCH] netfilter: ctnetlink: Make some parameters integer to avoid enum mismatch

[Matthias Kaehlcke]

Not all parameters passed to ctnetlink_parse_tuple() and
ctnetlink_exp_dump_tuple() match the enum type in the signatures of these
functions. Since this is intended change the argument type of to be an int
value.

Signed-off-by: Matthias Kaehlcke 
---
 net/netfilter/nf_conntrack_netlink.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c 
b/net/netfilter/nf_conntrack_netlink.c
index dc7dfd68fafe..775eb5d9165b 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1006,9 +1006,8 @@ static const struct nla_policy 
tuple_nla_policy[CTA_TUPLE_MAX+1] = {
 
 static int
 ctnetlink_parse_tuple(const struct nlattr * const cda[],
- struct nf_conntrack_tuple *tuple,
- enum ctattr_type type, u_int8_t l3num,
- struct nf_conntrack_zone *zone)
+ struct nf_conntrack_tuple *tuple, u32 type,
+ u_int8_t l3num, struct nf_conntrack_zone *zone)
 {
struct nlattr *tb[CTA_TUPLE_MAX+1];
int err;
@@ -2443,7 +2442,7 @@ static struct nfnl_ct_hook ctnetlink_glue_hook = {
 
 static int ctnetlink_exp_dump_tuple(struct sk_buff *skb,
const struct nf_conntrack_tuple *tuple,
-   enum ctattr_expect type)
+   u32 type)
 {
struct nlattr *nest_parms;
 
-- 
2.12.2.816.g281164-goog

[PATCH] nl80211: Fix enum type of variable in nl80211_put_sta_rate()

[Matthias Kaehlcke]

rate_flg is of type 'enum nl80211_attrs', however it is assigned with
'enum nl80211_rate_info' values. Change the type of rate_flg accordingly.

Signed-off-by: Matthias Kaehlcke 
---
 net/wireless/nl80211.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 2312dc2ffdb9..9af21a21ea6b 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -4151,7 +4151,7 @@ static bool nl80211_put_sta_rate(struct sk_buff *msg, 
struct rate_info *info,
struct nlattr *rate;
u32 bitrate;
u16 bitrate_compat;
-   enum nl80211_attrs rate_flg;
+   enum nl80211_rate_info rate_flg;
 
rate = nla_nest_start(msg, attr);
if (!rate)
-- 
2.12.2.762.g0e3151a226-goog

[PATCH] mac80211: ibss: Fix channel type enum in ieee80211_sta_join_ibss()

[Matthias Kaehlcke]

cfg80211_chandef_create() expects an 'enum nl80211_channel_type' as
channel type however in ieee80211_sta_join_ibss()
NL80211_CHAN_WIDTH_20_NOHT is passed in two occasions, which is of
the enum type 'nl80211_chan_width'. Change the value to NL80211_CHAN_NO_HT
(20 MHz, non-HT channel) of the channel type enum.

Signed-off-by: Matthias Kaehlcke 
---
 net/mac80211/ibss.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index 98999d3d5262..e957351976a2 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -425,7 +425,7 @@ static void ieee80211_sta_join_ibss(struct 
ieee80211_sub_if_data *sdata,
case NL80211_CHAN_WIDTH_5:
case NL80211_CHAN_WIDTH_10:
cfg80211_chandef_create(&chandef, cbss->channel,
-   NL80211_CHAN_WIDTH_20_NOHT);
+   NL80211_CHAN_NO_HT);
chandef.width = sdata->u.ibss.chandef.width;
break;
case NL80211_CHAN_WIDTH_80:
@@ -437,7 +437,7 @@ static void ieee80211_sta_join_ibss(struct 
ieee80211_sub_if_data *sdata,
default:
/* fall back to 20 MHz for unsupported modes */
cfg80211_chandef_create(&chandef, cbss->channel,
-   NL80211_CHAN_WIDTH_20_NOHT);
+   NL80211_CHAN_NO_HT);
break;
}
 
-- 
2.12.2.762.g0e3151a226-goog

[PATCH v2] cfg80211: Fix array-bounds warning in fragment copy

[Matthias Kaehlcke]

__ieee80211_amsdu_copy_frag intentionally initializes a pointer to
array[-1] to increment it later to valid values. clang rightfully
generates an array-bounds warning on the initialization statement.

Initialize the pointer to array[0] and change the algorithm from
increment before to increment after consume.

Signed-off-by: Matthias Kaehlcke 
---
Note: Resent to include linux-wireless in cc

 net/wireless/util.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/wireless/util.c b/net/wireless/util.c
index 68e5f2ecee1a..52795ae5337f 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -659,7 +659,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb, struct 
sk_buff *frame,
int offset, int len)
 {
struct skb_shared_info *sh = skb_shinfo(skb);
-   const skb_frag_t *frag = &sh->frags[-1];
+   const skb_frag_t *frag = &sh->frags[0];
struct page *frag_page;
void *frag_ptr;
int frag_len, frag_size;
@@ -672,10 +672,10 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb, struct 
sk_buff *frame,
 
while (offset >= frag_size) {
offset -= frag_size;
-   frag++;
frag_page = skb_frag_page(frag);
frag_ptr = skb_frag_address(frag);
frag_size = skb_frag_size(frag);
+   frag++;
}
 
frag_ptr += offset;
@@ -687,12 +687,12 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb, struct 
sk_buff *frame,
len -= cur_len;
 
while (len > 0) {
-   frag++;
frag_len = skb_frag_size(frag);
cur_len = min(len, frag_len);
__frame_add_frag(frame, skb_frag_page(frag),
 skb_frag_address(frag), cur_len, frag_len);
len -= cur_len;
+   frag++;
}
 }
 
-- 
2.12.2.715.g7642488e1d-goog

Re: [PATCH v2] cfg80211: Fix array-bounds warning in fragment copy

[Matthias Kaehlcke]

El Mon, Mar 27, 2017 at 12:58:22PM -0700 Matthias Kaehlcke ha dit:

> __ieee80211_amsdu_copy_frag intentionally initializes a pointer to
> array[-1] to increment it later to valid values. clang rightfully
> generates an array-bounds warning on the initialization statement.
> 
> Initialize the pointer to array[0] and change the algorithm from
> increment before to increment after consume.
> 
> Signed-off-by: Matthias Kaehlcke 
> ---
>  net/wireless/util.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/wireless/util.c b/net/wireless/util.c
> index 68e5f2ecee1a..52795ae5337f 100644
> --- a/net/wireless/util.c
> +++ b/net/wireless/util.c
> @@ -659,7 +659,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb, struct 
> sk_buff *frame,
>   int offset, int len)
>  {
>   struct skb_shared_info *sh = skb_shinfo(skb);
> - const skb_frag_t *frag = &sh->frags[-1];
> + const skb_frag_t *frag = &sh->frags[0];
>   struct page *frag_page;
>   void *frag_ptr;
>   int frag_len, frag_size;
> @@ -672,10 +672,10 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb, struct 
> sk_buff *frame,
>  
>   while (offset >= frag_size) {
>   offset -= frag_size;
> - frag++;
>   frag_page = skb_frag_page(frag);
>   frag_ptr = skb_frag_address(frag);
>   frag_size = skb_frag_size(frag);
> + frag++;
>   }
>  
>   frag_ptr += offset;
> @@ -687,12 +687,12 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb, struct 
> sk_buff *frame,
>   len -= cur_len;
>  
>   while (len > 0) {
> - frag++;
>   frag_len = skb_frag_size(frag);
>   cur_len = min(len, frag_len);
>   __frame_add_frag(frame, skb_frag_page(frag),
>skb_frag_address(frag), cur_len, frag_len);
>   len -= cur_len;
> + frag++;
>   }
>  }
>  

Ping, any feedback on this patch?

Thanks

Matthias

Re: [PATCH] ath9k: Add cast to u8 to FREQ2FBIN macro

[Matthias Kaehlcke]

Hi Joe,

El Thu, Apr 06, 2017 at 02:29:20PM -0700 Joe Perches ha dit:

> On Thu, 2017-04-06 at 14:21 -0700, Matthias Kaehlcke wrote:
> > The macro results are assigned to u8 variables/fields. Adding the cast
> > fixes plenty of clang warnings about "implicit conversion from 'int' to
> > 'u8'".
> > 
> > Signed-off-by: Matthias Kaehlcke 
> > ---
> >  drivers/net/wireless/ath/ath9k/eeprom.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/wireless/ath/ath9k/eeprom.h 
> > b/drivers/net/wireless/ath/ath9k/eeprom.h
> > index 30bf722e33ed..31390af6c33e 100644
> > --- a/drivers/net/wireless/ath/ath9k/eeprom.h
> > +++ b/drivers/net/wireless/ath/ath9k/eeprom.h
> > @@ -106,7 +106,7 @@
> >  #define AR9285_RDEXT_DEFAULT0x1F
> >  
> >  #define ATH9K_POW_SM(_r, _s)   (((_r) & 0x3f) << (_s))
> > -#define FREQ2FBIN(x, y)((y) ? ((x) - 2300) : (((x) - 4800) / 
> > 5))
> > +#define FREQ2FBIN(x, y)(u8)((y) ? ((x) - 2300) : (((x) - 4800) 
> > / 5))
> 
> Maybe better to use:
> 
> static inline u8 FREQ2FBIN(int x, int y)
> {
>   if (y)
>   return x - 2300;
>   return (x - 4800) / 5;
> }

Thanks for your suggestion! Unfortunately in this case an inline
function is not suitable since FREQ2FBIN() is mostly used for
structure initialization:

static const struct ar9300_eeprom ar9300_default = {
...
.calFreqPier2G = {
FREQ2FBIN(2412, 1),
FREQ2FBIN(2437, 1),
FREQ2FBIN(2472, 1),
},
...

Cheers

Matthias

[PATCH v2] mac80211: Fix clang warning about constant operand in logical operation

[Matthias Kaehlcke]

When clang detects a non-boolean constant in a logical operation it
generates a 'constant-logical-operand' warning. In
ieee80211_try_rate_control_ops_get() the result of strlen()
is used in a logical operation, clang resolves the expression to an
(integer) constant at compile time when clang's builtin strlen function
is used.

Change the condition to check for strlen() > 0 to make the constant
operand boolean and thus avoid the warning.

Signed-off-by: Matthias Kaehlcke 
---
Changes in v2:
- Changed expression to strlen() > 0 to make constant operand boolean
  instead of splitting up condition
- Updated commit message

 net/mac80211/rate.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index 206698bc93f4..694faf6ab574 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -173,9 +173,11 @@ ieee80211_rate_control_ops_get(const char *name)
/* try default if specific alg requested but not found */
ops = 
ieee80211_try_rate_control_ops_get(ieee80211_default_rc_algo);
 
-   /* try built-in one if specific alg requested but not found */
-   if (!ops && strlen(CONFIG_MAC80211_RC_DEFAULT))
+   /* Note: check for > 0 is intentional to avoid clang warning */
+   if (!ops && (strlen(CONFIG_MAC80211_RC_DEFAULT) > 0))
+   /* try built-in one if specific alg requested but not found */
ops = 
ieee80211_try_rate_control_ops_get(CONFIG_MAC80211_RC_DEFAULT);
+
kernel_param_unlock(THIS_MODULE);
 
return ops;
-- 
2.12.2.715.g7642488e1d-goog

Re: [PATCH] mac80211: Fix clang warning about constant operand in logical operation

[Matthias Kaehlcke]

El Fri, Apr 07, 2017 at 12:51:57AM +0200 Johannes Berg ha dit:

> On Thu, 2017-04-06 at 15:42 -0700, Matthias Kaehlcke wrote:
> > 
> > Thanks, it would also require to move the initialization of
> > ieee80211_default_rc_algo into an ifdef. If you can live with such a
> > solution I'm happy to change it.
> 
> I think that'd be something I can live with, yeah.
> 
> > >   git grep 'IS_ENABLED(' | grep '&&'
> > 
> > Indeed the warning is not triggered by these constructs. It seems
> > clang only emits the warning when the constant operand is not
> > boolean.
> 
> That points to just adding "> 0" to the condition here as another
> alternative solution, I guess? With a comment to make sure it's not
> removed again, that'd seem like the best thing to do.

Good point, that's more digestible. I'll send an updated change soon.

Matthias

Re: [PATCH] mac80211: Fix clang warning about constant operand in logical operation

[Matthias Kaehlcke]

El Thu, Apr 06, 2017 at 11:12:25PM +0200 Johannes Berg ha dit:

> On Thu, 2017-04-06 at 12:24 -0700, Matthias Kaehlcke wrote:
> 
> > I agree that the code looks worse :( I hoped to find a fix using a
> > preprocessor condition but wasn't successful.
> 
> It's actually easy - just remove the 'default ""' from Kconfig, and
> then the symbol won't be defined at all if it doesn't get a proper
> value. Then you can ifdef the whole thing.

Thanks, it would also require to move the initialization of
ieee80211_default_rc_algo into an ifdef. If you can live with such a
solution I'm happy to change it.

> > Some projects (like Chrome OS) build their kernel with all warnings
> > being treated as errors. Besides changing the 'offending' code the
> > alternatives are to disable the warning completely or to tell clang
> > not to use the builtin(s). IMO changing the code is the preferable
> > solution, especially since this is so far the only occurrence of the
> > warning that I have encountered.
> > 
> > I used goto instead of nested ifs since other functions in this file
> > use the same pattern. If nested ifs are preferred I can change that.
> 
> I don't really buy either argument. The warning is simply bogus - I'm
> very surprised you don't hit it with more similar macros or cases, like
> for example CONFIG_ENABLED(). Try
> 
>   git grep 'IS_ENABLED(' | grep '&&'
> 
> and you'll find lots of places that seem like they should trigger this
> warning.

Indeed the warning is not triggered by these constructs. It seems
clang only emits the warning when the constant operand is not boolean.

> You're advocating to make the code worse - not very significantly in
> this case, but still - just to quiet a compiler warning.

For Chrome OS we need to quiet the warning in one way or another,
otherwise our builds will fail due to warnings being treated as
errors. I'm reluctant to disable the warning completely since it can
be useful to detect certain errors, e.g. the use of a logical operator
when bitwise was intended.

And yes, in this case I would favor the slight deterioration of a
small section of code over losing a potentially useful check on the
entire kernel code.

I agree that this is a bit of a corner case, the code is definitely
not buggy by itself, ideally clang would detect that the constant is
a result of its own optimization and skip the warning.

Obviously we can always apply a patch locally, but ideally we try to
upstream changes that seem of general use so that others and our
future selves can benefit from it.

I have no intention to insist on this, we can live with a local patch
if you don't think it is useful.

Cheers

Matthias

[PATCH] ath9k: Add cast to u8 to FREQ2FBIN macro

[Matthias Kaehlcke]

The macro results are assigned to u8 variables/fields. Adding the cast
fixes plenty of clang warnings about "implicit conversion from 'int' to
'u8'".

Signed-off-by: Matthias Kaehlcke 
---
 drivers/net/wireless/ath/ath9k/eeprom.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/eeprom.h 
b/drivers/net/wireless/ath/ath9k/eeprom.h
index 30bf722e33ed..31390af6c33e 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom.h
+++ b/drivers/net/wireless/ath/ath9k/eeprom.h
@@ -106,7 +106,7 @@
 #define AR9285_RDEXT_DEFAULT0x1F
 
 #define ATH9K_POW_SM(_r, _s)   (((_r) & 0x3f) << (_s))
-#define FREQ2FBIN(x, y)((y) ? ((x) - 2300) : (((x) - 4800) / 
5))
+#define FREQ2FBIN(x, y)(u8)((y) ? ((x) - 2300) : (((x) - 4800) 
/ 5))
 #define FBIN2FREQ(x, y)((y) ? (2300 + x) : (4800 + 5 * x))
 #define ath9k_hw_use_flash(_ah)(!(_ah->ah_flags & AH_USE_EEPROM))
 
-- 
2.12.2.715.g7642488e1d-goog

Re: [PATCH] mac80211: Fix clang warning about constant operand in logical operation

[Matthias Kaehlcke]

Hi Johannes,

thanks for your comments

El Thu, Apr 06, 2017 at 09:11:18PM +0200 Johannes Berg ha dit:

> On Thu, 2017-04-06 at 11:56 -0700, Matthias Kaehlcke wrote:
> > Clang raises a warning about the expression 'strlen(CONFIG_XXX)'
> > being
> > used in a logical operation. Clangs' builtin strlen function resolves
> > the
> > expression to a constant at compile time, which causes clang to
> > generate
> > a 'constant-logical-operand' warning.
> > 
> > Split the if statement in two to avoid using the const expression in
> > a logical operation.
> > 
> I don't really see all much point in doing this for the warning's
> sake... hopefully it doesn't actually generate worse code, but I think
> the code ends up looking worse and people will forever wonder what the
> goto is really doing there.

I agree that the code looks worse :( I hoped to find a fix using a
preprocessor condition but wasn't successful.

Some projects (like Chrome OS) build their kernel with all warnings
being treated as errors. Besides changing the 'offending' code the
alternatives are to disable the warning completely or to tell clang
not to use the builtin(s). IMO changing the code is the preferable
solution, especially since this is so far the only occurrence of the
warning that I have encountered.

I used goto instead of nested ifs since other functions in this file
use the same pattern. If nested ifs are preferred I can change that.

Cheers

Matthias

[PATCH] mac80211: Fix clang warning about constant operand in logical operation

[Matthias Kaehlcke]

Clang raises a warning about the expression 'strlen(CONFIG_XXX)' being
used in a logical operation. Clangs' builtin strlen function resolves the
expression to a constant at compile time, which causes clang to generate
a 'constant-logical-operand' warning.

Split the if statement in two to avoid using the const expression in a
logical operation.

Signed-off-by: Matthias Kaehlcke 
---
 net/mac80211/rate.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index 206698bc93f4..68ff202d6380 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -173,9 +173,14 @@ ieee80211_rate_control_ops_get(const char *name)
/* try default if specific alg requested but not found */
ops = 
ieee80211_try_rate_control_ops_get(ieee80211_default_rc_algo);
 
+   if (ops)
+   goto unlock;
+
/* try built-in one if specific alg requested but not found */
-   if (!ops && strlen(CONFIG_MAC80211_RC_DEFAULT))
+   if (strlen(CONFIG_MAC80211_RC_DEFAULT))
ops = 
ieee80211_try_rate_control_ops_get(CONFIG_MAC80211_RC_DEFAULT);
+
+unlock:
kernel_param_unlock(THIS_MODULE);
 
return ops;
-- 
2.12.2.715.g7642488e1d-goog

[PATCH v2] cfg80211: Fix array-bounds warning in fragment copy

[Matthias Kaehlcke]

__ieee80211_amsdu_copy_frag intentionally initializes a pointer to
array[-1] to increment it later to valid values. clang rightfully
generates an array-bounds warning on the initialization statement.

Initialize the pointer to array[0] and change the algorithm from
increment before to increment after consume.

Signed-off-by: Matthias Kaehlcke 
---
 net/wireless/util.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/wireless/util.c b/net/wireless/util.c
index 68e5f2ecee1a..52795ae5337f 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -659,7 +659,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb, struct 
sk_buff *frame,
int offset, int len)
 {
struct skb_shared_info *sh = skb_shinfo(skb);
-   const skb_frag_t *frag = &sh->frags[-1];
+   const skb_frag_t *frag = &sh->frags[0];
struct page *frag_page;
void *frag_ptr;
int frag_len, frag_size;
@@ -672,10 +672,10 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb, struct 
sk_buff *frame,
 
while (offset >= frag_size) {
offset -= frag_size;
-   frag++;
frag_page = skb_frag_page(frag);
frag_ptr = skb_frag_address(frag);
frag_size = skb_frag_size(frag);
+   frag++;
}
 
frag_ptr += offset;
@@ -687,12 +687,12 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb, struct 
sk_buff *frame,
len -= cur_len;
 
while (len > 0) {
-   frag++;
frag_len = skb_frag_size(frag);
cur_len = min(len, frag_len);
__frame_add_frag(frame, skb_frag_page(frag),
 skb_frag_address(frag), cur_len, frag_len);
len -= cur_len;
+   frag++;
}
 }
 
-- 
2.12.1.578.ge9c3154ca4-goog

Re: [PATCH] cfg80211: Fix array-bounds warning in fragment copy

[Matthias Kaehlcke]

El Mon, Mar 27, 2017 at 12:47:59PM +0200 Johannes Berg ha dit:

> On Fri, 2017-03-24 at 18:06 -0700, Matthias Kaehlcke wrote:
> > __ieee80211_amsdu_copy_frag intentionally initializes a pointer to
> > array[-1] to increment it later to valid values. clang rightfully
> > generates an array-bounds warning on the initialization statement.
> > Work around this by initializing the pointer to array[0] and
> > decrementing it later, which allows to leave the rest of the
> > algorithm untouched.
> > 
> > Signed-off-by: Matthias Kaehlcke 
> > ---
> >  net/wireless/util.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/wireless/util.c b/net/wireless/util.c
> > index 68e5f2ecee1a..d3d459e4a070 100644
> > --- a/net/wireless/util.c
> > +++ b/net/wireless/util.c
> > @@ -659,7 +659,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb,
> > struct sk_buff *frame,
> >     int offset, int len)
> >  {
> >     struct skb_shared_info *sh = skb_shinfo(skb);
> > -   const skb_frag_t *frag = &sh->frags[-1];
> > +   const skb_frag_t *frag = &sh->frags[0];
> >     struct page *frag_page;
> >     void *frag_ptr;
> >     int frag_len, frag_size;
> > @@ -669,6 +669,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb,
> > struct sk_buff *frame,
> >     frag_page = virt_to_head_page(skb->head);
> >     frag_ptr = skb->data;
> >     frag_size = head_size;
> > +   frag--;
> 
> Isn't it just a question of time until the compiler will see through
> this trick and warn about it?

Maybe.

Actually it seems the algorithm can be easily adapted to increment the
pointer after consumption, which is clearer anyway. I will give this a
shot. I'm not sure how to exercise the code path for testing and would
appreciate help on this end.

Matthias

[PATCH] cfg80211: Fix array-bounds warning in fragment copy

[Matthias Kaehlcke]

__ieee80211_amsdu_copy_frag intentionally initializes a pointer to
array[-1] to increment it later to valid values. clang rightfully
generates an array-bounds warning on the initialization statement.
Work around this by initializing the pointer to array[0] and
decrementing it later, which allows to leave the rest of the
algorithm untouched.

Signed-off-by: Matthias Kaehlcke 
---
 net/wireless/util.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/wireless/util.c b/net/wireless/util.c
index 68e5f2ecee1a..d3d459e4a070 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -659,7 +659,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb, struct 
sk_buff *frame,
int offset, int len)
 {
struct skb_shared_info *sh = skb_shinfo(skb);
-   const skb_frag_t *frag = &sh->frags[-1];
+   const skb_frag_t *frag = &sh->frags[0];
struct page *frag_page;
void *frag_ptr;
int frag_len, frag_size;
@@ -669,6 +669,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb, struct 
sk_buff *frame,
frag_page = virt_to_head_page(skb->head);
frag_ptr = skb->data;
frag_size = head_size;
+   frag--;
 
while (offset >= frag_size) {
offset -= frag_size;
-- 
2.12.1.578.ge9c3154ca4-goog

[PATCH] PLIP driver: convert killed_timer_sem to completion

[Matthias Kaehlcke]

PLIP driver: convert the semaphore killed_timer_sem to
a completion

Signed-off-by: Matthias Kaehlcke <[EMAIL PROTECTED]>

--

diff --git a/drivers/net/plip.c b/drivers/net/plip.c
index 57c9866..fee3d7b 100644
--- a/drivers/net/plip.c
+++ b/drivers/net/plip.c
@@ -106,6 +106,7 @@ static const char version[] = "NET3 PLIP version 
2.4-parport [EMAIL PROTECTED]"
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -114,7 +115,6 @@ static const char version[] = "NET3 PLIP version 
2.4-parport [EMAIL PROTECTED]"
 #include 
 #include 
 #include 
-#include 
 
 /* Maximum number of devices to support. */
 #define PLIP_MAX  8
@@ -221,7 +221,7 @@ struct net_local {
int should_relinquish;
spinlock_t lock;
atomic_t kill_timer;
-   struct semaphore killed_timer_sem;
+   struct completion killed_timer_cmp;
 };
 
 static inline void enable_parport_interrupts (struct net_device *dev)
@@ -385,7 +385,7 @@ plip_timer_bh(struct work_struct *work)
schedule_delayed_work(&nl->timer, 1);
}
else {
-   up (&nl->killed_timer_sem);
+   complete(&nl->killed_timer_cmp);
}
 }
 
@@ -1112,9 +1112,9 @@ plip_close(struct net_device *dev)
 
if (dev->irq == -1)
{
-   init_MUTEX_LOCKED (&nl->killed_timer_sem);
+   init_completion(&nl->killed_timer_cmp);
atomic_set (&nl->kill_timer, 1);
-   down (&nl->killed_timer_sem);
+   wait_for_completion(&nl->killed_timer_cmp);
}
 
 #ifdef NOTDEF

-- 
Matthias Kaehlcke
Linux System Developer
Barcelona

 La libertad es como la mañana. Hay quienes esperan dormidos a que
   llegue, pero hay quienes desvelan y caminan la noche para alcanzarla
(Subcomandante Marcos)
 .''`.
using free software / Debian GNU/Linux | http://debian.org  : :'  :
`. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4  `-
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html