linux-kernel - Re: [PATCH 5/5] soc: qcom: llcc: Add regmap for Broadcast

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAA8EJppZ8GYqDTvdv-zvfE9gtzW2rOnp8Vft_zetorMP2kvF-Q@mail.gmail.com>
Date: Mon, 5 Feb 2024 06:36:29 +0100
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Satya Durga Srinivasu Prabhala <quic_satyap@...cinc.com>
Cc: Unnathi Chalicheemala <quic_uchalich@...cinc.com>, Bjorn Andersson <andersson@...nel.org>, 
	Konrad Dybcio <konrad.dybcio@...aro.org>, Rob Herring <robh+dt@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, kernel@...cinc.com
Subject: Re: [PATCH 5/5] soc: qcom: llcc: Add regmap for Broadcast_AND region

On Mon, 5 Feb 2024 at 02:01, Satya Durga Srinivasu Prabhala
<quic_satyap@...cinc.com> wrote:
>
>
> On 1/30/2024 10:57 AM, Dmitry Baryshkov wrote:
> > On Tue, 30 Jan 2024 at 19:52, Unnathi Chalicheemala
> > <quic_uchalich@...cinc.com> wrote:
> >> On 1/29/2024 2:03 PM, Dmitry Baryshkov wrote:
> >>> On Mon, 29 Jan 2024 at 20:17, Unnathi Chalicheemala
> >>> <quic_uchalich@...cinc.com> wrote:
> >>>>
> >>>>
> >>>> On 1/26/2024 12:29 PM, Dmitry Baryshkov wrote:
> >>>>> On Fri, 26 Jan 2024 at 21:48, Unnathi Chalicheemala
> >>>>> <quic_uchalich@...cinc.com> wrote:
> >>>>>> To support CSR programming, a broadcast interface is used to program
> >>>>>> all channels in a single command. Until SM8450 there was only one
> >>>>>> broadcast region (Broadcast_OR) used to broadcast write and check
> >>>>>> for status bit 0. From SM8450 onwards another broadcast region
> >>>>>> (Broadcast_AND) has been added which checks for status bit 1.
> >>>>>>
> >>>>>> Update llcc_drv_data structure with new regmap for Broadcast_AND
> >>>>>> region and initialize regmap for Broadcast_AND region when HW block
> >>>>>> version is greater than 4.1 for backwards compatibility.
> >>>>>>
> >>>>>> Switch from broadcast_OR to broadcast_AND region for checking
> >>>>>> status bit 1 as Broadcast_OR region checks only for bit 0.
> >>>>> This breaks backwards compatibility with the existing DT files,
> >>>>> doesn't it?
> >>>>>
> >>>> It shouldn't as checking for status bit 1 is happening only when the
> >>>> block
> >>>> version is greater than 4.1, which is when Broadcast_AND region support
> >>>> is added.
> >>> Let me reiterate, please: with the existing DT files. You are patching
> >>> DT files in patches 2-4, but this is not enough. DT files are
> >>> considered to be ABI. As such old DT files must continue to work with
> >>> newer kernels.
> >>>
> >> I'm sorry, I think I'm not understanding this right.
> >>
> >>>>>> While at it, also check return value after reading Broadcast_OR
> >>>>>> region in llcc_update_act_ctrl().
> >>>>> Separate patch, Fixes tag.
> >>>>>
> >>>> Ack. Will remove this from existing patch.
> >>>> Thanks for the review Dmitry!
> >>>>
> >>>>>> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@...cinc.com>
> >>>>>> ---
> >>>>>>   drivers/soc/qcom/llcc-qcom.c       | 12 +++++++++++-
> >>>>>>   include/linux/soc/qcom/llcc-qcom.h |  4 +++-
> >>>>>>   2 files changed, 14 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/soc/qcom/llcc-qcom.c
> >>>>>> b/drivers/soc/qcom/llcc-qcom.c
> >>>>>> index 4ca88eaebf06..5a2dac2d4772 100644
> >>>>>> --- a/drivers/soc/qcom/llcc-qcom.c
> >>>>>> +++ b/drivers/soc/qcom/llcc-qcom.c
> >>>>>> @@ -849,7 +849,7 @@ static int llcc_update_act_ctrl(u32 sid,
> >>>>>>                  return ret;
> >>>>>>
> >>>>>>          if (drv_data->version >= LLCC_VERSION_4_1_0_0) {
> >>>>>> -               ret =
> >>>>>> regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg,
> >>>>>> +               ret =
> >>>>>> regmap_read_poll_timeout(drv_data->bcast_and_regmap, status_reg,
> >>>>>>                                        slice_status, (slice_status &
> >>>>>> ACT_COMPLETE),
> >>>>>>                                        0, LLCC_STATUS_READ_DELAY);
> >> Above if condition will be true only for SM8450, 8550 and 8650 - whose DT
> >> files have been changed.
> >> It would never check for other existing DT files - I guess I'm failing to
> >> understand why the code
> >> would break with other DeviceTree files.
> > I'm saying that the driver must continue to work (well, at least not
> > to crash) even if somebody runs the kernel with older DT.
> Thanks Dmitry. While I get the ask, wondering why someone would use old
> DT while DT
> is also being updated in this series along with the driver change?

This is quite simple. The DT might be coming from the sources other
than just the kernel. It might be a part of the firmware. It might be
stored separately in the /boot partition, etc. The rule of thumb is
that you must never depend on the DT being updated together with the
kernel. This is one of the reasons why DT maintainers spend so much
time and effort to get DT bindings right in the first place. Once they
are merged, we have to support old DT nearly forever.


--
With best wishes
Dmitry