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: <20190503055809.GC14916@sirena.org.uk>
Date:   Fri, 3 May 2019 14:58:09 +0900
From:   Mark Brown <broonie@...nel.org>
To:     Fabien Parent <fparent@...libre.com>
Cc:     lgirdwood@...il.com, robh+dt@...nel.org, mark.rutland@....com,
        matthias.bgg@...il.com, perex@...ex.cz, tiwai@...e.com,
        kaichieh.chuang@...iatek.com, shunli.wang@...iatek.com,
        alsa-devel@...a-project.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/5] ASoC: mediatek: Add MT8516 PCM driver

On Thu, May 02, 2019 at 02:10:39PM +0200, Fabien Parent wrote:

> +static irqreturn_t mt8516_afe_irq_handler(int irq, void *dev_id)
> +{
> +	struct mtk_base_afe *afe = dev_id;
> +	unsigned int reg_value;
> +	unsigned int memif_status;
> +	int i, ret;
> +
> +	ret = regmap_read(afe->regmap, AFE_IRQ_STATUS, &reg_value);
> +	if (ret) {
> +		reg_value = AFE_IRQ_STATUS_BITS;
> +		goto exit_irq;
> +	}

...

> +exit_irq:
> +	regmap_write(afe->regmap, AFE_IRQ_CLR, reg_value & AFE_IRQ_STATUS_BITS);
> +
> +	return IRQ_HANDLED;
> +}

This unconditionally says it handled an interrupt regardless of what
happened.  This means that the interrupt line can't be shared and that
the error handling code in the generic interrupt subsystem can't tell if
something goes wrong and the interrupt gets stuck.

> +	ret = devm_request_irq(afe->dev, irq_id, mt8516_afe_irq_handler,
> +			       0, "Afe_ISR_Handle", (void *)afe);
> +	if (ret) {
> +		dev_err(afe->dev, "could not request_irq\n");
> +		return ret;
> +	}

Are you sure the interrupt handler can safely use managed resources,
especially given...

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	afe->base_addr = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(afe->base_addr))
> +		return PTR_ERR(afe->base_addr);
> +
> +	afe->regmap = devm_regmap_init_mmio(&pdev->dev, afe->base_addr,
> +		&mt8516_afe_regmap_config);
> +	if (IS_ERR(afe->regmap))
> +		return PTR_ERR(afe->regmap);

...that things like the register map and the I/O resources for the chip
are allocated after and therefore freed before before the interrupt is
freed.  Normally the interrupt should be one of the last things to be
allocated.

> +static int mt8516_afe_pcm_dev_remove(struct platform_device *pdev)
> +{
> +	return 0;
> +}

In general if functions can legitimately be empty they should just be
omitted, if they are required that usually means they have to do
something.

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ