[0/2] Fix Regression with AXP20X for 6.13-rc1

Message

Chris Morgan Dec. 10, 2024, 10:48 p.m. UTC

From: Chris Morgan <[e-mail]>

After performing a git bisect, I identified a commit that broke the
battery and charger driver for my AXP717 PMIC. This was caused by
commit e37ec3218870 ("mfd: axp20x: Allow multiple regulators").

After digging into it, it appears when mfd_add_devices was called with
a platform ID of PLATFORM_DEVID_NONE, the devm_iio_channel_get() call
made by the various AXP20X power drivers would not be able to generate
a dev_name(dev) for some reason, and the iio_channel_get() call used in
the devm_ helper would fall back to making a iio_channel_get_sys()
call. After the platform ID was updated, now iio_channel_get() is no
longer falling back to iio_channel_get_sys(). At least this is my
limited understanding of what happened.

To fix this, I added a new devm_ helper of devm_iio_channel_get_sys()
that directly calls iio_channel_get_sys(), and I updated all the
affected drivers with the new routine. I then no longer experienced
any issues with the drivers on my devices.

Chris Morgan (2):
  iio: core: Add devm_ API for iio_channel_get_sys
  power: supply: axp20x: Use devm_iio_channel_get_sys() for iio chans

 drivers/iio/inkern.c                    | 18 ++++++++++++++++++
 drivers/power/supply/axp20x_ac_power.c  |  4 ++--
 drivers/power/supply/axp20x_battery.c   | 16 ++++++++--------
 drivers/power/supply/axp20x_usb_power.c |  6 +++---
 include/linux/iio/consumer.h            | 20 ++++++++++++++++++++
 5 files changed, 51 insertions(+), 13 deletions(-)

Comments

Jonathan Cameron Dec. 11, 2024, 9:58 p.m. UTC | #1

On Tue, 10 Dec 2024 16:48:57 -0600
Chris Morgan <[e-mail]> wrote:

> From: Chris Morgan <[e-mail]>
> 
> After performing a git bisect, I identified a commit that broke the
> battery and charger driver for my AXP717 PMIC. This was caused by
> commit e37ec3218870 ("mfd: axp20x: Allow multiple regulators").
> 
> After digging into it, it appears when mfd_add_devices was called with
> a platform ID of PLATFORM_DEVID_NONE, the devm_iio_channel_get() call
> made by the various AXP20X power drivers would not be able to generate
> a dev_name(dev) for some reason, and the iio_channel_get() call used in
> the devm_ helper would fall back to making a iio_channel_get_sys()
> call. After the platform ID was updated, now iio_channel_get() is no
> longer falling back to iio_channel_get_sys(). At least this is my
> limited understanding of what happened.

The dev_name(dev) not getting a name doesn't sound quite right to me.

Time to look at the ancient creaking ghost that is the iio_map handling. 

struct iio_channel *iio_channel_get(struct device *dev,
				    const char *channel_name)
{
	const char *name = dev ? dev_name(dev) : NULL;
	struct iio_channel *channel;

	if (dev) {
		channel = fwnode_iio_channel_get_by_name(dev_fwnode(dev),
							 channel_name);
		if (!IS_ERR(channel) || PTR_ERR(channel) != -ENODEV)
			return channel;
	}

	return iio_channel_get_sys(name, channel_name);
}
EXPORT_SYMBOL_GPL(iio_channel_get);

We didn't invent the relevant phandle stuff in DT via the patch you point at
so all that matters is what gets passed to that iio_channel_get_sys()

So key here is that dev should be set, so we are passing dev_name(dev) into
iio_channel_get_sys()
I'm guessing that changed... 

Ah.  The iio_maps in
https://elixir.bootlin.com/linux/v6.12.4/source/drivers/iio/adc/axp20x_adc.c#L158
are our problem. Those hardcode the consumer_dev name. The fix just changed
those names. Back when this infrastructure was written we were in the world of
board files, so everything was hard coded in them - or in an MFD like this
it was treated as a singleton device.

So as to how to fix it... Assuming the new device names are the same for all
the mfd parts that make up each pmic, then you should be able to figure out the
 extra the number and build the channel maps to allow you to find the numbered
devices.

That's a lot lighter change than moving over to DT based phandles for all this.
(which is the modern way to handle it).

As a cheeky check, just edit those maps to whatever IDs you have and see
if it works.  Probably not an upstreamable solution but will confirm we have
it correct.

Your patch works because we allow for some fuzzy matching (I can't remember
why) that doesn't use the consumer device name.
That works as long as there is only one instance.  I'm guessing all this
mess came about because someone has a board with two of these devices. On such
a board we need the precise matching including the device name.

Jonathan

> 
> To fix this, I added a new devm_ helper of devm_iio_channel_get_sys()
> that directly calls iio_channel_get_sys(), and I updated all the
> affected drivers with the new routine. I then no longer experienced
> any issues with the drivers on my devices.
> 
> Chris Morgan (2):
>   iio: core: Add devm_ API for iio_channel_get_sys
>   power: supply: axp20x: Use devm_iio_channel_get_sys() for iio chans
> 
>  drivers/iio/inkern.c                    | 18 ++++++++++++++++++
>  drivers/power/supply/axp20x_ac_power.c  |  4 ++--
>  drivers/power/supply/axp20x_battery.c   | 16 ++++++++--------
>  drivers/power/supply/axp20x_usb_power.c |  6 +++---
>  include/linux/iio/consumer.h            | 20 ++++++++++++++++++++
>  5 files changed, 51 insertions(+), 13 deletions(-)
>

Jonathan Cameron Dec. 11, 2024, 10:06 p.m. UTC | #2

On Wed, 11 Dec 2024 21:58:26 +0000
Jonathan Cameron <[e-mail]> wrote:

> On Tue, 10 Dec 2024 16:48:57 -0600
> Chris Morgan <[e-mail]> wrote:
> 
> > From: Chris Morgan <[e-mail]>
> > 
> > After performing a git bisect, I identified a commit that broke the
> > battery and charger driver for my AXP717 PMIC. This was caused by
> > commit e37ec3218870 ("mfd: axp20x: Allow multiple regulators").
> > 
> > After digging into it, it appears when mfd_add_devices was called with
> > a platform ID of PLATFORM_DEVID_NONE, the devm_iio_channel_get() call
> > made by the various AXP20X power drivers would not be able to generate
> > a dev_name(dev) for some reason, and the iio_channel_get() call used in
> > the devm_ helper would fall back to making a iio_channel_get_sys()
> > call. After the platform ID was updated, now iio_channel_get() is no
> > longer falling back to iio_channel_get_sys(). At least this is my
> > limited understanding of what happened.  
> 
> The dev_name(dev) not getting a name doesn't sound quite right to me.
> 
> Time to look at the ancient creaking ghost that is the iio_map handling. 
> 
> struct iio_channel *iio_channel_get(struct device *dev,
> 				    const char *channel_name)
> {
> 	const char *name = dev ? dev_name(dev) : NULL;
> 	struct iio_channel *channel;
> 
> 	if (dev) {
> 		channel = fwnode_iio_channel_get_by_name(dev_fwnode(dev),
> 							 channel_name);
> 		if (!IS_ERR(channel) || PTR_ERR(channel) != -ENODEV)
> 			return channel;
> 	}
> 
> 	return iio_channel_get_sys(name, channel_name);
> }
> EXPORT_SYMBOL_GPL(iio_channel_get);
> 
> We didn't invent the relevant phandle stuff in DT via the patch you point at
> so all that matters is what gets passed to that iio_channel_get_sys()
> 
> So key here is that dev should be set, so we are passing dev_name(dev) into
> iio_channel_get_sys()
> I'm guessing that changed... 
> 
> Ah.  The iio_maps in
> https://elixir.bootlin.com/linux/v6.12.4/source/drivers/iio/adc/axp20x_adc.c#L158
> are our problem. Those hardcode the consumer_dev name. The fix just changed
> those names. Back when this infrastructure was written we were in the world of
> board files, so everything was hard coded in them - or in an MFD like this
> it was treated as a singleton device.

(note this driver is a little newer than that era, but used that old infrastructure
none the less)

> 
> So as to how to fix it... Assuming the new device names are the same for all
> the mfd parts that make up each pmic, then you should be able to figure out the
>  extra the number and build the channel maps to allow you to find the numbered
> devices.
> 
> That's a lot lighter change than moving over to DT based phandles for all this.
> (which is the modern way to handle it).
> 
> As a cheeky check, just edit those maps to whatever IDs you have and see
> if it works.  Probably not an upstreamable solution but will confirm we have
> it correct.
> 
> Your patch works because we allow for some fuzzy matching (I can't remember
> why) that doesn't use the consumer device name.
> That works as long as there is only one instance.  I'm guessing all this
> mess came about because someone has a board with two of these devices. On such
> a board we need the precise matching including the device name.
> 

+CC Sasha.  Whilst looking for the problem patch, I noticed that fix
has gone into stable.  If this is a common problem, we may want to drop
that briefly until we have a fix in place for this side effect as
the original patch fixed a configuration that had (I think?) never worked,
but broke ones that did.  I've no idea how common the devices are in
each configuration!

Jonathan


> Jonathan
> 
> > 
> > To fix this, I added a new devm_ helper of devm_iio_channel_get_sys()
> > that directly calls iio_channel_get_sys(), and I updated all the
> > affected drivers with the new routine. I then no longer experienced
> > any issues with the drivers on my devices.
> > 
> > Chris Morgan (2):
> >   iio: core: Add devm_ API for iio_channel_get_sys
> >   power: supply: axp20x: Use devm_iio_channel_get_sys() for iio chans
> > 
> >  drivers/iio/inkern.c                    | 18 ++++++++++++++++++
> >  drivers/power/supply/axp20x_ac_power.c  |  4 ++--
> >  drivers/power/supply/axp20x_battery.c   | 16 ++++++++--------
> >  drivers/power/supply/axp20x_usb_power.c |  6 +++---
> >  include/linux/iio/consumer.h            | 20 ++++++++++++++++++++
> >  5 files changed, 51 insertions(+), 13 deletions(-)
> >   
>