From: Boris Brezillon <bbrezillon@kernel.org>
To: Przemyslaw Gaj <pgaj@cadence.com>
Cc: linux-i3c@lists.infradead.org, psroka@cadence.com,
rafalc@cadence.com, vitor.soares@synopsys.com
Subject: Re: [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem
Date: Wed, 30 Jan 2019 08:56:26 +0100 [thread overview]
Message-ID: <20190130085626.7576b311@bbrezillon> (raw)
In-Reply-To: <20190129181335.GA8271@global.cadence.com>
On Tue, 29 Jan 2019 18:13:37 +0000
Przemyslaw Gaj <pgaj@cadence.com> wrote:
> The 01/15/2019 22:09, Boris Brezillon wrote:
> > EXTERNAL MAIL
> >
> >
> > On Fri, 11 Jan 2019 17:43:35 +0000
> > Przemyslaw Gaj <pgaj@cadence.com> wrote:
> >
> > > + i3c_bus_normaluse_unlock(i3cbus);
> > > + }
> > > +
> > > i3c_bus_normaluse_lock(i3cbus);
> > > ret = sprintf(buf, "%d-%llx\n", i3cbus->id,
> > > i3cbus->cur_master->info.pid);
> > > @@ -663,6 +728,12 @@ static int i3c_master_send_ccc_cmd_locked(struct i3c_master_controller *master,
> > > !rwsem_is_locked(&master->bus.lock)))
> > > return -EINVAL;
> > >
> > > + if (!i3c_master_owns_bus(master)) {
> > > + ret = i3c_master_request_mastership(master);
> > > + if (ret)
> > > + return ret;
> > > + }
> >
> > As I said above, I think bus ownership should be requested in
> > maintenance mode, which means it has to be done before entering this
> > function. You can probably start acquiring the lock in write (AKA
> > maintenance) mode and downgrade it to read (AKA normal) mode before we
> > start sending the frame (CCC, SDR, I2C or HDR).
> >
>
> So, I'll have to call all the _locked functions with maintenance lock.
> Inside i3c_master_send_ccc_cmd_locked() I will downgrade the lock to read
> (normal use).
No, definitely not. The prefix _locked() means that the lock has been
acquired by the caller, and the function itself is not responsible for
downgrading the lock. What I meant is that the
i3c_master_owns_bus()+i3c_master_request_mastership() sequence does not
belong in xxxx_locked() funcs. It should be done by the caller with the
lock held in write/maintenance mode, and then downgraded to a read/normal
lock once the bus has been acquired. i3c_master_send_ccc_cmd_locked()
was probably not the best example, as this function might be
intentionally called with the lock held in write/maintenance mode
sometimes (depends on the CCC command), but for other funcs, it should
be pretty easy to do:
int i3c_master_acquire_bus_ownership_locked(struct i3c_master_controller *master)
{
if (i3c_master_owns_bus(master))
return 0;
return i3c_master_request_mastership(master);
}
int i3c_device_do_priv_xfers(struct i3c_device *dev,
struct i3c_priv_xfer *xfers,
int nxfers)
{
...
i3c_bus_maintenance_lock(dev->bus);
ret = i3c_master_acquire_bus_ownership(master);
if (ret) {
i3c_bus_maintenance_unlock(dev->bus);
return ret;
}
i3c_bus_downgrade_maintenance_lock(dev->bus);
ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers);
i3c_bus_normaluse_unlock(dev->bus);
return ret;
}
> Isn't it inconsistent that after _locked functions I will
> unlock the bus from normal use, even if I locked it for maintenance?>
> Also, I will have to downgrade the lock in case of failure in all _locked
> functions to unlock the bus properly using i3c_bus_normaluse_lock(). DO you
> think this looks good?
No, see above for an explanation.
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
next prev parent reply other threads:[~2019-01-30 7:56 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-11 17:43 [PATCH v2 0/3] Add the I3C mastership request Przemyslaw Gaj
2019-01-11 17:43 ` [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem Przemyslaw Gaj
2019-01-15 21:09 ` Boris Brezillon
2019-01-28 10:37 ` Przemyslaw Gaj
2019-01-28 12:08 ` Boris Brezillon
2019-01-29 18:13 ` Przemyslaw Gaj
2019-01-30 7:56 ` Boris Brezillon [this message]
2019-01-30 8:29 ` Przemyslaw Gaj
2019-01-30 8:38 ` Boris Brezillon
2019-01-11 17:43 ` [PATCH v2 2/3] i3c: master: cdns: add support for mastership request to Cadence I3C master driver Przemyslaw Gaj
2019-01-15 21:36 ` Boris Brezillon
2019-01-11 17:43 ` [PATCH v2 3/3] i3c: master: Add module author Przemyslaw Gaj
2019-01-15 21:11 ` Boris Brezillon
2019-01-15 21:40 ` Boris Brezillon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190130085626.7576b311@bbrezillon \
--to=bbrezillon@kernel.org \
--cc=linux-i3c@lists.infradead.org \
--cc=pgaj@cadence.com \
--cc=psroka@cadence.com \
--cc=rafalc@cadence.com \
--cc=vitor.soares@synopsys.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).