| Message ID | 20191014095152.6276-1-bala24@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | occ-sensor: clean dt properties if sensor is not available | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (1785745d5a7eaefd7d0c135f2a3b0f5d86aefec5) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
On Mon, Oct 14, 2019 at 8:52 PM Balamuruhan S <bala24@linux.ibm.com> wrote: > > In `occ_sensor_init()` device tree node is created for sensor-goups > and performs `occ_sensor_sanity()` check to initialize the device > tree. But if there are no sensors like in Qemu, sanity check fails > but still device tree populates the sensor-groups node wrongly as > the node created is not cleaned up. Does having the empty node causing any problems? Nuisance warnings or whatever? > > Signed-off-by: Balamuruhan S <bala24@linux.ibm.com> > --- > hw/occ-sensor.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/hw/occ-sensor.c b/hw/occ-sensor.c > index d06ca725..ec116c9a 100644 > --- a/hw/occ-sensor.c > +++ b/hw/occ-sensor.c > @@ -491,6 +491,7 @@ bool occ_sensors_init(void) > struct dt_node *sg, *exports; > int occ_num = 0, i; > bool has_gpu = false; > + bool has_sensor = false; > > /* OCC inband sensors is only supported in P9 */ > if (proc_gen != proc_gen_p9) > @@ -526,7 +527,9 @@ bool occ_sensors_init(void) > md = get_names_block(hb); > > /* Sanity check of the Sensor Data Header Block */ > - if (!occ_sensor_sanity(hb, chip->id)) > + if (occ_sensor_sanity(hb, chip->id)) > + has_sensor = true; > + else > continue; > > phandles = malloc(hb->nr_sensors * sizeof(u32)); > @@ -587,6 +590,10 @@ bool occ_sensors_init(void) > free(phandles); > free(ptype); > } > + if (!has_sensor) { > + /* clear the device tree property for sensors */ > + dt_free(sg); > + } Would it make more sense to use list_empty(&sg->children) as the test? > > if (!occ_num) > return false; > -- > 2.14.5 >
On 10/14/19 3:21 PM, Balamuruhan S wrote: > In `occ_sensor_init()` device tree node is created for sensor-goups > and performs `occ_sensor_sanity()` check to initialize the device > tree. But if there are no sensors like in Qemu, sanity check fails > but still device tree populates the sensor-groups node wrongly as > the node created is not cleaned up. > > Signed-off-by: Balamuruhan S <bala24@linux.ibm.com> > --- > hw/occ-sensor.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/hw/occ-sensor.c b/hw/occ-sensor.c > index d06ca725..ec116c9a 100644 > --- a/hw/occ-sensor.c > +++ b/hw/occ-sensor.c > @@ -491,6 +491,7 @@ bool occ_sensors_init(void) > struct dt_node *sg, *exports; > int occ_num = 0, i; > bool has_gpu = false; > + bool has_sensor = false; > > /* OCC inband sensors is only supported in P9 */ > if (proc_gen != proc_gen_p9) > @@ -526,7 +527,9 @@ bool occ_sensors_init(void) > md = get_names_block(hb); > > /* Sanity check of the Sensor Data Header Block */ > - if (!occ_sensor_sanity(hb, chip->id)) > + if (occ_sensor_sanity(hb, chip->id)) > + has_sensor = true; > + else > continue; > > phandles = malloc(hb->nr_sensors * sizeof(u32)); > @@ -587,6 +590,10 @@ bool occ_sensors_init(void) > free(phandles); > free(ptype); > } > + if (!has_sensor) { > + /* clear the device tree property for sensors */ > + dt_free(sg); > + } Patch doesn't explain what is the issue? Are you hitting any issue -OR- just cleanup? May be its better to defer dt node creation until we find some sensors instead of freeing up at the end. -Vasant
On Mon, Oct 14, 2019 at 10:32:16PM +1100, Oliver O'Halloran wrote: > On Mon, Oct 14, 2019 at 8:52 PM Balamuruhan S <bala24@linux.ibm.com> wrote: > > > > In `occ_sensor_init()` device tree node is created for sensor-goups > > and performs `occ_sensor_sanity()` check to initialize the device > > tree. But if there are no sensors like in Qemu, sanity check fails > > but still device tree populates the sensor-groups node wrongly as > > the node created is not cleaned up. > > Does having the empty node causing any problems? Nuisance warnings or whatever? No, it doesn't cause any problem, I did observe an empty hanging node of `sensor-groups`. > > > > > Signed-off-by: Balamuruhan S <bala24@linux.ibm.com> > > --- > > hw/occ-sensor.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/hw/occ-sensor.c b/hw/occ-sensor.c > > index d06ca725..ec116c9a 100644 > > --- a/hw/occ-sensor.c > > +++ b/hw/occ-sensor.c > > @@ -491,6 +491,7 @@ bool occ_sensors_init(void) > > struct dt_node *sg, *exports; > > int occ_num = 0, i; > > bool has_gpu = false; > > + bool has_sensor = false; > > > > /* OCC inband sensors is only supported in P9 */ > > if (proc_gen != proc_gen_p9) > > @@ -526,7 +527,9 @@ bool occ_sensors_init(void) > > md = get_names_block(hb); > > > > /* Sanity check of the Sensor Data Header Block */ > > - if (!occ_sensor_sanity(hb, chip->id)) > > + if (occ_sensor_sanity(hb, chip->id)) > > + has_sensor = true; > > + else > > continue; > > > > phandles = malloc(hb->nr_sensors * sizeof(u32)); > > @@ -587,6 +590,10 @@ bool occ_sensors_init(void) > > free(phandles); > > free(ptype); > > } > > + if (!has_sensor) { > > + /* clear the device tree property for sensors */ > > + dt_free(sg); > > + } > > Would it make more sense to use list_empty(&sg->children) as the test? yes, we can avoid introducing `has_sensor` instead by using list_empty(). Thanks Oliver. > > > > > if (!occ_num) > > return false; > > -- > > 2.14.5 > >
On Mon, Oct 14, 2019 at 10:44:40PM +0530, Vasant Hegde wrote: > On 10/14/19 3:21 PM, Balamuruhan S wrote: > > In `occ_sensor_init()` device tree node is created for sensor-goups > > and performs `occ_sensor_sanity()` check to initialize the device > > tree. But if there are no sensors like in Qemu, sanity check fails > > but still device tree populates the sensor-groups node wrongly as > > the node created is not cleaned up. > > > > Signed-off-by: Balamuruhan S <bala24@linux.ibm.com> > > --- > > hw/occ-sensor.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/hw/occ-sensor.c b/hw/occ-sensor.c > > index d06ca725..ec116c9a 100644 > > --- a/hw/occ-sensor.c > > +++ b/hw/occ-sensor.c > > @@ -491,6 +491,7 @@ bool occ_sensors_init(void) > > struct dt_node *sg, *exports; > > int occ_num = 0, i; > > bool has_gpu = false; > > + bool has_sensor = false; > > > > /* OCC inband sensors is only supported in P9 */ > > if (proc_gen != proc_gen_p9) > > @@ -526,7 +527,9 @@ bool occ_sensors_init(void) > > md = get_names_block(hb); > > > > /* Sanity check of the Sensor Data Header Block */ > > - if (!occ_sensor_sanity(hb, chip->id)) > > + if (occ_sensor_sanity(hb, chip->id)) > > + has_sensor = true; > > + else > > continue; > > > > phandles = malloc(hb->nr_sensors * sizeof(u32)); > > @@ -587,6 +590,10 @@ bool occ_sensors_init(void) > > free(phandles); > > free(ptype); > > } > > + if (!has_sensor) { > > + /* clear the device tree property for sensors */ > > + dt_free(sg); > > + } > > Patch doesn't explain what is the issue? Are you hitting any issue -OR- just > cleanup? It doesn't create any issue, just a minor cleanup. > May be its better to defer dt node creation until we find some sensors > instead of freeing up at the end. Yeah I thought about it, but currently it creates the head node `sensor-groups`, iterate over each chip and its sensors to create child node. Finally if no valid sensors header block detected, initially created `sensor-groups` hangs empty. so we could not defer head dt node in this logic, please suggest if we can handle in better way. Thanks Vasant. > > -Vasant
diff --git a/hw/occ-sensor.c b/hw/occ-sensor.c index d06ca725..ec116c9a 100644 --- a/hw/occ-sensor.c +++ b/hw/occ-sensor.c @@ -491,6 +491,7 @@ bool occ_sensors_init(void) struct dt_node *sg, *exports; int occ_num = 0, i; bool has_gpu = false; + bool has_sensor = false; /* OCC inband sensors is only supported in P9 */ if (proc_gen != proc_gen_p9) @@ -526,7 +527,9 @@ bool occ_sensors_init(void) md = get_names_block(hb); /* Sanity check of the Sensor Data Header Block */ - if (!occ_sensor_sanity(hb, chip->id)) + if (occ_sensor_sanity(hb, chip->id)) + has_sensor = true; + else continue; phandles = malloc(hb->nr_sensors * sizeof(u32)); @@ -587,6 +590,10 @@ bool occ_sensors_init(void) free(phandles); free(ptype); } + if (!has_sensor) { + /* clear the device tree property for sensors */ + dt_free(sg); + } if (!occ_num) return false;
In `occ_sensor_init()` device tree node is created for sensor-goups and performs `occ_sensor_sanity()` check to initialize the device tree. But if there are no sensors like in Qemu, sanity check fails but still device tree populates the sensor-groups node wrongly as the node created is not cleaned up. Signed-off-by: Balamuruhan S <bala24@linux.ibm.com> --- hw/occ-sensor.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)