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: <f6cb2d7bc02dab409030ec42bf1d17c9@codeaurora.org>
References: <20200917122558.23110-1-rojay@codeaurora.org> <160037421089.4188128.9425314091585708436@swboyd.mtv.corp.google.com> <f6cb2d7bc02dab409030ec42bf1d17c9@codeaurora.org>
Subject: Re: [PATCH V4] i2c: i2c-qcom-geni: Add shutdown callback for i2c
From:   Stephen Boyd <swboyd@chromium.org>
Cc:     wsa@kernel.org, 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
To:     rojay@codeaurora.org
Date:   Mon, 21 Sep 2020 13:12:56 -0700
Message-ID: <160071917655.4188128.4175000228517858211@swboyd.mtv.corp.google.com>
User-Agent: alot/0.9.1
Precedence: bulk

Quoting rojay@codeaurora.org (2020-09-21 04:21:04)
> Hi Stephen,
>=20
> On 2020-09-18 01:53, Stephen Boyd wrote:
> > Quoting Roja Rani Yarubandi (2020-09-17 05:25:58)
> >> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c=20
> >> 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
> >>  };
> >>=20
> >>  struct geni_i2c_err_log {
> >> @@ -307,7 +311,6 @@ static void geni_i2c_abort_xfer(struct=20
> >> geni_i2c_dev *gi2c)
> >>=20
> >>         spin_lock_irqsave(&gi2c->lock, flags);
> >>         geni_i2c_err(gi2c, GENI_TIMEOUT);
> >> -       gi2c->cur =3D NULL;
> >=20
> > 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=20
> > the
> > irqhandler can race with this and try to complete the transfer while
> > it's being torn down here.
> >=20
> >>         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=20
> >> 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)
> >=20
> > So maybe pass cur to this function?
> >=20
>=20
> Sorry, i did not understand why to pass cur to this function?

I'm suggesting to copy the cur data out of the gi2c pointer and then
pass it to these functions so that it can't race with another transfer.
Something like an atomic exchange may work. I haven't thought about it
deeply, but we need to make sure the irq handler can't race with the
cleanup functions.