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]

Message-ID: <VI1PR0401MB22726CC099099547A0502C27923F0@VI1PR0401MB2272.eurprd04.prod.outlook.com>
Date:   Fri, 18 Sep 2020 15:02:39 +0000
From:   "Viorel Suman (OSS)" <viorel.suman@....nxp.com>
To:     Mark Brown <broonie@...nel.org>,
        "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>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        dl-linux-imx <linux-imx@....com>,
        Viorel Suman <viorel.suman@...il.com>
Subject: RE: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver

Hi Mark,

Thank you for your review.
 
> On Wed, Sep 16, 2020 at 12:17:55PM +0300, Viorel Suman (OSS) wrote:
> > +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.

Make sense, will improve this part in the next version.

> > +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.

The ARM core registers itself in "fsl_xcvr_prepare" (the code below) just for a subset of all supported interrupts: 
=====
	ret = regmap_update_bits(xcvr->regmap, FSL_XCVR_EXT_IER0,
				 FSL_XCVR_IRQ_EARC_ALL, FSL_XCVR_IRQ_EARC_ALL);
=====
FSL_XCVR_IRQ_EARC_ALL - this mask represents all the interrupts we are interested in and we handle in interrupt handler,
But this is just a subset of all interrupts the M0+ core is able to assert. Not very intuitive, I think I need to reword it somehow.

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

The interrupt may be asserted right before DMA starts to fill the TX FIFO if I recall correctly.
I've added it just to debug the IP behavior, will check and change it to err it in next version if it is the case.

> > +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?

No, will remove it in v2.

Thank you,
Viorel