| Message ID | 5b29881f-a11a-4230-a044-a60871d3d38c@kili.mountain (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] dmaengine: ti: k3-udma-glue: fix k3_udma_glue_tx_get_irq() error checking | expand |
On Wed, Sep 13, 2023 at 02:05:31PM +0300, Dan Carpenter wrote: > The problem is that xudma_pktdma_tflow_get_irq() returns zero on error > and k3_ringacc_get_ring_irq_num() returns negatives. This complicates > the error handling. Change it to always return negative errors. > > Both callers have other bugs as well. The am65_cpsw_nuss_init_tx_chns() > function doesn't preserve the error code but instead returns success. > In prueth_init_tx_chns() there is a signedness bug since "tx_chn->irq" > is unsigned so negative errors are not handled correctly. Hi Dan, I understand that the problems are related, but there are several of them. Could they be handled in separate patches (applied in a specific order) ? I suspect this would aid backporting, and, moreover, I think it is nice to try to work on a one-fix-per-patch basis. The above notwithstanding, I do agree with the correctness of your changes. > Fixes: 93a76530316a ("net: ethernet: ti: introduce am65x/j721e gigabit eth subsystem driver") > Fixes: 5b65781d06ea ("dmaengine: ti: k3-udma-glue: Add support for K3 PKTDMA") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > The k3_udma_glue_tx_get_irq() function is in dmaengine, but this bug > affects networking so I think the fix should go through networking. ...
On Fri, Sep 15, 2023 at 08:33:24AM +0200, Simon Horman wrote: > On Wed, Sep 13, 2023 at 02:05:31PM +0300, Dan Carpenter wrote: > > The problem is that xudma_pktdma_tflow_get_irq() returns zero on error > > and k3_ringacc_get_ring_irq_num() returns negatives. This complicates > > the error handling. Change it to always return negative errors. > > > > Both callers have other bugs as well. The am65_cpsw_nuss_init_tx_chns() > > function doesn't preserve the error code but instead returns success. > > In prueth_init_tx_chns() there is a signedness bug since "tx_chn->irq" > > is unsigned so negative errors are not handled correctly. > > Hi Dan, > > I understand that the problems are related, but there are several of them. > Could they be handled in separate patches (applied in a specific order) ? > I suspect this would aid backporting, and, moreover, I think it is nice > to try to work on a one-fix-per-patch basis. > > The above notwithstanding, I do agree with the correctness of your changes. > Sure. Let me write it like: patch 1: fix first caller patch 2: fix second caller patch 3: re-write both callers to cleaner API And we can push everything to net because it's nice to have one version of the API instead of a version for net and a different version in net-next. Or we could apply patch 3 to only net-next. regards, dan carpenter
diff --git a/drivers/dma/ti/k3-udma-glue.c b/drivers/dma/ti/k3-udma-glue.c index 789193ed0386..c278d5facf7d 100644 --- a/drivers/dma/ti/k3-udma-glue.c +++ b/drivers/dma/ti/k3-udma-glue.c @@ -558,6 +558,9 @@ int k3_udma_glue_tx_get_irq(struct k3_udma_glue_tx_channel *tx_chn) tx_chn->virq = k3_ringacc_get_ring_irq_num(tx_chn->ringtxcq); } + if (!tx_chn->virq) + return -ENXIO; + return tx_chn->virq; } EXPORT_SYMBOL_GPL(k3_udma_glue_tx_get_irq); diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c index bea6fc0f324c..24120605502f 100644 --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c @@ -1747,9 +1747,10 @@ static int am65_cpsw_nuss_init_tx_chns(struct am65_cpsw_common *common) } tx_chn->irq = k3_udma_glue_tx_get_irq(tx_chn->tx_chn); - if (tx_chn->irq <= 0) { + if (tx_chn->irq < 0) { dev_err(dev, "Failed to get tx dma irq %d\n", tx_chn->irq); + ret = tx_chn->irq; goto err; } diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c index 47b941fb0198..98b061a3f3c1 100644 --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c @@ -316,12 +316,12 @@ static int prueth_init_tx_chns(struct prueth_emac *emac) goto fail; } - tx_chn->irq = k3_udma_glue_tx_get_irq(tx_chn->tx_chn); - if (tx_chn->irq <= 0) { - ret = -EINVAL; + ret = k3_udma_glue_tx_get_irq(tx_chn->tx_chn); + if (ret < 0) { netdev_err(ndev, "failed to get tx irq\n"); goto fail; } + tx_chn->irq = ret; snprintf(tx_chn->name, sizeof(tx_chn->name), "%s-tx%d", dev_name(dev), tx_chn->id);
The problem is that xudma_pktdma_tflow_get_irq() returns zero on error and k3_ringacc_get_ring_irq_num() returns negatives. This complicates the error handling. Change it to always return negative errors. Both callers have other bugs as well. The am65_cpsw_nuss_init_tx_chns() function doesn't preserve the error code but instead returns success. In prueth_init_tx_chns() there is a signedness bug since "tx_chn->irq" is unsigned so negative errors are not handled correctly. Fixes: 93a76530316a ("net: ethernet: ti: introduce am65x/j721e gigabit eth subsystem driver") Fixes: 5b65781d06ea ("dmaengine: ti: k3-udma-glue: Add support for K3 PKTDMA") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- The k3_udma_glue_tx_get_irq() function is in dmaengine, but this bug affects networking so I think the fix should go through networking. drivers/dma/ti/k3-udma-glue.c | 3 +++ drivers/net/ethernet/ti/am65-cpsw-nuss.c | 3 ++- drivers/net/ethernet/ti/icssg/icssg_prueth.c | 6 +++--- 3 files changed, 8 insertions(+), 4 deletions(-)