[bug report] firmware: arm_scmi: Add per-channel Raw injection support

From: Dan Carpenter <error27@gmail.com>
To: cristian.marussi@arm.com
Cc: linux-arm-kernel@lists.infradead.org,
	Matthew Wilcox <willy@infradead.org>
Subject: [bug report] firmware: arm_scmi: Add per-channel Raw injection support
Date: Tue, 17 Jan 2023 13:43:44 +0300	[thread overview]
Message-ID: <Y8Z74DXkEqPW+iHF@kili> (raw)

Hello Cristian Marussi,

The patch 0ae95d6e8b75: "firmware: arm_scmi: Add per-channel Raw
injection support" from Jan 13, 2023, leads to the following Smatch
static checker warning:

	drivers/firmware/arm_scmi/raw_mode.c:1115 scmi_raw_mode_setup()
	warn: 'q' is an error pointer or valid

drivers/firmware/arm_scmi/raw_mode.c
    1087 static int scmi_raw_mode_setup(struct scmi_raw_mode_info *raw,
    1088                                u8 *channels, int num_chans)
    1089 {
    1090         int ret, idx;
    1091         void *gid;
    1092         struct device *dev = raw->handle->dev;
    1093 
    1094         gid = devres_open_group(dev, NULL, GFP_KERNEL);
    1095         if (!gid)
    1096                 return -ENOMEM;
    1097 
    1098         for (idx = 0; idx < SCMI_RAW_MAX_QUEUE; idx++) {
    1099                 raw->q[idx] = scmi_raw_queue_init(raw);
    1100                 if (IS_ERR(raw->q[idx])) {
    1101                         ret = PTR_ERR(raw->q[idx]);
    1102                         goto err;
    1103                 }
    1104         }
    1105 
    1106         if (num_chans > 1) {
    1107                 int i;
    1108 
    1109                 idr_init(&raw->queues_idr);
    1110 
    1111                 for (i = 0; i < num_chans; i++) {
    1112                         struct scmi_raw_queue *q;
    1113 
    1114                         q = scmi_raw_queue_init(raw);
--> 1115                         if (!q)
    1116                                 continue;

This should be something like:

	if (IS_ERR(q)) {
		ret = PTR_ERR(q);
		goto err;
	}

    1117 
    1118                         ret = idr_alloc(&raw->queues_idr, q,
    1119                                         channels[i], channels[i] + 1,
    1120                                         GFP_KERNEL);
    1121                         if (ret != channels[i])
    1122                                 dev_err(dev,
    1123                                         "Fail to allocate Raw queue 0x%02X\n",
    1124                                         channels[i]);

Heh.  I've never seen anyone use an IDR range of one value before...
Just printing an error is not correct error handling.  I think these
IDR values have to be freed on the error path?  Or is there a devm_
trick happening here?

    1125                 }
    1126         }
    1127 
    1128         ret = scmi_xfer_raw_worker_init(raw);
    1129         if (ret)
    1130                 goto err;
    1131 
    1132         devres_close_group(dev, gid);
    1133         raw->gid = gid;
    1134 
    1135         return 0;
    1136 
    1137 err:
    1138         devres_release_group(dev, gid);
    1139         return ret;
    1140 }

regards,
dan carpenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel