Hi,
First patch fixes kernel hang and incorrect states when
extcon_get/set_cable_state() is used.
Second patch fixes extcon_cable_get_state() users from getting old
state after notifier callback.
cheers,
-roger
Roger Quadros (2):
extcon: fix hang and extcon_get/set_cable_state().
extcon: Fix extcon_cable_get_state() from getting old state after
notification
drivers/extcon/extcon.c | 50 ++++++++++++++++++++++++++++++++++++-------------
1 file changed, 37 insertions(+), 13 deletions(-)
--
2.1.4
Users of find_cable_index_by_name() will cause a kernel hang
as the while loop counter is never incremented and end condition
is never reached.
extcon_get_cable_state() and extcon_set_cable_state() are broken
because they use cable index instead of cable id. This causes
the first cable state (cable.0) to be always invalid in sysfs
or extcon_get_cable_state() users.
Introduce a new function find_cable_id_by_name() that fixes
both of the above issues.
Fixes: commit 73b6ecdb93e8 ("extcon: Redefine the unique id of supported external connectors without 'enum extcon' type")
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Roger Quadros <[email protected]>
---
drivers/extcon/extcon.c | 36 +++++++++++++++++++++++++++---------
1 file changed, 27 insertions(+), 9 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 76157ab..868c6e2 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -124,15 +124,12 @@ static int find_cable_index_by_id(struct extcon_dev *edev, const unsigned int id
return -EINVAL;
}
-static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
+static int find_cable_id_by_name(struct extcon_dev *edev, const char *name)
{
- unsigned int id = EXTCON_NONE;
+ unsigned int id = -EINVAL;
int i = 0;
- if (edev->max_supported == 0)
- return -EINVAL;
-
- /* Find the the number of extcon cable */
+ /* Find the id of extcon cable */
while (extcon_name[i]) {
if (!strncmp(extcon_name[i], name, CABLE_NAME_MAX)) {
id = i;
@@ -140,6 +137,19 @@ static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
}
}
+ return id;
+};
+
+static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
+{
+ unsigned int id = EXTCON_NONE;
+
+ if (edev->max_supported == 0)
+ return -EINVAL;
+
+ /* Find the the number of extcon cable */
+ id = find_cable_id_by_name(edev, name);
+
if (id == EXTCON_NONE)
return -EINVAL;
@@ -228,9 +238,11 @@ static ssize_t cable_state_show(struct device *dev,
struct extcon_cable *cable = container_of(attr, struct extcon_cable,
attr_state);
+ int i = cable->cable_index;
+
return sprintf(buf, "%d\n",
extcon_get_cable_state_(cable->edev,
- cable->cable_index));
+ cable->edev->supported_cable[i]));
}
/**
@@ -341,6 +353,9 @@ int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id)
{
int index;
+ if (id == EXTCON_NONE)
+ return -EINVAL;
+
index = find_cable_index_by_id(edev, id);
if (index < 0)
return index;
@@ -361,7 +376,7 @@ EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
*/
int extcon_get_cable_state(struct extcon_dev *edev, const char *cable_name)
{
- return extcon_get_cable_state_(edev, find_cable_index_by_name
+ return extcon_get_cable_state_(edev, find_cable_id_by_name
(edev, cable_name));
}
EXPORT_SYMBOL_GPL(extcon_get_cable_state);
@@ -380,6 +395,9 @@ int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
u32 state;
int index;
+ if (id == EXTCON_NONE)
+ return -EINVAL;
+
index = find_cable_index_by_id(edev, id);
if (index < 0)
return index;
@@ -404,7 +422,7 @@ EXPORT_SYMBOL_GPL(extcon_set_cable_state_);
int extcon_set_cable_state(struct extcon_dev *edev,
const char *cable_name, bool cable_state)
{
- return extcon_set_cable_state_(edev, find_cable_index_by_name
+ return extcon_set_cable_state_(edev, find_cable_id_by_name
(edev, cable_name), cable_state);
}
EXPORT_SYMBOL_GPL(extcon_set_cable_state);
--
2.1.4
Currently the extcon code notifiers the interested listeners
before it updates the extcon state with the new state.
This will cause the listeners that use extcon_cable_get_state()
to get the stale state and loose the new state.
Fix this by first changing the extcon state variable and then
notifying listeners.
Signed-off-by: Roger Quadros <[email protected]>
---
drivers/extcon/extcon.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 868c6e2..26d1e1e 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -275,19 +275,25 @@ int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
spin_lock_irqsave(&edev->lock, flags);
if (edev->state != ((edev->state & ~mask) | (state & mask))) {
+ u32 old_state;
+
if (check_mutually_exclusive(edev, (edev->state & ~mask) |
(state & mask))) {
spin_unlock_irqrestore(&edev->lock, flags);
return -EPERM;
}
+ old_state = edev->state;
+ edev->state &= ~mask;
+ edev->state |= state & mask;
+
for (index = 0; index < edev->max_supported; index++) {
- if (is_extcon_changed(edev->state, state, index, &attached))
- raw_notifier_call_chain(&edev->nh[index], attached, edev);
+ if (is_extcon_changed(old_state, edev->state, index,
+ &attached))
+ raw_notifier_call_chain(&edev->nh[index],
+ attached, edev);
}
- edev->state &= ~mask;
- edev->state |= state & mask;
/* This could be in interrupt handler */
prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
--
2.1.4
On Mon, 2015-07-06 at 17:46 +0300, Roger Quadros wrote:
>
> -static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
> +static int find_cable_id_by_name(struct extcon_dev *edev, const char *name)
> {
> - unsigned int id = EXTCON_NONE;
> + unsigned int id = -EINVAL;
> int i = 0;
>
> - if (edev->max_supported == 0)
> - return -EINVAL;
> -
> - /* Find the the number of extcon cable */
> + /* Find the id of extcon cable */
> while (extcon_name[i]) {
> if (!strncmp(extcon_name[i], name, CABLE_NAME_MAX)) {
> id = i;
> @@ -140,6 +137,19 @@ static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
> }
> }
>
Thank you Roger, but this still hang. 'i' is not incremented.
Ivan
> + return id;
> +};
>
Hi,
On 07/07/15 15:40, Ivan T. Ivanov wrote:
>
> On Mon, 2015-07-06 at 17:46 +0300, Roger Quadros wrote:
>
>>
>> -static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
>> +static int find_cable_id_by_name(struct extcon_dev *edev, const char *name)
>> {
>> - unsigned int id = EXTCON_NONE;
>> + unsigned int id = -EINVAL;
>> int i = 0;
>>
>> - if (edev->max_supported == 0)
>> - return -EINVAL;
>> -
>> - /* Find the the number of extcon cable */
>> + /* Find the id of extcon cable */
>> while (extcon_name[i]) {
>> if (!strncmp(extcon_name[i], name, CABLE_NAME_MAX)) {
>> id = i;
>> @@ -140,6 +137,19 @@ static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
>> }
>> }
>>
>
> Thank you Roger, but this still hang. 'i' is not incremented.
You are right. Thanks for pointing out.
I will send out a v2.
>
> Ivan
>
>> + return id;
>> +};
>>
cheers,
-roger
Users of find_cable_index_by_name() will cause a kernel hang
as the while loop counter is never incremented and end condition
is never reached.
extcon_get_cable_state() and extcon_set_cable_state() are broken
because they use cable index instead of cable id. This causes
the first cable state (cable.0) to be always invalid in sysfs
or extcon_get_cable_state() users.
Introduce a new function find_cable_id_by_name() that fixes
both of the above issues.
Fixes: commit 73b6ecdb93e8 ("extcon: Redefine the unique id of supported external connectors without 'enum extcon' type")
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Roger Quadros <[email protected]>
---
drivers/extcon/extcon.c | 38 +++++++++++++++++++++++++++++---------
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 76157ab..987dd3c 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -124,22 +124,34 @@ static int find_cable_index_by_id(struct extcon_dev *edev, const unsigned int id
return -EINVAL;
}
-static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
+static int find_cable_id_by_name(struct extcon_dev *edev, const char *name)
{
- unsigned int id = EXTCON_NONE;
+ unsigned int id = -EINVAL;
int i = 0;
- if (edev->max_supported == 0)
- return -EINVAL;
-
- /* Find the the number of extcon cable */
+ /* Find the id of extcon cable */
while (extcon_name[i]) {
if (!strncmp(extcon_name[i], name, CABLE_NAME_MAX)) {
id = i;
break;
}
+
+ i++;
}
+ return id;
+};
+
+static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
+{
+ unsigned int id = EXTCON_NONE;
+
+ if (edev->max_supported == 0)
+ return -EINVAL;
+
+ /* Find the the number of extcon cable */
+ id = find_cable_id_by_name(edev, name);
+
if (id == EXTCON_NONE)
return -EINVAL;
@@ -228,9 +240,11 @@ static ssize_t cable_state_show(struct device *dev,
struct extcon_cable *cable = container_of(attr, struct extcon_cable,
attr_state);
+ int i = cable->cable_index;
+
return sprintf(buf, "%d\n",
extcon_get_cable_state_(cable->edev,
- cable->cable_index));
+ cable->edev->supported_cable[i]));
}
/**
@@ -341,6 +355,9 @@ int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id)
{
int index;
+ if (id == EXTCON_NONE)
+ return -EINVAL;
+
index = find_cable_index_by_id(edev, id);
if (index < 0)
return index;
@@ -361,7 +378,7 @@ EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
*/
int extcon_get_cable_state(struct extcon_dev *edev, const char *cable_name)
{
- return extcon_get_cable_state_(edev, find_cable_index_by_name
+ return extcon_get_cable_state_(edev, find_cable_id_by_name
(edev, cable_name));
}
EXPORT_SYMBOL_GPL(extcon_get_cable_state);
@@ -380,6 +397,9 @@ int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
u32 state;
int index;
+ if (id == EXTCON_NONE)
+ return -EINVAL;
+
index = find_cable_index_by_id(edev, id);
if (index < 0)
return index;
@@ -404,7 +424,7 @@ EXPORT_SYMBOL_GPL(extcon_set_cable_state_);
int extcon_set_cable_state(struct extcon_dev *edev,
const char *cable_name, bool cable_state)
{
- return extcon_set_cable_state_(edev, find_cable_index_by_name
+ return extcon_set_cable_state_(edev, find_cable_id_by_name
(edev, cable_name), cable_state);
}
EXPORT_SYMBOL_GPL(extcon_set_cable_state);
--
2.1.4
On Tue, 2015-07-07 at 16:06 +0300, Roger Quadros wrote:
> Users of find_cable_index_by_name() will cause a kernel hang
> as the while loop counter is never incremented and end condition
> is never reached.
>
> extcon_get_cable_state() and extcon_set_cable_state() are broken
> because they use cable index instead of cable id. This causes
> the first cable state (cable.0) to be always invalid in sysfs
> or extcon_get_cable_state() users.
>
> Introduce a new function find_cable_id_by_name() that fixes
> both of the above issues.
>
> Fixes: commit 73b6ecdb93e8 ("extcon: Redefine the unique id of supported external connectors without 'enum extcon' type")
> Cc: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Roger Quadros <[email protected]>
> ---
Thank you. It is fine now. For both patches.
Tested-by: Ivan T. Ivanov <[email protected]>
Hi Roger,
Thanks for your working.
I'll review, test and apply it on next week because I'm on vacation now.
Thanks,
Chanwoo Choi
On Tue, Jul 7, 2015 at 3:06 PM, Roger Quadros <[email protected]> wrote:
> Users of find_cable_index_by_name() will cause a kernel hang
> as the while loop counter is never incremented and end condition
> is never reached.
>
> extcon_get_cable_state() and extcon_set_cable_state() are broken
> because they use cable index instead of cable id. This causes
> the first cable state (cable.0) to be always invalid in sysfs
> or extcon_get_cable_state() users.
>
> Introduce a new function find_cable_id_by_name() that fixes
> both of the above issues.
>
> Fixes: commit 73b6ecdb93e8 ("extcon: Redefine the unique id of supported external connectors without 'enum extcon' type")
> Cc: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Roger Quadros <[email protected]>
> ---
> drivers/extcon/extcon.c | 38 +++++++++++++++++++++++++++++---------
> 1 file changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 76157ab..987dd3c 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -124,22 +124,34 @@ static int find_cable_index_by_id(struct extcon_dev *edev, const unsigned int id
> return -EINVAL;
> }
>
> -static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
> +static int find_cable_id_by_name(struct extcon_dev *edev, const char *name)
> {
> - unsigned int id = EXTCON_NONE;
> + unsigned int id = -EINVAL;
> int i = 0;
>
> - if (edev->max_supported == 0)
> - return -EINVAL;
> -
> - /* Find the the number of extcon cable */
> + /* Find the id of extcon cable */
> while (extcon_name[i]) {
> if (!strncmp(extcon_name[i], name, CABLE_NAME_MAX)) {
> id = i;
> break;
> }
> +
> + i++;
> }
>
> + return id;
> +};
> +
> +static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
> +{
> + unsigned int id = EXTCON_NONE;
> +
> + if (edev->max_supported == 0)
> + return -EINVAL;
> +
> + /* Find the the number of extcon cable */
> + id = find_cable_id_by_name(edev, name);
> +
> if (id == EXTCON_NONE)
> return -EINVAL;
>
> @@ -228,9 +240,11 @@ static ssize_t cable_state_show(struct device *dev,
> struct extcon_cable *cable = container_of(attr, struct extcon_cable,
> attr_state);
>
> + int i = cable->cable_index;
> +
> return sprintf(buf, "%d\n",
> extcon_get_cable_state_(cable->edev,
> - cable->cable_index));
> + cable->edev->supported_cable[i]));
> }
>
> /**
> @@ -341,6 +355,9 @@ int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id)
> {
> int index;
>
> + if (id == EXTCON_NONE)
> + return -EINVAL;
> +
> index = find_cable_index_by_id(edev, id);
> if (index < 0)
> return index;
> @@ -361,7 +378,7 @@ EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
> */
> int extcon_get_cable_state(struct extcon_dev *edev, const char *cable_name)
> {
> - return extcon_get_cable_state_(edev, find_cable_index_by_name
> + return extcon_get_cable_state_(edev, find_cable_id_by_name
> (edev, cable_name));
> }
> EXPORT_SYMBOL_GPL(extcon_get_cable_state);
> @@ -380,6 +397,9 @@ int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
> u32 state;
> int index;
>
> + if (id == EXTCON_NONE)
> + return -EINVAL;
> +
> index = find_cable_index_by_id(edev, id);
> if (index < 0)
> return index;
> @@ -404,7 +424,7 @@ EXPORT_SYMBOL_GPL(extcon_set_cable_state_);
> int extcon_set_cable_state(struct extcon_dev *edev,
> const char *cable_name, bool cable_state)
> {
> - return extcon_set_cable_state_(edev, find_cable_index_by_name
> + return extcon_set_cable_state_(edev, find_cable_id_by_name
> (edev, cable_name), cable_state);
> }
> EXPORT_SYMBOL_GPL(extcon_set_cable_state);
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Chanwoo,
On 10/07/15 18:54, Chanwoo Choi wrote:
> Hi Roger,
>
> Thanks for your working.
>
> I'll review, test and apply it on next week because I'm on vacation now.
Can you please take this for -rc cycle? Thanks.
cheers,
-roger
>
> Thanks,
> Chanwoo Choi
>
> On Tue, Jul 7, 2015 at 3:06 PM, Roger Quadros <[email protected]> wrote:
>> Users of find_cable_index_by_name() will cause a kernel hang
>> as the while loop counter is never incremented and end condition
>> is never reached.
>>
>> extcon_get_cable_state() and extcon_set_cable_state() are broken
>> because they use cable index instead of cable id. This causes
>> the first cable state (cable.0) to be always invalid in sysfs
>> or extcon_get_cable_state() users.
>>
>> Introduce a new function find_cable_id_by_name() that fixes
>> both of the above issues.
>>
>> Fixes: commit 73b6ecdb93e8 ("extcon: Redefine the unique id of supported external connectors without 'enum extcon' type")
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Signed-off-by: Roger Quadros <[email protected]>
>> ---
>> drivers/extcon/extcon.c | 38 +++++++++++++++++++++++++++++---------
>> 1 file changed, 29 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index 76157ab..987dd3c 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -124,22 +124,34 @@ static int find_cable_index_by_id(struct extcon_dev *edev, const unsigned int id
>> return -EINVAL;
>> }
>>
>> -static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
>> +static int find_cable_id_by_name(struct extcon_dev *edev, const char *name)
>> {
>> - unsigned int id = EXTCON_NONE;
>> + unsigned int id = -EINVAL;
>> int i = 0;
>>
>> - if (edev->max_supported == 0)
>> - return -EINVAL;
>> -
>> - /* Find the the number of extcon cable */
>> + /* Find the id of extcon cable */
>> while (extcon_name[i]) {
>> if (!strncmp(extcon_name[i], name, CABLE_NAME_MAX)) {
>> id = i;
>> break;
>> }
>> +
>> + i++;
>> }
>>
>> + return id;
>> +};
>> +
>> +static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
>> +{
>> + unsigned int id = EXTCON_NONE;
>> +
>> + if (edev->max_supported == 0)
>> + return -EINVAL;
>> +
>> + /* Find the the number of extcon cable */
>> + id = find_cable_id_by_name(edev, name);
>> +
>> if (id == EXTCON_NONE)
>> return -EINVAL;
>>
>> @@ -228,9 +240,11 @@ static ssize_t cable_state_show(struct device *dev,
>> struct extcon_cable *cable = container_of(attr, struct extcon_cable,
>> attr_state);
>>
>> + int i = cable->cable_index;
>> +
>> return sprintf(buf, "%d\n",
>> extcon_get_cable_state_(cable->edev,
>> - cable->cable_index));
>> + cable->edev->supported_cable[i]));
>> }
>>
>> /**
>> @@ -341,6 +355,9 @@ int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id)
>> {
>> int index;
>>
>> + if (id == EXTCON_NONE)
>> + return -EINVAL;
>> +
>> index = find_cable_index_by_id(edev, id);
>> if (index < 0)
>> return index;
>> @@ -361,7 +378,7 @@ EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
>> */
>> int extcon_get_cable_state(struct extcon_dev *edev, const char *cable_name)
>> {
>> - return extcon_get_cable_state_(edev, find_cable_index_by_name
>> + return extcon_get_cable_state_(edev, find_cable_id_by_name
>> (edev, cable_name));
>> }
>> EXPORT_SYMBOL_GPL(extcon_get_cable_state);
>> @@ -380,6 +397,9 @@ int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
>> u32 state;
>> int index;
>>
>> + if (id == EXTCON_NONE)
>> + return -EINVAL;
>> +
>> index = find_cable_index_by_id(edev, id);
>> if (index < 0)
>> return index;
>> @@ -404,7 +424,7 @@ EXPORT_SYMBOL_GPL(extcon_set_cable_state_);
>> int extcon_set_cable_state(struct extcon_dev *edev,
>> const char *cable_name, bool cable_state)
>> {
>> - return extcon_set_cable_state_(edev, find_cable_index_by_name
>> + return extcon_set_cable_state_(edev, find_cable_id_by_name
>> (edev, cable_name), cable_state);
>> }
>> EXPORT_SYMBOL_GPL(extcon_set_cable_state);
>> --
>> 2.1.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
Hi Roger,
On 07/27/2015 07:39 PM, Roger Quadros wrote:
> Chanwoo,
>
> On 10/07/15 18:54, Chanwoo Choi wrote:
>> Hi Roger,
>>
>> Thanks for your working.
>>
>> I'll review, test and apply it on next week because I'm on vacation now.
>
> Can you please take this for -rc cycle? Thanks.
I'm sorry for delay review. I'll do it within this week.
Thanks,
Chanwoo Choi
>
> cheers,
> -roger
>
>>
>> Thanks,
>> Chanwoo Choi
>>
>> On Tue, Jul 7, 2015 at 3:06 PM, Roger Quadros <[email protected]> wrote:
>>> Users of find_cable_index_by_name() will cause a kernel hang
>>> as the while loop counter is never incremented and end condition
>>> is never reached.
>>>
>>> extcon_get_cable_state() and extcon_set_cable_state() are broken
>>> because they use cable index instead of cable id. This causes
>>> the first cable state (cable.0) to be always invalid in sysfs
>>> or extcon_get_cable_state() users.
>>>
>>> Introduce a new function find_cable_id_by_name() that fixes
>>> both of the above issues.
>>>
>>> Fixes: commit 73b6ecdb93e8 ("extcon: Redefine the unique id of supported external connectors without 'enum extcon' type")
>>> Cc: Greg Kroah-Hartman <[email protected]>
>>> Signed-off-by: Roger Quadros <[email protected]>
>>> ---
>>> drivers/extcon/extcon.c | 38 +++++++++++++++++++++++++++++---------
>>> 1 file changed, 29 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>>> index 76157ab..987dd3c 100644
>>> --- a/drivers/extcon/extcon.c
>>> +++ b/drivers/extcon/extcon.c
>>> @@ -124,22 +124,34 @@ static int find_cable_index_by_id(struct extcon_dev *edev, const unsigned int id
>>> return -EINVAL;
>>> }
>>>
>>> -static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
>>> +static int find_cable_id_by_name(struct extcon_dev *edev, const char *name)
>>> {
>>> - unsigned int id = EXTCON_NONE;
>>> + unsigned int id = -EINVAL;
>>> int i = 0;
>>>
>>> - if (edev->max_supported == 0)
>>> - return -EINVAL;
>>> -
>>> - /* Find the the number of extcon cable */
>>> + /* Find the id of extcon cable */
>>> while (extcon_name[i]) {
>>> if (!strncmp(extcon_name[i], name, CABLE_NAME_MAX)) {
>>> id = i;
>>> break;
>>> }
>>> +
>>> + i++;
>>> }
>>>
>>> + return id;
>>> +};
>>> +
>>> +static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
>>> +{
>>> + unsigned int id = EXTCON_NONE;
>>> +
>>> + if (edev->max_supported == 0)
>>> + return -EINVAL;
>>> +
>>> + /* Find the the number of extcon cable */
>>> + id = find_cable_id_by_name(edev, name);
>>> +
>>> if (id == EXTCON_NONE)
>>> return -EINVAL;
>>>
>>> @@ -228,9 +240,11 @@ static ssize_t cable_state_show(struct device *dev,
>>> struct extcon_cable *cable = container_of(attr, struct extcon_cable,
>>> attr_state);
>>>
>>> + int i = cable->cable_index;
>>> +
>>> return sprintf(buf, "%d\n",
>>> extcon_get_cable_state_(cable->edev,
>>> - cable->cable_index));
>>> + cable->edev->supported_cable[i]));
>>> }
>>>
>>> /**
>>> @@ -341,6 +355,9 @@ int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id)
>>> {
>>> int index;
>>>
>>> + if (id == EXTCON_NONE)
>>> + return -EINVAL;
>>> +
>>> index = find_cable_index_by_id(edev, id);
>>> if (index < 0)
>>> return index;
>>> @@ -361,7 +378,7 @@ EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
>>> */
>>> int extcon_get_cable_state(struct extcon_dev *edev, const char *cable_name)
>>> {
>>> - return extcon_get_cable_state_(edev, find_cable_index_by_name
>>> + return extcon_get_cable_state_(edev, find_cable_id_by_name
>>> (edev, cable_name));
>>> }
>>> EXPORT_SYMBOL_GPL(extcon_get_cable_state);
>>> @@ -380,6 +397,9 @@ int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
>>> u32 state;
>>> int index;
>>>
>>> + if (id == EXTCON_NONE)
>>> + return -EINVAL;
>>> +
>>> index = find_cable_index_by_id(edev, id);
>>> if (index < 0)
>>> return index;
>>> @@ -404,7 +424,7 @@ EXPORT_SYMBOL_GPL(extcon_set_cable_state_);
>>> int extcon_set_cable_state(struct extcon_dev *edev,
>>> const char *cable_name, bool cable_state)
>>> {
>>> - return extcon_set_cable_state_(edev, find_cable_index_by_name
>>> + return extcon_set_cable_state_(edev, find_cable_id_by_name
>>> (edev, cable_name), cable_state);
>>> }
>>> EXPORT_SYMBOL_GPL(extcon_set_cable_state);
>>> --
>>> 2.1.4
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at http://www.tux.org/lkml/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
Hi Roger,
I add minor comment about code clean.
After I modified it by myself, I applied it on extcon-fixes.
Best Regards,
Chanwoo Choi
On 07/07/2015 10:06 PM, Roger Quadros wrote:
> Users of find_cable_index_by_name() will cause a kernel hang
> as the while loop counter is never incremented and end condition
> is never reached.
>
> extcon_get_cable_state() and extcon_set_cable_state() are broken
> because they use cable index instead of cable id. This causes
> the first cable state (cable.0) to be always invalid in sysfs
> or extcon_get_cable_state() users.
>
> Introduce a new function find_cable_id_by_name() that fixes
> both of the above issues.
>
> Fixes: commit 73b6ecdb93e8 ("extcon: Redefine the unique id of supported external connectors without 'enum extcon' type")
> Cc: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Roger Quadros <[email protected]>
> ---
> drivers/extcon/extcon.c | 38 +++++++++++++++++++++++++++++---------
> 1 file changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 76157ab..987dd3c 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -124,22 +124,34 @@ static int find_cable_index_by_id(struct extcon_dev *edev, const unsigned int id
> return -EINVAL;
> }
>
> -static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
> +static int find_cable_id_by_name(struct extcon_dev *edev, const char *name)
> {
> - unsigned int id = EXTCON_NONE;
> + unsigned int id = -EINVAL;
> int i = 0;
>
> - if (edev->max_supported == 0)
> - return -EINVAL;
> -
> - /* Find the the number of extcon cable */
> + /* Find the id of extcon cable */
> while (extcon_name[i]) {
> if (!strncmp(extcon_name[i], name, CABLE_NAME_MAX)) {
> id = i;
> break;
> }
> +
Remove blank line.
> + i++;
> }
>
> + return id;
> +};
> +
> +static int find_cable_index_by_name(struct extcon_dev *edev, const char *name)
> +{
> + unsigned int id = EXTCON_NONE;
> +
> + if (edev->max_supported == 0)
> + return -EINVAL;
> +
> + /* Find the the number of extcon cable */
> + id = find_cable_id_by_name(edev, name);
> +
> if (id == EXTCON_NONE)
> return -EINVAL;
>
> @@ -228,9 +240,11 @@ static ssize_t cable_state_show(struct device *dev,
> struct extcon_cable *cable = container_of(attr, struct extcon_cable,
> attr_state);
>
> + int i = cable->cable_index;
> +
> return sprintf(buf, "%d\n",
> extcon_get_cable_state_(cable->edev,
> - cable->cable_index));
> + cable->edev->supported_cable[i]));
> }
>
> /**
> @@ -341,6 +355,9 @@ int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id)
> {
> int index;
>
> + if (id == EXTCON_NONE)
> + return -EINVAL;
> +
This if statement don't be necessary.
Instead, I check the error value on extcon_get_cable_state() by checking the
return value of find_cable_id_by_name().
> index = find_cable_index_by_id(edev, id);
> if (index < 0)
> return index;
> @@ -361,7 +378,7 @@ EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
> */
> int extcon_get_cable_state(struct extcon_dev *edev, const char *cable_name)
> {
> - return extcon_get_cable_state_(edev, find_cable_index_by_name
> + return extcon_get_cable_state_(edev, find_cable_id_by_name
> (edev, cable_name));
I modified it as following:
unsigned int id;
id = find_cable_id_by_name(edev, cable_name);
if (id < 0)
return id;
return extcon_get_cable_state_(edev, id);
> }
> EXPORT_SYMBOL_GPL(extcon_get_cable_state);
> @@ -380,6 +397,9 @@ int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
> u32 state;
> int index;
>
> + if (id == EXTCON_NONE)
> + return -EINVAL;
> +
ditto.
> index = find_cable_index_by_id(edev, id);
> if (index < 0)
> return index;
> @@ -404,7 +424,7 @@ EXPORT_SYMBOL_GPL(extcon_set_cable_state_);
> int extcon_set_cable_state(struct extcon_dev *edev,
> const char *cable_name, bool cable_state)
> {
> - return extcon_set_cable_state_(edev, find_cable_index_by_name
> + return extcon_set_cable_state_(edev, find_cable_id_by_name
> (edev, cable_name), cable_state);
ditto.
Thanks,
Chanwoo Choi
Hi Roger,
On 07/06/2015 11:46 PM, Roger Quadros wrote:
> Currently the extcon code notifiers the interested listeners
> before it updates the extcon state with the new state.
> This will cause the listeners that use extcon_cable_get_state()
> to get the stale state and loose the new state.
>
> Fix this by first changing the extcon state variable and then
> notifying listeners.
>
> Signed-off-by: Roger Quadros <[email protected]>
> ---
> drivers/extcon/extcon.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
Applied it.
Thanks,
Chanwoo Choi
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 868c6e2..26d1e1e 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -275,19 +275,25 @@ int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
> spin_lock_irqsave(&edev->lock, flags);
>
> if (edev->state != ((edev->state & ~mask) | (state & mask))) {
> + u32 old_state;
> +
> if (check_mutually_exclusive(edev, (edev->state & ~mask) |
> (state & mask))) {
> spin_unlock_irqrestore(&edev->lock, flags);
> return -EPERM;
> }
>
> + old_state = edev->state;
> + edev->state &= ~mask;
> + edev->state |= state & mask;
> +
> for (index = 0; index < edev->max_supported; index++) {
> - if (is_extcon_changed(edev->state, state, index, &attached))
> - raw_notifier_call_chain(&edev->nh[index], attached, edev);
> + if (is_extcon_changed(old_state, edev->state, index,
> + &attached))
> + raw_notifier_call_chain(&edev->nh[index],
> + attached, edev);
> }
>
> - edev->state &= ~mask;
> - edev->state |= state & mask;
>
> /* This could be in interrupt handler */
> prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
>