linux-kernel - Re: [PATCH v2 08/14] spi: bcm63xx-hsspi: Handle cs

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]

Date:   Thu, 26 Jan 2023 16:12:48 +0100
From:   Jonas Gorski <jonas.gorski@...il.com>
To:     William Zhang <william.zhang@...adcom.com>
Cc:     Linux SPI List <linux-spi@...r.kernel.org>,
        Broadcom Kernel List <bcm-kernel-feedback-list@...adcom.com>,
        tomer.yacoby@...adcom.com, kursad.oney@...adcom.com,
        dregan@...l.com, f.fainelli@...il.com, anand.gore@...adcom.com,
        dan.beygelman@...adcom.com, joel.peshkin@...adcom.com,
        Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 08/14] spi: bcm63xx-hsspi: Handle cs_change correctly

On Tue, 24 Jan 2023 at 23:33, William Zhang <william.zhang@...adcom.com> wrote:
>
> The kernel SPI interface includes the cs_change flag that alters how
> the CS behaves.
>
> If we're in the middle of transfers, it tells us to unselect the
> CS momentarily since the target device requires that.
>
> If we're at the end of a transfer, it tells us to keep the CS
> selected, perhaps because the next transfer is likely targeted
> to the same device.
>
> We implement this scheme in the HSSPI driver in this change.
>
> Prior to this change, the CS would toggle momentarily if cs_change
> was set for the last transfer. This can be ignored by some or
> most devices, but the Microchip TPM2 device does not ignore it.
>
> With the change, the behavior is corrected and the 'glitch' is
> eliminated.
>
> Signed-off-by: Kursad Oney <kursad.oney@...adcom.com>
> Signed-off-by: William Zhang <william.zhang@...adcom.com>
>
> ---
>
> Changes in v2:
> - Fix unused variable ‘reg’ compile warning
>
>  drivers/spi/spi-bcm63xx-hsspi.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/spi/spi-bcm63xx-hsspi.c b/drivers/spi/spi-bcm63xx-hsspi.c
> index 55cbe7deba08..696e14abba2d 100644
> --- a/drivers/spi/spi-bcm63xx-hsspi.c
> +++ b/drivers/spi/spi-bcm63xx-hsspi.c
> @@ -338,7 +338,7 @@ static int bcm63xx_hsspi_transfer_one(struct spi_master *master,
>         struct spi_device *spi = msg->spi;
>         int status = -EINVAL;
>         int dummy_cs;
> -       u32 reg;
> +       bool restore_polarity = true;

While restore polarity is how this is implemented, I think using a
more semantic name like keep_cs would be better.

>
>         mutex_lock(&bs->msg_mutex);
>         /* This controller does not support keeping CS active during idle.
> @@ -367,16 +367,29 @@ static int bcm63xx_hsspi_transfer_one(struct spi_master *master,
>
>                 spi_transfer_delay_exec(t);
>
> -               if (t->cs_change)
> +               /*
> +                * cs_change rules:
> +                * (1) cs_change = 0 && last_xfer = 0:
> +                *     Do not touch the CS. On to the next xfer.
> +                * (2) cs_change = 1 && last_xfer = 0:
> +                *     Set cs = false before the next xfer.
> +                * (3) cs_change = 0 && last_xfer = 1:
> +                *     We want CS to be deactivated. So do NOT set cs = false,
> +                *     instead just restore the original polarity. This has the
> +                *     same effect of deactivating the CS.
> +                * (4) cs_change = 1 && last_xfer = 1:
> +                *     We want to keep CS active. So do NOT set cs = false, and
> +                *     make sure we do NOT reverse polarity.
> +                */
> +               if (t->cs_change && !list_is_last(&t->transfer_list, &msg->transfers))
>                         bcm63xx_hsspi_set_cs(bs, spi->chip_select, false);
> +
> +               restore_polarity = !t->cs_change;
>         }

I still find setting restore_polarity on each loop iteration when only
its last set value matters confusing and hard to read, so I still
propose keeping close to the generic implementation (
https://elixir.bootlin.com/linux/v6.1.8/source/drivers/spi/spi.c#L1560
) and do

if (t->cs_change) {
   if (list_is_last())
       restore_polarity = false;
   else
       bcm63xx_hsspi_set_cs(bs, spi->chip_select, false);
}

While there, you might also want to check the cs_off value(s) as well.



>
> -       mutex_lock(&bs->bus_mutex);
> -       reg = __raw_readl(bs->regs + HSSPI_GLOBAL_CTRL_REG);
> -       reg &= ~GLOBAL_CTRL_CS_POLARITY_MASK;
> -       reg |= bs->cs_polarity;
> -       __raw_writel(reg, bs->regs + HSSPI_GLOBAL_CTRL_REG);
> -       mutex_unlock(&bs->bus_mutex);
> +       bcm63xx_hsspi_set_cs(bs, dummy_cs, false);
> +       if (restore_polarity)
> +               bcm63xx_hsspi_set_cs(bs, spi->chip_select, false);
>
>         mutex_unlock(&bs->msg_mutex);
>         msg->status = status;
> --
> 2.37.3
>