linux-kernel - Re: [PATCH v5 4/4] drivers: clk: Add ZynqMP clock driver

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: <153896567067.119890.7785076104115447392@swboyd.mtv.corp.google.com>
Date:   Sun, 07 Oct 2018 19:27:50 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Jolly Shah <jolly.shah@...inx.com>, arm@...nel.org,
        linux-clk@...r.kernel.org, michal.simek@...inx.com,
        mturquette@...libre.com, olof@...om.net, sboyd@...eaurora.org
Cc:     rajanv@...inx.com, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, Jolly Shah <jolly.shah@...inx.com>,
        Rajan Vaja <rajan.vaja@...inx.com>,
        Tejas Patel <tejasp@...inx.com>,
        Shubhrajyoti Datta <shubhrajyoti.datta@...inx.com>,
        Jolly Shah <jollys@...inx.com>
Subject: Re: [PATCH v5 4/4] drivers: clk: Add ZynqMP clock driver

Quoting Jolly Shah (2018-09-28 15:18:00)
> diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
> new file mode 100644
> index 0000000..1b07d77
> --- /dev/null
> +++ b/drivers/clk/zynqmp/clkc.c
> @@ -0,0 +1,716 @@
[...]
> + * @type:      Clock type: CLK_TYPE_OUTPUT or CLK_TYPE_EXTERNAL
> + *
> + * Return: 0 on success else error code
> + */
> +static int zynqmp_get_clock_type(u32 clk_id, u32 *type)
> +{
> +       int ret;
> +
> +       ret = zynqmp_is_valid_clock(clk_id);
> +       if (ret == 1) {
> +               *type = clock[clk_id].type;
> +               return 0;
> +       }
> +
> +       return ret == 0 ? -EINVAL : ret;
> +}
> +
> +/**
> + * zynqmp_pm_clock_get_num_clocks() - Get number of clocks in system
> + * @nclocks:   Number of clocks in system/board.
> + *
> + * Call firmware API to get number of clocks.
> + *
> + * Return: 0 on success else error code.
> + */
> +static int zynqmp_pm_clock_get_num_clocks(u32 *nclocks)
> +{
> +       struct zynqmp_pm_query_data qdata = {0};
> +       __le32 ret_payload[PAYLOAD_ARG_CNT];

I asked about endianess but this isn't the right fix. Just marking
something as __le32 but then not using that type and just passing it to
some function that takes a u32 pointer doesn't work. So is the firmware
side always little endian? If so, maybe the ioctls can do the byte swap
and then the kernel API can be native CPU endian while the firmware
layer can be aware that things may need swapping. I'm suggesting that
this type is just u32 and then the eemi_ops functions accept u32 and do
the swapping themselves.

If you put back u32 here and elsewhere in this patch you can have my

Reviewed-by: Stephen Boyd <sboyd@...nel.org>

The fix to the underlying ops for endian correctness can come later, so
don't feel like it needs to be fixed right now.