Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20200917122558.23110-1-rojay@codeaurora.org>
References: <20200917122558.23110-1-rojay@codeaurora.org>
Subject: Re: [PATCH V4] i2c: i2c-qcom-geni: Add shutdown callback for i2c
From:   Stephen Boyd <swboyd@chromium.org>
Cc:     dianders@chromium.org, saiprakash.ranjan@codeaurora.org,
        gregkh@linuxfoundation.org, mka@chromium.org,
        akashast@codeaurora.org, msavaliy@qti.qualcomm.com,
        skakit@codeaurora.org, vkaur@codeaurora.org,
        pyarlaga@codeaurora.org, rnayak@codeaurora.org, agross@kernel.org,
        bjorn.andersson@linaro.org, linux-arm-msm@vger.kernel.org,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
        sumit.semwal@linaro.org, linux-media@vger.kernel.org,
        Roja Rani Yarubandi <rojay@codeaurora.org>
To:     Roja Rani Yarubandi <rojay@codeaurora.org>, wsa@kernel.org
Date:   Thu, 17 Sep 2020 13:23:30 -0700
Message-ID: <160037421089.4188128.9425314091585708436@swboyd.mtv.corp.google.com>
User-Agent: alot/0.9.1
Precedence: bulk

Quoting Roja Rani Yarubandi (2020-09-17 05:25:58)
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-=
qcom-geni.c
> index dead5db3315a..b0d8043c8cb2 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -86,6 +86,10 @@ struct geni_i2c_dev {
>         u32 clk_freq_out;
>         const struct geni_i2c_clk_fld *clk_fld;
>         int suspended;
> +       void *dma_buf;
> +       size_t xfer_len;
> +       dma_addr_t tx_dma;
> +       dma_addr_t rx_dma;

Do we need both tx_dma and rx_dma? Seems that we use cur->flags to
figure out if the transfer is tx or rx so we could have juat dma_buf and
dma_addr here?

>  };
> =20
>  struct geni_i2c_err_log {
> @@ -307,7 +311,6 @@ static void geni_i2c_abort_xfer(struct geni_i2c_dev *=
gi2c)
> =20
>         spin_lock_irqsave(&gi2c->lock, flags);
>         geni_i2c_err(gi2c, GENI_TIMEOUT);
> -       gi2c->cur =3D NULL;

This looks concerning. We're moving this out from under the spinlock.
The irq handler in this driver seems to hold the spinlock all the time
while processing and this function grabs it here to keep cur consistent
when aborting the transfer due to a timeout. Otherwise it looks like the
irqhandler can race with this and try to complete the transfer while
it's being torn down here.

>         geni_se_abort_m_cmd(&gi2c->se);
>         spin_unlock_irqrestore(&gi2c->lock, flags);
>         do {
> @@ -349,10 +352,62 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev=
 *gi2c)
>                 dev_err(gi2c->se.dev, "Timeout resetting TX_FSM\n");
>  }
> =20
> +static void geni_i2c_rx_msg_cleanup(struct geni_i2c_dev *gi2c)

So maybe pass cur to this function?

> +{
> +       struct geni_se *se =3D &gi2c->se;
> +
> +       gi2c->cur_rd =3D 0;
> +       if (gi2c->dma_buf) {
> +               if (gi2c->err)
> +                       geni_i2c_rx_fsm_rst(gi2c);
> +               geni_se_rx_dma_unprep(se, gi2c->rx_dma, gi2c->xfer_len);
> +               i2c_put_dma_safe_msg_buf(gi2c->dma_buf, gi2c->cur, !gi2c-=
>err);
> +       }
> +}
> +
> +static void geni_i2c_tx_msg_cleanup(struct geni_i2c_dev *gi2c)

And this one?

> +{
> +       struct geni_se *se =3D &gi2c->se;
> +
> +       gi2c->cur_wr =3D 0;
> +       if (gi2c->dma_buf) {
> +               if (gi2c->err)
> +                       geni_i2c_tx_fsm_rst(gi2c);
> +               geni_se_tx_dma_unprep(se, gi2c->tx_dma, gi2c->xfer_len);
> +               i2c_put_dma_safe_msg_buf(gi2c->dma_buf, gi2c->cur, !gi2c-=
>err);
> +       }
> +}
> +
> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
> +{
> +       int ret;
> +       u32 geni_status;
> +
> +       /* Resume device, as runtime suspend can happen anytime during tr=
ansfer */
> +       ret =3D pm_runtime_get_sync(gi2c->se.dev);
> +       if (ret < 0) {
> +               dev_err(gi2c->se.dev, "Failed to resume device: %d\n", re=
t);
> +               return;
> +       }
> +
> +       geni_status =3D readl_relaxed(gi2c->se.base + SE_GENI_STATUS);

And this probably needs to hold the lock?

> +       if (!(geni_status & M_GENI_CMD_ACTIVE))
> +               goto out;
> +
> +       geni_i2c_abort_xfer(gi2c);
> +       if (gi2c->cur->flags & I2C_M_RD)
> +               geni_i2c_rx_msg_cleanup(gi2c);
> +       else
> +               geni_i2c_tx_msg_cleanup(gi2c);
> +       gi2c->cur =3D NULL;

until here?

> +out:
> +       pm_runtime_put_sync_suspend(gi2c->se.dev);
> +}
> +
>  static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg=
 *msg,
>                                 u32 m_param)
>  {
> -       dma_addr_t rx_dma;
> +       dma_addr_t rx_dma =3D 0;
>         unsigned long time_left;
>         void *dma_buf =3D NULL;
>         struct geni_se *se =3D &gi2c->se;
> @@ -372,6 +427,10 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *=
gi2c, struct i2c_msg *msg,
>                 geni_se_select_mode(se, GENI_SE_FIFO);
>                 i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
>                 dma_buf =3D NULL;
> +       } else {
> +               gi2c->xfer_len =3D len;
> +               gi2c->rx_dma =3D rx_dma;
> +               gi2c->dma_buf =3D dma_buf;
>         }
> =20
>         geni_se_setup_m_cmd(se, I2C_READ, m_param);
> @@ -380,13 +439,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *=
gi2c, struct i2c_msg *msg,
>         if (!time_left)
>                 geni_i2c_abort_xfer(gi2c);
> =20
> -       gi2c->cur_rd =3D 0;
> -       if (dma_buf) {
> -               if (gi2c->err)
> -                       geni_i2c_rx_fsm_rst(gi2c);
> -               geni_se_rx_dma_unprep(se, rx_dma, len);
> -               i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
> -       }
> +       geni_i2c_rx_msg_cleanup(gi2c);
> =20
>         return gi2c->err;
>  }

It may make sense to extract the cleanup stuff into another patch. Then
have a patch after that which does the shutdown hook. So three patches
total.

> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni=
-se.c
> index d0e4f520cff8..0216b38c1e9a 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -705,7 +705,7 @@ void geni_se_tx_dma_unprep(struct geni_se *se, dma_ad=
dr_t iova, size_t len)
>  {
>         struct geni_wrapper *wrapper =3D se->wrapper;
> =20
> -       if (iova && !dma_mapping_error(wrapper->dev, iova))
> +       if (!dma_mapping_error(wrapper->dev, iova))
>                 dma_unmap_single(wrapper->dev, iova, len, DMA_TO_DEVICE);
>  }
>  EXPORT_SYMBOL(geni_se_tx_dma_unprep);
> @@ -722,7 +722,7 @@ void geni_se_rx_dma_unprep(struct geni_se *se, dma_ad=
dr_t iova, size_t len)
>  {
>         struct geni_wrapper *wrapper =3D se->wrapper;
> =20
> -       if (iova && !dma_mapping_error(wrapper->dev, iova))
> +       if (!dma_mapping_error(wrapper->dev, iova))
>                 dma_unmap_single(wrapper->dev, iova, len, DMA_FROM_DEVICE=
);
>  }
>  EXPORT_SYMBOL(geni_se_rx_dma_unprep);

I'd make this a different patch. Nothing depends on this change, right?