* [PATCH 1/9] net: davinci_cpdma: use dma_addr_t for DMA address
2016-01-27 14:04 [PATCH 0/9] network driver fixes Arnd Bergmann
@ 2016-01-27 14:04 ` Arnd Bergmann
2016-01-27 14:04 ` [PATCH 2/9] net: hp100: remove unnecessary #ifdefs Arnd Bergmann
` (8 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Arnd Bergmann @ 2016-01-27 14:04 UTC (permalink / raw)
To: David S. Miller; +Cc: linux-arm-kernel, Arnd Bergmann, netdev, linux-kernel
The davinci_cpdma mixes up physical addresses as seen from the CPU
and DMA addresses as seen from a DMA master, since it can operate
on both normal memory or an on-chip buffer. If dma_addr_t is
different from phys_addr_t, this means we get a compile-time warning
about the type mismatch:
ethernet/ti/davinci_cpdma.c: In function 'cpdma_desc_pool_create':
ethernet/ti/davinci_cpdma.c:182:48: error: passing argument 3 of 'dma_alloc_coherent' from incompatible pointer type [-Werror=incompatible-pointer-types]
pool->cpumap = dma_alloc_coherent(dev, size, &pool->phys,
In file included from ethernet/ti/davinci_cpdma.c:21:0:
dma-mapping.h:398:21: note: expected 'dma_addr_t * {aka long long unsigned int *}' but argument is of type 'phys_addr_t * {aka unsigned int *}'
static inline void *dma_alloc_coherent(struct device *dev, size_t size,
This slightly restructures the code so the address we use for
mapping RAM into a DMA address is always a dma_addr_t, avoiding
the warning. The code is correct even if both types are 32-bit
because the DMA master in this device only supports 32-bit addressing
anyway, independent of the types that are used.
We still assign this value to pool->phys, and that is wrong if
the driver is ever used with an IOMMU, but that value appears to
be never used, so there is no problem really. I've added a couple
of comments about where we do things that are slightly violating
the API.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/ethernet/ti/davinci_cpdma.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index 657b65bf5cac..18bf3a8fdc50 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -82,7 +82,7 @@ struct cpdma_desc {
struct cpdma_desc_pool {
phys_addr_t phys;
- u32 hw_addr;
+ dma_addr_t hw_addr;
void __iomem *iomap; /* ioremap map */
void *cpumap; /* dma_alloc map */
int desc_size, mem_size;
@@ -152,7 +152,7 @@ struct cpdma_chan {
* abstract out these details
*/
static struct cpdma_desc_pool *
-cpdma_desc_pool_create(struct device *dev, u32 phys, u32 hw_addr,
+cpdma_desc_pool_create(struct device *dev, u32 phys, dma_addr_t hw_addr,
int size, int align)
{
int bitmap_size;
@@ -176,13 +176,13 @@ cpdma_desc_pool_create(struct device *dev, u32 phys, u32 hw_addr,
if (phys) {
pool->phys = phys;
- pool->iomap = ioremap(phys, size);
+ pool->iomap = ioremap(phys, size); /* should be memremap? */
pool->hw_addr = hw_addr;
} else {
- pool->cpumap = dma_alloc_coherent(dev, size, &pool->phys,
+ pool->cpumap = dma_alloc_coherent(dev, size, &pool->hw_addr,
GFP_KERNEL);
- pool->iomap = pool->cpumap;
- pool->hw_addr = pool->phys;
+ pool->iomap = (void __iomem __force *)pool->cpumap;
+ pool->phys = pool->hw_addr; /* assumes no IOMMU, don't use this value */
}
if (pool->iomap)
--
2.7.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 2/9] net: hp100: remove unnecessary #ifdefs
2016-01-27 14:04 [PATCH 0/9] network driver fixes Arnd Bergmann
2016-01-27 14:04 ` [PATCH 1/9] net: davinci_cpdma: use dma_addr_t for DMA address Arnd Bergmann
@ 2016-01-27 14:04 ` Arnd Bergmann
2016-01-27 14:04 ` [PATCH 3/9] net: bgmac: clarify CONFIG_BCMA dependency Arnd Bergmann
` (7 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Arnd Bergmann @ 2016-01-27 14:04 UTC (permalink / raw)
To: David S. Miller
Cc: linux-arm-kernel, Arnd Bergmann, netdev, Jaroslav Kysela,
linux-kernel
Building the hp100 ethernet driver causes warnings when both the PCI
and EISA drivers are disabled:
ethernet/hp/hp100.c: In function 'hp100_module_init':
ethernet/hp/hp100.c:3047:2: warning: label 'out3' defined but not used [-Wunused-label]
ethernet/hp/hp100.c: At top level:
ethernet/hp/hp100.c:2828:13: warning: 'cleanup_dev' defined but not used [-Wunused-function]
We can easily avoid the warnings and make the driver look slightly
nicer by removing the #ifdefs that check for the CONFIG_PCI and
CONFIG_EISA, as all the registration functions are designed to
have no effect when the buses are disabled.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/ethernet/hp/hp100.c | 18 ------------------
1 file changed, 18 deletions(-)
diff --git a/drivers/net/ethernet/hp/hp100.c b/drivers/net/ethernet/hp/hp100.c
index 1d5c3e16d8f4..3daf2d4a7ca0 100644
--- a/drivers/net/ethernet/hp/hp100.c
+++ b/drivers/net/ethernet/hp/hp100.c
@@ -194,7 +194,6 @@ static const char *hp100_isa_tbl[] = {
};
#endif
-#ifdef CONFIG_EISA
static struct eisa_device_id hp100_eisa_tbl[] = {
{ "HWPF180" }, /* HP J2577 rev A */
{ "HWP1920" }, /* HP 27248B */
@@ -205,9 +204,7 @@ static struct eisa_device_id hp100_eisa_tbl[] = {
{ "" } /* Mandatory final entry ! */
};
MODULE_DEVICE_TABLE(eisa, hp100_eisa_tbl);
-#endif
-#ifdef CONFIG_PCI
static const struct pci_device_id hp100_pci_tbl[] = {
{PCI_VENDOR_ID_HP, PCI_DEVICE_ID_HP_J2585A, PCI_ANY_ID, PCI_ANY_ID,},
{PCI_VENDOR_ID_HP, PCI_DEVICE_ID_HP_J2585B, PCI_ANY_ID, PCI_ANY_ID,},
@@ -219,7 +216,6 @@ static const struct pci_device_id hp100_pci_tbl[] = {
{} /* Terminating entry */
};
MODULE_DEVICE_TABLE(pci, hp100_pci_tbl);
-#endif
static int hp100_rx_ratio = HP100_DEFAULT_RX_RATIO;
static int hp100_priority_tx = HP100_DEFAULT_PRIORITY_TX;
@@ -2842,7 +2838,6 @@ static void cleanup_dev(struct net_device *d)
free_netdev(d);
}
-#ifdef CONFIG_EISA
static int hp100_eisa_probe(struct device *gendev)
{
struct net_device *dev = alloc_etherdev(sizeof(struct hp100_private));
@@ -2884,9 +2879,7 @@ static struct eisa_driver hp100_eisa_driver = {
.remove = hp100_eisa_remove,
}
};
-#endif
-#ifdef CONFIG_PCI
static int hp100_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *ent)
{
@@ -2955,7 +2948,6 @@ static struct pci_driver hp100_pci_driver = {
.probe = hp100_pci_probe,
.remove = hp100_pci_remove,
};
-#endif
/*
* module section
@@ -3032,23 +3024,17 @@ static int __init hp100_module_init(void)
err = hp100_isa_init();
if (err && err != -ENODEV)
goto out;
-#ifdef CONFIG_EISA
err = eisa_driver_register(&hp100_eisa_driver);
if (err && err != -ENODEV)
goto out2;
-#endif
-#ifdef CONFIG_PCI
err = pci_register_driver(&hp100_pci_driver);
if (err && err != -ENODEV)
goto out3;
-#endif
out:
return err;
out3:
-#ifdef CONFIG_EISA
eisa_driver_unregister (&hp100_eisa_driver);
out2:
-#endif
hp100_isa_cleanup();
goto out;
}
@@ -3057,12 +3043,8 @@ static int __init hp100_module_init(void)
static void __exit hp100_module_exit(void)
{
hp100_isa_cleanup();
-#ifdef CONFIG_EISA
eisa_driver_unregister (&hp100_eisa_driver);
-#endif
-#ifdef CONFIG_PCI
pci_unregister_driver (&hp100_pci_driver);
-#endif
}
module_init(hp100_module_init)
--
2.7.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 3/9] net: bgmac: clarify CONFIG_BCMA dependency
2016-01-27 14:04 [PATCH 0/9] network driver fixes Arnd Bergmann
2016-01-27 14:04 ` [PATCH 1/9] net: davinci_cpdma: use dma_addr_t for DMA address Arnd Bergmann
2016-01-27 14:04 ` [PATCH 2/9] net: hp100: remove unnecessary #ifdefs Arnd Bergmann
@ 2016-01-27 14:04 ` Arnd Bergmann
2016-01-27 16:11 ` Paul Gortmaker
2016-01-27 14:04 ` [PATCH 4/9] net: moxart: use correct accessors for DMA memory Arnd Bergmann
` (6 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Arnd Bergmann @ 2016-01-27 14:04 UTC (permalink / raw)
To: David S. Miller
Cc: linux-arm-kernel, Arnd Bergmann, netdev, Michael Chan,
Paul Gortmaker, Rajesh Borundia, Rasesh Mody,
Rafał Miłecki, linux-kernel
The bgmac driver depends on BCMA_HOST_SOC, which is only used
when CONFIG_BCMA is enabled. However, it is a bool option and can
be set when CONFIG_BCMA=m, and then bgmac can be built-in, leading
to an obvious link error:
drivers/built-in.o: In function `bgmac_init':
:(.init.text+0x7f2c): undefined reference to `__bcma_driver_register'
drivers/built-in.o: In function `bgmac_exit':
:(.exit.text+0x110a): undefined reference to `bcma_driver_unregister'
To avoid this case, we need to depend on both BCMA and BCMA_SOC,
as this patch does. I'm also trying to make the dependency more
readable by splitting it into three lines, and adding a COMPILE_TEST
alternative so we can test-build it in all configurations that
support BCMA.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/ethernet/broadcom/Kconfig | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig
index 8550df189ceb..19f7cd02e085 100644
--- a/drivers/net/ethernet/broadcom/Kconfig
+++ b/drivers/net/ethernet/broadcom/Kconfig
@@ -151,8 +151,11 @@ config BNX2X_VXLAN
config BGMAC
tristate "BCMA bus GBit core support"
- depends on BCMA_HOST_SOC && HAS_DMA && (BCM47XX || ARCH_BCM_5301X)
+ depends on BCMA && BCMA_HOST_SOC
+ depends on HAS_DMA
+ depends on BCM47XX || ARCH_BCM_5301X || COMPILE_TEST
select PHYLIB
+ select FIXED_PHY
---help---
This driver supports GBit MAC and BCM4706 GBit MAC cores on BCMA bus.
They can be found on BCM47xx SoCs and provide gigabit ethernet.
--
2.7.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 3/9] net: bgmac: clarify CONFIG_BCMA dependency
2016-01-27 14:04 ` [PATCH 3/9] net: bgmac: clarify CONFIG_BCMA dependency Arnd Bergmann
@ 2016-01-27 16:11 ` Paul Gortmaker
2016-01-28 8:49 ` Arnd Bergmann
0 siblings, 1 reply; 24+ messages in thread
From: Paul Gortmaker @ 2016-01-27 16:11 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David S. Miller, linux-arm-kernel, netdev, Michael Chan,
Rajesh Borundia, Rasesh Mody, Rafał Miłecki,
linux-kernel
[[PATCH 3/9] net: bgmac: clarify CONFIG_BCMA dependency] On 27/01/2016 (Wed 15:04) Arnd Bergmann wrote:
> The bgmac driver depends on BCMA_HOST_SOC, which is only used
> when CONFIG_BCMA is enabled. However, it is a bool option and can
> be set when CONFIG_BCMA=m, and then bgmac can be built-in, leading
> to an obvious link error:
>
> drivers/built-in.o: In function `bgmac_init':
> :(.init.text+0x7f2c): undefined reference to `__bcma_driver_register'
> drivers/built-in.o: In function `bgmac_exit':
> :(.exit.text+0x110a): undefined reference to `bcma_driver_unregister'
>
> To avoid this case, we need to depend on both BCMA and BCMA_SOC,
> as this patch does. I'm also trying to make the dependency more
> readable by splitting it into three lines, and adding a COMPILE_TEST
> alternative so we can test-build it in all configurations that
> support BCMA.
It wasn't immediately clear to me from the above why you added the
select on FIXED_PHY.
P.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/9] net: bgmac: clarify CONFIG_BCMA dependency
2016-01-27 16:11 ` Paul Gortmaker
@ 2016-01-28 8:49 ` Arnd Bergmann
0 siblings, 0 replies; 24+ messages in thread
From: Arnd Bergmann @ 2016-01-28 8:49 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Paul Gortmaker, Rasesh Mody, netdev, Rafał Miłecki,
linux-kernel, Rajesh Borundia, Michael Chan, David S. Miller
On Wednesday 27 January 2016 11:11:10 Paul Gortmaker wrote:
> [[PATCH 3/9] net: bgmac: clarify CONFIG_BCMA dependency] On 27/01/2016 (Wed 15:04) Arnd Bergmann wrote:
>
> > The bgmac driver depends on BCMA_HOST_SOC, which is only used
> > when CONFIG_BCMA is enabled. However, it is a bool option and can
> > be set when CONFIG_BCMA=m, and then bgmac can be built-in, leading
> > to an obvious link error:
> >
> > drivers/built-in.o: In function `bgmac_init':
> > :(.init.text+0x7f2c): undefined reference to `__bcma_driver_register'
> > drivers/built-in.o: In function `bgmac_exit':
> > :(.exit.text+0x110a): undefined reference to `bcma_driver_unregister'
> >
> > To avoid this case, we need to depend on both BCMA and BCMA_SOC,
> > as this patch does. I'm also trying to make the dependency more
> > readable by splitting it into three lines, and adding a COMPILE_TEST
> > alternative so we can test-build it in all configurations that
> > support BCMA.
>
> It wasn't immediately clear to me from the above why you added the
> select on FIXED_PHY.
>
Right, I'll resend the patch with improved changelog. This series
is mostly patches for old and rare randconfig bugs, so I had built
thousands of configurations and fixed up everything until new warnings
or errors kept coming up. The FIXED_PHY error came up in the same
driver so I merged the two patches but did not notice how the changelog
failed to explain it.
Thanks,
Arnd
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/9] net: moxart: use correct accessors for DMA memory
2016-01-27 14:04 [PATCH 0/9] network driver fixes Arnd Bergmann
` (2 preceding siblings ...)
2016-01-27 14:04 ` [PATCH 3/9] net: bgmac: clarify CONFIG_BCMA dependency Arnd Bergmann
@ 2016-01-27 14:04 ` Arnd Bergmann
2016-01-28 12:36 ` David Laight
2016-01-27 14:04 ` [PATCH 5/9] net: fddi/defxx: avoid warning about uninitialized variable use Arnd Bergmann
` (5 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Arnd Bergmann @ 2016-01-27 14:04 UTC (permalink / raw)
To: David S. Miller
Cc: linux-arm-kernel, Arnd Bergmann, netdev, françois romieu,
linux-kernel
The moxart ethernet driver confuses coherent DMA buffers with
MMIO registers.
moxart_ether.c: In function 'moxart_mac_setup_desc_ring':
moxart_ether.c:146:428: error: passing argument 1 of '__fswab32' makes integer from pointer without a cast [-Werror=int-conversion]
moxart_ether.c:74:39: warning: incorrect type in argument 3 (different address spaces)
moxart_ether.c:74:39: expected void *cpu_addr
moxart_ether.c:74:39: got void [noderef] <asn:2>*tx_desc_base
This leaves the basic logic alone and uses normal pointers for
the virtual address of the descriptor. As we cannot use readl/writel
to access them, we also introduce our own moxart_desc_read
moxart_desc_write helpers that perform the same endianess swap
as the original code, but without the extra barriers and address
space conversion.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/ethernet/moxa/moxart_ether.c | 42 ++++++++++++++++++++------------
drivers/net/ethernet/moxa/moxart_ether.h | 4 +--
2 files changed, 28 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/moxa/moxart_ether.c b/drivers/net/ethernet/moxa/moxart_ether.c
index a10c928bbd6b..b2f1c0db9445 100644
--- a/drivers/net/ethernet/moxa/moxart_ether.c
+++ b/drivers/net/ethernet/moxa/moxart_ether.c
@@ -28,6 +28,16 @@
#include "moxart_ether.h"
+static inline void moxart_desc_write(u32 data, u32 *desc)
+{
+ *desc = cpu_to_le32(data);
+}
+
+static inline u32 moxart_desc_read(u32 *desc)
+{
+ return le32_to_cpu(*desc);
+}
+
static inline void moxart_emac_write(struct net_device *ndev,
unsigned int reg, unsigned long value)
{
@@ -112,7 +122,7 @@ static void moxart_mac_enable(struct net_device *ndev)
static void moxart_mac_setup_desc_ring(struct net_device *ndev)
{
struct moxart_mac_priv_t *priv = netdev_priv(ndev);
- void __iomem *desc;
+ void *desc;
int i;
for (i = 0; i < TX_DESC_NUM; i++) {
@@ -121,7 +131,7 @@ static void moxart_mac_setup_desc_ring(struct net_device *ndev)
priv->tx_buf[i] = priv->tx_buf_base + priv->tx_buf_size * i;
}
- writel(TX_DESC1_END, desc + TX_REG_OFFSET_DESC1);
+ moxart_desc_write(TX_DESC1_END, desc + TX_REG_OFFSET_DESC1);
priv->tx_head = 0;
priv->tx_tail = 0;
@@ -129,8 +139,8 @@ static void moxart_mac_setup_desc_ring(struct net_device *ndev)
for (i = 0; i < RX_DESC_NUM; i++) {
desc = priv->rx_desc_base + i * RX_REG_DESC_SIZE;
memset(desc, 0, RX_REG_DESC_SIZE);
- writel(RX_DESC0_DMA_OWN, desc + RX_REG_OFFSET_DESC0);
- writel(RX_BUF_SIZE & RX_DESC1_BUF_SIZE_MASK,
+ moxart_desc_write(RX_DESC0_DMA_OWN, desc + RX_REG_OFFSET_DESC0);
+ moxart_desc_write(RX_BUF_SIZE & RX_DESC1_BUF_SIZE_MASK,
desc + RX_REG_OFFSET_DESC1);
priv->rx_buf[i] = priv->rx_buf_base + priv->rx_buf_size * i;
@@ -141,12 +151,12 @@ static void moxart_mac_setup_desc_ring(struct net_device *ndev)
if (dma_mapping_error(&ndev->dev, priv->rx_mapping[i]))
netdev_err(ndev, "DMA mapping error\n");
- writel(priv->rx_mapping[i],
+ moxart_desc_write(priv->rx_mapping[i],
desc + RX_REG_OFFSET_DESC2 + RX_DESC2_ADDRESS_PHYS);
- writel(priv->rx_buf[i],
+ moxart_desc_write((uintptr_t)priv->rx_buf[i],
desc + RX_REG_OFFSET_DESC2 + RX_DESC2_ADDRESS_VIRT);
}
- writel(RX_DESC1_END, desc + RX_REG_OFFSET_DESC1);
+ moxart_desc_write(RX_DESC1_END, desc + RX_REG_OFFSET_DESC1);
priv->rx_head = 0;
@@ -201,14 +211,14 @@ static int moxart_rx_poll(struct napi_struct *napi, int budget)
napi);
struct net_device *ndev = priv->ndev;
struct sk_buff *skb;
- void __iomem *desc;
+ void *desc;
unsigned int desc0, len;
int rx_head = priv->rx_head;
int rx = 0;
while (rx < budget) {
desc = priv->rx_desc_base + (RX_REG_DESC_SIZE * rx_head);
- desc0 = readl(desc + RX_REG_OFFSET_DESC0);
+ desc0 = moxart_desc_read(desc + RX_REG_OFFSET_DESC0);
if (desc0 & RX_DESC0_DMA_OWN)
break;
@@ -250,7 +260,7 @@ static int moxart_rx_poll(struct napi_struct *napi, int budget)
priv->stats.multicast++;
rx_next:
- writel(RX_DESC0_DMA_OWN, desc + RX_REG_OFFSET_DESC0);
+ moxart_desc_write(RX_DESC0_DMA_OWN, desc + RX_REG_OFFSET_DESC0);
rx_head = RX_NEXT(rx_head);
priv->rx_head = rx_head;
@@ -310,7 +320,7 @@ static irqreturn_t moxart_mac_interrupt(int irq, void *dev_id)
static int moxart_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
{
struct moxart_mac_priv_t *priv = netdev_priv(ndev);
- void __iomem *desc;
+ void *desc;
unsigned int len;
unsigned int tx_head = priv->tx_head;
u32 txdes1;
@@ -319,7 +329,7 @@ static int moxart_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
desc = priv->tx_desc_base + (TX_REG_DESC_SIZE * tx_head);
spin_lock_irq(&priv->txlock);
- if (readl(desc + TX_REG_OFFSET_DESC0) & TX_DESC0_DMA_OWN) {
+ if (moxart_desc_read(desc + TX_REG_OFFSET_DESC0) & TX_DESC0_DMA_OWN) {
net_dbg_ratelimited("no TX space for packet\n");
priv->stats.tx_dropped++;
goto out_unlock;
@@ -337,9 +347,9 @@ static int moxart_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
priv->tx_len[tx_head] = len;
priv->tx_skb[tx_head] = skb;
- writel(priv->tx_mapping[tx_head],
+ moxart_desc_write(priv->tx_mapping[tx_head],
desc + TX_REG_OFFSET_DESC2 + TX_DESC2_ADDRESS_PHYS);
- writel(skb->data,
+ moxart_desc_write((uintptr_t)skb->data,
desc + TX_REG_OFFSET_DESC2 + TX_DESC2_ADDRESS_VIRT);
if (skb->len < ETH_ZLEN) {
@@ -354,8 +364,8 @@ static int moxart_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
txdes1 = TX_DESC1_LTS | TX_DESC1_FTS | (len & TX_DESC1_BUF_SIZE_MASK);
if (tx_head == TX_DESC_NUM_MASK)
txdes1 |= TX_DESC1_END;
- writel(txdes1, desc + TX_REG_OFFSET_DESC1);
- writel(TX_DESC0_DMA_OWN, desc + TX_REG_OFFSET_DESC0);
+ moxart_desc_write(txdes1, desc + TX_REG_OFFSET_DESC1);
+ moxart_desc_write(TX_DESC0_DMA_OWN, desc + TX_REG_OFFSET_DESC0);
/* start to send packet */
writel(0xffffffff, priv->base + REG_TX_POLL_DEMAND);
diff --git a/drivers/net/ethernet/moxa/moxart_ether.h b/drivers/net/ethernet/moxa/moxart_ether.h
index 2be9280d608c..93a9563ac7c6 100644
--- a/drivers/net/ethernet/moxa/moxart_ether.h
+++ b/drivers/net/ethernet/moxa/moxart_ether.h
@@ -300,7 +300,7 @@ struct moxart_mac_priv_t {
dma_addr_t rx_base;
dma_addr_t rx_mapping[RX_DESC_NUM];
- void __iomem *rx_desc_base;
+ void *rx_desc_base;
unsigned char *rx_buf_base;
unsigned char *rx_buf[RX_DESC_NUM];
unsigned int rx_head;
@@ -308,7 +308,7 @@ struct moxart_mac_priv_t {
dma_addr_t tx_base;
dma_addr_t tx_mapping[TX_DESC_NUM];
- void __iomem *tx_desc_base;
+ void *tx_desc_base;
unsigned char *tx_buf_base;
unsigned char *tx_buf[RX_DESC_NUM];
unsigned int tx_head;
--
2.7.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* RE: [PATCH 4/9] net: moxart: use correct accessors for DMA memory
2016-01-27 14:04 ` [PATCH 4/9] net: moxart: use correct accessors for DMA memory Arnd Bergmann
@ 2016-01-28 12:36 ` David Laight
2016-01-28 16:53 ` Arnd Bergmann
0 siblings, 1 reply; 24+ messages in thread
From: David Laight @ 2016-01-28 12:36 UTC (permalink / raw)
To: 'Arnd Bergmann', David S. Miller
Cc: linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org,
françois romieu, linux-kernel@vger.kernel.org
From: Arnd Bergmann
> Sent: 27 January 2016 14:05
> The moxart ethernet driver confuses coherent DMA buffers with
> MMIO registers.
>
> moxart_ether.c: In function 'moxart_mac_setup_desc_ring':
> moxart_ether.c:146:428: error: passing argument 1 of '__fswab32' makes integer from pointer without a
> cast [-Werror=int-conversion]
> moxart_ether.c:74:39: warning: incorrect type in argument 3 (different address spaces)
> moxart_ether.c:74:39: expected void *cpu_addr
> moxart_ether.c:74:39: got void [noderef] <asn:2>*tx_desc_base
>
> This leaves the basic logic alone and uses normal pointers for
> the virtual address of the descriptor. As we cannot use readl/writel
> to access them, we also introduce our own moxart_desc_read
> moxart_desc_write helpers that perform the same endianess swap
> as the original code, but without the extra barriers and address
> space conversion.
I'm pretty sure you need to add some explicit barriers:
> @@ -354,8 +364,8 @@ static int moxart_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> txdes1 = TX_DESC1_LTS | TX_DESC1_FTS | (len & TX_DESC1_BUF_SIZE_MASK);
> if (tx_head == TX_DESC_NUM_MASK)
> txdes1 |= TX_DESC1_END;
> - writel(txdes1, desc + TX_REG_OFFSET_DESC1);
> - writel(TX_DESC0_DMA_OWN, desc + TX_REG_OFFSET_DESC0);
> + moxart_desc_write(txdes1, desc + TX_REG_OFFSET_DESC1);
> + moxart_desc_write(TX_DESC0_DMA_OWN, desc + TX_REG_OFFSET_DESC0);
Those last two writes must happen in that order.
There may be others.
David
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/9] net: moxart: use correct accessors for DMA memory
2016-01-28 12:36 ` David Laight
@ 2016-01-28 16:53 ` Arnd Bergmann
2016-01-28 16:58 ` Arnd Bergmann
0 siblings, 1 reply; 24+ messages in thread
From: Arnd Bergmann @ 2016-01-28 16:53 UTC (permalink / raw)
To: linux-arm-kernel
Cc: David Laight, David S. Miller, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, françois romieu
On Thursday 28 January 2016 12:36:19 David Laight wrote:
> From: Arnd Bergmann
> > Sent: 27 January 2016 14:05
> > The moxart ethernet driver confuses coherent DMA buffers with
> > MMIO registers.
> >
> > moxart_ether.c: In function 'moxart_mac_setup_desc_ring':
> > moxart_ether.c:146:428: error: passing argument 1 of '__fswab32' makes integer from pointer without a
> > cast [-Werror=int-conversion]
> > moxart_ether.c:74:39: warning: incorrect type in argument 3 (different address spaces)
> > moxart_ether.c:74:39: expected void *cpu_addr
> > moxart_ether.c:74:39: got void [noderef] <asn:2>*tx_desc_base
> >
> > This leaves the basic logic alone and uses normal pointers for
> > the virtual address of the descriptor. As we cannot use readl/writel
> > to access them, we also introduce our own moxart_desc_read
> > moxart_desc_write helpers that perform the same endianess swap
> > as the original code, but without the extra barriers and address
> > space conversion.
>
> I'm pretty sure you need to add some explicit barriers:
>
> > @@ -354,8 +364,8 @@ static int moxart_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> > txdes1 = TX_DESC1_LTS | TX_DESC1_FTS | (len & TX_DESC1_BUF_SIZE_MASK);
> > if (tx_head == TX_DESC_NUM_MASK)
> > txdes1 |= TX_DESC1_END;
> > - writel(txdes1, desc + TX_REG_OFFSET_DESC1);
> > - writel(TX_DESC0_DMA_OWN, desc + TX_REG_OFFSET_DESC0);
> > + moxart_desc_write(txdes1, desc + TX_REG_OFFSET_DESC1);
> > + moxart_desc_write(TX_DESC0_DMA_OWN, desc + TX_REG_OFFSET_DESC0);
>
> Those last two writes must happen in that order.
> There may be others.
Makes sense. I looked at the ftmac100 driver, which is another driver
for the same hardware, and it's also missing barriers. We should
probably add them for both then.
I think for the SoC that uses this, a barrier() would be sufficient
because of the page flags that dma_alloc_coherent() uses on ARM for
non-coherent platforms, but to be on the safe side we need a full
rmb()/wmb(). Sending a version 2 now.
Arnd
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 5/9] net: fddi/defxx: avoid warning about uninitialized variable use
2016-01-27 14:04 [PATCH 0/9] network driver fixes Arnd Bergmann
` (3 preceding siblings ...)
2016-01-27 14:04 ` [PATCH 4/9] net: moxart: use correct accessors for DMA memory Arnd Bergmann
@ 2016-01-27 14:04 ` Arnd Bergmann
2016-01-27 15:15 ` Maciej W. Rozycki
2016-01-27 14:04 ` [PATCH 6/9] net: vxge: avoid unused function warnings Arnd Bergmann
` (4 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Arnd Bergmann @ 2016-01-27 14:04 UTC (permalink / raw)
To: David S. Miller
Cc: linux-arm-kernel, Arnd Bergmann, netdev, Maciej W. Rozycki,
linux-kernel
The defxx driver can be configured for different kinds of buses,
and appears to be handling this correctly, but the compiler cannot
see how it always initializes the bar_start and bar_length
fields it uses depending on the configured bus, so we get a warning
with recent gcc versions:
fddi/defxx.c: In function 'dfx_pci_unregister':
fddi/defxx.c:3726:3: warning: 'bar_len' may be used uninitialized in this function [-Wmaybe-uninitialized]
release_mem_region(bar_start[0], bar_len[0]);
fddi/defxx.c:3701:18: note: 'bar_len' was declared here
resource_size_t bar_len[3]; /* resource lengths */
fddi/defxx.c:3726:3: warning: 'bar_start' may be used uninitialized in this function [-Wmaybe-uninitialized]
release_mem_region(bar_start[0], bar_len[0]);
fddi/defxx.c:3700:18: note: 'bar_start' was declared here
resource_size_t bar_start[3]; /* pointers to ports */
^
fddi/defxx.c: In function 'dfx_pci_register':
fddi/defxx.c:617:18: warning: 'bar_len' may be used uninitialized in this function [-Wmaybe-uninitialized]
bp->base.mem = ioremap_nocache(bar_start[0], bar_len[0]);
fddi/defxx.c:537:18: note: 'bar_len' was declared here
resource_size_t bar_len[3]; /* resource length */
fddi/defxx.c:1125:2: warning: 'bar_start' may be used uninitialized in this function [-Wmaybe-uninitialized]
pr_info("%s: %s at %s addr = 0x%llx, IRQ = %d, Hardware addr = %pMF\n",
fddi/defxx.c:536:18: note: 'bar_start' was declared here
resource_size_t bar_start[3]; /* pointers to ports */
This adds code to ensure that the BAR values are initialized
even in the impossible case when a device gets probed that
does not belong to any bus. This shuts up the warning.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/fddi/defxx.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/fddi/defxx.c b/drivers/net/fddi/defxx.c
index 7f975a2c8990..e7cdd1226d39 100644
--- a/drivers/net/fddi/defxx.c
+++ b/drivers/net/fddi/defxx.c
@@ -484,6 +484,11 @@ static void dfx_get_bars(struct device *bdev,
bar_start[2] = bar_start[1] = 0;
bar_len[2] = bar_len[1] = 0;
}
+ if (!(dfx_bus_pci || dfx_bus_eisa || dfx_bus_tc)) {
+ dev_err(bdev, "invalid bus configuration\n");
+ bar_start[2] = bar_start[1] = bar_start[0] = 0;
+ bar_len[2] = bar_len[1] = bar_len[0] = 0;
+ }
}
static const struct net_device_ops dfx_netdev_ops = {
--
2.7.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 5/9] net: fddi/defxx: avoid warning about uninitialized variable use
2016-01-27 14:04 ` [PATCH 5/9] net: fddi/defxx: avoid warning about uninitialized variable use Arnd Bergmann
@ 2016-01-27 15:15 ` Maciej W. Rozycki
0 siblings, 0 replies; 24+ messages in thread
From: Maciej W. Rozycki @ 2016-01-27 15:15 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: David S. Miller, linux-arm-kernel, netdev, linux-kernel
On Wed, 27 Jan 2016, Arnd Bergmann wrote:
> This adds code to ensure that the BAR values are initialized
> even in the impossible case when a device gets probed that
> does not belong to any bus. This shuts up the warning.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/net/fddi/defxx.c | 5 +++++
> 1 file changed, 5 insertions(+)
NAK, fixed already, commit 62f2aaabcf41 ("defxx: fix build warning").
Thanks for looking into this problem though, always welcome!
Maciej
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 6/9] net: vxge: avoid unused function warnings
2016-01-27 14:04 [PATCH 0/9] network driver fixes Arnd Bergmann
` (4 preceding siblings ...)
2016-01-27 14:04 ` [PATCH 5/9] net: fddi/defxx: avoid warning about uninitialized variable use Arnd Bergmann
@ 2016-01-27 14:04 ` Arnd Bergmann
2016-01-27 14:04 ` [PATCH 7/9] net: macb: avoid uninitialized variables Arnd Bergmann
` (3 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Arnd Bergmann @ 2016-01-27 14:04 UTC (permalink / raw)
To: David S. Miller
Cc: linux-arm-kernel, Arnd Bergmann, netdev, Jon Mason, linux-kernel
When CONFIG_PCI_MSI is disabled, we get warnings about unused functions
in the vxge driver:
drivers/net/ethernet/neterion/vxge/vxge-main.c:2121:13: warning: 'adaptive_coalesce_tx_interrupts' defined but not used [-Wunused-function]
drivers/net/ethernet/neterion/vxge/vxge-main.c:2149:13: warning: 'adaptive_coalesce_rx_interrupts' defined but not used [-Wunused-function]
We could add another #ifdef here, but it's nicer to avoid those warnings
for good by converting the existing #ifdef to if(IS_ENABLED()), which has
the same effect but provides better compile-time coverage in general,
and lets the compiler understand better when the function is intentionally
unused.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/ethernet/neterion/vxge/vxge-main.c | 31 ++++++++++----------------
1 file changed, 12 insertions(+), 19 deletions(-)
diff --git a/drivers/net/ethernet/neterion/vxge/vxge-main.c b/drivers/net/ethernet/neterion/vxge/vxge-main.c
index 50d5604833ed..e0993eba5df3 100644
--- a/drivers/net/ethernet/neterion/vxge/vxge-main.c
+++ b/drivers/net/ethernet/neterion/vxge/vxge-main.c
@@ -2223,8 +2223,6 @@ static irqreturn_t vxge_isr_napi(int irq, void *dev_id)
return IRQ_NONE;
}
-#ifdef CONFIG_PCI_MSI
-
static irqreturn_t vxge_tx_msix_handle(int irq, void *dev_id)
{
struct vxge_fifo *fifo = (struct vxge_fifo *)dev_id;
@@ -2442,16 +2440,13 @@ static void vxge_rem_msix_isr(struct vxgedev *vdev)
if (vdev->config.intr_type == MSI_X)
pci_disable_msix(vdev->pdev);
}
-#endif
static void vxge_rem_isr(struct vxgedev *vdev)
{
-#ifdef CONFIG_PCI_MSI
- if (vdev->config.intr_type == MSI_X) {
+ if (IS_ENABLED(CONFIG_PCI_MSI) &&
+ vdev->config.intr_type == MSI_X) {
vxge_rem_msix_isr(vdev);
- } else
-#endif
- if (vdev->config.intr_type == INTA) {
+ } else if (vdev->config.intr_type == INTA) {
synchronize_irq(vdev->pdev->irq);
free_irq(vdev->pdev->irq, vdev);
}
@@ -2460,11 +2455,10 @@ static void vxge_rem_isr(struct vxgedev *vdev)
static int vxge_add_isr(struct vxgedev *vdev)
{
int ret = 0;
-#ifdef CONFIG_PCI_MSI
int vp_idx = 0, intr_idx = 0, intr_cnt = 0, msix_idx = 0, irq_req = 0;
int pci_fun = PCI_FUNC(vdev->pdev->devfn);
- if (vdev->config.intr_type == MSI_X)
+ if (IS_ENABLED(CONFIG_PCI_MSI) && vdev->config.intr_type == MSI_X)
ret = vxge_enable_msix(vdev);
if (ret) {
@@ -2475,7 +2469,7 @@ static int vxge_add_isr(struct vxgedev *vdev)
vdev->config.intr_type = INTA;
}
- if (vdev->config.intr_type == MSI_X) {
+ if (IS_ENABLED(CONFIG_PCI_MSI) && vdev->config.intr_type == MSI_X) {
for (intr_idx = 0;
intr_idx < (vdev->no_of_vpath *
VXGE_HW_VPATH_MSIX_ACTIVE); intr_idx++) {
@@ -2576,9 +2570,8 @@ static int vxge_add_isr(struct vxgedev *vdev)
vdev->vxge_entries[intr_cnt].in_use = 1;
vdev->vxge_entries[intr_cnt].arg = &vdev->vpaths[0];
}
-INTA_MODE:
-#endif
+INTA_MODE:
if (vdev->config.intr_type == INTA) {
snprintf(vdev->desc[0], VXGE_INTR_STRLEN,
"%s:vxge:INTA", vdev->ndev->name);
@@ -3889,12 +3882,12 @@ static void vxge_device_config_init(struct vxge_hw_device_config *device_config,
if (max_mac_vpath > VXGE_MAX_MAC_ADDR_COUNT)
max_mac_vpath = VXGE_MAX_MAC_ADDR_COUNT;
-#ifndef CONFIG_PCI_MSI
- vxge_debug_init(VXGE_ERR,
- "%s: This Kernel does not support "
- "MSI-X. Defaulting to INTA", VXGE_DRIVER_NAME);
- *intr_type = INTA;
-#endif
+ if (!IS_ENABLED(CONFIG_PCI_MSI)) {
+ vxge_debug_init(VXGE_ERR,
+ "%s: This Kernel does not support "
+ "MSI-X. Defaulting to INTA", VXGE_DRIVER_NAME);
+ *intr_type = INTA;
+ }
/* Configure whether MSI-X or IRQL. */
switch (*intr_type) {
--
2.7.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 7/9] net: macb: avoid uninitialized variables
2016-01-27 14:04 [PATCH 0/9] network driver fixes Arnd Bergmann
` (5 preceding siblings ...)
2016-01-27 14:04 ` [PATCH 6/9] net: vxge: avoid unused function warnings Arnd Bergmann
@ 2016-01-27 14:04 ` Arnd Bergmann
2016-01-27 15:51 ` Nicolas Ferre
2016-01-28 13:27 ` Sergei Shtylyov
2016-01-27 14:04 ` [PATCH 8/9] net: nb8800: avoid uninitialized variable warning Arnd Bergmann
` (2 subsequent siblings)
9 siblings, 2 replies; 24+ messages in thread
From: Arnd Bergmann @ 2016-01-27 14:04 UTC (permalink / raw)
To: David S. Miller
Cc: linux-arm-kernel, Arnd Bergmann, netdev, Nicolas Ferre,
linux-kernel
The macb_clk_init function returns three clock pointers, unless
the it fails to get the first ones. We correctly handle the
failure case by propagating the error from macb_probe, but
gcc does not realize this and incorrectly warns about a later
use of those:
In file included from /git/arm-soc/drivers/net/ethernet/cadence/macb.c:12:0:
drivers/net/ethernet/cadence/macb.c: In function 'macb_probe':
include/linux/clk.h:484:2: error: 'tx_clk' may be used uninitialized in this function [-Werror=maybe-uninitialized]
clk_disable(clk);
^
drivers/net/ethernet/cadence/macb.c:2822:28: note: 'tx_clk' was declared here
struct clk *pclk, *hclk, *tx_clk;
^
In file included from /git/arm-soc/drivers/net/ethernet/cadence/macb.c:12:0:
include/linux/clk.h:484:2: error: 'hclk' may be used uninitialized in this function [-Werror=maybe-uninitialized]
clk_disable(clk);
^
drivers/net/ethernet/cadence/macb.c:2822:21: note: 'hclk' was declared here
struct clk *pclk, *hclk, *tx_clk;
^
This shuts up the misleading warnings by ensuring that the
macb_clk_init() always stores something into all three pointers.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/ethernet/cadence/macb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 9d9984a87d42..d3aa74f9db79 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2268,6 +2268,7 @@ static int macb_clk_init(struct platform_device *pdev, struct clk **pclk,
{
int err;
+ *tx_clk = *hclk = NULL;
*pclk = devm_clk_get(&pdev->dev, "pclk");
if (IS_ERR(*pclk)) {
err = PTR_ERR(*pclk);
--
2.7.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 7/9] net: macb: avoid uninitialized variables
2016-01-27 14:04 ` [PATCH 7/9] net: macb: avoid uninitialized variables Arnd Bergmann
@ 2016-01-27 15:51 ` Nicolas Ferre
2016-01-27 16:04 ` Nicolas Ferre
2016-01-28 13:27 ` Sergei Shtylyov
1 sibling, 1 reply; 24+ messages in thread
From: Nicolas Ferre @ 2016-01-27 15:51 UTC (permalink / raw)
To: Arnd Bergmann, David S. Miller; +Cc: linux-arm-kernel, netdev, linux-kernel
Le 27/01/2016 15:04, Arnd Bergmann a écrit :
> The macb_clk_init function returns three clock pointers, unless
> the it fails to get the first ones. We correctly handle the
> failure case by propagating the error from macb_probe, but
> gcc does not realize this and incorrectly warns about a later
> use of those:
>
> In file included from /git/arm-soc/drivers/net/ethernet/cadence/macb.c:12:0:
> drivers/net/ethernet/cadence/macb.c: In function 'macb_probe':
> include/linux/clk.h:484:2: error: 'tx_clk' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> clk_disable(clk);
> ^
> drivers/net/ethernet/cadence/macb.c:2822:28: note: 'tx_clk' was declared here
> struct clk *pclk, *hclk, *tx_clk;
> ^
> In file included from /git/arm-soc/drivers/net/ethernet/cadence/macb.c:12:0:
> include/linux/clk.h:484:2: error: 'hclk' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> clk_disable(clk);
> ^
> drivers/net/ethernet/cadence/macb.c:2822:21: note: 'hclk' was declared here
> struct clk *pclk, *hclk, *tx_clk;
> ^
>
> This shuts up the misleading warnings by ensuring that the
> macb_clk_init() always stores something into all three pointers.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Okay Arnd, thanks!
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> drivers/net/ethernet/cadence/macb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index 9d9984a87d42..d3aa74f9db79 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -2268,6 +2268,7 @@ static int macb_clk_init(struct platform_device *pdev, struct clk **pclk,
> {
> int err;
>
> + *tx_clk = *hclk = NULL;
> *pclk = devm_clk_get(&pdev->dev, "pclk");
> if (IS_ERR(*pclk)) {
> err = PTR_ERR(*pclk);
>
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 7/9] net: macb: avoid uninitialized variables
2016-01-27 15:51 ` Nicolas Ferre
@ 2016-01-27 16:04 ` Nicolas Ferre
2016-01-28 16:32 ` Arnd Bergmann
0 siblings, 1 reply; 24+ messages in thread
From: Nicolas Ferre @ 2016-01-27 16:04 UTC (permalink / raw)
To: Arnd Bergmann, David S. Miller; +Cc: netdev, linux-kernel, linux-arm-kernel
Le 27/01/2016 16:51, Nicolas Ferre a écrit :
> Le 27/01/2016 15:04, Arnd Bergmann a écrit :
>> The macb_clk_init function returns three clock pointers, unless
>> the it fails to get the first ones. We correctly handle the
>> failure case by propagating the error from macb_probe, but
>> gcc does not realize this and incorrectly warns about a later
>> use of those:
>>
>> In file included from /git/arm-soc/drivers/net/ethernet/cadence/macb.c:12:0:
>> drivers/net/ethernet/cadence/macb.c: In function 'macb_probe':
>> include/linux/clk.h:484:2: error: 'tx_clk' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>> clk_disable(clk);
>> ^
>> drivers/net/ethernet/cadence/macb.c:2822:28: note: 'tx_clk' was declared here
>> struct clk *pclk, *hclk, *tx_clk;
>> ^
>> In file included from /git/arm-soc/drivers/net/ethernet/cadence/macb.c:12:0:
>> include/linux/clk.h:484:2: error: 'hclk' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>> clk_disable(clk);
>> ^
>> drivers/net/ethernet/cadence/macb.c:2822:21: note: 'hclk' was declared here
>> struct clk *pclk, *hclk, *tx_clk;
>> ^
>>
>> This shuts up the misleading warnings by ensuring that the
>> macb_clk_init() always stores something into all three pointers.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Okay Arnd, thanks!
>
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Oh, crap: actually this warning has just been fixed by Sudip Mukherjee
and is already queued by David here:
https://patchwork.ozlabs.org/patch/572610/
So, sorry, I've shot too fast: NACK...
Bye,
>> ---
>> drivers/net/ethernet/cadence/macb.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
>> index 9d9984a87d42..d3aa74f9db79 100644
>> --- a/drivers/net/ethernet/cadence/macb.c
>> +++ b/drivers/net/ethernet/cadence/macb.c
>> @@ -2268,6 +2268,7 @@ static int macb_clk_init(struct platform_device *pdev, struct clk **pclk,
>> {
>> int err;
>>
>> + *tx_clk = *hclk = NULL;
>> *pclk = devm_clk_get(&pdev->dev, "pclk");
>> if (IS_ERR(*pclk)) {
>> err = PTR_ERR(*pclk);
>>
>
>
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 7/9] net: macb: avoid uninitialized variables
2016-01-27 14:04 ` [PATCH 7/9] net: macb: avoid uninitialized variables Arnd Bergmann
2016-01-27 15:51 ` Nicolas Ferre
@ 2016-01-28 13:27 ` Sergei Shtylyov
1 sibling, 0 replies; 24+ messages in thread
From: Sergei Shtylyov @ 2016-01-28 13:27 UTC (permalink / raw)
To: Arnd Bergmann, David S. Miller
Cc: linux-arm-kernel, netdev, Nicolas Ferre, linux-kernel
Hello.
On 1/27/2016 5:04 PM, Arnd Bergmann wrote:
> The macb_clk_init function returns three clock pointers, unless
> the it fails to get the first ones. We correctly handle the
s/the//.
> failure case by propagating the error from macb_probe, but
> gcc does not realize this and incorrectly warns about a later
> use of those:
>
> In file included from /git/arm-soc/drivers/net/ethernet/cadence/macb.c:12:0:
> drivers/net/ethernet/cadence/macb.c: In function 'macb_probe':
Hm, didn't these 2 lines get swapped by chance?
> include/linux/clk.h:484:2: error: 'tx_clk' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> clk_disable(clk);
> ^
> drivers/net/ethernet/cadence/macb.c:2822:28: note: 'tx_clk' was declared here
> struct clk *pclk, *hclk, *tx_clk;
> ^
> In file included from /git/arm-soc/drivers/net/ethernet/cadence/macb.c:12:0:
> include/linux/clk.h:484:2: error: 'hclk' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> clk_disable(clk);
> ^
> drivers/net/ethernet/cadence/macb.c:2822:21: note: 'hclk' was declared here
> struct clk *pclk, *hclk, *tx_clk;
> ^
>
> This shuts up the misleading warnings by ensuring that the
> macb_clk_init() always stores something into all three pointers.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
[...]
MBR, Sergei
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 8/9] net: nb8800: avoid uninitialized variable warning
2016-01-27 14:04 [PATCH 0/9] network driver fixes Arnd Bergmann
` (6 preceding siblings ...)
2016-01-27 14:04 ` [PATCH 7/9] net: macb: avoid uninitialized variables Arnd Bergmann
@ 2016-01-27 14:04 ` Arnd Bergmann
2016-01-27 14:13 ` Måns Rullgård
2016-01-27 14:04 ` [PATCH 9/9] net: tg3: " Arnd Bergmann
2016-01-29 0:14 ` [PATCH 0/9] network driver fixes David Miller
9 siblings, 1 reply; 24+ messages in thread
From: Arnd Bergmann @ 2016-01-27 14:04 UTC (permalink / raw)
To: David S. Miller
Cc: linux-arm-kernel, Arnd Bergmann, netdev, Måns Rullgård,
linux-kernel
The nb8800_poll() function initializes the 'next' variable in the
loop looking for new input data. We know this will be called at
least once because 'budget' is a guaranteed to be a positive number
when we enter the function, but the compiler doesn't know that
and warns when the variable is used later:
drivers/net/ethernet/aurora/nb8800.c: In function 'nb8800_poll':
drivers/net/ethernet/aurora/nb8800.c:350:21: warning: 'next' may be used uninitialized in this function [-Wmaybe-uninitialized]
Changing the 'while() {}' loop to 'do {} while()' makes it obvious
to the compiler what is going on so it no longer warns.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/ethernet/aurora/nb8800.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index ecc4a334c507..f71ab2647a3b 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -302,7 +302,7 @@ static int nb8800_poll(struct napi_struct *napi, int budget)
nb8800_tx_done(dev);
again:
- while (work < budget) {
+ do {
struct nb8800_rx_buf *rxb;
unsigned int len;
@@ -330,7 +330,7 @@ again:
rxd->report = 0;
last = next;
work++;
- }
+ } while (work < budget);
if (work) {
priv->rx_descs[last].desc.config |= DESC_EOC;
--
2.7.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 8/9] net: nb8800: avoid uninitialized variable warning
2016-01-27 14:04 ` [PATCH 8/9] net: nb8800: avoid uninitialized variable warning Arnd Bergmann
@ 2016-01-27 14:13 ` Måns Rullgård
2016-01-27 15:21 ` Arnd Bergmann
0 siblings, 1 reply; 24+ messages in thread
From: Måns Rullgård @ 2016-01-27 14:13 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: David S. Miller, linux-arm-kernel, netdev, linux-kernel
Arnd Bergmann <arnd@arndb.de> writes:
> The nb8800_poll() function initializes the 'next' variable in the
> loop looking for new input data. We know this will be called at
> least once because 'budget' is a guaranteed to be a positive number
> when we enter the function, but the compiler doesn't know that
> and warns when the variable is used later:
>
> drivers/net/ethernet/aurora/nb8800.c: In function 'nb8800_poll':
> drivers/net/ethernet/aurora/nb8800.c:350:21: warning: 'next' may be used uninitialized in this function [-Wmaybe-uninitialized]
Which gcc version is this? 4.9 doesn't warn here, presumably because
it's clever enough to notice that the offending use of 'next' is under a
condition that can only be true if the first one was. Of course fixing
the code so older compilers don't warn is a good idea.
> Changing the 'while() {}' loop to 'do {} while()' makes it obvious
> to the compiler what is going on so it no longer warns.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Mans Rullgard <mans@mansr.com>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
> index ecc4a334c507..f71ab2647a3b 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -302,7 +302,7 @@ static int nb8800_poll(struct napi_struct *napi, int budget)
> nb8800_tx_done(dev);
>
> again:
> - while (work < budget) {
> + do {
> struct nb8800_rx_buf *rxb;
> unsigned int len;
>
> @@ -330,7 +330,7 @@ again:
> rxd->report = 0;
> last = next;
> work++;
> - }
> + } while (work < budget);
>
> if (work) {
> priv->rx_descs[last].desc.config |= DESC_EOC;
> --
> 2.7.0
>
--
Måns Rullgård
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 8/9] net: nb8800: avoid uninitialized variable warning
2016-01-27 14:13 ` Måns Rullgård
@ 2016-01-27 15:21 ` Arnd Bergmann
0 siblings, 0 replies; 24+ messages in thread
From: Arnd Bergmann @ 2016-01-27 15:21 UTC (permalink / raw)
To: Måns Rullgård
Cc: David S. Miller, linux-arm-kernel, netdev, linux-kernel
On Wednesday 27 January 2016 14:13:16 Måns Rullgård wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
>
> > The nb8800_poll() function initializes the 'next' variable in the
> > loop looking for new input data. We know this will be called at
> > least once because 'budget' is a guaranteed to be a positive number
> > when we enter the function, but the compiler doesn't know that
> > and warns when the variable is used later:
> >
> > drivers/net/ethernet/aurora/nb8800.c: In function 'nb8800_poll':
> > drivers/net/ethernet/aurora/nb8800.c:350:21: warning: 'next' may be used uninitialized in this function [-Wmaybe-uninitialized]
>
> Which gcc version is this? 4.9 doesn't warn here, presumably because
> it's clever enough to notice that the offending use of 'next' is under a
> condition that can only be true if the first one was. Of course fixing
> the code so older compilers don't warn is a good idea.
This was with gcc-5.2.1, but possibly in an unusual kernel configuration.
The only time I see it in my logs was with CONFIG_ARCH_IXP4XX=y, which
has its own mach/io.h and other headers that sometimes override generic
functions with a more elaborate version.
I have another patch for ixp4xx that simplifies its mach/io.h in order to
get rid of other false 'uninitialized use' warnings, but this one still
shows up with that other patch.
> > Changing the 'while() {}' loop to 'do {} while()' makes it obvious
> > to the compiler what is going on so it no longer warns.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Acked-by: Mans Rullgard <mans@mansr.com>
Thanks,
Arnd
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 9/9] net: tg3: avoid uninitialized variable warning
2016-01-27 14:04 [PATCH 0/9] network driver fixes Arnd Bergmann
` (7 preceding siblings ...)
2016-01-27 14:04 ` [PATCH 8/9] net: nb8800: avoid uninitialized variable warning Arnd Bergmann
@ 2016-01-27 14:04 ` Arnd Bergmann
2016-01-29 0:14 ` [PATCH 0/9] network driver fixes David Miller
9 siblings, 0 replies; 24+ messages in thread
From: Arnd Bergmann @ 2016-01-27 14:04 UTC (permalink / raw)
To: David S. Miller
Cc: linux-arm-kernel, Arnd Bergmann, netdev, Prashant Sreedharan,
Michael Chan, linux-kernel
The tg3_set_eeprom() function correctly initializes the 'start' variable,
but gcc generates a false warning:
drivers/net/ethernet/broadcom/tg3.c: In function 'tg3_set_eeprom':
drivers/net/ethernet/broadcom/tg3.c:12057:4: warning: 'start' may be used uninitialized in this function [-Wmaybe-uninitialized]
I have not come up with a way to restructure the code in a way that
avoids the warning without making it less readable, so this adds an
initialization for the declaration to shut up that warning.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/ethernet/broadcom/tg3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 9293675df7ba..49eea8981332 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -12016,7 +12016,7 @@ static int tg3_set_eeprom(struct net_device *dev, struct ethtool_eeprom *eeprom,
int ret;
u32 offset, len, b_offset, odd_len;
u8 *buf;
- __be32 start, end;
+ __be32 start = 0, end;
if (tg3_flag(tp, NO_NVRAM) ||
eeprom->magic != TG3_EEPROM_MAGIC)
--
2.7.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 0/9] network driver fixes
2016-01-27 14:04 [PATCH 0/9] network driver fixes Arnd Bergmann
` (8 preceding siblings ...)
2016-01-27 14:04 ` [PATCH 9/9] net: tg3: " Arnd Bergmann
@ 2016-01-29 0:14 ` David Miller
2016-01-29 12:56 ` Arnd Bergmann
9 siblings, 1 reply; 24+ messages in thread
From: David Miller @ 2016-01-29 0:14 UTC (permalink / raw)
To: arnd; +Cc: linux-arm-kernel, netdev
From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, 27 Jan 2016 15:04:50 +0100
> These are all fixes for relatively harmless bugs that showed up
> in my randconfig testing, so they should not be needed for v4.5
> but get merged into net-next.
>
> I've managed to address all 'uninitialized variable' warnings that
> I get in ARM randconfig kernels now, this series includes the
> last five I got in network drivers. They are often really annoying
> warnings but when we get new ones, they often are about actual
> bugs in corner cases, so I'm trying hard to eliminate the false
> positives here to get people to pay attention to added warnings.
> I've recently tried building with an older gcc and found tons more
> that are all bogus, this series only addresses the ones that
> gcc-5.2 finds.
Arnd I'm expecting a respin of this series to address with the
feedback you've been given.
Thanks.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 0/9] network driver fixes
2016-01-29 0:14 ` [PATCH 0/9] network driver fixes David Miller
@ 2016-01-29 12:56 ` Arnd Bergmann
0 siblings, 0 replies; 24+ messages in thread
From: Arnd Bergmann @ 2016-01-29 12:56 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: David Miller, netdev
On Thursday 28 January 2016 16:14:13 David Miller wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Wed, 27 Jan 2016 15:04:50 +0100
>
> > These are all fixes for relatively harmless bugs that showed up
> > in my randconfig testing, so they should not be needed for v4.5
> > but get merged into net-next.
> >
> > I've managed to address all 'uninitialized variable' warnings that
> > I get in ARM randconfig kernels now, this series includes the
> > last five I got in network drivers. They are often really annoying
> > warnings but when we get new ones, they often are about actual
> > bugs in corner cases, so I'm trying hard to eliminate the false
> > positives here to get people to pay attention to added warnings.
> > I've recently tried building with an older gcc and found tons more
> > that are all bogus, this series only addresses the ones that
> > gcc-5.2 finds.
>
> Arnd I'm expecting a respin of this series to address with the
> feedback you've been given.
>
Done. I wasn't sure if you planned to pick up the patches
individually, thanks for the clarification.
Arnd
^ permalink raw reply [flat|nested] 24+ messages in thread