linux-kernel - Re: [PATCH] can: mcp251x: convert to half-duplex SPI

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: <0b351fe3-8fe9-572f-fd85-e2aed22873e3@pengutronix.de>
Date:   Wed, 26 Feb 2020 08:37:02 +0100
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Tim Harvey <tharvey@...eworks.com>
Cc:     open list <linux-kernel@...r.kernel.org>,
        linux-can@...r.kernel.org, Wolfgang Grandegger <wg@...ndegger.com>,
        Timo Schlüßler <schluessler@...use.de>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH] can: mcp251x: convert to half-duplex SPI

On 2/25/20 11:25 PM, Tim Harvey wrote:
>> On 2/25/20 7:35 PM, Tim Harvey wrote:
>>> Some SPI host controllers such as the Cavium Thunder do not support
>>> full-duplex SPI. Using half-duplex transfers allows the driver to work
>>> with those host controllers.

Hmmm, at least none of the spi-cavium*.c have HALF_DUPLEX set....

I only find these ones:
> drivers/spi/spi-falcon.c:404:   master->flags = SPI_MASTER_HALF_DUPLEX;
> drivers/spi/spi-lp8841-rtc.c:194:       master->flags = SPI_MASTER_HALF_DUPLEX;
> drivers/spi/spi-mt7621.c:360:   master->flags = SPI_CONTROLLER_HALF_DUPLEX;
> drivers/spi/spi-mxs.c:576:      master->flags = SPI_MASTER_HALF_DUPLEX;
> drivers/spi/spi-omap-uwire.c:490:       master->flags = SPI_MASTER_HALF_DUPLEX;
> drivers/spi/spi-pic32-sqi.c:651:        master->flags           = SPI_MASTER_HALF_DUPLEX;
> drivers/spi/spi-pl022.c:1714:                           SSP_MICROWIRE_CHANNEL_HALF_DUPLEX)) {
> drivers/spi/spi-qcom-qspi.c:478:        master->flags = SPI_MASTER_HALF_DUPLEX;
> drivers/spi/spi-sprd-adi.c:521: ctlr->flags = SPI_MASTER_HALF_DUPLEX;
> drivers/spi/spi-stm32.c:160:#define STM32H7_SPI_HALF_DUPLEX             3
> drivers/spi/spi-stm32.c:1469:           mode = STM32H7_SPI_HALF_DUPLEX;
> drivers/spi/spi-stm32.c:1472:           mode = STM32H7_SPI_HALF_DUPLEX;
> drivers/spi/spi-ti-qspi.c:758:  master->flags = SPI_MASTER_HALF_DUPLEX;
> drivers/spi/spi-xcomm.c:222:    master->flags = SPI_MASTER_HALF_DUPLEX;

>> There are several transfers left in the driver, where both rx_buf and
>> tx_buf are set. How does your host controller driver know which one to
>> handle?

I'm trying to answer my question:
I think the spi host controller sets SPI_MASTER_HALF_DUPLEX if it only
supports half duplex and the spi framework checks that only either TX or
RX is set during one transfer.

> Your right... there is the mcp251x_hw_rx_frame() call that also uses
> spi_rx_buf after a synchronous transfer (I didn't see any others).
> I'll look at this again.

Have you hardware to test your changes? I think the SPI framework would
return an -EINVAL in that case....though the return value is sometimes
not checked by the driver :/

> In general is it an ok approach to switch the driver to half-duplex
> for this issue without the need of complicating things with a
> module/dt param?

AFAICS the chip doesn't do real full duplex. It either send or receives
data at the same time. With splitting one pseudo full duplex transfer
into two half duplex transfers brings some minor overhead, but I think
that's hardly measurable.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |