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: <20200907130731.2607-1-rojay@codeaurora.org>
References: <20200907130731.2607-1-rojay@codeaurora.org>
Subject: Re: [PATCH V3] 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, 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,
        dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org,
        Roja Rani Yarubandi <rojay@codeaurora.org>
To:     Roja Rani Yarubandi <rojay@codeaurora.org>, wsa@kernel.org
Date:   Tue, 08 Sep 2020 12:30:18 -0700
Message-ID: <159959341894.454335.3250696075143737399@swboyd.mtv.corp.google.com>
User-Agent: alot/0.9.1
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Why is dri-devel on here? And linaro-mm-sig?

Quoting Roja Rani Yarubandi (2020-09-07 06:07:31)
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-=
qcom-geni.c
> index dead5db3315a..b3609760909f 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>  struct geni_i2c_err_log {
> @@ -384,7 +387,8 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *g=
i2c, struct i2c_msg *msg,
>         if (dma_buf) {
>                 if (gi2c->err)
>                         geni_i2c_rx_fsm_rst(gi2c);
> -               geni_se_rx_dma_unprep(se, rx_dma, len);
> +               geni_se_rx_dma_unprep(se, gi2c->rx_dma, len);
> +               gi2c->rx_dma =3D (dma_addr_t)NULL;
>                 i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
>         }
> =20
> @@ -394,12 +398,12 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev =
*gi2c, struct i2c_msg *msg,
>  static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg=
 *msg,
>                                 u32 m_param)
>  {
> -       dma_addr_t tx_dma;
>         unsigned long time_left;
>         void *dma_buf =3D NULL;
>         struct geni_se *se =3D &gi2c->se;
>         size_t len =3D msg->len;
> =20
> +       gi2c->xfer_len =3D len;
>         if (!of_machine_is_compatible("lenovo,yoga-c630"))
>                 dma_buf =3D i2c_get_dma_safe_msg_buf(msg, 32);
> =20
> @@ -410,7 +414,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *g=
i2c, struct i2c_msg *msg,
> =20
>         writel_relaxed(len, se->base + SE_I2C_TX_TRANS_LEN);
> =20
> -       if (dma_buf && geni_se_tx_dma_prep(se, dma_buf, len, &tx_dma)) {
> +       if (dma_buf && geni_se_tx_dma_prep(se, dma_buf, len, &gi2c->tx_dm=
a)) {
>                 geni_se_select_mode(se, GENI_SE_FIFO);
>                 i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
>                 dma_buf =3D NULL;
> @@ -429,7 +433,8 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *g=
i2c, struct i2c_msg *msg,
>         if (dma_buf) {
>                 if (gi2c->err)
>                         geni_i2c_tx_fsm_rst(gi2c);
> -               geni_se_tx_dma_unprep(se, tx_dma, len);
> +               geni_se_tx_dma_unprep(se, gi2c->tx_dma, len);
> +               gi2c->tx_dma =3D (dma_addr_t)NULL;
>                 i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
>         }
> =20
> @@ -479,6 +484,51 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>         return ret;
>  }
> =20
> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
> +{
> +       int ret;
> +       u32 dma;
> +       u32 val;
> +       u32 geni_status;
> +       struct geni_se *se =3D &gi2c->se;
> +
> +       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);

Is this print really necessary? Doesn't PM core already print this sort
of information?

> +               return;
> +       }
> +
> +       geni_status =3D readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
> +       if (geni_status & M_GENI_CMD_ACTIVE) {

Please try to de-indent all this.

	if (!(geni_status & M_GENI_CMD_ACTIVE))
		goto out;

> +               geni_i2c_abort_xfer(gi2c);
> +               dma =3D readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
> +               if (dma) {

	if (!dma)
		goto out;

> +                       val =3D readl_relaxed(gi2c->se.base + SE_DMA_DEBU=
G_REG0);
> +                       if (val & DMA_TX_ACTIVE) {
> +                               gi2c->cur_wr =3D 0;
> +                               if (gi2c->err)
> +                                       geni_i2c_tx_fsm_rst(gi2c);
> +                               if (gi2c->tx_dma) {
> +                                       geni_se_tx_dma_unprep(se,
> +                                                gi2c->tx_dma, gi2c->xfer=
_len);
> +                                       gi2c->tx_dma =3D (dma_addr_t)NULL;

Almost nobody does this. In fact, grep shows me one hit in the kernel.
If nobody else is doing it something is probably wrong. When would dma
mode be active and tx_dma not be set to something that should be
stopped? If it really is necessary I suppose we should assign this to
DMA_MAPPING_ERROR instead of casting NULL. Then the check above for
tx_dma being valid can be dropped because geni_se_tx_dma_unprep()
already checks for a valid mapping before doing anything. But really, we
should probably be tracking the dma buffer mapped to the CPU as well as
the dma address that was used for the mapping. Not storing both is a
problem, see below.

> +                               }
> +                       } else if (val & DMA_RX_ACTIVE) {
> +                               gi2c->cur_rd =3D 0;
> +                               if (gi2c->err)
> +                                       geni_i2c_rx_fsm_rst(gi2c);
> +                               if (gi2c->rx_dma) {
> +                                       geni_se_rx_dma_unprep(se,
> +                                               gi2c->rx_dma, gi2c->xfer_=
len);

Looking closely it seems that the geni dma wrappers shouldn't even be
checking for an iova being non-zero. Instead they should make sure that
it just isn't invalid with !dma_mapping_error().

> +                                       gi2c->rx_dma =3D (dma_addr_t)NULL;

If we're stopping some dma transaction doesn't that mean the=20

                 i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);

code needs to run also? I fail to see where we free the buffer that has
been mapped for DMA.

> +                               }
> +                       }
> +               }
> +       }
> +

out:

> +       pm_runtime_put_sync_suspend(gi2c->se.dev);
> +}
> +