| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 29 Jul 2016 16:02:23 +0900
From: Chanwoo Choi <cw00.choi@...sung.com>
To: Guenter Roeck <groeck@...gle.com>
Cc: linux-kernel <linux-kernel@...r.kernel.org>,
myungjoo.ham@...sung.com, Chris Zhong <zyw@...k-chips.com>,
Guenter Roeck <groeck@...omium.org>, cwchoi00@...il.com,
"cpgs (cpgs@...sung.com)" <cpgs@...sung.com>
Subject: Re: [PATCH 2/6] extcon: Add the support for extcon property according
to extcon type
Hi Guenter,
On 2016년 07월 28일 02:24, Guenter Roeck wrote:
> On Tue, Jul 26, 2016 at 5:09 AM, Chanwoo Choi <cw00.choi@...sung.com> wrote:
>> This patch support the extcon property for the external connector
>> because each external connector might have the property according to
>> the H/W design and the specific characteristics.
>>
>> - EXTCON_PROP_USB_[property name]
>> - EXTCON_PROP_CHG_[property name]
>> - EXTCON_PROP_JACK_[property name]
>> - EXTCON_PROP_DISP_[property name]
>>
>> Add the new extcon APIs to get/set the property value as following:
>> - int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>> unsigned int prop,
>> union extcon_property_value *prop_val)
>> - int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>> unsigned int prop,
>> union extcon_property_value prop_val)
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
>> ---
>> drivers/extcon/extcon.c | 206 +++++++++++++++++++++++++++++++++++++++++++++++-
>> include/linux/extcon.h | 86 ++++++++++++++++++++
>> 2 files changed, 291 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index b1e6ee6194bc..2317aaea063f 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -196,6 +196,11 @@ struct extcon_cable {
>> struct device_attribute attr_state;
>>
>> struct attribute *attrs[3]; /* to be fed to attr_g.attrs */
>> +
>> + union extcon_property_value usb_propval[EXTCON_PROP_USB_CNT];
>> + union extcon_property_value chg_propval[EXTCON_PROP_CHG_CNT];
>> + union extcon_property_value jack_propval[EXTCON_PROP_JACK_CNT];
>> + union extcon_property_value disp_propval[EXTCON_PROP_DISP_CNT];
>> };
>>
>> static struct class *extcon_class;
>> @@ -248,6 +253,28 @@ static int find_cable_index_by_id(struct extcon_dev *edev, const unsigned int id
>> return -EINVAL;
>> }
>>
>> +static int get_extcon_type(unsigned int prop)
>> +{
>> + switch (prop) {
>> + case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
>> + return EXTCON_TYPE_USB;
>> + case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
>> + return EXTCON_TYPE_CHG;
>> + case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
>> + return EXTCON_TYPE_JACK;
>> + case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
>> + return EXTCON_TYPE_DISP;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static bool is_extcon_attached(struct extcon_dev *edev, unsigned int id,
>
> 'id' isn't used.
I'll remove it on parameter list.
>
>> + unsigned int index)
>> +{
>> + return ((!!(edev->state & (1 << index))) == 1) ? true : false;
>
> Minor comment: This is identical to
>
> return !!(edev->state & (1 << index));
> or, with bitops
> return !!(edev->state & BIT(index));
I'll use the bitops as you comment.
return !!(edev->state & BIT(index));
>
>> +}
>> +
>> static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
>> {
>> if (((prev >> idx) & 0x1) != ((new >> idx) & 0x1)) {
>> @@ -258,6 +285,41 @@ static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
>> return false;
>> }
>>
>> +static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
>> +{
>> + unsigned int type;
>> +
>> + /* Check whether the property is supported or not. */
>> + type = get_extcon_type(prop);
>> + if (type < 0)
>> + return false;
>> +
>> + /* Check whether a specific extcon id supports the property or not. */
>> + if (extcon_info[id].type | type)
>> + return true;
>> +
>> + return false;
>
> simpler:
> return !!(extcon_info[id].type & type);
OK. I'll use it as you comment.
>
> Strictly speaking, the !! isn't even necessary in those mask
> operations since C auto-converts to bool, though people sometimes get
> confused by that.
Thanks for your explanation. For readability, I remain the '!!' operation.
>
>> +}
>> +
>> +#define INIT_PROPERTY(name, name_lower, type) \
>> + if (EXTCON_TYPE_##name || type) { \
>> + for (i = 0; i < EXTCON_PROP_##name##_CNT; i++) \
>> + cable->name_lower##_propval[i] = val; \
>> + } \
>> +
>> +static void init_property(struct extcon_dev *edev, unsigned int id, int index)
>> +{
>> + unsigned int type = extcon_info[id].type;
>> + struct extcon_cable *cable = &edev->cables[index];
>> + union extcon_property_value val = (union extcon_property_value)(0);
>> + int i;
>> +
>> + INIT_PROPERTY(USB, usb, type);
>> + INIT_PROPERTY(CHG, chg, type);
>> + INIT_PROPERTY(JACK, jack, type);
>> + INIT_PROPERTY(DISP, disp, type);
>
> Just wondering if this would be a bit cleaner/simpler.
>
> switch(type) {
> case EXTCON_TYPE_USB:
> memset(cable->usb_propval, sizeof(cable->usb_propval), 0);
> break;
> case EXTCON_TYPE_CHG:
> memset(cable->chg_propval, sizeof(cable->chg_propval), 0);
> break;
> case EXTCON_TYPE_JACK:
> memset(cable->jack_propval, sizeof(cable->jack_propval), 0);
> break;
> case EXTCON_TYPE_DISP:
> memset(cable->disp_propval, sizeof(cable->disp_propval), 0);
> break;
> }
As you comment, I'll modify it as following:
But the each id is able to have the one more extcon type. So, I use the
'if' instead of 'switch'.
if (EXTCON_TYPE_USB & type)
memset(cable->usb_propval, sizeof(cable->usb_propval), 0);
if (EXTCON_TYPE_CHG & type)
memset(cable->chg_propval, sizeof(cable->chg_propval), 0);
if (EXTCON_TYPE_JACK & type)
memset(cable->jack_propval, sizeof(cable->jack_propval), 0);
if (EXTCON_TYPE_DISP & type)
memset(cable->disp_propval, sizeof(cable->disp_propval), 0);
>
>> +}
>> +
>> static ssize_t state_show(struct device *dev, struct device_attribute *attr,
>> char *buf)
>> {
>> @@ -421,7 +483,7 @@ int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id)
>> if (edev->max_supported && edev->max_supported <= index)
>> return -EINVAL;
>>
>> - return !!(edev->state & (1 << index));
>> + return (int)(is_extcon_attached(edev, id, index));
>> }
>> EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
>>
>> @@ -449,12 +511,154 @@ int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
>> if (edev->max_supported && edev->max_supported <= index)
>> return -EINVAL;
>>
>> + /*
>> + * Initialize the value of extcon property before setting
>> + * the detached state for an external connector.
>> + */
>> + if (!cable_state)
>> + init_property(edev, id, index);
>> +
>> + /* Set the state for external connector as the detached state. */
>> state = cable_state ? (1 << index) : 0;
>> return extcon_update_state(edev, 1 << index, state);
>> }
>> EXPORT_SYMBOL_GPL(extcon_set_cable_state_);
>>
>> /**
>> + * extcon_get_property() - Get the property value of a specific cable.
>> + * @edev: the extcon device that has the cable.
>> + * @id: the unique id of each external connector
>> + * in extcon enumeration.
>> + * @prop: the property id among enum extcon_property.
>> + * @prop_val: the pointer which store the value of property.
>> + *
>> + * When getting the property value of external connector, the external connector
>> + * should be attached. If detached state, function just return 0 without
>> + * property value. Also, the each property should be included in the list of
>> + * supported properties according to the type of external connectors.
>> + *
>> + * Returns 0 if success or error number if fail
>> + */
>> +int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>> + unsigned int prop,
>> + union extcon_property_value *prop_val)
>> +{
>> + struct extcon_cable *cable;
>> + unsigned long flags;
>> + int index, ret = 0;
>> +
>> + *prop_val = (union extcon_property_value)(0);
>> +
>> + if (!edev)
>> + return -EINVAL;
>> +
>> + /* Check whether the property is supported or not */
>> + if (!is_extcon_property_supported(id, prop))
>> + return -EINVAL;
>> +
>> + /* Find the cable index of external connector by using id */
>> + index = find_cable_index_by_id(edev, id);
>> + if (index < 0)
>> + return index;
>> +
>> + /*
>> + * Check whether the external connector is attached.
>> + * If external connector is detached, the user can not
>> + * get the property value.
>> + */
>> + if (!is_extcon_attached(edev, id, index))
>> + return 0;
>> +
>
> Wonder if this should be inside the lock. Otherwise the cable state
> might change after the check, but before the lock is acquired.
>
>> + cable = &edev->cables[index];
>> + spin_lock_irqsave(&edev->lock, flags);
You're right. I'll fix it as following.
spin_lock_irqsave(&edev->lock, flags);
if (!is_extcon_attached(edev, id, index)) {
spin_unlock_irqrestore(&edev->lock, flags);
return 0;
}
cable = &edev->cables[index];
>> +
>> + /* Get the property value according to extcon type */
>> + switch (prop) {
>> + case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
>> + *prop_val = cable->usb_propval[prop - EXTCON_PROP_USB_MIN];
>> + break;
>> + case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
>> + *prop_val = cable->chg_propval[prop - EXTCON_PROP_CHG_MIN];
>> + break;
>> + case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
>> + *prop_val = cable->jack_propval[prop - EXTCON_PROP_JACK_MIN];
>> + break;
>> + case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
>> + *prop_val = cable->disp_propval[prop - EXTCON_PROP_DISP_MIN];
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + break;
>> + }
>> +
>> + spin_unlock_irqrestore(&edev->lock, flags);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(extcon_get_property);
[snip]
Regards,
Chanwoo Choi
Powered by blists - more mailing lists