Some systems don't provide any ACPI power supplies, but drivers use
the function power_supply_is_system_supplied() to make policy decisions.
This logic works fine until a new device is added to the system that
provides a UCSI power supply. This power supply doesn't power the system
but the logic assumes it does.
This series adjusts the logic so that these power supplies are not
considered when making these policy decisions.
Mario Limonciello (2):
power: supply: Use the scope of power supplies to tell if power is
system supplied
usb: typec: ucsi: Don't create power supplies for dGPUs
drivers/i2c/busses/i2c-designware-pcidrv.c | 13 ++++++++++++-
drivers/i2c/busses/i2c-nvidia-gpu.c | 3 +++
drivers/power/supply/power_supply_core.c | 8 ++++++--
drivers/usb/typec/ucsi/psy.c | 14 ++++++++++++++
4 files changed, 35 insertions(+), 3 deletions(-)
--
2.34.1
power_supply_is_system_supplied() checks whether any power
supplies are present that aren't batteries to decide whether
the system is running on DC or AC. Downstream drivers use
this to make performance decisions.
Navi dGPUs include an UCSI function that has been exported
since commit 17631e8ca2d3 ("i2c: designware: Add driver
support for AMD NAVI GPU").
This UCSI function registers a power supply since commit
992a60ed0d5e ("usb: typec: ucsi: register with power_supply class")
but this is not a system power supply.
As the power supply for a dGPU is only for powering devices connected
to dGPU, create a device property to indicate that the UCSI endpoint
is only for the scope of `POWER_SUPPLY_SCOPE_DEVICE`.
Cc: Evan Quan <[email protected]>
Signed-off-by: Mario Limonciello <[email protected]>
---
drivers/i2c/busses/i2c-designware-pcidrv.c | 13 ++++++++++++-
drivers/i2c/busses/i2c-nvidia-gpu.c | 3 +++
drivers/usb/typec/ucsi/psy.c | 14 ++++++++++++++
3 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 782fe1ef3ca1..048ffa18def0 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -20,6 +20,7 @@
#include <linux/module.h>
#include <linux/pci.h>
#include <linux/pm_runtime.h>
+#include <linux/power_supply.h>
#include <linux/sched.h>
#include <linux/slab.h>
@@ -234,6 +235,16 @@ static const struct dev_pm_ops i2c_dw_pm_ops = {
SET_RUNTIME_PM_OPS(i2c_dw_pci_runtime_suspend, i2c_dw_pci_runtime_resume, NULL)
};
+static const struct property_entry dgpu_properties[] = {
+ /* USB-C doesn't power the system */
+ PROPERTY_ENTRY_U8("scope", POWER_SUPPLY_SCOPE_DEVICE),
+ {},
+};
+
+static const struct software_node dgpu_node = {
+ .properties = dgpu_properties,
+};
+
static int i2c_dw_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
{
@@ -325,7 +336,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
}
if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU) {
- dev->slave = i2c_new_ccgx_ucsi(&dev->adapter, dev->irq, NULL);
+ dev->slave = i2c_new_ccgx_ucsi(&dev->adapter, dev->irq, &dgpu_node);
if (IS_ERR(dev->slave))
return dev_err_probe(dev->dev, PTR_ERR(dev->slave),
"register UCSI failed\n");
diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
index a8b99e7f6262..26622d24bb1b 100644
--- a/drivers/i2c/busses/i2c-nvidia-gpu.c
+++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
@@ -14,6 +14,7 @@
#include <linux/platform_device.h>
#include <linux/pm.h>
#include <linux/pm_runtime.h>
+#include <linux/power_supply.h>
#include <asm/unaligned.h>
@@ -261,6 +262,8 @@ MODULE_DEVICE_TABLE(pci, gpu_i2c_ids);
static const struct property_entry ccgx_props[] = {
/* Use FW built for NVIDIA GPU only */
PROPERTY_ENTRY_STRING("firmware-name", "nvidia,gpu"),
+ /* USB-C doesn't power the system */
+ PROPERTY_ENTRY_U8("scope", POWER_SUPPLY_SCOPE_DEVICE),
{ }
};
diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c
index 56bf56517f75..384b42267f1f 100644
--- a/drivers/usb/typec/ucsi/psy.c
+++ b/drivers/usb/typec/ucsi/psy.c
@@ -27,8 +27,20 @@ static enum power_supply_property ucsi_psy_props[] = {
POWER_SUPPLY_PROP_VOLTAGE_NOW,
POWER_SUPPLY_PROP_CURRENT_MAX,
POWER_SUPPLY_PROP_CURRENT_NOW,
+ POWER_SUPPLY_PROP_SCOPE,
};
+static int ucsi_psy_get_scope(struct ucsi_connector *con,
+ union power_supply_propval *val)
+{
+ u8 scope = POWER_SUPPLY_SCOPE_UNKNOWN;
+ struct device *dev = con->ucsi->dev;
+
+ device_property_read_u8(dev, "scope", &scope);
+ val->intval = scope;
+ return 0;
+}
+
static int ucsi_psy_get_online(struct ucsi_connector *con,
union power_supply_propval *val)
{
@@ -194,6 +206,8 @@ static int ucsi_psy_get_prop(struct power_supply *psy,
return ucsi_psy_get_current_max(con, val);
case POWER_SUPPLY_PROP_CURRENT_NOW:
return ucsi_psy_get_current_now(con, val);
+ case POWER_SUPPLY_PROP_SCOPE:
+ return ucsi_psy_get_scope(con, val);
default:
return -EINVAL;
}
--
2.34.1
The logic used for power_supply_is_system_supplied() counts all power
supplies and:
* If no power supplies found assumes AC
* If non-battery power supplies found uses online to determine AC/DC.
- If any are onlined, assumes AC
- Othewise DC.
This logic makes sense for desktop systems that don't export an ACPI
battery, but it fails once you include a dGPU that provides a UCSI
power supply on a desktop system without any other power supplies.
The dGPU by default doesn't have anything plugged in so it's 'offline'.
This makes power_supply_is_system_supplied() return 0 with a count of
1 meaning all drivers that use this get a wrong judgement.
To fix this case adjust the logic to also examine the scope of the
power supply. If the power supply is deemed a device power supply,
then don't count it.
Cc: Evan Quan <[email protected]>
Suggested-by: Lijo Lazar <[email protected]>
Signed-off-by: Mario Limonciello <[email protected]>
---
drivers/power/supply/power_supply_core.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index ab986dbace16..d57f420ba8c3 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -348,6 +348,10 @@ static int __power_supply_is_system_supplied(struct device *dev, void *data)
struct power_supply *psy = dev_get_drvdata(dev);
unsigned int *count = data;
+ if (!psy->desc->get_property(psy, POWER_SUPPLY_PROP_SCOPE, &ret))
+ if (ret.intval == POWER_SUPPLY_SCOPE_DEVICE)
+ return 0;
+
(*count)++;
if (psy->desc->type != POWER_SUPPLY_TYPE_BATTERY)
if (!psy->desc->get_property(psy, POWER_SUPPLY_PROP_ONLINE,
@@ -366,8 +370,8 @@ int power_supply_is_system_supplied(void)
__power_supply_is_system_supplied);
/*
- * If no power class device was found at all, most probably we are
- * running on a desktop system, so assume we are on mains power.
+ * If no system scope power class device was found at all, most probably we
+ * are running on a desktop system, so assume we are on mains power.
*/
if (count == 0)
return 1;
--
2.34.1
On 5/16/2023 3:41 PM, Sebastian Reichel wrote:
> Hi,
>
> On Tue, May 16, 2023 at 01:25:40PM -0500, Mario Limonciello wrote:
>> The logic used for power_supply_is_system_supplied() counts all power
>> supplies and:
>> * If no power supplies found assumes AC
>> * If non-battery power supplies found uses online to determine AC/DC.
>> - If any are onlined, assumes AC
>> - Othewise DC.
>>
>> This logic makes sense for desktop systems that don't export an ACPI
>> battery, but it fails once you include a dGPU that provides a UCSI
>> power supply on a desktop system without any other power supplies.
>>
>> The dGPU by default doesn't have anything plugged in so it's 'offline'.
>> This makes power_supply_is_system_supplied() return 0 with a count of
>> 1 meaning all drivers that use this get a wrong judgement.
>>
>> To fix this case adjust the logic to also examine the scope of the
>> power supply. If the power supply is deemed a device power supply,
>> then don't count it.
>>
>> Cc: Evan Quan <[email protected]>
>> Suggested-by: Lijo Lazar <[email protected]>
>> Signed-off-by: Mario Limonciello <[email protected]>
>> ---
> Good find; the current logic should also break with a desktop PC
> once a POWER_SUPPLY_SCOPE_DEVICE battery device is attached (e.g.
> a HID device), because it increases the counter.
Yup!
> I suppose I can just apply this to my fixes branch since there is
> no compile time dependency to the second patch?
Yes, that's correct. I don't see a problem with the other patch going
through
another tree.
> -- Sebastian
>
>> drivers/power/supply/power_supply_core.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
>> index ab986dbace16..d57f420ba8c3 100644
>> --- a/drivers/power/supply/power_supply_core.c
>> +++ b/drivers/power/supply/power_supply_core.c
>> @@ -348,6 +348,10 @@ static int __power_supply_is_system_supplied(struct device *dev, void *data)
>> struct power_supply *psy = dev_get_drvdata(dev);
>> unsigned int *count = data;
>>
>> + if (!psy->desc->get_property(psy, POWER_SUPPLY_PROP_SCOPE, &ret))
>> + if (ret.intval == POWER_SUPPLY_SCOPE_DEVICE)
>> + return 0;
>> +
>> (*count)++;
>> if (psy->desc->type != POWER_SUPPLY_TYPE_BATTERY)
>> if (!psy->desc->get_property(psy, POWER_SUPPLY_PROP_ONLINE,
>> @@ -366,8 +370,8 @@ int power_supply_is_system_supplied(void)
>> __power_supply_is_system_supplied);
>>
>> /*
>> - * If no power class device was found at all, most probably we are
>> - * running on a desktop system, so assume we are on mains power.
>> + * If no system scope power class device was found at all, most probably we
>> + * are running on a desktop system, so assume we are on mains power.
>> */
>> if (count == 0)
>> return 1;
>> --
>> 2.34.1
>>
Hi,
On Tue, May 16, 2023 at 01:25:40PM -0500, Mario Limonciello wrote:
> The logic used for power_supply_is_system_supplied() counts all power
> supplies and:
> * If no power supplies found assumes AC
> * If non-battery power supplies found uses online to determine AC/DC.
> - If any are onlined, assumes AC
> - Othewise DC.
>
> This logic makes sense for desktop systems that don't export an ACPI
> battery, but it fails once you include a dGPU that provides a UCSI
> power supply on a desktop system without any other power supplies.
>
> The dGPU by default doesn't have anything plugged in so it's 'offline'.
> This makes power_supply_is_system_supplied() return 0 with a count of
> 1 meaning all drivers that use this get a wrong judgement.
>
> To fix this case adjust the logic to also examine the scope of the
> power supply. If the power supply is deemed a device power supply,
> then don't count it.
>
> Cc: Evan Quan <[email protected]>
> Suggested-by: Lijo Lazar <[email protected]>
> Signed-off-by: Mario Limonciello <[email protected]>
> ---
Good find; the current logic should also break with a desktop PC
once a POWER_SUPPLY_SCOPE_DEVICE battery device is attached (e.g.
a HID device), because it increases the counter.
I suppose I can just apply this to my fixes branch since there is
no compile time dependency to the second patch?
-- Sebastian
> drivers/power/supply/power_supply_core.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index ab986dbace16..d57f420ba8c3 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -348,6 +348,10 @@ static int __power_supply_is_system_supplied(struct device *dev, void *data)
> struct power_supply *psy = dev_get_drvdata(dev);
> unsigned int *count = data;
>
> + if (!psy->desc->get_property(psy, POWER_SUPPLY_PROP_SCOPE, &ret))
> + if (ret.intval == POWER_SUPPLY_SCOPE_DEVICE)
> + return 0;
> +
> (*count)++;
> if (psy->desc->type != POWER_SUPPLY_TYPE_BATTERY)
> if (!psy->desc->get_property(psy, POWER_SUPPLY_PROP_ONLINE,
> @@ -366,8 +370,8 @@ int power_supply_is_system_supplied(void)
> __power_supply_is_system_supplied);
>
> /*
> - * If no power class device was found at all, most probably we are
> - * running on a desktop system, so assume we are on mains power.
> + * If no system scope power class device was found at all, most probably we
> + * are running on a desktop system, so assume we are on mains power.
> */
> if (count == 0)
> return 1;
> --
> 2.34.1
>
Hi,
On Tue, May 16, 2023 at 03:44:17PM -0500, Limonciello, Mario wrote:
> On 5/16/2023 3:41 PM, Sebastian Reichel wrote:
> > Hi,
> >
> > On Tue, May 16, 2023 at 01:25:40PM -0500, Mario Limonciello wrote:
> > > The logic used for power_supply_is_system_supplied() counts all power
> > > supplies and:
> > > * If no power supplies found assumes AC
> > > * If non-battery power supplies found uses online to determine AC/DC.
> > > - If any are onlined, assumes AC
> > > - Othewise DC.
> > >
> > > This logic makes sense for desktop systems that don't export an ACPI
> > > battery, but it fails once you include a dGPU that provides a UCSI
> > > power supply on a desktop system without any other power supplies.
> > >
> > > The dGPU by default doesn't have anything plugged in so it's 'offline'.
> > > This makes power_supply_is_system_supplied() return 0 with a count of
> > > 1 meaning all drivers that use this get a wrong judgement.
> > >
> > > To fix this case adjust the logic to also examine the scope of the
> > > power supply. If the power supply is deemed a device power supply,
> > > then don't count it.
> > >
> > > Cc: Evan Quan <[email protected]>
> > > Suggested-by: Lijo Lazar <[email protected]>
> > > Signed-off-by: Mario Limonciello <[email protected]>
> > > ---
> > Good find; the current logic should also break with a desktop PC
> > once a POWER_SUPPLY_SCOPE_DEVICE battery device is attached (e.g.
> > a HID device), because it increases the counter.
>
> Yup!
>
> > I suppose I can just apply this to my fixes branch since there is
> > no compile time dependency to the second patch?
>
> Yes, that's correct.? I don't see a problem with the other patch going
> through another tree.
Ok, I modified the commit message to also mention the HID issue and
applied it to power-supply's fixes branch.
Thanks,
-- Sebastian
>
> > -- Sebastian
> >
> > > drivers/power/supply/power_supply_core.c | 8 ++++++--
> > > 1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> > > index ab986dbace16..d57f420ba8c3 100644
> > > --- a/drivers/power/supply/power_supply_core.c
> > > +++ b/drivers/power/supply/power_supply_core.c
> > > @@ -348,6 +348,10 @@ static int __power_supply_is_system_supplied(struct device *dev, void *data)
> > > struct power_supply *psy = dev_get_drvdata(dev);
> > > unsigned int *count = data;
> > > + if (!psy->desc->get_property(psy, POWER_SUPPLY_PROP_SCOPE, &ret))
> > > + if (ret.intval == POWER_SUPPLY_SCOPE_DEVICE)
> > > + return 0;
> > > +
> > > (*count)++;
> > > if (psy->desc->type != POWER_SUPPLY_TYPE_BATTERY)
> > > if (!psy->desc->get_property(psy, POWER_SUPPLY_PROP_ONLINE,
> > > @@ -366,8 +370,8 @@ int power_supply_is_system_supplied(void)
> > > __power_supply_is_system_supplied);
> > > /*
> > > - * If no power class device was found at all, most probably we are
> > > - * running on a desktop system, so assume we are on mains power.
> > > + * If no system scope power class device was found at all, most probably we
> > > + * are running on a desktop system, so assume we are on mains power.
> > > */
> > > if (count == 0)
> > > return 1;
> > > --
> > > 2.34.1
> > >
[AMD Official Use Only - General]
Series is Reviewed-and-tested-by: Evan Quan <[email protected]>
> -----Original Message-----
> From: Limonciello, Mario <[email protected]>
> Sent: Wednesday, May 17, 2023 2:26 AM
> To: [email protected]; [email protected]; [email protected];
> [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Quan, Evan
> <[email protected]>; Lazar, Lijo <[email protected]>; Goswami, Sanket
> <[email protected]>; Limonciello, Mario
> <[email protected]>
> Subject: [PATCH 0/2] Adjust logic for power_supply_is_system_supplied()
>
> Some systems don't provide any ACPI power supplies, but drivers use the
> function power_supply_is_system_supplied() to make policy decisions.
>
> This logic works fine until a new device is added to the system that provides a
> UCSI power supply. This power supply doesn't power the system but the logic
> assumes it does.
>
> This series adjusts the logic so that these power supplies are not considered
> when making these policy decisions.
>
> Mario Limonciello (2):
> power: supply: Use the scope of power supplies to tell if power is
> system supplied
> usb: typec: ucsi: Don't create power supplies for dGPUs
>
> drivers/i2c/busses/i2c-designware-pcidrv.c | 13 ++++++++++++-
> drivers/i2c/busses/i2c-nvidia-gpu.c | 3 +++
> drivers/power/supply/power_supply_core.c | 8 ++++++--
> drivers/usb/typec/ucsi/psy.c | 14 ++++++++++++++
> 4 files changed, 35 insertions(+), 3 deletions(-)
>
> --
> 2.34.1
On Tue, May 16, 2023 at 01:25:41PM -0500, Mario Limonciello wrote:
> power_supply_is_system_supplied() checks whether any power
> supplies are present that aren't batteries to decide whether
> the system is running on DC or AC. Downstream drivers use
> this to make performance decisions.
>
> Navi dGPUs include an UCSI function that has been exported
> since commit 17631e8ca2d3 ("i2c: designware: Add driver
> support for AMD NAVI GPU").
>
> This UCSI function registers a power supply since commit
> 992a60ed0d5e ("usb: typec: ucsi: register with power_supply class")
> but this is not a system power supply.
>
> As the power supply for a dGPU is only for powering devices connected
> to dGPU, create a device property to indicate that the UCSI endpoint
> is only for the scope of `POWER_SUPPLY_SCOPE_DEVICE`.
...
> +static const struct property_entry dgpu_properties[] = {
> + /* USB-C doesn't power the system */
> + PROPERTY_ENTRY_U8("scope", POWER_SUPPLY_SCOPE_DEVICE),
> + {},
Comma is not needed in terminator entry.
> +};
--
With Best Regards,
Andy Shevchenko
On Wed, May 17, 2023 at 01:22:25AM +0000, Quan, Evan wrote:
> [AMD Official Use Only - General]
>
> Series is Reviewed-and-tested-by: Evan Quan <[email protected]>
This is not how we supply tags (and most of the maintainers prefer non-combined
ones, so two in this case instead of one).
The idea is that tag is supplied as a separate line, not mixed with any other
text.
All this is well written in Submitting Patches documentation, please refresh
your memories :-)
--
With Best Regards,
Andy Shevchenko
On Wed, May 17, 2023 at 08:14:13PM +0300, Andy Shevchenko wrote:
> On Tue, May 16, 2023 at 01:25:41PM -0500, Mario Limonciello wrote:
> > power_supply_is_system_supplied() checks whether any power
> > supplies are present that aren't batteries to decide whether
> > the system is running on DC or AC. Downstream drivers use
> > this to make performance decisions.
> >
> > Navi dGPUs include an UCSI function that has been exported
> > since commit 17631e8ca2d3 ("i2c: designware: Add driver
> > support for AMD NAVI GPU").
> >
> > This UCSI function registers a power supply since commit
> > 992a60ed0d5e ("usb: typec: ucsi: register with power_supply class")
> > but this is not a system power supply.
> >
> > As the power supply for a dGPU is only for powering devices connected
> > to dGPU, create a device property to indicate that the UCSI endpoint
> > is only for the scope of `POWER_SUPPLY_SCOPE_DEVICE`.
>
> ...
>
> > +static const struct property_entry dgpu_properties[] = {
> > + /* USB-C doesn't power the system */
> > + PROPERTY_ENTRY_U8("scope", POWER_SUPPLY_SCOPE_DEVICE),
> > + {},
>
> Comma is not needed in terminator entry.
But it's encouraged!
On Wed, May 17, 2023 at 08:58:49PM +0200, Greg KH wrote:
> On Wed, May 17, 2023 at 08:14:13PM +0300, Andy Shevchenko wrote:
> > On Tue, May 16, 2023 at 01:25:41PM -0500, Mario Limonciello wrote:
...
> > > +static const struct property_entry dgpu_properties[] = {
> > > + /* USB-C doesn't power the system */
> > > + PROPERTY_ENTRY_U8("scope", POWER_SUPPLY_SCOPE_DEVICE),
> > > + {},
> >
> > Comma is not needed in terminator entry.
>
> But it's encouraged!
Why?! This might lead to the subtle mistakes going unnoticed during rebases.
Yes, it's quite unlikely, but it might be still possible to have the empty
entry in the middle of the array. Why should we shoot ourselves in the foot
if we can avoid it without any effort?
Hence I can say it's discouraged.
--
With Best Regards,
Andy Shevchenko
[AMD Official Use Only - General]
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Wednesday, May 17, 2023 2:14 PM
> To: Greg KH <[email protected]>
> Cc: Limonciello, Mario <[email protected]>;
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; Quan, Evan <[email protected]>; Lazar, Lijo
> <[email protected]>; Goswami, Sanket <[email protected]>
> Subject: Re: [PATCH 2/2] usb: typec: ucsi: Don't create power supplies for
> dGPUs
>
> On Wed, May 17, 2023 at 08:58:49PM +0200, Greg KH wrote:
> > On Wed, May 17, 2023 at 08:14:13PM +0300, Andy Shevchenko wrote:
> > > On Tue, May 16, 2023 at 01:25:41PM -0500, Mario Limonciello wrote:
>
> ...
>
> > > > +static const struct property_entry dgpu_properties[] = {
> > > > + /* USB-C doesn't power the system */
> > > > + PROPERTY_ENTRY_U8("scope", POWER_SUPPLY_SCOPE_DEVICE),
> > > > + {},
> > >
> > > Comma is not needed in terminator entry.
> >
> > But it's encouraged!
>
> Why?! This might lead to the subtle mistakes going unnoticed during rebases.
> Yes, it's quite unlikely, but it might be still possible to have the empty
> entry in the middle of the array. Why should we shoot ourselves in the foot
> if we can avoid it without any effort?
>
> Hence I can say it's discouraged.
>
I tend to agree with Andy here, you should never be placing something after the
terminator entry and putting a comma there means a mistake /can/ happen.
I will pick up the tags and respin a v2 with this changed and patch 1 dropped
since it went through another tree.