Re: [bug report] mtd: parsers: add support for Sercomm partitions

[Mikhail Zhilkin <csharper2005@gmail.com>]

Hi Dan,

On 5/18/2022 8:33 PM, Dan Carpenter wrote:
> Hello Mikhail Zhilkin,
>
> The patch 4213e556fe2a: "mtd: parsers: add support for Sercomm
> partitions" from May 3, 2022, leads to the following Smatch static
> checker warning:
>
> 	drivers/mtd/parsers/scpart.c:155 scpart_parse()
> 	warn: missing error code here? 'mtd_get_of_node()' failed. 'res' = '0'

Thank you for your feedback! Which key do you use for checking? I can't
get these warning.

~/_utils/smatch/smatch_scripts/kchecker --pedantic
drivers/mtd/parsers/scpart.c
  CHECK   scripts/mod/empty.c
  CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  CC [M]  drivers/mtd/parsers/scpart.o
  CHECK   drivers/mtd/parsers/scpart.c

> drivers/mtd/parsers/scpart.c
>     138 static int scpart_parse(struct mtd_info *master,
>     139                         const struct mtd_partition **pparts,
>     140                         struct mtd_part_parser_data *data)
>     141 {
>     142         struct sc_part_desc *scpart_map = NULL;
>     143         struct mtd_partition *parts = NULL;
>     144         struct device_node *mtd_node;
>     145         struct device_node *ofpart_node;
>     146         struct device_node *pp;
>     147         const char *partname;
>     148         int nr_scparts;
>     149         int nr_parts = 0;
>     150         int n;
>     151         int res = 0;
>     152 
>     153         mtd_node = mtd_get_of_node(master);
>     154         if (!mtd_node)
> --> 155                 goto out;
>                         ^^^^^^^^
>
>     156 
>     157         ofpart_node = of_get_child_by_name(mtd_node, "partitions");
>     158         if (!ofpart_node)
>     159                 goto out;
>                         ^^^^^^^^
> Are these supposed to be success paths?

Really, I think -ENOENT is more suitable here.

    ofpart_node = of_get_child_by_name(mtd_node, "partitions");
    if (!ofpart_node)
        pr_info("%s: 'partitions' subnode not found on %pOF.\n",
                master->name, mtd_node);
        res = -ENOENT;
        goto out;

    nr_scparts = scpart_find_partmap(master, &scpart_map);
    if (nr_scparts <= 0) {
        pr_info("No any partitions was found in 'SC PART MAP'.\n");
        res = -ENOENT;
        goto free;
    }


>     160 
>     161         nr_scparts = scpart_find_partmap(master, &scpart_map);
>     162         if (nr_scparts <= 0) {
>
> Is nr_scparts == 0 a success path?
>
>     163                 res = nr_scparts;
>     164                 goto free;
>     165         }
>     166 
>     167         parts = kcalloc(of_get_child_count(ofpart_node), sizeof(*parts),
>     168                 GFP_KERNEL);
>     169         if (!parts) {
>     170                 res = -ENOMEM;
>     171                 goto out;
>     172         }
>     173 
>     174         for_each_child_of_node(ofpart_node, pp) {
>     175                 u32 scpart_id;
>     176 
>     177                 if (of_property_read_u32(pp, "sercomm,scpart-id", &scpart_id))
>     178                         continue;
>     179 
>     180                 for (n = 0 ; n < nr_scparts ; n++)
>     181                         if ((scpart_map[n].part_id != ID_ALREADY_FOUND) &&
>     182                                         (scpart_id == scpart_map[n].part_id))
>     183                                 break;
>     184                 if (n >= nr_scparts)
>     185                         /* not match */
>     186                         continue;
>     187 
>     188                 /* add the partition found in OF into MTD partition array */
>     189                 parts[nr_parts].offset = scpart_map[n].part_offs;
>     190                 parts[nr_parts].size = scpart_map[n].part_bytes;
>     191                 parts[nr_parts].of_node = pp;
>     192 
>     193                 if (!of_property_read_string(pp, "label", &partname))
>     194                         parts[nr_parts].name = partname;
>     195                 if (of_property_read_bool(pp, "read-only"))
>     196                         parts[nr_parts].mask_flags |= MTD_WRITEABLE;
>     197                 if (of_property_read_bool(pp, "lock"))
>     198                         parts[nr_parts].mask_flags |= MTD_POWERUP_LOCK;
>     199 
>     200                 /* mark as 'done' */
>     201                 scpart_map[n].part_id = ID_ALREADY_FOUND;
>     202 
>     203                 nr_parts++;
>     204         }
>     205 
>     206         if (nr_parts > 0) {
>     207                 *pparts = parts;
>     208                 res = nr_parts;
>     209         } else
>     210                 pr_info("No partition in OF matches partition ID with 'SC PART MAP'.\n");
>     211 
>     212         of_node_put(pp);
>     213 
>     214 free:
>     215         kfree(scpart_map);
>     216         if (res <= 0)
>     217                 kfree(parts);
>     218 
>     219 out:
>     220         return res;
>     221 }
>
> regards,
> dan carpenter

-- 
Best regards,
Mikhail


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/