diff mbox series

misc: pci_endpoint_test: Remove superfluous code

Message ID 20240325142356.731039-1-cassel@kernel.org (mailing list archive)
State Superseded
Headers show
Series misc: pci_endpoint_test: Remove superfluous code | expand

Commit Message

Niklas Cassel March 25, 2024, 2:23 p.m. UTC 
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(-)

Comments

Frank Li March 26, 2024, 6:56 p.m. UTC | #1 
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
>
Niklas Cassel March 27, 2024, 4:10 p.m. UTC | #2 
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
> >
Frank Li March 27, 2024, 6:57 p.m. UTC | #3 
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
> > >
diff mbox series

Patch

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;
 	}