All the prior patches look pretty good. I've got everything staged for
pushing except this last patch:
On Mon, Mar 5, 2018 at 10:32 AM, Benjamin Tissoires
<benjamin.tissoi...@redhat.com> wrote:
> We take the same checks for the powersupply API than the one in the 3.17
> branch
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoi...@redhat.com>
> ---
> 3.7/wacom.h | 18 ++++++++-
> 3.7/wacom_sys.c | 113
> ++++++++++++++++++++++++++++++++++++++++++++------------
> 3.7/wacom_wac.c | 10 ++---
> 3 files changed, 110 insertions(+), 31 deletions(-)
>
> diff --git a/3.7/wacom.h b/3.7/wacom.h
> index de6c15f..50bdb5f 100644
> --- a/3.7/wacom.h
> +++ b/3.7/wacom.h
> @@ -110,6 +110,16 @@ MODULE_LICENSE(DRIVER_LICENSE);
> #define USB_VENDOR_ID_WACOM 0x056a
> #define USB_VENDOR_ID_LENOVO 0x17ef
>
> +#ifdef WACOM_POWERSUPPLY_41
> +#define WACOM_POWERSUPPLY_DEVICE(ps) (ps)
> +#define WACOM_POWERSUPPLY_REF(ps) (ps)
> +#define WACOM_POWERSUPPLY_DESC(ps) (ps##_desc)
> +#else
> +#define WACOM_POWERSUPPLY_DEVICE(ps) ((ps).dev)
> +#define WACOM_POWERSUPPLY_REF(ps) (&(ps))
> +#define WACOM_POWERSUPPLY_DESC(ps) (ps)
> +#endif
> +
> enum wacom_worker {
> WACOM_WORKER_WIRELESS,
> WACOM_WORKER_BATTERY,
> @@ -117,10 +127,14 @@ enum wacom_worker {
> };
>
> struct wacom_battery {
> + struct wacom *wacom;
> +#ifdef WACOM_POWERSUPPLY_41
> + struct power_supply_desc bat_desc;
> + struct power_supply *battery;
> +#else
> struct power_supply battery;
> - struct power_supply ac;
> +#endif
> char bat_name[WACOM_NAME_MAX];
> - char ac_name[WACOM_NAME_MAX];
> int bat_status;
> int battery_capacity;
> int bat_charging;
> diff --git a/3.7/wacom_sys.c b/3.7/wacom_sys.c
> index ef880dc..82cbb21 100644
> --- a/3.7/wacom_sys.c
> +++ b/3.7/wacom_sys.c
> @@ -1217,7 +1217,11 @@ static int wacom_battery_get_property(struct
> power_supply *psy,
> enum power_supply_property psp,
> union power_supply_propval *val)
> {
> +#ifdef WACOM_POWERSUPPLY_41
> + struct wacom_battery *battery = power_supply_get_drvdata(psy);
> +#else
> struct wacom_battery *battery = container_of(psy, struct
> wacom_battery, battery);
> +#endif
> int ret = 0;
>
> switch (psp) {
> @@ -1251,10 +1255,12 @@ static int wacom_battery_get_property(struct
> power_supply *psy,
> return ret;
> }
>
> +#ifndef WACOM_POWERSUPPLY_41
> static int __wacom_initialize_battery(struct wacom *wacom,
> struct wacom_battery *battery)
> {
> static atomic_t battery_no = ATOMIC_INIT(0);
> + struct device *dev = &wacom->usbdev->dev;
> static DEFINE_SPINLOCK(ps_lock);
> unsigned long flags;
> int error = 0;
> @@ -1264,29 +1270,71 @@ static int __wacom_initialize_battery(struct wacom
> *wacom,
>
> n = atomic_inc_return(&battery_no) - 1;
>
> - if (power_supply_get_by_name("wacom_battery"))
> - sprintf(battery->bat_name, "wacom_battery_%ld", n);
> - else
> - sprintf(battery->bat_name, "wacom_battery");
> -
> battery->battery.properties = wacom_battery_props;
> battery->battery.num_properties = ARRAY_SIZE(wacom_battery_props);
> battery->battery.get_property = wacom_battery_get_property;
> - battery->battery.name = battery->bat_name;
> + sprintf(wacom->battery.bat_name, "wacom_battery_%ld", n);
> + battery->battery.name = wacom->battery.bat_name;
> battery->battery.type = POWER_SUPPLY_TYPE_USB;
> battery->battery.use_for_apm = 0;
>
> - error = power_supply_register(&wacom->usbdev->dev,
> - &battery->battery);
> + error = power_supply_register(dev, &battery->battery);
> +
> + if (error)
> + return error;
>
> - if (!error)
> - power_supply_powers(&battery->battery,
> - &wacom->usbdev->dev);
> + power_supply_powers(WACOM_POWERSUPPLY_REF(battery->battery), dev);
>
> spin_unlock_irqrestore(&ps_lock, flags);
>
> + return 0;
> +}
This above changes modify behavior (no more unqualified
"wacom_battery" device), not just make the code compile under RHEL
7.5. It should really be broken out into a separate patch...
> +
> +#else
> +static int __wacom_initialize_battery(struct wacom *wacom,
> + struct wacom_battery *battery)
> +{
> + static atomic_t battery_no = ATOMIC_INIT(0);
> + struct device *dev = &wacom->usbdev->dev;
> + struct power_supply_config psy_cfg = { .drv_data = battery, };
> + struct power_supply *ps_bat;
> + struct power_supply_desc *bat_desc = &battery->bat_desc;
> + unsigned long n;
> + int error;
> +
> + if (!devres_open_group(dev, bat_desc, GFP_KERNEL))
> + return -ENOMEM;
> +
> + battery->wacom = wacom;
> +
> + n = atomic_inc_return(&battery_no) - 1;
> +
> + bat_desc->properties = wacom_battery_props;
> + bat_desc->num_properties = ARRAY_SIZE(wacom_battery_props);
> + bat_desc->get_property = wacom_battery_get_property;
> + sprintf(battery->bat_name, "wacom_battery_%ld", n);
> + bat_desc->name = battery->bat_name;
> + bat_desc->type = POWER_SUPPLY_TYPE_USB;
> + bat_desc->use_for_apm = 0;
> +
> + ps_bat = devm_power_supply_register(dev, bat_desc, &psy_cfg);
> + if (IS_ERR(ps_bat)) {
> + error = PTR_ERR(ps_bat);
> + goto err;
> + }
> +
> + power_supply_powers(ps_bat, &wacom->usbdev->dev);
> +
> + battery->battery = ps_bat;
> +
> + devres_close_group(dev, bat_desc);
> + return 0;
> +
> +err:
> + devres_release_group(dev, bat_desc);
> return error;
> }
> +#endif
>
> static int wacom_initialize_battery(struct wacom *wacom)
> {
> @@ -1298,19 +1346,26 @@ static int wacom_initialize_battery(struct wacom
> *wacom)
>
> static void wacom_destroy_battery(struct wacom *wacom)
> {
> - if (wacom->battery.battery.dev) {
> + if (WACOM_POWERSUPPLY_DEVICE(wacom->battery.battery)) {
> +#ifdef WACOM_POWERSUPPLY_41
> + devres_release_group(&wacom->usbdev->dev,
> + &wacom->battery.bat_desc);
> +#else
> power_supply_unregister(&wacom->battery.battery);
> - wacom->battery.battery.dev = NULL;
> +#endif
> + WACOM_POWERSUPPLY_DEVICE(wacom->battery.battery) = NULL;
> }
> }
>
> +#ifndef WACOM_POWERSUPPLY_41
> static void wacom_destroy_remote_battery(struct wacom_battery *battery)
> {
> - if (battery->battery.dev) {
> + if (WACOM_POWERSUPPLY_DEVICE(battery->battery)) {
> power_supply_unregister(&battery->battery);
> - battery->battery.dev = NULL;
> + WACOM_POWERSUPPLY_DEVICE(battery->battery) = NULL;
> }
> }
> +#endif
>
> static ssize_t wacom_show_remote_mode(struct kobject *kobj,
> struct kobj_attribute *kattr,
> @@ -1404,28 +1459,33 @@ static void wacom_remote_destroy_one(struct wacom
> *wacom, unsigned int index)
> remote->remotes[index].registered = false;
> spin_unlock_irqrestore(&remote->remote_lock, flags);
>
> - if (remote->remotes[index].group.name)
> - devres_release_group(&wacom->usbdev->dev,
> - &remote->remotes[index]);
> -
> if (remote->remotes[index].input)
> input_unregister_device(remote->remotes[index].input);
>
> +#ifdef WACOM_POWERSUPPLY_41
> + if (WACOM_POWERSUPPLY_DEVICE(remote->remotes[index].battery.battery))
> + devres_release_group(&wacom->usbdev->dev,
> +
> &remote->remotes[index].battery.bat_desc);
> +#else
> if (remote->remotes[index].battery.battery.dev)
> wacom_destroy_remote_battery(&remote->remotes[index].battery);
> +#endif
> +
> + if (remote->remotes[index].group.name)
> + devres_release_group(&wacom->usbdev->dev,
> + &remote->remotes[index]);
>
> for (i = 0; i < WACOM_MAX_REMOTES; i++) {
> if (remote->remotes[i].serial == serial) {
> remote->remotes[i].serial = 0;
> -
> /* Destroy the attribute group parts not
> * covered by devres for this kernel.
> */
> kfree((char *)remote->remotes[i].group.name);
> remote->remotes[i].group.name = NULL;
> remote->remotes[i].registered = false;
> - remote->remotes[i].battery.battery.dev = NULL;
> wacom->led.select[i] = WACOM_STATUS_UNKNOWN;
> +
> WACOM_POWERSUPPLY_DEVICE(remote->remotes[i].battery.battery) = NULL;
> }
> }
> }
> @@ -1468,16 +1528,21 @@ static const struct attribute *remote_unpair_attrs[]
> = {
> static void wacom_remotes_destroy(struct wacom *wacom)
> {
> struct wacom_remote *remote = wacom->remote;
> +
> +#ifndef WACOM_POWERSUPPLY_41
> int i;
> +#endif
>
> if (!remote)
> return;
>
> +#ifndef WACOM_POWERSUPPLY_41
> for (i = 0; i < WACOM_MAX_REMOTES; i++) {
> if (remote->remotes[i].registered) {
> wacom_remote_destroy_one(wacom, i);
> }
> }
> +#endif
This hunk caught my eye and I've spent a good long time staring at
it... I'm not convinced it's correct, but I'm also not convinced its
incorrect. The 3.17 tree I believe is relying on devres to clean up
all of the remote objects, and as long as that happens here it might
be okay. Though the put/free below make me wonder if reference counts
and objects will be in a sane state when this function ends, or if
we'll end up somehow dereferencing a NULL pointer when an EKR is
connected again.
Would you mind taking a second closer look to make sure this works as
intended? I'm having Aaron take a closer look as well since he's far
more familiar with the EKR than I am. There's a distinct code smell
here...
Jason
> kobject_put(remote->remote_dir);
> kfifo_free(&remote->remote_fifo);
> wacom->remote = NULL;
> @@ -1702,7 +1767,7 @@ static int wacom_remote_attach_battery(struct wacom
> *wacom, int index)
> if (!remote->remotes[index].registered)
> return 0;
>
> - if (remote->remotes[index].battery.battery.dev)
> + if (WACOM_POWERSUPPLY_DEVICE(remote->remotes[index].battery.battery))
> return 0;
>
> error = __wacom_initialize_battery(wacom,
> @@ -1858,11 +1923,11 @@ void wacom_battery_work(struct work_struct *work)
> struct wacom *wacom = container_of(work, struct wacom, battery_work);
>
> if ((wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) &&
> - !wacom->battery.battery.dev) {
> + !WACOM_POWERSUPPLY_DEVICE(wacom->battery.battery)) {
> wacom_initialize_battery(wacom);
> }
> else if (!(wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) &&
> - wacom->battery.battery.dev) {
> + WACOM_POWERSUPPLY_DEVICE(wacom->battery.battery)) {
> wacom_destroy_battery(wacom);
> }
> }
> diff --git a/3.7/wacom_wac.c b/3.7/wacom_wac.c
> index a02d191..a421aff 100644
> --- a/3.7/wacom_wac.c
> +++ b/3.7/wacom_wac.c
> @@ -76,8 +76,8 @@ static void __wacom_notify_battery(struct wacom_battery
> *battery,
> battery->bat_connected = bat_connected;
> battery->ps_connected = ps_connected;
>
> - if (battery->battery.dev)
> - power_supply_changed(&battery->battery);
> + if (WACOM_POWERSUPPLY_DEVICE(battery->battery))
> +
> power_supply_changed(WACOM_POWERSUPPLY_REF(battery->battery));
> }
> }
>
> @@ -1765,14 +1765,14 @@ static int wacom_status_irq(struct wacom_wac
> *wacom_wac, size_t len)
> wacom_notify_battery(wacom_wac,
> WACOM_POWER_SUPPLY_STATUS_AUTO,
> battery, charging, battery || charging,
> 1);
>
> - if (!wacom->battery.battery.dev &&
> + if (!WACOM_POWERSUPPLY_DEVICE(wacom->battery.battery) &&
> !(features->quirks & WACOM_QUIRK_BATTERY)) {
> features->quirks |= WACOM_QUIRK_BATTERY;
> wacom_schedule_work(wacom_wac, WACOM_WORKER_BATTERY);
> }
> }
> else if ((features->quirks & WACOM_QUIRK_BATTERY) &&
> - wacom->battery.battery.dev) {
> + WACOM_POWERSUPPLY_DEVICE(wacom->battery.battery)) {
> features->quirks &= ~WACOM_QUIRK_BATTERY;
> wacom_schedule_work(wacom_wac, WACOM_WORKER_BATTERY);
> wacom_notify_battery(wacom_wac, POWER_SUPPLY_STATUS_UNKNOWN,
> 0, 0, 0, 0);
> @@ -1810,7 +1810,7 @@ static int wacom_mspro_device_irq(struct wacom_wac
> *wacom)
> battery_level = data[1] & 0x7F;
> bat_charging = data[1] & 0x80;
>
> - if (!w->battery.battery.dev &&
> + if (!WACOM_POWERSUPPLY_DEVICE(w->battery.battery) &&
> !(features->quirks & WACOM_QUIRK_BATTERY)) {
> features->quirks |= WACOM_QUIRK_BATTERY;
> wacom_schedule_work(wacom, WACOM_WORKER_BATTERY);
> --
> 2.14.3
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Linuxwacom-devel mailing list
> Linuxwacom-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
