Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1410202 > unrolled thread
| Started by | Suzuki K Poulose <suzuki.poulose@arm.com> |
|---|---|
| First post | 2016-05-31 14:00 +0200 |
| Last post | 2016-06-01 11:40 +0200 |
| Articles | 3 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 3/5] coresight: Fix csdev connections initialisation Suzuki K Poulose <suzuki.poulose@arm.com> - 2016-05-31 14:00 +0200
Re: [PATCH 3/5] coresight: Fix csdev connections initialisation Mathieu Poirier <mathieu.poirier@linaro.org> - 2016-05-31 20:00 +0200
Re: [PATCH 3/5] coresight: Fix csdev connections initialisation Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2016-06-01 11:40 +0200
| From | Suzuki K Poulose <suzuki.poulose@arm.com> |
|---|---|
| Date | 2016-05-31 14:00 +0200 |
| Subject | [PATCH 3/5] coresight: Fix csdev connections initialisation |
| Message-ID | <rEQv0-2eq-15@gated-at.bofh.it> |
This is a cleanup patch.
coresight_device->conns holds an array to point to the devices
connected to the OUT ports of a component. Sinks, e.g ETR, do not
have an OUT port (nr_outport = 0), as it streams the trace to
memory via AXI.
At coresight_register() we do :
conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
if (!conns) {
ret = -ENOMEM;
goto err_kzalloc_conns;
}
For ETR, since the total size requested for kcalloc is zero, the return value is,
ZERO_SIZE_PTR ( != NULL). Hence, csdev->conns = ZERO_SIZE_PTR which cannot be
verified later to contain a valid pointer. The code which accesses the csdev->conns
is bounded by the csdev->nr_outport check, hence we don't try to dereference the
ZERO_SIZE_PTR. This patch cleans up the csdev->conns and csdev->refcnt, initialisation
to make sure we initialise it properly(i.e, either NULL or valid conns array).
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
drivers/hwtracing/coresight/coresight.c | 42 +++++++++++++++++++--------------
1 file changed, 24 insertions(+), 18 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 0fdaaf4..8410420 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -890,7 +890,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
int nr_refcnts = 1;
atomic_t *refcnts = NULL;
struct coresight_device *csdev;
- struct coresight_connection *conns;
+ struct coresight_connection *conns = NULL;
csdev = kzalloc(sizeof(*csdev), GFP_KERNEL);
if (!csdev) {
@@ -908,29 +908,35 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
nr_refcnts = desc->pdata->nr_outport;
}
- refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL);
- if (!refcnts) {
- ret = -ENOMEM;
- goto err_kzalloc_refcnts;
- }
+ if (nr_refcnts) {
+ refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL);
+ if (!refcnts) {
+ ret = -ENOMEM;
+ goto err_kzalloc_refcnts;
+ }
- csdev->refcnt = refcnts;
+ csdev->refcnt = refcnts;
+ }
csdev->nr_inport = desc->pdata->nr_inport;
csdev->nr_outport = desc->pdata->nr_outport;
- conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
- if (!conns) {
- ret = -ENOMEM;
- goto err_kzalloc_conns;
- }
- for (i = 0; i < csdev->nr_outport; i++) {
- conns[i].outport = desc->pdata->outports[i];
- conns[i].child_name = desc->pdata->child_names[i];
- conns[i].child_port = desc->pdata->child_ports[i];
- }
+ /* Initialise connections if there is at least one outport for this component */
+ if (csdev->nr_outport) {
+ conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
+ if (!conns) {
+ ret = -ENOMEM;
+ goto err_kzalloc_conns;
+ }
- csdev->conns = conns;
+ for (i = 0; i < csdev->nr_outport; i++) {
+ conns[i].outport = desc->pdata->outports[i];
+ conns[i].child_name = desc->pdata->child_names[i];
+ conns[i].child_port = desc->pdata->child_ports[i];
+ }
+
+ csdev->conns = conns;
+ }
csdev->type = desc->type;
csdev->subtype = desc->subtype;
--
1.9.1
[toc] | [next] | [standalone]
| From | Mathieu Poirier <mathieu.poirier@linaro.org> |
|---|---|
| Date | 2016-05-31 20:00 +0200 |
| Message-ID | <rEW7n-5MT-15@gated-at.bofh.it> |
| In reply to | #1410202 |
On 31 May 2016 at 05:57, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> This is a cleanup patch.
>
> coresight_device->conns holds an array to point to the devices
> connected to the OUT ports of a component. Sinks, e.g ETR, do not
> have an OUT port (nr_outport = 0), as it streams the trace to
> memory via AXI.
>
> At coresight_register() we do :
>
> conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
> if (!conns) {
> ret = -ENOMEM;
> goto err_kzalloc_conns;
> }
>
> For ETR, since the total size requested for kcalloc is zero, the return value is,
> ZERO_SIZE_PTR ( != NULL). Hence, csdev->conns = ZERO_SIZE_PTR which cannot be
> verified later to contain a valid pointer. The code which accesses the csdev->conns
> is bounded by the csdev->nr_outport check, hence we don't try to dereference the
> ZERO_SIZE_PTR. This patch cleans up the csdev->conns and csdev->refcnt, initialisation
> to make sure we initialise it properly(i.e, either NULL or valid conns array).
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> drivers/hwtracing/coresight/coresight.c | 42 +++++++++++++++++++--------------
> 1 file changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 0fdaaf4..8410420 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -890,7 +890,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
> int nr_refcnts = 1;
> atomic_t *refcnts = NULL;
> struct coresight_device *csdev;
> - struct coresight_connection *conns;
> + struct coresight_connection *conns = NULL;
>
> csdev = kzalloc(sizeof(*csdev), GFP_KERNEL);
> if (!csdev) {
> @@ -908,29 +908,35 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
> nr_refcnts = desc->pdata->nr_outport;
> }
>
> - refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL);
> - if (!refcnts) {
> - ret = -ENOMEM;
> - goto err_kzalloc_refcnts;
> - }
> + if (nr_refcnts) {
Did you manage to find a usecase where "nr_refcnts == 0" ? Since
components have at least one port this condition will always be true.
> + refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL);
> + if (!refcnts) {
> + ret = -ENOMEM;
> + goto err_kzalloc_refcnts;
> + }
>
> - csdev->refcnt = refcnts;
> + csdev->refcnt = refcnts;
> + }
>
> csdev->nr_inport = desc->pdata->nr_inport;
> csdev->nr_outport = desc->pdata->nr_outport;
> - conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
> - if (!conns) {
> - ret = -ENOMEM;
> - goto err_kzalloc_conns;
> - }
>
> - for (i = 0; i < csdev->nr_outport; i++) {
> - conns[i].outport = desc->pdata->outports[i];
> - conns[i].child_name = desc->pdata->child_names[i];
> - conns[i].child_port = desc->pdata->child_ports[i];
> - }
> + /* Initialise connections if there is at least one outport for this component */
> + if (csdev->nr_outport) {
> + conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
> + if (!conns) {
> + ret = -ENOMEM;
> + goto err_kzalloc_conns;
> + }
>
> - csdev->conns = conns;
> + for (i = 0; i < csdev->nr_outport; i++) {
> + conns[i].outport = desc->pdata->outports[i];
> + conns[i].child_name = desc->pdata->child_names[i];
> + conns[i].child_port = desc->pdata->child_ports[i];
> + }
> +
> + csdev->conns = conns;
The purpose of your patch is to correctly initialise csdev->conns to
NULL if there is no output port to speak of. As such shouldn't the
above statement be out of the if (csdev->nr_outport) {} ?.
I'm also getting a couple of checkpatch.pl warnings on this patch.
> + }
>
> csdev->type = desc->type;
> csdev->subtype = desc->subtype;
> --
> 1.9.1
>
[toc] | [prev] | [next] | [standalone]
| From | Suzuki K Poulose <Suzuki.Poulose@arm.com> |
|---|---|
| Date | 2016-06-01 11:40 +0200 |
| Message-ID | <rFaN4-6FG-9@gated-at.bofh.it> |
| In reply to | #1410504 |
On 31/05/16 18:55, Mathieu Poirier wrote:
> On 31 May 2016 at 05:57, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>> This is a cleanup patch.
>>
>> coresight_device->conns holds an array to point to the devices
>> connected to the OUT ports of a component. Sinks, e.g ETR, do not
>> have an OUT port (nr_outport = 0), as it streams the trace to
>> memory via AXI.
>>
>> At coresight_register() we do :
>>
>> conns = kcalloc(csdev->nr_outport, sizeof(*conns), GFP_KERNEL);
>> if (!conns) {
>> ret = -ENOMEM;
>> goto err_kzalloc_conns;
>> }
>>
>> For ETR, since the total size requested for kcalloc is zero, the return value is,
>> ZERO_SIZE_PTR ( != NULL). Hence, csdev->conns = ZERO_SIZE_PTR which cannot be
>> verified later to contain a valid pointer. The code which accesses the csdev->conns
>> is bounded by the csdev->nr_outport check, hence we don't try to dereference the
>> ZERO_SIZE_PTR. This patch cleans up the csdev->conns and csdev->refcnt, initialisation
>> to make sure we initialise it properly(i.e, either NULL or valid conns array).
>>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>> drivers/hwtracing/coresight/coresight.c | 42 +++++++++++++++++++--------------
>> 1 file changed, 24 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
>> index 0fdaaf4..8410420 100644
>> --- a/drivers/hwtracing/coresight/coresight.c
>> +++ b/drivers/hwtracing/coresight/coresight.c
>> @@ -890,7 +890,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>> int nr_refcnts = 1;
>> atomic_t *refcnts = NULL;
>> struct coresight_device *csdev;
>> - struct coresight_connection *conns;
>> + struct coresight_connection *conns = NULL;
>>
>> csdev = kzalloc(sizeof(*csdev), GFP_KERNEL);
>> if (!csdev) {
>> @@ -908,29 +908,35 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>> nr_refcnts = desc->pdata->nr_outport;
>> }
>>
>> - refcnts = kcalloc(nr_refcnts, sizeof(*refcnts), GFP_KERNEL);
>> - if (!refcnts) {
>> - ret = -ENOMEM;
>> - goto err_kzalloc_refcnts;
>> - }
>> + if (nr_refcnts) {
>
> Did you manage to find a usecase where "nr_refcnts == 0" ? Since
> components have at least one port this condition will always be true.
No, I didn't find one. While I was fixing the nr_outport, I thought it would
be good to check the refcnts as well.
>>
>> - csdev->conns = conns;
>> + for (i = 0; i < csdev->nr_outport; i++) {
>> + conns[i].outport = desc->pdata->outports[i];
>> + conns[i].child_name = desc->pdata->child_names[i];
>> + conns[i].child_port = desc->pdata->child_ports[i];
>> + }
>> +
>> + csdev->conns = conns;
>
> The purpose of your patch is to correctly initialise csdev->conns to
> NULL if there is no output port to speak of. As such shouldn't the
> above statement be out of the if (csdev->nr_outport) {} ?.
Not necessarily. We do a kzalloc() for csdev, which implies csdev->conns
is already NULL. I can move the code to make that explicit assignment.
>
> I'm also getting a couple of checkpatch.pl warnings on this patch.
I can fix those warnings. I ignored them initially, as it was for the length of the comment.
Cheers
Suzuki
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web