linux-kernel - Re: [PATCH 3/3] ASoC: rt5677: Add jack detection support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141007193542.GG4609@sirena.org.uk>
Date:	Tue, 7 Oct 2014 20:35:42 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Ben Zhang <benzh@...omium.org>
Cc:	alsa-devel@...a-project.org, Bard Liao <bardliao@...ltek.com>,
	Oder Chiou <oder_chiou@...ltek.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] ASoC: rt5677: Add jack detection support

On Fri, Oct 03, 2014 at 02:37:26PM -0700, Ben Zhang wrote:

> This patch adds support for jack detection using GPIOs on the codec.
> Plug detect signal and mic present signal can be routed from the physical
> audio jack to GPIOs on the codec. The codec is configured so that upon
> signal change, an IRQ(GPIO1) is fired. The codec interrupt handler reads

This then assumes that GPIO1 was configured as the IRQ - what happens if
it was connected with some other function on the board and some other
GPIO was assigned as IRQ?

> +	desc = devm_gpiod_get_index(&i2c->dev, "RT5677_GPIO",
> +			RT5677_GPIO_PLUG_DET);
> +	if (!IS_ERR(desc) && !gpiod_direction_input(desc))
> +		rt5677->gpio_plug_det = desc_to_gpio(desc)
> +				- rt5677->gpio_chip.base + 1;

I'm having trouble reading this, I think other reasonable users would
too.  It's not entirely clear to me what it's supposed to be doing or
why we are mixing the descriptor and number based GPIO APIs.

> +       /*
> +        * Loop to handle interrupts until the last i2c read shows no pending
> +        * irqs with a safeguard of 20 loops
> +        */
> +       for (i = 0; i < 20; i++) {

Eh?  Why are we doing this and why are we so sure that 20 is a good
value?

>  static int rt5677_probe(struct snd_soc_codec *codec)
>  {

> +       rt5677_setup_jack_detect(codec, rt5677);
>         return 0;

This sets up GPIO1 as an interrupt without verifying that we have an
interrupt assigned for it successfully and only in the ASoC level probe,
not in the chip level probe.  This means we might not have an interrupt
at all if the board configuration was buggy and means that we'll start
paying attention to the interrupt (which is requested in the i2c level
probe as it should be) without configuring the chip to be in the
appropriate state which means that it might start falsely flagging
interrupts.  I'd expect the configuration to be closer together, and
to have the CODEC set up ready to generate interrupts prior to starting
to listen to them.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)