Re: [PATCH] Fix sysfs_notify and kobject_uevent in fan_alarm_notify

[Guenter Roeck <linux@roeck-us.net>]

On Wed, Jun 19, 2019 at 10:47:34AM +0200, Christian Schneider wrote:
> Hi,
> unfrotunately I found a problem with my patch. It didn't appear on the
> devices, where I tested, only when distributing the patch to more devices,
> it occurs sometimes.
> 
> There is a possible race condition. In gpio_fan_probe, the IRQ handler for
> the fan alarm is registered before the hwmon_dev is initialised.
> So it is possible, that the handler executes, while fandata->hwmon_dev is
> still unitialised and the handler runs in an invalid pointer dereference.
> 
> The obvious fix, moving the
> /* Configure alarm GPIO if available. */
> if (fan_data->alarm_gpio) {
> 	err = fan_alarm_init(fan_data);
> 	if (err)
> 		return err;
> }
> block after
> 
> /* Make this driver part of hwmon class. */
> fan_data->hwmon_dev =
> 	devm_hwmon_device_register_with_groups(dev,
> 					       "gpio_fan", fan_data,
> 					       gpio_fan_groups);
> 
> doesn't work. Sysfs doesn't have a fan1_alarm attribute anymore then.
> 
> It is not yet clear to me, why creation of the attribute fails in this case.
> I need to investigate further.
> In the meanwhile, please revert my patch.
> 
> I'm sorry for the disturbance I created....
> 
No worries. Thanks for noticing and for reporting the problem. I dropped
the patch from the queue for now.

Guenter

> BR, Christian
> 
> 
> Am 17.06.2019 um 18:04 schrieb Guenter Roeck:
> >On Mon, Jun 17, 2019 at 11:33:43AM +0200, cschneider@radiodata.biz wrote:
> >>From: Christian Schneider <christian@ch-sc.de>
> >>
> >>sysfs_notify and kobject_uevent are passed the wrong kobject.
> >>that why notifications can't be received and uevents have the wrong path.
> >>this patch fixes this.
> >>
> >>Signed-off-by: Christian Schneider <cschneider@radiodata.biz>
> >
> >Applied.
> >
> >In the future, please version your patches, add a change log,
> >and reflect the subsystem and driver in the subject line.
> >
> >Thanks,
> >Guenter
> >
> >>---
> >>  drivers/hwmon/gpio-fan.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
> >>index 84753680a4e8..76377791ff0e 100644
> >>--- a/drivers/hwmon/gpio-fan.c
> >>+++ b/drivers/hwmon/gpio-fan.c
> >>@@ -54,8 +54,8 @@ static void fan_alarm_notify(struct work_struct *ws)
> >>  	struct gpio_fan_data *fan_data =
> >>  		container_of(ws, struct gpio_fan_data, alarm_work);
> >>-	sysfs_notify(&fan_data->dev->kobj, NULL, "fan1_alarm");
> >>-	kobject_uevent(&fan_data->dev->kobj, KOBJ_CHANGE);
> >>+	sysfs_notify(&fan_data->hwmon_dev->kobj, NULL, "fan1_alarm");
> >>+	kobject_uevent(&fan_data->hwmon_dev->kobj, KOBJ_CHANGE);
> >>  }
> >>  static irqreturn_t fan_alarm_irq_handler(int irq, void *dev_id)