linux-kernel - Re: [PATCH v9 15/15] spi: pensando-sr: Add AMD Pensando SoC System Resource

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: <e7180562-dddf-47ff-8e6d-34265521b10d@app.fastmail.com>
Date:   Thu, 19 Jan 2023 10:22:37 +0100
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Brad Larson" <brad@...sando.io>,
        linux-arm-kernel@...ts.infradead.org
Cc:     linux-kernel@...r.kernel.org,
        "linux-mmc @ vger . kernel . org" <linux-mmc@...r.kernel.org>,
        linux-spi@...r.kernel.org, adrian.hunter@...el.com,
        alcooperx@...il.com, "Andy Shevchenko" <andy.shevchenko@...il.com>,
        blarson@....com, brendan.higgins@...ux.dev,
        "Brian Norris" <briannorris@...omium.org>,
        brijeshkumar.singh@....com,
        "Catalin Marinas" <catalin.marinas@....com>,
        "David Gow" <davidgow@...gle.com>, gsomlo@...il.com,
        gerg@...ux-m68k.org, "Krzysztof Kozlowski" <krzk@...nel.org>,
        krzysztof.kozlowski+dt@...aro.org, "Lee Jones" <lee@...nel.org>,
        "Lee Jones" <lee.jones@...aro.org>,
        "Mark Brown" <broonie@...nel.org>, yamada.masahiro@...ionext.com,
        "Philipp Zabel" <p.zabel@...gutronix.de>, piotrs@...ence.com,
        p.yadav@...com, "Randy Dunlap" <rdunlap@...radead.org>,
        "Rob Herring" <robh+dt@...nel.org>,
        "Samuel Holland" <samuel@...lland.org>,
        "Serge Semin" <fancer.lancer@...il.com>, skhan@...uxfoundation.org,
        suravee.suthikulpanit@....com, thomas.lendacky@....com,
        "Tony Huang" <tonyhuang.sunplus@...il.com>,
        "Ulf Hansson" <ulf.hansson@...aro.org>, vaishnav.a@...com,
        "Will Deacon" <will@...nel.org>, devicetree@...r.kernel.org
Subject: Re: [PATCH v9 15/15] spi: pensando-sr: Add AMD Pensando SoC System Resource

On Thu, Jan 19, 2023, at 04:39, Brad Larson wrote:

> drivers/spi/spi-pensando-sr.c

I don't think that is the right place for this driver: it's not a spi
controller implementation but rather a user interface driver that should
be in another subsystem depending on its purpose. drivers/misc/ might
be an option, but ideally I think there should be a higher-level
interface.

I'm still confused about what this driver actually does, and how
the corresponding user space side is meant to be used.

> +config SPI_PENSANDO_SR
> +	bool "AMD Pensando SoC System Resource chip"
> +	depends on SPI_MASTER=y

Why can this not be a loadable module, i.e. a 'tristate' option?

> +
> +#define PENSR_MAX_REG		0xff
> +#define PENSR_CTRL0_REG		0x10
> +#define PENSR_SPI_CMD_REGRD	0x0b
> +#define PENSR_SPI_CMD_REGWR	0x02
> +#define SPI_IOC_MAGIC		'k'
> +
> +#define SPI_MSGSIZE(N) \
> +	((((N)*(sizeof(struct spi_ioc_transfer))) < (1 << _IOC_SIZEBITS)) \
> +		? ((N)*(sizeof(struct spi_ioc_transfer))) : 0)
> +#define SPI_IOC_MESSAGE(N)	_IOW(SPI_IOC_MAGIC, 0, char[SPI_MSGSIZE(N)])
> +
> +struct spi_ioc_transfer {
> +	__u64 tx_buf;
> +	__u64 rx_buf;
> +	__u32 len;
> +	__u32 speed_hz;
> +	__u16 delay_usecs;
> +	__u8 bits_per_word;
> +	__u8 cs_change;
> +	__u8 tx_nbits;
> +	__u8 rx_nbits;
> +	__u8 word_delay_usecs;
> +	__u8 pad;
> +};

When you create a new user interface, the interface definition should
be in include/uapi/linux/*.h. The structure name and command name should
indicate what driver they are used for, these names look overly
generic.

> +struct pensr_device {
> +	struct spi_device *spi_dev;
> +	struct reset_controller_dev rcdev;
> +	struct mutex buf_lock;
> +	spinlock_t spi_lock;
> +	u8 *tx_buffer;
> +	u8 *rx_buffer;
> +};
> +
> +static dev_t pensr_devt;
> +static struct pensr_device *pensr;
> +static struct class *pensr_class;
> +static unsigned int bufsiz = 4096;

Even if there is only ever a single instance of the device known to the
kernel, it is better style to avoid static variables but instead make
everything passed around as part of the device structure.


> +
> +	t[0].tx_buf = tx_buf;
> +	t[0].len = u_xfer->len;
> +	if (copy_from_user(tx_buf, (const u8 __user *) (uintptr_t) 
> u_xfer->tx_buf, u_xfer->len)) {
> +		ret = -EFAULT;
> +		goto done;
> +	}

Use u64_to_user_ptr() instead of open-coding the type cast.

> +static const struct file_operations pensr_spi_fops = {
> +	.owner =	THIS_MODULE,
> +	.unlocked_ioctl = pensr_spi_ioctl,

There should be a '.compat_ioctl = compat_ptr_ioctl,' line here to
allow the ioctl to work in 32-bit processes.

      Arnd