Even a mere glance at the STMMAC driver code reveals multiple problematic
parts. Here just the brightest of them: intermix of the platform-specific,
runtime and even LLDD-specific (Hello Intel) parameters defined in the
platform data, redundant (sometimes up to four) parameters defining a
device feature state, very unfortunate DW MAC id (MAC100, GMAC, QoS,
X-GMAC) and versioning interface, too bulky and clumsy HW-abstraction
interface with non-unified function prototypes accepting device-dependent
arguments instead of taking a single HW descriptor or STMMAC driver
private data aside with algo-dependent parameters, non-unified
declarations and definitions spread out between four header files
"stmmac.h", "stmmac_platform.h", <linux/stmmac.h>, common.h, hwif.h, code
snippet duplicated all over the driver code (i.e. DMA descriptors ring
access), and so on and so forth.
All of that makes the STMMAC driver very hard to read, maintain and
update. Thus any more-or-less complicated change not only turns to be
larger than it could be (which besides makes it harder to review) but also
may have higher risks to break things in unexpected places of the driver.
Therefore at the very least the code cleanup suggests itself here.
At first we didn't want to change the driver much in a hope to add Baikal
SoC GMAC and X-GMAC (based on the DW GMAC and X-GMAC IP-cores) support
with as less modifications as possible. But the more features we developed
the more problems we discovered including actual bugs (currently there are
three fixes patchset we managed to collect). So after a lot of thoughts we
decided first to provide a series of patchsets with bug fixes, cleanups,
optimizations and refactoring of the driver parts thus having a driver
more ready to be updated with our versions DW GMAC/X-GMAC features and
getting more stable, simpler to read and maintain code after all.
All the work which was done in regards to the announced alterations
(and yet to be done) is split up into a set of more-or-less coherent
patchsets which will be submitted one after another upon review process
advance. The only exception is the DT-bindings series which contains
independent changes and can be delivered before ahead the rest of the
patchsets.
This patchset is the first one in the series of updates implemented in the
framework of activity to simplify the DW MAC/STMMAC driver code and
provide Baikal GMAC/X-GMAC support after all:
+> [1: In-review v1]: net: stmmac: Fixes bundle #1
+> Link: ---you are looking at it---
[2: Stalled v1]: net: stmmac: Fixes bundle #2
Link: ---not submitted yet---
[3: Stalled v1]: net: stmmac: Fixes bundle #3
Link: ---not submitted yet---
[4: Stalled v1]: dt-bindings: net: dwmac: Extend clocks, props desc and constraints
Link: ---not submitted yet---
[5: Stalled v1]: dt-bindings: net: dwmac: Fix MTL queues and AXI-bus props
Link: ---not submitted yet---
[6: Stalled v1]: net: stmmac: Generic platform res, props and DMA cleanups
Link: ---not submitted yet---
[7: Stalled v1]: net: stmmac: Generic platform rst, phy and clk cleanups
Link: ---not submitted yet---
[8: Stalled v1]: net: stmmac: Main driver code cleanups bundle #1
Link: ---not submitted yet---
[9: Stalled v1]: net: stmmac: Main driver code cleanups bundle #2
Link: ---not submitted yet---
[10: Stalled v1]: net: stmmac: DW MAC HW info init refactoring
Link: ---not submitted yet---
[11: Stalled v1]: net: stmmac: Convert to using HW capabilities bundle #1
Link: ---not submitted yet---
[12: Stalled v1]: net: stmmac: Convert to using HW capabilities bundle #2
Link: ---not submitted yet---
[13: Stalled v1]: net: stmmac: Convert to using HW capabilities bundle #3
Link: ---not submitted yet---
[14: Stalled v1]: net: stmmac: Convert to using HW capabilities bundle #4
Link: ---not submitted yet---
[15: Stalled v1]: net: stmmac: Unify/simplify HW-interface
Link: ---not submitted yet---
[16: Stalled v1]: net: stmmac: Norm/Enh/etc DMA descriptor init fixes
Link: ---not submitted yet---
[17: Stalled v1]: net: stmmac: Norm/Enh/etc DMA descriptor init cleanups
Link: ---not submitted yet---
[18: Stalled v1]: net: stmmac: Main driver code cleanups bundle #3
Link: ---not submitted yet---
[..: In-prep] to be continued (IRQ handling refactoring, SW-reset-less config,
generic GPIO support, ARP offload support,
In-band RGMII link state, etc)
[..: In-prep] to be continued (Baikal-{T,M,L,S} SoCs GMAC, X-GMAC and XPCS
support)
Here is a short summary of what is introduced in this patchset.
The series starts with a fix, which I submitted two years ago. It concerns
the RTL8211E PHY working in the LPI-mode with the RX clock gating. In
particular it was discovered that disabling RXC in LPI (EEE) causes
RTL8211E PHY partial freeze until the next MDIO read operation from the
PHY CSRs. We suggest to fix that problem by dummy reading from the MMD
Data register each time the PC1R.10 bit is intended to be set. See the
patch log for more details.
Afterwards a set of the DW (G)MAC chain-mode fixes go. In particular the
first fix concerns setting the LS flag for jumbos if not the last segment
of the payload is passed within skb (similar fix was provided for the ring
mode sometime ago). The second patch fixes the invalid DMA descriptors
pointer calculation for jumbo frame transmission procedure. The third
modification prevents the global chain-mode flag modification for the
Sun8i NICs. Final update makes sure that ATDS flag (Extended Enhanced DMA
descriptors support is activated) is set no matter whether ring or chain
mode is activated.
Then two fixes of the data corruption and memory leaks are introduced. The
first one concerns the SKB saved in the Rx-state cache, which left
available after the NIC re-open cycle thus causing improper reception of
the first frame. The second patch fixes the Rx DMA descriptors memory
left allocated on the Tx DMA descriptors allocation failure.
Afterwards the Rx-mitigation algorithm fix is provided. Currently it
turned out to be malfunction since the Rx interrupt is only triggered
based on the Rx watchdog timer meanwhile it is supposed to be triggered
either by the watchdog timer or after receiving a defined number of
frames.
Then a fix of the buf_sz kernel module parameter goes. In particular
it converts the parameter to indicating the minimal value of the Rx DMA
buffers size instead of being completely useless.
The next patch fixes the inconsistency in the DW GMAC v4.10 DMA ops
descriptor, which for unknown reason was initialized with the pointer to
dwmac4_disable_dma_irq() procedure instead of using the dedicated for
v4.10 dwmac410_disable_dma_irq() method. That didn't cause any problem
because the modified bits matches in both IP-core revisions, but for
consistency we suggest to fix that.
The patchset continues with the PTP-part of the driver fix. The
corresponding patch prevents the stmmac_ptp_register() procedure from
modifying the fields of the ptp_clock_info static structure instance.
Finally the series is closed with a patch fixing the Loongson PCI MAC
glue-driver, which erroneously accepts the zero IRQ number as valid
(see of_irq_get*() semantics for details).
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Pavel Parkhomenko <[email protected]>
Cc: Christian Marangi <[email protected]>
Cc: Biao Huang <[email protected]>
Cc: Yang Yingliang <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Serge Semin (13):
net: phy: realtek: Fix events detection failure in LPI mode
net: stmmac: Omit last desc flag for non-linear jumbos in chain-mode
net: stmmac: Fix extended descriptors usage for jumbos in chain-mode
net: stmmac: dwmac-sun8i: Don't modify chain-mode module parameter
net: stmmac: Enable ATDS for chain-mode
net: stmmac: Free temporary Rx SKB on request
net: stmmac: Free Rx descs on Tx allocation failure
net: stmmac: Fix Rx IC bit setting procedure for Rx-mitigation
net: stmmac: Remove default maxmtu DT-platform setting
net: stmmac: Make buf_sz module parameter useful
net: stmmac: gmac4: Use dwmac410_disable_dma_irq for DW MAC v4.10 DMA
net: stmmac: Stop overriding the PTP clock info static instance
net: dwmac-loongson: Perceive zero IRQ as invalid
.../net/ethernet/stmicro/stmmac/chain_mode.c | 14 +++++--
.../ethernet/stmicro/stmmac/dwmac-loongson.c | 6 +--
.../net/ethernet/stmicro/stmmac/dwmac4_dma.c | 2 +-
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 39 +++++++++----------
.../ethernet/stmicro/stmmac/stmmac_platform.c | 5 ---
.../net/ethernet/stmicro/stmmac/stmmac_ptp.c | 19 ++++-----
drivers/net/phy/realtek.c | 37 ++++++++++++++++++
7 files changed, 77 insertions(+), 45 deletions(-)
--
2.39.2
If a DW MAC NIC is synthesized to support the extended descriptors, then
the driver uses dma_etx pointer to keep an array of once in the Tx DMA
Queue structures. For some reason the specific to the chained-mode
jumbo_frm() referred to the dma_tx pointer of DMA Tx Queue descriptor in
any case, which of course was initialized with NULL for the DW MACs
expecting extended descriptors being specified. So any attempt to send a
Jumbo frame in chain-mode caused "Unable to handle kernel paging request
at virtual address" kernel crash. Fix that by selecting a proper
descriptor pointer depending on whether the NIC supports extended
descriptors or not.
Fixes: c24602ef8664 ("stmmac: support extend descriptors")
Signed-off-by: Serge Semin <[email protected]>
---
Yeah, that Normal/Enhanced access pattern really annoying. Food for
thoughts about a more thorough cleanup of the driver.
---
drivers/net/ethernet/stmicro/stmmac/chain_mode.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/chain_mode.c b/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
index 60e4fa5060ce..9a92f0c5e577 100644
--- a/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
+++ b/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
@@ -24,7 +24,10 @@ static int jumbo_frm(void *p, struct sk_buff *skb, int csum)
unsigned int i = 1, len;
struct dma_desc *desc;
- desc = tx_q->dma_tx + entry;
+ if (priv->extend_desc)
+ desc = (struct dma_desc *)(tx_q->dma_etx + entry);
+ else
+ desc = tx_q->dma_tx + entry;
if (priv->plat->enh_desc)
bmax = BUF_SIZE_8KiB;
@@ -47,7 +50,11 @@ static int jumbo_frm(void *p, struct sk_buff *skb, int csum)
while (len != 0) {
tx_q->tx_skbuff[entry] = NULL;
entry = STMMAC_GET_ENTRY(entry, priv->dma_conf.dma_tx_size);
- desc = tx_q->dma_tx + entry;
+
+ if (priv->extend_desc)
+ desc = (struct dma_desc *)(tx_q->dma_etx + entry);
+ else
+ desc = tx_q->dma_tx + entry;
if (len > bmax) {
des2 = dma_map_single(priv->device,
--
2.39.2
Doing so activates the chain-mode for any DW MAC-based NIC on the platform
no matter with what parameter the module is loaded. Even if there is no
any other network controller on the SoC it is logically incorrect.
Fixes: 9f93ac8d4085 ("net-next: stmmac: Add dwmac-sun8i")
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 8f543c3ab5c5..2ed63acaee5b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6806,8 +6806,9 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
/* dwmac-sun8i only work in chain mode */
if (priv->plat->has_sun8i)
- chain_mode = 1;
- priv->chain_mode = chain_mode;
+ priv->chain_mode = 1;
+ else
+ priv->chain_mode = chain_mode;
/* Initialize HW Interface */
ret = stmmac_hwif_init(priv);
--
2.39.2
In case if an incoming frame couldn't be finished in one stmmac_rx()
method call an SKB used to collect data so far will be saved in the
corresponding Rx-queue state buffer. If the network device is closed
before the frame is completed the preserved SKB will be utilized on the
next network interface link uprising cycle right on the first frame
reception, which will cause having a confused set of SKB data. Let's free
the allocated Rx SKB then when all Rx-buffers are requested to be freed.
Fixes: ec222003bd94 ("net: stmmac: Prepare to add Split Header support")
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index ee4297a25521..4d643b1bbf65 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1545,6 +1545,10 @@ static void dma_free_rx_skbufs(struct stmmac_priv *priv,
for (i = 0; i < dma_conf->dma_rx_size; i++)
stmmac_free_rx_buffer(priv, rx_q, i);
+
+ if (rx_q->state_saved)
+ dev_kfree_skb(rx_q->state.skb);
+ rx_q->state_saved = false;
}
static int stmmac_alloc_rx_buffers(struct stmmac_priv *priv,
--
2.39.2
Indeed in accordance with the alloc_dma_desc_resources() method logic the
Rx descriptors will be left allocated if Tx descriptors allocation fails.
Fix it by calling the free_dma_rx_desc_resources() in case if the
alloc_dma_tx_desc_resources() method returns non-zero value.
While at it refactor the method a bit. Just move the Rx descriptors
allocation method invocation out of the local variables declaration block
and discard a pointless comment from there.
Fixes: 71fedb0198cb ("net: stmmac: break some functions into RX and TX scopes")
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 4d643b1bbf65..229f827d7572 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2182,13 +2182,15 @@ static int alloc_dma_tx_desc_resources(struct stmmac_priv *priv,
static int alloc_dma_desc_resources(struct stmmac_priv *priv,
struct stmmac_dma_conf *dma_conf)
{
- /* RX Allocation */
- int ret = alloc_dma_rx_desc_resources(priv, dma_conf);
+ int ret;
+ ret = alloc_dma_rx_desc_resources(priv, dma_conf);
if (ret)
return ret;
ret = alloc_dma_tx_desc_resources(priv, dma_conf);
+ if (ret)
+ free_dma_rx_desc_resources(priv, dma_conf);
return ret;
}
--
2.39.2
It wasn't stated why the Alternate Descriptor Size shouldn't have been
changed for the chained DMA descriptors, while the original commit did
introduce some chain_mode.c modifications. So the ATDS-related change in
commit c24602ef8664 ("stmmac: support extend descriptors") seems
contradicting. Anyway from the DW MAC/GMAC hardware point of view there is
no such limitation - whether the chained descriptors can't be used
together with the extended alternate descriptors. Moreover not setting the
BUS_MODE.ATDS flag will cause the driver malfunction in the framework of
the IPC Full Checksum and Advanced Timestamp feature, since the later
features require the additional 4-7 dwords of the DMA descriptor to set
some flags and a timestamp. So to speak in order to have all these
features working correctly in the chained mode too let's make sure the
ATDS flag is set irrespectively from the DMA descriptors mode.
Fixes: c24602ef8664 ("stmmac: support extend descriptors")
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 2ed63acaee5b..ee4297a25521 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2907,7 +2907,6 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
struct stmmac_rx_queue *rx_q;
struct stmmac_tx_queue *tx_q;
u32 chan = 0;
- int atds = 0;
int ret = 0;
if (!priv->plat->dma_cfg || !priv->plat->dma_cfg->pbl) {
@@ -2915,9 +2914,6 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
return -EINVAL;
}
- if (priv->extend_desc && (priv->mode == STMMAC_RING_MODE))
- atds = 1;
-
ret = stmmac_reset(priv, priv->ioaddr);
if (ret) {
dev_err(priv->device, "Failed to reset the dma\n");
@@ -2925,7 +2921,8 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
}
/* DMA Configuration */
- stmmac_dma_init(priv, priv->ioaddr, priv->plat->dma_cfg, atds);
+ stmmac_dma_init(priv, priv->ioaddr, priv->plat->dma_cfg,
+ priv->extend_desc);
if (priv->plat->axi)
stmmac_axi(priv, priv->ioaddr, priv->plat->axi);
--
2.39.2
Initializing maxmtu platform parameter in the stmmac_probe_config_dt()
method by default makes being pointless the DW MAC-specific maximum MTU
selection algorithm implemented in the stmmac_dvr_probe() method. At least
for xGMAC we'll always have a frame MTU limited with 9000 while it
supports units up to 16KB. Let's remove the default initialization of
the maxmtu platform setting then. We don't replace it with setting the
maxmtu with some greater value because a default maximum MTU is
calculated later in the stmmac_dvr_probe() anyway. That would have been a
pointless limitation too. Instead from now the main STMMAC driver code
will consider the out of bounds maxmtu value as invalid and will silently
replace it with a maximum MTU value specific to the corresponding DW MAC.
Note this alteration will only affect the xGMAC IP-cores due to the way
the MTU autodetecion algorithm is implemented. So from now the driver will
permit DW xGMACs to handle frames up to 16KB length (XGMAC_JUMBO_LEN). As
before DW GMAC IP-cores of v4.0 and higher and IP-cores with enhanced
descriptor support will be able to work with frames up to 8KB (JUMBO_LEN).
The rest of the NICs will support frames of SKB_MAX_HEAD(NET_SKB_PAD +
NET_IP_ALIGN) size.
Fixes: 7d9e6c5afab6 ("net: stmmac: Integrate XGMAC into main driver flow")
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ----
drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 5 -----
2 files changed, 9 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 32aa7953d296..e5cb4edc4e23 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7252,10 +7252,6 @@ int stmmac_dvr_probe(struct device *device,
if ((priv->plat->maxmtu < ndev->max_mtu) &&
(priv->plat->maxmtu >= ndev->min_mtu))
ndev->max_mtu = priv->plat->maxmtu;
- else if (priv->plat->maxmtu < ndev->min_mtu)
- dev_warn(priv->device,
- "%s: warning: maxmtu having invalid value (%d)\n",
- __func__, priv->plat->maxmtu);
if (flow_ctrl)
priv->flow_ctrl = FLOW_AUTO; /* RX/TX pause on */
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 067a40fe0a23..857411105a0a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -468,11 +468,6 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
plat->en_tx_lpi_clockgating =
of_property_read_bool(np, "snps,en-tx-lpi-clockgating");
- /* Set the maxmtu to a default of JUMBO_LEN in case the
- * parameter is not present in the device tree.
- */
- plat->maxmtu = JUMBO_LEN;
-
/* Set default value for multicast hash bins */
plat->multicast_filter_bins = HASH_TABLE_SIZE;
--
2.39.2
Since the first day of the driver been added to the kernel the Rx
DMA-buffer size module parameter hasn't been utilized for something
more-or-less useful. The STMMAC main code used to initialize the actual Rx
DMA-buffer size (priv->dma_buf_sz) with it by default, but then overwrote
it anyway in accordance with Network device MTU or with default
DMA_BUFFER_SIZE/DEFAULT_BUFSIZE macro. Moreover in the both older and
modern versions of the STMMAC driver the buf_sz module parameter was
overwritten on each Net-dev open procedure with a value calculated in
accordance with MTU and allowed maximum buffer size. Needless to say that
even though it doesn't affect the driver functionality much, setting the
generic parameter with a value specific to a private device is wrong,
seeing each DW MAC may have different maximum buffer size on the same
platform.
In order to finally make the buf_sz (Rx DMA buffer size) parameter really
useful let's stop setting it up with a value specific to a particular
controller and use it to calculate the Rx DMA-buffer size instead of MTU
if one is greater than the later. Thus we'll be able to tune the buffer
size up when it's necessary.
Fixes: 47dd7a540b8a ("net: add support for STMicroelectronics Ethernet controllers.")
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index e5cb4edc4e23..12de84b7e90d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -100,10 +100,10 @@ static int tc = TC_DEFAULT;
module_param(tc, int, 0644);
MODULE_PARM_DESC(tc, "DMA threshold control value");
-#define DEFAULT_BUFSIZE 1536
-static int buf_sz = DEFAULT_BUFSIZE;
-module_param(buf_sz, int, 0644);
-MODULE_PARM_DESC(buf_sz, "DMA buffer size");
+#define DEFAULT_BUFSIZE 1536U
+static unsigned int buf_sz = DEFAULT_BUFSIZE;
+module_param(buf_sz, uint, 0644);
+MODULE_PARM_DESC(buf_sz, "Rx DMA buffer size");
#define STMMAC_RX_COPYBREAK 256
@@ -3730,7 +3730,7 @@ stmmac_setup_dma_desc(struct stmmac_priv *priv, unsigned int mtu)
bfsize = 0;
if (bfsize < BUF_SIZE_16KiB)
- bfsize = stmmac_set_bfsize(mtu, 0);
+ bfsize = stmmac_set_bfsize(max(buf_sz, mtu), 0);
dma_conf->dma_buf_sz = bfsize;
/* Chose the tx/rx size from the already defined one in the
@@ -3817,7 +3817,6 @@ static int __stmmac_open(struct net_device *dev,
priv->rx_copybreak = STMMAC_RX_COPYBREAK;
- buf_sz = dma_conf->dma_buf_sz;
memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf));
stmmac_reset_queues_param(priv);
--
2.39.2
It had been defined as constant before commit 9a8a02c9d46d ("net: stmmac:
Add Flexible PPS support"). But then it was converted to be just static,
which fields may get to be modified on each stmmac_ptp_register()
invocation. Since that method is called from the driver probe method, a
concurrent DW *MAC NIC initialization causes the race condition for the
updated stmmac_ptp_clock_ops fields. That also may lead to setting an
inappropriate max_adj value, which was specific for one device, was
written to the stmmac_ptp_clock_ops, but then copied to the private
ptp_clock_info instance unmodified.
So to speak let's leave the stmmac_ptp_clock_ops content untouched and
just copy it to the device-specific instance of the ptp_clock_info
structure, which fields could be then accordingly modified. After that we
can get the const qualifier back to the stmmac_ptp_clock_ops instance
definition.
While at it remove pointless zero-initialization of the
stmmac_ptp_clock_ops fields. It's redundant since the structure is static.
Fixes: 9a8a02c9d46d ("net: stmmac: Add Flexible PPS support")
Fixes: 190f73ab4c43 ("net: stmmac: setup higher frequency clk support for EHL & TGL")
Fixes: f4da56529da6 ("net: stmmac: Add support for external trigger timestamping")
Signed-off-by: Serge Semin <[email protected]>
---
.../net/ethernet/stmicro/stmmac/stmmac_ptp.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index b4388ca8d211..19a28b1cc272 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
@@ -254,15 +254,10 @@ static int stmmac_getcrosststamp(struct ptp_clock_info *ptp,
}
/* structure describing a PTP hardware clock */
-static struct ptp_clock_info stmmac_ptp_clock_ops = {
+static const struct ptp_clock_info stmmac_ptp_clock_ops = {
.owner = THIS_MODULE,
.name = "stmmac ptp",
.max_adj = 62500000,
- .n_alarm = 0,
- .n_ext_ts = 0, /* will be overwritten in stmmac_ptp_register */
- .n_per_out = 0, /* will be overwritten in stmmac_ptp_register */
- .n_pins = 0,
- .pps = 0,
.adjfine = stmmac_adjust_freq,
.adjtime = stmmac_adjust_time,
.gettime64 = stmmac_get_time,
@@ -287,21 +282,21 @@ void stmmac_ptp_register(struct stmmac_priv *priv)
priv->pps[i].available = true;
}
- if (priv->plat->ptp_max_adj)
- stmmac_ptp_clock_ops.max_adj = priv->plat->ptp_max_adj;
-
/* Calculate the clock domain crossing (CDC) error if necessary */
priv->plat->cdc_error_adj = 0;
if (priv->plat->has_gmac4 && priv->plat->clk_ptp_rate)
priv->plat->cdc_error_adj = (2 * NSEC_PER_SEC) / priv->plat->clk_ptp_rate;
- stmmac_ptp_clock_ops.n_per_out = priv->dma_cap.pps_out_num;
- stmmac_ptp_clock_ops.n_ext_ts = priv->dma_cap.aux_snapshot_n;
-
rwlock_init(&priv->ptp_lock);
mutex_init(&priv->aux_ts_lock);
priv->ptp_clock_ops = stmmac_ptp_clock_ops;
+ if (priv->plat->ptp_max_adj)
+ priv->ptp_clock_ops.max_adj = priv->plat->ptp_max_adj;
+
+ priv->ptp_clock_ops.n_per_out = priv->dma_cap.pps_out_num;
+ priv->ptp_clock_ops.n_ext_ts = priv->dma_cap.aux_snapshot_n;
+
priv->ptp_clock = ptp_clock_register(&priv->ptp_clock_ops,
priv->device);
if (IS_ERR(priv->ptp_clock)) {
--
2.39.2
The WDT-less Rx-traffic mitigation is simple: enable the Rx-descriptor
Completion interrupt only when a particular number of descriptors handled
by the NIC. Alas it's broken now because the data variable rx_count_frames
is always set to zero due to commit 6fa9d691b91a ("net: stmmac: Prevent
divide-by-zero") (++rx_count_frames + rx_coal_frames > rx_coal_frames
always if no overflow happens). So Rx IC will be enabled for each
descriptor as soon as rx_coal_frames is non-zero or there is no Rx WDT
available, which basically disables the Rx-mitigation in the framework of
the number of received frames part. Fix that by discarding the statement:
rx_q->rx_count_frames += priv->rx_coal_frames.
Fixes: 6fa9d691b91a ("net: stmmac: Prevent divide-by-zero")
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 229f827d7572..32aa7953d296 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4627,7 +4627,6 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
stmmac_refill_desc3(priv, rx_q, p);
rx_q->rx_count_frames++;
- rx_q->rx_count_frames += priv->rx_coal_frames[queue];
if (rx_q->rx_count_frames > priv->rx_coal_frames[queue])
rx_q->rx_count_frames = 0;
@@ -4967,7 +4966,6 @@ static bool stmmac_rx_refill_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
stmmac_refill_desc3(priv, rx_q, rx_desc);
rx_q->rx_count_frames++;
- rx_q->rx_count_frames += priv->rx_coal_frames[queue];
if (rx_q->rx_count_frames > priv->rx_coal_frames[queue])
rx_q->rx_count_frames = 0;
--
2.39.2
From the very beginning of the DW GMAC v4.10 IP support the driver has used
an invalid DMA IRQ disable method to switch the DMA IRQs off. Since
commit 021bd5e36970 ("net: stmmac: Let TX and RX interrupts be
independently enabled/disabled") a valid method has been added to the
dwmac4_lib.c module, but the commit author forgot to initialize the
corresponding field of the DW MAC DMA operations descriptor with it. That
mistake hasn't caused any problem so far just because the RIE/TIE fields
match in both 4.x and 4.10 IPs. Anyway fix the inconsistency in order to
at least have a coherent driver code.
Fixes: 021bd5e36970 ("net: stmmac: Let TX and RX interrupts be independently enabled/disabled")
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
index d99fa028c646..2b85819a560f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
@@ -557,7 +557,7 @@ const struct stmmac_dma_ops dwmac410_dma_ops = {
.dma_rx_mode = dwmac4_dma_rx_chan_op_mode,
.dma_tx_mode = dwmac4_dma_tx_chan_op_mode,
.enable_dma_irq = dwmac410_enable_dma_irq,
- .disable_dma_irq = dwmac4_disable_dma_irq,
+ .disable_dma_irq = dwmac410_disable_dma_irq,
.start_tx = dwmac4_dma_start_tx,
.stop_tx = dwmac4_dma_stop_tx,
.start_rx = dwmac4_dma_start_rx,
--
2.39.2
Linux kernel defines zero IRQ number as invalid in case if IRQ couldn't be
mapped. Fix that for Loongson PCI MAC specific IRQs request procedure.
Fixes: 30bba69d7db4 ("stmmac: pci: Add dwmac support for Loongson")
Signed-off-by: Serge Semin <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
index a25c187d3185..907bdfcc07e9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
@@ -127,20 +127,20 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
res.addr = pcim_iomap_table(pdev)[0];
res.irq = of_irq_get_byname(np, "macirq");
- if (res.irq < 0) {
+ if (res.irq <= 0) {
dev_err(&pdev->dev, "IRQ macirq not found\n");
ret = -ENODEV;
goto err_disable_msi;
}
res.wol_irq = of_irq_get_byname(np, "eth_wake_irq");
- if (res.wol_irq < 0) {
+ if (res.wol_irq <= 0) {
dev_info(&pdev->dev, "IRQ eth_wake_irq not found, using macirq\n");
res.wol_irq = res.irq;
}
res.lpi_irq = of_irq_get_byname(np, "eth_lpi");
- if (res.lpi_irq < 0) {
+ if (res.lpi_irq <= 0) {
dev_err(&pdev->dev, "IRQ eth_lpi not found\n");
ret = -ENODEV;
goto err_disable_msi;
--
2.39.2
On Tue, Mar 14, 2023 at 01:42:37AM +0300, Serge Semin wrote:
> Linux kernel defines zero IRQ number as invalid in case if IRQ couldn't be
> mapped. Fix that for Loongson PCI MAC specific IRQs request procedure.
>
Looks a little odd but as I also checked and kernel does seem to treat
zero as mapping failure instead of -WHATEVER.
Fix looks fine.
Reviewed-by: Piotr Raczynski <[email protected]>
> Fixes: 30bba69d7db4 ("stmmac: pci: Add dwmac support for Loongson")
> Signed-off-by: Serge Semin <[email protected]>
> ---
> drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> index a25c187d3185..907bdfcc07e9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> @@ -127,20 +127,20 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
> res.addr = pcim_iomap_table(pdev)[0];
>
> res.irq = of_irq_get_byname(np, "macirq");
> - if (res.irq < 0) {
> + if (res.irq <= 0) {
> dev_err(&pdev->dev, "IRQ macirq not found\n");
> ret = -ENODEV;
> goto err_disable_msi;
> }
>
> res.wol_irq = of_irq_get_byname(np, "eth_wake_irq");
> - if (res.wol_irq < 0) {
> + if (res.wol_irq <= 0) {
> dev_info(&pdev->dev, "IRQ eth_wake_irq not found, using macirq\n");
> res.wol_irq = res.irq;
> }
>
> res.lpi_irq = of_irq_get_byname(np, "eth_lpi");
> - if (res.lpi_irq < 0) {
> + if (res.lpi_irq <= 0) {
> dev_err(&pdev->dev, "IRQ eth_lpi not found\n");
> ret = -ENODEV;
> goto err_disable_msi;
> --
> 2.39.2
>
>
On Tue, Mar 14, 2023 at 01:42:31AM +0300, Serge Semin wrote:
> Indeed in accordance with the alloc_dma_desc_resources() method logic the
> Rx descriptors will be left allocated if Tx descriptors allocation fails.
> Fix it by calling the free_dma_rx_desc_resources() in case if the
> alloc_dma_tx_desc_resources() method returns non-zero value.
>
> While at it refactor the method a bit. Just move the Rx descriptors
> allocation method invocation out of the local variables declaration block
> and discard a pointless comment from there.
>
LGTM, Thanks.
Reviewed-by: Piotr Raczynski <[email protected]>
> Fixes: 71fedb0198cb ("net: stmmac: break some functions into RX and TX scopes")
> Signed-off-by: Serge Semin <[email protected]>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 4d643b1bbf65..229f827d7572 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2182,13 +2182,15 @@ static int alloc_dma_tx_desc_resources(struct stmmac_priv *priv,
> static int alloc_dma_desc_resources(struct stmmac_priv *priv,
> struct stmmac_dma_conf *dma_conf)
> {
> - /* RX Allocation */
> - int ret = alloc_dma_rx_desc_resources(priv, dma_conf);
> + int ret;
>
> + ret = alloc_dma_rx_desc_resources(priv, dma_conf);
> if (ret)
> return ret;
>
> ret = alloc_dma_tx_desc_resources(priv, dma_conf);
> + if (ret)
> + free_dma_rx_desc_resources(priv, dma_conf);
>
> return ret;
> }
> --
> 2.39.2
>
>
On Tue, Mar 14, 2023 at 01:42:33AM +0300, Serge Semin wrote:
> Initializing maxmtu platform parameter in the stmmac_probe_config_dt()
> method by default makes being pointless the DW MAC-specific maximum MTU
> selection algorithm implemented in the stmmac_dvr_probe() method. At least
> for xGMAC we'll always have a frame MTU limited with 9000 while it
> supports units up to 16KB. Let's remove the default initialization of
> the maxmtu platform setting then. We don't replace it with setting the
> maxmtu with some greater value because a default maximum MTU is
> calculated later in the stmmac_dvr_probe() anyway. That would have been a
> pointless limitation too. Instead from now the main STMMAC driver code
> will consider the out of bounds maxmtu value as invalid and will silently
> replace it with a maximum MTU value specific to the corresponding DW MAC.
>
> Note this alteration will only affect the xGMAC IP-cores due to the way
> the MTU autodetecion algorithm is implemented. So from now the driver will
> permit DW xGMACs to handle frames up to 16KB length (XGMAC_JUMBO_LEN). As
> before DW GMAC IP-cores of v4.0 and higher and IP-cores with enhanced
> descriptor support will be able to work with frames up to 8KB (JUMBO_LEN).
> The rest of the NICs will support frames of SKB_MAX_HEAD(NET_SKB_PAD +
> NET_IP_ALIGN) size.
>
> Fixes: 7d9e6c5afab6 ("net: stmmac: Integrate XGMAC into main driver flow")
> Signed-off-by: Serge Semin <[email protected]>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ----
> drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 5 -----
> 2 files changed, 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 32aa7953d296..e5cb4edc4e23 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7252,10 +7252,6 @@ int stmmac_dvr_probe(struct device *device,
> if ((priv->plat->maxmtu < ndev->max_mtu) &&
> (priv->plat->maxmtu >= ndev->min_mtu))
> ndev->max_mtu = priv->plat->maxmtu;
> - else if (priv->plat->maxmtu < ndev->min_mtu)
> - dev_warn(priv->device,
> - "%s: warning: maxmtu having invalid value (%d)\n",
> - __func__, priv->plat->maxmtu);
Looks fine but by removing plat->maxmtu = JUMBO_LEN; you eliminate the
case of dev_warn here or you remove dev_warn since the driver will be
able to fix the mtu value?
>
> if (flow_ctrl)
> priv->flow_ctrl = FLOW_AUTO; /* RX/TX pause on */
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 067a40fe0a23..857411105a0a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -468,11 +468,6 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
> plat->en_tx_lpi_clockgating =
> of_property_read_bool(np, "snps,en-tx-lpi-clockgating");
>
> - /* Set the maxmtu to a default of JUMBO_LEN in case the
> - * parameter is not present in the device tree.
> - */
> - plat->maxmtu = JUMBO_LEN;
> -
> /* Set default value for multicast hash bins */
> plat->multicast_filter_bins = HASH_TABLE_SIZE;
>
> --
> 2.39.2
>
>
On Tue, Mar 14, 2023 at 01:42:30AM +0300, Serge Semin wrote:
> In case if an incoming frame couldn't be finished in one stmmac_rx()
> method call an SKB used to collect data so far will be saved in the
> corresponding Rx-queue state buffer. If the network device is closed
> before the frame is completed the preserved SKB will be utilized on the
> next network interface link uprising cycle right on the first frame
> reception, which will cause having a confused set of SKB data. Let's free
> the allocated Rx SKB then when all Rx-buffers are requested to be freed.
>
> Fixes: ec222003bd94 ("net: stmmac: Prepare to add Split Header support")
> Signed-off-by: Serge Semin <[email protected]>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ee4297a25521..4d643b1bbf65 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1545,6 +1545,10 @@ static void dma_free_rx_skbufs(struct stmmac_priv *priv,
>
> for (i = 0; i < dma_conf->dma_rx_size; i++)
> stmmac_free_rx_buffer(priv, rx_q, i);
> +
> + if (rx_q->state_saved)
> + dev_kfree_skb(rx_q->state.skb);
> + rx_q->state_saved = false;
> }
>
> static int stmmac_alloc_rx_buffers(struct stmmac_priv *priv,
> --
> 2.39.2
>
>
LGTM, thanks.
Reviewed-by: Piotr Raczynski <[email protected]>
On Tue, Mar 14, 2023 at 09:20:42AM +0100, Piotr Raczynski wrote:
> On Tue, Mar 14, 2023 at 01:42:37AM +0300, Serge Semin wrote:
> > Linux kernel defines zero IRQ number as invalid in case if IRQ couldn't be
> > mapped. Fix that for Loongson PCI MAC specific IRQs request procedure.
> >
>
> Looks a little odd but as I also checked and kernel does seem to treat
> zero as mapping failure instead of -WHATEVER.
See what Linus originally told about that:
https://lore.kernel.org/lkml/[email protected]/
>
> Fix looks fine.
>
> Reviewed-by: Piotr Raczynski <[email protected]>
Thanks.
-Serge(y)
>
> > Fixes: 30bba69d7db4 ("stmmac: pci: Add dwmac support for Loongson")
> > Signed-off-by: Serge Semin <[email protected]>
> > ---
> > drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > index a25c187d3185..907bdfcc07e9 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> > @@ -127,20 +127,20 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
> > res.addr = pcim_iomap_table(pdev)[0];
> >
> > res.irq = of_irq_get_byname(np, "macirq");
> > - if (res.irq < 0) {
> > + if (res.irq <= 0) {
> > dev_err(&pdev->dev, "IRQ macirq not found\n");
> > ret = -ENODEV;
> > goto err_disable_msi;
> > }
> >
> > res.wol_irq = of_irq_get_byname(np, "eth_wake_irq");
> > - if (res.wol_irq < 0) {
> > + if (res.wol_irq <= 0) {
> > dev_info(&pdev->dev, "IRQ eth_wake_irq not found, using macirq\n");
> > res.wol_irq = res.irq;
> > }
> >
> > res.lpi_irq = of_irq_get_byname(np, "eth_lpi");
> > - if (res.lpi_irq < 0) {
> > + if (res.lpi_irq <= 0) {
> > dev_err(&pdev->dev, "IRQ eth_lpi not found\n");
> > ret = -ENODEV;
> > goto err_disable_msi;
> > --
> > 2.39.2
> >
> >
On Tue, Mar 14, 2023 at 01:42:29AM +0300, Serge Semin wrote:
> It wasn't stated why the Alternate Descriptor Size shouldn't have been
> changed for the chained DMA descriptors, while the original commit did
> introduce some chain_mode.c modifications. So the ATDS-related change in
> commit c24602ef8664 ("stmmac: support extend descriptors") seems
> contradicting. Anyway from the DW MAC/GMAC hardware point of view there is
> no such limitation - whether the chained descriptors can't be used
> together with the extended alternate descriptors. Moreover not setting the
> BUS_MODE.ATDS flag will cause the driver malfunction in the framework of
> the IPC Full Checksum and Advanced Timestamp feature, since the later
> features require the additional 4-7 dwords of the DMA descriptor to set
> some flags and a timestamp. So to speak in order to have all these
> features working correctly in the chained mode too let's make sure the
> ATDS flag is set irrespectively from the DMA descriptors mode.
>
> Fixes: c24602ef8664 ("stmmac: support extend descriptors")
> Signed-off-by: Serge Semin <[email protected]>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 2ed63acaee5b..ee4297a25521 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2907,7 +2907,6 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
> struct stmmac_rx_queue *rx_q;
> struct stmmac_tx_queue *tx_q;
> u32 chan = 0;
> - int atds = 0;
> int ret = 0;
>
> if (!priv->plat->dma_cfg || !priv->plat->dma_cfg->pbl) {
> @@ -2915,9 +2914,6 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
> return -EINVAL;
> }
>
> - if (priv->extend_desc && (priv->mode == STMMAC_RING_MODE))
> - atds = 1;
> -
> ret = stmmac_reset(priv, priv->ioaddr);
> if (ret) {
> dev_err(priv->device, "Failed to reset the dma\n");
> @@ -2925,7 +2921,8 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
> }
>
> /* DMA Configuration */
> - stmmac_dma_init(priv, priv->ioaddr, priv->plat->dma_cfg, atds);
> + stmmac_dma_init(priv, priv->ioaddr, priv->plat->dma_cfg,
> + priv->extend_desc);
>
> if (priv->plat->axi)
> stmmac_axi(priv, priv->ioaddr, priv->plat->axi);
> --
> 2.39.2
>
>
Looks fine, thanks.
Reviewed-by: Piotr Raczynski <[email protected]>
On Tue, Mar 14, 2023 at 12:20:18PM +0100, Piotr Raczynski wrote:
> On Tue, Mar 14, 2023 at 01:42:33AM +0300, Serge Semin wrote:
> > Initializing maxmtu platform parameter in the stmmac_probe_config_dt()
> > method by default makes being pointless the DW MAC-specific maximum MTU
> > selection algorithm implemented in the stmmac_dvr_probe() method. At least
> > for xGMAC we'll always have a frame MTU limited with 9000 while it
> > supports units up to 16KB. Let's remove the default initialization of
> > the maxmtu platform setting then. We don't replace it with setting the
> > maxmtu with some greater value because a default maximum MTU is
> > calculated later in the stmmac_dvr_probe() anyway. That would have been a
> > pointless limitation too. Instead from now the main STMMAC driver code
> > will consider the out of bounds maxmtu value as invalid and will silently
> > replace it with a maximum MTU value specific to the corresponding DW MAC.
> >
> > Note this alteration will only affect the xGMAC IP-cores due to the way
> > the MTU autodetecion algorithm is implemented. So from now the driver will
> > permit DW xGMACs to handle frames up to 16KB length (XGMAC_JUMBO_LEN). As
> > before DW GMAC IP-cores of v4.0 and higher and IP-cores with enhanced
> > descriptor support will be able to work with frames up to 8KB (JUMBO_LEN).
> > The rest of the NICs will support frames of SKB_MAX_HEAD(NET_SKB_PAD +
> > NET_IP_ALIGN) size.
> >
> > Fixes: 7d9e6c5afab6 ("net: stmmac: Integrate XGMAC into main driver flow")
> > Signed-off-by: Serge Semin <[email protected]>
> > ---
> > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ----
> > drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 5 -----
> > 2 files changed, 9 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 32aa7953d296..e5cb4edc4e23 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -7252,10 +7252,6 @@ int stmmac_dvr_probe(struct device *device,
> > if ((priv->plat->maxmtu < ndev->max_mtu) &&
> > (priv->plat->maxmtu >= ndev->min_mtu))
> > ndev->max_mtu = priv->plat->maxmtu;
> > - else if (priv->plat->maxmtu < ndev->min_mtu)
> > - dev_warn(priv->device,
> > - "%s: warning: maxmtu having invalid value (%d)\n",
> > - __func__, priv->plat->maxmtu);
>
> Looks fine but by removing plat->maxmtu = JUMBO_LEN; you eliminate the
> case of dev_warn here or you remove dev_warn since the driver will be
> able to fix the mtu value?
That warning is kind of odd if not to say contradicting. Why having
max_mtu being less than the HW-specific min-value is more dangerous
than having it being greater than the max-value for which there is no
warning? The driver gets to the HW-specific min and max MTU values in
anyway. Why to warn in one case and leaving unnoticed another?..
Anyway getting back to this patch change I remove the warning since
plat_stmmaceth_data.maxmtu is now will be initialized with zero most
of the time (if glue-drivers leave it uninitialized or there is no
"max-frame-size" DT-property specified) which will falsely trigger
that warning.
-Serge(y)
> >
> > if (flow_ctrl)
> > priv->flow_ctrl = FLOW_AUTO; /* RX/TX pause on */
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > index 067a40fe0a23..857411105a0a 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > @@ -468,11 +468,6 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
> > plat->en_tx_lpi_clockgating =
> > of_property_read_bool(np, "snps,en-tx-lpi-clockgating");
> >
> > - /* Set the maxmtu to a default of JUMBO_LEN in case the
> > - * parameter is not present in the device tree.
> > - */
> > - plat->maxmtu = JUMBO_LEN;
> > -
> > /* Set default value for multicast hash bins */
> > plat->multicast_filter_bins = HASH_TABLE_SIZE;
> >
> > --
> > 2.39.2
> >
> >