On Fr, 2019-01-04 at 12:21 +0100, Andreas Färber wrote:
>
> +struct picogw_device {
> + struct serdev_device *serdev;
> +
> + u8 rx_buf[1024];
No, you cannot do that. AFAICT this buffer can be used for DMA.
Thus putting it into another data structure violates the rules
of DMA coherency. You must allocate it separately.
> +static int picogw_send_cmd(struct picogw_device *picodev, char cmd,
> + u8 addr, const void *data, u16 data_len)
> +{
> + struct serdev_device *sdev = picodev->serdev;
> + u8 buf[4];
> + int i, ret;
> +
> + buf[0] = cmd;
> + buf[1] = (data_len >> 8) & 0xff;
> + buf[2] = (data_len >> 0) & 0xff;
We have macros for converting endianness and unaligned access.
> + buf[3] = addr;
> +
> + dev_dbg(&sdev->dev, "%s: %c %02x %02x %02x\n", __func__, buf[0],
> + (unsigned int)buf[1], (unsigned int)buf[2], (unsigned int)buf[3]);
> + for (i = 0; i < data_len; i++) {
> + dev_dbg(&sdev->dev, "%s: data %02x\n", __func__, (unsigned int)((const u8*)data)[i]);
> + }
> +
> + ret = serdev_device_write_buf(sdev, buf, 4);
> + if (ret < 0)
> + return ret;
> + if (ret != 4)
> + return -EIO;
> +
> + if (data_len) {
> + ret = serdev_device_write_buf(sdev, data, data_len);
> + if (ret < 0)
> + return ret;
> + if (ret != data_len)
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static int picogw_recv_answer(struct picogw_device *picodev,
> + char *cmd, bool *ack, void *buf, int buf_len,
> + unsigned long timeout)
> +{
> + int len;
> +
> + timeout = wait_for_completion_timeout(&picodev->answer_comp, timeout);
> + if (!timeout)
> + return -ETIMEDOUT;
And? The IO is still scheduled. Simply erroring out is problematic.
If you see a timeout you need to kill the scheduled IO.
> +static int picogw_handle_answer(struct picogw_device *picodev)
> +{
> + struct device *dev = &picodev->serdev->dev;
> + unsigned int data_len = ((u16)picodev->rx_buf[1] << 8) | picodev->rx_buf[2];
> + int cmd_len = 4 + data_len;
> + int i, ret;
> +
> + if (picodev->rx_len < 4)
> + return 0;
> +
> + if (cmd_len > sizeof(picodev->rx_buf)) {
> + dev_warn(dev, "answer too long (%u)\n", data_len);
> + return 0;
> + }
> +
> + if (picodev->rx_len < cmd_len) {
> + dev_dbg(dev, "got %u, need %u bytes\n", picodev->rx_len, cmd_len);
> + return 0;
> + }
> +
> + dev_dbg(dev, "Answer %c =%u %s (%u)\n", picodev->rx_buf[0],
> + (unsigned int)picodev->rx_buf[3],
> + (picodev->rx_buf[3] == 1) ? "OK" : "K0",
> + data_len);
> + for (i = 0; i < data_len; i++) {
> + //dev_dbg(dev, "%s: %02x\n", __func__, (unsigned int)picodev->rx_buf[4 + i]);
> + }
> +
> + complete(&picodev->answer_comp);
> + ret = wait_for_completion_interruptible_timeout(&picodev->answer_read_comp, HZ / 2);
Problematic. You have no idea when that complete() will have an effect.
You are operating with an undefined timeout here. Theoretically it
could be negative.
Regards
Oliver
