* Re: [PATCH net] net/xdp: Fix suspicious RCU usage warning
From: Tariq Toukan @ 2018-07-18 14:13 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov, Tariq Toukan
Cc: David S. Miller, netdev, Eran Ben Elisha, Jesper Dangaard Brouer
In-Reply-To: <18c18ecb-173a-2df0-1189-b98b10624bc6@iogearbox.net>
On 17/07/2018 10:27 PM, Daniel Borkmann wrote:
> On 07/17/2018 06:47 PM, Alexei Starovoitov wrote:
>> On Tue, Jul 17, 2018 at 06:10:38PM +0300, Tariq Toukan wrote:
>>> Fix the warning below by calling rhashtable_lookup under
>>> RCU read lock.
>>>
...
>>> mutex_lock(&mem_id_lock);
>>>
>>> + rcu_read_lock();
>>> xa = rhashtable_lookup(mem_id_ht, &id, mem_id_rht_params);
>>> + rcu_read_unlock();
>>> if (!xa) {
>>
>> if it's an actual bug rcu_read_unlock seems to be misplaced.
>> It silences the warn, but rcu section looks wrong.
>
> I think that whole piece in __xdp_rxq_info_unreg_mem_model() should be:
>
> mutex_lock(&mem_id_lock);
> xa = rhashtable_lookup_fast(mem_id_ht, &id, mem_id_rht_params);
> if (xa && rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params) == 0)
> call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free);
> mutex_unlock(&mem_id_lock);
>
> Technically the RCU read side plus rhashtable_lookup() is the same, but lets
> use proper api. From the doc (https://lwn.net/Articles/751374/) object removal
> is wrapped around the RCU read side additionally, but in our case we're behind
> mem_id_lock for insertion/removal serialization.
>
> Cheers,
> Daniel
>
Just as Daniel stated, I think there's no actual bug here, but we still
want to silence the RCU warning.
Alexei, did you mean getting the if statement into the RCU lock critical
section?
^ permalink raw reply
* Re: [PATCH mlx5-next] RDMA/mlx5: Don't use cached IRQ affinity mask
From: Max Gurtovoy @ 2018-07-18 14:14 UTC (permalink / raw)
To: Sagi Grimberg, Steve Wise, 'Leon Romanovsky'
Cc: 'Doug Ledford', 'Jason Gunthorpe',
'RDMA mailing list', 'Saeed Mahameed',
'linux-netdev'
In-Reply-To: <0834cae6-33d6-3526-7d85-f5cae18c5487@grimberg.me>
On 7/18/2018 2:38 PM, Sagi Grimberg wrote:
>
>>> IMO we must fulfil the user wish to connect to N queues and not reduce
>>> it because of affinity overlaps. So in order to push Leon's patch we
>>> must also fix the blk_mq_rdma_map_queues to do a best effort mapping
>>> according the affinity and map the rest in naive way (in that way we
>>> will *always* map all the queues).
>>
>> That is what I would expect also. For example, in my node, where
>> there are
>> 16 cpus, and 2 numa nodes, I observe much better nvmf IOPS performance by
>> setting up my 16 driver completion event queues such that each is
>> bound to a
>> node-local cpu. So I end up with each nodel-local cpu having 2 queues
>> bound
>> to it. W/O adding support in iw_cxgb4 for ib_get_vector_affinity(),
>> this
>> works fine. I assumed adding ib_get_vector_affinity() would allow
>> this to
>> all "just work" by default, but I'm running into this connection failure
>> issue.
>>
>> I don't understand exactly what the blk_mq layer is trying to do, but I
>> assume it has ingress event queues and processing that it trying to align
>> with the drivers ingress cq event handling, so everybody stays on the
>> same
>> cpu (or at least node). But something else is going on. Is there
>> documentation on how this works somewhere?
>
> Does this (untested) patch help?
I'm not sure (I'll test it tomorrow) because the issue is the unmapped
queues and not the cpus.
for example, if the affinity of q=6 and q=12 returned the same cpumask
than q=6 will not be mapped and will fail to connect.
> --
> diff --git a/block/blk-mq-cpumap.c b/block/blk-mq-cpumap.c
> index 3eb169f15842..dbe962cb537d 100644
> --- a/block/blk-mq-cpumap.c
> +++ b/block/blk-mq-cpumap.c
> @@ -30,29 +30,34 @@ static int get_first_sibling(unsigned int cpu)
> return cpu;
> }
>
> -int blk_mq_map_queues(struct blk_mq_tag_set *set)
> +void blk_mq_map_queue(struct blk_mq_tag_set *set, unsigned int cpu)
> {
> unsigned int *map = set->mq_map;
> unsigned int nr_queues = set->nr_hw_queues;
> - unsigned int cpu, first_sibling;
> + unsigned int first_sibling;
>
> - for_each_possible_cpu(cpu) {
> - /*
> - * First do sequential mapping between CPUs and queues.
> - * In case we still have CPUs to map, and we have some
> number of
> - * threads per cores then map sibling threads to the
> same queue for
> - * performace optimizations.
> - */
> - if (cpu < nr_queues) {
> + /*
> + * First do sequential mapping between CPUs and queues.
> + * In case we still have CPUs to map, and we have some number of
> + * threads per cores then map sibling threads to the same queue for
> + * performace optimizations.
> + */
> + if (cpu < nr_queues) {
> + map[cpu] = cpu_to_queue_index(nr_queues, cpu);
> + } else {
> + first_sibling = get_first_sibling(cpu);
> + if (first_sibling == cpu)
> map[cpu] = cpu_to_queue_index(nr_queues, cpu);
> - } else {
> - first_sibling = get_first_sibling(cpu);
> - if (first_sibling == cpu)
> - map[cpu] = cpu_to_queue_index(nr_queues,
> cpu);
> - else
> - map[cpu] = map[first_sibling];
> - }
> + else
> + map[cpu] = map[first_sibling];
> }
> +}
> +EXPORT_SYMBOL_GPL(blk_mq_map_queue);
> +
> +int blk_mq_map_queues(struct blk_mq_tag_set *set)
> +{
> + for_each_possible_cpu(cpu)
> + blk_mq_map_queue(set, cpu);
>
> return 0;
> }
> diff --git a/block/blk-mq-rdma.c b/block/blk-mq-rdma.c
> index 996167f1de18..5e91789bea5b 100644
> --- a/block/blk-mq-rdma.c
> +++ b/block/blk-mq-rdma.c
> @@ -35,6 +35,10 @@ int blk_mq_rdma_map_queues(struct blk_mq_tag_set *set,
> const struct cpumask *mask;
> unsigned int queue, cpu;
>
> + /* reset all to */
> + for_each_possible_cpu(cpu)
> + set->mq_map[cpu] = UINT_MAX;
> +
> for (queue = 0; queue < set->nr_hw_queues; queue++) {
> mask = ib_get_vector_affinity(dev, first_vec + queue);
> if (!mask)
> @@ -44,6 +48,11 @@ int blk_mq_rdma_map_queues(struct blk_mq_tag_set *set,
> set->mq_map[cpu] = queue;
> }
>
> + for_each_possible_cpu(cpu) {
> + if (set->mq_map[cpu] == UINT_MAX)
> + blk_mq_map_queue(set, cpu);
> + }
> +
> return 0;
>
> fallback:
> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index e3147eb74222..7a9848a82475 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -283,6 +283,7 @@ int blk_mq_freeze_queue_wait_timeout(struct
> request_queue *q,
> unsigned long timeout);
>
> int blk_mq_map_queues(struct blk_mq_tag_set *set);
> +void blk_mq_map_queue(struct blk_mq_tag_set *set, unsigned int cpu);
> void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int
> nr_hw_queues);
>
> void blk_mq_quiesce_queue_nowait(struct request_queue *q);
> --
>
> It really is still a best effort thing...
^ permalink raw reply
* RE: [PATCH mlx5-next] RDMA/mlx5: Don't use cached IRQ affinity mask
From: Steve Wise @ 2018-07-18 14:25 UTC (permalink / raw)
To: 'Max Gurtovoy', 'Sagi Grimberg',
'Leon Romanovsky'
Cc: 'Doug Ledford', 'Jason Gunthorpe',
'RDMA mailing list', 'Saeed Mahameed',
'linux-netdev'
In-Reply-To: <9a4d8d50-19b0-fcaa-d4a3-6cfa2318a973@mellanox.com>
> -----Original Message-----
> From: Max Gurtovoy <maxg@mellanox.com>
> Sent: Wednesday, July 18, 2018 9:14 AM
> To: Sagi Grimberg <sagi@grimberg.me>; Steve Wise
> <swise@opengridcomputing.com>; 'Leon Romanovsky' <leon@kernel.org>
> Cc: 'Doug Ledford' <dledford@redhat.com>; 'Jason Gunthorpe'
> <jgg@mellanox.com>; 'RDMA mailing list' <linux-rdma@vger.kernel.org>;
> 'Saeed Mahameed' <saeedm@mellanox.com>; 'linux-netdev'
> <netdev@vger.kernel.org>
> Subject: Re: [PATCH mlx5-next] RDMA/mlx5: Don't use cached IRQ affinity
> mask
>
>
>
> On 7/18/2018 2:38 PM, Sagi Grimberg wrote:
> >
> >>> IMO we must fulfil the user wish to connect to N queues and not reduce
> >>> it because of affinity overlaps. So in order to push Leon's patch we
> >>> must also fix the blk_mq_rdma_map_queues to do a best effort
> mapping
> >>> according the affinity and map the rest in naive way (in that way we
> >>> will *always* map all the queues).
> >>
> >> That is what I would expect also. For example, in my node, where
> >> there are
> >> 16 cpus, and 2 numa nodes, I observe much better nvmf IOPS
> performance by
> >> setting up my 16 driver completion event queues such that each is
> >> bound to a
> >> node-local cpu. So I end up with each nodel-local cpu having 2 queues
> >> bound
> >> to it. W/O adding support in iw_cxgb4 for ib_get_vector_affinity(),
> >> this
> >> works fine. I assumed adding ib_get_vector_affinity() would allow
> >> this to
> >> all "just work" by default, but I'm running into this connection failure
> >> issue.
> >>
> >> I don't understand exactly what the blk_mq layer is trying to do, but I
> >> assume it has ingress event queues and processing that it trying to align
> >> with the drivers ingress cq event handling, so everybody stays on the
> >> same
> >> cpu (or at least node). But something else is going on. Is there
> >> documentation on how this works somewhere?
> >
> > Does this (untested) patch help?
>
> I'm not sure (I'll test it tomorrow) because the issue is the unmapped
> queues and not the cpus.
> for example, if the affinity of q=6 and q=12 returned the same cpumask
> than q=6 will not be mapped and will fail to connect.
>
> > --
> > diff --git a/block/blk-mq-cpumap.c b/block/blk-mq-cpumap.c
> > index 3eb169f15842..dbe962cb537d 100644
> > --- a/block/blk-mq-cpumap.c
> > +++ b/block/blk-mq-cpumap.c
> > @@ -30,29 +30,34 @@ static int get_first_sibling(unsigned int cpu)
> > return cpu;
> > }
> >
> > -int blk_mq_map_queues(struct blk_mq_tag_set *set)
> > +void blk_mq_map_queue(struct blk_mq_tag_set *set, unsigned int cpu)
> > {
There is already a static inline function named blk_mq_map_queue() in block/blk-mq.h. Did you mean to replace that? Or is this just a function name conflict?
> > unsigned int *map = set->mq_map;
> > unsigned int nr_queues = set->nr_hw_queues;
> > - unsigned int cpu, first_sibling;
> > + unsigned int first_sibling;
> >
> > - for_each_possible_cpu(cpu) {
> > - /*
> > - * First do sequential mapping between CPUs and queues.
> > - * In case we still have CPUs to map, and we have some
> > number of
> > - * threads per cores then map sibling threads to the
> > same queue for
> > - * performace optimizations.
> > - */
> > - if (cpu < nr_queues) {
> > + /*
> > + * First do sequential mapping between CPUs and queues.
> > + * In case we still have CPUs to map, and we have some number of
> > + * threads per cores then map sibling threads to the same queue for
> > + * performace optimizations.
> > + */
> > + if (cpu < nr_queues) {
> > + map[cpu] = cpu_to_queue_index(nr_queues, cpu);
> > + } else {
> > + first_sibling = get_first_sibling(cpu);
> > + if (first_sibling == cpu)
> > map[cpu] = cpu_to_queue_index(nr_queues, cpu);
> > - } else {
> > - first_sibling = get_first_sibling(cpu);
> > - if (first_sibling == cpu)
> > - map[cpu] = cpu_to_queue_index(nr_queues,
> > cpu);
> > - else
> > - map[cpu] = map[first_sibling];
> > - }
> > + else
> > + map[cpu] = map[first_sibling];
> > }
> > +}
> > +EXPORT_SYMBOL_GPL(blk_mq_map_queue);
> > +
> > +int blk_mq_map_queues(struct blk_mq_tag_set *set)
> > +{
> > + for_each_possible_cpu(cpu)
> > + blk_mq_map_queue(set, cpu);
> >
> > return 0;
> > }
> > diff --git a/block/blk-mq-rdma.c b/block/blk-mq-rdma.c
> > index 996167f1de18..5e91789bea5b 100644
> > --- a/block/blk-mq-rdma.c
> > +++ b/block/blk-mq-rdma.c
> > @@ -35,6 +35,10 @@ int blk_mq_rdma_map_queues(struct
> blk_mq_tag_set *set,
> > const struct cpumask *mask;
> > unsigned int queue, cpu;
> >
> > + /* reset all to */
> > + for_each_possible_cpu(cpu)
> > + set->mq_map[cpu] = UINT_MAX;
> > +
> > for (queue = 0; queue < set->nr_hw_queues; queue++) {
> > mask = ib_get_vector_affinity(dev, first_vec + queue);
> > if (!mask)
> > @@ -44,6 +48,11 @@ int blk_mq_rdma_map_queues(struct
> blk_mq_tag_set *set,
> > set->mq_map[cpu] = queue;
> > }
> >
> > + for_each_possible_cpu(cpu) {
> > + if (set->mq_map[cpu] == UINT_MAX)
> > + blk_mq_map_queue(set, cpu);
> > + }
> > +
> > return 0;
> >
> > fallback:
> > diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> > index e3147eb74222..7a9848a82475 100644
> > --- a/include/linux/blk-mq.h
> > +++ b/include/linux/blk-mq.h
> > @@ -283,6 +283,7 @@ int blk_mq_freeze_queue_wait_timeout(struct
> > request_queue *q,
> > unsigned long timeout);
> >
> > int blk_mq_map_queues(struct blk_mq_tag_set *set);
> > +void blk_mq_map_queue(struct blk_mq_tag_set *set, unsigned int cpu);
> > void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int
> > nr_hw_queues);
> >
> > void blk_mq_quiesce_queue_nowait(struct request_queue *q);
> > --
> >
> > It really is still a best effort thing...
^ permalink raw reply
* Re: [PATCH net-next v2 3/7] net: mvneta: increase number of buffers in RX and TX queue
From: Gregory CLEMENT @ 2018-07-18 15:37 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: David S. Miller, linux-kernel, netdev, Andrew Lunn, Jason Cooper,
Antoine Tenart, Maxime Chevallier, Nadav Haklai,
Yelena Krivosheev, Thomas Petazzoni, Miquèl Raynal,
Marcin Wojtas, Dmitri Epshtein, linux-arm-kernel,
Sebastian Hesselbarth
In-Reply-To: <20180713191720.GA17271@n2100.armlinux.org.uk>
Hi Russell King,
On ven., juil. 13 2018, Russell King - ARM Linux <linux@armlinux.org.uk> wrote:
> On Fri, Jul 13, 2018 at 06:18:37PM +0200, Gregory CLEMENT wrote:
>> From: Yelena Krivosheev <yelena@marvell.com>
>>
>> The initial values were too small leading to poor performance when using
>> the software buffer management.
>
> What does this do to latency when a large transfer is also ongoing
> (iow, the classic bufferbloat issue) ?
IXIA latency test had been done without seeing any differences for long
traffic (routing).
These new values offer better performance for the main usage of this SoC
(NAS applications), however both Rx and TX queues size can be change by
ethtool.
Gregory
>
>>
>> Signed-off-by: Yelena Krivosheev <yelena@marvell.com>
>> [gregory: extract from a larger patch]
>> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
>> ---
>> drivers/net/ethernet/marvell/mvneta.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
>> index f4e3943a745d..c22df28b07c8 100644
>> --- a/drivers/net/ethernet/marvell/mvneta.c
>> +++ b/drivers/net/ethernet/marvell/mvneta.c
>> @@ -295,10 +295,10 @@
>> #define MVNETA_RSS_LU_TABLE_SIZE 1
>>
>> /* Max number of Rx descriptors */
>> -#define MVNETA_MAX_RXD 128
>> +#define MVNETA_MAX_RXD 512
>>
>> /* Max number of Tx descriptors */
>> -#define MVNETA_MAX_TXD 532
>> +#define MVNETA_MAX_TXD 1024
>>
>> /* Max number of allowed TCP segments for software TSO */
>> #define MVNETA_MAX_TSO_SEGS 100
>> --
>> 2.18.0
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
> According to speedtest.net: 13Mbps down 490kbps up
--
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
^ permalink raw reply
* [PATCH 0/5] net: extend eth_platform_get_mac_address()
From: Bartosz Golaszewski @ 2018-07-18 16:10 UTC (permalink / raw)
To: Sekhar Nori, Kevin Hilman, Russell King, Grygorii Strashko,
David S . Miller, Srinivas Kandagatla, Lukas Wunner, Rob Herring,
Florian Fainelli, Dan Carpenter, Ivan Khoronzhuk, David Lechner,
Greg Kroah-Hartman, Andrew Lunn
Cc: linux-arm-kernel, linux-kernel, linux-omap, netdev,
Bartosz Golaszewski
This is a follow-up to a series I posted a while ago the goal of which
was to replace the at24 platform data with device properties. To do so
we need to somehow remove reading the MAC address from relevant board
files.
In my patches I used nvmem and MTD to read the MAC address from within
the davinci emac driver. It was suggested that we generalize it further
but since MTD doesn't support nvmem yet, the best we can do is to move
this code over to net core code.
The following patches modify the eth_platform_get_mac_address()
function which seems to be the best candidate for this code.
The first patch only visually shrinks the code because this routine
will grow significantly in the next patches.
The second patch adds an info message on success so that we keep
getting notifications from users who already are verbose.
The third patch calls is_valid_ether_addr() on the read address so
that we're sure it's correct.
The last two patches add nvmem and MTD support to the function. In
order to stay compatible with existing users, nvmem and MTD will be
tried last - after device tree and arch-specific callback.
If this series gets accepted I will modify my previous patches to
use it instead of handcoding the same operations in davinci_emac.
Bartosz Golaszewski (5):
net: visually shrink eth_platform_get_mac_address()
net: add an info message to eth_platform_get_mac_address()
net: fortify eth_platform_get_mac_address()
net: add support for nvmem to eth_platform_get_mac_address()
net: add MTD support to eth_platform_get_mac_address()
net/ethernet/eth.c | 77 ++++++++++++++++++++++++++++++++++++++--------
1 file changed, 65 insertions(+), 12 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH 3/5] net: fortify eth_platform_get_mac_address()
From: Bartosz Golaszewski @ 2018-07-18 16:10 UTC (permalink / raw)
To: Sekhar Nori, Kevin Hilman, Russell King, Grygorii Strashko,
David S . Miller, Srinivas Kandagatla, Lukas Wunner, Rob Herring,
Florian Fainelli, Dan Carpenter, Ivan Khoronzhuk, David Lechner,
Greg Kroah-Hartman, Andrew Lunn
Cc: linux-arm-kernel, linux-kernel, linux-omap, netdev,
Bartosz Golaszewski
In-Reply-To: <20180718161035.7005-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
We'll soon have more sources from which to read the MAC address in this
routine. Make sure the address is correct before returning it.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
net/ethernet/eth.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 2a2173324d9e..6b64586fd2af 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -544,7 +544,7 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
from = "arch callback";
}
- if (!addr)
+ if (!addr || !is_valid_ether_addr(addr))
return -ENODEV;
dev_info(dev, "read MAC address from %s\n", from);
--
2.17.1
^ permalink raw reply related
* [PATCH 4/5] net: add support for nvmem to eth_platform_get_mac_address()
From: Bartosz Golaszewski @ 2018-07-18 16:10 UTC (permalink / raw)
To: Sekhar Nori, Kevin Hilman, Russell King, Grygorii Strashko,
David S . Miller, Srinivas Kandagatla, Lukas Wunner, Rob Herring,
Florian Fainelli, Dan Carpenter, Ivan Khoronzhuk, David Lechner,
Greg Kroah-Hartman, Andrew Lunn
Cc: linux-arm-kernel, linux-kernel, linux-omap, netdev,
Bartosz Golaszewski
In-Reply-To: <20180718161035.7005-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Many non-DT platforms read the MAC address from EEPROM. Usually it's
either done with callbacks defined in board files or from SoC-specific
ethernet drivers.
In order to generalize this, try to read the MAC from nvmem in
eth_platform_get_mac_address() using a standard lookup name:
"mac-address".
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
net/ethernet/eth.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 6b64586fd2af..adf5bd03851f 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -54,6 +54,7 @@
#include <linux/if_ether.h>
#include <linux/of_net.h>
#include <linux/pci.h>
+#include <linux/nvmem-consumer.h>
#include <net/dst.h>
#include <net/arp.h>
#include <net/sock.h>
@@ -530,7 +531,10 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
struct device_node *dp = dev_is_pci(dev) ?
pci_device_to_OF_node(to_pci_dev(dev)) : dev->of_node;
const unsigned char *addr = NULL;
+ unsigned char addrbuf[ETH_ALEN];
+ struct nvmem_cell *nvmem;
const char *from = NULL;
+ size_t alen;
if (dp) {
addr = of_get_mac_address(dp);
@@ -544,6 +548,31 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
from = "arch callback";
}
+ if (!addr) {
+ nvmem = nvmem_cell_get(dev, "mac-address");
+ if (IS_ERR(nvmem) && PTR_ERR(nvmem) == -EPROBE_DEFER)
+ /* We may have a lookup registered for MAC address but
+ * the corresponding nvmem provider hasn't been
+ * registered yet.
+ */
+ return -EPROBE_DEFER;
+
+ if (!IS_ERR(nvmem)) {
+ addr = nvmem_cell_read(nvmem, &alen);
+ if (!IS_ERR(addr)) {
+ from = "nvmem";
+ /* Don't use ether_addr_copy() in case we
+ * didn't get the right size.
+ */
+ memcpy(addrbuf, addr, alen);
+ kfree(addr);
+ addr = addrbuf;
+ }
+
+ nvmem_cell_put(nvmem);
+ }
+ }
+
if (!addr || !is_valid_ether_addr(addr))
return -ENODEV;
--
2.17.1
^ permalink raw reply related
* [PATCH net-next v3 0/8] A fix and a few improvements on mvneta
From: Gregory CLEMENT @ 2018-07-18 16:10 UTC (permalink / raw)
To: David S. Miller, linux-kernel, netdev
Cc: Thomas Petazzoni, linux-arm-kernel, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Gregory CLEMENT, Yelena Krivosheev,
Nadav Haklai, Marcin Wojtas, Dmitri Epshtein, Antoine Tenart,
Miquèl Raynal, Maxime Chevallier
Hello,
This series brings some improvements for the mvneta driver and also
adds a fix.
Compared to the v2, the main change is another patch fixing a bug
in mtu_change.
Gregory
Changelog:
v1 -> v2
- In patch 2, use EXPORT_SYMBOL_GPL for mvneta_bm_get and
mvneta_bm_put to be used in module, reported by kbuild test robot.
- In patch 4, add the counter to the driver's ethtool state,
suggested by David Miller.
- In patch 6, use a single if, suggested by Marcin Wojtas
v2 -> v3
- Adding a patch fixing the mtu change issue
- Removing the inline keyword for mvneta_rx_refill() and let the
comiler decided, suggested by David Miller.
Andrew Lunn (1):
net: ethernet: mvneta: Fix napi structure mixup on armada 3700
Gregory CLEMENT (3):
net: mvneta: remove data pointer usage from device_node structure
net: mvneta: discriminate error cause for missed packet
net: mvneta: Allocate page for the descriptor
Yelena Krivosheev (4):
net: mvneta: fix mtu change on port without link
net: mvneta: increase number of buffers in RX and TX queue
net: mvneta: Verify hardware checksum only when offload checksum
feature is set
net: mvneta: Improve the buffer allocation method for SWBM
drivers/net/ethernet/marvell/mvneta.c | 409 ++++++++++++++---------
drivers/net/ethernet/marvell/mvneta_bm.c | 15 +
drivers/net/ethernet/marvell/mvneta_bm.h | 8 +-
3 files changed, 272 insertions(+), 160 deletions(-)
--
2.18.0
^ permalink raw reply
* [PATCH net-next v3 1/8] net: ethernet: mvneta: Fix napi structure mixup on armada 3700
From: Gregory CLEMENT @ 2018-07-18 16:10 UTC (permalink / raw)
To: David S. Miller, linux-kernel, netdev
Cc: Thomas Petazzoni, linux-arm-kernel, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Gregory CLEMENT, Yelena Krivosheev,
Nadav Haklai, Marcin Wojtas, Dmitri Epshtein, Antoine Tenart,
Miquèl Raynal, Maxime Chevallier
In-Reply-To: <20180718161057.28704-1-gregory.clement@bootlin.com>
From: Andrew Lunn <andrew@lunn.ch>
The mvneta Ethernet driver is used on a few different Marvell SoCs.
Some SoCs have per cpu interrupts for Ethernet events. Some SoCs have
a single interrupt, independent of the CPU. The driver handles this by
having a per CPU napi structure when there are per CPU interrupts, and
a global napi structure when there is a single interrupt.
When the napi core calls mvneta_poll(), it passes the napi
instance. This was not being propagated through the call chain, and
instead the per-cpu napi instance was passed to napi_gro_receive()
call. This breaks when there is a single global napi instance.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Fixes: 2636ac3cc2b4 ("net: mvneta: Add network support for Armada 3700 SoC")
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
drivers/net/ethernet/marvell/mvneta.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 0ad2f3f7da85..ec84db47d82d 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -1901,10 +1901,10 @@ static void mvneta_rxq_drop_pkts(struct mvneta_port *pp,
}
/* Main rx processing when using software buffer management */
-static int mvneta_rx_swbm(struct mvneta_port *pp, int rx_todo,
+static int mvneta_rx_swbm(struct napi_struct *napi,
+ struct mvneta_port *pp, int rx_todo,
struct mvneta_rx_queue *rxq)
{
- struct mvneta_pcpu_port *port = this_cpu_ptr(pp->ports);
struct net_device *dev = pp->dev;
int rx_done;
u32 rcvd_pkts = 0;
@@ -1959,7 +1959,7 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, int rx_todo,
skb->protocol = eth_type_trans(skb, dev);
mvneta_rx_csum(pp, rx_status, skb);
- napi_gro_receive(&port->napi, skb);
+ napi_gro_receive(napi, skb);
rcvd_pkts++;
rcvd_bytes += rx_bytes;
@@ -2001,7 +2001,7 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, int rx_todo,
mvneta_rx_csum(pp, rx_status, skb);
- napi_gro_receive(&port->napi, skb);
+ napi_gro_receive(napi, skb);
}
if (rcvd_pkts) {
@@ -2020,10 +2020,10 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, int rx_todo,
}
/* Main rx processing when using hardware buffer management */
-static int mvneta_rx_hwbm(struct mvneta_port *pp, int rx_todo,
+static int mvneta_rx_hwbm(struct napi_struct *napi,
+ struct mvneta_port *pp, int rx_todo,
struct mvneta_rx_queue *rxq)
{
- struct mvneta_pcpu_port *port = this_cpu_ptr(pp->ports);
struct net_device *dev = pp->dev;
int rx_done;
u32 rcvd_pkts = 0;
@@ -2085,7 +2085,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int rx_todo,
skb->protocol = eth_type_trans(skb, dev);
mvneta_rx_csum(pp, rx_status, skb);
- napi_gro_receive(&port->napi, skb);
+ napi_gro_receive(napi, skb);
rcvd_pkts++;
rcvd_bytes += rx_bytes;
@@ -2129,7 +2129,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int rx_todo,
mvneta_rx_csum(pp, rx_status, skb);
- napi_gro_receive(&port->napi, skb);
+ napi_gro_receive(napi, skb);
}
if (rcvd_pkts) {
@@ -2722,9 +2722,11 @@ static int mvneta_poll(struct napi_struct *napi, int budget)
if (rx_queue) {
rx_queue = rx_queue - 1;
if (pp->bm_priv)
- rx_done = mvneta_rx_hwbm(pp, budget, &pp->rxqs[rx_queue]);
+ rx_done = mvneta_rx_hwbm(napi, pp, budget,
+ &pp->rxqs[rx_queue]);
else
- rx_done = mvneta_rx_swbm(pp, budget, &pp->rxqs[rx_queue]);
+ rx_done = mvneta_rx_swbm(napi, pp, budget,
+ &pp->rxqs[rx_queue]);
}
if (rx_done < budget) {
--
2.18.0
^ permalink raw reply related
* [PATCH net-next v3 2/8] net: mvneta: fix mtu change on port without link
From: Gregory CLEMENT @ 2018-07-18 16:10 UTC (permalink / raw)
To: David S. Miller, linux-kernel, netdev
Cc: Thomas Petazzoni, linux-arm-kernel, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Gregory CLEMENT, Yelena Krivosheev,
Nadav Haklai, Marcin Wojtas, Dmitri Epshtein, Antoine Tenart,
Miquèl Raynal, Maxime Chevallier
In-Reply-To: <20180718161057.28704-1-gregory.clement@bootlin.com>
From: Yelena Krivosheev <yelena@marvell.com>
It is incorrect to enable TX/RX queues (call by mvneta_port_up()) for
port without link. Indeed MTU change for interface without link causes TX
queues to stuck.
Fixes: c5aff18204da ("net: mvneta: driver for Marvell Armada 370/XP
network unit")
Signed-off-by: Yelena Krivosheev <yelena@marvell.com>
[gregory.clement: adding Fixes tags and rewording commit log]
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
drivers/net/ethernet/marvell/mvneta.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index ec84db47d82d..3d1f4335b0d3 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -3196,7 +3196,6 @@ static int mvneta_change_mtu(struct net_device *dev, int mtu)
on_each_cpu(mvneta_percpu_enable, pp, true);
mvneta_start_dev(pp);
- mvneta_port_up(pp);
netdev_update_features(dev);
--
2.18.0
^ permalink raw reply related
* [PATCH net-next v3 5/8] net: mvneta: discriminate error cause for missed packet
From: Gregory CLEMENT @ 2018-07-18 16:10 UTC (permalink / raw)
To: David S. Miller, linux-kernel, netdev
Cc: Thomas Petazzoni, linux-arm-kernel, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Gregory CLEMENT, Yelena Krivosheev,
Nadav Haklai, Marcin Wojtas, Dmitri Epshtein, Antoine Tenart,
Miquèl Raynal, Maxime Chevallier
In-Reply-To: <20180718161057.28704-1-gregory.clement@bootlin.com>
In order to improve the diagnostic in case of error, make the distinction
between refill error and skb allocation error. Also make the information
available through the ethtool state.
Based on the work of Yelena Krivosheev <yelena@marvell.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
drivers/net/ethernet/marvell/mvneta.c | 28 +++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index e519439bac97..da2d56826295 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -328,6 +328,8 @@
enum {
ETHTOOL_STAT_EEE_WAKEUP,
+ ETHTOOL_STAT_SKB_ALLOC_ERR,
+ ETHTOOL_STAT_REFILL_ERR,
ETHTOOL_MAX_STATS,
};
@@ -375,6 +377,8 @@ static const struct mvneta_statistic mvneta_statistics[] = {
{ 0x3054, T_REG_32, "fc_sent", },
{ 0x300c, T_REG_32, "internal_mac_transmit_err", },
{ ETHTOOL_STAT_EEE_WAKEUP, T_SW, "eee_wakeup_errors", },
+ { ETHTOOL_STAT_SKB_ALLOC_ERR, T_SW, "skb_alloc_errors", },
+ { ETHTOOL_STAT_REFILL_ERR, T_SW, "refill_errors", },
};
struct mvneta_pcpu_stats {
@@ -589,9 +593,6 @@ struct mvneta_rx_queue {
/* num of rx descriptors in the rx descriptor ring */
int size;
- /* counter of times when mvneta_refill() failed */
- int missed;
-
u32 pkts_coal;
u32 time_coal;
@@ -609,6 +610,10 @@ struct mvneta_rx_queue {
/* Index of the next RX DMA descriptor to process */
int next_desc_to_proc;
+
+ /* error counters */
+ u32 skb_alloc_err;
+ u32 refill_err;
};
static enum cpuhp_state online_hpstate;
@@ -1946,8 +1951,13 @@ static int mvneta_rx_swbm(struct napi_struct *napi,
if (rx_bytes <= rx_copybreak) {
/* better copy a small frame and not unmap the DMA region */
skb = netdev_alloc_skb_ip_align(dev, rx_bytes);
- if (unlikely(!skb))
+ if (unlikely(!skb)) {
+ netdev_err(dev,
+ "Can't allocate skb on queue %d\n",
+ rxq->id);
+ rxq->skb_alloc_err++;
goto err_drop_frame;
+ }
dma_sync_single_range_for_cpu(dev->dev.parent,
phys_addr,
@@ -1972,7 +1982,7 @@ static int mvneta_rx_swbm(struct napi_struct *napi,
err = mvneta_rx_refill(pp, rx_desc, rxq);
if (err) {
netdev_err(dev, "Linux processing - Can't refill\n");
- rxq->missed++;
+ rxq->refill_err++;
goto err_drop_frame;
}
@@ -2102,7 +2112,7 @@ static int mvneta_rx_hwbm(struct napi_struct *napi,
err = hwbm_pool_refill(&bm_pool->hwbm_pool, GFP_ATOMIC);
if (err) {
netdev_err(dev, "Linux processing - Can't refill\n");
- rxq->missed++;
+ rxq->refill_err++;
goto err_drop_frame_ret_pool;
}
@@ -3963,6 +3973,12 @@ static void mvneta_ethtool_update_stats(struct mvneta_port *pp)
case ETHTOOL_STAT_EEE_WAKEUP:
val = phylink_get_eee_err(pp->phylink);
break;
+ case ETHTOOL_STAT_SKB_ALLOC_ERR:
+ val = pp->rxqs[0].skb_alloc_err;
+ break;
+ case ETHTOOL_STAT_REFILL_ERR:
+ val = pp->rxqs[0].refill_err;
+ break;
}
break;
}
--
2.18.0
^ permalink raw reply related
* [PATCH net-next v3 6/8] net: mvneta: Allocate page for the descriptor
From: Gregory CLEMENT @ 2018-07-18 16:10 UTC (permalink / raw)
To: David S. Miller, linux-kernel, netdev
Cc: Thomas Petazzoni, linux-arm-kernel, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Gregory CLEMENT, Yelena Krivosheev,
Nadav Haklai, Marcin Wojtas, Dmitri Epshtein, Antoine Tenart,
Miquèl Raynal, Maxime Chevallier
In-Reply-To: <20180718161057.28704-1-gregory.clement@bootlin.com>
Instead of trying to allocate the exact amount of memory for each
descriptor use a page for each of them, it allows to simplify the
allocation management and increase the performance of the driver.
Based on the work of Yelena Krivosheev <yelena@marvell.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
drivers/net/ethernet/marvell/mvneta.c | 62 +++++++++---------------
drivers/net/ethernet/marvell/mvneta_bm.h | 3 --
2 files changed, 24 insertions(+), 41 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index da2d56826295..6af583ef2af2 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -1795,47 +1795,30 @@ static void mvneta_txq_done(struct mvneta_port *pp,
}
}
-void *mvneta_frag_alloc(unsigned int frag_size)
-{
- if (likely(frag_size <= PAGE_SIZE))
- return netdev_alloc_frag(frag_size);
- else
- return kmalloc(frag_size, GFP_ATOMIC);
-}
-EXPORT_SYMBOL_GPL(mvneta_frag_alloc);
-
-void mvneta_frag_free(unsigned int frag_size, void *data)
-{
- if (likely(frag_size <= PAGE_SIZE))
- skb_free_frag(data);
- else
- kfree(data);
-}
-EXPORT_SYMBOL_GPL(mvneta_frag_free);
-
/* Refill processing for SW buffer management */
+/* Allocate page per descriptor */
static int mvneta_rx_refill(struct mvneta_port *pp,
struct mvneta_rx_desc *rx_desc,
- struct mvneta_rx_queue *rxq)
-
+ struct mvneta_rx_queue *rxq,
+ gfp_t gfp_mask)
{
dma_addr_t phys_addr;
- void *data;
+ struct page *page;
- data = mvneta_frag_alloc(pp->frag_size);
- if (!data)
+ page = __dev_alloc_page(gfp_mask);
+ if (!page)
return -ENOMEM;
- phys_addr = dma_map_single(pp->dev->dev.parent, data,
- MVNETA_RX_BUF_SIZE(pp->pkt_size),
- DMA_FROM_DEVICE);
+ /* map page for use */
+ phys_addr = dma_map_page(pp->dev->dev.parent, page, 0, PAGE_SIZE,
+ DMA_FROM_DEVICE);
if (unlikely(dma_mapping_error(pp->dev->dev.parent, phys_addr))) {
- mvneta_frag_free(pp->frag_size, data);
+ __free_page(page);
return -ENOMEM;
}
phys_addr += pp->rx_offset_correction;
- mvneta_rx_desc_fill(rx_desc, phys_addr, data, rxq);
+ mvneta_rx_desc_fill(rx_desc, phys_addr, page, rxq);
return 0;
}
@@ -1901,7 +1884,7 @@ static void mvneta_rxq_drop_pkts(struct mvneta_port *pp,
dma_unmap_single(pp->dev->dev.parent, rx_desc->buf_phys_addr,
MVNETA_RX_BUF_SIZE(pp->pkt_size), DMA_FROM_DEVICE);
- mvneta_frag_free(pp->frag_size, data);
+ __free_page(data);
}
}
@@ -1928,6 +1911,7 @@ static int mvneta_rx_swbm(struct napi_struct *napi,
struct mvneta_rx_desc *rx_desc = mvneta_rxq_next_desc_get(rxq);
struct sk_buff *skb;
unsigned char *data;
+ struct page *page;
dma_addr_t phys_addr;
u32 rx_status, frag_size;
int rx_bytes, err, index;
@@ -1936,7 +1920,10 @@ static int mvneta_rx_swbm(struct napi_struct *napi,
rx_status = rx_desc->status;
rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
index = rx_desc - rxq->descs;
- data = rxq->buf_virt_addr[index];
+ page = (struct page *)rxq->buf_virt_addr[index];
+ data = page_address(page);
+ /* Prefetch header */
+ prefetch(data);
phys_addr = rx_desc->buf_phys_addr - pp->rx_offset_correction;
if (!mvneta_rxq_desc_is_first_last(rx_status) ||
@@ -1979,7 +1966,7 @@ static int mvneta_rx_swbm(struct napi_struct *napi,
}
/* Refill processing */
- err = mvneta_rx_refill(pp, rx_desc, rxq);
+ err = mvneta_rx_refill(pp, rx_desc, rxq, GFP_KERNEL);
if (err) {
netdev_err(dev, "Linux processing - Can't refill\n");
rxq->refill_err++;
@@ -2773,9 +2760,11 @@ static int mvneta_rxq_fill(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
for (i = 0; i < num; i++) {
memset(rxq->descs + i, 0, sizeof(struct mvneta_rx_desc));
- if (mvneta_rx_refill(pp, rxq->descs + i, rxq) != 0) {
- netdev_err(pp->dev, "%s:rxq %d, %d of %d buffs filled\n",
- __func__, rxq->id, i, num);
+ if (mvneta_rx_refill(pp, rxq->descs + i, rxq,
+ GFP_KERNEL) != 0) {
+ netdev_err(pp->dev,
+ "%s:rxq %d, %d of %d buffs filled\n",
+ __func__, rxq->id, i, num);
break;
}
}
@@ -3189,8 +3178,6 @@ static int mvneta_change_mtu(struct net_device *dev, int mtu)
mvneta_bm_update_mtu(pp, mtu);
pp->pkt_size = MVNETA_RX_PKT_SIZE(dev->mtu);
- pp->frag_size = SKB_DATA_ALIGN(MVNETA_RX_BUF_SIZE(pp->pkt_size)) +
- SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
ret = mvneta_setup_rxqs(pp);
if (ret) {
@@ -3677,8 +3664,7 @@ static int mvneta_open(struct net_device *dev)
int ret;
pp->pkt_size = MVNETA_RX_PKT_SIZE(pp->dev->mtu);
- pp->frag_size = SKB_DATA_ALIGN(MVNETA_RX_BUF_SIZE(pp->pkt_size)) +
- SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+ pp->frag_size = PAGE_SIZE;
ret = mvneta_setup_rxqs(pp);
if (ret)
diff --git a/drivers/net/ethernet/marvell/mvneta_bm.h b/drivers/net/ethernet/marvell/mvneta_bm.h
index 9358626e51ec..c8425d35c049 100644
--- a/drivers/net/ethernet/marvell/mvneta_bm.h
+++ b/drivers/net/ethernet/marvell/mvneta_bm.h
@@ -130,9 +130,6 @@ struct mvneta_bm_pool {
};
/* Declarations and definitions */
-void *mvneta_frag_alloc(unsigned int frag_size);
-void mvneta_frag_free(unsigned int frag_size, void *data);
-
#if IS_ENABLED(CONFIG_MVNETA_BM)
struct mvneta_bm *mvneta_bm_get(struct device_node *node);
void mvneta_bm_put(struct mvneta_bm *priv);
--
2.18.0
^ permalink raw reply related
* [PATCH net-next v3 7/8] net: mvneta: Verify hardware checksum only when offload checksum feature is set
From: Gregory CLEMENT @ 2018-07-18 16:10 UTC (permalink / raw)
To: David S. Miller, linux-kernel, netdev
Cc: Thomas Petazzoni, linux-arm-kernel, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Gregory CLEMENT, Yelena Krivosheev,
Nadav Haklai, Marcin Wojtas, Dmitri Epshtein, Antoine Tenart,
Miquèl Raynal, Maxime Chevallier
In-Reply-To: <20180718161057.28704-1-gregory.clement@bootlin.com>
From: Yelena Krivosheev <yelena@marvell.com>
If the checksum offload feature is not set, then there is no point to
check the status of the hardware.
[gregory: extract from a larger patch]
Signed-off-by: Yelena Krivosheev <yelena@marvell.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
drivers/net/ethernet/marvell/mvneta.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 6af583ef2af2..079b515c54c1 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -1720,7 +1720,8 @@ static void mvneta_rx_error(struct mvneta_port *pp,
static void mvneta_rx_csum(struct mvneta_port *pp, u32 status,
struct sk_buff *skb)
{
- if ((status & MVNETA_RXD_L3_IP4) &&
+ if ((pp->dev->features & NETIF_F_RXCSUM) &&
+ (status & MVNETA_RXD_L3_IP4) &&
(status & MVNETA_RXD_L4_CSUM_OK)) {
skb->csum = 0;
skb->ip_summed = CHECKSUM_UNNECESSARY;
--
2.18.0
^ permalink raw reply related
* [PATCH net-next v3 8/8] net: mvneta: Improve the buffer allocation method for SWBM
From: Gregory CLEMENT @ 2018-07-18 16:10 UTC (permalink / raw)
To: David S. Miller, linux-kernel, netdev
Cc: Thomas Petazzoni, linux-arm-kernel, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Gregory CLEMENT, Yelena Krivosheev,
Nadav Haklai, Marcin Wojtas, Dmitri Epshtein, Antoine Tenart,
Miquèl Raynal, Maxime Chevallier
In-Reply-To: <20180718161057.28704-1-gregory.clement@bootlin.com>
From: Yelena Krivosheev <yelena@marvell.com>
With system having a small memory (around 256MB), the state "cannot
allocate memory to refill with new buffer" is reach pretty quickly.
By this patch we changed buffer allocation method to a better handling of
this use case by avoiding memory allocation issues.
Signed-off-by: Yelena Krivosheev <yelena@marvell.com>
[gregory: extract from a larger patch]
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
drivers/net/ethernet/marvell/mvneta.c | 281 +++++++++++++++++---------
1 file changed, 183 insertions(+), 98 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 079b515c54c1..55c2a56c5dae 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -483,7 +483,10 @@ struct mvneta_port {
#define MVNETA_RXD_ERR_RESOURCE (BIT(17) | BIT(18))
#define MVNETA_RXD_ERR_CODE_MASK (BIT(17) | BIT(18))
#define MVNETA_RXD_L3_IP4 BIT(25)
-#define MVNETA_RXD_FIRST_LAST_DESC (BIT(26) | BIT(27))
+#define MVNETA_RXD_LAST_DESC BIT(26)
+#define MVNETA_RXD_FIRST_DESC BIT(27)
+#define MVNETA_RXD_FIRST_LAST_DESC (MVNETA_RXD_FIRST_DESC | \
+ MVNETA_RXD_LAST_DESC)
#define MVNETA_RXD_L4_CSUM_OK BIT(30)
#if defined(__LITTLE_ENDIAN)
@@ -611,6 +614,14 @@ struct mvneta_rx_queue {
/* Index of the next RX DMA descriptor to process */
int next_desc_to_proc;
+ /* Index of first RX DMA descriptor to refill */
+ int first_to_refill;
+ u32 refill_num;
+
+ /* pointer to uncomplete skb buffer */
+ struct sk_buff *skb;
+ int left_size;
+
/* error counters */
u32 skb_alloc_err;
u32 refill_err;
@@ -626,6 +637,7 @@ static int txq_number = 8;
static int rxq_def;
static int rx_copybreak __read_mostly = 256;
+static int rx_header_size __read_mostly = 128;
/* HW BM need that each port be identify by a unique ID */
static int global_port_id;
@@ -1689,13 +1701,6 @@ static void mvneta_rx_error(struct mvneta_port *pp,
{
u32 status = rx_desc->status;
- if (!mvneta_rxq_desc_is_first_last(status)) {
- netdev_err(pp->dev,
- "bad rx status %08x (buffer oversize), size=%d\n",
- status, rx_desc->data_size);
- return;
- }
-
switch (status & MVNETA_RXD_ERR_CODE_MASK) {
case MVNETA_RXD_ERR_CRC:
netdev_err(pp->dev, "bad rx status %08x (crc error), size=%d\n",
@@ -1882,6 +1887,8 @@ static void mvneta_rxq_drop_pkts(struct mvneta_port *pp,
for (i = 0; i < rxq->size; i++) {
struct mvneta_rx_desc *rx_desc = rxq->descs + i;
void *data = rxq->buf_virt_addr[i];
+ if (!data || !(rx_desc->buf_phys_addr))
+ continue;
dma_unmap_single(pp->dev->dev.parent, rx_desc->buf_phys_addr,
MVNETA_RX_BUF_SIZE(pp->pkt_size), DMA_FROM_DEVICE);
@@ -1889,117 +1896,183 @@ static void mvneta_rxq_drop_pkts(struct mvneta_port *pp,
}
}
+static inline
+int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq)
+{
+ struct mvneta_rx_desc *rx_desc;
+ int curr_desc = rxq->first_to_refill;
+ int i;
+
+ for (i = 0; (i < rxq->refill_num) && (i < 64); i++) {
+ rx_desc = rxq->descs + curr_desc;
+ if (!(rx_desc->buf_phys_addr)) {
+ if (mvneta_rx_refill(pp, rx_desc, rxq, GFP_ATOMIC)) {
+ pr_err("Can't refill queue %d. Done %d from %d\n",
+ rxq->id, i, rxq->refill_num);
+ rxq->refill_err++;
+ break;
+ }
+ }
+ curr_desc = MVNETA_QUEUE_NEXT_DESC(rxq, curr_desc);
+ }
+ rxq->refill_num -= i;
+ rxq->first_to_refill = curr_desc;
+
+ return i;
+}
+
/* Main rx processing when using software buffer management */
static int mvneta_rx_swbm(struct napi_struct *napi,
- struct mvneta_port *pp, int rx_todo,
+ struct mvneta_port *pp, int budget,
struct mvneta_rx_queue *rxq)
{
struct net_device *dev = pp->dev;
- int rx_done;
+ int rx_todo, rx_proc;
+ int refill = 0;
u32 rcvd_pkts = 0;
u32 rcvd_bytes = 0;
/* Get number of received packets */
- rx_done = mvneta_rxq_busy_desc_num_get(pp, rxq);
-
- if (rx_todo > rx_done)
- rx_todo = rx_done;
-
- rx_done = 0;
+ rx_todo = mvneta_rxq_busy_desc_num_get(pp, rxq);
+ rx_proc = 0;
/* Fairness NAPI loop */
- while (rx_done < rx_todo) {
+ while ((rcvd_pkts < budget) && (rx_proc < rx_todo)) {
struct mvneta_rx_desc *rx_desc = mvneta_rxq_next_desc_get(rxq);
- struct sk_buff *skb;
unsigned char *data;
struct page *page;
dma_addr_t phys_addr;
- u32 rx_status, frag_size;
- int rx_bytes, err, index;
+ u32 rx_status, index;
+ int rx_bytes, skb_size, copy_size;
+ int frag_num, frag_size, frag_offset;
- rx_done++;
- rx_status = rx_desc->status;
- rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE);
index = rx_desc - rxq->descs;
page = (struct page *)rxq->buf_virt_addr[index];
data = page_address(page);
/* Prefetch header */
prefetch(data);
- phys_addr = rx_desc->buf_phys_addr - pp->rx_offset_correction;
- if (!mvneta_rxq_desc_is_first_last(rx_status) ||
- (rx_status & MVNETA_RXD_ERR_SUMMARY)) {
- mvneta_rx_error(pp, rx_desc);
-err_drop_frame:
- dev->stats.rx_errors++;
- /* leave the descriptor untouched */
- continue;
- }
+ phys_addr = rx_desc->buf_phys_addr;
+ rx_status = rx_desc->status;
+ rx_proc++;
+ rxq->refill_num++;
+
+ if (rx_status & MVNETA_RXD_FIRST_DESC) {
+ /* Check errors only for FIRST descriptor */
+ if (rx_status & MVNETA_RXD_ERR_SUMMARY) {
+ mvneta_rx_error(pp, rx_desc);
+ dev->stats.rx_errors++;
+ /* leave the descriptor untouched */
+ continue;
+ }
+ rx_bytes = rx_desc->data_size -
+ (ETH_FCS_LEN + MVNETA_MH_SIZE);
- if (rx_bytes <= rx_copybreak) {
- /* better copy a small frame and not unmap the DMA region */
- skb = netdev_alloc_skb_ip_align(dev, rx_bytes);
- if (unlikely(!skb)) {
+ /* Allocate small skb for each new packet */
+ skb_size = max(rx_copybreak, rx_header_size);
+ rxq->skb = netdev_alloc_skb_ip_align(dev, skb_size);
+ if (unlikely(!rxq->skb)) {
netdev_err(dev,
"Can't allocate skb on queue %d\n",
rxq->id);
+ dev->stats.rx_dropped++;
rxq->skb_alloc_err++;
- goto err_drop_frame;
+ continue;
}
+ copy_size = min(skb_size, rx_bytes);
+
+ /* Copy data from buffer to SKB, skip Marvell header */
+ memcpy(rxq->skb->data, data + MVNETA_MH_SIZE,
+ copy_size);
+ skb_put(rxq->skb, copy_size);
+ rxq->left_size = rx_bytes - copy_size;
+
+ mvneta_rx_csum(pp, rx_status, rxq->skb);
+ if (rxq->left_size == 0) {
+ int size = copy_size + MVNETA_MH_SIZE;
+
+ dma_sync_single_range_for_cpu(dev->dev.parent,
+ phys_addr, 0,
+ size,
+ DMA_FROM_DEVICE);
+
+ /* leave the descriptor and buffer untouched */
+ } else {
+ /* refill descriptor with new buffer later */
+ rx_desc->buf_phys_addr = 0;
+
+ frag_num = 0;
+ frag_offset = copy_size + MVNETA_MH_SIZE;
+ frag_size = min(rxq->left_size,
+ (int)(PAGE_SIZE - frag_offset));
+ skb_add_rx_frag(rxq->skb, frag_num, page,
+ frag_offset, frag_size,
+ PAGE_SIZE);
+ dma_unmap_single(dev->dev.parent, phys_addr,
+ PAGE_SIZE, DMA_FROM_DEVICE);
+ rxq->left_size -= frag_size;
+ }
+ } else {
+ /* Middle or Last descriptor */
+ if (unlikely(!rxq->skb)) {
+ pr_debug("no skb for rx_status 0x%x\n",
+ rx_status);
+ continue;
+ }
+ if (!rxq->left_size) {
+ /* last descriptor has only FCS */
+ /* and can be discarded */
+ dma_sync_single_range_for_cpu(dev->dev.parent,
+ phys_addr, 0,
+ ETH_FCS_LEN,
+ DMA_FROM_DEVICE);
+ /* leave the descriptor and buffer untouched */
+ } else {
+ /* refill descriptor with new buffer later */
+ rx_desc->buf_phys_addr = 0;
+
+ frag_num = skb_shinfo(rxq->skb)->nr_frags;
+ frag_offset = 0;
+ frag_size = min(rxq->left_size,
+ (int)(PAGE_SIZE - frag_offset));
+ skb_add_rx_frag(rxq->skb, frag_num, page,
+ frag_offset, frag_size,
+ PAGE_SIZE);
+
+ dma_unmap_single(dev->dev.parent, phys_addr,
+ PAGE_SIZE,
+ DMA_FROM_DEVICE);
+
+ rxq->left_size -= frag_size;
+ }
+ } /* Middle or Last descriptor */
- dma_sync_single_range_for_cpu(dev->dev.parent,
- phys_addr,
- MVNETA_MH_SIZE + NET_SKB_PAD,
- rx_bytes,
- DMA_FROM_DEVICE);
- skb_put_data(skb, data + MVNETA_MH_SIZE + NET_SKB_PAD,
- rx_bytes);
-
- skb->protocol = eth_type_trans(skb, dev);
- mvneta_rx_csum(pp, rx_status, skb);
- napi_gro_receive(napi, skb);
-
- rcvd_pkts++;
- rcvd_bytes += rx_bytes;
-
- /* leave the descriptor and buffer untouched */
+ if (!(rx_status & MVNETA_RXD_LAST_DESC))
+ /* no last descriptor this time */
continue;
- }
- /* Refill processing */
- err = mvneta_rx_refill(pp, rx_desc, rxq, GFP_KERNEL);
- if (err) {
- netdev_err(dev, "Linux processing - Can't refill\n");
- rxq->refill_err++;
- goto err_drop_frame;
+ if (rxq->left_size) {
+ pr_err("get last desc, but left_size (%d) != 0\n",
+ rxq->left_size);
+ dev_kfree_skb_any(rxq->skb);
+ rxq->left_size = 0;
+ rxq->skb = NULL;
+ continue;
}
-
- frag_size = pp->frag_size;
-
- skb = build_skb(data, frag_size > PAGE_SIZE ? 0 : frag_size);
-
- /* After refill old buffer has to be unmapped regardless
- * the skb is successfully built or not.
- */
- dma_unmap_single(dev->dev.parent, phys_addr,
- MVNETA_RX_BUF_SIZE(pp->pkt_size),
- DMA_FROM_DEVICE);
-
- if (!skb)
- goto err_drop_frame;
-
rcvd_pkts++;
- rcvd_bytes += rx_bytes;
+ rcvd_bytes += rxq->skb->len;
/* Linux processing */
- skb_reserve(skb, MVNETA_MH_SIZE + NET_SKB_PAD);
- skb_put(skb, rx_bytes);
-
- skb->protocol = eth_type_trans(skb, dev);
+ rxq->skb->protocol = eth_type_trans(rxq->skb, dev);
- mvneta_rx_csum(pp, rx_status, skb);
+ if (dev->features & NETIF_F_GRO)
+ napi_gro_receive(napi, rxq->skb);
+ else
+ netif_receive_skb(rxq->skb);
- napi_gro_receive(napi, skb);
+ /* clean uncomplete skb pointer in queue */
+ rxq->skb = NULL;
+ rxq->left_size = 0;
}
if (rcvd_pkts) {
@@ -2011,10 +2084,13 @@ static int mvneta_rx_swbm(struct napi_struct *napi,
u64_stats_update_end(&stats->syncp);
}
+ /* return some buffers to hardware queue, one at a time is too slow */
+ refill = mvneta_rx_refill_queue(pp, rxq);
+
/* Update rxq management counters */
- mvneta_rxq_desc_num_update(pp, rxq, rx_done, rx_done);
+ mvneta_rxq_desc_num_update(pp, rxq, rx_proc, refill);
- return rx_done;
+ return rcvd_pkts;
}
/* Main rx processing when using hardware buffer management */
@@ -2823,21 +2899,23 @@ static void mvneta_rxq_hw_init(struct mvneta_port *pp,
mvreg_write(pp, MVNETA_RXQ_BASE_ADDR_REG(rxq->id), rxq->descs_phys);
mvreg_write(pp, MVNETA_RXQ_SIZE_REG(rxq->id), rxq->size);
- /* Set Offset */
- mvneta_rxq_offset_set(pp, rxq, NET_SKB_PAD - pp->rx_offset_correction);
-
/* Set coalescing pkts and time */
mvneta_rx_pkts_coal_set(pp, rxq, rxq->pkts_coal);
mvneta_rx_time_coal_set(pp, rxq, rxq->time_coal);
if (!pp->bm_priv) {
- /* Fill RXQ with buffers from RX pool */
- mvneta_rxq_buf_size_set(pp, rxq,
- MVNETA_RX_BUF_SIZE(pp->pkt_size));
+ /* Set Offset */
+ mvneta_rxq_offset_set(pp, rxq, 0);
+ mvneta_rxq_buf_size_set(pp, rxq, pp->frag_size);
mvneta_rxq_bm_disable(pp, rxq);
mvneta_rxq_fill(pp, rxq, rxq->size);
} else {
+ /* Set Offset */
+ mvneta_rxq_offset_set(pp, rxq,
+ NET_SKB_PAD - pp->rx_offset_correction);
+
mvneta_rxq_bm_enable(pp, rxq);
+ /* Fill RXQ with buffers from RX pool */
mvneta_rxq_long_pool_set(pp, rxq);
mvneta_rxq_short_pool_set(pp, rxq);
mvneta_rxq_non_occup_desc_add(pp, rxq, rxq->size);
@@ -2866,6 +2944,9 @@ static void mvneta_rxq_deinit(struct mvneta_port *pp,
{
mvneta_rxq_drop_pkts(pp, rxq);
+ if (rxq->skb)
+ dev_kfree_skb_any(rxq->skb);
+
if (rxq->descs)
dma_free_coherent(pp->dev->dev.parent,
rxq->size * MVNETA_DESC_ALIGNED_SIZE,
@@ -2876,6 +2957,10 @@ static void mvneta_rxq_deinit(struct mvneta_port *pp,
rxq->last_desc = 0;
rxq->next_desc_to_proc = 0;
rxq->descs_phys = 0;
+ rxq->first_to_refill = 0;
+ rxq->refill_num = 0;
+ rxq->skb = NULL;
+ rxq->left_size = 0;
}
static int mvneta_txq_sw_init(struct mvneta_port *pp,
@@ -4366,14 +4451,6 @@ static int mvneta_probe(struct platform_device *pdev)
pp->dn = dn;
pp->rxq_def = rxq_def;
-
- /* Set RX packet offset correction for platforms, whose
- * NET_SKB_PAD, exceeds 64B. It should be 64B for 64-bit
- * platforms and 0B for 32-bit ones.
- */
- pp->rx_offset_correction =
- max(0, NET_SKB_PAD - MVNETA_RX_PKT_OFFSET_CORRECTION);
-
pp->indir[0] = rxq_def;
/* Get special SoC configurations */
@@ -4461,6 +4538,7 @@ static int mvneta_probe(struct platform_device *pdev)
SET_NETDEV_DEV(dev, &pdev->dev);
pp->id = global_port_id++;
+ pp->rx_offset_correction = 0; /* not relevant for SW BM */
/* Obtain access to BM resources if enabled and already initialized */
bm_node = of_parse_phandle(dn, "buffer-manager", 0);
@@ -4475,6 +4553,13 @@ static int mvneta_probe(struct platform_device *pdev)
pp->bm_priv = NULL;
}
}
+ /* Set RX packet offset correction for platforms, whose
+ * NET_SKB_PAD, exceeds 64B. It should be 64B for 64-bit
+ * platforms and 0B for 32-bit ones.
+ */
+ pp->rx_offset_correction = max(0,
+ NET_SKB_PAD -
+ MVNETA_RX_PKT_OFFSET_CORRECTION);
}
of_node_put(bm_node);
--
2.18.0
^ permalink raw reply related
* Re: [PATCH 1/5] net: visually shrink eth_platform_get_mac_address()
From: Andrew Lunn @ 2018-07-18 16:28 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Grygorii Strashko, David Lechner, Ivan Khoronzhuk,
Kevin Hilman, Greg Kroah-Hartman, Sekhar Nori, Russell King,
linux-kernel, Bartosz Golaszewski, Lukas Wunner,
Srinivas Kandagatla, linux-arm-kernel, netdev, Florian Fainelli,
linux-omap, David S . Miller, Dan Carpenter
In-Reply-To: <20180718161035.7005-2-brgl@bgdev.pl>
On Wed, Jul 18, 2018 at 06:10:31PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Initialize the variables with proper values so that we save a few
> lines of code before we extend this function in the follow-up patches.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> net/ethernet/eth.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index ee28440f57c5..da8530879e1e 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -527,15 +527,10 @@ unsigned char * __weak arch_get_platform_mac_address(void)
>
> int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
> {
> - const unsigned char *addr;
> - struct device_node *dp;
> + struct device_node *dp = dev_is_pci(dev) ?
> + pci_device_to_OF_node(to_pci_dev(dev)) : dev->of_node;
> + const unsigned char *addr = NULL;
Hi Bartosz
You are now in the net subsystem, which has its own set of additional
coding styles. One of them is reverse Christmas tree.
You might want to read Documentation/networking/netdev-FAQ.txt.
Andrew
^ permalink raw reply
* Re: [PATCH 2/5] net: add an info message to eth_platform_get_mac_address()
From: Andrew Lunn @ 2018-07-18 16:31 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Sekhar Nori, Kevin Hilman, Russell King, Grygorii Strashko,
David S . Miller, Srinivas Kandagatla, Lukas Wunner, Rob Herring,
Florian Fainelli, Dan Carpenter, Ivan Khoronzhuk, David Lechner,
Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-omap,
netdev, Bartosz Golaszewski
In-Reply-To: <20180718161035.7005-3-brgl@bgdev.pl>
On Wed, Jul 18, 2018 at 06:10:32PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Many drivers that read the MAC address from EEPROM or MTD emit a log
> message when they succeed. Since this function is meant to be reused
> in those drivers instead of reimplementing the same operation
> everywhere, add an info message when we successfully read the MAC
> address.
Hi Bartosz
This makes eth_platform_get_mac_address() generally more verbose,
which i guess people won't like. To keep it backwards compatible, it
would be better to issue the message just when EEPROM to MTD is used.
Andrew
^ permalink raw reply
* Re: [PATCH 1/5] net: visually shrink eth_platform_get_mac_address()
From: Bartosz Golaszewski @ 2018-07-18 16:31 UTC (permalink / raw)
To: Andrew Lunn
Cc: Sekhar Nori, Kevin Hilman, Russell King, Grygorii Strashko,
David S . Miller, Srinivas Kandagatla, Lukas Wunner, Rob Herring,
Florian Fainelli, Dan Carpenter, Ivan Khoronzhuk, David Lechner,
Greg Kroah-Hartman, Linux ARM, Linux Kernel Mailing List,
linux-omap, netdev, Bartosz Golaszewski
In-Reply-To: <20180718162849.GA12477@lunn.ch>
2018-07-18 18:28 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
> On Wed, Jul 18, 2018 at 06:10:31PM +0200, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>
>> Initialize the variables with proper values so that we save a few
>> lines of code before we extend this function in the follow-up patches.
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>> net/ethernet/eth.c | 11 +++--------
>> 1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
>> index ee28440f57c5..da8530879e1e 100644
>> --- a/net/ethernet/eth.c
>> +++ b/net/ethernet/eth.c
>> @@ -527,15 +527,10 @@ unsigned char * __weak arch_get_platform_mac_address(void)
>>
>> int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>> {
>> - const unsigned char *addr;
>> - struct device_node *dp;
>> + struct device_node *dp = dev_is_pci(dev) ?
>> + pci_device_to_OF_node(to_pci_dev(dev)) : dev->of_node;
>> + const unsigned char *addr = NULL;
>
> Hi Bartosz
>
> You are now in the net subsystem, which has its own set of additional
> coding styles. One of them is reverse Christmas tree.
>
> You might want to read Documentation/networking/netdev-FAQ.txt.
>
> Andrew
Hi Andrew,
it's still reverse Christmas tree in this patch except that now we're
taking the length of the variable + initializer into account. I'm not
sure if this is the right approach though.
Bart
^ permalink raw reply
* Re: [PATCH 2/5] net: add an info message to eth_platform_get_mac_address()
From: Bartosz Golaszewski @ 2018-07-18 16:33 UTC (permalink / raw)
To: Andrew Lunn
Cc: Sekhar Nori, Kevin Hilman, Russell King, Grygorii Strashko,
David S . Miller, Srinivas Kandagatla, Lukas Wunner, Rob Herring,
Florian Fainelli, Dan Carpenter, Ivan Khoronzhuk, David Lechner,
Greg Kroah-Hartman, Linux ARM, Linux Kernel Mailing List,
linux-omap, netdev, Bartosz Golaszewski
In-Reply-To: <20180718163109.GB12477@lunn.ch>
2018-07-18 18:31 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
> On Wed, Jul 18, 2018 at 06:10:32PM +0200, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>
>> Many drivers that read the MAC address from EEPROM or MTD emit a log
>> message when they succeed. Since this function is meant to be reused
>> in those drivers instead of reimplementing the same operation
>> everywhere, add an info message when we successfully read the MAC
>> address.
>
> Hi Bartosz
>
> This makes eth_platform_get_mac_address() generally more verbose,
> which i guess people won't like. To keep it backwards compatible, it
> would be better to issue the message just when EEPROM to MTD is used.
>
> Andrew
This function should be used at most once per interface - I guess it's
not like it's a huge impact on verbosity.
Bart
^ permalink raw reply
* Re: [PATCH 3/5] net: fortify eth_platform_get_mac_address()
From: Andrew Lunn @ 2018-07-18 16:35 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Sekhar Nori, Kevin Hilman, Russell King, Grygorii Strashko,
David S . Miller, Srinivas Kandagatla, Lukas Wunner, Rob Herring,
Florian Fainelli, Dan Carpenter, Ivan Khoronzhuk, David Lechner,
Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-omap,
netdev, Bartosz Golaszewski
In-Reply-To: <20180718161035.7005-4-brgl@bgdev.pl>
On Wed, Jul 18, 2018 at 06:10:33PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> We'll soon have more sources from which to read the MAC address in this
> routine. Make sure the address is correct before returning it.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v2 bpf 3/5] bpf: bpf_prog_array_free() should take a generic non-rcu pointer
From: Roman Gushchin @ 2018-07-18 16:01 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: netdev, linux-kernel, kernel-team, Alexei Starovoitov
In-Reply-To: <90bc1d99-da1b-065a-cd4f-457b7f17a533@iogearbox.net>
On Wed, Jul 18, 2018 at 03:07:48PM +0200, Daniel Borkmann wrote:
> On 07/18/2018 12:55 AM, Roman Gushchin wrote:
> > On Wed, Jul 18, 2018 at 12:38:50AM +0200, Daniel Borkmann wrote:
> >> On 07/17/2018 12:57 AM, Roman Gushchin wrote:
> >>> On Tue, Jul 17, 2018 at 12:30:18AM +0200, Daniel Borkmann wrote:
> >>>> On 07/13/2018 09:41 PM, Roman Gushchin wrote:
> >>>>> bpf_prog_array_free() should take a generic non-rcu pointer
> >>>>> as an argument, as freeing the objects assumes that we're
> >>>>> holding an exclusive rights on it.
> >>>>>
> >>>>> rcu_access_pointer() can be used to convert a __rcu pointer to
> >>>>> a generic pointer before passing it to bpf_prog_array_free(),
> >>>>> if necessary.
> >>>>>
> >>>>> This patch eliminates the following sparse warning:
> >>>>> kernel/bpf/core.c:1556:9: warning: incorrect type in argument 1 (different address spaces)
> >>>>> kernel/bpf/core.c:1556:9: expected struct callback_head *head
> >>>>> kernel/bpf/core.c:1556:9: got struct callback_head [noderef] <asn:4>*<noident>
> >>>>>
> >>>>> Fixes: 324bda9e6c5a ("bpf: multi program support for cgroup+bpf")
> >>>>> Signed-off-by: Roman Gushchin <guro@fb.com>
> >>>>> Cc: Alexei Starovoitov <ast@kernel.org>
> >>>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
> >>>>> ---
> >>>>> drivers/media/rc/bpf-lirc.c | 6 +++---
> >>>>> include/linux/bpf.h | 2 +-
> >>>>> kernel/bpf/cgroup.c | 11 ++++++-----
> >>>>> kernel/bpf/core.c | 5 ++---
> >>>>> kernel/trace/bpf_trace.c | 8 ++++----
> >>>>> 5 files changed, 16 insertions(+), 16 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
> >>>>> index fcfab6635f9c..509b262aa0dc 100644
> >>>>> --- a/drivers/media/rc/bpf-lirc.c
> >>>>> +++ b/drivers/media/rc/bpf-lirc.c
> >>>>> @@ -135,7 +135,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
> >>>>> goto unlock;
> >>>>>
> >>>>> rcu_assign_pointer(raw->progs, new_array);
> >>>>> - bpf_prog_array_free(old_array);
> >>>>> + bpf_prog_array_free(rcu_access_pointer(old_array));
> >>>>
> >>>> Taking this one as an example, why can't we already do the rcu_dereference() on the
> >>>> 'old_array = raw->progs' where we fetch the old_array initially? Then we also wouldn't
> >>>> need the rcu_access_pointer() on bpf_prog_array_free() and yet another rcu_dereference()
> >>>> inside the bpf_prog_array_copy() from your later patch?
> >>>
> >>> We can, but then we have to change bpf_prog_array_copy() args annotation,
> >>> and also all places, where it's called.
> >>> IMO, basically all local variables and function args marked as __rcu
> >>> should be not marked as RCU, but fixing them all is beyond this patchset.
> >>
> >> Right, agree, the __rcu markings seem somewhat arbitrary. :-( I think we need to
> >> investigate this a bit deeper and do a proper audit on the whole bpf prog array's
> >> RCU handling (probably won't get to it in next two weeks but put onto backlog just
> >> in case it's still unresolved till then). That said, given this has been there for
> >> quite a while and it's rc5 now, I think we could start out on bpf-next with the
> >> obvious candidates which should be okay even if it ends up bigger.
> >
> > Totally agree.
> >
> >> First two from this series we could already take in if you prefer.
> >
> > That would be nice!
>
> Ok, done, applied 1+2 to bpf-next, thanks Roman!
Thanks, Daniel!
^ permalink raw reply
* Re: [PATCH 4/5] net: add support for nvmem to eth_platform_get_mac_address()
From: Andrew Lunn @ 2018-07-18 16:42 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Grygorii Strashko, David Lechner, Ivan Khoronzhuk,
Kevin Hilman, Greg Kroah-Hartman, Sekhar Nori, Russell King,
linux-kernel, Bartosz Golaszewski, Lukas Wunner,
Srinivas Kandagatla, linux-arm-kernel, netdev, Florian Fainelli,
linux-omap, David S . Miller, Dan Carpenter
In-Reply-To: <20180718161035.7005-5-brgl@bgdev.pl>
> + if (!IS_ERR(nvmem)) {
> + addr = nvmem_cell_read(nvmem, &alen);
> + if (!IS_ERR(addr)) {
> + from = "nvmem";
> + /* Don't use ether_addr_copy() in case we
> + * didn't get the right size.
> + */
Please verify the size. A short read can still give a valid MAC
address, so the is_valid_ether_addr(addr) is not sufficient.
> + memcpy(addrbuf, addr, alen);
Another reason to check the length is that you appear to have a buffer
overflow here, if alen > 6.
Andrew
> + kfree(addr);
> + addr = addrbuf;
> + }
> +
> + nvmem_cell_put(nvmem);
> + }
> + }
> +
> if (!addr || !is_valid_ether_addr(addr))
> return -ENODEV;
>
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH 5/5] net: add MTD support to eth_platform_get_mac_address()
From: Andrew Lunn @ 2018-07-18 16:47 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Sekhar Nori, Kevin Hilman, Russell King, Grygorii Strashko,
David S . Miller, Srinivas Kandagatla, Lukas Wunner, Rob Herring,
Florian Fainelli, Dan Carpenter, Ivan Khoronzhuk, David Lechner,
Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-omap,
netdev, Bartosz Golaszewski
In-Reply-To: <20180718161035.7005-6-brgl@bgdev.pl>
On Wed, Jul 18, 2018 at 06:10:35PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> MTD doesn't support nvmem yet. Some platforms use MTD to read the MAC
> address from SPI flash. If we want this function to generalize reading
> the MAC address, we need to separately try to use MTD.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> net/ethernet/eth.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index adf5bd03851f..f7dbd2cff7f9 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -55,6 +55,7 @@
> #include <linux/of_net.h>
> #include <linux/pci.h>
> #include <linux/nvmem-consumer.h>
> +#include <linux/mtd/mtd.h>
> #include <net/dst.h>
> #include <net/arp.h>
> #include <net/sock.h>
> @@ -573,6 +574,25 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
> }
> }
>
> +#ifdef CONFIG_MTD
> + /* NOTE: this should go away as soon as MTD gets nvmem support. */
> + if (!addr) {
> + struct mtd_info *mtd;
> + int rv;
> +
> + mtd = get_mtd_device_nm("MAC-Address");
In order for this to go away, you need to keep backwards
compatibility. When using nvmem, you look for a cell called
"mac-address". Here you are looking for "MAC-Address". That is going
to make backwards compatibility harder. How do you plan to do it?
Andrew
^ permalink raw reply
* [PATCH 1/5] net: visually shrink eth_platform_get_mac_address()
From: Bartosz Golaszewski @ 2018-07-18 16:10 UTC (permalink / raw)
To: Sekhar Nori, Kevin Hilman, Russell King, Grygorii Strashko,
David S . Miller, Srinivas Kandagatla, Lukas Wunner, Rob Herring,
Florian Fainelli, Dan Carpenter, Ivan Khoronzhuk, David Lechner,
Greg Kroah-Hartman, Andrew Lunn
Cc: linux-arm-kernel, linux-kernel, linux-omap, netdev,
Bartosz Golaszewski
In-Reply-To: <20180718161035.7005-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Initialize the variables with proper values so that we save a few
lines of code before we extend this function in the follow-up patches.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
net/ethernet/eth.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index ee28440f57c5..da8530879e1e 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -527,15 +527,10 @@ unsigned char * __weak arch_get_platform_mac_address(void)
int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
{
- const unsigned char *addr;
- struct device_node *dp;
+ struct device_node *dp = dev_is_pci(dev) ?
+ pci_device_to_OF_node(to_pci_dev(dev)) : dev->of_node;
+ const unsigned char *addr = NULL;
- if (dev_is_pci(dev))
- dp = pci_device_to_OF_node(to_pci_dev(dev));
- else
- dp = dev->of_node;
-
- addr = NULL;
if (dp)
addr = of_get_mac_address(dp);
if (!addr)
--
2.17.1
^ permalink raw reply related
* [PATCH 2/5] net: add an info message to eth_platform_get_mac_address()
From: Bartosz Golaszewski @ 2018-07-18 16:10 UTC (permalink / raw)
To: Sekhar Nori, Kevin Hilman, Russell King, Grygorii Strashko,
David S . Miller, Srinivas Kandagatla, Lukas Wunner, Rob Herring,
Florian Fainelli, Dan Carpenter, Ivan Khoronzhuk, David Lechner,
Greg Kroah-Hartman, Andrew Lunn
Cc: linux-arm-kernel, linux-kernel, linux-omap, netdev,
Bartosz Golaszewski
In-Reply-To: <20180718161035.7005-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Many drivers that read the MAC address from EEPROM or MTD emit a log
message when they succeed. Since this function is meant to be reused
in those drivers instead of reimplementing the same operation
everywhere, add an info message when we successfully read the MAC
address.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
net/ethernet/eth.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index da8530879e1e..2a2173324d9e 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -530,15 +530,24 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
struct device_node *dp = dev_is_pci(dev) ?
pci_device_to_OF_node(to_pci_dev(dev)) : dev->of_node;
const unsigned char *addr = NULL;
+ const char *from = NULL;
- if (dp)
+ if (dp) {
addr = of_get_mac_address(dp);
- if (!addr)
+ if (addr)
+ from = "device tree";
+ }
+
+ if (!addr) {
addr = arch_get_platform_mac_address();
+ if (addr)
+ from = "arch callback";
+ }
if (!addr)
return -ENODEV;
+ dev_info(dev, "read MAC address from %s\n", from);
ether_addr_copy(mac_addr, addr);
return 0;
}
--
2.17.1
^ permalink raw reply related
* [PATCH 5/5] net: add MTD support to eth_platform_get_mac_address()
From: Bartosz Golaszewski @ 2018-07-18 16:10 UTC (permalink / raw)
To: Sekhar Nori, Kevin Hilman, Russell King, Grygorii Strashko,
David S . Miller, Srinivas Kandagatla, Lukas Wunner, Rob Herring,
Florian Fainelli, Dan Carpenter, Ivan Khoronzhuk, David Lechner,
Greg Kroah-Hartman, Andrew Lunn
Cc: linux-arm-kernel, linux-kernel, linux-omap, netdev,
Bartosz Golaszewski
In-Reply-To: <20180718161035.7005-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
MTD doesn't support nvmem yet. Some platforms use MTD to read the MAC
address from SPI flash. If we want this function to generalize reading
the MAC address, we need to separately try to use MTD.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
net/ethernet/eth.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index adf5bd03851f..f7dbd2cff7f9 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -55,6 +55,7 @@
#include <linux/of_net.h>
#include <linux/pci.h>
#include <linux/nvmem-consumer.h>
+#include <linux/mtd/mtd.h>
#include <net/dst.h>
#include <net/arp.h>
#include <net/sock.h>
@@ -573,6 +574,25 @@ int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
}
}
+#ifdef CONFIG_MTD
+ /* NOTE: this should go away as soon as MTD gets nvmem support. */
+ if (!addr) {
+ struct mtd_info *mtd;
+ int rv;
+
+ mtd = get_mtd_device_nm("MAC-Address");
+ if (!IS_ERR(mtd)) {
+ rv = mtd_read(mtd, 0, ETH_ALEN, &alen, addrbuf);
+ if (rv == 0) {
+ from = "MTD";
+ addr = addrbuf;
+ }
+
+ put_mtd_device(mtd);
+ }
+ }
+#endif /* CONFIG_MTD */
+
if (!addr || !is_valid_ether_addr(addr))
return -ENODEV;
--
2.17.1
^ permalink raw reply related
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