While trying to figure out how to allow bumping the MTU with the
ucc_geth driver, I fell into a rabbit hole and stumbled on a whole
bunch of issues of varying importance - some are outright bug fixes,
while most are a matter of simplifying the code to make it more
accessible.
At the end of digging around the code and data sheet to figure out how
it all works, I think the MTU issue might be fixed by a one-liner, but
I'm not sure it can be that simple. It does seem to work (ping -s X
works for larger values of X, and wireshark confirms that the packets
are not fragmented).
Re patch 2, someone in NXP should check how the hardware actually
works and make an updated reference manual available.
Rasmus Villemoes (20):
ethernet: ucc_geth: set dev->max_mtu to 1518
ethernet: ucc_geth: fix definition and size of ucc_geth_tx_global_pram
ethernet: ucc_geth: remove unused read of temoder field
soc: fsl: qe: make cpm_muram_offset take a const void* argument
soc: fsl: qe: store muram_vbase as a void pointer instead of u8
soc: fsl: qe: add cpm_muram_free_addr() helper
ethernet: ucc_geth: use qe_muram_free_addr()
ethernet: ucc_geth: remove unnecessary memset_io() calls
ethernet: ucc_geth: replace kmalloc+memset by kzalloc
ethernet: ucc_geth: remove {rx,tx}_glbl_pram_offset from struct
ucc_geth_private
ethernet: ucc_geth: fix use-after-free in ucc_geth_remove()
ethernet: ucc_geth: factor out parsing of {rx,tx}-clock{,-name}
properties
ethernet: ucc_geth: constify ugeth_primary_info
ethernet: ucc_geth: don't statically allocate eight ucc_geth_info
ethernet: ucc_geth: use UCC_GETH_{RX,TX}_BD_RING_ALIGNMENT macros
directly
ethernet: ucc_geth: remove bd_mem_part and all associated code
ethernet: ucc_geth: replace kmalloc_array()+for loop by kcalloc()
ethernet: ucc_geth: add helper to replace repeated switch statements
ethernet: ucc_geth: inform the compiler that numQueues is always 1
ethernet: ucc_geth: simplify rx/tx allocations
drivers/net/ethernet/freescale/ucc_geth.c | 553 ++++++++--------------
drivers/net/ethernet/freescale/ucc_geth.h | 15 +-
drivers/soc/fsl/qe/qe_common.c | 20 +-
include/soc/fsl/qe/qe.h | 15 +-
include/soc/fsl/qe/ucc_fast.h | 1 -
5 files changed, 219 insertions(+), 385 deletions(-)
--
2.23.0
These buffers have all just been handed out from qe_muram_alloc(), aka
cpm_muram_alloc(), and the helper cpm_muram_alloc_common() already
does
memset_io(cpm_muram_addr(start), 0, size);
Signed-off-by: Rasmus Villemoes <[email protected]>
---
drivers/net/ethernet/freescale/ucc_geth.c | 19 -------------------
1 file changed, 19 deletions(-)
diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index bbcdb77be9a8..f854ff90f238 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -2506,9 +2506,6 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
ugeth->p_tx_glbl_pram =
(struct ucc_geth_tx_global_pram __iomem *) qe_muram_addr(ugeth->
tx_glbl_pram_offset);
- /* Zero out p_tx_glbl_pram */
- memset_io((void __iomem *)ugeth->p_tx_glbl_pram, 0, sizeof(struct ucc_geth_tx_global_pram));
-
/* Fill global PRAM */
/* TQPTR */
@@ -2596,8 +2593,6 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
scheduler_offset);
out_be32(&ugeth->p_tx_glbl_pram->schedulerbasepointer,
ugeth->scheduler_offset);
- /* Zero out p_scheduler */
- memset_io((void __iomem *)ugeth->p_scheduler, 0, sizeof(struct ucc_geth_scheduler));
/* Set values in scheduler */
out_be32(&ugeth->p_scheduler->mblinterval,
@@ -2640,9 +2635,6 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
ugeth->p_tx_fw_statistics_pram =
(struct ucc_geth_tx_firmware_statistics_pram __iomem *)
qe_muram_addr(ugeth->tx_fw_statistics_pram_offset);
- /* Zero out p_tx_fw_statistics_pram */
- memset_io((void __iomem *)ugeth->p_tx_fw_statistics_pram,
- 0, sizeof(struct ucc_geth_tx_firmware_statistics_pram));
}
/* temoder */
@@ -2675,9 +2667,6 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
ugeth->p_rx_glbl_pram =
(struct ucc_geth_rx_global_pram __iomem *) qe_muram_addr(ugeth->
rx_glbl_pram_offset);
- /* Zero out p_rx_glbl_pram */
- memset_io((void __iomem *)ugeth->p_rx_glbl_pram, 0, sizeof(struct ucc_geth_rx_global_pram));
-
/* Fill global PRAM */
/* RQPTR */
@@ -2715,9 +2704,6 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
ugeth->p_rx_fw_statistics_pram =
(struct ucc_geth_rx_firmware_statistics_pram __iomem *)
qe_muram_addr(ugeth->rx_fw_statistics_pram_offset);
- /* Zero out p_rx_fw_statistics_pram */
- memset_io((void __iomem *)ugeth->p_rx_fw_statistics_pram, 0,
- sizeof(struct ucc_geth_rx_firmware_statistics_pram));
}
/* intCoalescingPtr */
@@ -2803,11 +2789,6 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
(struct ucc_geth_rx_bd_queues_entry __iomem *) qe_muram_addr(ugeth->
rx_bd_qs_tbl_offset);
out_be32(&ugeth->p_rx_glbl_pram->rbdqptr, ugeth->rx_bd_qs_tbl_offset);
- /* Zero out p_rx_bd_qs_tbl */
- memset_io((void __iomem *)ugeth->p_rx_bd_qs_tbl,
- 0,
- ug_info->numQueuesRx * (sizeof(struct ucc_geth_rx_bd_queues_entry) +
- sizeof(struct ucc_geth_rx_prefetched_bds)));
/* Setup the table */
/* Assume BD rings are already established */
--
2.23.0
struct ucc_geth_info is somewhat large, and on systems with only one
or two UCC instances, that just wastes a few KB of memory. So
allocate and populate a chunk of memory at probe time instead of
initializing them all during driver init.
Note that the existing "ug_info == NULL" check was dead code, as the
address of some static array element can obviously never be NULL.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
drivers/net/ethernet/freescale/ucc_geth.c | 32 +++++++++--------------
1 file changed, 12 insertions(+), 20 deletions(-)
diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index a06744d8b4af..273342233bba 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -157,8 +157,6 @@ static const struct ucc_geth_info ugeth_primary_info = {
.riscRx = QE_RISC_ALLOCATION_RISC1_AND_RISC2,
};
-static struct ucc_geth_info ugeth_info[8];
-
#ifdef DEBUG
static void mem_disp(u8 *addr, int size)
{
@@ -3714,25 +3712,23 @@ static int ucc_geth_probe(struct platform_device* ofdev)
if ((ucc_num < 0) || (ucc_num > 7))
return -ENODEV;
- ug_info = &ugeth_info[ucc_num];
- if (ug_info == NULL) {
- if (netif_msg_probe(&debug))
- pr_err("[%d] Missing additional data!\n", ucc_num);
- return -ENODEV;
- }
+ ug_info = kmalloc(sizeof(*ug_info), GFP_KERNEL);
+ if (ug_info == NULL)
+ return -ENOMEM;
+ memcpy(ug_info, &ugeth_primary_info, sizeof(*ug_info));
ug_info->uf_info.ucc_num = ucc_num;
err = ucc_geth_parse_clock(np, "rx", &ug_info->uf_info.rx_clock);
if (err)
- return err;
+ goto err_free_info;
err = ucc_geth_parse_clock(np, "tx", &ug_info->uf_info.tx_clock);
if (err)
- return err;
+ goto err_free_info;
err = of_address_to_resource(np, 0, &res);
if (err)
- return -EINVAL;
+ goto err_free_info;
ug_info->uf_info.regs = res.start;
ug_info->uf_info.irq = irq_of_parse_and_map(np, 0);
@@ -3745,7 +3741,7 @@ static int ucc_geth_probe(struct platform_device* ofdev)
*/
err = of_phy_register_fixed_link(np);
if (err)
- return err;
+ goto err_free_info;
ug_info->phy_node = of_node_get(np);
}
@@ -3876,6 +3872,8 @@ static int ucc_geth_probe(struct platform_device* ofdev)
of_phy_deregister_fixed_link(np);
of_node_put(ug_info->tbi_node);
of_node_put(ug_info->phy_node);
+err_free_info:
+ kfree(ug_info);
return err;
}
@@ -3886,6 +3884,7 @@ static int ucc_geth_remove(struct platform_device* ofdev)
struct ucc_geth_private *ugeth = netdev_priv(dev);
struct device_node *np = ofdev->dev.of_node;
+ kfree(ugeth->ug_info);
ucc_geth_memclean(ugeth);
if (of_phy_is_fixed_link(np))
of_phy_deregister_fixed_link(np);
@@ -3920,17 +3919,10 @@ static struct platform_driver ucc_geth_driver = {
static int __init ucc_geth_init(void)
{
- int i, ret;
-
if (netif_msg_drv(&debug))
pr_info(DRV_DESC "\n");
- for (i = 0; i < 8; i++)
- memcpy(&(ugeth_info[i]), &ugeth_primary_info,
- sizeof(ugeth_primary_info));
-
- ret = platform_driver_register(&ucc_geth_driver);
- return ret;
+ return platform_driver_register(&ucc_geth_driver);
}
static void __exit ucc_geth_exit(void)
--
2.23.0
Since kmalloc() is nowadays [1] guaranteed to return naturally
aligned (i.e., aligned to the size itself) memory for power-of-2
sizes, we don't need to over-allocate the align amount, compute an
aligned address within the allocation, and (for later freeing) also
storing the original pointer [2].
Instead, just round up the length we want to allocate to the alignment
requirements, then round that up to the next power of 2. In theory,
this could allocate up to about twice as much memory as we needed. In
practice, (a) kmalloc() would in most cases anyway return a
power-of-2-sized allocation and (b) with the default values of the
bdRingLen[RT]x fields, the length is already itself a power of 2
greater than the alignment.
So we actually end up saving memory compared to the current
situtation (e.g. for tx, we currently allocate 128+32 bytes, which
kmalloc() likely rounds up to 192 or 256; with this patch, we just
allocate 128 bytes.) Also struct ucc_geth_private becomes a little
smaller.
[1] 59bb47985c1d ("mm, sl[aou]b: guarantee natural alignment for
kmalloc(power-of-two)")
[2] That storing was anyway done in a u32, which works on 32 bit
machines, but is not very elegant and certainly makes a reader of the
code pause for a while.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
drivers/net/ethernet/freescale/ucc_geth.c | 50 ++++++++---------------
drivers/net/ethernet/freescale/ucc_geth.h | 2 -
2 files changed, 17 insertions(+), 35 deletions(-)
diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 3cad2255bd97..a4ed9209513e 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -1835,7 +1835,7 @@ static void ucc_geth_free_rx(struct ucc_geth_private *ugeth)
kfree(ugeth->rx_skbuff[i]);
- kfree((void *)ugeth->rx_bd_ring_offset[i]);
+ kfree(ugeth->p_rx_bd_ring[i]);
ugeth->p_rx_bd_ring[i] = NULL;
}
}
@@ -1872,10 +1872,8 @@ static void ucc_geth_free_tx(struct ucc_geth_private *ugeth)
kfree(ugeth->tx_skbuff[i]);
- if (ugeth->p_tx_bd_ring[i]) {
- kfree((void *)ugeth->tx_bd_ring_offset[i]);
- ugeth->p_tx_bd_ring[i] = NULL;
- }
+ kfree(ugeth->p_tx_bd_ring[i]);
+ ugeth->p_tx_bd_ring[i] = NULL;
}
}
@@ -2150,25 +2148,15 @@ static int ucc_geth_alloc_tx(struct ucc_geth_private *ugeth)
/* Allocate Tx bds */
for (j = 0; j < ucc_geth_tx_queues(ug_info); j++) {
- u32 align = UCC_GETH_TX_BD_RING_ALIGNMENT;
-
- /* Allocate in multiple of
- UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT,
- according to spec */
- length = ((ug_info->bdRingLenTx[j] * sizeof(struct qe_bd))
- / UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT)
- * UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT;
- if ((ug_info->bdRingLenTx[j] * sizeof(struct qe_bd)) %
- UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT)
- length += UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT;
-
- ugeth->tx_bd_ring_offset[j] =
- (u32) kmalloc((u32) (length + align), GFP_KERNEL);
-
- if (ugeth->tx_bd_ring_offset[j] != 0)
- ugeth->p_tx_bd_ring[j] =
- (u8 __iomem *)((ugeth->tx_bd_ring_offset[j] +
- align) & ~(align - 1));
+ u32 align = max(UCC_GETH_TX_BD_RING_ALIGNMENT,
+ UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT);
+ u32 alloc;
+
+ length = ug_info->bdRingLenTx[j] * sizeof(struct qe_bd);
+ alloc = round_up(length, align);
+ alloc = roundup_pow_of_two(alloc);
+
+ ugeth->p_tx_bd_ring[j] = kmalloc(alloc, GFP_KERNEL);
if (!ugeth->p_tx_bd_ring[j]) {
if (netif_msg_ifup(ugeth))
@@ -2176,9 +2164,7 @@ static int ucc_geth_alloc_tx(struct ucc_geth_private *ugeth)
return -ENOMEM;
}
/* Zero unused end of bd ring, according to spec */
- memset_io((void __iomem *)(ugeth->p_tx_bd_ring[j] +
- ug_info->bdRingLenTx[j] * sizeof(struct qe_bd)), 0,
- length - ug_info->bdRingLenTx[j] * sizeof(struct qe_bd));
+ memset(ugeth->p_tx_bd_ring[j] + length, 0, alloc - length);
}
/* Init Tx bds */
@@ -2225,15 +2211,13 @@ static int ucc_geth_alloc_rx(struct ucc_geth_private *ugeth)
/* Allocate Rx bds */
for (j = 0; j < ucc_geth_rx_queues(ug_info); j++) {
u32 align = UCC_GETH_RX_BD_RING_ALIGNMENT;
+ u32 alloc;
length = ug_info->bdRingLenRx[j] * sizeof(struct qe_bd);
- ugeth->rx_bd_ring_offset[j] =
- (u32) kmalloc((u32) (length + align), GFP_KERNEL);
- if (ugeth->rx_bd_ring_offset[j] != 0)
- ugeth->p_rx_bd_ring[j] =
- (u8 __iomem *)((ugeth->rx_bd_ring_offset[j] +
- align) & ~(align - 1));
+ alloc = round_up(length, align);
+ alloc = roundup_pow_of_two(alloc);
+ ugeth->p_rx_bd_ring[j] = kmalloc(alloc, GFP_KERNEL);
if (!ugeth->p_rx_bd_ring[j]) {
if (netif_msg_ifup(ugeth))
pr_err("Can not allocate memory for Rx bd rings\n");
diff --git a/drivers/net/ethernet/freescale/ucc_geth.h b/drivers/net/ethernet/freescale/ucc_geth.h
index 6539fed9cc22..ccc4ca1ae9b6 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.h
+++ b/drivers/net/ethernet/freescale/ucc_geth.h
@@ -1182,9 +1182,7 @@ struct ucc_geth_private {
struct ucc_geth_rx_bd_queues_entry __iomem *p_rx_bd_qs_tbl;
u32 rx_bd_qs_tbl_offset;
u8 __iomem *p_tx_bd_ring[NUM_TX_QUEUES];
- u32 tx_bd_ring_offset[NUM_TX_QUEUES];
u8 __iomem *p_rx_bd_ring[NUM_RX_QUEUES];
- u32 rx_bd_ring_offset[NUM_RX_QUEUES];
u8 __iomem *confBd[NUM_TX_QUEUES];
u8 __iomem *txBd[NUM_TX_QUEUES];
u8 __iomem *rxBd[NUM_RX_QUEUES];
--
2.23.0
On Sat, 5 Dec 2020 20:17:23 +0100 Rasmus Villemoes wrote:
> While trying to figure out how to allow bumping the MTU with the
> ucc_geth driver, I fell into a rabbit hole and stumbled on a whole
> bunch of issues of varying importance - some are outright bug fixes,
> while most are a matter of simplifying the code to make it more
> accessible.
>
> At the end of digging around the code and data sheet to figure out how
> it all works, I think the MTU issue might be fixed by a one-liner, but
> I'm not sure it can be that simple. It does seem to work (ping -s X
> works for larger values of X, and wireshark confirms that the packets
> are not fragmented).
>
> Re patch 2, someone in NXP should check how the hardware actually
> works and make an updated reference manual available.
Looks like a nice clean up on a quick look.
Please separate patches 1 and 11 (which are the two bug fixes I see)
rebase (retest) and post them against the net tree:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/
so they are available for backports.
The reset should go into net-next:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/
Please indicate the tree in the tag like [PATCH net] or [PATCH
net-next] so the test bots know which base to use for testing.
Thanks!
On 05/12/2020 21.53, Jakub Kicinski wrote:
> On Sat, 5 Dec 2020 20:17:23 +0100 Rasmus Villemoes wrote:
>> While trying to figure out how to allow bumping the MTU with the
>> ucc_geth driver, I fell into a rabbit hole and stumbled on a whole
>> bunch of issues of varying importance - some are outright bug fixes,
>> while most are a matter of simplifying the code to make it more
>> accessible.
>>
>> At the end of digging around the code and data sheet to figure out how
>> it all works, I think the MTU issue might be fixed by a one-liner, but
>> I'm not sure it can be that simple. It does seem to work (ping -s X
>> works for larger values of X, and wireshark confirms that the packets
>> are not fragmented).
>>
>> Re patch 2, someone in NXP should check how the hardware actually
>> works and make an updated reference manual available.
>
> Looks like a nice clean up on a quick look.
>
> Please separate patches 1 and 11 (which are the two bug fixes I see)
I think patch 2 is a bug fix as well, but I'd like someone from NXP to
comment.
> rebase (retest) and post them against the net tree:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/
So I thought this would go through Li Yang's tree. That's where my
previous QE related patches have gone through, and at least some need
some input from NXP folks - and what MAINTAINERS suggests. So not
marking the patches with net or net-next was deliberate. But I'm happy
to rearrange and send to net/net-next as appropriate if that's what you
and Li Yang can agree to.
Thanks,
Rasmus
On Sat, 5 Dec 2020 22:11:39 +0100 Rasmus Villemoes wrote:
> > Looks like a nice clean up on a quick look.
> >
> > Please separate patches 1 and 11 (which are the two bug fixes I see)
>
> I think patch 2 is a bug fix as well, but I'd like someone from NXP to
> comment.
Sure, makes sense.
> > rebase (retest) and post them against the net tree:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/
>
> So I thought this would go through Li Yang's tree. That's where my
> previous QE related patches have gone through, and at least some need
> some input from NXP folks - and what MAINTAINERS suggests. So not
> marking the patches with net or net-next was deliberate. But I'm happy
> to rearrange and send to net/net-next as appropriate if that's what you
> and Li Yang can agree to.
Ah, now I noticed you didn't CC all of the patches to netdev.
Please don't do that, build bots won't be able to validate partially
posted patches.
On 05/12/2020 22.27, Jakub Kicinski wrote:
> On Sat, 5 Dec 2020 22:11:39 +0100 Rasmus Villemoes wrote:
>>> rebase (retest) and post them against the net tree:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/
>>
>> So I thought this would go through Li Yang's tree. That's where my
>> previous QE related patches have gone through, and at least some need
>> some input from NXP folks - and what MAINTAINERS suggests. So not
>> marking the patches with net or net-next was deliberate. But I'm happy
>> to rearrange and send to net/net-next as appropriate if that's what you
>> and Li Yang can agree to.
>
> Ah, now I noticed you didn't CC all of the patches to netdev.
> Please don't do that, build bots won't be able to validate partially
> posted patches.
Hm, I mostly rely on scripts/get_maintainer.pl, invoked via a script I
give to --to-cmd/--cc-cmd. They are all sent to LKML, so buildbots
should be OK. But regardless of whether Li Yang will be handling it or
not, I'll try to remember to cc the whole series to netdev when resending.
Rasmus
On 06/12/2020 05:12, Rasmus Villemoes <[email protected]> wrote:
> -----Original Message-----
> From: Rasmus Villemoes <[email protected]>
> Sent: 2020??12??6?? 5:12
> To: Jakub Kicinski <[email protected]>
> Cc: Leo Li <[email protected]>; David S. Miller <[email protected]>;
> Qiang Zhao <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; Vladimir Oltean
> <[email protected]>
> Subject: Re: [PATCH 00/20] ethernet: ucc_geth: assorted fixes and
> simplifications
>
> On 05/12/2020 21.53, Jakub Kicinski wrote:
> > On Sat, 5 Dec 2020 20:17:23 +0100 Rasmus Villemoes wrote:
> >> While trying to figure out how to allow bumping the MTU with the
> >> ucc_geth driver, I fell into a rabbit hole and stumbled on a whole
> >> bunch of issues of varying importance - some are outright bug fixes,
> >> while most are a matter of simplifying the code to make it more
> >> accessible.
> >>
> >> At the end of digging around the code and data sheet to figure out
> >> how it all works, I think the MTU issue might be fixed by a
> >> one-liner, but I'm not sure it can be that simple. It does seem to
> >> work (ping -s X works for larger values of X, and wireshark confirms
> >> that the packets are not fragmented).
> >>
> >> Re patch 2, someone in NXP should check how the hardware actually
> >> works and make an updated reference manual available.
> >
> > Looks like a nice clean up on a quick look.
> >
> > Please separate patches 1 and 11 (which are the two bug fixes I see)
>
> I think patch 2 is a bug fix as well, but I'd like someone from NXP to comment.
It 's ok for me.
Best Regards,
Qiang Zhao
On 08/12/2020 04.07, Qiang Zhao wrote:
> On 06/12/2020 05:12, Rasmus Villemoes <[email protected]> wrote:
>
>> I think patch 2 is a bug fix as well, but I'd like someone from NXP to comment.
>
> It 's ok for me.
I was hoping for something a bit more than that. Can you please go check
with the people who made the hardware and those who wrote the manual
(probably not the same ones) what is actually up and down, and then
report on what they said.
It's fairly obvious that allocating 192 bytes instead of 128 should
never hurt (unless we run out of muram), but it would be nice with an
official "Yes, table 8-111 is wrong, it should say 192", or
alternatively, "No, table 8-53 is wrong, those MTU etc. fields don't
really exist". Extra points for providing details such as "first
revision of the IP had $foo, but that was never shipped in real
products, then $bar was changed", etc.
Thanks,
Rasmus
Le 05/12/2020 à 20:17, Rasmus Villemoes a écrit :
> struct ucc_geth_info is somewhat large, and on systems with only one
> or two UCC instances, that just wastes a few KB of memory. So
> allocate and populate a chunk of memory at probe time instead of
> initializing them all during driver init.
>
> Note that the existing "ug_info == NULL" check was dead code, as the
> address of some static array element can obviously never be NULL.
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
> drivers/net/ethernet/freescale/ucc_geth.c | 32 +++++++++--------------
> 1 file changed, 12 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
> index a06744d8b4af..273342233bba 100644
> --- a/drivers/net/ethernet/freescale/ucc_geth.c
> +++ b/drivers/net/ethernet/freescale/ucc_geth.c
> @@ -157,8 +157,6 @@ static const struct ucc_geth_info ugeth_primary_info = {
> .riscRx = QE_RISC_ALLOCATION_RISC1_AND_RISC2,
> };
>
> -static struct ucc_geth_info ugeth_info[8];
> -
> #ifdef DEBUG
> static void mem_disp(u8 *addr, int size)
> {
> @@ -3714,25 +3712,23 @@ static int ucc_geth_probe(struct platform_device* ofdev)
> if ((ucc_num < 0) || (ucc_num > 7))
> return -ENODEV;
>
> - ug_info = &ugeth_info[ucc_num];
> - if (ug_info == NULL) {
> - if (netif_msg_probe(&debug))
> - pr_err("[%d] Missing additional data!\n", ucc_num);
> - return -ENODEV;
> - }
> + ug_info = kmalloc(sizeof(*ug_info), GFP_KERNEL);
Could we use dev_kmalloc() instead, to avoid the freeing on the wait out and the err_free_info: path ?
> + if (ug_info == NULL)
> + return -ENOMEM;
> + memcpy(ug_info, &ugeth_primary_info, sizeof(*ug_info));
>
> ug_info->uf_info.ucc_num = ucc_num;
>
> err = ucc_geth_parse_clock(np, "rx", &ug_info->uf_info.rx_clock);
> if (err)
> - return err;
> + goto err_free_info;
> err = ucc_geth_parse_clock(np, "tx", &ug_info->uf_info.tx_clock);
> if (err)
> - return err;
> + goto err_free_info;
>
> err = of_address_to_resource(np, 0, &res);
> if (err)
> - return -EINVAL;
> + goto err_free_info;
>
> ug_info->uf_info.regs = res.start;
> ug_info->uf_info.irq = irq_of_parse_and_map(np, 0);
> @@ -3745,7 +3741,7 @@ static int ucc_geth_probe(struct platform_device* ofdev)
> */
> err = of_phy_register_fixed_link(np);
> if (err)
> - return err;
> + goto err_free_info;
> ug_info->phy_node = of_node_get(np);
> }
>
> @@ -3876,6 +3872,8 @@ static int ucc_geth_probe(struct platform_device* ofdev)
> of_phy_deregister_fixed_link(np);
> of_node_put(ug_info->tbi_node);
> of_node_put(ug_info->phy_node);
> +err_free_info:
> + kfree(ug_info);
>
> return err;
> }
> @@ -3886,6 +3884,7 @@ static int ucc_geth_remove(struct platform_device* ofdev)
> struct ucc_geth_private *ugeth = netdev_priv(dev);
> struct device_node *np = ofdev->dev.of_node;
>
> + kfree(ugeth->ug_info);
> ucc_geth_memclean(ugeth);
> if (of_phy_is_fixed_link(np))
> of_phy_deregister_fixed_link(np);
> @@ -3920,17 +3919,10 @@ static struct platform_driver ucc_geth_driver = {
>
> static int __init ucc_geth_init(void)
> {
> - int i, ret;
> -
> if (netif_msg_drv(&debug))
> pr_info(DRV_DESC "\n");
> - for (i = 0; i < 8; i++)
> - memcpy(&(ugeth_info[i]), &ugeth_primary_info,
> - sizeof(ugeth_primary_info));
> -
> - ret = platform_driver_register(&ucc_geth_driver);
>
> - return ret;
> + return platform_driver_register(&ucc_geth_driver);
> }
>
> static void __exit ucc_geth_exit(void)
>
On 08/12/2020 16.13, Christophe Leroy wrote:
>
>
> Le 05/12/2020 à 20:17, Rasmus Villemoes a écrit :
>> @@ -3714,25 +3712,23 @@ static int ucc_geth_probe(struct
>> platform_device* ofdev)
>> if ((ucc_num < 0) || (ucc_num > 7))
>> return -ENODEV;
>> - ug_info = &ugeth_info[ucc_num];
>> - if (ug_info == NULL) {
>> - if (netif_msg_probe(&debug))
>> - pr_err("[%d] Missing additional data!\n", ucc_num);
>> - return -ENODEV;
>> - }
>> + ug_info = kmalloc(sizeof(*ug_info), GFP_KERNEL);
>
> Could we use dev_kmalloc() instead, to avoid the freeing on the wait out
> and the err_free_info: path ?
Perhaps, but I don't think mixing ordinary kmalloc() with devm_ versions
in the same driver is a good idea - IIRC there are at least some rules
to obey if one does that, but I don't remember and can't find what they are.
Rasmus
On 05/12/2020 22.27, Jakub Kicinski wrote:
> On Sat, 5 Dec 2020 22:11:39 +0100 Rasmus Villemoes wrote:
>>> Looks like a nice clean up on a quick look.
>>>
>>> Please separate patches 1 and 11 (which are the two bug fixes I see)
>>
>> I think patch 2 is a bug fix as well, but I'd like someone from NXP to
>> comment.
>
> Sure, makes sense.
>
>>> rebase (retest) and post them against the net tree:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/
>>
>> So I thought this would go through Li Yang's tree.
Li, any preference? Will you take this series, or are you ok with the
three soc/fsl/qe patches going through the net tree along with the rest?
Rasmus