* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.