All of lore.kernel.org
 help / color / mirror / Atom feed
From: khali@linux-fr.org (Jean Delvare)
To: lm-sensors@vger.kernel.org
Subject: [lm-sensors] [patch] hwmon/w83791d - fix unchecked errs
Date: Mon, 25 Sep 2006 21:31:17 +0000	[thread overview]
Message-ID: <20060925233117.830083f6.khali@linux-fr.org> (raw)
In-Reply-To: <45173E99.6050608@gmail.com>

Hi Charles,

> In the original code, there was nothing that removed the sysfs files
> in the detach. Was that a bug? Or is the removal something new for the
> sysfs_create_group() call?  Adding and removing the module before
> didn't show any problems (would it show as a kernel memory leak
> visible via something like /proc/meminfo?)

No, it wasn't a real bug and there was no memory leak. Not removing the
files was simply considered not clean and bad, and given that it is now
really easy to remove them when using groups, we decided to fix both
problems (unchecked errors on creation and lack of deletion) at once.
We could have made separate patches, but given that we had to change
all drivers, doing it all in one pass sounded easier.

Another reason to now properly delete the files on driver detach is
that we plan to convert the i2c subsystem to the device driver model
someday. In the device driver model, devices exist independently of
drivers being loaded to handle them. This means that a devices will
survive its driver removal, and if we don't remove the files when the
driver is removed, we end up with dangling callbacks. Bad.

> I've modified the patch per the discussion below (attached). I have
> confirmed the sysfs files are created and the values are accessed
> appropriately, but is there a good way to validate the error cases?

The easiest way to test is to simulate an error at the end of the detect
function, i.e. do as if hwmon_device_register() had failed. That way
you train the whole error path. Of course it's even better to test each
possible failure individually, but that takes more time.

Anyway, I'm confident the patch is OK now - it was really the same fix
for all drivers. I'll apply the patch to my tree now.

Thanks,
-- 
Jean Delvare


      parent reply	other threads:[~2006-09-25 21:31 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-09-25  2:27 [lm-sensors] [patch] hwmon/w83791d - fix unchecked errs Jim Cromie
2006-09-25  9:48 ` Charles Spirakis
2006-09-25 13:35 ` Jean Delvare
2006-09-25 20:13 ` Charles Spirakis
2006-09-25 21:31 ` Jean Delvare [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20060925233117.830083f6.khali@linux-fr.org \
    --to=khali@linux-fr.org \
    --cc=lm-sensors@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.