linux-kernel - Re: [PATCH v7 5/6] spi: cs42l43: Add SPI controller 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]

Date: Thu, 18 Jan 2024 19:06:13 +0200
From: andy.shevchenko@...il.com
To: Charles Keepax <ckeepax@...nsource.cirrus.com>
Cc: broonie@...nel.org, lee@...nel.org, robh+dt@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
	linus.walleij@...aro.org, vkoul@...nel.org, lgirdwood@...il.com,
	yung-chuan.liao@...ux.intel.com, sanyog.r.kale@...el.com,
	pierre-louis.bossart@...ux.intel.com, alsa-devel@...a-project.org,
	patches@...nsource.cirrus.com, devicetree@...r.kernel.org,
	linux-gpio@...r.kernel.org, linux-spi@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 5/6] spi: cs42l43: Add SPI controller support

Fri, Aug 04, 2023 at 11:46:01AM +0100, Charles Keepax kirjoitti:
> From: Lucas Tanure <tanureal@...nsource.cirrus.com>
> 
> The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface
> (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed
> for portable applications. It provides a high dynamic range, stereo
> DAC for headphone output, two integrated Class D amplifiers for
> loudspeakers, and two ADCs for wired headset microphone input or
> stereo line input. PDM inputs are provided for digital microphones.
> 
> The SPI component incorporates a SPI controller interface for
> communication with other peripheral components.

..

> +		while (buf < block) {
> +			const u8 *word = min(buf + sizeof(u32), block);
> +			int pad = (buf + sizeof(u32)) - word;
> +
> +			while (buf < word) {
> +				val >>= BITS_PER_BYTE;
> +				val |= FIELD_PREP(GENMASK(31, 24), *buf);
> +
> +				buf++;
> +			}

Is this a reinvented way of get_unaligned_*() APIs?

> +			val >>= pad * BITS_PER_BYTE;
> +
> +			regmap_write(regmap, CS42L43_TX_DATA, val);
> +		}

..

> +			while (buf < word) {
> +				*buf = FIELD_GET(GENMASK(7, 0), val);
> +
> +				val >>= BITS_PER_BYTE;
> +				buf++;
> +			}

put_unaligned_*() ?

..

> +	/* select another internal CS, which doesn't exist, so CS 0 is not used */
> +	if (spi_get_csgpiod(spi, 0))
> +		spi_config1 |= 1 << CS42L43_SPI_SS_SEL_SHIFT;

BIT() ?

> +	if (spi->mode & SPI_CPOL)
> +		spi_config1 |= CS42L43_SPI_CPOL_MASK;
> +	if (spi->mode & SPI_CPHA)
> +		spi_config1 |= CS42L43_SPI_CPHA_MASK;
> +	if (spi->mode & SPI_3WIRE)
> +		spi_config1 |= CS42L43_SPI_THREE_WIRE_MASK;

..

> +	if (is_of_node(fwnode))
> +		fwnode = fwnode_get_named_child_node(fwnode, "spi");

You can actually drop these is_of_node() tests and use another variable. In
ACPI there can't be child node in small letters.

But main problem here (and in another driver where the similar is used) that
you bumped reference count for fwnode. I haven't seen where you drop it back.
Have you tested rmmod/modprobe in a loop?

..

> +	devm_pm_runtime_enable(priv->dev);

No error check? Why?

..

> +	ret = devm_spi_register_controller(priv->dev, priv->ctlr);
> +	if (ret) {
> +		pm_runtime_disable(priv->dev);

Ah! Are you sure you properly simulated faults when testing this code?

> +		dev_err(priv->dev, "Failed to register SPI controller: %d\n", ret);
> +	}
> +
> +	return ret;

-- 
With Best Regards,
Andy Shevchenko