linux-kernel - Re: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver

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]

Date:   Thu, 17 Sep 2020 14:53:06 +0100
From:   Mark Brown <broonie@...nel.org>
To:     "Viorel Suman (OSS)" <viorel.suman@....nxp.com>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, Timur Tabi <timur@...nel.org>,
        Nicolin Chen <nicoleotsuka@...il.com>,
        Xiubo Li <Xiubo.Lee@...il.com>,
        Fabio Estevam <festevam@...il.com>,
        Shengjiu Wang <shengjiu.wang@...il.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Matthias Schiffer <matthias.schiffer@...tq-group.com>,
        Cosmin-Gabriel Samoila <cosmin.samoila@....com>,
        Viorel Suman <viorel.suman@....com>,
        alsa-devel@...a-project.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        NXP Linux Team <linux-imx@....com>,
        Viorel Suman <viorel.suman@...il.com>
Subject: Re: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver

On Wed, Sep 16, 2020 at 12:17:55PM +0300, Viorel Suman (OSS) wrote:

This looks mostly good, a few smallish things below but nothing major.

> +static int fsl_xcvr_load_firmware(struct fsl_xcvr *xcvr)
> +{
> +	struct device *dev = &xcvr->pdev->dev;
> +	const struct firmware *fw;
> +	int ret = 0, rem, off, out, page = 0, size = FSL_XCVR_REG_OFFSET;
> +	u32 mask, val;
> +
> +	ret = request_firmware(&fw, xcvr->fw_name, dev);
> +	if (ret) {
> +		dev_err(dev, "failed to request firmware.\n");
> +		return ret;
> +	}
> +
> +	rem = fw->size;

It would be good to see some explicit validation of the image size, at
least printing an error message if the image is bigger than can be
loaded.  The code should be safe in that it won't overflow the device
region it's writing to but it feels like it'd be better to tell people
if we spot a problem rather than just silently truncating the file.

> +	/* RAM is 20KiB => max 10 pages 2KiB each */
> +	for (page = 0; page < 10; page++) {
> +		ret = regmap_update_bits(xcvr->regmap, FSL_XCVR_EXT_CTRL,
> +					 FSL_XCVR_EXT_CTRL_PAGE_MASK,
> +					 FSL_XCVR_EXT_CTRL_PAGE(page));

regmap does have paging support, though given that this is currently the
only place where paging is used this probably doesn't matter too much.

> +static irqreturn_t irq0_isr(int irq, void *devid)
> +{
> +	struct fsl_xcvr *xcvr = (struct fsl_xcvr *)devid;
> +	struct device *dev = &xcvr->pdev->dev;
> +	struct regmap *regmap = xcvr->regmap;
> +	void __iomem *reg_ctrl, *reg_buff;
> +	u32 isr, val, i;
> +
> +	regmap_read(regmap, FSL_XCVR_EXT_ISR, &isr);
> +	regmap_write(regmap, FSL_XCVR_EXT_ISR_CLR, isr);

This will unconditionally clear any interrupts, even those we don't
understand - it might be better to only clear bits that are supported so
the IRQ core can complain if there's something unexpected showing up.

> +	if (isr & FSL_XCVR_IRQ_FIFO_UOFL_ERR)
> +		dev_dbg(dev, "RX/TX FIFO full/empty\n");

Should this be dev_err()?

> +static irqreturn_t irq1_isr(int irq, void *devid)
> +{
> +	struct fsl_xcvr *xcvr = (struct fsl_xcvr *)devid;
> +	struct device *dev = &xcvr->pdev->dev;
> +
> +	dev_dbg(dev, "irq[1]: %d\n", irq);
> +
> +	return IRQ_HANDLED;
> +}

Is there any value in even requesting this and irq2 given the lack of
meaningful handling?

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