linux-kernel - Re: [PATCH 6/6] spi: octeon: Add thunderx 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: <20160724210452.GB6345@sirena.org.uk>
Date:	Sun, 24 Jul 2016 22:04:52 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Jan Glauber <jglauber@...ium.com>
Cc:	linux-kernel@...r.kernel.org, linux-spi@...r.kernel.org,
	"Steven J. Hill" <steven.hill@...ium.com>,
	David Daney <david.daney@...ium.com>
Subject: Re: [PATCH 6/6] spi: octeon: Add thunderx driver

On Sat, Jul 23, 2016 at 12:42:55PM +0200, Jan Glauber wrote:

> +config SPI_THUNDERX
> +	tristate "Cavium ThunderX SPI controller"
> +	depends on 64BIT && PCI && !CAVIUM_OCTEON_SOC

This is a *weird* and most likely broken set of dependencies - why
exclude this if we're on Octeon (or Octeon happens to have been enabled
in a config)?

> +static void thunderx_spi_clock_enable(struct device *dev, struct octeon_spi *p)
> +{
> +	int ret;
> +
> +	p->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(p->clk)) {
> +		p->clk = NULL;
> +		goto skip;
> +	}

This is really not clever - we should be requesting clocks on probe, not
only when we're trying to enable them, and using devm_ outside of probe
paths is usually a warning sign too.  Now, this is actually called from
probe so it works out fine but obviously it'd be better to improve the
power management to only enable the clock when needed and at that point
this function will be used and we'll fall into a bad pattern.

Given how tiny this function is and that we've not bothered splitting
out any of the other resource acquisition it's probably better to just
inline it into probe.

> +	dev_info(&pdev->dev, "Cavium SPI bus driver probed\n");

Again, this is just adding noise to the boot log.

> +#define PCI_DEVICE_ID_THUNDERX_SPI      0xa00b
> +
> +static const struct pci_device_id thunderx_spi_pci_id_table[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, PCI_DEVICE_ID_THUNDERX_SPI) },
> +	{ 0, }
> +};

The define for the device ID doesn't seem to be adding much here.

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