net: hisilicon fix some bugs in HNS drivers

This patch removes the hns driver hnae sysfs interface according to Arnd's review comments.

Signed-off-by huangdaode <[email protected]>

This patchset fixes the two bugs in HNS driver, one is remove the hnae sysfs interface according to the review comments from Arnd Bergmann <[email protected]>, another is fixing the wrong mac_id judgement bug which is found during internal tests.

change log:
v3:
remove the hnae sysfs interface.

v2:
1) remove first bug fix, which is fixed in another patch submitted by Arnd Bergmann <[email protected]>
2) change the code sytyle according to Joe.

v1:
initial version.

huangdaode (2):
net: hisilicon rm hnae sysfs interface
net: hisilicon fix a bug on Hisilicon Network Subsystem

 drivers/net/ethernet/hisilicon/hns/hnae.c         | 47 -----------------------
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c |  2 +-
 2 files changed, 1 insertion(+), 48 deletions(-)

Quoted 16 lines · On Friday 16 October 2015 11:54:15 huangdaode wrote:

This patchset fixes the two bugs in HNS driver, one is remove the hnae sysfs interface according to the review comments from Arnd Bergmann <[email protected]>, another is fixing the wrong mac_id judgement bug which is found during internal tests.

change log:
v3:
remove the hnae sysfs interface.

v2:
1) remove first bug fix, which is fixed in another patch submitted by Arnd Bergmann <[email protected]>
2) change the code sytyle according to Joe.

v1:
initial version.

Quoted 9 lines

huangdaode (2):
net: hisilicon rm hnae sysfs interface
net: hisilicon fix a bug on Hisilicon Network Subsystem

  drivers/net/ethernet/hisilicon/hns/hnae.c         | 47 -----------------------
  drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c |  2 +-
  2 files changed, 1 insertion(+), 48 deletions(-)

I found two more bugs in build testing, will follow up with patches in this thread

Arnd.

.

Some configurations fail to build the hns dsaf code because of a missing header file:

ethernet/hisilicon/hns/hns_dsaf_main.c: In function 'hns_dsaf_init':
ethernet/hisilicon/hns/hns_dsaf_main.c:1096:2: error: implicit declaration of function 'vzalloc' [-Werror=implicit-function-declaration]
priv->soft_mac_tbl = vzalloc(sizeof(*priv->soft_mac_tbl)

This adds the correct #include.

Signed-off-by Arnd Bergmann <[email protected]>

All errors (new ones prefixed by >>):
drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c: In function 'hns_dsaf_get_cfg':

Quoted 1 line

Quoted 1 line

drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c:151:3: error: implicit declaration of function 'iounmap' [-Werror=implicit-function-declaration]

      iounmap(dsaf_dev->io_base);
      ^

cc1: some warnings being treated as errors

vim +/iounmap +151 drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c

511e6bc0 huangdaode 2015-09-17 142 if (!dma_set_mask_and_coherent(dsaf_dev->dev, DMA_BIT_MASK(64ULL)))

[auto build test WARNING on net-next/master -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Arnd-Bergmann/net-hisilicon-add-OF-dependency/20151016-173818
config: ia64-allmodconfig (attached as .config)

reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64

The HNS MDIO driver fails to build on older ARM machines that are not yet converted to CONFIG_OF:

drivers/net/ethernet/hisilicon/hns_mdio.c: In function 'hns_mdio_bus_name':
drivers/net/ethernet/hisilicon/hns_mdio.c:405:14: error: 'OF_BAD_ADDR' undeclared (first use in this function)

  u64 taddr = OF_BAD_ADDR;
              ^
drivers/net/ethernet/hisilicon/hns_mdio.c:405:14: note: each undeclared identifier is reported only once for each function it appears in
drivers/net/ethernet/hisilicon/hns_mdio.c:409:11: error: implicit declaration of function 'of_translate_address' [-Werror=implicit-function-declaration]
   taddr = of_translate_address(np, addr);
           ^

This clarifies the dependency to ensure we don't attempt to build these drivers without CONFIG_OF, but also adds a COMPILE_TEST alternative to give us better build coverage testing.

Build-tested on x86 as well to ensure this actually works.

Signed-off-by Arnd Bergmann <[email protected]>

Sent: 16 October 2015 11:01
BITS_RX_EN is an 'unsigned long' constant, so the ones complement of that has bits set that do not fit into a 32-bit variable on 64-bit architectures, which causes a harmless gcc warning:

From: Arnd Bergmann

Quoted 4 lines

Sent: 16 October 2015 11:01
BITS_RX_EN is an 'unsigned long' constant, so the ones complement of that has bits set that do not fit into a 32-bit variable on 64-bit architectures, which causes a harmless gcc warning:

...

drivers/net/ethernet/hisilicon/hix5hd2_gmac.c static void hix5hd2_port_enable(struct hix5hd2_priv *priv) · line 375

Quoted 5 lines

static void hix5hd2_port_disable(struct hix5hd2_priv *priv) {

drivers/net/ethernet/hisilicon/hix5hd2_gmac.c +1 -1

-     writel_relaxed(~(BITS_RX_EN | BITS_TX_EN), priv->base + PORT_EN);
+     writel_relaxed(~(u32)(BITS_RX_EN | BITS_TX_EN), priv->base + PORT_EN);
      writel_relaxed(0, priv->base + DESC_WR_RD_ENA);

ISTM that just means that the constants shouldn't be 'long'.

Quoted 12 lines · On Friday 16 October 2015 11:14:44 David Laight wrote:

From: Arnd Bergmann

Quoted 4 lines

Sent: 16 October 2015 11:01
BITS_RX_EN is an 'unsigned long' constant, so the ones complement of that has bits set that do not fit into a 32-bit variable on 64-bit architectures, which causes a harmless gcc warning:

...

Quoted 5 lines

static void hix5hd2_port_disable(struct hix5hd2_priv *priv) {

drivers/net/ethernet/hisilicon/hix5hd2_gmac.c +1 -1

-     writel_relaxed(~(BITS_RX_EN | BITS_TX_EN), priv->base + PORT_EN);
+     writel_relaxed(~(u32)(BITS_RX_EN | BITS_TX_EN), priv->base + PORT_EN);
      writel_relaxed(0, priv->base + DESC_WR_RD_ENA);

Quoted 1 line

ISTM that just means that the constants shouldn't be 'long'.

Right, but that would probably mean changing the BIT() macro or not using it here. In the past I've argued against using that macro, but I've given up that fight.

Quoted 12 lines

Quoted 11 lines

Quoted 3 lines

BITS_RX_EN is an 'unsigned long' constant, so the ones complement of that has bits set that do not fit into a 32-bit variable on 64-bit architectures, which causes a harmless gcc warning:

...

drivers/net/ethernet/hisilicon/hix5hd2_gmac.c static void hix5hd2_port_enable(struct hix5hd2_priv *priv) · line 375

Quoted 5 lines

  static void hix5hd2_port_disable(struct hix5hd2_priv *priv)
  {

drivers/net/ethernet/hisilicon/hix5hd2_gmac.c +1 -1

-     writel_relaxed(~(BITS_RX_EN | BITS_TX_EN), priv->base + PORT_EN);
+     writel_relaxed(~(u32)(BITS_RX_EN | BITS_TX_EN), priv->base + PORT_EN);
       writel_relaxed(0, priv->base + DESC_WR_RD_ENA);

ISTM that just means that the constants shouldn't be 'long'.

Quoted 3 lines

Right, but that would probably mean changing the BIT() macro or not using it here. In the past I've argued against using that macro, but I've given up that fight.

Fight on... (Somebody must have gone to USC here)

There might be value in a BIT_U32 macro.
Maybe BIT_U64 too.

Quoted 16 lines · Joe Perches

Quoted 15 lines

Quoted 11 lines

Quoted 3 lines

BITS_RX_EN is an 'unsigned long' constant, so the ones complement of that has bits set that do not fit into a 32-bit variable on 64-bit architectures, which causes a harmless gcc warning:

...

drivers/net/ethernet/hisilicon/hix5hd2_gmac.c static void hix5hd2_port_enable(struct hix5hd2_priv *priv) · line 375

Quoted 5 lines

  static void hix5hd2_port_disable(struct hix5hd2_priv *priv)
  {

drivers/net/ethernet/hisilicon/hix5hd2_gmac.c +1 -1

-     writel_relaxed(~(BITS_RX_EN | BITS_TX_EN), priv->base + PORT_EN);
+     writel_relaxed(~(u32)(BITS_RX_EN | BITS_TX_EN), priv->base + PORT_EN);
       writel_relaxed(0, priv->base + DESC_WR_RD_ENA);

ISTM that just means that the constants shouldn't be 'long'.

Right, but that would probably mean changing the BIT() macro or not using it here. In the past I've argued against using that macro, but I've given up that fight.

Quoted 4 lines

Fight on... (Somebody must have gone to USC here)

There might be value in aefin BIT_U32 macro.
Maybe BIT_U64 too.

There's BIT_ULL() already.

Quoted 12 lines · Sergei Shtylyov

Quoted 10 lines · Joe Perches

Quoted 10 lines

Quoted 10 lines

Quoted 3 lines

BITS_RX_EN is an 'unsigned long' constant, so the ones complement of that has bits set that do not fit into a 32-bit variable on 64-bit architectures, which causes a harmless gcc warning:

...

Quoted 5 lines

  static void hix5hd2_port_disable(struct hix5hd2_priv *priv)
  {

drivers/net/ethernet/hisilicon/hix5hd2_gmac.c +1 -1

-     writel_relaxed(~(BITS_RX_EN | BITS_TX_EN), priv->base + PORT_EN);
+     writel_relaxed(~(u32)(BITS_RX_EN | BITS_TX_EN), priv->base + PORT_EN);
       writel_relaxed(0, priv->base + DESC_WR_RD_ENA);

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

ISTM that just means that the constants shouldn't be 'long'.

Right, but that would probably mean changing the BIT() macro or not using it here. In the past I've argued against using that macro, but I've given up that fight.

Fight on... (Somebody must have gone to USC here)

Quoted 4 lines

Quoted 2 lines

There might be value in aefin BIT_U32 macro.
Maybe BIT_U64 too.

There's BIT_ULL() already.

I know, but symmetry is good.
I think there'd be no harm in adding it. Perhaps adding all the sized variants would be useful.

Something like:

#define BIT_OF_TYPE(type, nr) \

({									\
	typeof(type) rtn;						\
	BUILD_BUG_ON(__builtin_constant_p(nr) &&			\
		     ((nr) < 0 ||					\
		      (nr) >= sizeof(type) * BITS_PER_BYTE));		\
	rtn = ((type)1) << (nr);					\
	rtn;								\
})

#define BIT_U8(nr) BIT_OF_TYPE(u8, nr)
#define BIT_U16(nr) BIT_OF_TYPE(u16, nr)
#define BIT_U32(nr) BIT_OF_TYPE(u32, nr)
#define BIT_U64(nr) BIT_OF_TYPE(u64, nr)

BITS_RX_EN is an 'unsigned long' constant, so the ones complement of that has bits set that do not fit into a 32-bit variable on 64-bit architectures, which causes a harmless gcc warning:

drivers/net/ethernet/hisilicon/hix5hd2_gmac.c: In function 'hix5hd2_port_disable':
drivers/net/ethernet/hisilicon/hix5hd2_gmac.c:374:2: warning: large integer implicitly truncated to unsigned type [-Woverflow]
writel_relaxed(~(BITS_RX_EN | BITS_TX_EN), priv->base + PORT_EN);

This adds a cast to (u32) to tell gcc that the code is indeed fine.

Signed-off-by Arnd Bergmann <[email protected]>

This patchset fixes the two bugs in HNS driver, one is remove the hnae sysfs interface according to the review comments from Arnd Bergmann <[email protected]>, another is fixing the wrong mac_id judgement bug which is found during internal tests.