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

[Cristian Marussi <cristian.marussi@arm.com>]

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

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