From: Greg Kroah-Hartman <gregkh@linuxfoundation•org>
To: Benedikt Spranger <b.spranger@linutronix•de>
Cc: netdev@vger•kernel.org,
Alexander Frank <Alexander.Frank@eberspaecher•com>,
Sebastian Andrzej Siewior <bigeasy@linutronix•de>,
"Hans J. Koch" <hjk@hansjkoch•de>,
Holger Dengler <dengler@linutronix•de>
Subject: Re: [PATCH 2/7] uio: Allow to create custom UIO attributes
Date: Tue, 13 Aug 2013 10:54:36 -0700 [thread overview]
Message-ID: <20130813175436.GF4098@kroah.com> (raw)
In-Reply-To: <1376384922-8519-4-git-send-email-b.spranger@linutronix.de>
On Tue, Aug 13, 2013 at 11:08:37AM +0200, Benedikt Spranger wrote:
> This patch the struct uio_attribute which represents a custom UIO
> attribute. The non-standard attributes are stored in a "attr" directory.
> This will be used by the flexcard driver which creates a UIO device that
> is using the "uio_pdrv" and requires one additional value to be read /
> written by the user.
>
> Cc: "Hans J. Koch" <hjk@hansjkoch•de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation•org>
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix•de>
> Signed-off-by: Holger Dengler <dengler@linutronix•de>
> ---
> drivers/uio/uio.c | 106 ++++++++++++++++++++++++++++++++++++++++++++-
> include/linux/uio_driver.h | 36 +++++++++++----
> 2 files changed, 133 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 9a95220..d66784a 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -39,6 +39,7 @@ struct uio_device {
> struct uio_info *info;
> struct kobject *map_dir;
> struct kobject *portio_dir;
> + struct kobject attr_dir;
If you embed a kobject into a structure, it is now in charge of the
memory reference counting for that object.
Which you did not do correctly:
> +static struct kobj_type uio_attr_type = {
> + .sysfs_ops = &uio_sysfs_ops,
> +};
As per the documentation in the kernel, I now get to make fun of the
fact that you obviously didn't test this code (hint, the kernel would
have told you bad things happened if you did...)
But I don't understand your main goal of a new kobject for attributes,
why? What problem are you trying to solve here that a new kobject, and
new sysfs files attached to the UIO object are going to solve?
> /**
> + * uio_add_user_attributes - add an extra UIO attribute
> + * @info: UIO device capabilities
> + */
> +static int uio_add_user_attributes(struct uio_info *info)
> +{
> + struct uio_device *idev = info->uio_dev;
> + const struct uio_attribute **uio_attr;
> + int i;
> + int ret = 0;
> +
> + uio_attr = info->attributes;
> + if (!uio_attr)
> + return 0;
> +
> + for (i = 0; uio_attr[i]; i++) {
> +
> + ret = sysfs_create_file(&idev->attr_dir, &uio_attr[i]->attr);
> + if (ret)
> + break;
> + }
> + if (ret) {
> + while (--i >= 0)
> + sysfs_remove_file(&idev->attr_dir, &uio_attr[i]->attr);
> + }
> + return ret;
> +}
Ick, you just raced with userspace and blew up any tool that was wanting
to watch for your new kobject to be created with the attributes attached
to it :(
Sending code that doesn't work is fine, for review if you have questions
about things, but please, mark it with a big "I HAVEN'T TESTED THIS"
statement somewhere.
greg k-h
next prev parent reply other threads:[~2013-08-13 17:54 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-13 9:08 [PATCH 0/7] add FlexRay support Benedikt Spranger
2013-08-13 9:08 ` Benedikt Spranger
2013-08-13 9:08 ` [PATCH 1/7] uio: add module owner to prevent inappropriate module unloading Benedikt Spranger
2013-08-13 17:48 ` Greg Kroah-Hartman
2013-08-14 7:19 ` Benedikt Spranger
2013-08-14 16:33 ` Greg Kroah-Hartman
2013-08-15 6:42 ` Benedikt Spranger
2013-08-15 6:59 ` Greg Kroah-Hartman
2013-08-15 7:27 ` Benedikt Spranger
2013-08-15 8:09 ` Greg Kroah-Hartman
2013-08-15 8:18 ` Sebastian Andrzej Siewior
2013-08-15 15:55 ` Greg Kroah-Hartman
2013-08-15 16:03 ` Sebastian Andrzej Siewior
2013-08-15 16:42 ` Greg Kroah-Hartman
2013-08-15 16:54 ` Sebastian Andrzej Siewior
2013-08-15 17:13 ` Greg Kroah-Hartman
2013-08-13 9:08 ` [PATCH 2/7] uio: Allow to create custom UIO attributes Benedikt Spranger
2013-08-13 17:54 ` Greg Kroah-Hartman [this message]
2013-08-13 9:08 ` [PATCH 3/7] mfd: core: copy DMA mask and params from parent Benedikt Spranger
2013-08-13 10:03 ` Lee Jones
2013-08-13 9:08 ` [PATCH 4/7] mfd: add MFD based flexcard driver Benedikt Spranger
2013-08-13 9:55 ` Lee Jones
2013-08-14 8:12 ` Benedikt Spranger
2013-08-14 9:45 ` Lee Jones
2013-08-13 9:08 ` [PATCH 5/7] clocksource: Add flexcard support Benedikt Spranger
2013-08-13 9:08 ` [PATCH 6/7] net: add the AF_FLEXRAY protocol Benedikt Spranger
2013-08-18 18:50 ` Oliver Hartkopp
2013-08-13 9:08 ` [PATCH 7/7] net: add a flexray driver Benedikt Spranger
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=20130813175436.GF4098@kroah.com \
--to=gregkh@linuxfoundation$(echo .)org \
--cc=Alexander.Frank@eberspaecher$(echo .)com \
--cc=b.spranger@linutronix$(echo .)de \
--cc=bigeasy@linutronix$(echo .)de \
--cc=dengler@linutronix$(echo .)de \
--cc=hjk@hansjkoch$(echo .)de \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox