linux-kernel - Re: [PATCH v17 1/2] fpga: lattice-sysconfig-spi: add Lattice sysCONFIG FPGA manager

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: <Y0l+AbjGSOyTaoqV@yilunxu-OptiPlex-7050>
Date:   Fri, 14 Oct 2022 23:19:29 +0800
From:   Xu Yilun <yilun.xu@...el.com>
To:     Ivan Bornyakov <i.bornyakov@...rotek.ru>
Cc:     mdf@...nel.org, hao.wu@...el.com, trix@...hat.com, dg@...ix.com,
        j.zink@...gutronix.de, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, linux-fpga@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...gutronix.de, system@...rotek.ru
Subject: Re: [PATCH v17 1/2] fpga: lattice-sysconfig-spi: add Lattice
 sysCONFIG FPGA manager

On 2022-10-11 at 22:38:20 +0300, Ivan Bornyakov wrote:
> Add support to the FPGA manager for programming Lattice ECP5 FPGA over
> slave SPI sysCONFIG interface.
> 
> sysCONFIG interface core functionality is separate from both ECP5 and
> SPI specifics, so support for other FPGAs with different port types can
> be added in the future.
> 
> Signed-off-by: Ivan Bornyakov <i.bornyakov@...rotek.ru>

[...]

> +
> +static int sysconfig_read_busy(struct sysconfig_priv *priv)
> +{
> +	const u8 lsc_check_busy[] = SYSCONFIG_LSC_CHECK_BUSY;
> +	u8 busy;
> +	int ret;
> +
> +	ret = sysconfig_cmd_read(priv, lsc_check_busy, sizeof(lsc_check_busy),
> +				 &busy, sizeof(busy));
> +
> +	return ret ? : busy;
> +}
> +
> +static int sysconfig_poll_busy(struct sysconfig_priv *priv)
> +{
> +	unsigned long timeout;
> +	int ret;
> +
> +	timeout = jiffies + msecs_to_jiffies(SYSCONFIG_POLL_BUSY_TIMEOUT_MS);
> +
> +	while (time_before(jiffies, timeout)) {
> +		ret = sysconfig_read_busy(priv);
> +		if (ret <= 0)
> +			return ret;
> +
> +		usleep_range(SYSCONFIG_POLL_INTERVAL_US,
> +			     SYSCONFIG_POLL_INTERVAL_US * 2);
> +	}
> +
> +	return -ETIMEDOUT;

As mentioned by Ahmad, could read_poll_timeout() be used?

> +}
> +
> +static int sysconfig_read_status(struct sysconfig_priv *priv, u32 *status)
> +{
> +	const u8 lsc_read_status[] = SYSCONFIG_LSC_READ_STATUS;
> +	__be32 device_status;
> +	int ret;
> +
> +	ret = sysconfig_cmd_read(priv, lsc_read_status, sizeof(lsc_read_status),
> +				 &device_status, sizeof(device_status));
> +	if (ret)
> +		return ret;
> +
> +	*status = be32_to_cpu(device_status);
> +
> +	return 0;
> +}
> +
> +static int sysconfig_poll_status(struct sysconfig_priv *priv, u32 *status)
> +{
> +	int ret = sysconfig_poll_busy(priv);
> +
> +	if (ret)
> +		return ret;
> +
> +	return sysconfig_read_status(priv, status);
> +}
> +
> +static int sysconfig_poll_gpio(struct gpio_desc *gpio, bool is_active)
> +{
> +	unsigned long timeout;
> +	int value;
> +
> +	timeout = jiffies + msecs_to_jiffies(SYSCONFIG_POLL_GPIO_TIMEOUT_MS);
> +
> +	while (time_before(jiffies, timeout)) {
> +		value = gpiod_get_value(gpio);
> +		if (value < 0)
> +			return value;
> +
> +		if ((is_active && value) || (!is_active && !value))
> +			return 0;
> +
> +		usleep_range(SYSCONFIG_POLL_INTERVAL_US,
> +			     SYSCONFIG_POLL_INTERVAL_US * 2);
> +	}
> +
> +	return -ETIMEDOUT;

Same.

[...]

> +int sysconfig_probe(struct sysconfig_priv *priv)
> +{
> +	struct gpio_desc *program, *init, *done;
> +	struct device *dev = priv->dev;
> +	struct fpga_manager *mgr;
> +
> +	if (!dev)
> +		return -ENODEV;
> +
> +	if (!priv->bitstream_burst_write_init) {
> +		dev_err(dev,
> +			"Callback for preparation for bitstream burst write is not defined\n");
> +		return -EOPNOTSUPP;

-EINVAL is better?

> +	}
> +
> +	if (!priv->bitstream_burst_write) {
> +		dev_err(dev,
> +			"Callback for bitstream burst write is not defined\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!priv->bitstream_burst_write_complete) {
> +		dev_err(dev,
> +			"Callback for finishing bitstream burst write is not defined\n");
> +		return -EOPNOTSUPP;
> +	}

command_transfer is optional?

And I think different err log for each missing callback is too trivial,
maybe just say like "ops missing" if any mandatory callback is missing.

> +
> +	program = devm_gpiod_get_optional(dev, "program", GPIOD_OUT_LOW);
> +	if (IS_ERR(program))
> +		return dev_err_probe(dev, PTR_ERR(program),
> +				     "Failed to get PROGRAM GPIO\n");
> +
> +	init = devm_gpiod_get_optional(dev, "init", GPIOD_IN);
> +	if (IS_ERR(init))
> +		return dev_err_probe(dev, PTR_ERR(init),
> +				     "Failed to get INIT GPIO\n");
> +
> +	done = devm_gpiod_get_optional(dev, "done", GPIOD_IN);
> +	if (IS_ERR(done))
> +		return dev_err_probe(dev, PTR_ERR(done),
> +				     "Failed to get DONE GPIO\n");
> +
> +	priv->program = program;
> +	priv->init = init;
> +	priv->done = done;
> +
> +	mgr = devm_fpga_mgr_register(dev, "Lattice sysCONFIG FPGA Manager",
> +				     &sysconfig_fpga_mgr_ops, priv);
> +
> +	return PTR_ERR_OR_ZERO(mgr);
> +}
> +EXPORT_SYMBOL(sysconfig_probe);