Message-ID: <20260123135501.7m5wqkcfluxqeowb@skbuf>
Date: Fri, 23 Jan 2026 15:55:01 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.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:
> > > > > > > > +	unsigned int base;
> > > > > > > 
> > > > > > > Hmm... resource_size_t ?
> > > > 
> > > > > > Well, regmap_read() takes "unsigned int reg".
> > > > > > https://elixir.bootlin.com/linux/v6.18.6/source/include/linux/regmap.h#L1297
> > > > > > So in practice, a truncation will be done somewhere if the register base
> > > > > > exceeds unsigned int storage capacity. But I didn't feel that it's worth
> > > > > > handling that.
> > > > > 
> > > > > Would this address your feedback?
> > > > 
> > > > Yes and no. See my remarks below.
> > 
> > ...
> > 
> > > > > -	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.
> 
> 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.
> 
> 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.
> 
> > > > > +		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. 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.
> 
> 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.
> 
> > > > > +			dev_err(config->parent,
> > > > > +				"Resource exceeds regmap API addressing possibilities\n");
> > > > > +			return ERR_PTR(-EINVAL);
> > > > > +		}
> > > > >  		mr->base = config->resource->start;
> > > > > +	}

A data structure which I find a bit under-utilized in the kernel is

/**
 * struct regmap_range - A register range, used for access related checks
 *                       (readable/writeable/volatile/precious checks)
 *
 * @range_min: address of first register
 * @range_max: address of last register
 */
struct regmap_range {
	unsigned int range_min;
	unsigned int range_max;
};

I could imagine a helper like:

/* Type adaptation between phy_addr_t and unsigned int */
static inline int __must_check regmap_range_from_resource(const struct resource *res,
							  struct regmap_range *range)
{
	struct resource r4g = DEFINE_RES(0, SZ_4G, res->flags);

	if (res->flags != IORESOURCE_REG) {
		pr_err("%s should be used only with IORESOURCE_REG resources\n");
		return -EINVAL;
	}

	if (!resource_contains(&r4g, res)) {
		pr_err("Resource exceeds regmap API addressing possibilities\n");
		return -EINVAL;
	}

	range->range_min = res->start;
	range->range_max = res->end;

	return 0;
}

and then proceed to use the simpler and validated regmap_range structure in the driver.
Too bad such use is not an established coding pattern...

Powered by blists - more mailing lists