The regmap world is seemingly split into two groups which attempt to
solve different problems. Effectively, this means that not all regmap
providers are compatible with all regmap consumers.
First, we have the group where the current mdio-regmap users fit:
altera_tse_main.c and dwmac-socfpga.c use devm_regmap_init_mmio() to
ioremap their pcs_base and obtain a regmap where address zero is the
first PCS register.
Second, we have the group where MFD parent drivers call
mfd_add_devices(), having previously initialized a non-MMIO (SPI, I2C)
regmap and added it to their devres list, and MFD child drivers use
dev_get_regmap(dev->parent, NULL) in their probe function, to find the
first (and single) regmap of the MFD parent. The address zero of this
regmap is global to the entire parent, so the children need to be
parent-aware and add their own offsets for the registers that they
should manage. This is essentially because MFD is seemingly coming from
a world where peripheral registers are all entangled with each other.
What I'm trying to support are potentially multiple instances of the
same kind of device, at well separated address space regions.
To provide isolated regmaps for each child device would essentially mean
solving the problem of how would each child device needs to find the
correct regmap. This further means that "dev_get_regmap(dev->parent,
NULL)" transforms either in:
- dev_get_regmap(dev, NULL): search in the child device's devres list,
not in the parent's. This means adding the regmap in between
platform_device_alloc() and platform_device_add(), but is
structurally impossible because &dev->devres_head is initialized way
too late, in device_initialize().
- dev_get_regmap(dev->parent, "unique-regmap-name"): now the child
device needs to know, in case there are multiple instances of it,
which one is it, to ask for the right one. I've seen
drivers/mfd/ocelot-core.c work around this rather elegantly, providing
a resource to the child, and then the child uses resource->name to
find the regmap of the same name in the parent. But then I also
stumbled upon drivers/net/pcs/pcs-xpcs-plat.c which I need to support
as a child platform device, and that superimposes its own naming
scheme for the resources: "direct" or "indirect" - scheme which is
obviously incompatible with namespacing per instance.
So a parent device needs to decide whether it is in the boat that
provides one isolated regmap for each child, or one big regmap for all.
The "one big regmap" is the lowest common denominator when considering
children like pcs-xpcs-plat.c.
This means that from mdio-regmap's perspective, it needs to deal with
regmaps coming from both kinds of providers, as neither of them is going
away.
Users who provide a big regmap but want to access only a window into it
should provide as a struct mdio_regmap_config field a resource that
describes the start and end of that window. Currently we only use the
start as an offset into the regmap, and hope that MDIO reads and writes
won't go past the end.
Cc: Mark Brown <broonie@kernel.org>
Cc: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
v1->v2: add Maxime's review tag
drivers/net/mdio/mdio-regmap.c | 7 +++++--
include/linux/mdio/mdio-regmap.h | 2 ++
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/mdio/mdio-regmap.c b/drivers/net/mdio/mdio-regmap.c
index 8a742a8d6387..2a0e9c519fa3 100644
--- a/drivers/net/mdio/mdio-regmap.c
+++ b/drivers/net/mdio/mdio-regmap.c
@@ -19,6 +19,7 @@
struct mdio_regmap_priv {
struct regmap *regmap;
+ unsigned int base;
u8 valid_addr;
};
@@ -31,7 +32,7 @@ static int mdio_regmap_read_c22(struct mii_bus *bus, int addr, int regnum)
if (ctx->valid_addr != addr)
return -ENODEV;
- ret = regmap_read(ctx->regmap, regnum, &val);
+ ret = regmap_read(ctx->regmap, ctx->base + regnum, &val);
if (ret < 0)
return ret;
@@ -46,7 +47,7 @@ static int mdio_regmap_write_c22(struct mii_bus *bus, int addr, int regnum,
if (ctx->valid_addr != addr)
return -ENODEV;
- return regmap_write(ctx->regmap, regnum, val);
+ return regmap_write(ctx->regmap, ctx->base + regnum, val);
}
struct mii_bus *devm_mdio_regmap_register(struct device *dev,
@@ -66,6 +67,8 @@ struct mii_bus *devm_mdio_regmap_register(struct device *dev,
mr = mii->priv;
mr->regmap = config->regmap;
mr->valid_addr = config->valid_addr;
+ if (config->resource)
+ mr->base = config->resource->start;
mii->name = DRV_NAME;
strscpy(mii->id, config->name, MII_BUS_ID_SIZE);
diff --git a/include/linux/mdio/mdio-regmap.h b/include/linux/mdio/mdio-regmap.h
index 679d9069846b..441cead97936 100644
--- a/include/linux/mdio/mdio-regmap.h
+++ b/include/linux/mdio/mdio-regmap.h
@@ -11,10 +11,12 @@
struct device;
struct regmap;
+struct resource;
struct mdio_regmap_config {
struct device *parent;
struct regmap *regmap;
+ const struct resource *resource;
char name[MII_BUS_ID_SIZE];
u8 valid_addr;
bool autoscan;
--
2.34.1On Thu, Jan 22, 2026 at 12:56:40PM +0200, Vladimir Oltean wrote:
> The regmap world is seemingly split into two groups which attempt to
> solve different problems. Effectively, this means that not all regmap
> providers are compatible with all regmap consumers.
>
> First, we have the group where the current mdio-regmap users fit:
> altera_tse_main.c and dwmac-socfpga.c use devm_regmap_init_mmio() to
> ioremap their pcs_base and obtain a regmap where address zero is the
> first PCS register.
>
> Second, we have the group where MFD parent drivers call
> mfd_add_devices(), having previously initialized a non-MMIO (SPI, I2C)
> regmap and added it to their devres list, and MFD child drivers use
> dev_get_regmap(dev->parent, NULL) in their probe function, to find the
> first (and single) regmap of the MFD parent. The address zero of this
> regmap is global to the entire parent, so the children need to be
> parent-aware and add their own offsets for the registers that they
> should manage. This is essentially because MFD is seemingly coming from
> a world where peripheral registers are all entangled with each other.
>
> What I'm trying to support are potentially multiple instances of the
> same kind of device, at well separated address space regions.
>
> To provide isolated regmaps for each child device would essentially mean
> solving the problem of how would each child device needs to find the
> correct regmap. This further means that "dev_get_regmap(dev->parent,
> NULL)" transforms either in:
> - dev_get_regmap(dev, NULL): search in the child device's devres list,
> not in the parent's. This means adding the regmap in between
> platform_device_alloc() and platform_device_add(), but is
> structurally impossible because &dev->devres_head is initialized way
> too late, in device_initialize().
> - dev_get_regmap(dev->parent, "unique-regmap-name"): now the child
> device needs to know, in case there are multiple instances of it,
> which one is it, to ask for the right one. I've seen
> drivers/mfd/ocelot-core.c work around this rather elegantly, providing
> a resource to the child, and then the child uses resource->name to
> find the regmap of the same name in the parent. But then I also
> stumbled upon drivers/net/pcs/pcs-xpcs-plat.c which I need to support
> as a child platform device, and that superimposes its own naming
> scheme for the resources: "direct" or "indirect" - scheme which is
> obviously incompatible with namespacing per instance.
>
> So a parent device needs to decide whether it is in the boat that
> provides one isolated regmap for each child, or one big regmap for all.
> The "one big regmap" is the lowest common denominator when considering
> children like pcs-xpcs-plat.c.
>
> This means that from mdio-regmap's perspective, it needs to deal with
> regmaps coming from both kinds of providers, as neither of them is going
> away.
>
> Users who provide a big regmap but want to access only a window into it
> should provide as a struct mdio_regmap_config field a resource that
> describes the start and end of that window. Currently we only use the
> start as an offset into the regmap, and hope that MDIO reads and writes
> won't go past the end.
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Maxime Chevallier <maxime.chevallier@bootlin.com>
FWIW, Cc list may be located after --- line. It will have the same effect on
emails (as regular tooling will parse and put them into email headers), but
will reduce unneeded noise in the commit message. List will be still available
on lore.kernel.org in the mail archives.
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
Cc: ...
...
...
> struct mdio_regmap_priv {
> struct regmap *regmap;
> + unsigned int base;
Hmm... resource_size_t ?
> u8 valid_addr;
> };
--
With Best Regards,
Andy Shevchenko
On Thu, Jan 22, 2026 at 02:06:23PM +0200, Andy Shevchenko wrote:
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: Maxime Chevallier <maxime.chevallier@bootlin.com>
>
> FWIW, Cc list may be located after --- line. It will have the same effect on
> emails (as regular tooling will parse and put them into email headers), but
> will reduce unneeded noise in the commit message. List will be still available
> on lore.kernel.org in the mail archives.
Thanks for the comment. I know it may be located after ---, but for me,
doing that implies an extra step which I find unnecessary (moving them
there after the git format-patch stage). I keep the Cc: in the commit
message in git so that it's preserved across revisions.
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > ---
>
> Cc: ...
> ...
>
> ...
>
> > struct mdio_regmap_priv {
> > struct regmap *regmap;
> > + 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.
> > u8 valid_addr;
> > };
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Thu, Jan 22, 2026 at 02:13:01PM +0200, Vladimir Oltean wrote: > On Thu, Jan 22, 2026 at 02:06:23PM +0200, Andy Shevchenko wrote: > > > Cc: Mark Brown <broonie@kernel.org> > > > Cc: Maxime Chevallier <maxime.chevallier@bootlin.com> > > > > FWIW, Cc list may be located after --- line. It will have the same effect on > > emails (as regular tooling will parse and put them into email headers), but > > will reduce unneeded noise in the commit message. List will be still available > > on lore.kernel.org in the mail archives. > > Thanks for the comment. I know it may be located after ---, but for me, > doing that implies an extra step which I find unnecessary (moving them > there after the git format-patch stage). I keep the Cc: in the commit > message in git so that it's preserved across revisions. You can keep them in your commit message. Many --- lines are also allowed! > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > > --- > > > > Cc: ... > > ... ... > > > + 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. Maybe a comment near to the assignment? -- With Best Regards, Andy Shevchenko
On Thu, Jan 22, 2026 at 02:13:01PM +0200, Vladimir Oltean wrote:
> > > struct mdio_regmap_priv {
> > > struct regmap *regmap;
> > > + 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?
diff --git a/drivers/net/mdio/mdio-regmap.c b/drivers/net/mdio/mdio-regmap.c
index 2a0e9c519fa3..416ff4e13e8f 100644
--- a/drivers/net/mdio/mdio-regmap.c
+++ b/drivers/net/mdio/mdio-regmap.c
@@ -67,8 +67,15 @@ struct mii_bus *devm_mdio_regmap_register(struct device *dev,
mr = mii->priv;
mr->regmap = config->regmap;
mr->valid_addr = config->valid_addr;
- if (config->resource)
+ if (config->resource) {
+ if (config->resource->start > U32_MAX ||
+ config->resource->end > U32_MAX) {
+ dev_err(config->parent,
+ "Resource exceeds regmap API addressing possibilities\n");
+ return ERR_PTR(-EINVAL);
+ }
mr->base = config->resource->start;
+ }
mii->name = DRV_NAME;
strscpy(mii->id, config->name, MII_BUS_ID_SIZE);
(needs to be replicated in a bunch of other places)
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:
...
> > > > + 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.
> diff --git a/drivers/net/mdio/mdio-regmap.c b/drivers/net/mdio/mdio-regmap.c
> index 2a0e9c519fa3..416ff4e13e8f 100644
> --- a/drivers/net/mdio/mdio-regmap.c
> +++ b/drivers/net/mdio/mdio-regmap.c
> @@ -67,8 +67,15 @@ struct mii_bus *devm_mdio_regmap_register(struct device *dev,
> mr = mii->priv;
> mr->regmap = config->regmap;
> mr->valid_addr = config->valid_addr;
> - 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.
> + if (config->resource->start > U32_MAX ||
> + config->resource->end > U32_MAX) {
Ideally it should be resource_overlaps() check. But see above.
> + 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
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:
>
> ...
>
> > > > > + 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.
>
> > diff --git a/drivers/net/mdio/mdio-regmap.c b/drivers/net/mdio/mdio-regmap.c
> > index 2a0e9c519fa3..416ff4e13e8f 100644
> > --- a/drivers/net/mdio/mdio-regmap.c
> > +++ b/drivers/net/mdio/mdio-regmap.c
> > @@ -67,8 +67,15 @@ struct mii_bus *devm_mdio_regmap_register(struct device *dev,
> > mr = mii->priv;
> > mr->regmap = config->regmap;
> > mr->valid_addr = config->valid_addr;
> > - 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.
> > + 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.
> > + 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
>
>
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:
...
> > > > > > + 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?
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/
> > > + 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
> > > + 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
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:
>
> ...
>
> > > > > > > + 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;
> > > > + }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
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
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...
On Fri, Jan 23, 2026 at 03:55:01PM +0200, Vladimir Oltean wrote:
> On Fri, Jan 23, 2026 at 02:15:29PM +0200, Vladimir Oltean wrote:
...
> 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;
> };
Not sure. See below.
> 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");
%pR
> 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...
Dunno about semantics, as I only saw the use of that in regard to the special
slices of regmap.
Also we have struct range in range.h. Maybe that one suits better? It has also
some interesting APIs.
--
With Best Regards,
Andy Shevchenko
On Fri, Jan 23, 2026 at 04:31:25PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 23, 2026 at 03:55:01PM +0200, Vladimir Oltean wrote:
> > On Fri, Jan 23, 2026 at 02:15:29PM +0200, Vladimir Oltean wrote:
>
> ...
>
> > 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;
> > };
>
> Not sure. See below.
>
> > 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");
>
> %pR
>
> > 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...
>
> Dunno about semantics, as I only saw the use of that in regard to the special
> slices of regmap.
>
> Also we have struct range in range.h. Maybe that one suits better? It has also
> some interesting APIs.
It is defined as
struct range {
u64 start;
u64 end;
};
So it could be used, but it still doesn't properly express the fact that
regmap takes unsigned int register offsets (which struct regmap_range does).
Furthermore, by using struct range you are coupling unrelated data types,
whereas by using struct regmap_range you are not (if the regmap_read()
prototype changes, the regmap_range field data types immediately follow
suit).
On Fri, Jan 23, 2026 at 05:10:49PM +0200, Vladimir Oltean wrote:
> On Fri, Jan 23, 2026 at 04:31:25PM +0200, Andy Shevchenko wrote:
> > On Fri, Jan 23, 2026 at 03:55:01PM +0200, Vladimir Oltean wrote:
> > > On Fri, Jan 23, 2026 at 02:15:29PM +0200, Vladimir Oltean wrote:
...
> > > 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;
> > > };
> >
> > Not sure. See below.
> >
> > > 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");
> >
> > %pR
> >
> > > 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...
> >
> > Dunno about semantics, as I only saw the use of that in regard to the special
> > slices of regmap.
> >
> > Also we have struct range in range.h. Maybe that one suits better? It has also
> > some interesting APIs.
>
> It is defined as
>
> struct range {
> u64 start;
> u64 end;
> };
>
> So it could be used, but it still doesn't properly express the fact that
> regmap takes unsigned int register offsets (which struct regmap_range does).
> Furthermore, by using struct range you are coupling unrelated data types,
> whereas by using struct regmap_range you are not (if the regmap_read()
> prototype changes, the regmap_range field data types immediately follow
> suit).
I'm fine with regmap_range, but I'm not a regmap maintainer.
--
With Best Regards,
Andy Shevchenko
On Fri, Jan 23, 2026 at 05:39:13PM +0200, Andy Shevchenko wrote: > On Fri, Jan 23, 2026 at 05:10:49PM +0200, Vladimir Oltean wrote: ... > I'm fine with regmap_range, but I'm not a regmap maintainer. Also TIL the range_*() APIs in overflow.h. -- With Best Regards, Andy Shevchenko
On Thu, Jan 22, 2026 at 02:13:01PM +0200, Vladimir Oltean wrote: > On Thu, Jan 22, 2026 at 02:06:23PM +0200, Andy Shevchenko wrote: > > > Cc: Mark Brown <broonie@kernel.org> > > > Cc: Maxime Chevallier <maxime.chevallier@bootlin.com> > > > > FWIW, Cc list may be located after --- line. It will have the same effect on > > emails (as regular tooling will parse and put them into email headers), but > > will reduce unneeded noise in the commit message. List will be still available > > on lore.kernel.org in the mail archives. > > Thanks for the comment. I know it may be located after ---, but for me, > doing that implies an extra step which I find unnecessary (moving them > there after the git format-patch stage). I keep the Cc: in the commit > message in git so that it's preserved across revisions. What I do with my individual patch versioning is in the editor, after my sign-off, add the "---" and put the version changes below. This means git format-patch will do what it normally does, and as the commit message is reproduced verbatum, you get all that included too. You do get an extra "---" line between the versioning changes and the diffstat, but that's fine. So in the commit message: Signed-off-by: ... --- Cc: ... Changes v2: ... will work fine. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Jan 22, 2026 at 12:16:35PM +0000, Russell King (Oracle) wrote: > On Thu, Jan 22, 2026 at 02:13:01PM +0200, Vladimir Oltean wrote: > > On Thu, Jan 22, 2026 at 02:06:23PM +0200, Andy Shevchenko wrote: > > > > Cc: Mark Brown <broonie@kernel.org> > > > > Cc: Maxime Chevallier <maxime.chevallier@bootlin.com> > > > > > > FWIW, Cc list may be located after --- line. It will have the same effect on > > > emails (as regular tooling will parse and put them into email headers), but > > > will reduce unneeded noise in the commit message. List will be still available > > > on lore.kernel.org in the mail archives. > > > > Thanks for the comment. I know it may be located after ---, but for me, > > doing that implies an extra step which I find unnecessary (moving them > > there after the git format-patch stage). I keep the Cc: in the commit > > message in git so that it's preserved across revisions. > > What I do with my individual patch versioning is in the editor, after > my sign-off, add the "---" and put the version changes below. This means > git format-patch will do what it normally does, and as the commit > message is reproduced verbatum, you get all that included too. > > You do get an extra "---" line between the versioning changes and the > diffstat, but that's fine. > > So in the commit message: > > Signed-off-by: ... > --- > Cc: ... > > Changes v2: > ... > > will work fine. Oh, that's quite a clever use. I can now keep my changelog in git too... Thanks for the hint!
© 2016 - 2026 Red Hat, Inc.