| 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: Sun, 17 May 2015 16:10:24 +0900
From: Krzysztof Kozlowski <k.kozlowski@...sung.com>
To: Chanwoo Choi <cw00.choi@...sung.com>
Cc: linux-kernel@...r.kernel.org, myungjoo.ham@...sung.com,
Kozik <k.kozlowski@...sung.com>,
ckeepax@...nsource.wolfsonmicro.com, gg@...mlogic.co.uk,
kishon@...com, jaewon02.kim@...sung.com, rogerq@...com,
ramakrishna.pallala@...el.com, george.cherian@...com, balbi@...com,
aaro.koskinen@....fi
Subject: Re: [PATCH 1/2] extcon: Use the unique id for external connector
instead of string
2015-05-15 23:31 GMT+09:00 Chanwoo Choi <cw00.choi@...sung.com>:
> This patch uses the unique id to identify the type of external connector instead
> of string name. The string name have the many potential issues. So, this patch
> defines the 'extcon' enumeration which includes all supported external connector
> on EXTCON subsystem. If new external connector is necessary, the unique id of
> new connector have to be added in 'extcon' enumeration. There are current
> supported external connector in 'enum extcon' as following:
I like the idea of switching to unique identifier. Some comments below.
>
> enum extcon {
> EXTCON_NONE = 0x0, /* NONE */
>
> /* USB external connector */
> EXTCON_USB = 0x1, /* USB */
> EXTCON_USB_HOST = 0x2, /* USB-HOST */
>
> /* Charger external connector */
> EXTCON_TA = 0x10, /* TA */
> EXTCON_FAST_CHARGER = 0x11, /* FAST-CHARGER */
> EXTCON_SLOW_CHARGER = 0x12, /* SLOW-CHARGER */
> EXTCON_CHARGE_DOWNSTREAM= 0x13, /* CHARGE-DOWNSTREAM */
>
> /* Audio and video external connector */
> EXTCON_LINE_IN = 0x20, /* LINE-IN */
> EXTCON_LINE_OUT = 0x21, /* LINE-OUT */
> EXTCON_MICROPHONE = 0x22, /* MICROPHONE */
> EXTCON_HEADPHONE = 0x23, /* HEADPHONE */
>
> EXTCON_HDMI = 0x30, /* HDMI */
> EXTCON_MHL = 0x31, /* MHL */
> EXTCON_DVI = 0x32, /* DVI */
> EXTCON_VGA = 0x33, /* VGA */
> EXTCON_SPDIF_IN = 0x34, /* SPDIF-IN */
> EXTCON_SPDIF_OUT = 0x35, /* SPDIF-OUT */
> EXTCON_VIDEO_IN = 0x36, /* VIDEO-IN */
> EXTCON_VIDEO_OUT = 0x37, /* VIDEO-OUT */
>
> /* Miscellaneous external connector */
> EXTCON_DOCK = 0x50, /* DOCK */
> EXTCON_JIG = 0x51, /* JIG */
> EXTCON_MECHANICAL = 0x52, /* MECHANICAL */
>
> __EXTCON_END,
> };
>
> For exmaple in extcon-arizoan.c:
s/exmaple/example/
s/arizoan/arizona/
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 2fb5f75..4aeb585 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -3,6 +3,9 @@
> *
> * External connector (extcon) class driver
> *
> + * Copyright (C) 2015 Samsung Electronics
> + * Author: Chanwoo Choi <cw00.choi@...sung.com>
> + *
> * Copyright (C) 2012 Samsung Electronics
> * Author: Donggeun Kim <dg77.kim@...sung.com>
> * Author: MyungJoo Ham <myungjoo.ham@...sung.com>
> @@ -32,36 +35,43 @@
> #include <linux/slab.h>
> #include <linux/sysfs.h>
>
> -/*
> - * extcon_cable_name suggests the standard cable names for commonly used
> - * cable types.
> - *
> - * However, please do not use extcon_cable_name directly for extcon_dev
> - * struct's supported_cable pointer unless your device really supports
> - * every single port-type of the following cable names. Please choose cable
> - * names that are actually used in your extcon device.
> - */
> -const char extcon_cable_name[][CABLE_NAME_MAX + 1] = {
> +#define SUPPORTED_CABLE_MAX 32
Why only 32 cables are supported? I mean what is the reason behind the
hard limit?
> +#define CABLE_NAME_MAX 30
> +
> +static const char *extcon_name[] = {
> + [EXTCON_NONE] = "NONE",
> +
> + /* USB external connector */
> [EXTCON_USB] = "USB",
> - [EXTCON_USB_HOST] = "USB-Host",
> + [EXTCON_USB_HOST] = "USB-HOST",
> +
> + /* Charger external connector */
> [EXTCON_TA] = "TA",
> - [EXTCON_FAST_CHARGER] = "Fast-charger",
> - [EXTCON_SLOW_CHARGER] = "Slow-charger",
> - [EXTCON_CHARGE_DOWNSTREAM] = "Charge-downstream",
> + [EXTCON_FAST_CHARGER] = "FAST-CHARGER",
> + [EXTCON_SLOW_CHARGER] = "SLOW-CHARGER",
> + [EXTCON_CHARGE_DOWNSTREAM] = "CHARGE-DOWNSTREAM",
> +
> + /* Audio/Video external connector */
> + [EXTCON_LINE_IN] = "LINE-IN",
> + [EXTCON_LINE_OUT] = "LINE-OUT",
> + [EXTCON_MICROPHONE] = "MICROPHONE",
> + [EXTCON_HEADPHONE] = "HEADPHONE",
> +
> [EXTCON_HDMI] = "HDMI",
> [EXTCON_MHL] = "MHL",
> [EXTCON_DVI] = "DVI",
> [EXTCON_VGA] = "VGA",
> - [EXTCON_DOCK] = "Dock",
> - [EXTCON_LINE_IN] = "Line-in",
> - [EXTCON_LINE_OUT] = "Line-out",
> - [EXTCON_MIC_IN] = "Microphone",
> - [EXTCON_HEADPHONE_OUT] = "Headphone",
> - [EXTCON_SPDIF_IN] = "SPDIF-in",
> - [EXTCON_SPDIF_OUT] = "SPDIF-out",
> - [EXTCON_VIDEO_IN] = "Video-in",
> - [EXTCON_VIDEO_OUT] = "Video-out",
> - [EXTCON_MECHANICAL] = "Mechanical",
> + [EXTCON_SPDIF_IN] = "SPDIF-IN",
> + [EXTCON_SPDIF_OUT] = "SPDIF-OUT",
> + [EXTCON_VIDEO_IN] = "VIDEO-IN",
> + [EXTCON_VIDEO_OUT] = "VIDEO-OUT",
> +
> + /* Etc external connector */
> + [EXTCON_DOCK] = "DOCK",
> + [EXTCON_JIG] = "JIG",
> + [EXTCON_MECHANICAL] = "MECHANICAL",
> +
> + NULL,
> };
This change does not look related to the topic. Can you split it to
separate patch?
>
> static struct class *extcon_class;
> @@ -118,11 +128,9 @@ static ssize_t state_show(struct device *dev, struct device_attribute *attr,
> if (edev->max_supported == 0)
> return sprintf(buf, "%u\n", edev->state);
>
> - for (i = 0; i < SUPPORTED_CABLE_MAX; i++) {
> - if (!edev->supported_cable[i])
> - break;
> + for (i = 0; i < edev->max_supported; i++) {
> count += sprintf(buf + count, "%s=%d\n",
> - edev->supported_cable[i],
> + extcon_name[edev->supported_cable[i]],
> !!(edev->state & (1 << i)));
> }
>
> @@ -171,9 +179,10 @@ static ssize_t cable_name_show(struct device *dev,
> {
> struct extcon_cable *cable = container_of(attr, struct extcon_cable,
> attr_name);
> + int i = cable->cable_index;
>
> return sprintf(buf, "%s\n",
> - cable->edev->supported_cable[cable->cable_index]);
> + extcon_name[cable->edev->supported_cable[i]]);
> }
>
> static ssize_t cable_state_show(struct device *dev,
> @@ -282,39 +291,57 @@ int extcon_set_state(struct extcon_dev *edev, u32 state)
> }
> EXPORT_SYMBOL_GPL(extcon_set_state);
>
> -/**
> - * extcon_find_cable_index() - Get the cable index based on the cable name.
> - * @edev: the extcon device that has the cable.
> - * @cable_name: cable name to be searched.
> - *
> - * Note that accessing a cable state based on cable_index is faster than
> - * cable_name because using cable_name induces a loop with strncmp().
> - * Thus, when get/set_cable_state is repeatedly used, using cable_index
> - * is recommended.
> - */
> -int extcon_find_cable_index(struct extcon_dev *edev, const char *cable_name)
> +static int extcon_find_cable_index(struct extcon_dev *edev,
> + const char *cable_name)
> {
> + enum extcon id = EXTCON_NONE;
> int i;
>
> - if (edev->supported_cable) {
> - for (i = 0; edev->supported_cable[i]; i++) {
> - if (!strncmp(edev->supported_cable[i],
> - cable_name, CABLE_NAME_MAX))
> - return i;
> + if (edev->max_supported == 0)
> + return -EINVAL;
> +
> + /* Find the the number of extcon cable */
> + for (i = 0; i < __EXTCON_END; i++) {
> + if (!extcon_name[i])
> + continue;
> + if (!strncmp(extcon_name[i], cable_name, CABLE_NAME_MAX)) {
> + id = i;
> + break;
> }
> }
>
> + if (id == EXTCON_NONE)
> + return -EINVAL;
> +
> + /* Find the the index of extcon cable in edev->supported_cable */
> + for (i = 0; i < edev->max_supported; i++) {
> + if (edev->supported_cable[i] == id)
> + return i;
> + }
> +
> return -EINVAL;
> }
> -EXPORT_SYMBOL_GPL(extcon_find_cable_index);
>
> /**
> * extcon_get_cable_state_() - Get the status of a specific cable.
> * @edev: the extcon device that has the cable.
> - * @index: cable index that can be retrieved by extcon_find_cable_index().
> + * @id: the unique id of each external connector in extcon enumeration.
> */
> -int extcon_get_cable_state_(struct extcon_dev *edev, int index)
> +int extcon_get_cable_state_(struct extcon_dev *edev, const enum extcon id)
> {
> + int i, index = -EINVAL;
> +
> + /* Find the the index of extcon cable in edev->supported_cable */
> + for (i = 0; edev->max_supported < i; i++) {
> + if (edev->supported_cable[i] == id) {
> + index = i;
> + break;
> + }
> + }
> +
> + if (i == edev->max_supported)
> + return -EINVAL;
> +
> if (index < 0 || (edev->max_supported && edev->max_supported <= index))
> return -EINVAL;
>
> @@ -339,15 +366,27 @@ EXPORT_SYMBOL_GPL(extcon_get_cable_state);
> /**
> * extcon_set_cable_state_() - Set the status of a specific cable.
> * @edev: the extcon device that has the cable.
> - * @index: cable index that can be retrieved by
> - * extcon_find_cable_index().
> - * @cable_state: the new cable status. The default semantics is
> + * @id: the unique id of each external connector
> + * in extcon enumeration.
> + * @state: the new cable status. The default semantics is
> * true: attached / false: detached.
> */
> -int extcon_set_cable_state_(struct extcon_dev *edev,
> - int index, bool cable_state)
> +int extcon_set_cable_state_(struct extcon_dev *edev, enum extcon id,
> + bool cable_state)
> {
> u32 state;
> + int i, index = -EINVAL;
> +
> + /* Find the the index of extcon cable in edev->supported_cable */
> + for (i = 0; i < edev->max_supported; i++) {
> + if (edev->supported_cable[i] == id) {
> + index = i;
> + break;
> + }
> + }
This loop shows in few places, maybe split it to separate static
function? Just to avoid duplication of code.
> +
> + if (i == edev->max_supported)
> + return -EINVAL;
>
> if (index < 0 || (edev->max_supported && edev->max_supported <= index))
> return -EINVAL;
> @@ -605,7 +644,7 @@ static void dummy_sysfs_dev_release(struct device *dev)
> *
> * Return the pointer of extcon device if success or ERR_PTR(err) if fail
> */
> -struct extcon_dev *extcon_dev_allocate(const char **supported_cable)
> +struct extcon_dev *extcon_dev_allocate(const enum extcon *supported_cable)
I think you also have to update the documentation. At least for
{devm}_extcon_dev_allocate but maybe in other places too. Previously
the documentation states that supported_cable is an array of strings.
Additionally AFAIU now it must end with EXTCON_NONE. This
sentinel-like info must be clearly documented.
> {
> struct extcon_dev *edev;
>
> @@ -659,7 +698,7 @@ static void devm_extcon_dev_release(struct device *dev, void *res)
> * or ERR_PTR(err) if fail
> */
> struct extcon_dev *devm_extcon_dev_allocate(struct device *dev,
> - const char **supported_cable)
> + const enum extcon *supported_cable)
> {
> struct extcon_dev **ptr, *edev;
>
> @@ -709,17 +748,15 @@ int extcon_dev_register(struct extcon_dev *edev)
> return ret;
> }
>
> - if (edev->supported_cable) {
> - /* Get size of array */
> - for (index = 0; edev->supported_cable[index]; index++)
> - ;
> - edev->max_supported = index;
> - } else {
> - edev->max_supported = 0;
> - }
> + if (!edev->supported_cable)
> + return -EINVAL;
>
> + for (; edev->supported_cable[index] != EXTCON_NONE; index++);
> +
> + edev->max_supported = index;
> if (index > SUPPORTED_CABLE_MAX) {
> - dev_err(&edev->dev, "extcon: maximum number of supported cables exceeded.\n");
> + dev_err(&edev->dev,
> + "exceed the maximum number of supported cables\n");
> return -EINVAL;
> }
>
> @@ -1070,6 +1107,7 @@ static void __exit extcon_class_exit(void)
> }
> module_exit(extcon_class_exit);
>
> +MODULE_AUTHOR("Chanwoo Choi <cw00.choi@...sung.com>");
> MODULE_AUTHOR("Mike Lockwood <lockwood@...roid.com>");
> MODULE_AUTHOR("Donggeun Kim <dg77.kim@...sung.com>");
> MODULE_AUTHOR("MyungJoo Ham <myungjoo.ham@...sung.com>");
> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
> index 799474d9d..de158a1 100644
> --- a/include/linux/extcon.h
> +++ b/include/linux/extcon.h
> @@ -1,6 +1,9 @@
> /*
> * External connector (extcon) class driver
> *
> + * Copyright (C) 2015 Samsung Electronics
> + * Author: Chanwoo Choi <cw00.choi@...sung.com>
> + *
> * Copyright (C) 2012 Samsung Electronics
> * Author: Donggeun Kim <dg77.kim@...sung.com>
> * Author: MyungJoo Ham <myungjoo.ham@...sung.com>
> @@ -27,8 +30,41 @@
> #include <linux/notifier.h>
> #include <linux/sysfs.h>
>
> -#define SUPPORTED_CABLE_MAX 32
> -#define CABLE_NAME_MAX 30
> +enum extcon {
> + EXTCON_NONE = 0x0, /* NONE */
> +
> + /* USB external connector */
> + EXTCON_USB = 0x1, /* USB */
> + EXTCON_USB_HOST = 0x2, /* USB-HOST */
> +
> + /* Charger external connector */
> + EXTCON_TA = 0x10, /* TA */
> + EXTCON_FAST_CHARGER = 0x11, /* FAST-CHARGER */
> + EXTCON_SLOW_CHARGER = 0x12, /* SLOW-CHARGER */
> + EXTCON_CHARGE_DOWNSTREAM= 0x13, /* CHARGE-DOWNSTREAM */
Space before '='. I know it will mess the alignment in indentation but
it would be still more readable.
> +
> + /* Audio/Video external connector */
> + EXTCON_LINE_IN = 0x20, /* LINE-IN */
> + EXTCON_LINE_OUT = 0x21, /* LINE-OUT */
> + EXTCON_MICROPHONE = 0x22, /* MICROPHONE */
> + EXTCON_HEADPHONE = 0x23, /* HEADPHONE */
> +
> + EXTCON_HDMI = 0x30, /* HDMI */
> + EXTCON_MHL = 0x31, /* MHL */
> + EXTCON_DVI = 0x32, /* DVI */
> + EXTCON_VGA = 0x33, /* VGA */
> + EXTCON_SPDIF_IN = 0x34, /* SPDIF-IN */
> + EXTCON_SPDIF_OUT = 0x35, /* SPDIF-OUT */
> + EXTCON_VIDEO_IN = 0x36, /* VIDEO-IN */
> + EXTCON_VIDEO_OUT = 0x37, /* VIDEO-OUT */
> +
> + /* Etc external connector */
> + EXTCON_DOCK = 0x50, /* DOCK */
> + EXTCON_JIG = 0x51, /* JIG */
> + EXTCON_MECHANICAL = 0x52, /* MECHANICAL */
> +
There is no benefit of each comment here. The comment just duplicates
name, it does not give any additional information. Get rid of it.
> + __EXTCON_END,
Why "__" prefix?
> +};
>
> /*
> * The standard cable name is to help support general notifier
> @@ -48,29 +84,6 @@
> * you don't need such convention. This convention is helpful when
> * notifier can distinguish but notifiee cannot.
> */
Isn't the comment above related to the enum "extcon_cable_name" which
you are removing?
> -enum extcon_cable_name {
> - EXTCON_USB = 0,
> - EXTCON_USB_HOST,
> - EXTCON_TA, /* Travel Adaptor */
> - EXTCON_FAST_CHARGER,
> - EXTCON_SLOW_CHARGER,
> - EXTCON_CHARGE_DOWNSTREAM, /* Charging an external device */
> - EXTCON_HDMI,
> - EXTCON_MHL,
> - EXTCON_DVI,
> - EXTCON_VGA,
> - EXTCON_DOCK,
> - EXTCON_LINE_IN,
> - EXTCON_LINE_OUT,
> - EXTCON_MIC_IN,
> - EXTCON_HEADPHONE_OUT,
> - EXTCON_SPDIF_IN,
> - EXTCON_SPDIF_OUT,
> - EXTCON_VIDEO_IN,
> - EXTCON_VIDEO_OUT,
> - EXTCON_MECHANICAL,
> -};
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists