[PATCH AUTOSEL 6.17-5.4] net: ethernet: ti: netcp: Standardize knav_dma_open_channel to return NULL on error

[Sasha Levin <sashal@kernel.org>]

From: Nishanth Menon <nm@ti.com>

[ Upstream commit 90a88306eb874fe4bbdd860e6c9787f5bbc588b5 ]

Make knav_dma_open_channel consistently return NULL on error instead
of ERR_PTR. Currently the header include/linux/soc/ti/knav_dma.h
returns NULL when the driver is disabled, but the driver
implementation does not even return NULL or ERR_PTR on failure,
causing inconsistency in the users. This results in a crash in
netcp_free_navigator_resources as followed (trimmed):

Unhandled fault: alignment exception (0x221) at 0xfffffff2
[fffffff2] *pgd=80000800207003, *pmd=82ffda003, *pte=00000000
Internal error: : 221 [#1] SMP ARM
Modules linked in:
CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.17.0-rc7 #1 NONE
Hardware name: Keystone
PC is at knav_dma_close_channel+0x30/0x19c
LR is at netcp_free_navigator_resources+0x2c/0x28c

[... TRIM...]

Call trace:
 knav_dma_close_channel from netcp_free_navigator_resources+0x2c/0x28c
 netcp_free_navigator_resources from netcp_ndo_open+0x430/0x46c
 netcp_ndo_open from __dev_open+0x114/0x29c
 __dev_open from __dev_change_flags+0x190/0x208
 __dev_change_flags from netif_change_flags+0x1c/0x58
 netif_change_flags from dev_change_flags+0x38/0xa0
 dev_change_flags from ip_auto_config+0x2c4/0x11f0
 ip_auto_config from do_one_initcall+0x58/0x200
 do_one_initcall from kernel_init_freeable+0x1cc/0x238
 kernel_init_freeable from kernel_init+0x1c/0x12c
 kernel_init from ret_from_fork+0x14/0x38
[... TRIM...]

Standardize the error handling by making the function return NULL on
all error conditions. The API is used in just the netcp_core.c so the
impact is limited.

Note, this change, in effect reverts commit 5b6cb43b4d62 ("net:
ethernet: ti: netcp_core: return error while dma channel open issue"),
but provides a less error prone implementation.

Suggested-by: Simon Horman <horms@kernel.org>
Suggested-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Link: https://patch.msgid.link/20251103162811.3730055-1-nm@ti.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

Based on my comprehensive analysis using semantic code analysis tools
and kernel repository examination, here is my determination:

## **YES** - This commit SHOULD be backported to stable kernel trees

---

## Analysis Summary

### Semantic Analysis Tools Used:

1. **mcp__semcode__find_function** - Located all affected functions
2. **mcp__semcode__find_callers** - Identified call sites and impact
   scope
3. **mcp__semcode__find_callchain** - Traced execution paths from user-
   space
4. **Read** - Examined header file and code structure
5. **Grep** - Verified net_device_ops assignment
6. **git log/show** - Investigated bug history and kernel version
   presence

### Key Findings:

#### 1. **User-Space Triggerable Crash** (CRITICAL)
- **Impact**: The bug causes a kernel crash (alignment exception at
  0xfffffff2) when opening a network interface
- **Trigger path**: User command (`ifconfig eth0 up` or `ip link set
  eth0 up`) → `__dev_open()` → `netcp_ndo_open()`
  (drivers/net/ethernet/ti/netcp_core.c:1942) →
  `netcp_setup_navigator_resources()` → `knav_dma_open_channel()`
- **Call graph analysis** shows `netcp_ndo_open` is assigned to
  `.ndo_open` in `netcp_netdev_ops` structure, making it directly user-
  triggerable

#### 2. **Root Cause Analysis**
The crash occurs due to inconsistent error handling:
- **Header stub** (include/linux/soc/ti/knav_dma.h:168): Returns `NULL`
  when driver disabled
- **Driver implementation** (drivers/soc/ti/knav_dma.c:407-487): Returns
  `(void *)-EINVAL` (ERR_PTR) on errors
- **Callers** check `IS_ERR()` after calling `knav_dma_open_channel()`
- **Cleanup code** in `netcp_free_navigator_resources()` (line 1548)
  only checks `if (netcp->rx_channel)` before calling
  `knav_dma_close_channel()`
- When `rx_channel` contains `-EINVAL` (0xfffffff2), it's non-NULL, so
  the check passes
- `knav_dma_close_channel()` attempts to dereference this invalid
  pointer → alignment exception crash

#### 3. **Impact Scope** (from mcp__semcode__find_callers)
- **Limited scope**: Only 2 callers of `knav_dma_open_channel()`:
  - `netcp_setup_navigator_resources()`
    (drivers/net/ethernet/ti/netcp_core.c:1582)
  - `netcp_txpipe_open()` (drivers/net/ethernet/ti/netcp_core.c:1326)
- **Affected hardware**: TI Keystone SoC users with NetCP driver
- **Files changed**: Only 2 files, both in the same subsystem

#### 4. **Bug Longevity**
- **Introduced**: v4.12 (2017) by commit 5b6cb43b4d625
- **Duration**: ~7-8 years of existence
- **Fixed in**: v6.18-rc5 (November 2024)
- Verified the buggy pattern exists in v6.6 LTS kernel

#### 5. **Change Characteristics**
- **Type**: Pure bug fix (crash fix)
- **Size**: Small and contained
  (drivers/net/ethernet/ti/netcp_core.c:1326-1366
  netcp_core.c:1582-1694, drivers/soc/ti/knav_dma.c:407-487)
- **Semantic changes** (from code inspection):
  - Standardizes return value from ERR_PTR to NULL on all error paths
  - Updates callers from `IS_ERR()` checks to simple NULL checks
  - Updates cleanup from `IS_ERR_OR_NULL()` to simple NULL checks
  - Changes error code from `PTR_ERR()` extraction to fixed `-EINVAL`
- **No architectural changes**: No struct modifications, no API
  additions
- **No new features**: Only error handling standardization

#### 6. **Backport Risk Assessment**
- **Risk level**: LOW
  - Changes are localized to error handling paths
  - Makes behavior consistent with header stub
  - No complex dependencies identified
  - Code structure in v6.6 is compatible (verified)
- **Regression potential**: Minimal
  - Affects only error paths that were already broken
  - Improves consistency between header and implementation
  - All callers updated in same commit

#### 7. **Stable Tree Compliance**
- ✅ **Fixes important bug**: Kernel crash
- ✅ **Small and contained**: 2 files, error handling only
- ✅ **No new features**: Pure bug fix
- ✅ **No architectural changes**: No struct/API changes
- ✅ **Obvious and correct**: Crash trace in commit message proves the
  fix
- ✅ **Self-contained**: No dependencies on other commits
- ❌ **Missing stable tags**: No "Cc: stable@vger.kernel.org" or "Fixes:"
  tag (oversight)

### Specific Code Reference Points:

1. **drivers/soc/ti/knav_dma.c:407-487**: Changed all error returns from
   `(void *)-EINVAL` to `NULL`
2. **drivers/net/ethernet/ti/netcp_core.c:1339**: Changed from
   `IS_ERR(tx_pipe->dma_channel)` to `!tx_pipe->dma_channel`
3. **drivers/net/ethernet/ti/netcp_core.c:1361**: Changed from
   `IS_ERR_OR_NULL()` to simple NULL check in cleanup
4. **drivers/net/ethernet/ti/netcp_core.c:1680**: Changed from
   `IS_ERR(netcp->rx_channel)` to `!netcp->rx_channel`

### Conclusion:

This commit is an **excellent candidate for stable backporting**:
- Fixes a real, user-triggerable kernel crash with documented stack
  trace
- Affects users of TI Keystone SoCs who configure network interfaces
- Small, well-contained, low-risk change
- Bug has existed since v4.12 (2017) affecting all stable trees
- Should be backported to all active stable kernels (at minimum v6.1+,
  v6.6+, v6.12+)

The absence of stable/Fixes tags appears to be an oversight and should
not prevent backporting given the clear evidence of crash and the
straightforward nature of the fix.

 drivers/net/ethernet/ti/netcp_core.c | 10 +++++-----
 drivers/soc/ti/knav_dma.c            | 14 +++++++-------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index 857820657bac5..5ee13db568f08 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -1338,10 +1338,10 @@ int netcp_txpipe_open(struct netcp_tx_pipe *tx_pipe)
 
 	tx_pipe->dma_channel = knav_dma_open_channel(dev,
 				tx_pipe->dma_chan_name, &config);
-	if (IS_ERR(tx_pipe->dma_channel)) {
+	if (!tx_pipe->dma_channel) {
 		dev_err(dev, "failed opening tx chan(%s)\n",
 			tx_pipe->dma_chan_name);
-		ret = PTR_ERR(tx_pipe->dma_channel);
+		ret = -EINVAL;
 		goto err;
 	}
 
@@ -1359,7 +1359,7 @@ int netcp_txpipe_open(struct netcp_tx_pipe *tx_pipe)
 	return 0;
 
 err:
-	if (!IS_ERR_OR_NULL(tx_pipe->dma_channel))
+	if (tx_pipe->dma_channel)
 		knav_dma_close_channel(tx_pipe->dma_channel);
 	tx_pipe->dma_channel = NULL;
 	return ret;
@@ -1678,10 +1678,10 @@ static int netcp_setup_navigator_resources(struct net_device *ndev)
 
 	netcp->rx_channel = knav_dma_open_channel(netcp->netcp_device->device,
 					netcp->dma_chan_name, &config);
-	if (IS_ERR(netcp->rx_channel)) {
+	if (!netcp->rx_channel) {
 		dev_err(netcp->ndev_dev, "failed opening rx chan(%s\n",
 			netcp->dma_chan_name);
-		ret = PTR_ERR(netcp->rx_channel);
+		ret = -EINVAL;
 		goto fail;
 	}
 
diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c
index a25ebe6cd5030..553ae7ee20f16 100644
--- a/drivers/soc/ti/knav_dma.c
+++ b/drivers/soc/ti/knav_dma.c
@@ -402,7 +402,7 @@ static int of_channel_match_helper(struct device_node *np, const char *name,
  * @name:	slave channel name
  * @config:	dma configuration parameters
  *
- * Returns pointer to appropriate DMA channel on success or error.
+ * Return: Pointer to appropriate DMA channel on success or NULL on error.
  */
 void *knav_dma_open_channel(struct device *dev, const char *name,
 					struct knav_dma_cfg *config)
@@ -414,13 +414,13 @@ void *knav_dma_open_channel(struct device *dev, const char *name,
 
 	if (!kdev) {
 		pr_err("keystone-navigator-dma driver not registered\n");
-		return (void *)-EINVAL;
+		return NULL;
 	}
 
 	chan_num = of_channel_match_helper(dev->of_node, name, &instance);
 	if (chan_num < 0) {
 		dev_err(kdev->dev, "No DMA instance with name %s\n", name);
-		return (void *)-EINVAL;
+		return NULL;
 	}
 
 	dev_dbg(kdev->dev, "initializing %s channel %d from DMA %s\n",
@@ -431,7 +431,7 @@ void *knav_dma_open_channel(struct device *dev, const char *name,
 	if (config->direction != DMA_MEM_TO_DEV &&
 	    config->direction != DMA_DEV_TO_MEM) {
 		dev_err(kdev->dev, "bad direction\n");
-		return (void *)-EINVAL;
+		return NULL;
 	}
 
 	/* Look for correct dma instance */
@@ -443,7 +443,7 @@ void *knav_dma_open_channel(struct device *dev, const char *name,
 	}
 	if (!dma) {
 		dev_err(kdev->dev, "No DMA instance with name %s\n", instance);
-		return (void *)-EINVAL;
+		return NULL;
 	}
 
 	/* Look for correct dma channel from dma instance */
@@ -463,14 +463,14 @@ void *knav_dma_open_channel(struct device *dev, const char *name,
 	if (!chan) {
 		dev_err(kdev->dev, "channel %d is not in DMA %s\n",
 				chan_num, instance);
-		return (void *)-EINVAL;
+		return NULL;
 	}
 
 	if (atomic_read(&chan->ref_count) >= 1) {
 		if (!check_config(chan, config)) {
 			dev_err(kdev->dev, "channel %d config miss-match\n",
 				chan_num);
-			return (void *)-EINVAL;
+			return NULL;
 		}
 	}
 
-- 
2.51.0