[PATCH 1/2] extcon: Use BIT() macro for the left-shift operation

[PATCH 1/2] extcon: Use BIT() macro for the left-shift operation

[Chanwoo Choi]

This patch just uses the BIT() macro to make the code simple.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/extcon/extcon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index e85704eda645..193a3a673d10 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -377,7 +377,7 @@ static ssize_t state_show(struct device *dev, struct device_attribute *attr,
 	for (i = 0; i < edev->max_supported; i++) {
 		count += sprintf(buf + count, "%s=%d\n",
 				extcon_info[edev->supported_cable[i]].name,
-				 !!(edev->state & (1 << i)));
+				 !!(edev->state & BIT(i)));
 	}
 
 	return count;
-- 
1.9.1

[PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors

[Chanwoo Choi]

The extcon core already provides the extcon_register_notifier() function
in order to register the notifier block which is used to monitor
the status change for the specific external connector such as EXTCON_USB,
EXTCON_USB_HOST and so on. The extcon consumer uses the this function.

The extcon consumer may need to monitor the all supported external
connectors from the extcon device. In this case, The extcon consumer
should have each notifier_block structure for each external connector.

This patch adds the new extcon_register_notifier_all() function
that extcon consumer is able to monitor the status change of all
supported external connectors by using only one notifier_block structure.

- List of new added functions:
int extcon_register_notifier_all(struct extcon_dev *edev,
			struct notifier_block *nb);
int extcon_unregister_notifier_all(struct extcon_dev *edev,
			struct notifier_block *nb);
int devm_extcon_register_notifier_all(struct device *dev,
			struct extcon_dev *edev, struct notifier_block *nb);
void devm_extcon_unregister_notifier_all(struct device *dev,
			struct extcon_dev *edev, struct notifier_block *nb);

Suggested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/extcon/devres.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/extcon/extcon.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/extcon/extcon.h |  3 +++
 include/linux/extcon.h  | 21 ++++++++++++----
 4 files changed, 145 insertions(+), 5 deletions(-)

diff --git a/drivers/extcon/devres.c b/drivers/extcon/devres.c
index b40eb1805927..3fc8eea52f1d 100644
--- a/drivers/extcon/devres.c
+++ b/drivers/extcon/devres.c
@@ -50,6 +50,13 @@ static void devm_extcon_dev_notifier_unreg(struct device *dev, void *res)
 	extcon_unregister_notifier(this->edev, this->id, this->nb);
 }
 
+static void devm_extcon_dev_notifier_all_unreg(struct device *dev, void *res)
+{
+	struct extcon_dev_notifier_devres *this = res;
+
+	extcon_unregister_notifier_all(this->edev, this->nb);
+}
+
 /**
  * devm_extcon_dev_allocate - Allocate managed extcon device
  * @dev:		device owning the extcon device being created
@@ -214,3 +221,60 @@ void devm_extcon_unregister_notifier(struct device *dev,
 			       devm_extcon_dev_match, edev));
 }
 EXPORT_SYMBOL(devm_extcon_unregister_notifier);
+
+/**
+ * devm_extcon_register_notifier_all()
+ *		- Resource-managed extcon_register_notifier_all()
+ * @dev:	device to allocate extcon device
+ * @edev:	the extcon device that has the external connecotr.
+ * @nb:		a notifier block to be registered.
+ *
+ * This function manages automatically the notifier of extcon device using
+ * device resource management and simplify the control of unregistering
+ * the notifier of extcon device.
+ *
+ * Note that the second parameter given to the callback of nb (val) is
+ * the current state and third parameter is the edev pointer.
+ *
+ * Returns 0 if success or negaive error number if failure.
+ */
+int devm_extcon_register_notifier_all(struct device *dev, struct extcon_dev *edev,
+				struct notifier_block *nb)
+{
+	struct extcon_dev_notifier_devres *ptr;
+	int ret;
+
+	ptr = devres_alloc(devm_extcon_dev_notifier_all_unreg, sizeof(*ptr),
+				GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	ret = extcon_register_notifier_all(edev, nb);
+	if (ret) {
+		devres_free(ptr);
+		return ret;
+	}
+
+	ptr->edev = edev;
+	ptr->nb = nb;
+	devres_add(dev, ptr);
+
+	return 0;
+}
+EXPORT_SYMBOL(devm_extcon_register_notifier_all);
+
+/**
+ * devm_extcon_unregister_notifier_all()
+			- Resource-managed extcon_unregister_notifier_all()
+ * @dev:	device to allocate extcon device
+ * @edev:	the extcon device that has the external connecotr.
+ * @nb:		a notifier block to be registered.
+ */
+void devm_extcon_unregister_notifier_all(struct device *dev,
+				struct extcon_dev *edev,
+				struct notifier_block *nb)
+{
+	WARN_ON(devres_release(dev, devm_extcon_dev_notifier_all_unreg,
+			       devm_extcon_dev_match, edev));
+}
+EXPORT_SYMBOL(devm_extcon_unregister_notifier_all);
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 193a3a673d10..4873fe8d5cd8 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -445,8 +445,19 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
 	spin_lock_irqsave(&edev->lock, flags);
 
 	state = !!(edev->state & BIT(index));
+
+	/*
+	 * Call functions in a raw notifier chain for the specific one
+	 * external connector.
+	 */
 	raw_notifier_call_chain(&edev->nh[index], state, edev);
 
+	/*
+	 * Call functions in a raw notifier chain for the all supported
+	 * external connectors.
+	 */
+	raw_notifier_call_chain(&edev->nh_all, state, edev);
+
 	/* This could be in interrupt handler */
 	prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
 	if (!prop_buf) {
@@ -951,6 +962,55 @@ int extcon_unregister_notifier(struct extcon_dev *edev, unsigned int id,
 }
 EXPORT_SYMBOL_GPL(extcon_unregister_notifier);
 
+/**
+ * extcon_register_notifier_all() - Register a notifier block to get the noti
+ *				of the status change for all supported external
+ *				connectors from extcon.
+ * @edev:	the extcon device that has the external connecotr.
+ * @nb:		a notifier block to be registered.
+ *
+ * Note that the second parameter given to the callback of nb (val) is
+ * the current state and third parameter is the edev pointer.
+ */
+int extcon_register_notifier_all(struct extcon_dev *edev,
+				struct notifier_block *nb)
+{
+	unsigned long flags;
+	int ret;
+
+	if (!edev || !nb)
+		return -EINVAL;
+
+	spin_lock_irqsave(&edev->lock, flags);
+	ret = raw_notifier_chain_register(&edev->nh_all, nb);
+	spin_unlock_irqrestore(&edev->lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(extcon_register_notifier_all);
+
+/**
+ * extcon_unregister_notifier_all() - Unregister a notifiee from the extcon device.
+ * @edev:	the extcon device that has the external connecotr.
+ * @nb:		a notifier block to be registered.
+ */
+int extcon_unregister_notifier_all(struct extcon_dev *edev,
+				struct notifier_block *nb)
+{
+	unsigned long flags;
+	int ret;
+
+	if (!edev || !nb)
+		return -EINVAL;
+
+	spin_lock_irqsave(&edev->lock, flags);
+	ret = raw_notifier_chain_unregister(&edev->nh_all, nb);
+	spin_unlock_irqrestore(&edev->lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(extcon_unregister_notifier_all);
+
 static struct attribute *extcon_attrs[] = {
 	&dev_attr_state.attr,
 	&dev_attr_name.attr,
@@ -1199,6 +1259,8 @@ int extcon_dev_register(struct extcon_dev *edev)
 	for (index = 0; index < edev->max_supported; index++)
 		RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]);
 
+	RAW_INIT_NOTIFIER_HEAD(&edev->nh_all);
+
 	dev_set_drvdata(&edev->dev, edev);
 	edev->state = 0;
 
diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
index 993ddccafe11..dddddcfa0587 100644
--- a/drivers/extcon/extcon.h
+++ b/drivers/extcon/extcon.h
@@ -21,6 +21,8 @@
  * @dev:		Device of this extcon.
  * @state:		Attach/detach state of this extcon. Do not provide at
  *			register-time.
+ * @nh_all:		Notifier for the state change events for all supported
+ *			external connectors from this extcon.
  * @nh:			Notifier for the state change events from this extcon
  * @entry:		To support list of extcon devices so that users can
  *			search for extcon devices based on the extcon name.
@@ -43,6 +45,7 @@ struct extcon_dev {
 
 	/* Internal data. Please do not set. */
 	struct device dev;
+	struct raw_notifier_head nh_all;
 	struct raw_notifier_head *nh;
 	struct list_head entry;
 	int max_supported;
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index 3929d2e8a3c7..61e8d8c591a4 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -270,11 +270,11 @@ extern int extcon_set_property_capability(struct extcon_dev *edev,
 				unsigned int id, unsigned int prop);
 
 /*
- * Following APIs are to monitor every action of a notifier.
- * Registrar gets notified for every external port of a connection device.
- * Probably this could be used to debug an action of notifier; however,
- * we do not recommend to use this for normal 'notifiee' device drivers who
- * want to be notified by a specific external port of the notifier.
+ * Following APIs are to monitor the status change of the external connectors.
+ * extcon_register_notifier(*edev, id, *nb) : Register a notifier
+ *			for specific external connector from the extcon.
+ * extcon_register_notifier_all(*edev, *nb) : Register a notifier
+ *			for all supported external connectors from the extcon.
  */
 extern int extcon_register_notifier(struct extcon_dev *edev, unsigned int id,
 				    struct notifier_block *nb);
@@ -287,6 +287,17 @@ extern void devm_extcon_unregister_notifier(struct device *dev,
 				struct extcon_dev *edev, unsigned int id,
 				struct notifier_block *nb);
 
+extern int extcon_register_notifier_all(struct extcon_dev *edev,
+				struct notifier_block *nb);
+extern int extcon_unregister_notifier_all(struct extcon_dev *edev,
+				struct notifier_block *nb);
+extern int devm_extcon_register_notifier_all(struct device *dev,
+				struct extcon_dev *edev,
+				struct notifier_block *nb);
+extern void devm_extcon_unregister_notifier_all(struct device *dev,
+				struct extcon_dev *edev,
+				struct notifier_block *nb);
+
 /*
  * Following API get the extcon device from devicetree.
  * This function use phandle of devicetree to get extcon device directly.
-- 
1.9.1

Re: [PATCH 1/2] extcon: Use BIT() macro for the left-shift operation

[Andy Shevchenko]

On Thu, Mar 30, 2017 at 11:39 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> This patch just uses the BIT() macro to make the code simple.

>         for (i = 0; i < edev->max_supported; i++) {
>                 count += sprintf(buf + count, "%s=%d\n",
>                                 extcon_info[edev->supported_cable[i]].name,
> -                                !!(edev->state & (1 << i)));
> +                                !!(edev->state & BIT(i)));
>         }

While change is okay, the above code is fragile. There is a potential
buffer overflow.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors

[Hans de Goede]

Hi Chanwoo,

On 30-03-17 10:39, Chanwoo Choi wrote:
> The extcon core already provides the extcon_register_notifier() function
> in order to register the notifier block which is used to monitor
> the status change for the specific external connector such as EXTCON_USB,
> EXTCON_USB_HOST and so on. The extcon consumer uses the this function.
>
> The extcon consumer may need to monitor the all supported external
> connectors from the extcon device. In this case, The extcon consumer
> should have each notifier_block structure for each external connector.
>
> This patch adds the new extcon_register_notifier_all() function
> that extcon consumer is able to monitor the status change of all
> supported external connectors by using only one notifier_block structure.
>
> - List of new added functions:
> int extcon_register_notifier_all(struct extcon_dev *edev,
> 			struct notifier_block *nb);
> int extcon_unregister_notifier_all(struct extcon_dev *edev,
> 			struct notifier_block *nb);
> int devm_extcon_register_notifier_all(struct device *dev,
> 			struct extcon_dev *edev, struct notifier_block *nb);
> void devm_extcon_unregister_notifier_all(struct device *dev,
> 			struct extcon_dev *edev, struct notifier_block *nb);
>
> Suggested-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/extcon/devres.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/extcon/extcon.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/extcon/extcon.h |  3 +++
>  include/linux/extcon.h  | 21 ++++++++++++----
>  4 files changed, 145 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/extcon/devres.c b/drivers/extcon/devres.c
> index b40eb1805927..3fc8eea52f1d 100644
> --- a/drivers/extcon/devres.c
> +++ b/drivers/extcon/devres.c
> @@ -50,6 +50,13 @@ static void devm_extcon_dev_notifier_unreg(struct device *dev, void *res)
>  	extcon_unregister_notifier(this->edev, this->id, this->nb);
>  }
>
> +static void devm_extcon_dev_notifier_all_unreg(struct device *dev, void *res)
> +{
> +	struct extcon_dev_notifier_devres *this = res;
> +
> +	extcon_unregister_notifier_all(this->edev, this->nb);
> +}
> +
>  /**
>   * devm_extcon_dev_allocate - Allocate managed extcon device
>   * @dev:		device owning the extcon device being created
> @@ -214,3 +221,60 @@ void devm_extcon_unregister_notifier(struct device *dev,
>  			       devm_extcon_dev_match, edev));
>  }
>  EXPORT_SYMBOL(devm_extcon_unregister_notifier);
> +
> +/**
> + * devm_extcon_register_notifier_all()
> + *		- Resource-managed extcon_register_notifier_all()
> + * @dev:	device to allocate extcon device
> + * @edev:	the extcon device that has the external connecotr.
> + * @nb:		a notifier block to be registered.
> + *
> + * This function manages automatically the notifier of extcon device using
> + * device resource management and simplify the control of unregistering
> + * the notifier of extcon device.
> + *
> + * Note that the second parameter given to the callback of nb (val) is
> + * the current state and third parameter is the edev pointer.
> + *
> + * Returns 0 if success or negaive error number if failure.
> + */
> +int devm_extcon_register_notifier_all(struct device *dev, struct extcon_dev *edev,
> +				struct notifier_block *nb)
> +{
> +	struct extcon_dev_notifier_devres *ptr;
> +	int ret;
> +
> +	ptr = devres_alloc(devm_extcon_dev_notifier_all_unreg, sizeof(*ptr),
> +				GFP_KERNEL);
> +	if (!ptr)
> +		return -ENOMEM;
> +
> +	ret = extcon_register_notifier_all(edev, nb);
> +	if (ret) {
> +		devres_free(ptr);
> +		return ret;
> +	}
> +
> +	ptr->edev = edev;
> +	ptr->nb = nb;
> +	devres_add(dev, ptr);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(devm_extcon_register_notifier_all);
> +
> +/**
> + * devm_extcon_unregister_notifier_all()
> +			- Resource-managed extcon_unregister_notifier_all()
> + * @dev:	device to allocate extcon device
> + * @edev:	the extcon device that has the external connecotr.
> + * @nb:		a notifier block to be registered.
> + */
> +void devm_extcon_unregister_notifier_all(struct device *dev,
> +				struct extcon_dev *edev,
> +				struct notifier_block *nb)
> +{
> +	WARN_ON(devres_release(dev, devm_extcon_dev_notifier_all_unreg,
> +			       devm_extcon_dev_match, edev));
> +}
> +EXPORT_SYMBOL(devm_extcon_unregister_notifier_all);
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 193a3a673d10..4873fe8d5cd8 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -445,8 +445,19 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
>  	spin_lock_irqsave(&edev->lock, flags);
>
>  	state = !!(edev->state & BIT(index));
> +
> +	/*
> +	 * Call functions in a raw notifier chain for the specific one
> +	 * external connector.
> +	 */
>  	raw_notifier_call_chain(&edev->nh[index], state, edev);
>
> +	/*
> +	 * Call functions in a raw notifier chain for the all supported
> +	 * external connectors.
> +	 */
> +	raw_notifier_call_chain(&edev->nh_all, state, edev);
> +

Passing state here is not useful since the receiver of
the notifier call will not know for which cable the state is.

Also this should be moved outside of the area of the
function holding the edev->lock spinlock, since we don't
pass state we do not need the lock and the called
notifier function may very well want to call extcon_get_state
to find out the state of one or more of the cables,
which takes the lock.

Otherwise looks good.

Regards,

Hans



>  	/* This could be in interrupt handler */
>  	prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
>  	if (!prop_buf) {
> @@ -951,6 +962,55 @@ int extcon_unregister_notifier(struct extcon_dev *edev, unsigned int id,
>  }
>  EXPORT_SYMBOL_GPL(extcon_unregister_notifier);
>
> +/**
> + * extcon_register_notifier_all() - Register a notifier block to get the noti
> + *				of the status change for all supported external
> + *				connectors from extcon.
> + * @edev:	the extcon device that has the external connecotr.
> + * @nb:		a notifier block to be registered.
> + *
> + * Note that the second parameter given to the callback of nb (val) is
> + * the current state and third parameter is the edev pointer.
> + */
> +int extcon_register_notifier_all(struct extcon_dev *edev,
> +				struct notifier_block *nb)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	if (!edev || !nb)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&edev->lock, flags);
> +	ret = raw_notifier_chain_register(&edev->nh_all, nb);
> +	spin_unlock_irqrestore(&edev->lock, flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(extcon_register_notifier_all);
> +
> +/**
> + * extcon_unregister_notifier_all() - Unregister a notifiee from the extcon device.
> + * @edev:	the extcon device that has the external connecotr.
> + * @nb:		a notifier block to be registered.
> + */
> +int extcon_unregister_notifier_all(struct extcon_dev *edev,
> +				struct notifier_block *nb)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	if (!edev || !nb)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&edev->lock, flags);
> +	ret = raw_notifier_chain_unregister(&edev->nh_all, nb);
> +	spin_unlock_irqrestore(&edev->lock, flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(extcon_unregister_notifier_all);
> +
>  static struct attribute *extcon_attrs[] = {
>  	&dev_attr_state.attr,
>  	&dev_attr_name.attr,
> @@ -1199,6 +1259,8 @@ int extcon_dev_register(struct extcon_dev *edev)
>  	for (index = 0; index < edev->max_supported; index++)
>  		RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]);
>
> +	RAW_INIT_NOTIFIER_HEAD(&edev->nh_all);
> +
>  	dev_set_drvdata(&edev->dev, edev);
>  	edev->state = 0;
>
> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
> index 993ddccafe11..dddddcfa0587 100644
> --- a/drivers/extcon/extcon.h
> +++ b/drivers/extcon/extcon.h
> @@ -21,6 +21,8 @@
>   * @dev:		Device of this extcon.
>   * @state:		Attach/detach state of this extcon. Do not provide at
>   *			register-time.
> + * @nh_all:		Notifier for the state change events for all supported
> + *			external connectors from this extcon.
>   * @nh:			Notifier for the state change events from this extcon
>   * @entry:		To support list of extcon devices so that users can
>   *			search for extcon devices based on the extcon name.
> @@ -43,6 +45,7 @@ struct extcon_dev {
>
>  	/* Internal data. Please do not set. */
>  	struct device dev;
> +	struct raw_notifier_head nh_all;
>  	struct raw_notifier_head *nh;
>  	struct list_head entry;
>  	int max_supported;
> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
> index 3929d2e8a3c7..61e8d8c591a4 100644
> --- a/include/linux/extcon.h
> +++ b/include/linux/extcon.h
> @@ -270,11 +270,11 @@ extern int extcon_set_property_capability(struct extcon_dev *edev,
>  				unsigned int id, unsigned int prop);
>
>  /*
> - * Following APIs are to monitor every action of a notifier.
> - * Registrar gets notified for every external port of a connection device.
> - * Probably this could be used to debug an action of notifier; however,
> - * we do not recommend to use this for normal 'notifiee' device drivers who
> - * want to be notified by a specific external port of the notifier.
> + * Following APIs are to monitor the status change of the external connectors.
> + * extcon_register_notifier(*edev, id, *nb) : Register a notifier
> + *			for specific external connector from the extcon.
> + * extcon_register_notifier_all(*edev, *nb) : Register a notifier
> + *			for all supported external connectors from the extcon.
>   */
>  extern int extcon_register_notifier(struct extcon_dev *edev, unsigned int id,
>  				    struct notifier_block *nb);
> @@ -287,6 +287,17 @@ extern void devm_extcon_unregister_notifier(struct device *dev,
>  				struct extcon_dev *edev, unsigned int id,
>  				struct notifier_block *nb);
>
> +extern int extcon_register_notifier_all(struct extcon_dev *edev,
> +				struct notifier_block *nb);
> +extern int extcon_unregister_notifier_all(struct extcon_dev *edev,
> +				struct notifier_block *nb);
> +extern int devm_extcon_register_notifier_all(struct device *dev,
> +				struct extcon_dev *edev,
> +				struct notifier_block *nb);
> +extern void devm_extcon_unregister_notifier_all(struct device *dev,
> +				struct extcon_dev *edev,
> +				struct notifier_block *nb);
> +
>  /*
>   * Following API get the extcon device from devicetree.
>   * This function use phandle of devicetree to get extcon device directly.
>

Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors

[Andy Shevchenko]

On Thu, Mar 30, 2017 at 11:39 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> The extcon core already provides the extcon_register_notifier() function
> in order to register the notifier block which is used to monitor
> the status change for the specific external connector such as EXTCON_USB,
> EXTCON_USB_HOST and so on. The extcon consumer uses the this function.
>
> The extcon consumer may need to monitor the all supported external
> connectors from the extcon device. In this case, The extcon consumer
> should have each notifier_block structure for each external connector.
>
> This patch adds the new extcon_register_notifier_all() function
> that extcon consumer is able to monitor the status change of all
> supported external connectors by using only one notifier_block structure.
>

> +/**
> + * extcon_register_notifier_all() - Register a notifier block to get the noti
> + *                             of the status change for all supported external
> + *                             connectors from extcon.
> + * @edev:      the extcon device that has the external connecotr.
> + * @nb:                a notifier block to be registered.
> + *
> + * Note that the second parameter given to the callback of nb (val) is
> + * the current state and third parameter is the edev pointer.
> + */

Have you checked how it looks like in resulting document file (man /
html / ...) ?
My concern is multi-line short function description.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] extcon: Use BIT() macro for the left-shift operation

[Chanwoo Choi]

On 2017년 03월 30일 17:59, Andy Shevchenko wrote:
> On Thu, Mar 30, 2017 at 11:39 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> This patch just uses the BIT() macro to make the code simple.
> 
>>         for (i = 0; i < edev->max_supported; i++) {
>>                 count += sprintf(buf + count, "%s=%d\n",
>>                                 extcon_info[edev->supported_cable[i]].name,
>> -                                !!(edev->state & (1 << i)));
>> +                                !!(edev->state & BIT(i)));
>>         }
> 
> While change is okay, the above code is fragile. There is a potential
> buffer overflow.

When extcon device is registered, extcon_dev_register() check a number of
supported external connectors. The maximum number of supported connectors
is 32. There is no buffer overflow.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors

[Chanwoo Choi]

On 2017년 03월 30일 18:04, Hans de Goede wrote:
> Hi Chanwoo,
> 
> On 30-03-17 10:39, Chanwoo Choi wrote:
>> The extcon core already provides the extcon_register_notifier() function
>> in order to register the notifier block which is used to monitor
>> the status change for the specific external connector such as EXTCON_USB,
>> EXTCON_USB_HOST and so on. The extcon consumer uses the this function.
>>
>> The extcon consumer may need to monitor the all supported external
>> connectors from the extcon device. In this case, The extcon consumer
>> should have each notifier_block structure for each external connector.
>>
>> This patch adds the new extcon_register_notifier_all() function
>> that extcon consumer is able to monitor the status change of all
>> supported external connectors by using only one notifier_block structure.
>>
>> - List of new added functions:
>> int extcon_register_notifier_all(struct extcon_dev *edev,
>>             struct notifier_block *nb);
>> int extcon_unregister_notifier_all(struct extcon_dev *edev,
>>             struct notifier_block *nb);
>> int devm_extcon_register_notifier_all(struct device *dev,
>>             struct extcon_dev *edev, struct notifier_block *nb);
>> void devm_extcon_unregister_notifier_all(struct device *dev,
>>             struct extcon_dev *edev, struct notifier_block *nb);
>>
>> Suggested-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  drivers/extcon/devres.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/extcon/extcon.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/extcon/extcon.h |  3 +++
>>  include/linux/extcon.h  | 21 ++++++++++++----
>>  4 files changed, 145 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/extcon/devres.c b/drivers/extcon/devres.c
>> index b40eb1805927..3fc8eea52f1d 100644
>> --- a/drivers/extcon/devres.c
>> +++ b/drivers/extcon/devres.c
>> @@ -50,6 +50,13 @@ static void devm_extcon_dev_notifier_unreg(struct device *dev, void *res)
>>      extcon_unregister_notifier(this->edev, this->id, this->nb);
>>  }
>>
>> +static void devm_extcon_dev_notifier_all_unreg(struct device *dev, void *res)
>> +{
>> +    struct extcon_dev_notifier_devres *this = res;
>> +
>> +    extcon_unregister_notifier_all(this->edev, this->nb);
>> +}
>> +
>>  /**
>>   * devm_extcon_dev_allocate - Allocate managed extcon device
>>   * @dev:        device owning the extcon device being created
>> @@ -214,3 +221,60 @@ void devm_extcon_unregister_notifier(struct device *dev,
>>                     devm_extcon_dev_match, edev));
>>  }
>>  EXPORT_SYMBOL(devm_extcon_unregister_notifier);
>> +
>> +/**
>> + * devm_extcon_register_notifier_all()
>> + *        - Resource-managed extcon_register_notifier_all()
>> + * @dev:    device to allocate extcon device
>> + * @edev:    the extcon device that has the external connecotr.
>> + * @nb:        a notifier block to be registered.
>> + *
>> + * This function manages automatically the notifier of extcon device using
>> + * device resource management and simplify the control of unregistering
>> + * the notifier of extcon device.
>> + *
>> + * Note that the second parameter given to the callback of nb (val) is
>> + * the current state and third parameter is the edev pointer.
>> + *
>> + * Returns 0 if success or negaive error number if failure.
>> + */
>> +int devm_extcon_register_notifier_all(struct device *dev, struct extcon_dev *edev,
>> +                struct notifier_block *nb)
>> +{
>> +    struct extcon_dev_notifier_devres *ptr;
>> +    int ret;
>> +
>> +    ptr = devres_alloc(devm_extcon_dev_notifier_all_unreg, sizeof(*ptr),
>> +                GFP_KERNEL);
>> +    if (!ptr)
>> +        return -ENOMEM;
>> +
>> +    ret = extcon_register_notifier_all(edev, nb);
>> +    if (ret) {
>> +        devres_free(ptr);
>> +        return ret;
>> +    }
>> +
>> +    ptr->edev = edev;
>> +    ptr->nb = nb;
>> +    devres_add(dev, ptr);
>> +
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL(devm_extcon_register_notifier_all);
>> +
>> +/**
>> + * devm_extcon_unregister_notifier_all()
>> +            - Resource-managed extcon_unregister_notifier_all()
>> + * @dev:    device to allocate extcon device
>> + * @edev:    the extcon device that has the external connecotr.
>> + * @nb:        a notifier block to be registered.
>> + */
>> +void devm_extcon_unregister_notifier_all(struct device *dev,
>> +                struct extcon_dev *edev,
>> +                struct notifier_block *nb)
>> +{
>> +    WARN_ON(devres_release(dev, devm_extcon_dev_notifier_all_unreg,
>> +                   devm_extcon_dev_match, edev));
>> +}
>> +EXPORT_SYMBOL(devm_extcon_unregister_notifier_all);
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index 193a3a673d10..4873fe8d5cd8 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -445,8 +445,19 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
>>      spin_lock_irqsave(&edev->lock, flags);
>>
>>      state = !!(edev->state & BIT(index));
>> +
>> +    /*
>> +     * Call functions in a raw notifier chain for the specific one
>> +     * external connector.
>> +     */
>>      raw_notifier_call_chain(&edev->nh[index], state, edev);
>>
>> +    /*
>> +     * Call functions in a raw notifier chain for the all supported
>> +     * external connectors.
>> +     */
>> +    raw_notifier_call_chain(&edev->nh_all, state, edev);
>> +
> 
> Passing state here is not useful since the receiver of
> the notifier call will not know for which cable the state is.

No, the receiver can use the 'state' value whether connector is attached
or detached. Also, the extcon have to keep the same value
for both extcon_register_notifier() and extcon_register_notfier_all().

> 
> Also this should be moved outside of the area of the
> function holding the edev->lock spinlock, since we don't
> pass state we do not need the lock and the called
> notifier function may very well want to call extcon_get_state
> to find out the state of one or more of the cables,
> which takes the lock.

The extcon uses the spinlock for the short delay.
Extcon should update the status of external connector
to the extcon consumer as soon as possible.

> 
> Otherwise looks good.
> 
> Regards,
> 
> Hans
> 
> 
> 
>>      /* This could be in interrupt handler */
>>      prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
>>      if (!prop_buf) {
>> @@ -951,6 +962,55 @@ int extcon_unregister_notifier(struct extcon_dev *edev, unsigned int id,
>>  }
>>  EXPORT_SYMBOL_GPL(extcon_unregister_notifier);
>>
>> +/**
>> + * extcon_register_notifier_all() - Register a notifier block to get the noti
>> + *                of the status change for all supported external
>> + *                connectors from extcon.
>> + * @edev:    the extcon device that has the external connecotr.
>> + * @nb:        a notifier block to be registered.
>> + *
>> + * Note that the second parameter given to the callback of nb (val) is
>> + * the current state and third parameter is the edev pointer.
>> + */
>> +int extcon_register_notifier_all(struct extcon_dev *edev,
>> +                struct notifier_block *nb)
>> +{
>> +    unsigned long flags;
>> +    int ret;
>> +
>> +    if (!edev || !nb)
>> +        return -EINVAL;
>> +
>> +    spin_lock_irqsave(&edev->lock, flags);
>> +    ret = raw_notifier_chain_register(&edev->nh_all, nb);
>> +    spin_unlock_irqrestore(&edev->lock, flags);
>> +
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(extcon_register_notifier_all);
>> +
>> +/**
>> + * extcon_unregister_notifier_all() - Unregister a notifiee from the extcon device.
>> + * @edev:    the extcon device that has the external connecotr.
>> + * @nb:        a notifier block to be registered.
>> + */
>> +int extcon_unregister_notifier_all(struct extcon_dev *edev,
>> +                struct notifier_block *nb)
>> +{
>> +    unsigned long flags;
>> +    int ret;
>> +
>> +    if (!edev || !nb)
>> +        return -EINVAL;
>> +
>> +    spin_lock_irqsave(&edev->lock, flags);
>> +    ret = raw_notifier_chain_unregister(&edev->nh_all, nb);
>> +    spin_unlock_irqrestore(&edev->lock, flags);
>> +
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(extcon_unregister_notifier_all);
>> +
>>  static struct attribute *extcon_attrs[] = {
>>      &dev_attr_state.attr,
>>      &dev_attr_name.attr,
>> @@ -1199,6 +1259,8 @@ int extcon_dev_register(struct extcon_dev *edev)
>>      for (index = 0; index < edev->max_supported; index++)
>>          RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]);
>>
>> +    RAW_INIT_NOTIFIER_HEAD(&edev->nh_all);
>> +
>>      dev_set_drvdata(&edev->dev, edev);
>>      edev->state = 0;
>>
>> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
>> index 993ddccafe11..dddddcfa0587 100644
>> --- a/drivers/extcon/extcon.h
>> +++ b/drivers/extcon/extcon.h
>> @@ -21,6 +21,8 @@
>>   * @dev:        Device of this extcon.
>>   * @state:        Attach/detach state of this extcon. Do not provide at
>>   *            register-time.
>> + * @nh_all:        Notifier for the state change events for all supported
>> + *            external connectors from this extcon.
>>   * @nh:            Notifier for the state change events from this extcon
>>   * @entry:        To support list of extcon devices so that users can
>>   *            search for extcon devices based on the extcon name.
>> @@ -43,6 +45,7 @@ struct extcon_dev {
>>
>>      /* Internal data. Please do not set. */
>>      struct device dev;
>> +    struct raw_notifier_head nh_all;
>>      struct raw_notifier_head *nh;
>>      struct list_head entry;
>>      int max_supported;
>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
>> index 3929d2e8a3c7..61e8d8c591a4 100644
>> --- a/include/linux/extcon.h
>> +++ b/include/linux/extcon.h
>> @@ -270,11 +270,11 @@ extern int extcon_set_property_capability(struct extcon_dev *edev,
>>                  unsigned int id, unsigned int prop);
>>
>>  /*
>> - * Following APIs are to monitor every action of a notifier.
>> - * Registrar gets notified for every external port of a connection device.
>> - * Probably this could be used to debug an action of notifier; however,
>> - * we do not recommend to use this for normal 'notifiee' device drivers who
>> - * want to be notified by a specific external port of the notifier.
>> + * Following APIs are to monitor the status change of the external connectors.
>> + * extcon_register_notifier(*edev, id, *nb) : Register a notifier
>> + *            for specific external connector from the extcon.
>> + * extcon_register_notifier_all(*edev, *nb) : Register a notifier
>> + *            for all supported external connectors from the extcon.
>>   */
>>  extern int extcon_register_notifier(struct extcon_dev *edev, unsigned int id,
>>                      struct notifier_block *nb);
>> @@ -287,6 +287,17 @@ extern void devm_extcon_unregister_notifier(struct device *dev,
>>                  struct extcon_dev *edev, unsigned int id,
>>                  struct notifier_block *nb);
>>
>> +extern int extcon_register_notifier_all(struct extcon_dev *edev,
>> +                struct notifier_block *nb);
>> +extern int extcon_unregister_notifier_all(struct extcon_dev *edev,
>> +                struct notifier_block *nb);
>> +extern int devm_extcon_register_notifier_all(struct device *dev,
>> +                struct extcon_dev *edev,
>> +                struct notifier_block *nb);
>> +extern void devm_extcon_unregister_notifier_all(struct device *dev,
>> +                struct extcon_dev *edev,
>> +                struct notifier_block *nb);
>> +
>>  /*
>>   * Following API get the extcon device from devicetree.
>>   * This function use phandle of devicetree to get extcon device directly.
>>
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors

[Chanwoo Choi]

On 2017년 03월 30일 18:05, Andy Shevchenko wrote:
> On Thu, Mar 30, 2017 at 11:39 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> The extcon core already provides the extcon_register_notifier() function
>> in order to register the notifier block which is used to monitor
>> the status change for the specific external connector such as EXTCON_USB,
>> EXTCON_USB_HOST and so on. The extcon consumer uses the this function.
>>
>> The extcon consumer may need to monitor the all supported external
>> connectors from the extcon device. In this case, The extcon consumer
>> should have each notifier_block structure for each external connector.
>>
>> This patch adds the new extcon_register_notifier_all() function
>> that extcon consumer is able to monitor the status change of all
>> supported external connectors by using only one notifier_block structure.
>>
> 
>> +/**
>> + * extcon_register_notifier_all() - Register a notifier block to get the noti
>> + *                             of the status change for all supported external
>> + *                             connectors from extcon.
>> + * @edev:      the extcon device that has the external connecotr.
>> + * @nb:                a notifier block to be registered.
>> + *
>> + * Note that the second parameter given to the callback of nb (val) is
>> + * the current state and third parameter is the edev pointer.
>> + */
> 
> Have you checked how it looks like in resulting document file (man /
> html / ...) ?
> My concern is multi-line short function description.
> 

Actually, I didn't consider the document file as you mentioned.
Do you think need to add more detailed description?

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 1/2] extcon: Use BIT() macro for the left-shift operation

[Andy Shevchenko]

On Thu, Mar 30, 2017 at 12:15 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> On 2017년 03월 30일 17:59, Andy Shevchenko wrote:
>> On Thu, Mar 30, 2017 at 11:39 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>> This patch just uses the BIT() macro to make the code simple.
>>
>>>         for (i = 0; i < edev->max_supported; i++) {
>>>                 count += sprintf(buf + count, "%s=%d\n",
>>>                                 extcon_info[edev->supported_cable[i]].name,
>>> -                                !!(edev->state & (1 << i)));
>>> +                                !!(edev->state & BIT(i)));
>>>         }
>>
>> While change is okay, the above code is fragile. There is a potential
>> buffer overflow.
>
> When extcon device is registered, extcon_dev_register() check a number of
> supported external connectors. The maximum number of supported connectors
> is 32. There is no buffer overflow.

Is there any limit for name? No, there is not (const char *name).
Though for now it is quite unlikely to have the issue.

Feel free to proceed despite my comment.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors

[Andy Shevchenko]

On Thu, Mar 30, 2017 at 12:24 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> On 2017년 03월 30일 18:05, Andy Shevchenko wrote:
>> On Thu, Mar 30, 2017 at 11:39 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>> The extcon core already provides the extcon_register_notifier() function
>>> in order to register the notifier block which is used to monitor
>>> the status change for the specific external connector such as EXTCON_USB,
>>> EXTCON_USB_HOST and so on. The extcon consumer uses the this function.
>>>
>>> The extcon consumer may need to monitor the all supported external
>>> connectors from the extcon device. In this case, The extcon consumer
>>> should have each notifier_block structure for each external connector.
>>>
>>> This patch adds the new extcon_register_notifier_all() function
>>> that extcon consumer is able to monitor the status change of all
>>> supported external connectors by using only one notifier_block structure.
>>>
>>
>>> +/**
>>> + * extcon_register_notifier_all() - Register a notifier block to get the noti
>>> + *                             of the status change for all supported external
>>> + *                             connectors from extcon.
>>> + * @edev:      the extcon device that has the external connecotr.
>>> + * @nb:                a notifier block to be registered.
>>> + *
>>> + * Note that the second parameter given to the callback of nb (val) is
>>> + * the current state and third parameter is the edev pointer.
>>> + */
>>
>> Have you checked how it looks like in resulting document file (man /
>> html / ...) ?
>> My concern is multi-line short function description.
>>
>
> Actually, I didn't consider the document file as you mentioned.
> Do you think need to add more detailed description?

What I meant is the quite long line in *short* description (fist line
heading with function name).
See Documentation/doc-guide/kernel-doc.rst for the detailed howto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] extcon: Use BIT() macro for the left-shift operation

[Chanwoo Choi]

On 2017년 03월 30일 19:38, Andy Shevchenko wrote:
> On Thu, Mar 30, 2017 at 12:15 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> On 2017년 03월 30일 17:59, Andy Shevchenko wrote:
>>> On Thu, Mar 30, 2017 at 11:39 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>> This patch just uses the BIT() macro to make the code simple.
>>>
>>>>         for (i = 0; i < edev->max_supported; i++) {
>>>>                 count += sprintf(buf + count, "%s=%d\n",
>>>>                                 extcon_info[edev->supported_cable[i]].name,
>>>> -                                !!(edev->state & (1 << i)));
>>>> +                                !!(edev->state & BIT(i)));
>>>>         }
>>>
>>> While change is okay, the above code is fragile. There is a potential
>>> buffer overflow.
>>
>> When extcon device is registered, extcon_dev_register() check a number of
>> supported external connectors. The maximum number of supported connectors
>> is 32. There is no buffer overflow.
> 
> Is there any limit for name? No, there is not (const char *name).
> Though for now it is quite unlikely to have the issue.

If there is problem, I want to fix it. 
I hope your more comment because I don't understand what you point out.
Did you mention the length of connector name? or buf?

> 
> Feel free to proceed despite my comment.
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors

[Chanwoo Choi]

On 2017년 03월 30일 19:42, Andy Shevchenko wrote:
> On Thu, Mar 30, 2017 at 12:24 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> On 2017년 03월 30일 18:05, Andy Shevchenko wrote:
>>> On Thu, Mar 30, 2017 at 11:39 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>> The extcon core already provides the extcon_register_notifier() function
>>>> in order to register the notifier block which is used to monitor
>>>> the status change for the specific external connector such as EXTCON_USB,
>>>> EXTCON_USB_HOST and so on. The extcon consumer uses the this function.
>>>>
>>>> The extcon consumer may need to monitor the all supported external
>>>> connectors from the extcon device. In this case, The extcon consumer
>>>> should have each notifier_block structure for each external connector.
>>>>
>>>> This patch adds the new extcon_register_notifier_all() function
>>>> that extcon consumer is able to monitor the status change of all
>>>> supported external connectors by using only one notifier_block structure.
>>>>
>>>
>>>> +/**
>>>> + * extcon_register_notifier_all() - Register a notifier block to get the noti
>>>> + *                             of the status change for all supported external
>>>> + *                             connectors from extcon.
>>>> + * @edev:      the extcon device that has the external connecotr.
>>>> + * @nb:                a notifier block to be registered.
>>>> + *
>>>> + * Note that the second parameter given to the callback of nb (val) is
>>>> + * the current state and third parameter is the edev pointer.
>>>> + */
>>>
>>> Have you checked how it looks like in resulting document file (man /
>>> html / ...) ?
>>> My concern is multi-line short function description.
>>>
>>
>> Actually, I didn't consider the document file as you mentioned.
>> Do you think need to add more detailed description?
> 
> What I meant is the quite long line in *short* description (fist line
> heading with function name).
> See Documentation/doc-guide/kernel-doc.rst for the detailed howto.
> 

If you metioned the following guide on Line 111
in the Documentation/doc-guide/kernel-doc.rst,
OK. I'll modify it.

Example kernel-doc function comment::

  /**
   * foobar() - Brief description of foobar.
   * @arg: Description of argument of foobar.
   *
   * Longer description of foobar.
   *
   * Return: Description of return value of foobar.
   */
  int foobar(int arg)

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors

[Andy Shevchenko]

On Thu, Mar 30, 2017 at 1:56 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> On 2017년 03월 30일 19:42, Andy Shevchenko wrote:
>> On Thu, Mar 30, 2017 at 12:24 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>> On 2017년 03월 30일 18:05, Andy Shevchenko wrote:
>>>> On Thu, Mar 30, 2017 at 11:39 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:

>>>>> +/**
>>>>> + * extcon_register_notifier_all() - Register a notifier block to get the noti
>>>>> + *                             of the status change for all supported external
>>>>> + *                             connectors from extcon.
>>>>> + * @edev:      the extcon device that has the external connecotr.
>>>>> + * @nb:                a notifier block to be registered.
>>>>> + *
>>>>> + * Note that the second parameter given to the callback of nb (val) is
>>>>> + * the current state and third parameter is the edev pointer.
>>>>> + */

> If you metioned the following guide on Line 111
> in the Documentation/doc-guide/kernel-doc.rst,

Yes, that's one.

> OK. I'll modify it.

Thanks.

>
> Example kernel-doc function comment::
>
>   /**
>    * foobar() - Brief description of foobar.
>    * @arg: Description of argument of foobar.
>    *
>    * Longer description of foobar.
>    *
>    * Return: Description of return value of foobar.
>    */
>   int foobar(int arg)
>
> --
> Best Regards,
> Chanwoo Choi
> Samsung Electronics



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] extcon: Use BIT() macro for the left-shift operation

[Andy Shevchenko]

On Thu, Mar 30, 2017 at 1:52 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> On 2017년 03월 30일 19:38, Andy Shevchenko wrote:
>> On Thu, Mar 30, 2017 at 12:15 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>> On 2017년 03월 30일 17:59, Andy Shevchenko wrote:

>>>>>         for (i = 0; i < edev->max_supported; i++) {
>>>>>                 count += sprintf(buf + count, "%s=%d\n",
>>>>>                                 extcon_info[edev->supported_cable[i]].name,
>>>>> -                                !!(edev->state & (1 << i)));
>>>>> +                                !!(edev->state & BIT(i)));
>>>>>         }
>>>>
>>>> While change is okay, the above code is fragile. There is a potential
>>>> buffer overflow.
>>>
>>> When extcon device is registered, extcon_dev_register() check a number of
>>> supported external connectors. The maximum number of supported connectors
>>> is 32. There is no buffer overflow.
>>
>> Is there any limit for name? No, there is not (const char *name).
>> Though for now it is quite unlikely to have the issue.
>
> If there is problem, I want to fix it.

For now it seems no problem.

> I hope your more comment because I don't understand what you point out.
> Did you mention the length of connector name? or buf?

name as a field of sturct __extcon_info.
Means, *potentially* even one record would be enough to break the boundaries.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors

[Hans de Goede]

Hi,

On 30-03-17 11:20, Chanwoo Choi wrote:
> On 2017년 03월 30일 18:04, Hans de Goede wrote:
>> Hi Chanwoo,
>>
>> On 30-03-17 10:39, Chanwoo Choi wrote:
>>> The extcon core already provides the extcon_register_notifier() function
>>> in order to register the notifier block which is used to monitor
>>> the status change for the specific external connector such as EXTCON_USB,
>>> EXTCON_USB_HOST and so on. The extcon consumer uses the this function.
>>>
>>> The extcon consumer may need to monitor the all supported external
>>> connectors from the extcon device. In this case, The extcon consumer
>>> should have each notifier_block structure for each external connector.
>>>
>>> This patch adds the new extcon_register_notifier_all() function
>>> that extcon consumer is able to monitor the status change of all
>>> supported external connectors by using only one notifier_block structure.
>>>
>>> - List of new added functions:
>>> int extcon_register_notifier_all(struct extcon_dev *edev,
>>>             struct notifier_block *nb);
>>> int extcon_unregister_notifier_all(struct extcon_dev *edev,
>>>             struct notifier_block *nb);
>>> int devm_extcon_register_notifier_all(struct device *dev,
>>>             struct extcon_dev *edev, struct notifier_block *nb);
>>> void devm_extcon_unregister_notifier_all(struct device *dev,
>>>             struct extcon_dev *edev, struct notifier_block *nb);
>>>
>>> Suggested-by: Hans de Goede <hdegoede@redhat.com>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>>  drivers/extcon/devres.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  drivers/extcon/extcon.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  drivers/extcon/extcon.h |  3 +++
>>>  include/linux/extcon.h  | 21 ++++++++++++----
>>>  4 files changed, 145 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/extcon/devres.c b/drivers/extcon/devres.c
>>> index b40eb1805927..3fc8eea52f1d 100644
>>> --- a/drivers/extcon/devres.c
>>> +++ b/drivers/extcon/devres.c
>>> @@ -50,6 +50,13 @@ static void devm_extcon_dev_notifier_unreg(struct device *dev, void *res)
>>>      extcon_unregister_notifier(this->edev, this->id, this->nb);
>>>  }
>>>
>>> +static void devm_extcon_dev_notifier_all_unreg(struct device *dev, void *res)
>>> +{
>>> +    struct extcon_dev_notifier_devres *this = res;
>>> +
>>> +    extcon_unregister_notifier_all(this->edev, this->nb);
>>> +}
>>> +
>>>  /**
>>>   * devm_extcon_dev_allocate - Allocate managed extcon device
>>>   * @dev:        device owning the extcon device being created
>>> @@ -214,3 +221,60 @@ void devm_extcon_unregister_notifier(struct device *dev,
>>>                     devm_extcon_dev_match, edev));
>>>  }
>>>  EXPORT_SYMBOL(devm_extcon_unregister_notifier);
>>> +
>>> +/**
>>> + * devm_extcon_register_notifier_all()
>>> + *        - Resource-managed extcon_register_notifier_all()
>>> + * @dev:    device to allocate extcon device
>>> + * @edev:    the extcon device that has the external connecotr.
>>> + * @nb:        a notifier block to be registered.
>>> + *
>>> + * This function manages automatically the notifier of extcon device using
>>> + * device resource management and simplify the control of unregistering
>>> + * the notifier of extcon device.
>>> + *
>>> + * Note that the second parameter given to the callback of nb (val) is
>>> + * the current state and third parameter is the edev pointer.
>>> + *
>>> + * Returns 0 if success or negaive error number if failure.
>>> + */
>>> +int devm_extcon_register_notifier_all(struct device *dev, struct extcon_dev *edev,
>>> +                struct notifier_block *nb)
>>> +{
>>> +    struct extcon_dev_notifier_devres *ptr;
>>> +    int ret;
>>> +
>>> +    ptr = devres_alloc(devm_extcon_dev_notifier_all_unreg, sizeof(*ptr),
>>> +                GFP_KERNEL);
>>> +    if (!ptr)
>>> +        return -ENOMEM;
>>> +
>>> +    ret = extcon_register_notifier_all(edev, nb);
>>> +    if (ret) {
>>> +        devres_free(ptr);
>>> +        return ret;
>>> +    }
>>> +
>>> +    ptr->edev = edev;
>>> +    ptr->nb = nb;
>>> +    devres_add(dev, ptr);
>>> +
>>> +    return 0;
>>> +}
>>> +EXPORT_SYMBOL(devm_extcon_register_notifier_all);
>>> +
>>> +/**
>>> + * devm_extcon_unregister_notifier_all()
>>> +            - Resource-managed extcon_unregister_notifier_all()
>>> + * @dev:    device to allocate extcon device
>>> + * @edev:    the extcon device that has the external connecotr.
>>> + * @nb:        a notifier block to be registered.
>>> + */
>>> +void devm_extcon_unregister_notifier_all(struct device *dev,
>>> +                struct extcon_dev *edev,
>>> +                struct notifier_block *nb)
>>> +{
>>> +    WARN_ON(devres_release(dev, devm_extcon_dev_notifier_all_unreg,
>>> +                   devm_extcon_dev_match, edev));
>>> +}
>>> +EXPORT_SYMBOL(devm_extcon_unregister_notifier_all);
>>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>>> index 193a3a673d10..4873fe8d5cd8 100644
>>> --- a/drivers/extcon/extcon.c
>>> +++ b/drivers/extcon/extcon.c
>>> @@ -445,8 +445,19 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
>>>      spin_lock_irqsave(&edev->lock, flags);
>>>
>>>      state = !!(edev->state & BIT(index));
>>> +
>>> +    /*
>>> +     * Call functions in a raw notifier chain for the specific one
>>> +     * external connector.
>>> +     */
>>>      raw_notifier_call_chain(&edev->nh[index], state, edev);
>>>
>>> +    /*
>>> +     * Call functions in a raw notifier chain for the all supported
>>> +     * external connectors.
>>> +     */
>>> +    raw_notifier_call_chain(&edev->nh_all, state, edev);
>>> +
>>
>> Passing state here is not useful since the receiver of
>> the notifier call will not know for which cable the state is.
>
> No, the receiver can use the 'state' value whether connector is attached
> or detached. Also, the extcon have to keep the same value
> for both extcon_register_notifier() and extcon_register_notfier_all().

Ok.

>> Also this should be moved outside of the area of the
>> function holding the edev->lock spinlock, since we don't
>> pass state we do not need the lock and the called
>> notifier function may very well want to call extcon_get_state
>> to find out the state of one or more of the cables,
>> which takes the lock.
>
> The extcon uses the spinlock for the short delay.
> Extcon should update the status of external connector
> to the extcon consumer as soon as possible.

Right, what I'm suggestion actually also applies to the
current cable notification, what I'm suggesting is to
move the notification like this:

--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -448,8 +448,6 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
         spin_lock_irqsave(&edev->lock, flags);

         state = !!(edev->state & BIT(index));
-   raw_notifier_call_chain(&edev->nh[index], state, edev);
-   raw_notifier_call_chain(&edev->nh_all, 0, edev);

         /* This could be in interrupt handler */
         prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
@@ -482,6 +480,10 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)

         /* Unlock early before uevent */
         spin_unlock_irqrestore(&edev->lock, flags);
+
+ raw_notifier_call_chain(&edev->nh[index], state, edev);
+ raw_notifier_call_chain(&edev->nh_all, 0, edev);
+
         kobject_uevent_env(&edev->dev.kobj, KOBJ_CHANGE, envp);
         free_page((unsigned long)prop_buf);


So that the nb.notifier_call function can call extcon_get_state
to find out what cable is plugged in without deadlocking because
extcon_get_state does spin_lock_irqsave(&edev->lock, flags) too.

This is esp. important for the edev->nh_all notifier chain
since when used in charger drivers the callback will want to call
extcon_get_state for all of: EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_DCP,
EXTCON_CHG_USB_CDP, etc. to find out how much current it can draw
from the port.

AFAICT tell there is no race in moving this outside of the locked
section of extcon_sync() and it avoids potential deadlocks in the
nb.notifier_call function.

Regards,

Hans


>>>      /* This could be in interrupt handler */
>>>      prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
>>>      if (!prop_buf) {
>>> @@ -951,6 +962,55 @@ int extcon_unregister_notifier(struct extcon_dev *edev, unsigned int id,
>>>  }
>>>  EXPORT_SYMBOL_GPL(extcon_unregister_notifier);
>>>
>>> +/**
>>> + * extcon_register_notifier_all() - Register a notifier block to get the noti
>>> + *                of the status change for all supported external
>>> + *                connectors from extcon.
>>> + * @edev:    the extcon device that has the external connecotr.
>>> + * @nb:        a notifier block to be registered.
>>> + *
>>> + * Note that the second parameter given to the callback of nb (val) is
>>> + * the current state and third parameter is the edev pointer.
>>> + */
>>> +int extcon_register_notifier_all(struct extcon_dev *edev,
>>> +                struct notifier_block *nb)
>>> +{
>>> +    unsigned long flags;
>>> +    int ret;
>>> +
>>> +    if (!edev || !nb)
>>> +        return -EINVAL;
>>> +
>>> +    spin_lock_irqsave(&edev->lock, flags);
>>> +    ret = raw_notifier_chain_register(&edev->nh_all, nb);
>>> +    spin_unlock_irqrestore(&edev->lock, flags);
>>> +
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(extcon_register_notifier_all);
>>> +
>>> +/**
>>> + * extcon_unregister_notifier_all() - Unregister a notifiee from the extcon device.
>>> + * @edev:    the extcon device that has the external connecotr.
>>> + * @nb:        a notifier block to be registered.
>>> + */
>>> +int extcon_unregister_notifier_all(struct extcon_dev *edev,
>>> +                struct notifier_block *nb)
>>> +{
>>> +    unsigned long flags;
>>> +    int ret;
>>> +
>>> +    if (!edev || !nb)
>>> +        return -EINVAL;
>>> +
>>> +    spin_lock_irqsave(&edev->lock, flags);
>>> +    ret = raw_notifier_chain_unregister(&edev->nh_all, nb);
>>> +    spin_unlock_irqrestore(&edev->lock, flags);
>>> +
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(extcon_unregister_notifier_all);
>>> +
>>>  static struct attribute *extcon_attrs[] = {
>>>      &dev_attr_state.attr,
>>>      &dev_attr_name.attr,
>>> @@ -1199,6 +1259,8 @@ int extcon_dev_register(struct extcon_dev *edev)
>>>      for (index = 0; index < edev->max_supported; index++)
>>>          RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]);
>>>
>>> +    RAW_INIT_NOTIFIER_HEAD(&edev->nh_all);
>>> +
>>>      dev_set_drvdata(&edev->dev, edev);
>>>      edev->state = 0;
>>>
>>> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
>>> index 993ddccafe11..dddddcfa0587 100644
>>> --- a/drivers/extcon/extcon.h
>>> +++ b/drivers/extcon/extcon.h
>>> @@ -21,6 +21,8 @@
>>>   * @dev:        Device of this extcon.
>>>   * @state:        Attach/detach state of this extcon. Do not provide at
>>>   *            register-time.
>>> + * @nh_all:        Notifier for the state change events for all supported
>>> + *            external connectors from this extcon.
>>>   * @nh:            Notifier for the state change events from this extcon
>>>   * @entry:        To support list of extcon devices so that users can
>>>   *            search for extcon devices based on the extcon name.
>>> @@ -43,6 +45,7 @@ struct extcon_dev {
>>>
>>>      /* Internal data. Please do not set. */
>>>      struct device dev;
>>> +    struct raw_notifier_head nh_all;
>>>      struct raw_notifier_head *nh;
>>>      struct list_head entry;
>>>      int max_supported;
>>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
>>> index 3929d2e8a3c7..61e8d8c591a4 100644
>>> --- a/include/linux/extcon.h
>>> +++ b/include/linux/extcon.h
>>> @@ -270,11 +270,11 @@ extern int extcon_set_property_capability(struct extcon_dev *edev,
>>>                  unsigned int id, unsigned int prop);
>>>
>>>  /*
>>> - * Following APIs are to monitor every action of a notifier.
>>> - * Registrar gets notified for every external port of a connection device.
>>> - * Probably this could be used to debug an action of notifier; however,
>>> - * we do not recommend to use this for normal 'notifiee' device drivers who
>>> - * want to be notified by a specific external port of the notifier.
>>> + * Following APIs are to monitor the status change of the external connectors.
>>> + * extcon_register_notifier(*edev, id, *nb) : Register a notifier
>>> + *            for specific external connector from the extcon.
>>> + * extcon_register_notifier_all(*edev, *nb) : Register a notifier
>>> + *            for all supported external connectors from the extcon.
>>>   */
>>>  extern int extcon_register_notifier(struct extcon_dev *edev, unsigned int id,
>>>                      struct notifier_block *nb);
>>> @@ -287,6 +287,17 @@ extern void devm_extcon_unregister_notifier(struct device *dev,
>>>                  struct extcon_dev *edev, unsigned int id,
>>>                  struct notifier_block *nb);
>>>
>>> +extern int extcon_register_notifier_all(struct extcon_dev *edev,
>>> +                struct notifier_block *nb);
>>> +extern int extcon_unregister_notifier_all(struct extcon_dev *edev,
>>> +                struct notifier_block *nb);
>>> +extern int devm_extcon_register_notifier_all(struct device *dev,
>>> +                struct extcon_dev *edev,
>>> +                struct notifier_block *nb);
>>> +extern void devm_extcon_unregister_notifier_all(struct device *dev,
>>> +                struct extcon_dev *edev,
>>> +                struct notifier_block *nb);
>>> +
>>>  /*
>>>   * Following API get the extcon device from devicetree.
>>>   * This function use phandle of devicetree to get extcon device directly.
>>>
>>
>>
>>
>
>

Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors

[Chanwoo Choi]

Hi,

On 2017년 03월 30일 23:58, Hans de Goede wrote:
> Hi,
> 
> On 30-03-17 11:20, Chanwoo Choi wrote:
>> On 2017년 03월 30일 18:04, Hans de Goede wrote:
>>> Hi Chanwoo,
>>>
>>> On 30-03-17 10:39, Chanwoo Choi wrote:
>>>> The extcon core already provides the extcon_register_notifier() function
>>>> in order to register the notifier block which is used to monitor
>>>> the status change for the specific external connector such as EXTCON_USB,
>>>> EXTCON_USB_HOST and so on. The extcon consumer uses the this function.
>>>>
>>>> The extcon consumer may need to monitor the all supported external
>>>> connectors from the extcon device. In this case, The extcon consumer
>>>> should have each notifier_block structure for each external connector.
>>>>
>>>> This patch adds the new extcon_register_notifier_all() function
>>>> that extcon consumer is able to monitor the status change of all
>>>> supported external connectors by using only one notifier_block structure.
>>>>
>>>> - List of new added functions:
>>>> int extcon_register_notifier_all(struct extcon_dev *edev,
>>>>             struct notifier_block *nb);
>>>> int extcon_unregister_notifier_all(struct extcon_dev *edev,
>>>>             struct notifier_block *nb);
>>>> int devm_extcon_register_notifier_all(struct device *dev,
>>>>             struct extcon_dev *edev, struct notifier_block *nb);
>>>> void devm_extcon_unregister_notifier_all(struct device *dev,
>>>>             struct extcon_dev *edev, struct notifier_block *nb);
>>>>
>>>> Suggested-by: Hans de Goede <hdegoede@redhat.com>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> ---
>>>>  drivers/extcon/devres.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  drivers/extcon/extcon.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>  drivers/extcon/extcon.h |  3 +++
>>>>  include/linux/extcon.h  | 21 ++++++++++++----
>>>>  4 files changed, 145 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/extcon/devres.c b/drivers/extcon/devres.c
>>>> index b40eb1805927..3fc8eea52f1d 100644
>>>> --- a/drivers/extcon/devres.c
>>>> +++ b/drivers/extcon/devres.c
>>>> @@ -50,6 +50,13 @@ static void devm_extcon_dev_notifier_unreg(struct device *dev, void *res)
>>>>      extcon_unregister_notifier(this->edev, this->id, this->nb);
>>>>  }
>>>>
>>>> +static void devm_extcon_dev_notifier_all_unreg(struct device *dev, void *res)
>>>> +{
>>>> +    struct extcon_dev_notifier_devres *this = res;
>>>> +
>>>> +    extcon_unregister_notifier_all(this->edev, this->nb);
>>>> +}
>>>> +
>>>>  /**
>>>>   * devm_extcon_dev_allocate - Allocate managed extcon device
>>>>   * @dev:        device owning the extcon device being created
>>>> @@ -214,3 +221,60 @@ void devm_extcon_unregister_notifier(struct device *dev,
>>>>                     devm_extcon_dev_match, edev));
>>>>  }
>>>>  EXPORT_SYMBOL(devm_extcon_unregister_notifier);
>>>> +
>>>> +/**
>>>> + * devm_extcon_register_notifier_all()
>>>> + *        - Resource-managed extcon_register_notifier_all()
>>>> + * @dev:    device to allocate extcon device
>>>> + * @edev:    the extcon device that has the external connecotr.
>>>> + * @nb:        a notifier block to be registered.
>>>> + *
>>>> + * This function manages automatically the notifier of extcon device using
>>>> + * device resource management and simplify the control of unregistering
>>>> + * the notifier of extcon device.
>>>> + *
>>>> + * Note that the second parameter given to the callback of nb (val) is
>>>> + * the current state and third parameter is the edev pointer.
>>>> + *
>>>> + * Returns 0 if success or negaive error number if failure.
>>>> + */
>>>> +int devm_extcon_register_notifier_all(struct device *dev, struct extcon_dev *edev,
>>>> +                struct notifier_block *nb)
>>>> +{
>>>> +    struct extcon_dev_notifier_devres *ptr;
>>>> +    int ret;
>>>> +
>>>> +    ptr = devres_alloc(devm_extcon_dev_notifier_all_unreg, sizeof(*ptr),
>>>> +                GFP_KERNEL);
>>>> +    if (!ptr)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    ret = extcon_register_notifier_all(edev, nb);
>>>> +    if (ret) {
>>>> +        devres_free(ptr);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    ptr->edev = edev;
>>>> +    ptr->nb = nb;
>>>> +    devres_add(dev, ptr);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(devm_extcon_register_notifier_all);
>>>> +
>>>> +/**
>>>> + * devm_extcon_unregister_notifier_all()
>>>> +            - Resource-managed extcon_unregister_notifier_all()
>>>> + * @dev:    device to allocate extcon device
>>>> + * @edev:    the extcon device that has the external connecotr.
>>>> + * @nb:        a notifier block to be registered.
>>>> + */
>>>> +void devm_extcon_unregister_notifier_all(struct device *dev,
>>>> +                struct extcon_dev *edev,
>>>> +                struct notifier_block *nb)
>>>> +{
>>>> +    WARN_ON(devres_release(dev, devm_extcon_dev_notifier_all_unreg,
>>>> +                   devm_extcon_dev_match, edev));
>>>> +}
>>>> +EXPORT_SYMBOL(devm_extcon_unregister_notifier_all);
>>>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>>>> index 193a3a673d10..4873fe8d5cd8 100644
>>>> --- a/drivers/extcon/extcon.c
>>>> +++ b/drivers/extcon/extcon.c
>>>> @@ -445,8 +445,19 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
>>>>      spin_lock_irqsave(&edev->lock, flags);
>>>>
>>>>      state = !!(edev->state & BIT(index));
>>>> +
>>>> +    /*
>>>> +     * Call functions in a raw notifier chain for the specific one
>>>> +     * external connector.
>>>> +     */
>>>>      raw_notifier_call_chain(&edev->nh[index], state, edev);
>>>>
>>>> +    /*
>>>> +     * Call functions in a raw notifier chain for the all supported
>>>> +     * external connectors.
>>>> +     */
>>>> +    raw_notifier_call_chain(&edev->nh_all, state, edev);
>>>> +
>>>
>>> Passing state here is not useful since the receiver of
>>> the notifier call will not know for which cable the state is.
>>
>> No, the receiver can use the 'state' value whether connector is attached
>> or detached. Also, the extcon have to keep the same value
>> for both extcon_register_notifier() and extcon_register_notfier_all().
> 
> Ok.
> 
>>> Also this should be moved outside of the area of the
>>> function holding the edev->lock spinlock, since we don't
>>> pass state we do not need the lock and the called
>>> notifier function may very well want to call extcon_get_state
>>> to find out the state of one or more of the cables,
>>> which takes the lock.
>>
>> The extcon uses the spinlock for the short delay.
>> Extcon should update the status of external connector
>> to the extcon consumer as soon as possible.
> 
> Right, what I'm suggestion actually also applies to the
> current cable notification, what I'm suggesting is to
> move the notification like this:
> 
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -448,8 +448,6 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
>         spin_lock_irqsave(&edev->lock, flags);
> 
>         state = !!(edev->state & BIT(index));
> -   raw_notifier_call_chain(&edev->nh[index], state, edev);
> -   raw_notifier_call_chain(&edev->nh_all, 0, edev);
> 
>         /* This could be in interrupt handler */
>         prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
> @@ -482,6 +480,10 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
> 
>         /* Unlock early before uevent */
>         spin_unlock_irqrestore(&edev->lock, flags);
> +
> + raw_notifier_call_chain(&edev->nh[index], state, edev);
> + raw_notifier_call_chain(&edev->nh_all, 0, edev);
> +
>         kobject_uevent_env(&edev->dev.kobj, KOBJ_CHANGE, envp);
>         free_page((unsigned long)prop_buf);
> 
> 
> So that the nb.notifier_call function can call extcon_get_state
> to find out what cable is plugged in without deadlocking because
> extcon_get_state does spin_lock_irqsave(&edev->lock, flags) too.
> 
> This is esp. important for the edev->nh_all notifier chain
> since when used in charger drivers the callback will want to call
> extcon_get_state for all of: EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_DCP,
> EXTCON_CHG_USB_CDP, etc. to find out how much current it can draw
> from the port.
> 
> AFAICT tell there is no race in moving this outside of the locked
> section of extcon_sync() and it avoids potential deadlocks in the
> nb.notifier_call function.


Actually, I knew that if the extcon consumer driver uses
the extcon_get_state() in the callback function, there is a deadlock
between extcon_sync() and extcon_get_state(). So, all extcon consumer
uses the workqueue when receiving the notfication from extcon.

But, extcon_sync() have to call the number of callback functions
in the notifier chanin. If one specific extcon consumer spend many
time in the own callback function, other extcon consumer might receive
the notification late. So, I think that each extcon consumer
better to use the work in the their callback function.

As I already said, I think that extcon focus on sending the notification
to all of extcon consumers.

> 
> Regards,
> 
> Hans
> 
> 
>>>>      /* This could be in interrupt handler */
>>>>      prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
>>>>      if (!prop_buf) {
>>>> @@ -951,6 +962,55 @@ int extcon_unregister_notifier(struct extcon_dev *edev, unsigned int id,
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(extcon_unregister_notifier);
>>>>
>>>> +/**
>>>> + * extcon_register_notifier_all() - Register a notifier block to get the noti
>>>> + *                of the status change for all supported external
>>>> + *                connectors from extcon.
>>>> + * @edev:    the extcon device that has the external connecotr.
>>>> + * @nb:        a notifier block to be registered.
>>>> + *
>>>> + * Note that the second parameter given to the callback of nb (val) is
>>>> + * the current state and third parameter is the edev pointer.
>>>> + */
>>>> +int extcon_register_notifier_all(struct extcon_dev *edev,
>>>> +                struct notifier_block *nb)
>>>> +{
>>>> +    unsigned long flags;
>>>> +    int ret;
>>>> +
>>>> +    if (!edev || !nb)
>>>> +        return -EINVAL;
>>>> +
>>>> +    spin_lock_irqsave(&edev->lock, flags);
>>>> +    ret = raw_notifier_chain_register(&edev->nh_all, nb);
>>>> +    spin_unlock_irqrestore(&edev->lock, flags);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(extcon_register_notifier_all);
>>>> +
>>>> +/**
>>>> + * extcon_unregister_notifier_all() - Unregister a notifiee from the extcon device.
>>>> + * @edev:    the extcon device that has the external connecotr.
>>>> + * @nb:        a notifier block to be registered.
>>>> + */
>>>> +int extcon_unregister_notifier_all(struct extcon_dev *edev,
>>>> +                struct notifier_block *nb)
>>>> +{
>>>> +    unsigned long flags;
>>>> +    int ret;
>>>> +
>>>> +    if (!edev || !nb)
>>>> +        return -EINVAL;
>>>> +
>>>> +    spin_lock_irqsave(&edev->lock, flags);
>>>> +    ret = raw_notifier_chain_unregister(&edev->nh_all, nb);
>>>> +    spin_unlock_irqrestore(&edev->lock, flags);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(extcon_unregister_notifier_all);
>>>> +
>>>>  static struct attribute *extcon_attrs[] = {
>>>>      &dev_attr_state.attr,
>>>>      &dev_attr_name.attr,
>>>> @@ -1199,6 +1259,8 @@ int extcon_dev_register(struct extcon_dev *edev)
>>>>      for (index = 0; index < edev->max_supported; index++)
>>>>          RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]);
>>>>
>>>> +    RAW_INIT_NOTIFIER_HEAD(&edev->nh_all);
>>>> +
>>>>      dev_set_drvdata(&edev->dev, edev);
>>>>      edev->state = 0;
>>>>
>>>> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
>>>> index 993ddccafe11..dddddcfa0587 100644
>>>> --- a/drivers/extcon/extcon.h
>>>> +++ b/drivers/extcon/extcon.h
>>>> @@ -21,6 +21,8 @@
>>>>   * @dev:        Device of this extcon.
>>>>   * @state:        Attach/detach state of this extcon. Do not provide at
>>>>   *            register-time.
>>>> + * @nh_all:        Notifier for the state change events for all supported
>>>> + *            external connectors from this extcon.
>>>>   * @nh:            Notifier for the state change events from this extcon
>>>>   * @entry:        To support list of extcon devices so that users can
>>>>   *            search for extcon devices based on the extcon name.
>>>> @@ -43,6 +45,7 @@ struct extcon_dev {
>>>>
>>>>      /* Internal data. Please do not set. */
>>>>      struct device dev;
>>>> +    struct raw_notifier_head nh_all;
>>>>      struct raw_notifier_head *nh;
>>>>      struct list_head entry;
>>>>      int max_supported;
>>>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
>>>> index 3929d2e8a3c7..61e8d8c591a4 100644
>>>> --- a/include/linux/extcon.h
>>>> +++ b/include/linux/extcon.h
>>>> @@ -270,11 +270,11 @@ extern int extcon_set_property_capability(struct extcon_dev *edev,
>>>>                  unsigned int id, unsigned int prop);
>>>>
>>>>  /*
>>>> - * Following APIs are to monitor every action of a notifier.
>>>> - * Registrar gets notified for every external port of a connection device.
>>>> - * Probably this could be used to debug an action of notifier; however,
>>>> - * we do not recommend to use this for normal 'notifiee' device drivers who
>>>> - * want to be notified by a specific external port of the notifier.
>>>> + * Following APIs are to monitor the status change of the external connectors.
>>>> + * extcon_register_notifier(*edev, id, *nb) : Register a notifier
>>>> + *            for specific external connector from the extcon.
>>>> + * extcon_register_notifier_all(*edev, *nb) : Register a notifier
>>>> + *            for all supported external connectors from the extcon.
>>>>   */
>>>>  extern int extcon_register_notifier(struct extcon_dev *edev, unsigned int id,
>>>>                      struct notifier_block *nb);
>>>> @@ -287,6 +287,17 @@ extern void devm_extcon_unregister_notifier(struct device *dev,
>>>>                  struct extcon_dev *edev, unsigned int id,
>>>>                  struct notifier_block *nb);
>>>>
>>>> +extern int extcon_register_notifier_all(struct extcon_dev *edev,
>>>> +                struct notifier_block *nb);
>>>> +extern int extcon_unregister_notifier_all(struct extcon_dev *edev,
>>>> +                struct notifier_block *nb);
>>>> +extern int devm_extcon_register_notifier_all(struct device *dev,
>>>> +                struct extcon_dev *edev,
>>>> +                struct notifier_block *nb);
>>>> +extern void devm_extcon_unregister_notifier_all(struct device *dev,
>>>> +                struct extcon_dev *edev,
>>>> +                struct notifier_block *nb);
>>>> +
>>>>  /*
>>>>   * Following API get the extcon device from devicetree.
>>>>   * This function use phandle of devicetree to get extcon device directly.
>>>>
>>>
>>>
>>>
>>
>>
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors

[Hans de Goede]

Hi,

On 03-04-17 09:24, Chanwoo Choi wrote:
> Hi,
>
> On 2017년 03월 30일 23:58, Hans de Goede wrote:
>> Hi,
>>
>> On 30-03-17 11:20, Chanwoo Choi wrote:
>>> On 2017년 03월 30일 18:04, Hans de Goede wrote:

<snip>

>>>> Also this should be moved outside of the area of the
>>>> function holding the edev->lock spinlock, since we don't
>>>> pass state we do not need the lock and the called
>>>> notifier function may very well want to call extcon_get_state
>>>> to find out the state of one or more of the cables,
>>>> which takes the lock.
>>>
>>> The extcon uses the spinlock for the short delay.
>>> Extcon should update the status of external connector
>>> to the extcon consumer as soon as possible.
>>
>> Right, what I'm suggestion actually also applies to the
>> current cable notification, what I'm suggesting is to
>> move the notification like this:
>>
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -448,8 +448,6 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
>>         spin_lock_irqsave(&edev->lock, flags);
>>
>>         state = !!(edev->state & BIT(index));
>> -   raw_notifier_call_chain(&edev->nh[index], state, edev);
>> -   raw_notifier_call_chain(&edev->nh_all, 0, edev);
>>
>>         /* This could be in interrupt handler */
>>         prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
>> @@ -482,6 +480,10 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
>>
>>         /* Unlock early before uevent */
>>         spin_unlock_irqrestore(&edev->lock, flags);
>> +
>> + raw_notifier_call_chain(&edev->nh[index], state, edev);
>> + raw_notifier_call_chain(&edev->nh_all, 0, edev);
>> +
>>         kobject_uevent_env(&edev->dev.kobj, KOBJ_CHANGE, envp);
>>         free_page((unsigned long)prop_buf);
>>
>>
>> So that the nb.notifier_call function can call extcon_get_state
>> to find out what cable is plugged in without deadlocking because
>> extcon_get_state does spin_lock_irqsave(&edev->lock, flags) too.
>>
>> This is esp. important for the edev->nh_all notifier chain
>> since when used in charger drivers the callback will want to call
>> extcon_get_state for all of: EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_DCP,
>> EXTCON_CHG_USB_CDP, etc. to find out how much current it can draw
>> from the port.
>>
>> AFAICT tell there is no race in moving this outside of the locked
>> section of extcon_sync() and it avoids potential deadlocks in the
>> nb.notifier_call function.
>
>
> Actually, I knew that if the extcon consumer driver uses
> the extcon_get_state() in the callback function, there is a deadlock
> between extcon_sync() and extcon_get_state(). So, all extcon consumer
> uses the workqueue when receiving the notfication from extcon.
>
> But, extcon_sync() have to call the number of callback functions
> in the notifier chanin. If one specific extcon consumer spend many
> time in the own callback function, other extcon consumer might receive
> the notification late. So, I think that each extcon consumer
> better to use the work in the their callback function.
>
> As I already said, I think that extcon focus on sending the notification
> to all of extcon consumers.

Ok, then lets keep your patches as they are, I've added the patches
from your extcon-test branch to my local repository, modified the drivers
which I've pending upstream which need this to use the new functionality
and tested things.

Everything looks and works good with these patches, so please add my:

Acked-and-Tested-by: Hans de Goede <hdegoede@redhat.com>

to them.

It would be great if you can push these patches to extcon-next and
then create a stable branch with these patches which other subsys
maintainers can merge, so that I can start submitting patches which
need this upstream (and also do a cleanup patch for the current axp288
charger code).

Regards,

Hans

Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors

[Chanwoo Choi]

Hi,

On 2017년 04월 03일 20:14, Hans de Goede wrote:
> Hi,
>
> On 03-04-17 09:24, Chanwoo Choi wrote:
>> Hi,
>>
>> On 2017년 03월 30일 23:58, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 30-03-17 11:20, Chanwoo Choi wrote:
>>>> On 2017년 03월 30일 18:04, Hans de Goede wrote:
>
> <snip>
>
>>>>> Also this should be moved outside of the area of the
>>>>> function holding the edev->lock spinlock, since we don't
>>>>> pass state we do not need the lock and the called
>>>>> notifier function may very well want to call extcon_get_state
>>>>> to find out the state of one or more of the cables,
>>>>> which takes the lock.
>>>>
>>>> The extcon uses the spinlock for the short delay.
>>>> Extcon should update the status of external connector
>>>> to the extcon consumer as soon as possible.
>>>
>>> Right, what I'm suggestion actually also applies to the
>>> current cable notification, what I'm suggesting is to
>>> move the notification like this:
>>>
>>> --- a/drivers/extcon/extcon.c
>>> +++ b/drivers/extcon/extcon.c
>>> @@ -448,8 +448,6 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
>>>         spin_lock_irqsave(&edev->lock, flags);
>>>
>>>         state = !!(edev->state & BIT(index));
>>> -   raw_notifier_call_chain(&edev->nh[index], state, edev);
>>> -   raw_notifier_call_chain(&edev->nh_all, 0, edev);
>>>
>>>         /* This could be in interrupt handler */
>>>         prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
>>> @@ -482,6 +480,10 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
>>>
>>>         /* Unlock early before uevent */
>>>         spin_unlock_irqrestore(&edev->lock, flags);
>>> +
>>> + raw_notifier_call_chain(&edev->nh[index], state, edev);
>>> + raw_notifier_call_chain(&edev->nh_all, 0, edev);
>>> +
>>>         kobject_uevent_env(&edev->dev.kobj, KOBJ_CHANGE, envp);
>>>         free_page((unsigned long)prop_buf);
>>>
>>>
>>> So that the nb.notifier_call function can call extcon_get_state
>>> to find out what cable is plugged in without deadlocking because
>>> extcon_get_state does spin_lock_irqsave(&edev->lock, flags) too.
>>>
>>> This is esp. important for the edev->nh_all notifier chain
>>> since when used in charger drivers the callback will want to call
>>> extcon_get_state for all of: EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_DCP,
>>> EXTCON_CHG_USB_CDP, etc. to find out how much current it can draw
>>> from the port.
>>>
>>> AFAICT tell there is no race in moving this outside of the locked
>>> section of extcon_sync() and it avoids potential deadlocks in the
>>> nb.notifier_call function.
>>
>>
>> Actually, I knew that if the extcon consumer driver uses
>> the extcon_get_state() in the callback function, there is a deadlock
>> between extcon_sync() and extcon_get_state(). So, all extcon consumer
>> uses the workqueue when receiving the notfication from extcon.
>>
>> But, extcon_sync() have to call the number of callback functions
>> in the notifier chanin. If one specific extcon consumer spend many
>> time in the own callback function, other extcon consumer might receive
>> the notification late. So, I think that each extcon consumer
>> better to use the work in the their callback function.
>>
>> As I already said, I think that extcon focus on sending the notification
>> to all of extcon consumers.
>
> Ok, then lets keep your patches as they are, I've added the patches
> from your extcon-test branch to my local repository, modified the drivers
> which I've pending upstream which need this to use the new functionality
> and tested things.
>
> Everything looks and works good with these patches, so please add my:
>
> Acked-and-Tested-by: Hans de Goede <hdegoede@redhat.com>
>
> to them.
>
> It would be great if you can push these patches to extcon-next and
> then create a stable branch with these patches which other subsys
> maintainers can merge, so that I can start submitting patches which
> need this upstream (and also do a cleanup patch for the current axp288
> charger code).
>

Sure, After reviewing the patches, I'll make the immutable branch
and send the pull request for other subsystem maintainer as you mentioned.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors

[Hans de Goede]

Hi,

On 03-04-17 13:14, Hans de Goede wrote:
> Hi,
>
> On 03-04-17 09:24, Chanwoo Choi wrote:

<snip>

>> As I already said, I think that extcon focus on sending the notification
>> to all of extcon consumers.
>
> Ok, then lets keep your patches as they are, I've added the patches
> from your extcon-test branch to my local repository, modified the drivers
> which I've pending upstream which need this to use the new functionality
> and tested things.
>
> Everything looks and works good with these patches, so please add my:
>
> Acked-and-Tested-by: Hans de Goede <hdegoede@redhat.com>
>
> to them.
>
> It would be great if you can push these patches to extcon-next and
> then create a stable branch with these patches which other subsys
> maintainers can merge, so that I can start submitting patches which
> need this upstream (and also do a cleanup patch for the current axp288
> charger code).

I see that you've created a "ib-extcon-4.12" branch now, can I
refer other maintainers to that when submitting patches using
the new extcon_register_notifier_all(), or should I wait a bit ?

Regards,

Hans

Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors

[Chanwoo Choi]

Hi,

On 2017년 04월 04일 19:47, Hans de Goede wrote:
> Hi,
> 
> On 03-04-17 13:14, Hans de Goede wrote:
>> Hi,
>>
>> On 03-04-17 09:24, Chanwoo Choi wrote:
> 
> <snip>
> 
>>> As I already said, I think that extcon focus on sending the notification
>>> to all of extcon consumers.
>>
>> Ok, then lets keep your patches as they are, I've added the patches
>> from your extcon-test branch to my local repository, modified the drivers
>> which I've pending upstream which need this to use the new functionality
>> and tested things.
>>
>> Everything looks and works good with these patches, so please add my:
>>
>> Acked-and-Tested-by: Hans de Goede <hdegoede@redhat.com>
>>
>> to them.
>>
>> It would be great if you can push these patches to extcon-next and
>> then create a stable branch with these patches which other subsys
>> maintainers can merge, so that I can start submitting patches which
>> need this upstream (and also do a cleanup patch for the current axp288
>> charger code).
> 
> I see that you've created a "ib-extcon-4.12" branch now, can I
> refer other maintainers to that when submitting patches using
> the new extcon_register_notifier_all(), or should I wait a bit ?

You need to wait for a few days before applying this patch on the extcon-next branch.

After applying this patch on extcon-next, I'll send the pull request patch
to both you and linux-kernel mailing list. And then you better to give the pull request mail
to other subsystem maintainer.

> 
> Regards,
> 
> Hans
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics