linux-kernel - Re: [PATCH v6 4/5] firmware: qcom_scm: Refactor code to support multiple download mode

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] [day] [month] [year] [list]

Message-ID: <CAHp75VdU+ee8_UveZp3SD2UKYYg4Jm0cYUpcXfVwMEVC1d0Bdw@mail.gmail.com>
Date:   Mon, 26 Jun 2023 20:12:52 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Mukesh Ojha <quic_mojha@...cinc.com>
Cc:     agross@...nel.org, andersson@...nel.org, konrad.dybcio@...aro.org,
        linus.walleij@...aro.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v6 4/5] firmware: qcom_scm: Refactor code to support
 multiple download mode

On Mon, Jun 26, 2023 at 7:28 PM Mukesh Ojha <quic_mojha@...cinc.com> wrote:
> On 5/27/2023 3:38 AM, andy.shevchenko@...il.com wrote:
> > Wed, Mar 29, 2023 at 01:16:51PM +0530, Mukesh Ojha kirjoitti:

...

> >>              ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
> >> -                            QCOM_DOWNLOAD_MODE_MASK,
> >> -                            enable ? QCOM_DOWNLOAD_FULLDUMP : 0);
> >> +                            QCOM_DOWNLOAD_MODE_MASK, download_mode);
> >
> > Can ping-pong style be avoided? I.e. do the right thing in the previous patch,
> > so you won't change lines that were introduced just before.
>
> If you notice, I have just converted download mode data type from bool
> to int in this patch and hence the changing the line here. Last patch
> was about just using the exported API, so i hope you would be fine here.

Thank you for elaboration. I'm fine with that.

...

> >  > Also, what about download_mode that doesn't fit to the above two?
>
> return sysfs_emit(buffer, "unknown\n"); ?

For example, yes.

...

> >> +static int set_download_mode(const char *val, const struct kernel_param *kp)
> >> +{
> >> +    u32 old = download_mode;
> >> +
> >> +    if (sysfs_streq(val, "full")) {
> >> +            download_mode = QCOM_DOWNLOAD_FULLDUMP;
> >> +    } else if (sysfs_streq(val, "off")) {
> >> +            download_mode = QCOM_DOWNLOAD_NODUMP;
> >
> > NIH sysfs_match_string().
>
> NIH ?

Not Invented Here

> My apology, if i did not get this..
> Do you want me to use sysfs_match_string()

Yes.

> and how would that help compare to what is present now ?

This will make your ABi gathered in one place (all strings and all
values) and less code will be used esp. if it's going to be expanded
in the future (isn't it in the next patches?).


> >> +    }
> >> +
> >> +    if (__scm)
> >> +            qcom_scm_set_download_mode(download_mode);
> >> +
> >> +    return 0;
> >> +}

...

> > Have you updated corresponding documentation about this parameter?
> > Or there is none?
>
> There is none as of yet outside this file; should that be good what i
> have added in 5/5..

It was a long time ago when I reviewed this. Just a note that all ABI
must be documented, debugfs is highly recommended to be documented.

-- 
With Best Regards,
Andy Shevchenko