* Re: [PATCH 2/3] x_tables: Use also dev->ifalias for interface matching
From: Patrick Schaaf @ 2015-01-12 17:19 UTC (permalink / raw)
To: Eric Dumazet
Cc: Richard Weinberger, davem, coreteam, netfilter-devel,
linux-kernel, netdev, bhutchings, john.fastabend, herbert,
vyasevic, jiri, vfalico, therbert, edumazet, yoshfuji, jmorris,
kuznet, kadlec, kaber, pablo, kay, stephen
In-Reply-To: <1421081514.4099.14.camel@edumazet-glaptop2.roam.corp.google.com>
On Monday 12 January 2015 08:51:54 Eric Dumazet wrote:
> On Mon, 2015-01-12 at 17:39 +0100, Patrick Schaaf wrote:
> >
> > Not to comment on the ifalias thing, which I think is unneccessary,
> > too, but matching on interface names instead of only ifindex, is
> > definitely needed, so that one can establish a full ruleset before
> > interfaces even exist. That's good practise at boottime, but also
> > needed for dynamic interface creation during runtime.
>
> Please do not send html messages : Your reply did not reach the lists.
Sigh. Sorry...
> Then, all you mention could have been solved by proper userspace
> support.
>
> Every time you add an interface or change device name, you could change
> firewalls rules if needed. Nothing shocking here.
That is totally impractical, IMO.
Interfaces come and go through many different actions. There's the admin
downing and upping stuff like bridges or bonds. There's stuff like libvirt /
KVM / qemu creating and destroying interfaces. In all these cases, in my
practise, I give the interfaces useful names to that I can prefix-match them
in iptables rules.
Dynamically modifying the ruleset for each such creation and destruction,
would be a huge burden. The base ruleset would need suitable "hooks" where
these rules were inserted (ordering matters!). The addition would hardly be
atomic (with traditional iptables, unless done by generating a whole new
ruleset and restoring). The programs (e.g. libvirt) would need to be able to
call out to these specially crafted rule generator scripts. The admin would
need to add them as pre/post actions to their static (manual) interface
configuration. Loading and looking at the ruleset before bringing up the
interface would be impossible.
Note that I do fully agree that it's sad that iptables rules waste all that
memory for each and every rule! I remember musing about improving that in
talks with Harald Welte back in the 90ies. A simple match would be perfectly
fine for me. Only having ifindex support, isn't.
best regards
Patrick
^ permalink raw reply
* Re: IPsec workshop at netdev01?
From: Nicolas Dichtel @ 2015-01-12 17:19 UTC (permalink / raw)
To: Florian Westphal, Steffen Klassert
Cc: netdev, Jamal Hadi Salim, Herbert Xu, David Miller
In-Reply-To: <20150107125554.GF11324@breakpoint.cc>
Le 07/01/2015 13:55, Florian Westphal a écrit :
> Steffen Klassert <steffen.klassert@secunet.com> wrote:
>> On Tue, Jan 06, 2015 at 06:00:26PM +0100, Florian Westphal wrote:
>>> Steffen Klassert <steffen.klassert@secunet.com> wrote:
>>>> - We still lack a 32/64 bit compatibiltiy layer for IPsec, this issue
>>>> comes up from time to time. Some solutions were proposed in the past
>>>> but all had problems. The current behaviour is broken if someone tries
>>>> to configure IPsec with 32 bit tools on a 64 bit machine. Can we get
>>>> this right somehow or is it better to just return an error in this case?
>>>
>>> FWIW I think
>>> http://patchwork.ozlabs.org/patch/49465/
>>>
>>> came closest to achieving full CONFIG_COMPAT support; since netlink is
>>> no longer async now I'm not sure we'd still need additonal 32-compat syscalls
>>> to make compat work for all cases.
>>>
>>> So "its ugly as hell" is probably the only problem that is hard to avoid ;-)
>>
>> Yeah, and it will be no fun to maintain it...
>
> Not sure, you'd have to make sure that no new attributes introduce need
> to add another compat hack.
>
> The best argument against supporting it is that this problem
> has existed for so long that there arguably isn't much demand
> (else, such patch would have been merged years ago).
In fact, there is regularly some proposals to fix this, but I think that
nobody has taken the time to make a patch that satisfies everybody.
There is certainly a number of "private" patch for this problem, hence it
can be good to have a consensus on this topic.
Regards,
Nicolas
^ permalink raw reply
* Re: [PATCH 2/3] x_tables: Use also dev->ifalias for interface matching
From: Eric Dumazet @ 2015-01-12 16:51 UTC (permalink / raw)
To: Patrick Schaaf
Cc: Richard Weinberger, davem, coreteam, netfilter-devel,
linux-kernel, netdev, bhutchings, john.fastabend, herbert,
vyasevic, jiri, vfalico, therbert, edumazet, yoshfuji, jmorris,
kuznet, kadlec, kaber, pablo, kay, stephen
In-Reply-To: <1425960.ovH4s7sjue@rofl>
On Mon, 2015-01-12 at 17:39 +0100, Patrick Schaaf wrote:
> > iptables should have used ifindex, its sad we allowed the substring
>
> > match in first place.
>
>
>
> Not to comment on the ifalias thing, which I think is unneccessary,
> too, but matching on interface names instead of only ifindex, is
> definitely needed, so that one can establish a full ruleset before
> interfaces even exist. That's good practise at boottime, but also
> needed for dynamic interface creation during runtime.
>
>
>
> A pure ifindex-during-packet-inspection approach might still work, but
> the ruleset must IMO keep the interface names. Maybe register them in
> a hash, keyed by name, with values an ifindex or ifindex set (for
> wildcard names), plus a reverse mapping from active ifindices to all
> places in these hash values where an ifindex has been remembered. On
> interface creation / destruction that structure could then be updated,
> and active packet filtering rules would refer to (and keep a refcount
> on) specific hash elements.
>
Please do not send html messages : Your reply did not reach the lists.
Then, all you mention could have been solved by proper userspace
support.
Every time you add an interface or change device name, you could change
firewalls rules if needed. Nothing shocking here.
The ruleset can indeed mention interface names, but the kernel part
really should not care about names, which are a 'human' convenient way
to represent things.
^ permalink raw reply
* Re: [PATCH net-next v3] tcp: avoid reducing cwnd when ACK+DSACK is received
From: Neal Cardwell @ 2015-01-12 16:48 UTC (permalink / raw)
To: Eric Dumazet
Cc: Sébastien Barré, David Miller, Yuchung Cheng, Netdev,
Gregory Detal, Nandita Dukkipati
In-Reply-To: <1421075729.4099.0.camel@edumazet-glaptop2.roam.corp.google.com>
On Mon, 2015-01-12 at 10:30 +0100, Sébastien Barré wrote:
> With TLP, the peer may reply to a probe with an
> ACK+D-SACK, with ack value set to tlp_high_seq. In the current code,
> such ACK+DSACK will be missed and only at next, higher ack will the TLP
> episode be considered done. Since the DSACK is not present anymore,
> this will cost a cwnd reduction.
Acked-by: Neal Cardwell <ncardwell@google.com>
Thanks!
neal
^ permalink raw reply
* Re: [PATCH 2/3] x_tables: Use also dev->ifalias for interface matching
From: Eric Dumazet @ 2015-01-12 16:46 UTC (permalink / raw)
To: Jan Engelhardt
Cc: Richard Weinberger, davem, coreteam, netfilter-devel,
linux-kernel, netdev, bhutchings, john.fastabend, herbert,
vyasevic, jiri, vfalico, therbert, edumazet, yoshfuji, jmorris,
kuznet, kadlec, kaber, pablo, kay, stephen
In-Reply-To: <alpine.LSU.2.11.1501121731510.25482@nerf60.vanv.qr>
On Mon, 2015-01-12 at 17:32 +0100, Jan Engelhardt wrote:
> On Monday 2015-01-12 17:04, Eric Dumazet wrote:
> >
> >iptables should have used ifindex [for interface matching],
> >it[']s sad we allowed the substring match in first place.
>
> How would you solve interface name wildcards with ifindices?
> (They come in handy if you have something like lots of tun+/veth+
> interfaces from openvpn/lxc.)
This is what I said : "it[']s sad we allowed the substring match in
first place."
This obviously referred to wildcards, in the in/out interface match for
every _single_ rule, consuming 64 bytes of memory per rule and per cpu !
Which is absolutely crazy in term of memory usage.
Matching tun+ or whatever could easily be done by a match (-m ...),
because you can factorize this quite easily (called once for a group of
rules)
^ permalink raw reply
* Re: [PATCH 2/3] x_tables: Use also dev->ifalias for interface matching
From: Jan Engelhardt @ 2015-01-12 16:32 UTC (permalink / raw)
To: Eric Dumazet
Cc: Richard Weinberger, davem, coreteam, netfilter-devel,
linux-kernel, netdev, bhutchings, john.fastabend, herbert,
vyasevic, jiri, vfalico, therbert, edumazet, yoshfuji, jmorris,
kuznet, kadlec, kaber, pablo, kay, stephen
In-Reply-To: <1421078666.4099.6.camel@edumazet-glaptop2.roam.corp.google.com>
On Monday 2015-01-12 17:04, Eric Dumazet wrote:
>
>iptables should have used ifindex [for interface matching],
>it[']s sad we allowed the substring match in first place.
How would you solve interface name wildcards with ifindices?
(They come in handy if you have something like lots of tun+/veth+
interfaces from openvpn/lxc.)
^ permalink raw reply
* [PATCH net] cxgb4vf: Initialize mdio_addr before using it
From: Hariprasad Shenai @ 2015-01-12 16:36 UTC (permalink / raw)
To: netdev; +Cc: davem, leedom, nirranjan, kumaras, Hariprasad Shenai
In commit 5ad24def21b205a8 ("cxgb4vf: Fix ethtool get_settings for VF driver")
mdio_addr of port_info structure was used unininitialzed. Fixing it.
Signed-off-by: Hariprasad Shenai <hariprasad@chelsio.com>
---
drivers/net/ethernet/chelsio/cxgb4vf/t4vf_hw.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/t4vf_hw.c b/drivers/net/ethernet/chelsio/cxgb4vf/t4vf_hw.c
index 21dc9a2..60426cf 100644
--- a/drivers/net/ethernet/chelsio/cxgb4vf/t4vf_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb4vf/t4vf_hw.c
@@ -323,6 +323,8 @@ int t4vf_port_init(struct adapter *adapter, int pidx)
return v;
v = be32_to_cpu(port_rpl.u.info.lstatus_to_modtype);
+ pi->mdio_addr = (v & FW_PORT_CMD_MDIOCAP_F) ?
+ FW_PORT_CMD_MDIOADDR_G(v) : -1;
pi->port_type = FW_PORT_CMD_PTYPE_G(v);
pi->mod_type = FW_PORT_MOD_TYPE_NA;
--
1.7.1
^ permalink raw reply related
* Re: [PATCH] brcmfmac: avoid duplicated suspend/resume operation
From: Arend van Spriel @ 2015-01-12 16:24 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Fu, Zhonghui, brudley, Franky Lin, meuleman, kvalo, linville,
pieterpg, hdegoede, wens, linux-wireless, brcm80211-dev-list,
netdev, linux-kernel@vger.kernel.org
In-Reply-To: <54B3D4FC.1090806@cogentembedded.com>
On 01/12/15 15:06, Sergei Shtylyov wrote:
> Hello.
>
> On 1/12/2015 9:41 AM, Fu, Zhonghui wrote:
>
>> From 8685c3c2746b4275fc808d9db23c364b2f54b52a Mon Sep 17 00:00:00 2001
>> From: Zhonghui Fu <zhonghui.fu@linux.intel.com>
>> Date: Mon, 12 Jan 2015 14:25:46 +0800
>> Subject: [PATCH] brcmfmac: avoid duplicated suspend/resume operation
>
> The lines above are not needed.
>
>> WiFi chip has 2 SDIO functions, and PM core will trigger
>> twice suspend/resume operations for one WiFi chip to do
>> the same things. This patch avoid this case.
>
>> Acked-by: Arend van Spriel<arend@broadcom.com>
>> Acked-by: Sergei Shtylyov<sergei.shtylyov@cogentembedded.com>
>> Signed-off-by: Zhonghui Fu <zhonghui.fu@linux.intel.com>
>> ---
>> drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 21
>> +++++++++++++++++----
>> 1 files changed, 17 insertions(+), 4 deletions(-)
>
>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>> b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>> index 9880dae..8f71485 100644
>> --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
>> @@ -1070,7 +1070,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func
>> *func,
>> */
>> if ((sdio_get_host_pm_caps(sdiodev->func[1]) & MMC_PM_KEEP_POWER) &&
>> ((sdio_get_host_pm_caps(sdiodev->func[1]) & MMC_PM_WAKE_SDIO_IRQ) ||
>> - (sdiodev->pdata && sdiodev->pdata->oob_irq_supported)))
>> + (sdiodev->pdata->oob_irq_supported)))
>
> Inner parens not needed on this line.
Well, actually this patch should not affect those line as it would
reintroduce a recently fixed issue.
Regards,
Arend
> [...]
>
> WBR, Sergei
>
^ permalink raw reply
* Re: [PATCH 2/3] x_tables: Use also dev->ifalias for interface matching
From: Richard Weinberger @ 2015-01-12 16:12 UTC (permalink / raw)
To: Eric Dumazet
Cc: davem, coreteam, netfilter-devel, linux-kernel, netdev,
bhutchings, john.fastabend, herbert, vyasevic, jiri, vfalico,
therbert, edumazet, yoshfuji, jmorris, kuznet, kadlec, kaber,
pablo, kay, stephen
In-Reply-To: <1421078666.4099.6.camel@edumazet-glaptop2.roam.corp.google.com>
Am 12.01.2015 um 17:04 schrieb Eric Dumazet:
> On Sun, 2015-01-11 at 21:52 +0100, Richard Weinberger wrote:
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> ---
>> include/linux/netfilter/x_tables.h | 22 ++++++++++++++++++++++
>> net/ipv4/netfilter/arp_tables.c | 28 +++++++++++++++++-----------
>> net/ipv4/netfilter/ip_tables.c | 15 +++++----------
>> net/ipv6/netfilter/ip6_tables.c | 18 +++++++-----------
>> net/netfilter/xt_physdev.c | 9 ++-------
>> 5 files changed, 53 insertions(+), 39 deletions(-)
>
> Richard, I dislike this, sorry.
That's fine, the series carries the "RFC" burning mark for a reason.
> iptables is already horribly expensive, you add another expensive step
> for every rule.
Yeah, you mean the extra unlikey() check?
> device aliasing can be done from user space.
How?
I did this series because I'm not so happy with the device renaming what udev
does.
The idea was to offer udev a better kernel interface to deal with aliases.
Such that one can use the regular names form the kernel and the predictable
names generated from udev.
For block devices it was easy, we have the good old symlink.
For network interface the kernel does not offer an API.
> iptables should have used ifindex, its sad we allowed the substring
> match in first place.
Maybe nftables can do better. :-)
Thanks,
//richard
^ permalink raw reply
* [PATCH v2 1/1] atm: remove deprecated use of pci api
From: Quentin Lambert @ 2015-01-12 16:10 UTC (permalink / raw)
To: Chas Williams; +Cc: linux-atm-general, netdev, linux-kernel
Replace occurences of the pci api by appropriate call to the dma api.
A simplified version of the semantic patch that finds this problem is as
follows: (http://coccinelle.lip6.fr)
@deprecated@
idexpression id;
position p;
@@
(
pci_dma_supported@p ( id, ...)
|
pci_alloc_consistent@p ( id, ...)
)
@bad1@
idexpression id;
position deprecated.p;
@@
...when != &id->dev
when != pci_get_drvdata ( id )
when != pci_enable_device ( id )
(
pci_dma_supported@p ( id, ...)
|
pci_alloc_consistent@p ( id, ...)
)
@depends on !bad1@
idexpression id;
expression direction;
position deprecated.p;
@@
(
- pci_dma_supported@p ( id,
+ dma_supported ( &id->dev,
...
+ , GFP_ATOMIC
)
|
- pci_alloc_consistent@p ( id,
+ dma_alloc_coherent ( &id->dev,
...
+ , GFP_ATOMIC
)
)
Signed-off-by: Quentin Lambert <lambert.quentin@gmail.com>
---
drivers/atm/eni.c | 8 +++++---
drivers/atm/he.c | 2 +-
drivers/atm/lanai.c | 9 +++++----
drivers/atm/nicstar.c | 4 ++--
drivers/atm/solos-pci.c | 2 +-
drivers/atm/zatm.c | 8 +++++---
6 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/drivers/atm/eni.c b/drivers/atm/eni.c
index c7fab3e..a128020 100644
--- a/drivers/atm/eni.c
+++ b/drivers/atm/eni.c
@@ -2246,7 +2246,8 @@ static int eni_init_one(struct pci_dev *pci_dev,
goto err_disable;
zero = &eni_dev->zero;
- zero->addr = pci_alloc_consistent(pci_dev, ENI_ZEROES_SIZE, &zero->dma);
+ zero->addr = dma_alloc_coherent(&pci_dev->dev, ENI_ZEROES_SIZE,
+ &zero->dma, GFP_ATOMIC);
if (!zero->addr)
goto err_kfree;
@@ -2277,7 +2278,8 @@ err_eni_release:
err_unregister:
atm_dev_deregister(dev);
err_free_consistent:
- pci_free_consistent(pci_dev, ENI_ZEROES_SIZE, zero->addr, zero->dma);
+ dma_free_coherent(&pci_dev->dev, ENI_ZEROES_SIZE, zero->addr,
+ zero->dma);
err_kfree:
kfree(eni_dev);
err_disable:
@@ -2302,7 +2304,7 @@ static void eni_remove_one(struct pci_dev *pdev)
eni_do_release(dev);
atm_dev_deregister(dev);
- pci_free_consistent(pdev, ENI_ZEROES_SIZE, zero->addr, zero->dma);
+ dma_free_coherent(&pdev->dev, ENI_ZEROES_SIZE, zero->addr, zero->dma);
kfree(ed);
pci_disable_device(pdev);
}
diff --git a/drivers/atm/he.c b/drivers/atm/he.c
index c39702b..69a2598 100644
--- a/drivers/atm/he.c
+++ b/drivers/atm/he.c
@@ -359,7 +359,7 @@ static int he_init_one(struct pci_dev *pci_dev,
if (pci_enable_device(pci_dev))
return -EIO;
- if (pci_set_dma_mask(pci_dev, DMA_BIT_MASK(32)) != 0) {
+ if (dma_set_mask(&pci_dev->dev, DMA_BIT_MASK(32)) != 0) {
printk(KERN_WARNING "he: no suitable dma available\n");
err = -EIO;
goto init_one_failure;
diff --git a/drivers/atm/lanai.c b/drivers/atm/lanai.c
index 93eaf8d..70fe734 100644
--- a/drivers/atm/lanai.c
+++ b/drivers/atm/lanai.c
@@ -346,7 +346,8 @@ static void lanai_buf_allocate(struct lanai_buffer *buf,
* everything, but the way the lanai uses DMA memory would
* make that a terrific pain. This is much simpler.
*/
- buf->start = pci_alloc_consistent(pci, size, &buf->dmaaddr);
+ buf->start = dma_alloc_coherent(&pci->dev, size, &buf->dmaaddr,
+ GFP_ATOMIC);
if (buf->start != NULL) { /* Success */
/* Lanai requires 256-byte alignment of DMA bufs */
APRINTK((buf->dmaaddr & ~0xFFFFFF00) == 0,
@@ -372,7 +373,7 @@ static void lanai_buf_deallocate(struct lanai_buffer *buf,
struct pci_dev *pci)
{
if (buf->start != NULL) {
- pci_free_consistent(pci, lanai_buf_size(buf),
+ dma_free_coherent(&pci->dev, lanai_buf_size(buf),
buf->start, buf->dmaaddr);
buf->start = buf->end = buf->ptr = NULL;
}
@@ -1953,12 +1954,12 @@ static int lanai_pci_start(struct lanai_dev *lanai)
return -ENXIO;
}
pci_set_master(pci);
- if (pci_set_dma_mask(pci, DMA_BIT_MASK(32)) != 0) {
+ if (dma_set_mask(&pci->dev, DMA_BIT_MASK(32)) != 0) {
printk(KERN_WARNING DEV_LABEL
"(itf %d): No suitable DMA available.\n", lanai->number);
return -EBUSY;
}
- if (pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(32)) != 0) {
+ if (dma_set_coherent_mask(&pci->dev, DMA_BIT_MASK(32)) != 0) {
printk(KERN_WARNING DEV_LABEL
"(itf %d): No suitable DMA available.\n", lanai->number);
return -EBUSY;
diff --git a/drivers/atm/nicstar.c b/drivers/atm/nicstar.c
index 9988ac9..aabb528 100644
--- a/drivers/atm/nicstar.c
+++ b/drivers/atm/nicstar.c
@@ -370,8 +370,8 @@ static int ns_init_card(int i, struct pci_dev *pcidev)
ns_init_card_error(card, error);
return error;
}
- if ((pci_set_dma_mask(pcidev, DMA_BIT_MASK(32)) != 0) ||
- (pci_set_consistent_dma_mask(pcidev, DMA_BIT_MASK(32)) != 0)) {
+ if ((dma_set_mask(&pcidev->dev, DMA_BIT_MASK(32)) != 0) ||
+ (dma_set_coherent_mask(&pcidev->dev, DMA_BIT_MASK(32)) != 0)) {
printk(KERN_WARNING
"nicstar%d: No suitable DMA available.\n", i);
error = 2;
diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
index 21b0bc6..48531b8 100644
--- a/drivers/atm/solos-pci.c
+++ b/drivers/atm/solos-pci.c
@@ -1210,7 +1210,7 @@ static int fpga_probe(struct pci_dev *dev, const struct pci_device_id *id)
goto out;
}
- err = pci_set_dma_mask(dev, DMA_BIT_MASK(32));
+ err = dma_set_mask(&dev->dev, DMA_BIT_MASK(32));
if (err) {
dev_warn(&dev->dev, "Failed to set 32-bit DMA mask\n");
goto out;
diff --git a/drivers/atm/zatm.c b/drivers/atm/zatm.c
index 969c3c2..b6456b1 100644
--- a/drivers/atm/zatm.c
+++ b/drivers/atm/zatm.c
@@ -1306,7 +1306,8 @@ static int zatm_start(struct atm_dev *dev)
if (!mbx_entries[i])
continue;
- mbx = pci_alloc_consistent(pdev, 2*MBX_SIZE(i), &mbx_dma);
+ mbx = dma_alloc_coherent(&pdev->dev, 2*MBX_SIZE(i), &mbx_dma,
+ GFP_ATOMIC);
if (!mbx) {
error = -ENOMEM;
goto out;
@@ -1318,7 +1319,8 @@ static int zatm_start(struct atm_dev *dev)
if (((unsigned long)mbx ^ mbx_dma) & 0xffff) {
printk(KERN_ERR DEV_LABEL "(itf %d): system "
"bus incompatible with driver\n", dev->number);
- pci_free_consistent(pdev, 2*MBX_SIZE(i), mbx, mbx_dma);
+ dma_free_coherent(&pdev->dev, 2*MBX_SIZE(i), mbx,
+ mbx_dma);
error = -ENODEV;
goto out;
}
@@ -1354,7 +1356,7 @@ out_tx:
kfree(zatm_dev->tx_map);
out:
while (i-- > 0) {
- pci_free_consistent(pdev, 2*MBX_SIZE(i),
+ dma_free_coherent(&pdev->dev, 2*MBX_SIZE(i),
(void *)zatm_dev->mbx_start[i],
zatm_dev->mbx_dma[i]);
}
--
1.9.1
^ permalink raw reply related
* [PATCH v2 0/1] moving from pci to dma
From: Quentin Lambert @ 2015-01-12 16:09 UTC (permalink / raw)
To: Chas Williams; +Cc: linux-atm-general, netdev, linux-kernel
The second version of this patch fix a mistake in the numbering of the
mails. The following paragraphs are the original cover letter of this
patch.
This patch replaces the references to the deprecated pci api with the
corresponding dma api.
To ensure that it was possible to access the dev field of pci_dev without
checking for nullity we looked for similar access in the execution
flow.
The most straight forward are "&id->dev" and "pci_get_drvdata(id)" where
id is the variable whose type is pci_dev.
We also found "pci_enable_device(id)" to be satisfying since the call
accesses other field without checking for nullity.
Quentin Lambert (1):
atm: remove deprecated use of pci api
drivers/atm/eni.c | 8 +++++---
drivers/atm/he.c | 2 +-
drivers/atm/lanai.c | 9 +++++----
drivers/atm/nicstar.c | 4 ++--
drivers/atm/solos-pci.c | 2 +-
drivers/atm/zatm.c | 8 +++++---
6 files changed, 19 insertions(+), 14 deletions(-)
--
1.9.1
^ permalink raw reply
* [patch net-next v2] tc: add BPF based action
From: Jiri Pirko @ 2015-01-12 16:09 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs, dborkman, ast, hannes
This action provides a possibility to exec custom BPF code.
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
v1->v2:
- fixed error path in _init
- added cleanup function to kill filter prog
---
include/net/tc_act/tc_bpf.h | 25 +++++
include/uapi/linux/tc_act/Kbuild | 1 +
include/uapi/linux/tc_act/tc_bpf.h | 31 ++++++
net/sched/Kconfig | 11 ++
net/sched/Makefile | 1 +
net/sched/act_bpf.c | 205 +++++++++++++++++++++++++++++++++++++
6 files changed, 274 insertions(+)
create mode 100644 include/net/tc_act/tc_bpf.h
create mode 100644 include/uapi/linux/tc_act/tc_bpf.h
create mode 100644 net/sched/act_bpf.c
diff --git a/include/net/tc_act/tc_bpf.h b/include/net/tc_act/tc_bpf.h
new file mode 100644
index 0000000..95e11da
--- /dev/null
+++ b/include/net/tc_act/tc_bpf.h
@@ -0,0 +1,25 @@
+/*
+ * Copyright (c) 2015 Jiri Pirko <jiri@resnulli.us>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __NET_TC_BPF_H
+#define __NET_TC_BPF_H
+
+#include <linux/filter.h>
+#include <net/act_api.h>
+
+struct tcf_bpf {
+ struct tcf_common common;
+ struct bpf_prog *filter;
+ struct sock_filter *bpf_ops;
+ u16 bpf_len;
+};
+#define to_bpf(a) \
+ container_of(a->priv, struct tcf_bpf, common)
+
+#endif /* __NET_TC_BPF_H */
diff --git a/include/uapi/linux/tc_act/Kbuild b/include/uapi/linux/tc_act/Kbuild
index b057da2..19d5219 100644
--- a/include/uapi/linux/tc_act/Kbuild
+++ b/include/uapi/linux/tc_act/Kbuild
@@ -8,3 +8,4 @@ header-y += tc_nat.h
header-y += tc_pedit.h
header-y += tc_skbedit.h
header-y += tc_vlan.h
+header-y += tc_bpf.h
diff --git a/include/uapi/linux/tc_act/tc_bpf.h b/include/uapi/linux/tc_act/tc_bpf.h
new file mode 100644
index 0000000..5288bd77
--- /dev/null
+++ b/include/uapi/linux/tc_act/tc_bpf.h
@@ -0,0 +1,31 @@
+/*
+ * Copyright (c) 2015 Jiri Pirko <jiri@resnulli.us>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_TC_BPF_H
+#define __LINUX_TC_BPF_H
+
+#include <linux/pkt_cls.h>
+
+#define TCA_ACT_BPF 13
+
+struct tc_act_bpf {
+ tc_gen;
+};
+
+enum {
+ TCA_ACT_BPF_UNSPEC,
+ TCA_ACT_BPF_TM,
+ TCA_ACT_BPF_PARMS,
+ TCA_ACT_BPF_OPS_LEN,
+ TCA_ACT_BPF_OPS,
+ __TCA_ACT_BPF_MAX,
+};
+#define TCA_ACT_BPF_MAX (__TCA_ACT_BPF_MAX - 1)
+
+#endif
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index c54c9d9..cc311e9 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -698,6 +698,17 @@ config NET_ACT_VLAN
To compile this code as a module, choose M here: the
module will be called act_vlan.
+config NET_ACT_BPF
+ tristate "BPF based action"
+ depends on NET_CLS_ACT
+ ---help---
+ Say Y here to execute BFP code on packets.
+
+ If unsure, say N.
+
+ To compile this code as a module, choose M here: the
+ module will be called act_bpf.
+
config NET_CLS_IND
bool "Incoming device classification"
depends on NET_CLS_U32 || NET_CLS_FW
diff --git a/net/sched/Makefile b/net/sched/Makefile
index 679f24a..7ca2b4e 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_NET_ACT_SIMP) += act_simple.o
obj-$(CONFIG_NET_ACT_SKBEDIT) += act_skbedit.o
obj-$(CONFIG_NET_ACT_CSUM) += act_csum.o
obj-$(CONFIG_NET_ACT_VLAN) += act_vlan.o
+obj-$(CONFIG_NET_ACT_BPF) += act_bpf.o
obj-$(CONFIG_NET_SCH_FIFO) += sch_fifo.o
obj-$(CONFIG_NET_SCH_CBQ) += sch_cbq.o
obj-$(CONFIG_NET_SCH_HTB) += sch_htb.o
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
new file mode 100644
index 0000000..1fac2f2
--- /dev/null
+++ b/net/sched/act_bpf.c
@@ -0,0 +1,205 @@
+/*
+ * Copyright (c) 2015 Jiri Pirko <jiri@resnulli.us>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/skbuff.h>
+#include <linux/rtnetlink.h>
+#include <linux/filter.h>
+#include <net/netlink.h>
+#include <net/pkt_sched.h>
+
+#include <linux/tc_act/tc_bpf.h>
+#include <net/tc_act/tc_bpf.h>
+
+#define BPF_TAB_MASK 15
+
+static int tcf_bpf(struct sk_buff *skb, const struct tc_action *a,
+ struct tcf_result *res)
+{
+ struct tcf_bpf *b = a->priv;
+ int action;
+ int filter_res;
+
+ spin_lock(&b->tcf_lock);
+ b->tcf_tm.lastuse = jiffies;
+ bstats_update(&b->tcf_bstats, skb);
+ action = b->tcf_action;
+
+ filter_res = BPF_PROG_RUN(b->filter, skb);
+ if (filter_res == -1)
+ goto drop;
+
+ goto unlock;
+
+drop:
+ action = TC_ACT_SHOT;
+ b->tcf_qstats.drops++;
+unlock:
+ spin_unlock(&b->tcf_lock);
+ return action;
+}
+static int tcf_bpf_dump(struct sk_buff *skb, struct tc_action *a,
+ int bind, int ref)
+{
+ unsigned char *tp = skb_tail_pointer(skb);
+ struct tcf_bpf *b = a->priv;
+ struct tc_act_bpf opt = {
+ .index = b->tcf_index,
+ .refcnt = b->tcf_refcnt - ref,
+ .bindcnt = b->tcf_bindcnt - bind,
+ .action = b->tcf_action,
+ };
+ struct tcf_t t;
+ struct nlattr *nla;
+
+ if (nla_put(skb, TCA_ACT_BPF_PARMS, sizeof(opt), &opt))
+ goto nla_put_failure;
+
+ if (nla_put_u16(skb, TCA_ACT_BPF_OPS_LEN, b->bpf_len))
+ goto nla_put_failure;
+
+ nla = nla_reserve(skb, TCA_ACT_BPF_OPS, b->bpf_len *
+ sizeof(struct sock_filter));
+ if (!nla)
+ goto nla_put_failure;
+
+ memcpy(nla_data(nla), b->bpf_ops, nla_len(nla));
+
+ t.install = jiffies_to_clock_t(jiffies - b->tcf_tm.install);
+ t.lastuse = jiffies_to_clock_t(jiffies - b->tcf_tm.lastuse);
+ t.expires = jiffies_to_clock_t(b->tcf_tm.expires);
+ if (nla_put(skb, TCA_ACT_BPF_TM, sizeof(t), &t))
+ goto nla_put_failure;
+ return skb->len;
+
+nla_put_failure:
+ nlmsg_trim(skb, tp);
+ return -1;
+}
+
+static const struct nla_policy act_bpf_policy[TCA_ACT_BPF_MAX + 1] = {
+ [TCA_ACT_BPF_PARMS] = { .len = sizeof(struct tc_act_bpf) },
+ [TCA_ACT_BPF_OPS_LEN] = { .type = NLA_U16 },
+ [TCA_ACT_BPF_OPS] = { .type = NLA_BINARY,
+ .len = sizeof(struct sock_filter) * BPF_MAXINSNS },
+};
+
+static int tcf_bpf_init(struct net *net, struct nlattr *nla,
+ struct nlattr *est, struct tc_action *a,
+ int ovr, int bind)
+{
+ struct nlattr *tb[TCA_ACT_BPF_MAX + 1];
+ struct tc_act_bpf *parm;
+ struct tcf_bpf *b;
+ u16 bpf_size, bpf_len;
+ struct sock_filter *bpf_ops;
+ struct sock_fprog_kern tmp;
+ struct bpf_prog *fp;
+ int ret;
+
+ if (!nla)
+ return -EINVAL;
+
+ ret = nla_parse_nested(tb, TCA_ACT_BPF_MAX, nla, act_bpf_policy);
+ if (ret < 0)
+ return ret;
+
+ if (!tb[TCA_ACT_BPF_PARMS] ||
+ !tb[TCA_ACT_BPF_OPS_LEN] || !tb[TCA_ACT_BPF_OPS])
+ return -EINVAL;
+ parm = nla_data(tb[TCA_ACT_BPF_PARMS]);
+
+ bpf_len = nla_get_u16(tb[TCA_ACT_BPF_OPS_LEN]);
+ if (bpf_len > BPF_MAXINSNS || bpf_len == 0)
+ return -EINVAL;
+
+ bpf_size = bpf_len * sizeof(*bpf_ops);
+ bpf_ops = kzalloc(bpf_size, GFP_KERNEL);
+ if (!bpf_ops)
+ return -ENOMEM;
+
+ memcpy(bpf_ops, nla_data(tb[TCA_ACT_BPF_OPS]), bpf_size);
+
+ tmp.len = bpf_len;
+ tmp.filter = bpf_ops;
+
+ ret = bpf_prog_create(&fp, &tmp);
+ if (ret)
+ goto free_bpf_ops;
+
+ if (!tcf_hash_check(parm->index, a, bind)) {
+ ret = tcf_hash_create(parm->index, est, a, sizeof(*b), bind);
+ if (ret)
+ goto destroy_fp;
+
+ ret = ACT_P_CREATED;
+ } else {
+ if (bind)
+ goto destroy_fp;
+ tcf_hash_release(a, bind);
+ if (!ovr) {
+ ret = -EEXIST;
+ goto destroy_fp;
+ }
+ }
+
+ b = to_bpf(a);
+ spin_lock_bh(&b->tcf_lock);
+ b->tcf_action = parm->action;
+ b->bpf_len = bpf_len;
+ b->bpf_ops = bpf_ops;
+ b->filter = fp;
+ spin_unlock_bh(&b->tcf_lock);
+
+ if (ret == ACT_P_CREATED)
+ tcf_hash_insert(a);
+ return ret;
+
+destroy_fp:
+ bpf_prog_destroy(fp);
+free_bpf_ops:
+ kfree(bpf_ops);
+ return ret;
+}
+
+static void tcf_bpf_cleanup(struct tc_action *a, int bind)
+{
+ struct tcf_bpf *b = a->priv;
+
+ bpf_prog_destroy(b->filter);
+}
+
+static struct tc_action_ops act_bpf_ops = {
+ .kind = "bpf",
+ .type = TCA_ACT_BPF,
+ .owner = THIS_MODULE,
+ .act = tcf_bpf,
+ .dump = tcf_bpf_dump,
+ .cleanup = tcf_bpf_cleanup,
+ .init = tcf_bpf_init,
+};
+
+static int __init bpf_init_module(void)
+{
+ return tcf_register_action(&act_bpf_ops, BPF_TAB_MASK);
+}
+
+static void __exit bpf_cleanup_module(void)
+{
+ tcf_unregister_action(&act_bpf_ops);
+}
+
+module_init(bpf_init_module);
+module_exit(bpf_cleanup_module);
+
+MODULE_AUTHOR("Jiri Pirko <jiri@resnulli.us>");
+MODULE_DESCRIPTION("TC BPF based action");
+MODULE_LICENSE("GPL v2");
--
1.9.3
^ permalink raw reply related
* Re: [PATCH 2/3] x_tables: Use also dev->ifalias for interface matching
From: Eric Dumazet @ 2015-01-12 16:04 UTC (permalink / raw)
To: Richard Weinberger
Cc: davem, coreteam, netfilter-devel, linux-kernel, netdev,
bhutchings, john.fastabend, herbert, vyasevic, jiri, vfalico,
therbert, edumazet, yoshfuji, jmorris, kuznet, kadlec, kaber,
pablo, kay, stephen
In-Reply-To: <1421009571-5279-3-git-send-email-richard@nod.at>
On Sun, 2015-01-11 at 21:52 +0100, Richard Weinberger wrote:
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> include/linux/netfilter/x_tables.h | 22 ++++++++++++++++++++++
> net/ipv4/netfilter/arp_tables.c | 28 +++++++++++++++++-----------
> net/ipv4/netfilter/ip_tables.c | 15 +++++----------
> net/ipv6/netfilter/ip6_tables.c | 18 +++++++-----------
> net/netfilter/xt_physdev.c | 9 ++-------
> 5 files changed, 53 insertions(+), 39 deletions(-)
Richard, I dislike this, sorry.
iptables is already horribly expensive, you add another expensive step
for every rule.
device aliasing can be done from user space.
iptables should have used ifindex, its sad we allowed the substring
match in first place.
^ permalink raw reply
* Re: [PATCH] moving from pci to dma
From: chas williams - CONTRACTOR @ 2015-01-12 15:39 UTC (permalink / raw)
To: Quentin Lambert; +Cc: linux-atm-general, netdev, linux-kernel
In-Reply-To: <54B3E91E.9060100@gmail.com>
On Mon, 12 Jan 2015 16:32:46 +0100
Quentin Lambert <lambert.quentin@gmail.com> wrote:
>
> On 12/01/2015 16:27, chas williams - CONTRACTOR wrote:
> > There doesn't seem to be a patch for review?
> >
> I made a mistake and forgot to number the mails. I did send them though.
> Would you like me to send them again, with the proper subject format?
>
>
> Quentin Lambert
>
That would be nice. Thanks!
^ permalink raw reply
* Re: [PATCH] moving from pci to dma
From: Quentin Lambert @ 2015-01-12 15:32 UTC (permalink / raw)
To: chas williams - CONTRACTOR; +Cc: linux-atm-general, netdev, linux-kernel
In-Reply-To: <20150112102700.2aa3b17e@thirdoffive.cmf.nrl.navy.mil>
On 12/01/2015 16:27, chas williams - CONTRACTOR wrote:
> There doesn't seem to be a patch for review?
>
I made a mistake and forgot to number the mails. I did send them though.
Would you like me to send them again, with the proper subject format?
Quentin Lambert
^ permalink raw reply
* Re: [PATCH] moving from pci to dma
From: chas williams - CONTRACTOR @ 2015-01-12 15:27 UTC (permalink / raw)
To: Quentin Lambert; +Cc: linux-atm-general, netdev, linux-kernel
In-Reply-To: <20150112100239.GA9009@sloth>
There doesn't seem to be a patch for review?
On Mon, 12 Jan 2015 11:02:39 +0100
Quentin Lambert <lambert.quentin@gmail.com> wrote:
> This patch replaces the references to the deprecated pci api with the
> corresponding dma api.
...
> Quentin Lambert (1):
> atm: remove deprecated use of pci api
>
> drivers/atm/eni.c | 8 +++++---
> drivers/atm/he.c | 2 +-
> drivers/atm/lanai.c | 9 +++++----
> drivers/atm/nicstar.c | 4 ++--
> drivers/atm/solos-pci.c | 2 +-
> drivers/atm/zatm.c | 8 +++++---
> 6 files changed, 19 insertions(+), 14 deletions(-)
>
^ permalink raw reply
* [PATCH 1/1 net-next] openvswitch: Remove unnecessary version.h inclusion
From: Syam Sidhardhan @ 2015-01-12 15:19 UTC (permalink / raw)
To: netdev, davem, dev, pshelar, syamsidhardh; +Cc: Syam Sidhardhan
version.h inclusion is not necessary as detected by versioncheck.
Signed-off-by: Syam Sidhardhan <s.syam@samsung.com>
Acked-by: Pravin B Shelar <pshelar@nicira.com>
---
No code changes. Add net-next prefix flag for net-next tree.
net/openvswitch/vport-geneve.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/net/openvswitch/vport-geneve.c b/net/openvswitch/vport-geneve.c
index 347fa23..70e2aae 100644
--- a/net/openvswitch/vport-geneve.c
+++ b/net/openvswitch/vport-geneve.c
@@ -9,8 +9,6 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#include <linux/version.h>
-
#include <linux/in.h>
#include <linux/ip.h>
#include <linux/net.h>
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH net-next v3] tcp: avoid reducing cwnd when ACK+DSACK is received
From: Eric Dumazet @ 2015-01-12 15:15 UTC (permalink / raw)
To: Sébastien Barré
Cc: David Miller, Neal Cardwell, Yuchung Cheng, netdev, Gregory Detal,
Nandita Dukkipati
In-Reply-To: <1421055040-8732-1-git-send-email-sebastien.barre@uclouvain.be>
On Mon, 2015-01-12 at 10:30 +0100, Sébastien Barré wrote:
> With TLP, the peer may reply to a probe with an
> ACK+D-SACK, with ack value set to tlp_high_seq. In the current code,
> such ACK+DSACK will be missed and only at next, higher ack will the TLP
> episode be considered done. Since the DSACK is not present anymore,
> this will cost a cwnd reduction.
Acked-by: Eric Dumazet <edumazet@google.com>
Thanks guys.
^ permalink raw reply
* [PATCH v5] can: Convert to runtime_pm
From: Kedareswara rao Appana @ 2015-01-12 15:04 UTC (permalink / raw)
To: wg, mkl, michal.simek, soren.brinkmann, grant.likely, robh+dt
Cc: linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
Kedareswara rao Appana
Instead of enabling/disabling clocks at several locations in the driver,
Use the runtime_pm framework. This consolidates the actions for runtime PM
In the appropriate callbacks and makes the driver more readable and mantainable.
Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
---
Changes for v5:
- Updated with the review comments.
Updated the remove fuction to use runtime_pm.
Chnages for v4:
- Updated with the review comments.
Changes for v3:
- Converted the driver to use runtime_pm.
Changes for v2:
- Removed the struct platform_device* from suspend/resume
as suggest by Lothar.
drivers/net/can/xilinx_can.c | 157 ++++++++++++++++++++++++++++-------------
1 files changed, 107 insertions(+), 50 deletions(-)
diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 6c67643..67aef00 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -32,6 +32,7 @@
#include <linux/can/dev.h>
#include <linux/can/error.h>
#include <linux/can/led.h>
+#include <linux/pm_runtime.h>
#define DRIVER_NAME "xilinx_can"
@@ -138,7 +139,7 @@ struct xcan_priv {
u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
u32 val);
- struct net_device *dev;
+ struct device *dev;
void __iomem *reg_base;
unsigned long irq_flags;
struct clk *bus_clk;
@@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
struct xcan_priv *priv = netdev_priv(ndev);
int ret;
+ ret = pm_runtime_get_sync(priv->dev);
+ if (ret < 0) {
+ netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
+ __func__, ret);
+ return ret;
+ }
+
ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
ndev->name, ndev);
if (ret < 0) {
@@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
goto err;
}
- ret = clk_prepare_enable(priv->can_clk);
- if (ret) {
- netdev_err(ndev, "unable to enable device clock\n");
- goto err_irq;
- }
-
- ret = clk_prepare_enable(priv->bus_clk);
- if (ret) {
- netdev_err(ndev, "unable to enable bus clock\n");
- goto err_can_clk;
- }
-
/* Set chip into reset mode */
ret = set_reset_mode(ndev);
if (ret < 0) {
netdev_err(ndev, "mode resetting failed!\n");
- goto err_bus_clk;
+ goto err_irq;
}
/* Common open */
ret = open_candev(ndev);
if (ret)
- goto err_bus_clk;
+ goto err_irq;
ret = xcan_chip_start(ndev);
if (ret < 0) {
@@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
err_candev:
close_candev(ndev);
-err_bus_clk:
- clk_disable_unprepare(priv->bus_clk);
-err_can_clk:
- clk_disable_unprepare(priv->can_clk);
err_irq:
free_irq(ndev->irq, ndev);
err:
+ pm_runtime_put(priv->dev);
+
return ret;
}
@@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
netif_stop_queue(ndev);
napi_disable(&priv->napi);
xcan_chip_stop(ndev);
- clk_disable_unprepare(priv->bus_clk);
- clk_disable_unprepare(priv->can_clk);
free_irq(ndev->irq, ndev);
close_candev(ndev);
can_led_event(ndev, CAN_LED_EVENT_STOP);
+ pm_runtime_put(priv->dev);
return 0;
}
@@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
struct xcan_priv *priv = netdev_priv(ndev);
int ret;
- ret = clk_prepare_enable(priv->can_clk);
- if (ret)
- goto err;
-
- ret = clk_prepare_enable(priv->bus_clk);
- if (ret)
- goto err_clk;
+ ret = pm_runtime_get_sync(priv->dev);
+ if (ret < 0) {
+ netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
+ __func__, ret);
+ return ret;
+ }
bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK;
bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
- clk_disable_unprepare(priv->bus_clk);
- clk_disable_unprepare(priv->can_clk);
+ pm_runtime_put(priv->dev);
return 0;
-
-err_clk:
- clk_disable_unprepare(priv->can_clk);
-err:
- return ret;
}
@@ -967,15 +953,45 @@ static const struct net_device_ops xcan_netdev_ops = {
/**
* xcan_suspend - Suspend method for the driver
- * @dev: Address of the platform_device structure
+ * @dev: Address of the device structure
*
* Put the driver into low power mode.
- * Return: 0 always
+ * Return: 0 on success and failure value on error
*/
static int __maybe_unused xcan_suspend(struct device *dev)
{
- struct platform_device *pdev = dev_get_drvdata(dev);
- struct net_device *ndev = platform_get_drvdata(pdev);
+ if (!device_may_wakeup(dev))
+ return pm_runtime_force_suspend(dev);
+
+ return 0;
+}
+
+/**
+ * xcan_resume - Resume from suspend
+ * @dev: Address of the device structure
+ *
+ * Resume operation after suspend.
+ * Return: 0 on success and failure value on error
+ */
+static int __maybe_unused xcan_resume(struct device *dev)
+{
+ if (!device_may_wakeup(dev))
+ return pm_runtime_force_resume(dev);
+
+ return 0;
+
+}
+
+/**
+ * xcan_runtime_suspend - Runtime suspend method for the driver
+ * @dev: Address of the device structure
+ *
+ * Put the driver into low power mode.
+ * Return: 0 always
+ */
+static int __maybe_unused xcan_runtime_suspend(struct device *dev)
+{
+ struct net_device *ndev = dev_get_drvdata(dev);
struct xcan_priv *priv = netdev_priv(ndev);
if (netif_running(ndev)) {
@@ -993,18 +1009,18 @@ static int __maybe_unused xcan_suspend(struct device *dev)
}
/**
- * xcan_resume - Resume from suspend
- * @dev: Address of the platformdevice structure
+ * xcan_runtime_resume - Runtime resume from suspend
+ * @dev: Address of the device structure
*
* Resume operation after suspend.
* Return: 0 on success and failure value on error
*/
-static int __maybe_unused xcan_resume(struct device *dev)
+static int __maybe_unused xcan_runtime_resume(struct device *dev)
{
- struct platform_device *pdev = dev_get_drvdata(dev);
- struct net_device *ndev = platform_get_drvdata(pdev);
+ struct net_device *ndev = dev_get_drvdata(dev);
struct xcan_priv *priv = netdev_priv(ndev);
int ret;
+ u32 isr, status;
ret = clk_enable(priv->bus_clk);
if (ret) {
@@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
ret = clk_enable(priv->can_clk);
if (ret) {
dev_err(dev, "Cannot enable clock.\n");
- clk_disable_unprepare(priv->bus_clk);
+ clk_disable(priv->bus_clk);
return ret;
}
priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
- priv->can.state = CAN_STATE_ERROR_ACTIVE;
+ isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
+ status = priv->read_reg(priv, XCAN_SR_OFFSET);
if (netif_running(ndev)) {
+ if (isr & XCAN_IXR_BSOFF_MASK) {
+ priv->can.state = CAN_STATE_BUS_OFF;
+ priv->write_reg(priv, XCAN_SRR_OFFSET,
+ XCAN_SRR_RESET_MASK);
+ } else if ((status & XCAN_SR_ESTAT_MASK) ==
+ XCAN_SR_ESTAT_MASK) {
+ priv->can.state = CAN_STATE_ERROR_PASSIVE;
+ } else if (status & XCAN_SR_ERRWRN_MASK) {
+ priv->can.state = CAN_STATE_ERROR_WARNING;
+ } else {
+ priv->can.state = CAN_STATE_ERROR_ACTIVE;
+ }
netif_device_attach(ndev);
netif_start_queue(ndev);
}
@@ -1030,7 +1059,10 @@ static int __maybe_unused xcan_resume(struct device *dev)
return 0;
}
-static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
+static const struct dev_pm_ops xcan_dev_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
+ SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
+};
/**
* xcan_probe - Platform registration call
@@ -1071,7 +1103,7 @@ static int xcan_probe(struct platform_device *pdev)
return -ENOMEM;
priv = netdev_priv(ndev);
- priv->dev = ndev;
+ priv->dev = &pdev->dev;
priv->can.bittiming_const = &xcan_bittiming_const;
priv->can.do_set_mode = xcan_do_set_mode;
priv->can.do_get_berr_counter = xcan_get_berr_counter;
@@ -1137,6 +1169,16 @@ static int xcan_probe(struct platform_device *pdev)
netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_irq_safe(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret < 0) {
+ netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
+ __func__, ret);
+ goto err_pmdisable;
+ }
+
ret = register_candev(ndev);
if (ret) {
dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
@@ -1144,15 +1186,19 @@ static int xcan_probe(struct platform_device *pdev)
}
devm_can_led_init(ndev);
- clk_disable_unprepare(priv->bus_clk);
- clk_disable_unprepare(priv->can_clk);
+
+ pm_runtime_put(&pdev->dev);
+
netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth:%d\n",
priv->reg_base, ndev->irq, priv->can.clock.freq,
priv->tx_max);
return 0;
+err_pmdisable:
+ pm_runtime_disable(&pdev->dev);
err_unprepare_disable_busclk:
+ pm_runtime_put(priv->dev);
clk_disable_unprepare(priv->bus_clk);
err_unprepare_disable_dev:
clk_disable_unprepare(priv->can_clk);
@@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
{
struct net_device *ndev = platform_get_drvdata(pdev);
struct xcan_priv *priv = netdev_priv(ndev);
+ int ret;
+
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret < 0) {
+ netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
+ __func__, ret);
+ return ret;
+ }
if (set_reset_mode(ndev) < 0)
netdev_err(ndev, "mode resetting failed!\n");
unregister_candev(ndev);
+ pm_runtime_disable(&pdev->dev);
netif_napi_del(&priv->napi);
+ clk_disable_unprepare(priv->bus_clk);
+ clk_disable_unprepare(priv->can_clk);
free_candev(ndev);
return 0;
--
1.7.4
^ permalink raw reply related
* RE: [PATCH v4] can: Convert to runtime_pm
From: Appana Durga Kedareswara Rao @ 2015-01-12 15:04 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: linux-can@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Soren Brinkmann,
grant.likely@linaro.org, wg@grandegger.com, Michal Simek
In-Reply-To: <54B3D1C0.200@pengutronix.de>
Hi Marc,
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Monday, January 12, 2015 7:23 PM
> To: Appana Durga Kedareswara Rao
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> wg@grandegger.com; Michal Simek
> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>
> On 01/12/2015 02:49 PM, Appana Durga Kedareswara Rao wrote:
> > Hi Marc,
> >
> >> -----Original Message-----
> >> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> >> Sent: Monday, January 12, 2015 6:56 PM
> >> To: Appana Durga Kedareswara Rao
> >> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> >> wg@grandegger.com; Michal Simek
> >> Subject: Re: [PATCH v4] can: Convert to runtime_pm
> >>
> >> On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote:
> >>> Hi Marc,
> >>>
> >>>> -----Original Message-----
> >>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> >>>> Sent: Sunday, January 11, 2015 9:11 PM
> >>>> To: Appana Durga Kedareswara Rao
> >>>> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> >>>> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> >>>> wg@grandegger.com
> >>>> Subject: Re: [PATCH v4] can: Convert to runtime_pm
> >>>>
> >>>> On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
> >>>> [...]
> >>>>>>> return ret;
> >>>>>>> }
> >>>>>>>
> >>>>>>> priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >>>>>>> priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >>>>>>>
> >>>>>>> if (netif_running(ndev)) {
> >>>>>>> priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>>>
> >>>>>> What happens if the device was not in ACTIVE state prior to the
> >>>>>> runtime_suspend?
> >>>>>>
> >>>>>
> >>>>> I am not sure about the state of CAN at this point of time.
> >>>>> I just followed what other drivers are following for run time suspend
> :).
> >>>>
> >>>> Please check the state of the hardware if you go with bus off into
> >>>> suspend and then resume.
> >>>>
> >>>
> >>> if (netif_running(ndev)) {
> >>> if (isr & XCAN_IXR_BSOFF_MASK) {
> >>> priv->can.state = CAN_STATE_BUS_OFF;
> >>> priv->write_reg(priv, XCAN_SRR_OFFSET,
> >>> XCAN_SRR_RESET_MASK);
> >>> } else if ((status & XCAN_SR_ESTAT_MASK) ==
> >>> XCAN_SR_ESTAT_MASK) {
> >>> priv->can.state = CAN_STATE_ERROR_PASSIVE;
> >>> } else if (status & XCAN_SR_ERRWRN_MASK) {
> >>> priv->can.state = CAN_STATE_ERROR_WARNING;
> >>> } else {
> >>> priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>> }
> >>> }
> >>>
> >>> Is the above code snippet ok for you?
> >>
> >> Yes, but what's the state of the hardware when it wakes up again?
> >
> > It depends on the previous state of the CAN.
> > I mean In Suspend we are putting the device in sleep mode and in
> > resume we are waking up by putting the device into the Configuration
> > mode. We are not doing any reset of the core in the suspend/resume so it
> depends on the previous state of the CAN when it wakes up that's why
> checking for the status of the CAN in the status register here to put the
> device in appropriate mode.
>
> I understand the software side, but I don't know how your hardware
> behaves. This is why I'm asking.
As far as I know the above is the our can controller behavior. Anyway I will check with the h/w team
And will get back to you regarding this. Meanwhile I am sending the next version of the patch
Please review it.
Regards,
Kedar.
>
> >
> >>
> >>>
> >>>>>>> netif_device_attach(ndev);
> >>>>>>> netif_start_queue(ndev);
> >>>>>>> }
> >>>>>>>
> >>>>>>> return 0;
> >>>>>>> }
> >>>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> @@ -1020,9 +1035,9 @@ static int __maybe_unused
> >> xcan_resume(struct
> >>>>>>> device *dev)
> >>>>>>>
> >>>>>>> priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >>>>>>> priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >>>>>>> - priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>>>>
> >>>>>>> if (netif_running(ndev)) {
> >>>>>>> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>>>> netif_device_attach(ndev);
> >>>>>>> netif_start_queue(ndev);
> >>>>>>> }
> >>>>>>> @@ -1030,7 +1045,10 @@ static int __maybe_unused
> >>>> xcan_resume(struct
> >>>>>> device *dev)
> >>>>>>> return 0;
> >>>>>>> }
> >>>>>>>
> >>>>>>> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,
> >>>>>> xcan_resume);
> >>>>>>> +static const struct dev_pm_ops xcan_dev_pm_ops = {
> >>>>>>> + SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
> >>>>>>> + SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend,
> >>>>>> xcan_runtime_resume,
> >>>>>>> +NULL) };
> >>>>>>>
> >>>>>>> /**
> >>>>>>> * xcan_probe - Platform registration call @@ -1071,7 +1089,7
> >>>>>>> @@ static int xcan_probe(struct platform_device *pdev)
> >>>>>>> return -ENOMEM;
> >>>>>>>
> >>>>>>> priv = netdev_priv(ndev);
> >>>>>>> - priv->dev = ndev;
> >>>>>>> + priv->dev = &pdev->dev;
> >>>>>>> priv->can.bittiming_const = &xcan_bittiming_const;
> >>>>>>> priv->can.do_set_mode = xcan_do_set_mode;
> >>>>>>> priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -
> >>>>>> 1137,15
> >>>>>>> +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
> >>>>>>>
> >>>>>>> netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
> >>>>>>>
> >>>>>>> + pm_runtime_set_active(&pdev->dev);
> >>>>>>> + pm_runtime_irq_safe(&pdev->dev);
> >>>>>>> + pm_runtime_enable(&pdev->dev);
> >>>>>>> + pm_runtime_get_sync(&pdev->dev);
> >>>>>> Check error values?
> >>>>>
> >>>>> Ok
> >>>>>>> +
> >>>>>>> ret = register_candev(ndev);
> >>>>>>> if (ret) {
> >>>>>>> dev_err(&pdev->dev, "fail to register failed
> >>>>>>> (err=%d)\n",
> >>>>>> ret);
> >>>>>>> + pm_runtime_put(priv->dev);
> >>>>>>
> >>>>>> Please move the pm_runtime_put into the common error exit path.
> >>>>>>
> >>>>>
> >>>>> Ok
> >>>>>
> >>>>>>> goto err_unprepare_disable_busclk;
> >>>>>>> }
> >>>>>>>
> >>>>>>> devm_can_led_init(ndev);
> >>>>>>> - clk_disable_unprepare(priv->bus_clk);
> >>>>>>> - clk_disable_unprepare(priv->can_clk);
> >>>>>>> +
> >>>>>>> + pm_runtime_put(&pdev->dev);
> >>>>>>> +
> >>>>>>> netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo
> >>>>>> depth:%d\n",
> >>>>>>> priv->reg_base, ndev->irq, priv->can.clock.freq,
> >>>>>>> priv->tx_max);
> >>>>>>>
> >>>>>>
> >>>>>> I think you have to convert the _remove() function, too. Have a
> >>>>>> look at the gpio-zynq.c driver:
> >>>>>>
> >>>>>>> static int zynq_gpio_remove(struct platform_device *pdev) {
> >>>>>>> struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> >>>>>>>
> >>>>>>> pm_runtime_get_sync(&pdev->dev);
> >>>>>>
> >>>>>> However I don't understand why the get_sync() is here. Maybe
> >>>>>> Sören can help?
> >>>>>
> >>>>> I converted the remove function to use the run-time PM and .
> >>>>> Below is the remove code snippet.
> >>>>>
> >>>>> ret = pm_runtime_get_sync(&pdev->dev);
> >>>>> if (ret < 0) {
> >>>>> netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> >>>>> __func__, ret);
> >>>>> return ret;
> >>>>> }
> >>>>>
> >>>>> if (set_reset_mode(ndev) < 0)
> >>>>> netdev_err(ndev, "mode resetting failed!\n");
> >>>>>
> >>>>> unregister_candev(ndev);
> >>>>> netif_napi_del(&priv->napi);
> >>>>> free_candev(ndev);
> >>>>
> >>>>> pm_runtime_disable(&pdev->dev);
> >>>>
> >>>> Can this make a call to xcan_runtime_*()? I'm asking since the ndev
> >>>> has been unregistered and already free()ed. Better move this
> >>>> directly after the set_reset_mode(). This way you are symmetric to
> >>>> the probe()
> >> function.
> >>>
> >>> If I move the pm_runtime_disable after the set_reset_mode I am
> >>> getting the below error.
> >>> ERROR:
> >>> xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
> >>> pm_runtime_get fail
> >>>
> >>> If I move the pm_runtime_disable after unregister_candev everything
> >>> is
> >> working fine.
> >>
> >> Fine - but who calls xcan_get_berr_counter here? Can you add a
> >> dump_stack() here?
> >>
> >
> > I think it is getting called from the atomic context.
> > When I am trying to do a rmmod I am getting the above error.
> > ERROR:
> > xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
> > pm_runtime_get fail.
> >
> > I am getting only the above error in the console when I do rmmod.
>
> Put a dump_stack into xcan_get_berr_counter(), then you'll see where it's
> called from. However calling from atomic context should be fine.
>
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Industrial Linux Solutions | Phone: +49-231-2826-924 |
> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
^ permalink raw reply
* Re: [PATCH net-next v2] tcp: avoid reducing cwnd when ACK+DSACK is received
From: Neal Cardwell @ 2015-01-12 15:02 UTC (permalink / raw)
To: David Laight
Cc: Yuchung Cheng, Eric Dumazet, Sébastien Barré,
David Miller, Netdev, Gregory Detal, Nandita Dukkipati
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1CAC5844@AcuExch.aculab.com>
On Mon, Jan 12, 2015 at 6:52 AM, David Laight <David.Laight@aculab.com> wrote:
>> if (flag & FLAG_DSACKING_ACK) {
>> /* This DSACK means original and TLP probe arrived; no loss */
>> tp->tlp_high_seq = 0;
>
> I think I'd add a 'return' here.
What's the benefit of adding 'return' in those two spots? That adds
extra code to read, with no change in behavior, and no increase in
maintainability.
neal
^ permalink raw reply
* Re: [PATCH] gianfar: correct the bad expression while writing bit-pattern
From: Claudiu Manoil @ 2015-01-12 14:22 UTC (permalink / raw)
To: Sanjeev Sharma; +Cc: davem, peter.senna, shemminger, netdev, linux-kernel
In-Reply-To: <1421048596-22639-1-git-send-email-sanjeev_sharma@mentor.com>
On 1/12/2015 9:43 AM, Sanjeev Sharma wrote:
> This patch correct the bad expression while writing the
> bit-pattern from software's buffer to hardware registers.
>
> Signed-off-by: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>
> ---
> drivers/net/ethernet/freescale/gianfar_ethtool.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> index 3e1a9c1..1ccca72 100644
> --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> @@ -1586,7 +1586,7 @@ static int gfar_write_filer_table(struct gfar_private *priv,
> return -EBUSY;
>
> /* Fill regular entries */
> - for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].ctrl);
> + for (; i < MAX_FILER_IDX - 1 && ( i < tab->fe[i].ctrl);
> i++)
Why do you think 'i' can be compared with the 'ctrl' field?
Is the control field an index (provide proof if yes)? I doubt it...
Thanks,
Claudiu
^ permalink raw reply
* Re: [PATCH] gianfar: correct the bad expression while writing bit-pattern
From: Sergei Shtylyov @ 2015-01-12 14:12 UTC (permalink / raw)
To: Sanjeev Sharma, davem
Cc: claudiu.manoil, peter.senna, shemminger, netdev, linux-kernel
In-Reply-To: <1421048596-22639-1-git-send-email-sanjeev_sharma@mentor.com>
Hello.
On 1/12/2015 10:43 AM, Sanjeev Sharma wrote:
> This patch correct the bad expression while writing the
> bit-pattern from software's buffer to hardware registers.
> Signed-off-by: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>
> ---
> drivers/net/ethernet/freescale/gianfar_ethtool.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> index 3e1a9c1..1ccca72 100644
> --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> @@ -1586,7 +1586,7 @@ static int gfar_write_filer_table(struct gfar_private *priv,
> return -EBUSY;
>
> /* Fill regular entries */
> - for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].ctrl);
> + for (; i < MAX_FILER_IDX - 1 && ( i < tab->fe[i].ctrl);
Inner parens not needed. And there shouldn't be space after (.
WBR, Sergei
^ permalink raw reply
* Re: [PATCH] brcmfmac: avoid duplicated suspend/resume operation
From: Sergei Shtylyov @ 2015-01-12 14:06 UTC (permalink / raw)
To: Fu, Zhonghui, brudley,
arend@broadcom.com >> Arend van Spriel, Franky Lin,
meuleman, kvalo, linville, pieterpg, hdegoede, wens,
linux-wireless, brcm80211-dev-list, netdev,
linux-kernel@vger.kernel.org
In-Reply-To: <54B36C88.3030609@linux.intel.com>
Hello.
On 1/12/2015 9:41 AM, Fu, Zhonghui wrote:
> From 8685c3c2746b4275fc808d9db23c364b2f54b52a Mon Sep 17 00:00:00 2001
> From: Zhonghui Fu <zhonghui.fu@linux.intel.com>
> Date: Mon, 12 Jan 2015 14:25:46 +0800
> Subject: [PATCH] brcmfmac: avoid duplicated suspend/resume operation
The lines above are not needed.
> WiFi chip has 2 SDIO functions, and PM core will trigger
> twice suspend/resume operations for one WiFi chip to do
> the same things. This patch avoid this case.
> Acked-by: Arend van Spriel<arend@broadcom.com>
> Acked-by: Sergei Shtylyov<sergei.shtylyov@cogentembedded.com>
> Signed-off-by: Zhonghui Fu <zhonghui.fu@linux.intel.com>
> ---
> drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 21 +++++++++++++++++----
> 1 files changed, 17 insertions(+), 4 deletions(-)
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
> index 9880dae..8f71485 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
> @@ -1070,7 +1070,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
> */
> if ((sdio_get_host_pm_caps(sdiodev->func[1]) & MMC_PM_KEEP_POWER) &&
> ((sdio_get_host_pm_caps(sdiodev->func[1]) & MMC_PM_WAKE_SDIO_IRQ) ||
> - (sdiodev->pdata && sdiodev->pdata->oob_irq_supported)))
> + (sdiodev->pdata->oob_irq_supported)))
Inner parens not needed on this line.
[...]
WBR, Sergei
^ permalink raw reply
* Re: [PATCH v4] can: Convert to runtime_pm
From: Marc Kleine-Budde @ 2015-01-12 13:53 UTC (permalink / raw)
To: Appana Durga Kedareswara Rao
Cc: linux-can@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Soren Brinkmann,
grant.likely@linaro.org, wg@grandegger.com, Michal Simek
In-Reply-To: <4a61c12b2e1d4103a41a3faf1d58837a@BN1BFFO11FD030.protection.gbl>
[-- Attachment #1: Type: text/plain, Size: 8610 bytes --]
On 01/12/2015 02:49 PM, Appana Durga Kedareswara Rao wrote:
> Hi Marc,
>
>> -----Original Message-----
>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>> Sent: Monday, January 12, 2015 6:56 PM
>> To: Appana Durga Kedareswara Rao
>> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
>> wg@grandegger.com; Michal Simek
>> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>>
>> On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote:
>>> Hi Marc,
>>>
>>>> -----Original Message-----
>>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>>>> Sent: Sunday, January 11, 2015 9:11 PM
>>>> To: Appana Durga Kedareswara Rao
>>>> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
>>>> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
>>>> wg@grandegger.com
>>>> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>>>>
>>>> On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
>>>> [...]
>>>>>>> return ret;
>>>>>>> }
>>>>>>>
>>>>>>> priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
>>>>>>> priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
>>>>>>>
>>>>>>> if (netif_running(ndev)) {
>>>>>>> priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>>
>>>>>> What happens if the device was not in ACTIVE state prior to the
>>>>>> runtime_suspend?
>>>>>>
>>>>>
>>>>> I am not sure about the state of CAN at this point of time.
>>>>> I just followed what other drivers are following for run time suspend :).
>>>>
>>>> Please check the state of the hardware if you go with bus off into
>>>> suspend and then resume.
>>>>
>>>
>>> if (netif_running(ndev)) {
>>> if (isr & XCAN_IXR_BSOFF_MASK) {
>>> priv->can.state = CAN_STATE_BUS_OFF;
>>> priv->write_reg(priv, XCAN_SRR_OFFSET,
>>> XCAN_SRR_RESET_MASK);
>>> } else if ((status & XCAN_SR_ESTAT_MASK) ==
>>> XCAN_SR_ESTAT_MASK) {
>>> priv->can.state = CAN_STATE_ERROR_PASSIVE;
>>> } else if (status & XCAN_SR_ERRWRN_MASK) {
>>> priv->can.state = CAN_STATE_ERROR_WARNING;
>>> } else {
>>> priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>> }
>>> }
>>>
>>> Is the above code snippet ok for you?
>>
>> Yes, but what's the state of the hardware when it wakes up again?
>
> It depends on the previous state of the CAN.
> I mean In Suspend we are putting the device in sleep mode and in resume we are waking up by putting the device into the
> Configuration mode. We are not doing any reset of the core in the suspend/resume so it depends on the previous state of the CAN
> when it wakes up that's why checking for the status of the CAN in the status register here to put the device in appropriate mode.
I understand the software side, but I don't know how your hardware
behaves. This is why I'm asking.
>
>>
>>>
>>>>>>> netif_device_attach(ndev);
>>>>>>> netif_start_queue(ndev);
>>>>>>> }
>>>>>>>
>>>>>>> return 0;
>>>>>>> }
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> @@ -1020,9 +1035,9 @@ static int __maybe_unused
>> xcan_resume(struct
>>>>>>> device *dev)
>>>>>>>
>>>>>>> priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
>>>>>>> priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
>>>>>>> - priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>>>
>>>>>>> if (netif_running(ndev)) {
>>>>>>> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>>> netif_device_attach(ndev);
>>>>>>> netif_start_queue(ndev);
>>>>>>> }
>>>>>>> @@ -1030,7 +1045,10 @@ static int __maybe_unused
>>>> xcan_resume(struct
>>>>>> device *dev)
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,
>>>>>> xcan_resume);
>>>>>>> +static const struct dev_pm_ops xcan_dev_pm_ops = {
>>>>>>> + SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
>>>>>>> + SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend,
>>>>>> xcan_runtime_resume,
>>>>>>> +NULL) };
>>>>>>>
>>>>>>> /**
>>>>>>> * xcan_probe - Platform registration call @@ -1071,7 +1089,7 @@
>>>>>>> static int xcan_probe(struct platform_device *pdev)
>>>>>>> return -ENOMEM;
>>>>>>>
>>>>>>> priv = netdev_priv(ndev);
>>>>>>> - priv->dev = ndev;
>>>>>>> + priv->dev = &pdev->dev;
>>>>>>> priv->can.bittiming_const = &xcan_bittiming_const;
>>>>>>> priv->can.do_set_mode = xcan_do_set_mode;
>>>>>>> priv->can.do_get_berr_counter = xcan_get_berr_counter; @@ -
>>>>>> 1137,15
>>>>>>> +1155,22 @@ static int xcan_probe(struct platform_device *pdev)
>>>>>>>
>>>>>>> netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
>>>>>>>
>>>>>>> + pm_runtime_set_active(&pdev->dev);
>>>>>>> + pm_runtime_irq_safe(&pdev->dev);
>>>>>>> + pm_runtime_enable(&pdev->dev);
>>>>>>> + pm_runtime_get_sync(&pdev->dev);
>>>>>> Check error values?
>>>>>
>>>>> Ok
>>>>>>> +
>>>>>>> ret = register_candev(ndev);
>>>>>>> if (ret) {
>>>>>>> dev_err(&pdev->dev, "fail to register failed
>>>>>>> (err=%d)\n",
>>>>>> ret);
>>>>>>> + pm_runtime_put(priv->dev);
>>>>>>
>>>>>> Please move the pm_runtime_put into the common error exit path.
>>>>>>
>>>>>
>>>>> Ok
>>>>>
>>>>>>> goto err_unprepare_disable_busclk;
>>>>>>> }
>>>>>>>
>>>>>>> devm_can_led_init(ndev);
>>>>>>> - clk_disable_unprepare(priv->bus_clk);
>>>>>>> - clk_disable_unprepare(priv->can_clk);
>>>>>>> +
>>>>>>> + pm_runtime_put(&pdev->dev);
>>>>>>> +
>>>>>>> netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo
>>>>>> depth:%d\n",
>>>>>>> priv->reg_base, ndev->irq, priv->can.clock.freq,
>>>>>>> priv->tx_max);
>>>>>>>
>>>>>>
>>>>>> I think you have to convert the _remove() function, too. Have a
>>>>>> look at the gpio-zynq.c driver:
>>>>>>
>>>>>>> static int zynq_gpio_remove(struct platform_device *pdev) {
>>>>>>> struct zynq_gpio *gpio = platform_get_drvdata(pdev);
>>>>>>>
>>>>>>> pm_runtime_get_sync(&pdev->dev);
>>>>>>
>>>>>> However I don't understand why the get_sync() is here. Maybe Sören
>>>>>> can help?
>>>>>
>>>>> I converted the remove function to use the run-time PM and .
>>>>> Below is the remove code snippet.
>>>>>
>>>>> ret = pm_runtime_get_sync(&pdev->dev);
>>>>> if (ret < 0) {
>>>>> netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
>>>>> __func__, ret);
>>>>> return ret;
>>>>> }
>>>>>
>>>>> if (set_reset_mode(ndev) < 0)
>>>>> netdev_err(ndev, "mode resetting failed!\n");
>>>>>
>>>>> unregister_candev(ndev);
>>>>> netif_napi_del(&priv->napi);
>>>>> free_candev(ndev);
>>>>
>>>>> pm_runtime_disable(&pdev->dev);
>>>>
>>>> Can this make a call to xcan_runtime_*()? I'm asking since the ndev
>>>> has been unregistered and already free()ed. Better move this directly
>>>> after the set_reset_mode(). This way you are symmetric to the probe()
>> function.
>>>
>>> If I move the pm_runtime_disable after the set_reset_mode I am
>>> getting the below error.
>>> ERROR:
>>> xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
>>> pm_runtime_get fail
>>>
>>> If I move the pm_runtime_disable after unregister_candev everything is
>> working fine.
>>
>> Fine - but who calls xcan_get_berr_counter here? Can you add a
>> dump_stack() here?
>>
>
> I think it is getting called from the atomic context.
> When I am trying to do a rmmod I am getting the above error.
> ERROR:
> xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
> pm_runtime_get fail.
>
> I am getting only the above error in the console when I do rmmod.
Put a dump_stack into xcan_get_berr_counter(), then you'll see where
it's called from. However calling from atomic context should be fine.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox