linux-kernel - Re: [PATCH v2 3/3] coresight: core: Disable helpers for devices that fail to enable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <98c1acbc-7e14-4aa1-9c1e-831be9eb4c91@quicinc.com>
Date: Thu, 3 Apr 2025 08:53:54 +0800
From: Jie Gan <quic_jiegan@...cinc.com>
To: Leo Yan <leo.yan@....com>, Mike Leach <mike.leach@...aro.org>
CC: Yabin Cui <yabinc@...gle.com>, 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 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?

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;


Thanks,
Jie

> 
> Thanks,
> Leo
>