| 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: <96264209-b0a5-cf76-06cc-eb067f4a4c7b@quicinc.com>
Date: Thu, 8 Dec 2022 20:24:57 +0530
From: Vijaya Krishna Nivarthi <quic_vnivarth@...cinc.com>
To: Doug Anderson <dianders@...omium.org>
CC: <agross@...nel.org>, <andersson@...nel.org>,
<konrad.dybcio@...aro.org>, <broonie@...nel.org>,
<linux-arm-msm@...r.kernel.org>, <linux-spi@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <quic_msavaliy@...cinc.com>,
<mka@...omium.org>, <swboyd@...omium.org>,
<quic_vtanuku@...cinc.com>, <vkoul@...nel.org>
Subject: Re: [V3] spi: spi-geni-qcom: Add support for SE DMA mode
Hi,
Uploaded V4.
On 12/2/2022 4:10 AM, Doug Anderson wrote:
> Hi,
>
> On Tue, Nov 29, 2022 at 1:23 AM Vijaya Krishna Nivarthi
> <quic_vnivarth@...cinc.com> wrote:
>> @@ -95,6 +97,7 @@ struct spi_geni_master {
>> struct dma_chan *tx;
>> struct dma_chan *rx;
>> int cur_xfer_mode;
>> + u32 cur_m_cmd;
> In v1, I said: "I don't think you need to store "cur_m_cmd" ..."
> ...you responded: Please note that cur_xfer can be NULL. Added further
> to comments."
>
> I don't see any comments about this.
>
> In any case, I'm still unclear about why this is needed. I guess
> you're looking at the code in handle_se_timeout(). I'll comment there.
>
Removed cur_m_cmd
>> @@ -162,6 +169,45 @@ static void handle_fifo_timeout(struct spi_master *spi,
>> */
>> mas->abort_failed = true;
>> }
>> +
>> +unmap_if_dma:
>> + if (mas->cur_xfer_mode == GENI_SE_DMA) {
>> + if (mas->cur_m_cmd & SPI_TX_ONLY) {
>> + spin_lock_irq(&mas->lock);
>> + reinit_completion(&mas->tx_reset_done);
>> + writel(1, se->base + SE_DMA_TX_FSM_RST);
>> + spin_unlock_irq(&mas->lock);
>> + time_left = wait_for_completion_timeout(&mas->tx_reset_done, HZ);
>> + if (!time_left)
>> + dev_err(mas->dev, "DMA TX RESET failed\n");
>> + }
>> + if (mas->cur_m_cmd & SPI_RX_ONLY) {
>> + spin_lock_irq(&mas->lock);
>> + reinit_completion(&mas->rx_reset_done);
>> + writel(1, se->base + SE_DMA_RX_FSM_RST);
>> + spin_unlock_irq(&mas->lock);
>> + time_left = wait_for_completion_timeout(&mas->rx_reset_done, HZ);
>> + if (!time_left)
>> + dev_err(mas->dev, "DMA RX RESET failed\n");
>> + }
>> +
>> + if (xfer) {
>> + if (xfer->tx_buf && xfer->tx_dma)
>> + geni_se_tx_dma_unprep(se, xfer->tx_dma, xfer->len);
>> + if (xfer->rx_buf && xfer->rx_dma)
>> + geni_se_rx_dma_unprep(se, xfer->rx_dma, xfer->len);
>> + } else {
>> + /*
>> + * This can happen if a timeout happened and we had to wait
>> + * for lock in this function because isr was holding the lock
>> + * and handling transfer completion at that time.
>> + * isr will set cur_xfer to NULL when done.
>> + * Unnecessary error but cannot be helped.
>> + * Only do reset, dma_unprep is already done by isr.
>> + */
>> + dev_err(mas->dev, "Cancel/Abort on completed SPI transfer\n");
>> + }
> For the above block of code, if "xfer" is NULL then do we actually
> need to issue the DMA TX Reset and the DMA RX Reset? As per your
> comments, the only case "xfer" can be NULL is if the ISR was holding
> the lock and handling the transfer completion at that time. If the ISR
> handled the transfer completion then we're not actually in a bad
> state, right? Thus, couldn't you do:
>
> if (xfer) {
> if (xfer->tx_buf && xfer->tx_dma) {
> // Do the FSM reset
> // Unprepare the DMA
> }
> if (xfer->rx_buf && xfer->rx_dma) {
> // Do the FSM reset
> // Unprepare the DMA
> }
> } else {
> dev_err(...);
> }
>
> That should be fine, right? ...and then we can get rid of the need for
> "cur_m_cmd" as per my previous comment, right?
>
> I'll also ask if we can downgrade the "dev_err" to a "dev_warn". I
> usually reserve dev_err for things that are fatal. Here we think we'll
> probably recover, right?
>
Done.
>> @@ -778,11 +836,39 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
>> */
>> spin_lock_irq(&mas->lock);
>> geni_se_setup_m_cmd(se, m_cmd, FRAGMENTATION);
>> - if (m_cmd & SPI_TX_ONLY) {
>> +
>> + if (mas->cur_xfer_mode == GENI_SE_DMA) {
>> + if (m_cmd & SPI_RX_ONLY) {
>> + ret = geni_se_rx_dma_prep(se, xfer->rx_buf,
>> + xfer->len, &xfer->rx_dma);
> In response to v1 I asked if it's really OK to use "xfer->rx_dma" for
> your purposes since it's supposed to be managed by the SPI framework.
>
> It still makes me nervous to use it, even though it seems to work.
> Since we're using it in an undocumented way, I'd be nervous that the
> SPI framework might change what it's doing and break us in the future.
>
> We can only have one TX and one RX transfer at a time anyway. Why
> don't we just have our own "rx_dma" and "tx_dma" in "struct
> spi_geni_master". It's only 16 extra bytes of data and it would make
> me feel less nervous.
Done.
>
> It still would be nice to eventually use the SPI framework to manage
> the mapping, but I agree that can be a future task.
>
Tracking this with a bug.
>> + if (ret) {
>> + dev_err(mas->dev, "Failed to setup Rx dma %d\n", ret);
>> + xfer->rx_dma = 0;
>> + goto unlock_and_return;
>> + }
>> + }
>> + if (m_cmd & SPI_TX_ONLY) {
>> + ret = geni_se_tx_dma_prep(se, (void *)xfer->tx_buf,
>> + xfer->len, &xfer->tx_dma);
> In v1 I asked about the above "void *" cast. You pointed out that it
> was to cast away constness. So I agree that you can keep it here for
> now, but could you also post a patch to change geni_se_tx_dma_prep()
> to take a "const void *"? You'll need a cast in _that_ function to
> remove the constness (since dma_map_single() is generic for both TX
> and RX), but it seems like a better place for it. Then a later patch
> could remove the cast here.
Tracking this with a bug.
>
>> + if (ret) {
>> + dev_err(mas->dev, "Failed to setup Tx dma %d\n", ret);
>> + xfer->tx_dma = 0;
>> + if (m_cmd & SPI_RX_ONLY && xfer->rx_dma) {
> Don't need "&& xfer->rx_dma". You _just_ mapped it above and if it had
> failed it would have returned an error. you don't need to
> double-check. You can trust that the framework knows what it's doing
> and won't return NULL to you. If it did return NULL to you because of
> a bug, it's not necessarily better to just silently skip unpreparing
> anyway.
Done.
>
>> @@ -823,39 +913,66 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
>>
>> spin_lock(&mas->lock);
>>
>> - if ((m_irq & M_RX_FIFO_WATERMARK_EN) || (m_irq & M_RX_FIFO_LAST_EN))
>> - geni_spi_handle_rx(mas);
>> -
>> - if (m_irq & M_TX_FIFO_WATERMARK_EN)
>> - geni_spi_handle_tx(mas);
>> -
>> - if (m_irq & M_CMD_DONE_EN) {
>> - if (mas->cur_xfer) {
>> + if (mas->cur_xfer_mode == GENI_SE_FIFO) {
>> + if ((m_irq & M_RX_FIFO_WATERMARK_EN) || (m_irq & M_RX_FIFO_LAST_EN))
>> + geni_spi_handle_rx(mas);
>> +
>> + if (m_irq & M_TX_FIFO_WATERMARK_EN)
>> + geni_spi_handle_tx(mas);
>> +
>> + if (m_irq & M_CMD_DONE_EN) {
>> + if (mas->cur_xfer) {
>> + spi_finalize_current_transfer(spi);
>> + mas->cur_xfer = NULL;
>> + /*
>> + * If this happens, then a CMD_DONE came before all the
>> + * Tx buffer bytes were sent out. This is unusual, log
>> + * this condition and disable the WM interrupt to
>> + * prevent the system from stalling due an interrupt
>> + * storm.
>> + *
>> + * If this happens when all Rx bytes haven't been
>> + * received, log the condition. The only known time
>> + * this can happen is if bits_per_word != 8 and some
>> + * registers that expect xfer lengths in num spi_words
>> + * weren't written correctly.
>> + */
>> + if (mas->tx_rem_bytes) {
>> + writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
>> + dev_err(mas->dev, "Premature done. tx_rem = %d bpw%d\n",
>> + mas->tx_rem_bytes, mas->cur_bits_per_word);
>> + }
>> + if (mas->rx_rem_bytes)
>> + dev_err(mas->dev, "Premature done. rx_rem = %d bpw%d\n",
>> + mas->rx_rem_bytes, mas->cur_bits_per_word);
>> + } else {
>> + complete(&mas->cs_done);
> Question: did you try actually using the chip select with your new
> GENI_SE_DMA? Does it work? I ask because I don't see anything that
> completes the "cs_done" in the DMA case of the ISR and I don't see
> anything in spi_geni_set_cs() that forces it to FIFO mode. Note: if
> you're only testing on trogdor/herobrine boards, you'd have to change
> them to not use a GPIO for chip select.
Forced FIFO in spi_geni_set_cs
Thank you,
Vijay/
>
> -Doug
Powered by blists - more mailing lists