Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230323000657.28664-1-blarson@amd.com> <20230323000657.28664-16-blarson@amd.com>
In-Reply-To: <20230323000657.28664-16-blarson@amd.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Thu, 23 Mar 2023 13:06:04 +0200
Message-ID: <CAHp75VcbDBUf2cH_6rRqn5RCGSEOWqE85Yn3gDhYiJPhGf1S=Q@mail.gmail.com>
Subject: Re: [PATCH v12 15/15] soc: amd: Add support for AMD Pensando SoC Controller
To:     Brad Larson <blarson@amd.com>
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-mmc@vger.kernel.org, linux-spi@vger.kernel.org,
        adrian.hunter@intel.com, alcooperx@gmail.com, arnd@arndb.de,
        brendan.higgins@linux.dev, briannorris@chromium.org,
        brijeshkumar.singh@amd.com, catalin.marinas@arm.com,
        davidgow@google.com, gsomlo@gmail.com, gerg@linux-m68k.org,
        krzk@kernel.org, krzysztof.kozlowski+dt@linaro.org, lee@kernel.org,
        lee.jones@linaro.org, broonie@kernel.org,
        yamada.masahiro@socionext.com, p.zabel@pengutronix.de,
        piotrs@cadence.com, p.yadav@ti.com, rdunlap@infradead.org,
        robh+dt@kernel.org, samuel@sholland.org, fancer.lancer@gmail.com,
        skhan@linuxfoundation.org, suravee.suthikulpanit@amd.com,
        thomas.lendacky@amd.com, tonyhuang.sunplus@gmail.com,
        ulf.hansson@linaro.org, vaishnav.a@ti.com, will@kernel.org,
        devicetree@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Mar 23, 2023 at 2:11=E2=80=AFAM Brad Larson <blarson@amd.com> wrote=
:
>
> The Pensando SoC controller is a SPI connected companion device
> that is present in all Pensando SoC board designs.  The essential
> board management registers are accessed on chip select 0 with
> board mgmt IO support accessed using additional chip selects.

...

> +config AMD_PENSANDO_CTRL
> +       tristate "AMD Pensando SoC Controller"
> +       depends on SPI_MASTER=3Dy
> +       depends on (ARCH_PENSANDO && OF) || COMPILE_TEST
> +       default y if ARCH_PENSANDO

       default ARCH_PENSANDO

?

> +       select REGMAP_SPI
> +       select MFD_SYSCON

...

> +/*
> + * AMD Pensando SoC Controller
> + *
> + * Userspace interface and reset driver support for SPI connected Pensan=
do SoC
> + * controller device.  This device is present in all Pensando SoC design=
s and
> + * contains board control/status regsiters and management IO support.

registers ?

> + *
> + * Copyright 2023 Advanced Micro Devices, Inc.
> + */

...

> +#include <linux/cdev.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>

Seems semi-random. Are you sure you use this and not missing mod_devicetabl=
e.h?

> +#include <linux/reset-controller.h>
> +#include <linux/spi/spi.h>

...

> +struct penctrl_device {
> +       struct spi_device *spi_dev;
> +       struct reset_controller_dev rcdev;

Perhaps swapping these two might provide a better code generation.

> +};

...

> +       struct spi_transfer t[2] =3D { 0 };

0 is not needed.

...

> +       if (_IOC_DIR(cmd) & _IOC_READ)
> +               ret =3D !access_ok((void __user *)arg, _IOC_SIZE(cmd));
> +       else if (_IOC_DIR(cmd) & _IOC_WRITE)
> +               ret =3D !access_ok((void __user *)arg, _IOC_SIZE(cmd));


Maybe you should create a temporary variable as

    void __user *in =3D ... arg;

?

> +       if (ret)
> +               return -EFAULT;

...

> +       /* Verify and prepare spi message */

SPI

> +       size =3D _IOC_SIZE(cmd);
> +       if ((size % sizeof(struct penctrl_spi_xfer)) !=3D 0) {

' !=3D 0' is redundant.

> +               ret =3D -EINVAL;
> +               goto done;
> +       }
> +       num_msgs =3D size / sizeof(struct penctrl_spi_xfer);

> +       if (num_msgs =3D=3D 0) {
> +               ret =3D -EINVAL;
> +               goto done;
> +       }

Can be unified with a previous check as

if (size =3D=3D 0 || size % ...)

> +       msg =3D memdup_user((struct penctrl_spi_xfer __user *)arg, size);
> +       if (!msg) {
> +               ret =3D PTR_ERR(msg);
> +               goto done;
> +       }

...

> +       if (copy_from_user((void *)(uintptr_t)tx_buf,
> +                          (void __user *)msg->tx_buf, msg->len)) {

Why are all these castings here?

> +               ret =3D -EFAULT;
> +               goto done;
> +       }

...

> +       if (copy_to_user((void __user *)msg->rx_buf,
> +                        (void *)(uintptr_t)rx_buf, msg->len))
> +               ret =3D -EFAULT;

Ditto.

...

> +       struct spi_transfer t[2] =3D { 0 };

0 is redundant.

...

> +       struct spi_transfer t[1] =3D { 0 };

Ditto.

Why is this an array?

...

> +       ret =3D spi_sync(spi_dev, &m);
> +       return ret;

return spi_sync(...);

...

> +       np =3D spi_dev->dev.parent->of_node;
> +       ret =3D of_property_read_u32(np, "num-cs", &num_cs);

Why not simply device_property_read_u32()?

> +       if (ret)
> +               return dev_err_probe(&spi_dev->dev, ret,
> +                                    "number of chip-selects not defined"=
);

...

> +       cdev =3D cdev_alloc();
> +       if (!cdev) {
> +               dev_err(&spi_dev->dev, "allocation of cdev failed");
> +               ret =3D -ENOMEM;

ret =3D dev_err_probe(...);

> +               goto cdev_failed;
> +       }

...

> +       ret =3D cdev_add(cdev, penctrl_devt, num_cs);
> +       if (ret) {

> +               dev_err(&spi_dev->dev, "register of cdev failed");

dev_err_probe() ?

> +               goto cdev_delete;
> +       }

...

> +       penctrl =3D kzalloc(sizeof(*penctrl), GFP_KERNEL);
> +       if (!penctrl) {

> +               ret =3D -ENOMEM;
> +               dev_err(&spi_dev->dev, "allocate driver data failed");

ret =3D dev_err_probe();
But we do not print memory allocation failure messages.

> +               goto cdev_delete;
> +       }

...

> +               if (IS_ERR(dev)) {
> +                       ret =3D IS_ERR(dev);
> +                       dev_err(&spi_dev->dev, "error creating device\n")=
;

ret =3D dev_err_probe();

> +                       goto cdev_delete;
> +               }
> +               dev_dbg(&spi_dev->dev, "created device major %u, minor %d=
\n",
> +                       MAJOR(penctrl_devt), cs);
> +       }

...

> +       spi_set_drvdata(spi_dev, penctrl);

Is it in use?

...

> +       penctrl->rcdev.of_node =3D spi_dev->dev.of_node;

device_set_node();

...

> +       ret =3D reset_controller_register(&penctrl->rcdev);
> +       if (ret)
> +               return dev_err_probe(&spi_dev->dev, ret,
> +                                    "failed to register reset controller=
\n");
> +       return ret;

return 0;

...

> +       device_destroy(penctrl_class, penctrl_devt);

Are you sure this is the correct API?

> +       return ret;

...

> +#include <linux/types.h>
> +#include <linux/ioctl.h>

Sorted?

--=20
With Best Regards,
Andy Shevchenko