[PATCH] coresight: etb10: Print size of buffer we fail to allocate

[PATCH] coresight: etb10: Print size of buffer we fail to allocate

[Mark Brown]

When we initialise the ETB driver we attempt to allocate a buffer suitable
for storing the data buffered in the hardware based on sizing information
reported by the hardware. Unfortunately if the hardware is not properly
configured (for example if power domains are not set up correctly) then we
may read back a nonsensically large value and therefore the allocation will
be too big to succeed. Print an error message showing the amount of memory
we tried to allocate if the buffer allocation fails to help users diagnose
such problems.

Normally it is bad practice to print an error message on memory allocation
failures since there are verbose core messages reported for this but in
this case where the allocation size might be incorrect it is a useful hint.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/hwtracing/coresight/coresight-etb10.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index 40049869aecd..46eb9f88a29f 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -462,8 +462,11 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id)
 
 	drvdata->buf = devm_kzalloc(dev,
 				    drvdata->buffer_depth * 4, GFP_KERNEL);
-	if (!drvdata->buf)
+	if (!drvdata->buf) {
+		dev_err(dev, "Failed to allocate %u bytes for buffer data\n",
+			drvdata->buffer_depth * 4);
 		return -ENOMEM;
+	}
 
 	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
 	if (!desc)
-- 
2.1.4

[PATCH] coresight: etb10: Print size of buffer we fail to allocate

[Mathieu Poirier]

On 8 April 2015 at 16:15, Mark Brown <broonie@kernel.org> wrote:
> When we initialise the ETB driver we attempt to allocate a buffer suitable
> for storing the data buffered in the hardware based on sizing information
> reported by the hardware. Unfortunately if the hardware is not properly
> configured (for example if power domains are not set up correctly) then we
> may read back a nonsensically large value and therefore the allocation will
> be too big to succeed. Print an error message showing the amount of memory
> we tried to allocate if the buffer allocation fails to help users diagnose
> such problems.
>
> Normally it is bad practice to print an error message on memory allocation
> failures since there are verbose core messages reported for this but in
> this case where the allocation size might be incorrect it is a useful hint.
>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  drivers/hwtracing/coresight/coresight-etb10.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
> index 40049869aecd..46eb9f88a29f 100644
> --- a/drivers/hwtracing/coresight/coresight-etb10.c
> +++ b/drivers/hwtracing/coresight/coresight-etb10.c
> @@ -462,8 +462,11 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id)
>
>         drvdata->buf = devm_kzalloc(dev,
>                                     drvdata->buffer_depth * 4, GFP_KERNEL);
> -       if (!drvdata->buf)
> +       if (!drvdata->buf) {
> +               dev_err(dev, "Failed to allocate %u bytes for buffer data\n",
> +                       drvdata->buffer_depth * 4);
>                 return -ENOMEM;
> +       }
>
>         desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
>         if (!desc)
> --
> 2.1.4
>

This could be helpful during the bringup phase of the device when the
HW implementation isn't showing what's expected/intended.  On the
flips side such a problem would show up pretty quickly, specifically
with the -ENOMEM guarding against future damage.

I'll take this patch but will rework the commit message a little.  If
the power domains are not setup properly the device won't show up on
AMBA and as such won't get probed.

Thanks,
Mathieu

[PATCH] coresight: etb10: Print size of buffer we fail to allocate

[Mark Brown]

On Thu, Apr 09, 2015 at 08:46:55AM -0600, Mathieu Poirier wrote:

> I'll take this patch but will rework the commit message a little.  If
> the power domains are not setup properly the device won't show up on
> AMBA and as such won't get probed.

This depends on what the hardware does and how the device is registered
- if the device is registered with an ID provided (which you can do)
then AMBA won't bother reading the ID back and verifying that it's
correct so there's no interaction with the hardware until we hit the
driver.

I've not checked the DT case, though it doesn't seem like there's any
check that the compatible string matches the ID read from the hardware
which is a bit interesting...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150409/4a4978ff/attachment.sig>

[PATCH] coresight: etb10: Print size of buffer we fail to allocate

[Mathieu Poirier]

On 9 April 2015 at 08:55, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Apr 09, 2015 at 08:46:55AM -0600, Mathieu Poirier wrote:
>
>> I'll take this patch but will rework the commit message a little.  If
>> the power domains are not setup properly the device won't show up on
>> AMBA and as such won't get probed.
>
> This depends on what the hardware does and how the device is registered
> - if the device is registered with an ID provided (which you can do)
> then AMBA won't bother reading the ID back and verifying that it's
> correct so there's no interaction with the hardware until we hit the
> driver.

Indeed possible, but in that case you'll get a core dump or the kernel
will simply hang waiting on the memory area being read.

>
> I've not checked the DT case, though it doesn't seem like there's any
> check that the compatible string matches the ID read from the hardware
> which is a bit interesting...

This is something I've been meaning to add for a while now but never
got around to do it.

[PATCH] coresight: etb10: Print size of buffer we fail to allocate

[Mark Brown]

On Thu, Apr 09, 2015 at 09:14:40AM -0600, Mathieu Poirier wrote:
> On 9 April 2015 at 08:55, Mark Brown <broonie@kernel.org> wrote:

> > This depends on what the hardware does and how the device is registered
> > - if the device is registered with an ID provided (which you can do)
> > then AMBA won't bother reading the ID back and verifying that it's
> > correct so there's no interaction with the hardware until we hit the
> > driver.

> Indeed possible, but in that case you'll get a core dump or the kernel
> will simply hang waiting on the memory area being read.

That's the "depends on what the hardware does" bit - some will either
read back nonsense or some predictable but undesirable value like all
1s.

> > I've not checked the DT case, though it doesn't seem like there's any
> > check that the compatible string matches the ID read from the hardware
> > which is a bit interesting...

> This is something I've been meaning to add for a while now but never
> got around to do it.

Yeah, and it's not clear what the best thing to do is - should we be
using the DT to fix up problems in the hardware or should we be
verifying that people got the DT right?  Usually it's the former...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150409/7078512a/attachment.sig>