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