[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CALJ9ZPM0wudVk9nVXgTH0kPyzJ0UXbRzVggWwkG3yvyyNoCERQ@mail.gmail.com>
Date: Thu, 3 Apr 2025 11:28:50 -0700
From: Yabin Cui <yabinc@...gle.com>
To: Jie Gan <quic_jiegan@...cinc.com>
Cc: Leo Yan <leo.yan@....com>, Mike Leach <mike.leach@...aro.org>,
Suzuki K Poulose <suzuki.poulose@....com>, James Clark <james.clark@...aro.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>, coresight@...ts.linaro.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] coresight: core: Disable helpers for devices that
fail to enable
On Wed, Apr 2, 2025 at 5:54 PM Jie Gan <quic_jiegan@...cinc.com> wrote:
>
>
>
> On 4/2/2025 10:12 PM, Leo Yan wrote:
> > On Wed, Apr 02, 2025 at 02:50:22PM +0100, Mike Leach wrote:
> >
> > [...]
> >
> >>>>> @@ -465,7 +465,7 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> >>>>> /* Enable all helpers adjacent to the path first */
> >>>>> ret = coresight_enable_helpers(csdev, mode, path);
> >>>>> if (ret)
> >>>>> - goto err;
> >>>>> + goto err_helper;
> >>>>> /*
> >>>>> * ETF devices are tricky... They can be a link or a sink,
> >>>>> * depending on how they are configured. If an ETF has been
> >>>>> @@ -480,14 +480,8 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
> >>>>> switch (type) {
> >>>>> case CORESIGHT_DEV_TYPE_SINK:
> >>>>> ret = coresight_enable_sink(csdev, mode, sink_data);
> >>>>> - /*
> >>>>> - * Sink is the first component turned on. If we
> >>>>> - * failed to enable the sink, there are no components
> >>>>> - * that need disabling. Disabling the path here
> >>>>> - * would mean we could disrupt an existing session.
> >>>>> - */
> >>>>> if (ret)
> >>>>> - goto out;
> >>>>> + goto err;
> >>
> >> Going to err here is wrong. The comment above specifically states that
> >> we do _not_ want to disable the path, yet the new code flow disables
> >> helpers.
> >
> > Okay, now I understand here avoids to disable source and links for a
> > sink error.
> >
> >> then falls through to coresight_disable_path_from() - which
> >> the original code avoided and which also disables helpers a second
> >> time.
> >
> > Seems to me, the conclusion for "disables helpers a second time" is
> > incorrect.
> >
> > I checked the coresight_disable_path_from() function, when the current
> > 'nd' is passed to it, it will iterate from the _next_ node after 'nd'.
> >
> > /* Here 'nd' will be skipped and start from the next node */
> > list_for_each_entry_continue(nd, &path->path_list, link) {
> > ...
> >
> > coresight_disable_helpers(csdev, path);
> > }
> >
> > This means the _current_ coresight device (here is sink device) will
> > not disable its helpers. Could you confirm for this?
Yes, without this patch:
If the current coresight device fails to enable, the code calls
coresight_disable_path_from() to disable devices and helpers after this
device. But it doesn't disable helpers for the current device.
With this patch:
1) When the helpers of the current device fails to enable, call
coresight_disable_path_from() to disable following devices along the
path.
2) When the current device fails to enable, first disable the helpers for
the current devices, then call coresight_disable_path_from() to disable
following devices and helpers.
Regarding the comment about not calling coresight_disable_path_from()
if a sink device fails to enable, I feel that's a case of it not being
necessary,
rather than something that causes logic error. So this patch calls it to avoid
duplicating coresight_disable_helpers(). If my understanding is wrong, I am
happy to change to duplicate coresight_disable_helpers() (in three places).
>
> It seems there is an exception that the helper devices connected to the
> sink? The sink device may not the first the device to be enabled in the
> path if the sink device has a helper device.
>
> So I think we should add following logic before return?
>
> switch (type) {
> case CORESIGHT_DEV_TYPE_SINK:
> ret = coresight_enable_sink(csdev, mode,
> sink_data);
> /*
> * Sink is the first component turned on. If we
> * failed to enable the sink, there are no
> components
> * that need disabling. Disabling the path here
> * would mean we could disrupt an existing session.
> */
> if (ret) {
> /* disable the helpers which connected to sink */
> coresight_disable_helpers(csdev, path);
> goto out;
> }
> break;
This is the change I initially considered. But there are two other cases
(case CORESIGHT_DEV_TYPE_LINK and case default) that may need
to call coresight_disable_helpers() before returning an error.
>
>
> Thanks,
> Jie
>
> >
> > Thanks,
> > Leo
> >
>
Powered by blists - more mailing lists