| Message ID | 74fe0b014334a5ac1db90dec46bdbf6c30c07895.1439002338.git.dhdang@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 07, 2015 at 08:18:48PM -0700, Duc Dang wrote: > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > index 890ad9d..5d03f8b 100644 > --- a/drivers/usb/host/xhci-plat.c > +++ b/drivers/usb/host/xhci-plat.c > @@ -93,14 +93,14 @@ static int xhci_plat_probe(struct platform_device *pdev) > if (irq < 0) > return -ENODEV; > > - /* Initialize dma_mask and coherent_dma_mask to 32-bits */ > - ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32)); > - if (ret) > - return ret; > - if (!pdev->dev.dma_mask) > - pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; > - else > - dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)); > + /* Try setting the coherent_dma_mask to 64 bits, then try 32 bits */ > + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); > + if (ret) { > + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); > + if (ret) > + return ret; > + } Note that dma_set_mask_and_coherent() and the original code are not equivalent because of this: if (!pdev->dev.dma_mask) pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; If we know that pdev->dev.dma_mask will always be initialised at this point, then the above change is fine. If not, it's introducing a regression - dma_set_mask_and_coherent() will fail if pdev->dev.dma_mask is NULL (depending on the architectures implementation of dma_set_mask()). Prefixing the above change with the two lines I mention above would ensure equivalent behaviour. Even if we do want to get rid of this, I'd advise to do it as a separate patch after this change, which can be independently reverted if there's problems with its removal.
On Sat, Aug 8, 2015 at 2:22 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Fri, Aug 07, 2015 at 08:18:48PM -0700, Duc Dang wrote: >> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c >> index 890ad9d..5d03f8b 100644 >> --- a/drivers/usb/host/xhci-plat.c >> +++ b/drivers/usb/host/xhci-plat.c >> @@ -93,14 +93,14 @@ static int xhci_plat_probe(struct platform_device *pdev) >> if (irq < 0) >> return -ENODEV; >> >> - /* Initialize dma_mask and coherent_dma_mask to 32-bits */ >> - ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32)); >> - if (ret) >> - return ret; >> - if (!pdev->dev.dma_mask) >> - pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; >> - else >> - dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)); >> + /* Try setting the coherent_dma_mask to 64 bits, then try 32 bits */ >> + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); >> + if (ret) { >> + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); >> + if (ret) >> + return ret; >> + } > > Note that dma_set_mask_and_coherent() and the original code are not > equivalent because of this: > > if (!pdev->dev.dma_mask) > pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; > > If we know that pdev->dev.dma_mask will always be initialised at this > point, then the above change is fine. If not, it's introducing a > regression - dma_set_mask_and_coherent() will fail if pdev->dev.dma_mask > is NULL (depending on the architectures implementation of dma_set_mask()). > > Prefixing the above change with the two lines I mention above would > ensure equivalent behaviour. Even if we do want to get rid of this, > I'd advise to do it as a separate patch after this change, which can > be independently reverted if there's problems with its removal. > Hi Russell, I will add the 2 lines you mentioned back to next version of the patch. It is safer to do it that way as I do not see pdev->dev.dma_mask gets initialized before the call dma_set_mask_and_coherent inside this xhci_plat.c file. > -- > FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up > according to speedtest.net.
On Saturday 08 August 2015 13:31:02 Duc Dang wrote: > > > > If we know that pdev->dev.dma_mask will always be initialised at this > > point, then the above change is fine. If not, it's introducing a > > regression - dma_set_mask_and_coherent() will fail if pdev->dev.dma_mask > > is NULL (depending on the architectures implementation of dma_set_mask()). > > > > Prefixing the above change with the two lines I mention above would > > ensure equivalent behaviour. Even if we do want to get rid of this, > > I'd advise to do it as a separate patch after this change, which can > > be independently reverted if there's problems with its removal. > > > Hi Russell, > > I will add the 2 lines you mentioned back to next version of the > patch. It is safer to do it that way as I do not see > pdev->dev.dma_mask gets initialized before the call > dma_set_mask_and_coherent inside this xhci_plat.c file. It would be good to add a WARN_ON() to the case where dma_mask is a NULL pointer at the least. That way, we will at least find out if there are some broken platforms that do not correctly initialize the mask pointer. Arnd
On Sat, Aug 15, 2015 at 1:05 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Saturday 08 August 2015 13:31:02 Duc Dang wrote: > > > > > > If we know that pdev->dev.dma_mask will always be initialised at this > > > point, then the above change is fine. If not, it's introducing a > > > regression - dma_set_mask_and_coherent() will fail if pdev->dev.dma_mask > > > is NULL (depending on the architectures implementation of dma_set_mask()). > > > > > > Prefixing the above change with the two lines I mention above would > > > ensure equivalent behaviour. Even if we do want to get rid of this, > > > I'd advise to do it as a separate patch after this change, which can > > > be independently reverted if there's problems with its removal. > > > > > Hi Russell, > > > > I will add the 2 lines you mentioned back to next version of the > > patch. It is safer to do it that way as I do not see > > pdev->dev.dma_mask gets initialized before the call > > dma_set_mask_and_coherent inside this xhci_plat.c file. > > It would be good to add a WARN_ON() to the case where dma_mask > is a NULL pointer at the least. That way, we will at least > find out if there are some broken platforms that do not correctly > initialize the mask pointer. Hi Arnd, So the check will look like this, please let me know what do you think: if (!pdev->dev.dma_mask) { WARN_ON(1); /* Initialize dma_mask if the broken platform code has not done so */ pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; } > > Arnd
On Wednesday 19 August 2015 14:28:33 Duc Dang wrote: > > Hi Arnd, > > So the check will look like this, please let me know what do you think: > if (!pdev->dev.dma_mask) { > WARN_ON(1); > /* Initialize dma_mask if the broken platform code has > not done so */ > pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; > } The condition can be written as if (WARN_ON(!pdev->dev.dma_mask)) and I'd use dma_coerce_mask_and_coherent() instead of manually setting the pointer, as an annotation for the fact that we are knowingly violating the API here. Those two points are just cosmetic though, aside from them, your code above is what I had in mind. Thanks, Arnd
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index 890ad9d..5d03f8b 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -93,14 +93,14 @@ static int xhci_plat_probe(struct platform_device *pdev) if (irq < 0) return -ENODEV; - /* Initialize dma_mask and coherent_dma_mask to 32-bits */ - ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32)); - if (ret) - return ret; - if (!pdev->dev.dma_mask) - pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; - else - dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)); + /* Try setting the coherent_dma_mask to 64 bits, then try 32 bits */ + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); + if (ret) { + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); + if (ret) + return ret; + } + hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev)); if (!hcd)