Message-ID: <aXOEcT-iyRrAMxFf@smile.fi.intel.com>
Date: Fri, 23 Jan 2026 16:23:45 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-kernel@...r.kernel.org,
	Herve Codina <herve.codina@...tlin.com>,
	Mark Brown <broonie@...nel.org>,
	Serge Semin <fancer.lancer@...il.com>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org,
	Jiawen Wu <jiawenwu@...stnetic.com>
Subject: Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with
 non-MMIO regmaps

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:
> > On Fri, Jan 23, 2026 at 12:18:48AM +0200, Vladimir Oltean wrote:
> > > On Thu, Jan 22, 2026 at 04:38:37PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Jan 22, 2026 at 03:47:04PM +0200, Vladimir Oltean wrote:
> > > > > On Thu, Jan 22, 2026 at 02:13:01PM +0200, Vladimir Oltean wrote:

...

> > > > > -	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...
> 
> > 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/
> > 
> > Fixes due to that finding:
> > https://lore.kernel.org/lkml/20251208200437.14199-1-ansuelsmth@gmail.com/
> > https://lore.kernel.org/lkml/20251208145654.5294-1-ilpo.jarvinen@linux.intel.com/
> 
> 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.

> > > > > +		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.

> > > > > +			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

Powered by blists - more mailing lists