Hello Bryan O'Donoghue,
The patch 23aa4f0cd327: "media: qcom: camss: Move VFE power-domain
specifics into vfe.c" from Nov 23, 2023 (linux-next), leads to the
following Smatch static checker warning:
drivers/media/platform/qcom/camss/camss.c:1572 camss_configure_pd()
error: 'camss->genpd' dereferencing possible ERR_PTR()
drivers/media/platform/qcom/camss/camss.c
1495 static int camss_configure_pd(struct camss *camss)
1496 {
1497 const struct camss_resources *res = camss->res;
1498 struct device *dev = camss->dev;
1499 int vfepd_num;
1500 int i;
1501 int ret;
1502
1503 camss->genpd_num = of_count_phandle_with_args(dev->of_node,
1504 "power-domains",
1505 "#power-domain-cells");
1506 if (camss->genpd_num < 0) {
1507 dev_err(dev, "Power domains are not defined for camss\n");
1508 return camss->genpd_num;
1509 }
1510
1511 /*
1512 * If a platform device has just one power domain, then it is attached
1513 * at platform_probe() level, thus there shall be no need and even no
1514 * option to attach it again, this is the case for CAMSS on MSM8916.
1515 */
1516 if (camss->genpd_num == 1)
1517 return 0;
1518
1519 /* count the # of VFEs which have flagged power-domain */
1520 for (vfepd_num = i = 0; i < camss->res->vfe_num; i++) {
1521 if (res->vfe_res[i].has_pd)
1522 vfepd_num++;
1523 }
1524
1525 /*
1526 * If the number of power-domains is greater than the number of VFEs
1527 * then the additional power-domain is for the entire CAMSS block.
1528 */
1529 if (!(camss->genpd_num > vfepd_num))
1530 return 0;
1531
1532 /*
1533 * If a power-domain name is defined try to use it.
1534 * It is possible we are running a new kernel with an old dtb so
1535 * fallback to indexes even if a pd_name is defined but not found.
1536 */
1537 if (camss->res->pd_name) {
1538 camss->genpd = dev_pm_domain_attach_by_name(camss->dev,
1539 camss->res->pd_name);
1540 if (IS_ERR(camss->genpd)) {
1541 ret = PTR_ERR(camss->genpd);
1542 goto fail_pm;
camss->genpd is an error pointer here
1543 }
1544 }
1545
1546 if (!camss->genpd) {
1547 /*
1548 * Legacy magic index. TITAN_TOP GDSC must be the last
1549 * item in the power-domain list.
1550 */
1551 camss->genpd = dev_pm_domain_attach_by_id(camss->dev,
1552 camss->genpd_num - 1);
1553 }
1554 if (IS_ERR_OR_NULL(camss->genpd)) {
This can only be NULL if CONFIG_PM is disabled. Generally, the
guidelines here would be to say that we should allow the driver to work
without CONFIG_PM. In other words, add NULL checks as necessary. I
only looked at this file and camss-vfe.c. This file doesn't need any
NULL checks and camss-vfe.c already seems to have appropriate NULL
checking.
But if this driver really can't work without CONFIG_PM then the correct
thing is to add a dependency to the Kconfig. Waiting until probe to
check for NULL and then refusing is wrong.
1555 if (!camss->genpd)
1556 ret = -ENODEV;
1557 else
1558 ret = PTR_ERR(camss->genpd);
1559 goto fail_pm;
1560 }
1561 camss->genpd_link = device_link_add(camss->dev, camss->genpd,
1562 DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME |
1563 DL_FLAG_RPM_ACTIVE);
1564 if (!camss->genpd_link) {
1565 ret = -EINVAL;
1566 goto fail_pm;
1567 }
1568
1569 return 0;
1570
1571 fail_pm:
--> 1572 dev_pm_domain_detach(camss->genpd, true);
^^^^^^^^^^^^
The error pointer will crash here. (NULL is not an issue because in
that situation dev_pm_domain_detach() is a no-op).
1573
1574 return ret;
1575 }
regards,
dan carpenter
