| 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
| ||
|
Message-ID: <2855efaf-79a5-f43b-ff8c-9c01a3f14df7@kernel.org>
Date: Mon, 14 Dec 2020 09:04:13 +0100
From: Jiri Slaby <jirislaby@...nel.org>
To: József Horváth <info@...istro.hu>,
'Greg Kroah-Hartman' <gregkh@...uxfoundation.org>,
'Rob Herring' <robh+dt@...nel.org>,
linux-serial@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] Serial: silabs si4455 serial driver
On 12. 12. 20, 8:09, József Horváth wrote:
> This is a serial port driver for
> Silicon Labs Si4455 Sub-GHz transciver.
>
> The goal of this driver is to removing wires
> between central(linux) device and remote serial devices/sensors,
> but keeping the original user software.
> It represents regular serial interface for the user space.
>
> Datasheet: https://www.silabs.com/documents/public/data-sheets/Si4455.pdf
A description of changes between v1..v4 here, please.
> Signed-off-by: József Horváth <info@...istro.hu>
...
> --- /dev/null
> +++ b/drivers/tty/serial/si4455.c
> @@ -0,0 +1,1328 @@
...
> +struct si4455_one {
What is this si4455_one good for? I mean, why not just inline these two
in si4455_port?
> + struct uart_port port;
> + struct work_struct tx_work;
> +};
> +
> +struct si4455_port {
> + struct mutex mutex; /* For syncing access to device */
> + int power_count;
> + bool connected;
If you put this int and bool after u32s below, the structure won't
contain holes AFAICS.
> + struct gpio_desc *shdn_gpio;
> + struct si4455_part_info part_info;
> + struct si4455_modem_status modem_status;
> + u32 tx_channel;
> + u32 rx_channel;
> + u32 package_size;
> + bool configured;
> + bool tx_pending;
> + bool rx_pending;
> + u32 current_rssi;
> + struct si4455_one one;
> +};
...
> +static int si4455_get_response(struct uart_port *port, int length, u8 *data)
> +{
> + int ret;
> + u8 data_out[] = { SI4455_CMD_ID_READ_CMD_BUFF };
> + u8 *data_in = NULL;
A useless initialization.
> + struct spi_transfer xfer[2];
> + int timeout = 10000;
> +
> + if (length > 0 && !data)
> + return -EINVAL;
> +
> + data_in = kzalloc(1 + length, GFP_KERNEL);
> + if (!data_in)
> + return -ENOMEM;
> +
> + memset(&xfer, 0x00, sizeof(xfer));
> + xfer[0].tx_buf = data_out;
> + xfer[0].len = sizeof(data_out);
> + xfer[1].rx_buf = data_in;
> + xfer[1].len = 1 + length;
> +
> + while (--timeout > 0) {
> + data_out[0] = SI4455_CMD_ID_READ_CMD_BUFF;
> + ret = spi_sync_transfer(to_spi_device(port->dev), xfer,
> + ARRAY_SIZE(xfer));
> + if (ret) {
> + dev_err(port->dev, "%s: spi_sync_transfer error(%i)", __func__, ret);
Missing \n. And a space before (.
> + break;
> + }
> +
> + if (data_in[0] == 0xFF) {
> + if (length > 0 && data)
> + memcpy(data, &data_in[1], length);
> +
> + break;
> + }
> + usleep_range(100, 200);
> + }
> + if (timeout == 0) {
> + dev_err(port->dev, "%s:timeout==%i", __func__, timeout);
> + ret = -EIO;
> + }
> + kfree(data_in);
> + return ret;
> +}
...
> +static int si4455_send_command(struct uart_port *port, int length, u8 *data)
> +{
> + int ret;
> +
> + ret = si4455_poll_cts(port);
> + if (ret) {
> + dev_err(port->dev,
> + "%s: si4455_poll_cts error(%i)", __func__, ret);
> + return ret;
> + }
Put a newline here.
> + ret = spi_write(to_spi_device(port->dev), data, length);
> + if (ret) {
> + dev_err(port->dev,
> + "%s: spi_write error(%i)", __func__, ret);
> + }
And here.
> + return ret;
> +}
> +
> +static int si4455_send_command_get_response(struct uart_port *port,
> + int out_length, u8 *data_out,
> + int in_length, u8 *data_in)
> +{
> + int ret;
> +
> + ret = si4455_send_command(port, out_length, data_out);
> + if (ret) {
> + dev_err(port->dev,
> + "%s: si4455_send_command error(%i)", __func__, ret);
> + return ret;
> + }
> +
> + ret = si4455_get_response(port, in_length, data_in);
> +
> + return ret;
Simplify by return si4455_get_response() directly.
> +}
> +
> +static int si4455_read_data(struct uart_port *port, u8 command, int poll,
> + int length, u8 *data)
> +{
> + int ret = 0;
> + u8 data_out[] = { command };
> + struct spi_transfer xfer[] = {
> + {
> + .tx_buf = data_out,
> + .len = sizeof(data_out),
> + }, {
> + .rx_buf = data,
> + .len = length,
> + }
> + };
> +
> + if (poll) {
> + ret = si4455_poll_cts(port);
> + if (ret)
> + return ret;
> + }
> +
> + ret = spi_sync_transfer(to_spi_device(port->dev),
> + xfer,
> + ARRAY_SIZE(xfer));
> + if (ret) {
> + dev_err(port->dev,
> + "%s: spi_sync_transfer error(%i)", __func__, ret);
\n here. And in all the other dev_errs all around.
> + }
A newline here.
> + return ret;
> +}
...
> +static int si4455_get_int_status(struct uart_port *port, u8 ph_clear,
> + u8 modem_clear, u8 chip_clear,
> + struct si4455_int_status *result)
> +{
> + int ret;
> + u8 data_out[] = {
> + SI4455_CMD_ID_GET_INT_STATUS,
> + ph_clear,
> + modem_clear,
> + chip_clear
> + };
> + u8 data_in[SI4455_CMD_REPLY_COUNT_GET_INT_STATUS];
> +
> + ret = si4455_send_command_get_response(port, sizeof(data_out), data_out,
> + sizeof(data_in), data_in);
> + if (ret) {
> + dev_err(port->dev,
> + "%s: si4455_send_command_get_response error(%i)",
> + __func__, ret);
return ret; here. And no else there:
> + } else {
> + result->int_pend = data_in[0];
> + result->int_status = data_in[1];
> + result->ph_pend = data_in[2];
> + result->ph_status = data_in[3];
> + result->modem_pend = data_in[4];
> + result->modem_status = data_in[5];
> + result->chip_pend = data_in[6];
> + result->chip_status = data_in[7];
> + }
> + return ret;
> +}
> +
> +static int si4455_get_modem_status(struct uart_port *port, u8 modem_clear,
> + struct si4455_modem_status *result)
> +{
> + int ret;
> + u8 data_out[] = {
> + SI4455_CMD_ID_GET_MODEM_STATUS,
> + modem_clear,
> + };
> + u8 data_in[SI4455_CMD_REPLY_COUNT_GET_MODEM_STATUS];
> +
> + ret = si4455_send_command_get_response(port, sizeof(data_out), data_out,
> + sizeof(data_in), data_in);
> + if (ret) {
> + dev_err(port->dev,
> + "%s: si4455_send_command_get_response error(%i)",
> + __func__, ret);
The same here and the other functions below.
> + } else {
> + result->modem_pend = data_in[0];
> + result->modem_status = data_in[1];
> + result->curr_rssi = data_in[2];
> + result->latch_rssi = data_in[3];
> + result->ant1_rssi = data_in[4];
> + result->ant2_rssi = data_in[5];
> + memcpy(&result->afc_freq_offset,
> + &data_in[6], sizeof(result->afc_freq_offset));
> + }
> + return ret;
> +}
...
> +static int si4455_configure(struct uart_port *port, const u8 *configuration_data)
> +{
> + int ret = 0;
> + u8 col;
> + u8 response;
> + u8 count;
> + struct si4455_int_status int_status = { 0 };
> + u8 radio_cmd[16u];
Any reason for 16 specifically _unsigned_?
> +
> + /* While cycle as far as the pointer points to a command */
> + while (*configuration_data != 0x00) {
> + /* Commands structure in the array:
> + * --------------------------------
> + * LEN | <LEN length of data>
Multiline comments should look like this:
/*
* comment 1
* comment 2
*/
> + */
> + count = *configuration_data++;
> + dev_dbg(port->dev, "%s: count(%u)",
> + __func__, count);
> + if (count > 16u) {
And here?
> + /* Initial configuration of Si4x55 */
> + if (SI4455_CMD_ID_WRITE_TX_FIFO
> + == *configuration_data) {
> + if (count > 128u) {
And here?
BTW too many nestings here. Could you switch the
SI4455_CMD_ID_WRITE_TX_FIFO if condition and break here. No need for
else branch then and the code would go one indentation level left.
> + /* Number of command bytes exceeds
> + * maximal allowable length
> + */
> + dev_err(port->dev, "%s: command length error(%i)",
> + __func__, count);
> + ret = -EINVAL;
> + break;
> + }
> +
> + /* Load array to the device */
> + configuration_data++;
> + ret = si4455_write_data(port,
> + SI4455_CMD_ID_WRITE_TX_FIFO,
> + 1,
> + count - 1,
> + configuration_data);
> + if (ret) {
> + dev_err(port->dev, "%s: si4455_write_data error(%i)",
> + __func__, ret);
> + break;
> + }
> +
> + /* Point to the next command */
> + configuration_data += count - 1;
> +
> + /* Continue command interpreter */
> + continue;
> + } else {
> + /* Number of command bytes exceeds
> + * maximal allowable length
> + */
> + ret = -EINVAL;
> + break;
> + }
> + }
> +
> + for (col = 0u; col < count; col++) {
> + radio_cmd[col] = *configuration_data;
> + configuration_data++;
> + }
> +
> + dev_dbg(port->dev, "%s: radio_cmd[0](%u)", __func__, radio_cmd[0]);
> + ret = si4455_send_command_get_response(port, count, radio_cmd,
> + 1, &response);
> + if (ret) {
> + dev_err(port->dev,
> + "%s: si4455_send_command_get_response error(%i)",
> + __func__, ret);
> + break;
> + }
> +
> + /* Check response byte of EZCONFIG_CHECK command */
> + if (radio_cmd[0] == SI4455_CMD_ID_EZCONFIG_CHECK) {
> + if (response) {
> + /* Number of command bytes exceeds
> + * maximal allowable length
> + */
> + ret = -EIO;
> + dev_err(port->dev, "%s: EZConfig check error(%i)",
> + __func__, radio_cmd[0]);
> + break;
> + }
> + }
> +
> + /* Get and clear all interrupts. An error has occurred... */
> + si4455_get_int_status(port, 0, 0, 0, &int_status);
> + if (int_status.chip_pend
> + & SI4455_CMD_GET_CHIP_STATUS_ERROR_PEND_MASK) {
> + ret = -EIO;
> + dev_err(port->dev, "%s: chip error(%i)",
> + __func__, int_status.chip_pend);
> + break;
> + }
> + }
> +
> + return ret;
> +}
...
> +static int si4455_do_work(struct uart_port *port)
> +{
> + int ret = 0;
> + struct si4455_port *s = dev_get_drvdata(port->dev);
> + struct circ_buf *xmit = &port->state->xmit;
> + unsigned int tx_pending = 0;
> + unsigned int tx_to_end = 0;
> + u8 *data = NULL;
> +
> + mutex_lock(&s->mutex);
> + dev_dbg(port->dev, "%s(connected=%i, configured=%i, power_count=%i)",
> + __func__, s->connected, s->configured, s->power_count);
> + if (s->connected && s->configured && s->power_count > 0) {
> + if (!(uart_circ_empty(xmit) || uart_tx_stopped(port) || s->tx_pending)) {
> + tx_pending = uart_circ_chars_pending(xmit);
> + if (s->package_size > 0) {
> + if (tx_pending >= s->package_size) {
> + tx_pending = s->package_size;
> + data = kzalloc(s->package_size, GFP_KERNEL);
> + tx_to_end = CIRC_CNT_TO_END(xmit->head,
> + xmit->tail,
> + UART_XMIT_SIZE);
> + if (tx_to_end < tx_pending) {
Again, too much spaghetti here.
> + memcpy(data,
> + xmit->buf + xmit->tail,
> + tx_to_end);
> + memcpy(data,
> + xmit->buf,
> + tx_pending - tx_to_end);
> + } else {
> + memcpy(data,
> + xmit->buf + xmit->tail,
> + tx_pending);
> + }
> + if (si4455_begin_tx(port, s->tx_channel,
> + tx_pending, data) == 0) {
> + s->tx_pending = true;
> + }
> + kfree(data);
> + }
> + } else {
> + //TODO: variable packet length
> + }
> + }
> + if (!s->tx_pending) {
> + if (s->package_size > 0) {
> + ret = si4455_begin_rx(port, s->rx_channel,
> + s->package_size);
> + } else {
> + //TODO: variable packet length
> + }
> + }
> + }
> + mutex_unlock(&s->mutex);
> + return ret;
> +}
...
> +static irqreturn_t si4455_port_irq(struct si4455_port *s)
> +{
> + struct uart_port *port = &s->one.port;
> + irqreturn_t ret = IRQ_NONE;
You never return IRQ_NONE, right?
> + struct si4455_int_status int_status = { 0 };
> + struct si4455_fifo_info fifo_info = { 0 };
> +
> + mutex_lock(&s->mutex);
> + if (s->connected && s->configured && s->power_count > 0) {
> + if (!si4455_get_int_status(port, 0, 0, 0, &int_status)) {
> + si4455_get_modem_status(port, 0, &s->modem_status);
> + if (int_status.chip_pend
> + & SI4455_CMD_GET_CHIP_STATUS_ERROR_PEND_BIT) {
> + dev_err(port->dev,
> + "%s: chip_status:CMD_ERROR_PEND",
> + __func__);
> + } else if (int_status.ph_pend
> + & SI4455_CMD_GET_INT_STATUS_PACKET_SENT_PEND_BIT) {
> + dev_dbg(port->dev,
> + "%s: ph_status:PACKET_SENT_PEND",
> + __func__);
> + si4455_handle_tx_pend(s);
> + } else if (int_status.ph_pend
> + & SI4455_CMD_GET_INT_STATUS_PACKET_RX_PEND_BIT) {
> + dev_dbg(port->dev,
> + "%s: ph_status:PACKET_RX_PEND",
> + __func__);
> + s->current_rssi = s->modem_status.curr_rssi;
> + si4455_fifo_info(port, 0, &fifo_info);
> + si4455_handle_rx_pend(s);
> + } else if (int_status.ph_pend
> + & SI4455_CMD_GET_INT_STATUS_CRC_ERROR_BIT) {
> + dev_dbg(port->dev,
> + "%s: ph_status:CRC_ERROR_PEND",
> + __func__);
> + }
> + ret = IRQ_HANDLED;
> + }
> + } else {
> + ret = IRQ_HANDLED;
How can this be IRQ_HANDLED when the device is off?
> + }
> + mutex_unlock(&s->mutex);
> + si4455_do_work(port);
> + return ret;
> +}
> +
> +static irqreturn_t si4455_ist(int irq, void *dev_id)
Why is this wrapper needed?
> +{
> + struct si4455_port *s = (struct si4455_port *)dev_id;
> + bool handled = false;
> +
> + if (si4455_port_irq(s) == IRQ_HANDLED)
> + handled = true;
> +
> + return IRQ_RETVAL(handled);
A weird way of just doing:
return si4455_port_irq(s);
> +}
...
> +static unsigned int si4455_tx_empty(struct uart_port *port)
> +{
> + return TIOCSER_TEMT;
Always free to send?
> +}
> +
> +static unsigned int si4455_get_mctrl(struct uart_port *port)
> +{
> + dev_dbg(port->dev, "%s", __func__);
> + return TIOCM_DSR | TIOCM_CAR;
And always carrier?
> +}
...
> +static void si4455_start_tx(struct uart_port *port)
> +{
> + struct si4455_one *one = container_of(port,
> + struct si4455_one,
> + port);
> +
> + dev_dbg(port->dev, "%s", __func__);
> +
> + if (!work_pending(&one->tx_work))
This if is pointless.
> + schedule_work(&one->tx_work);
> +}
...
> +static int __maybe_unused si4455_suspend(struct device *dev)
> +{
> + struct si4455_port *s = dev_get_drvdata(dev);
> +
> + uart_suspend_port(&si4455_uart, &s->one.port);
Newline here.
> + return 0;
> +}
...
> +static int si4455_probe(struct device *dev,
> + int irq)
> +{
> + int ret;
> + struct si4455_port *s;
> + const void *of_ptr;
> + char ez_fw_name[255] = { 0 };
> + const struct firmware *ez_fw = NULL;
> +
> + /* Alloc port structure */
> + dev_dbg(dev, "%s\n", __func__);
> + s = devm_kzalloc(dev, sizeof(*s), GFP_KERNEL);
> + if (!s) {
> + dev_err(dev, "Error allocating port structure\n");
> + return -ENOMEM;
> + }
> +
> + dev_set_drvdata(dev, s);
> + mutex_init(&s->mutex);
> +
> + s->shdn_gpio = devm_gpiod_get(dev, "shutdown", GPIOD_OUT_HIGH);
> + if (IS_ERR(s->shdn_gpio)) {
> + dev_err(dev, "Unable to reguest shdn gpio\n");
> + ret = -EINVAL;
> + goto out_generic;
> + }
> +
> + of_ptr = of_get_property(dev->of_node, "silabs,package-size", NULL);
> + if (IS_ERR_OR_NULL(of_ptr)) {
> + dev_err(dev, "dt silabs,package-size property not present\n");
> + ret = -EINVAL;
> + goto out_generic;
> + }
> + s->package_size = be32_to_cpup(of_ptr);
> + if (s->package_size > SI4455_FIFO_SIZE) {
> + dev_err(dev, "dt silabs,package-size property maximum is %i\n", SI4455_FIFO_SIZE);
> + ret = -EINVAL;
> + goto out_generic;
> + }
> +
> + of_ptr = of_get_property(dev->of_node, "silabs,tx-channel", NULL);
> + if (IS_ERR_OR_NULL(of_ptr)) {
> + dev_err(dev, "dt silabs,tx-channel property not present\n");
> + ret = -EINVAL;
> + goto out_generic;
> + }
> + s->tx_channel = be32_to_cpup(of_ptr);
> +
> + of_ptr = of_get_property(dev->of_node, "silabs,rx-channel", NULL);
> + if (IS_ERR_OR_NULL(of_ptr)) {
> + dev_err(dev, "dt silabs,rx-channel property not present\n");
> + ret = -EINVAL;
> + goto out_generic;
> + }
> + s->rx_channel = be32_to_cpup(of_ptr);
> +
> + of_ptr = of_get_property(dev->of_node, "silabs,ez-config", NULL);
> + if (IS_ERR_OR_NULL(of_ptr)) {
> + dev_err(dev, "dt silabs,ez-config property not present\n");
> + ret = -EINVAL;
> + goto out_generic;
> + }
> + strncpy(ez_fw_name, of_ptr, sizeof(ez_fw_name) - 1);
> +
> + /* Initialize port data */
> + s->one.port.dev = dev;
> + s->one.port.line = 0;
> + s->one.port.irq = irq;
> + s->one.port.type = PORT_SI4455;
> + s->one.port.fifosize = SI4455_FIFO_SIZE;
> + s->one.port.flags = UPF_FIXED_TYPE | UPF_LOW_LATENCY;
> + s->one.port.iotype = UPIO_PORT;
> + s->one.port.iobase = 0x00;
> + s->one.port.membase = (void __iomem *)~0;
This is a cast of int to a pointer, isn't it?
> + s->one.port.rs485_config = NULL;
> + s->one.port.ops = &si4455_ops;
> +
> + si4455_s_power(dev, 1);
> +
> + //detect
> + ret = si4455_get_part_info(&s->one.port, &s->part_info);
> + dev_dbg(dev, "si4455_get_part_info()==%i", ret);
> + if (ret == 0) {
> + dev_dbg(dev, "partInfo.chip_rev= %u", s->part_info.chip_rev);
> + dev_dbg(dev, "partInfo.part= %u", s->part_info.part);
> + dev_dbg(dev, "partInfo.pbuild= %u", s->part_info.pbuild);
> + dev_dbg(dev, "partInfo.id= %u", s->part_info.id);
> + dev_dbg(dev, "partInfo.customer= %u", s->part_info.customer);
> + dev_dbg(dev, "partInfo.rom_id= %u", s->part_info.rom_id);
> + dev_dbg(dev, "partInfo.bond= %u", s->part_info.bond);
> + if (s->part_info.part != 0x5544) {
> + dev_err(dev, "part(%u) error", s->part_info.part);
> + ret = -ENODEV;
> + }
> + }
> +
> + if (ret)
> + goto out_generic;
> +
> + ret = request_firmware(&ez_fw, ez_fw_name, dev);
> + if (ret) {
> + dev_err(dev, "firmware(%s) request error(%i)\n", ez_fw_name, ret);
> + ret = -EINVAL;
> + goto out_generic;
> + }
> +
> + ret = si4455_re_configure(&s->one.port, ez_fw);
> + release_firmware(ez_fw);
> + if (ret) {
> + dev_err(dev, "device configuration error(%i)\n", ret);
> + ret = -EINVAL;
> + goto out_generic;
> + }
> +
> + /* Initialize queue for start TX */
> + INIT_WORK(&s->one.tx_work, si4455_tx_proc);
Isn't this too late? It might not be right now. But I would do it right
after allocation, along with others above.
> +
> + /* Register port */
> + ret = uart_add_one_port(&si4455_uart, &s->one.port);
> + if (ret) {
> + s->one.port.dev = NULL;
> + goto out_uart;
> + }
> +
> + ret = sysfs_create_group(&dev->kobj, &si4455_attr_group);
> + if (ret) {
> + dev_err(dev, "sysfs_create_group error(%i)\n", ret);
> + goto out_uart;
> + }
> + /* Setup interrupt */
> + ret = devm_request_threaded_irq(dev, irq, NULL, si4455_ist,
> + IRQF_ONESHOT | IRQF_SHARED,
> + dev_name(dev), s);
> +
> + if (!ret)
> + return 0;
> +
> + dev_err(dev, "Unable to reguest IRQ %i\n", irq);
> + sysfs_remove_group(&dev->kobj, &si4455_attr_group);
> +
> +out_uart:
> + uart_remove_one_port(&si4455_uart, &s->one.port);
> +out_generic:
> + mutex_destroy(&s->mutex);
> + si4455_s_power(dev, 0);
> +
> + return ret;
> +}
...
> +static int __init si4455_uart_init(void)
> +{
> + int ret;
> +
> + ret = uart_register_driver(&si4455_uart);
> + if (ret)
> + return ret;
> + spi_register_driver(&si4455_spi_driver);
spi_register_driver can fail too.
> +
> + return 0;
> +}
regards,
--
js
Powered by blists - more mailing lists