* [bug report] ALSA: hda - Don't register a cb func if it is registered already
@ 2020-10-21 12:19 Dan Carpenter
2020-10-21 14:21 ` Hui Wang
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2020-10-21 12:19 UTC (permalink / raw)
To: hui.wang; +Cc: alsa-devel
Hello Hui Wang,
The patch f4794c6064a8: "ALSA: hda - Don't register a cb func if it
is registered already" from Sep 30, 2020, leads to the following
static checker warning:
sound/pci/hda/patch_sigmatel.c:3075 stac92hd71bxx_fixup_hp_m4()
warn: 'jack' can also be NULL
sound/pci/hda/patch_sigmatel.c
3069 /* Enable VREF power saving on GPIO1 detect */
3070 snd_hda_codec_write_cache(codec, codec->core.afg, 0,
3071 AC_VERB_SET_GPIO_UNSOLICITED_RSP_MASK, 0x02);
3072 jack = snd_hda_jack_detect_enable_callback(codec, codec->core.afg,
3073 stac_vref_event);
Originally snd_hda_jack_detect_enable_callback() would not return NULL
here.
3074 if (!IS_ERR(jack))
3075 jack->private_data = 0x02;
3076
3077 spec->gpio_mask |= 0x02;
But now we have this:
sound/pci/hda/hda_jack.c
301 struct hda_jack_callback *
302 snd_hda_jack_detect_enable_callback_mst(struct hda_codec *codec, hda_nid_t nid,
303 int dev_id, hda_jack_callback_fn func)
304 {
305 struct hda_jack_tbl *jack;
306 struct hda_jack_callback *callback = NULL;
307 int err;
308
309 jack = snd_hda_jack_tbl_new(codec, nid, dev_id);
310 if (!jack)
311 return ERR_PTR(-ENOMEM);
312 if (func && !func_is_already_in_callback_list(jack, func)) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We only allocate callback if there isn't one already.
313 callback = kzalloc(sizeof(*callback), GFP_KERNEL);
314 if (!callback)
315 return ERR_PTR(-ENOMEM);
316 callback->func = func;
317 callback->nid = jack->nid;
318 callback->dev_id = jack->dev_id;
319 callback->next = jack->callback;
320 jack->callback = callback;
321 }
322
323 if (jack->jack_detect)
324 return callback; /* already registered */
^^^^^^^^^^^^^^^
So presumably this should be jack->callback
325 jack->jack_detect = 1;
326 if (codec->jackpoll_interval > 0)
327 return callback; /* No unsol if we're polling instead */
^^^^^^^^^^^^^^^^
328 err = snd_hda_codec_write_cache(codec, nid, 0,
329 AC_VERB_SET_UNSOLICITED_ENABLE,
330 AC_USRSP_EN | jack->tag);
331 if (err < 0)
332 return ERR_PTR(err);
333 return callback;
^^^^^^^^^^^^^^^^
And these as well.
334 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] ALSA: hda - Don't register a cb func if it is registered already
2020-10-21 12:19 [bug report] ALSA: hda - Don't register a cb func if it is registered already Dan Carpenter
@ 2020-10-21 14:21 ` Hui Wang
2020-10-21 15:03 ` Hui Wang
0 siblings, 1 reply; 7+ messages in thread
From: Hui Wang @ 2020-10-21 14:21 UTC (permalink / raw)
To: Dan Carpenter; +Cc: alsa-devel
On 10/21/20 8:19 PM, Dan Carpenter wrote:
> Hello Hui Wang,
>
> The patch f4794c6064a8: "ALSA: hda - Don't register a cb func if it
> is registered already" from Sep 30, 2020, leads to the following
> static checker warning:
>
> sound/pci/hda/patch_sigmatel.c:3075 stac92hd71bxx_fixup_hp_m4()
> warn: 'jack' can also be NULL
>
> sound/pci/hda/patch_sigmatel.c
> 3069 /* Enable VREF power saving on GPIO1 detect */
> 3070 snd_hda_codec_write_cache(codec, codec->core.afg, 0,
> 3071 AC_VERB_SET_GPIO_UNSOLICITED_RSP_MASK, 0x02);
> 3072 jack = snd_hda_jack_detect_enable_callback(codec, codec->core.afg,
> 3073 stac_vref_event);
>
> Originally snd_hda_jack_detect_enable_callback() would not return NULL
> here.
>
> 3074 if (!IS_ERR(jack))
> 3075 jack->private_data = 0x02;
> 3076
> 3077 spec->gpio_mask |= 0x02;
>
> But now we have this:
>
> sound/pci/hda/hda_jack.c
> 301 struct hda_jack_callback *
> 302 snd_hda_jack_detect_enable_callback_mst(struct hda_codec *codec, hda_nid_t nid,
> 303 int dev_id, hda_jack_callback_fn func)
> 304 {
> 305 struct hda_jack_tbl *jack;
> 306 struct hda_jack_callback *callback = NULL;
> 307 int err;
> 308
> 309 jack = snd_hda_jack_tbl_new(codec, nid, dev_id);
> 310 if (!jack)
> 311 return ERR_PTR(-ENOMEM);
> 312 if (func && !func_is_already_in_callback_list(jack, func)) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> We only allocate callback if there isn't one already.
Yes, indeed, this is a problem.
>
> 313 callback = kzalloc(sizeof(*callback), GFP_KERNEL);
> 314 if (!callback)
> 315 return ERR_PTR(-ENOMEM);
> 316 callback->func = func;
> 317 callback->nid = jack->nid;
> 318 callback->dev_id = jack->dev_id;
> 319 callback->next = jack->callback;
> 320 jack->callback = callback;
> 321 }
> 322
> 323 if (jack->jack_detect)
> 324 return callback; /* already registered */
> ^^^^^^^^^^^^^^^
> So presumably this should be jack->callback
Looks like it is also not correct to return the jack->callback. Need to
take some time to write a fix for it.
Thanks for reporting the issue.
>
>
> 325 jack->jack_detect = 1;
> 326 if (codec->jackpoll_interval > 0)
> 327 return callback; /* No unsol if we're polling instead */
> ^^^^^^^^^^^^^^^^
>
> 328 err = snd_hda_codec_write_cache(codec, nid, 0,
> 329 AC_VERB_SET_UNSOLICITED_ENABLE,
> 330 AC_USRSP_EN | jack->tag);
> 331 if (err < 0)
> 332 return ERR_PTR(err);
> 333 return callback;
> ^^^^^^^^^^^^^^^^
> And these as well.
>
> 334 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] ALSA: hda - Don't register a cb func if it is registered already
2020-10-21 14:21 ` Hui Wang
@ 2020-10-21 15:03 ` Hui Wang
2020-10-21 15:59 ` Takashi Iwai
2020-10-21 17:27 ` Dan Carpenter
0 siblings, 2 replies; 7+ messages in thread
From: Hui Wang @ 2020-10-21 15:03 UTC (permalink / raw)
To: Dan Carpenter; +Cc: alsa-devel
Looks like this will not bring problem on patch_sigmatel.c, NULL and
valid kernel pointer are same for IS_ERR(), they will not make IS_ERR()
come true.
On 10/21/20 10:21 PM, Hui Wang wrote:
>
> On 10/21/20 8:19 PM, Dan Carpenter wrote:
>> Hello Hui Wang,
>>
>> The patch f4794c6064a8: "ALSA: hda - Don't register a cb func if it
>> is registered already" from Sep 30, 2020, leads to the following
>> static checker warning:
>>
>> sound/pci/hda/patch_sigmatel.c:3075 stac92hd71bxx_fixup_hp_m4()
>> warn: 'jack' can also be NULL
>>
>> sound/pci/hda/patch_sigmatel.c
>> 3069 /* Enable VREF power saving on GPIO1 detect */
>> 3070 snd_hda_codec_write_cache(codec, codec->core.afg, 0,
>> 3071 AC_VERB_SET_GPIO_UNSOLICITED_RSP_MASK, 0x02);
>> 3072 jack = snd_hda_jack_detect_enable_callback(codec,
>> codec->core.afg,
>> 3073 stac_vref_event);
>>
>> Originally snd_hda_jack_detect_enable_callback() would not return NULL
>> here.
>>
>> 3074 if (!IS_ERR(jack))
>> 3075 jack->private_data = 0x02;
>> 3076
>> 3077 spec->gpio_mask |= 0x02;
>>
>> But now we have this:
>>
>> sound/pci/hda/hda_jack.c
>> 301 struct hda_jack_callback *
>> 302 snd_hda_jack_detect_enable_callback_mst(struct hda_codec
>> *codec, hda_nid_t nid,
>> 303 int dev_id,
>> hda_jack_callback_fn func)
>> 304 {
>> 305 struct hda_jack_tbl *jack;
>> 306 struct hda_jack_callback *callback = NULL;
>> 307 int err;
>> 308
>> 309 jack = snd_hda_jack_tbl_new(codec, nid, dev_id);
>> 310 if (!jack)
>> 311 return ERR_PTR(-ENOMEM);
>> 312 if (func && !func_is_already_in_callback_list(jack,
>> func)) {
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> We only allocate callback if there isn't one already.
> Yes, indeed, this is a problem.
>>
>> 313 callback = kzalloc(sizeof(*callback),
>> GFP_KERNEL);
>> 314 if (!callback)
>> 315 return ERR_PTR(-ENOMEM);
>> 316 callback->func = func;
>> 317 callback->nid = jack->nid;
>> 318 callback->dev_id = jack->dev_id;
>> 319 callback->next = jack->callback;
>> 320 jack->callback = callback;
>> 321 }
>> 322
>> 323 if (jack->jack_detect)
>> 324 return callback; /* already registered */
>> ^^^^^^^^^^^^^^^
>> So presumably this should be jack->callback
>
> Looks like it is also not correct to return the jack->callback. Need
> to take some time to write a fix for it.
>
>
> Thanks for reporting the issue.
>
>>
>>
>> 325 jack->jack_detect = 1;
>> 326 if (codec->jackpoll_interval > 0)
>> 327 return callback; /* No unsol if we're
>> polling instead */
>> ^^^^^^^^^^^^^^^^
>>
>> 328 err = snd_hda_codec_write_cache(codec, nid, 0,
>> 329 AC_VERB_SET_UNSOLICITED_ENABLE,
>> 330 AC_USRSP_EN |
>> jack->tag);
>> 331 if (err < 0)
>> 332 return ERR_PTR(err);
>> 333 return callback;
>> ^^^^^^^^^^^^^^^^
>> And these as well.
>>
>> 334 }
>>
>> regards,
>> dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] ALSA: hda - Don't register a cb func if it is registered already
2020-10-21 15:03 ` Hui Wang
@ 2020-10-21 15:59 ` Takashi Iwai
2020-10-22 1:45 ` Hui Wang
2020-10-21 17:27 ` Dan Carpenter
1 sibling, 1 reply; 7+ messages in thread
From: Takashi Iwai @ 2020-10-21 15:59 UTC (permalink / raw)
To: Hui Wang; +Cc: alsa-devel, Dan Carpenter
On Wed, 21 Oct 2020 17:03:11 +0200,
Hui Wang wrote:
>
> Looks like this will not bring problem on patch_sigmatel.c, NULL and
> valid kernel pointer are same for IS_ERR(), they will not make
> IS_ERR() come true.
Right, and it's called only once, so the bug doesn't hit.
But we should address the potential bug nevertheless, of course :)
thanks,
Takashi
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] ALSA: hda - Don't register a cb func if it is registered already
2020-10-21 15:03 ` Hui Wang
2020-10-21 15:59 ` Takashi Iwai
@ 2020-10-21 17:27 ` Dan Carpenter
2020-10-22 1:42 ` Hui Wang
1 sibling, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2020-10-21 17:27 UTC (permalink / raw)
To: Hui Wang; +Cc: alsa-devel
On Wed, Oct 21, 2020 at 11:03:11PM +0800, Hui Wang wrote:
> Looks like this will not bring problem on patch_sigmatel.c, NULL and valid
> kernel pointer are same for IS_ERR(), they will not make IS_ERR() come true.
>
Correct, but that is the problem. You can dereference a valid pointer
but you can't dereference a NULL.
> On 10/21/20 10:21 PM, Hui Wang wrote:
> >
> > On 10/21/20 8:19 PM, Dan Carpenter wrote:
> > > Hello Hui Wang,
> > >
> > > The patch f4794c6064a8: "ALSA: hda - Don't register a cb func if it
> > > is registered already" from Sep 30, 2020, leads to the following
> > > static checker warning:
> > >
> > > sound/pci/hda/patch_sigmatel.c:3075 stac92hd71bxx_fixup_hp_m4()
> > > warn: 'jack' can also be NULL
> > >
> > > sound/pci/hda/patch_sigmatel.c
> > > 3069 /* Enable VREF power saving on GPIO1 detect */
> > > 3070 snd_hda_codec_write_cache(codec, codec->core.afg, 0,
> > > 3071 AC_VERB_SET_GPIO_UNSOLICITED_RSP_MASK, 0x02);
> > > 3072 jack = snd_hda_jack_detect_enable_callback(codec,
> > > codec->core.afg,
> > > 3073 stac_vref_event);
> > >
> > > Originally snd_hda_jack_detect_enable_callback() would not return NULL
> > > here.
> > >
> > > 3074 if (!IS_ERR(jack))
> > > 3075 jack->private_data = 0x02;
So if "jack" is NULL then it will say it's not an error pointer so it
will try to assign jack->private_data = 0x02; and oops.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] ALSA: hda - Don't register a cb func if it is registered already
2020-10-21 17:27 ` Dan Carpenter
@ 2020-10-22 1:42 ` Hui Wang
0 siblings, 0 replies; 7+ messages in thread
From: Hui Wang @ 2020-10-22 1:42 UTC (permalink / raw)
To: Dan Carpenter; +Cc: alsa-devel
On 10/22/20 1:27 AM, Dan Carpenter wrote:
> On Wed, Oct 21, 2020 at 11:03:11PM +0800, Hui Wang wrote:
>> Looks like this will not bring problem on patch_sigmatel.c, NULL and valid
>> kernel pointer are same for IS_ERR(), they will not make IS_ERR() come true.
>>
> Correct, but that is the problem. You can dereference a valid pointer
> but you can't dereference a NULL.
>
>> On 10/21/20 10:21 PM, Hui Wang wrote:
>>> On 10/21/20 8:19 PM, Dan Carpenter wrote:
>>>> Hello Hui Wang,
>>>>
>>>> The patch f4794c6064a8: "ALSA: hda - Don't register a cb func if it
>>>> is registered already" from Sep 30, 2020, leads to the following
>>>> static checker warning:
>>>>
>>>> sound/pci/hda/patch_sigmatel.c:3075 stac92hd71bxx_fixup_hp_m4()
>>>> warn: 'jack' can also be NULL
>>>>
>>>> sound/pci/hda/patch_sigmatel.c
>>>> 3069 /* Enable VREF power saving on GPIO1 detect */
>>>> 3070 snd_hda_codec_write_cache(codec, codec->core.afg, 0,
>>>> 3071 AC_VERB_SET_GPIO_UNSOLICITED_RSP_MASK, 0x02);
>>>> 3072 jack = snd_hda_jack_detect_enable_callback(codec,
>>>> codec->core.afg,
>>>> 3073 stac_vref_event);
>>>>
>>>> Originally snd_hda_jack_detect_enable_callback() would not return NULL
>>>> here.
>>>>
>>>> 3074 if (!IS_ERR(jack))
>>>> 3075 jack->private_data = 0x02;
> So if "jack" is NULL then it will say it's not an error pointer so it
> will try to assign jack->private_data = 0x02; and oops.
Right. Will fix it.
Thanks
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] ALSA: hda - Don't register a cb func if it is registered already
2020-10-21 15:59 ` Takashi Iwai
@ 2020-10-22 1:45 ` Hui Wang
0 siblings, 0 replies; 7+ messages in thread
From: Hui Wang @ 2020-10-22 1:45 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, Dan Carpenter
On 10/21/20 11:59 PM, Takashi Iwai wrote:
> On Wed, 21 Oct 2020 17:03:11 +0200,
> Hui Wang wrote:
>> Looks like this will not bring problem on patch_sigmatel.c, NULL and
>> valid kernel pointer are same for IS_ERR(), they will not make
>> IS_ERR() come true.
> Right, and it's called only once, so the bug doesn't hit.
>
> But we should address the potential bug nevertheless, of course :)
OK, will fix it.
Thanks.
>
> thanks,
>
> Takashi
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-10-22 1:46 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-21 12:19 [bug report] ALSA: hda - Don't register a cb func if it is registered already Dan Carpenter
2020-10-21 14:21 ` Hui Wang
2020-10-21 15:03 ` Hui Wang
2020-10-21 15:59 ` Takashi Iwai
2020-10-22 1:45 ` Hui Wang
2020-10-21 17:27 ` Dan Carpenter
2020-10-22 1:42 ` Hui Wang
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.