* [bug report] power: supply: Add charger driver for Rockchip RK817
@ 2022-10-10 13:14 Dan Carpenter
0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2022-10-10 13:14 UTC (permalink / raw)
To: macromorgan; +Cc: linux-pm
Hello Chris Morgan,
The patch 11cb8da0189b: "power: supply: Add charger driver for
Rockchip RK817" from Aug 26, 2022, leads to the following Smatch
static checker warning:
drivers/power/supply/rk817_charger.c:143 rk817_chg_cur_to_reg()
warn: signedness bug returning '(-22)'
drivers/power/supply/rk817_charger.c
124 static u8 rk817_chg_cur_to_reg(u32 chg_cur_ma)
^^
This is a u8.
125 {
126 if (chg_cur_ma >= 3500)
127 return CHG_3_5A;
128 else if (chg_cur_ma >= 3000)
129 return CHG_3A;
130 else if (chg_cur_ma >= 2750)
131 return CHG_2_75A;
132 else if (chg_cur_ma >= 2500)
133 return CHG_2_5A;
134 else if (chg_cur_ma >= 2000)
135 return CHG_2A;
136 else if (chg_cur_ma >= 1500)
137 return CHG_1_5A;
138 else if (chg_cur_ma >= 1000)
139 return CHG_1A;
140 else if (chg_cur_ma >= 500)
141 return CHG_0_5A;
142 else
--> 143 return -EINVAL;
^^^^^^^^^^^^^^^
Can't return a negative.
144 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread* [bug report] power: supply: Add charger driver for Rockchip RK817
@ 2023-05-04 10:58 Dan Carpenter
2023-05-08 18:46 ` Chris Morgan
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2023-05-04 10:58 UTC (permalink / raw)
To: macromorgan; +Cc: linux-pm
Hello Chris Morgan,
The patch 11cb8da0189b: "power: supply: Add charger driver for
Rockchip RK817" from Aug 26, 2022, leads to the following Smatch
static checker warning:
drivers/power/supply/rk817_charger.c:1198 rk817_charger_probe()
warn: inconsistent refcounting 'node->kobj.kref.refcount.refs.counter':
inc on: 1088,1105,1115,1124,1130,1136,1146,1160,1166,1170,1177,1186,1193
dec on: 1067
drivers/power/supply/rk817_charger.c
1048 static int rk817_charger_probe(struct platform_device *pdev)
1049 {
1050 struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent);
1051 struct rk817_charger *charger;
1052 struct device_node *node;
1053 struct power_supply_battery_info *bat_info;
1054 struct device *dev = &pdev->dev;
1055 struct power_supply_config pscfg = {};
1056 int plugin_irq, plugout_irq;
1057 int of_value;
1058 int ret;
1059
1060 node = of_get_child_by_name(dev->parent->of_node, "charger");
1061 if (!node)
1062 return -ENODEV;
1063
1064 charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
1065 if (!charger) {
1066 of_node_put(node);
This error path calls of_node_put() but probably they all should.
1067 return -ENOMEM;
1068 }
1069
1070 charger->rk808 = rk808;
1071
1072 charger->dev = &pdev->dev;
1073 platform_set_drvdata(pdev, charger);
1074
1075 rk817_bat_calib_vol(charger);
1076
1077 pscfg.drv_data = charger;
1078 pscfg.of_node = node;
1079
1080 /*
1081 * Get sample resistor value. Note only values of 10000 or 20000
1082 * microohms are allowed. Schematic for my test implementation (an
1083 * Odroid Go Advance) shows a 10 milliohm resistor for reference.
1084 */
1085 ret = of_property_read_u32(node, "rockchip,resistor-sense-micro-ohms",
1086 &of_value);
1087 if (ret < 0) {
1088 return dev_err_probe(dev, ret,
1089 "Error reading sample resistor value\n");
1090 }
1091 /*
1092 * Store as a 1 or a 2, since all we really use the value for is as a
1093 * divisor in some calculations.
1094 */
1095 charger->res_div = (of_value == 20000) ? 2 : 1;
1096
1097 /*
1098 * Get sleep enter current value. Not sure what this value is for
1099 * other than to help calibrate the relax threshold.
1100 */
1101 ret = of_property_read_u32(node,
1102 "rockchip,sleep-enter-current-microamp",
1103 &of_value);
1104 if (ret < 0) {
1105 return dev_err_probe(dev, ret,
1106 "Error reading sleep enter cur value\n");
1107 }
1108 charger->sleep_enter_current_ua = of_value;
1109
1110 /* Get sleep filter current value */
1111 ret = of_property_read_u32(node,
1112 "rockchip,sleep-filter-current-microamp",
1113 &of_value);
1114 if (ret < 0) {
1115 return dev_err_probe(dev, ret,
1116 "Error reading sleep filter cur value\n");
1117 }
1118
1119 charger->sleep_filter_current_ua = of_value;
1120
1121 charger->bat_ps = devm_power_supply_register(&pdev->dev,
1122 &rk817_bat_desc, &pscfg);
1123 if (IS_ERR(charger->bat_ps))
1124 return dev_err_probe(dev, -EINVAL,
1125 "Battery failed to probe\n");
1126
1127 charger->chg_ps = devm_power_supply_register(&pdev->dev,
1128 &rk817_chg_desc, &pscfg);
1129 if (IS_ERR(charger->chg_ps))
1130 return dev_err_probe(dev, -EINVAL,
1131 "Charger failed to probe\n");
1132
1133 ret = power_supply_get_battery_info(charger->bat_ps,
1134 &bat_info);
1135 if (ret) {
1136 return dev_err_probe(dev, ret,
1137 "Unable to get battery info: %d\n", ret);
1138 }
1139
1140 if ((bat_info->charge_full_design_uah <= 0) ||
1141 (bat_info->voltage_min_design_uv <= 0) ||
1142 (bat_info->voltage_max_design_uv <= 0) ||
1143 (bat_info->constant_charge_voltage_max_uv <= 0) ||
1144 (bat_info->constant_charge_current_max_ua <= 0) ||
1145 (bat_info->charge_term_current_ua <= 0)) {
1146 return dev_err_probe(dev, -EINVAL,
1147 "Required bat info missing or invalid\n");
1148 }
1149
1150 charger->bat_charge_full_design_uah = bat_info->charge_full_design_uah;
1151 charger->bat_voltage_min_design_uv = bat_info->voltage_min_design_uv;
1152 charger->bat_voltage_max_design_uv = bat_info->voltage_max_design_uv;
1153
1154 /*
1155 * Has to run after power_supply_get_battery_info as it depends on some
1156 * values discovered from that routine.
1157 */
1158 ret = rk817_battery_init(charger, bat_info);
1159 if (ret)
1160 return ret;
1161
1162 power_supply_put_battery_info(charger->bat_ps, bat_info);
1163
1164 plugin_irq = platform_get_irq(pdev, 0);
1165 if (plugin_irq < 0)
1166 return plugin_irq;
1167
1168 plugout_irq = platform_get_irq(pdev, 1);
1169 if (plugout_irq < 0)
1170 return plugout_irq;
1171
1172 ret = devm_request_threaded_irq(charger->dev, plugin_irq, NULL,
1173 rk817_plug_in_isr,
1174 IRQF_TRIGGER_RISING | IRQF_ONESHOT,
1175 "rk817_plug_in", charger);
1176 if (ret) {
1177 return dev_err_probe(&pdev->dev, ret,
1178 "plug_in_irq request failed!\n");
1179 }
1180
1181 ret = devm_request_threaded_irq(charger->dev, plugout_irq, NULL,
1182 rk817_plug_out_isr,
1183 IRQF_TRIGGER_RISING | IRQF_ONESHOT,
1184 "rk817_plug_out", charger);
1185 if (ret) {
1186 return dev_err_probe(&pdev->dev, ret,
1187 "plug_out_irq request failed!\n");
1188 }
1189
1190 ret = devm_delayed_work_autocancel(&pdev->dev, &charger->work,
1191 rk817_charging_monitor);
1192 if (ret)
1193 return ret;
1194
1195 /* Force the first update immediately. */
1196 mod_delayed_work(system_wq, &charger->work, 0);
1197
--> 1198 return 0;
1199 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [bug report] power: supply: Add charger driver for Rockchip RK817
2023-05-04 10:58 Dan Carpenter
@ 2023-05-08 18:46 ` Chris Morgan
2023-05-09 4:19 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Chris Morgan @ 2023-05-08 18:46 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-pm
On Thu, May 04, 2023 at 01:58:31PM +0300, Dan Carpenter wrote:
> Hello Chris Morgan,
>
> The patch 11cb8da0189b: "power: supply: Add charger driver for
> Rockchip RK817" from Aug 26, 2022, leads to the following Smatch
> static checker warning:
>
> drivers/power/supply/rk817_charger.c:1198 rk817_charger_probe()
> warn: inconsistent refcounting 'node->kobj.kref.refcount.refs.counter':
> inc on: 1088,1105,1115,1124,1130,1136,1146,1160,1166,1170,1177,1186,1193
> dec on: 1067
>
> drivers/power/supply/rk817_charger.c
> 1048 static int rk817_charger_probe(struct platform_device *pdev)
> 1049 {
> 1050 struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent);
> 1051 struct rk817_charger *charger;
> 1052 struct device_node *node;
> 1053 struct power_supply_battery_info *bat_info;
> 1054 struct device *dev = &pdev->dev;
> 1055 struct power_supply_config pscfg = {};
> 1056 int plugin_irq, plugout_irq;
> 1057 int of_value;
> 1058 int ret;
> 1059
> 1060 node = of_get_child_by_name(dev->parent->of_node, "charger");
> 1061 if (!node)
> 1062 return -ENODEV;
> 1063
> 1064 charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
> 1065 if (!charger) {
> 1066 of_node_put(node);
>
> This error path calls of_node_put() but probably they all should.
Thank you for pointing this out. So I should probably just add a "goto"
that puts the node at the end, and direct every error to this? Just
want to confirm.
>
> 1067 return -ENOMEM;
> 1068 }
> 1069
> 1070 charger->rk808 = rk808;
> 1071
> 1072 charger->dev = &pdev->dev;
> 1073 platform_set_drvdata(pdev, charger);
> 1074
> 1075 rk817_bat_calib_vol(charger);
> 1076
> 1077 pscfg.drv_data = charger;
> 1078 pscfg.of_node = node;
> 1079
> 1080 /*
> 1081 * Get sample resistor value. Note only values of 10000 or 20000
> 1082 * microohms are allowed. Schematic for my test implementation (an
> 1083 * Odroid Go Advance) shows a 10 milliohm resistor for reference.
> 1084 */
> 1085 ret = of_property_read_u32(node, "rockchip,resistor-sense-micro-ohms",
> 1086 &of_value);
> 1087 if (ret < 0) {
> 1088 return dev_err_probe(dev, ret,
> 1089 "Error reading sample resistor value\n");
> 1090 }
> 1091 /*
> 1092 * Store as a 1 or a 2, since all we really use the value for is as a
> 1093 * divisor in some calculations.
> 1094 */
> 1095 charger->res_div = (of_value == 20000) ? 2 : 1;
> 1096
> 1097 /*
> 1098 * Get sleep enter current value. Not sure what this value is for
> 1099 * other than to help calibrate the relax threshold.
> 1100 */
> 1101 ret = of_property_read_u32(node,
> 1102 "rockchip,sleep-enter-current-microamp",
> 1103 &of_value);
> 1104 if (ret < 0) {
> 1105 return dev_err_probe(dev, ret,
> 1106 "Error reading sleep enter cur value\n");
> 1107 }
> 1108 charger->sleep_enter_current_ua = of_value;
> 1109
> 1110 /* Get sleep filter current value */
> 1111 ret = of_property_read_u32(node,
> 1112 "rockchip,sleep-filter-current-microamp",
> 1113 &of_value);
> 1114 if (ret < 0) {
> 1115 return dev_err_probe(dev, ret,
> 1116 "Error reading sleep filter cur value\n");
> 1117 }
> 1118
> 1119 charger->sleep_filter_current_ua = of_value;
> 1120
> 1121 charger->bat_ps = devm_power_supply_register(&pdev->dev,
> 1122 &rk817_bat_desc, &pscfg);
> 1123 if (IS_ERR(charger->bat_ps))
> 1124 return dev_err_probe(dev, -EINVAL,
> 1125 "Battery failed to probe\n");
> 1126
> 1127 charger->chg_ps = devm_power_supply_register(&pdev->dev,
> 1128 &rk817_chg_desc, &pscfg);
> 1129 if (IS_ERR(charger->chg_ps))
> 1130 return dev_err_probe(dev, -EINVAL,
> 1131 "Charger failed to probe\n");
> 1132
> 1133 ret = power_supply_get_battery_info(charger->bat_ps,
> 1134 &bat_info);
> 1135 if (ret) {
> 1136 return dev_err_probe(dev, ret,
> 1137 "Unable to get battery info: %d\n", ret);
> 1138 }
> 1139
> 1140 if ((bat_info->charge_full_design_uah <= 0) ||
> 1141 (bat_info->voltage_min_design_uv <= 0) ||
> 1142 (bat_info->voltage_max_design_uv <= 0) ||
> 1143 (bat_info->constant_charge_voltage_max_uv <= 0) ||
> 1144 (bat_info->constant_charge_current_max_ua <= 0) ||
> 1145 (bat_info->charge_term_current_ua <= 0)) {
> 1146 return dev_err_probe(dev, -EINVAL,
> 1147 "Required bat info missing or invalid\n");
> 1148 }
> 1149
> 1150 charger->bat_charge_full_design_uah = bat_info->charge_full_design_uah;
> 1151 charger->bat_voltage_min_design_uv = bat_info->voltage_min_design_uv;
> 1152 charger->bat_voltage_max_design_uv = bat_info->voltage_max_design_uv;
> 1153
> 1154 /*
> 1155 * Has to run after power_supply_get_battery_info as it depends on some
> 1156 * values discovered from that routine.
> 1157 */
> 1158 ret = rk817_battery_init(charger, bat_info);
> 1159 if (ret)
> 1160 return ret;
> 1161
> 1162 power_supply_put_battery_info(charger->bat_ps, bat_info);
> 1163
> 1164 plugin_irq = platform_get_irq(pdev, 0);
> 1165 if (plugin_irq < 0)
> 1166 return plugin_irq;
> 1167
> 1168 plugout_irq = platform_get_irq(pdev, 1);
> 1169 if (plugout_irq < 0)
> 1170 return plugout_irq;
> 1171
> 1172 ret = devm_request_threaded_irq(charger->dev, plugin_irq, NULL,
> 1173 rk817_plug_in_isr,
> 1174 IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> 1175 "rk817_plug_in", charger);
> 1176 if (ret) {
> 1177 return dev_err_probe(&pdev->dev, ret,
> 1178 "plug_in_irq request failed!\n");
> 1179 }
> 1180
> 1181 ret = devm_request_threaded_irq(charger->dev, plugout_irq, NULL,
> 1182 rk817_plug_out_isr,
> 1183 IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> 1184 "rk817_plug_out", charger);
> 1185 if (ret) {
> 1186 return dev_err_probe(&pdev->dev, ret,
> 1187 "plug_out_irq request failed!\n");
> 1188 }
> 1189
> 1190 ret = devm_delayed_work_autocancel(&pdev->dev, &charger->work,
> 1191 rk817_charging_monitor);
> 1192 if (ret)
> 1193 return ret;
> 1194
> 1195 /* Force the first update immediately. */
> 1196 mod_delayed_work(system_wq, &charger->work, 0);
> 1197
> --> 1198 return 0;
> 1199 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [bug report] power: supply: Add charger driver for Rockchip RK817
2023-05-08 18:46 ` Chris Morgan
@ 2023-05-09 4:19 ` Dan Carpenter
2023-05-15 16:49 ` Chris Morgan
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2023-05-09 4:19 UTC (permalink / raw)
To: Chris Morgan; +Cc: linux-pm
On Mon, May 08, 2023 at 01:46:53PM -0500, Chris Morgan wrote:
> On Thu, May 04, 2023 at 01:58:31PM +0300, Dan Carpenter wrote:
> > Hello Chris Morgan,
> >
> > The patch 11cb8da0189b: "power: supply: Add charger driver for
> > Rockchip RK817" from Aug 26, 2022, leads to the following Smatch
> > static checker warning:
> >
> > drivers/power/supply/rk817_charger.c:1198 rk817_charger_probe()
> > warn: inconsistent refcounting 'node->kobj.kref.refcount.refs.counter':
> > inc on: 1088,1105,1115,1124,1130,1136,1146,1160,1166,1170,1177,1186,1193
> > dec on: 1067
> >
> > drivers/power/supply/rk817_charger.c
> > 1048 static int rk817_charger_probe(struct platform_device *pdev)
> > 1049 {
> > 1050 struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent);
> > 1051 struct rk817_charger *charger;
> > 1052 struct device_node *node;
> > 1053 struct power_supply_battery_info *bat_info;
> > 1054 struct device *dev = &pdev->dev;
> > 1055 struct power_supply_config pscfg = {};
> > 1056 int plugin_irq, plugout_irq;
> > 1057 int of_value;
> > 1058 int ret;
> > 1059
> > 1060 node = of_get_child_by_name(dev->parent->of_node, "charger");
> > 1061 if (!node)
> > 1062 return -ENODEV;
> > 1063
> > 1064 charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
> > 1065 if (!charger) {
> > 1066 of_node_put(node);
> >
> > This error path calls of_node_put() but probably they all should.
>
> Thank you for pointing this out. So I should probably just add a "goto"
> that puts the node at the end, and direct every error to this? Just
> want to confirm.
>
Yes, please.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [bug report] power: supply: Add charger driver for Rockchip RK817
2023-05-09 4:19 ` Dan Carpenter
@ 2023-05-15 16:49 ` Chris Morgan
2023-05-17 10:52 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Chris Morgan @ 2023-05-15 16:49 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-pm
On Tue, May 09, 2023 at 07:19:44AM +0300, Dan Carpenter wrote:
> On Mon, May 08, 2023 at 01:46:53PM -0500, Chris Morgan wrote:
> > On Thu, May 04, 2023 at 01:58:31PM +0300, Dan Carpenter wrote:
> > > Hello Chris Morgan,
> > >
> > > The patch 11cb8da0189b: "power: supply: Add charger driver for
> > > Rockchip RK817" from Aug 26, 2022, leads to the following Smatch
> > > static checker warning:
> > >
> > > drivers/power/supply/rk817_charger.c:1198 rk817_charger_probe()
> > > warn: inconsistent refcounting 'node->kobj.kref.refcount.refs.counter':
> > > inc on: 1088,1105,1115,1124,1130,1136,1146,1160,1166,1170,1177,1186,1193
> > > dec on: 1067
> > >
> > > drivers/power/supply/rk817_charger.c
> > > 1048 static int rk817_charger_probe(struct platform_device *pdev)
> > > 1049 {
> > > 1050 struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent);
> > > 1051 struct rk817_charger *charger;
> > > 1052 struct device_node *node;
> > > 1053 struct power_supply_battery_info *bat_info;
> > > 1054 struct device *dev = &pdev->dev;
> > > 1055 struct power_supply_config pscfg = {};
> > > 1056 int plugin_irq, plugout_irq;
> > > 1057 int of_value;
> > > 1058 int ret;
> > > 1059
> > > 1060 node = of_get_child_by_name(dev->parent->of_node, "charger");
> > > 1061 if (!node)
> > > 1062 return -ENODEV;
> > > 1063
> > > 1064 charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
> > > 1065 if (!charger) {
> > > 1066 of_node_put(node);
> > >
> > > This error path calls of_node_put() but probably they all should.
> >
> > Thank you for pointing this out. So I should probably just add a "goto"
> > that puts the node at the end, and direct every error to this? Just
> > want to confirm.
> >
>
> Yes, please.
>
> regards,
> dan carpenter
>
Would it be possible to get details on how the error was generated with smatch?
I tried based on the instructions from https://smatch.sourceforge.net/ but I
was not able to generate the error prior to attempting my fix. For my fix I
was just going to do an of_node_put(node) right after I set
`pscfg.of_node = node` and I want to make sure that solves the problem.
Thank you.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [bug report] power: supply: Add charger driver for Rockchip RK817
2023-05-15 16:49 ` Chris Morgan
@ 2023-05-17 10:52 ` Dan Carpenter
2023-05-17 17:17 ` Chris Morgan
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2023-05-17 10:52 UTC (permalink / raw)
To: Chris Morgan; +Cc: linux-pm
On Mon, May 15, 2023 at 11:49:07AM -0500, Chris Morgan wrote:
> On Tue, May 09, 2023 at 07:19:44AM +0300, Dan Carpenter wrote:
> > On Mon, May 08, 2023 at 01:46:53PM -0500, Chris Morgan wrote:
> > > On Thu, May 04, 2023 at 01:58:31PM +0300, Dan Carpenter wrote:
> > > > Hello Chris Morgan,
> > > >
> > > > The patch 11cb8da0189b: "power: supply: Add charger driver for
> > > > Rockchip RK817" from Aug 26, 2022, leads to the following Smatch
> > > > static checker warning:
> > > >
> > > > drivers/power/supply/rk817_charger.c:1198 rk817_charger_probe()
> > > > warn: inconsistent refcounting 'node->kobj.kref.refcount.refs.counter':
> > > > inc on: 1088,1105,1115,1124,1130,1136,1146,1160,1166,1170,1177,1186,1193
> > > > dec on: 1067
> > > >
> > > > drivers/power/supply/rk817_charger.c
> > > > 1048 static int rk817_charger_probe(struct platform_device *pdev)
> > > > 1049 {
> > > > 1050 struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent);
> > > > 1051 struct rk817_charger *charger;
> > > > 1052 struct device_node *node;
> > > > 1053 struct power_supply_battery_info *bat_info;
> > > > 1054 struct device *dev = &pdev->dev;
> > > > 1055 struct power_supply_config pscfg = {};
> > > > 1056 int plugin_irq, plugout_irq;
> > > > 1057 int of_value;
> > > > 1058 int ret;
> > > > 1059
> > > > 1060 node = of_get_child_by_name(dev->parent->of_node, "charger");
> > > > 1061 if (!node)
> > > > 1062 return -ENODEV;
> > > > 1063
> > > > 1064 charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
> > > > 1065 if (!charger) {
> > > > 1066 of_node_put(node);
> > > >
> > > > This error path calls of_node_put() but probably they all should.
> > >
> > > Thank you for pointing this out. So I should probably just add a "goto"
> > > that puts the node at the end, and direct every error to this? Just
> > > want to confirm.
> > >
> >
> > Yes, please.
> >
> > regards,
> > dan carpenter
> >
>
> Would it be possible to get details on how the error was generated with smatch?
> I tried based on the instructions from https://smatch.sourceforge.net/ but I
> was not able to generate the error prior to attempting my fix. For my fix I
> was just going to do an of_node_put(node) right after I set
> `pscfg.of_node = node` and I want to make sure that solves the problem.
>
> Thank you.
Hi Chris,
This warning relies on the cross function DB. It's not hard to build
the cross function DB, but it just takes like 8 hours or something.
smatch_scripts/build_kernel_data.sh
Your patch would silence the warning, but I had assumed we would want to
hold the reference for the lifetime of the driver...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [bug report] power: supply: Add charger driver for Rockchip RK817
2023-05-17 10:52 ` Dan Carpenter
@ 2023-05-17 17:17 ` Chris Morgan
0 siblings, 0 replies; 7+ messages in thread
From: Chris Morgan @ 2023-05-17 17:17 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Chris Morgan, linux-pm
Sorry, meant to say my "temporary" fix. Again just wanted to recreate
the problem first.
Thank you.
On Wed, May 17, 2023 at 5:55 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Mon, May 15, 2023 at 11:49:07AM -0500, Chris Morgan wrote:
> > On Tue, May 09, 2023 at 07:19:44AM +0300, Dan Carpenter wrote:
> > > On Mon, May 08, 2023 at 01:46:53PM -0500, Chris Morgan wrote:
> > > > On Thu, May 04, 2023 at 01:58:31PM +0300, Dan Carpenter wrote:
> > > > > Hello Chris Morgan,
> > > > >
> > > > > The patch 11cb8da0189b: "power: supply: Add charger driver for
> > > > > Rockchip RK817" from Aug 26, 2022, leads to the following Smatch
> > > > > static checker warning:
> > > > >
> > > > > drivers/power/supply/rk817_charger.c:1198 rk817_charger_probe()
> > > > > warn: inconsistent refcounting 'node->kobj.kref.refcount.refs.counter':
> > > > > inc on: 1088,1105,1115,1124,1130,1136,1146,1160,1166,1170,1177,1186,1193
> > > > > dec on: 1067
> > > > >
> > > > > drivers/power/supply/rk817_charger.c
> > > > > 1048 static int rk817_charger_probe(struct platform_device *pdev)
> > > > > 1049 {
> > > > > 1050 struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent);
> > > > > 1051 struct rk817_charger *charger;
> > > > > 1052 struct device_node *node;
> > > > > 1053 struct power_supply_battery_info *bat_info;
> > > > > 1054 struct device *dev = &pdev->dev;
> > > > > 1055 struct power_supply_config pscfg = {};
> > > > > 1056 int plugin_irq, plugout_irq;
> > > > > 1057 int of_value;
> > > > > 1058 int ret;
> > > > > 1059
> > > > > 1060 node = of_get_child_by_name(dev->parent->of_node, "charger");
> > > > > 1061 if (!node)
> > > > > 1062 return -ENODEV;
> > > > > 1063
> > > > > 1064 charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
> > > > > 1065 if (!charger) {
> > > > > 1066 of_node_put(node);
> > > > >
> > > > > This error path calls of_node_put() but probably they all should.
> > > >
> > > > Thank you for pointing this out. So I should probably just add a "goto"
> > > > that puts the node at the end, and direct every error to this? Just
> > > > want to confirm.
> > > >
> > >
> > > Yes, please.
> > >
> > > regards,
> > > dan carpenter
> > >
> >
> > Would it be possible to get details on how the error was generated with smatch?
> > I tried based on the instructions from https://smatch.sourceforge.net/ but I
> > was not able to generate the error prior to attempting my fix. For my fix I
> > was just going to do an of_node_put(node) right after I set
> > `pscfg.of_node = node` and I want to make sure that solves the problem.
> >
> > Thank you.
>
> Hi Chris,
>
> This warning relies on the cross function DB. It's not hard to build
> the cross function DB, but it just takes like 8 hours or something.
> smatch_scripts/build_kernel_data.sh
>
> Your patch would silence the warning, but I had assumed we would want to
> hold the reference for the lifetime of the driver...
>
> regards,
> dan carpenter
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-05-17 17:17 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-10 13:14 [bug report] power: supply: Add charger driver for Rockchip RK817 Dan Carpenter
-- strict thread matches above, loose matches on Subject: below --
2023-05-04 10:58 Dan Carpenter
2023-05-08 18:46 ` Chris Morgan
2023-05-09 4:19 ` Dan Carpenter
2023-05-15 16:49 ` Chris Morgan
2023-05-17 10:52 ` Dan Carpenter
2023-05-17 17:17 ` Chris Morgan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox