lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CABXOdTeVPhjZAHEmh3ceNd30jO3d-uvYK20EjcT-_N1kJ7ph_w@mail.gmail.com>
Date:	Mon, 1 Aug 2016 12:55:01 -0700
From:	Guenter Roeck <groeck@...gle.com>
To:	Chanwoo Choi <cw00.choi@...sung.com>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	myungjoo.ham@...sung.com, Chris Zhong <zyw@...k-chips.com>,
	Guenter Roeck <groeck@...omium.org>, chanwoo@...nel.org
Subject: Re: [PATCH v2 5/6] extcon: Add the synchronization extcon APIs to
 support the notification

On Sun, Jul 31, 2016 at 10:50 PM, Chanwoo Choi <cw00.choi@...sung.com> wrote:
> This patch adds the synchronization extcon APIs to support the notifications
> for both state and property. When extcon_*_sync() functions is called,
> the extcon informs the information from extcon provider to extcon client.
>
> The extcon driver may need to change the both state and multiple properties
> at the same time. After setting the data of a external connector,
> the extcon send the notification to client driver with the extcon_*_sync().
>
> The list of new extcon APIs as following:
> - extcon_sync() : Send the notification for each external connector to
>                 synchronize the information between extcon provider driver
>                 and extcon client driver.
> - extcon_set_state_sync() : Set the state of external connector with noti.
> - extcon_set_property_sync() : Set the property of external connector with noti.
>
> For example,
> case 1, change the state of external connector and synchronized the data.
>         extcon_set_state_sync(edev, EXTCON_USB, 1);
>
> case 2, change both the state and property of external connector
>         and synchronized the data.
>         extcon_set_state(edev, EXTCON_USB, 1);
>         extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_ID, 1);
>         extcon_sync(edev, EXTCON_USB);
>
> case 3, change the property of external connector and synchronized the data.
>         extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_VBUS, 0);
>         extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_ID, 1);
>         extcon_sync(edev, EXTCON_USB);
>
> case 4, change the property of external connector and synchronized the data.
>         extcon_set_property_sync(edev, EXTCON_USB, EXTCON_PROP_USB_VBUS, 0);
>
> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
> Tested-by: Chris Zhong <zyw@...k-chips.com>

Reviewed-by: Guenter Roeck <groeck@...omium.org>

[ couple of nitpicks below ]

> ---
>  drivers/extcon/extcon.c | 210 +++++++++++++++++++++++++++++++-----------------
>  include/linux/extcon.h  |  30 ++++++-
>  2 files changed, 164 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 207143347fb7..680246cceb62 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -279,14 +279,11 @@ static bool is_extcon_attached(struct extcon_dev *edev, unsigned int index)
>         return !!(edev->state & BIT(index));
>  }
>
> -static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
> +static bool is_extcon_changed(struct extcon_dev *edev, int index,
> +                               bool new_state)
>  {
> -       if (((prev >> idx) & 0x1) != ((new >> idx) & 0x1)) {
> -               *attached = ((new >> idx) & 0x1) ? true : false;
> -               return true;
> -       }
> -
> -       return false;
> +       int state = !!(edev->state & (1 << index));
> +       return (state != new_state) ? true : false;

This is identical to
            return state != new_state;

>  }
>
>  static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
> @@ -402,21 +399,13 @@ static ssize_t cable_state_show(struct device *dev,
>  }
>
>  /**
> - * extcon_update_state() - Update the cable attach states of the extcon device
> - *                        only for the masked bits.
> - * @edev:      the extcon device
> - * @mask:      the bit mask to designate updated bits.
> - * @state:     new cable attach status for @edev
> - *
> - * Changing the state sends uevent with environment variable containing
> - * the name of extcon device (envp[0]) and the state output (envp[1]).
> - * Tizen uses this format for extcon device to get events from ports.
> - * Android uses this format as well.
> + * extcon_sync()       - Synchronize the states for both the attached/detached
> + * @edev:              the extcon device that has the cable.
>   *
> - * Note that the notifier provides which bits are changed in the state
> - * variable with the val parameter (second) to the callback.
> + * This function send a notification to synchronize the all states of a
> + * specific external connector
>   */
> -static int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
> +int extcon_sync(struct extcon_dev *edev, unsigned int id)
>  {
>         char name_buf[120];
>         char state_buf[120];
> @@ -425,73 +414,58 @@ static int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
>         int env_offset = 0;
>         int length;
>         int index;
> +       int state;
>         unsigned long flags;
> -       bool attached;
>
>         if (!edev)
>                 return -EINVAL;
>
> +       index = find_cable_index_by_id(edev, id);
> +       if (index < 0)
> +               return index;
> +
>         spin_lock_irqsave(&edev->lock, flags);
>
> -       if (edev->state != ((edev->state & ~mask) | (state & mask))) {
> -               u32 old_state;
> +       state = !!(edev->state & (1 << index));

At some point it might make sense to update the entire file to use
BIT(index) instead of "1 << index".

> +       raw_notifier_call_chain(&edev->nh[index], state, edev);
>
> -               if (check_mutually_exclusive(edev, (edev->state & ~mask) |
> -                                                  (state & mask))) {
> -                       spin_unlock_irqrestore(&edev->lock, flags);
> -                       return -EPERM;
> -               }
> +       /* This could be in interrupt handler */
> +       prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
> +       if (!prop_buf) {
> +               /* Unlock early before uevent */
> +               spin_unlock_irqrestore(&edev->lock, flags);
>
> -               old_state = edev->state;
> -               edev->state &= ~mask;
> -               edev->state |= state & mask;
> +               dev_err(&edev->dev, "out of memory in extcon_set_state\n");
> +               kobject_uevent(&edev->dev.kobj, KOBJ_CHANGE);
>
> -               for (index = 0; index < edev->max_supported; index++) {
> -                       if (is_extcon_changed(old_state, edev->state, index,
> -                                             &attached))
> -                               raw_notifier_call_chain(&edev->nh[index],
> -                                                       attached, edev);
> -               }
> -
> -               /* This could be in interrupt handler */
> -               prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
> -               if (prop_buf) {
> -                       length = name_show(&edev->dev, NULL, prop_buf);
> -                       if (length > 0) {
> -                               if (prop_buf[length - 1] == '\n')
> -                                       prop_buf[length - 1] = 0;
> -                               snprintf(name_buf, sizeof(name_buf),
> -                                       "NAME=%s", prop_buf);
> -                               envp[env_offset++] = name_buf;
> -                       }
> -                       length = state_show(&edev->dev, NULL, prop_buf);
> -                       if (length > 0) {
> -                               if (prop_buf[length - 1] == '\n')
> -                                       prop_buf[length - 1] = 0;
> -                               snprintf(state_buf, sizeof(state_buf),
> -                                       "STATE=%s", prop_buf);
> -                               envp[env_offset++] = state_buf;
> -                       }
> -                       envp[env_offset] = NULL;
> -                       /* Unlock early before uevent */
> -                       spin_unlock_irqrestore(&edev->lock, flags);
> +               return 0;
> +       }
>
> -                       kobject_uevent_env(&edev->dev.kobj, KOBJ_CHANGE, envp);
> -                       free_page((unsigned long)prop_buf);
> -               } else {
> -                       /* Unlock early before uevent */
> -                       spin_unlock_irqrestore(&edev->lock, flags);
> +       length = name_show(&edev->dev, NULL, prop_buf);
> +       if (length > 0) {
> +               if (prop_buf[length - 1] == '\n')
> +                       prop_buf[length - 1] = 0;
> +               snprintf(name_buf, sizeof(name_buf), "NAME=%s", prop_buf);
> +               envp[env_offset++] = name_buf;
> +       }
>
> -                       dev_err(&edev->dev, "out of memory in extcon_set_state\n");
> -                       kobject_uevent(&edev->dev.kobj, KOBJ_CHANGE);
> -               }
> -       } else {
> -               /* No changes */
> -               spin_unlock_irqrestore(&edev->lock, flags);
> +       length = state_show(&edev->dev, NULL, prop_buf);
> +       if (length > 0) {
> +               if (prop_buf[length - 1] == '\n')
> +                       prop_buf[length - 1] = 0;
> +               snprintf(state_buf, sizeof(state_buf), "STATE=%s", prop_buf);
> +               envp[env_offset++] = state_buf;
>         }
> +       envp[env_offset] = NULL;
> +
> +       /* Unlock early before uevent */
> +       spin_unlock_irqrestore(&edev->lock, flags);
> +       kobject_uevent_env(&edev->dev.kobj, KOBJ_CHANGE, envp);
> +       free_page((unsigned long)prop_buf);
>
>         return 0;
>  }
> +EXPORT_SYMBOL_GPL(extcon_sync);
>
>  /**
>   * extcon_get_state() - Get the state of a external connector.
> @@ -520,17 +494,22 @@ EXPORT_SYMBOL_GPL(extcon_get_state);
>
>  /**
>   * extcon_set_state() - Set the state of a external connector.
> + *                     without a notification.
>   * @edev:              the extcon device that has the cable.
>   * @id:                        the unique id of each external connector
>   *                     in extcon enumeration.
>   * @state:             the new cable status. The default semantics is
>   *                     true: attached / false: detached.
> + *
> + * This function only set the state of a external connector without
> + * a notification. To synchronize the data of a external connector,
> + * use extcon_set_state_sync() and extcon_sync().
>   */
>  int extcon_set_state(struct extcon_dev *edev, unsigned int id,
>                                 bool cable_state)
>  {
> -       u32 state;
> -       int index;
> +       unsigned long flags;
> +       int index, ret = 0;
>
>         if (!edev)
>                 return -EINVAL;
> @@ -539,6 +518,18 @@ int extcon_set_state(struct extcon_dev *edev, unsigned int id,
>         if (index < 0)
>                 return index;
>
> +       spin_lock_irqsave(&edev->lock, flags);
> +
> +       /* Check whether the external connector's state is changed. */
> +       if (!is_extcon_changed(edev, index, cable_state))
> +               goto out;
> +
> +       if (check_mutually_exclusive(edev,
> +               (edev->state & ~(1 << index)) | (cable_state & (1 << index)))) {
> +               ret = -EPERM;
> +               goto out;
> +       }
> +
>         /*
>          * Initialize the value of extcon property before setting
>          * the detached state for an external connector.
> @@ -546,12 +537,56 @@ int extcon_set_state(struct extcon_dev *edev, unsigned int id,
>         if (!cable_state)
>                 init_property(edev, id, index);
>
> -       state = cable_state ? (1 << index) : 0;
> -       return extcon_update_state(edev, 1 << index, state);
> +       /* Update the state for a external connector. */
> +       if (cable_state)
> +               edev->state |= (1 << index);
> +       else
> +               edev->state &= ~(1 << index);
> +out:
> +       spin_unlock_irqrestore(&edev->lock, flags);
> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(extcon_set_state);
>
>  /**
> + * extcon_set_state_sync() - Set the state of a external connector
> + *                     with a notification.
> + * @edev:              the extcon device that has the cable.
> + * @id:                        the unique id of each external connector
> + *                     in extcon enumeration.
> + * @state:             the new cable status. The default semantics is
> + *                     true: attached / false: detached.
> + *
> + * This function set the state of external connector and synchronize the data
> + * by usning a notification.
> + */
> +int extcon_set_state_sync(struct extcon_dev *edev, unsigned int id,
> +                               bool cable_state)
> +{
> +       int ret, index;
> +       unsigned long flags;
> +
> +       index = find_cable_index_by_id(edev, id);
> +       if (index < 0)
> +               return index;
> +
> +       /* Check whether the external connector's state is changed. */
> +       spin_lock_irqsave(&edev->lock, flags);
> +       ret = is_extcon_changed(edev, index, cable_state);
> +       spin_unlock_irqrestore(&edev->lock, flags);
> +       if (!ret)
> +               return 0;
> +
> +       ret = extcon_set_state(edev, id, cable_state);
> +       if (ret < 0)
> +               return ret;
> +
> +       return extcon_sync(edev, id);
> +}
> +EXPORT_SYMBOL_GPL(extcon_set_state_sync);
> +
> +/**
>   * extcon_get_property() - Get the property value of a specific cable.
>   * @edev:              the extcon device that has the cable.
>   * @id:                        the unique id of each external connector
> @@ -702,6 +737,31 @@ int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>  EXPORT_SYMBOL_GPL(extcon_set_property);
>
>  /**
> + * extcon_set_property_sync() - Set the property value of a specific cable
> +                       with a notification.
> + * @prop_val:          the pointer including the new value of property.
> + *
> + * When setting the property value of external connector, the external connector
> + * should be attached. The each property should be included in the list of
> + * supported properties according to the type of external connectors.
> + *
> + * Returns 0 if success or error number if fail
> + */
> +int extcon_set_property_sync(struct extcon_dev *edev, unsigned int id,
> +                               unsigned int prop,
> +                               union extcon_property_value prop_val)
> +{
> +       int ret;
> +
> +       ret = extcon_set_property(edev, id, prop, prop_val);
> +       if (ret < 0)
> +               return ret;
> +
> +       return extcon_sync(edev, id);
> +}
> +EXPORT_SYMBOL_GPL(extcon_set_property_sync);
> +
> +/**
>   * extcon_get_property_capability() - Get the capability of property
>   *                     of an external connector.
>   * @edev:              the extcon device that has the cable.
> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
> index a38a42418195..f686204fb4c7 100644
> --- a/include/linux/extcon.h
> +++ b/include/linux/extcon.h
> @@ -227,6 +227,13 @@ extern void devm_extcon_dev_free(struct device *dev, struct extcon_dev *edev);
>  extern int extcon_get_state(struct extcon_dev *edev, unsigned int id);
>  extern int extcon_set_state(struct extcon_dev *edev, unsigned int id,
>                                    bool cable_state);
> +extern int extcon_set_state_sync(struct extcon_dev *edev, unsigned int id,
> +                               bool cable_state);
> +
> +/*
> + * Synchronize the state and property data for a specific external connector.
> + */
> +extern int extcon_sync(struct extcon_dev *edev, unsigned int id);
>
>  /*
>   * get/set_property access the property value of each external connector.
> @@ -238,6 +245,9 @@ extern int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>  extern int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>                                 unsigned int prop,
>                                 union extcon_property_value prop_val);
> +extern int extcon_set_property_sync(struct extcon_dev *edev, unsigned int id,
> +                               unsigned int prop,
> +                               union extcon_property_value prop_val);
>
>  /*
>   * get/set_property_capability set the capability of the property for each
> @@ -322,6 +332,17 @@ static inline int extcon_set_state(struct extcon_dev *edev, unsigned int id,
>         return 0;
>  }
>
> +static inline int extcon_set_state_sync(struct extcon_dev *edev, unsigned int id,
> +                               bool cable_state)
> +{
> +       return 0;
> +}
> +
> +static inline int extcon_sync(struct extcon_dev *edev, unsigned int id)
> +{
> +       return 0;
> +}
> +
>  static inline int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>                                         unsigned int prop,
>                                         union extcon_property_value *prop_val)
> @@ -335,6 +356,13 @@ static inline int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>         return 0;
>  }
>
> +static inline int extcon_set_property_sync(struct extcon_dev *edev,
> +                                       unsigned int id, unsigned int prop,
> +                                       union extcon_property_value prop_val)
> +{
> +       return 0;
> +}
> +
>  static inline int extcon_get_property_capability(struct extcon_dev *edev,
>                                         unsigned int id, unsigned int prop)
>  {
> @@ -416,6 +444,6 @@ static inline int extcon_get_cable_state_(struct extcon_dev *edev, unsigned int
>  static inline int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
>                                    bool cable_state)
>  {
> -       return extcon_set_state(edev, id, cable_state);
> +       return extcon_set_state_sync(edev, id, cable_state);
>  }
>  #endif /* __LINUX_EXTCON_H__ */
> --
> 1.9.1
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ