[PATCH V2 0/3] soc: ti: Fix crash in error path when DMA channel open fails

[PATCH V2 0/3] soc: ti: Fix crash in error path when DMA channel open fails

[Nishanth Menon]

Hi,

This is a respin of V1 of the series, to address a crash seen in
kernelci.org automated testing[1].

Changes in V2:
- Took Simon's suggestion in refactoring code to make
  knav_dma_open_channel return NULL and fix the callers accordingly.

Since the series does have inter-dependencies, I suggest the full series
could either go via net tree OR via SoC tree (if net maintainers dont
mind acking it). Personally, I have no issues of getting it via the net
tree.

I dropped the fixes tag here, any better suggestion to hit stable will
be nice to know, since the code is refactored a bit here.

V1: https://lore.kernel.org/all/20250926150853.2907028-1-nm@ti.com/

[1] https://dashboard.kernelci.org/log-viewer?itemId=ti%3A2eb55ed935eb42c292e02f59&org=ti&type=test&url=http%3A%2F%2Ffiles.kernelci.org%2F%2Fti%2Fmainline%2Fmaster%2Fv6.17-rc7-59-gbf40f4b87761%2Farm%2Fmulti_v7_defconfig%2BCONFIG_EFI%3Dy%2BCONFIG_ARM_LPAE%3Dy%2Bdebug%2Bkselftest%2Btinyconfig%2Fgcc-12%2Fbaseline-nfs-boot.nfs-k2hk-evm.txt.gz
Nishanth Menon (3):
  net: ethernet: ti: netcp: Handle both ERR_PTR and NULL from
    knav_dma_open_channel
  soc: ti: knav_dma: Make knav_dma_open_channel return NULL on error
  net: ethernet: ti: Remove IS_ERR_OR_NULL checks for
    knav_dma_open_channel

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

-- 
2.47.0

[PATCH V2 1/3] net: ethernet: ti: netcp: Handle both ERR_PTR and NULL from knav_dma_open_channel

[Nishanth Menon]

The knav_dma_open_channel function has inconsistent return behavior: the
header returns NULL when the driver is disabled, while the driver
implementation returns ERR_PTR(-EINVAL). This inconsistency creates
confusion for callers.

Prepare for standardizing the function to return NULL by updating callers
to handle both ERR_PTR and NULL return values using IS_ERR_OR_NULL()
checks.

Suggested-by: Simon Horman <horms@kernel.org>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
Changes in V2:
* renewed version

 drivers/net/ethernet/ti/netcp_core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index 857820657bac..2f9d26c791e3 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 (IS_ERR_OR_NULL(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;
 	}
 
@@ -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 (IS_ERR_OR_NULL(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;
 	}
 
-- 
2.47.0

[PATCH V2 2/3] soc: ti: knav_dma: Make knav_dma_open_channel return NULL on error

[Nishanth Menon]

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 NUL
when the driver is disabled, but the driver implementation returns
ERR_PTR(-EINVAL), creating API inconsistency for users.

Standardize the error handling by making the function return NULL on all
error conditions.

Suggested-by: Simon Horman <horms@kernel.org>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
Changes in V2:
* renewed version

 drivers/soc/ti/knav_dma.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c
index a25ebe6cd503..e69f0946de29 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.
+ * Returns 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.47.0

[PATCH V2 3/3] net: ethernet: ti: Remove IS_ERR_OR_NULL checks for knav_dma_open_channel

[Nishanth Menon]

knav_dma_open_channel now only returns NULL on failure instead of error
pointers. Replace IS_ERR_OR_NULL checks with simple NULL checks.

Suggested-by: Simon Horman <horms@kernel.org>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
Changes in V2:
* renewed version
* Dropped the fixes since code refactoring was involved.

V1: https://lore.kernel.org/all/20250926150853.2907028-1-nm@ti.com/

 drivers/net/ethernet/ti/netcp_core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index 2f9d26c791e3..5ee13db568f0 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -1338,7 +1338,7 @@ 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_OR_NULL(tx_pipe->dma_channel)) {
+	if (!tx_pipe->dma_channel) {
 		dev_err(dev, "failed opening tx chan(%s)\n",
 			tx_pipe->dma_chan_name);
 		ret = -EINVAL;
@@ -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,7 +1678,7 @@ 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_OR_NULL(netcp->rx_channel)) {
+	if (!netcp->rx_channel) {
 		dev_err(netcp->ndev_dev, "failed opening rx chan(%s\n",
 			netcp->dma_chan_name);
 		ret = -EINVAL;
-- 
2.47.0

Re: [PATCH V2 3/3] net: ethernet: ti: Remove IS_ERR_OR_NULL checks for knav_dma_open_channel

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 2130 bytes --]



On 9/30/2025 5:16 AM, Nishanth Menon wrote:
> knav_dma_open_channel now only returns NULL on failure instead of error
> pointers. Replace IS_ERR_OR_NULL checks with simple NULL checks.
> 
> Suggested-by: Simon Horman <horms@kernel.org>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
> Changes in V2:
> * renewed version
> * Dropped the fixes since code refactoring was involved.
> 

Whats the justification for splitting this apart from patch 1 of 3?

It seems like we ought to just do all this in a single patch. I don't
see the value in splitting this apart into 3 patches, unless someone
else on the list thinks it is valuable.

> V1: https://lore.kernel.org/all/20250926150853.2907028-1-nm@ti.com/
> 
>  drivers/net/ethernet/ti/netcp_core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
> index 2f9d26c791e3..5ee13db568f0 100644
> --- a/drivers/net/ethernet/ti/netcp_core.c
> +++ b/drivers/net/ethernet/ti/netcp_core.c
> @@ -1338,7 +1338,7 @@ 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_OR_NULL(tx_pipe->dma_channel)) {
> +	if (!tx_pipe->dma_channel) {
>  		dev_err(dev, "failed opening tx chan(%s)\n",
>  			tx_pipe->dma_chan_name);
>  		ret = -EINVAL;
> @@ -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,7 +1678,7 @@ 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_OR_NULL(netcp->rx_channel)) {
> +	if (!netcp->rx_channel) {
>  		dev_err(netcp->ndev_dev, "failed opening rx chan(%s\n",
>  			netcp->dma_chan_name);
>  		ret = -EINVAL;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH V2 2/3] soc: ti: knav_dma: Make knav_dma_open_channel return NULL on error

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 1764 bytes --]



On 9/30/2025 5:16 AM, Nishanth Menon wrote:
> 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 NUL
> when the driver is disabled, but the driver implementation returns
> ERR_PTR(-EINVAL), creating API inconsistency for users.
> 

I would word this as indicating that we don't even use ERR_PTR here at all!

> Standardize the error handling by making the function return NULL on all
> error conditions.
> 
> Suggested-by: Simon Horman <horms@kernel.org>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
> Changes in V2:
> * renewed version
> 
>  drivers/soc/ti/knav_dma.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c
> index a25ebe6cd503..e69f0946de29 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.
> + * Returns 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;

Wow. The driver doesn't even return ERR_PTR, but just directly casts
-EINVAL to a void *... thats quite ugly. Good to remove this.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH V2 3/3] net: ethernet: ti: Remove IS_ERR_OR_NULL checks for knav_dma_open_channel

[Nishanth Menon]

On 16:59-20250930, Jacob Keller wrote:
> 
> 
> On 9/30/2025 5:16 AM, Nishanth Menon wrote:
> > knav_dma_open_channel now only returns NULL on failure instead of error
> > pointers. Replace IS_ERR_OR_NULL checks with simple NULL checks.
> > 
> > Suggested-by: Simon Horman <horms@kernel.org>
> > Signed-off-by: Nishanth Menon <nm@ti.com>
> > ---
> > Changes in V2:
> > * renewed version
> > * Dropped the fixes since code refactoring was involved.
> > 
> 
> Whats the justification for splitting this apart from patch 1 of 3?
> 
> It seems like we ought to just do all this in a single patch. I don't
> see the value in splitting this apart into 3 patches, unless someone
> else on the list thinks it is valuable.

The only reason I have done that is to ensure the patches are
bisectable. at patch #1, we are still returning -EINVAL, the driver
should still function when we switch the return over to NULL.

[...]

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D
https://ti.com/opensource

Re: [PATCH V2 2/3] soc: ti: knav_dma: Make knav_dma_open_channel return NULL on error

[Nishanth Menon]

On 16:59-20250930, Jacob Keller wrote:
> 
> 
> On 9/30/2025 5:16 AM, Nishanth Menon wrote:
> > 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 NUL
> > when the driver is disabled, but the driver implementation returns
> > ERR_PTR(-EINVAL), creating API inconsistency for users.
> > 
> 
> I would word this as indicating that we don't even use ERR_PTR here at all!

Thanks. Yep, will do for the next respin.

> 
> > Standardize the error handling by making the function return NULL on all
> > error conditions.
> > 
> > Suggested-by: Simon Horman <horms@kernel.org>
> > Signed-off-by: Nishanth Menon <nm@ti.com>
> > ---
> > Changes in V2:
> > * renewed version
> > 
> >  drivers/soc/ti/knav_dma.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c
> > index a25ebe6cd503..e69f0946de29 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.
> > + * Returns 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;
> 
> Wow. The driver doesn't even return ERR_PTR, but just directly casts
> -EINVAL to a void *... thats quite ugly. Good to remove this.
> 




-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D
https://ti.com/opensource

Re: [PATCH V2 3/3] net: ethernet: ti: Remove IS_ERR_OR_NULL checks for knav_dma_open_channel

[Simon Horman]

On Wed, Oct 01, 2025 at 05:54:16AM -0500, Nishanth Menon wrote:
> On 16:59-20250930, Jacob Keller wrote:
> > 
> > 
> > On 9/30/2025 5:16 AM, Nishanth Menon wrote:
> > > knav_dma_open_channel now only returns NULL on failure instead of error
> > > pointers. Replace IS_ERR_OR_NULL checks with simple NULL checks.
> > > 
> > > Suggested-by: Simon Horman <horms@kernel.org>
> > > Signed-off-by: Nishanth Menon <nm@ti.com>
> > > ---
> > > Changes in V2:
> > > * renewed version
> > > * Dropped the fixes since code refactoring was involved.
> > > 
> > 
> > Whats the justification for splitting this apart from patch 1 of 3?
> > 
> > It seems like we ought to just do all this in a single patch. I don't
> > see the value in splitting this apart into 3 patches, unless someone
> > else on the list thinks it is valuable.
> 
> The only reason I have done that is to ensure the patches are
> bisectable. at patch #1, we are still returning -EINVAL, the driver
> should still function when we switch the return over to NULL.

Maybe we can simplify things and squash all three patches into one.
They seem inter-related.

Re: [PATCH V2 3/3] net: ethernet: ti: Remove IS_ERR_OR_NULL checks for knav_dma_open_channel

[Nishanth Menon]

On 16:58-20251001, Simon Horman wrote:
> On Wed, Oct 01, 2025 at 05:54:16AM -0500, Nishanth Menon wrote:
> > On 16:59-20250930, Jacob Keller wrote:
> > > 
> > > 
> > > On 9/30/2025 5:16 AM, Nishanth Menon wrote:
> > > > knav_dma_open_channel now only returns NULL on failure instead of error
> > > > pointers. Replace IS_ERR_OR_NULL checks with simple NULL checks.
> > > > 
> > > > Suggested-by: Simon Horman <horms@kernel.org>
> > > > Signed-off-by: Nishanth Menon <nm@ti.com>
> > > > ---
> > > > Changes in V2:
> > > > * renewed version
> > > > * Dropped the fixes since code refactoring was involved.
> > > > 
> > > 
> > > Whats the justification for splitting this apart from patch 1 of 3?
> > > 
> > > It seems like we ought to just do all this in a single patch. I don't
> > > see the value in splitting this apart into 3 patches, unless someone
> > > else on the list thinks it is valuable.
> > 
> > The only reason I have done that is to ensure the patches are
> > bisectable. at patch #1, we are still returning -EINVAL, the driver
> > should still function when we switch the return over to NULL.
> 
> Maybe we can simplify things and squash all three patches into one.
> They seem inter-related.

I have no issues as the SoC driver maintainer.. just need direction on
logistics: I will need either the network maintainers to agree to take
it in OR with their ack, I can queue it up.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D
https://ti.com/opensource

Re: [PATCH V2 3/3] net: ethernet: ti: Remove IS_ERR_OR_NULL checks for knav_dma_open_channel

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 1736 bytes --]



On 10/1/2025 9:58 AM, Nishanth Menon wrote:
> On 16:58-20251001, Simon Horman wrote:
>> On Wed, Oct 01, 2025 at 05:54:16AM -0500, Nishanth Menon wrote:
>>> On 16:59-20250930, Jacob Keller wrote:
>>>>
>>>>
>>>> On 9/30/2025 5:16 AM, Nishanth Menon wrote:
>>>>> knav_dma_open_channel now only returns NULL on failure instead of error
>>>>> pointers. Replace IS_ERR_OR_NULL checks with simple NULL checks.
>>>>>
>>>>> Suggested-by: Simon Horman <horms@kernel.org>
>>>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>>>> ---
>>>>> Changes in V2:
>>>>> * renewed version
>>>>> * Dropped the fixes since code refactoring was involved.
>>>>>
>>>>
>>>> Whats the justification for splitting this apart from patch 1 of 3?
>>>>
>>>> It seems like we ought to just do all this in a single patch. I don't
>>>> see the value in splitting this apart into 3 patches, unless someone
>>>> else on the list thinks it is valuable.
>>>
>>> The only reason I have done that is to ensure the patches are
>>> bisectable. at patch #1, we are still returning -EINVAL, the driver
>>> should still function when we switch the return over to NULL.
>>
>> Maybe we can simplify things and squash all three patches into one.
>> They seem inter-related.
> 
> I have no issues as the SoC driver maintainer.. just need direction on
> logistics: I will need either the network maintainers to agree to take
> it in OR with their ack, I can queue it up.
> 

I think it makes the most sense to squash everything together into one
patch.

The change looks small enough to me that I don't think it would cause
much conflict regardless of which tree it goes through. Hopefully one of
the maintainers can chime in their opinion here?

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]