Re: [PATCH net-next] r8169: add module param for control of ASPM disable
net-next is closed anyway. -- Ueimor
Re: [PATCH net-next] r8169: add module param for control of ASPM disable
Chunhao Lin :
[...]
> @@ -5878,6 +5881,20 @@ static void rtl_pcie_state_l2l3_enable(struct
> rtl8169_private *tp, bool enable)
> RTL_W8(Config3, data);
> }
>
> +static void rtl_hw_internal_aspm_clkreq_enable(struct rtl8169_private *tp,
> + bool enable)
> +{
> + void __iomem *ioaddr = tp->mmio_addr;
> +
> + if (enable) {
> + RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
> + RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
> + } else {
> + RTL_W8(Config2, RTL_R8(Config2) & ~ClkReqEn);
> + RTL_W8(Config5, RTL_R8(Config5) & ~ASPM_en);
> + }
> +}
s/enable(..., false)/disable()/
static void rtl_hw_internal_aspm_clkreq_enable(truct rtl8169_private *tp)
{
void __iomem *ioaddr = tp->mmio_addr;
RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
}
static void rtl_hw_internal_aspm_clkreq_disable(truct rtl8169_private *tp)
{
void __iomem *ioaddr = tp->mmio_addr;
RTL_W8(Config2, RTL_R8(Config2) & ~ClkReqEn);
RTL_W8(Config5, RTL_R8(Config5) & ~ASPM_en);
}
If you really want to factor something out, you may use helpers that set
or clear bits according to the 3-uple (tp, register, bits) but
foo_enable(..., false) is pointlessly convoluted.
--
Ueimor
Re: net: r8169: a question of memory barrier in the r8169 driver
Peter Zijlstra :
> On Fri, Jan 19, 2018 at 02:11:18AM +0100, Francois Romieu wrote:
> > Peter Zijlstra :
> > [...]
> > > There is only 1 variable afaict. Memory barriers need at least 2 in
> > > order to be able to do _anything_.
> >
> > I don't get your point: why don't {cur_tx, dirty_tx} qualify as said
> > two variables ?
>
> There wasn't any cur_tx in the code you provided.
/* A skbuff with nr_frags needs nr_frags+1 entries in the tx queue */
#define TX_FRAGS_READY_FOR(tp,nr_frags) \
(TX_SLOTS_AVAIL(tp) >= (nr_frags + 1))
#define TX_SLOTS_AVAIL(tp) \
(tp->dirty_tx + NUM_TX_DESC - tp->cur_tx)
Both are also used in rtl_tx.
I don't get your point. Even a single variable is scattered through
the system.
--
Ueimor
Re: net: r8169: a question of memory barrier in the r8169 driver
Jia-Ju Bai :
>
> On 2018/1/19 9:11, Francois Romieu wrote:
> > Jia-Ju Bai :
> > [...]
> > > The function rtl8169_start_xmit reads tp->dirty_tx in TX_FRAGS_READY_FOR:
> > > if (unlikely(!TX_FRAGS_READY_FOR(tp, skb_shinfo(skb)->nr_frags))) {
> > > netif_err(tp, drv, dev, "BUG! Tx Ring full when queue awake!\n");
> > > goto err_stop_0;
> > > }
> > > But there is no memory barrier around this code.
> > >
> > > Is there a possible data race here?
> > This code would not even be needed if rtl8169_start_xmit was only your
> > usual ndo_start_xmit handler: Realtek {ab / re}used it for GSO handling
> > (see r8169_csum_workaround).
> >
> > If the test is not a no-op in this GSO context, it's racy.
> >
>
> Thanks for reply.
> I didn't clearly understand your meaning...
It's fine.
> I wonder whether there is a possible data race and whether a "smp_mb" is
> needed before this code?
> By the way, do you mean that this code can be removed?
This code may be removed in a driver that properly stops itself its
tx queueing in the ndo_start_xmit handler (I would still keep it as
a bug detection helper but it's just a matter of taste). That's what
the r8169 driver used to aim at.
However, since e974604b453e87f8d864371786375d3d511fdf56, there is a piece
of code where the r8169 driver iteratively uses its own ndo_start_xmit
(without even checking its return value) in r8169_csum_workaround.
It is racy. Now, let's forget races for a few seconds: how is
r8169_csum_workaround supposed to work at all given that it does not care
if (the "unlikely(...)" test in) rtl8169_start_xmit succeeds or not ?
rtl8169_start_xmit can leave the skb as-is or map it to hardware descriptors
(whence late release in rtl_tx). net/core/dev.c::dev_hard_start_xmit cares.
r8169_csum_workaround doesn't.
--
Ueimor
Re: net: r8169: a question of memory barrier in the r8169 driver
Jia-Ju Bai :
[...]
> The function rtl8169_start_xmit reads tp->dirty_tx in TX_FRAGS_READY_FOR:
> if (unlikely(!TX_FRAGS_READY_FOR(tp, skb_shinfo(skb)->nr_frags))) {
> netif_err(tp, drv, dev, "BUG! Tx Ring full when queue awake!\n");
> goto err_stop_0;
> }
> But there is no memory barrier around this code.
>
> Is there a possible data race here?
This code would not even be needed if rtl8169_start_xmit was only your
usual ndo_start_xmit handler: Realtek {ab / re}used it for GSO handling
(see r8169_csum_workaround).
If the test is not a no-op in this GSO context, it's racy.
--
Ueimor
Re: net: r8169: a question of memory barrier in the r8169 driver
Peter Zijlstra :
[...]
> There is only 1 variable afaict. Memory barriers need at least 2 in
> order to be able to do _anything_.
I don't get your point: why don't {cur_tx, dirty_tx} qualify as said
two variables ?
--
Ueimor
Re: [PATCH 1/2] r8169: reinstate ALDPS for power saving
David Miller : [...] > The amount of coverage this change is going to get is very small as > well, meaning an even greater chance of regressions. Yes. > Therefore the only acceptable way to handle this is to have > a white-list, specific chips that have been explicitly tested > and are known to work with this feature, rather than the other > way around. > > Furthermore, you're not even checking the chip version, you're > checking instead whether the firmware is loaded or not. That > doesn't seem like a safe way to guard this at all. Actually the chip specific xyz_hw_phy_config methods call the relevant aldps enabling helper _but_ the 8168evl dedicated xyz_hw_phy_config doesn't. The firmware loaded check is just a distraction for the busy reviewer. -- Ueimor
Re: linux-next: Signed-off-by missing for commit in the net-next tree
Kirill Smelkov : [...] > I was keeping you in To and Cc all the time but got no reply at all since my > first posting from ~ 1 month ago. I thought it was longer than that. Sorry for the frustrating excess delay. As Eric already said there is no problem and I am perfectly fine with the current attribution of this code. Use of errno.h::ELNRNG is really unusual but it's a different topic. -- Ueimor
Re: r8169 Wake-on-LAN causes immediate ACPI GPE wakeup
Daniel Drake : [...] > Also, is there a standard behaviour defined for ethernet drivers > regarding wake-on-LAN? r8169 appears to enable wake-on-LAN by default > if it believes the hardware is capable of it, If so it isn't its designed behavior. The r8169 driver does not enable specific WoL event source (unicast packet, link, etc.). It should keep the current settings unless one of those holds: - explicit wol config from userspace (obviously :o) ) - runtime pm requires different settings to resume. The change should be temporary (save before suspend, restore after resume). The device is supposed to require both an event source + Config1.PMEnable. A problem may happen if some event source bit is already set while Config1.PMEnable is not. The driver has been forcing Config1.PMEnable since 5d06a99f543e734ceb53bbc9e550537be97f0c49. One may thus experience transition from inconsistent wol settings to enabled ones (if you want to dig it, check beforehand if Config1.PMEnable is really read-write or hardwired to 1). Whatever it seems rather orthogonal with your wake over wifi style instant resume symptom. -- Ueimor
Re: [PATCH v3 net-next 2/2] wan: dscc4: convert to plain DMA API
David Miller : [...] > Oops, this will need to be sent as a relative fixup as I've applied these > two patches to net-next, sorry Francois. No problem. It works perfectly this way. -- Ueimor
Re: [PATCH v3 net-next 2/2] wan: dscc4: convert to plain DMA API
Alexey Khoroshilov :
[...]
> diff --git a/drivers/net/wan/dscc4.c b/drivers/net/wan/dscc4.c
> index 8480dbf..a043fb1 100644
> --- a/drivers/net/wan/dscc4.c
> +++ b/drivers/net/wan/dscc4.c
[...]
> @@ -506,8 +506,9 @@ static void dscc4_release_ring(struct dscc4_dev_priv
> *dpriv)
> skbuff = dpriv->rx_skbuff;
> for (i = 0; i < RX_RING_SIZE; i++) {
> if (*skbuff) {
> - pci_unmap_single(pdev, le32_to_cpu(rx_fd->data),
> - RX_MAX(HDLC_MAX_MRU), PCI_DMA_FROMDEVICE);
> + dma_unmap_single(d, le32_to_cpu(rx_fd->data),
> + RX_MAX(HDLC_MAX_MRU),
> + DMA_FROM_DEVICE);
RX_MAX(HDLC_MAX_MRU), DMA_FROM_DEVICE);
[...]
> @@ -664,8 +665,8 @@ static inline void dscc4_rx_skb(struct dscc4_dev_priv
> *dpriv,
> goto refill;
> }
> pkt_len = TO_SIZE(le32_to_cpu(rx_fd->state2));
> - pci_unmap_single(pdev, le32_to_cpu(rx_fd->data),
> - RX_MAX(HDLC_MAX_MRU), PCI_DMA_FROMDEVICE);
> + dma_unmap_single(d, le32_to_cpu(rx_fd->data),
> + RX_MAX(HDLC_MAX_MRU), DMA_FROM_DEVICE);
dma_unmap_single(d, le32_to_cpu(rx_fd->data), RX_MAX(HDLC_MAX_MRU),
DMA_FROM_DEVICE);
[...]
> @@ -782,8 +783,8 @@ static int dscc4_init_one(struct pci_dev *pdev, const
> struct pci_device_id *ent)
>
> rc = -ENOMEM;
>
> - priv->iqcfg = (__le32 *) pci_alloc_consistent(pdev,
> - IRQ_RING_SIZE*sizeof(__le32), &priv->iqcfg_dma);
> + priv->iqcfg = (__le32 *)dma_alloc_coherent(&pdev->dev,
> + IRQ_RING_SIZE*sizeof(__le32), &priv->iqcfg_dma, GFP_KERNEL);
- the cast can go away
- please replace &pdev->dev with a local variable
priv->iqcfg = dma_alloc_coherent(d, IRQ_RING_SIZE*sizeof(__le32),
&priv->iqcfg_dma, GFP_KERNEL);
Same thing for iqtx and iqrx (beware of copy&paste + tx/rx mismatch).
Thanks.
--
Ueimor
Re: [PATCH net-next v2] wan: dscc4: add checks for dma mapping errors
Alexey Khoroshilov :
[...]
> diff --git a/drivers/net/wan/dscc4.c b/drivers/net/wan/dscc4.c
> index 799830f..6a9ffac 100644
> --- a/drivers/net/wan/dscc4.c
> +++ b/drivers/net/wan/dscc4.c
> @@ -518,23 +518,31 @@ static void dscc4_release_ring(struct dscc4_dev_priv
> *dpriv)
> static inline int try_get_rx_skb(struct dscc4_dev_priv *dpriv,
>struct net_device *dev)
> {
> + struct pci_dev *pdev = dpriv->pci_priv->pdev;
> unsigned int dirty = dpriv->rx_dirty%RX_RING_SIZE;
> struct RxFD *rx_fd = dpriv->rx_fd + dirty;
> const int len = RX_MAX(HDLC_MAX_MRU);
For the edification of the masses, you may follow a strict inverted
xmas tree style (aka longer lines first as long as it does not bug).
[...]
> @@ -1147,14 +1155,22 @@ static netdev_tx_t dscc4_start_xmit(struct sk_buff
> *skb,
> struct dscc4_dev_priv *dpriv = dscc4_priv(dev);
> struct dscc4_pci_priv *ppriv = dpriv->pci_priv;
> struct TxFD *tx_fd;
> + dma_addr_t addr;
> int next;
>
> + addr = pci_map_single(ppriv->pdev, skb->data, skb->len,
> + PCI_DMA_TODEVICE);
Use a local struct pci_dev *pdev and it fits on a single line.
At some point it will probably be converted to plain dma api and use a 'd' dev.
[...]
> @@ -1887,16 +1903,22 @@ static struct sk_buff *dscc4_init_dummy_skb(struct
> dscc4_dev_priv *dpriv)
>
> skb = dev_alloc_skb(DUMMY_SKB_SIZE);
> if (skb) {
> + struct pci_dev *pdev = dpriv->pci_priv->pdev;
> int last = dpriv->tx_dirty%TX_RING_SIZE;
> struct TxFD *tx_fd = dpriv->tx_fd + last;
> + dma_addr_t addr;
>
> skb->len = DUMMY_SKB_SIZE;
> skb_copy_to_linear_data(skb, version,
> strlen(version) % DUMMY_SKB_SIZE);
> tx_fd->state = FrameEnd | TO_STATE_TX(DUMMY_SKB_SIZE);
> - tx_fd->data = cpu_to_le32(pci_map_single(dpriv->pci_priv->pdev,
> - skb->data, DUMMY_SKB_SIZE,
> - PCI_DMA_TODEVICE));
> + addr = pci_map_single(pdev, skb->data, DUMMY_SKB_SIZE,
> + PCI_DMA_TODEVICE);
> + if (pci_dma_mapping_error(pdev, addr)) {
> + dev_kfree_skb_any(skb);
> + return NULL;
> + }
> + tx_fd->data = cpu_to_le32(addr);
> dpriv->tx_skbuff[last] = skb;
> }
> return skb;
It isn't technically wrong but please don't update tx_fd before the mapping
succeeds. It will look marginally better.
Thanks.
--
Ueimor
Re: [PATCH] wan: dscc4: add checks for dma mapping errors
Alexey Khoroshilov :
> The driver does not check if mapping dma memory succeed.
> The patch adds the checks and failure handling.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov
Please amend your subject line as:
Subject: [PATCH net-next v2 1/1] dscc4: add checks for dma mapping errors.
Rationale: davem is not supposed to guess the branch the patch should be
applied to.
[...]
> diff --git a/drivers/net/wan/dscc4.c b/drivers/net/wan/dscc4.c
> index 799830ffcae2..1a94f0a95b2c 100644
> --- a/drivers/net/wan/dscc4.c
> +++ b/drivers/net/wan/dscc4.c
> @@ -522,19 +522,27 @@ static inline int try_get_rx_skb(struct dscc4_dev_priv
> *dpriv,
> struct RxFD *rx_fd = dpriv->rx_fd + dirty;
> const int len = RX_MAX(HDLC_MAX_MRU);
> struct sk_buff *skb;
> - int ret = 0;
> + dma_addr_t addr;
>
> skb = dev_alloc_skb(len);
> dpriv->rx_skbuff[dirty] = skb;
> - if (skb) {
> - skb->protocol = hdlc_type_trans(skb, dev);
> - rx_fd->data = cpu_to_le32(pci_map_single(dpriv->pci_priv->pdev,
> - skb->data, len, PCI_DMA_FROMDEVICE));
> - } else {
> - rx_fd->data = 0;
> - ret = -1;
> - }
> - return ret;
> + if (!skb)
> + goto err_out;
> +
> + skb->protocol = hdlc_type_trans(skb, dev);
> + addr = pci_map_single(dpriv->pci_priv->pdev,
> + skb->data, len, PCI_DMA_FROMDEVICE);
> + if (pci_dma_mapping_error(dpriv->pci_priv->pdev, addr))
> + goto err_free_skb;
Nit: please use a local 'struct pci_dev *pdev = dpriv->pci_priv->pdev;'
[...]
> @@ -1147,14 +1155,22 @@ static netdev_tx_t dscc4_start_xmit(struct sk_buff
> *skb,
> struct dscc4_dev_priv *dpriv = dscc4_priv(dev);
> struct dscc4_pci_priv *ppriv = dpriv->pci_priv;
> struct TxFD *tx_fd;
> + dma_addr_t addr;
> int next;
>
> + addr = pci_map_single(ppriv->pdev, skb->data, skb->len,
> + PCI_DMA_TODEVICE);
> + if (pci_dma_mapping_error(ppriv->pdev, addr)) {
> + dev_kfree_skb_any(skb);
> + dev->stats.tx_errors++;
It should read 'dev->stats.tx_dropped++'.
--
Ueimor
Re: [PATCH V2 net-next 01/21] net-next/hinic: Initialize hw interface
Aviad Krawczyk :
[...]
> module_pci_driver - is not used in other drivers in the same segments, it
> is necessary ?
/me checks... Ok, there seems to be some overenthusiastic copy'paste.
See drivers/net/ethernet/intel/ixgb/ixgb_main.c:
[...]
/**
* ixgb_init_module - Driver Registration Routine
*
* ixgb_init_module is the first routine called when the driver is
* loaded. All it does is register with the PCI subsystem.
**/
static int __init
ixgb_init_module(void)
{
pr_info("%s - version %s\n", ixgb_driver_string, ixgb_driver_version);
pr_info("%s\n", ixgb_copyright);
return pci_register_driver(&ixgb_driver);
}
module_init(ixgb_init_module);
/**
* ixgb_exit_module - Driver Exit Cleanup Routine
*
* ixgb_exit_module is called just before the driver is removed
* from memory.
**/
static void __exit
ixgb_exit_module(void)
{
pci_unregister_driver(&ixgb_driver);
}
module_exit(ixgb_exit_module);
Driver version ought to be fed through ethtool, if ever. Copyright message
mildly contributes to a better world. So the whole stuff above could be:
module_pci_driver(ixgb_driver);
--
Ueimor
Re: [PATCH V2 net-next 01/21] net-next/hinic: Initialize hw interface
Aviad Krawczyk :
[...]
> hinic_remove - If insmod failed and someone calls rmmod, we will get a
> crash because the resource are already free. Therefore we test if the
> device exists, please tell me if you meant to something different
The module won't even proceed through the pci_driver remove method if
the probe method failed. See drivers/pci/bus.c::pci_bus_add_device and
track 'dev->is_added'. You don't need to believe me: try it.
I have no idea where your crash comes from but something does not
look quite right.
(use module_pci_driver() to save some boilerplate code btw)
[...]
> The priv data is in type void * because the
> caller can use any struct that it wants, like the priv data in Linux
> (netdev, irq, tasklet, work..) -
I disagree. A driver is a piece of glue between hardware and software.
It fills a kernel's contract. It is not supposed to introduce opaque
data (even if it's hard to resist).
> we can change it but if we will pass different struct
> in the future, we will have to change the prototype of the functions.
It's fine. If I do something wrong - and at some point I will - I'd
rather have it detected at compile time. Nobody wants to waste precious
hardware lab testing time because of excess void *.
> According to the other void *:
> The wq struct is used for cmdq, sq and rq. Therefore the wqe is in type
> void *. There are 4 operations get_wqe, write_wqe, read_wqe and put_wqe -
> there
> is no option that one function will be fed with a wrong pointer because the
> caller
> should use what it got in get_wqe function.
>
> When something is used as multiple types, it can be used as void * or union.
> Usually, I would prefer union. But, in this case if we will use union, maybe
> there is a chance of using the wrong wqe type in the wrong work queue type.
union * will at least catch being fed a wrong type. void * won't notice.
Let's take a practical example: hinic_sq_get_sges.
void hinic_sq_get_sges(void *wqe, struct hinic_sge *sges, int nr_sges)
^
{
struct hinic_sq_wqe *sq_wqe = (struct hinic_sq_wqe *)wqe;
static void free_all_tx_skbs(struct hinic_txq *txq)
{
struct hinic_dev *nic_dev = netdev_priv(txq->netdev);
struct hinic_sq *sq = txq->sq;
struct hinic_sq_wqe *wqe;
[...]
hinic_sq_get_sges(wqe, txq->free_sges, nr_sges);
static int free_tx_poll(struct napi_struct *napi, int budget)
{
[...]
struct hinic_sq_wqe *wqe;
[...]
hinic_sq_get_sges(wqe, txq->free_sges, nr_sges);
Why is it:
void hinic_sq_get_sges(void *wqe, ...
instead of:
void hinic_sq_get_sges(struct hinic_sq_wqe *wqe, ...
Because of a future change ?
--
Ueimor
Re: [PATCH V2 net-next 01/21] net-next/hinic: Initialize hw interface
Aviad Krawczyk :
[...]
> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c
> b/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c
> new file mode 100644
> index 000..fbc9de4
> --- /dev/null
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_hw_dev.c
[...]
> +/**
> + * hinic_init_hwdev - Initialize the NIC HW
> + * @hwdev: the NIC HW device that is returned from the initialization
> + * @pdev: the NIC pci device
> + *
> + * Return 0 - Success, negative - Failure
> + *
> + * Initialize the NIC HW device and return a pointer to it in the first arg
> + **/
Return a pointer and use ERR_PTR / IS_ERR ?
> +int hinic_init_hwdev(struct hinic_hwdev **hwdev, struct pci_dev *pdev)
> +{
> + struct hinic_pfhwdev *pfhwdev;
> + struct hinic_hwif *hwif;
> + int err;
> +
> + hwif = devm_kzalloc(&pdev->dev, sizeof(*hwif), GFP_KERNEL);
> + if (!hwif)
> + return -ENOMEM;
> +
> + err = hinic_init_hwif(hwif, pdev);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to init HW interface\n");
> + return err;
> + }
> +
> + if (!HINIC_IS_PF(hwif) && !HINIC_IS_PPF(hwif)) {
> + dev_err(&pdev->dev, "Unsupported PCI Function type\n");
> + err = -EFAULT;
> + goto func_type_err;
> + }
> +
> + pfhwdev = devm_kzalloc(&pdev->dev, sizeof(*pfhwdev), GFP_KERNEL);
> + if (!pfhwdev) {
> + err = -ENOMEM;
> + goto pfhwdev_alloc_err;
Intel, Mellanox, Broadcom, Amazon and friends use "err_xyz" labels.
Please consider using the same style.
[...]
> +void hinic_free_hwdev(struct hinic_hwdev *hwdev)
> +{
> + struct hinic_hwif *hwif = hwdev->hwif;
> + struct pci_dev *pdev = hwif->pdev;
> + struct hinic_pfhwdev *pfhwdev;
> +
> + if (!HINIC_IS_PF(hwif) && !HINIC_IS_PPF(hwif)) {
> + dev_err(&pdev->dev, "unsupported PCI Function type\n");
> + return;
> + }
If it succeeded in hinic_init_hwdev, how could it fail here ?
If it failed in hinic_init_hwdev, hinic_free_hwdev should not even
be called.
-> remove ?
[...]
> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_main.c
> b/drivers/net/ethernet/huawei/hinic/hinic_main.c
> new file mode 100644
> index 000..c61c769
> --- /dev/null
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_main.c
[...]
> +static void hinic_remove(struct pci_dev *pdev)
> +{
> + struct net_device *netdev = pci_get_drvdata(pdev);
> + struct hinic_dev *nic_dev;
> +
> + if (!netdev)
> + return;
Your driver is flawed if this test can ever succeed.
[...]
> +static int __init hinic_init(void)
> +{
> + return pci_register_driver(&hinic_driver);
> +}
> +
> +static void __exit hinic_exit(void)
> +{
> + pci_unregister_driver(&hinic_driver);
> +}
Use module_pci_driver(hinic_driver).
Remove hinic_init() and hinic_exit().
> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_pci_id_tbl.h
> b/drivers/net/ethernet/huawei/hinic/hinic_pci_id_tbl.h
> new file mode 100644
> index 000..1d92617
> --- /dev/null
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_pci_id_tbl.h
[...]
> +#ifndef HINIC_PCI_ID_TBL_H
> +#define HINIC_PCI_ID_TBL_H
> +
> +#ifndef PCI_VENDOR_ID_HUAWEI
> +#define PCI_VENDOR_ID_HUAWEI0x19e5
> +#endif
Useless: it duplicates include/linux/pci_ids.h
> +
> +#ifndef PCI_DEVICE_ID_HI1822_PF
> +#define PCI_DEVICE_ID_HI1822_PF 0x1822
> +#endif
Please move it to the .c file where it is actually used.
Extra:
grep -E 'void\ \*' drivers/net/ethernet/huawei/hinic/* makes me nervous.
At some point one function will be fed with a wrong pointer and the
compiler won't notice it.
For instance hinic_sq_read_wqe is only called with &skb. There's no
reason to declare it using a 'void **' argument.
--
Ueimor
Re: [PATCH net] sky2: Do not deadlock on sky2_hw_down
David Miller : > From: Joshua Emele > Date: Wed, 24 May 2017 15:43:18 -0700 [...] > > The sky2_hw_down uses sky2_tx_complete to free pending frames stuck in > > the HW queue. Because sky2_hw_down can be called from a process context, > > the call to u64_stats_update_begin can result in deadlock. > > > > Because the statistics do not require update as part of the sky2_hw_down > > sequence, prevent the update to avoid the deadlock. > > I disagree. Taking the interface down should cause events in the > statistics to be lost. > > You're going to have to find a way to fix this without eliding > the stats increments. NAPI processing is already disabled at this stage in the device close() path (and sky2_netpoll() uses napi_schedule). It's possible to add a conditionnal bh or irq disabling instruction to silent the warning but it should not be needed at all. -- Ueimor
Re: [PATCH v2] net: natsemi: ns83820: add checks for dma mapping error
Alexey Khoroshilov :
[...]
> diff --git a/drivers/net/ethernet/natsemi/ns83820.c
> b/drivers/net/ethernet/natsemi/ns83820.c
> index 729095db3e08..dfc64e1e31f9 100644
> --- a/drivers/net/ethernet/natsemi/ns83820.c
> +++ b/drivers/net/ethernet/natsemi/ns83820.c
[...]
> @@ -1183,6 +1193,32 @@ static netdev_tx_t ns83820_hard_start_xmit(struct
> sk_buff *skb,
> netif_start_queue(ndev);
>
> return NETDEV_TX_OK;
> +
> +dma_error:
> + do {
> + free_idx = (free_idx + NR_TX_DESC - 1) % NR_TX_DESC;
> + desc = dev->tx_descs + (free_idx * DESC_SIZE);
> + cmdsts = le32_to_cpu(desc[DESC_CMDSTS]);
> + len = cmdsts & CMDSTS_LEN_MASK;
> + buf = desc_addr_get(desc + DESC_BUFPTR);
> + if (desc == first_desc)
> + pci_unmap_single(dev->pci_dev,
> + buf,
> + len,
> + PCI_DMA_TODEVICE);
> + else
> + pci_unmap_page(dev->pci_dev,
> + buf,
> + len,
> + PCI_DMA_TODEVICE);
(use tabs + spaces to indent: code should line up right after the parenthesis)
(premature line breaks imho)
(nevermind, both can be avoided :o) )
> + desc[DESC_CMDSTS] = cpu_to_le32(0);
> + mb();
> + } while (desc != first_desc);
> +
> +dma_error_first:
> + dev_kfree_skb_any(skb);
> + ndev->stats.tx_errors++;
^ -> should be tx_dropped
> + return NETDEV_TX_OK;
> }
You only need a single test in the error loop if you mimic the map loop.
Something like:
diff --git a/drivers/net/ethernet/natsemi/ns83820.c
b/drivers/net/ethernet/natsemi/ns83820.c
index 729095d..5e2dbc9 100644
--- a/drivers/net/ethernet/natsemi/ns83820.c
+++ b/drivers/net/ethernet/natsemi/ns83820.c
@@ -1160,9 +1160,11 @@ static netdev_tx_t ns83820_hard_start_xmit(struct
sk_buff *skb,
buf = skb_frag_dma_map(&dev->pci_dev->dev, frag, 0,
skb_frag_size(frag), DMA_TO_DEVICE);
+ if (dma_mapping_error(&dev->pci_dev->dev, buf))
+ goto err_unmap_frags;
dprintk("frag: buf=%08Lx page=%08lx offset=%08lx\n",
(long long)buf, (long) page_to_pfn(frag->page),
frag->page_offset);
len = skb_frag_size(frag);
frag++;
nr_frags--;
@@ -1181,8 +1184,27 @@ static netdev_tx_t ns83820_hard_start_xmit(struct
sk_buff *skb,
/* Check again: we may have raced with a tx done irq */
if (stopped && (dev->tx_done_idx != tx_done_idx) && start_tx_okay(dev))
netif_start_queue(ndev);
-
+out:
return NETDEV_TX_OK;
+
+err_unmap_frags:
+ while (1) {
+ buf = desc_addr_get(desc + DESC_BUFPTR);
+ if (!--nr_frags)
+ break;
+
+ pci_unmap_page(dev->pci_dev, buf, len, PCI_DMA_TODEVICE);
+
+ free_idx = (free_idx - 1) % NR_TX_DESC;
+ desc = dev->tx_descs + (free_idx * DESC_SIZE);
+ len = le32_to_cpu(desc + DESC_CMDSTS) & CMDSTS_LEN_MASK;
+ }
+ pci_unmap_single(dev->pci_dev, buf, len, PCI_DMA_TODEVICE);
+
+err_free_skb:
+ dev_kfree_skb_any(skb);
+ ndev->stats.tx_dropped++;
+ goto out;
}
static void ns83820_update_stats(struct ns83820 *dev)
Thinking more about it, the driver seems rather unsafe if a failing
start_xmit closely follows a succeeding one. The driver should imho
map frags first *then* plug the remaining hole in the descriptor ring.
Until it does, the implicit assumption about descriptor ownership that
the error unroll loop relies on may be wrong.
--
Ueimor
Re: [regression v4.11] 617f01211baf ("8139too: use napi_complete_done()")
David Miller : [...] > One theory is that the interrupt masking isn't working properly > and interrupts are still arriving and hitting the NAPI state even > when we are actively polling NAPI. > > And this problem was masked by the locking done here. Yes. Ville, can you rule out irq sharing between the 8139 and some other device ? It's a candidate for unexpected interrupt handler invocation with older pc, even with properly working hardware. -- Ueimor
Re: net/atm: warning in alloc_tx/__might_sleep
Eric Dumazet :
> On Tue, Jan 10, 2017 at 9:35 AM, Cong Wang wrote:
> > On Mon, Jan 9, 2017 at 9:20 AM, Andrey Konovalov
> > wrote:
> >
> > The fix should be straight-forward. Mind to try the attached patch?
>
>
> You forgot to remove schedule() ?
It may be clearer to split alloc_tx in two parts: only the unsleepable
"if (sk_wmem_alloc_get(sk) && !atm_may_send(vcc, size)) {" part of it
contributes to the inner "while (!(skb = alloc_tx(vcc, eff))) {" block.
See net/atm/common.c
[...]
static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size)
{
struct sk_buff *skb;
struct sock *sk = sk_atm(vcc);
if (sk_wmem_alloc_get(sk) && !atm_may_send(vcc, size)) {
pr_debug("Sorry: wmem_alloc = %d, size = %d, sndbuf = %d\n",
sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
return NULL;
}
while (!(skb = alloc_skb(size, GFP_KERNEL)))
schedule();
pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
atomic_add(skb->truesize, &sk->sk_wmem_alloc);
return skb;
}
The waiting stuff is related to vcc drain but the code makes it look as
if it were also related to skb alloc (it isn't).
It may be obvious for you but it took me a while to figure what the
code is supposed to achieve.
--
Ueimor
Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock
Pavel Machek : [...] > Considering the memory barriers... is something like this neccessary > in the via-rhine ? Yes. > AFAICT... we need a barrier after making sure that descriptor is no > longer owned by DMA (to make sure we don't get stale data in rest of > descriptor)... and we need a barrier before giving the descriptor to > the dma, to make sure DMA engine sees the complete update? I would not expect stale data while processing a single transmit descriptor as the transmit completion does not use the rest of the descriptor at all in the via-rhine driver. However I agree that transmit descriptors should be read by the cpu with adequate ordering so the dma_rmb() should stay. Same kind of narrative for dma_wmb rhine_rx (s/read/written/ and s/cpu/device/). > diff --git a/drivers/net/ethernet/via/via-rhine.c > b/drivers/net/ethernet/via/via-rhine.c > index ba5c542..3806e72 100644 > --- a/drivers/net/ethernet/via/via-rhine.c > +++ b/drivers/net/ethernet/via/via-rhine.c [...] > @@ -2061,6 +2062,7 @@ static int rhine_rx(struct net_device *dev, int limit) > > if (desc_status & DescOwn) > break; > + dma_rmb(); > I agree with your explanation for this one (late vlan processing in a different word from the same descriptor). -- Ueimor
Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock
Pavel Machek : [...] > Won't this up/down the interface, in a way userspace can observe? It won't up/down the interface as it doesn't exactly mimic what the network code does (there's more than rtnl_lock). For the same reason it's broken if it races with the transmit path: it can release driver resources while the transmit path uses these. Btw the points below may not matter/hurt much for a proof a concept but they would need to be addressed as well: 1) unchecked (and avoidable) extra error paths due to stmmac_release() 2) racy cancel_work_sync. Low probability as it is, an irq + error could take place right after cancel_work_sync Lino, have you considered via-rhine.c since its "move work from irq to workqueue context" changes that started in 7ab87ff4c770eed71e3777936299292739fcd0fe [*] ? It's a shameless plug - originated in r8169.c - but it should be rather close to what the sxgbe and friends require. Thought / opinion ? [*] Including fixes/changes in: - 3a5a883a8a663b930908cae4abe5ec913b9b2fd2 - e1efa87241272104d6a12c8b9fcdc4f62634d447 - 810f19bcb862f8889b27e0c9d9eceac9593925dd - e45af497950a89459a0c4b13ffd91e1729fffef4 - a926592f5e4e900f3fa903298c4619a131e60963 - 559bcac35facfed49ab4f408e162971612dcfdf3 -- Ueimor
Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock
Lino Sanfilippo : [...] > OTOH Pavel said that he actually could produce a deadlock. Now I wonder if > this is caused by that locking scheme (in a way I have not figured out yet) > or if it is a different issue. stmmac_tx_err races with stmmac_xmit. -- Ueimor
Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock
Lino Sanfilippo : > The driver uses a private lock for synchronization between the xmit > function and the xmit completion handler, but since the NETIF_F_LLTX flag > is not set, the xmit function is also called with the xmit_lock held. > > On the other hand the xmit completion handler first takes the private lock > and (in case that the tx queue has been stopped) the xmit_lock, leading > to a reverse locking order and the potential danger of a deadlock. netif_tx_stop_queue is used by: 1. xmit function before releasing lock and returning. 2. sxgbe_restart_tx_queue() <- sxgbe_tx_interrupt <- sxgbe_reset_all_tx_queues() <- sxgbe_tx_timeout() Given xmit won't be called again until tx queue is enabled, it's not clear how a deadlock could happen due to #1. Regardless of deadlocks anywhere else, #2 has some serious problem due to the lack of exclusion between the tx queue restart handler and the xmit handler. -- Ueimor
Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable
Mark Lord : [...] > >From tracing through the powerpc arch code, this is the buffer that > is being directly DMA'd into. And the USB layer does an invalidate_dcache > on that entire buffer before initiating the DMA (confirmed via printk). > > The driver itself NEVER writes anything to that buffer, > and nobody else has a pointer to it other than the USB host controller, > so there's nothing else that can write to it either. > > According to the driver writer, the chip should only ever write a fresh > rx_desc struct at the beginning of a buffer, never ASCII data. > > So how does that buffer end up containing ASCII data from the NFS transfers? Through aliasing the URB was given a page that contains said (previously) received file. The ethernet chip/usb host does not write anything in it. There could be a device or a driver problem but it may not be the real problem. So far the analysis focused on "how was this corrupted content written into this receive buffer page ?". If I read David correctly (?) the "nobody else has a pointer to it other than the USB host controller" point may be replaced with "the pointer to it aliases some already used page". -- Ueimor
Re: [PATCH net 2/2] r8152: rx descriptor check
Hayes Wang : > Francois Romieu [mailto:rom...@fr.zoreil.com] > > Sent: Friday, November 11, 2016 8:13 PM > [...] > > Invalid packet size corrupted receive descriptors in Realtek's device > > reminds of CVE-2009-4537. > > Do you mean that the driver would get a packet exceed the size > which is set to RxMaxSize ? If it was possible to get it wrong once, it should be possible to get it wrong twice, especially if some part of the hardware design is recycled. I don't mean anything else. I won't speculate about some cache consistency issue or some badly aborted dma transaction to explain the memory corruption. -- Ueimor
Re: [PATCH net 2/2] r8152: rx descriptor check
Hayes Wang : > For some platforms, the data in memory is not the same with the one > from the device. That is, the data of memory is unbelievable. The > check is used to find out this situation. Invalid packet size corrupted receive descriptors in Realtek's device reminds of CVE-2009-4537. Is the silicium of both devices different enough to prevent the same exploit to happen ? -- Ueimor
Re: [PATCH v2] r8169: set coherent DMA mask as well as streaming DMA mask
Ard Biesheuvel : > PCI devices that are 64-bit DMA capable should set the coherent > DMA mask as well as the streaming DMA mask. On some architectures, > these are managed separately, and so the coherent DMA mask will be > left at its default value of 32 if it is not set explicitly. This > results in errors such as > > r8169 Gigabit Ethernet driver 2.3LK-NAPI loaded > hwdev DMA mask = 0x, dev_addr = 0x0080fbfff000 > swiotlb: coherent allocation failed for device :02:00.0 size=4096 > CPU: 0 PID: 1062 Comm: systemd-udevd Not tainted 4.8.0+ #35 > Hardware name: AMD Seattle/Seattle, BIOS 10:53:24 Oct 13 2016 > > on systems without memory that is 32-bit addressable by PCI devices. > > Signed-off-by: Ard Biesheuvel Acked-by: Francois Romieu Unless someone plans to plug an acenic, a 83820 (pci-e gem board, anyone ?) on top of a pci <-> pci-e adapter on this kind of motherboard, no other network driver that uses the pci_... dma api exhibits this mixed 32 / 64 bit support bug. I haven't checked devices with 32 < mask < 64 nor plain DMA api converted ones. -- Ueimor
Re: [PATCH] via-velocity: remove null pointer check on array tdinfo->skb_dma
Colin King : > From: Colin Ian King > > tdinfo->skb_dma is a 7 element array of dma_addr_t hence cannot be > null, so the pull pointer check on tdinfo->skb_dma is redundant. > Remove it. > > Signed-off-by: Colin Ian King Acked-by: Francois Romieu -- Ueimor
Re: [PATCH] Networking: Core: netpoll: Fixed a missing spin_unlock
Salil Kapur :
> I was looking at v3.12. Can we submit patches for stable versions?
It has already been fixed in 3.12.8 as 56399d8b44beae5b80e1eda0350ab6af72baf4d0
("netpoll: Fix missing TXQ unlock and and OOPS.") by davem.
3.12.8 dates back to 2014/01. The current 3.12.x version is 3.12.62.
--
Ueimor
Re: [PATCH] Networking: Core: netpoll: Fixed a missing spin_unlock
Salil Kapur :
[...]
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index fc75c9e..9124f76 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -386,8 +386,10 @@ void netpoll_send_skb_on_dev(struct netpoll *np, struct
> sk_buff *skb,
>
> !vlan_hw_offload_capable(netif_skb_features(skb),
>
> skb->vlan_proto)) {
> skb = __vlan_put_tag(skb,
> skb->vlan_proto, vlan_tx_tag_get(skb));
> - if (unlikely(!skb))
> + if (unlikely(!skb)) {
> + __netif_tx_unlock(txq);
> break;
> + }
> skb->vlan_tci = 0;
> }
>
Your kernel is outdated: __vlan_put_tag has disappeared from net/core/netpoll.c
since 62749e2cb3c4a7da3eaa5c01a7e787aebeff8536 ("vlan: rename __vlan_put_tag to
vlan_insert_tag_set_proto") by Jiri Pirko somewhere in 2014.
--
Ueimor
Re: [Intel-wired-lan] [e1000_netpoll] BUG: sleeping function called from invalid context at kernel/irq/manage.c:110
Eric Dumazet : [...] > I would prefer having a definitive advice from Thomas Gleixner and/or > others if disable_irq() is forbidden from IRQ path. > > As I said, about all netpoll() methods in net drivers use disable_irq() > so a lot of patches would be needed. s/about all/many/ There has been a WARN_ONCE(!irqs_disabled() in netpoll_send_skb_on_dev for quite some time now but it's apparently screened by too many tests to be effective. :o/ -- Ueimor
Re: [PATCH net 1/3] r8169:fix kernel log spam when set or get hardware wol setting.
Hau : [...] > > Either the driver resumes the device so that it can perform requested > > operation or it signals .set_wol failure when the device is suspended. > > > > If the driver does something else, "spam removal" translates to "silent > > failure". > > Because "tp->saved_wolopts" will be used to set hardware wol capability in > rtl8169_runtime_resume(). So I prefer to keep "wol->wolopts" to > " tp->saved_wolopts " in runtime suspend state and set this to this > "wol->wolopts" to hardware in in rtl8169_runtime_resume(). It would be fine if it could be proven that rtl8169_runtime_resume() will always be run before software state is lost. -- Ueimor
Re: [PATCH net 1/3] r8169:fix kernel log spam when set or get hardware wol setting.
Chunhao Lin :
[...]
> diff --git a/drivers/net/ethernet/realtek/r8169.c
> b/drivers/net/ethernet/realtek/r8169.c
> index 0e62d74..f07604f 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -1749,13 +1749,21 @@ static u32 __rtl8169_get_wol(struct rtl8169_private
> *tp)
> static void rtl8169_get_wol(struct net_device *dev, struct ethtool_wolinfo
> *wol)
> {
> struct rtl8169_private *tp = netdev_priv(dev);
> + struct pci_dev *pdev = tp->pci_dev;
Nit: you may directly use "struct device *d = &tp->pci_dev->dev;"
--
Ueimor
Re: [PATCH net 1/3] r8169:fix kernel log spam when set or get hardware wol setting.
Chunhao Lin : [...] > diff --git a/drivers/net/ethernet/realtek/r8169.c > b/drivers/net/ethernet/realtek/r8169.c > index 0e62d74..f07604f 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c [...] > @@ -1852,12 +1863,17 @@ static int rtl8169_set_wol(struct net_device *dev, > struct ethtool_wolinfo *wol) > tp->features |= RTL_FEATURE_WOL; > else > tp->features &= ~RTL_FEATURE_WOL; > - __rtl8169_set_wol(tp, wol->wolopts); > + if (pm_runtime_active(&pdev->dev)) > + __rtl8169_set_wol(tp, wol->wolopts); > + else > + tp->saved_wolopts = wol->wolopts; > > rtl_unlock_work(tp); > > device_set_wakeup_enable(&tp->pci_dev->dev, wol->wolopts); > > + pm_runtime_put_noidle(&pdev->dev); > + > return 0; Either the driver resumes the device so that it can perform requested operation or it signals .set_wol failure when the device is suspended. If the driver does something else, "spam removal" translates to "silent failure". -- Ueimor
Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)
Netanel Belgazal :
[...]
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h
> > b/drivers/net/ethernet/amazon/ena/ena_com.h
> > new file mode 100644
> > index 000..e49ba43
> > --- /dev/null
> > [...]
> > +static inline void ena_com_update_intr_reg(struct ena_eth_io_intr_reg
> > *intr_reg,
> > + u32 rx_delay_interval,
> > + u32 tx_delay_interval,
> > + bool unmask)
> > +{
> > + intr_reg->intr_control = 0;
> > + intr_reg->intr_control |= rx_delay_interval &
> > + ENA_ETH_IO_INTR_REG_RX_INTR_DELAY_MASK;
> > +
> > + intr_reg->intr_control |=
> > + (tx_delay_interval << ENA_ETH_IO_INTR_REG_TX_INTR_DELAY_SHIFT)
> > + & ENA_ETH_IO_INTR_REG_RX_INTR_DELAY_MASK;
> > ^^ TX ?
> >
> > Extra: you should not return NETDEV_TX_BUSY in the xmit handler while
> > queueing is still enabled. Please drop packet and return NETDEV_TX_OK.
> Ack.
>
> Thanks for your review.
Ack/nack regarding use of ..._RX_INTR_DELAY_MASK with ..._TX_INTR_DELAY_SHIFT ?
--
Ueimor
Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)
Netanel Belgazal :
[...]
More stuff below.
> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c
> b/drivers/net/ethernet/amazon/ena/ena_com.c
> new file mode 100644
> index 000..357e10f
> --- /dev/null
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
[...]
> +static int ena_get_dev_stats(struct ena_com_dev *ena_dev,
> + struct ena_admin_aq_get_stats_cmd *get_cmd,
> + struct ena_admin_acq_get_stats_resp *get_resp,
At first sight it should be possible avoid one pointer argument with:
struct ena_something {
struct ena_admin_aq_get_stats_cmd cmd;
struct ena_admin_acq_get_stats_resp resp;
};
[...]
> +int ena_com_get_dev_extended_stats(struct ena_com_dev *ena_dev, char *buff,
> +u32 len)
Where is it used ?
> +{
> + int ret = 0;
> + struct ena_admin_aq_get_stats_cmd get_cmd;
> + struct ena_admin_acq_get_stats_resp get_resp;
> + void *virt_addr;
int rc = -ENOMEM;
char *str;
> + dma_addr_t phys_addr;
> +
> + virt_addr = dma_alloc_coherent(ena_dev->dmadev,
> +len,
> +&phys_addr,
> +GFP_KERNEL | __GFP_ZERO);
> + if (!virt_addr) {
> + ret = -ENOMEM;
> + goto done;
> + }
> + memset(&get_cmd, 0x0, sizeof(get_cmd));
> + ret = ena_com_mem_addr_set(ena_dev,
> +&get_cmd.u.control_buffer.address,
> +phys_addr);
> + if (unlikely(ret)) {
> + ena_trc_err("memory address set failed\n");
> + return ret;
> + }
> + get_cmd.u.control_buffer.length = len;
> +
> + get_cmd.device_id = ena_dev->stats_func;
> + get_cmd.queue_idx = ena_dev->stats_queue;
> +
> + ret = ena_get_dev_stats(ena_dev, &get_cmd, &get_resp,
> + ENA_ADMIN_GET_STATS_TYPE_EXTENDED);
> + if (ret < 0)
> + goto free_ext_stats_mem;
> +
> + ret = snprintf(buff, len, "%s", (char *)virt_addr);
So ENA_ADMIN_GET_STATS_TYPE_EXTENDED fills a buffer with bytes in the
[0; 127] range, right ?
> +
> +free_ext_stats_mem:
> + dma_free_coherent(ena_dev->dmadev, len, virt_addr, phys_addr);
> +done:
> + return ret;
> +}
> +
> +int ena_com_set_dev_mtu(struct ena_com_dev *ena_dev, int mtu)
> +{
> + struct ena_com_admin_queue *admin_queue;
> + struct ena_admin_set_feat_cmd cmd;
> + struct ena_admin_set_feat_resp resp;
> + int ret = 0;
Should be -ENODEV or left uninitialized.
> +
> + if (unlikely(!ena_dev)) {
> + ena_trc_err("%s : ena_dev is NULL\n", __func__);
> + return -ENODEV;
> + }
Does it mean that ena_com_dev may go away while the net_device is in use ?
> +
> + if (!ena_com_check_supported_feature_id(ena_dev, ENA_ADMIN_MTU)) {
> + ena_trc_info("Feature %d isn't supported\n", ENA_ADMIN_MTU);
> + return -EPERM;
> + }
rc = ena_com_check_supported_feature_id(...
if (rc < 0) {
ena_trc_info(...
goto out;
}
> +
> + memset(&cmd, 0x0, sizeof(cmd));
> + admin_queue = &ena_dev->admin_queue;
> +
> + cmd.aq_common_descriptor.opcode = ENA_ADMIN_SET_FEATURE;
> + cmd.aq_common_descriptor.flags = 0;
> + cmd.feat_common.feature_id = ENA_ADMIN_MTU;
> + cmd.u.mtu.mtu = mtu;
> +
> + ret = ena_com_execute_admin_command(admin_queue,
> + (struct ena_admin_aq_entry *)&cmd,
> + sizeof(cmd),
> + (struct ena_admin_acq_entry *)&resp,
> + sizeof(resp));
> +
> + if (unlikely(ret)) {
> + ena_trc_err("Failed to set mtu %d. error: %d\n", mtu, ret);
> + return -EINVAL;
ena_com_execute_admin_command should return a proper status code.
[...]
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h
b/drivers/net/ethernet/amazon/ena/ena_com.h
new file mode 100644
index 000..e49ba43
--- /dev/null
[...]
+static inline void ena_com_update_intr_reg(struct ena_eth_io_intr_reg
*intr_reg,
+ u32 rx_delay_interval,
+ u32 tx_delay_interval,
+ bool unmask)
+{
+ intr_reg->intr_control = 0;
+ intr_reg->intr_control |= rx_delay_interval &
+ ENA_ETH_IO_INTR_REG_RX_INTR_DELAY_MASK;
+
+ intr_reg->intr_control |=
+ (tx_delay_interval << ENA_ETH_IO_INTR_REG_TX_INTR_DELAY_SHIFT)
+ & ENA_ETH_IO_INTR_REG_RX_INTR_DELAY_MASK;
^^ TX ?
Extra: you should not return NETDEV_TX_BUSY in the xmit handler while
queueing is still enabled. Please drop packet and return NETDEV_TX_OK.
--
Ueimor
Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)
Netanel Belgazal :
[...]
Very limited review below.
> diff --git a/Documentation/networking/ena.txt
> b/Documentation/networking/ena.txt
> new file mode 100644
> index 000..528544f
> --- /dev/null
> +++ b/Documentation/networking/ena.txt
> @@ -0,0 +1,330 @@
> +Linux kernel driver for Elastic Network Adapter (ENA) family:
> +=
> +
> +Overview:
> +=
> +The ENA driver provides a modern Ethernet device interface optimized
> +for high performance and low CPU overhead.
Marketing boilerplate. How much of it will still be true in 6 months ?
6 years ?
Hint: stay technical. If in doubt, make it boring. :o)
[...]
> +ENA devices allow high speed and low overhead Ethernet traffic
> +processing by providing a dedicated Tx/Rx queue pair per host CPU, a
> +dedicated MSI-X interrupt vector per Tx/Rx queue pair, adaptive
> +interrupt moderation, and CPU cacheline optimized data placement.
How many queues exactly ?
[...]
> +Memory Allocations:
> +===
> +DMA Coherent buffers are allocated for the following DMA rings:
> +- Tx submission ring (For regular mode; for LLQ mode it is allocated
> + using kzalloc)
> +- Tx completion ring
> +- Rx submission ring
> +- Rx completion ring
> +- Admin submission ring
> +- Admin completion ring
> +- AENQ ring
Useless documentation (implementation detail).
Nobody will maintain it when the driver changes. Please remove.
[...]
> +MTU:
> +
> +The driver supports an arbitrarily large MTU with a maximum that is
> +negotiated with the device. The driver configures MTU using the
> +SetFeature command (ENA_ADMIN_MTU property). The user can change MTU
> +via ifconfig(8) and ip(8).
Please avoid advertising legacy ifconfig.
[...]
> diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> new file mode 100644
> index 000..8516e5e
> --- /dev/null
> +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
[...]
> +/* admin commands opcodes */
> +enum ena_admin_aq_opcode {
> + /* create submission queue */
> + ENA_ADMIN_CREATE_SQ = 1,
> +
> + /* destroy submission queue */
> + ENA_ADMIN_DESTROY_SQ = 2,
> +
> + /* create completion queue */
> + ENA_ADMIN_CREATE_CQ = 3,
> +
> + /* destroy completion queue */
> + ENA_ADMIN_DESTROY_CQ = 4,
> +
> + /* get capabilities of particular feature */
> + ENA_ADMIN_GET_FEATURE = 8,
> +
> + /* get capabilities of particular feature */
> + ENA_ADMIN_SET_FEATURE = 9,
> +
> + /* get statistics */
> + ENA_ADMIN_GET_STATS = 11,
> +};
The documentation above simply duplicates the member names -> useless
visual payload.
Please replace space with tab before "=" to line things up.
[...]
> +/* Basic Statistics Command. */
> +struct ena_admin_basic_stats {
> + /* word 0 : */
> + u32 tx_bytes_low;
> +
> + /* word 1 : */
> + u32 tx_bytes_high;
> +
> + /* word 2 : */
> + u32 tx_pkts_low;
> +
> + /* word 3 : */
> + u32 tx_pkts_high;
> +
> + /* word 4 : */
> + u32 rx_bytes_low;
> +
> + /* word 5 : */
> + u32 rx_bytes_high;
> +
> + /* word 6 : */
> + u32 rx_pkts_low;
> +
> + /* word 7 : */
> + u32 rx_pkts_high;
> +
> + /* word 8 : */
> + u32 rx_drops_low;
> +
> + /* word 9 : */
> + u32 rx_drops_high;
> +};
struct zorg {
u32 ...;
u32 ...;
u32 ...;
u32 ...;
u32 ...;
u32 ...;
u32 ...;
u32 ...;
u32 ...;
u32 ...;
u32 ...;
};
-> No comment needed to figure the offset.
Comment removed => no need to check if offset starts at word 0 or word 1.
[...]
> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c
> b/drivers/net/ethernet/amazon/ena/ena_com.c
> new file mode 100644
> index 000..357e10f
> --- /dev/null
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
[...]
> +static inline int ena_com_mem_addr_set(struct ena_com_dev *ena_dev,
> +struct ena_common_mem_addr *ena_addr,
> +dma_addr_t addr)
> +{
> + if ((addr & GENMASK_ULL(ena_dev->dma_addr_bits - 1, 0)) != addr) {
> + ena_trc_err("dma address has more bits that the device
> supports\n");
> + return -EINVAL;
> + }
> +
> + ena_addr->mem_addr_low = (u32)addr;
> + ena_addr->mem_addr_high =
> + ((addr & GENMASK_ULL(ena_dev->dma_addr_bits - 1, 32)) >> 32);
No need to "... & GENMASK" given the test above.
> +
> + return 0;
> +}
> +
> +static int ena_com_admin_init_sq(struct ena_com_admin_queue *queue)
> +{
> + queue->sq.entries =
> + dma_alloc_coherent(queue->q_dmadev,
> +ADMIN_SQ_SIZE(queue->q_depth),
> +&queue->sq.dma_addr,
> +GFP_KERNEL | __GFP_ZERO);
1. Use dma_zalloc_coherent().
2. Style
Re: r8169: Unconditionally disabling ASPM
Paul Menzel : [...] > As over five years have passed now, do you think that is still needed? > I wonder why no module parameter was added back then, where users could > enable ASPM if it works on their systems? Because there is no such > situation and it always fails? It was enabled again (d64ec841517a25f6d468bde9f67e5b4cffdc67c7) then disabled (4521e1a94279ce610d3f9b7945c17d581f804242). It's closer to 3.5 years :o) Module parameters are frowned upon. Lin, is there some interest in selectively [*] enabling (or disabling) ASPM support in the r8169 driver or will it be unreliable ? [*] Based on DMI information for instance. -- Ueimor
Re: [PATCH net-next 2/6] atl1c: remove private tx lock
Florian Westphal :
> Francois Romieu wrote:
[...]
> > Play it safe and keep the implicit local_irq_{save / restore} call ?
> >
> > It may not be needed but it will help avoiding any unexpected regression
> > report pointing at the NETDEV_TX_LOCKED removal change.
>
> I thought about that but it doesn't prevent the irq handler from
> running on another CPU, so leaving it around seemed like cargo culting
> to me...
I don't mind removing it in a different patch at all. I'd rather see
the commit history underline that it's unrelated to whatever
NETDEV_TX_LOCKED / LLTX change.
> I don't have an atl1c, but the atl1e in my laptop seems to work fine
> with the (similar) change.
>
> If you disagree I can respin with local_irq_save of course, but, if
> 'playing it safe' is main goal then its simpler to convert
> spin_trylock_irqsave to spin_lock_irqsave.
Your call, really.
--
Ueimor
Re: [PATCH net-next 2/6] atl1c: remove private tx lock
Florian Westphal :
[...]
> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index d0084d4..a3200ea 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
[...]
> @@ -2217,16 +2215,10 @@ static netdev_tx_t atl1c_xmit_frame(struct sk_buff
> *skb,
> }
>
> tpd_req = atl1c_cal_tpd_req(skb);
> - if (!spin_trylock_irqsave(&adapter->tx_lock, flags)) {
> - if (netif_msg_pktdata(adapter))
> - dev_info(&adapter->pdev->dev, "tx locked\n");
> - return NETDEV_TX_LOCKED;
> - }
>
> if (atl1c_tpd_avail(adapter, type) < tpd_req) {
> /* no enough descriptor, just stop queue */
> netif_stop_queue(netdev);
> - spin_unlock_irqrestore(&adapter->tx_lock, flags);
> return NETDEV_TX_BUSY;
> }
>
Play it safe and keep the implicit local_irq_{save / restore} call ?
It may not be needed but it will help avoiding any unexpected regression
report pointing at the NETDEV_TX_LOCKED removal change.
--
Ueimor
Re: [PATCH] net: sxgbe: fix error paths in sxgbe_platform_probe()
Rasmus Villemoes : > We need to use post-decrement to ensure that irq_dispose_mapping is > also called on priv->rxq[0]->irq_no; moreover, if one of the above for > loops failed already at i==0 (so we reach one of these labels with > that value of i), we'll enter an essentially infinite loop of > out-of-bounds accesses. > > Signed-off-by: Rasmus Villemoes (ok, i is signed) Reviewed-by: Francois Romieu Someone messed with my review on 2014/03/25 and got it wrong. :o/ Two years after the initial submission, there is zero change regarding use of sxgbe_core_ops for extension or manageability. The extra indirection is ripe for removal during next net-next. -- Ueimor
Re: SYN flooding on port 80 + DMAR:[DMA Write] faults
Toralf Förster : > Today my server (64 bit hardened Gentoo kernel) was faced a SYN-flood attack. > I do wonder if the DMAR events points to an issue in the kernel ? Please send a compressed log including all 'fault addr' lines as well as the (module probe time) XID line from the r8169 driver. -- Ueimor
Re: [PATCH net] r8169:Remove unnecessary phy reset for pcie nic when setting link spped.
Hau : [...] > Unless pcie nic has bug, pcie nic does not need to reset phy to let phy link > on. > > There is a counter for phy speed down. If phy is in link down state, this > counter will start to count down. When it count to 0, phy will speed down. > Reset phy will reset this counter and prevent phy from speed down. Thanks for the clarification. -- Ueimor
Re: [PATCH net] r8169:Remove unnecessary phy reset for pcie nic when setting link spped.
Chunhao Lin :
> For pcie nic, after setting link speed and thers is no link driver does not
> need
> to do phy reset untill link up.
>
> For some pcie nics, to do this will also reset phy speed down counter and
> prevent
> phy from auto speed down.
>
> This patch fix the issue reported in following link.
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1547151
>
> Signed-off-by: Chunhao Lin
> ---
> drivers/net/ethernet/realtek/r8169.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c
> b/drivers/net/ethernet/realtek/r8169.c
> index dd2cf37..94f08f1 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -1999,7 +1999,8 @@ static int rtl8169_set_speed(struct net_device *dev,
> goto out;
>
> if (netif_running(dev) && (autoneg == AUTONEG_ENABLE) &&
> - (advertising & ADVERTISED_1000baseT_Full)) {
> + (advertising & ADVERTISED_1000baseT_Full) &&
> + !pci_is_pcie(tp->pci_dev)) {
> mod_timer(&tp->timer, jiffies + RTL8169_PHY_TIMEOUT);
> }
> out:
Can you clarify:
- actually this patch does not care about the link at all. So when there's
link no phy reset is needed either, right ?
- does "this" in "to do this" means that
1. phy reset prevents phy from auto speed down
2. avoiding phy reset prevents phy from auto speed down
I would expect 1. from the rtl_wol_pll_power_down + rtl_speed_down +
rtl8169_set_speed combo (i.e. we want the driver to allow auto speed down)
but it's a bit ambiguous.
--
Ueimor
Re: Softirq priority inversion from "softirq: reduce latencies"
Mike Galbraith :
[...]
> Hrm, relatively new + tasklet woes rings a bell. Ah, that..
>
>
> What's worse is that at the point where this code was written it was
> already well known that tasklets are a steaming pile of crap and
> should die.
>
>
> Source thereof https://lwn.net/Articles/588457/
tasklets are ingrained in the dmaengine API (see
Documentation/dmaengine/client.txt
and drivers/dma/virt-dma.h::vchan_cookie_complete).
Moving everything to irq context or handling his own sub-{jiffy/ms} timer
while losing async dma doesn't exactly smell like roses either. :o(
--
Ueimor
Re: [PATCH net 1/3] r8169:fix nic sometimes doesn't work after changing the mac address.
Chunhao Lin :
> When there is no AC power, NIC doesn't work after changing mac address.
> Please refer to following link.
> http://www.spinics.net/lists/netdev/msg356572.html
>
> This issue is caused by runtime power management. When there is no AC power,
> if we
> put NIC down (ifconfig down), the driver will be put in runtime suspend state
> and
> device will in D3 state. In this time, driver cannot access hardware
> regisers. So
> if you set new mac address during this time, it will not work. And then, after
> resume, the NIC will keep using the old mac address and so the network will
> not
> work normally.
>
> In this patch I add detecting runtime pm state when setting mac address. If
> driver is in runtime suspend, I will skip setting mac address and set the new
> mac address during runtime resume.
Instead of taking the device out of suspended mode to perform the required
action, the driver is moving to a model where 1) said action may be scheduled
to a later time - or result from past time work - and 2) rpm handler must
handle a lot of pm unrelated work.
rtl8169_ethtool_ops.{get_wol, get_regs, get_settings} aren't even fixed
yet (what about the .set_xyz handlers ?).
I can't help thinking that the driver should return to a state where it
stupidly does what it is asked to. No software caching, plain device
access, resume when needed, suspend as "suspend" instead of suspend as
"anticipate whatever may happen to avoid waking up".
--
Ueimor
Re: [PATCH net v4] r8169:fix "rtl_counters_cond == 1 (loop: 1000, delay: 10)" log spam.
Chunhao Lin : [...] Fine with me. Is there any chance for the set of chipset dependent registers that are safe to be read when in D3 state to be documented ? -- Ueimor
Re: [PATCH net v3] r8169:fix "rtl_counters_cond == 1 (loop: 1000, delay: 10)" log spam.
Chunhao Lin : [...] > diff --git a/drivers/net/ethernet/realtek/r8169.c > b/drivers/net/ethernet/realtek/r8169.c > index 537974c..404be51 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c [...] > @@ -7853,6 +7859,11 @@ static int rtl8169_runtime_suspend(struct device > *device) > > rtl8169_net_suspend(dev); > > + /* Update counters before going runtime suspend */ > + if (netif_running(dev)) This test is useless (always true): - rtl_open [...] pm_runtime_get_sync(&pdev->dev); [...] tp->TxDescArray = blah [...] - rtl8169_close [...] pm_runtime_get_sync(&pdev->dev); [...] tp->TxDescArray = NULL; - rtl8169_runtime_suspend [...] if (!tp->TxDescArray) return 0; (the implicit smp barriers are mildly obvious, ok) -- Ueimor
Re: [PATCH net v2] r8169:fix "rtl_counters_cond == 1 (loop: 1000, delay: 10)" log spam.
Chunhao Lin :
[...]
> diff --git a/drivers/net/ethernet/realtek/r8169.c
> b/drivers/net/ethernet/realtek/r8169.c
> index 537974c..a645f8d 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -7730,10 +7730,13 @@ rtl8169_get_stats64(struct net_device *dev, struct
> rtnl_link_stats64 *stats)
> {
> struct rtl8169_private *tp = netdev_priv(dev);
> void __iomem *ioaddr = tp->mmio_addr;
> + struct pci_dev *pdev = tp->pci_dev;
+ struct device *d = &tp->pci_dev->dev;
(the patch does not use pdev alone)
[...]
> @@ -7761,7 +7764,8 @@ rtl8169_get_stats64(struct net_device *dev, struct
> rtnl_link_stats64 *stats)
>* Fetch additonal counter values missing in stats collected by driver
>* from tally counters.
>*/
> - rtl8169_update_counters(dev);
> + if (pm_runtime_active(&pdev->dev))
> + rtl8169_update_counters(dev);
pm_runtime_active() won't change after pm_runtime_get_noresume(). You may
set a boolean active = pm_runtime_active(d) before testing netif_running().
[...]
> @@ -7842,6 +7848,12 @@ static int rtl8169_runtime_suspend(struct device
> *device)
> struct pci_dev *pdev = to_pci_dev(device);
> struct net_device *dev = pci_get_drvdata(pdev);
> struct rtl8169_private *tp = netdev_priv(dev);
> + void __iomem *ioaddr = tp->mmio_addr;
> +
> + /* Update counters before going runtime suspend */
> + if (netif_running(dev))
> + rtl8169_rx_missed(dev, ioaddr);
> + rtl8169_update_counters(dev);
>
> if (!tp->TxDescArray)
> return 0;
Nits:
- the tp->TxDescArray test provides the required synchronization: see
rtl8169_{open/close} and their pm_runtime_{get / put}.
- ioaddr is not really needed : tp->mmio_addr appears only once and it does
not mess the 72..80 cols limit.
- even if the device can only be automatically runtime suspended some time
after a link down event, you may address davem's point regarding stats
reliability and move rtl8169_rx_missed + rtl8169_update_counters after
rtl8169_net_suspend.
--
Ueimor
Re: [PATCH net] r8169:fix "rtl_counters_cond == 1 (loop: 1000, delay: 10)" log spam.
Chunhao Lin : [...] > I add checking driver's pm runtime status in rtl8169_get_stats64() to fix > this issue. Would you consider taking the device out of suspended mode during rtl8169_get_stats64 to prevent outdated stats ? -- Ueimor
Re: [PATCH v2] net: ethernet: davicom: fix devicetree irq resource
Robert Jarzmik : > Francois Romieu writes: [...] > > If you have some spare time, it would be nice to avoid db->irq_wake leak > > on probe failure or driver removal. > Sorry but not in this patch. Of course. Different topic => different patch. > I suppose the right patch would be to use devm_*() in the probe function for > ioremaps and request_irqs, which would address this point. I'd rather avoid devm_* on netdev. -- Ueimor
Re: [PATCH v2] net: ethernet: davicom: fix devicetree irq resource
Robert Jarzmik :
[...]
> diff --git a/drivers/net/ethernet/davicom/dm9000.c
> b/drivers/net/ethernet/davicom/dm9000.c
> index cf94b72dbacd..2bae5c8c1f85 100644
> --- a/drivers/net/ethernet/davicom/dm9000.c
> +++ b/drivers/net/ethernet/davicom/dm9000.c
[...]
> @@ -1500,15 +1496,21 @@ dm9000_probe(struct platform_device *pdev)
>
> db->addr_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> db->data_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> - db->irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>
> - if (db->addr_res == NULL || db->data_res == NULL ||
> - db->irq_res == NULL) {
> - dev_err(db->dev, "insufficient resources\n");
> + if (!db->addr_res || !db->data_res) {
> + dev_err(db->dev, "insufficient resources addr=%p data=%p\n",
> + db->addr_res, db->data_res);
> ret = -ENOENT;
> goto out;
> }
>
> + ndev->irq = platform_get_irq(pdev, 0);
> + if (ndev->irq < 0) {
> + dev_err(db->dev, "interrupt resource unavailable: %d\n",
> + ndev->irq);
> + return ndev->irq;
> + }
> +
Leak: please set 'ret = ndev->irq;' and proceed to the 'out' label.
If you have some spare time, it would be nice to avoid db->irq_wake leak
on probe failure or driver removal.
--
Ueimor
Re: [PATCH 3/3] rsi: Replace variable initialisations by assignments in rsi_send_data_pkt()
SF Markus Elfring : > From: Markus Elfring > Date: Sat, 2 Jan 2016 15:25:34 +0100 > > Replace explicit initialisation for two local variables at the beginning > by assignments. It makes no sense for the 'adapter' variable. -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] net-gianfar: Less function calls in gfar_ethflow_to_filer_table() after error detection
Julia Lawall :
> On Fri, 1 Jan 2016, SF Markus Elfring wrote:
[...]
> > > Normally, one returns -ENOMEM for this case, but it looks like this
> > > function is returning 0 on failure.
> >
> > Should a symbol like "false" be used instead of such a special number?
>
> Maybe it's better than 0 and 1...
Your suggestion about -ENOMEM is consistent with the callchain. Nothing
else is needed.
Btw:
1. kfree does not care about NULL parameter, especially in this hardly
timing sensitive path.
2. kmalloc_array for small kernel controlled arrays of integers (see
drivers/net/ethernet/freescale/gianfar.h), seriously ?
I'd suggest the janitor to introduce a dedicated struct to embed both
gfar_private.ftp_rqf{p, c}r then use a single, plain kmalloc in
gfar_ethflow_to_filer_table.
Happy tasteful 2016 :o)
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 1/3] r8169:Fix typo in setting RTL8168EP and RTL8168H D3cold PFM mode
Chunhao Lin : > The register for setting D3code PFM mode is MISC_1, not DLLPR. > > Signed-off-by: Chunhao Lin Reviewed-by: Francois Romieu -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: 4.3+: Atheros ethernet fails after resume from s2ram, due to order 4 allocation
Pavel Machek : [...] > Ok, so what went on is easy.. any ideas how to fix it ? The driver should 1) prohibit holes in its receive ring, 2) allocate before pushing data up in the stack 3) drop packets when it can't allocate a fresh buffer and 4) stop releasing receive buffers - and any resource for that matter - during suspend. Really. -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: severe regression in alx ethernet driver
Jarod Wilson : [...] > They do at least have a signed-off-by in the patches attached to the bug, > so I'm working on touching up the descriptions and formatting, regression > testing them on my laptop that has an alx-driven E2200 in it (which isn't > affected by this bug), and then I can ship them up for proper review. You can forget the last refill-me-harder patch and save a roundtrip. The driver should 1) prohibit holes in its receive ring, 2) allocate before pushing data up in the stack and 3) drop packets when it can't allocate a fresh buffer. -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3 v2] dl2k: Implement suspend
Ondrej Zary :
[...]
> diff --git a/drivers/net/ethernet/dlink/dl2k.c
> b/drivers/net/ethernet/dlink/dl2k.c
> index 9e9baa0..28a96d3 100644
> --- a/drivers/net/ethernet/dlink/dl2k.c
> +++ b/drivers/net/ethernet/dlink/dl2k.c
[...]
> @@ -1824,11 +1831,55 @@ rio_remove1 (struct pci_dev *pdev)
> }
> }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int rio_suspend(struct device *device)
> +{
> + struct net_device *dev = dev_get_drvdata(device);
> + struct netdev_private *np = netdev_priv(dev);
> +
> + if (!netif_running(dev))
> + return 0;
> +
> + netif_device_detach(dev);
> + del_timer_sync(&np->timer);
> + rio_hw_stop(dev);
> +
> + return 0;
> +}
> +
> +static int rio_resume(struct device *device)
> +{
> + struct net_device *dev = dev_get_drvdata(device);
> + struct netdev_private *np = netdev_priv(dev);
> +
> + if (!netif_running(dev))
> + return 0;
> +
> + rio_reset_ring(np);
Almost. You should give the Rx/Tx descriptor 'status' word some love.
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] dl2k: Implement suspend
Ondrej Zary :
[...]
> diff --git a/drivers/net/ethernet/dlink/dl2k.c
> b/drivers/net/ethernet/dlink/dl2k.c
> index 9e9baa0..b53dfa7 100644
> --- a/drivers/net/ethernet/dlink/dl2k.c
> +++ b/drivers/net/ethernet/dlink/dl2k.c
> @@ -1824,11 +1824,57 @@ rio_remove1 (struct pci_dev *pdev)
> }
> }
>
> +#ifdef CONFIG_PM
> +static int rio_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> + struct net_device *dev = pci_get_drvdata(pdev);
> + struct netdev_private *np = netdev_priv(dev);
> +
> + pci_save_state(pdev);
Cargo-cultism ?
> +
> + if (netif_running(dev)) {
> + netif_device_detach(dev);
> + del_timer_sync(&np->timer);
> + rio_hw_stop(dev);
> + free_list(dev);
If free_list is used here, so must alloc_list be in resume, whence
an extra failure opportunity.
You may not need to free both Tx and Rx here.
[...]
> static struct pci_driver rio_driver = {
> .name = "dl2k",
> .id_table = rio_pci_tbl,
> .probe = rio_probe1,
> .remove = rio_remove1,
> +#ifdef CONFIG_PM
> + .suspend= rio_suspend,
> + .resume = rio_resume,
> +#endif /* CONFIG_PM */
It looks a bit old school.
See Documentation/power/pci.txt and drivers/net/ethernet/via/via-rhine.c
for an instance of SIMPLE_DEV_PM_OPS.
At some point you'll probably support runtime power management though.
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] dl2k: Handle memory allocation errors in alloc_list
Ondrej Zary : > If memory allocation fails in alloc_list(), free the already allocated > memory and return -ENODEV. In rio_open(), call alloc_list() first and > abort if it fails. Move HW access (set RFDListPtr) out ot alloc_list(). > > Signed-off-by: Ondrej Zary ENODEV vs ENOMEM aside, it's ok with me. The driver may avoid depleting the receive ring when netdev_alloc_skb_ip_align fails in receive_packet (drop and increase stats->rx_dropped). Then you may replace the pci_* dma helpers with the plain dma_* ones (they can fail). And perform a plain napi conversion. -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] dl2k: Implement suspend
Ondrej Zary :
[...]
> diff --git a/drivers/net/ethernet/dlink/dl2k.c
> b/drivers/net/ethernet/dlink/dl2k.c
> index ccca479..23d13c5 100644
> --- a/drivers/net/ethernet/dlink/dl2k.c
> +++ b/drivers/net/ethernet/dlink/dl2k.c
[..]
> @@ -522,6 +515,28 @@ rio_open (struct net_device *dev)
> macctrl |= (np->tx_flow) ? TxFlowControlEnable : 0;
> macctrl |= (np->rx_flow) ? RxFlowControlEnable : 0;
> dw16(MACCtrl, macctrl);
> +}
> +
> +static int rio_open(struct net_device *dev)
> +{
> + struct netdev_private *np = netdev_priv(dev);
> + const int irq = np->pdev->irq;
> + int i;
> +
> + i = request_irq(irq, rio_interrupt, IRQF_SHARED, dev->name, dev);
> + if (i)
> + return i;
> +
> + alloc_list(dev);
> +
> + rio_hw_init(dev);
- request shared irq
- allocate memory
- init chipset
It may have worked so far but you ought to nail it before your
changes start to look like regression (if shit can happen, it will).
Btw:
- alloc_list should not be allowed to return partially filled receive
ring. Please make it return a proper status code.
The driver itself should rather drop received data than leave holes
in its receive ring.
- the suspend / resume logic does not need a complete free / alloc
cycle. Use the same buffers and reset the cur_tx, cur_rx indexes
as needed.
Go wild.
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Mans Rullgard :
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> new file mode 100644
> index 000..11cd389
> --- /dev/null
> +++ b/drivers/net/ethernet/aurora/nb8800.c
[...]
> +static int nb8800_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
[...]
> +
> + netdev_sent_queue(dev, skb->len);
> +
> + smp_mb__before_spinlock();
> + spin_lock_irqsave(&priv->tx_lock, flags);
At some point you may consider performing both Tx and Rx from napi context
and thus replacing priv->tx_lock with netif_tx_lock.
> +
> + if (!skb->xmit_more) {
> + priv->tx_chain->ready = true;
> + priv->tx_chain = NULL;
> + nb8800_tx_dma_start(dev);
> + }
> +
> + spin_unlock_irqrestore(&priv->tx_lock, flags);
> +
> + priv->tx_next = next;
Are there strong reasons why nb8800_tx_done could not kick between
spin_unlock_irqrestore and the non-local update of priv->tx_next ?
[...]
> +static irqreturn_t nb8800_irq(int irq, void *dev_id)
> +{
> + struct net_device *dev = dev_id;
> + struct nb8800_priv *priv = netdev_priv(dev);
> + u32 val;
> +
> + /* tx interrupt */
> + val = nb8800_readl(priv, NB8800_TXC_SR);
> + if (val) {
[...]
> + }
> +
> + /* rx interrupt */
> + val = nb8800_readl(priv, NB8800_RXC_SR);
> + if (val) {
[...]
> + }
> +
> + return IRQ_HANDLED;
Returning IRQ_HANDLED is fine if one of those hold:
1. you're sure that at least one of the "if" branch is used
2. you'll be able to quickly figure out what's happening whenever 1. stops
being true.
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Måns Rullgård : > Francois Romieu writes: [...] > > It looks like it receives, then tries to allocate new resources. If so > > it may deplete the ring and you should instead consider allocating new > > resources first, then receive data if alloc + map suceeded. > > The hardware receives a frame and stores it in the provided DMA buffer, > then raises an interrupt and moves on to the next buffer. When a buffer > is handed over to the network stack, a new one has to take its place in > the DMA queue. I'm not sure how you're suggesting this be done > differently. - The hardware raises an interrupt and moves on to the next packet - The driver allocates a new buffer - call it Bob - and maps it. - If it succeeds - The driver unmaps the received packet - The driver hands the received packet to the network stack - The driver hands Bob to the hardware - It it fails - The driver updates the dropped packet stats - The driver recycles the received - yet non-unmapped - packet to the hardware -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Mans Rullgard :
[...]
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> new file mode 100644
> index 000..ce61439
> --- /dev/null
> +++ b/drivers/net/ethernet/aurora/nb8800.c
[...]
> +static int nb8800_mdio_read(struct mii_bus *bus, int phy_id, int reg)
> +{
> + struct nb8800_priv *priv = bus->priv;
> + int val;
> +
> + if (!nb8800_mdio_wait(bus))
> + return -ETIMEDOUT;
> +
> + val = MIIAR_ADDR(phy_id) | MIIAR_REG(reg);
> +
> + nb8800_writel(priv, NB8800_MDIO_CMD, val);
> + udelay(10);
> + nb8800_writel(priv, NB8800_MDIO_CMD, val | MDIO_CMD_GO);
> +
> + if (!nb8800_mdio_wait(bus))
> + return -ETIMEDOUT;
You may add a "nb8800_mdio_cmd" helper to factor the lines above in
nb8800_mdio_{read / write}. It could save some bytes.
[...]
> +static void nb8800_mac_tx(struct net_device *dev, bool enable)
> +{
> + struct nb8800_priv *priv = netdev_priv(dev);
> +
> + while (nb8800_readl(priv, NB8800_TXC_CR) & TCR_EN)
> + cpu_relax();
> +
> + if (enable)
> + nb8800_set_bits(b, priv, NB8800_TX_CTL1, TX_EN);
> + else
> + nb8800_clear_bits(b, priv, NB8800_TX_CTL1, TX_EN);
[...]
> + if (enable)
> + nb8800_set_bits(b, priv, NB8800_RX_CTL, RX_EN);
> + else
> + nb8800_clear_bits(b, priv, NB8800_RX_CTL, RX_EN);
[...]
> + if (enable)
> + nb8800_set_bits(b, priv, NB8800_RX_CTL, RX_AF_EN);
> + else
> + nb8800_clear_bits(b, priv, NB8800_RX_CTL, RX_AF_EN);
Factor out ?
[...]
> +static int nb8800_poll(struct napi_struct *napi, int budget)
> +{
> + struct net_device *dev = napi->dev;
> + struct nb8800_priv *priv = netdev_priv(dev);
> + struct nb8800_dma_desc *rx;
> + int work = 0;
> + int last = priv->rx_eoc;
> + int next;
> +
> + while (work < budget) {
> + struct rx_buf *rx_buf;
> + u32 report;
> + int len;
> +
> + next = (last + 1) & (RX_DESC_COUNT - 1);
> +
> + rx_buf = &priv->rx_bufs[next];
> + rx = &priv->rx_descs[next];
> + report = rx->report;
> +
> + if (!report)
> + break;
> +
> + if (IS_RX_ERROR(report)) {
> + nb8800_rx_error(dev, report);
> + } else if (likely(rx_buf->page)) {
> + len = RX_BYTES_TRANSFERRED(report);
> + nb8800_receive(dev, next, len);
> + }
> +
> + rx->report = 0;
> + if (!rx_buf->page)
> + nb8800_alloc_rx(dev, next, true);
It looks like it receives, then tries to allocate new resources. If so
it may deplete the ring and you should instead consider allocating new
resources first, then receive data if alloc + map suceeded.
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] via-rhine: fix VLAN receive handling error in 4.2.x
Andrej Ota :
> Fixes commit 810f19bcb862 ("via-rhine: add consistent memory barrier in
> vlan receive code.") which broke VLAN tag parsing on receive.
>
> Because eth_type_trans() consumes ethernet header worth of bytes, a call
> to read TCI from packet using rhine_rx_vlan_tag() no longer works as it's
> reading from an invalid offset.
>
> Tested to be working on PCEngines Alix board.
>
> Fixes: 810f19bcb862 ("via-rhine: add consistent memory barrier in vlan
> receive code.")
> Signed-off-by: Andrej Ota
Minor nits:
- Ethernet driver patches should usually be sent to net...@vger.kernel.org.
Their state is thus tracked at
http://patchwork.ozlabs.org/project/netdev/list/
- The more specific about the target branch, the better:
[PATCH net 1/1] via-rhine: blah
-> patch against davem's bugfixes branch
[PATCH net-next 19/73] foo: more balh
-> patch against davem's dev branch
- davem will take care of -stable propagation
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: via-rhine: fix VLAN receive handling error in 4.2.x
Andrej :
[...]
> Choosing between changing rhine_get_vlan_tci(), which retrieves TCI from
> skb->data, or moving eth_type_trans() invocation after rhine_rx_vlan_tag(),
> I chose the latter.
Can you send a patch with a proper Signed-off-by and a single line
'Fixes: 810f19bcb862 ("via-rhine: add consistent memory barrier in vlan
receive code.")' or do you want me to do that ?
The patch ought to be based against davem's net but it wouldn't make
any noticeable difference.
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] r8169: Fix sleeping function called during get_stats64
Corinna Vinschen :
[...]
> diff --git a/drivers/net/ethernet/realtek/r8169.c
> b/drivers/net/ethernet/realtek/r8169.c
> index 24dcbe6..c12 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -833,7 +833,8 @@ struct rtl8169_private {
> unsigned features;
>
> struct mii_if_info mii;
> - struct rtl8169_counters counters;
> + dma_addr_t CntPhysAddr;
> + struct rtl8169_counters *CntArray;
I'd rather see less camel case than more.
[...]
> @@ -2245,32 +2230,13 @@ static bool rtl8169_reset_counters(struct net_device
> *dev)
> if (tp->mac_version < RTL_GIGA_MAC_VER_19)
> return true;
>
> - counters = rtl8169_map_counters(dev, &paddr, CounterReset);
> - if (!counters)
> - return false;
> -
> - if (!rtl_udelay_loop_wait_low(tp, &rtl_reset_counters_cond, 10, 1000))
> - ret = false;
> -
> - rtl8169_unmap_counters(dev, paddr, counters);
> -
> - return ret;
> -}
> -
> -DECLARE_RTL_COND(rtl_counters_cond)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - return RTL_R32(CounterAddrLow) & CounterDump;
> +return rtl8169_do_counters(dev, CounterReset);
spaces instead of tab.
[...]
> @@ -8483,7 +8451,10 @@ static int rtl_init_one(struct pci_dev *pdev, const
> struct pci_device_id *ent)
> out:
> return rc;
>
> -err_out_msi_4:
> +err_out_msi_5:
> + dma_free_coherent(&pdev->dev, sizeof(*tp->CntArray), tp->CntArray,
> + tp->CntPhysAddr);
> +err_out_cnt_4:
> netif_napi_del(&tp->napi);
These labels are supposed to suggest what should be done.
Acked-by: Francois Romieu
Tested on 8168b (RTL_GIGA_MAC_VER_17).
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net] r8169: Fix sleeping function called during get_stats64
Corinna Vinschen : [...] > diff --git a/drivers/net/ethernet/realtek/r8169.c > b/drivers/net/ethernet/realtek/r8169.c > index 24dcbe6..630811a 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c [...] > + if (!paddr) > + return false; I guess this is the secret recipe. What about replacing this part with an adequate barrier in rtl_init_one ? [...] > @@ -8447,9 +8411,14 @@ static int rtl_init_one(struct pci_dev *pdev, const > struct pci_device_id *ent) > > tp->rtl_fw = RTL_FIRMWARE_UNKNOWN; > > + tp->CntArray = dma_alloc_coherent (&pdev->dev, sizeof(*tp->CntArray), > +&tp->CntPhysAddr, GFP_KERNEL); > + if (!tp->CntArray) > + goto err_out_cnt_4; > + rc is still zero here so rtl_init_one will return success. > rc = register_netdev(dev); > if (rc < 0) > - goto err_out_msi_4; > + goto err_out_msi_5; > > pci_set_drvdata(pdev, dev); -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC, RFT PATCH 1/2] dl2k: Add support for IP1000A-based cards
Ondrej Zary : [...] > Actually, gigabit works with this patch. The "PHY magic" part contains > mii_write(9, 0x0700) which makes gigabit work. It'd be extatic if you could set mii->supports_gmii for the appropriate PHY so that dl2k.c::rio_get_settings returns sensible gigabit status (note: neither ipg.c nor dl2k.c currently does). -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: ipg and dl2k mess
Ondrej Zary : [...] > The patch below is enough to make my IP1000A card work with dl2k driver - no > more lost packets and hangs. Haven't tested gigabit speed yet - the PHY will > probably need some tweaking but that should be easy. Neither dl2k nor ipg uses napi. They are a bit dusty. > So maybe we should add IP1000A support to dl2k and remove the broken ipg > driver. Do you mean to merge both ? > Does anyone have HW to test? I have a PCI 32 IC Plus IP1000A at hand (courtesy of IC Plus Corp). -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sky2: Add module parameter for passing the MAC address
Liviu Dudau : > On Wed, Aug 05, 2015 at 05:40:57PM +0100, Stephen Hemminger wrote: [...] > > Yes, I can see that this can be a real problem, and other drivers > > solve the problem. The standard method is to assign a random mac address > > (and then let scripts overwrite) rather than introducing module parameter. > > Module parameters are discouraged because they are device specific. > > > > I agree. However, in my case, the boards people have assigned MAC addresses > to the chip, they just didn't built the board in such a way as to allow one > to store that MAC address in a permanent way :( And no, I can't use the DT > because the chip is actually on the PCIe bus. > > Even with the generation of a random address, it still needs to be copied > into the device, so I would guess that a version of the patch I've sent is > still relevant? Assuming a random address is generated, could you elaborate what is needed that sky2_set_mac_address fails to provide ? -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] wan: dscc4: use msecs_to_jiffies for conversions
David Miller : [...] > Whoever wrote these things probably wanted whatever this amounts > to when HZ=100, so that is the only valid transformation you can > make to fix this up here. It's linux kernel illiterate style from 13 years ago but I commented dscc4_pci_reset. wait_ack_cec and xpr_ack are dumb busy loops that break on short integer sign change. Hackish hint. > Otherwise you seriously risk breaking the driver. How seriously is hard to tell. I've never tried it at really low line rates though. Please apply. Thanks. -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [net:r8169] kernel 4.0.0 double "link down" on boot
rh_ : > I see these double "link down" now in 4.0 > > [ 19.927880] r8169 :03:00.2 eth0: link down > [ 19.927902] r8169 :03:00.2 eth0: link down > [ 22.32] r8169 :03:00.2 eth0: link up > > Everything seems to be working fine. Is this an indicator of a problem? No. > Safe to ignore? Mostly. > If so why? The driver is notified of a link event. It schedules a link check. The link may be back to its original state before the driver notices the change. As long as you don't experience high frequency link events for an extended duration, you should not care. [...] > Does it make sense to check 3.19 ? No. -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1] mv643xx_eth: only account for work done in rxq_process in poll callback.
Nicolas Schichan :
[...]
> I was trying to minimize the code changes wrt the current source, but if you
> want that change to be in the same patch, I can certainly respin a V2 with it.
No need to respin, you elegantly minimized the changes. The body of the
while loop could be more spartan, especially with bql in sight. Say:
@@ -1041,52 +1041,37 @@ out:
mp->work_tx_end &= ~(1 << txq->index);
}
-static int txq_reclaim(struct tx_queue *txq, int budget, int force)
+static void txq_reclaim(struct tx_queue *txq, int force)
{
struct mv643xx_eth_private *mp = txq_to_mp(txq);
- struct netdev_queue *nq = netdev_get_tx_queue(mp->dev, txq->index);
- int reclaimed;
+ struct net_device *dev = mp->dev;
+ struct netdev_queue *nq = netdev_get_tx_queue(dev, txq->index);
+ int used = txq->tx_used_desc;
+ int count;
- __netif_tx_lock_bh(nq);
+ mp->work_tx &= ~(1 << txq->index);
- reclaimed = 0;
- while (reclaimed < budget && txq->tx_desc_count > 0) {
- int tx_index;
- struct tx_desc *desc;
- u32 cmd_sts;
- char desc_dma_map;
-
- tx_index = txq->tx_used_desc;
- desc = &txq->tx_desc_area[tx_index];
- desc_dma_map = txq->tx_desc_mapping[tx_index];
+ __netif_tx_lock(nq, smp_processor_id());
- cmd_sts = desc->cmd_sts;
+ for (count = txq->tx_desc_count; count > 0; count--) {
+ struct tx_desc *desc = &txq->tx_desc_area[used];
+ char desc_dma_map = txq->tx_desc_mapping[used];
- if (cmd_sts & BUFFER_OWNED_BY_DMA) {
+ if (desc->cmd_sts & BUFFER_OWNED_BY_DMA) {
if (!force)
break;
- desc->cmd_sts = cmd_sts & ~BUFFER_OWNED_BY_DMA;
+ desc->cmd_sts &= ~BUFFER_OWNED_BY_DMA;
}
- txq->tx_used_desc = tx_index + 1;
- if (txq->tx_used_desc == txq->tx_ring_size)
- txq->tx_used_desc = 0;
-
- reclaimed++;
- txq->tx_desc_count--;
+ used = (used + 1) % txq->tx_ring_size;
if (!IS_TSO_HEADER(txq, desc->buf_ptr)) {
-
if (desc_dma_map == DESC_DMA_MAP_PAGE)
- dma_unmap_page(mp->dev->dev.parent,
- desc->buf_ptr,
- desc->byte_cnt,
- DMA_TO_DEVICE);
+ dma_unmap_page(dev->dev.parent, desc->buf_ptr,
+ desc->byte_cnt, DMA_TO_DEVICE);
else
- dma_unmap_single(mp->dev->dev.parent,
-desc->buf_ptr,
-desc->byte_cnt,
-DMA_TO_DEVICE);
+ dma_unmap_single(dev->dev.parent, desc->buf_ptr,
+desc->byte_cnt, DMA_TO_DEVICE);
}
if (cmd_sts & TX_ENABLE_INTERRUPT) {
@@ -1097,18 +1082,15 @@ static int txq_reclaim(struct tx_queue *txq, int
budget, int force)
}
if (cmd_sts & ERROR_SUMMARY) {
- netdev_info(mp->dev, "tx error\n");
- mp->dev->stats.tx_errors++;
+ netdev_info(dev, "tx error\n");
+ dev->stats.tx_errors++;
}
-
}
- __netif_tx_unlock_bh(nq);
-
- if (reclaimed < budget)
- mp->work_tx &= ~(1 << txq->index);
+ txq->tx_used_desc = used;
+ txq->tx_desc_count = count;
- return reclaimed;
+ __netif_tx_unlock(nq);
}
Nobody uses txq_reclaim return status code. You can turn it void.
[...]
> > work_tx is also updated in irq context. I'd rather see "clear_flag() then
> > reclaim()" than "reclaim() then clear_flag()" in a subsequent patch.
>
> Just to be sure that I understand the issue here, under normal conditions,
> work_tx is updated in irq context via mv643xx_eth_collect_events() and then
> the mv643xx interrupts are masked and napi_schedule() is called. Only once all
> the work has been completed in the poll callback and the work flags have been
> cleared, are the interrupt unmasked and napi_complete() is called. As far as I
> can see there should be no issue here.
IRQF_SHARED and mv643xx_eth_netpoll make me marginally nervous.
My concern was mostly stylistic given the current code. Things get
easier - call me lazy - if the bit is cleared before reclaiming.
> The only problem I can see is in OOM condition when napi_schedule is called
> from a timer callback (oom_timer_wrapper()) which will result in the poll
> callback being
Re: [PATCH v1] mv643xx_eth: only account for work done in rxq_process in poll callback.
Nicolas Schichan :
[...]
> diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c
> b/drivers/net/ethernet/marvell/mv643xx_eth.c
> index 1c75829..52bc56b 100644
> --- a/drivers/net/ethernet/marvell/mv643xx_eth.c
> +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
[...]
> @@ -1050,7 +1049,7 @@ static int txq_reclaim(struct tx_queue *txq, int
> budget, int force)
> __netif_tx_lock_bh(nq);
>
> reclaimed = 0;
> - while (reclaimed < budget && txq->tx_desc_count > 0) {
> + while (txq->tx_desc_count > 0) {
> int tx_index;
> struct tx_desc *desc;
> u32 cmd_sts;
You may use a local 'int count = txq->tx_desc_count' variable then
perform a single update at the end of the locked section.
txq->tx_used_desc could be reworked in a similar way.
> @@ -1105,8 +1104,7 @@ static int txq_reclaim(struct tx_queue *txq, int
> budget, int force)
>
> __netif_tx_unlock_bh(nq);
>
> - if (reclaimed < budget)
> - mp->work_tx &= ~(1 << txq->index);
> + mp->work_tx &= ~(1 << txq->index);
>
> return reclaimed;
> }
work_tx is also updated in irq context. I'd rather see "clear_flag() then
reclaim()" than "reclaim() then clear_flag()" in a subsequent patch.
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] atheros/atlx: Simplify bit manipulations
Rasmus Villemoes : [...] > diff --git a/drivers/net/ethernet/atheros/atlx/atl2.c > b/drivers/net/ethernet/atheros/atlx/atl2.c > index 84a09e8ddd9c..46d1b959daa8 100644 > --- a/drivers/net/ethernet/atheros/atlx/atl2.c > +++ b/drivers/net/ethernet/atheros/atlx/atl2.c > @@ -1278,14 +1278,10 @@ static void atl2_setup_pcicmd(struct pci_dev *pdev) > > pci_read_config_word(pdev, PCI_COMMAND, &cmd); > > - if (cmd & PCI_COMMAND_INTX_DISABLE) > - cmd &= ~PCI_COMMAND_INTX_DISABLE; > - if (cmd & PCI_COMMAND_IO) > - cmd &= ~PCI_COMMAND_IO; > - if (0 == (cmd & PCI_COMMAND_MEMORY)) > - cmd |= PCI_COMMAND_MEMORY; > - if (0 == (cmd & PCI_COMMAND_MASTER)) > - cmd |= PCI_COMMAND_MASTER; > + cmd &= ~PCI_COMMAND_INTX_DISABLE; > + cmd &= ~PCI_COMMAND_IO; > + cmd |= PCI_COMMAND_MEMORY; > + cmd |= PCI_COMMAND_MASTER; > pci_write_config_word(pdev, PCI_COMMAND, cmd); Mostly open-coded pci_set_master, pci_enable_device_mem and pci_intx. I'd suggest to ignore the PCI_COMMAND_IO bit at all then use the standard pci helpers. -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ethernet: atheros: Add nss-gmac driver
Stephen Wang :
> This driver adds support for the internal GMACs on IPQ806x SoCs.
> It supports the device-tree and will register up to 4 ethernet
> interfaces.
>
> Signed-off-by: Stephen Wang
> ---
[...]
> +/*
> + * NSS GMAC data plane ops, default would be slowpath and can be override by
> + * nss-drv
> + */
> +struct nss_gmac_data_plane_ops {
> + int (*open)(void *ctx, uint32_t tx_desc_ring, uint32_t rx_desc_ring,
> + uint32_t mode);
> + int (*close)(void *ctx);
> + int (*link_state)(void *ctx, uint32_t link_state);
> + int (*mac_addr)(void *ctx, uint8_t *addr);
> + int (*change_mtu)(void *ctx, uint32_t mtu);
> + int (*xmit)(void *ctx, struct sk_buff *os_buf);
> +};
Weak types.
[...]
> +/*
> + * nss_gmac_register_offload()
> + *
> + * @param[netdev] netdev instance that is going to register
> + * @param[dp_ops] dataplan ops for chaning mac addr/mtu/link status
> + * @param[ctx] passing the ctx of this nss_phy_if to gmac
> + *
> + * @return Return SUCCESS or FAILURE
> + */
> +int nss_gmac_override_data_plane(struct net_device *netdev,
> + struct nss_gmac_data_plane_ops *dp_ops,
> + void *ctx)
> +{
> + struct nss_gmac_dev *gmacdev = (struct nss_gmac_dev
> *)netdev_priv(netdev);
> +
> + BUG_ON(!gmacdev);
> +
> + if (!dp_ops->open || !dp_ops->close || !dp_ops->link_state
> + || !dp_ops->mac_addr || !dp_ops->change_mtu || !dp_ops->xmit) {
> + netdev_dbg(netdev, "%s: All the op functions must be present,
> reject this registeration",
> + __func__);
> + return NSS_GMAC_FAILURE;
> + }
> +
> + /*
> + * If this gmac is up, close the netdev to force TX/RX stop
> + */
> + if (test_bit(__NSS_GMAC_UP, &gmacdev->flags))
> + nss_gmac_linux_close(netdev);
> +
> + /* Recored the data_plane_ctx, data_plane_ops */
> + gmacdev->data_plane_ctx = ctx;
> + gmacdev->data_plane_ops = dp_ops;
> + gmacdev->first_linkup_done = 0;
> +
> + return NSS_GMAC_SUCCESS;
> +}
> +EXPORT_SYMBOL(nss_gmac_override_data_plane);
Why is this hook needed ?
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 0/9] r8169:update hardware ephy parameter
Chunhao Lin : > Update hardware ephy parameter to improve pcie compatibility. Neither the code nor the commit message helps to figure which kind of behavioral change should/could be expected. Bandwidth ? Latency ? Power management ? Compliance ? Stability ? -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 7/9] r8169:update rtl8168dp ephy parameter
Chunhao Lin :
[...]
> diff --git a/drivers/net/ethernet/realtek/r8169.c
> b/drivers/net/ethernet/realtek/r8169.c
> index a979519..42eda35 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
[...]
> static void rtl_hw_start_8168d_4(struct rtl8169_private *tp)
> {
> void __iomem *ioaddr = tp->mmio_addr;
> struct pci_dev *pdev = tp->pci_dev;
> static const struct ephy_info e_info_8168d_4[] = {
> - { 0x0b, ~0, 0x48 },
> - { 0x19, 0x20, 0x50 },
> - { 0x0c, ~0, 0x20 }
> + { 0x0b, 0x, 0x0048 },
> + { 0x19, 0x0020, 0x0050 },
> + { 0x0c, 0x0100, 0x0020 },
> + { 0x10, 0x0004, 0x }
> };
> - int i;
>
> rtl_csi_access_enable_1(tp);
>
> - rtl_tx_performance_tweak(pdev, 0x5 << MAX_READ_REQUEST_SHIFT);
> + if (tp->dev->mtu <= ETH_DATA_LEN)
> + rtl_tx_performance_tweak(pdev, 0x5 << MAX_READ_REQUEST_SHIFT);
[...]
> @@ -6328,11 +6308,8 @@ static void rtl_hw_start_8168(struct net_device *dev)
> break;
>
> case RTL_GIGA_MAC_VER_28:
> - rtl_hw_start_8168d_4(tp);
> - break;
> -
> case RTL_GIGA_MAC_VER_31:
> - rtl_hw_start_8168dp(tp);
> + rtl_hw_start_8168d_4(tp);
RTL_GIGA_MAC_VER_28 would thus use a mtu dependant rtl_tx_performance_tweak
in its hw_start handler but would not include one in its jumbo_ops helpers.
It does not seem completely right.
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 5/9] r8169:update rtl8168fb ephy parameter
Chunhao Lin : [rtl_hw_start_8168f_1 and rtl_hw_start_8168f_2 changes] Different ephy_info data, same code. You may consider factoring it out. -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] r8169: Use load_acquire() and store_release() to reduce memory barrier overhead
Alexander Duyck : > On 11/13/2014 01:30 PM, Francois Romieu wrote: > > Alexander Duyck : > > [...] > >> In addition the r8169 uses a rmb() however I believe it is placed > >> incorrectly > >> as I assume it supposed to be ordering descriptor reads after the check for > >> ownership. > > Not exactly. It's a barrier against compiler optimization from 2004. > > It should not matter. > > Okay. Do you recall the kind of problem it was you were seeing ? Mildly, I had to grep the local archives. The relevant code used to be included in the irq handler at that time (napi support for this driver took place in may 2004). One did not want a runaway loop in the Tx reaper. Compiler optimization was suggested by Manfred Spraul in the thread below: http://marc.info/?l=linux-kernel&m=108096868119004 > The origin of the rmb() for the Intel drivers was a PowerPC issue in > which it was fetching the length of a buffer before it checked the DD > bit (equivalent of DescOwn). I'm wondering if the issue you were seeing > was something similar where it had reordered reads in the descriptor to > cause that type of result. The problem was only reported on Intel 32 bit + slackware + gcc 3.2.3. Adam Nielsen - Cc: added - did not return for this bug. -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] r8169: Use load_acquire() and store_release() to reduce memory barrier overhead
Alexander Duyck : [...] > In addition the r8169 uses a rmb() however I believe it is placed incorrectly > as I assume it supposed to be ordering descriptor reads after the check for > ownership. Not exactly. It's a barrier against compiler optimization from 2004. It should not matter. However I disagree with the change below: > @@ -7284,11 +7280,11 @@ static int rtl_rx(struct net_device *dev, struct > rtl8169_private *tp, u32 budget > struct RxDesc *desc = tp->RxDescArray + entry; > u32 status; > > - rmb(); > - status = le32_to_cpu(desc->opts1) & tp->opts1_mask; > - > + status = cpu_to_le32(load_acquire(&desc->opts1)); > if (status & DescOwn) > break; > + > + status &= tp->opts1_mask; -> tp->opts1_mask is not __le32 tainted. Btw, should I consider the sketch above as a skeleton in my r8169 closet ? NIC CPU0 CPU1 | CPU | NIC | CPU | CPU | | CPU | NIC | CPU | CPU | ^ tx_dirty [start_xmit... | CPU | CPU | CPU | CPU | (NIC did it's job) [rtl_tx... | ... | ... | NIC | NIC | (ring update) (tx_dirty increases) | CPU | CPU | ??? | ??? | tx_dirty ? reaping about-to-be-sent buffers on some platforms ? ...start_xmit] -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: e100: Laptop battery drain and WoL settings from EEPROM
Ondrej Zary : [...] > > Looks like this laptop is probably WoL-capable even on battery. > > > > Measured the current from AC adapter: > > around 20mA with WoL inactive (shut down from Windows or by power button in > > GRUB) around 40mA with WoL active (shut down from Linux) > > > > So to work-around this problem, users must disable WoL manually on each > > boot. Or configure udev so that ethtool disables WoL. The current policy has been here for ages. Some users may rely on it. There must be some strong rationale for their setup to have to be changed. -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next v2 2/3] r8152: clear theflagofSCHEDULE_TASKLETin tasklet
Hayes Wang : > Francois Romieu [mailto:rom...@fr.zoreil.com] > > Sent: Monday, November 03, 2014 6:53 AM > [...] > > test_and_clear_bit (dense) or clear_bit would be more idiomatic. > > Excuse me. Any suggestion? > Should I use clear_bit directly, or something else? > Or, do I have to remove this patch? The performance explanation leaves me a bit unconvinced. Without any figure one could simply go for the always locked clear_bit because of: 1. the "I'm racy" message that the open-coded test + set sends 2. the extra work needed to avoid 1 (comment, explain, ...). The extra time could thus be used to see what happens when napi is shoehorned in this tasklet machinery. I'd naively expect it to be relevant for efficiency. I won't mind if your agenda is completely different. :o) -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next v2 2/3] r8152: clear the flagofSCHEDULE_TASKLET in tasklet
Hayes Wang : > David Miller [da...@davemloft.net] [...] > > If another thread of control sets the bit between the test and the > > clear, you will lose an event. > > It is fine. The flag is used to schedule a tasklet, so if the tasklet is > starting running, all the other plans for scheduling a tasklet could > be cleared. test_and_clear_bit (dense) or clear_bit would be more idiomatic. -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 net-next] r8169:add support for RTL8168EP
Hau :
[...]
> Do you mean I should collect similar hardware parameters setting into one
> function ? or I should set hardware parameters according to hardware
> feature support version?
static void r8168dp_ocp_write(...)
[...]
static void r8168ep_ocp_write(...)
[...]
static void ocp_write(...)
{
switch (...
case ...
r8168dp_ocp_write
case ...
r8168ep_ocp_write
[...]
static void rtl8168dp_driver_start(...)
[...]
static void rtl8168ep_driver_start(...)
[...]
etc.
Nothing more. At some point the helpers themselves may turn into data
struct members. Things don't need to be immediately right - if ever.
However you really want to avoid unrelated changes in your patches:
shuffling code and changing features at the same time hurts reviews,
late regression hunts, backports, etc.
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 net-next] r8169:add support for RTL8168EP
Chun-Hao Lin :
[...]
> diff --git a/drivers/net/ethernet/realtek/r8169.c
> b/drivers/net/ethernet/realtek/r8169.c
> index 54476ba..3efdf4d 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
[...]
> @@ -1276,6 +1273,52 @@ static void rtl_w0w1_eri(struct rtl8169_private *tp,
> int addr, u32 mask, u32 p,
> rtl_eri_write(tp, addr, mask, (val & ~m) | p, type);
> }
>
> +static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg)
> +{
> + void __iomem *ioaddr = tp->mmio_addr;
> +
> + switch (tp->mac_version) {
> + case RTL_GIGA_MAC_VER_27:
> + case RTL_GIGA_MAC_VER_28:
> + case RTL_GIGA_MAC_VER_31:
> + RTL_W32(OCPAR, ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> + return rtl_udelay_loop_wait_high(tp, &rtl_ocpar_cond, 100, 20)
> + ? RTL_R32(OCPDR) : ~0;
'?' should be on the previous line. There isn't more room than needed but
it's still ok.
[...]
> +static void rtl8168ep_2_hw_phy_config(struct rtl8169_private *tp)
> +{
[...]
> + rtl_w0w1_phy(tp, 0x14, 0x7C00, ~0x7Cff);
Nit:rtl_w0w1_phy(tp, 0x14, 0x7c00, ~0x7cff);
[...]
+ rtl_writephy(tp, 0x1f, 0x0bC8);
Nit:rtl_writephy(tp, 0x1f, 0x0bc8);
Tangent: please find feature wise identical code below without moves.
I have avoided introducing more helpers that may have kept the code
more balanced for 8168dp vs 8168ep as it wasn't the point and it is
still possible to change the code from there.
It provides a different review experience. Feel free to give it a thought.
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 54476ba..54cd116 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -155,6 +155,9 @@ enum mac_version {
RTL_GIGA_MAC_VER_46,
RTL_GIGA_MAC_VER_47,
RTL_GIGA_MAC_VER_48,
+ RTL_GIGA_MAC_VER_49,
+ RTL_GIGA_MAC_VER_50,
+ RTL_GIGA_MAC_VER_51,
RTL_GIGA_MAC_NONE = 0xff,
};
@@ -302,6 +305,15 @@ static const struct {
[RTL_GIGA_MAC_VER_48] =
_R("RTL8107e", RTL_TD_1, FIRMWARE_8107E_2,
JUMBO_1K, false),
+ [RTL_GIGA_MAC_VER_49] =
+ _R("RTL8168ep/8111ep", RTL_TD_1, NULL,
+ JUMBO_9K, false),
+ [RTL_GIGA_MAC_VER_50] =
+ _R("RTL8168ep/8111ep", RTL_TD_1, NULL,
+ JUMBO_9K, false),
+ [RTL_GIGA_MAC_VER_51] =
+ _R("RTL8168ep/8111ep", RTL_TD_1, NULL,
+ JUMBO_9K, false),
};
#undef _R
@@ -400,6 +412,10 @@ enum rtl_registers {
FuncEvent = 0xf0,
FuncEventMask = 0xf4,
FuncPresetState = 0xf8,
+ IBCR0 = 0xf8,
+ IBCR2 = 0xf9,
+ IBIMR0 = 0xfa,
+ IBISR0 = 0xfb,
FuncForceEvent = 0xfc,
};
@@ -467,6 +483,7 @@ enum rtl8168_registers {
#define ERIAR_EXGMAC (0x00 << ERIAR_TYPE_SHIFT)
#define ERIAR_MSIX (0x01 << ERIAR_TYPE_SHIFT)
#define ERIAR_ASF (0x02 << ERIAR_TYPE_SHIFT)
+#define ERIAR_OOB (0x02 << ERIAR_TYPE_SHIFT)
#define ERIAR_MASK_SHIFT 12
#define ERIAR_MASK_0001(0x1 << ERIAR_MASK_SHIFT)
#define ERIAR_MASK_0011(0x3 << ERIAR_MASK_SHIFT)
@@ -942,7 +959,7 @@ DECLARE_RTL_COND(rtl_ocpar_cond)
return RTL_R32(OCPAR) & OCPAR_FLAG;
}
-static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg)
+static u32 r8168dp_ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg)
{
void __iomem *ioaddr = tp->mmio_addr;
@@ -952,7 +969,8 @@ static u32 ocp_read(struct rtl8169_private *tp, u8 mask,
u16 reg)
RTL_R32(OCPDR) : ~0;
}
-static void ocp_write(struct rtl8169_private *tp, u8 mask, u16 reg, u32 data)
+static void r8168dp_ocp_write(struct rtl8169_private *tp, u8 mask, u16 reg,
+ u32 data)
{
void __iomem *ioaddr = tp->mmio_addr;
@@ -1276,6 +1294,43 @@ static void rtl_w0w1_eri(struct rtl8169_private *tp, int
addr, u32 mask, u32 p,
rtl_eri_write(tp, addr, mask, (val & ~m) | p, type);
}
+static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg)
+{
+ switch (tp->mac_version) {
+ case RTL_GIGA_MAC_VER_27:
+ case RTL_GIGA_MAC_VER_28:
+ case RTL_GIGA_MAC_VER_31:
+ return r8168dp_ocp_read(tp, mask, reg);
+ case RTL_GIGA_MAC_VER_49:
+ case RTL_GIGA_MAC_VER_50:
+ case RTL_GIGA_MAC_VER_51:
+ return rtl_eri_read(tp, reg, ERIAR_OOB);
+ default:
+ BUG();
+ return ~0;
+ }
+}
+
+static void ocp_write(struct rtl8169_private *tp, u8 mask, u16 reg, u32 data)
+{
+
Re: [PATCH net-next 07/10] r8169:change function name of function "rtl_w1w0_eri"
Chun-Hao Lin : > Change the name of this function to "rtl_w0_w1_eri". > It is more suitable for this function's behavior. Afaiks it used to follow the same rule as the one I outlined in the comment to #6/10 for rtl_w0_w1_phy. Could you elaborate (or say so if it should be clear for me after some sleep) ? [...] > diff --git a/drivers/net/ethernet/realtek/r8169.c > b/drivers/net/ethernet/realtek/r8169.c > index e68fe5e..e79def5 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c [...] > @@ -1629,14 +1629,14 @@ static void __rtl8169_set_wol(struct rtl8169_private > *tp, u32 wolopts) > case RTL_GIGA_MAC_VER_48: > tmp = ARRAY_SIZE(cfg) - 1; > if (wolopts & WAKE_MAGIC) > - rtl_w1w0_eri(tp, > + rtl_w0w1_eri(tp, >0x0dc, >ERIAR_MASK_0100, >MagicPacket_v2, >0x, >ERIAR_EXGMAC); > else > - rtl_w1w0_eri(tp, > + rtl_w0w1_eri(tp, >0x0dc, >ERIAR_MASK_0100, >0x, Please turn these into: rtl_..._eri(tp, 0x0dc, ERIAR_MASK_0100, MagicPacket_v2, 0x, ERIAR_EXGMAC); -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 06/10] r8169:change function name and behavior of function "rtl_w1w0_phy"
Chun-Hao Lin : > Change function name from "rtl_w1w0_phy" to "rtl_w0w1_phy". > And its behavior from "or first then mask" to "mask first then or". - I don't mind if its changed for a reason but the commit message doesn't tell why. Is it related to the overlap of the masks below ? - rtl_w1w0_phy was supposed to abbreviate "write ones then write zeros" where parameters are taken in this order. You won't hit the 80 cols limit if you replace it with something like "rtl_set_clear_phy" (if you use something longer it won't fit for rtl_w1w0_eri). [...] > diff --git a/drivers/net/ethernet/realtek/r8169.c > b/drivers/net/ethernet/realtek/r8169.c > index 1bf6696..e68fe5e 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c [...] > @@ -3575,33 +3575,33 @@ static void rtl8168h_1_hw_phy_config(struct > rtl8169_private *tp) > /* CHN EST parameters adjust - giga master */ > rtl_writephy(tp, 0x1f, 0x0a43); > rtl_writephy(tp, 0x13, 0x809b); > - rtl_w1w0_phy(tp, 0x14, 0x8000, 0xf800); > + rtl_w0w1_phy(tp, 0x14, 0x8000, 0xf800); > rtl_writephy(tp, 0x13, 0x80a2); > - rtl_w1w0_phy(tp, 0x14, 0x8000, 0xff00); > + rtl_w0w1_phy(tp, 0x14, 0x8000, 0xff00); > rtl_writephy(tp, 0x13, 0x80a4); > - rtl_w1w0_phy(tp, 0x14, 0x8500, 0xff00); > + rtl_w0w1_phy(tp, 0x14, 0x8500, 0xff00); > rtl_writephy(tp, 0x13, 0x809c); > - rtl_w1w0_phy(tp, 0x14, 0xbd00, 0xff00); > + rtl_w0w1_phy(tp, 0x14, 0xbd00, 0xff00); > rtl_writephy(tp, 0x1f, 0x); > > /* CHN EST parameters adjust - giga slave */ > rtl_writephy(tp, 0x1f, 0x0a43); > rtl_writephy(tp, 0x13, 0x80ad); > - rtl_w1w0_phy(tp, 0x14, 0x7000, 0xf800); > + rtl_w0w1_phy(tp, 0x14, 0x7000, 0xf800); > rtl_writephy(tp, 0x13, 0x80b4); > - rtl_w1w0_phy(tp, 0x14, 0x5000, 0xff00); > + rtl_w0w1_phy(tp, 0x14, 0x5000, 0xff00); > rtl_writephy(tp, 0x13, 0x80ac); > - rtl_w1w0_phy(tp, 0x14, 0x4000, 0xff00); > + rtl_w0w1_phy(tp, 0x14, 0x4000, 0xff00); > rtl_writephy(tp, 0x1f, 0x); > > /* CHN EST parameters adjust - fnet */ > rtl_writephy(tp, 0x1f, 0x0a43); > rtl_writephy(tp, 0x13, 0x808e); > - rtl_w1w0_phy(tp, 0x14, 0x1200, 0xff00); > + rtl_w0w1_phy(tp, 0x14, 0x1200, 0xff00); > rtl_writephy(tp, 0x13, 0x8090); > - rtl_w1w0_phy(tp, 0x14, 0xe500, 0xff00); > + rtl_w0w1_phy(tp, 0x14, 0xe500, 0xff00); > rtl_writephy(tp, 0x13, 0x8092); > - rtl_w1w0_phy(tp, 0x14, 0x9f00, 0xff00); > + rtl_w0w1_phy(tp, 0x14, 0x9f00, 0xff00); > rtl_writephy(tp, 0x1f, 0x); > > /* enable R-tune & PGA-retune function */ > @@ -3620,57 +3620,57 @@ static void rtl8168h_1_hw_phy_config(struct > rtl8169_private *tp) > dout_tapbin &= 0xf000; > rtl_writephy(tp, 0x1f, 0x0a43); > rtl_writephy(tp, 0x13, 0x827a); > - rtl_w1w0_phy(tp, 0x14, dout_tapbin, 0xf000); > + rtl_w0w1_phy(tp, 0x14, dout_tapbin, 0xf000); > rtl_writephy(tp, 0x13, 0x827b); > - rtl_w1w0_phy(tp, 0x14, dout_tapbin, 0xf000); > + rtl_w0w1_phy(tp, 0x14, dout_tapbin, 0xf000); > rtl_writephy(tp, 0x13, 0x827c); > - rtl_w1w0_phy(tp, 0x14, dout_tapbin, 0xf000); > + rtl_w0w1_phy(tp, 0x14, dout_tapbin, 0xf000); > rtl_writephy(tp, 0x13, 0x827d); > - rtl_w1w0_phy(tp, 0x14, dout_tapbin, 0xf000); > + rtl_w0w1_phy(tp, 0x14, dout_tapbin, 0xf000); [...] -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next] r8169:add support for RTL8168EP
Chun-Hao Lin :
> RTL8168EP is Realtek PCIe Gigabit Ethernet controller.
> It is a successor chip of RTL8168DP.
>
> This patch add support for this chip.
It does more than that.
Did Hayes review it ?
> Signed-off-by: Chun-Hao Lin
> ---
> drivers/net/ethernet/realtek/r8169.c | 611
> +--
> 1 file changed, 514 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c
> b/drivers/net/ethernet/realtek/r8169.c
> index 1d81238..0ead9a7 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -155,6 +155,9 @@ enum mac_version {
> RTL_GIGA_MAC_VER_46,
> RTL_GIGA_MAC_VER_47,
> RTL_GIGA_MAC_VER_48,
> + RTL_GIGA_MAC_VER_49,
> + RTL_GIGA_MAC_VER_50,
> + RTL_GIGA_MAC_VER_51,
> RTL_GIGA_MAC_NONE = 0xff,
> };
>
> @@ -302,6 +305,15 @@ static const struct {
> [RTL_GIGA_MAC_VER_48] =
> _R("RTL8107e", RTL_TD_1, FIRMWARE_8107E_2,
> JUMBO_1K, false),
> + [RTL_GIGA_MAC_VER_49] =
> + _R("RTL8168ep/8111ep", RTL_TD_1, NULL,
> + JUMBO_9K, false),
> + [RTL_GIGA_MAC_VER_50] =
> + _R("RTL8168ep/8111ep", RTL_TD_1, NULL,
> + JUMBO_9K, false),
> + [RTL_GIGA_MAC_VER_51] =
> + _R("RTL8168ep/8111ep", RTL_TD_1, NULL,
> + JUMBO_9K, false),
> };
> #undef _R
>
> @@ -400,6 +412,10 @@ enum rtl_registers {
> FuncEvent = 0xf0,
> FuncEventMask = 0xf4,
> FuncPresetState = 0xf8,
> + IBCR0 = 0xf8,
> + IBCR2 = 0xf9,
> + IBIMR0 = 0xfa,
> + IBISR0 = 0xfb,
> FuncForceEvent = 0xfc,
> };
>
> @@ -467,6 +483,7 @@ enum rtl8168_registers {
> #define ERIAR_EXGMAC (0x00 << ERIAR_TYPE_SHIFT)
> #define ERIAR_MSIX (0x01 << ERIAR_TYPE_SHIFT)
> #define ERIAR_ASF(0x02 << ERIAR_TYPE_SHIFT)
> +#define ERIAR_OOB(0x02 << ERIAR_TYPE_SHIFT)
> #define ERIAR_MASK_SHIFT 12
> #define ERIAR_MASK_0001 (0x1 << ERIAR_MASK_SHIFT)
> #define ERIAR_MASK_0011 (0x3 << ERIAR_MASK_SHIFT)
> @@ -935,93 +952,10 @@ static const struct rtl_cond name = {
> \
> \
> static bool name ## _check(struct rtl8169_private *tp)
>
> -DECLARE_RTL_COND(rtl_ocpar_cond)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - return RTL_R32(OCPAR) & OCPAR_FLAG;
> -}
Feel free to move this function around but please do it in a separate
patch. You are adding extra noise and thus making this stuff harder
to review than it should be.
> -
> -static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - RTL_W32(OCPAR, ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> -
> - return rtl_udelay_loop_wait_high(tp, &rtl_ocpar_cond, 100, 20) ?
> - RTL_R32(OCPDR) : ~0;
> -}
(this one is modified)
> -
> -static void ocp_write(struct rtl8169_private *tp, u8 mask, u16 reg, u32 data)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - RTL_W32(OCPDR, data);
> - RTL_W32(OCPAR, OCPAR_FLAG | ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> -
> - rtl_udelay_loop_wait_low(tp, &rtl_ocpar_cond, 100, 20);
> -}
(this one is modified)
> -
> -DECLARE_RTL_COND(rtl_eriar_cond)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - return RTL_R32(ERIAR) & ERIAR_FLAG;
> -}
Feel free to move this function around but please do it in a separate
patch.
> -
> -static void rtl8168_oob_notify(struct rtl8169_private *tp, u8 cmd)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - RTL_W8(ERIDR, cmd);
> - RTL_W32(ERIAR, 0x800010e8);
> - msleep(2);
> -
> - if (!rtl_udelay_loop_wait_low(tp, &rtl_eriar_cond, 100, 5))
> - return;
> -
> - ocp_write(tp, 0x1, 0x30, 0x0001);
> -}
This one is modified but it is also modified for the existing 8168DP.
Mantra: please do it in a separate patch.
> -
> #define OOB_CMD_RESET0x00
> #define OOB_CMD_DRIVER_START 0x05
> #define OOB_CMD_DRIVER_STOP 0x06
>
> -static u16 rtl8168_get_ocp_reg(struct rtl8169_private *tp)
> -{
> - return (tp->mac_version == RTL_GIGA_MAC_VER_31) ? 0xb8 : 0x10;
> -}
> -
> -DECLARE_RTL_COND(rtl_ocp_read_cond)
> -{
> - u16 reg;
> -
> - reg = rtl8168_get_ocp_reg(tp);
> -
> - return ocp_read(tp, 0x0f, reg) & 0x0800;
> -}
(this one is simply moved around)
> -
> -static void rtl8168_driver_start(struct rtl8169_private *tp)
> -{
> - rtl8168_oob_notify(tp, OOB_CMD_DRIVER_START);
> -
> - rtl_msleep_loop_wait_high(tp, &rtl_ocp_read_cond, 10, 10);
> -}
(this one is
Re: [RFC PATCH linux-next] et131x: Promote staging et131x driver to drivers/net
Joe Perches : > On Tue, 2014-09-23 at 21:02 +0200, Francois Romieu wrote: [...] > > How about kernel tinification ? > > The tiny case where a large number of ethernet drivers are included? No. A couple of bytes here and there vs a cosmetic kernel wide change. -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH linux-next] et131x: Promote staging et131x driver to drivers/net
Mark Einon : [...] > > No need for the #define here, just assigne et131x_pm_ops to .driver.pm > > directly, its members will be NULL and thus never called. Also, you can > > make et131x_pm_ops const. > > Ok, I can change this. > > Btw, this appears to be a fairly standard way of using .driver.pm among > ethernet drivers, e.g. see 3com/3c59x.c, atheros, marvell... - perhaps > there is a case for changing all instances of this code? How about kernel tinification ? -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net 1/2] r8169: fix the default setting of rx vlan
Hayes Wang :
> Francois Romieu [mailto:rom...@fr.zoreil.com]
> > Sent: Saturday, September 13, 2014 3:40 AM
> [...]
> > The same fix should be relevant for NETIF_F_RXCSUM. You may thus as
> > well remove the "changed" test in __rtl8169_set_features and keep
> > everything there.
>
> In the probe function (rtl_init_one), RxChkSum has been assgined to
> tp->cp_cmd, so only the vlan setting is incorrect. Therefore, I fix
> it only. Do I still have to remove the "changed" test?
You are right, this part does work.
However it's getting messy and the code stomps CPlusCmd in rtl_open right
before it's given a chance to be read in hw_start. Something like the
untested patch below should avoid it and keep related things together.
Thoughts ?
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index 7a7860a..ef2cee5 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1783,33 +1783,31 @@ static void __rtl8169_set_features(struct net_device
*dev,
netdev_features_t features)
{
struct rtl8169_private *tp = netdev_priv(dev);
- netdev_features_t changed = features ^ dev->features;
void __iomem *ioaddr = tp->mmio_addr;
+ u32 rx_config;
- if (!(changed & (NETIF_F_RXALL | NETIF_F_RXCSUM |
-NETIF_F_HW_VLAN_CTAG_RX)))
- return;
+ rx_config = RTL_R32(RxConfig);
+ if (features & NETIF_F_RXALL)
+ rx_config |= (AcceptErr | AcceptRunt);
+ else
+ rx_config &= ~(AcceptErr | AcceptRunt);
- if (changed & (NETIF_F_RXCSUM | NETIF_F_HW_VLAN_CTAG_RX)) {
- if (features & NETIF_F_RXCSUM)
- tp->cp_cmd |= RxChkSum;
- else
- tp->cp_cmd &= ~RxChkSum;
+ RTL_W32(RxConfig, rx_config);
- if (features & NETIF_F_HW_VLAN_CTAG_RX)
- tp->cp_cmd |= RxVlan;
- else
- tp->cp_cmd &= ~RxVlan;
+ if (features & NETIF_F_RXCSUM)
+ tp->cp_cmd |= RxChkSum;
+ else
+ tp->cp_cmd &= ~RxChkSum;
- RTL_W16(CPlusCmd, tp->cp_cmd);
- RTL_R16(CPlusCmd);
- }
- if (changed & NETIF_F_RXALL) {
- int tmp = (RTL_R32(RxConfig) & ~(AcceptErr | AcceptRunt));
- if (features & NETIF_F_RXALL)
- tmp |= (AcceptErr | AcceptRunt);
- RTL_W32(RxConfig, tmp);
- }
+ if (features & NETIF_F_HW_VLAN_CTAG_RX)
+ tp->cp_cmd |= RxVlan;
+ else
+ tp->cp_cmd &= ~RxVlan;
+
+ tp->cp_cmd |= RTL_R16(CPlusCmd) & ~(RxVlan | RxChkSum);
+
+ RTL_W16(CPlusCmd, tp->cp_cmd);
+ RTL_R16(CPlusCmd);
}
static int rtl8169_set_features(struct net_device *dev,
@@ -1817,8 +1815,11 @@ static int rtl8169_set_features(struct net_device *dev,
{
struct rtl8169_private *tp = netdev_priv(dev);
+ features &= NETIF_F_RXALL | NETIF_F_RXCSUM | NETIF_F_HW_VLAN_CTAG_RX;
+
rtl_lock_work(tp);
- __rtl8169_set_features(dev, features);
+ if (features ^ dev->features);
+ __rtl8169_set_features(dev, features);
rtl_unlock_work(tp);
return 0;
@@ -6707,12 +6708,7 @@ static int rtl_open(struct net_device *dev)
rtl8169_init_phy(dev, tp);
- if (dev->features & NETIF_F_HW_VLAN_CTAG_RX)
- tp->cp_cmd |= RxVlan;
- else
- tp->cp_cmd &= ~RxVlan;
-
- RTL_W16(CPlusCmd, tp->cp_cmd);
+ __rtl8169_set_features(dev, dev->features);
rtl_pll_power_up(tp);
@@ -7123,8 +7119,7 @@ static void rtl_hw_initialize(struct rtl8169_private *tp)
}
}
-static int
-rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
+static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
{
const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data;
const unsigned int region = cfg->region;
@@ -7199,7 +7194,7 @@ rtl_init_one(struct pci_dev *pdev, const struct
pci_device_id *ent)
goto err_out_mwi_2;
}
- tp->cp_cmd = RxChkSum;
+ tp->cp_cmd = 0;
if ((sizeof(dma_addr_t) > 4) &&
!pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) && use_dac) {
@@ -7240,13 +7235,6 @@ rtl_init_one(struct pci_dev *pdev, const struct
pci_device_id *ent)
pci_set_master(pdev);
- /*
-* Pretend we are using VLANs; This bypasses a nasty bug where
-* Interrupts stop flowing on high load on 8110SCd controllers.
-*/
- if (tp->mac_version == RTL_GIGA_MAC_VER_05)
Re: 3.17-rc5 fails compile (arch/x86/pci/irq.c)
Pete Clements : > Fyi: i386 up -- problem introduced in rc3. Fix available for ~2 weeks: https://lkml.kernel.org/r/1409382916-10649-1-git-send-email-jiang@linux.intel.com -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net 1/2] r8169: fix the default setting of rx vlan
Hayes Wang :
> If the parameter "features" of __rtl8169_set_features() is equal to
> dev->features, the variable "changed" is alwayes 0, and nothing would
> be changed.
[...]
> diff --git a/drivers/net/ethernet/realtek/r8169.c
> b/drivers/net/ethernet/realtek/r8169.c
> index 91652e7..f3ce284 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -6707,7 +6707,12 @@ static int rtl_open(struct net_device *dev)
>
> rtl8169_init_phy(dev, tp);
>
> - __rtl8169_set_features(dev, dev->features);
> + if (dev->features & NETIF_F_HW_VLAN_CTAG_RX)
> + tp->cp_cmd |= RxVlan;
> + else
> + tp->cp_cmd &= ~RxVlan;
> +
> + RTL_W16(CPlusCmd, tp->cp_cmd);
Damn good catch.
The same fix should be relevant for NETIF_F_RXCSUM. You may thus as
well remove the "changed" test in __rtl8169_set_features and keep
everything there.
The commit message could notify the driver don't behave as expected since
6bbe021d405fff46b64a08dca51b06897b897a67 ("r8169: Support RX-ALL flag.")
to ease stable people's work.
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH net-next 4/4] r8152: support firmware files
Hayes Wang :
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 937d132..63542cc 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
[...]
> +static void rtl_request_firmware(struct r8152 *tp)
> +{
> + char *fw_name = NULL;
> +
> + if (tp->rtl_fw.fw)
> + goto out_request;
> +
> + switch (tp->version) {
> + case RTL_VER_01:
> + fw_name = "rtl_nic/rtl8152-1.fw";
> + break;
> + case RTL_VER_02:
> + fw_name = "rtl_nic/rtl8152-2.fw";
> + break;
> + case RTL_VER_03:
> + fw_name = "rtl_nic/rtl8153-1.fw";
> + break;
> + case RTL_VER_04:
> + fw_name = "rtl_nic/rtl8153-2.fw";
> + break;
> + case RTL_VER_05:
> + fw_name = "rtl_nic/rtl8153-3.fw";
> + break;
The driver should use MODULE_FIRMWARE() for these files.
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3] netxen: fix ethtool rx_dropped information in ethtool get_ethtool_stats()
Ethan Zhao : [...] > you leave us without a tool to track the net core part. Would you like to > hack the kernel code and rebuild just for peek the dropping packets ? Search 'dropwatch' on your favorite engine search. [...] > More radically, and maybe you should write patches to fix them to keep > their focus. That is to prove you have principle and always fight for it. It's 15 years late but it's still a good advice. :o) -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3] netxen: fix ethtool rx_dropped information in ethtool get_ethtool_stats()
Ethan Zhao : > On Sat, Jul 19, 2014 at 6:19 PM, Francois Romieu wrote: > > Ethan Zhao : [...] > > I'd rather see ethtool stats provides one of those: > > 1. hardware stats only > Sounds very clear, is it done clear, if so we need other tools ? > Then why net core increase dev->dropped like > > atomic_long_inc(&skb->dev->rx_dropped); > > and mess some of the info to dev. just let ifconfig report it without detail ? (side note: ifconfig is a bit 90ish) Regarding the "without details" part, I don't expect ifconfig to tell users that packets are dropped at net/core/dev.c:3279 because of empty softnet_data queue in enqueue_to_backlog. More to the point below. [...] > > 2. needs works. 1. hurts any requirement where it would priviously had > > been so easy to add a few lines of code in the driver and push the mess > > straight from kernel to the end user application. > > Maybe some guys think that is enough to mess them together while not > identify the information from net core or driver. that is better than > lose info /no info. Probably. > My case is user could be confused when run 'ifconfig' and 'ethtool' > to this netxen device. ifconfig reports packets dropped. but ethtool > says zero. does that mean it is not issue of driver forever ? s/driver/hardware/ Hardware correctly delivers packets if ethtool says zero [*] "correctly" must be understood in the context of said hardware abilities. [*] Of course we don't live in a world of unicorns and it's almost unavoidable to check drivers's code to figure which stats are _really_ hardware only in ethtool output. A part of ethtool if close to bare metal abilities - including protocol offloading related ones - whereas ifconfig provides a software oriented vision of layer 2 operations. People at QLogic have not included *any* hardware counter in their netxen ethtool stats code. Nada, keud. It's a bit deceptive for a gigabit ethernet card and http://www.qlogic.com/Search/Pages/Search.aspx?k=NX2031 gives no hint. I won't claim it's intuitive. However: $ LANG=C man ifconfig [...] NOTE This program is obsolete! For replacement check ip addr and ip link. For statistics use ip -s link. $ LANG=C man ip [...] -f, -family Specifies the protocol family to use. The protocol family iden- tifier can be one of inet, inet6, bridge, ipx, dnet or link. If this option is not present, the protocol family is guessed from other arguments. If the rest of the command line does not give enough information to guess the family, ip falls back to the default one, usually inet or any. link is a special family identifier meaning that no networking protocol is involved. ^^ (huh, "special"...) $ LANG=C man ethtool [...] -S --statistics Queries the specified network device for NIC- and driver-specif- ic statistics. Some code stands between drivers and the upper part of the topmost no-networking-procol layer, whence the differing stats: they are not the same thing. Users can understand it. > who could tell where the packets are dropped, and for what ? One can't always answer beyond some coarse implementation level. There is no such thing as a kernel MIB for the net core internal details. Would you disagree that some of those - rx_dropped locations for instance - are better kept in the scope of debug and trace utilities ? > So I would like the name of the data fields that ethtool output is be > better defined. Instead of saying is not covered by ethtool. I respectfully disagree. ethtool and ip are two different tools. Regarding netxen driver, I do not want more ip / ifconfig stats in ethtool code. It's quite the opposite: netxen ethtool code has no business duplicating those and it should instead focus on real hardware stats. -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3] netxen: fix ethtool rx_dropped information in ethtool get_ethtool_stats()
Ethan Zhao : > ?? 2014??7??192:21??Rajesh Borundia > ?? [...] > > I think ethtool stats are adapter specific. Driver does not maintain > > rx_dropped stats and we also don't get it from adapter. I am not > > sure if ethtool stats should match ifconfig stats as ifconfig also > > adds net core stats to adapter stats. > I held the same point as yours before I read more drivers implementation > under Ethernet dir, in fact there is no specification/convention that > ethtool info is adapter specific. All information output by ethtool is > based on the information name itself, one fact we want this patch is > netxen defined 'rx dropped' field but output nothing. It may be fine as a requirement of the day but it does not bode well for the future as ethtool stats API won't get less messy. I'd rather see ethtool stats provides one of those: 1. hardware stats only 2. driver dependent stats - anything you want - as long as the hardware only part of the stats is clearly identified 2. needs works. 1. hurts any requirement where it would priviously had been so easy to add a few lines of code in the driver and push the mess straight from kernel to the end user application. -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
