| 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
| ||
|
Date: Fri, 28 Sep 2018 17:15:41 +0000
From: Jolly Shah <JOLLYS@...inx.com>
To: Olof Johansson <olof@...om.net>
CC: Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...eaurora.org>,
Michal Simek <michals@...inx.com>,
ARM-SoC Maintainers <arm@...nel.org>,
linux-clk <linux-clk@...r.kernel.org>,
Rajan Vaja <RAJANV@...inx.com>,
Linux ARM Mailing List <linux-arm-kernel@...ts.infradead.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Rajan Vaja <RAJANV@...inx.com>
Subject: RE: [PATCH v3 2/4] firmware: xilinx: Add zynqmp IOCTL API for device
control
Hi Olof,
Thanks for the review. Pushed v4 with suggested fixes. Let us know if series looks good and we can create pull request for same.
Thanks,
Jolly Shah
> -----Original Message-----
> From: Olof Johansson [mailto:olof@...om.net]
> Sent: Wednesday, September 26, 2018 1:49 PM
> To: Jolly Shah <JOLLYS@...inx.com>
> Cc: Michael Turquette <mturquette@...libre.com>; Stephen Boyd
> <sboyd@...eaurora.org>; Michal Simek <michals@...inx.com>; ARM-SoC
> Maintainers <arm@...nel.org>; linux-clk <linux-clk@...r.kernel.org>; Rajan
> Vaja <RAJANV@...inx.com>; Linux ARM Mailing List <linux-arm-
> kernel@...ts.infradead.org>; Linux Kernel Mailing List <linux-
> kernel@...r.kernel.org>; Rajan Vaja <RAJANV@...inx.com>; Jolly Shah
> <JOLLYS@...inx.com>
> Subject: Re: [PATCH v3 2/4] firmware: xilinx: Add zynqmp IOCTL API for device
> control
>
> Hi,
>
> Just nits on code readability below. Approach looks OK to me.
>
> On Wed, Sep 26, 2018 at 11:13 AM Jolly Shah <jolly.shah@...inx.com> wrote:
> >
> > From: Rajan Vaja <rajan.vaja@...inx.com>
> >
> > Add ZynqMP firmware IOCTL API to control and configure devices like
> > PLLs, SD, Gem, etc.
> >
> > Signed-off-by: Rajan Vaja <rajan.vaja@...inx.com>
> > Signed-off-by: Jolly Shah <jollys@...inx.com>
> > ---
> > drivers/firmware/xilinx/zynqmp.c | 43
> ++++++++++++++++++++++++++++++++++++
> > include/linux/firmware/xlnx-zynqmp.h | 4 +++-
> > 2 files changed, 46 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/xilinx/zynqmp.c
> > b/drivers/firmware/xilinx/zynqmp.c
> > index 84b3fd2..671a37a 100644
> > --- a/drivers/firmware/xilinx/zynqmp.c
> > +++ b/drivers/firmware/xilinx/zynqmp.c
> > @@ -428,6 +428,48 @@ static int zynqmp_pm_clock_getparent(u32 clock_id,
> u32 *parent_id)
> > return ret;
> > }
> >
> > +/**
> > + * zynqmp_is_valid_ioctl() - Check whether IOCTL ID is valid or not
> > + * @ioctl_id: IOCTL ID
> > + *
> > + * Return: 0 if IOCTL is valid, else -EINVAL */ static inline int
> > +zynqmp_is_valid_ioctl(u32 ioctl_id)
>
> I think most who come across the use of this would expect an
> .*is_valid() to return true (non-0) when valid, and 0 otherwise.
>
> > +{
> > + if (ioctl_id == IOCTL_SET_PLL_FRAC_MODE ||
> > + ioctl_id == IOCTL_GET_PLL_FRAC_MODE ||
> > + ioctl_id == IOCTL_SET_PLL_FRAC_DATA ||
> > + ioctl_id == IOCTL_GET_PLL_FRAC_DATA)
> > + return 0;
>
> This is purely a matter of taste, and no requirement to change, but I find a
> switch slightly easier to read for this kind of usage:
>
> switch(ioctl_id) {
> case IOCTL_SET_PLL_FRAC_MODE:
> case IOCTL_GET_PLL_FRAC_MODE:
> case IOCTL_SET_PLL_FRAC_DATA:
> case IOCTL_GET_PLL_FRAC_DATA:
> return 1;
> default:
> return 0;
> }
>
> > +
> > + return -EINVAL;
> > +}
> > +
> > +/**
> > + * zynqmp_pm_ioctl() - PM IOCTL API for device control and configs
> > + * @node_id: Node ID of the device
> > + * @ioctl_id: ID of the requested IOCTL
> > + * @arg1: Argument 1 to requested IOCTL call
> > + * @arg2: Argument 2 to requested IOCTL call
> > + * @out: Returned output value
> > + *
> > + * This function calls IOCTL to firmware for device control and configuration.
> > + *
> > + * Return: Returns status, either success or error+reason */ static
> > +int zynqmp_pm_ioctl(u32 node_id, u32 ioctl_id, u32 arg1, u32 arg2,
> > + u32 *out)
> > +{
> > + int ret;
> > +
> > + ret = zynqmp_is_valid_ioctl(ioctl_id);
> > + if (ret)
> > + return ret;
>
> So with changed return values, this would turn into:
>
> if (!zynqmp_is_valid_ioctl(ioctl_id))
> return -EINVAL;
>
> > +
> > + return zynqmp_pm_invoke_fn(PM_IOCTL, node_id, ioctl_id,
> > + arg1, arg2, out); }
> > +
> > static const struct zynqmp_eemi_ops eemi_ops = {
> > .get_api_version = zynqmp_pm_get_api_version,
> > .query_data = zynqmp_pm_query_data, @@ -440,6 +482,7 @@ static
> > const struct zynqmp_eemi_ops eemi_ops = {
> > .clock_getrate = zynqmp_pm_clock_getrate,
> > .clock_setparent = zynqmp_pm_clock_setparent,
> > .clock_getparent = zynqmp_pm_clock_getparent,
> > + .ioctl = zynqmp_pm_ioctl,
> > };
> >
> > /**
> > diff --git a/include/linux/firmware/xlnx-zynqmp.h
> > b/include/linux/firmware/xlnx-zynqmp.h
> > index 015e130..7a9db08 100644
> > --- a/include/linux/firmware/xlnx-zynqmp.h
> > +++ b/include/linux/firmware/xlnx-zynqmp.h
> > @@ -34,7 +34,8 @@
> >
> > enum pm_api_id {
> > PM_GET_API_VERSION = 1,
> > - PM_QUERY_DATA = 35,
> > + PM_IOCTL = 34,
> > + PM_QUERY_DATA,
> > PM_CLOCK_ENABLE,
> > PM_CLOCK_DISABLE,
> > PM_CLOCK_GETSTATE,
> > @@ -99,6 +100,7 @@ struct zynqmp_eemi_ops {
> > int (*clock_getrate)(u32 clock_id, u64 *rate);
> > int (*clock_setparent)(u32 clock_id, u32 parent_id);
> > int (*clock_getparent)(u32 clock_id, u32 *parent_id);
> > + int (*ioctl)(u32 node_id, u32 ioctl_id, u32 arg1, u32 arg2,
> > + u32 *out);
> > };
> >
> > #if IS_REACHABLE(CONFIG_ARCH_ZYNQMP)
> > --
> > 2.7.4
> >
Powered by blists - more mailing lists