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

Cristian Marussi
2023-01-17T14:31:59Z

On Tue, Jan 17, 2023 at 01:43:44PM +0300, Dan Carpenter wrote:
> Hello Cristian Marussi,
>
Hi Dan,

> 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;
> }
>

Beside the (sadly for me usual) bug on error-check the idea was here really
not to bail out if these additional per-channels debugfs entries fails to be
created since I have anyway a bare minimum support available so I can
carry on.

> 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...

Well, the choice was between wasting an hashtable or a radix-tree IDR for a
few mapping channel--->q and I went for the 1-range IDR which is already
used in SCMI stack to map various refs to channel numbers.
(I avoid in general to put a simple plain 256 array on the stack that can
lead to stack-size issues especially while compiling on armv7)

> 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?
>

This comes from the above (maybe ill) choice of not bailing out on error
for these entries; everything is freed on final exit and kept unused on errors
But it can lead indeed to a situation in which only some entries are unexpectedly
not working, so it has to better handled.

I'll fix and repost.

Thanks,
Cristian