* [bug report] firmware: arm_scmi: Add per-channel Raw injection support
@ 2023-01-17 10:43 Dan Carpenter
2023-01-17 14:30 ` Cristian Marussi
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2023-01-17 10:43 UTC (permalink / raw)
To: cristian.marussi; +Cc: linux-arm-kernel, Matthew Wilcox
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] firmware: arm_scmi: Add per-channel Raw injection support
2023-01-17 10:43 [bug report] firmware: arm_scmi: Add per-channel Raw injection support Dan Carpenter
@ 2023-01-17 14:30 ` Cristian Marussi
2023-01-17 14:49 ` Matthew Wilcox
0 siblings, 1 reply; 4+ messages in thread
From: Cristian Marussi @ 2023-01-17 14:30 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-arm-kernel, Matthew Wilcox
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] firmware: arm_scmi: Add per-channel Raw injection support
2023-01-17 14:30 ` Cristian Marussi
@ 2023-01-17 14:49 ` Matthew Wilcox
2023-01-18 12:26 ` Cristian Marussi
0 siblings, 1 reply; 4+ messages in thread
From: Matthew Wilcox @ 2023-01-17 14:49 UTC (permalink / raw)
To: Cristian Marussi; +Cc: Dan Carpenter, linux-arm-kernel
On Tue, Jan 17, 2023 at 02:30:43PM +0000, Cristian Marussi wrote:
> > 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)
The "range of one" idiom is fairly frequently used, but generally it is
an indication that you want to use the XArray API instead of the IDR API,
which is deprecated in any case.
I've given up tilting at that windmill as I'm more concerned with the
folio work, but it'd be nice if you could convert from IDR to XArray here.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] firmware: arm_scmi: Add per-channel Raw injection support
2023-01-17 14:49 ` Matthew Wilcox
@ 2023-01-18 12:26 ` Cristian Marussi
0 siblings, 0 replies; 4+ messages in thread
From: Cristian Marussi @ 2023-01-18 12:26 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Dan Carpenter, linux-arm-kernel
On Tue, Jan 17, 2023 at 02:49:31PM +0000, Matthew Wilcox wrote:
> On Tue, Jan 17, 2023 at 02:30:43PM +0000, Cristian Marussi wrote:
> > > 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)
>
> The "range of one" idiom is fairly frequently used, but generally it is
> an indication that you want to use the XArray API instead of the IDR API,
> which is deprecated in any case.
>
> I've given up tilting at that windmill as I'm more concerned with the
> folio work, but it'd be nice if you could convert from IDR to XArray here.
Hi,
thanks for the hint Matthew, never used XArrays till now.
I'll reworked the above with XArray in:
https://lore.kernel.org/linux-arm-kernel/20230118121426.492864-17-cristian.marussi@arm.com/T/#u
Thanks,
Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-18 12:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-17 10:43 [bug report] firmware: arm_scmi: Add per-channel Raw injection support Dan Carpenter
2023-01-17 14:30 ` Cristian Marussi
2023-01-17 14:49 ` Matthew Wilcox
2023-01-18 12:26 ` Cristian Marussi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox