google_battery, google_bms: plug memleak: case 2.2.2: for_each_child_of_node() (v6.12 prep)
Case 2.2.2 from below bug:
If returning or breaking early from within
for_each_child_of_node(), a reference to the current device_node
remains and needs to be dropped, either right away, or later if
used past that loop to avoid a memleak.
These drivers call gbms_batt_id_node() at various times, and each time
either a new node (via for_each_child_of_node()) or the same node is
returned. If a new node is returned, it must be free'd, if the same
node is returned, it must not be free'd. Currently, new nodes returned
are never free'd, causing a memleak each time.
Since a new node must be free'd and the existing node doesn't, and
since callers don't necessarily know which type of node was returned,
the correct approach to plug this leak here is to always return a node
pointer that can safely be free'd. This can be achieved by simply
adding an extra reference to the existing node if applicable. This way,
callers can unconditionally free the returned node.
Do so, and use scoped cleanup to simplify all cleanup paths.
This commit plugs memleaks in normal code paths.
Test: TH
Bug: 370679843
Change-Id: I5317821c2299a175fb16f6458ba8ee770c31211f
Signed-off-by: André Draszik <[email protected]>
Signed-off-by: Will McVicker <[email protected]>
diff --git a/google_battery.c b/google_battery.c
index 45cfbbd..d52f926 100644
--- a/google_battery.c
+++ b/google_battery.c
@@ -5690,6 +5690,7 @@
{
struct gbms_chg_profile *profile = &batt_drv->chg_profile;
int ret = 0;
+ struct device_node *batt_id_node __free(device_node) = NULL;
/* handle retry */
if (!profile->cccm_limits) {
@@ -5698,8 +5699,10 @@
return -EINVAL;
}
+ batt_id_node = gbms_batt_id_node(node);
+
/* this is in mAh */
- ret = of_property_read_u32(gbms_batt_id_node(node),
+ ret = of_property_read_u32(batt_id_node,
"google,chg-battery-capacity",
&batt_drv->battery_capacity);
/* google,chg-battery-capacity does not exist in the child_node */
@@ -5742,7 +5745,7 @@
}
/* TODO: dump the AACR table if supported */
- ret = gbms_read_aacr_limits(profile, gbms_batt_id_node(node));
+ ret = gbms_read_aacr_limits(profile, batt_id_node);
if (ret == 0)
pr_info("AACR: supported\n");
@@ -11083,6 +11086,7 @@
struct bhi_data *bhi_data = &health_data->bhi_data;
u16 capacity_boundary[BHI_TREND_POINTS_SIZE];
int ret, i;
+ struct device_node *batt_id_node __free(device_node) = NULL;
/* set upper_bound value to BHI_CAPACITY_MAX(0xFFFF) */
memset(bhi_data->upper_bound.limit, 0xFF, sizeof(bhi_data->upper_bound.limit));
@@ -11145,7 +11149,9 @@
/* need battery id to get right trend points */
batt_drv->batt_id = GPSY_GET_PROP(batt_drv->fg_psy, GBMS_PROP_BATT_ID);
- ret = of_property_read_u16_array(gbms_batt_id_node(batt_drv->device->of_node),
+ batt_id_node = gbms_batt_id_node(batt_drv->device->of_node);
+
+ ret = of_property_read_u16_array(batt_id_node,
"google,bhi-l-bound", &capacity_boundary[0],
BHI_TREND_POINTS_SIZE);
if (ret == 0 && bhi_bound_validity_check(capacity_boundary, 0,
@@ -11163,7 +11169,7 @@
bhi_data->lower_bound.limit[6], bhi_data->lower_bound.limit[7],
bhi_data->lower_bound.limit[8], bhi_data->lower_bound.limit[9]);
- ret = of_property_read_u16_array(gbms_batt_id_node(batt_drv->device->of_node),
+ ret = of_property_read_u16_array(batt_id_node,
"google,bhi-u-bound", &capacity_boundary[0],
BHI_TREND_POINTS_SIZE);
if (ret == 0 && bhi_bound_validity_check(capacity_boundary, batt_drv->battery_capacity,
@@ -11181,7 +11187,7 @@
bhi_data->upper_bound.limit[6], bhi_data->upper_bound.limit[7],
bhi_data->upper_bound.limit[8], bhi_data->upper_bound.limit[9]);
- ret = of_property_read_u16_array(gbms_batt_id_node(batt_drv->device->of_node),
+ ret = of_property_read_u16_array(batt_id_node,
"google,bhi-l-trigger", &capacity_boundary[0],
BHI_TREND_POINTS_SIZE);
if (ret == 0 && bhi_bound_validity_check(capacity_boundary, BHI_CAPACITY_MIN,
@@ -11197,7 +11203,7 @@
bhi_data->lower_bound.trigger[8], bhi_data->lower_bound.trigger[9]);
}
- ret = of_property_read_u16_array(gbms_batt_id_node(batt_drv->device->of_node),
+ ret = of_property_read_u16_array(batt_id_node,
"google,bhi-u-trigger", &capacity_boundary[0],
BHI_TREND_POINTS_SIZE);
if (ret == 0 && bhi_bound_validity_check(capacity_boundary, BHI_CAPACITY_MIN,
@@ -11306,6 +11312,7 @@
struct batt_drv *batt_drv = container_of(work, struct batt_drv,
init_work.work);
struct device_node *node = batt_drv->device->of_node;
+ struct device_node *batt_id_node __free(device_node) = NULL;
struct power_supply *fg_psy = batt_drv->fg_psy;
const char *batt_vs_tz_name = NULL;
int init_delay_ms, ret = 0;
@@ -11618,12 +11625,14 @@
if (batt_drv->dc_irdrop)
pr_info("dc irdrop is enabled\n");
- batt_drv->pullback_current = of_property_read_bool(gbms_batt_id_node(node),
+ batt_id_node = gbms_batt_id_node(node);
+
+ batt_drv->pullback_current = of_property_read_bool(batt_id_node,
"google,pullback-current");
if (batt_drv->pullback_current)
pr_info("pullback current is enabled\n");
- batt_drv->allow_higher_fv = of_property_read_bool(gbms_batt_id_node(node),
+ batt_drv->allow_higher_fv = of_property_read_bool(batt_id_node,
"google,allow-higher-fv");
if (batt_drv->allow_higher_fv)
pr_info("allow higher fv is enabled\n");
diff --git a/google_bms.c b/google_bms.c
index 374c106..44f65bf 100644
--- a/google_bms.c
+++ b/google_bms.c
@@ -23,6 +23,7 @@
#define gbms_err(p, fmt, ...) \
pr_err("%s: " fmt, gbms_owner(p), ##__VA_ARGS__)
+#include <linux/cleanup.h>
#include <linux/kernel.h>
#include <linux/printk.h>
#include <linux/module.h>
@@ -123,7 +124,7 @@
if (ret < 0) {
pr_warn("Failed to get batt_id (%d)\n", ret);
- return config_node;
+ return of_node_get(config_node);
}
for_each_child_of_node(config_node, child_node) {
@@ -136,7 +137,7 @@
return child_node;
}
- return config_node;
+ return of_node_get(config_node);
}
EXPORT_SYMBOL_GPL(gbms_batt_id_node);
@@ -184,6 +185,7 @@
struct device_node *node)
{
int ret;
+ struct device_node *batt_id_node __free(device_node) = NULL;
profile->temp_nb_limits =
of_property_count_elems_of_size(node, "google,chg-temp-limits",
@@ -207,8 +209,10 @@
return ret;
}
+ batt_id_node = gbms_batt_id_node(node);
+
profile->volt_nb_limits =
- of_property_count_elems_of_size(gbms_batt_id_node(node), "google,chg-cv-limits",
+ of_property_count_elems_of_size(batt_id_node, "google,chg-cv-limits",
sizeof(u32));
/* google,chg-cv-limits does not exist in the child_node */
if (profile->volt_nb_limits <= 0)
@@ -225,7 +229,7 @@
GBMS_CHG_VOLT_NB_LIMITS_MAX);
return -EINVAL;
}
- ret = of_property_read_u32_array(gbms_batt_id_node(node), "google,chg-cv-limits",
+ ret = of_property_read_u32_array(batt_id_node, "google,chg-cv-limits",
(u32 *)profile->volt_limits,
profile->volt_nb_limits);
/* google,chg-cv-limits does not exist in the child_node */
