| Message ID | 1439416290-21228-3-git-send-email-jeremy.linton@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2015/8/13 5:51, Jeremy Linton wrote: > If the ehci driver fails to configure the dma settings then display > a dev error instead of simply failing. This is triggered in an > ACPI world if the user fails to set the _CCA on the device. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > drivers/usb/host/ehci-platform.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c > index 2593def..82e396f 100644 > --- a/drivers/usb/host/ehci-platform.c > +++ b/drivers/usb/host/ehci-platform.c > @@ -162,8 +162,10 @@ static int ehci_platform_probe(struct platform_device *dev) > > err = dma_coerce_mask_and_coherent(&dev->dev, > pdata->dma_mask_64 ? DMA_BIT_MASK(64) : DMA_BIT_MASK(32)); > - if (err) > + if (err) { > + dev_err(&dev->dev, "Error: DMA setup failed\n"); > return err; > + } > > irq = platform_get_irq(dev, 0); > if (irq < 0) { Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org> Thanks Hanjun
On Wed, 12 Aug 2015, Jeremy Linton wrote: > If the ehci driver fails to configure the dma settings then display > a dev error instead of simply failing. This is triggered in an > ACPI world if the user fails to set the _CCA on the device. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > drivers/usb/host/ehci-platform.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c > index 2593def..82e396f 100644 > --- a/drivers/usb/host/ehci-platform.c > +++ b/drivers/usb/host/ehci-platform.c > @@ -162,8 +162,10 @@ static int ehci_platform_probe(struct platform_device *dev) > > err = dma_coerce_mask_and_coherent(&dev->dev, > pdata->dma_mask_64 ? DMA_BIT_MASK(64) : DMA_BIT_MASK(32)); > - if (err) > + if (err) { > + dev_err(&dev->dev, "Error: DMA setup failed\n"); > return err; > + } > > irq = platform_get_irq(dev, 0); > if (irq < 0) { > Too bad the ohci-platform driver doesn't get the same update, but it clearly wouldn't belong in this patch set. Anyway, Acked-by: Alan Stern <stern@rowland.harvard.edu> Alan Stern
On Wednesday 12 August 2015 16:51:29 Jeremy Linton wrote: > diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c > index 2593def..82e396f 100644 > --- a/drivers/usb/host/ehci-platform.c > +++ b/drivers/usb/host/ehci-platform.c > @@ -162,8 +162,10 @@ static int ehci_platform_probe(struct platform_device *dev) > > err = dma_coerce_mask_and_coherent(&dev->dev, > pdata->dma_mask_64 ? DMA_BIT_MASK(64) : DMA_BIT_MASK(32)); > - if (err) > + if (err) { > + dev_err(&dev->dev, "Error: DMA setup failed\n"); > return err; > + } We should really stop doing this: the platform should provide the correct dma mask when creating the device, instead of setting a bogus pdata field. Do not duplicate this bug for ACPI. Arnd
Arnd, On 08/14/2015 04:19 PM, Arnd Bergmann wrote: > On Wednesday 12 August 2015 16:51:29 Jeremy Linton wrote: >> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c >> index 2593def..82e396f 100644 >> --- a/drivers/usb/host/ehci-platform.c >> +++ b/drivers/usb/host/ehci-platform.c >> @@ -162,8 +162,10 @@ static int ehci_platform_probe(struct platform_device *dev) >> >> err = dma_coerce_mask_and_coherent(&dev->dev, >> pdata->dma_mask_64 ? DMA_BIT_MASK(64) : DMA_BIT_MASK(32)); >> - if (err) >> + if (err) { >> + dev_err(&dev->dev, "Error: DMA setup failed\n"); >> return err; >> + } > > We should really stop doing this: the platform should provide the > correct dma mask when creating the device, instead of setting a > bogus pdata field. Do not duplicate this bug for ACPI. I'm not sure I understand, I'm not changing the mask anywhere, all I'm doing is printing a message if the mask cannot be set. AKA if CCA is missing or set to 0 then, this line is triggered because the dma_coerce_mask_and_coherent() routine returns with a failure indicating there aren't any DMA ops to set a mask on. Right now what happens is the device detection displays that there is an EHCI controller then silently disappears with telling the user why it didn't configure. Thanks,
On Friday 14 August 2015 16:44:12 Jeremy Linton wrote: > On 08/14/2015 04:19 PM, Arnd Bergmann wrote: > > On Wednesday 12 August 2015 16:51:29 Jeremy Linton wrote: > >> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c > >> index 2593def..82e396f 100644 > >> --- a/drivers/usb/host/ehci-platform.c > >> +++ b/drivers/usb/host/ehci-platform.c > >> @@ -162,8 +162,10 @@ static int ehci_platform_probe(struct platform_device *dev) > >> > >> err = dma_coerce_mask_and_coherent(&dev->dev, > >> pdata->dma_mask_64 ? DMA_BIT_MASK(64) : DMA_BIT_MASK(32)); > >> - if (err) > >> + if (err) { > >> + dev_err(&dev->dev, "Error: DMA setup failed\n"); > >> return err; > >> + } > > > > We should really stop doing this: the platform should provide the > > correct dma mask when creating the device, instead of setting a > > bogus pdata field. Do not duplicate this bug for ACPI. > > > I'm not sure I understand, I'm not changing the mask anywhere, all I'm > doing is printing a message if the mask cannot be set. The problem is that dma_coerce_mask_and_coherent() is not a proper interface for a driver to use, it's an annotation that tells us that the driver is trying to do something horrible by overriding the dma mask that was set by the platform. > AKA if CCA is missing or set to 0 then, this line is triggered because > the dma_coerce_mask_and_coherent() routine returns with a failure > indicating there aren't any DMA ops to set a mask on. Right now what > happens is the device detection displays that there is an EHCI > controller then silently disappears with telling the user why it didn't > configure. Adding a warning is fine, but the text is not right here (the driver is not setting up the dma mask, but trying to override the one that has been set up by the platform), and the solution should really be to not call dma_coerce_mask_and_coherent() at all for ACPI. The main reason that we have not removed this call here yet is that there is still a MIPS platform that relies on it. We should probably remove it for DT already and only do it if platform data was provided, but that requires careful testing. Arnd
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c index 2593def..82e396f 100644 --- a/drivers/usb/host/ehci-platform.c +++ b/drivers/usb/host/ehci-platform.c @@ -162,8 +162,10 @@ static int ehci_platform_probe(struct platform_device *dev) err = dma_coerce_mask_and_coherent(&dev->dev, pdata->dma_mask_64 ? DMA_BIT_MASK(64) : DMA_BIT_MASK(32)); - if (err) + if (err) { + dev_err(&dev->dev, "Error: DMA setup failed\n"); return err; + } irq = platform_get_irq(dev, 0); if (irq < 0) {
If the ehci driver fails to configure the dma settings then display a dev error instead of simply failing. This is triggered in an ACPI world if the user fails to set the _CCA on the device. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- drivers/usb/host/ehci-platform.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)