| 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
| ||
|
Date: Wed, 6 Jun 2018 21:47:46 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Mark Jonas <mark.jonas@...bosch.com>
Cc: Wolfgang Grandegger <wg@...ndegger.com>,
Marc Kleine-Budde <mkl@...gutronix.de>,
linux-can@...r.kernel.org, netdev <netdev@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Heiko Schocher <hs@...x.de>, Zhu Yi <yi.zhu5@...bosch.com>
Subject: Re: [PATCH 2/5] spi: implement companion-spi driver
On Tue, Jun 5, 2018 at 9:43 PM, Mark Jonas <mark.jonas@...bosch.com> wrote:
> The low level companion-spi driver encapsulates the communication
> details with the companion processor, and provides interface for
> the upper level drivers to access.
> +int companion_do_io_tx(struct device *parent,
> + const char __user *buf,
> + size_t count)
> +{
> + struct companion_spi_priv *priv = dev_get_drvdata(parent);
> + unsigned int copied;
> + int error;
> + struct companion_packet p;
> +
> + /*TODO: support mutiple packets in one write in future*/
Hmm... Either fix this, or remove comment?
> + if (copy_from_user(p.data, buf, sizeof(p)) == 0) {
> + if (is_can_type(&p))
> + return -EINVAL;
> + } else {
> + dev_info(parent, "copy from user not succeed in one call\n");
Shouldn't it return immediately?
> + }
> +
> + error = qm_io_txq_in(&priv->pm.qm, buf, count, &copied);
...what the point to call if we got garbage from user space?
> + if (!error) {
The usual pattern is to check for errors first.
> + wake_up_interruptible(&priv->wait);
> + priv->pm.stats.io_tx++;
> + return copied;
> + } else {
> + priv->pm.stats.io_tx_overflows++;
> + }
> + return error;
> +}
> + ... qm_io_rxq_out(&priv->pm.qm, buf, count, &copied);
> + ... pm_can_data_tx(&priv->pm, port, prio, cf);
Oh, oh, oh.
These namespaces are too generic, moreover pm is kinda occupied by
power management. You bring a lot of confusion here, I think.
> + err = pm_can_get_txq_status(pm, port);
> + if (!err) {
if (err)
return err;
> + }
> + return err;
> + int ret, value;
> +
> + ret = sscanf(buf, "%d", &value);
> + if (ret != 1) {
> + }
You have to be consistent in your code.
I've already noticed
err
error
and now
ret
Choose one and stick with it.
Also check your entire patch series' code for consistency.
> +static DEVICE_ATTR(dump_packets, S_IRUGO | S_IWUSR,
> + show_dump_packets, store_dump_packets);
We have helpers, like DEVICE_ATTR_RW().
> + ret = snprintf(buf + pos, PAGE_SIZE - pos,
> + "\ntx: %u, rx: %u, err: %u\n\n",
> + total,
> + priv->pm.stats.can_rx_overflows[i],
> + priv->pm.stats.can_err_overflows[i]);
Please, avoid leading '\n'.
> + gpio_set_value(priv->cs_gpios, priv->cs_gpios_assert);
> + gpio_set_value(priv->cs_gpios, !priv->cs_gpios_assert);
Can you switch to GPIO descriptor API?
> + unsigned int count = READY_POLL_US / READY_POLL_US_GRAN;
> + while (count--) {
For counting like this better to have
do {
} while (--count);
The rationale, reader at first glance will know that the loop will
iterate at least once.
> + if (slave_is_not_busy(priv))
> + return 0;
> +
> + udelay(READY_POLL_US_GRAN);
Should it be atomic?
Can it use read_poll_* type of helpers instead?
> + }
Above comments applicable to entire code you have.
> +static void companion_spi_cpu_to_be32(char *buf)
> +{
> + u32 *buf32 = (u32*)buf;
> + int i;
> +
> + for (i = 0; i < (BCP_PACKET_SIZE / sizeof(u32)); i++, buf32++)
> + *buf32 = cpu_to_be32(*buf32);
> +}
This entire function should be replaced by one of the helpers from
include/linux/byteorder/generic.h
I guess cpu_to_be32_array() is a right one.
> +static void companion_spi_be32_to_cpu(char *buf)
> +{
> + u32 *buf32 = (u32*)buf;
> + int i;
> +
> + for (i = 0; i < (BCP_PACKET_SIZE / sizeof(u32)); i++, buf32++)
> + *buf32 = be32_to_cpu(*buf32);
> +}
Ditto.
Recommendation: check your code against existing helpers.
> + p = (const struct companion_packet*)transfer->tx_buf;
> + companion_spi_cpu_to_be32((char*)transfer->tx_buf);
If tx_buf is type of void * all these castings are redundant.
Also looking at the function, did you consider to use whatever SPI
core provides, like CS handling, messages handling and so on?
> +static int companion_spi_thread(void *data)
> +{
> + struct companion_spi_priv *priv = data;
> + struct companion_packet tx_packet;
> + struct companion_packet rx_packet;
> + struct spi_message message;
> + struct spi_transfer transfer;
> +
> + memset(&transfer, 0, sizeof(transfer));
> + transfer.tx_buf = tx_packet.data;
> + transfer.rx_buf = rx_packet.data;
> + transfer.len = sizeof(struct companion_packet);
> + transfer.cs_change = 0;
Redundant.
> + transfer.bits_per_word = 32;
> +
> + for (;;) {
Infinite loops are evil in most of the cases.
I see here
do {
} while (kthread_should_stop());
> + if (wait_event_interruptible(priv->wait,
> + kthread_should_stop() ||
> + slave_has_request(priv) ||
> + qm_has_tx_data(&priv->pm.qm)))
> + continue;
> +
> + if (kthread_should_stop())
> + break;
> +
> + pm_prepare_tx(&priv->pm, &tx_packet);
> + companion_spi_transceive(priv, &message, &transfer);
> + pm_on_tx_done(&priv->pm);
> + pm_on_rx_done(&priv->pm, &rx_packet);
> + }
> +
> + return 0;
> +}
> +static const struct of_device_id companion_spi_of_match[] = {
> + { .compatible = DRIVER_NAME, .data = NULL, },
> + { /* sentinel */ },
terminators better without commas.
> +};
> +static struct spi_driver companion_spi_driver = {
> + .driver = {
> + .name = DRIVER_NAME,
> + .owner = THIS_MODULE,
This is redundant, macro you are using below sets that.
> + .of_match_table = of_match_ptr(companion_spi_of_match),
> + },
> + .probe = companion_spi_probe,
> + .remove = companion_spi_remove,
> +};
> +module_spi_driver(companion_spi_driver);
> +static void io_process(struct companion_protocol_manager *pm,
> + const struct companion_packet *p)
Something wrong with indentation in this file.
> +#define CHECK_SIZE(x) BUILD_BUG_ON(sizeof(struct companion_packet) != \
> + sizeof(x))
Better to split like
_SIZE(x) \
BUILD_BUG_ON()
> +void pm_init(struct companion_protocol_manager *pm)
Unfortunately, horrible name for the function.
Namespace is kinda occupied, name itself way too generic.
> + if (ctrl & CAN_CTRLMODE_LISTENONLY)
> + p.mode = BCP_CAN_MODE_LISTEN;
> + else
> + return -EOPNOTSUPP;
if (!(cond))
return -ERRNO;
?
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
Do you still need this text?
> + * TODO: add more statistics fields and export to sysfs
Do it or remove the comment?
> + * TODO: re-think the data structure for handle CAN response
Ditto.
> +/**
> + * BCP status field definitions
> + */
> +#define BCP_STATUS_SUCCESS 0x00u
> +#define BCP_STATUS_UNKNOWN 0x01u
> +#define BCP_STATUS_OTHER 0x02u
BIT() ?
> +struct companion_packet {
> + __u8 data[BCP_PACKET_SIZE];
> +};
Is it going from / to user space? Otherwise why __ kind of type?
> +#define CAN_MAX_IN_A_ROW 8
> +
> +
> +
Too many blank lines.
> +int companion_do_can_err(struct device *parent,
> + u8 port,
> + struct can_berr_counter *bec,
> + u8 *state,
> + u8 *code);
+ blank line here.
> +#define COMPANION_CAN_STATE_WARNING 0x01u
> +#define COMPANION_CAN_STATE_PASSIVE 0x02u
> +#define COMPANION_CAN_STATE_BUS_OFF 0x04u
> +#define COMPANION_CAN_ERROR_STUFF 0x01u
> +#define COMPANION_CAN_ERROR_FORM 0x02u
> +#define COMPANION_CAN_ERROR_ACK 0x04u
> +#define COMPANION_CAN_ERROR_BIT1 0x08u
> +#define COMPANION_CAN_ERROR_BIT0 0x10u
> +#define COMPANION_CAN_ERROR_CRC 0x20u
> +#define COMPANION_CAN_ERROR_RXOV 0x80u
BIT() ?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists