| 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
| ||
|
Message-ID: <CAGTfZH3L-FZ4+cNHYMpuRqWSbcvEZoOYQdJzxOK8yLofFc5dVw@mail.gmail.com>
Date: Sat, 19 Apr 2014 16:13:20 +0900
From: Chanwoo Choi <cwchoi00@...il.com>
To: Sangjung Woo <sangjung.woo@...sung.com>
Cc: MyungJoo Ham <myungjoo.ham@...sung.com>,
Chanwoo Choi <cw00.choi@...sung.com>,
linux-kernel <linux-kernel@...r.kernel.org>,
Krzysztof Kozlowski <k.kozlowski@...sung.com>,
Seung-Woo Kim <sw0312.kim@...sung.com>
Subject: Re: [PATCHv3 1/8] extcon: Add resource-managed extcon register function
Hi Sangjung,
On Fri, Apr 18, 2014 at 9:32 AM, Sangjung Woo <sangjung.woo@...sung.com> wrote:
> Add resource-managed extcon device register function for convenience.
> For example, if a extcon device is attached with new
> devm_extcon_dev_register(), that extcon device is automatically
> unregistered on driver detach.
>
> Signed-off-by: Sangjung Woo <sangjung.woo@...sung.com>
> ---
> drivers/extcon/extcon-class.c | 72 +++++++++++++++++++++++++++++++++++++++++
> include/linux/extcon.h | 17 ++++++++++
> 2 files changed, 89 insertions(+)
>
> diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c
> index 7ab21aa..e177edb6 100644
> --- a/drivers/extcon/extcon-class.c
> +++ b/drivers/extcon/extcon-class.c
> @@ -819,6 +819,78 @@ void extcon_dev_unregister(struct extcon_dev *edev)
> }
> EXPORT_SYMBOL_GPL(extcon_dev_unregister);
>
> +
> +/*
> + * Device resource management
> + */
This comment is un-necessary because this patchset(v3) remove 'struct
extcon_devres'.
> +static void devm_extcon_dev_release(struct device *dev, void *res)
> +{
> + struct extcon_dev *devres = res;
> +
> + extcon_dev_unregister(devres);
I prefer following function call withou defining separate 'devres' variable.
But, this casting on the first argument is only for readability.
extcon_dev_unregister((strcut extcon_dev *)res);
> +}
> +
> +static int devm_extcon_dev_match(struct device *dev, void *res, void *data)
> +{
> + struct extcon_dev *devres = res;
> + struct extcon_dev *match = data;
> + return devres == match;
To simplify code, I think you could change checking code as following:
return res == data;
> +}
> +
> +/**
> + * devm_extcon_dev_register() - Resource-managed extcon_dev_register()
> + * @dev: device to allocate extcon device
> + * @edev: the new extcon device to register
> + *
> + * Managed extcon_dev_register() function. If extcon device is attached with
> + * this function, that extcon device is automatically unregistered on driver
> + * detach. Internally this function calls extcon_dev_register() function.
> + * To get more information, refer that function.
> + *
> + * If extcon device is registered with this function and the device needs to be
> + * unregistered separately, devm_extcon_dev_unregister() should be used.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int devm_extcon_dev_register(struct device *dev, struct extcon_dev *edev)
> +{
> + struct extcon_dev *devres;
The 'devres' in this function don't mean 'device resource structure'.
So I think it is not proper name.
I think you should use other general name (e.g., 'ptr' or 'res'
instead of 'devres')
> + int ret;
> +
> + devres = devres_alloc(devm_extcon_dev_release, sizeof(*devres)
Other subsytem used double pointer to get device resource from
devres_alloc() instead of
single pointer.(devres is single pointer) I can't find subsystem
using single pointer of devm function.
First of all, We have to analyze the correct reason using only double
pointer instead of single pointer whether single pointer use is good
or not.
> + GFP_KERNEL);
> + if (!devres)
> + return -ENOMEM;
> +
> + ret = extcon_dev_register(edev);
> + if (ret) {
> + devres_free(devres);
> + return ret;
> + }
> +
> + devres = edev;
> + devres_add(dev, devres);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_extcon_dev_register);
> +
> +/**
> + * devm_extcon_dev_unregister() - Resource-managed extcon_dev_unregister()
> + * @dev: device the extcon belongs to
> + * @edev: the extcon device to unregister
> + *
> + * Unregister extcon device that is registered with devm_extcon_dev_register()
> + * function.
> + */
> +void devm_extcon_dev_unregister(struct device *dev, struct extcon_dev *edev)
> +{
> + WARN_ON(devres_release(dev, devm_extcon_dev_release,
> + devm_extcon_dev_match, edev));
> +}
> +EXPORT_SYMBOL_GPL(devm_extcon_dev_unregister);
> +
> #ifdef CONFIG_OF
> /*
> * extcon_get_edev_by_phandle - Get the extcon device from devicetree
> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
> index f488145..35f3343 100644
> --- a/include/linux/extcon.h
> +++ b/include/linux/extcon.h
> @@ -188,6 +188,14 @@ extern void extcon_dev_unregister(struct extcon_dev *edev);
> extern struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name);
>
> /*
> + * Resource-managed extcon device register function.
> + */
> +extern int devm_extcon_dev_register(struct device *dev,
> + struct extcon_dev *edev);
> +extern void devm_extcon_dev_unregister(struct device *dev,
> + struct extcon_dev *edev);
I prefer that this function meet indentation of function definition
needs as following:
extern int devm_extcon_dev_register(struct device *dev,
struct extcon_dev *edev);
extern void devm_extcon_dev_unregister(struct device *dev,
struct extcon_dev *edev);
> +
> +/*
> * get/set/update_state access the 32b encoded state value, which represents
> * states of all possible cables of the multistate port. For example, if one
> * calls extcon_set_state(edev, 0x7), it may mean that all the three cables
> @@ -254,6 +262,15 @@ static inline int extcon_dev_register(struct extcon_dev *edev)
>
> static inline void extcon_dev_unregister(struct extcon_dev *edev) { }
>
> +static inline devm_extcon_dev_register(struct device *dev,
> + struct extcon_dev *edev)
Missing the type of return value.
-> static inline int devm_extcon_dev_register(...
Also, ditto about function definition identification.
> +{
> + return 0;
> +}
> +
> +static inline devm_extcon_dev_unregister(struct device *dev,
ditto.
-> static inline void devm_extcon_dev_unregister(...
Thanks,
Chanwoo Choi
--
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