Re: [PATCH v4 4/6] power: supply: lenovo_yoga_c630_battery: add Lenovo C630 driver

[Dmitry Baryshkov]

On Wed, May 29, 2024 at 04:51:54PM +0100, Bryan O'Donoghue wrote:
> On 28/05/2024 21:44, Dmitry Baryshkov wrote:
> > On the Lenovo Yoga C630 WOS laptop the EC provides access to the adapter
> > and battery status. Add the driver to read power supply status on the
> > laptop.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> >   drivers/power/supply/Kconfig                    |   9 +
> >   drivers/power/supply/Makefile                   |   1 +
> >   drivers/power/supply/lenovo_yoga_c630_battery.c | 479 ++++++++++++++++++++++++
> >   3 files changed, 489 insertions(+)
> > 
> > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> > index 3e31375491d5..55ab8e90747d 100644
> > --- a/drivers/power/supply/Kconfig
> > +++ b/drivers/power/supply/Kconfig
> > @@ -167,6 +167,15 @@ config BATTERY_LEGO_EV3
> >   	help
> >   	  Say Y here to enable support for the LEGO MINDSTORMS EV3 battery.
> > +config BATTERY_LENOVO_YOGA_C630
> > +	tristate "Lenovo Yoga C630 battery"
> > +	depends on OF && EC_LENOVO_YOGA_C630
> > +	help
> > +	  This driver enables battery support on the Lenovo Yoga C630 laptop.
> > +
> > +	  To compile the driver as a module, choose M here: the module will be
> > +	  called lenovo_yoga_c630_battery.
> > +
> >   config BATTERY_PMU
> >   	tristate "Apple PMU battery"
> >   	depends on PPC32 && ADB_PMU
> > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> > index 58b567278034..8ebbdcf92dac 100644
> > --- a/drivers/power/supply/Makefile
> > +++ b/drivers/power/supply/Makefile
> > @@ -32,6 +32,7 @@ obj-$(CONFIG_BATTERY_DS2782)	+= ds2782_battery.o
> >   obj-$(CONFIG_BATTERY_GAUGE_LTC2941)	+= ltc2941-battery-gauge.o
> >   obj-$(CONFIG_BATTERY_GOLDFISH)	+= goldfish_battery.o
> >   obj-$(CONFIG_BATTERY_LEGO_EV3)	+= lego_ev3_battery.o
> > +obj-$(CONFIG_BATTERY_LENOVO_YOGA_C630) += lenovo_yoga_c630_battery.o
> >   obj-$(CONFIG_BATTERY_PMU)	+= pmu_battery.o
> >   obj-$(CONFIG_BATTERY_QCOM_BATTMGR)	+= qcom_battmgr.o
> >   obj-$(CONFIG_BATTERY_OLPC)	+= olpc_battery.o
> > diff --git a/drivers/power/supply/lenovo_yoga_c630_battery.c b/drivers/power/supply/lenovo_yoga_c630_battery.c
> > new file mode 100644
> > index 000000000000..76152ad38d46
> > --- /dev/null
> > +++ b/drivers/power/supply/lenovo_yoga_c630_battery.c
> > @@ -0,0 +1,479 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2022-2024, Linaro Ltd
> > + * Authors:
> > + *    Bjorn Andersson
> > + *    Dmitry Baryshkov
> > + */
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/delay.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_data/lenovo-yoga-c630.h>
> > +#include <linux/power_supply.h>
> > +
> > +struct yoga_c630_psy {
> > +	struct yoga_c630_ec *ec;
> > +	struct device *dev;
> > +	struct device_node *of_node;
> > +	struct notifier_block nb;
> > +	struct mutex lock;
> 
> Do locks still not require a
> 
> struct mutex lock; /* this mutex locks this thing */

Not required, but let me add the doc.

> 
> > +
> > +	struct power_supply *adp_psy;
> > +	struct power_supply *bat_psy;
> > +
> > +	unsigned long last_status_update;
> > +
> > +	bool adapter_online;
> > +
> > +	bool unit_mA;
> > +
> > +	bool bat_present;
> > +	unsigned int bat_status;
> > +	unsigned int design_capacity;
> > +	unsigned int design_voltage;
> > +	unsigned int full_charge_capacity;
> > +
> > +	unsigned int capacity_now;
> > +	unsigned int voltage_now;
> > +
> > +	int current_now;
> > +	int rate_now;
> > +};
> > +
> > +#define LENOVO_EC_CACHE_TIME		(10 * HZ)
> > +
> > +#define LENOVO_EC_ADPT_STATUS		0xa3
> > +#define LENOVO_EC_ADPT_PRESENT		BIT(7)
> > +#define LENOVO_EC_BAT_ATTRIBUTES	0xc0
> > +#define LENOVO_EC_BAT_ATTR_UNIT_IS_MA	BIT(1)
> > +#define LENOVO_EC_BAT_STATUS		0xc1
> > +#define LENOVO_EC_BAT_REMAIN_CAPACITY	0xc2
> > +#define LENOVO_EC_BAT_VOLTAGE		0xc6
> > +#define LENOVO_EC_BAT_DESIGN_VOLTAGE	0xc8
> > +#define LENOVO_EC_BAT_DESIGN_CAPACITY	0xca
> > +#define LENOVO_EC_BAT_FULL_CAPACITY	0xcc
> > +#define LENOVO_EC_BAT_CURRENT		0xd2
> > +#define LENOVO_EC_BAT_FULL_FACTORY	0xd6
> > +#define LENOVO_EC_BAT_PRESENT		0xda
> > +#define LENOVO_EC_BAT_FULL_REGISTER	0xdb
> > +#define LENOVO_EC_BAT_FULL_IS_FACTORY	BIT(0)
> > +
> > +/* the mutex should already be locked */
> > +static int yoga_c630_psy_update_bat_info(struct yoga_c630_psy *ecbat)
> > +{
> > +	struct yoga_c630_ec *ec = ecbat->ec;
> > +	int val;
> > +
> > +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_PRESENT);
> > +	if (val < 0)
> > +		return val;
> > +	ecbat->bat_present = !!(val & BIT(0));
> > +	if (!ecbat->bat_present)
> > +		return val;
> > +
> > +	val = yoga_c630_ec_read8(ec, LENOVO_EC_BAT_ATTRIBUTES);
> > +	if (val < 0)
> > +		return val;
> > +	ecbat->unit_mA = val & LENOVO_EC_BAT_ATTR_UNIT_IS_MA;
> > +
> > +	val = yoga_c630_ec_read16(ec, LENOVO_EC_BAT_DESIGN_CAPACITY);
> > +	if (val < 0)
> > +		return val;
> > +	ecbat->design_capacity = val * 1000;
> > +
> > +	msleep(50);
> 
> What's this for ? Also do you really want to hold a mutex for 50
> milliseconds ?

DSDT has these delays after each read, so I can only assume it is required.
Sleeping outside of the mutex() would mean that a concurrent thread
might break into this delay and query the EC.

[skipped]


> > +static int yoga_c630_psy_probe(struct auxiliary_device *adev,
> > +				   const struct auxiliary_device_id *id)
> > +{
> > +	struct yoga_c630_ec *ec = adev->dev.platform_data;
> > +	struct power_supply_config adp_cfg = {};
> > +	struct device *dev = &adev->dev;
> > +	struct yoga_c630_psy *ecbat;
> > +	int ret;
> > +
> > +	ecbat = devm_kzalloc(&adev->dev, sizeof(*ecbat), GFP_KERNEL);
> > +	if (!ecbat)
> > +		return -ENOMEM;
> > +
> > +	ecbat->ec = ec;
> > +	ecbat->dev = dev;
> > +	mutex_init(&ecbat->lock);
> > +	ecbat->of_node = adev->dev.parent->of_node;
> > +	ecbat->nb.notifier_call = yoga_c630_psy_notify;
> > +
> > +	auxiliary_set_drvdata(adev, ecbat);
> > +
> > +	adp_cfg.drv_data = ecbat;
> > +	adp_cfg.of_node = ecbat->of_node;
> > +	adp_cfg.supplied_to = (char **)&yoga_c630_psy_bat_psy_desc_mA.name;
> > +	adp_cfg.num_supplicants = 1;
> > +	ecbat->adp_psy = devm_power_supply_register_no_ws(dev, &yoga_c630_psy_adpt_psy_desc, &adp_cfg);
> > +	if (IS_ERR(ecbat->adp_psy)) {
> > +		dev_err(dev, "failed to register AC adapter supply\n");
> > +		return PTR_ERR(ecbat->adp_psy);
> > +	}
> > +
> > +	mutex_lock(&ecbat->lock);
> 
> Do you really need this lock here in your probe() function ? What's the
> parallel path of execution you are mitigating against here ?

Notifications from the battery driver can already happen at this point.
Also once the fist power supply is registered, userspace can potentially
access it, triggering EC access and updates of the PSY registration.

> 
> > +
> > +	ret = yoga_c630_psy_update_bat_info(ecbat);
> > +	if (ret)
> > +		goto err_unlock;
> > +
> > +	ret = yoga_c630_psy_register_bat_psy(ecbat);
> > +	if (ret)
> > +		goto err_unlock;
> > +
> > +	mutex_unlock(&ecbat->lock);
> > +


-- 
With best wishes
Dmitry