* [PATCH] staging: greybus: codecs: fix type confusion with dedicated list iterator variable
@ 2022-03-19 20:20 Jakob Koschel
2022-03-21 8:48 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Jakob Koschel @ 2022-03-19 20:20 UTC (permalink / raw)
To: Vaibhav Agarwal
Cc: Jakob Koschel, linux-kernel, greybus-dev, linux-staging,
Mark Greer, Johan Hovold, Alex Elder, Greg Kroah-Hartman,
Mike Rapoport, Brian Johannesmeyer, Cristiano Giuffrida, Bos,
H.J.
If the list does not exit early then data == NULL and 'module' does not
point to a valid list element.
Using 'module' in such a case is not valid and was therefore removed.
In preparation to limit the scope of the list iterator to the list
traversal loop, use a dedicated pointer pointing to the found element [1].
Link: https://lore.kernel.org/all/YhdfEIwI4EdtHdym@kroah.com/
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
drivers/staging/greybus/audio_codec.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
index b589cf6b1d03..0f50d1e51e2c 100644
--- a/drivers/staging/greybus/audio_codec.c
+++ b/drivers/staging/greybus/audio_codec.c
@@ -497,7 +497,7 @@ static int gbcodec_prepare(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
int ret;
- struct gbaudio_module_info *module;
+ struct gbaudio_module_info *module = NULL, *iter;
struct gbaudio_data_connection *data;
struct gb_bundle *bundle;
struct gbaudio_codec_info *codec = dev_get_drvdata(dai->dev);
@@ -511,11 +511,13 @@ static int gbcodec_prepare(struct snd_pcm_substream *substream,
return -ENODEV;
}
- list_for_each_entry(module, &codec->module_list, list) {
+ list_for_each_entry(iter, &codec->module_list, list) {
/* find the dai */
- data = find_data(module, dai->id);
- if (data)
+ data = find_data(iter, dai->id);
+ if (data) {
+ module = iter;
break;
+ }
}
if (!data) {
dev_err(dai->dev, "DATA connection missing\n");
@@ -563,7 +565,7 @@ static int gbcodec_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
{
int ret;
struct gbaudio_data_connection *data;
- struct gbaudio_module_info *module;
+ struct gbaudio_module_info *module = NULL, *iter;
struct gb_bundle *bundle;
struct gbaudio_codec_info *codec = dev_get_drvdata(dai->dev);
struct gbaudio_stream_params *params;
@@ -592,15 +594,17 @@ static int gbcodec_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
return ret;
}
- list_for_each_entry(module, &codec->module_list, list) {
+ list_for_each_entry(iter, &codec->module_list, list) {
/* find the dai */
- data = find_data(module, dai->id);
- if (data)
+ data = find_data(iter, dai->id);
+ if (data) {
+ module = iter;
break;
+ }
}
if (!data) {
- dev_err(dai->dev, "%s:%s DATA connection missing\n",
- dai->name, module->name);
+ dev_err(dai->dev, "%s DATA connection missing\n",
+ dai->name);
mutex_unlock(&codec->lock);
return -ENODEV;
}
base-commit: 34e047aa16c0123bbae8e2f6df33e5ecc1f56601
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] staging: greybus: codecs: fix type confusion with dedicated list iterator variable
2022-03-19 20:20 [PATCH] staging: greybus: codecs: fix type confusion with dedicated list iterator variable Jakob Koschel
@ 2022-03-21 8:48 ` Dan Carpenter
2022-03-21 9:06 ` Jakob Koschel
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2022-03-21 8:48 UTC (permalink / raw)
To: Jakob Koschel
Cc: Vaibhav Agarwal, linux-kernel, greybus-dev, linux-staging,
Mark Greer, Johan Hovold, Alex Elder, Greg Kroah-Hartman,
Mike Rapoport, Brian Johannesmeyer, Cristiano Giuffrida, Bos,
H.J.
The subject says that it fixes a bug but it does not.
On Sat, Mar 19, 2022 at 09:20:58PM +0100, Jakob Koschel wrote:
> If the list does not exit early then data == NULL and 'module' does not
> point to a valid list element.
> Using 'module' in such a case is not valid and was therefore removed.
This paragraph is confusing jumble words. Just say: "This code is fine".
>
> In preparation to limit the scope of the list iterator to the list
> traversal loop, use a dedicated pointer pointing to the found element [1].
This paragraph is the information we need. Just add something like
"This patch has no effect on runtime".
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: greybus: codecs: fix type confusion with dedicated list iterator variable
2022-03-21 8:48 ` Dan Carpenter
@ 2022-03-21 9:06 ` Jakob Koschel
2022-03-21 9:21 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Jakob Koschel @ 2022-03-21 9:06 UTC (permalink / raw)
To: Dan Carpenter
Cc: Vaibhav Agarwal, Linux Kernel Mailing List, greybus-dev,
linux-staging, Mark Greer, Johan Hovold, Alex Elder,
Greg Kroah-Hartman, Mike Rapport, Brian Johannesmeyer,
Cristiano Giuffrida, Bos, H.J.
> On 21. Mar 2022, at 09:48, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> The subject says that it fixes a bug but it does not.
Thank you for your review!
I don't agree that this doesn't fix a bug:
> + }
> }
> if (!data) {
> - dev_err(dai->dev, "%s:%s DATA connection missing\n",
> - dai->name, module->name);
Using 'module' when data == NULL is *guaranteed* to be a type confused
bogus pointer. It fundamentally can never be correct.
If I should still change the wording please let me know.
> + dev_err(dai->dev, "%s DATA connection missing\n",
> + dai->name);
> mutex_unlock(&codec->lock);
> return -ENODEV;
> }
>
> On Sat, Mar 19, 2022 at 09:20:58PM +0100, Jakob Koschel wrote:
>> If the list does not exit early then data == NULL and 'module' does not
>> point to a valid list element.
>> Using 'module' in such a case is not valid and was therefore removed.
>
> This paragraph is confusing jumble words. Just say: "This code is fine".
>
>>
>> In preparation to limit the scope of the list iterator to the list
>> traversal loop, use a dedicated pointer pointing to the found element [1].
>
> This paragraph is the information we need. Just add something like
> "This patch has no effect on runtime".
As mentioned above, this code effects runtime (in one out of the two cases).
>
> regards,
> dan carpenter
Thanks,
Jakob
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] staging: greybus: codecs: fix type confusion with dedicated list iterator variable
2022-03-21 9:06 ` Jakob Koschel
@ 2022-03-21 9:21 ` Dan Carpenter
2022-03-21 9:27 ` Jakob Koschel
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2022-03-21 9:21 UTC (permalink / raw)
To: Jakob Koschel
Cc: Vaibhav Agarwal, Linux Kernel Mailing List, greybus-dev,
linux-staging, Mark Greer, Johan Hovold, Alex Elder,
Greg Kroah-Hartman, Mike Rapport, Brian Johannesmeyer,
Cristiano Giuffrida, Bos, H.J.
On Mon, Mar 21, 2022 at 10:06:13AM +0100, Jakob Koschel wrote:
>
> > On 21. Mar 2022, at 09:48, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > The subject says that it fixes a bug but it does not.
>
> Thank you for your review!
>
> I don't agree that this doesn't fix a bug:
>
> > + }
> > }
> > if (!data) {
> > - dev_err(dai->dev, "%s:%s DATA connection missing\n",
> > - dai->name, module->name);
>
> Using 'module' when data == NULL is *guaranteed* to be a type confused
> bogus pointer. It fundamentally can never be correct.
Ah. I did not read all the way to the end of the patch.
The bugfix needs to be sent as it's own patch. Just the one liner. It
needs a fixes tag as well.
[PATCH] staging: greybus: fix Oops in error message
The "module" pointer is invalid here. It's the list iterator and we
exited the loop without finding a valid entry.
Fixes: 6dd67645f22c ("greybus: audio: Use single codec driver registration")
Signed-off-by: You
if (!data) {
- dev_err(dai->dev, "%s:%s DATA connection missing\n",
- dai->name, module->name);
+ dev_err(dai->dev, "%s DATA connection missing\n",
+ dai->name);
mutex_unlock(&codec->lock);
We're happy to apply the other stuff as well, but we don't mix cleanups
and bug fixes like that.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] staging: greybus: codecs: fix type confusion with dedicated list iterator variable
2022-03-21 9:21 ` Dan Carpenter
@ 2022-03-21 9:27 ` Jakob Koschel
2022-03-21 12:18 ` [greybus-dev] " Alex Elder
0 siblings, 1 reply; 6+ messages in thread
From: Jakob Koschel @ 2022-03-21 9:27 UTC (permalink / raw)
To: Dan Carpenter
Cc: Vaibhav Agarwal, Linux Kernel Mailing List, greybus-dev,
linux-staging, Mark Greer, Johan Hovold, Alex Elder,
Greg Kroah-Hartman, Mike Rapport, Brian Johannesmeyer,
Cristiano Giuffrida, Bos, H.J.
> On 21. Mar 2022, at 10:21, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, Mar 21, 2022 at 10:06:13AM +0100, Jakob Koschel wrote:
>>
>>> On 21. Mar 2022, at 09:48, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>>
>>> The subject says that it fixes a bug but it does not.
>>
>> Thank you for your review!
>>
>> I don't agree that this doesn't fix a bug:
>>
>>> + }
>>> }
>>> if (!data) {
>>> - dev_err(dai->dev, "%s:%s DATA connection missing\n",
>>> - dai->name, module->name);
>>
>> Using 'module' when data == NULL is *guaranteed* to be a type confused
>> bogus pointer. It fundamentally can never be correct.
>
> Ah. I did not read all the way to the end of the patch.
>
> The bugfix needs to be sent as it's own patch. Just the one liner. It
> needs a fixes tag as well.
>
> [PATCH] staging: greybus: fix Oops in error message
>
> The "module" pointer is invalid here. It's the list iterator and we
> exited the loop without finding a valid entry.
>
> Fixes: 6dd67645f22c ("greybus: audio: Use single codec driver registration")
> Signed-off-by: You
>
> if (!data) {
> - dev_err(dai->dev, "%s:%s DATA connection missing\n",
> - dai->name, module->name);
> + dev_err(dai->dev, "%s DATA connection missing\n",
> + dai->name);
> mutex_unlock(&codec->lock);
>
> We're happy to apply the other stuff as well, but we don't mix cleanups
> and bug fixes like that.
ok great, I'll separate and resubmit both. Thanks!
>
> regards,
> dan carpenter
Jakob
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [greybus-dev] Re: [PATCH] staging: greybus: codecs: fix type confusion with dedicated list iterator variable
2022-03-21 9:27 ` Jakob Koschel
@ 2022-03-21 12:18 ` Alex Elder
0 siblings, 0 replies; 6+ messages in thread
From: Alex Elder @ 2022-03-21 12:18 UTC (permalink / raw)
To: Jakob Koschel, Dan Carpenter
Cc: Linux Kernel Mailing List, greybus-dev, linux-staging,
Johan Hovold, Alex Elder, Mike Rapport, Brian Johannesmeyer,
Cristiano Giuffrida, Bos, H.J.
On 3/21/22 4:27 AM, Jakob Koschel wrote:
>
I just released some messages that were marked as spam. So
this looks to me like it's already been seen on the list.
I'm not sure why this happens but if it seems like deja vu,
you're not imagining things. Please know that this could
happen from time to time, but I'll see if I can find out
how to avoid it.
-Alex
>> On 21. Mar 2022, at 10:21, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>
>> On Mon, Mar 21, 2022 at 10:06:13AM +0100, Jakob Koschel wrote:
>>>
>>>> On 21. Mar 2022, at 09:48, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>>>
>>>> The subject says that it fixes a bug but it does not.
>>>
>>> Thank you for your review!
>>>
>>> I don't agree that this doesn't fix a bug:
>>>
>>>> + }
>>>> }
>>>> if (!data) {
>>>> - dev_err(dai->dev, "%s:%s DATA connection missing\n",
>>>> - dai->name, module->name);
>>>
>>> Using 'module' when data == NULL is *guaranteed* to be a type confused
>>> bogus pointer. It fundamentally can never be correct.
>>
>> Ah. I did not read all the way to the end of the patch.
>>
>> The bugfix needs to be sent as it's own patch. Just the one liner. It
>> needs a fixes tag as well.
>>
>> [PATCH] staging: greybus: fix Oops in error message
>>
>> The "module" pointer is invalid here. It's the list iterator and we
>> exited the loop without finding a valid entry.
>>
>> Fixes: 6dd67645f22c ("greybus: audio: Use single codec driver registration")
>> Signed-off-by: You
>>
>> if (!data) {
>> - dev_err(dai->dev, "%s:%s DATA connection missing\n",
>> - dai->name, module->name);
>> + dev_err(dai->dev, "%s DATA connection missing\n",
>> + dai->name);
>> mutex_unlock(&codec->lock);
>>
>> We're happy to apply the other stuff as well, but we don't mix cleanups
>> and bug fixes like that.
>
> ok great, I'll separate and resubmit both. Thanks!
>
>>
>> regards,
>> dan carpenter
>
> Jakob
>
> _______________________________________________
> greybus-dev mailing list -- greybus-dev@lists.linaro.org
> To unsubscribe send an email to greybus-dev-leave@lists.linaro.org
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-21 12:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-19 20:20 [PATCH] staging: greybus: codecs: fix type confusion with dedicated list iterator variable Jakob Koschel
2022-03-21 8:48 ` Dan Carpenter
2022-03-21 9:06 ` Jakob Koschel
2022-03-21 9:21 ` Dan Carpenter
2022-03-21 9:27 ` Jakob Koschel
2022-03-21 12:18 ` [greybus-dev] " Alex Elder
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.