* [PATCH] misc: pci_endpoint_test: Remove superfluous code @ 2024-03-25 14:23 Niklas Cassel 2024-03-26 18:56 ` Frank Li 0 siblings, 1 reply; 4+ messages in thread From: Niklas Cassel @ 2024-03-25 14:23 UTC (permalink / raw) To: Manivannan Sadhasivam, Krzysztof Wilczyński, Kishon Vijay Abraham I, Arnd Bergmann, Greg Kroah-Hartman Cc: Niklas Cassel, linux-pci There are two things that made me read this code multiple times: 1) There is a parenthesis around the first conditional, but not around the second conditional. This is inconsistent, and makes you assume that the return value should be treated differently. 2) There is no need to explicitly write != 0 in a conditional. Remove the superfluous parenthesis and != 0. No functional change intended. Signed-off-by: Niklas Cassel <cassel@kernel.org> --- drivers/misc/pci_endpoint_test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index bf64d3aff7d8..1005dfdf664e 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -854,8 +854,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, init_completion(&test->irq_raised); mutex_init(&test->mutex); - if ((dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48)) != 0) && - dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0) { + if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48)) && + dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32))) { dev_err(dev, "Cannot set DMA mask\n"); return -EINVAL; } -- 2.44.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] misc: pci_endpoint_test: Remove superfluous code 2024-03-25 14:23 [PATCH] misc: pci_endpoint_test: Remove superfluous code Niklas Cassel @ 2024-03-26 18:56 ` Frank Li 2024-03-27 16:10 ` Niklas Cassel 0 siblings, 1 reply; 4+ messages in thread From: Frank Li @ 2024-03-26 18:56 UTC (permalink / raw) To: Niklas Cassel Cc: Manivannan Sadhasivam, Krzysztof Wilczyński, Kishon Vijay Abraham I, Arnd Bergmann, Greg Kroah-Hartman, linux-pci On Mon, Mar 25, 2024 at 03:23:55PM +0100, Niklas Cassel wrote: > There are two things that made me read this code multiple times: > > 1) There is a parenthesis around the first conditional, but not > around the second conditional. This is inconsistent, and makes > you assume that the return value should be treated differently. > > 2) There is no need to explicitly write != 0 in a conditional. > > Remove the superfluous parenthesis and != 0. > > No functional change intended. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > drivers/misc/pci_endpoint_test.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > index bf64d3aff7d8..1005dfdf664e 100644 > --- a/drivers/misc/pci_endpoint_test.c > +++ b/drivers/misc/pci_endpoint_test.c > @@ -854,8 +854,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, > init_completion(&test->irq_raised); > mutex_init(&test->mutex); > > - if ((dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48)) != 0) && > - dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0) { > + if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48)) && > + dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32))) { Actaully above orginal code is wrong. If dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48)) failure, dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) must be failure. Needn't retry 32bit. https://lore.kernel.org/linux-pci/925da248-f582-6dd5-57ab-0fadc4e84f9e@wanadoo.fr/ I am also strange where 48 come from. It should be EP side access windows capiblity. Idealy, it should read from BAR0 or use device id to decide dma mask. If EP side only support 32bit dma space. dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48) still return success because host side may support more than 48bit DMA cap. endpoint_test will set > 4G address to EP side. EP actaully can't access it. Most dwc ep controller it should be 64. //64bit mask never return failure. dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); if (drvdata.flags & 32_BIT_ONLY) if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) err_code.. Or if (dma_set_mask_and_coherent(&pdev->dev, drvdata.dmamask)) err_code; Frank > dev_err(dev, "Cannot set DMA mask\n"); > return -EINVAL; > } > -- > 2.44.0 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] misc: pci_endpoint_test: Remove superfluous code 2024-03-26 18:56 ` Frank Li @ 2024-03-27 16:10 ` Niklas Cassel 2024-03-27 18:57 ` Frank Li 0 siblings, 1 reply; 4+ messages in thread From: Niklas Cassel @ 2024-03-27 16:10 UTC (permalink / raw) To: Frank Li Cc: Manivannan Sadhasivam, Krzysztof Wilczyński, Kishon Vijay Abraham I, Arnd Bergmann, Greg Kroah-Hartman, linux-pci On Tue, Mar 26, 2024 at 02:56:01PM -0400, Frank Li wrote: > On Mon, Mar 25, 2024 at 03:23:55PM +0100, Niklas Cassel wrote: > > There are two things that made me read this code multiple times: > > > > 1) There is a parenthesis around the first conditional, but not > > around the second conditional. This is inconsistent, and makes > > you assume that the return value should be treated differently. > > > > 2) There is no need to explicitly write != 0 in a conditional. > > > > Remove the superfluous parenthesis and != 0. > > > > No functional change intended. > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > --- > > drivers/misc/pci_endpoint_test.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > > index bf64d3aff7d8..1005dfdf664e 100644 > > --- a/drivers/misc/pci_endpoint_test.c > > +++ b/drivers/misc/pci_endpoint_test.c > > @@ -854,8 +854,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, > > init_completion(&test->irq_raised); > > mutex_init(&test->mutex); > > > > - if ((dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48)) != 0) && > > - dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0) { > > + if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48)) && > > + dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32))) { > > Actaully above orginal code is wrong. If > dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48)) failure, > dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) must be failure. > Needn't retry 32bit. > > https://lore.kernel.org/linux-pci/925da248-f582-6dd5-57ab-0fadc4e84f9e@wanadoo.fr/ To be honest, I do not really understand how this works, and I don't want to spend time reading the DMA-API code to understand why it can't fail. Feel free to send a patch that you think is better than the one in $subject. (No need to give me any credit.) > > I am also strange where 48 come from. It should be EP side access windows > capiblity. Idealy, it should read from BAR0 or use device id to decide > dma mask. If EP side only support 32bit dma space. Yes, I agree that it depends on the EP's capability. (and I also wonder where 48 came from :) ) Namely the outbound iATUs on the EP side. (and the eDMA's capability on the EP side). At least the iATU in DWC controller can map a 64-bit address target address. (Regardless if the EP has configured its BARs as 32-bit or 64-bit). However, this feels like a bigger patch than just fixing some stylistic changes. If you feel like you want to tacle this problem, feel free to send a patch series. (It is outside the scope of what I wanted to fix, which is to just make the existing code more readable.) Kind regards, Niklas > > dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48) still return > success because host side may support more than 48bit DMA cap. > > endpoint_test will set > 4G address to EP side. EP actaully can't access > it. > > Most dwc ep controller it should be 64. > > //64bit mask never return failure. > dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); > > if (drvdata.flags & 32_BIT_ONLY) > if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) > err_code.. > > Or > > if (dma_set_mask_and_coherent(&pdev->dev, drvdata.dmamask)) > err_code; > > Frank > > > dev_err(dev, "Cannot set DMA mask\n"); > > return -EINVAL; > > } > > -- > > 2.44.0 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] misc: pci_endpoint_test: Remove superfluous code 2024-03-27 16:10 ` Niklas Cassel @ 2024-03-27 18:57 ` Frank Li 0 siblings, 0 replies; 4+ messages in thread From: Frank Li @ 2024-03-27 18:57 UTC (permalink / raw) To: Niklas Cassel Cc: Manivannan Sadhasivam, Krzysztof Wilczyński, Kishon Vijay Abraham I, Arnd Bergmann, Greg Kroah-Hartman, linux-pci On Wed, Mar 27, 2024 at 05:10:32PM +0100, Niklas Cassel wrote: > On Tue, Mar 26, 2024 at 02:56:01PM -0400, Frank Li wrote: > > On Mon, Mar 25, 2024 at 03:23:55PM +0100, Niklas Cassel wrote: > > > There are two things that made me read this code multiple times: > > > > > > 1) There is a parenthesis around the first conditional, but not > > > around the second conditional. This is inconsistent, and makes > > > you assume that the return value should be treated differently. > > > > > > 2) There is no need to explicitly write != 0 in a conditional. > > > > > > Remove the superfluous parenthesis and != 0. > > > > > > No functional change intended. > > > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > > --- > > > drivers/misc/pci_endpoint_test.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > > > index bf64d3aff7d8..1005dfdf664e 100644 > > > --- a/drivers/misc/pci_endpoint_test.c > > > +++ b/drivers/misc/pci_endpoint_test.c > > > @@ -854,8 +854,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, > > > init_completion(&test->irq_raised); > > > mutex_init(&test->mutex); > > > > > > - if ((dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48)) != 0) && > > > - dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) != 0) { > > > + if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48)) && > > > + dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32))) { > > > > Actaully above orginal code is wrong. If > > dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48)) failure, > > dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) must be failure. > > Needn't retry 32bit. > > > > https://lore.kernel.org/linux-pci/925da248-f582-6dd5-57ab-0fadc4e84f9e@wanadoo.fr/ > > To be honest, I do not really understand how this works, > and I don't want to spend time reading the DMA-API code > to understand why it can't fail. > > Feel free to send a patch that you think is better than > the one in $subject. (No need to give me any credit.) > > > > > > I am also strange where 48 come from. It should be EP side access windows > > capiblity. Idealy, it should read from BAR0 or use device id to decide > > dma mask. If EP side only support 32bit dma space. > > Yes, I agree that it depends on the EP's capability. > (and I also wonder where 48 came from :) ) > > Namely the outbound iATUs on the EP side. > (and the eDMA's capability on the EP side). > > At least the iATU in DWC controller can map a 64-bit address target address. > (Regardless if the EP has configured its BARs as 32-bit or 64-bit). > > However, this feels like a bigger patch than just fixing some > stylistic changes. > It doesn't make sense to fix wrong code's stylistic instead of fix the real problem. Frank > If you feel like you want to tacle this problem, feel free to send > a patch series. (It is outside the scope of what I wanted to fix, > which is to just make the existing code more readable.) > > > Kind regards, > Niklas > > > > > dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48) still return > > success because host side may support more than 48bit DMA cap. > > > > endpoint_test will set > 4G address to EP side. EP actaully can't access > > it. > > > > Most dwc ep controller it should be 64. > > > > //64bit mask never return failure. > > dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); > > > > if (drvdata.flags & 32_BIT_ONLY) > > if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)) > > err_code.. > > > > Or > > > > if (dma_set_mask_and_coherent(&pdev->dev, drvdata.dmamask)) > > err_code; > > > > Frank > > > > > dev_err(dev, "Cannot set DMA mask\n"); > > > return -EINVAL; > > > } > > > -- > > > 2.44.0 > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-03-27 18:57 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-25 14:23 [PATCH] misc: pci_endpoint_test: Remove superfluous code Niklas Cassel 2024-03-26 18:56 ` Frank Li 2024-03-27 16:10 ` Niklas Cassel 2024-03-27 18:57 ` Frank Li
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox