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