Re: [PATCHv2 net-next] net: fec_mpc52xx: remove dead resource size check

[Rosen Penev]

On Sun, Sep 20, 2026 at 3:47 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] drivers/net/ethernet/freescale/fec_mpc52xx.c: the patch
>   deletes the only runtime validation of the firmware-supplied register…
>
> --
>
> net: fec_mpc52xx: remove dead resource size check
>
> This removes the minimum size test on the FEC register resource in
> mpc52xx_fec_probe(), on the grounds that every in-tree device tree
> describes the FEC node as a 0x400 byte region and the stale error message
> referenced a file deleted in 2008.
>
> > diff --git a/drivers/net/ethernet/freescale/fec_mpc52xx.c b/drivers/net/ethernet/freescale/fec_mpc52xx.c
> > index 5188551add0b7..14ad03d75598c 100644
> > --- a/drivers/net/ethernet/freescale/fec_mpc52xx.c
> > +++ b/drivers/net/ethernet/freescale/fec_mpc52xx.c
> > @@ -834,13 +834,6 @@ static int mpc52xx_fec_probe(struct platform_device *op)
> >               pr_err("Error while parsing device node resource\n");
> >               goto err_netdev;
> >       }
> > -     if (resource_size(&mem) < sizeof(struct mpc52xx_fec)) {
> > -             pr_err("invalid resource size (%lx < %x), check mpc52xx_devices.c\n",
> > -                    (unsigned long)resource_size(&mem),
> > -                    sizeof(struct mpc52xx_fec));
> > -             rv = -EINVAL;
> > -             goto err_netdev;
> > -     }
> >
> >       if (!request_mem_region(mem.start, sizeof(struct mpc52xx_fec),
> >                               DRIVER_NAME)) {
>
> [Severity: Medium]
> Is the "can never be true" reasoning in the changelog accurate?
>
> The changelog says:
>
>   "but every in-tree device tree describes the FEC node as a 0x400 byte
>    region, which is exactly the size of the struct, so the condition can
>    never be true."
>
> The in-tree DTs (charon.dts, lite5200.dts, mpc5200b.dtsi, tqm5200.dts) do
> all use reg = <0x3000 0x400>, but the device tree is firmware-supplied, so
> the blob is not restricted to the in-tree copies. Bootloader-generated DTBs,
> out-of-tree board files and overlays can supply a shorter reg.
>
> Does anything else reject a short reg? of_address_to_resource() only checks
> representability and add-overflow:
>
> drivers/of/address.c:__of_address_resource_bounds() {
>         if (overflows_type(start, r->start))
>                 return -EOVERFLOW;
>
>         r->start = start;
>
>         if (!size)
>                 r->end = wrapping_sub(typeof(r->end), r->start, 1);
>         else if (size && check_add_overflow(r->start, size - 1, &r->end))
>                 return -EOVERFLOW;
> }
>
> So reg = <0x3000 0x100> translates fine and reaches the code below.
>
> Also, Documentation/devicetree/bindings/powerpc/fsl/mpc5200.txt does not
> state a length requirement for the fec node's reg, so with the check gone
> the 0x400 assumption is neither validated in code nor documented in the
> binding.
>
> [Severity: Medium]
> After this hunk, does mpc52xx_fec_probe() consult resource_size(&mem) at
> all? Both the reservation and the mapping use the hard-coded struct size:
>
>         if (!request_mem_region(mem.start, sizeof(struct mpc52xx_fec),
>                                 DRIVER_NAME)) {
>         ...
>         priv->fec = ioremap(mem.start, sizeof(struct mpc52xx_fec));
>
> __request_region() validates against the global resource tree, not against
> mem.end, so a short reg is silently over-claimed rather than rejected. The
> declared length then has no effect on the driver at all.
>
> For a truncated reg, probe now proceeds and touches registers past the
> described window during hardware setup:
>
> drivers/net/ethernet/freescale/fec_mpc52xx.c:mpc52xx_fec_hw_init() {
>         ...
>         out_be32(&fec->rfifo_alarm, 0x0000030c);
>         out_be32(&fec->tfifo_alarm, 0x00000100);
>         ...
> }
>
> drivers/net/ethernet/freescale/fec_mpc52xx.c:mpc52xx_fec_reset_stats() {
>         ...
>         memset_io(&fec->rmon_t_drop, 0,
>                    offsetof(struct mpc52xx_fec, reserved10) -
>                    offsetof(struct mpc52xx_fec, rmon_t_drop));
>         ...
> }
>
> Previously the same input returned -EINVAL with a diagnostic. If the
> over-claimed range overlaps a neighbouring reservation, the failure now
> surfaces as -EBUSY from request_mem_region() instead.
>
> Would it make sense to either honour resource_size(&mem) here (for example
> via devm_platform_ioremap_resource() or of_iomap()), or keep the minimum
> size check and just fix the message? The stale mpc52xx_devices.c reference
> and the %x versus size_t format mismatch look like the only genuinely dead
> parts of the removed block.
It is close to impossible to get anything similar to this merged.
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916223228.271815-1-rosenp%40gmail.com