Re: [PATCH] misc: pci_endpoint_test: Remove superfluous code

[Niklas Cassel <cassel@kernel.org>]

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