hwmon_device_register_with_info registration API issue

hwmon_device_register_with_info registration API issue

[Lucas Magasweran]

Hi Jean and al,

Thank you for your Kernel Recipes 2016 talk about the new hwmon
registration API [1].

I'm trying to use the latest hwmon_device_register_with_info() API
where hwmon core handles the sysfs attributes for me. However, I cannot
use it with a NULL parent struct device *dev and non-NULL struct
hwmon_chip_info *chip. My platform driver module_init() calls
hwmon_device_register_with_info(). __hwmon_device_register() has a NULL
pointer dereference error because it uses device managed memory
allocation internally.

For example,

my_hwmon_dev = hwmon_device_register_with_info(NULL, "my_name", NULL,
                                               &my_hwmon_chip_info,
                                               NULL);
--> __hwmon_device_register(NULL, "my_name", NULL,
                            &my_hwmon_chip_info, NULL);
----> hwdev->groups = devm_kcalloc(NULL, ngroups,
                                   sizeof(*groups), GFP_KERNEL);

I see that later in the function dev is checked for NULL in "hdev-
>of_node = dev ? dev->of_node : NULL;" but it cannot be NULL for
devm_kcalloc().

I tried the following 4.10.17 patch because I expected the hwmon
attributes to be stored with the hwmon device directly and not the
struct device *dev parent. However, none of my attributes are showing
up in /sys/class/hwmon#/.

Is my approach wrong?

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 3932f92..2e32589 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -560,6 +560,17 @@ __hwmon_device_register(struct device *dev, const
char *name, void *drvdata,
 
        hdev = &hwdev->dev;
 
+       hwdev->name = name;
+       hdev->class = &hwmon_class;
+       hdev->parent = dev;
+       hdev->of_node = dev ? dev->of_node : NULL;
+       hwdev->chip = chip;
+       dev_set_drvdata(hdev, drvdata);
+       dev_set_name(hdev, HWMON_ID_FORMAT, id);
+       err = device_register(hdev);
+       if (err)
+               goto free_hwmon;
+
        if (chip) {
                struct attribute **attrs;
                int ngroups = 2; /* terminating NULL plus &hwdev-
>groups */
@@ -568,14 +579,14 @@ __hwmon_device_register(struct device *dev, const
char *name, void *drvdata,
                        for (i = 0; groups[i]; i++)
                                ngroups++;
 
-               hwdev->groups = devm_kcalloc(dev, ngroups,
sizeof(*groups),
+               hwdev->groups = devm_kcalloc(dev ? dev : hdev, ngroups,
sizeof(*groups),
                                             GFP_KERNEL);
                if (!hwdev->groups) {
                        err = -ENOMEM;
                        goto free_hwmon;
                }
 
-               attrs = __hwmon_create_attrs(dev, drvdata, chip);
+               attrs = __hwmon_create_attrs(dev ? dev : hdev, drvdata,
chip);
                if (IS_ERR(attrs)) {
                        err = PTR_ERR(attrs);
                        goto free_hwmon;
@@ -595,17 +606,6 @@ __hwmon_device_register(struct device *dev, const
char *name, void *drvdata,
                hdev->groups = groups;
        }
 
-       hwdev->name = name;
-       hdev->class = &hwmon_class;
-       hdev->parent = dev;
-       hdev->of_node = dev ? dev->of_node : NULL;
-       hwdev->chip = chip;
-       dev_set_drvdata(hdev, drvdata);
-       dev_set_name(hdev, HWMON_ID_FORMAT, id);
-       err = device_register(hdev);
-       if (err)
-               goto free_hwmon;
-
        if (chip && chip->ops->read &&
            chip->info[0]->type == hwmon_chip &&
            (chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {


 [1] https://kernel-recipes.org/en/2016/talks/new-hwmon-device-registra
tion-api/

Re: hwmon_device_register_with_info registration API issue

[Guenter Roeck]

On Sat, May 05, 2018 at 12:22:29AM +0200, Lucas Magasweran wrote:
> Hi Jean and al,
> 
> Thank you for your Kernel Recipes 2016 talk about the new hwmon
> registration API [1].
> 
> I'm trying to use the latest hwmon_device_register_with_info() API
> where hwmon core handles the sysfs attributes for me. However, I cannot
> use it with a NULL parent struct device *dev and non-NULL struct
> hwmon_chip_info *chip. My platform driver module_init() calls
> hwmon_device_register_with_info(). __hwmon_device_register() has a NULL
> pointer dereference error because it uses device managed memory
> allocation internally.
> 
> For example,
> 
> my_hwmon_dev = hwmon_device_register_with_info(NULL, "my_name", NULL,
>                                                &my_hwmon_chip_info,
>                                                NULL);
> --> __hwmon_device_register(NULL, "my_name", NULL,
>                             &my_hwmon_chip_info, NULL);
> ----> hwdev->groups = devm_kcalloc(NULL, ngroups,
>                                    sizeof(*groups), GFP_KERNEL);
> 
> I see that later in the function dev is checked for NULL in "hdev-
> >of_node = dev ? dev->of_node : NULL;" but it cannot be NULL for
> devm_kcalloc().
> 
> I tried the following 4.10.17 patch because I expected the hwmon
> attributes to be stored with the hwmon device directly and not the
> struct device *dev parent. However, none of my attributes are showing
> up in /sys/class/hwmon#/.
> 

I assume you mean /sys/class/hwmon/hwmon#/.

> Is my approach wrong?
> 

Yes. Parent must not be NULL if chip is used. The use case of parent==NULL
and chip!=NULL is not and will not be supported. Also, the only reason
for supporting a NULL parent is that the thermal code uses it, and it
is all but impossible to change the thermal code to provide a device
pointer.

I would strongly suggest to modify your code and provide
a parent (a pointer to either a platform or hardware device).
Most likely that means to convert it to a platform device.

If you insist in not providing a parent, you'll have to use
hwmon_device_register_with_groups(). Of course, that means you'll
have to handle the attributes.

Guenter

[PATCH] hwmon: (core) check parent dev != NULL when chip != NULL

[Lucas Magasweran]

Guenter, thank you for clarifying the usage of the registration API.

Here is a proposed patch that checks for NULL parent if chip is non-NULL
to avoid the null pointer dereference when used incorrectly with an 
update to the documentation.

--

Lucas Magasweran (1):
  hwmon: (core) check parent dev != NULL when chip != NULL

 Documentation/hwmon/hwmon-kernel-api.txt | 3 ++-
 drivers/hwmon/hwmon.c                    | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH] hwmon: (core) check parent dev != NULL when chip != NULL

[Lucas Magasweran]

hwmon_device_register_with_info() registration API requires a
non-NULL parent device when chip is non-NULL.

This commit adds a check and documents this requirement.

Signed-off-by: Lucas Magasweran <lucas.magasweran@ieee.org>
---
 Documentation/hwmon/hwmon-kernel-api.txt | 3 ++-
 drivers/hwmon/hwmon.c                    | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/hwmon/hwmon-kernel-api.txt b/Documentation/hwmon/hwmon-kernel-api.txt
index 53a8066..eb7a78a 100644
--- a/Documentation/hwmon/hwmon-kernel-api.txt
+++ b/Documentation/hwmon/hwmon-kernel-api.txt
@@ -71,7 +71,8 @@ hwmon_device_register_with_info is the most comprehensive and preferred means
 to register a hardware monitoring device. It creates the standard sysfs
 attributes in the hardware monitoring core, letting the driver focus on reading
 from and writing to the chip instead of having to bother with sysfs attributes.
-Its parameters are described in more detail below.
+The parent device parameter cannot be NULL with non-NULL chip info. Its
+parameters are described in more detail below.
 
 devm_hwmon_device_register_with_info is similar to
 hwmon_device_register_with_info. However, it is device managed, meaning the
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 32083e4..e88c019 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -698,6 +698,9 @@ hwmon_device_register_with_info(struct device *dev, const char *name,
 	if (chip && (!chip->ops || !chip->ops->is_visible || !chip->info))
 		return ERR_PTR(-EINVAL);
 
+	if (chip && !dev)
+		return ERR_PTR(-EINVAL);
+
 	return __hwmon_device_register(dev, name, drvdata, chip, extra_groups);
 }
 EXPORT_SYMBOL_GPL(hwmon_device_register_with_info);
-- 
2.7.4

Re: hwmon: (core) check parent dev != NULL when chip != NULL

[Guenter Roeck]

On Tue, May 08, 2018 at 04:43:33AM -0700, Lucas Magasweran wrote:
> hwmon_device_register_with_info() registration API requires a
> non-NULL parent device when chip is non-NULL.
> 
> This commit adds a check and documents this requirement.
> 
> Signed-off-by: Lucas Magasweran <lucas.magasweran@ieee.org>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  Documentation/hwmon/hwmon-kernel-api.txt | 3 ++-
>  drivers/hwmon/hwmon.c                    | 3 +++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/hwmon/hwmon-kernel-api.txt b/Documentation/hwmon/hwmon-kernel-api.txt
> index 53a8066..eb7a78a 100644
> --- a/Documentation/hwmon/hwmon-kernel-api.txt
> +++ b/Documentation/hwmon/hwmon-kernel-api.txt
> @@ -71,7 +71,8 @@ hwmon_device_register_with_info is the most comprehensive and preferred means
>  to register a hardware monitoring device. It creates the standard sysfs
>  attributes in the hardware monitoring core, letting the driver focus on reading
>  from and writing to the chip instead of having to bother with sysfs attributes.
> -Its parameters are described in more detail below.
> +The parent device parameter cannot be NULL with non-NULL chip info. Its
> +parameters are described in more detail below.
>  
>  devm_hwmon_device_register_with_info is similar to
>  hwmon_device_register_with_info. However, it is device managed, meaning the
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 32083e4..e88c019 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -698,6 +698,9 @@ hwmon_device_register_with_info(struct device *dev, const char *name,
>  	if (chip && (!chip->ops || !chip->ops->is_visible || !chip->info))
>  		return ERR_PTR(-EINVAL);
>  
> +	if (chip && !dev)
> +		return ERR_PTR(-EINVAL);
> +
>  	return __hwmon_device_register(dev, name, drvdata, chip, extra_groups);
>  }
>  EXPORT_SYMBOL_GPL(hwmon_device_register_with_info);