On Fri, Jan 23, 2026 at 02:15:29PM +0200, Vladimir Oltean wrote:

On Fri, Jan 23, 2026 at 09:20:58AM +0200, Andy Shevchenko wrote:

quoted

On Fri, Jan 23, 2026 at 12:18:48AM +0200, Vladimir Oltean wrote:

quoted

On Thu, Jan 22, 2026 at 04:38:37PM +0200, Andy Shevchenko wrote:

quoted

On Thu, Jan 22, 2026 at 03:47:04PM +0200, Vladimir Oltean wrote:

quoted

On Thu, Jan 22, 2026 at 02:13:01PM +0200, Vladimir Oltean wrote:

...

quoted

quoted

quoted

quoted

-	if (config->resource)
+	if (config->resource) {

Btw, this might be not enough, one should check size and flags as well
before use. There was a discussion about this recently. Maybe we should
just move to a simple unsigned int in the config for now? Because handling
resources maybe considered as over engineering in this case.

The resource flags are never taken into consideration, but I can for
sure replace the resource in struct mdio_regmap_config with just an
unsigned int start and an end, but that doesn't get rid of the resource
usage. The dev_get_resource(dev->parent, NULL) call is how we learn of
where our register window is located in the "one big regmap" provided by
the parent (SJA1105). So we still need this check somewhere else if we
wanted to not fail silently in case of address bits truncation.

Hmm... Bu why we can't embed the full struct resource in such a case?

We can also embed the full struct resource, I never said we can't...

quoted

Because resource should have a flag check, otherwise it's a wrong check.

Discussion I mentioned is this:
https://lore.kernel.org/lkml/20251207215359.28895-1-ansuelsmth@gmail.com/ (local)

Fixes due to that finding:
https://lore.kernel.org/lkml/20251208200437.14199-1-ansuelsmth@gmail.com/ (local)
https://lore.kernel.org/lkml/20251208145654.5294-1-ilpo.jarvinen@linux.intel.com/ (local)

The linked issues seem unrelated; they are caused by the assumption that
resource_size() can be zero. But I'm not using the resource_size()
helper, and even if I were, I'm not testing it against zero.

I referred to the full discussion, and not just to the OP message.
During discussion it was explicitly said that:
1) doing

	struct resource foo = {};

is wrong, and

2) checking the resource parameters (start, end), a.k.a. size is wrong
without checking flags.

So it is related.

As opposed to the PCI BAR case, we don't keep around in an altered form
the resources exceeding 4G.
Just need to reject them once and be done with them.

I'm not sure I follow here. The PCI case is much more complicated (it has
resources even in 64-bit space that can be resplit, remerged, etc. It's
a dynamic living thing due to hotplug and bridges and USB4/Thunderbolt.
I am definitely not talking about all of this.

Also, what else to even check about the resource flags? We get the
resource using "platform_get_resource(pdev, IORESOURCE_REG, 0)", so we
know they're of that type. I don't think IORESOURCE_REG resources have
any other valid bits in flags except for IORESOURCE_TYPE_BITS.

They can (not sure that is possible with current code, but in general)
be disabled, or size can be 0. Maybe even more, I haven't checked that.

quoted

quoted

quoted

quoted

+		if (config->resource->start > U32_MAX ||
+		    config->resource->end > U32_MAX) {

Ideally it should be resource_overlaps() check. But see above.

resource_overlaps_with_what? The only problem is that the resource can
exceed the 32 bit representation that regmap works with.

Obviously with the 4G address space :-)

	struct resource r4g = DEFINE_RESOURCE...(..., 0, SZ_4G...);

	if (resource_overlaps(&r4g, config->resource))
		aiaiai! // using %pR to print the content

This is a buggy replacement of my intention.

Sorry for that, I haven't given enough time to think about it.

I need to sanity check that
my IORESOURCE_REG resource is entirely within the 0-4G region.

The correct way to express this using helpers:

	if (!resource_contains(&r4g, config->resource))
		nazad!

but... you see my point? In trying to make use of "standard" helpers, we
overcomplicate simple things and introduce bugs.

I see, but do you see my points? I may have made a mistake, no doubts,
the point of using helpers is to avoid other, more subtle bugs.

My initially proposed test can be written even simpler:

	if (config->resource->end > U32_MAX) {
		...

because end >= start, so also testing resource->start is redundant.

Not at all, both needs to be checked and flags.

quoted

quoted

quoted

quoted

+			dev_err(config->parent,
+				"Resource exceeds regmap API addressing possibilities\n");
+			return ERR_PTR(-EINVAL);
+		}
 		mr->base = config->resource->start;
+	}

-- 
With Best Regards,
Andy Shevchenko