hwmon_device_register_with_info registration API issue

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-

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?

index 3932f92..2e32589 100644
@@ -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-

@@ -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/

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

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

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

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.

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

index 53a8066..eb7a78a 100644
@@ -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

index 32083e4..e88c019 100644
@@ -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

Applied to hwmon-next.

Thanks,
Guenter