linux-kernel - Re: [PATCH v2 04/15] soc: renesas: rz-sysc: Add SoC detection support

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMuHMdWhGvXhjTV+NYzw2WoDuH6BxQWFdUZC_qkon5vSYsjAhg@mail.gmail.com>
Date: Tue, 10 Dec 2024 15:59:17 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Claudiu <claudiu.beznea@...on.dev>
Cc: vkoul@...nel.org, kishon@...nel.org, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, p.zabel@...gutronix.de, magnus.damm@...il.com, 
	gregkh@...uxfoundation.org, yoshihiro.shimoda.uh@...esas.com, 
	christophe.jaillet@...adoo.fr, linux-phy@...ts.infradead.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org, linux-usb@...r.kernel.org, 
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH v2 04/15] soc: renesas: rz-sysc: Add SoC detection support

Hi Claudiu,

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@...on.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>
> The RZ SYSC controller has registers that keep the SoC ID data. Add
> driver support to retrieve the SoC ID and register a SoC driver.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> ---
>
> Changes in v2:
> - this was patch 05/16 in v1
> - changed patch title and description
> - added SoC initialization code in its own function
> - addressed the review comments
> - introduced struct rz_sysc_soc_id_init_data and adjusted the code
>   accordingly
> - dropped the RZ/G3S SoC detection code (it will be introduced in
>   a separate patch)

Thanks for your patch!

> --- a/drivers/soc/renesas/rz-sysc.c
> +++ b/drivers/soc/renesas/rz-sysc.c
> @@ -211,6 +214,59 @@ static int rz_sysc_signals_init(struct rz_sysc *sysc,
>         return 0;
>  }
>
> +static int rz_sysc_soc_init(struct rz_sysc *sysc, const struct of_device_id *match)
> +{
> +       const struct rz_sysc_init_data *sysc_data = match->data;
> +       const struct rz_sysc_soc_id_init_data *soc_data = sysc_data->soc_id_init_data;
> +       struct soc_device_attribute *soc_dev_attr;
> +       const char *soc_id_start, *soc_id_end;
> +       u32 val, revision, specific_id;
> +       struct soc_device *soc_dev;
> +       char soc_id[32] = {0};
> +       u8 size;

unsigned int (or size_t?)

> +
> +       if (!soc_data || !soc_data->family || !soc_data->offset ||
> +           !soc_data->revision_mask)
> +               return -EINVAL;

Cannot happen?

> +
> +       soc_id_start = strchr(match->compatible, ',') + 1;
> +       soc_id_end = strchr(match->compatible, '-');
> +       size = soc_id_end - soc_id_start;
> +       if (size > 32)
> +               size = 32;

sizeof(soc_id) instead of hardcoded 32

> +       strscpy(soc_id, soc_id_start, size);

Doesn't this wipe the last character, as strscpy() always
NUL-terminates the destination buffer?

> +
> +       soc_dev_attr = devm_kzalloc(sysc->dev, sizeof(*soc_dev_attr), GFP_KERNEL);
> +       if (!soc_dev_attr)
> +               return -ENOMEM;
> +
> +       soc_dev_attr->family = soc_data->family;

Shouldn't you duplicate family? It's in __initconst, hence freed later.

> +       soc_dev_attr->soc_id = devm_kstrdup(sysc->dev, soc_id, GFP_KERNEL);
> +       if (!soc_dev_attr->soc_id)
> +               return -ENOMEM;
> +
> +       val = readl(sysc->base + soc_data->offset);
> +       revision = field_get(soc_data->revision_mask, val);
> +       specific_id = field_get(soc_data->specific_id_mask, val);
> +       soc_dev_attr->revision = devm_kasprintf(sysc->dev, GFP_KERNEL, "%u", revision);
> +       if (!soc_dev_attr->revision)
> +               return -ENOMEM;
> +
> +       if (soc_data->id && specific_id != soc_data->id) {
> +               dev_warn(sysc->dev, "SoC mismatch (product = 0x%x)\n", specific_id);
> +               return -ENODEV;
> +       }
> +
> +       dev_info(sysc->dev, "Detected Renesas %s %s Rev %s\n", soc_dev_attr->family,
> +                soc_dev_attr->soc_id, soc_dev_attr->revision);
> +
> +       soc_dev = soc_device_register(soc_dev_attr);
> +       if (IS_ERR(soc_dev))
> +               return PTR_ERR(soc_dev);
> +
> +       return 0;
> +}
> +
>  static struct regmap_config rz_sysc_regmap = {
>         .name = "rz_sysc_regs",
>         .reg_bits = 32,
> @@ -235,14 +291,15 @@ MODULE_DEVICE_TABLE(of, rz_sysc_match);
>  static int rz_sysc_probe(struct platform_device *pdev)
>  {
>         const struct rz_sysc_init_data *data;
> +       const struct of_device_id *match;
>         struct device *dev = &pdev->dev;
> -       struct rz_sysc *sysc;
>         struct regmap *regmap;
> +       struct rz_sysc *sysc;
>         int ret;
>
> -       data = device_get_match_data(dev);
> -       if (!data || !data->max_register_offset)
> -               return -EINVAL;
> +       match = of_match_node(rz_sysc_match, dev->of_node);
> +       if (!match || !match->data)

!match->data cannot happen

> +               return -ENODEV;
>
>         sysc = devm_kzalloc(dev, sizeof(*sysc), GFP_KERNEL);
>         if (!sysc)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds