| Message ID | 20241125033446.3290936-1-zhangheng@kylinos.cn (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: Use dma_set_mask_and_coherent to set DMA mask | expand |
On Mon, Nov 25, 2024 at 11:34:46AM +0800, zhangheng wrote: > Many drivers still have two explicit calls of dma_set_mask() and > dma_set_coherent_mask(). > > Let's simplify with dma_set_mask_and_coherent(). Is simplification a sufficient justification? https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#clean-up-patches 1.6.6. Clean-up patches Netdev discourages patches which perform simple clean-ups, which are not in the context of other work. For example: Addressing checkpatch.pl warnings Addressing Local variable ordering issues Conversions to device-managed APIs (devm_ helpers) This is because it is felt that the churn that such changes produce comes at a greater cost than the value of such clean-ups. What is the value of this simplification? What is the likelihood you have actually broken something? The problem with these sorts of patches is that they are often made blindly without understanding the code and a small percentage actually break things. As a result, Maintainers need to look at these patches and spend the time to actually understand them. I would prefer to spend that time on new drivers, rather than old code which works and does not really benefit from simplification. Andrew
diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c index 3f6204de9e6b..ad58e589a723 100644 --- a/drivers/net/ethernet/altera/altera_tse_main.c +++ b/drivers/net/ethernet/altera/altera_tse_main.c @@ -1219,11 +1219,8 @@ static int altera_tse_probe(struct platform_device *pdev) goto err_free_netdev; } - if (!dma_set_mask(priv->device, DMA_BIT_MASK(priv->dmaops->dmamask))) { - dma_set_coherent_mask(priv->device, - DMA_BIT_MASK(priv->dmaops->dmamask)); - } else if (!dma_set_mask(priv->device, DMA_BIT_MASK(32))) { - dma_set_coherent_mask(priv->device, DMA_BIT_MASK(32)); + if (dma_set_mask_and_coherent(priv->device, DMA_BIT_MASK(priv->dmaops->dmamask))) { + dma_set_mask_and_coherent(priv->device, DMA_BIT_MASK(32)); } else { ret = -EIO; goto err_free_netdev; diff --git a/drivers/net/ethernet/atheros/atlx/atl2.c b/drivers/net/ethernet/atheros/atlx/atl2.c index fa9a4919f25d..aa3aba4a59f3 100644 --- a/drivers/net/ethernet/atheros/atlx/atl2.c +++ b/drivers/net/ethernet/atheros/atlx/atl2.c @@ -1329,8 +1329,7 @@ static int atl2_probe(struct pci_dev *pdev, const struct pci_device_id *ent) * until the kernel has the proper infrastructure to support 64-bit DMA * on these devices. */ - if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)) && - dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32))) { + if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32))) { printk(KERN_ERR "atl2: No usable DMA configuration, aborting\n"); err = -EIO; goto err_dma; diff --git a/drivers/net/ethernet/micrel/ksz884x.c b/drivers/net/ethernet/micrel/ksz884x.c index dc1d9f774565..d74c319852a5 100644 --- a/drivers/net/ethernet/micrel/ksz884x.c +++ b/drivers/net/ethernet/micrel/ksz884x.c @@ -6559,8 +6559,7 @@ static int pcidev_init(struct pci_dev *pdev, const struct pci_device_id *id) result = -ENODEV; - if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)) || - dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32))) + if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32))) return result; reg_base = pci_resource_start(pdev, 0); diff --git a/drivers/net/ethernet/rdc/r6040.c b/drivers/net/ethernet/rdc/r6040.c index f4d434c379e7..a3e65ccd48ce 100644 --- a/drivers/net/ethernet/rdc/r6040.c +++ b/drivers/net/ethernet/rdc/r6040.c @@ -1041,12 +1041,7 @@ static int r6040_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) goto err_out; /* this should always be supported */ - err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)); - if (err) { - dev_err(&pdev->dev, "32-bit PCI DMA addresses not supported by the card\n"); - goto err_out_disable_dev; - } - err = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32)); + err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); if (err) { dev_err(&pdev->dev, "32-bit PCI DMA addresses not supported by the card\n"); goto err_out_disable_dev; diff --git a/drivers/net/ethernet/silan/sc92031.c b/drivers/net/ethernet/silan/sc92031.c index ff4197f5e46d..228f2608d288 100644 --- a/drivers/net/ethernet/silan/sc92031.c +++ b/drivers/net/ethernet/silan/sc92031.c @@ -1409,11 +1409,7 @@ static int sc92031_probe(struct pci_dev *pdev, const struct pci_device_id *id) pci_set_master(pdev); - err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)); - if (unlikely(err < 0)) - goto out_set_dma_mask; - - err = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32)); + err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); if (unlikely(err < 0)) goto out_set_dma_mask;
Many drivers still have two explicit calls of dma_set_mask() and dma_set_coherent_mask(). Let's simplify with dma_set_mask_and_coherent(). Signed-off-by: zhangheng <zhangheng@kylinos.cn> --- change for v2 - Update commit message: Add why do this drivers/net/ethernet/altera/altera_tse_main.c | 7 ++----- drivers/net/ethernet/atheros/atlx/atl2.c | 3 +-- drivers/net/ethernet/micrel/ksz884x.c | 3 +-- drivers/net/ethernet/rdc/r6040.c | 7 +------ drivers/net/ethernet/silan/sc92031.c | 6 +----- 5 files changed, 6 insertions(+), 20 deletions(-)