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

Reply via email to