* [PATCH] Fix sysfs_notify and kobject_uevent in fan_alarm_notify @ 2019-06-17 9:33 cschneider 2019-06-17 17:04 ` Guenter Roeck 0 siblings, 1 reply; 8+ messages in thread From: cschneider @ 2019-06-17 9:33 UTC (permalink / raw) To: linux-hwmon; +Cc: Christian Schneider, Christian Schneider 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> --- 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) -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix sysfs_notify and kobject_uevent in fan_alarm_notify 2019-06-17 9:33 [PATCH] Fix sysfs_notify and kobject_uevent in fan_alarm_notify cschneider @ 2019-06-17 17:04 ` Guenter Roeck 2019-06-19 8:47 ` Christian Schneider 0 siblings, 1 reply; 8+ messages in thread From: Guenter Roeck @ 2019-06-17 17:04 UTC (permalink / raw) To: cschneider; +Cc: linux-hwmon, Christian Schneider 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix sysfs_notify and kobject_uevent in fan_alarm_notify 2019-06-17 17:04 ` Guenter Roeck @ 2019-06-19 8:47 ` Christian Schneider 2019-06-19 10:00 ` Christian Schneider 2019-06-19 19:55 ` Guenter Roeck 0 siblings, 2 replies; 8+ messages in thread From: Christian Schneider @ 2019-06-19 8:47 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-hwmon, Christian Schneider 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.... 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix sysfs_notify and kobject_uevent in fan_alarm_notify 2019-06-19 8:47 ` Christian Schneider @ 2019-06-19 10:00 ` Christian Schneider 2019-06-19 19:53 ` Guenter Roeck 2019-06-19 19:55 ` Guenter Roeck 1 sibling, 1 reply; 8+ messages in thread From: Christian Schneider @ 2019-06-19 10:00 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-hwmon, Christian Schneider Ok, seems that moving that block, where fan_alarm_init is called down, isn't a problem on master. I need to use 4.9, cause we have an embedded device that needs patches, and they are only available for older kernels. In 4.9, fan_alarm_init() does fan_data->alarm = alarm; When fan_alarm_init() is called after devm_hwmon_device_register_with_groups(), then fan_data->alarm is not yet set, so the sysfs attribute is not created. when I move the fan_data->alarm = alarm; out of fan_alarm_init() before devm_hwmon_device_register_with_groups() and the fan_alarm_init() after, then it works, and doesn't have a race condition. On master, populating fan_data->alarm_gpio is done in gpio_fan_get_of_data(fan_data); as far as I understand, so moving the fan_alarm_init() block further down shouldn't be a problem. I don't know, how I can test this with master kernel. Unfortunately, the device, I'm using doesn't have a recent kernel. And on my normal (desktop) pc, I don't have a gpio fan. How should I proceed now? BR, Christian Am 19.06.2019 um 09:47 schrieb Christian Schneider: > 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.... > > 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix sysfs_notify and kobject_uevent in fan_alarm_notify 2019-06-19 10:00 ` Christian Schneider @ 2019-06-19 19:53 ` Guenter Roeck 2019-06-21 8:30 ` Christian Schneider 0 siblings, 1 reply; 8+ messages in thread From: Guenter Roeck @ 2019-06-19 19:53 UTC (permalink / raw) To: Christian Schneider; +Cc: linux-hwmon, Christian Schneider On Wed, Jun 19, 2019 at 12:00:37PM +0200, Christian Schneider wrote: > Ok, seems that moving that block, where fan_alarm_init is called down, isn't > a problem on master. > I need to use 4.9, cause we have an embedded device that needs patches, and > they are only available for older kernels. > > In 4.9, fan_alarm_init() does fan_data->alarm = alarm; > When fan_alarm_init() is called after > devm_hwmon_device_register_with_groups(), then fan_data->alarm is not yet > set, so the sysfs attribute is not created. > > when I move the fan_data->alarm = alarm; out of fan_alarm_init() before > devm_hwmon_device_register_with_groups() and the fan_alarm_init() after, > then it works, and doesn't have a race condition. > > On master, populating fan_data->alarm_gpio is done in > gpio_fan_get_of_data(fan_data); as far as I understand, so moving the > fan_alarm_init() block further down shouldn't be a problem. > > I don't know, how I can test this with master kernel. Unfortunately, the > device, I'm using doesn't have a recent kernel. And on my normal (desktop) > pc, I don't have a gpio fan. > How should I proceed now? > Essentially, we'll need two patches: One to move fan_alarm_init() after devm_hwmon_device_register_with_groups(), and then the notification patch. For the backport to v4.9, the first patch will have to be slightly modified: The first part will a simple fan_data->alarm = pdata->alarm; at the location where fan_alarm_init() is called now, the second part is to move the call to fan_alarm_init() after devm_hwmon_device_register_with_groups(). It is ok to test the result with your 4.9 based kernel; we'll just have to be careful when reviewing the mainline patch. Thanks, Guenter > > BR, Christian > > Am 19.06.2019 um 09:47 schrieb Christian Schneider: > >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.... > > > >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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix sysfs_notify and kobject_uevent in fan_alarm_notify 2019-06-19 19:53 ` Guenter Roeck @ 2019-06-21 8:30 ` Christian Schneider 2019-06-21 14:05 ` Guenter Roeck 0 siblings, 1 reply; 8+ messages in thread From: Christian Schneider @ 2019-06-21 8:30 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-hwmon, Christian Schneider Am 19.06.2019 um 20:53 schrieb Guenter Roeck: > On Wed, Jun 19, 2019 at 12:00:37PM +0200, Christian Schneider wrote: >> Ok, seems that moving that block, where fan_alarm_init is called down, isn't >> a problem on master. >> I need to use 4.9, cause we have an embedded device that needs patches, and >> they are only available for older kernels. >> >> In 4.9, fan_alarm_init() does fan_data->alarm = alarm; >> When fan_alarm_init() is called after >> devm_hwmon_device_register_with_groups(), then fan_data->alarm is not yet >> set, so the sysfs attribute is not created. >> >> when I move the fan_data->alarm = alarm; out of fan_alarm_init() before >> devm_hwmon_device_register_with_groups() and the fan_alarm_init() after, >> then it works, and doesn't have a race condition. >> >> On master, populating fan_data->alarm_gpio is done in >> gpio_fan_get_of_data(fan_data); as far as I understand, so moving the >> fan_alarm_init() block further down shouldn't be a problem. >> >> I don't know, how I can test this with master kernel. Unfortunately, the >> device, I'm using doesn't have a recent kernel. And on my normal (desktop) >> pc, I don't have a gpio fan. >> How should I proceed now? >> > > Essentially, we'll need two patches: One to move fan_alarm_init() after > devm_hwmon_device_register_with_groups(), and then the notification patch. > For the backport to v4.9, the first patch will have to be slightly modified: > The first part will a simple > fan_data->alarm = pdata->alarm; > at the location where fan_alarm_init() is called now, the second part is to move > the call to fan_alarm_init() after devm_hwmon_device_register_with_groups(). > > It is ok to test the result with your 4.9 based kernel; we'll just have to be > careful when reviewing the mainline patch. > > Thanks, > Guenter Ok, it seems like it is working as expected now on my 4.9 kernel. No race-condition and notification is received. I will adopt the changes to mainline kernel, but not sure if I can make it today. Might be Monday, till I have the patch. BR, Christian > >> >> BR, Christian >> >> Am 19.06.2019 um 09:47 schrieb Christian Schneider: >>> 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.... >>> >>> 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix sysfs_notify and kobject_uevent in fan_alarm_notify 2019-06-21 8:30 ` Christian Schneider @ 2019-06-21 14:05 ` Guenter Roeck 0 siblings, 0 replies; 8+ messages in thread From: Guenter Roeck @ 2019-06-21 14:05 UTC (permalink / raw) To: Christian Schneider; +Cc: linux-hwmon, Christian Schneider On 6/21/19 1:30 AM, Christian Schneider wrote: > Am 19.06.2019 um 20:53 schrieb Guenter Roeck: >> On Wed, Jun 19, 2019 at 12:00:37PM +0200, Christian Schneider wrote: >>> Ok, seems that moving that block, where fan_alarm_init is called down, isn't >>> a problem on master. >>> I need to use 4.9, cause we have an embedded device that needs patches, and >>> they are only available for older kernels. >>> >>> In 4.9, fan_alarm_init() does fan_data->alarm = alarm; >>> When fan_alarm_init() is called after >>> devm_hwmon_device_register_with_groups(), then fan_data->alarm is not yet >>> set, so the sysfs attribute is not created. >>> >>> when I move the fan_data->alarm = alarm; out of fan_alarm_init() before >>> devm_hwmon_device_register_with_groups() and the fan_alarm_init() after, >>> then it works, and doesn't have a race condition. >>> >>> On master, populating fan_data->alarm_gpio is done in >>> gpio_fan_get_of_data(fan_data); as far as I understand, so moving the >>> fan_alarm_init() block further down shouldn't be a problem. >>> >>> I don't know, how I can test this with master kernel. Unfortunately, the >>> device, I'm using doesn't have a recent kernel. And on my normal (desktop) >>> pc, I don't have a gpio fan. >>> How should I proceed now? >>> >> >> Essentially, we'll need two patches: One to move fan_alarm_init() after >> devm_hwmon_device_register_with_groups(), and then the notification patch. >> For the backport to v4.9, the first patch will have to be slightly modified: >> The first part will a simple >> fan_data->alarm = pdata->alarm; >> at the location where fan_alarm_init() is called now, the second part is to move >> the call to fan_alarm_init() after devm_hwmon_device_register_with_groups(). >> >> It is ok to test the result with your 4.9 based kernel; we'll just have to be >> careful when reviewing the mainline patch. >> >> Thanks, >> Guenter > > Ok, > it seems like it is working as expected now on my 4.9 kernel. No race-condition and notification is received. > I will adopt the changes to mainline kernel, but not sure if I can make it today. Might be Monday, till I have the patch. > No hurry. Thanks, Guenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix sysfs_notify and kobject_uevent in fan_alarm_notify 2019-06-19 8:47 ` Christian Schneider 2019-06-19 10:00 ` Christian Schneider @ 2019-06-19 19:55 ` Guenter Roeck 1 sibling, 0 replies; 8+ messages in thread From: Guenter Roeck @ 2019-06-19 19:55 UTC (permalink / raw) To: Christian Schneider; +Cc: linux-hwmon, Christian Schneider 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-06-21 14:05 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-06-17 9:33 [PATCH] Fix sysfs_notify and kobject_uevent in fan_alarm_notify cschneider 2019-06-17 17:04 ` Guenter Roeck 2019-06-19 8:47 ` Christian Schneider 2019-06-19 10:00 ` Christian Schneider 2019-06-19 19:53 ` Guenter Roeck 2019-06-21 8:30 ` Christian Schneider 2019-06-21 14:05 ` Guenter Roeck 2019-06-19 19:55 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox