From: Boris Brezillon <boris.brezillon@collabora.com>
To: Parshuram Raju Thombare <pthombar@cadence.com>
Cc: Milind Parab <mparab@cadence.com>,
"bbrezillon@kernel.org" <bbrezillon@kernel.org>,
"praneeth@ti.com" <praneeth@ti.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"vitor.soares@synopsys.com" <vitor.soares@synopsys.com>,
Przemyslaw Gaj <pgaj@cadence.com>,
"linux-i3c@lists.infradead.org" <linux-i3c@lists.infradead.org>
Subject: Re: [PATCH v3] i3c: master: fix for SETDASA and DAA process
Date: Thu, 20 Aug 2020 21:18:39 +0200 [thread overview]
Message-ID: <20200820211839.54a42cf6@collabora.com> (raw)
In-Reply-To: <20200820210311.5934fc2a@collabora.com>
On Thu, 20 Aug 2020 21:03:11 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:
> On Thu, 20 Aug 2020 18:16:14 +0000
> Parshuram Raju Thombare <pthombar@cadence.com> wrote:
>
> > >Hm, not sure that qualifies as a fix. The current implementation was
> > >correct, it was just reserving a slot in the device table for devices
> > >that didn't have an init address or on which SETDASA failed.
> > If I3C controllers like ours use hardware slots to store slave devices info,
> > due to limited available slots this can cause issue.
> > If some slots are lost due to
> > 1. only init_dyn_addr and no static_addr in DT
> > OR
> > 2. SETDASA failed
>
> Well, having a slot with a static address is valid, though I agree
> it's not really useful.
>
> > at the end of DAA some devices may be left without dyn_addr allocated from master
> > and hence can't work properly.
>
> My point is, there's no address or device slot leak, it's just that
> reserving a slot for I3C devices that only have a static address is
> kind of useless. But let's be honest, given the number of I3C devices
> available out there, I don't think it will hurt us before quite some
> time :P. That's not to say we shouldn't address that, I just don't
> think it deserves a Fixes tag.
>
> > I think during our discussion we recognized this change as a bug.
>
> IIRC, I was talking about the first patch in the series.
>
> > That is the reason I added fixes tag, but if you think otherwise I can remove this tag.
> >
> > >> -static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev)
> > >> +static int i3c_master_pre_assign_dyn_addr(struct i3c_master_controller
> > >That function now does more than just assigning a dynamic address: it
> > >also creates the i3c_dev_desc. It should be renamed accordingly
> > >(i3c_master_early_i3c_dev_add() maybe).
> > Ok
> >
> > >You should reserve the address before calling
> > >i3c_master_pre_assign_dyn_addr():
> > >
> > > /*
> > > * We don't attach devices which are not addressable
> > > * (no static_addr and dyn_addr) and devices with
> > > * static_addr but no init_dyn_addr will participate in DAA.
> > > */
> > > if (!i3cboardinfo->init_dyn_addr ||
> > > !i3cboardinfo->static_addr)
> > > continue;
> > Don't we want to cover the case when only init_dyn_addr is present ?
>
> Uh, yes, my bad.
>
> > I am not sure if we can't have init_dyn_addr without static_addr.
>
> You can, when you want to assign a specific dynamic address to a device
> that doesn't have a static address (see the 'try to assign init_addr
> dance' in i3c_dev_add()).
>
> > May be what we need is
> > if (!i3cboardinfo->init_dyn_addr)
> > continue;
> >
> > ret = i3c_bus_get_addr_slot_status(&master->bus,
> > i3cboardinfo->init_dyn_addr);
> > if (ret != I3C_ADDR_SLOT_FREE) {
> > ret = -EBUSY;
> > goto err_rstdaa;
> > }
> >
> > i3c_bus_set_addr_slot_status(&master->bus,
> > i3cboardinfo->init_dyn_addr,
> > I3C_ADDR_SLOT_I3C_DEV);
> >
> > if (i3cboardinfo->static_addr)
> > i3c_master_pre_assign_dyn_addr(master, i3cboardinfo);
>
> Yep, that's correct.
>
> > IMHO this is functionally same to what I sent. Just that init_dyn_addr is reserved before,
> > and we leverage the change in reattach to bypass failure due to second attempt
> > to get init_dyn_addr in reattach called from i3c_master_pre_assign_dyn_addr().
>
> Unless I'm missing something, your solution didn't reserve the
> init address when there's no static address, and we definitely want
> that to happen, otherwise another device might steal it during DAA.
My bad, it did. This being said, I find it much easier to follow when
the reservation happens in one place, so I'm still in favor of the new
version.
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
prev parent reply other threads:[~2020-08-20 19:18 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-20 13:38 [PATCH v3] i3c: master: fix for SETDASA and DAA process Parshuram Thombare
2020-08-20 14:48 ` Boris Brezillon
2020-08-20 18:16 ` Parshuram Raju Thombare
2020-08-20 19:03 ` Boris Brezillon
2020-08-20 19:18 ` Boris Brezillon [this message]
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=20200820211839.54a42cf6@collabora.com \
--to=boris.brezillon@collabora.com \
--cc=bbrezillon@kernel.org \
--cc=linux-i3c@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mparab@cadence.com \
--cc=pgaj@cadence.com \
--cc=praneeth@ti.com \
--cc=pthombar@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).