driver ignores SPI bus lock

[LouisLambda]

I have a device that the kernel reliably fails to initialize when using the driver. After debugging, it looks like it has to do with the driver taking a lock via master->bus_lock_flag during initialization, which somehow changes the bus traffic, I suspect it has to do with CS but haven't pinned it down.

To check, I implemented a simplified version of set_cs/transfer_one so that I could test the default kernel impl of transfer_one_message. The problem immediately went way.

To make sure I correctly identified the problem, I modified transfer_one_message from the project so it does not release CS if the bus is locked - again, the problem went away.

Please have a look at this. Here's my modified version, if it helps:

static int ch341_spi_transfer_one_message(struct spi_master *master,
                      struct spi_message *m)
{
    struct ch341_device *dev = spi_master_get_devdata(master);
    struct spi_device *spi = m->spi;
    struct spi_client *client = &dev->spi_clients[spi->chip_select];
    struct gpio_desc *cs;
    bool lsb = spi->mode & SPI_LSB_FIRST;
    struct spi_transfer *xfer;
    unsigned int tx_len = 0;
    unsigned int buf_idx = 0;
    int status;
    bool bus_is_locked = false;
    unsigned long flags;

    spin_lock_irqsave(&master->bus_lock_spinlock, flags);
    bus_is_locked = master->bus_lock_flag;
    spin_unlock_irqrestore(&master->bus_lock_spinlock, flags);

    if (spi->mode & SPI_NO_CS) {
        cs = NULL;
    } else {
        cs = client->gpio;

        if (spi->mode & SPI_CS_HIGH)
            gpiod_set_value_cansleep(cs, 1);
        else
            gpiod_set_value_cansleep(cs, 0);
    }
    list_for_each_entry(xfer, &m->transfers, transfer_list) {;
        buf_idx = copy_to_device(client->buf, buf_idx, xfer->tx_buf, xfer->len, lsb);
        tx_len += xfer->len;
    }

    status = spi_transfer(dev, client->buf, buf_idx, buf_idx - tx_len);
    if (cs && !bus_is_locked) {
        if (spi->mode & SPI_CS_HIGH)
            gpiod_set_value_cansleep(cs, 0);
        else
            gpiod_set_value_cansleep(cs, 1);
    }

    if (status >= 0) {
        buf_idx = 0;
        list_for_each_entry(xfer, &m->transfers, transfer_list)
            buf_idx = copy_from_device(xfer->rx_buf, client->buf,
                           buf_idx, xfer->len, lsb);

        m->actual_length = tx_len;
        status = 0;
    }

    m->status = status;
    spi_finalize_current_message(master);

    return 0;
}

[frank-zago]

Some spi devices take the release of their chip select line as some sort of reset. That may be what's happening. However since no other spi driver is even checking the bus_lock_flag, I think this is not the right fix. It's possible that the ch341 driver doesn't handle the cs mode properly.

[LouisLambda]

The comments in include/linux/spi/spi.h about cs_change are starting to make more sense.

To keep CS high, the protocol driver needs to set cs_change=0 for the last transfer in a message and take a lock on the bus so no other device gets talked to in the meantime (which would de-assert the CS left active)

So you're right, the SPI controller driver doesn't need to know about the lock.

I did add the cs_change logic to ch341_transfer_one_message and at the time I thought it didn't solve the issue. But now I seem to recall it did change the error I was getting, and that was due to an unrelated issue.

So it's probably just #9 that needs fixing.