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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx/
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
> >
