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

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

[Rosen Penev]

The check bails out if the FEC register resource is smaller than
sizeof(struct mpc52xx_fec), 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 error message also points at mpc52xx_devices.c, which was removed
in 2008 by commit 917f0af9e5a9 ("powerpc: Remove arch/ppc and
include/asm-ppc").

Assisted-by: LLM
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: instead of fixing format, remove the dead check.
 drivers/net/ethernet/freescale/fec_mpc52xx.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_mpc52xx.c b/drivers/net/ethernet/freescale/fec_mpc52xx.c
index 90379c26f300..dcf378a02835 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)) {
-- 
2.55.0

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

[Andrew Lunn]

On Wed, Sep 16, 2026 at 03:32:28PM -0700, Rosen Penev wrote:
> The check bails out if the FEC register resource is smaller than
> sizeof(struct mpc52xx_fec), 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.

We generally don't fully trust DT blobs.

Does this test do any actual harm in its current state?
Why remove it?

	Andrew

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

[Rosen Penev]

On Thu, Sep 17, 2026 at 7:56 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Sep 16, 2026 at 03:32:28PM -0700, Rosen Penev wrote:
> > The check bails out if the FEC register resource is smaller than
> > sizeof(struct mpc52xx_fec), 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.
>
> We generally don't fully trust DT blobs.
>
> Does this test do any actual harm in its current state?
> Why remove it?
Previous discussion:

https://lore.kernel.org/netdev/20260518044047.1002235-1-rosenp@gmail.com/
>
>         Andrew

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

[netdev-bot+sashiko]

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.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916223228.271815-1-rosenp%40gmail.com

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@kernel.org> 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