Re: [PATCH v1 3/4] ARM: rpi: validate devicetree compatible instead of changing model name

[Trent Piepho <trent.piepho@igorinstitute.com>]

On Mon, Jan 17, 2022 at 4:06 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> +struct rpi_priv;
> +struct rpi_machine_data {
> +       unsigned int hw_id;
> +#define RPI_OLD_SCHEMA                 BIT(0)
> +       unsigned int flags;

hw_id is only a byte.  Both of these fields could be u8, which would
make the struct smaller.

> +struct rpi_priv {
> +       struct device_d *dev;
> +       const struct rpi_machine_data *dcfg;

Doesn't seem like there is any need to have dcfg saved in this struct.
It looks like it's used just once, in the same function that finds the
value.  rpi_get_dcfg() could return the value directly, rather than
indirectly by writing it into a field of a struct passed as an
argument.

>
> +static int rpi_get_dcfg(struct rpi_priv *priv)
> +{
> +       const struct rpi_machine_data *dcfg;
> +       int ret;
> +
> +       dcfg = of_device_get_match_data(priv->dev);
> +       if (!dcfg) {
> +               ret = -EINVAL;
> +               goto exit_get_dcfg;
> +       }

Then later:
> +       ret = rpi_get_dcfg(priv);
> +       if (ret)
> +               goto free_priv;

It looks like any board that doesn't have match data will be rejected
and fail to init.  But then what about these boards:

>  static const struct of_device_id rpi_of_match[] = {
>         /* BCM2711 based Boards */
>         { .compatible = "raspberrypi,400" },
> @@ -465,24 +599,24 @@ static const struct of_device_id rpi_of_match[] = {
>         { .compatible = "raspberrypi,4-model-b" },

Looks like they'll get rejected since they have no match data.

> +
> +       for (; dcfg->hw_id != UINT_MAX; dcfg++) {
> +               if (priv->hw_id & 0x800000) {
> +                       if (dcfg->hw_id != ((priv->hw_id >> 4) & 0xff))
> +                               continue;
> +               } else {
> +                       if (!(dcfg->flags & RPI_OLD_SCHEMA))
> +                               continue;
> +                       if (dcfg->hw_id != (priv->hw_id & 0xff))
> +                               continue;
> +               }
> +
> +               priv->dcfg = dcfg;
> +               break;

Could just return 0 here instead of break.  Or better, "return dcfg",
as there's no reason not to just return the value like a normal
function.

> +       }
> +
> +       if (!priv->dcfg) {
> +               ret = -ENODEV;
> +               goto exit_get_dcfg;
> +       }

Then this can become ret = -ENODEV and the if and goto go away.

> +
> +       priv = xzalloc(sizeof(*priv));
> +       if (!priv)
> +               return -ENOMEM;

No need to check the return value of "x" alloc functions.  That's the
whole point of the x version.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox